fix: parse bootnode IP addresses

[MegaRedHand]

We had a hardcoded value of 127.0.0.1 for bootnode IPs. This worked for local devnets but not for remote deployments.

This PR adds parsing from the ENRs to populate this value, including both "ip" (IPv4) and "ip6" (IPv6) fields, but using the IPv4 address only if both are specified.

[github-actions]

🤖 Kimi Code Review

Security & Correctness Issues

Line 465-467: The expect calls will panic if:

• The ENR is missing the "ip" key (common in real networks)
• The IP bytes aren't valid IPv4 (could be IPv6)

Line 468: Hardcoded IPv4 assumption - this will crash on IPv6-only networks.

Suggested Fix

Replace the parsing logic with proper error handling:

let ip = record.pairs
    .iter()
    .find(|(key, _)| key.as_ref() == b"ip")
    .and_then(|(_, bytes)| {
        // Try IPv4 first, then IPv6
        Ipv4Addr::decode(bytes.as_ref())
            .map(IpAddr::from)
            .or_else(|_| Ipv6Addr::decode(bytes.as_ref()).map(IpAddr::from))
    })
    .unwrap_or_else(|| {
        // Log warning and skip this ENR instead of panicking
        warn!("Skipping ENR without valid IP: {}", enr);
        return None;
    })?;

Also, the function should return Result<Vec<Bootnode>, ParseError> instead of panicking on malformed input.

Minor Issues

• Line 458: Remove unused Ipv4Addr import after fixing the IPv6 support
• Consider adding validation that quic_port is within valid range (1-65535)

---

Automated review by Kimi (Moonshot AI) · custom prompt

[github-actions]

🤖 Codex Code Review

Focused review of the diff in crates/net/p2p/src/lib.rs (no consensus-layer code touched in this PR).

Findings (ordered by severity)

• Potential crash on malformed/unexpected ENRs: parse_enrs now expects an ip key and unwraps IPv4 decoding; a peer-provided ENR missing ip, using only ip6, or containing malformed bytes will panic and can take the node down. This is a network-sourced input, so it should be handled as fallible data. Consider returning a Result or skipping bad entries instead of panicking. crates/net/p2p/src/lib.rs:461-466
• IPv6 ENRs unsupported: Only ip (IPv4) is checked. Many ENRs include ip6 without ip; those will now hard-fail (panic) rather than be skipped or supported. Prefer checking ip then ip6, and decode accordingly (Ipv4Addr/Ipv6Addr). crates/net/p2p/src/lib.rs:461-466
• Behavior change from safe default to hard failure: Previously bootnodes defaulted to 127.0.0.1 if ip wasn’t parsed; now it panics. If those ENRs are from config or user input, this becomes a reliability regression. If you want strict validation, surface a clear error to the caller instead of panicking. crates/net/p2p/src/lib.rs:461-470

Security implications

• This introduces a straightforward DoS vector: a malformed or IPv6-only ENR can crash the client during bootnode parsing. If the ENRs are user-provided, this is a footgun; if they are network-provided, it’s a remote DoS risk. Treat ENR parsing as untrusted input.

Performance

• One extra linear scan of record.pairs to find ip is negligible, but you now scan pairs three times. Consider a single pass or building a small map if this runs on large lists, though likely not hot.

Rust best practices

• Avoid unwrap/expect on untrusted input. Return Result<Vec<Bootnode>>, or log + skip invalid entries. This improves reliability and makes failures explicit.

Suggested direction

• Parse ip or ip6 with graceful fallback; if neither exists, skip the ENR or return an error.
• Replace the expect/unwrap chain with ok_or / ? to avoid panics.

If you want, I can draft a small refactor to handle ip/ip6 safely and keep bootnode parsing resilient.

Suggested next steps:

1. If you want a patch, tell me whether you prefer “skip invalid ENRs” or “return error to caller”.

---

Automated review by OpenAI Codex · custom prompt

[github-actions]

🤖 Claude Code Review

Based on my review of the PR, here's my analysis:

---

PR Review: Fix bootnode IP address parsing

Summary

The PR correctly fixes the hardcoded 127.0.0.1 IP address by parsing it from ENR records. The implementation is sound for its intended use case.

Findings

⚠️ IPv6 Support Issue (Medium Priority)

Location: crates/net/p2p/src/lib.rs:466

The code only handles IPv4 addresses:

let ip = IpAddr::from(Ipv4Addr::decode(ip_bytes.as_ref()).expect("invalid IPv4 address"));

