From d25c53b06e42e721303744cfa319ec4667217f8a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Gr=C3=BCner?= <47506558+MegaRedHand@users.noreply.github.com> Date: Tue, 25 Aug 2026 16:05:17 -0300 Subject: [PATCH 01/11] refactor(l1): move the contact table into the discovery server The peer table held two unrelated things: the nodes discovery has heard of, and the peers RLPx is connected to. A consumer that only wants discv5 had to spin up an actor carrying `PeerConnection`, `Capability` and a `Store`-backed fork-id filter to get at any of it, and the discv5 tests had to build an in-memory store to test a nonce counter. Contacts, k-buckets, the connection pool and discv5 sessions now live in a `ContactTable` owned outright by `DiscoveryServer` as plain state, so every discv4/discv5 handler reaches them by `&mut self` instead of paying a message hop per inbound packet. The peer table keeps only connected peers, their scores and their request slots. The two halves meet at five points, all of them now pointing one way, from RLPx into discovery: - dial candidates: `next_dial_candidate` returns a `Node`, filtered against a connected set discovery maintains itself - `mark_connected` / `mark_disconnected`, cast alongside the existing `new_connected_peer` and `remove_peer` calls - `set_unwanted` / `set_disposable`, cast from the connection server and the peer handler - lookup pacing, which used to read `target_peers_completion` back out of the peer table and is now computed from discovery's own connected count That last one was the only call pointing from discovery into RLPx; inverting it is what lets discovery run without a peer table at all. Everything crossing the boundary is now a cast except the dial-candidate request, so two actors with sequential mailboxes can never call into each other. Two fixes fell out of the move: - `Contact.session` duplicated the standalone session store and was read only as a fallback, which outlived the disconnect cleanup: a session was never actually dropped for a node that still had a contact. The field is gone and the store is the single source of truth. - `target_reached` was dead, and identical to `target_peers_reached`. --- cmd/ethrex/initializers.rs | 7 +- cmd/ethrex/l2/initializers.rs | 8 +- .../networking/p2p/discovery/contact_table.rs | 1352 ++++++++++++++++ .../p2p/discovery/discv4_handlers.rs | 78 +- .../p2p/discovery/discv5_handlers.rs | 118 +- crates/networking/p2p/discovery/lookup.rs | 2 +- crates/networking/p2p/discovery/mod.rs | 10 +- crates/networking/p2p/discovery/server.rs | 209 ++- crates/networking/p2p/network.rs | 17 +- crates/networking/p2p/peer_filter.rs | 13 + crates/networking/p2p/peer_handler.rs | 17 +- crates/networking/p2p/peer_table.rs | 1398 +---------------- .../p2p/rlpx/connection/handshake.rs | 1 + .../networking/p2p/rlpx/connection/server.rs | 20 +- crates/networking/p2p/rlpx/initiator.rs | 4 +- crates/networking/p2p/sync/snap_sync.rs | 2 +- crates/networking/rpc/test_utils.rs | 12 +- .../p2p/discovery/discv5_server_tests.rs | 53 +- 18 files changed, 1757 insertions(+), 1564 deletions(-) create mode 100644 crates/networking/p2p/discovery/contact_table.rs diff --git a/cmd/ethrex/initializers.rs b/cmd/ethrex/initializers.rs index 7fae0ab6c52..b02096ae2a2 100644 --- a/cmd/ethrex/initializers.rs +++ b/cmd/ethrex/initializers.rs @@ -470,6 +470,7 @@ pub async fn init_network( let discovery_config = DiscoveryConfig { discv4_enabled: opts.discv4_enabled, discv5_enabled: opts.discv5_enabled, + target_peers: opts.target_peers, }; ethrex_p2p::start_network(context, bootnodes, discovery_config) @@ -900,8 +901,7 @@ pub async fn init_l1( let local_node_record = get_local_node_record(&datadir, &local_p2p_node, &signer); - let peer_table = - PeerTableServer::spawn(local_p2p_node.node_id(), opts.target_peers, store.clone()); + let peer_table = PeerTableServer::spawn(opts.target_peers); // TODO: Check every module starts properly. let tracker = TaskTracker::new(); @@ -925,7 +925,8 @@ pub async fn init_l1( let initiator = RLPxInitiator::spawn(p2p_context.clone()); - let peer_handler = PeerHandler::new(peer_table.clone(), initiator); + let peer_handler = + PeerHandler::new(peer_table.clone(), initiator, p2p_context.discovery.clone()); init_rpc_api( &opts, diff --git a/cmd/ethrex/l2/initializers.rs b/cmd/ethrex/l2/initializers.rs index 5d0e467d58d..32ee1fbfe17 100644 --- a/cmd/ethrex/l2/initializers.rs +++ b/cmd/ethrex/l2/initializers.rs @@ -285,11 +285,7 @@ pub async fn init_l2( if !opts.sequencer_opts.based { blockchain.set_synced(); } - let peer_table = PeerTableServer::spawn( - local_p2p_node.node_id(), - opts.node_opts.target_peers, - store.clone(), - ); + let peer_table = PeerTableServer::spawn(opts.node_opts.target_peers); let p2p_context = P2PContext::new( local_p2p_node.clone(), network_config, @@ -321,7 +317,7 @@ pub async fn init_l2( ) .expect("P2P context could not be created"); let initiator = RLPxInitiator::spawn(p2p_context.clone()); - let peer_handler = PeerHandler::new(peer_table, initiator); + let peer_handler = PeerHandler::new(peer_table, initiator, p2p_context.discovery.clone()); // Create SyncManager let syncer = SyncManager::new( diff --git a/crates/networking/p2p/discovery/contact_table.rs b/crates/networking/p2p/discovery/contact_table.rs new file mode 100644 index 00000000000..724650b507c --- /dev/null +++ b/crates/networking/p2p/discovery/contact_table.rs @@ -0,0 +1,1352 @@ +//! The contacts discovery knows about, and the Kademlia routing table over them. +//! +//! Owned outright by [`DiscoveryServer`](super::DiscoveryServer) as plain state: +//! every discv4 and discv5 handler runs inside that actor's message loop, so +//! they reach the table by `&mut self` rather than paying a message hop per +//! inbound packet. +//! +//! Nothing here knows what RLPx is. A consumer that only wants discovery says +//! what makes a peer worth keeping through a [`PeerFilter`], and learns which +//! of them are worth dialing through [`ContactTable::next_dial_candidate`]. +//! Whoever does dial reports back with [`ContactTable::mark_connected`] and +//! [`ContactTable::mark_disconnected`], which is all the table needs to keep +//! its candidates and its lookup pacing honest. +//! +//! The table is protocol-agnostic across the two discovery protocols. The key +//! abstraction is using `Bytes` for ping identifiers: +//! - discv4: converts H256 ping hash to Bytes +//! - discv5: already uses Bytes for req_id +//! +//! Each contact is tagged with the protocol that discovered it, allowing +//! protocol-specific lookups to only query compatible contacts. + +use crate::{ + metrics::METRICS, + peer_filter::PeerFilter, + types::{Node, NodeRecord}, + utils::distance, +}; +use bytes::Bytes; +use ethrex_common::{H256, U256}; +use indexmap::IndexMap; +use rand::seq::{IteratorRandom, SliceRandom}; +use rustc_hash::{FxHashMap, FxHashSet}; +use std::{ + net::IpAddr, + time::{Duration, Instant}, +}; + +/// Session information for discv5 protocol. +/// Contains symmetric keys derived from ECDH for message encryption/decryption. +pub use crate::discv5::session::Session; +/// Maximum number of ENRs to return in a FindNode response (discv4 compatible). +pub(crate) const MAX_NODES_IN_NEIGHBORS_PACKET: usize = 16; +/// Maximum number of ENRs to return in a discv5 FindNode response. +const MAX_ENRS_PER_FINDNODE_RESPONSE: usize = 16; + +/// Number of k-buckets in the Kademlia routing table (one per bit of the 256-bit node ID). +const NUMBER_OF_BUCKETS: usize = 256; +/// Maximum number of contacts per k-bucket (Kademlia k parameter). +pub const MAX_NODES_PER_BUCKET: usize = 16; +/// Maximum number of replacement entries per k-bucket. +const MAX_REPLACEMENTS_PER_BUCKET: usize = 10; +/// Maximum number of entries in the flat connection candidate pool. +/// This pool is separate from the k-bucket routing table and retains +/// more contacts for RLPx connection initiation than the k-bucket +/// structure allows (256 × 16 = 4,096 vs this larger capacity). +/// 10K matches what Reth and Nethermind use for their candidate pools. +const MAX_CONNECTION_POOL_SIZE: usize = 10_000; + +/// A single k-bucket in the Kademlia routing table. +/// Each bucket stores contacts at a specific XOR distance range from the local node. +#[derive(Debug, Clone, Default)] +pub struct KBucket { + pub(crate) contacts: Vec<(H256, Contact)>, + pub(crate) replacements: Vec<(H256, Contact)>, +} + +impl KBucket { + /// Find a contact by node ID in the main list. + fn get(&self, node_id: &H256) -> Option<&Contact> { + self.contacts + .iter() + .find(|(id, _)| id == node_id) + .map(|(_, c)| c) + } + + /// Find a contact by node ID in either the main or replacement list. + fn get_any(&self, node_id: &H256) -> Option<&Contact> { + self.get(node_id).or_else(|| { + self.replacements + .iter() + .find(|(id, _)| id == node_id) + .map(|(_, c)| c) + }) + } + + /// Find a mutable reference to a contact by node ID (main or replacement list). + fn get_mut(&mut self, node_id: &H256) -> Option<&mut Contact> { + if let Some((_, c)) = self.contacts.iter_mut().find(|(id, _)| id == node_id) { + return Some(c); + } + self.replacements + .iter_mut() + .find(|(id, _)| id == node_id) + .map(|(_, c)| c) + } + + /// Check if a contact exists in this bucket (main or replacement list). + fn contains(&self, node_id: &H256) -> bool { + self.contacts.iter().any(|(id, _)| id == node_id) + || self.replacements.iter().any(|(id, _)| id == node_id) + } + + /// Insert a contact into the bucket. Returns true if inserted into main list. + /// If the bucket is full, the contact is added to the replacement list instead. + fn insert(&mut self, node_id: H256, contact: Contact) -> bool { + if self.contacts.len() < MAX_NODES_PER_BUCKET { + self.contacts.push((node_id, contact)); + true + } else { + self.insert_replacement(node_id, contact); + false + } + } + + /// Add a contact to the replacement list, evicting the oldest if full. + fn insert_replacement(&mut self, node_id: H256, contact: Contact) { + if self.replacements.len() >= MAX_REPLACEMENTS_PER_BUCKET { + self.replacements.remove(0); + } + self.replacements.push((node_id, contact)); + } + + /// Remove a contact from the main list and promote a replacement if available. + /// Returns the promoted replacement's node ID, if any. + fn remove_and_promote(&mut self, node_id: &H256) -> Option { + let idx = self.contacts.iter().position(|(id, _)| id == node_id)?; + self.contacts.remove(idx); + if !self.replacements.is_empty() { + let (replacement_id, replacement) = self.replacements.remove(0); + self.contacts.push((replacement_id, replacement)); + Some(replacement_id) + } else { + None + } + } +} + +/// Computes the bucket index for a node relative to the local node. +/// Uses XOR distance: bucket = floor(log2(XOR(local, remote))), i.e. the +/// position of the highest set bit minus 1. +/// Returns None for the local node itself (XOR = 0). +fn bucket_index(local_node_id: &H256, node_id: &H256) -> Option { + 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) + } +} + +/// Computes the raw XOR distance between two node IDs. +/// Used for comparing relative closeness: a is closer to target than b +/// iff xor_distance(target, a) < xor_distance(target, b). +pub(crate) fn xor_distance(a: &H256, b: &H256) -> H256 { + *a ^ *b +} + +/// Identifies which discovery protocol was used to find a contact. +/// This allows protocol-specific lookups to only query compatible contacts. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +pub enum DiscoveryProtocol { + /// Contact discovered via discv4 protocol + Discv4, + /// Contact discovered via discv5 protocol + Discv5, +} + +#[derive(Debug, Clone)] +pub struct Contact { + pub node: Node, + /// Whether this contact is reachable via discv4 protocol. + pub is_discv4: bool, + /// Whether this contact is reachable via discv5 protocol. + pub is_discv5: bool, + /// The timestamp when the contact was last sent a ping. + /// If None, the contact has never been pinged. + pub validation_timestamp: Option, + /// The identifier of the last unacknowledged ping sent to this contact, or + /// None if no ping was sent yet or it was already acknowledged. + /// - discv4: H256 hash converted to Bytes + /// - discv5: request ID as Bytes + pub ping_id: Option, + + /// The hash of the last unacknowledged ENRRequest sent to this contact, or + /// None if no request was sent yet or it was already acknowledged. + pub enr_request_hash: Option, + + /// ENR associated with this contact, if it was provided by the peer. + pub record: Option, + /// This contact failed to respond our Ping. + pub disposable: bool, + /// Set to true after we send a successful ENRResponse to it. + pub knows_us: bool, + /// This is a known-bad peer (on another network, no matching capabilities, etc) + pub unwanted: bool, + /// Whether this contact's last known ENR made it through the consumer's + /// [`PeerFilter`], or `None` while it has never been filtered. + /// + /// Unfiltered stays dialable: a contact discovered without an ENR never + /// reaches the filter at all, and treating that as a rejection would leave + /// it permanently untriable. That is why bootnodes, which arrive as bare + /// endpoints, are dialable before they have published anything. + pub passes_filter: Option, +} + +impl Contact { + pub fn was_validated(&self) -> bool { + self.validation_timestamp.is_some() && !self.has_pending_ping() + } + + pub fn has_pending_ping(&self) -> bool { + self.ping_id.is_some() + } + + pub fn record_ping_sent(&mut self, ping_id: Bytes) { + self.validation_timestamp = Some(Instant::now()); + self.ping_id = Some(ping_id); + } + + pub fn record_enr_request_sent(&mut self, request_hash: H256) { + self.enr_request_hash = Some(request_hash); + } + + /// Stores `record` if it answers the ENR request we have outstanding. + /// + /// Returns whether it was stored, so the caller knows whether the contact's + /// cached [`Self::passes_filter`] still describes the record it holds. A + /// response whose hash does not match is ignored outright: letting it + /// through would let a peer restate its own standing from a record we + /// refused to keep. + pub fn record_enr_response_received(&mut self, request_hash: H256, record: NodeRecord) -> bool { + if self + .enr_request_hash + .take_if(|h| *h == request_hash) + .is_some() + { + self.record = Some(record); + return true; + } + false + } + + pub fn has_pending_enr_request(&self) -> bool { + self.enr_request_hash.is_some() + } +} + +impl Contact { + pub fn new(node: Node, protocol: DiscoveryProtocol) -> Self { + Self { + node, + is_discv4: protocol == DiscoveryProtocol::Discv4, + is_discv5: protocol == DiscoveryProtocol::Discv5, + validation_timestamp: None, + ping_id: None, + enr_request_hash: None, + record: None, + disposable: false, + knows_us: true, + unwanted: false, + passes_filter: None, + } + } + + /// Check if this contact supports the given protocol. + pub fn supports_protocol(&self, protocol: DiscoveryProtocol) -> bool { + match protocol { + DiscoveryProtocol::Discv4 => self.is_discv4, + DiscoveryProtocol::Discv5 => self.is_discv5, + } + } + + /// Mark this contact as supporting the given protocol. + pub fn add_protocol(&mut self, protocol: DiscoveryProtocol) { + match protocol { + DiscoveryProtocol::Discv4 => self.is_discv4 = true, + DiscoveryProtocol::Discv5 => self.is_discv5 = true, + } + } +} + +/// Result of contact validation. +#[derive(Debug, Clone)] +pub enum ContactValidation { + Valid(Box), + InvalidContact, + UnknownContact, + IpMismatch, +} + +/// Everything discovery knows about the nodes it has found. +/// +/// Four stores, deliberately kept apart: +/// - `buckets`, the Kademlia routing table, answering the protocol's own +/// "who is near this id" questions. +/// - `connection_pool`, a much larger flat pool of dialable nodes. The +/// k-buckets cap out at 256 x 16 = 4,096 and evict by distance, which is the +/// right policy for routing and the wrong one for finding someone to talk to. +/// - `sessions`, discv5's symmetric keys, kept independently of contacts so a +/// session survives a node whose ENR we cannot yet parse. +/// - `connected`, the ids the consumer has told us it is talking to. +pub struct ContactTable { + local_node_id: H256, + buckets: Vec, + /// Flat pool of discovered contacts for 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 x 16 = 4,096 max; this pool: up to 10,000). + /// K-buckets are still used for all Kademlia protocol operations. + connection_pool: IndexMap, + /// Standalone session store, independent of contacts. + /// Allows sessions to be stored even before the contact's ENR is known/parseable. + sessions: FxHashMap, + /// What this consumer requires of a discovered peer. Judged as each ENR + /// arrives, over either discovery protocol; the answer is cached on the + /// contact as [`Contact::passes_filter`]. + filter: Box, + /// Nodes the consumer has reported as connected. Kept here rather than read + /// back from the consumer so discovery never has to call into it: the two + /// lifecycle casts are the only thing crossing the boundary. + connected: FxHashSet, + /// Nodes already offered to the dialer this cycle, cleared once the pool is + /// exhausted so failed dials get another turn. + already_tried_peers: FxHashSet, + /// How many connections the consumer wants, used only to pace lookups. + target_peers: usize, +} + +// Hand-written because `Box` is not `Debug`, and requiring that +// of every consumer's filter buys less than keeping the table printable. +impl std::fmt::Debug for ContactTable { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("ContactTable") + .field("local_node_id", &self.local_node_id) + .field("connection_pool", &self.connection_pool) + .field("sessions", &self.sessions) + .field("connected", &self.connected) + .field("already_tried_peers", &self.already_tried_peers) + .field("target_peers", &self.target_peers) + .finish_non_exhaustive() + } +} + +impl ContactTable { + pub fn new(local_node_id: H256, target_peers: usize, filter: Box) -> Self { + Self { + local_node_id, + buckets: vec![KBucket::default(); NUMBER_OF_BUCKETS], + connection_pool: IndexMap::with_capacity(MAX_CONNECTION_POOL_SIZE), + sessions: Default::default(), + filter, + connected: Default::default(), + already_tried_peers: Default::default(), + target_peers, + } + } + + // --- Consumer lifecycle --- + + /// Record that the consumer is now connected to `node_id`, so it stops + /// being offered as a dial candidate and counts towards lookup pacing. + pub fn mark_connected(&mut self, node_id: H256) { + self.connected.insert(node_id); + } + + /// Record that the consumer's connection to `node_id` is gone. + /// + /// Also drops the node's discv5 session: the keys were negotiated for a + /// peer we are no longer talking to, and keeping them would leave the + /// session store growing with every peer that ever connected. + pub fn mark_disconnected(&mut self, node_id: &H256) { + self.connected.remove(node_id); + self.sessions.remove(node_id); + } + + /// How far along the consumer is towards the connection count it wants. + /// Feeds the lookup interval: a node with no peers looks hard, a full one + /// coasts. + pub fn peer_completion(&self) -> f64 { + if self.target_peers == 0 { + return 1.0; + } + self.connected.len() as f64 / self.target_peers as f64 + } + + // --- Sessions --- + + /// The discv5 session for a node, if one was ever negotiated. + /// + /// The standalone store is the only place a session lives. Contacts used to + /// keep a second copy, which nothing needed and which outlived the + /// disconnect cleanup below, so a session was never actually dropped for a + /// node that still had a contact. + pub fn session(&self, node_id: &H256) -> Option { + self.sessions.get(node_id).cloned() + } + + pub fn set_session(&mut self, node_id: H256, session: Session) { + self.sessions.insert(node_id, session); + } + + // --- Contact flags --- + + /// Mark a contact as one we should stop keeping: it failed to answer a ping, + /// or the consumer found it useless. Pruned on the next [`Self::prune`]. + pub fn set_disposable(&mut self, node_id: &H256) { + if let Some(contact) = self.get_contact_mut(node_id) { + contact.disposable = true; + } + } + + /// Mark a contact as known-bad: on another network, no matching + /// capabilities, or otherwise rejected by the consumer. Never dialed again. + pub fn set_unwanted(&mut self, node_id: &H256) { + if let Some(contact) = self.get_contact_mut(node_id) { + contact.unwanted = true; + } + } + + /// Record that we answered this contact's ENR request, so it has a bond + /// with us and is worth dialing. + pub fn mark_knows_us(&mut self, node_id: &H256) { + if let Some(contact) = self.get_contact_mut(node_id) { + contact.knows_us = true; + } + } + + pub fn record_ping_sent(&mut self, node_id: &H256, ping_id: Bytes) { + if let Some(contact) = self.get_contact_mut(node_id) { + contact.record_ping_sent(ping_id); + } + } + + /// Clear the outstanding ping if `ping_id` is the one we are waiting on. + pub fn record_pong_received(&mut self, node_id: &H256, ping_id: &Bytes) { + if let Some(contact) = self.get_contact_mut(node_id) + && contact + .ping_id + .as_ref() + .map(|value| value == ping_id) + .unwrap_or(false) + { + contact.ping_id = None; + } + } + + /// Insert a node discovered over `protocol`, returning whether it was new. + /// + /// Returns true for any genuinely new node, even if it overflows to the + /// replacement list. This ensures the caller sends a reciprocal ping + /// which establishes the bond needed for FindNode validation. + pub async fn insert_if_new(&mut self, node: Node, protocol: DiscoveryProtocol) -> bool { + let node_id = node.node_id(); + // Always add to the connection pool + self.insert_to_connection_pool(node_id, node.clone()); + if self.contact_exists(&node_id) { + return false; + } + let contact = Contact::new(node, protocol); + self.insert_contact(node_id, contact); + METRICS.record_new_discovery().await; + true + } + + // --- K-bucket accessors --- + + /// Get the bucket index for a node ID, or None if it's the local node. + fn bucket_for(&self, node_id: &H256) -> Option { + bucket_index(&self.local_node_id, node_id) + } + + /// Look up a contact by node ID in main or replacement list (O(K) within the bucket). + pub fn get_contact(&self, node_id: &H256) -> Option<&Contact> { + let idx = self.bucket_for(node_id)?; + self.buckets[idx].get_any(node_id) + } + + /// Look up a mutable reference to a contact by node ID. + pub(crate) fn get_contact_mut(&mut self, node_id: &H256) -> Option<&mut Contact> { + let idx = self.bucket_for(node_id)?; + self.buckets[idx].get_mut(node_id) + } + + /// Check if a contact exists in any bucket (main or replacement list). + fn contact_exists(&self, node_id: &H256) -> bool { + let Some(idx) = self.bucket_for(node_id) else { + return false; + }; + self.buckets[idx].contains(node_id) + } + + /// Insert a contact into the appropriate k-bucket. Returns true if inserted + /// into the main list, false if the node went to the replacement list or is + /// the local node. + fn insert_contact(&mut self, node_id: H256, contact: Contact) -> bool { + #[cfg(feature = "metrics")] + let start = std::time::Instant::now(); + + let Some(idx) = self.bucket_for(&node_id) else { + return false; + }; + let result = self.buckets[idx].insert(node_id, contact); + + #[cfg(feature = "metrics")] + { + use ethrex_metrics::p2p::METRICS_P2P; + METRICS_P2P.observe_insert_contact_duration(start.elapsed().as_secs_f64()); + } + + result + } + + /// Insert a node into the flat connection pool for RLPx initiation. + /// Evicts the oldest entry when the pool is at capacity. + fn insert_to_connection_pool(&mut self, node_id: H256, node: Node) { + if self.connection_pool.contains_key(&node_id) { + return; + } + if self.connection_pool.len() >= MAX_CONNECTION_POOL_SIZE { + self.connection_pool.shift_remove_index(0); + } + self.connection_pool.insert(node_id, node); + } + + /// Look up a contact by node ID in either the main or replacement list. + fn get_contact_or_replacement(&self, node_id: &H256) -> Option<&Contact> { + let idx = self.bucket_for(node_id)?; + self.buckets[idx].get_any(node_id) + } + + /// Look up a mutable reference in either the main or replacement list. + fn get_contact_or_replacement_mut(&mut self, node_id: &H256) -> Option<&mut Contact> { + let idx = self.bucket_for(node_id)?; + let bucket = &mut self.buckets[idx]; + // Search main list first, then replacement list. + // Done inline to avoid borrow-checker issues with or_else closures. + if let Some(pos) = bucket.contacts.iter().position(|(id, _)| id == node_id) { + return Some(&mut bucket.contacts[pos].1); + } + if let Some(pos) = bucket.replacements.iter().position(|(id, _)| id == node_id) { + return Some(&mut bucket.replacements[pos].1); + } + None + } + + /// Iterate over all contacts across all buckets (main and replacement lists). + fn iter_contacts(&self) -> impl Iterator { + self.buckets.iter().flat_map(|bucket| { + bucket + .contacts + .iter() + .chain(bucket.replacements.iter()) + .map(|(id, c)| (id, c)) + }) + } + + // --- Contact operations --- + + /// Prune disposable contacts from both main and replacement lists. + /// When a main contact is removed, a replacement is automatically promoted. + /// Pruned contacts remain in the connection pool so they can be retried + /// later — the RLPx handshake will reject them if they're truly bad. + pub fn prune(&mut self) { + for bucket in &mut self.buckets { + // Collect disposable contacts from main list + let main_disposable: Vec = bucket + .contacts + .iter() + .filter(|(_, c)| c.disposable) + .map(|(id, _)| *id) + .collect(); + + // Remove from main list and promote replacements + for node_id in main_disposable { + bucket.remove_and_promote(&node_id); + } + + // Remove disposable contacts from replacement list + // (these don't get promoted, just removed) + bucket.replacements.retain(|(_, c)| !c.disposable); + } + } + + /// Pick the next node to hand to the RLPx dialer, or `None` when the pool + /// holds nothing worth trying right now. + /// + /// Draws from the flat connection pool using O(1) random index probing: + /// pick a random start index and scan forward (wrapping) until an eligible + /// candidate turns up or the pool is exhausted. + /// + /// Skips anything already connected, anything tried since the last reset, + /// and any contact that is unwanted, does not know us, or failed the + /// consumer's [`PeerFilter`]. + pub fn next_dial_candidate(&mut self) -> Option { + let pool_len = self.connection_pool.len(); + if pool_len == 0 { + return None; + } + + let start = rand::random::() % pool_len; + for offset in 0..pool_len { + let idx = (start + offset) % pool_len; + let Some((node_id, node)) = self.connection_pool.get_index(idx) else { + continue; + }; + let node_id = *node_id; + + if self.connected.contains(&node_id) + || self.already_tried_peers.contains(&node_id) + || self + .get_contact_or_replacement(&node_id) + .map(|c| !c.knows_us || c.unwanted || c.passes_filter == Some(false)) + .unwrap_or(false) + { + continue; + } + + let node = node.clone(); + self.already_tried_peers.insert(node_id); + return Some(node); + } + + // Exhausted all candidates — reset tried set for next cycle. + tracing::trace!("Resetting list of tried peers."); + self.already_tried_peers.clear(); + None + } + + /// Get the `count` closest nodes from the connection pool, sorted by XOR distance to `target`. + pub fn closest_from_pool(&self, target: H256, count: usize) -> Vec<(H256, Node)> { + let mut nodes: Vec<(H256, Node, H256)> = Vec::with_capacity(count); + + for (node_id, node) in &self.connection_pool { + let dist = xor_distance(&target, node_id); + if nodes.len() < count { + nodes.push((*node_id, node.clone(), dist)); + } else if let Some((farthest_idx, _)) = + nodes.iter().enumerate().max_by_key(|(_, (_, _, d))| *d) + && dist < nodes[farthest_idx].2 + { + nodes[farthest_idx] = (*node_id, node.clone(), dist); + } + } + + nodes.sort_by(|a, b| a.2.cmp(&b.2)); + nodes.into_iter().map(|(id, node, _)| (id, node)).collect() + } + + /// Get contact for ENR lookup (discv4 only) + pub fn contact_for_enr_lookup(&mut self) -> Option { + self.iter_contacts() + .filter(|(_, c)| { + c.is_discv4 + && c.was_validated() + && !c.has_pending_enr_request() + && c.record.is_none() + && !c.disposable + }) + .map(|(_, c)| c) + .collect::>() + .choose(&mut rand::rngs::OsRng) + .cloned() + .cloned() + } + + pub fn contact_to_revalidate( + &self, + revalidation_interval: Duration, + protocol: DiscoveryProtocol, + ) -> Option> { + self.iter_contacts() + .filter(|(_, c)| { + c.supports_protocol(protocol) + && Self::is_validation_needed(c, revalidation_interval) + }) + .map(|(_, c)| c) + .choose(&mut rand::rngs::OsRng) + .cloned() + .map(Box::new) + } + + pub fn validate_contact(&self, node_id: H256, sender_ip: IpAddr) -> ContactValidation { + let Some(contact) = self.get_contact(&node_id) else { + return ContactValidation::UnknownContact; + }; + if !contact.was_validated() { + return ContactValidation::InvalidContact; + } + + // Check that the IP address from which we receive the request matches the one we have stored + // to prevent amplification attacks. + if sender_ip != contact.node.ip { + return ContactValidation::IpMismatch; + } + ContactValidation::Valid(Box::new(contact.clone())) + } + + /// Get closest nodes using raw XOR distance for accurate ordering. + pub fn closest_nodes(&self, node_id: H256) -> Vec { + #[cfg(feature = "metrics")] + let scan_start = std::time::Instant::now(); + + let mut nodes: Vec<(Node, H256)> = vec![]; + + for (contact_id, contact) in self.iter_contacts() { + let dist = xor_distance(&node_id, contact_id); + if nodes.len() < MAX_NODES_IN_NEIGHBORS_PACKET { + nodes.push((contact.node.clone(), dist)); + } else if let Some((farthest_idx, _)) = + nodes.iter().enumerate().max_by_key(|(_, (_, d))| *d) + && dist < nodes[farthest_idx].1 + { + nodes[farthest_idx] = (contact.node.clone(), dist); + } + } + + #[cfg(feature = "metrics")] + { + use ethrex_metrics::p2p::METRICS_P2P; + METRICS_P2P.observe_iter_contacts_duration(scan_start.elapsed().as_secs_f64()); + } + + nodes.into_iter().map(|(node, _)| node).collect() + } + + /// Get nodes at distances for discv5 (returns Vec). + /// Uses the discv5 spec log-distance: `floor(log2(XOR))` for non-zero XOR. + /// Distance 0 is reserved for the local node itself (handled by the caller), + /// so contacts start at distance >= 1. + pub fn nodes_at_distances(&self, distances: &[u32]) -> Vec { + self.iter_contacts() + .filter_map(|(contact_id, contact)| { + let dist = distance(&self.local_node_id, contact_id) as u32; + if distances.contains(&dist) { + contact.record.clone() + } else { + None + } + }) + .take(MAX_ENRS_PER_FINDNODE_RESPONSE) + .collect() + } + + pub async fn new_contacts(&mut self, nodes: Vec, protocol: DiscoveryProtocol) { + for node in nodes { + let node_id = node.node_id(); + if node_id == self.local_node_id { + continue; + } + #[cfg(feature = "metrics")] + let insert_start = std::time::Instant::now(); + + // Always add to the connection pool (regardless of k-bucket capacity) + self.insert_to_connection_pool(node_id, node.clone()); + + if self.contact_exists(&node_id) { + // Contact already exists (main or replacement list), update protocol + if let Some(contact) = self.get_contact_or_replacement_mut(&node_id) { + contact.add_protocol(protocol); + } + } else { + let contact = Contact::new(node, protocol); + self.insert_contact(node_id, contact); + METRICS.record_new_discovery().await; + } + + #[cfg(feature = "metrics")] + { + use ethrex_metrics::p2p::METRICS_P2P; + METRICS_P2P.observe_insert_contact_duration(insert_start.elapsed().as_secs_f64()); + } + } + } + + pub fn record_enr_request_sent(&mut self, node_id: H256, request_hash: H256) { + if let Some(contact) = self.get_contact_mut(&node_id) { + contact.record_enr_request_sent(request_hash); + } + } + + pub fn record_enr_response_received( + &mut self, + node_id: H256, + request_hash: H256, + record: NodeRecord, + ) { + // Filtered here, before the mutable borrow, so a record that reaches us + // over discv4 is judged by the same filter as one that arrives over + // discv5. The verdict is recorded only if the record was actually + // stored, so it always describes the record the contact holds. + let passes_filter = self.filter.accepts(&record); + if let Some(contact) = self.get_contact_mut(&node_id) + && contact.record_enr_response_received(request_hash, record) + { + contact.passes_filter = Some(passes_filter); + } + } + + pub async fn new_contact_records(&mut self, node_records: Vec) { + for node_record in node_records { + if !node_record.verify_signature() { + continue; + } + if let Ok(node) = Node::from_enr(&node_record) { + let node_id = node.node_id(); + if node_id == self.local_node_id { + continue; + } + + // Always add to the connection pool (regardless of k-bucket capacity) + self.insert_to_connection_pool(node_id, node.clone()); + + if self.contact_exists(&node_id) { + // Check if we need to evaluate fork_id before taking + // the mutable borrow. + let should_update = self + .get_contact_or_replacement(&node_id) + .map(|c| match c.record.as_ref() { + None => true, + Some(r) => node_record.seq > r.seq, + }) + .unwrap_or(false); + // Filtered here, before the mutable borrow, and only when + // the record is newer than the one we already hold. + let passes_filter = should_update.then(|| self.filter.accepts(&node_record)); + if let Some(contact) = self.get_contact_or_replacement_mut(&node_id) { + contact.add_protocol(DiscoveryProtocol::Discv5); + if should_update { + if contact.node.ip != node.ip || contact.node.udp_port != node.udp_port + { + contact.validation_timestamp = None; + contact.ping_id = None; + } + contact.node = node; + contact.record = Some(node_record); + contact.passes_filter = passes_filter; + } + } + } else { + let passes_filter = self.filter.accepts(&node_record); + let mut contact = Contact::new(node, DiscoveryProtocol::Discv5); + contact.passes_filter = Some(passes_filter); + contact.record = Some(node_record); + self.insert_contact(node_id, contact); + METRICS.record_new_discovery().await; + } + } + } + } + + fn is_validation_needed(contact: &Contact, revalidation_interval: Duration) -> bool { + if contact.disposable { + return false; + } + + let sent_ping_ttl = Duration::from_secs(30); + + if contact.has_pending_ping() { + // Outstanding ping — only re-ping if it timed out (stale). + contact + .validation_timestamp + .map(|ts| Instant::now().saturating_duration_since(ts) > sent_ping_ttl) + .unwrap_or(false) + } else { + // No pending ping — check if never validated or validation expired. + !contact.was_validated() + || contact + .validation_timestamp + .map(|ts| Instant::now().saturating_duration_since(ts) > revalidation_interval) + .unwrap_or(false) + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::types::NodeRecordPairs; + use ethrex_common::H512; + use std::net::Ipv4Addr; + + /// Helper: build a dummy contact with a unique node derived from `seed`. + fn dummy_contact(seed: u8) -> (H256, Contact) { + let pk = H512::from_low_u64_be(seed as u64 + 1); + let node = Node::new(IpAddr::V4(Ipv4Addr::new(127, 0, 0, seed)), 30303, 30303, pk); + let node_id = node.node_id(); + let contact = Contact::new(node, DiscoveryProtocol::Discv4); + (node_id, contact) + } + + /// A filter with a fixed answer, so peer-table behaviour can be exercised + /// without a storage engine or a real chain behind it. + struct FixedAnswer(bool); + + impl PeerFilter for FixedAnswer { + fn accepts(&self, _record: &NodeRecord) -> bool { + self.0 + } + } + + fn table_with(filter: impl PeerFilter + 'static) -> ContactTable { + ContactTable::new(H256::zero(), 10, Box::new(filter)) + } + + /// A signed record for `seed`'s node at sequence number `seq`. + fn record_for(seed: u8, seq: u64) -> (H256, NodeRecord) { + let signer = secp256k1::SecretKey::from_slice(&[seed.max(1); 32]).unwrap(); + let record = NodeRecord::from_pairs( + seq, + &signer, + NodeRecordPairs { + ip: Some(Ipv4Addr::new(127, 0, 0, seed)), + udp_port: Some(30303), + ..Default::default() + }, + ) + .unwrap(); + (Node::from_enr(&record).unwrap().node_id(), record) + } + + // --- the filter decides which contacts are dialable --- + + #[tokio::test] + async fn an_arriving_record_is_run_through_the_filter() { + let mut table = table_with(FixedAnswer(false)); + let (node_id, record) = record_for(1, 1); + + table.new_contact_records(vec![record]).await; + + let contact = table.get_contact(&node_id).expect("contact inserted"); + assert_eq!(contact.passes_filter, Some(false)); + } + + #[tokio::test] + async fn a_rejected_contact_is_never_offered_for_dialing() { + let mut table = table_with(FixedAnswer(false)); + let (node_id, record) = record_for(2, 1); + + table.new_contact_records(vec![record]).await; + assert!(table.get_contact(&node_id).is_some(), "contact is present"); + + assert!( + table.next_dial_candidate().is_none(), + "a rejected contact must not be handed out to dial" + ); + } + + #[tokio::test] + async fn an_accepted_contact_is_offered_for_dialing() { + let mut table = table_with(FixedAnswer(true)); + let (node_id, record) = record_for(3, 1); + + table.new_contact_records(vec![record]).await; + + // Asserting the stored answer too, not just dialability: `None` is also + // dialable, so `is_some()` alone would pass even if the filter never ran. + assert_eq!( + table.get_contact(&node_id).unwrap().passes_filter, + Some(true) + ); + assert!(table.next_dial_candidate().is_some()); + } + + #[tokio::test] + async fn a_contact_discovered_without_a_record_is_never_filtered() { + // Bootnodes and discv4 neighbours arrive as bare endpoints. They have + // published nothing to judge, so they must stay dialable rather than be + // written off by a filter that never saw them. + let mut table = table_with(FixedAnswer(false)); + let (node_id, record) = record_for(6, 1); + let node = Node::from_enr(&record).unwrap(); + + table + .new_contacts(vec![node], DiscoveryProtocol::Discv4) + .await; + + assert_eq!(table.get_contact(&node_id).unwrap().passes_filter, None); + assert!(table.next_dial_candidate().is_some()); + } + + #[tokio::test] + async fn a_discv4_enr_response_is_run_through_the_filter() { + // The discv4 path used to bypass the filter entirely and write a + // hardcoded fork-id verdict into the same field, so a consumer's own + // policy was overridden depending on which protocol found the peer. + let mut table = table_with(FixedAnswer(false)); + let (node_id, record) = record_for(7, 1); + let node = Node::from_enr(&record).unwrap(); + let request_hash = H256::repeat_byte(0xab); + + table + .new_contacts(vec![node], DiscoveryProtocol::Discv4) + .await; + table.record_enr_request_sent(node_id, request_hash); + table.record_enr_response_received(node_id, request_hash, record); + + assert_eq!( + table.get_contact(&node_id).unwrap().passes_filter, + Some(false) + ); + } + + #[tokio::test] + async fn an_unsolicited_enr_response_does_not_set_the_verdict() { + // The record is not stored when the hash does not match, so recording a + // verdict from it would let a peer restate its own standing from a + // record the table refused to keep. + let mut table = table_with(FixedAnswer(true)); + let (node_id, record) = record_for(8, 1); + let node = Node::from_enr(&record).unwrap(); + + table + .new_contacts(vec![node], DiscoveryProtocol::Discv4) + .await; + table.record_enr_request_sent(node_id, H256::repeat_byte(0x01)); + table.record_enr_response_received(node_id, H256::repeat_byte(0x02), record); + + let contact = table.get_contact(&node_id).unwrap(); + assert_eq!(contact.passes_filter, None); + assert!(contact.record.is_none(), "the record must not be stored"); + } + + /// Rejects the first record it is shown and accepts every later one, so a + /// test can tell whether a second record was filtered at all. + #[derive(Default)] + struct AcceptsFromTheSecondRecordOn(std::sync::atomic::AtomicUsize); + + impl PeerFilter for AcceptsFromTheSecondRecordOn { + fn accepts(&self, _record: &NodeRecord) -> bool { + self.0.fetch_add(1, std::sync::atomic::Ordering::SeqCst) > 0 + } + } + + #[tokio::test] + async fn a_rejection_is_reconsidered_on_a_newer_record() { + // The reason a rejection is stored rather than acted on once: the peer + // republishes and we look again, instead of writing it off for the life + // of the process over a fork id read against a head we had not synced. + let mut table = table_with(AcceptsFromTheSecondRecordOn::default()); + let (node_id, first) = record_for(4, 1); + let (_, newer) = record_for(4, 2); + + table.new_contact_records(vec![first]).await; + assert_eq!( + table.get_contact(&node_id).unwrap().passes_filter, + Some(false) + ); + + table.new_contact_records(vec![newer]).await; + assert_eq!( + table.get_contact(&node_id).unwrap().passes_filter, + Some(true), + "a higher-seq record must get a fresh hearing" + ); + } + + #[tokio::test] + async fn an_older_record_does_not_re_filter_the_contact() { + // `should_update` false means the record has nothing new to say, so the + // answer already on the contact has to survive it. + let mut table = table_with(AcceptsFromTheSecondRecordOn::default()); + let (node_id, first) = record_for(5, 2); + let (_, older) = record_for(5, 1); + + table.new_contact_records(vec![first]).await; + table.new_contact_records(vec![older]).await; + + assert_eq!( + table.get_contact(&node_id).unwrap().passes_filter, + Some(false), + "a stale record must not overwrite the answer we hold" + ); + } + + // --- Consumer lifecycle --- + + #[tokio::test] + async fn a_connected_contact_is_not_offered_for_dialing_again() { + let mut table = table_with(FixedAnswer(true)); + let (node_id, record) = record_for(9, 1); + + table.new_contact_records(vec![record]).await; + table.mark_connected(node_id); + + assert!( + table.next_dial_candidate().is_none(), + "a node the consumer is already connected to must not be dialed again" + ); + } + + #[tokio::test] + async fn a_disconnected_contact_becomes_dialable_again() { + let mut table = table_with(FixedAnswer(true)); + let (node_id, record) = record_for(10, 1); + + table.new_contact_records(vec![record]).await; + table.mark_connected(node_id); + table.mark_disconnected(&node_id); + + assert_eq!( + table.next_dial_candidate().map(|n| n.node_id()), + Some(node_id) + ); + } + + #[tokio::test] + async fn disconnecting_drops_the_discv5_session() { + // The keys were negotiated for a peer we are no longer talking to, and + // holding them would grow the session store with every peer that ever + // connected. + let mut table = table_with(FixedAnswer(true)); + let (node_id, record) = record_for(11, 1); + + table.new_contact_records(vec![record]).await; + table.set_session( + node_id, + Session { + outbound_key: [1; 16], + inbound_key: [2; 16], + }, + ); + assert!(table.session(&node_id).is_some()); + + table.mark_disconnected(&node_id); + + assert!(table.session(&node_id).is_none()); + } + + #[tokio::test] + async fn a_candidate_is_offered_once_per_cycle() { + // `already_tried_peers` is what stops a failed dial from being retried + // immediately, and it has to clear once the pool is exhausted or a peer + // that failed once would never be tried again. + let mut table = table_with(FixedAnswer(true)); + let (node_id, record) = record_for(12, 1); + + table.new_contact_records(vec![record]).await; + + assert_eq!( + table.next_dial_candidate().map(|n| n.node_id()), + Some(node_id) + ); + assert!( + table.next_dial_candidate().is_none(), + "the same candidate must not be handed out twice in one cycle" + ); + assert_eq!( + table.next_dial_candidate().map(|n| n.node_id()), + Some(node_id), + "the exhausted cycle resets, so the candidate comes back around" + ); + } + + #[test] + fn peer_completion_tracks_the_connected_count() { + let mut table = table_with(FixedAnswer(true)); + assert_eq!(table.peer_completion(), 0.0); + + for seed in 0..5u8 { + table.mark_connected(H256::from_low_u64_be(seed as u64 + 1)); + } + + // `table_with` targets 10 peers. + assert_eq!(table.peer_completion(), 0.5); + } + + #[test] + fn peer_completion_is_complete_when_nothing_is_wanted() { + // A consumer that asks for no peers is always done, and must not divide + // by zero to find that out. + let table = ContactTable::new(H256::zero(), 0, Box::new(FixedAnswer(true))); + assert_eq!(table.peer_completion(), 1.0); + } + + // --- KBucket::insert --- + + #[test] + fn insert_into_empty_bucket() { + let mut bucket = KBucket::default(); + let (id, contact) = dummy_contact(1); + assert!(bucket.insert(id, contact)); + assert_eq!(bucket.contacts.len(), 1); + assert!(bucket.replacements.is_empty()); + } + + #[test] + fn insert_fills_bucket_then_goes_to_replacements() { + let mut bucket = KBucket::default(); + + // Fill the main list to capacity. + for i in 0..MAX_NODES_PER_BUCKET as u8 { + let (id, contact) = dummy_contact(i); + assert!(bucket.insert(id, contact), "contact {i} should go to main"); + } + assert_eq!(bucket.contacts.len(), MAX_NODES_PER_BUCKET); + + // The next insert should go to the replacement list. + let (id, contact) = dummy_contact(200); + assert!(!bucket.insert(id, contact)); + assert_eq!(bucket.contacts.len(), MAX_NODES_PER_BUCKET); + assert_eq!(bucket.replacements.len(), 1); + } + + // --- KBucket::contains --- + + #[test] + fn contains_checks_main_and_replacement() { + let mut bucket = KBucket::default(); + + let (id_main, contact_main) = dummy_contact(1); + bucket.insert(id_main, contact_main); + assert!(bucket.contains(&id_main)); + + // Fill bucket so next goes to replacement. + for i in 2..=(MAX_NODES_PER_BUCKET as u8) { + let (id, c) = dummy_contact(i); + bucket.insert(id, c); + } + let (id_repl, contact_repl) = dummy_contact(100); + bucket.insert(id_repl, contact_repl); + + assert!(bucket.contains(&id_repl)); + assert!(!bucket.contains(&H256::zero())); + } + + // --- KBucket::get / get_any --- + + #[test] + fn get_returns_main_list_only() { + let mut bucket = KBucket::default(); + let (id, contact) = dummy_contact(1); + bucket.insert(id, contact); + assert!(bucket.get(&id).is_some()); + assert!(bucket.get(&H256::zero()).is_none()); + } + + #[test] + fn get_any_returns_from_replacement() { + let mut bucket = KBucket::default(); + // Fill main list. + for i in 0..MAX_NODES_PER_BUCKET as u8 { + let (id, c) = dummy_contact(i); + bucket.insert(id, c); + } + // Insert into replacements. + let (id_repl, c_repl) = dummy_contact(200); + bucket.insert(id_repl, c_repl); + + assert!(bucket.get(&id_repl).is_none()); // not in main + assert!(bucket.get_any(&id_repl).is_some()); // found via replacement + } + + // --- KBucket::remove_and_promote --- + + #[test] + fn remove_and_promote_with_replacement() { + let mut bucket = KBucket::default(); + + // Fill main list. + let mut main_ids = Vec::new(); + for i in 0..MAX_NODES_PER_BUCKET as u8 { + let (id, c) = dummy_contact(i); + main_ids.push(id); + bucket.insert(id, c); + } + + // Add a replacement. + let (repl_id, repl_contact) = dummy_contact(200); + bucket.insert(repl_id, repl_contact); + + // Remove a main contact — the replacement should be promoted. + let promoted = bucket.remove_and_promote(&main_ids[0]); + assert_eq!(promoted, Some(repl_id)); + assert_eq!(bucket.contacts.len(), MAX_NODES_PER_BUCKET); + assert!(bucket.replacements.is_empty()); + assert!(!bucket.contains(&main_ids[0])); + assert!(bucket.contains(&repl_id)); + } + + #[test] + fn remove_and_promote_without_replacement() { + let mut bucket = KBucket::default(); + let (id, c) = dummy_contact(1); + bucket.insert(id, c); + + let promoted = bucket.remove_and_promote(&id); + assert!(promoted.is_none()); + assert!(bucket.contacts.is_empty()); + } + + #[test] + fn remove_nonexistent_returns_none() { + let mut bucket = KBucket::default(); + assert!(bucket.remove_and_promote(&H256::zero()).is_none()); + } + + // --- Replacement eviction --- + + #[test] + fn replacement_list_evicts_oldest_when_full() { + let mut bucket = KBucket::default(); + // Fill main list. + for i in 0..MAX_NODES_PER_BUCKET as u8 { + let (id, c) = dummy_contact(i); + bucket.insert(id, c); + } + + // Fill replacement list beyond capacity. + let mut repl_ids = Vec::new(); + for i in 0..(MAX_REPLACEMENTS_PER_BUCKET + 2) as u8 { + let seed = 100 + i; + let (id, c) = dummy_contact(seed); + repl_ids.push(id); + bucket.insert(id, c); + } + + assert_eq!(bucket.replacements.len(), MAX_REPLACEMENTS_PER_BUCKET); + // The oldest two should have been evicted. + assert!(!bucket.contains(&repl_ids[0])); + assert!(!bucket.contains(&repl_ids[1])); + // The most recent ones should still be there. + assert!(bucket.contains(repl_ids.last().unwrap())); + } + + // --- bucket_index --- + + #[test] + fn bucket_index_self_is_none() { + let id = H256::random(); + assert_eq!(bucket_index(&id, &id), None); + } + + #[test] + fn bucket_index_minimal_distance() { + let local = H256::zero(); + // XOR distance = 1 → highest bit is bit 0 → bucket 0 + let mut remote = H256::zero(); + remote.0[31] = 1; + assert_eq!(bucket_index(&local, &remote), Some(0)); + } + + #[test] + fn bucket_index_maximal_distance() { + let local = H256::zero(); + // XOR distance has highest bit at position 255 → bucket 255 + let mut remote = H256::zero(); + remote.0[0] = 0x80; + assert_eq!(bucket_index(&local, &remote), Some(255)); + } +} diff --git a/crates/networking/p2p/discovery/discv4_handlers.rs b/crates/networking/p2p/discovery/discv4_handlers.rs index e85df5cc624..cd38877ed77 100644 --- a/crates/networking/p2p/discovery/discv4_handlers.rs +++ b/crates/networking/p2p/discovery/discv4_handlers.rs @@ -1,4 +1,5 @@ use crate::{ + discovery::contact_table::{Contact, ContactValidation, DiscoveryProtocol}, discovery::lookup::{IterativeLookup, LOOKUP_ALPHA, LOOKUP_BUCKET_SIZE}, discv4::{ messages::{ @@ -8,7 +9,6 @@ use crate::{ server::{Discv4Message, EXPIRATION_SECONDS}, }, metrics::METRICS, - peer_table::{Contact, ContactValidation, DiscoveryProtocol, PeerTableServerProtocol as _}, types::{Endpoint, Node}, utils::{ get_msg_expiration_from_seconds, is_msg_expired, node_id, public_key_from_signing_key, @@ -114,11 +114,11 @@ impl DiscoveryServer { pub(crate) async fn discv4_revalidate(&mut self) -> Result<(), DiscoveryServerError> { if let Some(contact) = self - .peer_table - .get_contact_to_revalidate(REVALIDATION_INTERVAL, DiscoveryProtocol::Discv4) - .await? + .contacts + .contact_to_revalidate(REVALIDATION_INTERVAL, DiscoveryProtocol::Discv4) { - self.discv4_send_ping(&contact.node).await?; + let node = contact.node.clone(); + self.discv4_send_ping(&node).await?; } Ok(()) } @@ -156,9 +156,8 @@ impl DiscoveryServer { // Seed with closest known nodes from the connection pool let seed = self - .peer_table - .get_closest_from_pool(target_id, LOOKUP_BUCKET_SIZE) - .await?; + .contacts + .closest_from_pool(target_id, LOOKUP_BUCKET_SIZE); if seed.is_empty() { trace!( protocol = "discv4", @@ -216,7 +215,7 @@ impl DiscoveryServer { for (idx, node_id, node, message) in queries { if let Err(e) = self.udp_socket.send_to(&message, &node.udp_addr()).await { debug!(protocol = "discv4", sending = "FindNode", addr = ?node.udp_addr(), err=?e, "Error sending message"); - self.peer_table.set_disposable(node_id)?; + self.contacts.set_disposable(&node_id); METRICS.record_new_discarded_node(); if let Some(discv4) = &mut self.discv4 && let Some((lookup, _)) = discv4.active_lookups.get_mut(idx) @@ -240,8 +239,9 @@ impl DiscoveryServer { } pub(crate) async fn discv4_enr_lookup(&mut self) -> Result<(), DiscoveryServerError> { - if let Some(contact) = self.peer_table.get_contact_for_enr_lookup().await? { - self.discv4_send_enr_request(&contact.node).await?; + if let Some(contact) = self.contacts.contact_for_enr_lookup() { + let node = contact.node.clone(); + self.discv4_send_enr_request(&node).await?; } Ok(()) } @@ -267,12 +267,12 @@ impl DiscoveryServer { trace!(protocol = "discv4", sent = "Ping", to = %format!("{:#x}", node.public_key)); METRICS.record_ping_sent().await; let ping_id = Bytes::copy_from_slice(ping_hash.as_bytes()); - self.peer_table.record_ping_sent(node.node_id(), ping_id)?; + self.contacts.record_ping_sent(&node.node_id(), ping_id); Ok(()) } async fn discv4_send_pong( - &self, + &mut self, ping_hash: H256, node: &Node, ) -> Result<(), DiscoveryServerError> { @@ -290,7 +290,7 @@ impl DiscoveryServer { } async fn discv4_send_neighbors( - &self, + &mut self, neighbors: Vec, node: &Node, ) -> Result<(), DiscoveryServerError> { @@ -305,13 +305,13 @@ impl DiscoveryServer { let expiration: u64 = get_msg_expiration_from_seconds(EXPIRATION_SECONDS); let enr_request = Message::ENRRequest(ENRRequestMessage { expiration }); let enr_request_hash = self.discv4_send_else_dispose(enr_request, node).await?; - self.peer_table - .record_enr_request_sent(node.node_id(), enr_request_hash)?; + self.contacts + .record_enr_request_sent(node.node_id(), enr_request_hash); Ok(()) } async fn discv4_send_enr_response( - &self, + &mut self, request_hash: H256, from: std::net::SocketAddr, ) -> Result<(), DiscoveryServerError> { @@ -331,19 +331,17 @@ impl DiscoveryServer { self.discv4_send_pong(hash, &node).await?; if self - .peer_table + .contacts .insert_if_new(node.clone(), DiscoveryProtocol::Discv4) .await - .unwrap_or(false) { self.discv4_send_ping(&node).await?; } else { let node_id = node_id(&sender_public_key); let stored_enr_seq = self - .peer_table - .get_contact(node_id) - .await? - .and_then(|c| c.record) + .contacts + .get_contact(&node_id) + .and_then(|c| c.record.as_ref()) .map(|r| r.seq); let received_enr_seq = ping_message.enr_seq; @@ -362,19 +360,24 @@ impl DiscoveryServer { message: PongMessage, node_id: H256, ) -> Result<(), DiscoveryServerError> { - let Some(contact) = self.peer_table.get_contact(node_id).await? else { + // Everything needed from the contact is copied out before the mutable + // borrows below: the table is plain state now, not an actor behind a + // channel, so a live `&Contact` would pin it for the rest of the call. + let Some((node, stored_enr_seq)) = self + .contacts + .get_contact(&node_id) + .map(|c| (c.node.clone(), c.record.as_ref().map(|r| r.seq))) + else { return Ok(()); }; let ping_id = Bytes::copy_from_slice(message.ping_hash.as_bytes()); - self.peer_table.record_pong_received(node_id, ping_id)?; - - let stored_enr_seq = contact.record.map(|r| r.seq); + self.contacts.record_pong_received(&node_id, &ping_id); let received_enr_seq = message.enr_seq; if let (Some(received), Some(stored)) = (received_enr_seq, stored_enr_seq) && received > stored { - self.discv4_send_enr_request(&contact.node).await?; + self.discv4_send_enr_request(&node).await?; } Ok(()) @@ -392,7 +395,7 @@ impl DiscoveryServer { .await { let target_id = node_id(&target); - let neighbors = self.peer_table.get_closest_nodes(target_id).await?; + let neighbors = self.contacts.closest_nodes(target_id); for chunk in neighbors.chunks(8) { let _ = self @@ -429,8 +432,9 @@ impl DiscoveryServer { } let nodes = neighbors_message.nodes; - self.peer_table - .new_contacts(nodes.clone(), DiscoveryProtocol::Discv4)?; + self.contacts + .new_contacts(nodes.clone(), DiscoveryProtocol::Discv4) + .await; // Feed results into ALL active lookups (but don't advance — the timer // drives lookup progress so that traffic stays controlled). @@ -468,7 +472,7 @@ impl DiscoveryServer { return Ok(()); } - self.peer_table.mark_knows_us(node_id)?; + self.contacts.mark_knows_us(&node_id); Ok(()) } @@ -488,11 +492,11 @@ impl DiscoveryServer { return Ok(()); } - self.peer_table.record_enr_response_received( + self.contacts.record_enr_response_received( node_id, enr_response_message.request_hash, enr_response_message.node_record.clone(), - )?; + ); Ok(()) } @@ -504,7 +508,7 @@ impl DiscoveryServer { from: std::net::SocketAddr, message_type: &str, ) -> Result { - match self.peer_table.validate_contact(node_id, from.ip()).await? { + match self.contacts.validate_contact(node_id, from.ip()) { ContactValidation::UnknownContact => { debug!(protocol = "discv4", received = message_type, to = %format!("{sender_public_key:#x}"), "Unknown contact, skipping"); Err(DiscoveryServerError::InvalidContact) @@ -538,7 +542,7 @@ impl DiscoveryServer { } async fn discv4_send( - &self, + &mut self, message: Message, addr: std::net::SocketAddr, ) -> Result { @@ -571,7 +575,7 @@ impl DiscoveryServer { .expect("first 32 bytes are the message hash"); if let Err(e) = self.udp_socket.send_to(&buf, node.udp_addr()).await { debug!(protocol = "discv4", sending = ?message, addr = ?node.udp_addr(), to = ?node.node_id(), err=?e, "Error sending message"); - self.peer_table.set_disposable(node.node_id())?; + self.contacts.set_disposable(&node.node_id()); METRICS.record_new_discarded_node(); return Err(e.into()); } diff --git a/crates/networking/p2p/discovery/discv5_handlers.rs b/crates/networking/p2p/discovery/discv5_handlers.rs index e1d43e922bc..bd31f8c79c8 100644 --- a/crates/networking/p2p/discovery/discv5_handlers.rs +++ b/crates/networking/p2p/discovery/discv5_handlers.rs @@ -1,4 +1,5 @@ use crate::{ + discovery::contact_table::{ContactValidation, DiscoveryProtocol}, discovery::lookup::{IterativeLookup, LOOKUP_ALPHA, LOOKUP_BUCKET_SIZE}, discv5::{ messages::{ @@ -12,7 +13,6 @@ use crate::{ }, }, metrics::METRICS, - peer_table::{ContactValidation, DiscoveryProtocol, PeerTableServerProtocol as _}, rlpx::utils::compress_pubkey, types::{Node, NodeRecord}, utils::{distance, node_id}, @@ -73,11 +73,7 @@ impl DiscoveryServer { // (an unauthenticated single-packet DoS of the discv5 actor). let src_id = Ordinary::src_id(&packet)?; - let decrypt_key = self - .peer_table - .get_session_info(src_id) - .await? - .map(|s| s.inbound_key); + let decrypt_key = self.contacts.session(&src_id).map(|s| s.inbound_key); let discv5 = self.discv5.as_mut().expect("discv5 state must exist"); @@ -169,7 +165,7 @@ impl DiscoveryServer { &node.node_id(), ); - self.peer_table.set_session_info(node.node_id(), session)?; + self.contacts.set_session(node.node_id(), session); let whoareyou = WhoAreYou::decode(&packet)?; let record = (self.local_node_record.seq != whoareyou.enr_seq) @@ -196,7 +192,7 @@ impl DiscoveryServer { DiscoveryServerError::CryptographyError("Invalid ephemeral pubkey".into()) })?; - let src_pubkey = if let Some(contact) = self.peer_table.get_contact(src_id).await? { + let src_pubkey = if let Some(contact) = self.contacts.get_contact(&src_id) { compress_pubkey(contact.node.public_key) } else if let Some(record) = &authdata.record { if !record.verify_signature() { @@ -243,7 +239,9 @@ impl DiscoveryServer { } if let Some(record) = &authdata.record { - self.peer_table.new_contact_records(vec![record.clone()])?; + self.contacts + .new_contact_records(vec![record.clone()]) + .await; } let session = derive_session_keys( @@ -255,7 +253,7 @@ impl DiscoveryServer { false, ); - self.peer_table.set_session_info(src_id, session.clone())?; + self.contacts.set_session(src_id, session.clone()); let discv5 = self.discv5.as_mut().expect("discv5 state must exist"); discv5.session_ips.insert( src_id, @@ -277,12 +275,13 @@ impl DiscoveryServer { pub(crate) async fn discv5_revalidate(&mut self) -> Result<(), DiscoveryServerError> { if let Some(contact) = self - .peer_table - .get_contact_to_revalidate(REVALIDATION_INTERVAL, DiscoveryProtocol::Discv5) - .await? - && let Err(e) = self.discv5_send_ping(&contact.node).await + .contacts + .contact_to_revalidate(REVALIDATION_INTERVAL, DiscoveryProtocol::Discv5) { - trace!(protocol = "discv5", node = %contact.node.node_id(), err = ?e, "Failed to send revalidation PING"); + let node = contact.node.clone(); + if let Err(e) = self.discv5_send_ping(&node).await { + trace!(protocol = "discv5", node = %node.node_id(), err = ?e, "Failed to send revalidation PING"); + } } Ok(()) } @@ -318,9 +317,8 @@ impl DiscoveryServer { // Seed with closest known nodes from the connection pool let seed = self - .peer_table - .get_closest_from_pool(target_id, LOOKUP_BUCKET_SIZE) - .await?; + .contacts + .closest_from_pool(target_id, LOOKUP_BUCKET_SIZE); if seed.is_empty() { trace!( protocol = "discv5", @@ -365,7 +363,7 @@ impl DiscoveryServer { let find_node_msg = self.discv5_build_find_node_for_target(target, &node); if let Err(e) = self.discv5_send_ordinary(find_node_msg, &node).await { debug!(protocol = "discv5", sending = "FindNode", addr = ?node.udp_addr(), err=?e, "Error sending message"); - self.peer_table.set_disposable(node_id)?; + self.contacts.set_disposable(&node_id); METRICS.record_new_discarded_node(); if let Some(discv5) = &mut self.discv5 && let Some(lookup) = discv5.active_lookups.get_mut(idx) @@ -411,9 +409,12 @@ impl DiscoveryServer { }); if outbound_key.is_none() - && let Some(contact) = self.peer_table.get_contact(sender_id).await? + && let Some(node) = self + .contacts + .get_contact(&sender_id) + .map(|c| c.node.clone()) { - return self.discv5_send_ordinary(pong, &contact.node).await; + return self.discv5_send_ordinary(pong, &node).await; } let key = self .discv5_resolve_outbound_key(&sender_id, outbound_key) @@ -429,25 +430,29 @@ impl DiscoveryServer { pong_message: PongMessage, sender_id: H256, ) -> Result<(), DiscoveryServerError> { - self.peer_table - .record_pong_received(sender_id, pong_message.req_id)?; - - if let Some(contact) = self.peer_table.get_contact(sender_id).await? { - let cached_seq = contact.record.as_ref().map_or(0, |r| r.seq); - if pong_message.enr_seq > cached_seq { - trace!( - protocol = "discv5", - from = %sender_id, - cached_seq, - pong_seq = pong_message.enr_seq, - "ENR seq mismatch, requesting updated ENR (FINDNODE distance 0)" - ); - let find_node = Message::FindNode(FindNodeMessage { - req_id: generate_req_id(), - distances: vec![0], - }); - self.discv5_send_ordinary(find_node, &contact.node).await?; - } + self.contacts + .record_pong_received(&sender_id, &pong_message.req_id); + + // Copied out rather than held: the table is plain state, so a live + // `&Contact` would block the `&mut self` send below. + if let Some((node, cached_seq)) = self + .contacts + .get_contact(&sender_id) + .map(|c| (c.node.clone(), c.record.as_ref().map_or(0, |r| r.seq))) + && pong_message.enr_seq > cached_seq + { + trace!( + protocol = "discv5", + from = %sender_id, + cached_seq, + pong_seq = pong_message.enr_seq, + "ENR seq mismatch, requesting updated ENR (FINDNODE distance 0)" + ); + let find_node = Message::FindNode(FindNodeMessage { + req_id: generate_req_id(), + distances: vec![0], + }); + self.discv5_send_ordinary(find_node, &node).await?; } let discv5 = self.discv5.as_mut().expect("discv5 state must exist"); @@ -478,11 +483,7 @@ impl DiscoveryServer { sender_addr: SocketAddr, outbound_key: Option<[u8; 16]>, ) -> Result<(), DiscoveryServerError> { - let send_to_contact = match self - .peer_table - .validate_contact(sender_id, sender_addr.ip()) - .await? - { + let send_to_contact = match self.contacts.validate_contact(sender_id, sender_addr.ip()) { ContactValidation::Valid(contact) => Some(*contact), ContactValidation::UnknownContact => None, reason => { @@ -492,9 +493,8 @@ impl DiscoveryServer { }; let mut nodes = self - .peer_table - .get_nodes_at_distances(find_node_message.distances.clone()) - .await?; + .contacts + .nodes_at_distances(&find_node_message.distances); if find_node_message.distances.contains(&0) { nodes.push(self.local_node_record.clone()); } @@ -541,8 +541,9 @@ impl DiscoveryServer { &mut self, nodes_message: NodesMessage, ) -> Result<(), DiscoveryServerError> { - self.peer_table - .new_contact_records(nodes_message.nodes.clone())?; + self.contacts + .new_contact_records(nodes_message.nodes.clone()) + .await; // Feed results into ALL active lookups (but don't advance — the timer // drives lookup progress so that traffic stays controlled). @@ -572,7 +573,7 @@ impl DiscoveryServer { }); self.discv5_send_ordinary(ping, node).await?; - self.peer_table.record_ping_sent(node.node_id(), req_id)?; + self.contacts.record_ping_sent(&node.node_id(), req_id); Ok(()) } @@ -591,7 +592,7 @@ impl DiscoveryServer { src_id: self.local_node.node_id(), message: message.clone(), }; - let encrypt_key = match self.peer_table.get_session_info(node.node_id()).await? { + let encrypt_key = match self.contacts.session(&node.node_id()) { Some(s) => s.outbound_key, None => { trace!( @@ -620,14 +621,14 @@ impl DiscoveryServer { } async fn discv5_resolve_outbound_key( - &self, + &mut self, node_id: &H256, key: Option<[u8; 16]>, ) -> Result<[u8; 16], DiscoveryServerError> { if let Some(key) = key { return Ok(key); } - match self.peer_table.get_session_info(*node_id).await? { + match self.contacts.session(node_id) { Some(s) => Ok(s.outbound_key), None => { trace!( @@ -688,7 +689,7 @@ impl DiscoveryServer { record, message: message.clone(), }; - let encrypt_key = match self.peer_table.get_session_info(node.node_id()).await? { + let encrypt_key = match self.contacts.session(&node.node_id()) { Some(s) => s.outbound_key, None => { trace!( @@ -781,9 +782,8 @@ impl DiscoveryServer { let mut rng = OsRng; let enr_seq = self - .peer_table - .get_contact(src_id) - .await? + .contacts + .get_contact(&src_id) .map_or(0, |c| c.record.as_ref().map_or(0, |r| r.seq)); let who_are_you = WhoAreYou { @@ -815,7 +815,7 @@ impl DiscoveryServer { } async fn discv5_send_packet( - &self, + &mut self, packet: &Packet, dest_id: &H256, addr: SocketAddr, diff --git a/crates/networking/p2p/discovery/lookup.rs b/crates/networking/p2p/discovery/lookup.rs index 5401bd24233..4a081a68ccf 100644 --- a/crates/networking/p2p/discovery/lookup.rs +++ b/crates/networking/p2p/discovery/lookup.rs @@ -1,4 +1,4 @@ -use crate::peer_table::xor_distance; +use crate::discovery::contact_table::xor_distance; use crate::types::Node; use ethrex_common::H256; use rustc_hash::FxHashSet; diff --git a/crates/networking/p2p/discovery/mod.rs b/crates/networking/p2p/discovery/mod.rs index d3fb7e2fbe1..306f90dd1bd 100644 --- a/crates/networking/p2p/discovery/mod.rs +++ b/crates/networking/p2p/discovery/mod.rs @@ -11,12 +11,17 @@ //! 2. Otherwise → DiscV5 pub mod codec; +pub mod contact_table; mod discv4_handlers; mod discv5_handlers; pub mod lookup; pub mod server; -pub use server::{DiscoveryServer, DiscoveryServerError, is_discv4_packet}; +pub use contact_table::{Contact, ContactTable, ContactValidation, DiscoveryProtocol, Session}; +pub use server::{ + DiscoveryHandle, DiscoveryServer, DiscoveryServerError, DiscoveryServerProtocol, + is_discv4_packet, +}; use std::time::Duration; @@ -25,6 +30,9 @@ use std::time::Duration; pub struct DiscoveryConfig { pub discv4_enabled: bool, pub discv5_enabled: bool, + /// How many connections the consumer wants. Discovery never opens one; it + /// uses this only to pace its lookups against how far along the consumer is. + pub target_peers: usize, } /// Lookup interval bounds for the RLPx initiator's connection attempts. The diff --git a/crates/networking/p2p/discovery/server.rs b/crates/networking/p2p/discovery/server.rs index 7e1a2eae668..5e4ae45d0a3 100644 --- a/crates/networking/p2p/discovery/server.rs +++ b/crates/networking/p2p/discovery/server.rs @@ -7,10 +7,11 @@ use crate::{ messages::{Packet as Discv5Packet, PacketCodecError}, server::{Discv5Message, Discv5State, update_local_ip}, }, - peer_table::{DiscoveryProtocol, PeerTable, PeerTableServerProtocol as _}, + peer_filter::PeerFilter, types::{Node, NodeRecord}, }; use bytes::BytesMut; +use ethrex_common::H256; use ethrex_common::utils::keccak; use futures::StreamExt; use secp256k1::SecretKey; @@ -19,8 +20,8 @@ use spawned_concurrency::{ error::ActorError, protocol, tasks::{ - Actor, ActorStart as _, Context, Handler, send_after, send_interval, send_message_on, - spawn_listener, + Actor, ActorRef, ActorStart as _, Context, Handler, Response, send_after, send_interval, + send_message_on, spawn_listener, }, }; use std::{net::SocketAddr, sync::Arc, time::Duration}; @@ -29,7 +30,11 @@ use tokio::net::UdpSocket; use tokio_util::udp::UdpFramed; use tracing::{debug, error, info, trace}; -use super::{DiscoveryConfig, codec::DiscriminatingCodec, lookup_interval_function}; +use super::{ + DiscoveryConfig, codec::DiscriminatingCodec, contact_table::ContactTable, + contact_table::DiscoveryProtocol, lookup_interval_function, +}; +use std::sync::OnceLock; /// Minimum packet size for a valid discv4 packet. /// hash (32) + signature (65) + type (1) = 98 bytes @@ -58,7 +63,7 @@ pub enum DiscoveryServerError { #[error("Unknown or invalid contact")] InvalidContact, #[error(transparent)] - PeerTable(#[from] ActorError), + Actor(#[from] ActorError), #[error(transparent)] Store(#[from] ethrex_storage::error::StoreError), #[error("Internal error {0}")] @@ -72,6 +77,16 @@ pub enum DiscoveryServerError { #[protocol] pub trait DiscoveryServerProtocol: Send + Sync { fn raw_packet(&self, data: BytesMut, from: SocketAddr) -> Result<(), ActorError>; + /// The consumer established a connection to this node: stop offering it as + /// a dial candidate, and count it towards how hard we look for more. + fn mark_connected(&self, node_id: H256) -> Result<(), ActorError>; + /// The consumer's connection to this node is gone. + fn mark_disconnected(&self, node_id: H256) -> Result<(), ActorError>; + /// This node is known-bad to the consumer (wrong network, no usable + /// capabilities): never offer it again. + fn set_unwanted(&self, node_id: H256) -> Result<(), ActorError>; + /// This node is not worth keeping in the routing table. + fn set_disposable(&self, node_id: H256) -> Result<(), ActorError>; fn revalidate_v4(&self) -> Result<(), ActorError>; fn revalidate_v5(&self) -> Result<(), ActorError>; fn lookup_v4(&self) -> Result<(), ActorError>; @@ -79,6 +94,85 @@ pub trait DiscoveryServerProtocol: Send + Sync { fn enr_lookup(&self) -> Result<(), ActorError>; fn prune(&self) -> Result<(), ActorError>; fn shutdown(&self) -> Result<(), ActorError>; + + /// The next node worth dialing, or `None` when nothing in the pool is + /// eligible right now. + fn next_dial_candidate(&self) -> Response>; +} + +/// The consumer's handle on a discovery server. +/// +/// Discovery is started after the context that reaches it, and is not started +/// at all when p2p is disabled, so the handle is filled in once the server is +/// up and is inert until then. +/// +/// Every message across this boundary but one is a cast. Discovery never calls +/// back into its consumer, which is what keeps two actors with sequential +/// mailboxes from deadlocking on each other. +#[derive(Clone, Debug, Default)] +pub struct DiscoveryHandle(Arc>>); + +impl DiscoveryHandle { + pub fn new() -> Self { + Self::default() + } + + /// Publish the running server to every clone of this handle. A second call + /// is ignored, and reported as `false`. + pub fn set(&self, server: ActorRef) -> bool { + self.0.set(server).is_ok() + } + + /// Casts are dropped on the floor while discovery is down: there is no + /// contact table to record them in, and no later moment at which replaying + /// them would mean anything. + fn server(&self) -> Option<&ActorRef> { + self.0.get() + } + + pub fn mark_connected(&self, node_id: H256) { + if let Some(server) = self.server() { + let _ = server.mark_connected(node_id); + } + } + + pub fn mark_disconnected(&self, node_id: H256) { + if let Some(server) = self.server() { + let _ = server.mark_disconnected(node_id); + } + } + + pub fn set_unwanted(&self, node_id: H256) { + if let Some(server) = self.server() { + let _ = server.set_unwanted(node_id); + } + } + + pub fn set_disposable(&self, node_id: H256) { + if let Some(server) = self.server() { + let _ = server.set_disposable(node_id); + } + } + + /// Ask discovery to drop the contacts it has written off, so replacements + /// waiting in the k-buckets get promoted. + pub fn prune(&self) { + if let Some(server) = self.server() { + let _ = server.prune(); + } + } + + /// The next node discovery thinks is worth dialing. `None` when discovery + /// is down, has nothing eligible, or the request failed. + pub async fn next_dial_candidate(&self) -> Option { + let server = self.server()?; + server + .next_dial_candidate() + .await + .inspect_err(|e| debug!(err=?e, "Failed to ask discovery for a dial candidate")) + .ok() + .flatten() + } } pub struct DiscoveryServer { @@ -86,7 +180,9 @@ pub struct DiscoveryServer { pub local_node_record: NodeRecord, pub(crate) signer: SecretKey, pub(crate) udp_socket: Arc, - pub peer_table: PeerTable, + /// Everything known about nodes we have merely heard of. Plain state: the + /// handlers below all run inside this actor, so they reach it directly. + pub(crate) contacts: ContactTable, pub(crate) config: DiscoveryConfig, pub discv4: Option, pub discv5: Option, @@ -96,6 +192,7 @@ impl std::fmt::Debug for DiscoveryServer { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.debug_struct("DiscoveryServer") .field("local_node", &self.local_node) + .field("contacts", &self.contacts) .field("discv4_enabled", &self.discv4.is_some()) .field("discv5_enabled", &self.discv5.is_some()) .finish() @@ -105,25 +202,34 @@ impl std::fmt::Debug for DiscoveryServer { #[actor(protocol = DiscoveryServerProtocol)] impl DiscoveryServer { /// Starts the discovery actor, advertising `local_node_record` as this - /// node's ENR. + /// node's ENR, and returns a handle on the running server. + /// + /// `filter` is what this consumer requires of a discovered peer: every ENR + /// discovery sees is judged by it. A contact discovered without an ENR is + /// never screened and stays dialable, so bootnodes are usable before they + /// have published anything. pub async fn spawn( local_node: Node, local_node_record: NodeRecord, signer: SecretKey, udp_socket: Arc, - peer_table: PeerTable, + filter: Box, bootnodes: Vec, config: DiscoveryConfig, - ) -> Result<(), DiscoveryServerError> { + ) -> Result, DiscoveryServerError> { debug!("Starting discovery server"); + let mut contacts = ContactTable::new(local_node.node_id(), config.target_peers, filter); + let discv4 = if config.discv4_enabled { info!( protocol = "discv4", count = bootnodes.len(), "Adding bootnodes" ); - peer_table.new_contacts(bootnodes.clone(), DiscoveryProtocol::Discv4)?; + contacts + .new_contacts(bootnodes.clone(), DiscoveryProtocol::Discv4) + .await; Some(Discv4State::default()) } else { None @@ -135,7 +241,9 @@ impl DiscoveryServer { count = bootnodes.len(), "Adding bootnodes" ); - peer_table.new_contacts(bootnodes.clone(), DiscoveryProtocol::Discv5)?; + contacts + .new_contacts(bootnodes.clone(), DiscoveryProtocol::Discv5) + .await; Some(Discv5State::default()) } else { None @@ -146,7 +254,7 @@ impl DiscoveryServer { local_node_record, signer, udp_socket: udp_socket.clone(), - peer_table: peer_table.clone(), + contacts, config, discv4, discv5, @@ -159,9 +267,7 @@ impl DiscoveryServer { } } - server.start(); - - Ok(()) + Ok(server.start()) } #[started] @@ -268,7 +374,7 @@ impl DiscoveryServer { let _ = self.discv4_lookup().await.inspect_err( |e| error!(protocol = "discv4", err=?e, "Error performing Discovery lookup"), ); - let interval = self.get_lookup_interval().await; + let interval = self.get_lookup_interval(); send_after(interval, ctx.clone(), discovery_server_protocol::LookupV4); } @@ -282,7 +388,7 @@ impl DiscoveryServer { let _ = self.discv5_lookup().await.inspect_err( |e| error!(protocol = "discv5", err=?e, "Error performing Discovery lookup"), ); - let interval = self.get_lookup_interval().await; + let interval = self.get_lookup_interval(); send_after(interval, ctx.clone(), discovery_server_protocol::LookupV5); } @@ -296,7 +402,7 @@ impl DiscoveryServer { let _ = self.discv4_enr_lookup().await.inspect_err( |e| error!(protocol = "discv4", err=?e, "Error performing Discovery lookup"), ); - let interval = self.get_lookup_interval().await; + let interval = self.get_lookup_interval(); send_after(interval, ctx.clone(), discovery_server_protocol::EnrLookup); } @@ -309,6 +415,51 @@ impl DiscoveryServer { .inspect_err(|e| error!(err=?e, "Error Pruning peer table")); } + #[send_handler] + async fn handle_mark_connected( + &mut self, + msg: discovery_server_protocol::MarkConnected, + _ctx: &Context, + ) { + self.contacts.mark_connected(msg.node_id); + } + + #[send_handler] + async fn handle_mark_disconnected( + &mut self, + msg: discovery_server_protocol::MarkDisconnected, + _ctx: &Context, + ) { + self.contacts.mark_disconnected(&msg.node_id); + } + + #[send_handler] + async fn handle_set_unwanted( + &mut self, + msg: discovery_server_protocol::SetUnwanted, + _ctx: &Context, + ) { + self.contacts.set_unwanted(&msg.node_id); + } + + #[send_handler] + async fn handle_set_disposable( + &mut self, + msg: discovery_server_protocol::SetDisposable, + _ctx: &Context, + ) { + self.contacts.set_disposable(&msg.node_id); + } + + #[request_handler] + async fn handle_next_dial_candidate( + &mut self, + _msg: discovery_server_protocol::NextDialCandidate, + _ctx: &Context, + ) -> Option { + self.contacts.next_dial_candidate() + } + #[send_handler] async fn handle_shutdown( &mut self, @@ -371,7 +522,7 @@ impl DiscoveryServer { } async fn prune(&mut self) -> Result<(), DiscoveryServerError> { - self.peer_table.prune_table()?; + self.contacts.prune(); if let Some(discv4) = &mut self.discv4 { let expiration = Duration::from_secs(crate::discv4::server::EXPIRATION_SECONDS); discv4 @@ -401,12 +552,8 @@ impl DiscoveryServer { Ok(()) } - pub(crate) async fn get_lookup_interval(&self) -> Duration { - let peer_completion = self - .peer_table - .target_peers_completion() - .await - .unwrap_or_default(); + pub(crate) fn get_lookup_interval(&self) -> Duration { + let peer_completion = self.contacts.peer_completion(); lookup_interval_function( peer_completion, ITERATIVE_LOOKUP_INITIAL_MS, @@ -427,6 +574,12 @@ pub fn is_discv4_packet(data: &[u8]) -> bool { #[cfg(any(test, feature = "test-utils"))] impl DiscoveryServer { + /// The contact table this server owns, so a test can seed it before driving + /// the handlers by hand. + pub fn contacts_mut(&mut self) -> &mut ContactTable { + &mut self.contacts + } + /// Builds a DiscoveryServer suitable for unit tests of discv5 handlers. /// Only discv5 state is initialized; discv4 is disabled. /// Uses a dummy initial lookup interval. @@ -435,17 +588,19 @@ impl DiscoveryServer { local_node_record: NodeRecord, signer: SecretKey, udp_socket: Arc, - peer_table: PeerTable, + filter: Box, ) -> Self { + let local_node_id = local_node.node_id(); Self { local_node, local_node_record, signer, udp_socket, - peer_table, + contacts: ContactTable::new(local_node_id, 10, filter), config: DiscoveryConfig { discv4_enabled: false, discv5_enabled: true, + target_peers: 10, }, discv4: None, discv5: Some(Discv5State::default()), diff --git a/crates/networking/p2p/network.rs b/crates/networking/p2p/network.rs index 231ee80a005..37cff9e8f19 100644 --- a/crates/networking/p2p/network.rs +++ b/crates/networking/p2p/network.rs @@ -4,8 +4,9 @@ use crate::rlpx::l2::l2_connection::P2PBasedContext; #[derive(Clone, Debug)] pub struct P2PBasedContext; use crate::{ - discovery::{DiscoveryConfig, DiscoveryServer, DiscoveryServerError}, + discovery::{DiscoveryConfig, DiscoveryHandle, DiscoveryServer, DiscoveryServerError}, metrics::{CurrentStepValue, METRICS}, + peer_filter::EthForkIdFilter, peer_table::{PeerData, PeerTable, PeerTableServerProtocol as _}, rlpx::{ connection::server::{PeerConnBroadcastSender, PeerConnection}, @@ -37,6 +38,9 @@ pub struct P2PContext { pub tracker: TaskTracker, pub signer: SecretKey, pub table: PeerTable, + /// Reaches the discovery server, once one is running. Inert when p2p runs + /// without discovery. + pub discovery: DiscoveryHandle, pub storage: Store, pub blockchain: Arc, pub(crate) broadcast: PeerConnBroadcastSender, @@ -96,6 +100,7 @@ impl P2PContext { tracker, signer, table: peer_table, + discovery: DiscoveryHandle::new(), storage, blockchain, broadcast: channel_broadcast_send_end, @@ -152,12 +157,12 @@ pub async fn start_network( let local_node_record = build_local_node_record(&context).await?; - DiscoveryServer::spawn( + let discovery = DiscoveryServer::spawn( context.local_node.clone(), local_node_record, context.signer, udp_socket, - context.table.clone(), + Box::new(EthForkIdFilter::new(context.storage.clone())), bootnodes, config, ) @@ -166,6 +171,12 @@ pub async fn start_network( error!("Failed to start discovery server: {e}"); })?; + // Publishes the server to every clone of the context, which is what lets the + // initiator ask for dial candidates and connections report their lifecycle. + if !context.discovery.set(discovery) { + error!("Discovery server was already set on this context"); + } + context.tracker.spawn(serve_p2p_requests(context.clone())); Ok(()) diff --git a/crates/networking/p2p/peer_filter.rs b/crates/networking/p2p/peer_filter.rs index 518afd1b593..a76b5990181 100644 --- a/crates/networking/p2p/peer_filter.rs +++ b/crates/networking/p2p/peer_filter.rs @@ -103,6 +103,19 @@ impl PeerFilter for EthForkIdFilter { } } +/// Accepts every peer discovery finds. +/// +/// What a consumer that only wants the discovery stack passes: it has no +/// requirement of its own to express, and screening on our behalf would only +/// throw away peers it might want. +pub struct AcceptAllFilter; + +impl PeerFilter for AcceptAllFilter { + fn accepts(&self, _record: &NodeRecord) -> bool { + true + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/crates/networking/p2p/peer_handler.rs b/crates/networking/p2p/peer_handler.rs index 98850826494..83320bc8e4b 100644 --- a/crates/networking/p2p/peer_handler.rs +++ b/crates/networking/p2p/peer_handler.rs @@ -1,3 +1,4 @@ +use crate::discovery::DiscoveryHandle; use crate::rlpx::initiator::RLPxInitiator; use crate::{ metrics::{CurrentStepValue, METRICS}, @@ -50,6 +51,9 @@ pub use crate::snap::{DumpError, RequestMetadata, RequestStorageTrieNodesError, #[derive(Debug, Clone)] pub struct PeerHandler { pub peer_table: PeerTable, + /// Reports peers that turned out to be useless back to discovery, and asks + /// it to prune. Inert when discovery is not running. + pub discovery: DiscoveryHandle, pub initiator: ActorRef, } @@ -150,9 +154,14 @@ async fn ask_peer_head_number( } impl PeerHandler { - pub fn new(peer_table: PeerTable, initiator: ActorRef) -> PeerHandler { + pub fn new( + peer_table: PeerTable, + initiator: ActorRef, + discovery: DiscoveryHandle, + ) -> PeerHandler { Self { peer_table, + discovery, initiator, } } @@ -606,7 +615,7 @@ impl PeerHandler { "Peer returned more block bodies than requested, disposing" ); self.peer_table.record_failure(peer_id)?; - let _ = self.peer_table.set_disposable(peer_id); + self.discovery.set_disposable(peer_id); return Ok(None); } if !block_bodies.is_empty() { @@ -763,7 +772,7 @@ impl PeerHandler { _ => { debug!("Didn't receive receipts from peer, penalizing peer {peer_id}"); self.peer_table.record_failure(peer_id)?; - let _ = self.peer_table.set_disposable(peer_id); + self.discovery.set_disposable(peer_id); return Ok(None); } }; @@ -781,7 +790,7 @@ impl PeerHandler { if receipts.len() > block_hashes_len { debug!("Received oversized receipts from peer {peer_id}, penalizing"); self.peer_table.record_failure(peer_id)?; - let _ = self.peer_table.set_disposable(peer_id); + self.discovery.set_disposable(peer_id); return Ok(None); } // Success is recorded by the caller, once the receipts have been diff --git a/crates/networking/p2p/peer_table.rs b/crates/networking/p2p/peer_table.rs index 948a166ecda..3cc26f27a47 100644 --- a/crates/networking/p2p/peer_table.rs +++ b/crates/networking/p2p/peer_table.rs @@ -1,39 +1,31 @@ -//! Unified peer table for both discv4 and discv5 discovery protocols. +//! The peers this node is connected to over RLPx, and how well each is serving us. //! -//! This module provides a protocol-agnostic peer table that stores contact -//! information discovered through either discv4 or discv5. The key abstraction -//! is using `Bytes` for ping identifiers: -//! - discv4: converts H256 ping hash to Bytes -//! - discv5: already uses Bytes for req_id +//! Strictly the live side of the network: a node reaches this table only once a +//! connection is established, and leaves it when the connection drops. What +//! discovery knows about nodes it has merely heard of lives in +//! [`ContactTable`](crate::discovery::ContactTable), owned by the discovery +//! server, which knows nothing about RLPx. //! -//! Each contact is tagged with the protocol that discovered it, allowing -//! protocol-specific lookups to only query compatible contacts. +//! Peers are scored (`record_success` / `record_failure`) and their in-flight +//! request count tracked, so selection can spread load across peers that are +//! actually answering. A selected peer comes back with a [`RequestPermit`] +//! holding its slot for as long as the request is outstanding. use crate::{ - metrics::METRICS, - peer_filter::{EthForkIdFilter, PeerFilter}, rlpx::{connection::server::PeerConnection, p2p::Capability}, types::{Node, NodeRecord}, - utils::distance, }; -use bytes::Bytes; -use ethrex_common::{H256, U256}; -use ethrex_storage::Store; +use ethrex_common::H256; use indexmap::IndexMap; use rand::distributions::WeightedIndex; use rand::prelude::Distribution; -use rand::seq::{IteratorRandom, SliceRandom}; -use rustc_hash::{FxHashMap, FxHashSet}; use spawned_concurrency::{ actor, error::ActorError, protocol, tasks::{Actor, ActorRef, ActorStart as _, Context, Handler, Response, send_message_on}, }; -use std::{ - net::IpAddr, - time::{Duration, Instant}, -}; +use std::net::IpAddr; const MAX_SCORE: i64 = 50; const MIN_SCORE: i64 = -50; @@ -47,254 +39,6 @@ const REQUESTS_WEIGHT: i64 = 1; const MAX_CONCURRENT_REQUESTS_PER_PEER: i64 = 100; /// The target number of RLPx connections to reach. pub const TARGET_PEERS: usize = 100; -/// Maximum number of ENRs to return in a FindNode response (discv4 compatible). -pub(crate) const MAX_NODES_IN_NEIGHBORS_PACKET: usize = 16; -/// Maximum number of ENRs to return in a discv5 FindNode response. -const MAX_ENRS_PER_FINDNODE_RESPONSE: usize = 16; - -/// Number of k-buckets in the Kademlia routing table (one per bit of the 256-bit node ID). -const NUMBER_OF_BUCKETS: usize = 256; -/// Maximum number of contacts per k-bucket (Kademlia k parameter). -pub const MAX_NODES_PER_BUCKET: usize = 16; -/// Maximum number of replacement entries per k-bucket. -const MAX_REPLACEMENTS_PER_BUCKET: usize = 10; -/// Maximum number of entries in the flat connection candidate pool. -/// This pool is separate from the k-bucket routing table and retains -/// more contacts for RLPx connection initiation than the k-bucket -/// structure allows (256 × 16 = 4,096 vs this larger capacity). -/// 10K matches what Reth and Nethermind use for their candidate pools. -const MAX_CONNECTION_POOL_SIZE: usize = 10_000; - -/// A single k-bucket in the Kademlia routing table. -/// Each bucket stores contacts at a specific XOR distance range from the local node. -#[derive(Debug, Clone, Default)] -pub struct KBucket { - pub(crate) contacts: Vec<(H256, Contact)>, - pub(crate) replacements: Vec<(H256, Contact)>, -} - -impl KBucket { - /// Find a contact by node ID in the main list. - fn get(&self, node_id: &H256) -> Option<&Contact> { - self.contacts - .iter() - .find(|(id, _)| id == node_id) - .map(|(_, c)| c) - } - - /// Find a contact by node ID in either the main or replacement list. - fn get_any(&self, node_id: &H256) -> Option<&Contact> { - self.get(node_id).or_else(|| { - self.replacements - .iter() - .find(|(id, _)| id == node_id) - .map(|(_, c)| c) - }) - } - - /// Find a mutable reference to a contact by node ID (main or replacement list). - fn get_mut(&mut self, node_id: &H256) -> Option<&mut Contact> { - if let Some((_, c)) = self.contacts.iter_mut().find(|(id, _)| id == node_id) { - return Some(c); - } - self.replacements - .iter_mut() - .find(|(id, _)| id == node_id) - .map(|(_, c)| c) - } - - /// Check if a contact exists in this bucket (main or replacement list). - fn contains(&self, node_id: &H256) -> bool { - self.contacts.iter().any(|(id, _)| id == node_id) - || self.replacements.iter().any(|(id, _)| id == node_id) - } - - /// Insert a contact into the bucket. Returns true if inserted into main list. - /// If the bucket is full, the contact is added to the replacement list instead. - fn insert(&mut self, node_id: H256, contact: Contact) -> bool { - if self.contacts.len() < MAX_NODES_PER_BUCKET { - self.contacts.push((node_id, contact)); - true - } else { - self.insert_replacement(node_id, contact); - false - } - } - - /// Add a contact to the replacement list, evicting the oldest if full. - fn insert_replacement(&mut self, node_id: H256, contact: Contact) { - if self.replacements.len() >= MAX_REPLACEMENTS_PER_BUCKET { - self.replacements.remove(0); - } - self.replacements.push((node_id, contact)); - } - - /// Remove a contact from the main list and promote a replacement if available. - /// Returns the promoted replacement's node ID, if any. - fn remove_and_promote(&mut self, node_id: &H256) -> Option { - let idx = self.contacts.iter().position(|(id, _)| id == node_id)?; - self.contacts.remove(idx); - if !self.replacements.is_empty() { - let (replacement_id, replacement) = self.replacements.remove(0); - self.contacts.push((replacement_id, replacement)); - Some(replacement_id) - } else { - None - } - } -} - -/// Computes the bucket index for a node relative to the local node. -/// Uses XOR distance: bucket = floor(log2(XOR(local, remote))), i.e. the -/// position of the highest set bit minus 1. -/// Returns None for the local node itself (XOR = 0). -fn bucket_index(local_node_id: &H256, node_id: &H256) -> Option { - 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) - } -} - -/// Computes the raw XOR distance between two node IDs. -/// Used for comparing relative closeness: a is closer to target than b -/// iff xor_distance(target, a) < xor_distance(target, b). -pub(crate) fn xor_distance(a: &H256, b: &H256) -> H256 { - *a ^ *b -} - -/// Identifies which discovery protocol was used to find a contact. -/// This allows protocol-specific lookups to only query compatible contacts. -#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] -pub enum DiscoveryProtocol { - /// Contact discovered via discv4 protocol - Discv4, - /// Contact discovered via discv5 protocol - Discv5, -} - -/// Session information for discv5 protocol. -/// Contains symmetric keys derived from ECDH for message encryption/decryption. -pub use crate::discv5::session::Session; - -#[derive(Debug, Clone)] -pub struct Contact { - pub node: Node, - /// Whether this contact is reachable via discv4 protocol. - pub is_discv4: bool, - /// Whether this contact is reachable via discv5 protocol. - pub is_discv5: bool, - /// The timestamp when the contact was last sent a ping. - /// If None, the contact has never been pinged. - pub validation_timestamp: Option, - /// The identifier of the last unacknowledged ping sent to this contact, or - /// None if no ping was sent yet or it was already acknowledged. - /// - discv4: H256 hash converted to Bytes - /// - discv5: request ID as Bytes - pub ping_id: Option, - - /// The hash of the last unacknowledged ENRRequest sent to this contact, or - /// None if no request was sent yet or it was already acknowledged. - pub enr_request_hash: Option, - - /// ENR associated with this contact, if it was provided by the peer. - pub record: Option, - /// This contact failed to respond our Ping. - pub disposable: bool, - /// Set to true after we send a successful ENRResponse to it. - pub knows_us: bool, - /// This is a known-bad peer (on another network, no matching capabilities, etc) - pub unwanted: bool, - /// Whether this contact's last known ENR made it through the consumer's - /// [`PeerFilter`], or `None` while it has never been filtered. - /// - /// Unfiltered stays dialable: a contact discovered without an ENR never - /// reaches the filter at all, and treating that as a rejection would leave - /// it permanently untriable. That is why bootnodes, which arrive as bare - /// endpoints, are dialable before they have published anything. - pub passes_filter: Option, - /// Session information for discv5 (None for discv4 contacts) - session: Option, -} - -impl Contact { - pub fn was_validated(&self) -> bool { - self.validation_timestamp.is_some() && !self.has_pending_ping() - } - - pub fn has_pending_ping(&self) -> bool { - self.ping_id.is_some() - } - - pub fn record_ping_sent(&mut self, ping_id: Bytes) { - self.validation_timestamp = Some(Instant::now()); - self.ping_id = Some(ping_id); - } - - pub fn record_enr_request_sent(&mut self, request_hash: H256) { - self.enr_request_hash = Some(request_hash); - } - - /// Stores `record` if it answers the ENR request we have outstanding. - /// - /// Returns whether it was stored, so the caller knows whether the contact's - /// cached [`Self::passes_filter`] still describes the record it holds. A - /// response whose hash does not match is ignored outright: letting it - /// through would let a peer restate its own standing from a record we - /// refused to keep. - pub fn record_enr_response_received(&mut self, request_hash: H256, record: NodeRecord) -> bool { - if self - .enr_request_hash - .take_if(|h| *h == request_hash) - .is_some() - { - self.record = Some(record); - return true; - } - false - } - - pub fn has_pending_enr_request(&self) -> bool { - self.enr_request_hash.is_some() - } -} - -impl Contact { - pub fn new(node: Node, protocol: DiscoveryProtocol) -> Self { - Self { - node, - is_discv4: protocol == DiscoveryProtocol::Discv4, - is_discv5: protocol == DiscoveryProtocol::Discv5, - validation_timestamp: None, - ping_id: None, - enr_request_hash: None, - record: None, - disposable: false, - knows_us: true, - unwanted: false, - passes_filter: None, - session: None, - } - } - - /// Check if this contact supports the given protocol. - pub fn supports_protocol(&self, protocol: DiscoveryProtocol) -> bool { - match protocol { - DiscoveryProtocol::Discv4 => self.is_discv4, - DiscoveryProtocol::Discv5 => self.is_discv5, - } - } - - /// Mark this contact as supporting the given protocol. - pub fn add_protocol(&mut self, protocol: DiscoveryProtocol) { - match protocol { - DiscoveryProtocol::Discv4 => self.is_discv4 = true, - DiscoveryProtocol::Discv5 => self.is_discv5 = true, - } - } -} #[derive(Debug, Clone)] pub struct PeerData { @@ -348,15 +92,6 @@ pub struct PeerDiagnostics { pub last_response_time: Option, } -/// Result of contact validation. -#[derive(Debug, Clone)] -pub enum ContactValidation { - Valid(Box), - InvalidContact, - UnknownContact, - IpMismatch, -} - /// Reservation handle for a peer request slot. /// /// **Contract:** when a `RequestPermit` exists, the `requests` counter for @@ -408,9 +143,6 @@ impl Drop for RequestPermit { #[protocol] pub trait PeerTableServerProtocol: Send + Sync { // Send (cast) methods - fn new_contacts(&self, nodes: Vec, protocol: DiscoveryProtocol) - -> Result<(), ActorError>; - fn new_contact_records(&self, node_records: Vec) -> Result<(), ActorError>; fn new_connected_peer( &self, node: Node, @@ -418,42 +150,18 @@ pub trait PeerTableServerProtocol: Send + Sync { capabilities: Vec, is_inbound: bool, ) -> Result<(), ActorError>; - fn set_session_info(&self, node_id: H256, session: Session) -> Result<(), ActorError>; fn remove_peer(&self, node_id: H256) -> Result<(), ActorError>; fn dec_requests(&self, node_id: H256) -> Result<(), ActorError>; - fn set_unwanted(&self, node_id: H256) -> Result<(), ActorError>; fn record_success(&self, node_id: H256) -> Result<(), ActorError>; fn record_failure(&self, node_id: H256) -> Result<(), ActorError>; fn record_critical_failure(&self, node_id: H256) -> Result<(), ActorError>; - fn record_ping_sent(&self, node_id: H256, ping_id: Bytes) -> Result<(), ActorError>; - fn record_pong_received(&self, node_id: H256, ping_id: Bytes) -> Result<(), ActorError>; - fn record_enr_request_sent(&self, node_id: H256, request_hash: H256) -> Result<(), ActorError>; - fn record_enr_response_received( - &self, - node_id: H256, - request_hash: H256, - record: NodeRecord, - ) -> Result<(), ActorError>; - fn set_disposable(&self, node_id: H256) -> Result<(), ActorError>; - fn mark_knows_us(&self, node_id: H256) -> Result<(), ActorError>; - fn prune_table(&self) -> Result<(), ActorError>; fn shutdown(&self) -> Result<(), ActorError>; // Request (call) methods fn peer_count(&self) -> Response; fn peer_count_by_capabilities(&self, capabilities: Vec) -> Response; - fn target_reached(&self) -> Response; fn target_peers_reached(&self) -> Response; fn target_peers_completion(&self) -> Response; - fn get_contact_to_initiate(&self) -> Response>>; - fn get_contact_for_enr_lookup(&self) -> Response>>; - fn get_closest_from_pool(&self, target: H256, count: usize) -> Response>; - fn get_contact(&self, node_id: H256) -> Response>>; - fn get_contact_to_revalidate( - &self, - revalidation_interval: Duration, - protocol: DiscoveryProtocol, - ) -> Response>>; fn get_best_peer( &self, capabilities: Vec, @@ -475,87 +183,38 @@ pub trait PeerTableServerProtocol: Send + Sync { fn get_connected_nodes(&self) -> Response>; fn get_peers_with_capabilities(&self) -> Response)>>; - fn insert_if_new(&self, node: Node, protocol: DiscoveryProtocol) -> Response; - fn validate_contact(&self, node_id: H256, sender_ip: IpAddr) -> Response; - fn get_closest_nodes(&self, node_id: H256) -> Response>; - fn get_nodes_at_distances(&self, distances: Vec) -> Response>; fn get_peers_data(&self) -> Response>; fn get_random_peer(&self, capabilities: Vec) -> Response>; - fn get_session_info(&self, node_id: H256) -> Response>; fn get_peer_diagnostics(&self) -> Response>; fn get_peer_connection(&self, peer_id: H256) -> Response>; } pub struct PeerTableServer { - local_node_id: H256, - buckets: Vec, peers: IndexMap, - already_tried_peers: FxHashSet, + /// How many connections this node wants. Only ever compared against + /// `peers.len()`; discovery keeps its own copy to pace its lookups. target_peers: usize, - /// What this consumer requires of a discovered peer. Judged as each ENR - /// arrives, over either discovery protocol; the answer is cached on the - /// contact as [`Contact::passes_filter`]. - filter: Box, - /// Standalone session store, independent of contacts. - /// Allows sessions to be stored even before the contact's ENR is known/parseable. - sessions: FxHashMap, - /// 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. - connection_pool: IndexMap, } -// Hand-written because `Box` is not `Debug`, and requiring that -// of every consumer's filter buys less than keeping the actor's state printable. impl std::fmt::Debug for PeerTableServer { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.debug_struct("PeerTableServer") - .field("local_node_id", &self.local_node_id) .field("peers", &self.peers) - .field("already_tried_peers", &self.already_tried_peers) .field("target_peers", &self.target_peers) - .field("sessions", &self.sessions) - .field("connection_pool", &self.connection_pool) - .finish_non_exhaustive() + .finish() } } #[actor(protocol = PeerTableServerProtocol)] impl PeerTableServer { - /// Spawn a peer table for an Ethereum L1 node, screening every ENR it sees - /// for an EIP-2124 fork id compatible with our chain. - /// - /// A contact discovered without an ENR is never screened and stays dialable, - /// so bootnodes are usable before they have published anything. See - /// [`Self::spawn_with_filter`] for consumers on other networks. - pub fn spawn(local_node_id: H256, target_peers: usize, store: Store) -> PeerTable { - Self::spawn_with_filter(local_node_id, target_peers, EthForkIdFilter::new(store)) - } - - pub fn spawn_with_filter( - local_node_id: H256, - target_peers: usize, - filter: impl PeerFilter + 'static, - ) -> PeerTable { - PeerTableServer::new(local_node_id, target_peers, Box::new(filter)).start() + pub fn spawn(target_peers: usize) -> PeerTable { + PeerTableServer::new(target_peers).start() } - pub(crate) fn new( - local_node_id: H256, - target_peers: usize, - filter: Box, - ) -> Self { + pub(crate) fn new(target_peers: usize) -> Self { Self { - local_node_id, - buckets: vec![KBucket::default(); NUMBER_OF_BUCKETS], peers: Default::default(), - already_tried_peers: Default::default(), target_peers, - filter, - sessions: Default::default(), - connection_pool: IndexMap::with_capacity(MAX_CONNECTION_POOL_SIZE), } } @@ -570,24 +229,6 @@ impl PeerTableServer { // === Send handlers === - #[send_handler] - async fn handle_new_contacts( - &mut self, - msg: peer_table_server_protocol::NewContacts, - _ctx: &Context, - ) { - self.do_new_contacts(msg.nodes, msg.protocol).await; - } - - #[send_handler] - async fn handle_new_contact_records( - &mut self, - msg: peer_table_server_protocol::NewContactRecords, - _ctx: &Context, - ) { - self.do_new_contact_records(msg.node_records).await; - } - #[send_handler] async fn handle_new_connected_peer( &mut self, @@ -600,20 +241,6 @@ impl PeerTableServer { self.peers.insert(new_peer_id, new_peer); } - #[send_handler] - async fn handle_set_session_info( - &mut self, - msg: peer_table_server_protocol::SetSessionInfo, - _ctx: &Context, - ) { - // Store in the standalone sessions map (always succeeds, no contact required). - self.sessions.insert(msg.node_id, msg.session.clone()); - // Also update the contact's cached session if the contact exists. - if let Some(contact) = self.get_contact_mut(&msg.node_id) { - contact.session = Some(msg.session); - } - } - #[send_handler] async fn handle_remove_peer( &mut self, @@ -621,9 +248,6 @@ impl PeerTableServer { _ctx: &Context, ) { self.peers.swap_remove(&msg.node_id); - // Also drop the standalone discv5 session so it isn't retained after the peer leaves - // (the sessions map was previously insert-only and grew per handshake). - self.sessions.remove(&msg.node_id); } #[send_handler] @@ -647,17 +271,6 @@ impl PeerTableServer { }); } - #[send_handler] - async fn handle_set_unwanted( - &mut self, - msg: peer_table_server_protocol::SetUnwanted, - _ctx: &Context, - ) { - if let Some(contact) = self.get_contact_mut(&msg.node_id) { - contact.unwanted = true; - } - } - #[send_handler] async fn handle_record_success( &mut self, @@ -696,83 +309,6 @@ impl PeerTableServer { .and_modify(|peer_data| peer_data.score = MIN_SCORE_CRITICAL); } - #[send_handler] - async fn handle_record_ping_sent( - &mut self, - msg: peer_table_server_protocol::RecordPingSent, - _ctx: &Context, - ) { - if let Some(contact) = self.get_contact_mut(&msg.node_id) { - contact.record_ping_sent(msg.ping_id); - } - } - - #[send_handler] - async fn handle_record_pong_received( - &mut self, - msg: peer_table_server_protocol::RecordPongReceived, - _ctx: &Context, - ) { - 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; - } - } - - #[send_handler] - async fn handle_record_enr_request_sent( - &mut self, - msg: peer_table_server_protocol::RecordEnrRequestSent, - _ctx: &Context, - ) { - self.do_record_enr_request_sent(msg.node_id, msg.request_hash); - } - - #[send_handler] - async fn handle_record_enr_response_received( - &mut self, - msg: peer_table_server_protocol::RecordEnrResponseReceived, - _ctx: &Context, - ) { - self.do_record_enr_response_received(msg.node_id, msg.request_hash, msg.record); - } - - #[send_handler] - async fn handle_set_disposable( - &mut self, - msg: peer_table_server_protocol::SetDisposable, - _ctx: &Context, - ) { - if let Some(contact) = self.get_contact_mut(&msg.node_id) { - contact.disposable = true; - } - } - - #[send_handler] - async fn handle_mark_knows_us( - &mut self, - msg: peer_table_server_protocol::MarkKnowsUs, - _ctx: &Context, - ) { - if let Some(contact) = self.get_contact_mut(&msg.node_id) { - contact.knows_us = true; - } - } - - #[send_handler] - async fn handle_prune_table( - &mut self, - _msg: peer_table_server_protocol::PruneTable, - _ctx: &Context, - ) { - self.prune(); - } - #[send_handler] async fn handle_shutdown( &mut self, @@ -802,15 +338,6 @@ impl PeerTableServer { self.do_peer_count_by_capabilities(msg.capabilities) } - #[request_handler] - async fn handle_target_reached( - &mut self, - _msg: peer_table_server_protocol::TargetReached, - _ctx: &Context, - ) -> bool { - self.peers.len() >= self.target_peers - } - #[request_handler] async fn handle_target_peers_reached( &mut self, @@ -829,51 +356,6 @@ impl PeerTableServer { self.peers.len() as f64 / self.target_peers as f64 } - #[request_handler] - async fn handle_get_contact_to_initiate( - &mut self, - _msg: peer_table_server_protocol::GetContactToInitiate, - _ctx: &Context, - ) -> Option> { - self.do_get_contact_to_initiate().map(Box::new) - } - - #[request_handler] - async fn handle_get_closest_from_pool( - &mut self, - msg: peer_table_server_protocol::GetClosestFromPool, - _ctx: &Context, - ) -> Vec<(H256, Node)> { - self.do_get_closest_from_pool(msg.target, msg.count) - } - - #[request_handler] - async fn handle_get_contact_for_enr_lookup( - &mut self, - _msg: peer_table_server_protocol::GetContactForEnrLookup, - _ctx: &Context, - ) -> Option> { - self.do_get_contact_for_enr_lookup().map(Box::new) - } - - #[request_handler] - async fn handle_get_contact( - &mut self, - msg: peer_table_server_protocol::GetContact, - _ctx: &Context, - ) -> Option> { - self.get_contact(&msg.node_id).cloned().map(Box::new) - } - - #[request_handler] - async fn handle_get_contact_to_revalidate( - &mut self, - msg: peer_table_server_protocol::GetContactToRevalidate, - _ctx: &Context, - ) -> Option> { - self.do_get_contact_to_revalidate(msg.revalidation_interval, msg.protocol) - } - #[request_handler] async fn handle_get_best_peer( &mut self, @@ -980,54 +462,6 @@ impl PeerTableServer { .collect() } - #[request_handler] - async fn handle_insert_if_new( - &mut self, - msg: peer_table_server_protocol::InsertIfNew, - _ctx: &Context, - ) -> bool { - let node_id = msg.node.node_id(); - // Always add to the connection pool - self.insert_to_connection_pool(node_id, msg.node.clone()); - if self.contact_exists(&node_id) { - return false; - } - let contact = Contact::new(msg.node, msg.protocol); - // Return true for any genuinely new node, even if it overflows to the - // replacement list. This ensures the caller sends a reciprocal ping - // which establishes the bond needed for FindNode validation. - self.insert_contact(node_id, contact); - METRICS.record_new_discovery().await; - true - } - - #[request_handler] - async fn handle_validate_contact( - &mut self, - msg: peer_table_server_protocol::ValidateContact, - _ctx: &Context, - ) -> ContactValidation { - self.do_validate_contact(msg.node_id, msg.sender_ip) - } - - #[request_handler] - async fn handle_get_closest_nodes( - &mut self, - msg: peer_table_server_protocol::GetClosestNodes, - _ctx: &Context, - ) -> Vec { - self.do_get_closest_nodes(msg.node_id) - } - - #[request_handler] - async fn handle_get_nodes_at_distances( - &mut self, - msg: peer_table_server_protocol::GetNodesAtDistances, - _ctx: &Context, - ) -> Vec { - self.do_get_nodes_at_distances(&msg.distances) - } - #[request_handler] async fn handle_get_peers_data( &mut self, @@ -1056,19 +490,6 @@ impl PeerTableServer { )) } - #[request_handler] - async fn handle_get_session_info( - &mut self, - msg: peer_table_server_protocol::GetSessionInfo, - _ctx: &Context, - ) -> Option { - // Check standalone sessions map first; fall back to contact.session. - self.sessions - .get(&msg.node_id) - .cloned() - .or_else(|| self.get_contact(&msg.node_id)?.session.clone()) - } - #[request_handler] async fn handle_get_peer_connection( &mut self, @@ -1112,98 +533,6 @@ impl PeerTableServer { // === Private helper methods === - // --- K-bucket accessors --- - - /// Get the bucket index for a node ID, or None if it's the local node. - fn bucket_for(&self, node_id: &H256) -> Option { - bucket_index(&self.local_node_id, node_id) - } - - /// Look up a contact by node ID in main or replacement list (O(K) within the bucket). - fn get_contact(&self, node_id: &H256) -> Option<&Contact> { - let idx = self.bucket_for(node_id)?; - self.buckets[idx].get_any(node_id) - } - - /// Look up a mutable reference to a contact by node ID. - fn get_contact_mut(&mut self, node_id: &H256) -> Option<&mut Contact> { - let idx = self.bucket_for(node_id)?; - self.buckets[idx].get_mut(node_id) - } - - /// Check if a contact exists in any bucket (main or replacement list). - fn contact_exists(&self, node_id: &H256) -> bool { - let Some(idx) = self.bucket_for(node_id) else { - return false; - }; - self.buckets[idx].contains(node_id) - } - - /// Insert a contact into the appropriate k-bucket. Returns true if inserted - /// into the main list, false if the node went to the replacement list or is - /// the local node. - fn insert_contact(&mut self, node_id: H256, contact: Contact) -> bool { - #[cfg(feature = "metrics")] - let start = std::time::Instant::now(); - - let Some(idx) = self.bucket_for(&node_id) else { - return false; - }; - let result = self.buckets[idx].insert(node_id, contact); - - #[cfg(feature = "metrics")] - { - use ethrex_metrics::p2p::METRICS_P2P; - METRICS_P2P.observe_insert_contact_duration(start.elapsed().as_secs_f64()); - } - - result - } - - /// Insert a node into the flat connection pool for RLPx initiation. - /// Evicts the oldest entry when the pool is at capacity. - fn insert_to_connection_pool(&mut self, node_id: H256, node: Node) { - if self.connection_pool.contains_key(&node_id) { - return; - } - if self.connection_pool.len() >= MAX_CONNECTION_POOL_SIZE { - self.connection_pool.shift_remove_index(0); - } - self.connection_pool.insert(node_id, node); - } - - /// Look up a contact by node ID in either the main or replacement list. - fn get_contact_or_replacement(&self, node_id: &H256) -> Option<&Contact> { - let idx = self.bucket_for(node_id)?; - self.buckets[idx].get_any(node_id) - } - - /// Look up a mutable reference in either the main or replacement list. - fn get_contact_or_replacement_mut(&mut self, node_id: &H256) -> Option<&mut Contact> { - let idx = self.bucket_for(node_id)?; - let bucket = &mut self.buckets[idx]; - // Search main list first, then replacement list. - // Done inline to avoid borrow-checker issues with or_else closures. - if let Some(pos) = bucket.contacts.iter().position(|(id, _)| id == node_id) { - return Some(&mut bucket.contacts[pos].1); - } - if let Some(pos) = bucket.replacements.iter().position(|(id, _)| id == node_id) { - return Some(&mut bucket.replacements[pos].1); - } - None - } - - /// Iterate over all contacts across all buckets (main and replacement lists). - fn iter_contacts(&self) -> impl Iterator { - self.buckets.iter().flat_map(|bucket| { - bucket - .contacts - .iter() - .chain(bucket.replacements.iter()) - .map(|(id, c)| (id, c)) - }) - } - // --- Peer selection --- fn weight_peer(&self, score: &i64, requests: &i64) -> i64 { @@ -1281,297 +610,6 @@ impl PeerTableServer { .collect() } - // --- Contact operations --- - - /// Prune disposable contacts from both main and replacement lists. - /// When a main contact is removed, a replacement is automatically promoted. - /// Pruned contacts remain in the connection pool so they can be retried - /// later — the RLPx handshake will reject them if they're truly bad. - fn prune(&mut self) { - for bucket in &mut self.buckets { - // Collect disposable contacts from main list - let main_disposable: Vec = bucket - .contacts - .iter() - .filter(|(_, c)| c.disposable) - .map(|(id, _)| *id) - .collect(); - - // Remove from main list and promote replacements - for node_id in main_disposable { - bucket.remove_and_promote(&node_id); - } - - // Remove disposable contacts from replacement list - // (these don't get promoted, just removed) - bucket.replacements.retain(|(_, c)| !c.disposable); - } - } - - fn do_get_contact_to_initiate(&mut self) -> Option { - // Draw from the flat connection pool using O(1) random index probing. - // Pick a random start index and scan forward (wrapping) until we find - // an eligible candidate or complete a full loop. - let pool_len = self.connection_pool.len(); - if pool_len == 0 { - return None; - } - - let start = rand::random::() % pool_len; - for offset in 0..pool_len { - let idx = (start + offset) % pool_len; - let Some((node_id, node)) = self.connection_pool.get_index(idx) else { - continue; - }; - let node_id = *node_id; - - if self.peers.contains_key(&node_id) - || self.already_tried_peers.contains(&node_id) - || self - .get_contact_or_replacement(&node_id) - .map(|c| !c.knows_us || c.unwanted || c.passes_filter == Some(false)) - .unwrap_or(false) - { - continue; - } - - let node = node.clone(); - self.already_tried_peers.insert(node_id); - let contact = self - .get_contact_or_replacement(&node_id) - .cloned() - .unwrap_or_else(|| Contact::new(node, DiscoveryProtocol::Discv4)); - return Some(contact); - } - - // Exhausted all candidates — reset tried set for next cycle. - tracing::trace!("Resetting list of tried peers."); - self.already_tried_peers.clear(); - None - } - - /// Get the `count` closest nodes from the connection pool, sorted by XOR distance to `target`. - fn do_get_closest_from_pool(&self, target: H256, count: usize) -> Vec<(H256, Node)> { - let mut nodes: Vec<(H256, Node, H256)> = Vec::with_capacity(count); - - for (node_id, node) in &self.connection_pool { - let dist = xor_distance(&target, node_id); - if nodes.len() < count { - nodes.push((*node_id, node.clone(), dist)); - } else if let Some((farthest_idx, _)) = - nodes.iter().enumerate().max_by_key(|(_, (_, _, d))| *d) - && dist < nodes[farthest_idx].2 - { - nodes[farthest_idx] = (*node_id, node.clone(), dist); - } - } - - nodes.sort_by(|a, b| a.2.cmp(&b.2)); - nodes.into_iter().map(|(id, node, _)| (id, node)).collect() - } - - /// Get contact for ENR lookup (discv4 only) - fn do_get_contact_for_enr_lookup(&mut self) -> Option { - self.iter_contacts() - .filter(|(_, c)| { - c.is_discv4 - && c.was_validated() - && !c.has_pending_enr_request() - && c.record.is_none() - && !c.disposable - }) - .map(|(_, c)| c) - .collect::>() - .choose(&mut rand::rngs::OsRng) - .cloned() - .cloned() - } - - fn do_get_contact_to_revalidate( - &self, - revalidation_interval: Duration, - protocol: DiscoveryProtocol, - ) -> Option> { - self.iter_contacts() - .filter(|(_, c)| { - c.supports_protocol(protocol) - && Self::is_validation_needed(c, revalidation_interval) - }) - .map(|(_, c)| c) - .choose(&mut rand::rngs::OsRng) - .cloned() - .map(Box::new) - } - - fn do_validate_contact(&self, node_id: H256, sender_ip: IpAddr) -> ContactValidation { - let Some(contact) = self.get_contact(&node_id) else { - return ContactValidation::UnknownContact; - }; - if !contact.was_validated() { - return ContactValidation::InvalidContact; - } - - // Check that the IP address from which we receive the request matches the one we have stored - // to prevent amplification attacks. - if sender_ip != contact.node.ip { - return ContactValidation::IpMismatch; - } - ContactValidation::Valid(Box::new(contact.clone())) - } - - /// Get closest nodes using raw XOR distance for accurate ordering. - fn do_get_closest_nodes(&self, node_id: H256) -> Vec { - #[cfg(feature = "metrics")] - let scan_start = std::time::Instant::now(); - - let mut nodes: Vec<(Node, H256)> = vec![]; - - for (contact_id, contact) in self.iter_contacts() { - let dist = xor_distance(&node_id, contact_id); - if nodes.len() < MAX_NODES_IN_NEIGHBORS_PACKET { - nodes.push((contact.node.clone(), dist)); - } else if let Some((farthest_idx, _)) = - nodes.iter().enumerate().max_by_key(|(_, (_, d))| *d) - && dist < nodes[farthest_idx].1 - { - nodes[farthest_idx] = (contact.node.clone(), dist); - } - } - - #[cfg(feature = "metrics")] - { - use ethrex_metrics::p2p::METRICS_P2P; - METRICS_P2P.observe_iter_contacts_duration(scan_start.elapsed().as_secs_f64()); - } - - nodes.into_iter().map(|(node, _)| node).collect() - } - - /// Get nodes at distances for discv5 (returns Vec). - /// Uses the discv5 spec log-distance: `floor(log2(XOR))` for non-zero XOR. - /// Distance 0 is reserved for the local node itself (handled by the caller), - /// so contacts start at distance >= 1. - fn do_get_nodes_at_distances(&self, distances: &[u32]) -> Vec { - self.iter_contacts() - .filter_map(|(contact_id, contact)| { - let dist = distance(&self.local_node_id, contact_id) as u32; - if distances.contains(&dist) { - contact.record.clone() - } else { - None - } - }) - .take(MAX_ENRS_PER_FINDNODE_RESPONSE) - .collect() - } - - async fn do_new_contacts(&mut self, nodes: Vec, protocol: DiscoveryProtocol) { - for node in nodes { - let node_id = node.node_id(); - if node_id == self.local_node_id { - continue; - } - #[cfg(feature = "metrics")] - let insert_start = std::time::Instant::now(); - - // Always add to the connection pool (regardless of k-bucket capacity) - self.insert_to_connection_pool(node_id, node.clone()); - - if self.contact_exists(&node_id) { - // Contact already exists (main or replacement list), update protocol - if let Some(contact) = self.get_contact_or_replacement_mut(&node_id) { - contact.add_protocol(protocol); - } - } else { - let contact = Contact::new(node, protocol); - self.insert_contact(node_id, contact); - METRICS.record_new_discovery().await; - } - - #[cfg(feature = "metrics")] - { - use ethrex_metrics::p2p::METRICS_P2P; - METRICS_P2P.observe_insert_contact_duration(insert_start.elapsed().as_secs_f64()); - } - } - } - - fn do_record_enr_request_sent(&mut self, node_id: H256, request_hash: H256) { - if let Some(contact) = self.get_contact_mut(&node_id) { - contact.record_enr_request_sent(request_hash); - } - } - - fn do_record_enr_response_received( - &mut self, - node_id: H256, - request_hash: H256, - record: NodeRecord, - ) { - // Filtered here, before the mutable borrow, so a record that reaches us - // over discv4 is judged by the same filter as one that arrives over - // discv5. The verdict is recorded only if the record was actually - // stored, so it always describes the record the contact holds. - let passes_filter = self.filter.accepts(&record); - if let Some(contact) = self.get_contact_mut(&node_id) - && contact.record_enr_response_received(request_hash, record) - { - contact.passes_filter = Some(passes_filter); - } - } - - async fn do_new_contact_records(&mut self, node_records: Vec) { - for node_record in node_records { - if !node_record.verify_signature() { - continue; - } - if let Ok(node) = Node::from_enr(&node_record) { - let node_id = node.node_id(); - if node_id == self.local_node_id { - continue; - } - - // Always add to the connection pool (regardless of k-bucket capacity) - self.insert_to_connection_pool(node_id, node.clone()); - - if self.contact_exists(&node_id) { - // Check if we need to evaluate fork_id before taking - // the mutable borrow. - let should_update = self - .get_contact_or_replacement(&node_id) - .map(|c| match c.record.as_ref() { - None => true, - Some(r) => node_record.seq > r.seq, - }) - .unwrap_or(false); - // Filtered here, before the mutable borrow, and only when - // the record is newer than the one we already hold. - let passes_filter = should_update.then(|| self.filter.accepts(&node_record)); - if let Some(contact) = self.get_contact_or_replacement_mut(&node_id) { - contact.add_protocol(DiscoveryProtocol::Discv5); - if should_update { - if contact.node.ip != node.ip || contact.node.udp_port != node.udp_port - { - contact.validation_timestamp = None; - contact.ping_id = None; - } - contact.node = node; - contact.record = Some(node_record); - contact.passes_filter = passes_filter; - } - } - } else { - let passes_filter = self.filter.accepts(&node_record); - let mut contact = Contact::new(node, DiscoveryProtocol::Discv5); - contact.passes_filter = Some(passes_filter); - contact.record = Some(node_record); - self.insert_contact(node_id, contact); - METRICS.record_new_discovery().await; - } - } - } - } - fn do_peer_count_by_capabilities(&self, capabilities: Vec) -> usize { self.peers .values() @@ -1625,406 +663,6 @@ impl PeerTableServer { let idx = dist.sample(&mut rand::rngs::OsRng); Some((peers[idx].0, peers[idx].1.clone(), peers[idx].3.clone())) } - - fn is_validation_needed(contact: &Contact, revalidation_interval: Duration) -> bool { - if contact.disposable { - return false; - } - - let sent_ping_ttl = Duration::from_secs(30); - - if contact.has_pending_ping() { - // Outstanding ping — only re-ping if it timed out (stale). - contact - .validation_timestamp - .map(|ts| Instant::now().saturating_duration_since(ts) > sent_ping_ttl) - .unwrap_or(false) - } else { - // No pending ping — check if never validated or validation expired. - !contact.was_validated() - || contact - .validation_timestamp - .map(|ts| Instant::now().saturating_duration_since(ts) > revalidation_interval) - .unwrap_or(false) - } - } } pub type PeerTable = ActorRef; - -#[cfg(test)] -mod tests { - use super::*; - use crate::types::NodeRecordPairs; - use ethrex_common::H512; - use std::net::Ipv4Addr; - - /// Helper: build a dummy contact with a unique node derived from `seed`. - fn dummy_contact(seed: u8) -> (H256, Contact) { - let pk = H512::from_low_u64_be(seed as u64 + 1); - let node = Node::new(IpAddr::V4(Ipv4Addr::new(127, 0, 0, seed)), 30303, 30303, pk); - let node_id = node.node_id(); - let contact = Contact::new(node, DiscoveryProtocol::Discv4); - (node_id, contact) - } - - /// A filter with a fixed answer, so peer-table behaviour can be exercised - /// without a storage engine or a real chain behind it. - struct FixedAnswer(bool); - - impl PeerFilter for FixedAnswer { - fn accepts(&self, _record: &NodeRecord) -> bool { - self.0 - } - } - - fn table_with(filter: impl PeerFilter + 'static) -> PeerTableServer { - PeerTableServer::new(H256::zero(), 10, Box::new(filter)) - } - - /// A signed record for `seed`'s node at sequence number `seq`. - fn record_for(seed: u8, seq: u64) -> (H256, NodeRecord) { - let signer = secp256k1::SecretKey::from_slice(&[seed.max(1); 32]).unwrap(); - let record = NodeRecord::from_pairs( - seq, - &signer, - NodeRecordPairs { - ip: Some(Ipv4Addr::new(127, 0, 0, seed)), - udp_port: Some(30303), - ..Default::default() - }, - ) - .unwrap(); - (Node::from_enr(&record).unwrap().node_id(), record) - } - - // --- the filter decides which contacts are dialable --- - - #[tokio::test] - async fn an_arriving_record_is_run_through_the_filter() { - let mut table = table_with(FixedAnswer(false)); - let (node_id, record) = record_for(1, 1); - - table.do_new_contact_records(vec![record]).await; - - let contact = table.get_contact(&node_id).expect("contact inserted"); - assert_eq!(contact.passes_filter, Some(false)); - } - - #[tokio::test] - async fn a_rejected_contact_is_never_offered_for_dialing() { - let mut table = table_with(FixedAnswer(false)); - let (node_id, record) = record_for(2, 1); - - table.do_new_contact_records(vec![record]).await; - assert!(table.get_contact(&node_id).is_some(), "contact is present"); - - assert!( - table.do_get_contact_to_initiate().is_none(), - "a rejected contact must not be handed out to dial" - ); - } - - #[tokio::test] - async fn an_accepted_contact_is_offered_for_dialing() { - let mut table = table_with(FixedAnswer(true)); - let (node_id, record) = record_for(3, 1); - - table.do_new_contact_records(vec![record]).await; - - // Asserting the stored answer too, not just dialability: `None` is also - // dialable, so `is_some()` alone would pass even if the filter never ran. - assert_eq!( - table.get_contact(&node_id).unwrap().passes_filter, - Some(true) - ); - assert!(table.do_get_contact_to_initiate().is_some()); - } - - #[tokio::test] - async fn a_contact_discovered_without_a_record_is_never_filtered() { - // Bootnodes and discv4 neighbours arrive as bare endpoints. They have - // published nothing to judge, so they must stay dialable rather than be - // written off by a filter that never saw them. - let mut table = table_with(FixedAnswer(false)); - let (node_id, record) = record_for(6, 1); - let node = Node::from_enr(&record).unwrap(); - - table - .do_new_contacts(vec![node], DiscoveryProtocol::Discv4) - .await; - - assert_eq!(table.get_contact(&node_id).unwrap().passes_filter, None); - assert!(table.do_get_contact_to_initiate().is_some()); - } - - #[tokio::test] - async fn a_discv4_enr_response_is_run_through_the_filter() { - // The discv4 path used to bypass the filter entirely and write a - // hardcoded fork-id verdict into the same field, so a consumer's own - // policy was overridden depending on which protocol found the peer. - let mut table = table_with(FixedAnswer(false)); - let (node_id, record) = record_for(7, 1); - let node = Node::from_enr(&record).unwrap(); - let request_hash = H256::repeat_byte(0xab); - - table - .do_new_contacts(vec![node], DiscoveryProtocol::Discv4) - .await; - table.do_record_enr_request_sent(node_id, request_hash); - table.do_record_enr_response_received(node_id, request_hash, record); - - assert_eq!( - table.get_contact(&node_id).unwrap().passes_filter, - Some(false) - ); - } - - #[tokio::test] - async fn an_unsolicited_enr_response_does_not_set_the_verdict() { - // The record is not stored when the hash does not match, so recording a - // verdict from it would let a peer restate its own standing from a - // record the table refused to keep. - let mut table = table_with(FixedAnswer(true)); - let (node_id, record) = record_for(8, 1); - let node = Node::from_enr(&record).unwrap(); - - table - .do_new_contacts(vec![node], DiscoveryProtocol::Discv4) - .await; - table.do_record_enr_request_sent(node_id, H256::repeat_byte(0x01)); - table.do_record_enr_response_received(node_id, H256::repeat_byte(0x02), record); - - let contact = table.get_contact(&node_id).unwrap(); - assert_eq!(contact.passes_filter, None); - assert!(contact.record.is_none(), "the record must not be stored"); - } - - /// Rejects the first record it is shown and accepts every later one, so a - /// test can tell whether a second record was filtered at all. - #[derive(Default)] - struct AcceptsFromTheSecondRecordOn(std::sync::atomic::AtomicUsize); - - impl PeerFilter for AcceptsFromTheSecondRecordOn { - fn accepts(&self, _record: &NodeRecord) -> bool { - self.0.fetch_add(1, std::sync::atomic::Ordering::SeqCst) > 0 - } - } - - #[tokio::test] - async fn a_rejection_is_reconsidered_on_a_newer_record() { - // The reason a rejection is stored rather than acted on once: the peer - // republishes and we look again, instead of writing it off for the life - // of the process over a fork id read against a head we had not synced. - let mut table = table_with(AcceptsFromTheSecondRecordOn::default()); - let (node_id, first) = record_for(4, 1); - let (_, newer) = record_for(4, 2); - - table.do_new_contact_records(vec![first]).await; - assert_eq!( - table.get_contact(&node_id).unwrap().passes_filter, - Some(false) - ); - - table.do_new_contact_records(vec![newer]).await; - assert_eq!( - table.get_contact(&node_id).unwrap().passes_filter, - Some(true), - "a higher-seq record must get a fresh hearing" - ); - } - - #[tokio::test] - async fn an_older_record_does_not_re_filter_the_contact() { - // `should_update` false means the record has nothing new to say, so the - // answer already on the contact has to survive it. - let mut table = table_with(AcceptsFromTheSecondRecordOn::default()); - let (node_id, first) = record_for(5, 2); - let (_, older) = record_for(5, 1); - - table.do_new_contact_records(vec![first]).await; - table.do_new_contact_records(vec![older]).await; - - assert_eq!( - table.get_contact(&node_id).unwrap().passes_filter, - Some(false), - "a stale record must not overwrite the answer we hold" - ); - } - - // --- KBucket::insert --- - - #[test] - fn insert_into_empty_bucket() { - let mut bucket = KBucket::default(); - let (id, contact) = dummy_contact(1); - assert!(bucket.insert(id, contact)); - assert_eq!(bucket.contacts.len(), 1); - assert!(bucket.replacements.is_empty()); - } - - #[test] - fn insert_fills_bucket_then_goes_to_replacements() { - let mut bucket = KBucket::default(); - - // Fill the main list to capacity. - for i in 0..MAX_NODES_PER_BUCKET as u8 { - let (id, contact) = dummy_contact(i); - assert!(bucket.insert(id, contact), "contact {i} should go to main"); - } - assert_eq!(bucket.contacts.len(), MAX_NODES_PER_BUCKET); - - // The next insert should go to the replacement list. - let (id, contact) = dummy_contact(200); - assert!(!bucket.insert(id, contact)); - assert_eq!(bucket.contacts.len(), MAX_NODES_PER_BUCKET); - assert_eq!(bucket.replacements.len(), 1); - } - - // --- KBucket::contains --- - - #[test] - fn contains_checks_main_and_replacement() { - let mut bucket = KBucket::default(); - - let (id_main, contact_main) = dummy_contact(1); - bucket.insert(id_main, contact_main); - assert!(bucket.contains(&id_main)); - - // Fill bucket so next goes to replacement. - for i in 2..=(MAX_NODES_PER_BUCKET as u8) { - let (id, c) = dummy_contact(i); - bucket.insert(id, c); - } - let (id_repl, contact_repl) = dummy_contact(100); - bucket.insert(id_repl, contact_repl); - - assert!(bucket.contains(&id_repl)); - assert!(!bucket.contains(&H256::zero())); - } - - // --- KBucket::get / get_any --- - - #[test] - fn get_returns_main_list_only() { - let mut bucket = KBucket::default(); - let (id, contact) = dummy_contact(1); - bucket.insert(id, contact); - assert!(bucket.get(&id).is_some()); - assert!(bucket.get(&H256::zero()).is_none()); - } - - #[test] - fn get_any_returns_from_replacement() { - let mut bucket = KBucket::default(); - // Fill main list. - for i in 0..MAX_NODES_PER_BUCKET as u8 { - let (id, c) = dummy_contact(i); - bucket.insert(id, c); - } - // Insert into replacements. - let (id_repl, c_repl) = dummy_contact(200); - bucket.insert(id_repl, c_repl); - - assert!(bucket.get(&id_repl).is_none()); // not in main - assert!(bucket.get_any(&id_repl).is_some()); // found via replacement - } - - // --- KBucket::remove_and_promote --- - - #[test] - fn remove_and_promote_with_replacement() { - let mut bucket = KBucket::default(); - - // Fill main list. - let mut main_ids = Vec::new(); - for i in 0..MAX_NODES_PER_BUCKET as u8 { - let (id, c) = dummy_contact(i); - main_ids.push(id); - bucket.insert(id, c); - } - - // Add a replacement. - let (repl_id, repl_contact) = dummy_contact(200); - bucket.insert(repl_id, repl_contact); - - // Remove a main contact — the replacement should be promoted. - let promoted = bucket.remove_and_promote(&main_ids[0]); - assert_eq!(promoted, Some(repl_id)); - assert_eq!(bucket.contacts.len(), MAX_NODES_PER_BUCKET); - assert!(bucket.replacements.is_empty()); - assert!(!bucket.contains(&main_ids[0])); - assert!(bucket.contains(&repl_id)); - } - - #[test] - fn remove_and_promote_without_replacement() { - let mut bucket = KBucket::default(); - let (id, c) = dummy_contact(1); - bucket.insert(id, c); - - let promoted = bucket.remove_and_promote(&id); - assert!(promoted.is_none()); - assert!(bucket.contacts.is_empty()); - } - - #[test] - fn remove_nonexistent_returns_none() { - let mut bucket = KBucket::default(); - assert!(bucket.remove_and_promote(&H256::zero()).is_none()); - } - - // --- Replacement eviction --- - - #[test] - fn replacement_list_evicts_oldest_when_full() { - let mut bucket = KBucket::default(); - // Fill main list. - for i in 0..MAX_NODES_PER_BUCKET as u8 { - let (id, c) = dummy_contact(i); - bucket.insert(id, c); - } - - // Fill replacement list beyond capacity. - let mut repl_ids = Vec::new(); - for i in 0..(MAX_REPLACEMENTS_PER_BUCKET + 2) as u8 { - let seed = 100 + i; - let (id, c) = dummy_contact(seed); - repl_ids.push(id); - bucket.insert(id, c); - } - - assert_eq!(bucket.replacements.len(), MAX_REPLACEMENTS_PER_BUCKET); - // The oldest two should have been evicted. - assert!(!bucket.contains(&repl_ids[0])); - assert!(!bucket.contains(&repl_ids[1])); - // The most recent ones should still be there. - assert!(bucket.contains(repl_ids.last().unwrap())); - } - - // --- bucket_index --- - - #[test] - fn bucket_index_self_is_none() { - let id = H256::random(); - assert_eq!(bucket_index(&id, &id), None); - } - - #[test] - fn bucket_index_minimal_distance() { - let local = H256::zero(); - // XOR distance = 1 → highest bit is bit 0 → bucket 0 - let mut remote = H256::zero(); - remote.0[31] = 1; - assert_eq!(bucket_index(&local, &remote), Some(0)); - } - - #[test] - fn bucket_index_maximal_distance() { - let local = H256::zero(); - // XOR distance has highest bit at position 255 → bucket 255 - let mut remote = H256::zero(); - remote.0[0] = 0x80; - assert_eq!(bucket_index(&local, &remote), Some(255)); - } -} diff --git a/crates/networking/p2p/rlpx/connection/handshake.rs b/crates/networking/p2p/rlpx/connection/handshake.rs index 77c843ff5de..74a20eda6b1 100644 --- a/crates/networking/p2p/rlpx/connection/handshake.rs +++ b/crates/networking/p2p/rlpx/connection/handshake.rs @@ -145,6 +145,7 @@ pub(crate) async fn perform( client_version: context.client_version.clone(), connection_broadcast_send: context.broadcast.clone(), peer_table: context.table.clone(), + discovery: context.discovery.clone(), #[cfg(feature = "l2")] l2_state: context .based_context diff --git a/crates/networking/p2p/rlpx/connection/server.rs b/crates/networking/p2p/rlpx/connection/server.rs index 106d778a5da..2f3e5f1fe3c 100644 --- a/crates/networking/p2p/rlpx/connection/server.rs +++ b/crates/networking/p2p/rlpx/connection/server.rs @@ -7,6 +7,7 @@ use crate::rlpx::l2::{ }; use crate::{ backend, + discovery::DiscoveryHandle, metrics::METRICS, network::P2PContext, peer_table::{PeerTable, PeerTableServerProtocol as _}, @@ -286,6 +287,9 @@ pub struct Established { /// See https://github.com/lambdaclass/ethrex/issues/3388 pub(crate) connection_broadcast_send: PeerConnBroadcastSender, pub(crate) peer_table: PeerTable, + /// Reports this connection's lifecycle back to discovery, so a node we are + /// talking to stops being offered as a dial candidate. + pub(crate) discovery: DiscoveryHandle, #[cfg(feature = "l2")] pub(crate) l2_state: L2ConnState, pub(crate) tx_broadcaster: ActorRef, @@ -389,12 +393,9 @@ impl PeerConnectionServer { match &reason { PeerConnectionError::NoMatchingCapabilities | PeerConnectionError::HandshakeError(_) => { - if let Err(e) = established_state - .peer_table - .set_unwanted(established_state.node.node_id()) - { - debug!("Failed to set peer as unwanted: {e}"); - } + established_state + .discovery + .set_unwanted(established_state.node.node_id()); } _ => {} } @@ -461,6 +462,12 @@ impl PeerConnectionServer { { debug!("Failed to remove peer from table: {e}"); } + // Pairs with the `mark_connected` in `initialize_connection`: this + // branch is the only teardown an established connection takes, and + // it runs even when a handler panicked. + established_state + .discovery + .mark_disconnected(established_state.node.node_id()); // Free the peer's tx-broadcaster index (and clear its bit across known txs) so // the broadcaster's per-peer index map / PeerMask widths stay bounded to live peers. if let Err(e) = established_state @@ -822,6 +829,7 @@ where state.capabilities.clone(), state.is_inbound, )?; + state.discovery.mark_connected(state.node.node_id()); trace!(peer=%state.node, "Peer connection initialized."); diff --git a/crates/networking/p2p/rlpx/initiator.rs b/crates/networking/p2p/rlpx/initiator.rs index 2f339415a81..001be674eab 100644 --- a/crates/networking/p2p/rlpx/initiator.rs +++ b/crates/networking/p2p/rlpx/initiator.rs @@ -107,8 +107,8 @@ impl RLPxInitiator { async fn do_look_for_peer(&mut self) -> Result<(), RLPxInitiatorError> { if !self.context.table.target_peers_reached().await? { - if let Some(contact) = self.context.table.get_contact_to_initiate().await? { - PeerConnection::spawn_as_initiator(self.context.clone(), &contact.node); + if let Some(node) = self.context.discovery.next_dial_candidate().await { + PeerConnection::spawn_as_initiator(self.context.clone(), &node); METRICS.record_new_rlpx_conn_attempt().await; }; } else { diff --git a/crates/networking/p2p/sync/snap_sync.rs b/crates/networking/p2p/sync/snap_sync.rs index 950f2bc70fa..45ba74291f1 100644 --- a/crates/networking/p2p/sync/snap_sync.rs +++ b/crates/networking/p2p/sync/snap_sync.rs @@ -153,7 +153,7 @@ pub async fn sync_cycle_snap( loop { // Prune dead/unresponsive peers periodically to allow replacements to be promoted - let _ = peers.peer_table.prune_table(); + peers.discovery.prune(); debug!("Requesting Block Headers from {current_head}"); diff --git a/crates/networking/rpc/test_utils.rs b/crates/networking/rpc/test_utils.rs index 19aa82589ca..2310836cac4 100644 --- a/crates/networking/rpc/test_utils.rs +++ b/crates/networking/rpc/test_utils.rs @@ -27,6 +27,7 @@ use ethrex_common::{ }, }; use ethrex_p2p::{ + discovery::DiscoveryHandle, network::P2PContext, peer_handler::PeerHandler, peer_table::{PeerTable, PeerTableServer, TARGET_PEERS}, @@ -379,9 +380,14 @@ pub async fn dummy_sync_manager() -> SyncManager { /// Creates a dummy PeerHandler for tests where interacting with peers is not needed /// This should only be used in tests as it won't be able to interact with the node's connected peers -pub async fn dummy_peer_handler(store: Store) -> PeerHandler { - let peer_table = PeerTableServer::spawn(H256::random(), TARGET_PEERS, store); - PeerHandler::new(peer_table.clone(), dummy_actor(peer_table).await) +pub async fn dummy_peer_handler(_store: Store) -> PeerHandler { + let peer_table = PeerTableServer::spawn(TARGET_PEERS); + // No discovery server behind this handle: the dummy handler never dials. + PeerHandler::new( + peer_table.clone(), + dummy_actor(peer_table).await, + DiscoveryHandle::new(), + ) } /// Creates a dummy RLPx initiator actor for tests diff --git a/test/tests/p2p/discovery/discv5_server_tests.rs b/test/tests/p2p/discovery/discv5_server_tests.rs index 9f1c0272d59..85e9c8249e0 100644 --- a/test/tests/p2p/discovery/discv5_server_tests.rs +++ b/test/tests/p2p/discovery/discv5_server_tests.rs @@ -1,11 +1,10 @@ use bytes::Bytes; use ethrex_common::H256; use ethrex_p2p::discovery::DiscoveryServer; +use ethrex_p2p::discovery::{ContactTable, Session as ContactSession}; use ethrex_p2p::discv5::messages::PongMessage; -use ethrex_p2p::discv5::session::Session; -use ethrex_p2p::peer_table::{PeerTable, PeerTableServer, PeerTableServerProtocol as _}; +use ethrex_p2p::peer_filter::AcceptAllFilter; use ethrex_p2p::types::{Node, NodeRecord}; -use ethrex_storage::{EngineType, Store}; use rand::{SeedableRng, rngs::StdRng}; use rustc_hash::FxHashSet; use secp256k1::SecretKey; @@ -16,26 +15,23 @@ use std::{ }; use tokio::net::UdpSocket; -async fn test_server(peer_table: Option) -> DiscoveryServer { +async fn test_server(contacts: Option) -> DiscoveryServer { let local_node = Node::from_enode_url( "enode://d860a01f9722d78051619d1e2351aba3f43f943f6f00718d1b9baa4101932a1f5011f16bb2b1bb35db20d6fe28fa0bf09636d26a87d31de9ec6203eeedb1f666@18.138.108.67:30303", ).expect("Bad enode url"); let signer = SecretKey::new(&mut rand::rngs::OsRng); let local_node_record = NodeRecord::from_node(&local_node, 1, &signer).unwrap(); - let peer_table = peer_table.unwrap_or_else(|| { - PeerTableServer::spawn( - local_node.node_id(), - 10, - Store::new("", EngineType::InMemory).expect("Failed to create store"), - ) - }); - DiscoveryServer::new_for_discv5_test( + let mut server = DiscoveryServer::new_for_discv5_test( local_node, local_node_record, signer, Arc::new(UdpSocket::bind("127.0.0.1:0").await.unwrap()), - peer_table, - ) + Box::new(AcceptAllFilter), + ); + if let Some(contacts) = contacts { + *server.contacts_mut() = contacts; + } + server } /// Helper to get a mutable reference to the discv5 state. @@ -175,34 +171,29 @@ async fn test_enr_update_request_on_pong() { let remote_node = Node::from_enr(&remote_record).expect("Should create node from record"); let remote_node_id = remote_node.node_id(); - let peer_table = PeerTableServer::spawn( - local_node.node_id(), - 10, - Store::new("", EngineType::InMemory).expect("Failed to create store"), + let mut contacts = ContactTable::new(local_node.node_id(), 10, Box::new(AcceptAllFilter)); + contacts.new_contact_records(vec![remote_record]).await; + contacts.set_session( + remote_node_id, + ContactSession { + outbound_key: [0u8; 16], + inbound_key: [0u8; 16], + }, ); - peer_table.new_contact_records(vec![remote_record]).unwrap(); - - let session = Session { - outbound_key: [0u8; 16], - inbound_key: [0u8; 16], - }; - peer_table - .set_session_info(remote_node_id, session) - .unwrap(); - let mut server = DiscoveryServer::new_for_discv5_test( local_node, local_node_record, signer, Arc::new(UdpSocket::bind("127.0.0.1:0").await.unwrap()), - peer_table, + Box::new(AcceptAllFilter), ); + *server.contacts_mut() = contacts; - let contact = server.peer_table.get_contact(remote_node_id).await.unwrap(); + let contact = server.contacts_mut().get_contact(&remote_node_id).cloned(); assert!( contact.is_some(), - "Contact should have been added to peer_table" + "Contact should have been added to the contact table" ); let contact = contact.unwrap(); assert_eq!( From dac34dc773675399eeac3e4bdc1a03a2f1fb80bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Gr=C3=BCner?= <47506558+MegaRedHand@users.noreply.github.com> Date: Tue, 25 Aug 2026 16:29:27 -0300 Subject: [PATCH 02/11] fix(l1): dial the contact's endpoint, and stop RLPx teardown from dropping discv5 sessions Two defects found reviewing the contact-table move. `next_dial_candidate` handed the dialer the node from the connection pool. The pool is written once on first sight and never refreshed, while the contact's node is replaced whenever a higher-seq ENR arrives, so the two diverge: a peer first heard of over an unauthenticated discv4 Neighbors packet sits in the pool with whatever endpoint that packet claimed, often TCP port 0, and every later dial used it even after the peer published a signed record correcting the address. `already_tried_peers` clears once the pool is exhausted, so it retried the wrong address indefinitely. The previous code returned the k-bucket contact and only fell back to the pool when the buckets had evicted the id; restore that, and keep the pool entry as the fallback it was. Removing `Contact.session` made the disconnect cleanup effective for the first time, which exposed that it fires too widely: the connection actor's teardown runs for any state that reached `Established`, including attempts rejected during capability negotiation, so an RLPx handshake we turned down for having no `eth` capability destroyed a perfectly good discv5 session and forced a WHOAREYOU round trip to reach a node we could already talk to. The consumer's connection and the discovery session are separate conversations with the same node, so `mark_disconnected` no longer touches sessions. `prune` drops them along with the contact instead, which is both the layer that owns their lifetime and what bounds the store. --- .../networking/p2p/discovery/contact_table.rs | 138 ++++++++++++++---- crates/networking/p2p/discovery/server.rs | 44 ++++++ crates/networking/p2p/peer_table.rs | 4 +- crates/networking/rpc/test_utils.rs | 8 +- .../p2p/discovery/discv5_server_tests.rs | 32 ++-- 5 files changed, 177 insertions(+), 49 deletions(-) diff --git a/crates/networking/p2p/discovery/contact_table.rs b/crates/networking/p2p/discovery/contact_table.rs index 724650b507c..fc7d3c7280b 100644 --- a/crates/networking/p2p/discovery/contact_table.rs +++ b/crates/networking/p2p/discovery/contact_table.rs @@ -318,8 +318,8 @@ pub struct ContactTable { /// contact as [`Contact::passes_filter`]. filter: Box, /// Nodes the consumer has reported as connected. Kept here rather than read - /// back from the consumer so discovery never has to call into it: the two - /// lifecycle casts are the only thing crossing the boundary. + /// back from the consumer, so that discovery never has to call into it: + /// every message across that boundary travels inward. connected: FxHashSet, /// Nodes already offered to the dialer this cycle, cleared once the pool is /// exhausted so failed dials get another turn. @@ -367,12 +367,14 @@ impl ContactTable { /// Record that the consumer's connection to `node_id` is gone. /// - /// Also drops the node's discv5 session: the keys were negotiated for a - /// peer we are no longer talking to, and keeping them would leave the - /// session store growing with every peer that ever connected. + /// Deliberately leaves the discv5 session alone. The consumer's connection + /// and the discovery session are separate conversations with the same node: + /// an RLPx attempt that is rejected for having no capability we want says + /// nothing about the discv5 keys, and tearing them down there would force a + /// WHOAREYOU round trip to talk to a node we are still perfectly able to + /// reach. Sessions go when the contact does, in [`Self::prune`]. pub fn mark_disconnected(&mut self, node_id: &H256) { self.connected.remove(node_id); - self.sessions.remove(node_id); } /// How far along the consumer is towards the connection count it wants. @@ -561,8 +563,13 @@ impl ContactTable { /// Prune disposable contacts from both main and replacement lists. /// When a main contact is removed, a replacement is automatically promoted. /// Pruned contacts remain in the connection pool so they can be retried - /// later — the RLPx handshake will reject them if they're truly bad. + /// later: the consumer will reject them on connecting if they are truly bad. + /// + /// Dropping a contact drops its discv5 session too. That is what bounds the + /// session store, which would otherwise grow by one entry per handshake for + /// the life of the process. pub fn prune(&mut self) { + let mut pruned: Vec = Vec::new(); for bucket in &mut self.buckets { // Collect disposable contacts from main list let main_disposable: Vec = bucket @@ -575,11 +582,20 @@ impl ContactTable { // Remove from main list and promote replacements for node_id in main_disposable { bucket.remove_and_promote(&node_id); + pruned.push(node_id); } // Remove disposable contacts from replacement list // (these don't get promoted, just removed) - bucket.replacements.retain(|(_, c)| !c.disposable); + bucket.replacements.retain(|(id, c)| { + if c.disposable { + pruned.push(*id); + } + !c.disposable + }); + } + for node_id in pruned { + self.sessions.remove(&node_id); } } @@ -602,22 +618,28 @@ impl ContactTable { let start = rand::random::() % pool_len; for offset in 0..pool_len { let idx = (start + offset) % pool_len; - let Some((node_id, node)) = self.connection_pool.get_index(idx) else { + let Some((node_id, pool_node)) = self.connection_pool.get_index(idx) else { continue; }; let node_id = *node_id; + let contact = self.get_contact_or_replacement(&node_id); if self.connected.contains(&node_id) || self.already_tried_peers.contains(&node_id) - || self - .get_contact_or_replacement(&node_id) + || contact .map(|c| !c.knows_us || c.unwanted || c.passes_filter == Some(false)) .unwrap_or(false) { continue; } - let node = node.clone(); + // The contact's endpoint wins over the pool's. A pool entry is + // written on first sight and never refreshed, so for a node first + // heard of over an unauthenticated discv4 Neighbors packet it can + // hold a wrong or zero TCP port forever, while the contact tracks + // whatever the newest signed ENR says. Falls back to the pool for + // an id the k-buckets have already evicted. + let node = contact.map_or_else(|| pool_node.clone(), |c| c.node.clone()); self.already_tried_peers.insert(node_id); return Some(node); } @@ -904,6 +926,14 @@ mod tests { ContactTable::new(H256::zero(), 10, Box::new(filter)) } + /// Session keys whose values are irrelevant; only presence is asserted. + fn session() -> Session { + Session { + outbound_key: [1; 16], + inbound_key: [2; 16], + } + } + /// A signed record for `seed`'s node at sequence number `seq`. fn record_for(seed: u8, seq: u64) -> (H256, NodeRecord) { let signer = secp256k1::SecretKey::from_slice(&[seed.max(1); 32]).unwrap(); @@ -1106,26 +1136,84 @@ mod tests { } #[tokio::test] - async fn disconnecting_drops_the_discv5_session() { - // The keys were negotiated for a peer we are no longer talking to, and - // holding them would grow the session store with every peer that ever - // connected. + async fn disconnecting_leaves_the_discv5_session_alone() { + // An RLPx teardown is not a discovery event. Dropping the keys here + // would force a WHOAREYOU round trip on a node we can still reach, and + // it fires on connections that were rejected before they ever carried + // traffic. let mut table = table_with(FixedAnswer(true)); let (node_id, record) = record_for(11, 1); table.new_contact_records(vec![record]).await; - table.set_session( - node_id, - Session { - outbound_key: [1; 16], - inbound_key: [2; 16], - }, - ); - assert!(table.session(&node_id).is_some()); + table.set_session(node_id, session()); table.mark_disconnected(&node_id); - assert!(table.session(&node_id).is_none()); + assert!(table.session(&node_id).is_some()); + } + + #[tokio::test] + async fn pruning_a_contact_drops_its_discv5_session() { + // What actually bounds the session store: without this it grows by one + // entry per handshake for the life of the process. + let mut table = table_with(FixedAnswer(true)); + let (node_id, record) = record_for(14, 1); + + table.new_contact_records(vec![record]).await; + table.set_session(node_id, session()); + table.set_disposable(&node_id); + + table.prune(); + + assert!(table.get_contact(&node_id).is_none(), "contact is pruned"); + assert!( + table.session(&node_id).is_none(), + "its session goes with it" + ); + } + + #[tokio::test] + async fn a_dial_candidate_uses_the_endpoint_from_the_newest_record() { + // The connection pool is written on first sight and never refreshed, so + // a node first heard of over an unauthenticated discv4 Neighbors packet + // sits there with whatever port that packet claimed. Dialing the pool + // entry rather than the contact would keep hammering that address after + // the node published a signed ENR correcting it. + let mut table = table_with(FixedAnswer(true)); + let signer = secp256k1::SecretKey::from_slice(&[15; 32]).unwrap(); + let record = NodeRecord::from_pairs( + 2, + &signer, + NodeRecordPairs { + ip: Some(Ipv4Addr::new(127, 0, 0, 15)), + udp_port: Some(30303), + tcp_port: Some(30303), + ..Default::default() + }, + ) + .unwrap(); + let announced = Node::from_enr(&record).unwrap(); + let node_id = announced.node_id(); + + // First sighting: a bare endpoint with the wrong TCP port. + let stale = Node::new( + IpAddr::V4(Ipv4Addr::new(127, 0, 0, 15)), + 30303, + 0, + announced.public_key, + ); + table + .new_contacts(vec![stale], DiscoveryProtocol::Discv4) + .await; + // Then the signed record, which corrects it. + table.new_contact_records(vec![record]).await; + + let candidate = table.next_dial_candidate().expect("a candidate"); + assert_eq!(candidate.node_id(), node_id); + assert_eq!( + candidate.tcp_port, 30303, + "the dialer must get the endpoint from the newest record, not the first sighting" + ); } #[tokio::test] diff --git a/crates/networking/p2p/discovery/server.rs b/crates/networking/p2p/discovery/server.rs index 5e4ae45d0a3..e80319dbb86 100644 --- a/crates/networking/p2p/discovery/server.rs +++ b/crates/networking/p2p/discovery/server.rs @@ -607,3 +607,47 @@ impl DiscoveryServer { } } } + +#[cfg(test)] +mod tests { + use super::*; + + #[tokio::test] + async fn an_unpublished_handle_is_inert() { + // Every consumer holds this handle from the moment the P2P context is + // built, which is before discovery starts and forever if p2p runs + // without it. Casts must be dropped quietly and the one request must + // answer `None` rather than wait for a server that may never arrive. + let handle = DiscoveryHandle::new(); + + handle.mark_connected(H256::repeat_byte(1)); + handle.mark_disconnected(H256::repeat_byte(1)); + handle.set_unwanted(H256::repeat_byte(1)); + handle.set_disposable(H256::repeat_byte(1)); + handle.prune(); + + assert!(handle.next_dial_candidate().await.is_none()); + } + + #[tokio::test] + async fn a_handle_is_published_once() { + let handle = DiscoveryHandle::new(); + let server = DiscoveryServer::new_for_discv5_test( + Node::from_enode_url( + "enode://d860a01f9722d78051619d1e2351aba3f43f943f6f00718d1b9baa4101932a1f5011f16bb2b1bb35db20d6fe28fa0bf09636d26a87d31de9ec6203eeedb1f666@18.138.108.67:30303", + ) + .expect("bad enode url"), + NodeRecord::default(), + SecretKey::new(&mut rand::rngs::OsRng), + Arc::new(UdpSocket::bind("127.0.0.1:0").await.expect("bind")), + Box::new(crate::peer_filter::AcceptAllFilter), + ) + .start(); + + assert!(handle.set(server.clone())); + assert!( + !handle.set(server), + "a second publish must be refused, not silently swap the server" + ); + } +} diff --git a/crates/networking/p2p/peer_table.rs b/crates/networking/p2p/peer_table.rs index 3cc26f27a47..55d2fd8c28c 100644 --- a/crates/networking/p2p/peer_table.rs +++ b/crates/networking/p2p/peer_table.rs @@ -191,8 +191,8 @@ pub trait PeerTableServerProtocol: Send + Sync { pub struct PeerTableServer { peers: IndexMap, - /// How many connections this node wants. Only ever compared against - /// `peers.len()`; discovery keeps its own copy to pace its lookups. + /// How many connections this node wants. Discovery keeps its own copy to + /// pace its lookups, fed from the same config value. target_peers: usize, } diff --git a/crates/networking/rpc/test_utils.rs b/crates/networking/rpc/test_utils.rs index 2310836cac4..b16f8c1ba23 100644 --- a/crates/networking/rpc/test_utils.rs +++ b/crates/networking/rpc/test_utils.rs @@ -284,7 +284,7 @@ pub async fn start_test_api() -> tokio::task::JoinHandle<()> { local_p2p_node, local_node_record, dummy_sync_manager().await, - dummy_peer_handler(storage).await, + dummy_peer_handler().await, ClientVersion::new( "ethrex".to_string(), "0.1.0".to_string(), @@ -329,7 +329,7 @@ pub async fn default_context_with_storage(storage: Store) -> RpcApiContext { blockchain: blockchain.clone(), active_filters: Default::default(), syncer: Some(Arc::new(dummy_sync_manager().await)), - peer_handler: Some(dummy_peer_handler(storage).await), + peer_handler: Some(dummy_peer_handler().await), node_data: NodeData { jwt_secret: Default::default(), local_p2p_node: example_p2p_node(), @@ -362,7 +362,7 @@ pub async fn dummy_sync_manager() -> SyncManager { merkle_pool(), )); SyncManager::new( - dummy_peer_handler(store).await, + dummy_peer_handler().await, &SyncMode::Full, CancellationToken::new(), blockchain, @@ -380,7 +380,7 @@ pub async fn dummy_sync_manager() -> SyncManager { /// Creates a dummy PeerHandler for tests where interacting with peers is not needed /// This should only be used in tests as it won't be able to interact with the node's connected peers -pub async fn dummy_peer_handler(_store: Store) -> PeerHandler { +pub async fn dummy_peer_handler() -> PeerHandler { let peer_table = PeerTableServer::spawn(TARGET_PEERS); // No discovery server behind this handle: the dummy handler never dials. PeerHandler::new( diff --git a/test/tests/p2p/discovery/discv5_server_tests.rs b/test/tests/p2p/discovery/discv5_server_tests.rs index 85e9c8249e0..6e60c916812 100644 --- a/test/tests/p2p/discovery/discv5_server_tests.rs +++ b/test/tests/p2p/discovery/discv5_server_tests.rs @@ -15,23 +15,19 @@ use std::{ }; use tokio::net::UdpSocket; -async fn test_server(contacts: Option) -> DiscoveryServer { +async fn test_server() -> DiscoveryServer { let local_node = Node::from_enode_url( "enode://d860a01f9722d78051619d1e2351aba3f43f943f6f00718d1b9baa4101932a1f5011f16bb2b1bb35db20d6fe28fa0bf09636d26a87d31de9ec6203eeedb1f666@18.138.108.67:30303", ).expect("Bad enode url"); let signer = SecretKey::new(&mut rand::rngs::OsRng); let local_node_record = NodeRecord::from_node(&local_node, 1, &signer).unwrap(); - let mut server = DiscoveryServer::new_for_discv5_test( + DiscoveryServer::new_for_discv5_test( local_node, local_node_record, signer, Arc::new(UdpSocket::bind("127.0.0.1:0").await.unwrap()), Box::new(AcceptAllFilter), - ); - if let Some(contacts) = contacts { - *server.contacts_mut() = contacts; - } - server + ) } /// Helper to get a mutable reference to the discv5 state. @@ -42,7 +38,7 @@ fn discv5(server: &mut DiscoveryServer) -> &mut ethrex_p2p::discv5::server::Disc #[tokio::test] async fn test_next_nonce_counter() { let mut rng = StdRng::seed_from_u64(7); - let mut server = test_server(None).await; + let mut server = test_server().await; let n1 = discv5(&mut server).next_nonce(&mut rng); let n2 = discv5(&mut server).next_nonce(&mut rng); @@ -54,7 +50,7 @@ async fn test_next_nonce_counter() { #[tokio::test] async fn test_whoareyou_rate_limiting() { - let mut server = test_server(None).await; + let mut server = test_server().await; let nonce = [0u8; 12]; // Use a public IP so rate limiting is actually exercised (private IPs are exempt). @@ -105,7 +101,7 @@ async fn test_whoareyou_rate_limiting() { #[tokio::test] async fn test_global_whoareyou_rate_limiting() { - let mut server = test_server(None).await; + let mut server = test_server().await; let nonce = [0u8; 12]; discv5(&mut server).whoareyou_global_window_start = Instant::now(); @@ -135,7 +131,7 @@ async fn test_global_whoareyou_rate_limiting() { #[tokio::test] async fn test_whoareyou_rate_limit_lru_cache_works() { - let mut server = test_server(None).await; + let mut server = test_server().await; let nonce = [0u8; 12]; // Bypass the global rate limit so we can insert many entries @@ -251,7 +247,7 @@ async fn test_enr_update_request_on_pong() { #[tokio::test] async fn test_ip_voting_updates_ip_on_threshold() { - let mut server = test_server(None).await; + let mut server = test_server().await; let original_ip = server.local_node.ip; let new_ip: IpAddr = "203.0.113.50".parse().unwrap(); @@ -273,7 +269,7 @@ async fn test_ip_voting_updates_ip_on_threshold() { #[tokio::test] async fn test_ip_voting_same_peer_votes_once() { - let mut server = test_server(None).await; + let mut server = test_server().await; let new_ip: IpAddr = "203.0.113.50".parse().unwrap(); let same_voter = H256::from_low_u64_be(1); @@ -290,7 +286,7 @@ async fn test_ip_voting_same_peer_votes_once() { #[tokio::test] async fn test_ip_voting_no_update_if_same_ip() { - let mut server = test_server(None).await; + let mut server = test_server().await; let original_ip = server.local_node.ip; let voter1 = H256::from_low_u64_be(1); @@ -308,7 +304,7 @@ async fn test_ip_voting_no_update_if_same_ip() { #[tokio::test] async fn test_handle_pong_same_ip_does_not_bump_enr_seq() { - let mut server = test_server(None).await; + let mut server = test_server().await; let original_ip = server.local_node.ip; let original_seq = server.local_node_record.seq; @@ -338,7 +334,7 @@ async fn test_handle_pong_same_ip_does_not_bump_enr_seq() { #[tokio::test] async fn test_ip_voting_split_votes_no_update() { - let mut server = test_server(None).await; + let mut server = test_server().await; let original_ip = server.local_node.ip; let ip1: IpAddr = "203.0.113.50".parse().unwrap(); @@ -361,7 +357,7 @@ async fn test_ip_voting_split_votes_no_update() { #[tokio::test] async fn test_ip_vote_cleanup() { - let mut server = test_server(None).await; + let mut server = test_server().await; let ip: IpAddr = "203.0.113.50".parse().unwrap(); let voter1 = H256::from_low_u64_be(1); @@ -380,7 +376,7 @@ async fn test_ip_vote_cleanup() { #[tokio::test] async fn test_ip_voting_ignores_private_ips() { - let mut server = test_server(None).await; + let mut server = test_server().await; let voter1 = H256::from_low_u64_be(1); let voter2 = H256::from_low_u64_be(2); From b6419a65e5d88a416bee6711d4b2aaf30cf74e62 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Gr=C3=BCner?= <47506558+MegaRedHand@users.noreply.github.com> Date: Tue, 25 Aug 2026 18:20:07 -0300 Subject: [PATCH 03/11] fix(l1): bound the discv5 session store on age, not on contact lifetime Review of the previous commit found the session cleanup it introduced barely fires. `prune` reaps only contacts marked `disposable`, and `set_unwanted` has exactly one caller: the RLPx capability rejection. So for the precise case that commit was written about, a peer we complete a discv5 handshake with and then turn down over RLPx, the contact is marked unwanted, never pruned, and its session is retained for the life of the process. Claiming `prune` bounded the store was wrong. Nor is the contact the right thing to hang the lifetime on. A contact can leave the table without ever being disposable, evicted from a replacement queue by a newer arrival, and a session can be stored for a node whose ENR never parsed into a contact at all, which is the documented reason the store is standalone. Both leave an entry the contact-driven path can no longer reach. That is reachable from outside. The WHOAREYOU rate limiter is keyed on `(ip, src_id)`, so a fresh `src_id` per packet only ever meets the global 100/s cap; each completed handshake orphans an entry. Sessions now carry when they were established and expire from the same prune tick on `SESSION_TTL`, which `Discv5State::session_ips` already used for the other half of the same session. Sharing the constant fixes an asymmetry too: the `session_ips` entry expired after an hour while the keys it guards lived forever, so the IP-rebinding check silently stopped applying to a session that still decrypted. Also from the same review: - `next_dial_candidate` did its bucket lookup before the two set checks rather than after, so the pass that clears `already_tried_peers` walked the whole pool doing O(k) scans, inside the loop that also drains UDP. - `compress_pubkey` moved from `rlpx::utils` to `crate::utils`. It is plain secp256k1 point handling that discv5's handshake needs, and importing it was the last thing tying discovery to the wire protocol. - `DiscoveryServerError::Store` was dead, and put a storage type in discovery's public error enum. - The teardown comment claimed `stopped()` runs even when a handler panics. True of message handlers, false of `started()`, which is where `mark_connected` is sent. - `PeerFilter`'s docs still named the peer table's message loop. - `target_peers_completion` divided by zero on `--p2p.target-peers 0`. The contact table guarded this; the peer table did not. Tests cover the TTL sweep, prune reaching a replacement-list contact, and the pool fallback in `next_dial_candidate`, each verified to fail without its fix. Dropped `a_handle_is_published_once`: it asserted `OnceLock`'s own semantics and paid a real UDP bind and a process-global SIGINT handler in the shared test binary for it. --- .../networking/p2p/discovery/contact_table.rs | 153 ++++++++++++++++-- .../p2p/discovery/discv5_handlers.rs | 3 +- crates/networking/p2p/discovery/server.rs | 24 --- crates/networking/p2p/discv5/server.rs | 12 +- crates/networking/p2p/peer_filter.rs | 8 +- crates/networking/p2p/peer_table.rs | 3 + .../networking/p2p/rlpx/connection/server.rs | 13 +- crates/networking/p2p/rlpx/utils.rs | 18 +-- crates/networking/p2p/sync/snap_sync.rs | 6 +- crates/networking/p2p/utils.rs | 20 +++ 10 files changed, 193 insertions(+), 67 deletions(-) diff --git a/crates/networking/p2p/discovery/contact_table.rs b/crates/networking/p2p/discovery/contact_table.rs index fc7d3c7280b..fd24e693d34 100644 --- a/crates/networking/p2p/discovery/contact_table.rs +++ b/crates/networking/p2p/discovery/contact_table.rs @@ -36,6 +36,7 @@ use std::{ time::{Duration, Instant}, }; +use crate::discv5::server::SESSION_TTL; /// Session information for discv5 protocol. /// Contains symmetric keys derived from ECDH for message encryption/decryption. pub use crate::discv5::session::Session; @@ -310,9 +311,14 @@ pub struct ContactTable { /// allows (k-buckets: 256 x 16 = 4,096 max; this pool: up to 10,000). /// K-buckets are still used for all Kademlia protocol operations. connection_pool: IndexMap, - /// Standalone session store, independent of contacts. - /// Allows sessions to be stored even before the contact's ENR is known/parseable. - sessions: FxHashMap, + /// Standalone session store, independent of contacts. Allows a session to be stored + /// before the contact's ENR is known or parseable, which is why it cannot simply live + /// on the contact. + /// + /// Each entry carries when it was established, because a remote peer decides how many + /// of these we hold: every handshake inserts one, and nothing about a handshake + /// obliges the peer to ever come back. [`Self::prune`] evicts on [`SESSION_TTL`]. + sessions: FxHashMap, /// What this consumer requires of a discovered peer. Judged as each ENR /// arrives, over either discovery protocol; the answer is cached on the /// contact as [`Contact::passes_filter`]. @@ -387,6 +393,15 @@ impl ContactTable { self.connected.len() as f64 / self.target_peers as f64 } + /// Backdates every stored session by `by`, so a test can drive the TTL sweep in + /// [`Self::prune`] without waiting out a real hour. + #[cfg(test)] + pub(crate) fn age_sessions_for_test(&mut self, by: Duration) { + for (_, established_at) in self.sessions.values_mut() { + *established_at -= by; + } + } + // --- Sessions --- /// The discv5 session for a node, if one was ever negotiated. @@ -396,11 +411,18 @@ impl ContactTable { /// disconnect cleanup below, so a session was never actually dropped for a /// node that still had a contact. pub fn session(&self, node_id: &H256) -> Option { - self.sessions.get(node_id).cloned() + self.sessions + .get(node_id) + .map(|(session, _)| session.clone()) } + /// Store a session, stamped with the moment it was established. + /// + /// Re-handshaking restamps it, which is the only way an entry's life is extended: + /// merely using a session does not, so keys expire on the same schedule as the + /// `session_ips` entry guarding them and a still-wanted peer simply re-handshakes. pub fn set_session(&mut self, node_id: H256, session: Session) { - self.sessions.insert(node_id, session); + self.sessions.insert(node_id, (session, Instant::now())); } // --- Contact flags --- @@ -565,9 +587,16 @@ impl ContactTable { /// Pruned contacts remain in the connection pool so they can be retried /// later: the consumer will reject them on connecting if they are truly bad. /// - /// Dropping a contact drops its discv5 session too. That is what bounds the - /// session store, which would otherwise grow by one entry per handshake for - /// the life of the process. + /// Dropping a contact drops its discv5 session too, and any session older than + /// [`SESSION_TTL`] goes with it. + /// + /// The age sweep is what actually bounds the store. Pruning by contact is not + /// enough on its own: a contact can leave the table without ever being marked + /// disposable (evicted from a replacement queue, or simply never pruned because + /// it was only ever marked `unwanted`), and a session can be stored for a node + /// whose ENR never parsed into a contact at all. Both leave an entry that the + /// contact-driven path can no longer reach, and a peer can create them as fast + /// as the WHOAREYOU rate limit allows. pub fn prune(&mut self) { let mut pruned: Vec = Vec::new(); for bucket in &mut self.buckets { @@ -597,6 +626,11 @@ impl ContactTable { for node_id in pruned { self.sessions.remove(&node_id); } + + let now = Instant::now(); + self.sessions.retain(|_, (_, established_at)| { + now.saturating_duration_since(*established_at) < SESSION_TTL + }); } /// Pick the next node to hand to the RLPx dialer, or `None` when the pool @@ -622,13 +656,19 @@ impl ContactTable { continue; }; let node_id = *node_id; - let contact = self.get_contact_or_replacement(&node_id); - if self.connected.contains(&node_id) - || self.already_tried_peers.contains(&node_id) - || contact - .map(|c| !c.knows_us || c.unwanted || c.passes_filter == Some(false)) - .unwrap_or(false) + // Two set lookups before the bucket walk: on the pass that clears + // `already_tried_peers` every entry is rejected here, and doing the + // O(k) bucket scan first would turn that into a full walk of the pool + // inside the loop that also has to drain UDP. + if self.connected.contains(&node_id) || self.already_tried_peers.contains(&node_id) { + continue; + } + + let contact = self.get_contact_or_replacement(&node_id); + if contact + .map(|c| !c.knows_us || c.unwanted || c.passes_filter == Some(false)) + .unwrap_or(false) { continue; } @@ -1172,6 +1212,91 @@ mod tests { ); } + #[tokio::test] + async fn a_session_outlives_neither_its_ttl_nor_a_node_it_was_never_matched_to() { + // The store's real bound. A session can be reached by neither the + // contact-driven path nor anything else: this one belongs to a node id + // that never became a contact at all, which is the documented reason the + // store is standalone. Only age can reclaim it. + let mut table = table_with(FixedAnswer(true)); + let orphan = H256::repeat_byte(0x5e); + + table.set_session(orphan, session()); + table.prune(); + assert!( + table.session(&orphan).is_some(), + "a fresh session must survive a prune" + ); + + table.age_sessions_for_test(SESSION_TTL); + table.prune(); + + assert!( + table.session(&orphan).is_none(), + "an aged session is reaped" + ); + } + + #[tokio::test] + async fn pruning_reaches_a_contact_in_the_replacement_list() { + // The replacement half of `prune` had no coverage: dropping the + // `pruned.push` inside `retain` left every test green. + let mut table = table_with(FixedAnswer(true)); + + // Fill one bucket's main list so the next arrival becomes a replacement. + let (target_id, _) = dummy_contact(1); + let bucket = bucket_index(&table.local_node_id, &target_id).expect("not the local node"); + let mut overflow = Vec::new(); + for seed in 0..u8::MAX { + let (id, contact) = dummy_contact(seed); + if bucket_index(&table.local_node_id, &id) == Some(bucket) { + overflow.push(id); + table.insert_contact(id, contact); + if overflow.len() > MAX_NODES_PER_BUCKET { + break; + } + } + } + let replacement = *overflow.last().expect("a contact overflowed the bucket"); + assert!( + table.buckets[bucket] + .replacements + .iter() + .any(|(id, _)| *id == replacement), + "the last insert landed in the replacement list" + ); + + table.set_session(replacement, session()); + table.set_disposable(&replacement); + table.prune(); + + assert!(table.get_contact(&replacement).is_none()); + assert!( + table.session(&replacement).is_none(), + "a replacement-list contact takes its session with it" + ); + } + + #[tokio::test] + async fn a_pooled_node_with_no_contact_is_still_dialable() { + // The fallback arm of `next_dial_candidate`. Contacts get pruned out of the + // buckets while their pool entry stays, and those nodes must still be + // offered, or a prune would quietly retire them. + let mut table = table_with(FixedAnswer(true)); + let (node_id, record) = record_for(16, 1); + + table.new_contact_records(vec![record]).await; + table.set_disposable(&node_id); + table.prune(); + assert!(table.get_contact(&node_id).is_none(), "contact is gone"); + + assert_eq!( + table.next_dial_candidate().map(|n| n.node_id()), + Some(node_id), + "the pool entry is the fallback, and it is still dialable" + ); + } + #[tokio::test] async fn a_dial_candidate_uses_the_endpoint_from_the_newest_record() { // The connection pool is written on first sight and never refreshed, so diff --git a/crates/networking/p2p/discovery/discv5_handlers.rs b/crates/networking/p2p/discovery/discv5_handlers.rs index bd31f8c79c8..f6471fcecec 100644 --- a/crates/networking/p2p/discovery/discv5_handlers.rs +++ b/crates/networking/p2p/discovery/discv5_handlers.rs @@ -13,9 +13,8 @@ use crate::{ }, }, metrics::METRICS, - rlpx::utils::compress_pubkey, types::{Node, NodeRecord}, - utils::{distance, node_id}, + utils::{compress_pubkey, distance, node_id}, }; use bytes::{Bytes, BytesMut}; use ethrex_common::{H256, H512}; diff --git a/crates/networking/p2p/discovery/server.rs b/crates/networking/p2p/discovery/server.rs index e80319dbb86..4a5c2c3bef4 100644 --- a/crates/networking/p2p/discovery/server.rs +++ b/crates/networking/p2p/discovery/server.rs @@ -64,8 +64,6 @@ pub enum DiscoveryServerError { InvalidContact, #[error(transparent)] Actor(#[from] ActorError), - #[error(transparent)] - Store(#[from] ethrex_storage::error::StoreError), #[error("Internal error {0}")] InternalError(String), #[error("Cryptography Error {0}")] @@ -628,26 +626,4 @@ mod tests { assert!(handle.next_dial_candidate().await.is_none()); } - - #[tokio::test] - async fn a_handle_is_published_once() { - let handle = DiscoveryHandle::new(); - let server = DiscoveryServer::new_for_discv5_test( - Node::from_enode_url( - "enode://d860a01f9722d78051619d1e2351aba3f43f943f6f00718d1b9baa4101932a1f5011f16bb2b1bb35db20d6fe28fa0bf09636d26a87d31de9ec6203eeedb1f666@18.138.108.67:30303", - ) - .expect("bad enode url"), - NodeRecord::default(), - SecretKey::new(&mut rand::rngs::OsRng), - Arc::new(UdpSocket::bind("127.0.0.1:0").await.expect("bind")), - Box::new(crate::peer_filter::AcceptAllFilter), - ) - .start(); - - assert!(handle.set(server.clone())); - assert!( - !handle.set(server), - "a second publish must be refused, not silently swap the server" - ); - } } diff --git a/crates/networking/p2p/discv5/server.rs b/crates/networking/p2p/discv5/server.rs index cf79ffcaf70..6ec99f801e3 100644 --- a/crates/networking/p2p/discv5/server.rs +++ b/crates/networking/p2p/discv5/server.rs @@ -23,9 +23,15 @@ const IP_VOTE_WINDOW: Duration = Duration::from_secs(300); const IP_VOTE_THRESHOLD: usize = 3; /// Timeout for pending messages awaiting WhoAreYou response. const MESSAGE_CACHE_TIMEOUT: Duration = Duration::from_secs(2); -/// Max age of a `session_ips` entry before it is evicted. Bounds the map: it is inserted -/// per discv5 handshake and (absent this) was never removed for nodes we don't keep as peers. -const SESSION_TTL: Duration = Duration::from_secs(3600); +/// Max age of a discv5 session before it is evicted. Bounds both halves of a session: +/// the symmetric keys in the contact table and the `session_ips` entry that guards them. +/// Both are inserted per handshake, by a remote peer's schedule, so without this neither +/// is ever removed for a node we do not keep as a peer. +/// +/// Shared rather than duplicated: when the keys outlive their `session_ips` entry, the +/// IP-rebinding check in `discv5_handle_ordinary` silently stops applying to a session +/// that still decrypts. +pub const SESSION_TTL: Duration = Duration::from_secs(3600); /// Source IP a discv5 session was established from, paired with when it was recorded so stale /// entries can be evicted (see `SESSION_TTL`). diff --git a/crates/networking/p2p/peer_filter.rs b/crates/networking/p2p/peer_filter.rs index a76b5990181..0c639c5af85 100644 --- a/crates/networking/p2p/peer_filter.rs +++ b/crates/networking/p2p/peer_filter.rs @@ -18,9 +18,11 @@ use tracing::debug; /// discv5 alike. A contact discovered without an ENR is never filtered and stays /// dialable. /// -/// Implementations run inside the peer table's message loop, so a slow `accepts` -/// stalls every other peer-table operation: keep it to work proportional to the -/// record, and cache anything expensive at construction. +/// Implementations run inside the discovery server's message loop, so a slow +/// `accepts` stalls inbound UDP, revalidation, lookups, and the dial-candidate +/// request the initiator is waiting on. A remote peer decides how often this runs: +/// an unsolicited discv5 NODES message is filtered per ENR it carries. Keep it to +/// work proportional to the record, and cache anything expensive at construction. /// /// `accepts` is synchronous, which states that requirement in the type rather /// than in this comment. It also keeps the trait object-safe without boxing a diff --git a/crates/networking/p2p/peer_table.rs b/crates/networking/p2p/peer_table.rs index 55d2fd8c28c..69b418da0a0 100644 --- a/crates/networking/p2p/peer_table.rs +++ b/crates/networking/p2p/peer_table.rs @@ -353,6 +353,9 @@ impl PeerTableServer { _msg: peer_table_server_protocol::TargetPeersCompletion, _ctx: &Context, ) -> f64 { + if self.target_peers == 0 { + return 1.0; + } self.peers.len() as f64 / self.target_peers as f64 } diff --git a/crates/networking/p2p/rlpx/connection/server.rs b/crates/networking/p2p/rlpx/connection/server.rs index 2f3e5f1fe3c..b1595997ffb 100644 --- a/crates/networking/p2p/rlpx/connection/server.rs +++ b/crates/networking/p2p/rlpx/connection/server.rs @@ -462,9 +462,16 @@ impl PeerConnectionServer { { debug!("Failed to remove peer from table: {e}"); } - // Pairs with the `mark_connected` in `initialize_connection`: this - // branch is the only teardown an established connection takes, and - // it runs even when a handler panicked. + // Pairs with the `mark_connected` in `initialize_connection`. This is + // the only teardown an established connection takes, and it still runs + // when a message handler panics, because the actor loop catches the + // unwind before falling through to `stopped()`. + // + // It does not cover a panic inside `started()` itself, which cancels the + // actor and returns without running this hook. `mark_connected` is sent + // from there, so that window can strand an id in discovery's connected + // set. `remove_peer` above is lost to the same window, leaving the peer + // table's own map equally stale, so the two stores at least agree. established_state .discovery .mark_disconnected(established_state.node.node_id()); diff --git a/crates/networking/p2p/rlpx/utils.rs b/crates/networking/p2p/rlpx/utils.rs index 70c1ba3e78a..574d010993d 100644 --- a/crates/networking/p2p/rlpx/utils.rs +++ b/crates/networking/p2p/rlpx/utils.rs @@ -1,4 +1,4 @@ -use ethrex_common::H512; +pub use crate::utils::{compress_pubkey, decompress_pubkey}; use ethrex_rlp::error::{RLPDecodeError, RLPEncodeError}; use secp256k1::ecdh::shared_secret_point; use secp256k1::{PublicKey, SecretKey}; @@ -46,22 +46,6 @@ pub fn kdf(secret: &[u8], output: &mut [u8]) -> Result<(), CryptographyError> { .map_err(|error| CryptographyError::CouldNotGetKeyFromSecret(error.to_string())) } -/// Decompresses the received public key -pub fn decompress_pubkey(pk: &PublicKey) -> H512 { - let bytes = pk.serialize_uncompressed(); - debug_assert_eq!(bytes[0], 4); - H512::from_slice(&bytes[1..]) -} - -/// Compresses the received public key -/// The received value is the uncompressed public key of a node, with the first byte omitted (0x04). -pub fn compress_pubkey(pk: H512) -> Option { - let mut full_pk = [0u8; 65]; - full_pk[0] = 0x04; - full_pk[1..].copy_from_slice(&pk.0); - PublicKey::from_slice(&full_pk).ok() -} - pub fn snappy_compress(encoded_data: Vec) -> Result, RLPEncodeError> { let mut snappy_encoder = SnappyEncoder::new(); let mut msg_data = vec![0; max_compress_len(encoded_data.len()) + 1]; diff --git a/crates/networking/p2p/sync/snap_sync.rs b/crates/networking/p2p/sync/snap_sync.rs index 45ba74291f1..b4f027756c6 100644 --- a/crates/networking/p2p/sync/snap_sync.rs +++ b/crates/networking/p2p/sync/snap_sync.rs @@ -152,7 +152,11 @@ pub async fn sync_cycle_snap( let mut attempts = 0; loop { - // Prune dead/unresponsive peers periodically to allow replacements to be promoted + // Prune dead/unresponsive contacts periodically to allow replacements to be + // promoted. Note this runs discovery's whole prune, not just the k-bucket walk: + // it also expires discv5 sessions and can finalise an IP-vote round. All of that + // is time-gated and the discovery server already runs the same call every five + // seconds, so this only ever brings the next sweep forward. peers.discovery.prune(); debug!("Requesting Block Headers from {current_head}"); diff --git a/crates/networking/p2p/utils.rs b/crates/networking/p2p/utils.rs index 040277d9272..22cf90f3f4f 100644 --- a/crates/networking/p2p/utils.rs +++ b/crates/networking/p2p/utils.rs @@ -227,3 +227,23 @@ pub fn distance(node_id_1: &H256, node_id_2: &H256) -> usize { let distance = U256::from_big_endian(xor.as_bytes()); distance.bits() } + +/// Decompresses the received public key +pub fn decompress_pubkey(pk: &PublicKey) -> H512 { + let bytes = pk.serialize_uncompressed(); + debug_assert_eq!(bytes[0], 4); + H512::from_slice(&bytes[1..]) +} + +/// Compresses the received public key +/// The received value is the uncompressed public key of a node, with the first byte omitted (0x04). +/// +/// Lives here rather than under `rlpx` because it is plain secp256k1 point handling that +/// discv5's handshake needs too, and importing it from `rlpx` was the last thing tying +/// discovery to the wire protocol. +pub fn compress_pubkey(pk: H512) -> Option { + let mut full_pk = [0u8; 65]; + full_pk[0] = 0x04; + full_pk[1..].copy_from_slice(&pk.0); + PublicKey::from_slice(&full_pk).ok() +} From f4a6a531e5b8564015e1a475f2e085f71fc32661 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Gr=C3=BCner?= <47506558+MegaRedHand@users.noreply.github.com> Date: Tue, 25 Aug 2026 19:05:11 -0300 Subject: [PATCH 04/11] refactor(l1): report peer status to discovery through one message The consumer had four separate casts for four things it might learn about a peer: `mark_connected`, `mark_disconnected`, `set_unwanted`, `set_disposable`. They now travel as one `update_status(node_id, PeerStatus)`, taking `DiscoveryHandle` from eight methods to five. `PeerStatus` is deliberately a report of one event rather than a state machine, and its docs say so, because the four are not mutually exclusive over a peer's life. `Connected` and `Disconnected` toggle a membership; `Unwanted` and `Disposable` are verdicts that accumulate on the contact and are never cleared. Two rules follow, and getting either wrong is silent: - A verdict never disconnects. `Disposable` is reported by the sync layer for a peer whose connection is still up, when it serves a malformed response, so folding the verdict into the connected set would hand a peer we are actively talking to back to the dialer. - A disconnection never clears a verdict, or hanging up on a peer we had rejected would quietly rehabilitate it. Both are now tested, and each of the three plausible ways to mis-wire the match arm fails one of those tests. Discovery's own `set_disposable` calls, on a UDP send failure, stay as direct calls on the table: `update_status` is the door the consumer comes through, not an internal one. --- .../networking/p2p/discovery/contact_table.rs | 94 ++++++++++++++++++- crates/networking/p2p/discovery/mod.rs | 4 +- crates/networking/p2p/discovery/server.rs | 85 ++++------------- crates/networking/p2p/peer_handler.rs | 11 ++- .../networking/p2p/rlpx/connection/server.rs | 19 ++-- 5 files changed, 131 insertions(+), 82 deletions(-) diff --git a/crates/networking/p2p/discovery/contact_table.rs b/crates/networking/p2p/discovery/contact_table.rs index fd24e693d34..b37a4a80353 100644 --- a/crates/networking/p2p/discovery/contact_table.rs +++ b/crates/networking/p2p/discovery/contact_table.rs @@ -282,6 +282,36 @@ impl Contact { } } +/// What the consumer has just learned about a peer. +/// +/// A report of one event, not a state machine. The variants are not mutually +/// exclusive over a peer's life and each touches only what it names: +/// [`Self::Connected`] and [`Self::Disconnected`] toggle a membership, while +/// [`Self::Unwanted`] and [`Self::Disposable`] are verdicts that accumulate on +/// the contact and are never cleared. +/// +/// In particular the two verdicts deliberately leave the connected set alone. A +/// live peer that serves a bad response is reported `Disposable` by the sync +/// layer without its connection going anywhere, and treating that as a +/// disconnection would put a peer we are still talking to back into the dial +/// pool. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum PeerStatus { + /// A connection to this peer is up: stop offering it as a dial candidate, + /// and count it towards how hard discovery looks for more. + Connected, + /// The connection is gone. Says nothing about whether the peer is worth + /// having; verdicts already recorded stay recorded. + Disconnected, + /// Known-bad to the consumer: on another network, or advertising no + /// capability it wants. Never offered as a dial candidate again. + Unwanted, + /// Not worth keeping in the routing table. Dropped by the next + /// [`ContactTable::prune`], though its pool entry survives so it can still + /// be dialed. + Disposable, +} + /// Result of contact validation. #[derive(Debug, Clone)] pub enum ContactValidation { @@ -365,9 +395,23 @@ impl ContactTable { // --- Consumer lifecycle --- + /// Record what the consumer has just learned about a peer. + /// + /// The single door through which the consumer reports anything about a + /// peer; see [`PeerStatus`] for what each variant does and, more + /// importantly, what it deliberately does not touch. + pub fn update_status(&mut self, node_id: H256, status: PeerStatus) { + match status { + PeerStatus::Connected => self.mark_connected(node_id), + PeerStatus::Disconnected => self.mark_disconnected(&node_id), + PeerStatus::Unwanted => self.set_unwanted(&node_id), + PeerStatus::Disposable => self.set_disposable(&node_id), + } + } + /// Record that the consumer is now connected to `node_id`, so it stops /// being offered as a dial candidate and counts towards lookup pacing. - pub fn mark_connected(&mut self, node_id: H256) { + pub(crate) fn mark_connected(&mut self, node_id: H256) { self.connected.insert(node_id); } @@ -379,7 +423,7 @@ impl ContactTable { /// nothing about the discv5 keys, and tearing them down there would force a /// WHOAREYOU round trip to talk to a node we are still perfectly able to /// reach. Sessions go when the contact does, in [`Self::prune`]. - pub fn mark_disconnected(&mut self, node_id: &H256) { + pub(crate) fn mark_disconnected(&mut self, node_id: &H256) { self.connected.remove(node_id); } @@ -429,7 +473,7 @@ impl ContactTable { /// Mark a contact as one we should stop keeping: it failed to answer a ping, /// or the consumer found it useless. Pruned on the next [`Self::prune`]. - pub fn set_disposable(&mut self, node_id: &H256) { + pub(crate) fn set_disposable(&mut self, node_id: &H256) { if let Some(contact) = self.get_contact_mut(node_id) { contact.disposable = true; } @@ -437,7 +481,7 @@ impl ContactTable { /// Mark a contact as known-bad: on another network, no matching /// capabilities, or otherwise rejected by the consumer. Never dialed again. - pub fn set_unwanted(&mut self, node_id: &H256) { + pub(crate) fn set_unwanted(&mut self, node_id: &H256) { if let Some(contact) = self.get_contact_mut(node_id) { contact.unwanted = true; } @@ -1366,6 +1410,48 @@ mod tests { ); } + #[tokio::test] + async fn a_verdict_does_not_disconnect_a_live_peer() { + // The reason `PeerStatus` is a report and not a state. The sync layer + // marks a peer `Disposable` when it serves a bad response, and that peer + // is still connected: folding the verdict into the connected set would + // hand a peer we are actively talking to back to the dialer. + let mut table = table_with(FixedAnswer(true)); + let (node_id, record) = record_for(17, 1); + + table.new_contact_records(vec![record]).await; + table.update_status(node_id, PeerStatus::Connected); + table.update_status(node_id, PeerStatus::Disposable); + + assert_eq!(table.peer_completion(), 0.1, "still counted as connected"); + assert!( + table.next_dial_candidate().is_none(), + "a connected peer must not be offered for dialing, verdict or not" + ); + + table.update_status(node_id, PeerStatus::Unwanted); + assert_eq!(table.peer_completion(), 0.1, "nor does the other verdict"); + } + + #[tokio::test] + async fn disconnecting_leaves_earlier_verdicts_standing() { + // The other half of the same rule: verdicts accumulate, so a later + // disconnection must not quietly rehabilitate a peer we rejected. + let mut table = table_with(FixedAnswer(true)); + let (node_id, record) = record_for(18, 1); + + table.new_contact_records(vec![record]).await; + table.update_status(node_id, PeerStatus::Connected); + table.update_status(node_id, PeerStatus::Unwanted); + table.update_status(node_id, PeerStatus::Disconnected); + + assert_eq!(table.peer_completion(), 0.0, "no longer connected"); + assert!( + table.next_dial_candidate().is_none(), + "but still unwanted, so still never dialed" + ); + } + #[test] fn peer_completion_tracks_the_connected_count() { let mut table = table_with(FixedAnswer(true)); diff --git a/crates/networking/p2p/discovery/mod.rs b/crates/networking/p2p/discovery/mod.rs index 306f90dd1bd..8f393e5f3e9 100644 --- a/crates/networking/p2p/discovery/mod.rs +++ b/crates/networking/p2p/discovery/mod.rs @@ -17,7 +17,9 @@ mod discv5_handlers; pub mod lookup; pub mod server; -pub use contact_table::{Contact, ContactTable, ContactValidation, DiscoveryProtocol, Session}; +pub use contact_table::{ + Contact, ContactTable, ContactValidation, DiscoveryProtocol, PeerStatus, Session, +}; pub use server::{ DiscoveryHandle, DiscoveryServer, DiscoveryServerError, DiscoveryServerProtocol, is_discv4_packet, diff --git a/crates/networking/p2p/discovery/server.rs b/crates/networking/p2p/discovery/server.rs index 4a5c2c3bef4..e3eb7b6774e 100644 --- a/crates/networking/p2p/discovery/server.rs +++ b/crates/networking/p2p/discovery/server.rs @@ -32,7 +32,7 @@ use tracing::{debug, error, info, trace}; use super::{ DiscoveryConfig, codec::DiscriminatingCodec, contact_table::ContactTable, - contact_table::DiscoveryProtocol, lookup_interval_function, + contact_table::DiscoveryProtocol, contact_table::PeerStatus, lookup_interval_function, }; use std::sync::OnceLock; @@ -75,16 +75,9 @@ pub enum DiscoveryServerError { #[protocol] pub trait DiscoveryServerProtocol: Send + Sync { fn raw_packet(&self, data: BytesMut, from: SocketAddr) -> Result<(), ActorError>; - /// The consumer established a connection to this node: stop offering it as - /// a dial candidate, and count it towards how hard we look for more. - fn mark_connected(&self, node_id: H256) -> Result<(), ActorError>; - /// The consumer's connection to this node is gone. - fn mark_disconnected(&self, node_id: H256) -> Result<(), ActorError>; - /// This node is known-bad to the consumer (wrong network, no usable - /// capabilities): never offer it again. - fn set_unwanted(&self, node_id: H256) -> Result<(), ActorError>; - /// This node is not worth keeping in the routing table. - fn set_disposable(&self, node_id: H256) -> Result<(), ActorError>; + /// Report what the consumer has just learned about a peer. See + /// [`PeerStatus`] for what each variant does. + fn update_status(&self, node_id: H256, status: PeerStatus) -> Result<(), ActorError>; fn revalidate_v4(&self) -> Result<(), ActorError>; fn revalidate_v5(&self) -> Result<(), ActorError>; fn lookup_v4(&self) -> Result<(), ActorError>; @@ -128,27 +121,12 @@ impl DiscoveryHandle { self.0.get() } - pub fn mark_connected(&self, node_id: H256) { + /// Report what just happened to a peer. See [`PeerStatus`]: the variants + /// are separate reports rather than exclusive states, so a peer can be + /// reported `Disposable` while its connection is still up. + pub fn update_status(&self, node_id: H256, status: PeerStatus) { if let Some(server) = self.server() { - let _ = server.mark_connected(node_id); - } - } - - pub fn mark_disconnected(&self, node_id: H256) { - if let Some(server) = self.server() { - let _ = server.mark_disconnected(node_id); - } - } - - pub fn set_unwanted(&self, node_id: H256) { - if let Some(server) = self.server() { - let _ = server.set_unwanted(node_id); - } - } - - pub fn set_disposable(&self, node_id: H256) { - if let Some(server) = self.server() { - let _ = server.set_disposable(node_id); + let _ = server.update_status(node_id, status); } } @@ -414,39 +392,12 @@ impl DiscoveryServer { } #[send_handler] - async fn handle_mark_connected( + async fn handle_update_status( &mut self, - msg: discovery_server_protocol::MarkConnected, + msg: discovery_server_protocol::UpdateStatus, _ctx: &Context, ) { - self.contacts.mark_connected(msg.node_id); - } - - #[send_handler] - async fn handle_mark_disconnected( - &mut self, - msg: discovery_server_protocol::MarkDisconnected, - _ctx: &Context, - ) { - self.contacts.mark_disconnected(&msg.node_id); - } - - #[send_handler] - async fn handle_set_unwanted( - &mut self, - msg: discovery_server_protocol::SetUnwanted, - _ctx: &Context, - ) { - self.contacts.set_unwanted(&msg.node_id); - } - - #[send_handler] - async fn handle_set_disposable( - &mut self, - msg: discovery_server_protocol::SetDisposable, - _ctx: &Context, - ) { - self.contacts.set_disposable(&msg.node_id); + self.contacts.update_status(msg.node_id, msg.status); } #[request_handler] @@ -618,10 +569,14 @@ mod tests { // answer `None` rather than wait for a server that may never arrive. let handle = DiscoveryHandle::new(); - handle.mark_connected(H256::repeat_byte(1)); - handle.mark_disconnected(H256::repeat_byte(1)); - handle.set_unwanted(H256::repeat_byte(1)); - handle.set_disposable(H256::repeat_byte(1)); + for status in [ + PeerStatus::Connected, + PeerStatus::Disconnected, + PeerStatus::Unwanted, + PeerStatus::Disposable, + ] { + handle.update_status(H256::repeat_byte(1), status); + } handle.prune(); assert!(handle.next_dial_candidate().await.is_none()); diff --git a/crates/networking/p2p/peer_handler.rs b/crates/networking/p2p/peer_handler.rs index 83320bc8e4b..2a884cb9e74 100644 --- a/crates/networking/p2p/peer_handler.rs +++ b/crates/networking/p2p/peer_handler.rs @@ -1,4 +1,4 @@ -use crate::discovery::DiscoveryHandle; +use crate::discovery::{DiscoveryHandle, PeerStatus}; use crate::rlpx::initiator::RLPxInitiator; use crate::{ metrics::{CurrentStepValue, METRICS}, @@ -615,7 +615,8 @@ impl PeerHandler { "Peer returned more block bodies than requested, disposing" ); self.peer_table.record_failure(peer_id)?; - self.discovery.set_disposable(peer_id); + self.discovery + .update_status(peer_id, PeerStatus::Disposable); return Ok(None); } if !block_bodies.is_empty() { @@ -772,7 +773,8 @@ impl PeerHandler { _ => { debug!("Didn't receive receipts from peer, penalizing peer {peer_id}"); self.peer_table.record_failure(peer_id)?; - self.discovery.set_disposable(peer_id); + self.discovery + .update_status(peer_id, PeerStatus::Disposable); return Ok(None); } }; @@ -790,7 +792,8 @@ impl PeerHandler { if receipts.len() > block_hashes_len { debug!("Received oversized receipts from peer {peer_id}, penalizing"); self.peer_table.record_failure(peer_id)?; - self.discovery.set_disposable(peer_id); + self.discovery + .update_status(peer_id, PeerStatus::Disposable); return Ok(None); } // Success is recorded by the caller, once the receipts have been diff --git a/crates/networking/p2p/rlpx/connection/server.rs b/crates/networking/p2p/rlpx/connection/server.rs index b1595997ffb..eb66e5b0071 100644 --- a/crates/networking/p2p/rlpx/connection/server.rs +++ b/crates/networking/p2p/rlpx/connection/server.rs @@ -7,7 +7,7 @@ use crate::rlpx::l2::{ }; use crate::{ backend, - discovery::DiscoveryHandle, + discovery::{DiscoveryHandle, PeerStatus}, metrics::METRICS, network::P2PContext, peer_table::{PeerTable, PeerTableServerProtocol as _}, @@ -393,9 +393,10 @@ impl PeerConnectionServer { match &reason { PeerConnectionError::NoMatchingCapabilities | PeerConnectionError::HandshakeError(_) => { - established_state - .discovery - .set_unwanted(established_state.node.node_id()); + established_state.discovery.update_status( + established_state.node.node_id(), + PeerStatus::Unwanted, + ); } _ => {} } @@ -462,19 +463,19 @@ impl PeerConnectionServer { { debug!("Failed to remove peer from table: {e}"); } - // Pairs with the `mark_connected` in `initialize_connection`. This is + // Pairs with the `Connected` report in `initialize_connection`. This is // the only teardown an established connection takes, and it still runs // when a message handler panics, because the actor loop catches the // unwind before falling through to `stopped()`. // // It does not cover a panic inside `started()` itself, which cancels the - // actor and returns without running this hook. `mark_connected` is sent + // actor and returns without running this hook. `Connected` is reported // from there, so that window can strand an id in discovery's connected // set. `remove_peer` above is lost to the same window, leaving the peer // table's own map equally stale, so the two stores at least agree. established_state .discovery - .mark_disconnected(established_state.node.node_id()); + .update_status(established_state.node.node_id(), PeerStatus::Disconnected); // Free the peer's tx-broadcaster index (and clear its bit across known txs) so // the broadcaster's per-peer index map / PeerMask widths stay bounded to live peers. if let Err(e) = established_state @@ -836,7 +837,9 @@ where state.capabilities.clone(), state.is_inbound, )?; - state.discovery.mark_connected(state.node.node_id()); + state + .discovery + .update_status(state.node.node_id(), PeerStatus::Connected); trace!(peer=%state.node, "Peer connection initialized."); From f698c6bdcd184b6a2f1fad54c33326579131cfa4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Gr=C3=BCner?= <47506558+MegaRedHand@users.noreply.github.com> Date: Wed, 26 Aug 2026 11:49:20 -0300 Subject: [PATCH 05/11] fix(l1): make connected-and-unwanted unrepresentable, and stop sync disposing contacts Two of the eight states the three peer flags can take were incoherent, and both involved a peer being connected and unwanted at once. `Unwanted` is reported when an RLPx handshake is rejected for capabilities, and that always happens before the connection is registered, so a single connection actor can never produce the pair. Two can: nothing dedupes connection attempts by node id, so while one connection is live a second attempt to the same peer can die on a transient `HandshakeError` and permanently retire a peer we are happily syncing from. Invisibly, since the verdict is only ever read by the dial filter. `update_status` now drops an `Unwanted` report for a node in the connected set. A live connection is better evidence of wantedness than a failed redundant handshake, and putting the rule in the type that owns both pieces of state makes the contradiction unrepresentable rather than merely unlikely. The underlying duplicate-connection race is untouched and still leaves a stale `peers` entry; that is pre-existing and wants a real dedup. The connected-and-disposable case was worse, because it was reached on purpose. `PeerHandler` marked peers disposable on a malformed response, under a comment saying it wanted to "drop the peer rather than just scoring it down". That is not what the flag does. It leaves the connection up and the peer selectable, deletes the peer's Kademlia contact over an eth-protocol fault, leaves it dialable because the dial filter never reads `disposable`, and is erased within five seconds when prune drops the contact and the flag with it. Those three sites now call `record_critical_failure`, which is the peer table's own mechanism for this, is what the same file already uses ninety lines away for the same class of violation, and actually does what the comment claimed. That leaves `disposable` with no sender outside discovery, so `PeerStatus` drops the variant. It now means only what its field doc always said: a contact did not answer us over UDP, which only discovery can observe. Connected-and-disposable stays reachable and is finally coherent, meaning TCP is up while a UDP send failed. --- .../networking/p2p/discovery/contact_table.rs | 75 +++++++++++++------ crates/networking/p2p/discovery/server.rs | 1 - crates/networking/p2p/peer_handler.rs | 16 ++-- 3 files changed, 58 insertions(+), 34 deletions(-) diff --git a/crates/networking/p2p/discovery/contact_table.rs b/crates/networking/p2p/discovery/contact_table.rs index b37a4a80353..5e213bd992a 100644 --- a/crates/networking/p2p/discovery/contact_table.rs +++ b/crates/networking/p2p/discovery/contact_table.rs @@ -290,11 +290,16 @@ impl Contact { /// [`Self::Unwanted`] and [`Self::Disposable`] are verdicts that accumulate on /// the contact and are never cleared. /// -/// In particular the two verdicts deliberately leave the connected set alone. A -/// live peer that serves a bad response is reported `Disposable` by the sync -/// layer without its connection going anywhere, and treating that as a -/// disconnection would put a peer we are still talking to back into the dial -/// pool. +/// [`Self::Unwanted`] deliberately leaves the connected set alone: it is a +/// verdict about whether to dial a peer in future, not a statement that the +/// current connection is over. +/// +/// There is no variant for `disposable`. That flag means a contact did not +/// answer us over UDP, which only discovery is in a position to observe, so it +/// is set internally rather than reported. The sync layer used to reach for it +/// to mean "stop using this peer", which is not what it does: it deletes the +/// peer's routing-table entry, leaves the connection up, and is erased by the +/// next prune. Scoring the peer down is the mechanism for that. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum PeerStatus { /// A connection to this peer is up: stop offering it as a dial candidate, @@ -305,11 +310,10 @@ pub enum PeerStatus { Disconnected, /// Known-bad to the consumer: on another network, or advertising no /// capability it wants. Never offered as a dial candidate again. + /// + /// Ignored for a peer that is currently connected; see + /// [`ContactTable::update_status`]. Unwanted, - /// Not worth keeping in the routing table. Dropped by the next - /// [`ContactTable::prune`], though its pool entry survives so it can still - /// be dialed. - Disposable, } /// Result of contact validation. @@ -404,8 +408,19 @@ impl ContactTable { match status { PeerStatus::Connected => self.mark_connected(node_id), PeerStatus::Disconnected => self.mark_disconnected(&node_id), + // A peer we are connected to is, demonstrably, wanted. This report + // can only reach us for one while a second connection attempt to the + // same node fails its handshake, and nothing dedupes those: a + // transient error on the redundant attempt would otherwise mark a + // peer we are happily syncing from as never-dial-again, permanently + // and invisibly. The live connection is the better evidence. + PeerStatus::Unwanted if self.connected.contains(&node_id) => { + tracing::debug!( + peer = %node_id, + "Ignoring unwanted verdict for a peer we are connected to" + ); + } PeerStatus::Unwanted => self.set_unwanted(&node_id), - PeerStatus::Disposable => self.set_disposable(&node_id), } } @@ -1411,26 +1426,40 @@ mod tests { } #[tokio::test] - async fn a_verdict_does_not_disconnect_a_live_peer() { - // The reason `PeerStatus` is a report and not a state. The sync layer - // marks a peer `Disposable` when it serves a bad response, and that peer - // is still connected: folding the verdict into the connected set would - // hand a peer we are actively talking to back to the dialer. + async fn a_connected_peer_is_never_marked_unwanted() { + // Connected-and-unwanted is a contradiction, and it was reachable: two + // connection actors can race for one node id, and a transient handshake + // error on the loser would permanently retire a peer the winner is + // syncing from. The live connection outranks the verdict. let mut table = table_with(FixedAnswer(true)); let (node_id, record) = record_for(17, 1); table.new_contact_records(vec![record]).await; table.update_status(node_id, PeerStatus::Connected); - table.update_status(node_id, PeerStatus::Disposable); + table.update_status(node_id, PeerStatus::Unwanted); - assert_eq!(table.peer_completion(), 0.1, "still counted as connected"); - assert!( - table.next_dial_candidate().is_none(), - "a connected peer must not be offered for dialing, verdict or not" + assert_eq!(table.peer_completion(), 0.1, "still connected"); + table.update_status(node_id, PeerStatus::Disconnected); + assert_eq!( + table.next_dial_candidate().map(|n| n.node_id()), + Some(node_id), + "and still dialable once it disconnects: the verdict never landed" ); + } + #[tokio::test] + async fn an_unwanted_verdict_lands_when_the_peer_is_not_connected() { + // The guard must not swallow the verdict it exists to record. + let mut table = table_with(FixedAnswer(true)); + let (node_id, record) = record_for(19, 1); + + table.new_contact_records(vec![record]).await; table.update_status(node_id, PeerStatus::Unwanted); - assert_eq!(table.peer_completion(), 0.1, "nor does the other verdict"); + + assert!( + table.next_dial_candidate().is_none(), + "an unwanted peer that was never connected is never dialed" + ); } #[tokio::test] @@ -1440,9 +1469,11 @@ mod tests { let mut table = table_with(FixedAnswer(true)); let (node_id, record) = record_for(18, 1); + // The verdict has to be recorded while disconnected, since a connected + // peer's verdict is ignored outright. table.new_contact_records(vec![record]).await; - table.update_status(node_id, PeerStatus::Connected); table.update_status(node_id, PeerStatus::Unwanted); + table.update_status(node_id, PeerStatus::Connected); table.update_status(node_id, PeerStatus::Disconnected); assert_eq!(table.peer_completion(), 0.0, "no longer connected"); diff --git a/crates/networking/p2p/discovery/server.rs b/crates/networking/p2p/discovery/server.rs index e3eb7b6774e..d916c7683de 100644 --- a/crates/networking/p2p/discovery/server.rs +++ b/crates/networking/p2p/discovery/server.rs @@ -573,7 +573,6 @@ mod tests { PeerStatus::Connected, PeerStatus::Disconnected, PeerStatus::Unwanted, - PeerStatus::Disposable, ] { handle.update_status(H256::repeat_byte(1), status); } diff --git a/crates/networking/p2p/peer_handler.rs b/crates/networking/p2p/peer_handler.rs index 2a884cb9e74..d0d2c59a949 100644 --- a/crates/networking/p2p/peer_handler.rs +++ b/crates/networking/p2p/peer_handler.rs @@ -1,4 +1,4 @@ -use crate::discovery::{DiscoveryHandle, PeerStatus}; +use crate::discovery::DiscoveryHandle; use crate::rlpx::initiator::RLPxInitiator; use crate::{ metrics::{CurrentStepValue, METRICS}, @@ -607,16 +607,14 @@ impl PeerHandler { { if block_bodies.len() > block_hashes_len { // More bodies than hashes requested: a protocol violation, so - // drop the peer rather than just scoring it down. + // bottom out its score rather than just nudging it down. debug!( %peer_id, got = block_bodies.len(), requested = block_hashes_len, "Peer returned more block bodies than requested, disposing" ); - self.peer_table.record_failure(peer_id)?; - self.discovery - .update_status(peer_id, PeerStatus::Disposable); + self.peer_table.record_critical_failure(peer_id)?; return Ok(None); } if !block_bodies.is_empty() { @@ -772,9 +770,7 @@ impl PeerHandler { } _ => { debug!("Didn't receive receipts from peer, penalizing peer {peer_id}"); - self.peer_table.record_failure(peer_id)?; - self.discovery - .update_status(peer_id, PeerStatus::Disposable); + self.peer_table.record_critical_failure(peer_id)?; return Ok(None); } }; @@ -791,9 +787,7 @@ impl PeerHandler { } if receipts.len() > block_hashes_len { debug!("Received oversized receipts from peer {peer_id}, penalizing"); - self.peer_table.record_failure(peer_id)?; - self.discovery - .update_status(peer_id, PeerStatus::Disposable); + self.peer_table.record_critical_failure(peer_id)?; return Ok(None); } // Success is recorded by the caller, once the receipts have been From f6f4a832a1362e8601a8e6b447686ea139cbcb8c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Gr=C3=BCner?= <47506558+MegaRedHand@users.noreply.github.com> Date: Wed, 26 Aug 2026 11:59:08 -0300 Subject: [PATCH 06/11] refactor(l1): rename PeerStatus to PeerEvent, and Unwanted to Rejected `PeerStatus` invited reading the variants as exclusive states of a peer, which is what made `Unwanted` look like it subsumed `Disconnected`: any standing "we do not want this peer" implies not being connected to it. The variants are not states. They are things that happened, and two of the three were already named that way. `Unwanted` was the odd one out, naming the flag it sets rather than the event that sets it, and it is now `Rejected`: the moment a connection attempt was turned away. The type is `PeerEvent` and the message `record_peer_event`, matching the `record_*` convention the peer table already uses for noting that something happened. Nothing about behaviour changes. The rename does make the disjointness visible that the old naming hid: a handshake is refused before the connection is registered, so `Rejected` is never a transition out of `Connected`, and the two cannot describe the same peer at the same moment. --- .../networking/p2p/discovery/contact_table.rs | 80 ++++++++++--------- crates/networking/p2p/discovery/mod.rs | 2 +- crates/networking/p2p/discovery/server.rs | 34 ++++---- .../networking/p2p/rlpx/connection/server.rs | 10 +-- 4 files changed, 66 insertions(+), 60 deletions(-) diff --git a/crates/networking/p2p/discovery/contact_table.rs b/crates/networking/p2p/discovery/contact_table.rs index 5e213bd992a..c935742d7d8 100644 --- a/crates/networking/p2p/discovery/contact_table.rs +++ b/crates/networking/p2p/discovery/contact_table.rs @@ -282,15 +282,20 @@ impl Contact { } } -/// What the consumer has just learned about a peer. +/// Something that happened to the consumer's relationship with a peer. /// -/// A report of one event, not a state machine. The variants are not mutually -/// exclusive over a peer's life and each touches only what it names: -/// [`Self::Connected`] and [`Self::Disconnected`] toggle a membership, while -/// [`Self::Unwanted`] and [`Self::Disposable`] are verdicts that accumulate on -/// the contact and are never cleared. +/// Events, not states, and named as such: nothing here describes a peer's +/// standing condition, so no variant can be read as subsuming another. A +/// standing "we do not want this peer" would imply not being connected to it, +/// which is why the verdict is spelled [`Self::Rejected`], the moment we turned +/// an attempt away, rather than after the `unwanted` flag it happens to set. /// -/// [`Self::Unwanted`] deliberately leaves the connected set alone: it is a +/// The three are also disjoint in practice, which the old naming obscured: +/// [`Self::Rejected`] is only ever reported for an attempt that never became a +/// connection, since a handshake is refused before the connection is +/// registered. It is not a transition out of [`Self::Connected`]. +/// +/// [`Self::Rejected`] deliberately leaves the connected set alone: it is a /// verdict about whether to dial a peer in future, not a statement that the /// current connection is over. /// @@ -301,19 +306,20 @@ impl Contact { /// peer's routing-table entry, leaves the connection up, and is erased by the /// next prune. Scoring the peer down is the mechanism for that. #[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub enum PeerStatus { - /// A connection to this peer is up: stop offering it as a dial candidate, - /// and count it towards how hard discovery looks for more. +pub enum PeerEvent { + /// A connection was established: stop offering this peer as a dial + /// candidate, and count it towards how hard discovery looks for more. Connected, - /// The connection is gone. Says nothing about whether the peer is worth - /// having; verdicts already recorded stay recorded. + /// An established connection ended, for any reason. Carries no judgement, + /// and does not disturb one already recorded. Disconnected, - /// Known-bad to the consumer: on another network, or advertising no - /// capability it wants. Never offered as a dial candidate again. + /// A connection attempt was turned away, because the peer is on another + /// network or advertises no capability the consumer wants. Sets the + /// contact's `unwanted` flag, so it is never dialed again. /// /// Ignored for a peer that is currently connected; see - /// [`ContactTable::update_status`]. - Unwanted, + /// [`ContactTable::record_peer_event`]. + Rejected, } /// Result of contact validation. @@ -399,28 +405,28 @@ impl ContactTable { // --- Consumer lifecycle --- - /// Record what the consumer has just learned about a peer. + /// Record something that happened to the consumer's relationship with a peer. /// /// The single door through which the consumer reports anything about a - /// peer; see [`PeerStatus`] for what each variant does and, more + /// peer; see [`PeerEvent`] for what each variant does and, more /// importantly, what it deliberately does not touch. - pub fn update_status(&mut self, node_id: H256, status: PeerStatus) { - match status { - PeerStatus::Connected => self.mark_connected(node_id), - PeerStatus::Disconnected => self.mark_disconnected(&node_id), + pub fn record_peer_event(&mut self, node_id: H256, event: PeerEvent) { + match event { + PeerEvent::Connected => self.mark_connected(node_id), + PeerEvent::Disconnected => self.mark_disconnected(&node_id), // A peer we are connected to is, demonstrably, wanted. This report // can only reach us for one while a second connection attempt to the // same node fails its handshake, and nothing dedupes those: a // transient error on the redundant attempt would otherwise mark a // peer we are happily syncing from as never-dial-again, permanently // and invisibly. The live connection is the better evidence. - PeerStatus::Unwanted if self.connected.contains(&node_id) => { + PeerEvent::Rejected if self.connected.contains(&node_id) => { tracing::debug!( peer = %node_id, - "Ignoring unwanted verdict for a peer we are connected to" + "Ignoring rejection of a peer we are connected to" ); } - PeerStatus::Unwanted => self.set_unwanted(&node_id), + PeerEvent::Rejected => self.set_unwanted(&node_id), } } @@ -1426,8 +1432,8 @@ mod tests { } #[tokio::test] - async fn a_connected_peer_is_never_marked_unwanted() { - // Connected-and-unwanted is a contradiction, and it was reachable: two + async fn a_connected_peer_is_never_rejected() { + // Connected-and-rejected is a contradiction, and it was reachable: two // connection actors can race for one node id, and a transient handshake // error on the loser would permanently retire a peer the winner is // syncing from. The live connection outranks the verdict. @@ -1435,11 +1441,11 @@ mod tests { let (node_id, record) = record_for(17, 1); table.new_contact_records(vec![record]).await; - table.update_status(node_id, PeerStatus::Connected); - table.update_status(node_id, PeerStatus::Unwanted); + table.record_peer_event(node_id, PeerEvent::Connected); + table.record_peer_event(node_id, PeerEvent::Rejected); assert_eq!(table.peer_completion(), 0.1, "still connected"); - table.update_status(node_id, PeerStatus::Disconnected); + table.record_peer_event(node_id, PeerEvent::Disconnected); assert_eq!( table.next_dial_candidate().map(|n| n.node_id()), Some(node_id), @@ -1448,17 +1454,17 @@ mod tests { } #[tokio::test] - async fn an_unwanted_verdict_lands_when_the_peer_is_not_connected() { - // The guard must not swallow the verdict it exists to record. + async fn a_rejection_lands_when_the_peer_is_not_connected() { + // The guard must not swallow the rejection it exists to record. let mut table = table_with(FixedAnswer(true)); let (node_id, record) = record_for(19, 1); table.new_contact_records(vec![record]).await; - table.update_status(node_id, PeerStatus::Unwanted); + table.record_peer_event(node_id, PeerEvent::Rejected); assert!( table.next_dial_candidate().is_none(), - "an unwanted peer that was never connected is never dialed" + "a peer rejected while disconnected is never dialed" ); } @@ -1472,9 +1478,9 @@ mod tests { // The verdict has to be recorded while disconnected, since a connected // peer's verdict is ignored outright. table.new_contact_records(vec![record]).await; - table.update_status(node_id, PeerStatus::Unwanted); - table.update_status(node_id, PeerStatus::Connected); - table.update_status(node_id, PeerStatus::Disconnected); + table.record_peer_event(node_id, PeerEvent::Rejected); + table.record_peer_event(node_id, PeerEvent::Connected); + table.record_peer_event(node_id, PeerEvent::Disconnected); assert_eq!(table.peer_completion(), 0.0, "no longer connected"); assert!( diff --git a/crates/networking/p2p/discovery/mod.rs b/crates/networking/p2p/discovery/mod.rs index 8f393e5f3e9..7413efe09c6 100644 --- a/crates/networking/p2p/discovery/mod.rs +++ b/crates/networking/p2p/discovery/mod.rs @@ -18,7 +18,7 @@ pub mod lookup; pub mod server; pub use contact_table::{ - Contact, ContactTable, ContactValidation, DiscoveryProtocol, PeerStatus, Session, + Contact, ContactTable, ContactValidation, DiscoveryProtocol, PeerEvent, Session, }; pub use server::{ DiscoveryHandle, DiscoveryServer, DiscoveryServerError, DiscoveryServerProtocol, diff --git a/crates/networking/p2p/discovery/server.rs b/crates/networking/p2p/discovery/server.rs index d916c7683de..e0688624b5c 100644 --- a/crates/networking/p2p/discovery/server.rs +++ b/crates/networking/p2p/discovery/server.rs @@ -32,7 +32,7 @@ use tracing::{debug, error, info, trace}; use super::{ DiscoveryConfig, codec::DiscriminatingCodec, contact_table::ContactTable, - contact_table::DiscoveryProtocol, contact_table::PeerStatus, lookup_interval_function, + contact_table::DiscoveryProtocol, contact_table::PeerEvent, lookup_interval_function, }; use std::sync::OnceLock; @@ -75,9 +75,9 @@ pub enum DiscoveryServerError { #[protocol] pub trait DiscoveryServerProtocol: Send + Sync { fn raw_packet(&self, data: BytesMut, from: SocketAddr) -> Result<(), ActorError>; - /// Report what the consumer has just learned about a peer. See - /// [`PeerStatus`] for what each variant does. - fn update_status(&self, node_id: H256, status: PeerStatus) -> Result<(), ActorError>; + /// Report something that happened to the consumer's relationship with a + /// peer. See [`PeerEvent`] for what each variant does. + fn record_peer_event(&self, node_id: H256, event: PeerEvent) -> Result<(), ActorError>; fn revalidate_v4(&self) -> Result<(), ActorError>; fn revalidate_v5(&self) -> Result<(), ActorError>; fn lookup_v4(&self) -> Result<(), ActorError>; @@ -121,12 +121,12 @@ impl DiscoveryHandle { self.0.get() } - /// Report what just happened to a peer. See [`PeerStatus`]: the variants - /// are separate reports rather than exclusive states, so a peer can be - /// reported `Disposable` while its connection is still up. - pub fn update_status(&self, node_id: H256, status: PeerStatus) { + /// Report something that happened to a peer. See [`PeerEvent`]: these are + /// events, not a state machine, and `Rejected` is only ever reported for an + /// attempt that never became a connection. + pub fn record_peer_event(&self, node_id: H256, event: PeerEvent) { if let Some(server) = self.server() { - let _ = server.update_status(node_id, status); + let _ = server.record_peer_event(node_id, event); } } @@ -392,12 +392,12 @@ impl DiscoveryServer { } #[send_handler] - async fn handle_update_status( + async fn handle_record_peer_event( &mut self, - msg: discovery_server_protocol::UpdateStatus, + msg: discovery_server_protocol::RecordPeerEvent, _ctx: &Context, ) { - self.contacts.update_status(msg.node_id, msg.status); + self.contacts.record_peer_event(msg.node_id, msg.event); } #[request_handler] @@ -569,12 +569,12 @@ mod tests { // answer `None` rather than wait for a server that may never arrive. let handle = DiscoveryHandle::new(); - for status in [ - PeerStatus::Connected, - PeerStatus::Disconnected, - PeerStatus::Unwanted, + for event in [ + PeerEvent::Connected, + PeerEvent::Disconnected, + PeerEvent::Rejected, ] { - handle.update_status(H256::repeat_byte(1), status); + handle.record_peer_event(H256::repeat_byte(1), event); } handle.prune(); diff --git a/crates/networking/p2p/rlpx/connection/server.rs b/crates/networking/p2p/rlpx/connection/server.rs index eb66e5b0071..0f5d11999cb 100644 --- a/crates/networking/p2p/rlpx/connection/server.rs +++ b/crates/networking/p2p/rlpx/connection/server.rs @@ -7,7 +7,7 @@ use crate::rlpx::l2::{ }; use crate::{ backend, - discovery::{DiscoveryHandle, PeerStatus}, + discovery::{DiscoveryHandle, PeerEvent}, metrics::METRICS, network::P2PContext, peer_table::{PeerTable, PeerTableServerProtocol as _}, @@ -393,9 +393,9 @@ impl PeerConnectionServer { match &reason { PeerConnectionError::NoMatchingCapabilities | PeerConnectionError::HandshakeError(_) => { - established_state.discovery.update_status( + established_state.discovery.record_peer_event( established_state.node.node_id(), - PeerStatus::Unwanted, + PeerEvent::Rejected, ); } _ => {} @@ -475,7 +475,7 @@ impl PeerConnectionServer { // table's own map equally stale, so the two stores at least agree. established_state .discovery - .update_status(established_state.node.node_id(), PeerStatus::Disconnected); + .record_peer_event(established_state.node.node_id(), PeerEvent::Disconnected); // Free the peer's tx-broadcaster index (and clear its bit across known txs) so // the broadcaster's per-peer index map / PeerMask widths stay bounded to live peers. if let Err(e) = established_state @@ -839,7 +839,7 @@ where )?; state .discovery - .update_status(state.node.node_id(), PeerStatus::Connected); + .record_peer_event(state.node.node_id(), PeerEvent::Connected); trace!(peer=%state.node, "Peer connection initialized."); From 43216e7cb84c6cb070d85192fc9d9288a1467bb1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Gr=C3=BCner?= <47506558+MegaRedHand@users.noreply.github.com> Date: Wed, 26 Aug 2026 18:16:58 -0300 Subject: [PATCH 07/11] fix(l1): make peer registration a claim, so duplicate connections lose cleanly Nothing deduped connections by node id. Two actors could reach registration for one peer: crossing dials, a peer opening a second socket, or a stale connected set letting us dial someone we already hold. Identity is only known after the handshake, so neither the inbound admission semaphore, which caps count rather than who, nor the initiator's candidate filter can catch it earlier. `new_connected_peer` overwrote on a duplicate key, which left two live sockets behind one table entry. The displaced connection stayed open but invisible to `get_best_peer`, and the first actor to stop then removed the survivor's entry, reported it disconnected to discovery, and freed its broadcaster index. The peer was live, unusable, uncounted, and offered as a dial candidate again. Registration is now a request returning whether the slot was granted. The check and the insert both run in the peer table's message loop, so the claim is atomic between two actors, and the loser hangs up with `DisconnectSent(AlreadyConnected)` rather than registering. That reaches `connection_failed`, which sends the peer a `Disconnect(AlreadyConnected)`: the behaviour devp2p expects of the receiving side, and which this node previously only ever handled on the way in. It also makes the existing "already connected, don't replace it" arm reachable, which until now nothing could construct. Teardown is gated on a new `registered` flag. Without it the loser's `stopped()` hook still releases the winner's registration, which is most of the bug. The peer table keeps no test module since the contact split; this adds one for the claim, including that a slot is released on disconnect so a reconnecting peer is not turned away for the life of the process. Known limit: a half-open connection still holds its slot until the actor notices, and a peer that reconnects in the meantime is refused. Previously it would have displaced the zombie, at the cost of the eviction bug above. Neither handles that case well, and it wants a liveness check the actor runtime does not currently expose. --- crates/networking/p2p/peer_table.rs | 123 ++++++++++++++++-- .../p2p/rlpx/connection/handshake.rs | 1 + .../networking/p2p/rlpx/connection/server.rs | 86 +++++++----- 3 files changed, 165 insertions(+), 45 deletions(-) diff --git a/crates/networking/p2p/peer_table.rs b/crates/networking/p2p/peer_table.rs index 69b418da0a0..a03cccb40e6 100644 --- a/crates/networking/p2p/peer_table.rs +++ b/crates/networking/p2p/peer_table.rs @@ -143,13 +143,7 @@ impl Drop for RequestPermit { #[protocol] pub trait PeerTableServerProtocol: Send + Sync { // Send (cast) methods - fn new_connected_peer( - &self, - node: Node, - connection: PeerConnection, - capabilities: Vec, - is_inbound: bool, - ) -> Result<(), ActorError>; + fn remove_peer(&self, node_id: H256) -> Result<(), ActorError>; fn dec_requests(&self, node_id: H256) -> Result<(), ActorError>; fn record_success(&self, node_id: H256) -> Result<(), ActorError>; @@ -158,6 +152,20 @@ pub trait PeerTableServerProtocol: Send + Sync { fn shutdown(&self) -> Result<(), ActorError>; // Request (call) methods + + /// Claim the slot for this peer, returning whether it was granted. + /// + /// `false` means we already hold a connection to this node id and the + /// caller lost a race, so it should hang up rather than register. The + /// check and the insert both happen in this actor's message loop, which is + /// what makes the claim atomic between two connection actors. + fn new_connected_peer( + &self, + node: Node, + connection: PeerConnection, + capabilities: Vec, + is_inbound: bool, + ) -> Response; fn peer_count(&self) -> Response; fn peer_count_by_capabilities(&self, capabilities: Vec) -> Response; fn target_peers_reached(&self) -> Response; @@ -229,16 +237,18 @@ impl PeerTableServer { // === Send handlers === - #[send_handler] + #[request_handler] async fn handle_new_connected_peer( &mut self, msg: peer_table_server_protocol::NewConnectedPeer, _ctx: &Context, - ) { - let new_peer_id = msg.node.node_id(); - let mut new_peer = PeerData::new(msg.node, None, Some(msg.connection), msg.capabilities); - new_peer.is_connection_inbound = msg.is_inbound; - self.peers.insert(new_peer_id, new_peer); + ) -> bool { + self.do_new_connected_peer( + msg.node, + Some(msg.connection), + msg.capabilities, + msg.is_inbound, + ) } #[send_handler] @@ -538,6 +548,31 @@ impl PeerTableServer { // --- Peer selection --- + /// Claim the slot for `node`, returning whether it was granted. + /// + /// Refuses rather than overwrites. Two connection actors can reach this + /// point for one node id (crossing dials, or a peer opening a second + /// socket), and overwriting let the second silently displace the first: the + /// displaced connection stayed open but became invisible to peer selection, + /// and whichever actor stopped first then removed the survivor's entry on + /// its way out. + fn do_new_connected_peer( + &mut self, + node: Node, + connection: Option, + capabilities: Vec, + is_inbound: bool, + ) -> bool { + let new_peer_id = node.node_id(); + if self.peers.contains_key(&new_peer_id) { + return false; + } + let mut new_peer = PeerData::new(node, None, connection, capabilities); + new_peer.is_connection_inbound = is_inbound; + self.peers.insert(new_peer_id, new_peer); + true + } + fn weight_peer(&self, score: &i64, requests: &i64) -> i64 { score * SCORE_WEIGHT - requests * REQUESTS_WEIGHT } @@ -669,3 +704,65 @@ impl PeerTableServer { } pub type PeerTable = ActorRef; + +#[cfg(test)] +mod tests { + use super::*; + use ethrex_common::H512; + use std::net::Ipv4Addr; + + fn node(seed: u8) -> Node { + Node::new( + IpAddr::V4(Ipv4Addr::new(127, 0, 0, seed)), + 30303, + 30303, + H512::from_low_u64_be(seed as u64 + 1), + ) + } + + /// Registration with no live connection behind it: enough to exercise the + /// claim, which only ever looks at the node id. + fn claim(table: &mut PeerTableServer, node: Node) -> bool { + table.do_new_connected_peer(node, None, vec![], false) + } + + #[test] + fn a_second_connection_to_the_same_peer_is_refused() { + // Two connection actors can reach registration for one node id, and the + // loser has to find out so it can hang up. Overwriting instead left two + // live sockets and one table entry, so the first actor to stop evicted + // the other's connection. + let mut table = PeerTableServer::new(10); + + assert!( + claim(&mut table, node(1)), + "first connection claims the slot" + ); + assert!(!claim(&mut table, node(1)), "the second is refused"); + assert_eq!(table.peers.len(), 1, "and did not displace the first"); + } + + #[test] + fn a_different_peer_is_unaffected() { + let mut table = PeerTableServer::new(10); + + assert!(claim(&mut table, node(1))); + assert!(claim(&mut table, node(2)), "the claim is per node id"); + assert_eq!(table.peers.len(), 2); + } + + #[test] + fn a_peer_can_register_again_after_disconnecting() { + // The claim must not outlive the connection, or a reconnecting peer + // would be turned away for the life of the process. + let mut table = PeerTableServer::new(10); + + assert!(claim(&mut table, node(3))); + table.peers.swap_remove(&node(3).node_id()); + + assert!( + claim(&mut table, node(3)), + "a reconnect is granted the slot" + ); + } +} diff --git a/crates/networking/p2p/rlpx/connection/handshake.rs b/crates/networking/p2p/rlpx/connection/handshake.rs index 74a20eda6b1..f3d3b40dc88 100644 --- a/crates/networking/p2p/rlpx/connection/handshake.rs +++ b/crates/networking/p2p/rlpx/connection/handshake.rs @@ -154,6 +154,7 @@ pub(crate) async fn perform( current_requests: HashMap::new(), disconnect_reason: None, is_validated: false, + registered: false, serve_request_window_start: std::time::Instant::now(), serve_requests_in_window: 0, txs_sent_to_peer: 0, diff --git a/crates/networking/p2p/rlpx/connection/server.rs b/crates/networking/p2p/rlpx/connection/server.rs index 0f5d11999cb..ea4b431693c 100644 --- a/crates/networking/p2p/rlpx/connection/server.rs +++ b/crates/networking/p2p/rlpx/connection/server.rs @@ -298,6 +298,10 @@ pub struct Established { pub(crate) disconnect_reason: Option, // Indicates if the peer has been validated (ie. the connection was established successfully) pub(crate) is_validated: bool, + /// Whether this actor holds the peer table's slot for this node id. Only the + /// actor that claimed it may release it: a duplicate connection that lost + /// the race must leave the winner's registration alone on its way out. + pub(crate) registered: bool, // Rate limiting: start of the current incoming-request window pub(crate) serve_request_window_start: Instant, // Rate limiting: number of data-serving requests received in the current window @@ -457,32 +461,38 @@ impl PeerConnectionServer { ) .await; } - if let Err(e) = established_state - .peer_table - .remove_peer(established_state.node.node_id()) - { - debug!("Failed to remove peer from table: {e}"); - } - // Pairs with the `Connected` report in `initialize_connection`. This is - // the only teardown an established connection takes, and it still runs - // when a message handler panics, because the actor loop catches the - // unwind before falling through to `stopped()`. - // - // It does not cover a panic inside `started()` itself, which cancels the - // actor and returns without running this hook. `Connected` is reported - // from there, so that window can strand an id in discovery's connected - // set. `remove_peer` above is lost to the same window, leaving the peer - // table's own map equally stale, so the two stores at least agree. - established_state - .discovery - .record_peer_event(established_state.node.node_id(), PeerEvent::Disconnected); - // Free the peer's tx-broadcaster index (and clear its bit across known txs) so - // the broadcaster's per-peer index map / PeerMask widths stay bounded to live peers. - if let Err(e) = established_state - .tx_broadcaster - .remove_peer(established_state.node.node_id()) - { - debug!("Failed to remove peer from tx broadcaster: {e}"); + // Gated on `registered`: an actor that lost the claim never put + // anything in these tables, and releasing on its way out would + // evict the connection that won. + if established_state.registered { + if let Err(e) = established_state + .peer_table + .remove_peer(established_state.node.node_id()) + { + debug!("Failed to remove peer from table: {e}"); + } + // Pairs with the `Connected` report in `initialize_connection`. This is + // the only teardown an established connection takes, and it still runs + // when a message handler panics, because the actor loop catches the + // unwind before falling through to `stopped()`. + // + // It does not cover a panic inside `started()` itself, which cancels the + // actor and returns without running this hook. `Connected` is reported + // from there, so that window can strand an id in discovery's connected + // set. `remove_peer` above is lost to the same window, leaving the peer + // table's own map equally stale, so the two stores at least agree. + established_state.discovery.record_peer_event( + established_state.node.node_id(), + PeerEvent::Disconnected, + ); + // Free the peer's tx-broadcaster index (and clear its bit across known txs) so + // the broadcaster's per-peer index map / PeerMask widths stay bounded to live peers. + if let Err(e) = established_state + .tx_broadcaster + .remove_peer(established_state.node.node_id()) + { + debug!("Failed to remove peer from tx broadcaster: {e}"); + } } established_state.teardown().await; } @@ -831,12 +841,24 @@ where handle: ctx.actor_ref(), }; - state.peer_table.new_connected_peer( - state.node.clone(), - connection.clone(), - state.capabilities.clone(), - state.is_inbound, - )?; + // Claim the slot before telling anyone we have this peer. Losing means + // another actor already holds a connection to the same node id, so we hang + // up with the reason devp2p expects and touch neither table. + let claimed = state + .peer_table + .new_connected_peer( + state.node.clone(), + connection.clone(), + state.capabilities.clone(), + state.is_inbound, + ) + .await?; + if !claimed { + return Err(PeerConnectionError::DisconnectSent( + DisconnectReason::AlreadyConnected, + )); + } + state.registered = true; state .discovery .record_peer_event(state.node.node_id(), PeerEvent::Connected); From 532cdcf90515ac7c12b27e8f4aa7f338d55d6fd7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Gr=C3=BCner?= <47506558+MegaRedHand@users.noreply.github.com> Date: Wed, 26 Aug 2026 18:19:49 -0300 Subject: [PATCH 08/11] chore(l1): call the contact table by its name in the merged discv5 comment --- crates/networking/p2p/discovery/discv5_handlers.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/networking/p2p/discovery/discv5_handlers.rs b/crates/networking/p2p/discovery/discv5_handlers.rs index 2ff33be1308..0dfea8895bc 100644 --- a/crates/networking/p2p/discovery/discv5_handlers.rs +++ b/crates/networking/p2p/discovery/discv5_handlers.rs @@ -543,7 +543,7 @@ impl DiscoveryServer { ) -> Result<(), DiscoveryServerError> { // Only accept a NODES that answers a FINDNODE we actually sent to this // peer. Without the check, any peer that has completed a handshake can - // push ENRs of its choosing into our peer table with an unsolicited + // push ENRs of its choosing into our contact table with an unsolicited // NODES, and we then hand them back out in our own FINDNODE responses. let solicited = self.discv5.as_ref().is_some_and(|discv5| { discv5 From 5876f1f77840bd3cc26b6f9a1324722c53ba254f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Gr=C3=BCner?= <47506558+MegaRedHand@users.noreply.github.com> Date: Fri, 28 Aug 2026 16:45:28 -0300 Subject: [PATCH 09/11] fix(l1): expire discv5 sessions on idle time, not on age `SESSION_TTL` arrived in #6900 as a bound on `session_ips`, the map recording which IP a session was established from. Evicting an entry there costs nothing on the wire, so an hour, round and safely large, was a fine number to pick. Applying that same constant to the session keys changed what the number buys: a peer we are still talking to loses its keys mid conversation, and the next discv5 message to it pays a WHOAREYOU and a handshake before it is delivered. If that exchange outruns `MESSAGE_CACHE_TIMEOUT`, the message that triggered it is already gone from `pending_by_nonce` and is never retried, so the lookup round is lost outright. What the TTL exists to bound is the peer that handshakes once and never comes back, and nothing about such a peer refreshes anything. So the sweep now measures idle time instead: a session lives as long as it is used, while the handshake-and-vanish entries still go after an hour. Only inbound traffic that decrypted and arrived from the session's own address counts as use. A send would prove no more than that we still want the peer, and refreshing on one would keep alive precisely the sessions this is meant to reap. Both halves of a session are refreshed in the same place, because keys that outlive their `session_ips` entry silently lose the IP-rebinding check in `discv5_handle_ordinary`: a missing entry reads as nothing to compare against. That invariant is why the refresh sits after the match rather than in the decrypt arm, whose sibling arms return through `&mut self`, and it is what the new handler tests pin down, by aging both halves past the TTL and asserting they survive or are reaped together. Known limit: a session in continuous use is never rotated. Bounding how much traffic sits under one set of keys wants a separate maximum lifetime, kept apart from this idle bound so each can be reasoned about on its own. --- .../networking/p2p/discovery/contact_table.rs | 80 ++++++++-- .../p2p/discovery/discv5_handlers.rs | 147 +++++++++++++++++- crates/networking/p2p/discv5/server.rs | 31 ++-- 3 files changed, 231 insertions(+), 27 deletions(-) diff --git a/crates/networking/p2p/discovery/contact_table.rs b/crates/networking/p2p/discovery/contact_table.rs index c935742d7d8..903a10199f0 100644 --- a/crates/networking/p2p/discovery/contact_table.rs +++ b/crates/networking/p2p/discovery/contact_table.rs @@ -355,9 +355,11 @@ pub struct ContactTable { /// before the contact's ENR is known or parseable, which is why it cannot simply live /// on the contact. /// - /// Each entry carries when it was established, because a remote peer decides how many + /// Each entry carries when it was last used, because a remote peer decides how many /// of these we hold: every handshake inserts one, and nothing about a handshake - /// obliges the peer to ever come back. [`Self::prune`] evicts on [`SESSION_TTL`]. + /// obliges the peer to ever come back. [`Self::prune`] evicts entries left idle for + /// [`SESSION_TTL`], so the store is bounded by the peers still talking to us rather + /// than by every peer that ever handshaked. sessions: FxHashMap, /// What this consumer requires of a discovered peer. Judged as each ENR /// arrives, over either discovery protocol; the answer is cached on the @@ -458,12 +460,12 @@ impl ContactTable { self.connected.len() as f64 / self.target_peers as f64 } - /// Backdates every stored session by `by`, so a test can drive the TTL sweep in - /// [`Self::prune`] without waiting out a real hour. + /// Backdates the last use of every stored session by `by`, so a test can drive the + /// idle sweep in [`Self::prune`] without waiting out a real hour. #[cfg(test)] pub(crate) fn age_sessions_for_test(&mut self, by: Duration) { - for (_, established_at) in self.sessions.values_mut() { - *established_at -= by; + for (_, last_used) in self.sessions.values_mut() { + *last_used -= by; } } @@ -481,15 +483,32 @@ impl ContactTable { .map(|(session, _)| session.clone()) } - /// Store a session, stamped with the moment it was established. + /// Store a session, stamped as used now. /// - /// Re-handshaking restamps it, which is the only way an entry's life is extended: - /// merely using a session does not, so keys expire on the same schedule as the - /// `session_ips` entry guarding them and a still-wanted peer simply re-handshakes. + /// The stamp is what [`Self::prune`] ages out. A re-handshake restamps it, and so + /// does ordinary use through [`Self::touch_session`]. pub fn set_session(&mut self, node_id: H256, session: Session) { self.sessions.insert(node_id, (session, Instant::now())); } + /// Extend a session's life: it was just used to decrypt a packet that arrived from + /// the address the session was established from. + /// + /// Only inbound, verified use counts. A send proves only that we still want the + /// peer, so refreshing on one would keep alive the sessions of peers that never + /// answer, which is the population the TTL exists to bound. + /// + /// Callers must refresh the matching `session_ips` entry in the same breath: keys + /// that outlive their guard silently drop the IP-rebinding check in + /// `discv5_handle_ordinary`, where a missing entry reads as nothing to compare + /// against. Does nothing for a node with no stored session, so a packet racing + /// [`Self::prune`] cannot resurrect one. + pub fn touch_session(&mut self, node_id: &H256) { + if let Some((_, last_used)) = self.sessions.get_mut(node_id) { + *last_used = Instant::now(); + } + } + // --- Contact flags --- /// Mark a contact as one we should stop keeping: it failed to answer a ping, @@ -652,10 +671,10 @@ impl ContactTable { /// Pruned contacts remain in the connection pool so they can be retried /// later: the consumer will reject them on connecting if they are truly bad. /// - /// Dropping a contact drops its discv5 session too, and any session older than + /// Dropping a contact drops its discv5 session too, and any session left unused for /// [`SESSION_TTL`] goes with it. /// - /// The age sweep is what actually bounds the store. Pruning by contact is not + /// The idle sweep is what actually bounds the store. Pruning by contact is not /// enough on its own: a contact can leave the table without ever being marked /// disposable (evicted from a replacement queue, or simply never pruned because /// it was only ever marked `unwanted`), and a session can be stored for a node @@ -693,9 +712,8 @@ impl ContactTable { } let now = Instant::now(); - self.sessions.retain(|_, (_, established_at)| { - now.saturating_duration_since(*established_at) < SESSION_TTL - }); + self.sessions + .retain(|_, (_, last_used)| now.saturating_duration_since(*last_used) < SESSION_TTL); } /// Pick the next node to hand to the RLPx dialer, or `None` when the pool @@ -1302,6 +1320,38 @@ mod tests { ); } + #[tokio::test] + async fn using_a_session_keeps_it_out_of_the_idle_sweep() { + // Why the sweep is idle-based: a peer we are still talking to must not be + // made to re-handshake mid-conversation. That costs a WHOAREYOU round trip + // and loses the message that triggered it if the exchange is slow. + let mut table = table_with(FixedAnswer(true)); + let node_id = H256::repeat_byte(0x7a); + + table.set_session(node_id, session()); + table.age_sessions_for_test(SESSION_TTL); + table.touch_session(&node_id); + table.prune(); + + assert!( + table.session(&node_id).is_some(), + "a session used inside the TTL survives the sweep" + ); + } + + #[tokio::test] + async fn touching_a_reaped_session_does_not_resurrect_it() { + // `touch_session` runs for every packet that decrypts, which includes one + // that raced `prune`. Inserting here would defeat the bound entirely, since + // the keys are gone and nothing could decrypt with the entry anyway. + let mut table = table_with(FixedAnswer(true)); + let node_id = H256::repeat_byte(0x7b); + + table.touch_session(&node_id); + + assert!(table.session(&node_id).is_none()); + } + #[tokio::test] async fn pruning_reaches_a_contact_in_the_replacement_list() { // The replacement half of `prune` had no coverage: dropping the diff --git a/crates/networking/p2p/discovery/discv5_handlers.rs b/crates/networking/p2p/discovery/discv5_handlers.rs index 0dfea8895bc..45480361bec 100644 --- a/crates/networking/p2p/discovery/discv5_handlers.rs +++ b/crates/networking/p2p/discovery/discv5_handlers.rs @@ -112,6 +112,18 @@ impl DiscoveryServer { } }; + // The packet decrypted and arrived from the address this session was established + // from, which is the only thing that keeps a session alive. Both halves are + // refreshed together: keys outliving their `session_ips` entry would silently lose + // the IP-rebinding check above. Placed after the match so a packet from a + // mismatched IP, or one that failed to decrypt, extends nothing. + self.contacts.touch_session(&src_id); + if let Some(discv5) = self.discv5.as_mut() + && let Some(source) = discv5.session_ips.get_mut(&src_id) + { + source.last_used = Instant::now(); + } + tracing::trace!(protocol = "discv5", received = %ordinary.message, from = %src_id, %addr); self.discv5_handle_message(ordinary, addr, None).await @@ -258,7 +270,7 @@ impl DiscoveryServer { src_id, SessionSource { ip: addr.ip(), - established_at: std::time::Instant::now(), + last_used: std::time::Instant::now(), }, ); @@ -936,3 +948,136 @@ fn generate_req_id() -> Bytes { let mut rng = OsRng; Bytes::from(rng.r#gen::().to_be_bytes().to_vec()) } + +#[cfg(test)] +mod tests { + use super::*; + use crate::discv5::server::SESSION_TTL; + use crate::discv5::session::Session; + use crate::peer_filter::AcceptAllFilter; + use std::net::IpAddr; + use std::sync::Arc; + use tokio::net::UdpSocket; + + /// The peer's outbound key, which is our inbound key: what a packet from it is + /// encrypted with and what the handler must look up to decrypt one. + const PEER_KEY: [u8; 16] = [7; 16]; + const SESSION_IP: &str = "127.0.0.1"; + + async fn server_with_session(session_ip: IpAddr) -> (DiscoveryServer, H256) { + let local_node = Node::from_enode_url( + "enode://d860a01f9722d78051619d1e2351aba3f43f943f6f00718d1b9baa4101932a1f5011f16bb2b1bb35db20d6fe28fa0bf09636d26a87d31de9ec6203eeedb1f666@18.138.108.67:30303", + ).expect("Bad enode url"); + let signer = SecretKey::new(&mut OsRng); + let local_node_record = NodeRecord::from_node(&local_node, 1, &signer).unwrap(); + let mut server = DiscoveryServer::new_for_discv5_test( + local_node, + local_node_record, + signer, + Arc::new(UdpSocket::bind("127.0.0.1:0").await.unwrap()), + Box::new(AcceptAllFilter), + ); + + let peer_id = H256::repeat_byte(0xab); + server.contacts_mut().set_session( + peer_id, + Session { + outbound_key: [8; 16], + inbound_key: PEER_KEY, + }, + ); + server + .discv5 + .as_mut() + .expect("discv5 state must exist") + .session_ips + .insert( + peer_id, + SessionSource { + ip: session_ip, + last_used: Instant::now(), + }, + ); + (server, peer_id) + } + + /// An unsolicited NODES message: it reaches the refresh, then `discv5_handle_message` + /// drops it for having no matching request, so the packet has no other effect. + fn packet_from(peer_id: H256) -> Packet { + Ordinary { + src_id: peer_id, + message: Message::Nodes(NodesMessage { + req_id: Bytes::from_static(&[1, 2, 3]), + total: 1, + nodes: vec![], + }), + } + .encode(&[0; 12], [0; 16], &PEER_KEY) + .expect("failed to encode test packet") + } + + fn age_session(server: &mut DiscoveryServer, by: Duration) { + server.contacts_mut().age_sessions_for_test(by); + for source in server + .discv5 + .as_mut() + .expect("discv5 state must exist") + .session_ips + .values_mut() + { + source.last_used -= by; + } + } + + /// Runs both halves' sweeps and reports which survived, as `(keys, session_ips)`. + fn survives_sweeps(server: &mut DiscoveryServer, peer_id: &H256) -> (bool, bool) { + server.contacts_mut().prune(); + let keys = server.contacts_mut().session(peer_id).is_some(); + let discv5 = server.discv5.as_mut().expect("discv5 state must exist"); + discv5.cleanup_stale_entries(); + (keys, discv5.session_ips.contains_key(peer_id)) + } + + #[tokio::test] + async fn a_packet_that_decrypts_refreshes_both_halves_of_the_session() { + // The halves must move together. Keys that outlive their `session_ips` entry + // silently lose the IP-rebinding check, since a missing entry reads as nothing + // to compare against. + let (mut server, peer_id) = server_with_session(SESSION_IP.parse().unwrap()).await; + age_session(&mut server, SESSION_TTL); + + server + .discv5_handle_ordinary( + packet_from(peer_id), + format!("{SESSION_IP}:30304").parse().unwrap(), + ) + .await + .expect("handling an unsolicited NODES packet should not fail"); + + assert_eq!( + survives_sweeps(&mut server, &peer_id), + (true, true), + "using a session must keep both its keys and its IP guard alive" + ); + } + + #[tokio::test] + async fn a_packet_from_another_ip_refreshes_nothing() { + // The rebinding check answers this one with a WHOAREYOU. Refreshing here would + // let whoever can reach us from another address hold a session open forever. + let (mut server, peer_id) = server_with_session(SESSION_IP.parse().unwrap()).await; + age_session(&mut server, SESSION_TTL); + + // The WHOAREYOU reply goes out over a real socket to a port with no listener; + // whether that send reports an error is irrelevant to what is asserted here. + let _ = server + .discv5_handle_ordinary(packet_from(peer_id), "127.0.0.2:30304".parse().unwrap()) + .await; + + assert_eq!( + survives_sweeps(&mut server, &peer_id), + (false, false), + "an aged session used from the wrong address is still reaped" + ); + } +} diff --git a/crates/networking/p2p/discv5/server.rs b/crates/networking/p2p/discv5/server.rs index 5c56b76ebbc..326bcb5614e 100644 --- a/crates/networking/p2p/discv5/server.rs +++ b/crates/networking/p2p/discv5/server.rs @@ -24,26 +24,35 @@ const IP_VOTE_WINDOW: Duration = Duration::from_secs(300); const IP_VOTE_THRESHOLD: usize = 3; /// Timeout for pending messages awaiting WhoAreYou response. const MESSAGE_CACHE_TIMEOUT: Duration = Duration::from_secs(2); -/// Max age of a discv5 session before it is evicted. Bounds both halves of a session: -/// the symmetric keys in the contact table and the `session_ips` entry that guards them. -/// Both are inserted per handshake, by a remote peer's schedule, so without this neither -/// is ever removed for a node we do not keep as a peer. +/// Max time a discv5 session may sit unused before it is evicted. Bounds both halves of +/// a session: the symmetric keys in the contact table and the `session_ips` entry that +/// guards them. Both are inserted per handshake, by a remote peer's schedule, so without +/// this neither is ever removed for a node we do not keep as a peer. +/// +/// Idle rather than absolute, so a peer we are still talking to is never made to +/// re-handshake mid-conversation: that costs a WHOAREYOU round trip and drops the message +/// that triggered it if the exchange outruns `MESSAGE_CACHE_TIMEOUT`. The +/// handshake-once-and-vanish population this exists to bound is unaffected, since nothing +/// refreshes those. /// /// Shared rather than duplicated: when the keys outlive their `session_ips` entry, the /// IP-rebinding check in `discv5_handle_ordinary` silently stops applying to a session -/// that still decrypts. +/// that still decrypts. Both halves are refreshed together, for that same reason. pub const SESSION_TTL: Duration = Duration::from_secs(3600); /// How long an outstanding FINDNODE stays eligible to be answered. Generous /// enough for a multi-packet NODES response over a slow link, short enough that /// a request id cannot be replayed against us much later. const PENDING_FINDNODE_TIMEOUT: Duration = Duration::from_secs(10); -/// Source IP a discv5 session was established from, paired with when it was recorded so stale -/// entries can be evicted (see `SESSION_TTL`). +/// Source IP a discv5 session was established from, paired with when the session was last +/// used so idle entries can be evicted (see `SESSION_TTL`). +/// +/// The IP itself is never refreshed, only the timestamp: rebinding is still detected for +/// the whole life of the session. #[derive(Debug, Clone)] pub struct SessionSource { pub ip: IpAddr, - pub established_at: Instant, + pub last_used: Instant, } /// Discv5-specific state held within the unified DiscoveryServer. @@ -62,8 +71,8 @@ pub struct Discv5State { pub whoareyou_global_count: u32, /// Start of the current global rate limit window. pub whoareyou_global_window_start: Instant, - /// Tracks the source IP that each session was established from, with the insertion time - /// so stale entries can be evicted (see `SESSION_TTL`). + /// Tracks the source IP that each session was established from, with the time the + /// session was last used so idle entries can be evicted (see `SESSION_TTL`). pub session_ips: FxHashMap, /// Collects recipient_addr IPs from PONGs for external IP detection via majority voting. pub ip_votes: FxHashMap>, @@ -144,7 +153,7 @@ impl Discv5State { let before_sessions = self.session_ips.len(); self.session_ips - .retain(|_node_id, source| now.duration_since(source.established_at) < SESSION_TTL); + .retain(|_node_id, source| now.duration_since(source.last_used) < SESSION_TTL); let removed_sessions = before_sessions - self.session_ips.len(); let total_removed = removed_messages + removed_challenges + removed_sessions; From aacca7631d043b0ccc4fe380b541afc2dca235e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Gr=C3=BCner?= <47506558+MegaRedHand@users.noreply.github.com> Date: Fri, 28 Aug 2026 18:46:07 -0300 Subject: [PATCH 10/11] fix(l1): correct discovery's mirror of the connected set from the peer table Discovery keeps its own set of the peers the consumer is connected to, because it must never call into that consumer: two actors with sequential mailboxes that call each other can deadlock, so every message across the boundary travels inward. The set is fed by `PeerEvent` casts from each connection actor. A mirror can drift where a read cannot. `Connected` is reported from `initialize_connection`, which runs inside `started()`, while the paired `Disconnected` is reported from `stopped()`, and an actor whose `started()` panics never runs the latter. The stranded id is not cosmetic: it inflates `peer_completion`, which is what paces the iterative lookups, so discovery walks the easing curve towards its 10s ceiling while believing it has peers it does not have; and `next_dial_candidate` skips anything in the set, so the node is never dialed again for the life of the process. Neither is recoverable from inside discovery, which has no way to ask. So the consumer pushes the truth. The RLPx initiator already holds both handles and already runs on a timer, so it reads the peer table's connected ids every five seconds and casts them to discovery, which replaces its set and logs when the two disagreed. `connected_peer_ids` returns ids alone rather than reusing `get_connected_peers`, which clones a `PeerConnection` per peer to answer. Five seconds is deliberately slow. This corrects a mirror that is right in every ordinary connect and disconnect; it is not the mechanism that keeps it current, and polling faster would only add traffic to the actor that also drains UDP. --- .../networking/p2p/discovery/contact_table.rs | 69 +++++++++++++++++++ crates/networking/p2p/discovery/server.rs | 22 ++++++ crates/networking/p2p/peer_table.rs | 13 ++++ crates/networking/p2p/rlpx/initiator.rs | 29 ++++++++ 4 files changed, 133 insertions(+) diff --git a/crates/networking/p2p/discovery/contact_table.rs b/crates/networking/p2p/discovery/contact_table.rs index 903a10199f0..66e6149f7a7 100644 --- a/crates/networking/p2p/discovery/contact_table.rs +++ b/crates/networking/p2p/discovery/contact_table.rs @@ -450,6 +450,28 @@ impl ContactTable { self.connected.remove(node_id); } + /// Replace the mirror of the consumer's connections with the authoritative + /// set it just reported. + /// + /// [`Self::record_peer_event`] keeps this set current in the normal case, but + /// it is fed by casts from per-connection actors, and an actor that dies + /// before its teardown hook runs reports `Connected` with no matching + /// `Disconnected`. A stranded id is not cosmetic: it inflates + /// [`Self::peer_completion`], which stretches the lookup interval, and it + /// makes [`Self::next_dial_candidate`] skip that node for the life of the + /// process. Nothing here can be recovered by discovery on its own, so the + /// consumer pushes the truth periodically. + pub fn reconcile_connected(&mut self, connected: FxHashSet) { + if self.connected != connected { + tracing::debug!( + mirrored = self.connected.len(), + actual = connected.len(), + "Correcting discovery's view of connected peers" + ); + self.connected = connected; + } + } + /// How far along the consumer is towards the connection count it wants. /// Feeds the lookup interval: a node with no peers looks hard, a full one /// coasts. @@ -1339,6 +1361,53 @@ mod tests { ); } + #[tokio::test] + async fn reconciling_releases_a_peer_the_consumer_never_reported_leaving() { + // A connection actor that dies before its teardown hook runs reports + // `Connected` and never `Disconnected`. The stranded id is not cosmetic: + // it inflates `peer_completion`, which stretches the lookup interval, and + // it hides the node from the dialer for the life of the process. + let mut table = table_with(FixedAnswer(true)); + let (node_id, record) = record_for(21, 1); + table.new_contact_records(vec![record]).await; + + table.mark_connected(node_id); + assert_eq!( + table.peer_completion(), + 0.1, + "`table_with` targets 10 peers" + ); + assert!( + table.next_dial_candidate().is_none(), + "a connected peer is not offered to the dialer" + ); + + table.reconcile_connected(FxHashSet::default()); + + assert_eq!(table.peer_completion(), 0.0); + assert_eq!( + table.next_dial_candidate().map(|node| node.node_id()), + Some(node_id), + "the released peer is dialable again" + ); + } + + #[tokio::test] + async fn reconciling_keeps_a_peer_the_consumer_still_holds() { + let mut table = table_with(FixedAnswer(true)); + let (node_id, record) = record_for(22, 1); + table.new_contact_records(vec![record]).await; + table.mark_connected(node_id); + + table.reconcile_connected(FxHashSet::from_iter([node_id])); + + assert_eq!(table.peer_completion(), 0.1); + assert!( + table.next_dial_candidate().is_none(), + "a peer the consumer still holds stays out of the dialer's pool" + ); + } + #[tokio::test] async fn touching_a_reaped_session_does_not_resurrect_it() { // `touch_session` runs for every packet that decrypts, which includes one diff --git a/crates/networking/p2p/discovery/server.rs b/crates/networking/p2p/discovery/server.rs index 5cb4fb1b7fd..d61ebdbd293 100644 --- a/crates/networking/p2p/discovery/server.rs +++ b/crates/networking/p2p/discovery/server.rs @@ -85,6 +85,10 @@ pub trait DiscoveryServerProtocol: Send + Sync { /// Report something that happened to the consumer's relationship with a /// peer. See [`PeerEvent`] for what each variant does. fn record_peer_event(&self, node_id: H256, event: PeerEvent) -> Result<(), ActorError>; + /// Hand discovery the full set of node ids the consumer is connected to, so + /// it can correct a mirror that drifted. See + /// [`ContactTable::reconcile_connected`]. + fn sync_connected_peers(&self, peer_ids: Vec) -> Result<(), ActorError>; fn revalidate_v4(&self) -> Result<(), ActorError>; fn revalidate_v5(&self) -> Result<(), ActorError>; fn lookup_v4(&self) -> Result<(), ActorError>; @@ -137,6 +141,14 @@ impl DiscoveryHandle { } } + /// Tell discovery which peers the consumer is actually connected to. A cast + /// like the rest: discovery corrects itself from it and answers nothing. + pub fn sync_connected_peers(&self, peer_ids: Vec) { + if let Some(server) = self.server() { + let _ = server.sync_connected_peers(peer_ids); + } + } + /// Ask discovery to drop the contacts it has written off, so replacements /// waiting in the k-buckets get promoted. pub fn prune(&self) { @@ -434,6 +446,16 @@ impl DiscoveryServer { self.contacts.record_peer_event(msg.node_id, msg.event); } + #[send_handler] + async fn handle_sync_connected_peers( + &mut self, + msg: discovery_server_protocol::SyncConnectedPeers, + _ctx: &Context, + ) { + self.contacts + .reconcile_connected(msg.peer_ids.into_iter().collect()); + } + #[request_handler] async fn handle_next_dial_candidate( &mut self, diff --git a/crates/networking/p2p/peer_table.rs b/crates/networking/p2p/peer_table.rs index 126903762dc..540853d0bd9 100644 --- a/crates/networking/p2p/peer_table.rs +++ b/crates/networking/p2p/peer_table.rs @@ -175,6 +175,10 @@ pub trait PeerTableServerProtocol: Send + Sync { is_inbound: bool, ) -> Response; fn peer_count(&self) -> Response; + /// The node ids this table currently holds a connection for. Cheap on + /// purpose: discovery mirrors this set and needs it often, and cloning the + /// connections to answer would be wasted work. + fn connected_peer_ids(&self) -> Response>; fn peer_count_by_capabilities(&self, capabilities: Vec) -> Response; fn target_peers_reached(&self) -> Response; fn target_peers_completion(&self) -> Response; @@ -464,6 +468,15 @@ impl PeerTableServer { .collect() } + #[request_handler] + async fn handle_connected_peer_ids( + &mut self, + _msg: peer_table_server_protocol::ConnectedPeerIds, + _ctx: &Context, + ) -> Vec { + self.peers.keys().copied().collect() + } + #[request_handler] async fn handle_get_connected_peers( &mut self, diff --git a/crates/networking/p2p/rlpx/initiator.rs b/crates/networking/p2p/rlpx/initiator.rs index 001be674eab..e58b3b57d69 100644 --- a/crates/networking/p2p/rlpx/initiator.rs +++ b/crates/networking/p2p/rlpx/initiator.rs @@ -22,10 +22,16 @@ pub enum RLPxInitiatorError { #[protocol] pub trait RlpxInitiatorProtocol: Send + Sync { fn look_for_peer(&self) -> Result<(), ActorError>; + fn reconcile_discovery_peers(&self) -> Result<(), ActorError>; fn initiate(&self, node: Node) -> Result<(), ActorError>; fn shutdown(&self) -> Result<(), ActorError>; } +/// How often the peer table's connected set is pushed to discovery. Slow on +/// purpose: this is a correction for a mirror that is right almost all the time, +/// not the mechanism that keeps it current. +const CONNECTED_RECONCILE_INTERVAL: Duration = Duration::from_secs(5); + #[derive(Debug, Clone)] pub struct RLPxInitiator { context: P2PContext, @@ -56,6 +62,7 @@ impl RLPxInitiator { None => state.start(), }; let _ = actor_ref.send(rlpx_initiator_protocol::LookForPeer); + let _ = actor_ref.send(rlpx_initiator_protocol::ReconcileDiscoveryPeers); actor_ref } @@ -86,6 +93,28 @@ impl RLPxInitiator { ); } + /// Push the peer table's connected set to discovery, which mirrors it but + /// cannot read it back: every message across that boundary travels inward. + /// The mirror is event-fed and a connection actor that dies before its + /// teardown hook leaves an id stranded in it, so this is what bounds the + /// damage to one interval. + #[send_handler] + async fn handle_reconcile_discovery_peers( + &mut self, + _msg: rlpx_initiator_protocol::ReconcileDiscoveryPeers, + ctx: &Context, + ) { + match self.context.table.connected_peer_ids().await { + Ok(peer_ids) => self.context.discovery.sync_connected_peers(peer_ids), + Err(e) => debug!(err=?e, "Failed to read connected peers for discovery"), + } + send_after( + CONNECTED_RECONCILE_INTERVAL, + ctx.clone(), + rlpx_initiator_protocol::ReconcileDiscoveryPeers, + ); + } + #[send_handler] async fn handle_initiate( &mut self, From 2919b005695b75ad81777edc48e24eada3a4a5f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Gr=C3=BCner?= <47506558+MegaRedHand@users.noreply.github.com> Date: Fri, 28 Aug 2026 19:03:51 -0300 Subject: [PATCH 11/11] ci(l1): stop counting hive cases that hive abandoned before asking the client The `Invalid Missing Ancestor Syncing ReOrg` family fails a run or two out of every few, one or two cases at a time, never the same ones twice. Every failure carries the same verdict: `Unable to customize payload: no transactions available for modification`. The case wants the payload CLMocker last built so it can corrupt one transaction field; when that payload has no transactions there is nothing to corrupt and hive abandons the case. No `newPayload`, no `forkchoiceUpdated`, nothing asked of the client, nothing said about it. The empty payload is hive's own. The suite starts an in-process geth as its secondary client and CLMocker rotates payload production between that node and the client under test. geth returns an empty block from `buildPayload` immediately and swaps in the full one only when the background build lands, while CLMocker waits a fixed second before fetching. A loaded runner that pushes the first full build past that second yields the empty fixture. In the `CanonicalReOrg=False` variants the client under test is not even in the picture: it is removed from CLMocker before canonical production, the mock is started with no peers, and the ethrex log captured for such a failure holds no engine call for the whole test. main fails these tests too, on the same code that passes them the next run. So the exclusion is on the verdict hive logged for that case, read from the simulation log, and never on the test name: these tests still fail CI for a wrong `INVALID`, a bad `latestValidHash`, or a sync that never finishes. What was ignored is named in the job output rather than counted, because a case that stops being intermittent should be visible rather than buried in a tally. A case records only byte offsets into the simulation log and the verdict line falls outside them, hence the small helper that scans the log per suite. Upstream: #4610, and #3105 before it. --- .github/scripts/check-hive-results.sh | 36 +++++++++- .github/scripts/hive_fixture_failures.py | 87 ++++++++++++++++++++++++ docs/known_issues.md | 38 +++++++++++ 3 files changed, 159 insertions(+), 2 deletions(-) create mode 100644 .github/scripts/hive_fixture_failures.py diff --git a/.github/scripts/check-hive-results.sh b/.github/scripts/check-hive-results.sh index ab84083ade8..c6e6721341c 100755 --- a/.github/scripts/check-hive-results.sh +++ b/.github/scripts/check-hive-results.sh @@ -78,10 +78,19 @@ KNOWN_EXCLUDED_TESTS=( "eth_getTransactionReceipt/get-dynamic-fee" ) +# Verdicts hive writes when it could not build the fixture a case needs. Such a +# case is abandoned before the client under test is asked anything, so counting +# it makes CI red for a defect in the simulator's own setup. Matched against the +# verdict hive logged for that case, never against the test name, so a genuine +# failure of the same test still counts. See docs/known_issues.md. +KNOWN_FIXTURE_FAILURE_SIGNATURES=( + "Unable to customize payload: no transactions available for modification" +) + # Build a jq filter that excludes the known-excluded tests. -exclude_filter='true' +base_exclude_filter='true' for pattern in "${KNOWN_EXCLUDED_TESTS[@]}"; do - exclude_filter="${exclude_filter} and (.name | contains(\"${pattern}\") | not)" + base_exclude_filter="${base_exclude_filter} and (.name | contains(\"${pattern}\") | not)" done for json_file in "${json_files[@]}"; do @@ -90,11 +99,34 @@ for json_file in "${json_files[@]}"; do fi suite_name="$(jq -r '.name // empty' "${json_file}")" + + # Per suite, because the fixture failures have to be read out of that suite's + # simulation log: a case records only byte offsets into it, and the verdict + # line sits outside the range they cover. + exclude_filter="${base_exclude_filter}" + while IFS= read -r ignored_name; do + [ -z "${ignored_name}" ] && continue + exclude_filter="${exclude_filter} and (.name != $(jq -Rn --arg name "${ignored_name}" '$name'))" + done < <( + HIVE_JSON="${json_file}" \ + HIVE_WORKSPACE_LOGS="${workspace_logs_dir}" \ + HIVE_SIGNATURES="$(printf '%s\n' "${KNOWN_FIXTURE_FAILURE_SIGNATURES[@]}")" \ + python3 "$(dirname "$0")/hive_fixture_failures.py" + ) + failed_cases="$(jq '[.testCases[]? | select(.summaryResult.pass != true) | select('"${exclude_filter}"')] | length' "${json_file}")" skipped_excluded="$(jq '[.testCases[]? | select(.summaryResult.pass != true) | select(('"${exclude_filter}"') | not)] | length' "${json_file}")" if [ "${skipped_excluded}" -gt 0 ]; then echo "Ignoring ${skipped_excluded} known-excluded test(s) in ${suite_name:-$(basename "${json_file}")}" + # Named, not just counted. One of these turning up in every run has stopped + # being a flake, and a bare number would bury that. + jq -r ' + .testCases[]? + | select(.summaryResult.pass != true) + | select(('"${exclude_filter}"') | not) + | " - ignored: " + (.name // "unknown test") + ' "${json_file}" fi if [ "${failed_cases}" -gt 0 ]; then diff --git a/.github/scripts/hive_fixture_failures.py b/.github/scripts/hive_fixture_failures.py new file mode 100644 index 00000000000..430d00fff03 --- /dev/null +++ b/.github/scripts/hive_fixture_failures.py @@ -0,0 +1,87 @@ +#!/usr/bin/env python3 +"""Name the hive cases that failed while hive was building its own fixture. + +Some hive cases abandon themselves before the client under test is asked +anything: the simulator could not assemble the payload it intended to corrupt, +so there is no assertion about the client left to pass or fail. Those are +reported like any other failure, and counting them makes CI red for a defect in +the simulator's own setup. + +Such a case is identified by the verdict hive writes to the simulation log, + + FAIL (): + +and never by its name alone, so a genuine failure of the same test still counts. +The case's `summaryResult.log` offsets bracket its RPC traffic rather than this +line, which is why the whole log is scanned for the verdict instead. + +Reads the suite JSON named by HIVE_JSON and the newline-separated signatures in +HIVE_SIGNATURES; writes the matching case names, one per line, to stdout. +""" + +import json +import os +import pathlib +import re +import sys + + +def find_sim_log(json_path: pathlib.Path, rel: str) -> pathlib.Path | None: + """Locate the simulation log, which hive names relative to the results dir.""" + if not rel: + return None + candidates = [ + pathlib.Path(rel), + json_path.parent / rel, + json_path.parent / pathlib.Path(rel).name, + ] + workspace = os.environ.get("HIVE_WORKSPACE_LOGS", "") + if workspace: + candidates.append(pathlib.Path(workspace) / pathlib.Path(rel).name) + for candidate in candidates: + if candidate.is_file(): + return candidate + return None + + +def main() -> int: + json_path = pathlib.Path(os.environ["HIVE_JSON"]) + signatures = [s for s in os.environ.get("HIVE_SIGNATURES", "").splitlines() if s] + if not signatures: + return 0 + + try: + results = json.loads(json_path.read_text(errors="replace")) + except (OSError, ValueError): + # An unreadable result file is the caller's problem to report, not ours; + # excluding nothing keeps every failure counted. + return 0 + + sim_log = find_sim_log(json_path, results.get("simLog") or "") + if sim_log is None: + return 0 + + log = sim_log.read_text(errors="replace") + verdicts: set[str] = set() + for signature in signatures: + pattern = r"^FAIL \((.+?)\): " + re.escape(signature) + verdicts.update(re.findall(pattern, log, re.MULTILINE)) + if not verdicts: + return 0 + + # hive's verdict line drops the fork and client suffixes the result file + # keeps ("... Invalid P9" against "... Invalid P9 (Paris) (ethrex)"), so the + # case name is matched by prefix. + cases = results.get("testCases") or {} + entries = cases.values() if isinstance(cases, dict) else cases + for case in entries: + if (case.get("summaryResult") or {}).get("pass"): + continue + name = case.get("name") or "" + if any(name.startswith(verdict) for verdict in verdicts): + print(name) + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/docs/known_issues.md b/docs/known_issues.md index 2a98d8b871e..25c15bd23ce 100644 --- a/docs/known_issues.md +++ b/docs/known_issues.md @@ -93,3 +93,41 @@ fixed in the tree. Recommended: Until then, the compromise of that key is silent and durable: signatures would still verify against the committed `.github/minisign.pub`. + +--- + +### Hive cases abandoned before the client is asked anything + +**Where:** `KNOWN_FIXTURE_FAILURE_SIGNATURES` in `.github/scripts/check-hive-results.sh`, +resolved per suite by `.github/scripts/hive_fixture_failures.py`. One signature today: +`Unable to customize payload: no transactions available for modification`, which the +`engine-api` suite reports for cases in the `Invalid Missing Ancestor Syncing ReOrg` family. + +**Why:** those cases take the payload CLMocker last built, corrupt one transaction field in it, +and check what the client makes of the result. A payload with no transactions has nothing to +corrupt, so hive abandons the case there (`simulators/ethereum/engine/helper/customizer.go`): +no `newPayload`, no `forkchoiceUpdated`, no statement about the client at all. Counting it puts +CI in the red over the simulator's own setup. + +**The empty payload is hive's own.** The suite starts an in-process geth as the secondary +client (`GethNodeEngineStarter`, `NoDiscovery: true`) and CLMocker rotates payload production +between it and the client under test. geth's `buildPayload` returns an empty block immediately +and replaces it only once the background full build lands; `Resolve` hands back that full block +if it exists and the empty one otherwise (`miner/payload_building.go`, go-ethereum v1.16.7). +CLMocker then waits a fixed second between `forkchoiceUpdated` and `getPayload` +(`clmock.go`, `DefaultPayloadProductionClientDelay`), so a loaded runner that pushes the first +full build past that second produces an empty fixture. + +The client under test need not be involved at all. In the `CanonicalReOrg=False` variants it is +removed from CLMocker before canonical production and the mock is started with no peers, and +the ethrex container log captured for such a failure carries no engine call for the entire +test. Tracked upstream as #4610, and before that #3105. + +**Scope:** the match is against the `FAIL (): ` line hive wrote for that case, +never against the test name, so the same tests still fail CI for any other reason: a wrong +`INVALID` verdict, a bad `latestValidHash`, a sync that never completes. Ignored cases are +named in the job output rather than only counted, so one that starts appearing every run is +visible. + +**Removal:** delete the signature when hive stops fetching payloads on a fixed delay, or when +#4610 is fixed upstream.