feat: add announce addrs in host, #1250#1268
Conversation
sumanjeet0012
left a comment
sumanjeet0012
left a comment
There was a problem hiding this comment.
@asabya Please also add a newsfragment file and include documentation for the example file.
- Add newsfragment documenting announce_addrs feature - Add README with usage examples and NAT/ngrok notes - Fix IHost.get_addrs() docstring: tuple → list with clearer description - Remove unused Callable import from libp2p/__init__.py - Fix logging in announce_addrs example: remove conflicting root logger setLevel(WARNING)
libp2p/host/basic_host.py
Outdated
| self.psk = psk | ||
|
|
||
| # Address announcement configuration | ||
| self._announce_addrs = list(announce_addrs) if announce_addrs else None |
There was a problem hiding this comment.
Would there be a case where a user would want to announce 0 addresses? If so, we'd need to check if announce_addrs is not None vs if announce_addrs to get that behavior. An empty sequence would result in fallback behavior.
There was a problem hiding this comment.
Not sure why user would like to announce nothing..
But if user wants to do it anyway, this will be the cases.
announce_addrs is None => fallback to transport_addrs
announce_addrs is [] => get_addrs return []
announce_addrs not any of the above => return announce_addrs
examples/announce_addrs/README.md
Outdated
There was a problem hiding this comment.
A better, more discoverable place for this file would be to reformat and move it to examples.announce_addrs.rst. That way it would get included in the docs. You can follow other examples there and literalinclude the example code at the bottom. See examples.chat.rst as an example.
libp2p/host/basic_host.py
Outdated
|
|
||
| if self._announce_addrs is not None: | ||
| addrs = list(self._announce_addrs) | ||
| else: | ||
| addrs = self.get_transport_addrs() | ||
|
|
||
| return [addr.encapsulate(p2p_part) for addr in addrs] |
There was a problem hiding this comment.
This doesn't check before encapsulating, so we could end up with an invalid double-suffixed addr if the user provides a full p2p mulitaddr. As a minimal example:
key_pair = create_new_key_pair()
swarm = new_swarm(key_pair)
host = BasicHost(swarm)
peer_id_str = str(host.get_id())
host_with_p2p_announce = BasicHost(
swarm,
announce_addrs=[Multiaddr(f"/ip4/1.2.3.4/tcp/4001/p2p/{peer_id_str}")],
)
assert (
str(host_with_p2p_announce.get_addrs()[0]).count(f"/p2p/{peer_id_str}") == 2
libp2p/host/basic_host.py
Outdated
There was a problem hiding this comment.
I'm concerned that after this PR, a user providing announce_addrs will break UPnP behavior. UPnP should still have access to the addresses returned by get_transport_addrs since it's mapping local ports.
Please let me know if I'm missing something!
There was a problem hiding this comment.
this should use self.get_transport_addrs() instead of self.get_addrs().
- Use identity check (`is not None`) for announce_addrs so empty list is respected instead of falling back to transport addrs - Guard against double /p2p/ suffix in get_addrs() - Use get_transport_addrs() in UPnP port mapping to avoid using announced addrs for local port extraction - Move example docs from README.md to Sphinx rst with literalinclude
PR #1268 Review: announce_addrs SupportWhat This DoesAdds What's Good
Issues FoundCritical
Major Minor Other Notes
Ready to Merge?Not yet. The critical peer ID validation issue needs addressing, plus the missing |
AI PR Review: #1268 — feat: add announce addrs in host1. Summary of Changes
2. Branch Sync Status and Merge ConflictsBranch sync (
|
| # | Topic | pacrob’s concern | Status | Where it is addressed |
|---|---|---|---|---|
| 1 | Empty announce_addrs |
Using truthiness (if announce_addrs) would treat [] like “unset” and fall back to transport addrs; users who want to announce zero addresses need is not None. |
Resolved | BasicHost uses identity on None when assigning _announce_addrs (None → fallback; [] → empty list). See §“Code references” below. |
| 2 | Example docs location | Prefer Sphinx docs/examples.announce_addrs.rst with literalinclude (like examples.chat.rst) instead of only a README. |
Resolved | docs/examples.announce_addrs.rst includes .. literalinclude:: ../examples/announce_addrs/announce_addrs.py and is linked from docs/examples.rst. |
| 3 | Double /p2p/ suffix |
Encapsulating every announce addr with /p2p/{id} could produce invalid addrs if the user already passed a full multiaddr ending in /p2p/.... |
Resolved | get_addrs() leaves addrs that already contain a p2p component unchanged; others get encapsulate(p2p_part). Note: this avoids duplicate /p2p/ but does not require the embedded id to match the host (still flagged in §4 Critical). |
| 4 | UPnP vs announced addrs | After the feature, code that used get_addrs() for port mapping could use announced addresses and break UPnP (which needs real local listener ports). |
Resolved | UPnP add/remove port mapping iterates get_transport_addrs(), not get_addrs(). See §“Code references” below. |
Code references (pacrob items 1, 3, 4):
libp2p/host/basic_host.py — lines 254–257
# Address announcement configuration
self._announce_addrs = (
list(announce_addrs) if announce_addrs is not None else None
)libp2p/host/basic_host.py — lines 363–376
p2p_part = multiaddr.Multiaddr(f"/p2p/{self.get_id()!s}")
if self._announce_addrs is not None:
addrs = list(self._announce_addrs)
else:
addrs = self.get_transport_addrs()
result = []
for addr in addrs:
if "p2p" in [p.name for p in addr.protocols()]:
result.append(addr)
else:
result.append(addr.encapsulate(p2p_part))
return resultlibp2p/host/basic_host.py — lines 408–411
if await upnp_manager.discover():
for addr in self.get_transport_addrs():
if port := addr.value_for_protocol("tcp"):
await upnp_manager.add_port_mapping(int(port), "TCP")Summary: Yes — pacrob’s maintainer review items are implemented as requested. Nothing in that set remains an open code defect on this branch. Remaining gaps in this document (newsfragment filename, peer-id validation, BasicHost docstring, extra tests) are additional concerns — either from other reviewers (e.g. sumanjeet0012’s newsfragment/docs request — largely done except fragment naming) or from deeper edge-case / project-convention review (§4).
3. Strengths
- Problem fit: Directly targets #1250: you cannot bind to a public IP on EC2, but you still need to advertise a dialable address;
announce_addrsmatches how other libp2p stacks handle this. - UPnP: Port mapping iterates
get_transport_addrs()(local listeners), notget_addrs(), so announced addresses do not break UPnP’s need for real listening ports — aligns with maintainer feedback on the PR. - Double
/p2p/:get_addrs()skips encapsulation when ap2pprotocol component is already present, avoiding invalid double suffixes when users pass a full multiaddr. - Empty list semantics:
_announce_addrsusesis not None, soannounce_addrs=[]advertises nothing instead of falling back to transport addrs. - Docs:
docs/examples.announce_addrs.rstwithliteralincludefollows the pattern requested in review (moved from a standalone README). - API surface:
new_host(..., announce_addrs=...)andIHostdocumentation updated for discoverability.
4. Issues Found
Critical
-
File:
newsfragments/1268.feature.rst -
Line(s): (entire file)
-
Issue: Newsfragment is named with the PR number (
1268) instead of the issue number (1250). Project convention (see other fragments:1254.internal.rst,1273.feature.rst, etc.) and the review prompt requirenewsfragments/<ISSUE_NUMBER>.<type>.rst. -
Suggestion: Rename to
1250.feature.rst(and remove1268.feature.rst). Adjust release-note cross-refs if any tooling assumes PR numbers. -
File:
libp2p/host/basic_host.py -
Line(s): 365–376
-
Issue: If
announce_addrsincludes a/p2p/<peer-id>component that does not matchself.get_id(), that wrong id is returned unchanged (branch treats anyp2pcomponent as “already complete”). Remote peers may trust advertised addresses for dialing/routing, causing identity confusion or failed dials. -
Suggestion: Either strip existing
p2pcomponents and always re-encapsulate withself.get_id(), or validate that any embedded peer id equalsself.get_id()and raise or log + replace. Prefer matching go-libp2p/js-libp2p behavior if specified elsewhere.
Major
-
File:
libp2p/host/basic_host.py -
Line(s): 195–209 (docstring)
-
Issue:
BasicHost.__init__documents many parameters but omits:param announce_addrs:even thoughRoutedHostandnew_host()document it. -
Suggestion: Add a
:param announce_addrs:line (behavior:None→ use listen addrs inget_addrs(); non-None→ replace;[]→ empty advertised set). -
File:
tests/core/host/test_basic_host.py -
Line(s): 140–157
-
Issue: Tests cover the happy path (single announce addr without
/p2p/). Missing: emptyannounce_addrs, multiple addrs, addr that already contains correct/p2p/, and wrong peer id in/p2p/once validation is implemented. -
Suggestion: Add parametrized or small focused tests for these cases.
-
File: PR #1268 (PR body)
-
Line(s): N/A
-
Issue: Body references “Issue Listening Addresses on an AWS EC2 don't include public IPs #1250” but does not use a closing keyword (
Fixes #1250,Closes #1250). Optional for merge but improves tracking and GitHub linking. -
Suggestion: Add
Fixes #1250(orCloses #1250) in the description.
Minor
-
File:
libp2p/host/basic_host.py -
Line(s): 354–358
-
Issue:
get_addrs()docstring: “Otherwise listen addresses are used” is missing terminal punctuation after “used”. -
Suggestion: End the sentence with a period (and optionally mention
[]explicitly). -
File:
libp2p/host/basic_host.py -
Line(s): 372
-
Issue:
"p2p" in [p.name for p in addr.protocols()]allocates a list each time; minor hot-path cost. -
Suggestion:
any(p.name == "p2p" for p in addr.protocols())(or project-preferred helper if one exists for multiaddr inspection).
Note: An earlier human review asked for examples/announce_addrs/__init__.py. Other examples under examples/chat and examples/echo do not use __init__.py either; Sphinx literalinclude works without it. Treat as optional unless maintainers require it for packaging.
5. Security Review
-
Risk: Announcing a multiaddr with
/p2p/<not-self>poisons peer routing / Identify data with a conflicting peer id. -
Impact: High for trust and connectivity (misleading advertisements); medium if most callers only pass host:port multiaddrs.
-
Mitigation: Validate or normalize peer id in
get_addrs()as in §4 Critical; document that announce addrs should be transport-only or must match the host id. -
Risk: No new network input parsing in this PR;
announce_addrsis caller-supplied (trusted config). No subprocess/temp-file changes. -
Impact: Low for classic injection; concern is semantic (wrong id), not buffer overflow.
6. Documentation and Examples
- Sphinx:
docs/examples.announce_addrs.rstis included fromdocs/examples.rstand builds cleanly (examples.announce_addrsread duringmake linux-docs). - Interfaces:
IHost.get_addrsinabc.pydocuments that advertised addrs may differ whenannounce_addrsis set. - Gap:
BasicHost.__init__docstring still missingannounce_addrs(see Major).
7. Newsfragment Requirement
- Present:
newsfragments/1268.feature.rstwith user-facing text and trailing newline — content is appropriate. - BLOCKER: Filename must follow issue number 1250, not PR 1268 (see §4 Critical). Until renamed to
1250.feature.rst, this does not meet the project’s stated towncrier convention used acrossnewsfragments/.
Maintainer checklist: PR references issue #1250 in prose; formal “Fixes #1250” still recommended.
8. Tests and Validation
All commands run as: source venv/bin/activate && make <target> with logs under downloads/AI-PR-REVIEWS/1268/.
Lint (make lint)
- Exit code: 0
- Result: All hooks passed (yaml, toml, ruff, ruff format, mdformat, mypy, pyrefly,
.rsttop-level check, path audit).
Typecheck (make typecheck)
- Exit code: 0
- Result: mypy and pyrefly pre-commit hooks passed.
Tests (make test)
- Exit code: 0
- Summary:
2563 passed, 15 skipped, 24 warnings in 98.25s - Notes: 24 warnings in suite (deprecations/resources/pytest); no failures attributed to this PR.
Documentation (make linux-docs)
- Exit code: 0
- Result:
sphinx-build -b html -Wsucceeded;examples.announce_addrsprocessed.
9. Recommendations for Improvement
- Rename newsfragment to
1250.feature.rstand delete1268.feature.rst. - Implement peer-id normalization or validation for announce multiaddrs containing
/p2p/. - Complete
BasicHost.__init__docstring forannounce_addrs. - Extend tests for
[], multiple addrs, pre-suffixed/p2p/, and wrong peer id after (2) is defined. - Add
Fixes #1250to the PR body and re-request review from sumanjeet0012 (currently “changes requested”) after addressing remaining items.
10. Questions for the Author
- Should only transport components (ip/dns/tcp/quic/…) be allowed in
announce_addrs, with/p2p/always applied by the host? That would simplify correctness and match “listen addrs + id” mental model. - Is there a spec or parity target (go-libp2p / js-libp2p) for how mismatched
/p2p/in announce addrs should behave (strip, error, warn)?
11. Overall Assessment
| Dimension | Assessment |
|---|---|
| Quality rating | Good — design matches the problem; implementation is readable; CI-clean on this branch. |
| Security impact | Low–Medium — no classic memory/injection issues; medium concern for mis-specified /p2p/ in announcements until handled. |
| Merge readiness | Needs fixes — newsfragment naming (blocker), peer-id handling (strongly recommended before merge), docstring + tests polish. |
| Confidence | High for behavior reviewed in code and tests; medium on edge cases until more tests land. |
Reviewer status (GitHub): pacrob — inline review threads are addressed in code (see Maintainer feedback: pacrob above); the author should mark those conversations resolved on GitHub if not already. sumanjeet0012 has changes requested (newsfragment + docs — docs done; confirm newsfragment rename to 1250.feature.rst and re-request review).
End of review.
Strip any existing /p2p/ component from announce addresses and always re-encapsulate with the host's own peer ID, mirroring js-libp2p behaviour. This prevents identity confusion when users pass announce addrs containing a mismatched peer ID. Also addresses remaining review items: - Rename newsfragment from 1268 (PR) to 1250 (issue number) - Add :param announce_addrs: to BasicHost.__init__ docstring - Add tests for empty list, multiple addrs, correct/wrong peer ID - Fix missing period in get_addrs() docstring
Good catch @acul71. Updated the PR with the fixes. |
|
This is a very solid and practical improvement, @asabya 👏 Appreciate your efforts. Adding support for It’s great to see the careful handling of edge cases, avoiding double Overall, this is a meaningful addition that improves both developer experience and network reliability. @acul71 and @pacrob: Thank you for your review and detailed feedback. Appreciate it. Ready for merge. |
|
@seetadev @pacrob Full review here: AI PR Review: #1268 — feat: add announce addrs in hostReview date: 2026-03-28 Prerequisites completed
1. Summary of changesWhat: Adds optional Callers pass a sequence of # Address announcement configuration
self._announce_addrs = (
list(announce_addrs) if announce_addrs is not None else None
)
announce_addrs: Sequence[multiaddr.Multiaddr] | None = None,
) -> IHost:
"""
Create a new libp2p host based on the given parameters.
:param key_pair: optional choice of the ``KeyPair``
:param muxer_opt: optional choice of stream muxer
:param sec_opt: optional choice of security upgrade
:param peerstore_opt: optional peerstore
:param disc_opt: optional discovery
:param muxer_preference: optional explicit muxer preference
:param listen_addrs: optional list of multiaddrs to listen on
:param enable_mDNS: whether to enable mDNS discovery
:param bootstrap: optional list of bootstrap peer addresses as strings
:param enable_quic: optinal choice to use QUIC for transport
:param enable_autotls: optinal choice to use AutoTLS for security
:param quic_transport_opt: optional configuration for quic transport
:param tls_client_config: optional TLS client configuration for WebSocket transport
:param tls_server_config: optional TLS server configuration for WebSocket transport
:param resource_manager: optional resource manager for connection/stream limits
:type resource_manager: :class:`libp2p.rcmgr.ResourceManager` or None
:param psk: optional pre-shared key (PSK)
:param bootstrap_allow_ipv6: if True, bootstrap accepts IPv6+TCP addresses
:param bootstrap_dns_timeout: DNS resolution timeout in seconds per attempt
:param bootstrap_dns_max_retries: max DNS resolution retries with backoff
:param connection_config: optional connection configuration for connection manager
:param announce_addrs: if set, these replace listen addrs in get_addrs()
:return: return a host instance
""" if disc_opt is not None:
return RoutedHost(
network=swarm,
router=disc_opt,
enable_mDNS=enable_mDNS,
enable_upnp=enable_upnp,
bootstrap=bootstrap,
resource_manager=resource_manager,
bootstrap_allow_ipv6=bootstrap_allow_ipv6,
bootstrap_dns_timeout=bootstrap_dns_timeout,
bootstrap_dns_max_retries=bootstrap_dns_max_retries,
announce_addrs=announce_addrs,
)
return BasicHost(
network=swarm,
enable_mDNS=enable_mDNS,
bootstrap=bootstrap,
enable_upnp=enable_upnp,
negotiate_timeout=negotiate_timeout,
resource_manager=resource_manager,
bootstrap_allow_ipv6=bootstrap_allow_ipv6,
bootstrap_dns_timeout=bootstrap_dns_timeout,
bootstrap_dns_max_retries=bootstrap_dns_max_retries,
announce_addrs=announce_addrs,
)Advertised addresses are built in p2p_part = multiaddr.Multiaddr(f"/p2p/{self.get_id()!s}")
if self._announce_addrs is not None:
addrs = list(self._announce_addrs)
else:
addrs = self.get_transport_addrs()
result = []
for addr in addrs:
# Strip any existing /p2p/ component, then always append our own.
# This avoids identity confusion when announce addrs contain a
# mismatched peer ID (mirrors js-libp2p behaviour).
try:
p2p_value = addr.value_for_protocol("p2p")
except ProtocolLookupError:
p2p_value = None
if p2p_value:
addr = addr.decapsulate(multiaddr.Multiaddr(f"/p2p/{p2p_value}"))
result.append(addr.encapsulate(p2p_part))
return resultTypical usage (conceptual): import multiaddr
from libp2p import new_host
from libp2p.crypto.ed25519 import create_new_key_pair
key_pair = create_new_key_pair()
public = multiaddr.Multiaddr("/ip4/203.0.113.50/tcp/4001")
host = new_host(key_pair=key_pair, announce_addrs=[public])
# host.get_addrs() -> [/ip4/203.0.113.50/tcp/4001/p2p/<local_peer_id>]Issue: #1250 — on AWS/NAT, Identify and peer records showed private listen IPs; remote peers attempted unroutable dials. Main files: Breaking changes: None; new optional parameter defaulting to 2. Branch sync status and merge conflictsBranch sync (
|
| Dimension | Assessment |
|---|---|
| Quality rating | Excellent — design matches libp2p usage; edge cases and UPnP interaction are handled; tests and docs are solid. |
| Security impact | Low — identity confusion mitigated by normalization; config-driven surface. |
| Merge readiness | Ready from code/CI/docs/newsfragment; needs re-review or status update from requested reviewers on GitHub if “changes requested” remains. |
| Confidence | High — full suite green on reviewed branch; behavior verified in unit tests. |
Maintainer feedback (pacrob)
Per code inspection on this branch, all four of pacrob’s review threads on PR #1268 are fulfilled in code (nothing left open as a defect). GitHub may still label comments “Outdated” after edits; the author can mark conversations resolved once satisfied.
| # | Topic | Status | Where it is addressed |
|---|---|---|---|
| 1 | Empty announce_addrs — use is not None so [] advertises nothing instead of falling back to transport addrs (truthiness would treat [] like unset). |
Done | _announce_addrs uses list(announce_addrs) if announce_addrs is not None else None; get_addrs() branches on self._announce_addrs is not None. |
| 2 | Example docs — prefer Sphinx docs/examples.announce_addrs.rst with literalinclude (like examples.chat.rst), not only a standalone README.md. |
Done | docs/examples.announce_addrs.rst exists and is linked from docs/examples.rst. |
| 3 | Double /p2p/ — avoid invalid addrs when announce multiaddrs already end with /p2p/…. |
Done | Strip any existing p2p component, then encapsulate with this host’s id (also fixes mismatched embedded ids). |
| 4 | UPnP — port mapping must use real listener addresses (get_transport_addrs()), not get_addrs() / announced addrs. |
Done | UPnP add/remove port mapping loops over get_transport_addrs(). |
Code references
- Empty list vs
None:
# Address announcement configuration
self._announce_addrs = (
list(announce_addrs) if announce_addrs is not None else None
) if self._announce_addrs is not None:
addrs = list(self._announce_addrs)
else:
addrs = self.get_transport_addrs()-
Docs: see
docs/examples.announce_addrs.rst(not repeated here). -
/p2p/normalization:
result = []
for addr in addrs:
# Strip any existing /p2p/ component, then always append our own.
# This avoids identity confusion when announce addrs contain a
# mismatched peer ID (mirrors js-libp2p behaviour).
try:
p2p_value = addr.value_for_protocol("p2p")
except ProtocolLookupError:
p2p_value = None
if p2p_value:
addr = addr.decapsulate(multiaddr.Multiaddr(f"/p2p/{p2p_value}"))
result.append(addr.encapsulate(p2p_part))
return result- UPnP:
if self.upnp is not None:
upnp_manager = self.upnp
logger.debug("Starting UPnP discovery and port mapping")
if await upnp_manager.discover():
for addr in self.get_transport_addrs():
if port := addr.value_for_protocol("tcp"):
await upnp_manager.add_port_mapping(int(port), "TCP") if self.upnp and self.upnp.get_external_ip():
upnp_manager = self.upnp
logger.debug("Removing UPnP port mappings")
for addr in self.get_transport_addrs():
if port := addr.value_for_protocol("tcp"):
await upnp_manager.remove_port_mapping(int(port), "TCP")End of review.
|
@asabya : Thank you so much for your great efforts and persistence. Appreciate it. Thanks @acul71, @sumanjeet0012 and @pacrob for the thorough reviews and follow-ups here. Appreciate it. The implementation for announce addresses in the host looks well thought through and directly addresses the underlying issue with public IP visibility (especially in environments like EC2). The linkage to #1250 is clear, and it’s good to see validation across lint, typecheck, tests, and docs, all contributing to confidence in merge readiness. Great to see that earlier feedback (including from @sumanjeet0012 and @pacrob) has been addressed. From my side, this looks ready to go. Since merging this will auto-close the linked issue while PR #1284 is still referencing it, we may want to keep the issue open (or relink appropriately) to avoid losing tracking continuity. Otherwise, LGTM, happy to see this land 🚀 |
Tested on azure. Should work fine on other providers also.
|
6 participants
What was wrong?
The public IP doesn't show up in get_addrs()
Closes #1250
How was it fixed?
Added announce addrs in host. The callers sets the announce multiaddr that the host should announce. Overrides the listen addrs in
get_addrsCute Animal Picture