Issue: ENR records can contain both ip (IPv4) and ip6 (IPv6) fields according to EIP-778. If a bootnode provides an IPv6-only ENR, this code will panic with "node record missing IP address".

Recommendation:

// Try IPv4 first, fall back to IPv6
let ip = if let Some((_, ip_bytes)) = record.pairs.iter().find(|(k, _)| k.as_ref() == b"ip") {
    IpAddr::V4(Ipv4Addr::decode(ip_bytes.as_ref()).expect("invalid IPv4 address"))
} else if let Some((_, ip6_bytes)) = record.pairs.iter().find(|(k, _)| k.as_ref() == b"ip6") {
    IpAddr::V6(Ipv6Addr::decode(ip6_bytes.as_ref()).expect("invalid IPv6 address"))
} else {
    panic!("node record missing IP address (neither 'ip' nor 'ip6' found)")
};

Note: This would require adding Ipv6Addr to the imports on line 3.

🔍 Error Handling Consistency

Location: crates/net/p2p/src/lib.rs:440-475

The function uses a mix of .unwrap() and .expect() for error handling. While this is consistent with the existing pattern in parse_enrs, all errors here are unrecoverable panics.

Observation: This is acceptable for bootnode parsing (fail-fast on invalid configuration), but consider whether a more graceful degradation (skip invalid ENRs with a warning) would be better for production deployments.

✅ Correct Implementation Details

1. RLP Decoding: Properly uses Ipv4Addr::decode() from the RLP library
2. Field Lookup: Correctly searches for the "ip" key in ENR pairs
3. Type Conversion: Proper use of IpAddr::from() for type conversion
4. Import Added: Correctly added Ipv4Addr to imports

📝 Minor: Missing Test Coverage

There are no tests for parse_enrs(). Consider adding a test case with:

• Valid ENR with IPv4
• Valid ENR with IPv6 (if supported)
• Invalid ENR (malformed IP)

Overall Assessment

Approval Status: ✅ Approve with suggestions

The PR correctly fixes the immediate issue for IPv4 bootnodes. The IPv6 limitation should be addressed if:

1. The network plans to support IPv6 bootnodes
2. Cross-client compatibility is important (other clients may use IPv6)

For local/testnet deployments with IPv4-only bootnodes, this implementation is sufficient.

---

Automated review by Claude (Anthropic) · custom prompt

[greptile-apps]

Greptile Overview

Greptile Summary

• Replaced hardcoded bootnode IP (127.0.0.1) with IP parsed from each ENR node record.
• parse_enrs now extracts the ip field alongside quic port and secp256k1 public key, then uses it when building the dial Multiaddr.
• Main integration point is start_p2p dialing bootnodes using the parsed Bootnode { ip, quic_port, public_key }.
• Current implementation only handles IPv4 via ENR key ip; IPv6-only/ip6 ENRs will fail at runtime.

Confidence Score: 3/5

• Merge is reasonable after fixing IPv6/ENR IP key handling in bootnode parsing.
• Change is small and localized, but the new parsing will reliably panic on valid bootnode ENRs that advertise only ip6 or omit ip, which is a likely production configuration in remote/IPv6 environments.
• crates/net/p2p/src/lib.rs (bootnode ENR IP parsing)

Important Files Changed

Filename	Overview
crates/net/p2p/src/lib.rs	Parses bootnode IP from ENR record instead of hardcoding 127.0.0.1, but the new code only supports the ENR ip (IPv4) key and will panic for valid ENRs that use ip6/non-IPv4 encodings.

Sequence Diagram

sequenceDiagram
    participant Cfg as Config (bootnodes ENRs)
    participant P2P as p2p::start_p2p
    participant Parser as parse_enrs
    participant ENR as NodeRecord
    participant Swarm as libp2p::Swarm

    Cfg->>P2P: bootnodes: Vec<String>
    P2P->>Parser: parse_enrs(enrs)
    loop each enr_str
        Parser->>ENR: base64 decode + RLP decode
        Parser->>ENR: find "quic" + "secp256k1"
        Parser->>ENR: find "ip" (new)
        Parser-->>Parser: decode IPv4 bytes => IpAddr
        Parser-->>P2P: Bootnode { ip, quic_port, public_key }
    end
    P2P->>Swarm: dial /ip{ip}/udp/{quic_port}/quic-v1/p2p/{peer_id}
    Swarm-->>P2P: ConnectionEstablished / ConnectionClosed events

Loading

[greptile-apps]

_{1 file reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}