🤖 Kimi Code Review

Review of PR #6633

The changes are straightforward and generally look good. Two minor notes:

1. Defensive arithmetic (server.rs)

Lines 94, 100: The subtraction before - after is safe because retain() only removes elements, but consider using saturating_sub to make the intent explicit and prevent any future panics if the logic changes:

let removed_messages = before_messages.saturating_sub(self.pending_by_nonce.len());
let removed_challenges = before_challenges.saturating_sub(self.pending_challenges.len());

2. Missing implementation visibility

discv5_server_tests.rs: The new test test_handle_pong_same_ip_does_not_bump_enr_seq asserts that discv5_handle_pong contains a guard to skip ENR updates when the voted IP matches the current IP.

Verify that this guard logic exists in the implementation (not shown in diff). If the implementation is missing, the test will fail. The test comment references this guard explicitly, so ensure the production code matches this expectation.

Summary

• The tracing addition is appropriate for debugging stale entry cleanup without log spam (guarded by if total_removed > 0)
• Test coverage for the "same IP" edge case is valuable for preventing unnecessary ENR sequence bumps
• No security or consensus issues identified

Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt

Lines of code report

Total lines added: 15
Total lines removed: 0
Total lines changed: 15

+-----------------------------------------------+-------+------+
| File                                          | Lines | Diff |
+-----------------------------------------------+-------+------+
| ethrex/crates/networking/p2p/discv5/server.rs | 255   | +15  |
+-----------------------------------------------+-------+------+

Greptile Summary

This PR adds a focused integration test for the discv5 IP-voting guard and restores a trace! log that was dropped during the refactor to Discv5State.

• Test (discv5_server_tests.rs): test_handle_pong_same_ip_does_not_bump_enr_seq drives discv5_handle_pong three times (reaching IP_VOTE_THRESHOLD = 3) with the local node's own IP as the reported address, then asserts local_node_record.seq is unchanged — directly locking in the winning_ip != self.local_node.ip guard in discv5_handlers.rs:379 that was never previously exercised by tests.
• Logging (server.rs): Restores the cleanup trace by computing before/after lengths around both retain calls and emitting a single structured trace! only when entries were actually removed, avoiding per-tick noise.

Confidence Score: 5/5

The changes are safe to merge: one restores an observability log that was inadvertently dropped in a refactor, and the other adds a targeted integration test closing a known test gap.

Both changes are narrow and well-scoped. The trace restoration uses safe arithmetic (retain can only shrink the maps) and is guarded against noise. The new test correctly drives discv5_handle_pong end-to-end and the seq assertion is the meaningful check that would catch a removal of the guard — it cannot produce a false positive.

No files require special attention.

Sequence Diagram

sequenceDiagram
    participant Test
    participant DiscoveryServer
    participant Discv5State
    participant PeerTable

    loop "i = 1, 2, 3  (i = 3 triggers threshold)"
        Test->>DiscoveryServer: "discv5_handle_pong(PongMessage{ip=original_ip}, sender_id=i)"
        DiscoveryServer->>PeerTable: record_pong_received(sender_id, req_id)
        PeerTable-->>DiscoveryServer: Ok (no-op, unknown node)
        DiscoveryServer->>PeerTable: get_contact(sender_id)
        PeerTable-->>DiscoveryServer: None
        DiscoveryServer->>Discv5State: record_ip_vote(original_ip, sender_id)
        alt "i < 3"
            Discv5State-->>DiscoveryServer: None (below threshold)
        else "i = 3"
            Discv5State-->>DiscoveryServer: Some(original_ip) (round ended)
            DiscoveryServer->>DiscoveryServer: "guard: winning_ip != local_node.ip? FALSE"
            Note over DiscoveryServer: update_local_ip NOT called, ENR seq unchanged
        end
        DiscoveryServer-->>Test: Ok(())
    end
    Test->>Test: "assert local_node.ip == original_ip"
    Test->>Test: "assert local_node_record.seq == original_seq"

_{Reviews (1): Last reviewed commit: "test(l1): cover discv5 IP-voting guard a..." | Re-trigger Greptile}

🤖 Codex Code Review

• No findings. The trace added in server.rs is side-effect free, and the new regression test in discv5_server_tests.rs correctly covers the winning_ip != self.local_node.ip guard in discv5_handlers.rs, which is the right behavior to avoid unnecessary ENR re-signing / sequence bumps.

• Non-blocking: discv5_server_tests.rs implicitly depends on test_server() using a public local_node.ip. If that fixture ever changes to loopback/private, record_ip_vote() will ignore the PONGs and this test will pass without exercising the intended guard. An explicit precondition/assertion there would make the regression coverage less brittle.

• Testing: I could not run cargo test in this sandbox because Cargo needs to create the libssz git dependency state under read-only ~/.cargo.

Automated review by OpenAI Codex · gpt-5.4 · custom prompt

🤖 Claude Code Review

