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.
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