feat(l1): decouple discv4 address from RLPx address

[azteca1998]

Closes #5424

Summary

• Extends NetworkConfig with four address fields instead of two: discovery_bind_addr, discovery_external_addr, rlpx_bind_addr, rlpx_external_addr
• Adds --discovery.addr CLI flag (env: ETHREX_P2P_DISCOVERY_ADDR) to bind the UDP discovery socket independently of the RLPx TCP socket
• discv4 and discv5 servers now receive a discovery_local_node built from discovery_external_addr, so their Ping from endpoints advertise the correct address
• Implements tcp6/udp6 ENR field parsing in NodeRecordPairs (removes the TODO)
• NodeRecord::from_node() now emits ip6/tcp6/udp6 for IPv6 node addresses instead of incorrectly encoding them under the IPv4 ip/tcp/udp keys

Address resolution logic

--discovery.addr set?	Result
No	discovery_bind_addr = rlpx_bind_addr; discovery_external_addr = rlpx_external_addr
Yes, specific IP	discovery_bind_addr = given IP; discovery_external_addr = given IP
Yes, 0.0.0.0/::	discovery_bind_addr = given wildcard; discovery_external_addr = rlpx_external_addr

Part of

This is PR 2 of 3 for IPv6 support (#5354):

• PR 1: decouple bind vs. external address (Decouple local interface address from externally announced address #5425) ← base branch
• PR 2 (this): decouple discv4 address from RLPx address (Support different addresses for discv4 and RLPx #5424)
• PR 3: ENR-based outbound connections + dual-stack listeners (Add support for IPv6 addresses #5354)

[github-actions]

🤖 Kimi Code Review

Review Summary

The PR adds support for running discovery (UDP) on a different address/interface than RLPx (TCP), which is a useful feature for complex network setups. The implementation is generally solid, but there are a few issues to address.

Issues Found

1. Critical: Inconsistent Node Creation in Network Module (crates/networking/p2p/network.rs:131-141)

The discovery_local_node is created using discovery_external_addr but the original Node (created in initializers.rs) uses rlpx_external_addr. This creates inconsistency in how the node's IP is advertised across different protocols.

Fix: The discovery node should use the same external address logic as the main node, or the design needs clarification on why discovery should advertise a different IP.

2. IPv6 Support Issue in ENR Creation (crates/networking/p2p/types.rs:415-430)

The ENR creation logic has a bug: it only adds IPv6-specific ENR entries (ip6, tcp6, udp6) when the IP is IPv6, but it should also support IPv4-mapped IPv6 addresses (::ffff:0:0/96) which should use the IPv4 ENR keys for compatibility.

Fix: Check if IPv6 address is IPv4-mapped and use IPv4 ENR keys in that case.

3. Missing Validation for Discovery Address (cmd/ethrex/initializers.rs:398-404)

The discovery address parsing uses expect() which will panic on invalid input. This should use proper error handling.

Fix: Return a Result instead of panicking, or use unwrap_or_else with proper error logging.

4. Documentation Inconsistency (cmd/ethrex/cli.rs:283)

The help text says "Defaults to --p2p.addr (or 0.0.0.0 if unset)" but the actual implementation defaults to rlpx_bind_addr which might be different from --p2p.addr when NAT is involved.

Fix: Update help text to accurately reflect the default behavior.

Minor Issues

5. Code Duplication (crates/networking/p2p/types.rs:415-430)

The ENR creation logic for IPv4 vs IPv6 is repetitive. Consider extracting into helper functions.

6. Missing Tests (crates/networking/p2p/types.rs)

No tests were added for the new IPv6 ENR functionality or the split address configuration.

Security Considerations

• The PR doesn't introduce obvious security vulnerabilities
• The ability to bind discovery to a different interface could be useful for security (e.g., keeping discovery on a private network while allowing RLPx on public)
• However, the inconsistent node creation (Issue 1) could lead to fingerprinting issues

Performance Impact

• Minimal performance impact - just additional configuration options
• The ENR creation changes are O(1) and won't affect performance

Overall Assessment

The feature is valuable and the implementation is mostly correct, but Issues 1 and 2 need to be addressed before merging. The IPv6 handling in ENR creation particularly needs careful review for compatibility with existing Ethereum clients.

---

Automated review by Kimi (Moonshot AI) · custom prompt

[github-actions]

Lines of code report

Total lines added: 59
Total lines removed: 0
Total lines changed: 59

Detailed view

+-----------------------------------------+-------+------+
| File                                    | Lines | Diff |
+-----------------------------------------+-------+------+
| ethrex/cmd/ethrex/cli.rs                | 981   | +10  |
+-----------------------------------------+-------+------+
| ethrex/cmd/ethrex/initializers.rs       | 643   | +13  |
+-----------------------------------------+-------+------+
| ethrex/crates/networking/p2p/network.rs | 661   | +6   |
+-----------------------------------------+-------+------+
| ethrex/crates/networking/p2p/types.rs   | 497   | +30  |
+-----------------------------------------+-------+------+

[github-actions]

🤖 Codex Code Review

1. High – NAT advertisement regression when --discovery.addr is set
   At initializers.rs:404, discovery_external_addr is forced to discovery_bind_addr whenever bind is non-wildcard, even if --nat.extip is configured. Then discovery uses that address in Ping/ENR via network.rs:131.
   Impact: a node behind NAT with --nat.extip=<public> and --discovery.addr=<private> will advertise a private/unreachable discovery endpoint.
   Suggestion: keep discovery_external_addr derived from --nat.extip/RLPx external by default, and only override with bind address via an explicit separate “discovery external/announce IP” option.

2. Medium – ENR IPv6 port decoding is incomplete (tcp6/udp6 ignored in Node::from_enr)
   This PR adds IPv6 ENR keys on encode (types.rs:430) and decode into NodeRecordPairs, but Node::from_enr still only reads tcp/udp (types.rs:263).
   Impact: IPv6-only ENRs (with ip6 + tcp6/udp6) can still fail with “No port found in record”, so discovery table ingestion may drop valid IPv6 peers.
   Suggestion: when selected IP is v6, resolve ports from udp6/tcp6 first (with fallback rules), then construct Node.

3. Low – CLI help text does not match actual default behavior
   Help says discovery addr defaults to --p2p.addr or 0.0.0.0 if unset (cli.rs:284), but current logic defaults to rlpx_bind_addr (initializers.rs:398), and when both --p2p.addr/--nat.extip are unset that becomes auto-detected local IP, not wildcard.
   Impact: operator confusion and wrong expectations about interface exposure.
   Suggestion: either update help text or change code to match stated behavior.

I could not run tests in this environment because cargo/rustup failed with a read-only rustup temp dir error.

---

Automated review by OpenAI Codex · custom prompt

[github-actions]

🤖 Claude Code Review

Here is the review:

---

PR Review: feat(p2p): decouple discv4 address from RLPx address

Overall: The structural change is clean and well-reasoned. The IPv6 ENR key fix (ip6/tcp6/udp6) is a genuine correctness improvement. However, there is one correctness regression introduced by refactoring from_node.

---

Bug: ENR pair ordering regression in NodeRecord::from_node

File: crates/networking/p2p/types.rs, lines 408–441

EIP-778 requires: "The key/value pairs must be sorted by key and must be unique." The signature is computed over the RLP-encoded pairs, so out-of-order pairs produce a signature over non-canonical content.

Before this PR, the push order was: "id", "ip", "secp256k1", "tcp", "udp" — which is coincidentally alphabetically sorted and therefore valid.

After this PR, "secp256k1" is pushed before the IP/port block, so the actual push order becomes:

"id" → "secp256k1" → "ip" / "tcp" / "udp"

But the required sorted order is:

"id" < "ip" < "secp256k1" < "tcp" < "udp"

"secp256k1" (s) sorts after "ip" (i), so the pairs are no longer ordered. The same issue applies to the IPv6 arm ("ip6" < "secp256k1" < "tcp6" < "udp6").

When does this matter? set_fork_id sorts and re-signs, masking the problem in the happy path. But there are two code paths where the record from from_node is used without a subsequent set_fork_id:

• discv4/server.rs:109–115 — if storage.get_fork_id().await returns Err, the record is used as-is.
• discv5/server.rs:925–937 (update_local_ip) — if decode_pairs().eth is None (no fork_id present), set_fork_id is skipped and the unsigned ordering is propagated.

In both cases, the advertised ENR will have an invalid structure that spec-compliant peers may reject.

Fix: Either restore the original push order (push IP/port entries before "secp256k1"), or add an explicit sort before sign_record inside from_node.

---

Minor: discovery_local_node carries RLPx TCP port under a mismatched IP family

File: crates/networking/p2p/network.rs, lines 131–136

let discovery_local_node = Node::new(
    context.network_config.discovery_external_addr,
    context.network_config.udp_port,
    context.network_config.tcp_port,   // RLPx TCP port
    context.network_config.public_key,
);

When discovery_external_addr is IPv4 but rlpx_external_addr is IPv6 (the split-transport scenario this PR enables), NodeRecord::from_node will emit ip/tcp/udp (IPv4 keys) with a TCP port that points to an IPv6-only RLPx listener. Peers discovering the node via IPv4 discv4 will attempt TCP connections to an endpoint they cannot reach.

This is an inherent architectural constraint of the current design (pre-PR 3 in the series), but worth a comment at the construction site noting the invariant: the advertised tcp_port is only reachable if the peer can also reach rlpx_external_addr on the same family.

---

Positive observations

• The From<NodeRecordPairs> serialization (lines 512–548) pushes pairs in correctly sorted alphabetical order: eth, id, ip, ip6, secp256k1, tcp, tcp6, udp, udp6.
• The IPv6 ENR encoding fix in from_node (using ip6/tcp6/udp6 instead of ip/tcp/udp for IPv6 nodes) is a correct and important fix.
• The NetworkConfig field rename is clean and communicates intent clearly; the four-field split is a good abstraction.
• The address resolution logic for discovery_external_addr (wildcard fallback to rlpx_external_addr) matches the documented table in the PR description.

---

Automated review by Claude (Anthropic) · custom prompt