Greptile Summary

This PR reverts #6458 ("reintroduce proper Kademlia k-bucket routing table") due to a snapsync performance regression. The revert replaces the 256-bucket Kademlia structure in PeerTableServer with a flat IndexMap, removes local_node_id from the server's persistent state, and instead threads it through as an explicit argument to the distance-aware methods (new_contacts, new_contact_records, get_nodes_at_distances).

Confidence Score: 5/5

Safe to merge — clean revert with all call sites updated consistently and no logic errors introduced.

All findings are P2 style suggestions. The revert is mechanically consistent: spawn signatures, protocol call sites (discv4, discv5), test utilities, and Grafana dashboards are all updated in lock-step.

No files require special attention.

Sequence Diagram

sequenceDiagram
    participant DS as DiscoveryServer (v4/v5)
    participant PT as PeerTableServer
    participant IM as IndexMap (flat contacts)

    DS->>PT: new_contacts(nodes, local_node_id, protocol)
    PT->>IM: entry(node_id) — insert if Vacant

    DS->>PT: get_nodes_at_distances(local_node_id, distances)
    PT->>IM: iter all contacts
    PT-->>DS: Vec<NodeRecord> within requested log-distances

    DS->>PT: new_contact_records(records, local_node_id)
    PT->>IM: entry(node_id) — insert or update record

This is a comment left during a code review.
Path: crates/networking/p2p/peer_table.rs
Line: 1134-1135

Comment:
**Redundant wrapper around `utils::distance`**

`Self::distance` is a private static method that immediately delegates to `utils::distance`. This indirection adds no value — call sites using `Self::distance` (e.g. `get_closest_nodes`) could directly call the already-imported `distance` from utils, matching what `do_get_nodes_at_distances` does on line 977.

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (1): Last reviewed commit: "Revert "feat(l1): reintroduce proper Kad..." | Re-trigger Greptile}

🤖 Claude Code Review

Here is my review of PR #6505:

PR #6505 — revert(l1): revert "reintroduce proper Kademlia k-bucket routing table"

Overview

This is a non-clean revert of #6458 (Kademlia k-bucket routing table) due to a snapsync performance regression. Rather than fully restoring the original pre-#6458 flat IndexMap implementation, it introduces several behavioral and API changes: local_node_id is removed from PeerTableServer state and passed per-call instead, TARGET_CONTACTS is introduced, and the do_get_closest_nodes algorithm is rewritten. These differences from the original pre-#6458 code warrant careful review.

Critical Bugs

1. do_get_closest_nodes uses log-distance instead of raw XOR distance

peer_table.rs, around line 896:

let dist = Self::distance(&node_id, contact_id);

Self::distance delegates to utils::distance, which returns floor(log2(XOR)) as a usize. This is a coarse bucket index, not the raw XOR distance. Multiple nodes can share the same usize distance (any two nodes in the same Kademlia bucket), making the "16 closest nodes" selection meaningless for most inputs. The pre-#6458 code correctly used xor_distance returning H256, which gives per-node precision. This directly affects the correctness of discv4 NEIGHBORS responses.

2. do_get_closest_nodes replacement logic is incorrect

The loop that replaces farther nodes in the candidate list is broken:

} else {
    for (i, (_, d)) in &mut nodes.iter().enumerate() {
        if dist < *d {
            nodes[i] = (contact.node.clone(), dist);
            break;
        }
    }
}

This replaces the first node whose distance exceeds dist, not the globally farthest node. The pre-#6458 code (and the #6458 code before this revert) correctly used max_by_key to find the farthest entry before replacing it. The current logic will fill the result list with sub-optimal candidates whenever the iteration order doesn't place the true farthest node first.

Functional Regressions

3. TargetReached semantics change breaks discovery termination

peer_table.rs, line 623:

self.contacts.len() >= TARGET_CONTACTS && self.peers.len() >= self.target_peers

