From 12f05e74cca4e566841638555a2909721c40fbab Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Tue, 28 Nov 2023 15:16:45 +0100 Subject: [PATCH 1/3] Fix logic mistake in `increase_address_connections` --- lib/src/network/basic_peering_strategy.rs | 89 +++++++++++++++++++---- wasm-node/CHANGELOG.md | 4 + 2 files changed, 78 insertions(+), 15 deletions(-) diff --git a/lib/src/network/basic_peering_strategy.rs b/lib/src/network/basic_peering_strategy.rs index 19724f363e..9b71593b7a 100644 --- a/lib/src/network/basic_peering_strategy.rs +++ b/lib/src/network/basic_peering_strategy.rs @@ -349,12 +349,24 @@ where address: Vec, max_addresses: usize, ) -> InsertAddressResult { - self.insert_address_inner(peer_id, address, max_addresses, 0, false) + let Some(&peer_id_index) = self.peer_ids_indices.get(peer_id) else { + return InsertAddressResult::UnknownPeer; + }; + + match self.insert_address_inner(peer_id_index, address, max_addresses, 0, false) { + InsertAddressConnectionsResult::AlreadyKnown => InsertAddressResult::AlreadyKnown, + InsertAddressConnectionsResult::Inserted { address_removed } => { + InsertAddressResult::Inserted { address_removed } + } + } } /// Increases the number of connections of the given address. If the address isn't known, it /// is inserted. /// + /// Contrary to [`BasicPeeringStrategy::insert_address`], the address is inserted anyway if + /// the `PeerId` isn't known. + /// /// > **Note**: Use this function if you establish a connection and accidentally reach a /// > certain [`PeerId`]. /// @@ -367,22 +379,19 @@ where peer_id: &PeerId, address: Vec, max_addresses: usize, - ) -> InsertAddressResult { - self.insert_address_inner(peer_id, address, max_addresses, 1, true) + ) -> InsertAddressConnectionsResult { + let peer_id_index = self.get_or_insert_peer_index(peer_id); + self.insert_address_inner(peer_id_index, address, max_addresses, 1, true) } fn insert_address_inner( &mut self, - peer_id: &PeerId, + peer_id_index: usize, address: Vec, max_addresses: usize, initial_num_connections: u32, increase_if_present: bool, - ) -> InsertAddressResult { - let Some(&peer_id_index) = self.peer_ids_indices.get(peer_id) else { - return InsertAddressResult::UnknownPeer; - }; - + ) -> InsertAddressConnectionsResult { match self.addresses.entry((peer_id_index, address.clone())) { btree_map::Entry::Vacant(entry) => { entry.insert(initial_num_connections); @@ -410,7 +419,7 @@ where .remove(&(peer_id_index, address_removed.clone())); } - InsertAddressResult::Inserted { address_removed } + InsertAddressConnectionsResult::Inserted { address_removed } } btree_map::Entry::Occupied(entry) => { let entry = entry.into_mut(); @@ -420,7 +429,7 @@ where .unwrap_or_else(|| panic!("overflow in number of connections")); } - InsertAddressResult::AlreadyKnown + InsertAddressConnectionsResult::AlreadyKnown } } } @@ -836,8 +845,7 @@ pub enum InsertChainPeerResult { Duplicate, } -/// See [`BasicPeeringStrategy::insert_address`] and -/// [`BasicPeeringStrategy::increase_address_connections`]. +/// See [`BasicPeeringStrategy::insert_address`]. pub enum InsertAddressResult { /// Address has been successfully inserted. Inserted { @@ -851,6 +859,18 @@ pub enum InsertAddressResult { UnknownPeer, } +/// See [`BasicPeeringStrategy::increase_address_connections`]. +pub enum InsertAddressConnectionsResult { + /// Address has been inserted. + Inserted { + /// If the maximum number of addresses is reached, an old address might have been + /// removed. If so, this contains the address. + address_removed: Option>, + }, + /// Address was already known. + AlreadyKnown, +} + /// See [`BasicPeeringStrategy::unassign_slot_and_ban`]. pub enum UnassignSlotAndBan { /// Peer wasn't assigned to the given chain. @@ -1005,7 +1025,10 @@ where #[cfg(test)] mod tests { - use super::{BasicPeeringStrategy, Config, InsertAddressResult, InsertChainPeerResult}; + use super::{ + BasicPeeringStrategy, Config, InsertAddressConnectionsResult, InsertAddressResult, + InsertChainPeerResult, + }; use crate::network::service::{peer_id::PublicKey, PeerId}; use core::time::Duration; @@ -1066,7 +1089,7 @@ mod tests { assert!(matches!( bps.increase_address_connections(&peer_id, Vec::new(), usize::max_value()), - InsertAddressResult::Inserted { + InsertAddressConnectionsResult::Inserted { address_removed: None } )); @@ -1086,5 +1109,41 @@ mod tests { assert_eq!(bps.peer_addresses(&peer_id).count(), 0); } + #[test] + fn address_not_inserted_when_peer_has_no_chain_association() { + let mut bps = BasicPeeringStrategy::::new(Config { + randomness_seed: [0; 32], + peers_capacity: 0, + chains_capacity: 0, + }); + + let peer_id = PeerId::from_public_key(&PublicKey::Ed25519([0; 32])); + + assert!(matches!( + bps.insert_address(&peer_id, Vec::new(), usize::max_value()), + InsertAddressResult::UnknownPeer + )); + + assert_eq!(bps.peer_addresses(&peer_id).count(), 0); + } + + #[test] + fn address_connections_inserted_when_peer_has_no_chain_association() { + let mut bps = BasicPeeringStrategy::::new(Config { + randomness_seed: [0; 32], + peers_capacity: 0, + chains_capacity: 0, + }); + + let peer_id = PeerId::from_public_key(&PublicKey::Ed25519([0; 32])); + + assert!(matches!( + bps.increase_address_connections(&peer_id, Vec::new(), usize::max_value()), + InsertAddressConnectionsResult::Inserted { .. } + )); + + assert_eq!(bps.peer_addresses(&peer_id).count(), 1); + } + // TODO: more tests } diff --git a/wasm-node/CHANGELOG.md b/wasm-node/CHANGELOG.md index 847d1f6d3f..54219a9db7 100644 --- a/wasm-node/CHANGELOG.md +++ b/wasm-node/CHANGELOG.md @@ -7,6 +7,10 @@ - The order in which smoldot connects to peers is now random rather than depending on the peer. ([#1424](https://github.com/smol-dot/smoldot/pull/1424)) - Increase the rate at which connections are opened to 10 per second, with a pool of 8 simultaneous connections openings. ([#1425](https://github.com/smol-dot/smoldot/pull/1425)) +### Fixed + +- Fix panic when disconnecting from a peer whose identity didn't match the identity that was expected when connecting to it. + ## 2.0.12 - 2023-11-27 ### Fixed From 8c2b5c998f959e721d8bb6ae15d6d5d1c9257fca Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Tue, 28 Nov 2023 15:17:34 +0100 Subject: [PATCH 2/3] PR link --- wasm-node/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/wasm-node/CHANGELOG.md b/wasm-node/CHANGELOG.md index 54219a9db7..ab2c2db20b 100644 --- a/wasm-node/CHANGELOG.md +++ b/wasm-node/CHANGELOG.md @@ -9,7 +9,7 @@ ### Fixed -- Fix panic when disconnecting from a peer whose identity didn't match the identity that was expected when connecting to it. +- Fix panic when disconnecting from a peer whose identity didn't match the identity that was expected when connecting to it. ([#1431](https://github.com/smol-dot/smoldot/pull/1431)) ## 2.0.12 - 2023-11-27 From c254deb92d60ec9894ebcf0e5a4e164177f6a157 Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Tue, 28 Nov 2023 15:20:47 +0100 Subject: [PATCH 3/3] Fix full node no longer compiling --- full-node/src/network_service.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/full-node/src/network_service.rs b/full-node/src/network_service.rs index 53c64a8294..70cfea8c23 100644 --- a/full-node/src/network_service.rs +++ b/full-node/src/network_service.rs @@ -843,7 +843,7 @@ async fn background_task(mut inner: Inner) { remote_addr.as_ref(), ); debug_assert!(_was_in.is_ok()); - if let basic_peering_strategy::InsertAddressResult::Inserted { + if let basic_peering_strategy::InsertAddressConnectionsResult::Inserted { address_removed: Some(addr_rm), } = inner.peering_strategy.increase_address_connections( &peer_id,