fix(l1): update existing contact ENR on NODES response

[ElFantasma]

Motivation

new_contact_records() only inserted new contacts (Entry::Vacant), silently discarding updated ENRs for already-known peers. This made the FINDNODE(distance=0) request from #5910 ineffective — the request was sent but the response couldn't update the existing record.

Description

• Handle Entry::Occupied in new_contact_records(): when the incoming record has a higher seq than the existing one, update the contact's node, record, and is_fork_id_valid.
• Add ENR signature validation (verify_signature()) upfront for all records, which was previously missing even for new contacts.
• Hoist shared logic (discarded contacts check, fork ID validation) before the match to avoid duplication.

Closes #6166

[github-actions]

🤖 Kimi Code Review

Security & Correctness Issues

1. Missing signature verification in new_contact_records (Line 846-847)

The signature verification is only performed for new contacts, but not for existing contacts being updated. This allows an attacker to:

• Send a valid ENR with higher sequence number
• Replace it with an invalid signature ENR later
• The invalid signature won't be caught because the verify_signature() check is skipped for Entry::Occupied

Fix: Move signature verification before the match statement:

if !node_record.verify_signature() {
    continue;
}

2. Race condition in ENR sequence handling (Lines 865-885)

The current implementation uses != instead of > for sequence comparison, which violates the Ethereum Node Discovery v5 spec. This could lead to:

• Accepting outdated ENRs with lower sequence numbers
• Potential DoS via sequence number rollback attacks

Fix: Change line 880 from:

