Lines of code report

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

+-----------------------------------------------+-------+------+
| File                                          | Lines | Diff |
+-----------------------------------------------+-------+------+
| ethrex/cmd/ethrex/initializers.rs             | 667   | +1   |
+-----------------------------------------------+-------+------+
| ethrex/cmd/ethrex/l2/initializers.rs          | 367   | +4   |
+-----------------------------------------------+-------+------+
| ethrex/crates/networking/p2p/discv4/server.rs | 693   | -7   |
+-----------------------------------------------+-------+------+
| ethrex/crates/networking/p2p/discv5/server.rs | 1518  | +10  |
+-----------------------------------------------+-------+------+
| ethrex/crates/networking/p2p/peer_table.rs    | 1229  | +243 |
+-----------------------------------------------+-------+------+

🤖 Codex Code Review

Findings

1. High: replacement-cache entries are treated as “present” for dedup, but almost every read/update path ignores them, so saturated buckets will silently drop discovery state. contact_exists() checks both main and replacement entries (peer_table.rs:927), while get_contact(), get_contact_mut(), and iter_contacts() only traverse bucket.contacts (peer_table.rs:915, peer_table.rs:921, peer_table.rs:945). On top of that, insert_contact() discards the KBucket::insert() return value and always reports success (peer_table.rs:935). That means a full bucket can accept a node only into replacements, yet callers still behave as if it is active: insert_if_new() returns true (peer_table.rs:830), discv4::handle_ping() sends a validation ping (discv4/server.rs:510), and later handle_pong() / get_contact() cannot find that node (discv4/server.rs:542). The same bug hits ENR/session updates: new_contact_records() branches on contact_exists(), then fails to update anything if the node is only in replacements (peer_table.rs:1151). This breaks discovery behavior once any bucket fills up.

2. Medium: do_new_contacts() can accumulate duplicate replacement entries for the same node. It only checks get_contact_mut() before inserting (peer_table.rs:1129), so if a node is already in replacements but not in contacts, the code inserts it again. insert_replacement() then blindly push()es and evicts FIFO (peer_table.rs:105). Repeated Neighbors/NODES responses for the same peer will crowd out other replacement candidates and make promotion behavior nondeterministic.

I did not run cargo test; in this environment cargo/rustup tried to write under read-only ~/.cargo / ~/.rustup, so verification was blocked.

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

🤖 Claude Code Review

Here is the review of PR #6458:

Review: feat(l1): reintroduce proper Kademlia k-bucket routing table

Overall the approach is solid — the structural change from a flat IndexMap to 256 k-buckets is correct and the cleanup of redundant local_node_id parameters is a clear improvement. There are a few correctness issues worth addressing before merging.

Bug 1 — do_new_contacts may produce duplicate replacement-list entries

File: crates/networking/p2p/peer_table.rs

do_new_contacts uses get_contact_mut to detect whether a node already exists, but get_contact_mut only searches the main contact list, not the replacement list:

if let Some(contact) = self.get_contact_mut(&node_id) {
    contact.add_protocol(protocol);
} else {
    let contact = Contact::new(node, protocol);
    if self.insert_contact(node_id, contact) { ... }
}

