Repository navigation
feat(l1): kademlia k-bucket routing table v2 - #6511
Conversation
…talls Fixes snapsync failures where peer count stays constant and sync eventually fails with "Failed to receive block headers" after hours of operation. Root cause: After PR #6458 introduced Kademlia k-buckets, peers that became unresponsive during sync weren't marked as disposable, so they remained in the routing table indefinitely. New peers went into replacement lists but were never promoted because dead peers weren't pruned. Changes: - Enhanced prune() to remove disposable contacts from both main and replacement lists, with automatic promotion of replacements - Mark peers as disposable when they timeout during RLPx operations (block headers, block bodies, sync head requests) - Added periodic pruning in the snap_sync main loop to ensure dead peers are regularly removed and replaced Evidence from CI artifacts showed peer count stuck at 6 throughout 3h35m sync before failure. This fix enables peer rotation so healthy peers from replacement lists can take over when active peers become unresponsive.
The Kademlia k-bucket implementation only iterated over main bucket contacts, ignoring replacement entries. This caused peer starvation because dead contacts in the main list were never replaced by fresher peers from the replacement list. Fix iter_contacts() and do_get_contact_to_initiate() to also check replacement contacts, allowing the node to discover and connect to peers that were previously invisible to the peer selection logic.
KBucket::get_mut and get_contact only searched the main contact list, so any state mutation (set_disposable, ping tracking, find_node count, mark_knows_us) silently failed for contacts in the replacement list. Since iter_contacts and do_get_contact_to_initiate now return replacement contacts, this caused phantom contacts that were visible to selection but invisible to updates. Update get_contact to use get_any (main + replacements) and get_mut to search both lists, ensuring all contact state mutations work regardless of which list holds the contact.
…able Add a separate IndexMap<H256, Node> connection pool (capacity 50K) for RLPx connection initiation, decoupled from the k-bucket routing table (which is limited to 256 × 16 = 4,096 contacts by Kademlia design). All discovered contacts are inserted into both the k-buckets (for Kademlia protocol operations like FindNode/GetClosestNodes) and the connection pool (for peer connection initiation). This restores the large candidate pool that existed before the k-bucket migration while preserving correct Kademlia routing semantics. The connection pool is: - Populated on every contact discovery (discv4, discv5, insert_if_new) - Cleaned during prune() when contacts are marked disposable - Capped at 50K entries with oldest-first eviction - Used with random selection and k-bucket state filtering
Matches the candidate pool size used by Reth and Nethermind.
- Replace O(n) collect-then-choose in do_get_contact_to_initiate with O(k) random index probing on the IndexMap (rand % len, scan forward). The old approach scanned all 10K pool entries, cloned eligible ones into a Vec, then randomly picked — blocking the peer_table actor and starving snap sync's get_best_peer calls. - Replace collect-then-choose in do_get_contact_for_lookup with IteratorRandom::choose (single-pass reservoir sampling, zero alloc). - Remove discarded_contacts permanent blacklist entirely. Contacts pruned from k-buckets now remain in the connection pool so they can be retried — the RLPx handshake rejects truly incompatible peers. Previously, a single timeout permanently blacklisted a contact from both the pool and re-discovery.
Lines of code reportTotal lines added: Detailed view |
azteca1998
changed the title
azteca1998
changed the title
Resolve conflicts in p2p module by adapting HEAD's new discovery/ handler files to main's PeerTable API (k-bucket table stores local_node_id internally, removing extra node_id arguments from callers).
Replace the direct random-contact lookup with a geth-style iterative convergence approach: generate a random target, seed with closest known nodes from the connection pool, query alpha=3 closest not-yet-asked nodes, feed responses back, and iterate until convergence or timeout.
Change is_finished() to consider a lookup converged as soon as all entries have been queried, without waiting for in-flight responses. Late responses still get processed via handle_neighbors and feed into the connection pool for future lookups. This prevents a single unresponsive node from blocking the entire lookup for up to 20 seconds (the timeout), which was causing very slow peer acquisition compared to the old random-contact approach.
Instead of returning after clearing a finished lookup (which waits for the easing interval before starting the next one), immediately start a new lookup in the same tick. This matches geth's behavior of continuous lookup chaining during bootstrapping.
…ment list When a new peer pings us and the k-bucket at their distance is full, insert_if_new was returning false (node went to replacement list), which prevented the reciprocal ping. Without the reciprocal ping, the bond is never established from our side, so we reject their FindNode requests as UnknownContact. This cascading visibility loss explains the ~2x slower peer acquisition compared to main (which uses a flat map with no capacity limit). Now insert_if_new returns true for any genuinely new node regardless of whether it lands in the main list or replacement list. The replacement-list contact is reachable by get_contact (via get_any), so the full ping→pong→validate chain works correctly.
…t lookups, and adaptive alpha Pre-bond newly discovered nodes by pinging them in discv4_handle_neighbors so remote nodes accept our future FindNode queries. Track already-pinged nodes to avoid re-pinging bonded contacts. Run up to 3 concurrent iterative lookups (targeting different ID-space regions) when below 30 peers, reverting to 1 above threshold. Use aggressive alpha=16 (query all entries at once) during bootstrap instead of the normal alpha=3, since most queries fail due to unbonded nodes early on.
Pre-bonding was pinging every new node from every Neighbors response unconditionally, even after reaching 100 peers. This generated ~39M outgoing pings in 3.5 hours (~3100/sec). Now only pre-bonds when peer_count < BOOTSTRAP_THRESHOLD (30).
Lookups were chaining continuously — a finished lookup was immediately replaced in the same tick, keeping the timer locked at ACTIVE_LOOKUP_TICK (50ms) forever. This caused ~360 FindNode/sec at steady-state. Now, above BOOTSTRAP_THRESHOLD (30 peers), when the active lookup finishes the function returns with empty active_lookups, letting the timer fall back to get_lookup_interval() (~600ms at 100% peer completion).
Pre-bonding was pinging every node from Neighbors responses (~16 per response), but the lookup only queries a few of those. Move pre-bond pings to advance_v4_lookup so we only ping the exact nodes the lookup selects to query — dramatically reducing the UDP burst during bootstrap.
Remove concurrent lookups (MAX_CONCURRENT_LOOKUPS), adaptive alpha (BOOTSTRAP_ALPHA), and use a slower 200ms tick during bootstrap instead of 50ms. This reduces FindNode and Ping traffic by ~10-20x while still reaching 100 peers quickly via pre-bonding and back-to-back chaining.
…urve Remove back-to-back lookup chaining, BOOTSTRAP_THRESHOLD, ACTIVE_LOOKUP_TICK, and BOOTSTRAP_LOOKUP_TICK. The existing easing curve (100ms→600ms based on peer completion) already provides fast lookups at low peer counts. Pre-bonding is the only kademlia-specific addition and is now always active (idempotent via pinged_nodes set). This should bring traffic in line with main while still benefiting from pre-bonding's higher per-lookup success rate.
The lookup timer was calling advance_v4/v5_lookup on every tick even when a lookup was already active, causing redundant FindNode queries. Active lookups are already driven forward by incoming Neighbors/Nodes responses. Now the timer only starts new lookups; convergence is entirely response-driven. This eliminates the discv5 FindNode burst where fast-converging lookups generated 45K+ queries in 3 minutes.
Each iterative lookup generates ~16 FindNode messages vs 1 in the old single-shot approach. Scale the discovery lookup interval accordingly: 2s initial (vs 100ms) to 10s steady-state (vs 600ms). This produces similar per-second FindNode traffic as main while preserving the easing curve behavior. The RLPx initiator keeps its own faster interval since it only controls TCP connection attempts.
Two changes to balance discovery speed vs traffic: 1. Lookups are now entirely timer-driven: each tick advances by alpha=3 queries. Responses feed results but don't trigger new queries. This throttles discv5 lookups (which converged in <200ms response-driven) to ~6s per lookup, matching the controlled pace of discv4. 2. Pre-bonding moved from advance_v4_lookup to handle_neighbors. By pinging nodes when they're discovered (not when they're queried), bonds have ~1s+ to establish before the next lookup tick, greatly improving discv4 FindNode response rate.
Pre-bonding was responsible for most of the bootstrap traffic burst (~1,800 pings in the first minute). Remove it and rely on natural bonding (incoming pings, reciprocal pings, mutual discovery) for discv4 nodes to accept our FindNode queries.
Remove pinged_nodes field and all its bookkeeping (bootnode recording, pruning, handle_ping tracking) since pre-bonding was removed. Fix redundant closure clippy lints in lookup tests (|i| make_node(i) → make_node).
… bootstrap The 1s interval gave 3 FindNode/sec (alpha=3 × 1 tick/sec), while main sends 10 FindNode/sec. This caused kademlia to discover peers ~2x slower in the first 5 minutes, leading to 32% slower snapsync (5h17m vs 4h). Lowering to 500ms gives 6 FindNode/sec, closing most of the gap while keeping bootstrap traffic well under 10x main's.
🤖 Kimi Code ReviewThis is a substantial and well-architected PR implementing proper Kademlia routing tables and iterative lookups for the discovery layer. The code quality is high, with good separation of concerns between the k-bucket routing table and the connection pool. Summary of Changes
Code ReviewCorrectness & Safety1. Kademlia Bucket Index Calculation ( fn bucket_index(local_node_id: &H256, node_id: &H256) -> Option<usize> {
let xor = *local_node_id ^ *node_id;
let dist = U256::from_big_endian(xor.as_bytes());
if dist.is_zero() {
None
} else {
Some(dist.bits() - 1)
}
}Correctly computes 2. Lookup Convergence Logic ( pub fn is_finished(&self) -> bool {
if self.started_at.elapsed() >= LOOKUP_TIMEOUT {
return true;
}
!self.result.iter().any(|e| !e.queried)
}The logic correctly implements Kademlia convergence: finished when all k-closest nodes have been queried or timeout. The comment correctly notes that stragglers are not waited for. 3. Active Lookups Management ( self.discv4
.as_mut()
.expect("discv4 state must exist")
.active_lookups
.retain(|(l, _)| !l.is_finished());Properly cleans up finished lookups before processing. The Performance Considerations1. Connection Pool Scanning ( 2. Result Feeding ( for (lookup, _) in &mut discv4.active_lookups {
lookup.feed_results(entries.clone());
}Clones the entries vector for each active lookup. With typically 1-2 active lookups and 16 entries, this is negligible. Rust Idioms & Best Practices1. Efficient Optional Handling ( if let Some(contact) = self.get_contact_mut(&msg.node_id)
&& contact
.ping_id
.as_ref()
.map(|value| *value == msg.ping_id)
.unwrap_or(false)
{
contact.ping_id = None;
}Good use of let-chains (Rust 1.65+) for concise optional logic. 2. Borrow Checker Workaround ( Security & Robustness1. Peer Penalization ( let _ = self.peer_table.set_disposable(peer_id);Properly marks peers as disposable when they return empty headers or timeout on block bodies, allowing the k-bucket replacement mechanism to evict them. 2. Message Expiration ( Minor Observations1. Metric Description Accuracy ( 2. Test Coverage
3. Dead Code Removal Suggestions (Non-blocking)
Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
|
🤖 Codex Code Review
Aside from those, the refactor is directionally good: passing I couldn’t run Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
🤖 Claude Code ReviewNow I have a thorough understanding of the changes. Let me write the review. PR #6511 — Kademlia k-bucket routing table v2Overall: Solid architectural upgrade. The code is well-structured, well-tested, and the design rationale is clear. A few correctness and consistency issues are worth addressing before merge.
|
Greptile SummaryThis PR replaces the flat peer table with a proper Kademlia k-bucket routing table (256 buckets × 16 entries, with 10-slot replacement lists per bucket) and a decoupled 10K flat connection pool for RLPx initiation. Discovery lookups are redesigned from a single-shot FindNode broadcast into a timer-driven iterative convergence lookup (geth-style, alpha=3) for both discv4 and discv5, with a scaled 500ms→10s easing interval to keep traffic comparable to the previous approach.
Confidence Score: 4/5Safe to merge; all findings are non-blocking style and maintainability concerns with no effect on correctness under the current single-active-lookup design. The architectural change is well-scoped and backed by traffic/sync benchmarks. The dead crates/networking/p2p/discovery/lookup.rs (dead queries_in_flight field) and crates/networking/p2p/peer_table.rs (stale pool size doc comment, unsorted do_get_closest_nodes output).
|
| Filename | Overview |
|---|---|
| crates/networking/p2p/peer_table.rs | Core architectural change: replaces flat peer table with 256-bucket Kademlia routing table (k=16) plus a decoupled 10K connection pool. Logic is generally sound; doc comment incorrectly says pool is "up to 50,000"; do_get_closest_nodes doesn't sort its output by distance. |
| crates/networking/p2p/discovery/lookup.rs | New iterative convergence lookup with geth-style alpha=3 querying. Well-structured with tests. queries_in_flight is tracked but never consulted by is_finished() — effectively dead state that could mislead maintainers. |
| crates/networking/p2p/discovery/discv4_handlers.rs | Timer-driven iterative lookup for discv4: creates one cached FindNode per lookup, sends alpha=3 per tick. record_response() is always credited to the first active lookup rather than the lookup that dispatched the specific query — benign under the single-lookup invariant but fragile. |
| crates/networking/p2p/discovery/discv5_handlers.rs | Parallel discv5 iterative lookup using distance-bucketed FindNode. Same record_response() first-only attribution pattern as discv4. Otherwise clean. |
| crates/networking/p2p/discovery/server.rs | Unified discovery server with updated timer constants for iterative lookups (500ms→10s easing). Discv4State is now derived Default removing manual construction. Clean refactor. |
| crates/networking/p2p/discv4/server.rs | Removed the cached random FindNode message from Discv4State; now stored per-lookup. Active lookups list added. Default derive replaces manual new. Clean simplification. |
| crates/networking/p2p/peer_handler.rs | Marks peers disposable on empty-headers response and on block-body timeout, allowing pruning to recycle those k-bucket slots. Targeted improvement to churn handling. |
| crates/networking/p2p/sync/snap_sync.rs | Adds a prune_table() fire-and-forget call at the top of each sync loop iteration to keep the k-buckets fresh during snap sync. |
Sequence Diagram
sequenceDiagram
participant Timer
participant DiscoveryServer
participant IterativeLookup
participant PeerTable
participant RemoteNode
Timer->>DiscoveryServer: LookupV4/V5 tick
DiscoveryServer->>DiscoveryServer: retain(not finished)
alt active_lookups is empty
DiscoveryServer->>PeerTable: get_closest_from_pool(target, 16)
PeerTable-->>DiscoveryServer: seed nodes (sorted by XOR dist)
DiscoveryServer->>IterativeLookup: new(target, seeds)
DiscoveryServer->>DiscoveryServer: push lookup to active_lookups
end
DiscoveryServer->>IterativeLookup: "next_to_query(alpha=3)"
IterativeLookup-->>DiscoveryServer: [(node_id, node) x 3]
loop for each queried node
DiscoveryServer->>RemoteNode: FindNode (UDP)
DiscoveryServer->>PeerTable: pending_find_node.insert(node_id, now)
end
RemoteNode-->>DiscoveryServer: Neighbors / Nodes (UDP)
DiscoveryServer->>PeerTable: new_contacts(nodes)
DiscoveryServer->>IterativeLookup: feed_results(entries)
DiscoveryServer->>IterativeLookup: record_response()
Note over Timer,DiscoveryServer: next tick fires (500ms-10s later)
Timer->>DiscoveryServer: LookupV4/V5 tick
DiscoveryServer->>IterativeLookup: is_finished()?
alt "all queried OR elapsed >= 20s"
DiscoveryServer->>DiscoveryServer: retain removes lookup
else still unqueried entries
DiscoveryServer->>IterativeLookup: "next_to_query(alpha=3)"
DiscoveryServer->>RemoteNode: FindNode (UDP)
end
Comments Outside Diff (1)
-
crates/networking/p2p/peer_table.rs, line 1364-1389 (link)do_get_closest_nodesreturns the k-closest set in arbitrary orderThe function correctly maintains a heap-like structure to find the
MAX_NODES_IN_NEIGHBORS_PACKETclosest nodes, butnodes.into_iter().map(|(node, _)| node).collect()returns them in insertion order, not sorted by XOR distance. By contrast,do_get_closest_from_pool(used for seeding lookups) sorts before returning. The discv4 spec doesn't mandate ordering in a Neighbors packet, so this is not a protocol violation, but recipients that use the response to seed their own lookups get a sub-optimal ordering.Prompt To Fix With AI
This is a comment left during a code review. Path: crates/networking/p2p/peer_table.rs Line: 1364-1389 Comment: **`do_get_closest_nodes` returns the k-closest set in arbitrary order** The function correctly maintains a heap-like structure to find the `MAX_NODES_IN_NEIGHBORS_PACKET` closest nodes, but `nodes.into_iter().map(|(node, _)| node).collect()` returns them in insertion order, not sorted by XOR distance. By contrast, `do_get_closest_from_pool` (used for seeding lookups) sorts before returning. The discv4 spec doesn't mandate ordering in a Neighbors packet, so this is not a protocol violation, but recipients that use the response to seed their own lookups get a sub-optimal ordering. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 4 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 4
crates/networking/p2p/discovery/lookup.rs:33-35
**`queries_in_flight` is tracked but never consulted**
`queries_in_flight` is incremented by `next_to_query` and decremented by `record_response`/`record_timeout`, but `is_finished()` ignores it entirely — convergence is driven purely by whether all entries are `queried`. The field documents a useful concept but has no effect on behavior; it will mislead future maintainers who expect it to gate lookup completion or backpressure.
### Issue 2 of 4
crates/networking/p2p/peer_table.rs:482-486
Documentation says "up to 50,000" but the constant is 10,000. Stale copy-paste from an earlier design iteration.
```suggestion
/// Flat pool of discovered contacts for RLPx connection initiation.
/// Decoupled from the k-bucket routing table so that connection initiation
/// has access to a much larger candidate pool than the k-bucket structure
/// allows (k-buckets: 256 × 16 = 4,096 max; this pool: up to 10,000).
/// K-buckets are still used for all Kademlia protocol operations.
```
### Issue 3 of 4
crates/networking/p2p/discovery/discv4_handlers.rs:438-447
**`record_response()` always credits the first lookup, not the one that sent the query**
`feed_results` is correctly broadcast to all active lookups, but `record_response()` is only called on `active_lookups.first_mut()`. The architecture currently enforces a single-active-lookup invariant (a new lookup is only started when the list is empty), so `first()` is always the only entry. However, if that invariant ever relaxes, lookups at indices > 0 will accumulate a `queries_in_flight` counter that never decreases, leaving them wedged until the 20-second timeout fires. The same pattern exists in `discv5_handle_nodes_message`.
### Issue 4 of 4
crates/networking/p2p/peer_table.rs:1364-1389
**`do_get_closest_nodes` returns the k-closest set in arbitrary order**
The function correctly maintains a heap-like structure to find the `MAX_NODES_IN_NEIGHBORS_PACKET` closest nodes, but `nodes.into_iter().map(|(node, _)| node).collect()` returns them in insertion order, not sorted by XOR distance. By contrast, `do_get_closest_from_pool` (used for seeding lookups) sorts before returning. The discv4 spec doesn't mandate ordering in a Neighbors packet, so this is not a protocol violation, but recipients that use the response to seed their own lookups get a sub-optimal ordering.
Reviews (1): Last reviewed commit: "Merge branch 'main' into feat/kademlia-v..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| queries_in_flight: usize, | ||
| started_at: Instant, | ||
| } |
There was a problem hiding this comment.
queries_in_flight is tracked but never consulted
queries_in_flight is incremented by next_to_query and decremented by record_response/record_timeout, but is_finished() ignores it entirely — convergence is driven purely by whether all entries are queried. The field documents a useful concept but has no effect on behavior; it will mislead future maintainers who expect it to gate lookup completion or backpressure.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/networking/p2p/discovery/lookup.rs
Line: 33-35
Comment:
**`queries_in_flight` is tracked but never consulted**
`queries_in_flight` is incremented by `next_to_query` and decremented by `record_response`/`record_timeout`, but `is_finished()` ignores it entirely — convergence is driven purely by whether all entries are `queried`. The field documents a useful concept but has no effect on behavior; it will mislead future maintainers who expect it to gate lookup completion or backpressure.
How can I resolve this? If you propose a fix, please make it concise.| /// Flat pool of discovered contacts for RLPx connection initiation. | ||
| /// Decoupled from the k-bucket routing table so that connection initiation | ||
| /// has access to a much larger candidate pool than the k-bucket structure | ||
| /// allows (k-buckets: 256 × 16 = 4,096 max; this pool: up to 50,000). | ||
| /// K-buckets are still used for all Kademlia protocol operations. |
There was a problem hiding this comment.
| /// Flat pool of discovered contacts for RLPx connection initiation. | |
| /// Decoupled from the k-bucket routing table so that connection initiation | |
| /// has access to a much larger candidate pool than the k-bucket structure | |
| /// allows (k-buckets: 256 × 16 = 4,096 max; this pool: up to 50,000). | |
| /// K-buckets are still used for all Kademlia protocol operations. | |
| /// Flat pool of discovered contacts for RLPx connection initiation. | |
| /// Decoupled from the k-bucket routing table so that connection initiation | |
| /// has access to a much larger candidate pool than the k-bucket structure | |
| /// allows (k-buckets: 256 × 16 = 4,096 max; this pool: up to 10,000). | |
| /// K-buckets are still used for all Kademlia protocol operations. |
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/networking/p2p/peer_table.rs
Line: 482-486
Comment:
Documentation says "up to 50,000" but the constant is 10,000. Stale copy-paste from an earlier design iteration.
```suggestion
/// Flat pool of discovered contacts for RLPx connection initiation.
/// Decoupled from the k-bucket routing table so that connection initiation
/// has access to a much larger candidate pool than the k-bucket structure
/// allows (k-buckets: 256 × 16 = 4,096 max; this pool: up to 10,000).
/// K-buckets are still used for all Kademlia protocol operations.
```
How can I resolve this? If you propose a fix, please make it concise.| if let Some(discv4) = &mut self.discv4 { | ||
| let entries: Vec<(H256, Node)> = | ||
| nodes.iter().map(|n| (n.node_id(), n.clone())).collect(); | ||
| for (lookup, _) in &mut discv4.active_lookups { | ||
| lookup.feed_results(entries.clone()); | ||
| } | ||
| if let Some((lookup, _)) = discv4.active_lookups.first_mut() { | ||
| lookup.record_response(); | ||
| } | ||
| } |
There was a problem hiding this comment.
record_response() always credits the first lookup, not the one that sent the query
feed_results is correctly broadcast to all active lookups, but record_response() is only called on active_lookups.first_mut(). The architecture currently enforces a single-active-lookup invariant (a new lookup is only started when the list is empty), so first() is always the only entry. However, if that invariant ever relaxes, lookups at indices > 0 will accumulate a queries_in_flight counter that never decreases, leaving them wedged until the 20-second timeout fires. The same pattern exists in discv5_handle_nodes_message.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/networking/p2p/discovery/discv4_handlers.rs
Line: 438-447
Comment:
**`record_response()` always credits the first lookup, not the one that sent the query**
`feed_results` is correctly broadcast to all active lookups, but `record_response()` is only called on `active_lookups.first_mut()`. The architecture currently enforces a single-active-lookup invariant (a new lookup is only started when the list is empty), so `first()` is always the only entry. However, if that invariant ever relaxes, lookups at indices > 0 will accumulate a `queries_in_flight` counter that never decreases, leaving them wedged until the 20-second timeout fires. The same pattern exists in `discv5_handle_nodes_message`.
How can I resolve this? If you propose a fix, please make it concise.
ElFantasma
left a comment
ElFantasma
left a comment
There was a problem hiding this comment.
There is no CHANGELOG.md entry for this change in the diff. For a routing-table architecture migration this size, an entry under a Perf or Refactor section (with a one-liner pointing at the structural shift) would help anyone bisecting a future P2P regression understand the cutover. Easy to add as a follow-up.
| for (lookup, _) in &mut discv4.active_lookups { | ||
| lookup.feed_results(entries.clone()); | ||
| } | ||
| if let Some((lookup, _)) = discv4.active_lookups.first_mut() { |
There was a problem hiding this comment.
record_response attribution mismatch. feed_results is called on EVERY active lookup just above (line 437-441), but record_response is called only on active_lookups.first_mut(). So if multiple lookups are in flight, only the first one's queries_in_flight counter is decremented for any Neighbors response, regardless of which lookup the FindNode actually came from.
| return; | ||
| } | ||
| if self.connection_pool.len() >= MAX_CONNECTION_POOL_SIZE { | ||
| self.connection_pool.shift_remove_index(0); |
There was a problem hiding this comment.
shift_remove_index(0) is O(N) per overflow. Once the pool reaches the 10K cap, every new insert pays a 10K-element shift to drop the front entry. Under churn (e.g., bootstrap burst, peer rotation), that's a real cost on the hot path of every newly-discovered contact.
IndexMap has swap_remove_index(0) which is O(1) but breaks insertion order — and the LRU-like eviction here depends on insertion order to drop the OLDEST entry. So swap_remove_index would evict the wrong thing.
Two cleaner shapes:
VecDeque<(H256, Node)>+ separateFxHashMap<H256, usize>index:pop_front()is O(1),push_back()is O(1) amortized. Indices in the map need updating on pop, but only ~once per overflow, not per access.- Ring-buffer with monotonic head/tail counters: pure O(1), no shifting, no map index updates.
Not urgent — under steady state the pool fills slowly. But under bootstrap or recovery the cost is real (the PR description's "~630 FindNode/min during bootstrap" implies a steady stream of inserts the moment the pool hits cap).
## Summary Replaces the flat peer table with a proper Kademlia k-bucket routing table and iterative convergence lookup (geth-style), while maintaining comparable traffic and sync performance to main. ### Key changes - **Kademlia k-bucket routing table**: 256 buckets × 16 entries with replacement lists, XOR distance-based routing - **Flat connection pool**: Decoupled from k-buckets (up to 10K entries) so RLPx connection initiation has a larger candidate pool - **Iterative convergence lookup**: Seeds from connection pool, queries alpha=3 closest unqueried nodes per timer tick, feeds Neighbors/Nodes responses back into the result set - **Timer-driven lookup advance**: Each timer tick advances by alpha=3 queries. Responses feed results into the lookup but don't trigger new queries, naturally throttling traffic - **Scaled lookup interval**: 500ms initial → 10s steady-state (vs main's 100ms→600ms) to account for iterative lookups generating ~16 FindNodes each vs main's 1 ### Traffic comparison (Prometheus metrics, 30 min capture) | Metric | main (srv10, /min) | kademlia (srv3, /min) | Ratio | |--------|-------------------|-----------------------|-------| | v4 FindNode (steady) | 131 | 144 | 1.1x | | v5 FindNode (steady) | 132 | 148 | 1.1x | | v4 Ping | 61 | 34 | 0.6x | | v5 Ping | 61 | 60 | 1.0x | | **Total FindNode** | **263** | **292** | **1.1x** | | **Total Ping** | **122** | **94** | **0.8x** | Bootstrap burst (first 5 min): ~630 FindNode/min total, dropping to ~290/min by minute 29. ### Multisync comparison (same hardware, same Docker setup) Three-network snapsync (mainnet + sepolia + hoodi) on srv3: | Network | main | kademlia | Delta | |---------|------|----------|-------| | hoodi | baseline | −1 min | faster | | sepolia | baseline | −1 min | faster | | mainnet | baseline | +2 min | slower | **Result: within noise.** No regression on sync time despite the architectural change. Note: Docker multisync uses bridge networking (outbound-only, no inbound P2P), which limits peer count to ~50-60 and disproportionately affects kademlia (pool diversity matters more when 100% of peers come from the pool). Bare-metal performance should be slightly better since inbound connections add peer diversity independent of discovery strategy. ### Bare-metal peer discovery (clean restart, no peer cache) | Time | Kademlia peers | Main peers | |------|---------------|------------| | 1m | 3 | 8 | | 5m | 17 | 34 | | 10m | 55 | 61 | | 20m | 97 | 88 | Slower first 5 min, catches up by 10 min, beats main at 20 min. ### Why Kademlia matters The current flat peer table works well for filling RLPx slots but doesn't provide structured routing. Kademlia gives us: - **O(log n) routing** for future DHT lookups (e.g., EIP-7594 PeerDAS) - **Resilience**: k-buckets with replacement lists handle churn better - **Scalability**: structured routing scales to large networks without random probing - **Standards compliance**: aligns with devp2p/discv5 spec expectations ### What was explored and removed Several approaches were tried and removed for generating excessive traffic: - **Pre-bonding** (pinging discovered nodes before querying): 7x traffic burst - **Concurrent lookups**: multiplied traffic without proportional peer gains - **Response-driven advance**: discv5 lookups converged in <200ms, churning 60+ lookups/min - **Back-to-back lookup chaining**: kept tick permanently active The final design uses timer-driven advance with a scaled easing curve, naturally throttling traffic while benefiting from structured routing.
…aclass#7163) **Motivation** `DiscoveryConfig::initial_lookup_interval` is dead data. lambdaclass#6511 (kademlia k-bucket routing table v2) replaced the configurable bound in `DiscoveryServer::get_lookup_interval` with its own constants, because each iterative lookup emits ~16 FindNode messages instead of 1 and needed a rescaled schedule: ```diff lookup_interval_function( peer_completion, - self.config.initial_lookup_interval, - super::LOOKUP_INTERVAL_MS, + ITERATIVE_LOOKUP_INITIAL_MS, // 500.0 + ITERATIVE_LOOKUP_INTERVAL_MS, // 10_000.0 ) ``` The field itself survived, still threaded from `--p2p.lookup-interval` through `P2PContext` and `start_network` into a `DiscoveryServer` that never reads it. The only live consumer of the flag is the RLPx initiator, so the help text was also misleading. While tracing it, a second problem turned up: `impl Default for Options` sets `lookup_interval: Default::default()`, which is `0.0` for `f64` rather than the `INITIAL_LOOKUP_INTERVAL_MS` that clap uses for the flag. `default_l1()`, `default_l2()` and the L2 node options all build on that impl, so they hand the RLPx initiator a 0ms lower bound. With no peers connected the easing curve returns `Duration::ZERO`, and `handle_look_for_peer` re-arms its timer with no delay at all. Present since lambdaclass#5518 added the flag. **Description** - Remove `initial_lookup_interval` from `DiscoveryConfig` and stop overriding it in `start_network`. `DiscoveryConfig`'s `Default` impl goes with it: both remaining fields are set explicitly at the single construction site, and clap already defaults them to `true`. - Reword `--p2p.lookup-interval`'s help to describe what it actually controls, and refresh the now-stale doc comments on `INITIAL_LOOKUP_INTERVAL_MS` / `LOOKUP_INTERVAL_MS` / `lookup_interval_function`. - Default `Options::lookup_interval` to `INITIAL_LOOKUP_INTERVAL_MS`. No behavior change for the discovery server, which was already running on its own constants. The second commit does change behavior for the dev/L2 paths that go through `Options::default()`, restoring the 100ms floor they were meant to have. **Checklist** - [ ] Updated `STORE_SCHEMA_VERSION` (crates/storage/lib.rs) if the PR includes breaking changes to the `Store` requiring a re-sync.
4 participants
Summary
Replaces the flat peer table with a proper Kademlia k-bucket routing table and iterative convergence lookup (geth-style), while maintaining comparable traffic and sync performance to main.
Key changes
Traffic comparison (Prometheus metrics, 30 min capture)
Bootstrap burst (first 5 min): ~630 FindNode/min total, dropping to ~290/min by minute 29.
Multisync comparison (same hardware, same Docker setup)
Three-network snapsync (mainnet + sepolia + hoodi) on srv3:
Result: within noise. No regression on sync time despite the architectural change.
Note: Docker multisync uses bridge networking (outbound-only, no inbound P2P), which limits peer count to ~50-60 and disproportionately affects kademlia (pool diversity matters more when 100% of peers come from the pool). Bare-metal performance should be slightly better since inbound connections add peer diversity independent of discovery strategy.
Bare-metal peer discovery (clean restart, no peer cache)
Slower first 5 min, catches up by 10 min, beats main at 20 min.
Why Kademlia matters
The current flat peer table works well for filling RLPx slots but doesn't provide structured routing. Kademlia gives us:
What was explored and removed
Several approaches were tried and removed for generating excessive traffic:
The final design uses timer-driven advance with a scaled easing curve, naturally throttling traffic while benefiting from structured routing.