Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
167 changes: 142 additions & 25 deletions cmd/ethrex/initializers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -360,27 +360,27 @@ pub fn get_signer(datadir: &Path) -> SecretKey {
}
}

pub fn get_local_p2p_node(opts: &Options, signer: &SecretKey) -> (Node, NetworkConfig) {
let tcp_port = opts.p2p_port.parse().expect("Failed to parse p2p port");
let udp_port = opts
.discovery_port
.parse()
.expect("Failed to parse discovery port");

let local_public_key = public_key_from_signing_key(signer);

// Determine bind and external addresses.
//
// --nat.extip sets the address announced to peers (for nodes behind NAT).
// --p2p.addr sets the bind address (defaults to the auto-detected local IP
// when --nat.extip is not given, or to the unspecified address when it is:
// 0.0.0.0 for IPv4, :: for IPv6).
let (bind_addr, external_addr): (IpAddr, IpAddr) = match (&opts.p2p_addr, &opts.nat_extip) {
/// Decide the bind and externally-announced addresses for the P2P endpoint.
///
/// Precedence:
/// - `--nat.extip` wins for the announced address; bind comes from `--p2p.addr` if given,
/// else the unspecified address of the matching family.
/// - `--p2p.addr` alone is used for both bind and announce, except when it's an unspecified
/// address (`0.0.0.0` / `::`). In that case the announced address falls back to the
/// auto-detected local IP of the matching family; this avoids advertising `0.0.0.0` in
/// the ENR, which would make the node unreachable for inbound connections. Operators
/// behind NAT still need `--nat.extip` for that case to resolve correctly.
/// - With neither flag set, the auto-detected local IP is used for both bind and announce.
fn resolve_p2p_endpoints(
p2p_addr: Option<&str>,
nat_extip: Option<&str>,
local_v4: Option<IpAddr>,
local_v6: Option<IpAddr>,
) -> (IpAddr, IpAddr) {
match (p2p_addr, nat_extip) {
(_, Some(extip)) => {
let external: IpAddr = extip.parse().expect("Failed to parse --nat.extip address");
let bind: IpAddr = opts
.p2p_addr
.as_deref()
let bind: IpAddr = p2p_addr
.map(|a| {
let addr: IpAddr = a.parse().expect("Failed to parse p2p address");
assert!(
Expand All @@ -399,16 +399,59 @@ pub fn get_local_p2p_node(opts: &Options, signer: &SecretKey) -> (Node, NetworkC
(bind, external)
}
(Some(addr), None) => {
let ip: IpAddr = addr.parse().expect("Failed to parse p2p address");
(ip, ip)
let bind: IpAddr = addr.parse().expect("Failed to parse p2p address");
if bind.is_unspecified() {
// Stay in the same address family: an IPv4 socket can't accept
// inbound IPv6 connections (and vice versa), so falling back
// across families would just advertise an unreachable address.
let external = if bind.is_ipv6() { local_v6 } else { local_v4 };
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)
Comment on lines +403 to +415
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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.

}
None => {
warn!(
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=<ip> or --p2p.addr=<ip> to fix."
);
(bind, bind)
}
}
} else {
(bind, bind)
}
}
(None, None) => {
let ip = local_ip().unwrap_or_else(|_| {
local_ipv6().expect("Neither ipv4 nor ipv6 local address found")
});
let ip = local_v4
.or(local_v6)
.expect("Neither ipv4 nor ipv6 local address found");
(ip, ip)
}
};
}
}

pub fn get_local_p2p_node(opts: &Options, signer: &SecretKey) -> (Node, NetworkConfig) {
let tcp_port = opts.p2p_port.parse().expect("Failed to parse p2p port");
let udp_port = opts
.discovery_port
.parse()
.expect("Failed to parse discovery port");

let local_public_key = public_key_from_signing_key(signer);

let (bind_addr, external_addr) = resolve_p2p_endpoints(
opts.p2p_addr.as_deref(),
opts.nat_extip.as_deref(),
local_ip().ok(),
local_ipv6().ok(),
);

let node = Node::new(external_addr, udp_port, tcp_port, local_public_key);
let network_config = NetworkConfig {
Expand Down Expand Up @@ -812,3 +855,77 @@ pub async fn regenerate_head_state(

Ok(())
}

#[cfg(test)]
mod tests {
use super::resolve_p2p_endpoints;
use std::net::IpAddr;

fn ip(s: &str) -> IpAddr {
s.parse().unwrap()
}

#[test]
fn p2p_addr_unspecified_v4_announces_local_ip() {
let local = ip("10.0.0.5");
let (bind, ext) = resolve_p2p_endpoints(Some("0.0.0.0"), None, Some(local), None);
assert_eq!(bind, ip("0.0.0.0"));
assert_eq!(ext, local);
}

#[test]
fn p2p_addr_unspecified_without_local_ip_keeps_unspecified() {
let (bind, ext) = resolve_p2p_endpoints(Some("0.0.0.0"), None, None, None);
assert_eq!(bind, ip("0.0.0.0"));
assert_eq!(ext, ip("0.0.0.0"));
}

#[test]
fn extip_overrides_unspecified_bind() {
let (bind, ext) = resolve_p2p_endpoints(
Some("0.0.0.0"),
Some("203.0.113.5"),
Some(ip("10.0.0.5")),
None,
);
assert_eq!(bind, ip("0.0.0.0"));
assert_eq!(ext, ip("203.0.113.5"));
}

#[test]
fn specific_p2p_addr_used_for_both() {
let (bind, ext) =
resolve_p2p_endpoints(Some("10.0.0.5"), None, Some(ip("192.168.1.1")), None);
assert_eq!(bind, ip("10.0.0.5"));
assert_eq!(ext, ip("10.0.0.5"));
}

#[test]
fn no_flags_uses_local_v4_when_available() {
let local = ip("10.0.0.5");
let (bind, ext) = resolve_p2p_endpoints(None, None, Some(local), Some(ip("fe80::1")));
assert_eq!(bind, local);
assert_eq!(ext, local);
}

#[test]
fn extip_only_uses_unspecified_bind() {
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"));
}
Comment thread
edg-l marked this conversation as resolved.

#[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);
}

#[test]
#[should_panic(expected = "--p2p.addr and --nat.extip must use the same address family")]
fn family_mismatch_panics() {
let _ = resolve_p2p_endpoints(Some("0.0.0.0"), Some("::1"), None, None);
}
}
Loading