feat(l1): dual-stack IPv6 UDP discovery sockets

[azteca1998]

Summary

• One UDP socket per address family: start_network() now iterates over NetworkConfig.discovery_bind_addrs and binds a dedicated UDP socket per entry. A dual-stack node (e.g. --discovery.addr 0.0.0.0,::) gets two independent sockets, one for IPv4 and one for IPv6.
• Independent discv4/discv5 per socket: each socket spawns its own Discv4Server and Discv5Server instances (backed by the same shared PeerTable) and its own DiscoveryMultiplexer. Responses always go back on the same-family socket they arrived on.
• Correct Ping from endpoint: each server's local_node carries the external address for that socket's family, so discv4 Ping/Pong packets advertise the right address to remote peers.
• --discovery.addr becomes a Vec: accepts comma-separated addresses (e.g. --discovery.addr 0.0.0.0,::) mirroring --p2p.addr. When omitted, defaults to the same set as --p2p.addr, so dual-stack RLPx automatically implies dual-stack discovery with no extra flags.
• NetworkConfig refactor: discovery_bind_addr/discovery_external_addr (scalar) replaced by discovery_bind_addrs/discovery_external_addrs (Vec<IpAddr>); bind_udp_addr() replaced by bind_udp_addrs().

Depends on

• feat(l1): dual-stack IPv6 support and ENR-based outbound connections #6376 (dual-stack RLPx TCP listeners + ENR-based outbound connections)

Usage

Single-stack IPv4 (unchanged default):

ethrex  # discovery.addr defaults to p2p.addr, which auto-detects local IPv4

Dual-stack:

ethrex --p2p.addr 0.0.0.0,:: --nat.extip <ipv4> 
# discovery.addr defaults to [0.0.0.0, ::], two UDP sockets are bound

IPv6-only:

ethrex --p2p.addr :: --nat.extip <ipv6>

Override discovery to IPv4-only while RLPx is dual-stack:

ethrex --p2p.addr 0.0.0.0,:: --discovery.addr 0.0.0.0

Test plan

• cargo check passes (verified locally)
• Default single-stack: one UDP socket bound, behaviour identical to before
• --discovery.addr 0.0.0.0,::: two UDP sockets visible via lsof -iUDP:30303
• IPv6 discv4 ping received on IPv6 socket and responded on the same socket
• Discovered IPv6 peers land in the shared peer table

[github-actions]

🤖 Kimi Code Review

Review Summary

This PR adds dual-stack support for discovery by allowing multiple UDP bind addresses. The changes are well-structured and follow Rust best practices. However, there are a few issues to address:

Critical Issues

1. Port exhaustion risk (crates/networking/p2p/network.rs:122-191): The loop creates separate UDP sockets for each bind address but uses the same port number for all. This will fail on most systems when binding to both IPv4 and IPv6 on the same port (e.g., 0.0.0.0:30303 and [::]:30303). The OS typically reserves the port for the first socket, causing the second bind to fail with AddrInUse.

2. Missing validation for discovery address count (cmd/ethrex/initializers.rs:415-425): There's no validation that the number of discovery addresses doesn't exceed 2 (IPv4 + IPv6). Users could supply 3+ addresses leading to undefined behavior.

Security Concerns

3. Unvalidated IP parsing (cmd/ethrex/initializers.rs:421-424): Using .expect() on user input parsing can panic. Should use proper error handling and provide user-friendly error messages.

Performance & Correctness

4. Inefficient bootnode cloning (crates/networking/p2p/network.rs:141, 161): The bootnodes.clone() inside the loop is inefficient when there are multiple discovery addresses. Clone once outside the loop.

5. Missing length validation for address vectors (crates/networking/p2p/types.rs): The discovery_bind_addrs and discovery_external_addrs vectors should always have the same length, but this invariant isn't enforced or documented.

Minor Issues