The pre-#6458 behavior was simply self.peers.len() >= self.target_peers. The new condition requires both 100,000 contacts and the peer connection target. In snapsync mode (the exact scenario that motivated this revert), the node connects to a small number of peers and likely never accumulates 100k contacts. This means TargetReached will perpetually return false, causing continuous, aggressive discovery activity. This is likely to make the snapsync performance regression worse, not better.

4. Unbounded contacts map

TARGET_CONTACTS = 100_000 is only checked in TargetReached — it is never enforced as a capacity limit during insertion. do_new_contacts and do_new_contact_records unconditionally insert via Entry::Vacant. The old k-bucket table was bounded to ~4,096 main contacts + ~2,560 replacements. The new flat map can grow without limit, which may itself cause the memory/performance issues that motivated reverting #6458.

API Design Concern

5. local_node_id passed per-call rather than stored

The pre-#6458 PeerTableServer also stored local_node_id in its state. This PR removes it and passes it into every call site: new_contacts, new_contact_records, get_nodes_at_distances. The local node ID is static for the server's lifetime — passing it repeatedly increases call-site complexity and the chance of passing the wrong node ID. The simplification in PeerTableServer::spawn and ::new is not worth the added burden at every call site.

Missing Tests

6. Removed tests, no replacements

The PR removes 8+ unit tests covering KBucket::insert, KBucket::remove_and_promote, bucket_index, and replacement eviction. The replacement do_get_closest_nodes algorithm has no tests, despite having two correctness bugs (Points 1 and 2 above). Given the non-trivial rewrite here, at minimum a test verifying that do_get_closest_nodes returns the N XOR-closest contacts should be added.

Minor Points

7. Stale Prometheus metric names

metrics/p2p.rs, lines 39–50: The histogram names (ethrex_kademlia_insert_contact_duration_seconds, ethrex_kademlia_iter_contacts_duration_seconds) are retained even though the Kademlia implementation was removed. The descriptions were updated but the names were not. If these metrics are still emitted (metrics feature enabled), they will mislead operators. Either remove the metrics recording entirely or rename the metrics to reflect the simpler structure.

8. Trivial Self::distance wrapper

peer_table.rs, lines 1131–1133:

fn distance(node_id_1: &H256, node_id_2: &H256) -> usize {
    distance(node_id_1, node_id_2)
}

This wrapper adds no value and shadows the module-level distance import. Call distance() directly from the module import.

Summary

The two correctness bugs in do_get_closest_nodes (wrong distance type, wrong replacement algorithm) and the TargetReached regression (requiring 100k contacts) are the most important issues to resolve before merging. The unbounded contact map should also be addressed to avoid reintroducing the memory pressure that may have caused the original regression.

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

🤖 Kimi Code Review

Critical Issues

1. Bug in do_get_closest_nodes: Incorrect distance metric and replacement logic (crates/networking/p2p/peer_table.rs, lines 936-960)

The function uses Self::distance() (log2 distance / bucket index) instead of the full XOR distance (H256). This breaks the "closest nodes" calculation because nodes in the same bucket have identical log2 distances but different actual XOR distances.

Additionally, the replacement logic is buggy:

for (i, (_, d)) in &mut nodes.iter().enumerate() {
    if dist < *d {
        nodes[i] = (contact.node.clone(), dist);
        break;  // Wrong: doesn't find the farthest node
    }
}

This replaces the first node with greater distance, not the farthest node in the list.

Fix: Use full XOR distance (H256) and find the maximum distance node for replacement:

let dist = *node_id ^ *contact_id; // XOR as H256
if nodes.len() < MAX_NODES_IN_NEIGHBORS_PACKET {
    nodes.push((contact.node.clone(), dist));
} else if let Some((max_idx, (_, max_dist))) = nodes.iter().enumerate().max_by_key(|(_, (_, d))| *d) {
    if dist < *max_dist {
        nodes[max_idx] = (contact.node.clone(), dist);
    }
}

2. target_reached logic prevents discovery completion (crates/networking/p2p/peer_table.rs, line 606)

