fix(l1): announce local IP when --p2p.addr is unspecified#6713
Conversation
When `--p2p.addr=0.0.0.0` (or `::`) was passed without `--nat.extip`, ethrex used the bind address as the externally-announced address, ending up with `enode://...@0.0.0.0:30303` in the ENR. Peers can't dial back to that, so the node had 0 inbound peers. Now the announce address falls back to the auto-detected local IP when the bind is unspecified; `--nat.extip` still overrides. If no local IP is detectable, log a loud warning instead of silently advertising the unspecified address.
|
🤖 Claude Code ReviewHere is my review of PR #6713: PR Review:
|
🤖 Kimi Code ReviewThe PR refactors P2P endpoint resolution to prevent nodes from advertising Code Quality & Correctness
Testing The unit tests cover the main scenarios well. Consider adding coverage for:
Example test to add: #[test]
fn p2p_addr_unspecified_v6_prefers_v6() {
let local_v6 = ip("fe80::1");
let local_v4 = ip("10.0.0.5");
let (bind, ext) = resolve_p2p_endpoints(Some("::"), None, Some(local_v4), Some(local_v6));
assert_eq!(bind, ip("::"));
assert_eq!(ext, local_v6); // Should prefer IPv6 when bind is IPv6
}Documentation & Style
Security & Performance
Verdict: Approve with minor suggestion to expand IPv6 test coverage. Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
🤖 Codex Code Review
No other correctness or security issues stood out in this patch. I couldn’t run Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
Lines of code reportTotal lines added: Detailed view |
Greptile SummaryFixes the long-standing issue where launching with
Confidence Score: 3/5The core fix is correct and well-tested for the common cases, but the cross-family fallback path can silently produce a broken bind/announce pair without any log warning. The primary bug (advertising cmd/ethrex/initializers.rs — the cross-family fallback in
|
| Filename | Overview |
|---|---|
| cmd/ethrex/initializers.rs | Extracts P2P address resolution into a testable resolve_p2p_endpoints function and fixes the 0.0.0.0/:: announce bug. The auto-detect fallback path can silently return a cross-family (bind, external) pair when the detected local IP family doesn't match the bind family. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["resolve_p2p_endpoints(p2p_addr, nat_extip, local_v4, local_v6)"] --> B{nat_extip set?}
B -- Yes --> C["external = nat_extip\nbind = p2p_addr (or UNSPECIFIED of same family)"]
B -- No --> D{p2p_addr set?}
D -- Yes --> E{bind.is_unspecified?}
E -- No --> F["return (bind, bind)"]
E -- Yes --> G{local IP of matching family?}
G -- Found --> H["INFO: announce auto-detected IP\nreturn (bind, local_ip)"]
G -- Not found, cross-family available --> I["⚠️ Silent mismatch:\nreturn (bind_v4, local_v6) or (bind_v6, local_v4)"]
G -- None at all --> J["WARN: announce unspecified\nreturn (bind, bind)"]
D -- No --> K["return (local_v4 or local_v6, same)"]
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
cmd/ethrex/initializers.rs:403-416
**Silent cross-family address mismatch in unspecified bind fallback**
When `--p2p.addr=0.0.0.0` is given and `local_v4` is `None` but `local_v6` is available, the code returns `(IpAddr::V4(0.0.0.0), IpAddr::V6(fe80::1))` — an IPv4 bind address paired with an IPv6 external address. The node would bind only on IPv4 sockets but advertise an IPv6 ENR, so peers would attempt IPv6 connections and find nothing listening. The symmetric case (`--p2p.addr=::` with only IPv4 local) produces the inverse mismatch. Neither case emits a warning (unlike the zero-addresses case on line 418). The `(_, Some(extip))` arm already guards this with an explicit `assert!` on family equality — the same guard (or at minimum a `warn!`) is needed here before returning the cross-family pair.
Reviews (1): Last reviewed commit: "fix(l1): announce local IP when --p2p.ad..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if bind.is_unspecified() { | ||
| let external = if bind.is_ipv6() { | ||
| local_v6.or(local_v4) | ||
| } else { | ||
| local_v4.or(local_v6) | ||
| }; | ||
| match external { | ||
| Some(ext) => { | ||
| info!( | ||
| announced = %ext, | ||
| bind = %bind, | ||
| "--p2p.addr is unspecified; announcing auto-detected local IP. Set --nat.extip to override." | ||
| ); | ||
| (bind, ext) |
There was a problem hiding this comment.
When --p2p.addr=0.0.0.0 is given and local_v4 is None but local_v6 is available, the code returns (IpAddr::V4(0.0.0.0), IpAddr::V6(fe80::1)) — an IPv4 bind address paired with an IPv6 external address. The node would bind only on IPv4 sockets but advertise an IPv6 ENR, so peers would attempt IPv6 connections and find nothing listening. The symmetric case (--p2p.addr=:: with only IPv4 local) produces the inverse mismatch. Neither case emits a warning (unlike the zero-addresses case on line 418). The (_, Some(extip)) arm already guards this with an explicit assert! on family equality — the same guard (or at minimum a warn!) is needed here before returning the cross-family pair.
Prompt To Fix With AI
This is a comment left during a code review.
Path: cmd/ethrex/initializers.rs
Line: 403-416
Comment:
**Silent cross-family address mismatch in unspecified bind fallback**
When `--p2p.addr=0.0.0.0` is given and `local_v4` is `None` but `local_v6` is available, the code returns `(IpAddr::V4(0.0.0.0), IpAddr::V6(fe80::1))` — an IPv4 bind address paired with an IPv6 external address. The node would bind only on IPv4 sockets but advertise an IPv6 ENR, so peers would attempt IPv6 connections and find nothing listening. The symmetric case (`--p2p.addr=::` with only IPv4 local) produces the inverse mismatch. Neither case emits a warning (unlike the zero-addresses case on line 418). The `(_, Some(extip))` arm already guards this with an explicit `assert!` on family equality — the same guard (or at minimum a `warn!`) is needed here before returning the cross-family pair.
How can I resolve this? If you propose a fix, please make it concise.
| let external = if bind.is_ipv6() { | ||
| local_v6.or(local_v4) | ||
| } else { | ||
| local_v4.or(local_v6) |
There was a problem hiding this comment.
Cross-family fallback may advertise an unreachable address. When bind is 0.0.0.0 and local_v4 is None but local_v6 is Some, this falls back to the IPv6 address — but the v4 socket can't accept inbound v6 connections, so peers reaching the ENR endpoint will fail to connect. Same in the v6 branch (line 405) when only local_v4 is available.
In practice this only matters on single-stack hosts where the other stack's local-IP probe somehow succeeds (rare), so it's not a regression vs. main. But the warn-then-fallback path below might be the better default when only the wrong-family IP is found: keep bind as the announced address and emit the warning, since at least operators won't think peering is healthy when it isn't. Consider:
let external = if bind.is_ipv6() { local_v6 } else { local_v4 };(drop the cross-family .or(...))
Non-blocking; corner of a corner. Flagging because the test matrix doesn't cover the v6 path of this branch either — see body.
| bind = %bind, | ||
| "--p2p.addr is unspecified and no local IP could be detected; \ | ||
| announcing the unspecified address. Inbound peer connections will fail. \ | ||
| Set --nat.extip to fix this." |
There was a problem hiding this comment.
nit: the message says "Set --nat.extip to fix this" but the branch is only reached when local_ip() and local_ipv6() both failed — which usually means the host genuinely has no routable IP detectable. An operator hitting this path doesn't necessarily know what extip to pass either. --p2p.addr=<explicit-ip> is the more general alternative.
Suggest:
"--p2p.addr is unspecified and no local IP could be detected; \
announcing the unspecified address. Inbound peer connections will fail. \
Set --nat.extip or use --p2p.addr=<ip> to fix."Low priority — just operator UX.
| let (bind, ext) = resolve_p2p_endpoints(None, Some("203.0.113.5"), None, None); | ||
| assert_eq!(bind, ip("0.0.0.0")); | ||
| assert_eq!(ext, ip("203.0.113.5")); | ||
| } |
There was a problem hiding this comment.
Worth adding a test for the IPv6 unspecified path to mirror p2p_addr_unspecified_v4_announces_local_ip:
#[test]
fn p2p_addr_unspecified_v6_announces_local_ipv6() {
let local6 = ip("fe80::1");
let (bind, ext) = resolve_p2p_endpoints(Some("::"), None, None, Some(local6));
assert_eq!(bind, ip("::"));
assert_eq!(ext, local6);
}The v6 arm of the if bind.is_ipv6() { local_v6.or(local_v4) } switch is currently uncovered. Also missing: a #[should_panic] test for the --p2p.addr / --nat.extip family-mismatch assertion. Both are cheap.
3 participants
Summary
When ethrex is launched with
--p2p.addr=0.0.0.0(or::) and no--nat.extip, the announce/enode/ENR address ended up as the bind address verbatim, i.e.enode://…@0.0.0.0:30303. Peers cannot dial back to that, so the node sits at 0 inbound peers. Spotted on bal-devnet-7 (the deployment passes--p2p.addr=0.0.0.0without--nat.extip).This patch:
get_local_p2p_nodeinto a small pure functionresolve_p2p_endpoints, so it can be unit-tested.--p2p.addris given and resolves to an unspecified address, the announced address now falls back to the auto-detected local IP (the samelocal_ip()/local_ipv6()call the no-flag branch already used).--nat.extipstill overrides; specific bind addresses are unchanged.This is the minimal fix. A follow-up could implement devp2p endpoint prediction (learning the external IP from the
tofield on incoming PINGs/PONGs, à la geth'sUDPEndpointStatement), which would remove the need for operators to set--nat.extipat all on cloud hosts. Out of scope for this PR.Test plan
cargo fmt -p ethrexcargo clippy -p ethrex --all-targets -- -D warningscargo test -p ethrex --lib initializers::— 6 new unit tests pass--p2p.addr=0.0.0.0and confirmadmin_nodeInforeports the host IP in the enode/ENR and inbound peers > 0.