6. Incomplete help text (cmd/ethrex/cli.rs:291-297): The help mentions "one or two comma-separated addresses" but doesn't specify the format (IPv4/IPv6) or validation rules.

7. Default value inconsistency (cmd/ethrex/cli.rs:286): The discovery_addr field uses Vec<String> but the help text implies optional usage. Consider using Option<Vec<String>> or clarifying the empty vec behavior.

Suggested Fixes

1. For the port exhaustion issue, either:

   • Use different ports for IPv4/IPv6 discovery (e.g., 30303 and 30304)
   • Implement SO_REUSEPORT where supported
   • Document this limitation clearly

2. Add validation in initializers.rs:

if discovery_bind_addrs.len() > 2 {
    panic!("Maximum 2 discovery addresses supported (IPv4 + IPv6)");
}

3. Replace .expect() with proper error handling:

.map(|a| a.parse()
    .map_err(|_| format!("Invalid discovery address: {}", a)))?

4. Move bootnodes.clone() outside the loop.

---

Automated review by Kimi (Moonshot AI) · custom prompt

[github-actions]

🤖 Claude Code Review

PR #6377 — feat(p2p): dual-stack IPv6 UDP discovery sockets Review

Overall: The design is sound — one UDP socket per address family is the correct approach for dual-stack. The code is well-commented and follows existing patterns. However there are a few correctness and robustness issues worth addressing before merge.

---

Bugs / Correctness

1. Silent zip truncation in network.rs (~line 133)

let udp_pairs: Vec<(SocketAddr, IpAddr)> = context
    .network_config
    .bind_udp_addrs()
    .into_iter()
    .zip(
        context
            .network_config
            .discovery_external_addrs
            .iter()
            .copied(),
    )
    .collect();

Iterator::zip silently truncates to the shorter iterator. If discovery_bind_addrs and discovery_external_addrs diverge in length for any reason (bug in initializer, future NetworkConfig construction path), some sockets are silently dropped with no error or warning. Since both vecs are constructed in lock-step in the initializer, this is unlikely in practice, but the invariant is not enforced at the type level.

Suggestion: add a debug_assert_eq! (or a runtime check with an early return/error) before the zip:

debug_assert_eq!(
    context.network_config.discovery_bind_addrs.len(),
    context.network_config.discovery_external_addrs.len(),
    "discovery_bind_addrs and discovery_external_addrs must have equal length"
);

---

2. Panic on user input in initializers.rs

opts.discovery_addr
    .iter()
    .map(|a| a.parse().expect("Failed to parse --discovery.addr address"))
    .collect()

.expect() on user-supplied input causes a panic rather than a clean error message. The previous code had the same problem, but now that discovery_addr is a Vec it's worth fixing. Should propagate an error (anyhow::bail! / ?) consistent with the rest of the initializer function.

---

3. Half-started state on bind error in network.rs

The loop spawns discv4/discv5 servers and starts the multiplexer for each socket in sequence:

for (bind_addr, external_addr) in udp_pairs {
    let udp_socket = Arc::new(
        UdpSocket::bind(bind_addr)
            .await
            .map_err(NetworkError::UdpSocketError)?,  // early return here
    );
    // ... spawns servers ...
    DiscoveryMultiplexer::new(...).start();
}

If the first socket binds and its servers are spawned successfully, but the second socket's bind() fails (e.g. EADDRINUSE when binding [::]:30303 on Linux where IPV6_V6ONLY is off by default), the function returns an error but the first socket's tasks are already running. This leaves the node in a half-started state.

The fix depends on the broader task lifecycle — at minimum, a comment documenting this behaviour and the OS-level caveat about IPV6_V6ONLY would help operators diagnose a port-conflict failure.

---

Minor Issues

4. num_args = 0.. doesn't enforce the "one or two" documented limit (cli.rs)

The long_help says "One or two comma-separated addresses" but num_args = 0.. accepts any count. Either tighten to num_args = 0..=2 or remove the "one or two" wording and let users supply however many addresses they want.