if pong_message.enr_seq != cached_seq {

to:

if pong_message.enr_seq > cached_seq {

3. Potential panic in generate_req_id (Line 902)

The OsRng is created fresh for each call, which could theoretically fail if the OS entropy source is exhausted. While unlikely, this could cause panics in production.

Fix: Use a shared RNG instance or handle the error case:

fn generate_req_id() -> Bytes {
    static RNG: once_cell::sync::Lazy<Mutex<OsRng>> = once_cell::sync::Lazy::new(|| Mutex::new(OsRng));
    let mut rng = RNG.lock().unwrap();
    Bytes::from(rng.r#gen::<u64>().to_be_bytes().to_vec())
}

4. Inefficient ENR updates (Lines 873-885)

When updating an existing contact, the code re-fetches the contact from the peer table after already having it in the match arm. This is redundant.

Fix: Use the already available occupied_entry directly instead of calling get_contact again.

5. Test reliability issues

• The test test_enr_update_request_on_pong uses hardcoded IP addresses and ports which could conflict in CI
• The test doesn't verify the actual FINDNODE message content being sent
• Missing test for signature verification in ENR updates

Minor Issues

1. Code duplication: The generate_req_id() function is simple but could be inlined since it's only used in 3 places
2. Logging: Consider adding debug logs for signature verification failures
3. Documentation: The comment about using != instead of > should be updated to reflect the spec compliance

Summary

The PR introduces important ENR update functionality but has critical security issues around signature verification and sequence number handling that must be addressed before merging.

---

Automated review by Kimi (Moonshot AI) · custom prompt

[github-actions]

🤖 Codex Code Review

Findings (ordered by severity)

• High: Repeated FINDNODE spam when remote enr_seq is lower than cached. handle_pong now triggers on != (line 479-497), but new_contact_records only updates on > (line 871-881). If we ever cache a higher seq and the peer rolls back or is behind, every subsequent PONG will trigger a FINDNODE, never update the record, and keep adding to pending_by_nonce and network traffic. Suggest gating to pong_message.enr_seq > cached_seq, or special-case cached_seq == 0 && pong_seq > 0 to fetch missing ENR, and/or add a cooldown to avoid repeated requests. crates/networking/p2p/discv5/server.rs:479-497

• Medium: Unnecessary fork-id validation work for stale ENRs. new_contact_records computes is_fork_id_valid before knowing whether the ENR will be inserted/updated, and still does so even if node_record.seq <= existing_seq (line 854-877). This calls into storage for every stale/duplicate ENR and can be abused to increase load. Consider moving the fork-id validation into the Vacant branch and the Occupied update branch after the seq comparison. crates/networking/p2p/discv5/peer_table.rs:854-877

Notes

• Signature verification before Node::from_enr looks good.
• Update-on-newer-seq behavior is a reasonable improvement; just watch the interaction with the new PONG logic above.

If you want, I can propose a minimal patch to fix the FINDNODE trigger and move the fork-id check behind the seq gate.

Potential next steps

1. Adjust handle_pong to only request ENR when pong_seq > cached_seq (or add a cooldown if you want to keep != for safety).
2. Move fork-id validation to only run when the record will actually be inserted/updated.

---

Automated review by OpenAI Codex · custom prompt

[github-actions]

🤖 Claude Code Review

Now let me compile my review. I have enough context to provide a comprehensive code review.

Code Review: PR #6172 - fix(l1): update existing contact ENR on NODES response

Overall Assessment

This PR successfully addresses the issue where updated ENRs for existing contacts were silently discarded. The implementation is sound and the added test coverage is excellent. However, there are several concerns regarding correctness, security, and potential edge cases.

---

Critical Issues

1. Incomplete Contact State Preservation (peer_table.rs:878-881)

When updating an existing contact, only node, record, and is_fork_id_valid are updated. Other important state fields are not preserved, which could lead to bugs:

contact.node = node;
contact.record = Some(node_record);
contact.is_fork_id_valid = is_fork_id_valid;

Missing updates:

• validation_timestamp - Should this be reset when the node changes? If the IP/port changes, the old validation is stale.
• ping_req_id - If node address changed, pending ping should likely be cleared.
• disposable - May need reconsideration if the node's endpoint changed.
• knows_us - Should likely be preserved (not reset).
• unwanted - Should be preserved unless we want to give peers a second chance.

Recommendation: Add a comment explaining why these fields are intentionally preserved, or update them if needed. At minimum:

// If the node's network address changed, clear validation state
if contact.node.ip != node.ip || contact.node.tcp_port != node.tcp_port {
    contact.validation_timestamp = None;
    contact.ping_req_id = None;
}

---

2. ENR Sequence Number Semantics (server.rs:480-481, 485-486)

The comment justifies using != instead of > to handle "rollbacks or resets":

// The spec says to check for `>`, but we use `!=` to be more defensive
// (e.g. handle rollbacks or resets).
let cached_seq = contact.record.as_ref().map_or(0, |r| r.seq);
if pong_message.enr_seq != cached_seq {

Issues:

1. Spec Deviation: The DiscV5 spec explicitly says ENR sequence numbers are monotonically increasing. Using != deviates from spec without strong justification.
2. DoS Vector: A malicious peer could repeatedly send PONGs with enr_seq = cached_seq - 1 to trigger unnecessary FINDNODE requests, wasting bandwidth and processing.
3. Legitimate Downgrades: ENR seq decreases should not happen in practice. If they do, it indicates either:
   • A spoofing attack
   • Node crashed and lost state (but kept same node ID)
   • Implementation bug

Recommendation:

• Use > as per spec for the primary check
• Add a separate check for < that logs a warning about potential issues
• Consider rate-limiting FINDNODE(distance=0) requests per peer

let cached_seq = contact.record.as_ref().map_or(0, |r| r.seq);
if pong_message.enr_seq > cached_seq {
    // Normal case: peer has newer ENR
    let find_node = Message::FindNode(FindNodeMessage { /* ... */ });
    self.send_ordinary(find_node, &contact.node).await?;
} else if pong_message.enr_seq < cached_seq {
    warn!(
        from = %sender_id,
        cached_seq,
        pong_seq = pong_message.enr_seq,
        "ENR sequence number decreased - possible node reset or attack"
    );
}

---

High Priority Issues

3. Race Condition in seq Comparison (peer_table.rs:877)

The comparison node_record.seq > existing_seq has a subtle issue when existing_seq = 0:

let existing_seq = occupied_entry
    .get()
    .record
    .as_ref()
    .map_or(0, |r| r.seq);
if node_record.seq > existing_seq {

Problem: If we have a contact with record = None (which maps to existing_seq = 0), any incoming ENR with seq = 0 won't update it. According to the ENR spec, seq = 0 is a valid initial sequence number.

Recommendation: Change to >= for the case where we have no record:

let should_update = match occupied_entry.get().record.as_ref() {
    None => true, // Always accept first ENR
    Some(r) => node_record.seq > r.seq,
};
if should_update {
    // ... update
}

---

4. Missing Metrics Update (peer_table.rs:871-883)

When updating an existing contact with a new ENR, no metric is recorded. This makes it hard to monitor how often ENR updates occur.

Recommendation: Add a metric:

Entry::Occupied(mut occupied_entry) => {
    // ...
    if node_record.seq > existing_seq {
        let contact = occupied_entry.get_mut();
        // ... update fields
        METRICS.record_enr_update().await; // New metric
    }
}

---

Medium Priority Issues

5. Code Duplication in Fork ID Validation (peer_table.rs:854-862)

The fork ID validation logic is duplicated and executed for all records, even if they won't be used (e.g., if seq check fails).

Recommendation: Move fork ID validation inside the update branches to avoid unnecessary async work:

match self.contacts.entry(node_id) {
    Entry::Vacant(vacant_entry) => {
        let is_fork_id_valid = validate_fork_id(&node_record, &self.store).await;
        // ... create contact
    }
    Entry::Occupied(mut occupied_entry) => {
        if should_update {
            let is_fork_id_valid = validate_fork_id(&node_record, &self.store).await;
            // ... update contact
        }
    }
}

---

6. Test Gap: ENR Update Flow (server.rs:1010-1099)

The test test_enr_update_request_on_pong verifies that FINDNODE is sent when enr_seq differs, but doesn't verify:

1. That the subsequent NODES response actually updates the contact
2. That the update preserves/resets appropriate fields
3. That seq rollback scenarios are handled

Recommendation: Add a follow-up test:

#[tokio::test]
async fn test_enr_update_completion() {
    // Setup with initial ENR seq=5
    // Send PONG with seq=10 (triggers FINDNODE)
    // Simulate NODES response with seq=10 ENR
    // Verify contact was updated with new ENR
    // Verify old fields preserved appropriately
}

---

Low Priority / Style Issues

7. Function Extraction (server.rs:901-906)

Good refactor extracting generate_req_id(), but consider making it more explicit:

/// Generate a unique request ID for discovery protocol messages.
fn generate_req_id() -> Bytes {
    let mut rng = OsRng;
    Bytes::from(rng.r#gen::<u64>().to_be_bytes().to_vec())
}

---

8. Unnecessary Clone (server.rs:476)

The req_id is cloned but might not need to be:

.record_pong_received(&sender_id, pong_message.req_id.clone())

Check if record_pong_received actually needs ownership. If not, change signature to accept &Bytes.

---

Positive Aspects ✅

1. ENR Signature Validation: Excellent addition at peer_table.rs:846
2. Code Organization: Hoisting common checks before the match reduces duplication
3. Test Coverage: Comprehensive test verifying both same-seq and different-seq scenarios
4. Documentation: Good inline comments explaining the logic and spec deviation
5. Atomic Updates: Using Entry::Occupied properly ensures thread-safe updates

---

Security Considerations

1. DoS via Signature Validation: verify_signature() involves cryptographic operations. A malicious peer could send many invalid ENRs. Consider rate-limiting validation attempts per source IP.

2. State Confusion: If ENR seq can decrease (with != check), a peer could potentially downgrade to an older ENR with different capabilities, potentially bypassing security checks.

---

Summary

The PR correctly implements ENR updates for existing contacts and adds proper signature validation. The main concerns are:

1. Must fix: ENR seq comparison should follow spec (> not !=) or provide stronger justification
2. Should fix: Handle seq = 0 edge case properly
3. Should fix: Clarify which contact fields should be preserved vs reset
4. Nice to have: Add metrics and improve test coverage

The code is generally well-structured and the problem is solved correctly at a high level, but the details need refinement to ensure spec compliance and security.

---

Automated review by Claude (Anthropic) · custom prompt

[github-actions]

Lines of code report

Total lines added: 26
Total lines removed: 0
Total lines changed: 26

Detailed view

+---------------------------------------------------+-------+------+
| File                                              | Lines | Diff |
+---------------------------------------------------+-------+------+
| ethrex/crates/networking/p2p/discv5/peer_table.rs | 1206  | +26  |
+---------------------------------------------------+-------+------+

[greptile-apps]

Greptile Overview

Greptile Summary

This PR fixes a bug where ENR (Ethereum Node Record) updates for existing contacts were silently discarded. The new_contact_records() function now properly handles the Entry::Occupied case by comparing sequence numbers and updating the contact's node information when a higher seq is received.

• ENR Update Logic: Properly updates existing contacts when incoming ENR has higher seq number, updating node, record, and is_fork_id_valid fields
• Signature Validation: Added upfront verify_signature() check for all incoming ENRs (previously missing even for new contacts)
• PONG Handler Enhancement: Implemented ENR seq mismatch detection that triggers FINDNODE(distance=0) to fetch updated ENRs
• Code Quality: Extracted generate_req_id() helper to reduce duplication and improved code organization by hoisting shared validation logic
• Test Coverage: Added comprehensive test test_enr_update_request_on_pong() covering both matching and mismatching seq scenarios

Confidence Score: 5/5

• This PR is safe to merge with no blocking issues identified
• The implementation correctly addresses the stated bug with proper sequence number comparison logic, adds important security validation (signature verification), includes well-structured tests, and follows good engineering practices. The changes are focused and don't introduce side effects.
• No files require special attention

Important Files Changed

Filename	Overview
crates/networking/p2p/discv5/peer_table.rs	Updated new_contact_records() to handle ENR updates for existing contacts by checking seq numbers and adding signature verification
crates/networking/p2p/discv5/server.rs	Added ENR update request on PONG with mismatched seq, extracted generate_req_id() helper, and added comprehensive test coverage

Sequence Diagram

sequenceDiagram
    participant Local as Local Node
    participant Remote as Remote Node
    participant PT as Peer Table
    
    Note over Local,Remote: ENR Update Flow via PONG
    
    Remote->>Local: PONG (enr_seq=10)
    Local->>PT: get_contact(remote_id)
    PT-->>Local: Contact (cached_seq=5)
    
    alt enr_seq != cached_seq
        Note over Local: ENR seq mismatch detected
        Local->>Remote: FINDNODE(distance=0)
        Remote->>Local: NODES(enr with seq=10)
        Local->>PT: new_contact_records([enr])
        
        PT->>PT: verify_signature()
        
        alt Entry::Occupied && new_seq > cached_seq
            Note over PT: Update existing contact
            PT->>PT: Update node, record, is_fork_id_valid
        else Entry::Vacant
            Note over PT: Insert new contact
            PT->>PT: Create and insert new Contact
        end
    else enr_seq == cached_seq
        Note over Local: No update needed
    end

Loading

[ElFantasma]

Addressing agent reviews

Fixes applied

1. FINDNODE spam from != + > mismatch (Codex, Claude) — handle_pong used != while new_contact_records used >. If a peer's PONG had a lower seq than cached, we'd send FINDNODE every time but never update, causing infinite requests. Fixed by changing handle_pong to use > (committed on base branch discv5-server-enr-update-on-pong). Added test case for lower seq scenario.

2. Fork-id validation for stale ENRs (Codex, Claude) — is_fork_id_valid was computed (async, hits storage) before knowing whether the ENR would actually be used. Moved the validation inside each match arm so it only runs when the record will be inserted/updated.

3. record = None with seq=0 edge case (Claude) — If an existing contact had record = None (mapped to existing_seq = 0) and the incoming ENR also had seq = 0, the > check would fail and the record would never be stored. Fixed by matching on Option directly: None => true (always accept first ENR), Some(r) => node_record.seq > r.seq.

False positives dismissed

• Kimi docs: add milestones #1 ("Missing signature verification for occupied") — Wrong. verify_signature() is already called upfront at line 846, before the Entry match. It applies to all records regardless of whether the contact exists.

• Kimi docs: update milestones. #4 ("Inefficient re-fetching of contact") — Wrong. The code uses occupied_entry.get_mut() directly from the match arm. There's no re-fetch.

• Kimi build: add Github actions boilerplate #3 ("OsRng panic") — OsRng doesn't fail on any real OS. The suggested Lazy<Mutex<OsRng>> adds unnecessary complexity for a non-issue.

• Claude "Incomplete state preservation" — Fields like validation_timestamp, ping_req_id, knows_us etc. reflect the ongoing relationship with the peer, not the ENR content. They should be preserved as-is when only the ENR record updates. Resetting validation state on IP change would be over-engineering since a PONG was just received (proving reachability).

• Claude/Codex "Missing metrics/test gaps" — Nice-to-have, not blocking.