From 8e7acde68ea428c527068df6bedda76be8d584d5 Mon Sep 17 00:00:00 2001 From: ljedrz Date: Thu, 13 Mar 2025 17:16:33 +0100 Subject: [PATCH 01/13] fix: ensure that the resolver's maps may only be modified in tandem Signed-off-by: ljedrz --- node/router/src/handshake.rs | 4 ++-- node/router/src/helpers/resolver.rs | 24 ++++++++++-------------- node/router/src/lib.rs | 8 ++++---- 3 files changed, 16 insertions(+), 20 deletions(-) diff --git a/node/router/src/handshake.rs b/node/router/src/handshake.rs index bdcb542711..5d2fe9136b 100644 --- a/node/router/src/handshake.rs +++ b/node/router/src/handshake.rs @@ -211,7 +211,7 @@ impl Router { // Finalize the connecting peer information. self.connecting_peers.lock().insert(peer_ip, Some(Peer::new(peer_ip, peer_addr, &peer_request))); // Adds a bidirectional map between the listener address and (ambiguous) peer address. - self.resolver.insert_peer(peer_ip, peer_addr); + self.resolver.write().insert_peer(peer_ip, peer_addr); Ok((peer_ip, framed)) } @@ -296,7 +296,7 @@ impl Router { // Finalize the connecting peer information. self.connecting_peers.lock().insert(peer_ip, Some(Peer::new(peer_ip, peer_addr, &peer_request))); // Adds a bidirectional map between the listener address and (ambiguous) peer address. - self.resolver.insert_peer(peer_ip, peer_addr); + self.resolver.write().insert_peer(peer_ip, peer_addr); Ok((peer_ip, framed)) } diff --git a/node/router/src/helpers/resolver.rs b/node/router/src/helpers/resolver.rs index 8f9aad3f23..351f449ba9 100644 --- a/node/router/src/helpers/resolver.rs +++ b/node/router/src/helpers/resolver.rs @@ -13,18 +13,14 @@ // See the License for the specific language governing permissions and // limitations under the License. -#[cfg(feature = "locktick")] -use locktick::parking_lot::RwLock; -#[cfg(not(feature = "locktick"))] -use parking_lot::RwLock; use std::{collections::HashMap, net::SocketAddr}; #[derive(Debug)] pub struct Resolver { /// The map of the listener address to (ambiguous) peer address. - from_listener: RwLock>, + from_listener: HashMap, /// The map of the (ambiguous) peer address to listener address. - to_listener: RwLock>, + to_listener: HashMap, } impl Default for Resolver { @@ -42,24 +38,24 @@ impl Resolver { /// Returns the listener address for the given (ambiguous) peer address, if it exists. pub fn get_listener(&self, peer_addr: &SocketAddr) -> Option { - self.to_listener.read().get(peer_addr).copied() + self.to_listener.get(peer_addr).copied() } /// Returns the (ambiguous) peer address for the given listener address, if it exists. pub fn get_ambiguous(&self, peer_ip: &SocketAddr) -> Option { - self.from_listener.read().get(peer_ip).copied() + self.from_listener.get(peer_ip).copied() } /// Inserts a bidirectional mapping of the listener address and the (ambiguous) peer address. - pub fn insert_peer(&self, listener_ip: SocketAddr, peer_addr: SocketAddr) { - self.from_listener.write().insert(listener_ip, peer_addr); - self.to_listener.write().insert(peer_addr, listener_ip); + pub fn insert_peer(&mut self, listener_ip: SocketAddr, peer_addr: SocketAddr) { + self.from_listener.insert(listener_ip, peer_addr); + self.to_listener.insert(peer_addr, listener_ip); } /// Removes the bidirectional mapping of the listener address and the (ambiguous) peer address. - pub fn remove_peer(&self, listener_ip: &SocketAddr) { - if let Some(peer_addr) = self.from_listener.write().remove(listener_ip) { - self.to_listener.write().remove(&peer_addr); + pub fn remove_peer(&mut self, listener_ip: &SocketAddr) { + if let Some(peer_addr) = self.from_listener.remove(listener_ip) { + self.to_listener.remove(&peer_addr); } } } diff --git a/node/router/src/lib.rs b/node/router/src/lib.rs index 2dcd7ba6be..e445d689e6 100644 --- a/node/router/src/lib.rs +++ b/node/router/src/lib.rs @@ -96,7 +96,7 @@ pub struct InnerRouter { /// The cache. cache: Cache, /// The resolver. - resolver: Resolver, + resolver: RwLock, /// The set of trusted peers. trusted_peers: HashSet, /// The map of connected peer IPs to their peer handlers. @@ -337,12 +337,12 @@ impl Router { /// Returns the listener IP address from the (ambiguous) peer address. pub fn resolve_to_listener(&self, peer_addr: &SocketAddr) -> Option { - self.resolver.get_listener(peer_addr) + self.resolver.read().get_listener(peer_addr) } /// Returns the (ambiguous) peer address from the listener IP address. pub fn resolve_to_ambiguous(&self, peer_ip: &SocketAddr) -> Option { - self.resolver.get_ambiguous(peer_ip) + self.resolver.read().get_ambiguous(peer_ip) } /// Returns `true` if the node is connected to the given peer IP. @@ -610,7 +610,7 @@ impl Router { /// Removes the connected peer and adds them to the candidate peers. pub fn remove_connected_peer(&self, peer_ip: SocketAddr) { // Removes the bidirectional map between the listener address and (ambiguous) peer address. - self.resolver.remove_peer(&peer_ip); + self.resolver.write().remove_peer(&peer_ip); // Remove this peer from the connected peers, if it exists. self.connected_peers.write().remove(&peer_ip); // Add the peer to the candidate peers. From e3fd45f1d157213025925289a6c59d21f6340c39 Mon Sep 17 00:00:00 2001 From: ljedrz Date: Thu, 13 Mar 2025 18:38:38 +0100 Subject: [PATCH 02/13] logs: no longer WARN if queued messages can no longer be attributed to a peer Signed-off-by: ljedrz --- node/router/src/inbound.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/node/router/src/inbound.rs b/node/router/src/inbound.rs index 1433928eb2..7c934314d2 100644 --- a/node/router/src/inbound.rs +++ b/node/router/src/inbound.rs @@ -78,7 +78,10 @@ pub trait Inbound: Reading + Outbound { // Retrieve the listener IP for the peer. let peer_ip = match self.router().resolve_to_listener(&peer_addr) { Some(peer_ip) => peer_ip, - None => bail!("Unable to resolve the (ambiguous) peer address '{peer_addr}'"), + None => { + // No longer connected to the peer. + return Ok(()); + } }; // Drop the peer, if they have sent more than `MESSAGE_LIMIT` messages From 76b21d851e4e3c3a3b8471e67d87e43edee6ceac Mon Sep 17 00:00:00 2001 From: ljedrz Date: Thu, 13 Mar 2025 18:39:03 +0100 Subject: [PATCH 03/13] fix: remove connected peer entries before the resolver ones Signed-off-by: ljedrz --- node/router/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/node/router/src/lib.rs b/node/router/src/lib.rs index e445d689e6..a02223d565 100644 --- a/node/router/src/lib.rs +++ b/node/router/src/lib.rs @@ -609,10 +609,10 @@ impl Router { /// Removes the connected peer and adds them to the candidate peers. pub fn remove_connected_peer(&self, peer_ip: SocketAddr) { - // Removes the bidirectional map between the listener address and (ambiguous) peer address. - self.resolver.write().remove_peer(&peer_ip); // Remove this peer from the connected peers, if it exists. self.connected_peers.write().remove(&peer_ip); + // Removes the bidirectional map between the listener address and (ambiguous) peer address. + self.resolver.write().remove_peer(&peer_ip); // Add the peer to the candidate peers. self.candidate_peers.write().insert(peer_ip); // Clear cached entries applicable to the peer. From 66143ed80af1eb6bf9634cff7a59f19db8ef1170 Mon Sep 17 00:00:00 2001 From: ljedrz Date: Fri, 14 Mar 2025 10:37:36 +0100 Subject: [PATCH 04/13] fix: clear peer cache entries before re-registering it as a candidate peer Signed-off-by: ljedrz --- node/router/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/node/router/src/lib.rs b/node/router/src/lib.rs index a02223d565..95f4e19bf3 100644 --- a/node/router/src/lib.rs +++ b/node/router/src/lib.rs @@ -613,10 +613,10 @@ impl Router { self.connected_peers.write().remove(&peer_ip); // Removes the bidirectional map between the listener address and (ambiguous) peer address. self.resolver.write().remove_peer(&peer_ip); - // Add the peer to the candidate peers. - self.candidate_peers.write().insert(peer_ip); // Clear cached entries applicable to the peer. self.cache.clear_peer_entries(peer_ip); + // Add the peer to the candidate peers. + self.candidate_peers.write().insert(peer_ip); #[cfg(feature = "metrics")] self.update_metrics(); } From 1fa44d26cd2f8a685ddb10d7deab0217574be23a Mon Sep 17 00:00:00 2001 From: ljedrz Date: Fri, 14 Mar 2025 11:32:14 +0100 Subject: [PATCH 05/13] fix: don't report protocol violation when a peer performs a clean disconnect Signed-off-by: ljedrz --- node/router/src/inbound.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/node/router/src/inbound.rs b/node/router/src/inbound.rs index 7c934314d2..a013aac843 100644 --- a/node/router/src/inbound.rs +++ b/node/router/src/inbound.rs @@ -158,7 +158,10 @@ pub trait Inbound: Reading + Outbound { bail!("Peer '{peer_ip}' is not following the protocol") } Message::Disconnect(message) => { - bail!("{:?}", message.reason) + // The peer informs us that they had disconnected. Disconnect from them too. + debug!("Peer '{peer_ip}' decided to disconnect due to '{:?}'", message.reason); + self.router().disconnect(peer_ip); + Ok(()) } Message::PeerRequest(..) => match self.peer_request(peer_ip) { true => Ok(()), From 5d71d2ea8a719f55a14bb415d06dca5ec3b3666f Mon Sep 17 00:00:00 2001 From: ljedrz Date: Fri, 14 Mar 2025 11:51:08 +0100 Subject: [PATCH 06/13] fix: perform a fallback connection artifact cleanup if necessary Signed-off-by: ljedrz --- node/router/src/lib.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/node/router/src/lib.rs b/node/router/src/lib.rs index 95f4e19bf3..a715bf781a 100644 --- a/node/router/src/lib.rs +++ b/node/router/src/lib.rs @@ -261,6 +261,12 @@ impl Router { } disconnected } else { + // FIXME (ljedrz): this shouldn't be necessary; it's a double-check + // that the higher-level collection is consistent with the resolver. + if router.is_connected(&peer_ip) { + warn!("Fallback connection artifact cleanup (report this to @ljedrz)"); + router.remove_connected_peer(peer_ip); + } false } }) From 135130f9f340b859ca0abb6194e9c84c4aae45a6 Mon Sep 17 00:00:00 2001 From: ljedrz Date: Tue, 1 Jul 2025 09:43:33 +0200 Subject: [PATCH 07/13] api: make the return value of Inbound::inbound more meaningful Signed-off-by: ljedrz --- node/router/src/inbound.rs | 38 ++++++++++++++++++++------------------ 1 file changed, 20 insertions(+), 18 deletions(-) diff --git a/node/router/src/inbound.rs b/node/router/src/inbound.rs index a013aac843..2c8af183d3 100644 --- a/node/router/src/inbound.rs +++ b/node/router/src/inbound.rs @@ -73,14 +73,16 @@ pub trait Inbound: Reading + Outbound { } } - /// Handles the inbound message from the peer. - async fn inbound(&self, peer_addr: SocketAddr, message: Message) -> Result<()> { + /// Handles the inbound message from the peer. The returned value indicates whether + /// the connection is still active, and errors causing a disconnect once they are + /// propagated to the caller. + async fn inbound(&self, peer_addr: SocketAddr, message: Message) -> Result { // Retrieve the listener IP for the peer. let peer_ip = match self.router().resolve_to_listener(&peer_addr) { Some(peer_ip) => peer_ip, None => { // No longer connected to the peer. - return Ok(()); + return Ok(false); } }; @@ -118,7 +120,7 @@ pub trait Inbound: Reading + Outbound { let node = self.clone(); match spawn_blocking(move || node.block_request(peer_ip, message)).await? { - true => Ok(()), + true => Ok(true), false => bail!("Peer '{peer_ip}' sent an invalid block request"), } } @@ -149,7 +151,7 @@ pub trait Inbound: Reading + Outbound { // Process the block response. let node = self.clone(); match spawn_blocking(move || node.block_response(peer_ip, blocks.0)).await? { - true => Ok(()), + true => Ok(true), false => bail!("Peer '{peer_ip}' sent an invalid block response"), } } @@ -161,10 +163,10 @@ pub trait Inbound: Reading + Outbound { // The peer informs us that they had disconnected. Disconnect from them too. debug!("Peer '{peer_ip}' decided to disconnect due to '{:?}'", message.reason); self.router().disconnect(peer_ip); - Ok(()) + Ok(false) } Message::PeerRequest(..) => match self.peer_request(peer_ip) { - true => Ok(()), + true => Ok(true), false => bail!("Peer '{peer_ip}' sent an invalid peer request"), }, Message::PeerResponse(message) => { @@ -177,7 +179,7 @@ pub trait Inbound: Reading + Outbound { } match self.peer_response(peer_ip, &message.peers) { - true => Ok(()), + true => Ok(true), false => bail!("Peer '{peer_ip}' sent an invalid peer response"), } } @@ -211,12 +213,12 @@ pub trait Inbound: Reading + Outbound { // Process the ping message. match self.ping(peer_ip, message) { - true => Ok(()), + true => Ok(true), false => bail!("Peer '{peer_ip}' sent an invalid ping"), } } Message::Pong(message) => match self.pong(peer_ip, message) { - true => Ok(()), + true => Ok(true), false => bail!("Peer '{peer_ip}' sent an invalid pong"), }, Message::PuzzleRequest(..) => { @@ -228,7 +230,7 @@ pub trait Inbound: Reading + Outbound { } // Process the puzzle request. match self.puzzle_request(peer_ip) { - true => Ok(()), + true => Ok(true), false => bail!("Peer '{peer_ip}' sent an invalid puzzle request"), } } @@ -247,7 +249,7 @@ pub trait Inbound: Reading + Outbound { }; // Process the puzzle response. match self.puzzle_response(peer_ip, message.epoch_hash, header) { - true => Ok(()), + true => Ok(true), false => bail!("Peer '{peer_ip}' sent an invalid puzzle response"), } } @@ -255,7 +257,7 @@ pub trait Inbound: Reading + Outbound { // Do not process unconfirmed solutions if the node is too far behind. if !self.is_within_sync_leniency() { trace!("Skipped processing unconfirmed solution '{}' (node is syncing)", message.solution_id); - return Ok(()); + return Ok(true); } // Update the timestamp for the unconfirmed solution. @@ -263,7 +265,7 @@ pub trait Inbound: Reading + Outbound { // Determine whether to propagate the solution. if seen_before { trace!("Skipping 'UnconfirmedSolution' from '{peer_ip}'"); - return Ok(()); + return Ok(true); } // Clone the serialized message. let serialized = message.clone(); @@ -278,7 +280,7 @@ pub trait Inbound: Reading + Outbound { } // Handle the unconfirmed solution. match self.unconfirmed_solution(peer_ip, serialized, solution).await { - true => Ok(()), + true => Ok(true), false => bail!("Peer '{peer_ip}' sent an invalid unconfirmed solution"), } } @@ -286,7 +288,7 @@ pub trait Inbound: Reading + Outbound { // Do not process unconfirmed solutions if the node is too far behind. if !self.is_within_sync_leniency() { trace!("Skipped processing unconfirmed transaction '{}' (node is syncing)", message.transaction_id); - return Ok(()); + return Ok(true); } // Update the timestamp for the unconfirmed transaction. let seen_before = @@ -294,7 +296,7 @@ pub trait Inbound: Reading + Outbound { // Determine whether to propagate the transaction. if seen_before { trace!("Skipping 'UnconfirmedTransaction' from '{peer_ip}'"); - return Ok(()); + return Ok(true); } // Clone the serialized message. let serialized = message.clone(); @@ -309,7 +311,7 @@ pub trait Inbound: Reading + Outbound { } // Handle the unconfirmed transaction. match self.unconfirmed_transaction(peer_ip, serialized, transaction).await { - true => Ok(()), + true => Ok(true), false => bail!("Peer '{peer_ip}' sent an invalid unconfirmed transaction"), } } From c3bf307ba7466fd08c317c00aa060a86e933f779 Mon Sep 17 00:00:00 2001 From: ljedrz Date: Tue, 1 Jul 2025 09:46:57 +0200 Subject: [PATCH 08/13] logs: add a TRACE log whenever an inbound message is dropped Signed-off-by: ljedrz --- node/router/src/inbound.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/node/router/src/inbound.rs b/node/router/src/inbound.rs index 2c8af183d3..0f9fae8ea9 100644 --- a/node/router/src/inbound.rs +++ b/node/router/src/inbound.rs @@ -82,6 +82,7 @@ pub trait Inbound: Reading + Outbound { Some(peer_ip) => peer_ip, None => { // No longer connected to the peer. + trace!("Dropping a {} from {peer_addr} - no longer connected.", message.name()); return Ok(false); } }; From 3a5ceed7edb199c7ba7e4e33123756c35ac916b0 Mon Sep 17 00:00:00 2001 From: ljedrz Date: Tue, 1 Jul 2025 10:07:20 +0200 Subject: [PATCH 09/13] fix: hold the write guards for longer when inserting/removing connected peers Signed-off-by: ljedrz --- node/router/src/lib.rs | 31 ++++++++++++++++++++++--------- 1 file changed, 22 insertions(+), 9 deletions(-) diff --git a/node/router/src/lib.rs b/node/router/src/lib.rs index a715bf781a..c5ee122210 100644 --- a/node/router/src/lib.rs +++ b/node/router/src/lib.rs @@ -533,6 +533,12 @@ impl Router { /// Inserts the given peer into the connected peers. pub fn insert_connected_peer(&self, peer_ip: SocketAddr) { + // Hold the write locks for the duration of this function to + // avoid any of them from becoming unaligned with the others. + let mut connected_peers = self.connected_peers.write(); + let mut candidate_peers = self.candidate_peers.write(); + let mut restricted_peers = self.restricted_peers.write(); + // Move the peer from "connecting" to "connected". let peer = match self.connecting_peers.lock().remove(&peer_ip) { Some(Some(peer)) => peer, @@ -545,12 +551,13 @@ impl Router { return; } }; - // Add an entry for this `Peer` in the connected peers. - self.connected_peers.write().insert(peer_ip, peer); + // Remove this peer from the candidate peers, if it exists. - self.candidate_peers.write().remove(&peer_ip); + candidate_peers.remove(&peer_ip); + // Add an entry for this `Peer` in the connected peers. + connected_peers.insert(peer_ip, peer); // Remove this peer from the restricted peers, if it exists. - self.restricted_peers.write().remove(&peer_ip); + restricted_peers.remove(&peer_ip); #[cfg(feature = "metrics")] self.update_metrics(); info!("Connected to '{peer_ip}'"); @@ -615,14 +622,20 @@ impl Router { /// Removes the connected peer and adds them to the candidate peers. pub fn remove_connected_peer(&self, peer_ip: SocketAddr) { - // Remove this peer from the connected peers, if it exists. - self.connected_peers.write().remove(&peer_ip); + // Hold the write locks for the duration of this function to + // avoid any of them from becoming unaligned with the others. + let mut resolver = self.resolver.write(); + let mut connected_peers = self.connected_peers.write(); + let mut candidate_peers = self.candidate_peers.write(); + // Removes the bidirectional map between the listener address and (ambiguous) peer address. - self.resolver.write().remove_peer(&peer_ip); + resolver.remove_peer(&peer_ip); + // Remove this peer from the connected peers, if it exists. + connected_peers.remove(&peer_ip); + // Add the peer to the candidate peers. + candidate_peers.insert(peer_ip); // Clear cached entries applicable to the peer. self.cache.clear_peer_entries(peer_ip); - // Add the peer to the candidate peers. - self.candidate_peers.write().insert(peer_ip); #[cfg(feature = "metrics")] self.update_metrics(); } From 0aaac3f32cbe5b76f178c92346af81d9422e10d3 Mon Sep 17 00:00:00 2001 From: ljedrz Date: Tue, 1 Jul 2025 10:12:46 +0200 Subject: [PATCH 10/13] cleanup: remove Resolver::new (in favor of Default::default) Signed-off-by: ljedrz --- node/router/src/helpers/resolver.rs | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-) diff --git a/node/router/src/helpers/resolver.rs b/node/router/src/helpers/resolver.rs index 351f449ba9..592f565030 100644 --- a/node/router/src/helpers/resolver.rs +++ b/node/router/src/helpers/resolver.rs @@ -15,7 +15,7 @@ use std::{collections::HashMap, net::SocketAddr}; -#[derive(Debug)] +#[derive(Debug, Default)] pub struct Resolver { /// The map of the listener address to (ambiguous) peer address. from_listener: HashMap, @@ -23,19 +23,7 @@ pub struct Resolver { to_listener: HashMap, } -impl Default for Resolver { - /// Initializes a new instance of the resolver. - fn default() -> Self { - Self::new() - } -} - impl Resolver { - /// Initializes a new instance of the resolver. - pub fn new() -> Self { - Self { from_listener: Default::default(), to_listener: Default::default() } - } - /// Returns the listener address for the given (ambiguous) peer address, if it exists. pub fn get_listener(&self, peer_addr: &SocketAddr) -> Option { self.to_listener.get(peer_addr).copied() From e80c823a8854a6e1d03fc22d8cd2133ef6170900 Mon Sep 17 00:00:00 2001 From: ljedrz Date: Tue, 1 Jul 2025 10:19:32 +0200 Subject: [PATCH 11/13] docs: add a doc comment for the Resolver Signed-off-by: ljedrz --- node/router/src/helpers/resolver.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/node/router/src/helpers/resolver.rs b/node/router/src/helpers/resolver.rs index 592f565030..c04b582c5e 100644 --- a/node/router/src/helpers/resolver.rs +++ b/node/router/src/helpers/resolver.rs @@ -15,6 +15,9 @@ use std::{collections::HashMap, net::SocketAddr}; +/// The `Resolver` provides the means to map the connected address (used in the lower-level +/// `tcp` module internals, and provided by the OS) to the listener address (used as the +/// unique peer identifier in the higher-level functions), and the other way around. #[derive(Debug, Default)] pub struct Resolver { /// The map of the listener address to (ambiguous) peer address. From 67b9d81a1df0c06bfc4e43d0dbd7d908718fddb9 Mon Sep 17 00:00:00 2001 From: ljedrz Date: Tue, 1 Jul 2025 10:20:47 +0200 Subject: [PATCH 12/13] api: reduce the visibility of the Resolver Signed-off-by: ljedrz --- node/router/src/helpers/mod.rs | 2 +- node/router/src/helpers/resolver.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/node/router/src/helpers/mod.rs b/node/router/src/helpers/mod.rs index 607f9a0519..915d3212de 100644 --- a/node/router/src/helpers/mod.rs +++ b/node/router/src/helpers/mod.rs @@ -20,4 +20,4 @@ mod peer; pub use peer::*; mod resolver; -pub use resolver::*; +pub(crate) use resolver::*; diff --git a/node/router/src/helpers/resolver.rs b/node/router/src/helpers/resolver.rs index c04b582c5e..d6057a8f7f 100644 --- a/node/router/src/helpers/resolver.rs +++ b/node/router/src/helpers/resolver.rs @@ -19,7 +19,7 @@ use std::{collections::HashMap, net::SocketAddr}; /// `tcp` module internals, and provided by the OS) to the listener address (used as the /// unique peer identifier in the higher-level functions), and the other way around. #[derive(Debug, Default)] -pub struct Resolver { +pub(crate) struct Resolver { /// The map of the listener address to (ambiguous) peer address. from_listener: HashMap, /// The map of the (ambiguous) peer address to listener address. From 4c1799cbf206c602c786ed479340907f996098a6 Mon Sep 17 00:00:00 2001 From: ljedrz Date: Tue, 1 Jul 2025 13:08:47 +0200 Subject: [PATCH 13/13] Revert "fix: hold the write guards for longer when inserting/removing connected peers" This reverts commit 3a5ceed7edb199c7ba7e4e33123756c35ac916b0. --- node/router/src/lib.rs | 31 +++++++++---------------------- 1 file changed, 9 insertions(+), 22 deletions(-) diff --git a/node/router/src/lib.rs b/node/router/src/lib.rs index c5ee122210..a715bf781a 100644 --- a/node/router/src/lib.rs +++ b/node/router/src/lib.rs @@ -533,12 +533,6 @@ impl Router { /// Inserts the given peer into the connected peers. pub fn insert_connected_peer(&self, peer_ip: SocketAddr) { - // Hold the write locks for the duration of this function to - // avoid any of them from becoming unaligned with the others. - let mut connected_peers = self.connected_peers.write(); - let mut candidate_peers = self.candidate_peers.write(); - let mut restricted_peers = self.restricted_peers.write(); - // Move the peer from "connecting" to "connected". let peer = match self.connecting_peers.lock().remove(&peer_ip) { Some(Some(peer)) => peer, @@ -551,13 +545,12 @@ impl Router { return; } }; - - // Remove this peer from the candidate peers, if it exists. - candidate_peers.remove(&peer_ip); // Add an entry for this `Peer` in the connected peers. - connected_peers.insert(peer_ip, peer); + self.connected_peers.write().insert(peer_ip, peer); + // Remove this peer from the candidate peers, if it exists. + self.candidate_peers.write().remove(&peer_ip); // Remove this peer from the restricted peers, if it exists. - restricted_peers.remove(&peer_ip); + self.restricted_peers.write().remove(&peer_ip); #[cfg(feature = "metrics")] self.update_metrics(); info!("Connected to '{peer_ip}'"); @@ -622,20 +615,14 @@ impl Router { /// Removes the connected peer and adds them to the candidate peers. pub fn remove_connected_peer(&self, peer_ip: SocketAddr) { - // Hold the write locks for the duration of this function to - // avoid any of them from becoming unaligned with the others. - let mut resolver = self.resolver.write(); - let mut connected_peers = self.connected_peers.write(); - let mut candidate_peers = self.candidate_peers.write(); - - // Removes the bidirectional map between the listener address and (ambiguous) peer address. - resolver.remove_peer(&peer_ip); // Remove this peer from the connected peers, if it exists. - connected_peers.remove(&peer_ip); - // Add the peer to the candidate peers. - candidate_peers.insert(peer_ip); + self.connected_peers.write().remove(&peer_ip); + // Removes the bidirectional map between the listener address and (ambiguous) peer address. + self.resolver.write().remove_peer(&peer_ip); // Clear cached entries applicable to the peer. self.cache.clear_peer_entries(peer_ip); + // Add the peer to the candidate peers. + self.candidate_peers.write().insert(peer_ip); #[cfg(feature = "metrics")] self.update_metrics(); }