Changed from:

self.peers.len() >= self.target_peers

To:

self.contacts.len() >= TARGET_CONTACTS && self.peers.len() >= self.target_peers

This requires 100,000 contacts (hardcoded TARGET_CONTACTS) before considering the target reached. In small networks or during bootstrap, this will stall discovery indefinitely.

Fix: Revert to checking only peer count, or use a much lower threshold (e.g., self.contacts.len() >= self.target_peers).

3. Unbounded memory growth in contacts (crates/networking/p2p/peer_table.rs)

The IndexMap has no size limit. While TARGET_CONTACTS is defined as 100,000, it's only checked in target_reached, not enforced. A malicious actor can flood the node with fake Node IDs, causing OOM.

Fix: Enforce the limit in do_new_contacts and do_new_contact_records:

if self.contacts.len() >= TARGET_CONTACTS {
    // Evict oldest or random entry, or skip insertion
    return;
}

Minor Issues

4. Inefficient do_get_closest_nodes complexity (crates/networking/p2p/peer_table.rs, lines 936-960)

Scanning all contacts (potentially 100k) for every FindNode request is O(N). The previous Kademlia implementation was O(K) per bucket. Consider maintaining a sorted structure or accepting the trade-off with a comment.

5. Missing tests (crates/networking/p2p/peer_table.rs)

The Kademlia-specific tests were removed (lines 1321-1514), but no tests were added for the new flat-map implementation. Add tests for:

• do_get_closest_nodes returning correct 16 closest nodes
• do_new_contacts deduplication
• target_reached behavior

6. Unused distance wrapper (`crates

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

Lines of code report

Total lines added: 7
Total lines removed: 258
Total lines changed: 265

+-----------------------------------------------+-------+------+
| File                                          | Lines | Diff |
+-----------------------------------------------+-------+------+
| ethrex/cmd/ethrex/initializers.rs             | 636   | -1   |
+-----------------------------------------------+-------+------+
| ethrex/cmd/ethrex/l2/initializers.rs          | 363   | -4   |
+-----------------------------------------------+-------+------+
| ethrex/crates/networking/p2p/discv4/server.rs | 700   | +7   |
+-----------------------------------------------+-------+------+
| ethrex/crates/networking/p2p/discv5/server.rs | 1508  | -10  |
+-----------------------------------------------+-------+------+
| ethrex/crates/networking/p2p/peer_table.rs    | 986   | -243 |
+-----------------------------------------------+-------+------+

🤖 Codex Code Review

1. High: get_closest_nodes no longer guarantees the returned set is actually the closest 16 nodes. In peer_table.rs:940, once the buffer is full, the new code replaces the first entry with a larger distance (peer_table.rs:950-peer_table.rs:955) instead of the current farthest entry. Counterexample: if the buffer holds distances [80, 100, 70] and a node at distance 75 arrives, this code replaces 80 and leaves 100, so the result is wrong. That can degrade discv4 NEIGHBORS responses and discovery convergence. Suggest restoring the previous “replace farthest” logic or using a bounded max-heap / select_nth_unstable_by_key.

2. High: the PR removes the bounded routing-table structure but never replaces it with an enforced capacity. New contacts are inserted unconditionally in peer_table.rs:988-peer_table.rs:1023 and peer_table.rs:1026-peer_table.rs:1068, while prune() only drops contacts already marked disposable (peer_table.rs:845-peer_table.rs:855). TARGET_CONTACTS is only observed in target_reached() (peer_table.rs:604-peer_table.rs:609); it does not cap the table. A hostile peer can keep feeding unique valid nodes/ENRs and force unbounded memory growth plus O(n) scans across discovery paths. This is a concrete DoS regression; the table still needs a hard eviction policy.

No consensus/EVM/RLP-sensitive code was touched here; the review findings are confined to discovery/peer-table behavior.

I couldn’t run the Rust tests in this sandbox because cargo attempted to use rustup, which failed on a read-only filesystem.

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