Now I have all the context I need. Let me write the review.

PR Review: test(l1): cover discv5 IP-voting guard and restore stale-entry cleanup trace

Summary: Two small, targeted additions — a restored trace log and a new test. The logic is correct. Two moderate concerns and a couple of minor ones.

crates/networking/p2p/discv5/server.rs

Subtraction safety (lines 96, 103)

let removed_messages = before_messages - self.pending_by_nonce.len();
let removed_challenges = before_challenges - self.pending_challenges.len();

Plain subtraction is safe here because HashMap::retain can only decrease the map's length, so before >= after is guaranteed. The existing reviewer suggestion to use saturating_sub would add noise without adding safety. The code as written is fine.

Trace log guard (lines 106–114) — The if total_removed > 0 guard is correct and avoids per-tick spam. Structured field protocol = "discv5" is consistent with the rest of the codebase. Good.

test/tests/p2p/discovery/discv5_server_tests.rs

Test validity

I traced the full execution path to confirm the test actually exercises the guard:

• record_pong_received is a fire-and-forget actor send; with req_id = b"" it returns Ok(()) silently (the actor simply finds no matching ping_id and does nothing), so no early return occurs.
• get_contact(H256::from_low_u64_be(i)) returns Ok(None) for these unregistered IDs, skipping the ENR seq-mismatch FINDNODE.
• 18.138.108.67 is a public IP — is_private_ip returns false — so all three votes land.
• On the third call, total_votes == 3 >= IP_VOTE_THRESHOLD, finalize_ip_vote_round returns Some(18.138.108.67), and the guard winning_ip != self.local_node.ip is false, so update_local_ip is correctly skipped.

The test is valid and exercises exactly what it claims.

Missing first_ip_vote_round_completed assertion (moderate)

The existing test_ip_voting_no_update_if_same_ip ends with:

assert!(discv5(&mut server).first_ip_vote_round_completed);

The new test omits this check. Without it, if the voting round silently fails to complete (e.g. record_ip_vote returns None all three times), both local_node.ip == original_ip and local_node_record.seq == original_seq trivially pass — there is no mutation to detect. Adding this assertion makes the test falsifiable:

assert!(discv5(&mut server).first_ip_vote_round_completed, "voting round should have completed");
assert_eq!(server.local_node.ip, original_ip);
assert_eq!(server.local_node_record.seq, original_seq);

Threshold coupling (minor)

The loop for i in 1..=3u64 hardcodes knowledge that IP_VOTE_THRESHOLD == 3. If the threshold changes, the round won't complete and both assertions trivially pass. This is the same pattern used in test_ip_voting_no_update_if_same_ip, so it is at least consistent, but consider using IP_VOTE_THRESHOLD directly if it's pub.

Empty req_id (minor)

req_id: Bytes::from_static(b"") is intentionally empty (no pending PING in the test setup), and it works because the actor silently ignores unmatched req_ids rather than returning an error. A brief inline comment would help the next reader:

req_id: Bytes::from_static(b""), // no matching PING needed; peer_table silently ignores it

Overall

The implementation is correct. The trace restoration is clean and appropriately gated. The test correctly drives the production handler path (not just the state method), which was the stated gap. The first_ip_vote_round_completed assertion is the one change worth making before merge; the rest are polish.

Automated review by Claude (Anthropic) · sonnet · custom prompt

Addressed feedback from bot reviews in 87de359 and 4cddfb0:

• From Claude (moderate) — Added assert!(first_ip_vote_round_completed) to test_handle_pong_same_ip_does_not_bump_enr_seq to make the test falsifiable: without it, a regression that prevents finalization would make both local_node.ip and local_node_record.seq assertions trivially pass without ever exercising the guard.
• From Claude (minor) — Added a comment explaining why an empty req_id is fine (record_pong_received silently ignores unmatched req_ids).

Not addressed:

• From Kimi — saturating_sub for the cleanup arithmetic. retain() can only shrink the maps, so the subtraction cannot underflow; defensive arithmetic for an impossible scenario isn't worth adding.
• From Claude (minor) — Threshold coupling (for i in 1..=3u64). The new round-completion assertion already catches this: if IP_VOTE_THRESHOLD changes and the loop runs short, the round won't complete and the assertion fails. Not worth exposing the constant as pub just for this.

⚠️ Known Issues — intentionally skipped tests

Source: docs/known_issues.md

Known Issues

Tests intentionally excluded from CI. Source of truth for the Known
Issues section the L1 workflow appends to each ef-tests job summary
and posts as a sticky PR comment.

EF Tests — Stateless coverage narrowed to EIP-8025 optional-proofs

make -C tooling/ef_tests/blockchain test calls test-stateless-zkevm
instead of test-stateless. The zkevm@v0.3.3 fixtures are filled against
bal@v5.6.1, out of sync with current bal spec; the broad target trips ~549
fixtures. Re-broaden once the zkevm bundle is regenerated.