When a node is already in the replacement list, get_contact_mut returns None, so the else branch runs. insert_contact → KBucket::insert finds the main list still full (that's why the node was in replacements), so it calls insert_replacement — adding a second copy of the same node to the replacement list.

do_new_contact_records correctly uses contact_exists (which checks both lists). The fix for do_new_contacts is to replace the get_contact_mut guard with contact_exists and only call add_protocol on the main-list node (or accept the skip for replacement-list nodes, as do_new_contact_records does):

if self.contact_exists(&node_id) {
    if let Some(contact) = self.get_contact_mut(&node_id) {
        contact.add_protocol(protocol);
    }
} else { ... }

Bug 2 — insert_contact discards KBucket::insert's return value, over-counting metrics

File: crates/networking/p2p/peer_table.rs

KBucket::insert returns true only when the contact enters the main list, and false when it overflows to the replacement list. But insert_contact ignores that value:

fn insert_contact(&mut self, node_id: H256, contact: Contact) -> bool {
    let Some(idx) = self.bucket_for(&node_id) else { return false; };
    self.buckets[idx].insert(node_id, contact);  // return value discarded
    true  // always true if not local node
}

Every call site does if self.insert_contact(...) { METRICS.record_new_discovery().await; }, so the metric fires for contacts that only reached the replacement list and are not yet discoverable. If accurate metrics matter, propagate the inner return:

fn insert_contact(&mut self, node_id: H256, contact: Contact) -> bool {
    let Some(idx) = self.bucket_for(&node_id) else { return false; };
    self.buckets[idx].insert(node_id, contact)
}

The docstring currently says "Returns true if inserted (into main list or replacement list)" — which is technically consistent with the current implementation, but conflicts with the caller's intent of only counting genuine discoveries.

Correctness — ENR updates silently dropped for replacement-list nodes

File: crates/networking/p2p/peer_table.rs, do_new_contact_records

contact_exists returns true for replacement-list nodes, so the "update existing contact" branch is taken. But get_contact/get_contact_mut return None for those nodes, causing should_update to default to false and the mutable update block to be skipped entirely. A replacement-list contact whose ENR is refreshed before it gets promoted will therefore be promoted with stale ENR data.

A comment explaining this intentional skip would remove ambiguity. If updating replacement-list ENRs is desired, a separate get_replacement_mut accessor would be needed.

Correctness — remove_and_promote uses LIFO, not FIFO

File: crates/networking/p2p/peer_table.rs, KBucket::remove_and_promote

fn remove_and_promote(&mut self, node_id: &H256) -> Option<H256> {
    ...
    if let Some((replacement_id, replacement)) = self.replacements.pop() {  // LIFO

insert_replacement evicts from the front (oldest, remove(0)) but remove_and_promote promotes from the back (newest, pop()). Standard Kademlia promotes the longest-waiting candidate. The inconsistency should be intentional or corrected — self.replacements.remove(0) here if FIFO promotion is desired.

Minor — do_get_nodes_at_distances accepts a redundant local_node_id parameter

File: crates/networking/p2p/peer_table.rs

fn do_get_nodes_at_distances(&self, local_node_id: H256, distances: &[u32]) -> Vec<NodeRecord> {
    self.iter_contacts()
        .filter_map(|(contact_id, contact)| {
            let dist = distance(&local_node_id, contact_id) as u32;

Now that PeerTableServer stores self.local_node_id, the parameter is redundant. The caller passes msg.local_node_id from the message, which should always equal self.local_node_id. This should use self.local_node_id directly to remove the surface for divergence.

Minor — KBucket fields are pub

KBucket.contacts and KBucket.replacements are public, allowing callers to bypass all bucket invariants (capacity enforcement, replacement promotion). Unless external test introspection is required, these should be pub(crate) at most.

Positive notes

• The fix for do_get_closest_nodes (finding the farthest node before replacing, instead of break-on-first) is a real correctness improvement.
• The borrow-checker split in do_new_contact_records (immutable should_update check before mutable update) is clean.
• Removal of the flat contacts: IndexMap and the old distance() wrapper is a good cleanup.
• Keeping the logarithmic distance() function in do_get_nodes_at_distances for discv5 (where distances are specified as log₂ buckets) is correct and intentional.

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

🤖 Codex Code Review

1. Medium: replacement entries are treated as “already present” for deduplication, but most of the table logic cannot read or update them. contact_exists() checks both contacts and replacements at peer_table.rs:927, while get_contact() and get_contact_mut() only search contacts at peer_table.rs:916 and peer_table.rs:922. That breaks do_new_contact_records(): the contact_exists() branch at peer_table.rs:1151 can be taken for a replacement-only node, but the subsequent reads/writes become no-ops, so fresher ENRs, protocol flags, and fork-id validation never land. The same asymmetry can also make callers observe “known node” while get_contact() still returns None. I would either make the accessors search replacements too, or keep replacements out of contact_exists() and treat them as cache-only until promotion.

2. Medium: do_new_contacts() can insert duplicate node IDs into the replacement list. When a node already exists only in replacements, get_contact_mut() misses it at peer_table.rs:1129, so the code falls through to insert_contact() at peer_table.rs:1134. KBucket::insert_replacement() does not deduplicate at peer_table.rs:106, so repeated Neighbors/NODES messages can fill a bucket’s replacement cache with the same peer and evict distinct candidates. That is a correctness and discovery-quality regression; the simplest fix is to check contact_exists() before inserting, or dedup inside insert_replacement().

No EVM, gas-accounting, trie, or consensus-rule logic is touched here; the review surface is discovery/routing behavior only.

I could not run the affected tests in this sandbox because Cargo needs to resolve the git dependency libssz, and network access is blocked.

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

Greptile Summary

Replaces the flat IndexMap<H256, Contact> with a proper 256-bucket Kademlia routing table (KBucket with 16 main slots and 10 replacements each), uses raw XOR distance for get_closest_nodes, and removes the redundant local_node_id parameter from new_contacts/new_contact_records by storing it in PeerTableServer.

• P1 — duplicate replacement entries: do_new_contacts uses get_contact_mut to check existence, but that helper only searches the main bucket list. A node already in the replacement list will pass the None branch and be inserted a second time into replacements on every subsequent re-announcement, silently corrupting the replacement queue.
• P2 — metric overcounting: insert_contact ignores KBucket::insert's bool return (which signals main-list vs replacement-list placement) and always returns true, so METRICS.record_new_discovery() fires even for replacement-only insertions.

Confidence Score: 4/5

Safe to merge after fixing the replacement-list duplicate insertion bug in do_new_contacts.

One P1 defect (duplicate replacement entries in do_new_contacts) needs to be resolved before merge; remaining findings are P2 quality issues that do not block correctness of the primary discovery path.

crates/networking/p2p/peer_table.rs — specifically do_new_contacts (line ~1129) and insert_contact (line ~937)

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[do_new_contacts / do_new_contact_records] --> B{discarded or local node?}
    B -- yes --> Z[skip]
    B -- no --> C{get_contact_mut found?\nmain list only}
    C -- yes --> D[add_protocol on existing contact]
    C -- no --> E{contact_exists?\nchecks replacements too}
    E -- yes, do_new_contact_records --> F[update replacement entry]
    E -- no / do_new_contacts skips this check --> G[insert_contact]
    G --> H[bucket_index XOR distance]
    H --> I{main list full?}
    I -- no --> J[push to contacts main list\nreturn true]
    I -- yes --> K[insert_replacement\nreturn false — IGNORED by insert_contact]
    K --> L[⚠ duplicate if node already in replacements]
    J --> M[record_new_discovery metric]
    K --> M

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

Comment:
**Replacement-list contacts are silently duplicated**

`get_contact_mut` only searches `bucket.contacts` (the main list), not `bucket.replacements`. When a node is already sitting in the replacement list and `do_new_contacts` is called for it again, the `if let Some(contact)` branch is skipped, a brand-new `Contact` is constructed, and `insert_contact` → `KBucket::insert_replacement` pushes a **second copy** of the same node ID into `replacements`. The protocol field on the original replacement entry is also never updated.

`do_new_contact_records` avoids this by checking `self.contact_exists(&node_id)` (which covers both lists). `do_new_contacts` should do the same:

```rust
async fn do_new_contacts(&mut self, nodes: Vec<Node>, protocol: DiscoveryProtocol) {
    for node in nodes {
        let node_id = node.node_id();
        if self.discarded_contacts.contains(&node_id) || node_id == self.local_node_id {
            continue;
        }
        if let Some(contact) = self.get_contact_mut(&node_id) {
            contact.add_protocol(protocol);
        } else if self.contact_exists(&node_id) {
            // Node is in the replacement list; update its protocol entry there.
            if let Some(idx) = self.bucket_for(&node_id) {
                if let Some((_, c)) = self.buckets[idx]
                    .replacements
                    .iter_mut()
                    .find(|(id, _)| id == &node_id)
                {
                    c.add_protocol(protocol);
                }
            }
        } else {
            let contact = Contact::new(node, protocol);
            if self.insert_contact(node_id, contact) {
                METRICS.record_new_discovery().await;
            }
        }
    }
}
```

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

---

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

Comment:
**`insert_contact` discards `KBucket::insert`'s return value, over-counting discoveries**

`KBucket::insert` returns `false` when the bucket is full and the contact is placed in the replacement list instead. `insert_contact` ignores that value and unconditionally returns `true` for any non-local node — so callers (e.g. `do_new_contacts`, `handle_insert_if_new`) record `METRICS.record_new_discovery()` even for contacts that only land in the replacement list.

```rust
fn insert_contact(&mut self, node_id: H256, contact: Contact) -> bool {
    let Some(idx) = self.bucket_for(&node_id) else {
        return false;
    };
    self.buckets[idx].insert(node_id, contact)   // propagate the bucket's return value
}
```

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

---

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

Comment:
**Redundant `local_node_id` parameter in `get_nodes_at_distances`**

`PeerTableServer` now owns `self.local_node_id`, so passing `local_node_id` from outside is redundant — callers must pass the same value that the routing table was constructed with, otherwise distance calculations diverge from bucket placement. `do_get_nodes_at_distances` could simply use `self.local_node_id` and the parameter could be dropped from both the protocol definition and the public API.

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

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

🤖 Claude Code Review

Now I have everything I need for a thorough review. Let me compile my findings.

Review: feat(l1): reintroduce proper Kademlia k-bucket routing table (#6458)

The overall structure is sound — moving from a flat IndexMap to proper k-buckets is the right direction, and the GenServer ownership pattern is preserved cleanly. The API cleanup (moving local_node_id into the struct) is a net improvement. However, there are a few correctness issues in the bucket mechanics that need attention before merge.

Bugs

1. do_new_contacts can create duplicate replacement-list entries

peer_table.rs:1129

if let Some(contact) = self.get_contact_mut(&node_id) {
    contact.add_protocol(protocol);
} else {
    // ...
    self.insert_contact(node_id, contact);  // ← runs even when node is in replacements
}

get_contact_mut (line 922) only searches bucket.contacts — it skips bucket.replacements. If a node was previously added to the replacement list (because the main bucket was full), get_contact_mut returns None, and the code inserts the node into replacements again, creating duplicates. Over time a replacement slot can contain the same node ID multiple times.

The correct guard is contact_exists() (which checks both lists), matching what do_new_contact_records already does (line 1151).

2. Replacement promotion is LIFO instead of FIFO — diverges from Kademlia spec

peer_table.rs:120

fn remove_and_promote(&mut self, node_id: &H256) -> Option<H256> {
    // ...
    if let Some((replacement_id, replacement)) = self.replacements.pop() {  // ← newest

insert_replacement (line 108) evicts the oldest entry (remove(0)) when the list is full, treating it as a FIFO queue. But remove_and_promote promotes the newest entry (pop()), making promotion LIFO. These two policies are inconsistent, and the LIFO promotion deviates from the Kademlia specification (§2.4) which requires the oldest queued candidate to be promoted first.

Fix: change self.replacements.pop() to self.replacements.remove(0) (or switch the storage to VecDeque for efficiency).

3. Silent ENR record loss for nodes that are only in the replacement list

peer_table.rs:1151–1179

When contact_exists returns true for a replacement-list node, the code then calls get_contact(&node_id) (line 1155), which returns None because it only searches main contacts. This causes should_update = false (via .unwrap_or(false) on line 1160). Then get_contact_mut on line 1166 also returns None, so the update block is silently skipped entirely. The incoming ENR record — including its fork-ID — is dropped with no diagnostic.

This is a consequence of the inconsistency between contact_exists (checks both lists) and get_contact/get_contact_mut (checks main list only). Either the replacement list needs its own update path, or the API contract must be clarified.

4. insert_contact discards KBucket::insert's return value, skewing metrics

peer_table.rs:937–943

fn insert_contact(&mut self, node_id: H256, contact: Contact) -> bool {
    let Some(idx) = self.bucket_for(&node_id) else { return false; };
    self.buckets[idx].insert(node_id, contact);  // return value ignored
    true   // ← true even when contact only reached the replacement list
}

KBucket::insert returns true for main-list insertions and false for replacement-list insertions (line 93–103), but insert_contact always returns true (as long as the bucket exists). Consequences:

• METRICS.record_new_discovery() fires for replacement-list additions — those are not yet "discovered" from a connectivity standpoint.
• InsertIfNew (used by discovery) returns true to the caller even when the node only reached the replacement list, which may cause incorrect decision-making upstream.

Minor Issues

5. do_get_nodes_at_distances still accepts local_node_id as a parameter

peer_table.rs:1109

fn do_get_nodes_at_distances(&self, local_node_id: H256, distances: &[u32]) -> Vec<NodeRecord> {

The whole point of this PR is to store local_node_id in self. This function was partially migrated (switched to iter_contacts) but still takes the local ID as a parameter instead of using self.local_node_id. Left as-is it creates an inconsistency that forces callers to track and pass the value.

6. Redundant variable in remove_and_promote

peer_table.rs:121

if let Some((replacement_id, replacement)) = self.replacements.pop() {
    let promoted_id = replacement_id;           // ← dead rename
    self.contacts.push((replacement_id, replacement));
    Some(promoted_id)

promoted_id is just a rename of replacement_id; the binding serves no purpose. Should be Some(replacement_id) directly.

7. pub fields on KBucket

peer_table.rs:66–67

pub struct KBucket {
    pub contacts: Vec<(H256, Contact)>,
    pub replacements: Vec<(H256, Contact)>,
}

Exposing both fields as fully pub bypasses encapsulation and allows external code to mutate the invariants (e.g., inserting duplicates, reordering). If external crates only need read access for diagnostics/tests, pub(crate) would be more appropriate. If true external visibility is needed, consider accessor methods.

What Looks Good

• The bucket_index / xor_distance math is correct: U256::bits() - 1 correctly gives floor(log2(XOR)), producing bucket indices 0–255 matching NUMBER_OF_BUCKETS = 256.
• The do_get_closest_nodes rewrite correctly identifies the farthest element (via max_by_key) before replacing, fixing the previous bug where only the first farther element was replaced.
• Removing TARGET_CONTACTS = 100_000 from TargetReached is the right call — the new table caps at 256 × 16 = 4 096 main contacts, making the old threshold permanently unreachable.
• The refactoring from IndexMap::Entry to get_contact_mut / insert_contact is clean and readable throughout the handler methods.
• The API signature cleanup (local_node_id removed from new_contacts/new_contact_records) is an improvement.

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