5. bootnodes.clone() inside the loop (network.rs)

bootnodes is cloned once per socket (for Discv4Server::spawn) and potentially again for Discv5Server::spawn. For a dual-stack node this means 4 clones instead of 2. Since bootnode lists are small this is not a performance concern, but it's worth noting for awareness. No action required.

6. context.local_node.node_id() is shared across multiplexers

Both DiscoveryMultiplexer instances use the same node_id. This is correct — the node ID is derived from the private key and is family-agnostic — but a brief comment noting this intention would aid reviewers unfamiliar with the design.

---

What looks good

• The per-family external address derivation logic in initializers.rs correctly handles the NAT + wildcard combinations for both IPv4 and IPv6.
• Keeping a single shared PeerTable across all sockets avoids duplicating peer state.
• from_node() in types.rs is correctly updated to the new Vec API.
• The comment block above start_network clearly explains the purpose of udp_pairs.

---

Automated review by Claude (Anthropic) · custom prompt

[github-actions]

🤖 Codex Code Review

Findings:

1. High: partial startup leaves background discovery tasks running on error
   In network.rs, each UDP family is started immediately inside the loop, and DiscoveryMultiplexer::start() is fire-and-forget.
   If a later bind/spawn fails (e.g., second family), start_network returns Err, but earlier discovery tasks stay alive. That creates inconsistent node state (startup reported failed, yet discovery still running and ports occupied).
   Suggested fix: stage startup in two phases:

   • phase 1: validate config and bind/spawn all per-family components, collecting handles;
   • phase 2: start multiplexers only after all families succeeded, or explicitly abort already-started tasks on failure.

2. Medium: silent truncation when pairing bind/external discovery addresses
   zip() pairing silently drops extra entries if discovery_bind_addrs.len() != discovery_external_addrs.len().
   That can make configured sockets or announced addresses disappear without error, which is risky for discovery correctness.
   Suggested fix: explicitly check equal lengths and return a config error if mismatched.

3. Low/Medium: CLI contract says “one or two”, but parser accepts zero or many
   --discovery.addr uses num_args = 0.., and initializer accepts all parsed entries without cardinality/family validation (initializers.rs).
   This allows unexpected configurations (empty via bare flag, >2 addresses, duplicate family), which can produce ambiguous behavior and extra discovery workers.
   Suggested fix: enforce 1..=2 for explicit flag usage and validate “max one IPv4 + max one IPv6”.

No EVM opcode/gas/consensus/state-transition logic is touched in this diff, so no direct execution-layer correctness concerns from these changes.

---

Automated review by OpenAI Codex · custom prompt

[github-actions]

Lines of code report

Total lines added: 130
Total lines removed: 0
Total lines changed: 130

Detailed view

+-----------------------------------------------------------+-------+------+
| File                                                      | Lines | Diff |
+-----------------------------------------------------------+-------+------+
| ethrex/cmd/ethrex/cli.rs                                  | 992   | +5   |
+-----------------------------------------------------------+-------+------+
| ethrex/cmd/ethrex/initializers.rs                         | 676   | +11  |
+-----------------------------------------------------------+-------+------+
| ethrex/crates/networking/p2p/discv4/server.rs             | 675   | +8   |
+-----------------------------------------------------------+-------+------+
| ethrex/crates/networking/p2p/discv5/server.rs             | 852   | +3   |
+-----------------------------------------------------------+-------+------+
| ethrex/crates/networking/p2p/network.rs                   | 700   | +36  |
+-----------------------------------------------------------+-------+------+
| ethrex/crates/networking/p2p/peer_table.rs                | 1379  | +53  |
+-----------------------------------------------------------+-------+------+
| ethrex/crates/networking/p2p/rlpx/connection/handshake.rs | 493   | +3   |
+-----------------------------------------------------------+-------+------+
| ethrex/crates/networking/p2p/types.rs                     | 580   | +11  |
+-----------------------------------------------------------+-------+------+