From 22d6b481a623b063c18a6f80c7481a37ee3b4d37 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 17 Sep 2024 19:37:12 +0300 Subject: [PATCH 01/66] manager/address: Fix typo Signed-off-by: Alexandru Vasile --- src/transport/manager/address.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/transport/manager/address.rs b/src/transport/manager/address.rs index e18d7c05..21e7f2ee 100644 --- a/src/transport/manager/address.rs +++ b/src/transport/manager/address.rs @@ -196,7 +196,7 @@ impl AddressStore { self.by_score.is_empty() } - /// Check if address is already in the a + /// Check if address is already in the address store. pub fn contains(&self, address: &Multiaddr) -> bool { self.by_address.contains(address) } From 86e017f3c7ce9b948a38812a4c7066c72407bfdc Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Wed, 18 Sep 2024 17:10:38 +0300 Subject: [PATCH 02/66] manager/address: Store addresses using a HashMap Signed-off-by: Alexandru Vasile --- src/transport/manager/address.rs | 70 +++++++++++++------------------- src/transport/manager/mod.rs | 5 ++- 2 files changed, 32 insertions(+), 43 deletions(-) diff --git a/src/transport/manager/address.rs b/src/transport/manager/address.rs index 21e7f2ee..f5ace187 100644 --- a/src/transport/manager/address.rs +++ b/src/transport/manager/address.rs @@ -20,10 +20,13 @@ use crate::{types::ConnectionId, PeerId}; +use std::collections::HashMap; + use multiaddr::{Multiaddr, Protocol}; use multihash::Multihash; -use std::collections::{BinaryHeap, HashSet}; +/// Maximum number of addresses tracked for a peer. +const MAX_ADDRESSES: usize = 32; #[allow(clippy::derived_hash_with_manual_eq)] #[derive(Debug, Clone, Hash)] @@ -134,19 +137,16 @@ impl Ord for AddressRecord { /// Store for peer addresses. #[derive(Debug)] pub struct AddressStore { - //// Addresses sorted by score. - pub by_score: BinaryHeap, - - /// Addresses queryable by hashing them for faster lookup. - pub by_address: HashSet, + /// Addresses available. + pub addresses: HashMap, } impl FromIterator for AddressStore { fn from_iter>(iter: T) -> Self { let mut store = AddressStore::new(); for address in iter { - if let Some(address) = AddressRecord::from_multiaddr(address) { - store.insert(address); + if let Some(record) = AddressRecord::from_multiaddr(address) { + store.insert(record); } } @@ -158,8 +158,7 @@ impl FromIterator for AddressStore { fn from_iter>(iter: T) -> Self { let mut store = AddressStore::new(); for record in iter { - store.by_address.insert(record.address.clone()); - store.by_score.push(record); + store.insert(record); } store @@ -186,51 +185,40 @@ impl AddressStore { /// Create new [`AddressStore`]. pub fn new() -> Self { Self { - by_score: BinaryHeap::new(), - by_address: HashSet::new(), + addresses: HashMap::with_capacity(MAX_ADDRESSES), } } /// Check if [`AddressStore`] is empty. pub fn is_empty(&self) -> bool { - self.by_score.is_empty() + self.addresses.is_empty() } /// Check if address is already in the address store. pub fn contains(&self, address: &Multiaddr) -> bool { - self.by_address.contains(address) + self.addresses.contains_key(address) } - /// Insert new address record into [`AddressStore`] with default address score. - pub fn insert(&mut self, mut record: AddressRecord) { - if self.by_address.contains(record.address()) { - return; - } - - record.connection_id = None; - self.by_address.insert(record.address.clone()); - self.by_score.push(record); - } - - /// Pop address with the highest score from [`AddressStore`]. - pub fn pop(&mut self) -> Option { - self.by_score.pop().map(|record| { - self.by_address.remove(&record.address); - record - }) - } - - /// Take at most `limit` `AddressRecord`s from [`AddressStore`]. - pub fn take(&mut self, limit: usize) -> Vec { - let mut records = Vec::new(); - - for _ in 0..limit { - match self.pop() { - Some(record) => records.push(record), - None => break, + /// Update the address record into [`AddressStore`] with the provided score. + /// + /// If the address is not in the store, it will be inserted. + pub fn insert(&mut self, record: AddressRecord) { + match self.addresses.entry(record.address.clone()) { + std::collections::hash_map::Entry::Occupied(occupied_entry) => { + let found_record = occupied_entry.into_mut(); + found_record.update_score(record.score); + found_record.connection_id = record.connection_id; + } + std::collections::hash_map::Entry::Vacant(vacant_entry) => { + vacant_entry.insert(record.clone()); } } + } + /// Return the available addresses sorted by score. + pub fn addresses(&self) -> Vec { + let mut records = self.addresses.values().cloned().collect::>(); + records.sort_by(|lhs, rhs| rhs.score.cmp(&lhs.score)); records } } diff --git a/src/transport/manager/mod.rs b/src/transport/manager/mod.rs index 6816bbf2..a09777c9 100644 --- a/src/transport/manager/mod.rs +++ b/src/transport/manager/mod.rs @@ -435,7 +435,7 @@ impl TransportManager { let PeerContext { state, secondary_connection, - mut addresses, + addresses, } = match peers.remove(&peer) { None => return Err(Error::PeerDoesntExist(peer)), Some( @@ -482,8 +482,9 @@ impl TransportManager { } let mut records: HashMap<_, _> = addresses - .take(limit) + .addresses() .into_iter() + .take(limit) .map(|record| (record.address().clone(), record)) .collect(); From 8ec9e54da7eab1acb21e2f5b1e23928bbbe215cd Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Wed, 18 Sep 2024 17:12:37 +0300 Subject: [PATCH 03/66] manager: Keep dial addresses around even if the peer is connected Signed-off-by: Alexandru Vasile --- src/transport/manager/mod.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/transport/manager/mod.rs b/src/transport/manager/mod.rs index a09777c9..f62a00bd 100644 --- a/src/transport/manager/mod.rs +++ b/src/transport/manager/mod.rs @@ -667,10 +667,8 @@ impl TransportManager { Entry::Occupied(occupied) => { let context = occupied.into_mut(); - // For a better address tacking, see: - // https://github.com/paritytech/litep2p/issues/180 - // - // TODO: context.addresses.insert(record.clone()); + // Keep the provided record around for possible future dials. + context.addresses.insert(record.clone()); tracing::debug!( target: LOG_TARGET, From 83b9361cf9fb8458fc5e8facb5a52f99269f5305 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Wed, 18 Sep 2024 17:16:52 +0300 Subject: [PATCH 04/66] manager: Update dial failure address scores Signed-off-by: Alexandru Vasile --- src/transport/manager/address.rs | 7 +++++++ src/transport/manager/mod.rs | 13 ++++--------- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/src/transport/manager/address.rs b/src/transport/manager/address.rs index f5ace187..eeec795d 100644 --- a/src/transport/manager/address.rs +++ b/src/transport/manager/address.rs @@ -215,6 +215,13 @@ impl AddressStore { } } + /// Update the score of an existing address. + pub fn update(&mut self, address: &Multiaddr, score: i32) { + if let Some(record) = self.addresses.get_mut(address) { + record.update_score(score); + } + } + /// Return the available addresses sorted by score. pub fn addresses(&self) -> Vec { let mut records = self.addresses.values().cloned().collect::>(); diff --git a/src/transport/manager/mod.rs b/src/transport/manager/mod.rs index f62a00bd..1dd720c5 100644 --- a/src/transport/manager/mod.rs +++ b/src/transport/manager/mod.rs @@ -769,9 +769,7 @@ impl TransportManager { return Ok(()); } - record.update_score(SCORE_CONNECT_FAILURE); - context.addresses.insert(record.clone()); - + context.addresses.update(record.address(), SCORE_CONNECT_FAILURE); context.state = PeerState::Disconnected { dial_record: None }; Ok(()) } @@ -780,7 +778,7 @@ impl TransportManager { } PeerState::Connected { record, - dial_record: Some(mut dial_record), + dial_record: Some(dial_record), } => { if dial_record.connection_id() != &Some(connection_id) { tracing::warn!( @@ -799,9 +797,7 @@ impl TransportManager { return Ok(()); } - dial_record.update_score(SCORE_CONNECT_FAILURE); - context.addresses.insert(dial_record); - + context.addresses.update(record.address(), SCORE_CONNECT_FAILURE); context.state = PeerState::Connected { record, dial_record: None, @@ -834,8 +830,7 @@ impl TransportManager { return Ok(()); } - dial_record.update_score(SCORE_CONNECT_FAILURE); - context.addresses.insert(dial_record); + context.addresses.update(dial_record.address(), SCORE_CONNECT_FAILURE); Ok(()) } From b28a234898e4ab665b30703ca8a325be235715e7 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Wed, 18 Sep 2024 17:18:37 +0300 Subject: [PATCH 05/66] manager: Clean up the disconnected dial record on failure Signed-off-by: Alexandru Vasile --- src/transport/manager/mod.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/transport/manager/mod.rs b/src/transport/manager/mod.rs index 1dd720c5..2bc6474c 100644 --- a/src/transport/manager/mod.rs +++ b/src/transport/manager/mod.rs @@ -831,6 +831,7 @@ impl TransportManager { } context.addresses.update(dial_record.address(), SCORE_CONNECT_FAILURE); + context.state = PeerState::Disconnected { dial_record: None }; Ok(()) } From 68e33288f53153f64a54e2986e30b3f57f47dfa7 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Thu, 19 Sep 2024 11:40:08 +0300 Subject: [PATCH 06/66] manager: Bump addresses to 64 Signed-off-by: Alexandru Vasile --- src/transport/manager/address.rs | 4 ++-- src/transport/manager/mod.rs | 5 +---- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/src/transport/manager/address.rs b/src/transport/manager/address.rs index eeec795d..db18d77e 100644 --- a/src/transport/manager/address.rs +++ b/src/transport/manager/address.rs @@ -26,7 +26,7 @@ use multiaddr::{Multiaddr, Protocol}; use multihash::Multihash; /// Maximum number of addresses tracked for a peer. -const MAX_ADDRESSES: usize = 32; +const MAX_ADDRESSES: usize = 64; #[allow(clippy::derived_hash_with_manual_eq)] #[derive(Debug, Clone, Hash)] @@ -216,7 +216,7 @@ impl AddressStore { } /// Update the score of an existing address. - pub fn update(&mut self, address: &Multiaddr, score: i32) { + pub fn update_score(&mut self, address: &Multiaddr, score: i32) { if let Some(record) = self.addresses.get_mut(address) { record.update_score(score); } diff --git a/src/transport/manager/mod.rs b/src/transport/manager/mod.rs index 2bc6474c..2fa8a702 100644 --- a/src/transport/manager/mod.rs +++ b/src/transport/manager/mod.rs @@ -805,7 +805,7 @@ impl TransportManager { Ok(()) } PeerState::Disconnected { - dial_record: Some(mut dial_record), + dial_record: Some(dial_record), } => { tracing::debug!( target: LOG_TARGET, @@ -898,7 +898,6 @@ impl TransportManager { // state to `Disconnected` true => match context.secondary_connection.take() { None => { - context.addresses.insert(record); context.state = PeerState::Disconnected { dial_record: actual_dial_record, }; @@ -909,7 +908,6 @@ impl TransportManager { })) } Some(secondary_connection) => { - context.addresses.insert(record); context.state = PeerState::Connected { record: secondary_connection, dial_record: actual_dial_record, @@ -945,7 +943,6 @@ impl TransportManager { "secondary connection closed", ); - context.addresses.insert(secondary_connection); context.state = PeerState::Connected { record, dial_record: actual_dial_record, From 4ad526fa6a005cf74209a60db6e367fc807b74b4 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Thu, 19 Sep 2024 12:31:22 +0300 Subject: [PATCH 07/66] manager: Make add_known_address more robust to track multiple peer addrs Signed-off-by: Alexandru Vasile --- src/transport/manager/address.rs | 3 +- src/transport/manager/handle.rs | 77 ++++++++++++++++++-------------- 2 files changed, 45 insertions(+), 35 deletions(-) diff --git a/src/transport/manager/address.rs b/src/transport/manager/address.rs index db18d77e..a51b341b 100644 --- a/src/transport/manager/address.rs +++ b/src/transport/manager/address.rs @@ -199,9 +199,10 @@ impl AddressStore { self.addresses.contains_key(address) } - /// Update the address record into [`AddressStore`] with the provided score. + /// Insert the address record into [`AddressStore`] with the provided score. /// /// If the address is not in the store, it will be inserted. + /// Otherwise, the score and connection ID will be updated. pub fn insert(&mut self, record: AddressRecord) { match self.addresses.entry(record.address.clone()) { std::collections::hash_map::Entry::Occupied(occupied_entry) => { diff --git a/src/transport/manager/handle.rs b/src/transport/manager/handle.rs index 0ded6406..775b7a49 100644 --- a/src/transport/manager/handle.rs +++ b/src/transport/manager/handle.rs @@ -181,49 +181,58 @@ impl TransportManagerHandle { addresses: impl Iterator, ) -> usize { let mut peers = self.peers.write(); - let addresses = addresses - .filter_map(|address| { - (self.supported_transport(&address) && !self.is_local_address(&address)) - .then_some(AddressRecord::from_multiaddr(address)?) - }) - .collect::>(); - - // if all of the added addresses belonged to unsupported transports, exit early - let num_added = addresses.len(); - if num_added == 0 { - tracing::debug!( - target: LOG_TARGET, - ?peer, - "didn't add any addresses for peer because transport is not supported", - ); - return 0usize; + let mut peer_addresses = HashMap::new(); + + for address in addresses { + // There is not supported transport configured that can dial this address. + if !self.supported_transport(&address) { + continue; + } + if self.is_local_address(&address) { + continue; + } + + // Check the peer ID if present. + if let Some(Protocol::P2p(multihash)) = address.iter().last() { + // Ignore the address if the peer ID is invalid. + let Ok(peer_id) = PeerId::from_multihash(multihash.clone()) else { + continue; + }; + + // This can correspond to the provided peerID or to a different one. + // It is important to keep track of all addresses to have a healthy + // address store to dial from. + peer_addresses.entry(peer_id).or_insert_with(HashSet::new).insert(address); + continue; + } + + // Add the provided peer ID to the address. + let address = address.with(Protocol::P2p(multihash::Multihash::from(peer.clone()))); + peer_addresses.entry(*peer).or_insert_with(HashSet::new).insert(address); } + let num_added = peer_addresses.get(peer).map_or(0, |addresses| addresses.len()); + tracing::trace!( target: LOG_TARGET, ?peer, - ?addresses, + ?peer_addresses, "add known addresses", ); - match peers.get_mut(peer) { - Some(context) => - for record in addresses { - if !context.addresses.contains(record.address()) { - context.addresses.insert(record); - } - }, - None => { - peers.insert( - *peer, - PeerContext { - state: PeerState::Disconnected { dial_record: None }, - addresses: AddressStore::from_iter(addresses), - secondary_connection: None, - }, - ); - } + for (peer, addresses) in peer_addresses { + let mut entry = peers.entry(peer).or_insert_with(|| PeerContext { + state: PeerState::Disconnected { dial_record: None }, + addresses: AddressStore::new(), + secondary_connection: None, + }); + + // All addresses should be valid at this point, since the peer ID was either added or + // double checked. + entry.addresses.extend( + addresses.into_iter().filter_map(|addr| AddressRecord::from_multiaddr(addr)), + ); } num_added From a924fa2109fb7131369bb1740c7ab3c0ad62c430 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Thu, 19 Sep 2024 12:38:19 +0300 Subject: [PATCH 08/66] manager: Construct the store with dialing address present Signed-off-by: Alexandru Vasile --- src/transport/manager/mod.rs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/transport/manager/mod.rs b/src/transport/manager/mod.rs index 2fa8a702..125264f5 100644 --- a/src/transport/manager/mod.rs +++ b/src/transport/manager/mod.rs @@ -707,7 +707,7 @@ impl TransportManager { state: PeerState::Dialing { record: record.clone(), }, - addresses: AddressStore::new(), + addresses: AddressStore::from_iter(std::iter::once(record.clone())), secondary_connection: None, }); } @@ -752,7 +752,6 @@ impl TransportManager { PeerState::Disconnected { dial_record: None }, ) { PeerState::Dialing { ref mut record } => { - debug_assert_eq!(record.connection_id(), &Some(connection_id)); if record.connection_id() != &Some(connection_id) { tracing::warn!( target: LOG_TARGET, @@ -769,7 +768,7 @@ impl TransportManager { return Ok(()); } - context.addresses.update(record.address(), SCORE_CONNECT_FAILURE); + context.addresses.update_score(record.address(), SCORE_CONNECT_FAILURE); context.state = PeerState::Disconnected { dial_record: None }; Ok(()) } @@ -797,7 +796,7 @@ impl TransportManager { return Ok(()); } - context.addresses.update(record.address(), SCORE_CONNECT_FAILURE); + context.addresses.update_score(record.address(), SCORE_CONNECT_FAILURE); context.state = PeerState::Connected { record, dial_record: None, @@ -830,7 +829,7 @@ impl TransportManager { return Ok(()); } - context.addresses.update(dial_record.address(), SCORE_CONNECT_FAILURE); + context.addresses.update_score(dial_record.address(), SCORE_CONNECT_FAILURE); context.state = PeerState::Disconnected { dial_record: None }; Ok(()) From 445ca743000df456778cce52a765d008e362ac74 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Thu, 19 Sep 2024 14:44:13 +0300 Subject: [PATCH 09/66] manager: Track dial failure addresses regardless of peer states Signed-off-by: Alexandru Vasile --- src/transport/manager/handle.rs | 2 +- src/transport/manager/mod.rs | 72 ++++++++++++++++++++++++++++++++- 2 files changed, 72 insertions(+), 2 deletions(-) diff --git a/src/transport/manager/handle.rs b/src/transport/manager/handle.rs index 775b7a49..88f332a2 100644 --- a/src/transport/manager/handle.rs +++ b/src/transport/manager/handle.rs @@ -222,7 +222,7 @@ impl TransportManagerHandle { ); for (peer, addresses) in peer_addresses { - let mut entry = peers.entry(peer).or_insert_with(|| PeerContext { + let entry = peers.entry(peer).or_insert_with(|| PeerContext { state: PeerState::Disconnected { dial_record: None }, addresses: AddressStore::new(), secondary_connection: None, diff --git a/src/transport/manager/mod.rs b/src/transport/manager/mod.rs index 125264f5..4c5740e2 100644 --- a/src/transport/manager/mod.rs +++ b/src/transport/manager/mod.rs @@ -22,7 +22,7 @@ use crate::{ addresses::PublicAddresses, codec::ProtocolCodec, crypto::ed25519::Keypair, - error::{AddressError, DialError, Error}, + error::{AddressError, DialError, Error, NegotiationError}, executor::Executor, protocol::{InnerTransportEvent, TransportService}, transport::{ @@ -74,9 +74,13 @@ const LOG_TARGET: &str = "litep2p::transport-manager"; /// Score for a working address. const SCORE_CONNECT_SUCCESS: i32 = 100i32; +/// Score for a negotiated connection with a different peer ID than expected. +const SCORE_MAY_CONNECT: i32 = 50i32; /// Score for a non-working address. const SCORE_CONNECT_FAILURE: i32 = -100i32; +/// Score for a dial failure due to a timeout. +const SCORE_TIMEOUT_FAILURE: i32 = -50i32; /// The connection established result. #[derive(Debug, Clone, Copy, Eq, PartialEq)] @@ -723,6 +727,70 @@ impl TransportManager { Ok(()) } + // Update the address on a dial failure. + fn update_address_on_dial_failure(&mut self, mut address: Multiaddr, error: &DialError) { + let mut peers = self.peers.write(); + + let score = match error { + DialError::Timeout => SCORE_TIMEOUT_FAILURE, + DialError::AddressError(_) => SCORE_CONNECT_FAILURE, + DialError::DnsError(_) => SCORE_CONNECT_FAILURE, + DialError::NegotiationError(negotiation_error) => match negotiation_error { + // Check if the address corresponds to a different peer ID than the one we're + // dialing. This can happen if the node operation restarts the node. + // + // In this case the address is reachable, however the peer ID is different. + // Keep track of this address for future dials. + // + // Note: this is happening quite often in practice and is the primary reason + // of negotiation failures. + NegotiationError::PeerIdMismatch(_, provided) => { + let context = peers.entry(*provided).or_insert_with(|| PeerContext { + state: PeerState::Disconnected { dial_record: None }, + addresses: AddressStore::new(), + secondary_connection: None, + }); + + if !std::matches!(address.iter().last(), Some(Protocol::P2p(_))) { + address.pop(); + } + context.addresses.insert(AddressRecord::new( + &provided, + address.clone(), + SCORE_MAY_CONNECT, + None, + )); + + return; + } + // Timeout during the negotiation phase. + NegotiationError::Timeout => SCORE_TIMEOUT_FAILURE, + // Treat other errors as connection failures. + _ => SCORE_CONNECT_FAILURE, + }, + }; + + // Extract the peer ID at this point to give `NegotiationError::PeerIdMismatch` a chance to + // propagate. + let peer_id = match address.iter().last() { + Some(Protocol::P2p(hash)) => PeerId::from_multihash(hash).ok(), + _ => None, + }; + let Some(peer_id) = peer_id else { + return; + }; + + // We need a valid context for this peer to keep track of failed addresses. + let context = peers.entry(peer_id).or_insert_with(|| PeerContext { + state: PeerState::Disconnected { dial_record: None }, + addresses: AddressStore::new(), + secondary_connection: None, + }); + context + .addresses + .insert(AddressRecord::new(&peer_id, address.clone(), score, None)); + } + /// Handle dial failure. fn on_dial_failure(&mut self, connection_id: ConnectionId) -> crate::Result<()> { let peer = self.pending_connections.remove(&connection_id).ok_or_else(|| { @@ -1532,6 +1600,8 @@ impl TransportManager { "failed to dial peer", ); + self.update_address_on_dial_failure(address.clone(), &error); + if let Ok(()) = self.on_dial_failure(connection_id) { match address.iter().last() { Some(Protocol::P2p(hash)) => match PeerId::from_multihash(hash) { From 7220f7a18cb94503b15ee4494c20248aa2246ce9 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Thu, 19 Sep 2024 14:47:34 +0300 Subject: [PATCH 10/66] manager: Update addresses only in dedicated function Signed-off-by: Alexandru Vasile --- src/transport/manager/mod.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/transport/manager/mod.rs b/src/transport/manager/mod.rs index 4c5740e2..e3269aef 100644 --- a/src/transport/manager/mod.rs +++ b/src/transport/manager/mod.rs @@ -792,6 +792,8 @@ impl TransportManager { } /// Handle dial failure. + /// + /// The main purpose of this function is to advance the internal `PeerState`. fn on_dial_failure(&mut self, connection_id: ConnectionId) -> crate::Result<()> { let peer = self.pending_connections.remove(&connection_id).ok_or_else(|| { tracing::error!( @@ -836,7 +838,6 @@ impl TransportManager { return Ok(()); } - context.addresses.update_score(record.address(), SCORE_CONNECT_FAILURE); context.state = PeerState::Disconnected { dial_record: None }; Ok(()) } @@ -864,7 +865,6 @@ impl TransportManager { return Ok(()); } - context.addresses.update_score(record.address(), SCORE_CONNECT_FAILURE); context.state = PeerState::Connected { record, dial_record: None, @@ -897,7 +897,6 @@ impl TransportManager { return Ok(()); } - context.addresses.update_score(dial_record.address(), SCORE_CONNECT_FAILURE); context.state = PeerState::Disconnected { dial_record: None }; Ok(()) @@ -1600,6 +1599,9 @@ impl TransportManager { "failed to dial peer", ); + // Update the addresses on dial failure regardless of the + // internal peer context state. This ensures a robust address tracking + // while taking into account the error type. self.update_address_on_dial_failure(address.clone(), &error); if let Ok(()) = self.on_dial_failure(connection_id) { From f56e8c06ec7ae8be1ec46bb58c256256915044c2 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Thu, 19 Sep 2024 14:53:20 +0300 Subject: [PATCH 11/66] manager: Remove unneeded methods Signed-off-by: Alexandru Vasile --- src/transport/manager/address.rs | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/src/transport/manager/address.rs b/src/transport/manager/address.rs index a51b341b..7866f7d2 100644 --- a/src/transport/manager/address.rs +++ b/src/transport/manager/address.rs @@ -194,11 +194,6 @@ impl AddressStore { self.addresses.is_empty() } - /// Check if address is already in the address store. - pub fn contains(&self, address: &Multiaddr) -> bool { - self.addresses.contains_key(address) - } - /// Insert the address record into [`AddressStore`] with the provided score. /// /// If the address is not in the store, it will be inserted. @@ -216,13 +211,6 @@ impl AddressStore { } } - /// Update the score of an existing address. - pub fn update_score(&mut self, address: &Multiaddr, score: i32) { - if let Some(record) = self.addresses.get_mut(address) { - record.update_score(score); - } - } - /// Return the available addresses sorted by score. pub fn addresses(&self) -> Vec { let mut records = self.addresses.values().cloned().collect::>(); From a0fb0c9a6ed081f0e39749676962dc0d09b8e13e Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Thu, 19 Sep 2024 14:58:23 +0300 Subject: [PATCH 12/66] manager: Keep track of the latest connection ID Signed-off-by: Alexandru Vasile --- src/transport/manager/address.rs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/transport/manager/address.rs b/src/transport/manager/address.rs index 7866f7d2..4400f69a 100644 --- a/src/transport/manager/address.rs +++ b/src/transport/manager/address.rs @@ -108,6 +108,16 @@ impl AddressRecord { self.score = self.score.saturating_add(score); } + /// Update the `ConnectionId` for the [`AddressRecord`]. + pub fn update_connection_id(&mut self, connection_id: Option) { + match (self.connection_id, connection_id) { + (Some(_), Some(_)) | (None, Some(_)) => { + self.connection_id = connection_id; + } + _ => {} + }; + } + /// Set `ConnectionId` for the [`AddressRecord`]. pub fn set_connection_id(&mut self, connection_id: ConnectionId) { self.connection_id = Some(connection_id); @@ -203,7 +213,7 @@ impl AddressStore { std::collections::hash_map::Entry::Occupied(occupied_entry) => { let found_record = occupied_entry.into_mut(); found_record.update_score(record.score); - found_record.connection_id = record.connection_id; + found_record.update_connection_id(record.connection_id); } std::collections::hash_map::Entry::Vacant(vacant_entry) => { vacant_entry.insert(record.clone()); From 40e149b2c07cbbbe18571013e3b3792d98c98ee8 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Thu, 19 Sep 2024 15:03:40 +0300 Subject: [PATCH 13/66] manager: Better constants for scoring Signed-off-by: Alexandru Vasile --- src/transport/manager/mod.rs | 33 +++++++++++++++++++-------------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/src/transport/manager/mod.rs b/src/transport/manager/mod.rs index e3269aef..1844c9be 100644 --- a/src/transport/manager/mod.rs +++ b/src/transport/manager/mod.rs @@ -72,15 +72,20 @@ pub(crate) mod handle; /// Logging target for the file. const LOG_TARGET: &str = "litep2p::transport-manager"; -/// Score for a working address. -const SCORE_CONNECT_SUCCESS: i32 = 100i32; -/// Score for a negotiated connection with a different peer ID than expected. -const SCORE_MAY_CONNECT: i32 = 50i32; +/// Scores for address records. +mod scores { + /// Score indicating that the connection was successfully established. + pub const CONNECTION_ESTABLISHED: i32 = 100i32; -/// Score for a non-working address. -const SCORE_CONNECT_FAILURE: i32 = -100i32; -/// Score for a dial failure due to a timeout. -const SCORE_TIMEOUT_FAILURE: i32 = -50i32; + /// Score for a connection with a peer using a different ID than expected. + pub const DIFFERENT_PEER_ID: i32 = 50i32; + + /// Score for failing to connect due to an invalid or unreachable address. + pub const CONNECTION_FAILURE: i32 = -100i32; + + /// Score for a connection attempt that failed due to a timeout. + pub const TIMEOUT_FAILURE: i32 = -50i32; +} /// The connection established result. #[derive(Debug, Clone, Copy, Eq, PartialEq)] @@ -732,9 +737,9 @@ impl TransportManager { let mut peers = self.peers.write(); let score = match error { - DialError::Timeout => SCORE_TIMEOUT_FAILURE, - DialError::AddressError(_) => SCORE_CONNECT_FAILURE, - DialError::DnsError(_) => SCORE_CONNECT_FAILURE, + DialError::Timeout => scores::CONNECTION_ESTABLISHED, + DialError::AddressError(_) => scores::CONNECTION_FAILURE, + DialError::DnsError(_) => scores::CONNECTION_FAILURE, DialError::NegotiationError(negotiation_error) => match negotiation_error { // Check if the address corresponds to a different peer ID than the one we're // dialing. This can happen if the node operation restarts the node. @@ -757,16 +762,16 @@ impl TransportManager { context.addresses.insert(AddressRecord::new( &provided, address.clone(), - SCORE_MAY_CONNECT, + scores::DIFFERENT_PEER_ID, None, )); return; } // Timeout during the negotiation phase. - NegotiationError::Timeout => SCORE_TIMEOUT_FAILURE, + NegotiationError::Timeout => scores::TIMEOUT_FAILURE, // Treat other errors as connection failures. - _ => SCORE_CONNECT_FAILURE, + _ => scores::CONNECTION_FAILURE, }, }; From 7e157475524df92e8dce58fc0be004c99bc3ce69 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Thu, 19 Sep 2024 15:31:31 +0300 Subject: [PATCH 14/66] manager: Update addresses on connection established Signed-off-by: Alexandru Vasile --- src/transport/manager/mod.rs | 67 +++++++++++++++++++++++------------- 1 file changed, 43 insertions(+), 24 deletions(-) diff --git a/src/transport/manager/mod.rs b/src/transport/manager/mod.rs index 1844c9be..d716df29 100644 --- a/src/transport/manager/mod.rs +++ b/src/transport/manager/mod.rs @@ -1065,6 +1065,37 @@ impl TransportManager { } } + /// Update the address on a connection established. + fn update_address_on_connection_established(&mut self, peer: PeerId, endpoint: &Endpoint) { + // The connection can be inbound or outbound. + // For the inbound connection type, in most cases, the remote peer dialed + // with an ephemeral port which it might not be listening on. + // Therefore, we only insert the address into the store if we're the dialer. + if endpoint.is_listener() { + return; + } + + let mut peers = self.peers.write(); + + let record = AddressRecord::new( + &peer, + endpoint.address().clone(), + scores::CONNECTION_ESTABLISHED, + Some(endpoint.connection_id()), + ); + + let context = peers.entry(peer).or_insert_with(|| PeerContext { + state: PeerState::Connected { + record: record.clone(), + dial_record: None, + }, + addresses: AddressStore::new(), + secondary_connection: None, + }); + + context.addresses.insert(record); + } + fn on_connection_established( &mut self, peer: PeerId, @@ -1115,19 +1146,6 @@ impl TransportManager { "secondary connection already exists, ignoring connection", ); - // insert address into the store only if we're the dialer - // - // if we're the listener, remote might have dialed with an ephemeral port - // which it might not be listening, making this address useless - if endpoint.is_listener() { - context.addresses.insert(AddressRecord::new( - &peer, - endpoint.address().clone(), - SCORE_CONNECT_SUCCESS, - None, - )) - } - return Ok(ConnectionEstablishedResult::Reject); } None => match dial_record.take() { @@ -1145,7 +1163,7 @@ impl TransportManager { context.secondary_connection = Some(AddressRecord::new( &peer, endpoint.address().clone(), - SCORE_CONNECT_SUCCESS, + scores::CONNECTION_ESTABLISHED, Some(endpoint.connection_id()), )); } @@ -1161,7 +1179,7 @@ impl TransportManager { context.secondary_connection = Some(AddressRecord::new( &peer, endpoint.address().clone(), - SCORE_CONNECT_SUCCESS, + scores::CONNECTION_ESTABLISHED, Some(endpoint.connection_id()), )); } @@ -1212,7 +1230,7 @@ impl TransportManager { record: AddressRecord::new( &peer, endpoint.address().clone(), - SCORE_CONNECT_SUCCESS, + scores::CONNECTION_ESTABLISHED, Some(endpoint.connection_id()), ), dial_record: Some(record.clone()), @@ -1262,18 +1280,17 @@ impl TransportManager { let record = match records.remove(endpoint.address()) { Some(mut record) => { - record.update_score(SCORE_CONNECT_SUCCESS); + record.update_score(scores::CONNECTION_ESTABLISHED); record.set_connection_id(endpoint.connection_id()); record } None => AddressRecord::new( &peer, endpoint.address().clone(), - SCORE_CONNECT_SUCCESS, + scores::CONNECTION_ESTABLISHED, Some(endpoint.connection_id()), ), }; - context.addresses.extend(records.iter().map(|(_, record)| record)); context.state = PeerState::Connected { record, @@ -1302,7 +1319,7 @@ impl TransportManager { AddressRecord::new( &peer, endpoint.address().clone(), - SCORE_CONNECT_SUCCESS, + scores::CONNECTION_ESTABLISHED, Some(endpoint.connection_id()), ), Some(dial_record), @@ -1312,7 +1329,7 @@ impl TransportManager { AddressRecord::new( &peer, endpoint.address().clone(), - SCORE_CONNECT_SUCCESS, + scores::CONNECTION_ESTABLISHED, Some(endpoint.connection_id()), ), None, @@ -1333,7 +1350,7 @@ impl TransportManager { record: AddressRecord::new( &peer, endpoint.address().clone(), - SCORE_CONNECT_SUCCESS, + scores::CONNECTION_ESTABLISHED, Some(endpoint.connection_id()), ), dial_record: None, @@ -1412,7 +1429,7 @@ impl TransportManager { // all other address records back to `AddressStore`. and ask // transport to negotiate the let mut dial_record = records.remove(&address).expect("address to exist"); - dial_record.update_score(SCORE_CONNECT_SUCCESS); + dial_record.update_score(scores::CONNECTION_ESTABLISHED); // negotiate the connection match self @@ -1522,7 +1539,7 @@ impl TransportManager { if transports.is_empty() { for (_, mut record) in records { - record.update_score(SCORE_CONNECT_FAILURE); + record.update_score(scores::CONNECTION_FAILURE); context.addresses.insert(record); } @@ -1690,6 +1707,8 @@ impl TransportManager { } TransportEvent::ConnectionEstablished { peer, endpoint } => { self.opening_errors.remove(&endpoint.connection_id()); + self.update_address_on_connection_established(peer, &endpoint); + match self.on_connection_established(peer, &endpoint) { Err(error) => { tracing::debug!( From 9ce8a206e0c979f23b0ef89cf4dd7ac733173ee3 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Thu, 19 Sep 2024 16:21:06 +0300 Subject: [PATCH 15/66] manager: Replace AddressRecord with DialRecord Signed-off-by: Alexandru Vasile --- src/transport/manager/mod.rs | 106 ++++++++++++++++----------------- src/transport/manager/types.rs | 21 +++++-- 2 files changed, 66 insertions(+), 61 deletions(-) diff --git a/src/transport/manager/mod.rs b/src/transport/manager/mod.rs index d716df29..94215ea1 100644 --- a/src/transport/manager/mod.rs +++ b/src/transport/manager/mod.rs @@ -29,7 +29,7 @@ use crate::{ manager::{ address::{AddressRecord, AddressStore}, handle::InnerTransportManagerCommand, - types::{PeerContext, PeerState}, + types::{DialRecord, PeerContext, PeerState}, }, Endpoint, Transport, TransportEvent, }, @@ -672,6 +672,11 @@ impl TransportManager { { let mut peers = self.peers.write(); + let dial_record = DialRecord { + address: record.address().clone(), + connection_id, + }; + match peers.entry(remote_peer_id) { Entry::Occupied(occupied) => { let context = occupied.into_mut(); @@ -706,7 +711,7 @@ impl TransportManager { } PeerState::Disconnected { dial_record: None } => { context.state = PeerState::Dialing { - record: record.clone(), + record: dial_record, }; } } @@ -714,7 +719,7 @@ impl TransportManager { Entry::Vacant(vacant) => { vacant.insert(PeerContext { state: PeerState::Dialing { - record: record.clone(), + record: dial_record, }, addresses: AddressStore::from_iter(std::iter::once(record.clone())), secondary_connection: None, @@ -827,7 +832,7 @@ impl TransportManager { PeerState::Disconnected { dial_record: None }, ) { PeerState::Dialing { ref mut record } => { - if record.connection_id() != &Some(connection_id) { + if record.connection_id != connection_id { tracing::warn!( target: LOG_TARGET, ?peer, @@ -853,7 +858,7 @@ impl TransportManager { record, dial_record: Some(dial_record), } => { - if dial_record.connection_id() != &Some(connection_id) { + if dial_record.connection_id != connection_id { tracing::warn!( target: LOG_TARGET, ?peer, @@ -886,7 +891,7 @@ impl TransportManager { "dial failed for a disconnected peer", ); - if dial_record.connection_id() != &Some(connection_id) { + if dial_record.connection_id != connection_id { tracing::warn!( target: LOG_TARGET, ?peer, @@ -961,7 +966,7 @@ impl TransportManager { PeerState::Connected { record, dial_record: actual_dial_record, - } => match record.connection_id() == &Some(connection_id) { + } => match record.connection_id == connection_id { // primary connection was closed // // if secondary connection exists, switch to using it while keeping peer in @@ -980,7 +985,12 @@ impl TransportManager { } Some(secondary_connection) => { context.state = PeerState::Connected { - record: secondary_connection, + record: DialRecord { + address: secondary_connection.address().clone(), + connection_id: secondary_connection + .connection_id() + .expect("Secondary has connection ID; qed"), + }, dial_record: actual_dial_record, }; @@ -1086,7 +1096,10 @@ impl TransportManager { let context = peers.entry(peer).or_insert_with(|| PeerContext { state: PeerState::Connected { - record: record.clone(), + record: DialRecord { + address: endpoint.address().clone(), + connection_id: endpoint.connection_id(), + }, dial_record: None, }, addresses: AddressStore::new(), @@ -1149,9 +1162,7 @@ impl TransportManager { return Ok(ConnectionEstablishedResult::Reject); } None => match dial_record.take() { - Some(record) - if record.connection_id() == &Some(endpoint.connection_id()) => - { + Some(record) if record.connection_id == endpoint.connection_id() => { tracing::debug!( target: LOG_TARGET, ?peer, @@ -1201,7 +1212,7 @@ impl TransportManager { }, }, PeerState::Dialing { ref record, .. } => { - match record.connection_id() == &Some(endpoint.connection_id()) { + match record.connection_id == endpoint.connection_id() { true => { tracing::trace!( target: LOG_TARGET, @@ -1227,12 +1238,10 @@ impl TransportManager { ); context.state = PeerState::Connected { - record: AddressRecord::new( - &peer, - endpoint.address().clone(), - scores::CONNECTION_ESTABLISHED, - Some(endpoint.connection_id()), - ), + record: DialRecord { + address: endpoint.address().clone(), + connection_id: endpoint.connection_id(), + }, dial_record: Some(record.clone()), }; } @@ -1278,22 +1287,11 @@ impl TransportManager { .expect("`ConnectionId` to exist"), ); - let record = match records.remove(endpoint.address()) { - Some(mut record) => { - record.update_score(scores::CONNECTION_ESTABLISHED); - record.set_connection_id(endpoint.connection_id()); - record - } - None => AddressRecord::new( - &peer, - endpoint.address().clone(), - scores::CONNECTION_ESTABLISHED, - Some(endpoint.connection_id()), - ), - }; - context.state = PeerState::Connected { - record, + record: DialRecord { + address: endpoint.address().clone(), + connection_id: endpoint.connection_id(), + }, dial_record: None, }; } @@ -1310,28 +1308,23 @@ impl TransportManager { ); let (record, dial_record) = match dial_record.take() { - Some(mut dial_record) => - if dial_record.address() == endpoint.address() { - dial_record.set_connection_id(endpoint.connection_id()); + Some(dial_record) => + if &dial_record.address == endpoint.address() { (dial_record, None) } else { ( - AddressRecord::new( - &peer, - endpoint.address().clone(), - scores::CONNECTION_ESTABLISHED, - Some(endpoint.connection_id()), - ), + DialRecord { + address: endpoint.address().clone(), + connection_id: endpoint.connection_id(), + }, Some(dial_record), ) }, None => ( - AddressRecord::new( - &peer, - endpoint.address().clone(), - scores::CONNECTION_ESTABLISHED, - Some(endpoint.connection_id()), - ), + DialRecord { + address: endpoint.address().clone(), + connection_id: endpoint.connection_id(), + }, None, ), }; @@ -1347,12 +1340,10 @@ impl TransportManager { peer, PeerContext { state: PeerState::Connected { - record: AddressRecord::new( - &peer, - endpoint.address().clone(), - scores::CONNECTION_ESTABLISHED, - Some(endpoint.connection_id()), - ), + record: DialRecord { + address: endpoint.address().clone(), + connection_id: endpoint.connection_id(), + }, dial_record: None, }, addresses: AddressStore::new(), @@ -1451,7 +1442,10 @@ impl TransportManager { self.pending_connections.insert(connection_id, peer); context.state = PeerState::Dialing { - record: dial_record, + record: DialRecord { + address: address.clone(), + connection_id, + }, }; for (_, record) in records { diff --git a/src/transport/manager/types.rs b/src/transport/manager/types.rs index b367a086..7d60db7b 100644 --- a/src/transport/manager/types.rs +++ b/src/transport/manager/types.rs @@ -51,8 +51,8 @@ pub enum SupportedTransport { pub enum PeerState { /// `Litep2p` is connected to peer. Connected { - /// Address record. - record: AddressRecord, + /// The established record of the connection. + record: DialRecord, /// Dial address, if it exists. /// @@ -60,7 +60,7 @@ pub enum PeerState { /// the local node and connection was established successfully. This dial address /// is stored for processing later when the dial attempt concluded as either /// successful/failed. - dial_record: Option, + dial_record: Option, }, /// Connection to peer is opening over one or more addresses. @@ -78,7 +78,7 @@ pub enum PeerState { /// Peer is being dialed. Dialing { /// Address record. - record: AddressRecord, + record: DialRecord, }, /// `Litep2p` is not connected to peer. @@ -90,10 +90,21 @@ pub enum PeerState { /// been closed before the dial concluded which means that /// [`crate::transport::manager::TransportManager`] must be prepared to handle the dial /// failure even after the connection has been closed. - dial_record: Option, + dial_record: Option, }, } +/// The dial record. +#[allow(clippy::derived_hash_with_manual_eq)] +#[derive(Debug, Clone, Hash)] +pub struct DialRecord { + /// Address that was dialed. + pub address: Multiaddr, + + /// Connection ID resulted from dialing. + pub connection_id: ConnectionId, +} + /// Peer context. #[derive(Debug)] pub struct PeerContext { From 47f97b549716b6bbfd810aae4c07c79c11195931 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Thu, 19 Sep 2024 16:42:48 +0300 Subject: [PATCH 16/66] manager: Remove AddressRecord from the Opening state Signed-off-by: Alexandru Vasile --- src/transport/manager/mod.rs | 55 +++++++++++++--------------------- src/transport/manager/types.rs | 4 +-- 2 files changed, 22 insertions(+), 37 deletions(-) diff --git a/src/transport/manager/mod.rs b/src/transport/manager/mod.rs index 94215ea1..d2a7c457 100644 --- a/src/transport/manager/mod.rs +++ b/src/transport/manager/mod.rs @@ -490,11 +490,11 @@ impl TransportManager { return Ok(()); } - let mut records: HashMap<_, _> = addresses + let records: HashSet<_> = addresses .addresses() .into_iter() .take(limit) - .map(|record| (record.address().clone(), record)) + .map(|record| record.address().clone()) .collect(); if records.is_empty() { @@ -502,8 +502,8 @@ impl TransportManager { } let locked_addresses = self.listen_addresses.read(); - for record in records.values() { - if locked_addresses.contains(record.as_ref()) { + for record in &records { + if locked_addresses.contains(record) { tracing::warn!( target: LOG_TARGET, ?peer, @@ -535,9 +535,7 @@ impl TransportManager { let mut quic = Vec::new(); let mut tcp = Vec::new(); - for (address, record) in &mut records { - record.set_connection_id(connection_id); - + for address in &records { #[cfg(feature = "quic")] if address.iter().any(|p| std::matches!(&p, Protocol::QuicV1)) { quic.push(address.clone()); @@ -1274,18 +1272,7 @@ impl TransportManager { // since an inbound connection was removed, the outbound connection can be // removed from pending dials - // - // all records have the same `ConnectionId` so it doesn't matter which of them - // is used to remove the pending dial - self.pending_connections.remove( - &records - .iter() - .next() - .expect("record to exist") - .1 - .connection_id() - .expect("`ConnectionId` to exist"), - ); + self.pending_connections.remove(&connection_id); context.state = PeerState::Connected { record: DialRecord { @@ -1393,9 +1380,9 @@ impl TransportManager { PeerState::Disconnected { dial_record: None }, ) { PeerState::Opening { - mut records, connection_id, transports, + .. } => { tracing::trace!( target: LOG_TARGET, @@ -1414,14 +1401,6 @@ impl TransportManager { .cancel(connection_id); } - // set peer state to `Dialing` to signal that the connection is fully opening - // - // set the succeeded `AddressRecord` as the one that is used for dialing and move - // all other address records back to `AddressStore`. and ask - // transport to negotiate the - let mut dial_record = records.remove(&address).expect("address to exist"); - dial_record.update_score(scores::CONNECTION_ESTABLISHED); - // negotiate the connection match self .transports @@ -1434,7 +1413,6 @@ impl TransportManager { target: LOG_TARGET, ?peer, ?connection_id, - ?dial_record, ?transport, "negotiation started" ); @@ -1448,9 +1426,12 @@ impl TransportManager { }, }; - for (_, record) in records { - context.addresses.insert(record); - } + context.addresses.insert(AddressRecord::new( + &peer, + address, + scores::CONNECTION_ESTABLISHED, + Some(connection_id), + )); Ok(()) } @@ -1532,9 +1513,13 @@ impl TransportManager { transports.remove(&transport); if transports.is_empty() { - for (_, mut record) in records { - record.update_score(scores::CONNECTION_FAILURE); - context.addresses.insert(record); + for address in records { + context.addresses.insert(AddressRecord::new( + &peer, + address, + scores::CONNECTION_FAILURE, + None, + )); } tracing::trace!( diff --git a/src/transport/manager/types.rs b/src/transport/manager/types.rs index 7d60db7b..e7969ed8 100644 --- a/src/transport/manager/types.rs +++ b/src/transport/manager/types.rs @@ -25,7 +25,7 @@ use crate::{ use multiaddr::Multiaddr; -use std::collections::{HashMap, HashSet}; +use std::collections::HashSet; /// Supported protocols. #[derive(Debug, Copy, Clone, Hash, PartialEq, Eq)] @@ -66,7 +66,7 @@ pub enum PeerState { /// Connection to peer is opening over one or more addresses. Opening { /// Address records used for dialing. - records: HashMap, + records: HashSet, /// Connection ID. connection_id: ConnectionId, From 7f3147d73dab7d7aaccc583ae7a245dca8402dba Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Thu, 19 Sep 2024 16:58:07 +0300 Subject: [PATCH 17/66] manager: Remove assert Signed-off-by: Alexandru Vasile --- src/transport/manager/mod.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/transport/manager/mod.rs b/src/transport/manager/mod.rs index d2a7c457..5ea4e590 100644 --- a/src/transport/manager/mod.rs +++ b/src/transport/manager/mod.rs @@ -1250,8 +1250,6 @@ impl TransportManager { connection_id, ref transports, } => { - debug_assert!(std::matches!(endpoint, &Endpoint::Listener { .. })); - tracing::trace!( target: LOG_TARGET, ?peer, From d6e2d4db2f1a988385e2f6e2e11f3bb92896ec79 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Thu, 19 Sep 2024 17:31:45 +0300 Subject: [PATCH 18/66] manager: Rename DialRecord to ConnectionRecord Signed-off-by: Alexandru Vasile --- src/transport/manager/address.rs | 5 ---- src/transport/manager/mod.rs | 48 ++++++++++++++------------------ src/transport/manager/types.rs | 30 ++++++++++++-------- 3 files changed, 40 insertions(+), 43 deletions(-) diff --git a/src/transport/manager/address.rs b/src/transport/manager/address.rs index 4400f69a..74d9759f 100644 --- a/src/transport/manager/address.rs +++ b/src/transport/manager/address.rs @@ -98,11 +98,6 @@ impl AddressRecord { &self.address } - /// Get connection ID. - pub fn connection_id(&self) -> &Option { - &self.connection_id - } - /// Update score of an address. pub fn update_score(&mut self, score: i32) { self.score = self.score.saturating_add(score); diff --git a/src/transport/manager/mod.rs b/src/transport/manager/mod.rs index 5ea4e590..b6f96e4c 100644 --- a/src/transport/manager/mod.rs +++ b/src/transport/manager/mod.rs @@ -29,7 +29,7 @@ use crate::{ manager::{ address::{AddressRecord, AddressStore}, handle::InnerTransportManagerCommand, - types::{DialRecord, PeerContext, PeerState}, + types::{ConnectionRecord, PeerContext, PeerState}, }, Endpoint, Transport, TransportEvent, }, @@ -670,7 +670,7 @@ impl TransportManager { { let mut peers = self.peers.write(); - let dial_record = DialRecord { + let dial_record = ConnectionRecord { address: record.address().clone(), connection_id, }; @@ -983,11 +983,9 @@ impl TransportManager { } Some(secondary_connection) => { context.state = PeerState::Connected { - record: DialRecord { - address: secondary_connection.address().clone(), - connection_id: secondary_connection - .connection_id() - .expect("Secondary has connection ID; qed"), + record: ConnectionRecord { + address: secondary_connection.address.clone(), + connection_id: secondary_connection.connection_id, }, dial_record: actual_dial_record, }; @@ -998,7 +996,7 @@ impl TransportManager { // secondary connection was closed false => match context.secondary_connection.take() { Some(secondary_connection) => { - if secondary_connection.connection_id() != &Some(connection_id) { + if secondary_connection.connection_id != connection_id { tracing::debug!( target: LOG_TARGET, ?peer, @@ -1094,7 +1092,7 @@ impl TransportManager { let context = peers.entry(peer).or_insert_with(|| PeerContext { state: PeerState::Connected { - record: DialRecord { + record: ConnectionRecord { address: endpoint.address().clone(), connection_id: endpoint.connection_id(), }, @@ -1169,12 +1167,10 @@ impl TransportManager { "dialed connection opened as secondary connection", ); - context.secondary_connection = Some(AddressRecord::new( - &peer, - endpoint.address().clone(), - scores::CONNECTION_ESTABLISHED, - Some(endpoint.connection_id()), - )); + context.secondary_connection = Some(ConnectionRecord { + address: endpoint.address().clone(), + connection_id: endpoint.connection_id(), + }); } None => { tracing::debug!( @@ -1185,12 +1181,10 @@ impl TransportManager { "secondary connection", ); - context.secondary_connection = Some(AddressRecord::new( - &peer, - endpoint.address().clone(), - scores::CONNECTION_ESTABLISHED, - Some(endpoint.connection_id()), - )); + context.secondary_connection = Some(ConnectionRecord { + address: endpoint.address().clone(), + connection_id: endpoint.connection_id(), + }); } Some(record) => { tracing::warn!( @@ -1236,7 +1230,7 @@ impl TransportManager { ); context.state = PeerState::Connected { - record: DialRecord { + record: ConnectionRecord { address: endpoint.address().clone(), connection_id: endpoint.connection_id(), }, @@ -1273,7 +1267,7 @@ impl TransportManager { self.pending_connections.remove(&connection_id); context.state = PeerState::Connected { - record: DialRecord { + record: ConnectionRecord { address: endpoint.address().clone(), connection_id: endpoint.connection_id(), }, @@ -1298,7 +1292,7 @@ impl TransportManager { (dial_record, None) } else { ( - DialRecord { + ConnectionRecord { address: endpoint.address().clone(), connection_id: endpoint.connection_id(), }, @@ -1306,7 +1300,7 @@ impl TransportManager { ) }, None => ( - DialRecord { + ConnectionRecord { address: endpoint.address().clone(), connection_id: endpoint.connection_id(), }, @@ -1325,7 +1319,7 @@ impl TransportManager { peer, PeerContext { state: PeerState::Connected { - record: DialRecord { + record: ConnectionRecord { address: endpoint.address().clone(), connection_id: endpoint.connection_id(), }, @@ -1418,7 +1412,7 @@ impl TransportManager { self.pending_connections.insert(connection_id, peer); context.state = PeerState::Dialing { - record: DialRecord { + record: ConnectionRecord { address: address.clone(), connection_id, }, diff --git a/src/transport/manager/types.rs b/src/transport/manager/types.rs index e7969ed8..7812439c 100644 --- a/src/transport/manager/types.rs +++ b/src/transport/manager/types.rs @@ -18,10 +18,7 @@ // FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER // DEALINGS IN THE SOFTWARE. -use crate::{ - transport::manager::address::{AddressRecord, AddressStore}, - types::ConnectionId, -}; +use crate::{transport::manager::address::AddressStore, types::ConnectionId}; use multiaddr::Multiaddr; @@ -52,7 +49,7 @@ pub enum PeerState { /// `Litep2p` is connected to peer. Connected { /// The established record of the connection. - record: DialRecord, + record: ConnectionRecord, /// Dial address, if it exists. /// @@ -60,7 +57,7 @@ pub enum PeerState { /// the local node and connection was established successfully. This dial address /// is stored for processing later when the dial attempt concluded as either /// successful/failed. - dial_record: Option, + dial_record: Option, }, /// Connection to peer is opening over one or more addresses. @@ -78,7 +75,7 @@ pub enum PeerState { /// Peer is being dialed. Dialing { /// Address record. - record: DialRecord, + record: ConnectionRecord, }, /// `Litep2p` is not connected to peer. @@ -90,14 +87,25 @@ pub enum PeerState { /// been closed before the dial concluded which means that /// [`crate::transport::manager::TransportManager`] must be prepared to handle the dial /// failure even after the connection has been closed. - dial_record: Option, + dial_record: Option, }, } -/// The dial record. +/// The connection record keeps track of the connection ID and the address of the connection. +/// +/// The connection ID is used to track the connection in the transport layer. +/// While the address is used to keep a healthy view of the network for dialing purposes. +/// +/// # Note +/// +/// The structure is used to keep track of: +/// +/// - dialing state for outbound connections. +/// - established outbound connections via [`PeerState::Connected`]. +/// - established inbound connections via `PeerContext::secondary_connection`. #[allow(clippy::derived_hash_with_manual_eq)] #[derive(Debug, Clone, Hash)] -pub struct DialRecord { +pub struct ConnectionRecord { /// Address that was dialed. pub address: Multiaddr, @@ -112,7 +120,7 @@ pub struct PeerContext { pub state: PeerState, /// Secondary connection, if it's open. - pub secondary_connection: Option, + pub secondary_connection: Option, /// Known addresses of peer. pub addresses: AddressStore, From 1eda8a76df85953381b8b8c7bb57b441d1b0f241 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Thu, 19 Sep 2024 17:35:03 +0300 Subject: [PATCH 19/66] manager/address: Remove connection ID from address store Signed-off-by: Alexandru Vasile --- src/transport/manager/address.rs | 38 +++----------------------------- src/transport/manager/mod.rs | 27 +++++------------------ 2 files changed, 8 insertions(+), 57 deletions(-) diff --git a/src/transport/manager/address.rs b/src/transport/manager/address.rs index 74d9759f..201f1f02 100644 --- a/src/transport/manager/address.rs +++ b/src/transport/manager/address.rs @@ -18,7 +18,7 @@ // FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER // DEALINGS IN THE SOFTWARE. -use crate::{types::ConnectionId, PeerId}; +use crate::PeerId; use std::collections::HashMap; @@ -36,9 +36,6 @@ pub struct AddressRecord { /// Address. address: Multiaddr, - - /// Connection ID, if specified. - connection_id: Option, } impl AsRef for AddressRecord { @@ -50,12 +47,7 @@ impl AsRef for AddressRecord { impl AddressRecord { /// Create new `AddressRecord` and if `address` doesn't contain `P2p`, /// append the provided `PeerId` to the address. - pub fn new( - peer: &PeerId, - address: Multiaddr, - score: i32, - connection_id: Option, - ) -> Self { + pub fn new(peer: &PeerId, address: Multiaddr, score: i32) -> Self { let address = if !std::matches!(address.iter().last(), Some(Protocol::P2p(_))) { address.with(Protocol::P2p( Multihash::from_bytes(&peer.to_bytes()).expect("valid peer id"), @@ -64,11 +56,7 @@ impl AddressRecord { address }; - Self { - address, - score, - connection_id, - } + Self { address, score } } /// Create `AddressRecord` from `Multiaddr`. @@ -83,7 +71,6 @@ impl AddressRecord { Some(AddressRecord { address, score: 0i32, - connection_id: None, }) } @@ -102,21 +89,6 @@ impl AddressRecord { pub fn update_score(&mut self, score: i32) { self.score = self.score.saturating_add(score); } - - /// Update the `ConnectionId` for the [`AddressRecord`]. - pub fn update_connection_id(&mut self, connection_id: Option) { - match (self.connection_id, connection_id) { - (Some(_), Some(_)) | (None, Some(_)) => { - self.connection_id = connection_id; - } - _ => {} - }; - } - - /// Set `ConnectionId` for the [`AddressRecord`]. - pub fn set_connection_id(&mut self, connection_id: ConnectionId) { - self.connection_id = Some(connection_id); - } } impl PartialEq for AddressRecord { @@ -208,7 +180,6 @@ impl AddressStore { std::collections::hash_map::Entry::Occupied(occupied_entry) => { let found_record = occupied_entry.into_mut(); found_record.update_score(record.score); - found_record.update_connection_id(record.connection_id); } std::collections::hash_map::Entry::Vacant(vacant_entry) => { vacant_entry.insert(record.clone()); @@ -253,7 +224,6 @@ mod tests { .with(Protocol::from(address.ip())) .with(Protocol::Tcp(address.port())), score, - None, ) } @@ -277,7 +247,6 @@ mod tests { .with(Protocol::Tcp(address.port())) .with(Protocol::Ws(std::borrow::Cow::Owned("/".to_string()))), score, - None, ) } @@ -301,7 +270,6 @@ mod tests { .with(Protocol::Udp(address.port())) .with(Protocol::QuicV1), score, - None, ) } diff --git a/src/transport/manager/mod.rs b/src/transport/manager/mod.rs index b6f96e4c..1f585460 100644 --- a/src/transport/manager/mod.rs +++ b/src/transport/manager/mod.rs @@ -601,7 +601,7 @@ impl TransportManager { pub async fn dial_address(&mut self, address: Multiaddr) -> crate::Result<()> { self.connection_limits.on_dial_address()?; - let mut record = AddressRecord::from_multiaddr(address) + let record = AddressRecord::from_multiaddr(address) .ok_or(Error::AddressError(AddressError::PeerIdMissing))?; if self.listen_addresses.read().contains(record.as_ref()) { @@ -665,7 +665,6 @@ impl TransportManager { // set connection id for the address record and put peer into `Dialing` state let connection_id = self.next_connection_id(); - record.set_connection_id(connection_id); { let mut peers = self.peers.write(); @@ -766,7 +765,6 @@ impl TransportManager { &provided, address.clone(), scores::DIFFERENT_PEER_ID, - None, )); return; @@ -794,9 +792,7 @@ impl TransportManager { addresses: AddressStore::new(), secondary_connection: None, }); - context - .addresses - .insert(AddressRecord::new(&peer_id, address.clone(), score, None)); + context.addresses.insert(AddressRecord::new(&peer_id, address.clone(), score)); } /// Handle dial failure. @@ -1087,7 +1083,6 @@ impl TransportManager { &peer, endpoint.address().clone(), scores::CONNECTION_ESTABLISHED, - Some(endpoint.connection_id()), ); let context = peers.entry(peer).or_insert_with(|| PeerContext { @@ -1422,7 +1417,6 @@ impl TransportManager { &peer, address, scores::CONNECTION_ESTABLISHED, - Some(connection_id), )); Ok(()) @@ -1510,7 +1504,6 @@ impl TransportManager { &peer, address, scores::CONNECTION_FAILURE, - None, )); } @@ -2682,12 +2675,7 @@ mod tests { state => panic!("invalid state: {state:?}"), }; - let dial_record = Some(AddressRecord::new( - &peer, - address2.clone(), - 0, - Some(ConnectionId::from(0usize)), - )); + let dial_record = Some(AddressRecord::new(&peer, address2.clone(), 0)); peer_context.state = PeerState::Connected { record, @@ -3899,13 +3887,8 @@ mod tests { let mut peers = manager.peers.write(); let state = PeerState::Connected { - record: AddressRecord::new(&peer, first_addr.clone(), 0, Some(first_connection_id)), - dial_record: Some(AddressRecord::new( - &peer, - first_addr.clone(), - 0, - Some(second_connection_id), - )), + record: AddressRecord::new(&peer, first_addr.clone(), 0), + dial_record: Some(AddressRecord::new(&peer, first_addr.clone(), 0)), }; let peer_context = PeerContext { From 526244b1130d364183be1047fce3b06e72c9b30a Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Thu, 19 Sep 2024 17:47:34 +0300 Subject: [PATCH 20/66] manager/types: Add wrappers for consturcting connection records Signed-off-by: Alexandru Vasile --- src/transport/manager/mod.rs | 50 ++++++++-------------------------- src/transport/manager/types.rs | 42 ++++++++++++++++++++++++++-- 2 files changed, 50 insertions(+), 42 deletions(-) diff --git a/src/transport/manager/mod.rs b/src/transport/manager/mod.rs index 1f585460..c6ccb472 100644 --- a/src/transport/manager/mod.rs +++ b/src/transport/manager/mod.rs @@ -1087,10 +1087,7 @@ impl TransportManager { let context = peers.entry(peer).or_insert_with(|| PeerContext { state: PeerState::Connected { - record: ConnectionRecord { - address: endpoint.address().clone(), - connection_id: endpoint.connection_id(), - }, + record: ConnectionRecord::from_endpoint(peer, endpoint), dial_record: None, }, addresses: AddressStore::new(), @@ -1162,10 +1159,8 @@ impl TransportManager { "dialed connection opened as secondary connection", ); - context.secondary_connection = Some(ConnectionRecord { - address: endpoint.address().clone(), - connection_id: endpoint.connection_id(), - }); + context.secondary_connection = + Some(ConnectionRecord::from_endpoint(peer, endpoint)); } None => { tracing::debug!( @@ -1176,10 +1171,8 @@ impl TransportManager { "secondary connection", ); - context.secondary_connection = Some(ConnectionRecord { - address: endpoint.address().clone(), - connection_id: endpoint.connection_id(), - }); + context.secondary_connection = + Some(ConnectionRecord::from_endpoint(peer, endpoint)); } Some(record) => { tracing::warn!( @@ -1225,10 +1218,7 @@ impl TransportManager { ); context.state = PeerState::Connected { - record: ConnectionRecord { - address: endpoint.address().clone(), - connection_id: endpoint.connection_id(), - }, + record: ConnectionRecord::from_endpoint(peer, endpoint), dial_record: Some(record.clone()), }; } @@ -1262,10 +1252,7 @@ impl TransportManager { self.pending_connections.remove(&connection_id); context.state = PeerState::Connected { - record: ConnectionRecord { - address: endpoint.address().clone(), - connection_id: endpoint.connection_id(), - }, + record: ConnectionRecord::from_endpoint(peer, endpoint), dial_record: None, }; } @@ -1287,20 +1274,11 @@ impl TransportManager { (dial_record, None) } else { ( - ConnectionRecord { - address: endpoint.address().clone(), - connection_id: endpoint.connection_id(), - }, + ConnectionRecord::from_endpoint(peer, endpoint), Some(dial_record), ) }, - None => ( - ConnectionRecord { - address: endpoint.address().clone(), - connection_id: endpoint.connection_id(), - }, - None, - ), + None => (ConnectionRecord::from_endpoint(peer, endpoint), None), }; context.state = PeerState::Connected { @@ -1314,10 +1292,7 @@ impl TransportManager { peer, PeerContext { state: PeerState::Connected { - record: ConnectionRecord { - address: endpoint.address().clone(), - connection_id: endpoint.connection_id(), - }, + record: ConnectionRecord::from_endpoint(peer, endpoint), dial_record: None, }, addresses: AddressStore::new(), @@ -1407,10 +1382,7 @@ impl TransportManager { self.pending_connections.insert(connection_id, peer); context.state = PeerState::Dialing { - record: ConnectionRecord { - address: address.clone(), - connection_id, - }, + record: ConnectionRecord::new(peer, address.clone(), connection_id), }; context.addresses.insert(AddressRecord::new( diff --git a/src/transport/manager/types.rs b/src/transport/manager/types.rs index 7812439c..59fba4c3 100644 --- a/src/transport/manager/types.rs +++ b/src/transport/manager/types.rs @@ -18,9 +18,14 @@ // FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER // DEALINGS IN THE SOFTWARE. -use crate::{transport::manager::address::AddressStore, types::ConnectionId}; +use crate::{ + transport::{manager::address::AddressStore, Endpoint}, + types::ConnectionId, + PeerId, +}; -use multiaddr::Multiaddr; +use multiaddr::{Multiaddr, Protocol}; +use multihash::Multihash; use std::collections::HashSet; @@ -106,13 +111,44 @@ pub enum PeerState { #[allow(clippy::derived_hash_with_manual_eq)] #[derive(Debug, Clone, Hash)] pub struct ConnectionRecord { - /// Address that was dialed. + /// Address of the connection. + /// + /// The address must contain the peer ID extension `/p2p/`. pub address: Multiaddr, /// Connection ID resulted from dialing. pub connection_id: ConnectionId, } +impl ConnectionRecord { + /// Construct a new connection record. + pub fn new(peer: PeerId, address: Multiaddr, connection_id: ConnectionId) -> Self { + Self { + address: Self::ensure_peer_id(peer, address), + connection_id, + } + } + + /// Create a new connection record from the peer ID and the endpoint. + pub fn from_endpoint(peer: PeerId, endpoint: &Endpoint) -> Self { + Self { + address: Self::ensure_peer_id(peer, endpoint.address().clone()), + connection_id: endpoint.connection_id(), + } + } + + /// Ensures the peer ID is present in the address. + fn ensure_peer_id(peer: PeerId, address: Multiaddr) -> Multiaddr { + if !std::matches!(address.iter().last(), Some(Protocol::P2p(_))) { + address.with(Protocol::P2p( + Multihash::from_bytes(&peer.to_bytes()).expect("valid peer id"), + )) + } else { + address + } + } +} + /// Peer context. #[derive(Debug)] pub struct PeerContext { From dcac138b27ab7a5998b836912ed261ad0670b693 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Thu, 19 Sep 2024 18:12:29 +0300 Subject: [PATCH 21/66] manager/address: Implement eviction algorithm Signed-off-by: Alexandru Vasile --- src/transport/manager/address.rs | 35 ++++++++++++++++++++++++++------ 1 file changed, 29 insertions(+), 6 deletions(-) diff --git a/src/transport/manager/address.rs b/src/transport/manager/address.rs index 201f1f02..c6455a1e 100644 --- a/src/transport/manager/address.rs +++ b/src/transport/manager/address.rs @@ -176,15 +176,38 @@ impl AddressStore { /// If the address is not in the store, it will be inserted. /// Otherwise, the score and connection ID will be updated. pub fn insert(&mut self, record: AddressRecord) { - match self.addresses.entry(record.address.clone()) { - std::collections::hash_map::Entry::Occupied(occupied_entry) => { - let found_record = occupied_entry.into_mut(); - found_record.update_score(record.score); + let num_addresses = self.addresses.len(); + + if let Some(occupied) = self.addresses.get_mut(record.address()) { + occupied.update_score(record.score); + return; + } + + // The eviction algorithm favours addresses with higher scores. + // + // This algorithm has the following implications: + // - it keeps the best addresses in the store. + // - if the store is at capacity, the worst address will be evicted. + // - an address that is not dialed yet (with score zero) will be preferred over an address + // that already failed (with negative score). + if num_addresses > MAX_ADDRESSES { + // No need to keep track of negative addresses if we are at capacity. + if record.score < 0 { + return; } - std::collections::hash_map::Entry::Vacant(vacant_entry) => { - vacant_entry.insert(record.clone()); + + let Some(min_record) = self.addresses.values().min().cloned() else { + return; + }; + // The lowest score is better than the new record. + if record.score < min_record.score { + return; } + self.addresses.remove(min_record.address()); } + + // Insert the record. + self.addresses.insert(record.address.clone(), record); } /// Return the available addresses sorted by score. From fc3576292bd7036fe66a33022318d54e2e230149 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Thu, 19 Sep 2024 18:27:29 +0300 Subject: [PATCH 22/66] manager: Rename recrod to address record Signed-off-by: Alexandru Vasile --- src/transport/manager/mod.rs | 51 ++++++++++++++++++++---------------- 1 file changed, 29 insertions(+), 22 deletions(-) diff --git a/src/transport/manager/mod.rs b/src/transport/manager/mod.rs index c6ccb472..a4fcb6e7 100644 --- a/src/transport/manager/mod.rs +++ b/src/transport/manager/mod.rs @@ -601,19 +601,19 @@ impl TransportManager { pub async fn dial_address(&mut self, address: Multiaddr) -> crate::Result<()> { self.connection_limits.on_dial_address()?; - let record = AddressRecord::from_multiaddr(address) + let address_record = AddressRecord::from_multiaddr(address) .ok_or(Error::AddressError(AddressError::PeerIdMissing))?; - if self.listen_addresses.read().contains(record.as_ref()) { + if self.listen_addresses.read().contains(address_record.as_ref()) { return Err(Error::TriedToDialSelf); } - tracing::debug!(target: LOG_TARGET, address = ?record.address(), "dial address"); + tracing::debug!(target: LOG_TARGET, address = ?address_record.address(), "dial address"); - let mut protocol_stack = record.as_ref().iter(); + let mut protocol_stack = address_record.as_ref().iter(); match protocol_stack .next() - .ok_or_else(|| Error::TransportNotSupported(record.address().clone()))? + .ok_or_else(|| Error::TransportNotSupported(address_record.address().clone()))? { Protocol::Ip4(_) | Protocol::Ip6(_) => {} Protocol::Dns(_) | Protocol::Dns4(_) | Protocol::Dns6(_) => {} @@ -623,28 +623,33 @@ impl TransportManager { ?transport, "invalid transport, expected `ip4`/`ip6`" ); - return Err(Error::TransportNotSupported(record.address().clone())); + return Err(Error::TransportNotSupported( + address_record.address().clone(), + )); } }; let supported_transport = match protocol_stack .next() - .ok_or_else(|| Error::TransportNotSupported(record.address().clone()))? + .ok_or_else(|| Error::TransportNotSupported(address_record.address().clone()))? { Protocol::Tcp(_) => match protocol_stack.next() { #[cfg(feature = "websocket")] Some(Protocol::Ws(_)) | Some(Protocol::Wss(_)) => SupportedTransport::WebSocket, Some(Protocol::P2p(_)) => SupportedTransport::Tcp, - _ => return Err(Error::TransportNotSupported(record.address().clone())), + _ => + return Err(Error::TransportNotSupported( + address_record.address().clone(), + )), }, #[cfg(feature = "quic")] Protocol::Udp(_) => match protocol_stack .next() - .ok_or_else(|| Error::TransportNotSupported(record.address().clone()))? + .ok_or_else(|| Error::TransportNotSupported(address_record.address().clone()))? { Protocol::QuicV1 => SupportedTransport::Quic, _ => { - tracing::debug!(target: LOG_TARGET, address = ?record.address(), "expected `quic-v1`"); + tracing::debug!(target: LOG_TARGET, address = ?address_record.address(), "expected `quic-v1`"); return Err(Error::TransportNotSupported(record.address().clone())); } }, @@ -655,31 +660,31 @@ impl TransportManager { "invalid protocol" ); - return Err(Error::TransportNotSupported(record.address().clone())); + return Err(Error::TransportNotSupported( + address_record.address().clone(), + )); } }; // when constructing `AddressRecord`, `PeerId` was verified to be part of the address let remote_peer_id = - PeerId::try_from_multiaddr(record.address()).expect("`PeerId` to exist"); + PeerId::try_from_multiaddr(address_record.address()).expect("`PeerId` to exist"); // set connection id for the address record and put peer into `Dialing` state let connection_id = self.next_connection_id(); + let dial_record = ConnectionRecord { + address: address_record.address().clone(), + connection_id, + }; { let mut peers = self.peers.write(); - - let dial_record = ConnectionRecord { - address: record.address().clone(), - connection_id, - }; - match peers.entry(remote_peer_id) { Entry::Occupied(occupied) => { let context = occupied.into_mut(); // Keep the provided record around for possible future dials. - context.addresses.insert(record.clone()); + context.addresses.insert(address_record.clone()); tracing::debug!( target: LOG_TARGET, @@ -718,7 +723,7 @@ impl TransportManager { state: PeerState::Dialing { record: dial_record, }, - addresses: AddressStore::from_iter(std::iter::once(record.clone())), + addresses: AddressStore::from_iter(std::iter::once(address_record.clone())), secondary_connection: None, }); } @@ -727,8 +732,10 @@ impl TransportManager { self.transports .get_mut(&supported_transport) - .ok_or(Error::TransportNotSupported(record.address().clone()))? - .dial(connection_id, record.address().clone())?; + .ok_or(Error::TransportNotSupported( + address_record.address().clone(), + ))? + .dial(connection_id, address_record.address().clone())?; self.pending_connections.insert(connection_id, remote_peer_id); Ok(()) From 250c289702e394b37074b975c6aa5efc50778b5e Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Thu, 19 Sep 2024 18:33:33 +0300 Subject: [PATCH 23/66] manager/handle: Keep WriteLock for a shorter time Signed-off-by: Alexandru Vasile --- src/transport/manager/handle.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/transport/manager/handle.rs b/src/transport/manager/handle.rs index 88f332a2..c29aa793 100644 --- a/src/transport/manager/handle.rs +++ b/src/transport/manager/handle.rs @@ -180,8 +180,6 @@ impl TransportManagerHandle { peer: &PeerId, addresses: impl Iterator, ) -> usize { - let mut peers = self.peers.write(); - let mut peer_addresses = HashMap::new(); for address in addresses { @@ -221,6 +219,7 @@ impl TransportManagerHandle { "add known addresses", ); + let mut peers = self.peers.write(); for (peer, addresses) in peer_addresses { let entry = peers.entry(peer).or_insert_with(|| PeerContext { state: PeerState::Disconnected { dial_record: None }, From 6da7acf124ec975717bf99f85b9147bf92a13727 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Thu, 19 Sep 2024 18:34:46 +0300 Subject: [PATCH 24/66] manager/peer_state: Implement on_dial state advancing Signed-off-by: Alexandru Vasile --- src/transport/manager/handle.rs | 6 +--- src/transport/manager/mod.rs | 53 +++++---------------------------- src/transport/manager/types.rs | 46 +++++++++++++++++++++++++++- 3 files changed, 53 insertions(+), 52 deletions(-) diff --git a/src/transport/manager/handle.rs b/src/transport/manager/handle.rs index c29aa793..6f3ca3fe 100644 --- a/src/transport/manager/handle.rs +++ b/src/transport/manager/handle.rs @@ -221,11 +221,7 @@ impl TransportManagerHandle { let mut peers = self.peers.write(); for (peer, addresses) in peer_addresses { - let entry = peers.entry(peer).or_insert_with(|| PeerContext { - state: PeerState::Disconnected { dial_record: None }, - addresses: AddressStore::new(), - secondary_connection: None, - }); + let entry = peers.entry(peer).or_insert_with(|| PeerContext::default()); // All addresses should be valid at this point, since the peer ID was either added or // double checked. diff --git a/src/transport/manager/mod.rs b/src/transport/manager/mod.rs index a4fcb6e7..9e3cfe8b 100644 --- a/src/transport/manager/mod.rs +++ b/src/transport/manager/mod.rs @@ -679,55 +679,16 @@ impl TransportManager { { let mut peers = self.peers.write(); - match peers.entry(remote_peer_id) { - Entry::Occupied(occupied) => { - let context = occupied.into_mut(); - // Keep the provided record around for possible future dials. - context.addresses.insert(address_record.clone()); + let context = peers.entry(remote_peer_id).or_insert_with(|| PeerContext::default()); - tracing::debug!( - target: LOG_TARGET, - peer = ?remote_peer_id, - state = ?context.state, - "peer state exists", - ); + // Keep the provided record around for possible future dials. + context.addresses.insert(address_record.clone()); - match context.state { - PeerState::Connected { .. } => { - return Err(Error::AlreadyConnected); - } - PeerState::Dialing { .. } | PeerState::Opening { .. } => { - return Ok(()); - } - PeerState::Disconnected { - dial_record: Some(_), - } => { - tracing::debug!( - target: LOG_TARGET, - peer = ?remote_peer_id, - state = ?context.state, - "peer is already being dialed from a disconnected state" - ); - return Ok(()); - } - PeerState::Disconnected { dial_record: None } => { - context.state = PeerState::Dialing { - record: dial_record, - }; - } - } - } - Entry::Vacant(vacant) => { - vacant.insert(PeerContext { - state: PeerState::Dialing { - record: dial_record, - }, - addresses: AddressStore::from_iter(std::iter::once(address_record.clone())), - secondary_connection: None, - }); - } - }; + // Dialing from an invalid state, or another dial is in progress. + if let Some(immediate_result) = context.state.on_dial_record(dial_record) { + return immediate_result; + } } self.transports diff --git a/src/transport/manager/types.rs b/src/transport/manager/types.rs index 59fba4c3..99b3631c 100644 --- a/src/transport/manager/types.rs +++ b/src/transport/manager/types.rs @@ -21,7 +21,7 @@ use crate::{ transport::{manager::address::AddressStore, Endpoint}, types::ConnectionId, - PeerId, + Error, PeerId, }; use multiaddr::{Multiaddr, Protocol}; @@ -96,6 +96,40 @@ pub enum PeerState { }, } +impl PeerState { + /// Advances the peer state on a dial attempt. + /// The dialing is happing on a single address. + /// + /// Provides a response if the dialing should return immediately. + /// + /// # Transitions + /// + /// [`PeerState::Disconnected`] -> [`PeerState::Dialing`] + pub fn on_dial_record(&mut self, dial_record: ConnectionRecord) -> Option> { + match self { + // The peer is already connected, no need to dial a second time. + Self::Connected { .. } => { + return Some(Err(Error::AlreadyConnected)); + } + // The dialing state is already in progress, an event will be emitted later. + Self::Dialing { .. } + | Self::Opening { .. } + | Self::Disconnected { + dial_record: Some(_), + } => { + return Some(Ok(())); + } + // The peer is disconnected, start dialing. + Self::Disconnected { dial_record: None } => { + *self = Self::Dialing { + record: dial_record, + }; + return None; + } + } + } +} + /// The connection record keeps track of the connection ID and the address of the connection. /// /// The connection ID is used to track the connection in the transport layer. @@ -161,3 +195,13 @@ pub struct PeerContext { /// Known addresses of peer. pub addresses: AddressStore, } + +impl Default for PeerContext { + fn default() -> Self { + Self { + state: PeerState::Disconnected { dial_record: None }, + secondary_connection: None, + addresses: AddressStore::new(), + } + } +} From b26dbd137059fbf0628fa45717999e64ae56eb97 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Thu, 19 Sep 2024 19:42:52 +0300 Subject: [PATCH 25/66] manager: Type safe state transition and trransports more modular Signed-off-by: Alexandru Vasile --- src/transport/manager/address.rs | 4 +- src/transport/manager/handle.rs | 2 +- src/transport/manager/mod.rs | 202 +++++++++---------------------- src/transport/manager/types.rs | 77 +++++++++--- 4 files changed, 121 insertions(+), 164 deletions(-) diff --git a/src/transport/manager/address.rs b/src/transport/manager/address.rs index c6455a1e..6f0b9f82 100644 --- a/src/transport/manager/address.rs +++ b/src/transport/manager/address.rs @@ -211,10 +211,10 @@ impl AddressStore { } /// Return the available addresses sorted by score. - pub fn addresses(&self) -> Vec { + pub fn addresses(&self, limit: usize) -> Vec { let mut records = self.addresses.values().cloned().collect::>(); records.sort_by(|lhs, rhs| rhs.score.cmp(&lhs.score)); - records + records.into_iter().take(limit).map(|record| record.address).collect() } } diff --git a/src/transport/manager/handle.rs b/src/transport/manager/handle.rs index 6f3ca3fe..a6a907e9 100644 --- a/src/transport/manager/handle.rs +++ b/src/transport/manager/handle.rs @@ -25,7 +25,7 @@ use crate::{ executor::Executor, protocol::ProtocolSet, transport::manager::{ - address::{AddressRecord, AddressStore}, + address::AddressRecord, types::{PeerContext, PeerState, SupportedTransport}, ProtocolContext, TransportManagerEvent, LOG_TARGET, }, diff --git a/src/transport/manager/mod.rs b/src/transport/manager/mod.rs index 9e3cfe8b..6d2dccbb 100644 --- a/src/transport/manager/mod.rs +++ b/src/transport/manager/mod.rs @@ -45,7 +45,7 @@ use parking_lot::RwLock; use tokio::sync::mpsc::{channel, Receiver, Sender}; use std::{ - collections::{hash_map::Entry, HashMap, HashSet}, + collections::{HashMap, HashSet}, pin::Pin, sync::{ atomic::{AtomicUsize, Ordering}, @@ -328,7 +328,7 @@ impl TransportManager { } /// Get next connection ID. - fn next_connection_id(&mut self) -> ConnectionId { + fn next_connection_id(&self) -> ConnectionId { let connection_id = self.next_connection_id.fetch_add(1usize, Ordering::Relaxed); ConnectionId::from(connection_id) @@ -423,6 +423,29 @@ impl TransportManager { self.transport_manager_handle.add_known_address(&peer, address) } + /// Return multiple addresses to dial on supported protocols. + fn open_addresses(addresses: &[Multiaddr]) -> HashMap> { + let mut transports = HashMap::>::new(); + + for address in addresses.iter().cloned() { + #[cfg(feature = "quic")] + if address.iter().any(|p| std::matches!(&p, Protocol::QuicV1)) { + transports.entry(SupportedTransport::Quic).or_default().push(address); + continue; + } + + #[cfg(feature = "websocket")] + if address.iter().any(|p| std::matches!(&p, Protocol::Ws(_) | Protocol::Wss(_))) { + transports.entry(SupportedTransport::WebSocket).or_default().push(address); + continue; + } + + transports.entry(SupportedTransport::Tcp).or_default().push(address); + } + + transports + } + /// Dial peer using `PeerId`. /// /// Returns an error if the peer is unknown or the peer is already connected. @@ -438,156 +461,46 @@ impl TransportManager { } let mut peers = self.peers.write(); - // if the peer is disconnected, return its context - // - // otherwise set the state back what it was and return dial status to caller - let PeerContext { - state, - secondary_connection, - addresses, - } = match peers.remove(&peer) { - None => return Err(Error::PeerDoesntExist(peer)), - Some( - context @ PeerContext { - state: PeerState::Connected { .. }, - .. - }, - ) => { - peers.insert(peer, context); - return Err(Error::AlreadyConnected); - } - Some( - context @ PeerContext { - state: PeerState::Dialing { .. } | PeerState::Opening { .. }, - .. - }, - ) => { - peers.insert(peer, context); - return Ok(()); - } - Some(context) => context, - }; - - if let PeerState::Disconnected { - dial_record: Some(_), - } = &state - { - tracing::debug!( - target: LOG_TARGET, - ?peer, - "peer is already being dialed", - ); - - peers.insert( - peer, - PeerContext { - state, - secondary_connection, - addresses, - }, - ); - - return Ok(()); - } + let context = peers.entry(peer).or_insert_with(|| PeerContext::default()); - let records: HashSet<_> = addresses - .addresses() - .into_iter() - .take(limit) - .map(|record| record.address().clone()) - .collect(); + let disconnected_state = match context.state.initiate_dial() { + Ok(disconnected_peer) => disconnected_peer, + // Dialing from an invalid state, or another dial is in progress. + Err(immediate_result) => return immediate_result, + }; - if records.is_empty() { + // The addresses are sorted by score and contain the remote peer ID. + // We double checked above that the remote peer is not the local peer. + let dial_addresses = context.addresses.addresses(limit); + if dial_addresses.is_empty() { return Err(Error::NoAddressAvailable(peer)); } - - let locked_addresses = self.listen_addresses.read(); - for record in &records { - if locked_addresses.contains(record) { - tracing::warn!( - target: LOG_TARGET, - ?peer, - ?record, - "tried to dial self", - ); - - debug_assert!(false); - return Err(Error::TriedToDialSelf); - } - } - drop(locked_addresses); - - // set connection id for the address record and put peer into `Opening` state - let connection_id = - ConnectionId::from(self.next_connection_id.fetch_add(1usize, Ordering::Relaxed)); + let connection_id = self.next_connection_id(); tracing::debug!( target: LOG_TARGET, ?connection_id, - addresses = ?records, + addresses = ?dial_addresses, "dial remote peer", ); - let mut transports = HashSet::new(); - #[cfg(feature = "websocket")] - let mut websocket = Vec::new(); - #[cfg(feature = "quic")] - let mut quic = Vec::new(); - let mut tcp = Vec::new(); + let transports = Self::open_addresses(&dial_addresses); + disconnected_state.dial_addresses( + connection_id, + dial_addresses.iter().cloned().collect(), + transports.keys().cloned().collect(), + ); - for address in &records { - #[cfg(feature = "quic")] - if address.iter().any(|p| std::matches!(&p, Protocol::QuicV1)) { - quic.push(address.clone()); - transports.insert(SupportedTransport::Quic); + for (transport, addresses) in transports { + if addresses.is_empty() { continue; } - #[cfg(feature = "websocket")] - if address.iter().any(|p| std::matches!(&p, Protocol::Ws(_) | Protocol::Wss(_))) { - websocket.push(address.clone()); - transports.insert(SupportedTransport::WebSocket); + let Some(installed_transport) = self.transports.get_mut(&transport) else { continue; - } - - tcp.push(address.clone()); - transports.insert(SupportedTransport::Tcp); - } - - peers.insert( - peer, - PeerContext { - state: PeerState::Opening { - records, - connection_id, - transports, - }, - secondary_connection, - addresses, - }, - ); - - if !tcp.is_empty() { - self.transports - .get_mut(&SupportedTransport::Tcp) - .expect("transport to be supported") - .open(connection_id, tcp)?; - } - - #[cfg(feature = "quic")] - if !quic.is_empty() { - self.transports - .get_mut(&SupportedTransport::Quic) - .expect("transport to be supported") - .open(connection_id, quic)?; - } + }; - #[cfg(feature = "websocket")] - if !websocket.is_empty() { - self.transports - .get_mut(&SupportedTransport::WebSocket) - .expect("transport to be supported") - .open(connection_id, websocket)?; + installed_transport.open(connection_id, addresses)?; } self.pending_connections.insert(connection_id, peer); @@ -685,10 +598,11 @@ impl TransportManager { // Keep the provided record around for possible future dials. context.addresses.insert(address_record.clone()); - // Dialing from an invalid state, or another dial is in progress. - if let Some(immediate_result) = context.state.on_dial_record(dial_record) { - return immediate_result; - } + match context.state.initiate_dial() { + Ok(disconnected_peer) => disconnected_peer.dial_record(dial_record), + // Dialing from an invalid state, or another dial is in progress. + Err(immediate_result) => return immediate_result, + }; } self.transports @@ -1193,7 +1107,7 @@ impl TransportManager { } } PeerState::Opening { - ref mut records, + ref mut addresses, connection_id, ref transports, } => { @@ -1201,7 +1115,7 @@ impl TransportManager { target: LOG_TARGET, ?peer, dial_connection_id = ?connection_id, - dial_records = ?records, + dial_records = ?addresses, dial_transports = ?transports, listener_endpoint = ?endpoint, "inbound connection while opening an outbound connection", @@ -1425,7 +1339,7 @@ impl TransportManager { PeerState::Disconnected { dial_record: None }, ) { PeerState::Opening { - records, + addresses, connection_id, mut transports, } => { @@ -1439,7 +1353,7 @@ impl TransportManager { transports.remove(&transport); if transports.is_empty() { - for address in records { + for address in addresses { context.addresses.insert(AddressRecord::new( &peer, address, @@ -1459,7 +1373,7 @@ impl TransportManager { self.pending_connections.insert(connection_id, peer); context.state = PeerState::Opening { - records, + addresses, connection_id, transports, }; diff --git a/src/transport/manager/types.rs b/src/transport/manager/types.rs index 99b3631c..102b66b8 100644 --- a/src/transport/manager/types.rs +++ b/src/transport/manager/types.rs @@ -68,7 +68,7 @@ pub enum PeerState { /// Connection to peer is opening over one or more addresses. Opening { /// Address records used for dialing. - records: HashSet, + addresses: HashSet, /// Connection ID. connection_id: ConnectionId, @@ -96,20 +96,19 @@ pub enum PeerState { }, } +pub type InitiateDialError = Result<(), Error>; + impl PeerState { - /// Advances the peer state on a dial attempt. - /// The dialing is happing on a single address. - /// - /// Provides a response if the dialing should return immediately. - /// - /// # Transitions + /// Provides a disconnected state object if the peer can initiate a dial. /// - /// [`PeerState::Disconnected`] -> [`PeerState::Dialing`] - pub fn on_dial_record(&mut self, dial_record: ConnectionRecord) -> Option> { + /// From the disconnected state, the peer can be dialed on a single address or multiple + /// addresses. The provided state leverages the type system to ensure the peer + /// can transition gracefully to the next state. + pub fn initiate_dial(&mut self) -> Result { match self { // The peer is already connected, no need to dial a second time. Self::Connected { .. } => { - return Some(Err(Error::AlreadyConnected)); + return Err(Err(Error::AlreadyConnected)); } // The dialing state is already in progress, an event will be emitted later. Self::Dialing { .. } @@ -117,19 +116,63 @@ impl PeerState { | Self::Disconnected { dial_record: Some(_), } => { - return Some(Ok(())); + return Err(Ok(())); } // The peer is disconnected, start dialing. - Self::Disconnected { dial_record: None } => { - *self = Self::Dialing { - record: dial_record, - }; - return None; - } + Self::Disconnected { dial_record: None } => return Ok(DisconnectedState::new(self)), } } } +pub struct DisconnectedState<'a> { + state: &'a mut PeerState, +} + +impl<'a> DisconnectedState<'a> { + /// Constructs a new [`DisconnectedState`]. + /// + /// # Panics + /// + /// Panics if the state is not [`PeerState::Disconnected`]. + fn new(state: &'a mut PeerState) -> Self { + assert!(matches!( + state, + PeerState::Disconnected { dial_record: None } + )); + + Self { state } + } + + /// Dial the peer on a single address. + /// + /// # Transitions + /// + /// [`PeerState::Disconnected`] -> [`PeerState::Dialing`] + pub fn dial_record(self, dial_record: ConnectionRecord) { + *self.state = PeerState::Dialing { + record: dial_record, + }; + } + + /// Dial the peer on multiple addresses. + /// + /// # Transitions + /// + /// [`PeerState::Disconnected`] -> [`PeerState::Opening`] + pub fn dial_addresses( + self, + connection_id: ConnectionId, + addresses: HashSet, + transports: HashSet, + ) { + *self.state = PeerState::Opening { + addresses, + connection_id, + transports, + }; + } +} + /// The connection record keeps track of the connection ID and the address of the connection. /// /// The connection ID is used to track the connection in the transport layer. From 90e5b454b577b3e797ac443219a9c13ff6ed3cf3 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Thu, 19 Sep 2024 19:57:13 +0300 Subject: [PATCH 26/66] manager/peer_state: Implement on_dial_failure state advancing Signed-off-by: Alexandru Vasile --- src/transport/manager/mod.rs | 125 +++------------------------------ src/transport/manager/types.rs | 44 ++++++++++++ 2 files changed, 54 insertions(+), 115 deletions(-) diff --git a/src/transport/manager/mod.rs b/src/transport/manager/mod.rs index 6d2dccbb..dca526f3 100644 --- a/src/transport/manager/mod.rs +++ b/src/transport/manager/mod.rs @@ -634,11 +634,7 @@ impl TransportManager { // Note: this is happening quite often in practice and is the primary reason // of negotiation failures. NegotiationError::PeerIdMismatch(_, provided) => { - let context = peers.entry(*provided).or_insert_with(|| PeerContext { - state: PeerState::Disconnected { dial_record: None }, - addresses: AddressStore::new(), - secondary_connection: None, - }); + let context = peers.entry(*provided).or_insert_with(|| PeerContext::default()); if !std::matches!(address.iter().last(), Some(Protocol::P2p(_))) { address.pop(); @@ -669,11 +665,7 @@ impl TransportManager { }; // We need a valid context for this peer to keep track of failed addresses. - let context = peers.entry(peer_id).or_insert_with(|| PeerContext { - state: PeerState::Disconnected { dial_record: None }, - addresses: AddressStore::new(), - secondary_connection: None, - }); + let context = peers.entry(peer_id).or_insert_with(|| PeerContext::default()); context.addresses.insert(AddressRecord::new(&peer_id, address.clone(), score)); } @@ -691,116 +683,19 @@ impl TransportManager { })?; let mut peers = self.peers.write(); - let context = peers.get_mut(&peer).ok_or_else(|| { - tracing::error!( + let context = peers.entry(peer).or_insert_with(|| PeerContext::default()); + + if !context.state.on_dial_failure(connection_id) { + tracing::warn!( target: LOG_TARGET, ?peer, ?connection_id, - "dial failed for a peer that doesn't exist", + state = ?context.state, + "invalid state for dial failure", ); - debug_assert!(false); - - Error::InvalidState - })?; - - match std::mem::replace( - &mut context.state, - PeerState::Disconnected { dial_record: None }, - ) { - PeerState::Dialing { ref mut record } => { - if record.connection_id != connection_id { - tracing::warn!( - target: LOG_TARGET, - ?peer, - ?connection_id, - ?record, - "unknown dial failure for a dialing peer", - ); - - context.state = PeerState::Dialing { - record: record.clone(), - }; - debug_assert!(false); - return Ok(()); - } - - context.state = PeerState::Disconnected { dial_record: None }; - Ok(()) - } - PeerState::Opening { .. } => { - todo!(); - } - PeerState::Connected { - record, - dial_record: Some(dial_record), - } => { - if dial_record.connection_id != connection_id { - tracing::warn!( - target: LOG_TARGET, - ?peer, - ?connection_id, - ?record, - "unknown dial failure for a connected peer", - ); - - context.state = PeerState::Connected { - record, - dial_record: Some(dial_record), - }; - debug_assert!(false); - return Ok(()); - } - - context.state = PeerState::Connected { - record, - dial_record: None, - }; - Ok(()) - } - PeerState::Disconnected { - dial_record: Some(dial_record), - } => { - tracing::debug!( - target: LOG_TARGET, - ?connection_id, - ?dial_record, - "dial failed for a disconnected peer", - ); - - if dial_record.connection_id != connection_id { - tracing::warn!( - target: LOG_TARGET, - ?peer, - ?connection_id, - ?dial_record, - "unknown dial failure for a disconnected peer", - ); - - context.state = PeerState::Disconnected { - dial_record: Some(dial_record), - }; - debug_assert!(false); - return Ok(()); - } - - context.state = PeerState::Disconnected { dial_record: None }; - - Ok(()) - } - state => { - tracing::warn!( - target: LOG_TARGET, - ?peer, - ?connection_id, - ?state, - "invalid state for dial failure", - ); - context.state = state; - - debug_assert!(false); - Ok(()) - } } + + Ok(()) } fn on_pending_incoming_connection(&mut self) -> crate::Result<()> { diff --git a/src/transport/manager/types.rs b/src/transport/manager/types.rs index 102b66b8..6a37c4d4 100644 --- a/src/transport/manager/types.rs +++ b/src/transport/manager/types.rs @@ -118,10 +118,54 @@ impl PeerState { } => { return Err(Ok(())); } + // The peer is disconnected, start dialing. Self::Disconnected { dial_record: None } => return Ok(DisconnectedState::new(self)), } } + + /// Handle dial failure. + /// + /// Returns `true` if the dial record was cleared, false otherwise. + /// + /// # Transitions + /// - [`PeerState::Dialing`] (with record) -> [`PeerState::Disconnected`] + /// - [`PeerState::Connected`] (with dial record) -> [`PeerState::Connected`] + /// - [`PeerState::Disconnected`] (with dial record) -> [`PeerState::Disconnected`] + pub fn on_dial_failure(&mut self, connection_id: ConnectionId) -> bool { + match self { + // Clear the dial record if the connection ID matches. + Self::Dialing { record } => + if record.connection_id == connection_id { + *self = Self::Disconnected { dial_record: None }; + return true; + }, + + Self::Connected { + record, + dial_record: Some(dial_record), + } => + if dial_record.connection_id == connection_id { + *self = Self::Connected { + record: record.clone(), + dial_record: None, + }; + return true; + }, + + Self::Disconnected { + dial_record: Some(dial_record), + } => + if dial_record.connection_id == connection_id { + *self = Self::Disconnected { dial_record: None }; + return true; + }, + + _ => (), + }; + + return false; + } } pub struct DisconnectedState<'a> { From 5cd79491a4b29efef1d11d5f31db578e216c0f7d Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 20 Sep 2024 14:08:00 +0300 Subject: [PATCH 27/66] manager/peer_state: Merge secondary connection with PeerState Signed-off-by: Alexandru Vasile --- src/transport/manager/types.rs | 97 ++++++++++++++++++++++++++++++---- 1 file changed, 88 insertions(+), 9 deletions(-) diff --git a/src/transport/manager/types.rs b/src/transport/manager/types.rs index 6a37c4d4..ff452211 100644 --- a/src/transport/manager/types.rs +++ b/src/transport/manager/types.rs @@ -19,7 +19,7 @@ // DEALINGS IN THE SOFTWARE. use crate::{ - transport::{manager::address::AddressStore, Endpoint}, + transport::{common::listener::DialAddresses, manager::address::AddressStore, Endpoint}, types::ConnectionId, Error, PeerId, }; @@ -56,13 +56,13 @@ pub enum PeerState { /// The established record of the connection. record: ConnectionRecord, - /// Dial address, if it exists. + /// Secondary record, this can either be a dial record or an established connection. /// /// While the local node was dialing a remote peer, the remote peer might've dialed /// the local node and connection was established successfully. This dial address /// is stored for processing later when the dial attempt concluded as either /// successful/failed. - dial_record: Option, + secondary: Option, }, /// Connection to peer is opening over one or more addresses. @@ -80,7 +80,7 @@ pub enum PeerState { /// Peer is being dialed. Dialing { /// Address record. - record: ConnectionRecord, + dial_record: ConnectionRecord, }, /// `Litep2p` is not connected to peer. @@ -96,6 +96,15 @@ pub enum PeerState { }, } +/// The state of the secondary connection. +#[derive(Debug)] +pub enum SecondaryOrDialing { + /// The secondary connection is established. + Secondary(ConnectionRecord), + /// The primary connection is established, but the secondary connection is still dialing. + Dialing(ConnectionRecord), +} + pub type InitiateDialError = Result<(), Error>; impl PeerState { @@ -106,7 +115,7 @@ impl PeerState { /// can transition gracefully to the next state. pub fn initiate_dial(&mut self) -> Result { match self { - // The peer is already connected, no need to dial a second time. + // The peer is already connected, no need to dial again. Self::Connected { .. } => { return Err(Err(Error::AlreadyConnected)); } @@ -135,20 +144,20 @@ impl PeerState { pub fn on_dial_failure(&mut self, connection_id: ConnectionId) -> bool { match self { // Clear the dial record if the connection ID matches. - Self::Dialing { record } => - if record.connection_id == connection_id { + Self::Dialing { dial_record } => + if dial_record.connection_id == connection_id { *self = Self::Disconnected { dial_record: None }; return true; }, Self::Connected { record, - dial_record: Some(dial_record), + secondary: Some(SecondaryOrDialing::Dialing(dial_record)), } => if dial_record.connection_id == connection_id { *self = Self::Connected { record: record.clone(), - dial_record: None, + secondary: None, }; return true; }, @@ -166,6 +175,76 @@ impl PeerState { return false; } + + /// Returns `true` if the connection should be accepted by the transport manager. + pub fn on_connection_established( + &mut self, + connection: ConnectionRecord, + ) -> (bool, Option<(ConnectionId, HashSet)>) { + match self { + // Transform the dial record into a secondary connection. + Self::Connected { + secondary: Some(SecondaryOrDialing::Dialing(dial_record)), + .. + } => + if dial_record.connection_id == connection.connection_id { + *self = Self::Connected { + record: connection.clone(), + secondary: Some(SecondaryOrDialing::Secondary(connection)), + }; + + return (true, None); + }, + // There's place for a secondary connection. + Self::Connected { + secondary: None, .. + } => { + *self = Self::Connected { + record: connection.clone(), + secondary: Some(SecondaryOrDialing::Secondary(connection)), + }; + + return (true, None); + } + + // Convert the dial record into a primary connection or preserve it. + Self::Dialing { dial_record } + | Self::Disconnected { + dial_record: Some(dial_record), + } => + if dial_record.connection_id == connection.connection_id { + *self = Self::Connected { + record: connection.clone(), + secondary: None, + }; + return (true, None); + } else { + *self = Self::Connected { + record: connection, + secondary: Some(SecondaryOrDialing::Dialing(dial_record.clone())), + }; + return (true, None); + }, + + // Accept the incoming connection and cancel opening dials. + Self::Opening { + addresses, + connection_id, + transports, + } => { + *self = Self::Connected { + record: connection, + secondary: None, + }; + + return (true, Some((*connection_id, transports.clone()))); + } + + _ => {} + }; + + return (false, None); + } } pub struct DisconnectedState<'a> { From 3804db4e06fff3c2709eb00d7399d6a143419f9f Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 20 Sep 2024 14:18:19 +0300 Subject: [PATCH 28/66] manager/peer_state: Handle connection closed transition Signed-off-by: Alexandru Vasile --- src/transport/manager/types.rs | 54 +++++++++++++++++++++++++++++++--- 1 file changed, 50 insertions(+), 4 deletions(-) diff --git a/src/transport/manager/types.rs b/src/transport/manager/types.rs index ff452211..6c499774 100644 --- a/src/transport/manager/types.rs +++ b/src/transport/manager/types.rs @@ -19,7 +19,7 @@ // DEALINGS IN THE SOFTWARE. use crate::{ - transport::{common::listener::DialAddresses, manager::address::AddressStore, Endpoint}, + transport::{manager::address::AddressStore, Endpoint}, types::ConnectionId, Error, PeerId, }; @@ -245,6 +245,54 @@ impl PeerState { return (false, None); } + + /// Returns `true` if the connection was closed and the state was updated. + pub fn on_connection_closed(&mut self, connection_id: ConnectionId) -> bool { + match self { + Self::Connected { record, secondary } => { + // Primary connection closed. + if record.connection_id == connection_id { + match secondary { + // Promote secondary connection to primary. + Some(SecondaryOrDialing::Secondary(secondary)) => { + *self = Self::Connected { + record: secondary.clone(), + secondary: None, + }; + } + // Preserve the dial record. + Some(SecondaryOrDialing::Dialing(dial_record)) => { + *self = Self::Disconnected { + dial_record: Some(dial_record.clone()), + }; + } + None => { + *self = Self::Disconnected { dial_record: None }; + } + }; + + return true; + } + + match secondary { + // Secondary connection closed. + Some(SecondaryOrDialing::Secondary(secondary)) + if secondary.connection_id == connection_id => + { + *self = Self::Connected { + record: record.clone(), + secondary: None, + }; + return true; + } + + _ => return false, + } + } + + _ => false, + } + } } pub struct DisconnectedState<'a> { @@ -272,9 +320,7 @@ impl<'a> DisconnectedState<'a> { /// /// [`PeerState::Disconnected`] -> [`PeerState::Dialing`] pub fn dial_record(self, dial_record: ConnectionRecord) { - *self.state = PeerState::Dialing { - record: dial_record, - }; + *self.state = PeerState::Dialing { dial_record }; } /// Dial the peer on multiple addresses. From 6912dda31f5aad12edd8a5c20669e7715b727ac1 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 20 Sep 2024 15:41:13 +0300 Subject: [PATCH 29/66] manager/peer_state: Adjust connection established return Signed-off-by: Alexandru Vasile --- src/transport/manager/types.rs | 57 ++++++++++++++++++---------------- 1 file changed, 31 insertions(+), 26 deletions(-) diff --git a/src/transport/manager/types.rs b/src/transport/manager/types.rs index 6c499774..2c1e7b58 100644 --- a/src/transport/manager/types.rs +++ b/src/transport/manager/types.rs @@ -49,7 +49,7 @@ pub enum SupportedTransport { } /// Peer state. -#[derive(Debug)] +#[derive(Debug, Clone, PartialEq)] pub enum PeerState { /// `Litep2p` is connected to peer. Connected { @@ -97,7 +97,7 @@ pub enum PeerState { } /// The state of the secondary connection. -#[derive(Debug)] +#[derive(Debug, Clone, PartialEq)] pub enum SecondaryOrDialing { /// The secondary connection is established. Secondary(ConnectionRecord), @@ -177,10 +177,7 @@ impl PeerState { } /// Returns `true` if the connection should be accepted by the transport manager. - pub fn on_connection_established( - &mut self, - connection: ConnectionRecord, - ) -> (bool, Option<(ConnectionId, HashSet)>) { + pub fn on_connection_established(&mut self, connection: ConnectionRecord) -> bool { match self { // Transform the dial record into a secondary connection. Self::Connected { @@ -193,7 +190,7 @@ impl PeerState { secondary: Some(SecondaryOrDialing::Secondary(connection)), }; - return (true, None); + return true; }, // There's place for a secondary connection. Self::Connected { @@ -204,7 +201,7 @@ impl PeerState { secondary: Some(SecondaryOrDialing::Secondary(connection)), }; - return (true, None); + return true; } // Convert the dial record into a primary connection or preserve it. @@ -217,36 +214,41 @@ impl PeerState { record: connection.clone(), secondary: None, }; - return (true, None); + return true; } else { *self = Self::Connected { record: connection, secondary: Some(SecondaryOrDialing::Dialing(dial_record.clone())), }; - return (true, None); + return true; }, - // Accept the incoming connection and cancel opening dials. - Self::Opening { - addresses, - connection_id, - transports, - } => { + Self::Disconnected { dial_record: None } => { + *self = Self::Connected { + record: connection, + secondary: None, + }; + + return true; + } + + // Accept the incoming connection. + Self::Opening { .. } => { *self = Self::Connected { record: connection, secondary: None, }; - return (true, Some((*connection_id, transports.clone()))); + return true; } _ => {} }; - return (false, None); + return false; } - /// Returns `true` if the connection was closed and the state was updated. + /// Returns `true` if the connection was closed. pub fn on_connection_closed(&mut self, connection_id: ConnectionId) -> bool { match self { Self::Connected { record, secondary } => { @@ -265,13 +267,17 @@ impl PeerState { *self = Self::Disconnected { dial_record: Some(dial_record.clone()), }; + + // This is the only case where the connection transitions from + // [`PeerState::Connected`] to [`PeerState::Disconnected`]. + return true; } None => { *self = Self::Disconnected { dial_record: None }; } }; - return true; + return false; } match secondary { @@ -283,15 +289,14 @@ impl PeerState { record: record.clone(), secondary: None, }; - return true; } - - _ => return false, + _ => (), } } - - _ => false, + _ => (), } + + false } } @@ -355,7 +360,7 @@ impl<'a> DisconnectedState<'a> { /// - established outbound connections via [`PeerState::Connected`]. /// - established inbound connections via `PeerContext::secondary_connection`. #[allow(clippy::derived_hash_with_manual_eq)] -#[derive(Debug, Clone, Hash)] +#[derive(Debug, Clone, Hash, PartialEq)] pub struct ConnectionRecord { /// Address of the connection. /// From 900ff49c95536a6cf1c4e4bd567944172df84568 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 20 Sep 2024 15:41:32 +0300 Subject: [PATCH 30/66] manger: Use new peer state Signed-off-by: Alexandru Vasile --- src/transport/manager/mod.rs | 403 ++++++++--------------------------- 1 file changed, 84 insertions(+), 319 deletions(-) diff --git a/src/transport/manager/mod.rs b/src/transport/manager/mod.rs index dca526f3..28c80b34 100644 --- a/src/transport/manager/mod.rs +++ b/src/transport/manager/mod.rs @@ -27,7 +27,7 @@ use crate::{ protocol::{InnerTransportEvent, TransportService}, transport::{ manager::{ - address::{AddressRecord, AddressStore}, + address::AddressRecord, handle::InnerTransportManagerCommand, types::{ConnectionRecord, PeerContext, PeerState}, }, @@ -673,6 +673,8 @@ impl TransportManager { /// /// The main purpose of this function is to advance the internal `PeerState`. fn on_dial_failure(&mut self, connection_id: ConnectionId) -> crate::Result<()> { + tracing::trace!(target: LOG_TARGET, ?connection_id, "on dial failure"); + let peer = self.pending_connections.remove(&connection_id).ok_or_else(|| { tracing::error!( target: LOG_TARGET, @@ -685,7 +687,10 @@ impl TransportManager { let mut peers = self.peers.write(); let context = peers.entry(peer).or_insert_with(|| PeerContext::default()); - if !context.state.on_dial_failure(connection_id) { + let previous_state = context.state.clone(); + context.state.on_dial_failure(connection_id); + + if context.state == previous_state { tracing::warn!( target: LOG_TARGET, ?peer, @@ -693,6 +698,15 @@ impl TransportManager { state = ?context.state, "invalid state for dial failure", ); + } else { + tracing::trace!( + target: LOG_TARGET, + ?peer, + ?connection_id, + ?previous_state, + state = ?context.state, + "on dial failure completed" + ); } Ok(()) @@ -708,140 +722,40 @@ impl TransportManager { &mut self, peer: PeerId, connection_id: ConnectionId, - ) -> crate::Result> { + ) -> Option { + tracing::trace!(target: LOG_TARGET, ?peer, ?connection_id, "connection closed"); + self.connection_limits.on_connection_closed(connection_id); let mut peers = self.peers.write(); - let Some(context) = peers.get_mut(&peer) else { + let context = peers.entry(peer).or_insert_with(|| PeerContext::default()); + + let previous_state = context.state.clone(); + let connection_closed = context.state.on_connection_closed(connection_id); + + if context.state == previous_state { tracing::warn!( target: LOG_TARGET, ?peer, ?connection_id, - "cannot handle closed connection: peer doesn't exist", + state = ?context.state, + "invalid state for a closed connection", + ); + } else { + tracing::trace!( + target: LOG_TARGET, + ?peer, + ?connection_id, + ?previous_state, + state = ?context.state, + "on connection closed completed" ); - debug_assert!(false); - return Err(Error::PeerDoesntExist(peer)); - }; - - tracing::trace!( - target: LOG_TARGET, - ?peer, - ?connection_id, - "connection closed", - ); - - match std::mem::replace( - &mut context.state, - PeerState::Disconnected { dial_record: None }, - ) { - PeerState::Connected { - record, - dial_record: actual_dial_record, - } => match record.connection_id == connection_id { - // primary connection was closed - // - // if secondary connection exists, switch to using it while keeping peer in - // `Connected` state and if there's only one connection, set peer - // state to `Disconnected` - true => match context.secondary_connection.take() { - None => { - context.state = PeerState::Disconnected { - dial_record: actual_dial_record, - }; - - Ok(Some(TransportEvent::ConnectionClosed { - peer, - connection_id, - })) - } - Some(secondary_connection) => { - context.state = PeerState::Connected { - record: ConnectionRecord { - address: secondary_connection.address.clone(), - connection_id: secondary_connection.connection_id, - }, - dial_record: actual_dial_record, - }; - - Ok(None) - } - }, - // secondary connection was closed - false => match context.secondary_connection.take() { - Some(secondary_connection) => { - if secondary_connection.connection_id != connection_id { - tracing::debug!( - target: LOG_TARGET, - ?peer, - ?connection_id, - "unknown connection was closed, potentially ignored tertiary connection", - ); - - context.secondary_connection = Some(secondary_connection); - context.state = PeerState::Connected { - record, - dial_record: actual_dial_record, - }; - - return Ok(None); - } - - tracing::trace!( - target: LOG_TARGET, - ?peer, - ?connection_id, - "secondary connection closed", - ); - - context.state = PeerState::Connected { - record, - dial_record: actual_dial_record, - }; - Ok(None) - } - None => { - tracing::warn!( - target: LOG_TARGET, - ?peer, - ?connection_id, - "non-primary connection was closed but secondary connection doesn't exist", - ); - - debug_assert!(false); - Err(Error::InvalidState) - } - }, - }, - PeerState::Disconnected { dial_record } => match context.secondary_connection.take() { - Some(record) => { - tracing::warn!( - target: LOG_TARGET, - ?peer, - ?connection_id, - ?record, - ?dial_record, - "peer is disconnected but secondary connection exists", - ); - - debug_assert!(false); - context.state = PeerState::Disconnected { dial_record }; - Err(Error::InvalidState) - } - None => { - context.state = PeerState::Disconnected { dial_record }; - - Ok(Some(TransportEvent::ConnectionClosed { - peer, - connection_id, - })) - } - }, - state => { - tracing::warn!(target: LOG_TARGET, ?peer, ?connection_id, ?state, "invalid state for a closed connection"); - debug_assert!(false); - Err(Error::InvalidState) - } } + + connection_closed.then_some(TransportEvent::ConnectionClosed { + peer, + connection_id, + }) } /// Update the address on a connection established. @@ -862,15 +776,7 @@ impl TransportManager { scores::CONNECTION_ESTABLISHED, ); - let context = peers.entry(peer).or_insert_with(|| PeerContext { - state: PeerState::Connected { - record: ConnectionRecord::from_endpoint(peer, endpoint), - dial_record: None, - }, - addresses: AddressStore::new(), - secondary_connection: None, - }); - + let context = peers.entry(peer).or_insert_with(|| PeerContext::default()); context.addresses.insert(record); } @@ -909,177 +815,47 @@ impl TransportManager { } let mut peers = self.peers.write(); - match peers.get_mut(&peer) { - Some(context) => match context.state { - PeerState::Connected { - ref mut dial_record, - .. - } => match context.secondary_connection { - Some(_) => { - tracing::debug!( - target: LOG_TARGET, - ?peer, - connection_id = ?endpoint.connection_id(), - ?endpoint, - "secondary connection already exists, ignoring connection", - ); - - return Ok(ConnectionEstablishedResult::Reject); - } - None => match dial_record.take() { - Some(record) if record.connection_id == endpoint.connection_id() => { - tracing::debug!( - target: LOG_TARGET, - ?peer, - connection_id = ?endpoint.connection_id(), - address = ?endpoint.address(), - "dialed connection opened as secondary connection", - ); - - context.secondary_connection = - Some(ConnectionRecord::from_endpoint(peer, endpoint)); - } - None => { - tracing::debug!( - target: LOG_TARGET, - ?peer, - connection_id = ?endpoint.connection_id(), - address = ?endpoint.address(), - "secondary connection", - ); - - context.secondary_connection = - Some(ConnectionRecord::from_endpoint(peer, endpoint)); - } - Some(record) => { - tracing::warn!( - target: LOG_TARGET, - ?peer, - connection_id = ?endpoint.connection_id(), - address = ?endpoint.address(), - dial_record = ?record, - "unknown connection opened as secondary connection, discarding", - ); - - // Preserve the dial record. - *dial_record = Some(record); - - return Ok(ConnectionEstablishedResult::Reject); - } - }, - }, - PeerState::Dialing { ref record, .. } => { - match record.connection_id == endpoint.connection_id() { - true => { - tracing::trace!( - target: LOG_TARGET, - ?peer, - connection_id = ?endpoint.connection_id(), - ?endpoint, - ?record, - "connection opened to remote", - ); - - context.state = PeerState::Connected { - record: record.clone(), - dial_record: None, - }; - } - false => { - tracing::trace!( - target: LOG_TARGET, - ?peer, - connection_id = ?endpoint.connection_id(), - ?endpoint, - "connection opened by remote while local node was dialing", - ); - - context.state = PeerState::Connected { - record: ConnectionRecord::from_endpoint(peer, endpoint), - dial_record: Some(record.clone()), - }; - } - } - } - PeerState::Opening { - ref mut addresses, - connection_id, - ref transports, - } => { - tracing::trace!( - target: LOG_TARGET, - ?peer, - dial_connection_id = ?connection_id, - dial_records = ?addresses, - dial_transports = ?transports, - listener_endpoint = ?endpoint, - "inbound connection while opening an outbound connection", - ); + let context = peers.entry(peer).or_insert_with(|| PeerContext::default()); - // cancel all pending dials - transports.iter().for_each(|transport| { - self.transports - .get_mut(transport) - .expect("transport to exist") - .cancel(connection_id); - }); + let previous_state = context.state.clone(); + let connection_accepted = context + .state + .on_connection_established(ConnectionRecord::from_endpoint(peer, endpoint)); - // since an inbound connection was removed, the outbound connection can be - // removed from pending dials - self.pending_connections.remove(&connection_id); + tracing::trace!( + target: LOG_TARGET, + ?peer, + ?endpoint, + ?previous_state, + state = ?context.state, + "on connection established completed" + ); - context.state = PeerState::Connected { - record: ConnectionRecord::from_endpoint(peer, endpoint), - dial_record: None, - }; - } - PeerState::Disconnected { - ref mut dial_record, - } => { - tracing::trace!( - target: LOG_TARGET, - ?peer, - connection_id = ?endpoint.connection_id(), - ?endpoint, - ?dial_record, - "connection opened by remote or delayed dial succeeded", - ); + if connection_accepted { + // Cancel all pending dials if the connection was established. + if let PeerState::Opening { + connection_id, + transports, + .. + } = previous_state + { + // cancel all pending dials + transports.iter().for_each(|transport| { + self.transports + .get_mut(transport) + .expect("transport to exist") + .cancel(connection_id); + }); - let (record, dial_record) = match dial_record.take() { - Some(dial_record) => - if &dial_record.address == endpoint.address() { - (dial_record, None) - } else { - ( - ConnectionRecord::from_endpoint(peer, endpoint), - Some(dial_record), - ) - }, - None => (ConnectionRecord::from_endpoint(peer, endpoint), None), - }; - - context.state = PeerState::Connected { - record, - dial_record, - }; - } - }, - None => { - peers.insert( - peer, - PeerContext { - state: PeerState::Connected { - record: ConnectionRecord::from_endpoint(peer, endpoint), - dial_record: None, - }, - addresses: AddressStore::new(), - secondary_connection: None, - }, - ); + // since an inbound connection was removed, the outbound connection can be + // removed from pending dials + self.pending_connections.remove(&connection_id); } + + return Ok(ConnectionEstablishedResult::Accept); } - Ok(ConnectionEstablishedResult::Accept) + Ok(ConnectionEstablishedResult::Reject) } fn on_connection_opened( @@ -1102,17 +878,7 @@ impl TransportManager { }; let mut peers = self.peers.write(); - let context = peers.get_mut(&peer).ok_or_else(|| { - tracing::warn!( - target: LOG_TARGET, - ?peer, - ?connection_id, - "connection opened but peer doesn't exist", - ); - - debug_assert!(false); - Error::InvalidState - })?; + let context = peers.entry(peer).or_insert_with(|| PeerContext::default()); match std::mem::replace( &mut context.state, @@ -1159,7 +925,11 @@ impl TransportManager { self.pending_connections.insert(connection_id, peer); context.state = PeerState::Dialing { - record: ConnectionRecord::new(peer, address.clone(), connection_id), + dial_record: ConnectionRecord::new( + peer, + address.clone(), + connection_id, + ), }; context.addresses.insert(AddressRecord::new( @@ -1300,13 +1070,8 @@ impl TransportManager { peer, connection: connection_id, } => match self.on_connection_closed(peer, connection_id) { - Ok(None) => {} - Ok(Some(event)) => return Some(event), - Err(error) => tracing::error!( - target: LOG_TARGET, - ?error, - "failed to handle closed connection", - ), + None => {} + Some(event) => return Some(event), } }, command = self.cmd_rx.recv() => match command? { From e206588020e9a85d2662cd5a5756a3fd59238f5a Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 20 Sep 2024 16:25:24 +0300 Subject: [PATCH 31/66] manager/tests: Adjust tests to compile Signed-off-by: Alexandru Vasile --- src/transport/manager/address.rs | 22 +-- src/transport/manager/handle.rs | 37 ++--- src/transport/manager/mod.rs | 230 +++++++++++++++---------------- 3 files changed, 143 insertions(+), 146 deletions(-) diff --git a/src/transport/manager/address.rs b/src/transport/manager/address.rs index 6f0b9f82..b9906326 100644 --- a/src/transport/manager/address.rs +++ b/src/transport/manager/address.rs @@ -311,16 +311,18 @@ mod tests { store.insert(quic_address_record(&mut rng)); } - let known_addresses = store.by_address.len(); + let known_addresses = store.addresses.len(); assert!(known_addresses >= 3); - let taken = store.take(known_addresses - 2); + let taken = store.addresses(known_addresses - 2); assert_eq!(known_addresses - 2, taken.len()); assert!(!store.is_empty()); let mut prev: Option = None; - for record in taken { - assert!(!store.contains(record.address())); + for address in taken { + assert!(!store.addresses.contains_key(&address)); + + let record = store.addresses.get(&address).unwrap().clone(); if let Some(previous) = prev { assert!(previous.score > record.score); @@ -339,14 +341,16 @@ mod tests { store.insert(ws_address_record(&mut rng)); store.insert(quic_address_record(&mut rng)); - assert_eq!(store.by_address.len(), 3); + assert_eq!(store.addresses.len(), 3); - let taken = store.take(8usize); + let taken = store.addresses(8usize); assert_eq!(taken.len(), 3); assert!(store.is_empty()); let mut prev: Option = None; for record in taken { + let record = store.addresses.get(&record).unwrap().clone(); + if prev.is_none() { prev = Some(record); } else { @@ -381,10 +385,9 @@ mod tests { .collect::>(); store.extend(records); - for record in store.by_score { + for record in store.addresses.values().cloned() { let stored = cloned.get(record.address()).unwrap(); assert_eq!(stored.score(), record.score()); - assert_eq!(stored.connection_id(), record.connection_id()); assert_eq!(stored.address(), record.address()); } } @@ -413,10 +416,9 @@ mod tests { let cloned = records.iter().cloned().collect::>(); store.extend(records.iter().map(|(_, record)| record)); - for record in store.by_score { + for record in store.addresses.values().cloned() { let stored = cloned.get(record.address()).unwrap(); assert_eq!(stored.score(), record.score()); - assert_eq!(stored.connection_id(), record.connection_id()); assert_eq!(stored.address(), record.address()); } } diff --git a/src/transport/manager/handle.rs b/src/transport/manager/handle.rs index a6a907e9..2f9320c4 100644 --- a/src/transport/manager/handle.rs +++ b/src/transport/manager/handle.rs @@ -331,6 +331,8 @@ impl TransportHandle { #[cfg(test)] mod tests { + use crate::transport::manager::{address::AddressStore, types::ConnectionRecord}; + use super::*; use multihash::Multihash; use parking_lot::lock_api::RwLock; @@ -447,14 +449,14 @@ mod tests { peer, PeerContext { state: PeerState::Connected { - record: AddressRecord::from_multiaddr( - Multiaddr::empty() + record: ConnectionRecord { + address: Multiaddr::empty() .with(Protocol::Ip4(std::net::Ipv4Addr::new(127, 0, 0, 1))) .with(Protocol::Tcp(8888)) .with(Protocol::P2p(Multihash::from(peer))), - ) - .unwrap(), - dial_record: None, + connection_id: ConnectionId::from(0), + }, + secondary: None, }, secondary_connection: None, addresses: AddressStore::from_iter( @@ -490,13 +492,13 @@ mod tests { peer, PeerContext { state: PeerState::Dialing { - record: AddressRecord::from_multiaddr( - Multiaddr::empty() + dial_record: ConnectionRecord { + address: Multiaddr::empty() .with(Protocol::Ip4(std::net::Ipv4Addr::new(127, 0, 0, 1))) .with(Protocol::Tcp(8888)) .with(Protocol::P2p(Multihash::from(peer))), - ) - .unwrap(), + connection_id: ConnectionId::from(0), + }, }, secondary_connection: None, addresses: AddressStore::from_iter( @@ -558,15 +560,14 @@ mod tests { peer, PeerContext { state: PeerState::Disconnected { - dial_record: Some( - AddressRecord::from_multiaddr( - Multiaddr::empty() - .with(Protocol::Ip4(std::net::Ipv4Addr::new(127, 0, 0, 1))) - .with(Protocol::Tcp(8888)) - .with(Protocol::P2p(Multihash::from(peer))), - ) - .unwrap(), - ), + dial_record: Some(ConnectionRecord::new( + peer, + Multiaddr::empty() + .with(Protocol::Ip4(std::net::Ipv4Addr::new(127, 0, 0, 1))) + .with(Protocol::Tcp(8888)) + .with(Protocol::P2p(Multihash::from(peer))), + ConnectionId::from(0), + )), }, secondary_connection: None, addresses: AddressStore::from_iter( diff --git a/src/transport/manager/mod.rs b/src/transport/manager/mod.rs index 28c80b34..989bf12c 100644 --- a/src/transport/manager/mod.rs +++ b/src/transport/manager/mod.rs @@ -563,7 +563,9 @@ impl TransportManager { Protocol::QuicV1 => SupportedTransport::Quic, _ => { tracing::debug!(target: LOG_TARGET, address = ?address_record.address(), "expected `quic-v1`"); - return Err(Error::TransportNotSupported(record.address().clone())); + return Err(Error::TransportNotSupported( + address_record.address().clone(), + )); } }, protocol => { @@ -1347,6 +1349,7 @@ impl TransportManager { #[cfg(test)] mod tests { + use crate::transport::manager::{address::AddressStore, types::SecondaryOrDialing}; use limits::ConnectionLimitsConfig; use multihash::Multihash; @@ -1362,6 +1365,7 @@ mod tests { use std::{ net::{Ipv4Addr, Ipv6Addr}, sync::Arc, + usize, }; /// Setup TCP address and connection id. @@ -1812,8 +1816,8 @@ mod tests { assert_eq!(manager.pending_connections.len(), 1); match &manager.peers.read().get(&peer).unwrap().state { - PeerState::Dialing { record } => { - assert_eq!(record.address(), &dial_address); + PeerState::Dialing { dial_record } => { + assert_eq!(dial_record.address, dial_address); } state => panic!("invalid state for peer: {state:?}"), } @@ -1833,9 +1837,9 @@ mod tests { let peer = peers.get(&peer).unwrap(); match &peer.state { - PeerState::Connected { dial_record, .. } => { - assert!(dial_record.is_none()); - assert!(peer.addresses.contains(&dial_address)); + PeerState::Connected { secondary, .. } => { + assert!(secondary.is_none()); + assert!(peer.addresses.addresses.contains_key(&dial_address)); } state => panic!("invalid state: {state:?}"), } @@ -1879,8 +1883,8 @@ mod tests { assert_eq!(manager.pending_connections.len(), 1); match &manager.peers.read().get(&peer).unwrap().state { - PeerState::Dialing { record } => { - assert_eq!(record.address(), &dial_address); + PeerState::Dialing { dial_record } => { + assert_eq!(dial_record.address, dial_address); } state => panic!("invalid state for peer: {state:?}"), } @@ -1906,7 +1910,7 @@ mod tests { dial_record: Some(dial_record), .. } => { - assert_eq!(dial_record.address(), &dial_address); + assert_eq!(dial_record.address, dial_address); } state => panic!("invalid state: {state:?}"), } @@ -1922,7 +1926,7 @@ mod tests { PeerState::Disconnected { dial_record: None, .. } => { - assert!(peer.addresses.contains(&dial_address)); + assert!(peer.addresses.addresses.contains_key(&dial_address)); } state => panic!("invalid state: {state:?}"), } @@ -1966,8 +1970,8 @@ mod tests { assert_eq!(manager.pending_connections.len(), 1); match &manager.peers.read().get(&peer).unwrap().state { - PeerState::Dialing { record } => { - assert_eq!(record.address(), &dial_address); + PeerState::Dialing { dial_record } => { + assert_eq!(dial_record.address, dial_address); } state => panic!("invalid state for peer: {state:?}"), } @@ -1993,7 +1997,7 @@ mod tests { dial_record: Some(dial_record), .. } => { - assert_eq!(dial_record.address(), &dial_address); + assert_eq!(dial_record.address, dial_address); } state => panic!("invalid state: {state:?}"), } @@ -2012,7 +2016,7 @@ mod tests { match &peer.state { PeerState::Connected { - dial_record: None, .. + secondary: None, .. } => {} state => panic!("invalid state: {state:?}"), } @@ -2069,7 +2073,7 @@ mod tests { match &peer.state { PeerState::Connected { - dial_record: None, .. + secondary: None, .. } => { assert!(peer.secondary_connection.is_none()); } @@ -2091,11 +2095,10 @@ mod tests { match &context.state { PeerState::Connected { - dial_record: None, .. + secondary: None, .. } => { let seconary_connection = context.secondary_connection.as_ref().unwrap(); - assert_eq!(seconary_connection.address(), &address2); - assert_eq!(seconary_connection.score(), SCORE_CONNECT_SUCCESS); + assert_eq!(seconary_connection.address, address2); } state => panic!("invalid state: {state:?}"), } @@ -2115,12 +2118,11 @@ mod tests { match &peer.state { PeerState::Connected { - dial_record: None, .. + secondary: None, .. } => { let seconary_connection = peer.secondary_connection.as_ref().unwrap(); - assert_eq!(seconary_connection.address(), &address2); - assert_eq!(seconary_connection.score(), SCORE_CONNECT_SUCCESS); - assert!(peer.addresses.contains(&address3)); + assert_eq!(seconary_connection.address, address2); + assert!(peer.addresses.addresses(usize::MAX).contains(&address3)); } state => panic!("invalid state: {state:?}"), } @@ -2171,7 +2173,7 @@ mod tests { match &peer.state { PeerState::Connected { - dial_record: None, .. + secondary: None, .. } => { assert!(peer.secondary_connection.is_none()); } @@ -2189,11 +2191,10 @@ mod tests { state => panic!("invalid state: {state:?}"), }; - let dial_record = Some(AddressRecord::new(&peer, address2.clone(), 0)); - + let dial_record = ConnectionRecord::new(peer, address2.clone(), ConnectionId::from(0)); peer_context.state = PeerState::Connected { record, - dial_record, + secondary: Some(SecondaryOrDialing::Dialing(dial_record)), }; } @@ -2273,7 +2274,7 @@ mod tests { match &peer.state { PeerState::Connected { - dial_record: None, .. + secondary: None, .. } => { assert!(peer.secondary_connection.is_none()); } @@ -2298,18 +2299,17 @@ mod tests { match &context.state { PeerState::Connected { - dial_record: None, .. + secondary: None, .. } => { let seconary_connection = context.secondary_connection.as_ref().unwrap(); - assert_eq!(seconary_connection.address(), &address2); - assert_eq!(seconary_connection.score(), SCORE_CONNECT_SUCCESS); + assert_eq!(seconary_connection.address, address2); } state => panic!("invalid state: {state:?}"), } drop(peers); // close the secondary connection and verify that the peer remains connected - let emit_event = manager.on_connection_closed(peer, ConnectionId::from(1usize)).unwrap(); + let emit_event = manager.on_connection_closed(peer, ConnectionId::from(1usize)); assert!(emit_event.is_none()); let peers = manager.peers.read(); @@ -2317,12 +2317,12 @@ mod tests { match &context.state { PeerState::Connected { - dial_record: None, + secondary: None, record, } => { assert!(context.secondary_connection.is_none()); - assert!(context.addresses.contains(&address2)); - assert_eq!(record.connection_id(), &Some(ConnectionId::from(0usize))); + assert!(context.addresses.addresses(usize::MAX).contains(&address2)); + assert_eq!(record.connection_id, ConnectionId::from(0usize)); } state => panic!("invalid state: {state:?}"), } @@ -2376,7 +2376,7 @@ mod tests { match &peer.state { PeerState::Connected { - dial_record: None, .. + secondary: None, .. } => { assert!(peer.secondary_connection.is_none()); } @@ -2401,11 +2401,10 @@ mod tests { match &context.state { PeerState::Connected { - dial_record: None, .. + secondary: None, .. } => { let seconary_connection = context.secondary_connection.as_ref().unwrap(); - assert_eq!(seconary_connection.address(), &address2); - assert_eq!(seconary_connection.score(), SCORE_CONNECT_SUCCESS); + assert_eq!(seconary_connection.address, address2); } state => panic!("invalid state: {state:?}"), } @@ -2413,7 +2412,7 @@ mod tests { // close the primary connection and verify that the peer remains connected // while the primary connection address is stored in peer addresses - let emit_event = manager.on_connection_closed(peer, ConnectionId::from(0usize)).unwrap(); + let emit_event = manager.on_connection_closed(peer, ConnectionId::from(0usize)); assert!(emit_event.is_none()); let peers = manager.peers.read(); @@ -2421,12 +2420,12 @@ mod tests { match &context.state { PeerState::Connected { - dial_record: None, + secondary: None, record, } => { assert!(context.secondary_connection.is_none()); - assert!(context.addresses.contains(&address1)); - assert_eq!(record.connection_id(), &Some(ConnectionId::from(1usize))); + assert!(context.addresses.addresses(usize::MAX).contains(&address1)); + assert_eq!(record.connection_id, ConnectionId::from(1usize)); } state => panic!("invalid state: {state:?}"), } @@ -2489,7 +2488,7 @@ mod tests { match &peer.state { PeerState::Connected { - dial_record: None, .. + secondary: None, .. } => { assert!(peer.secondary_connection.is_none()); } @@ -2514,11 +2513,10 @@ mod tests { match &context.state { PeerState::Connected { - dial_record: None, .. + secondary: None, .. } => { let seconary_connection = context.secondary_connection.as_ref().unwrap(); - assert_eq!(seconary_connection.address(), &address2); - assert_eq!(seconary_connection.score(), SCORE_CONNECT_SUCCESS); + assert_eq!(seconary_connection.address, address2); } state => panic!("invalid state: {state:?}"), } @@ -2538,11 +2536,11 @@ mod tests { let peers = manager.peers.read(); let context = peers.get(&peer).unwrap(); - assert!(context.addresses.contains(&address3)); + assert!(context.addresses.addresses(usize::MAX).contains(&address3)); drop(peers); // close the tertiary connection that was ignored - let emit_event = manager.on_connection_closed(peer, ConnectionId::from(2usize)).unwrap(); + let emit_event = manager.on_connection_closed(peer, ConnectionId::from(2usize)); assert!(emit_event.is_none()); // verify that the state remains unchanged @@ -2551,11 +2549,14 @@ mod tests { match &context.state { PeerState::Connected { - dial_record: None, .. + secondary: None, .. } => { let seconary_connection = context.secondary_connection.as_ref().unwrap(); - assert_eq!(seconary_connection.address(), &address2); - assert_eq!(seconary_connection.score(), SCORE_CONNECT_SUCCESS); + assert_eq!(seconary_connection.address, address2); + assert_eq!( + context.addresses.addresses.get(&address2).unwrap().score(), + scores::CONNECTION_ESTABLISHED + ); } state => panic!("invalid state: {state:?}"), } @@ -2773,14 +2774,14 @@ mod tests { peer, PeerContext { state: PeerState::Connected { - record: AddressRecord::from_multiaddr( - Multiaddr::empty() + record: ConnectionRecord { + address: Multiaddr::empty() .with(Protocol::Ip4(std::net::Ipv4Addr::new(127, 0, 0, 1))) .with(Protocol::Tcp(8888)) .with(Protocol::P2p(Multihash::from(peer))), - ) - .unwrap(), - dial_record: None, + connection_id: ConnectionId::from(0usize), + }, + secondary: None, }, secondary_connection: None, addresses: AddressStore::from_iter( @@ -2821,13 +2822,13 @@ mod tests { peer, PeerContext { state: PeerState::Dialing { - record: AddressRecord::from_multiaddr( - Multiaddr::empty() + dial_record: ConnectionRecord { + address: Multiaddr::empty() .with(Protocol::Ip4(std::net::Ipv4Addr::new(127, 0, 0, 1))) .with(Protocol::Tcp(8888)) .with(Protocol::P2p(Multihash::from(peer))), - ) - .unwrap(), + connection_id: ConnectionId::from(0usize), + }, }, secondary_connection: None, addresses: AddressStore::from_iter( @@ -2852,10 +2853,10 @@ mod tests { let peer_context = peers.get(&peer).unwrap(); match &peer_context.state { - PeerState::Dialing { record } => { + PeerState::Dialing { dial_record } => { assert_eq!( - record.address(), - &Multiaddr::empty() + dial_record.address, + Multiaddr::empty() .with(Protocol::Ip4(std::net::Ipv4Addr::new(127, 0, 0, 1))) .with(Protocol::Tcp(8888)) .with(Protocol::P2p(Multihash::from(peer))) @@ -2884,15 +2885,14 @@ mod tests { peer, PeerContext { state: PeerState::Disconnected { - dial_record: Some( - AddressRecord::from_multiaddr( - Multiaddr::empty() - .with(Protocol::Ip4(std::net::Ipv4Addr::new(127, 0, 0, 1))) - .with(Protocol::Tcp(8888)) - .with(Protocol::P2p(Multihash::from(peer))), - ) - .unwrap(), - ), + dial_record: Some(ConnectionRecord::new( + peer, + Multiaddr::empty() + .with(Protocol::Ip4(std::net::Ipv4Addr::new(127, 0, 0, 1))) + .with(Protocol::Tcp(8888)) + .with(Protocol::P2p(Multihash::from(peer))), + ConnectionId::from(0), + )), }, secondary_connection: None, addresses: AddressStore::new(), @@ -3100,19 +3100,15 @@ mod tests { let peers = manager.peers.read(); match peers.get(&peer).unwrap() { PeerContext { - state: - PeerState::Connected { - record, - dial_record, - }, + state: PeerState::Connected { record, secondary }, secondary_connection, addresses, } => { - assert!(!addresses.contains(record.address())); - assert!(dial_record.is_none()); + assert!(!addresses.addresses.contains_key(&record.address)); + assert!(secondary.is_none()); assert!(secondary_connection.is_none()); - assert_eq!(record.address(), &dial_address); - assert_eq!(record.connection_id(), &Some(connection_id)); + assert_eq!(record.address, dial_address); + assert_eq!(record.connection_id, connection_id); } state => panic!("invalid peer state: {state:?}"), } @@ -3192,19 +3188,15 @@ mod tests { let peers = manager.peers.read(); match peers.get(&peer).unwrap() { PeerContext { - state: - PeerState::Connected { - record, - dial_record, - }, + state: PeerState::Connected { record, secondary }, secondary_connection, addresses, } => { assert!(addresses.is_empty()); - assert!(dial_record.is_none()); + assert!(secondary.is_none()); assert!(secondary_connection.is_none()); - assert_eq!(record.address(), &dial_address); - assert_eq!(record.connection_id(), &Some(connection_id)); + assert_eq!(record.address, dial_address); + assert_eq!(record.connection_id, connection_id); } state => panic!("invalid peer state: {state:?}"), } @@ -3392,7 +3384,7 @@ mod tests { // Random peer ID. let peer = PeerId::random(); - let (first_addr, first_connection_id) = setup_dial_addr(peer, 0); + let (first_addr, _first_connection_id) = setup_dial_addr(peer, 0); let second_connection_id = ConnectionId::from(1); let different_connection_id = ConnectionId::from(2); @@ -3401,8 +3393,12 @@ mod tests { let mut peers = manager.peers.write(); let state = PeerState::Connected { - record: AddressRecord::new(&peer, first_addr.clone(), 0), - dial_record: Some(AddressRecord::new(&peer, first_addr.clone(), 0)), + record: ConnectionRecord::new(peer, first_addr.clone(), ConnectionId::from(0)), + secondary: Some(SecondaryOrDialing::Dialing(ConnectionRecord::new( + peer, + first_addr.clone(), + ConnectionId::from(0), + ))), }; let peer_context = PeerContext { @@ -3466,8 +3462,8 @@ mod tests { let peers = manager.peers.read(); let peer_context = peers.get(&peer).unwrap(); match &peer_context.state { - PeerState::Dialing { record } => { - assert_eq!(record.address(), &first_addr); + PeerState::Dialing { dial_record } => { + assert_eq!(dial_record.address, first_addr); } state => panic!("invalid state: {state:?}"), } @@ -3487,21 +3483,20 @@ mod tests { match &peer_context.state { PeerState::Connected { record, - dial_record, + secondary: Some(SecondaryOrDialing::Dialing(dial_record)), } => { - assert_eq!(record.address(), &remote_addr); - assert_eq!(record.connection_id(), &Some(remote_connection_id)); + assert_eq!(record.address, remote_addr); + assert_eq!(record.connection_id, remote_connection_id); - let dial_record = dial_record.as_ref().unwrap(); - assert_eq!(dial_record.address(), &first_addr); - assert_eq!(dial_record.connection_id(), &Some(first_connection_id)) + assert_eq!(dial_record.address, first_addr); + assert_eq!(dial_record.connection_id, first_connection_id) } state => panic!("invalid state: {state:?}"), } } // Step 3. The peer disconnects while we have a dialing in flight. - let event = manager.on_connection_closed(peer, remote_connection_id).unwrap().unwrap(); + let event = manager.on_connection_closed(peer, remote_connection_id).unwrap(); match event { TransportEvent::ConnectionClosed { peer: event_peer, @@ -3518,8 +3513,8 @@ mod tests { match &peer_context.state { PeerState::Disconnected { dial_record } => { let dial_record = dial_record.as_ref().unwrap(); - assert_eq!(dial_record.address(), &first_addr); - assert_eq!(dial_record.connection_id(), &Some(first_connection_id)); + assert_eq!(dial_record.address, first_addr); + assert_eq!(dial_record.connection_id, first_connection_id); } state => panic!("invalid state: {state:?}"), } @@ -3534,8 +3529,8 @@ mod tests { match &peer_context.state { PeerState::Disconnected { dial_record } => { let dial_record = dial_record.as_ref().unwrap(); - assert_eq!(dial_record.address(), &first_addr); - assert_eq!(dial_record.connection_id(), &Some(first_connection_id)); + assert_eq!(dial_record.address, first_addr); + assert_eq!(dial_record.connection_id, first_connection_id); } state => panic!("invalid state: {state:?}"), } @@ -3555,15 +3550,14 @@ mod tests { match &peer_context.state { PeerState::Connected { record, - dial_record, + secondary: Some(SecondaryOrDialing::Dialing(dial_record)), } => { - assert_eq!(record.address(), &remote_addr); - assert_eq!(record.connection_id(), &Some(remote_connection_id)); + assert_eq!(record.address, remote_addr); + assert_eq!(record.connection_id, remote_connection_id); // We have not overwritten the first dial record in step 4. - let dial_record = dial_record.as_ref().unwrap(); - assert_eq!(dial_record.address(), &first_addr); - assert_eq!(dial_record.connection_id(), &Some(first_connection_id)); + assert_eq!(dial_record.address, first_addr); + assert_eq!(dial_record.connection_id, first_connection_id); } state => panic!("invalid state: {state:?}"), } @@ -3618,14 +3612,14 @@ mod tests { let peers = manager.peers.read(); let peer_context = peers.get(&peer).unwrap(); match &peer_context.state { - PeerState::Dialing { record } => { - assert_eq!(record.address(), &dial_address); + PeerState::Dialing { dial_record } => { + assert_eq!(dial_record.address, dial_address); } state => panic!("invalid state: {state:?}"), } // The address is not saved yet. - assert!(!peer_context.addresses.contains(&dial_address)); + assert!(!peer_context.addresses.addresses(usize::MAX).contains(&dial_address)); } let second_address = Multiaddr::empty() @@ -3643,14 +3637,14 @@ mod tests { let peer_context = peers.get(&peer).unwrap(); match &peer_context.state { // Must still be dialing the first address. - PeerState::Dialing { record } => { - assert_eq!(record.address(), &dial_address); + PeerState::Dialing { dial_record } => { + assert_eq!(dial_record.address, dial_address); } state => panic!("invalid state: {state:?}"), } - assert!(!peer_context.addresses.contains(&dial_address)); - assert!(!peer_context.addresses.contains(&second_address)); + assert!(!peer_context.addresses.addresses(usize::MAX).contains(&dial_address)); + assert!(!peer_context.addresses.addresses(usize::MAX).contains(&second_address)); } } From d6dec2c56174029ddd162c995a2be409f0206f6a Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 20 Sep 2024 16:35:02 +0300 Subject: [PATCH 32/66] manager: Remove secondary connection from PeerContext Signed-off-by: Alexandru Vasile --- src/transport/manager/handle.rs | 7 ++- src/transport/manager/mod.rs | 86 +++++++++++++++------------------ src/transport/manager/types.rs | 4 -- 3 files changed, 41 insertions(+), 56 deletions(-) diff --git a/src/transport/manager/handle.rs b/src/transport/manager/handle.rs index 2f9320c4..1a367bbb 100644 --- a/src/transport/manager/handle.rs +++ b/src/transport/manager/handle.rs @@ -458,7 +458,7 @@ mod tests { }, secondary: None, }, - secondary_connection: None, + addresses: AddressStore::from_iter( vec![Multiaddr::empty() .with(Protocol::Ip4(std::net::Ipv4Addr::new(127, 0, 0, 1))) @@ -500,7 +500,7 @@ mod tests { connection_id: ConnectionId::from(0), }, }, - secondary_connection: None, + addresses: AddressStore::from_iter( vec![Multiaddr::empty() .with(Protocol::Ip4(std::net::Ipv4Addr::new(127, 0, 0, 1))) @@ -534,7 +534,6 @@ mod tests { peer, PeerContext { state: PeerState::Disconnected { dial_record: None }, - secondary_connection: None, addresses: AddressStore::new(), }, ); @@ -569,7 +568,7 @@ mod tests { ConnectionId::from(0), )), }, - secondary_connection: None, + addresses: AddressStore::from_iter( vec![Multiaddr::empty() .with(Protocol::Ip4(std::net::Ipv4Addr::new(127, 0, 0, 1))) diff --git a/src/transport/manager/mod.rs b/src/transport/manager/mod.rs index 989bf12c..f5ef6734 100644 --- a/src/transport/manager/mod.rs +++ b/src/transport/manager/mod.rs @@ -1709,7 +1709,6 @@ mod tests { PeerContext { state: PeerState::Disconnected { dial_record: None }, addresses: AddressStore::new(), - secondary_connection: None, }, ); @@ -2074,9 +2073,7 @@ mod tests { match &peer.state { PeerState::Connected { secondary: None, .. - } => { - assert!(peer.secondary_connection.is_none()); - } + } => {} state => panic!("invalid state: {state:?}"), } } @@ -2095,10 +2092,10 @@ mod tests { match &context.state { PeerState::Connected { - secondary: None, .. + secondary: Some(SecondaryOrDialing::Secondary(secondary_connection)), + .. } => { - let seconary_connection = context.secondary_connection.as_ref().unwrap(); - assert_eq!(seconary_connection.address, address2); + assert_eq!(secondary_connection.address, address2); } state => panic!("invalid state: {state:?}"), } @@ -2118,11 +2115,15 @@ mod tests { match &peer.state { PeerState::Connected { - secondary: None, .. + secondary: Some(SecondaryOrDialing::Secondary(secondary_connection)), + .. } => { - let seconary_connection = peer.secondary_connection.as_ref().unwrap(); - assert_eq!(seconary_connection.address, address2); - assert!(peer.addresses.addresses(usize::MAX).contains(&address3)); + assert_eq!(secondary_connection.address, address2); + assert!(peer.addresses.addresses.contains_key(&address2)); + assert_eq!( + !peer.addresses.addresses.get(&address3).unwrap().score(), + scores::CONNECTION_ESTABLISHED + ); } state => panic!("invalid state: {state:?}"), } @@ -2174,9 +2175,7 @@ mod tests { match &peer.state { PeerState::Connected { secondary: None, .. - } => { - assert!(peer.secondary_connection.is_none()); - } + } => {} state => panic!("invalid state: {state:?}"), } } @@ -2275,9 +2274,7 @@ mod tests { match &peer.state { PeerState::Connected { secondary: None, .. - } => { - assert!(peer.secondary_connection.is_none()); - } + } => {} state => panic!("invalid state: {state:?}"), } } @@ -2299,10 +2296,10 @@ mod tests { match &context.state { PeerState::Connected { - secondary: None, .. + secondary: Some(SecondaryOrDialing::Secondary(secondary_connection)), + .. } => { - let seconary_connection = context.secondary_connection.as_ref().unwrap(); - assert_eq!(seconary_connection.address, address2); + assert_eq!(secondary_connection.address, address2); } state => panic!("invalid state: {state:?}"), } @@ -2320,8 +2317,11 @@ mod tests { secondary: None, record, } => { - assert!(context.secondary_connection.is_none()); - assert!(context.addresses.addresses(usize::MAX).contains(&address2)); + assert!(context.addresses.addresses.contains_key(&address2)); + assert_eq!( + context.addresses.addresses.get(&address2).unwrap().score(), + scores::CONNECTION_ESTABLISHED + ); assert_eq!(record.connection_id, ConnectionId::from(0usize)); } state => panic!("invalid state: {state:?}"), @@ -2377,9 +2377,7 @@ mod tests { match &peer.state { PeerState::Connected { secondary: None, .. - } => { - assert!(peer.secondary_connection.is_none()); - } + } => {} state => panic!("invalid state: {state:?}"), } } @@ -2401,10 +2399,10 @@ mod tests { match &context.state { PeerState::Connected { - secondary: None, .. + secondary: Some(SecondaryOrDialing::Secondary(secondary_connection)), + .. } => { - let seconary_connection = context.secondary_connection.as_ref().unwrap(); - assert_eq!(seconary_connection.address, address2); + assert_eq!(secondary_connection.address, address2); } state => panic!("invalid state: {state:?}"), } @@ -2423,8 +2421,7 @@ mod tests { secondary: None, record, } => { - assert!(context.secondary_connection.is_none()); - assert!(context.addresses.addresses(usize::MAX).contains(&address1)); + assert!(context.addresses.addresses.contains_key(&address1)); assert_eq!(record.connection_id, ConnectionId::from(1usize)); } state => panic!("invalid state: {state:?}"), @@ -2489,9 +2486,7 @@ mod tests { match &peer.state { PeerState::Connected { secondary: None, .. - } => { - assert!(peer.secondary_connection.is_none()); - } + } => {} state => panic!("invalid state: {state:?}"), } } @@ -2513,10 +2508,10 @@ mod tests { match &context.state { PeerState::Connected { - secondary: None, .. + secondary: Some(SecondaryOrDialing::Secondary(secondary_connection)), + .. } => { - let seconary_connection = context.secondary_connection.as_ref().unwrap(); - assert_eq!(seconary_connection.address, address2); + assert_eq!(secondary_connection.address, address2); } state => panic!("invalid state: {state:?}"), } @@ -2549,10 +2544,10 @@ mod tests { match &context.state { PeerState::Connected { - secondary: None, .. + secondary: Some(SecondaryOrDialing::Secondary(secondary_connection)), + .. } => { - let seconary_connection = context.secondary_connection.as_ref().unwrap(); - assert_eq!(seconary_connection.address, address2); + assert_eq!(secondary_connection.address, address2); assert_eq!( context.addresses.addresses.get(&address2).unwrap().score(), scores::CONNECTION_ESTABLISHED @@ -2783,7 +2778,7 @@ mod tests { }, secondary: None, }, - secondary_connection: None, + addresses: AddressStore::from_iter( vec![Multiaddr::empty() .with(Protocol::Ip4(std::net::Ipv4Addr::new(127, 0, 0, 1))) @@ -2830,7 +2825,7 @@ mod tests { connection_id: ConnectionId::from(0usize), }, }, - secondary_connection: None, + addresses: AddressStore::from_iter( vec![Multiaddr::empty() .with(Protocol::Ip4(std::net::Ipv4Addr::new(127, 0, 0, 1))) @@ -2894,7 +2889,7 @@ mod tests { ConnectionId::from(0), )), }, - secondary_connection: None, + addresses: AddressStore::new(), }, ); @@ -3101,12 +3096,10 @@ mod tests { match peers.get(&peer).unwrap() { PeerContext { state: PeerState::Connected { record, secondary }, - secondary_connection, addresses, } => { assert!(!addresses.addresses.contains_key(&record.address)); assert!(secondary.is_none()); - assert!(secondary_connection.is_none()); assert_eq!(record.address, dial_address); assert_eq!(record.connection_id, connection_id); } @@ -3189,12 +3182,10 @@ mod tests { match peers.get(&peer).unwrap() { PeerContext { state: PeerState::Connected { record, secondary }, - secondary_connection, addresses, } => { assert!(addresses.is_empty()); assert!(secondary.is_none()); - assert!(secondary_connection.is_none()); assert_eq!(record.address, dial_address); assert_eq!(record.connection_id, connection_id); } @@ -3397,13 +3388,12 @@ mod tests { secondary: Some(SecondaryOrDialing::Dialing(ConnectionRecord::new( peer, first_addr.clone(), - ConnectionId::from(0), + second_connection_id, ))), }; let peer_context = PeerContext { state, - secondary_connection: None, addresses: AddressStore::from_iter(vec![first_addr.clone()].into_iter()), }; diff --git a/src/transport/manager/types.rs b/src/transport/manager/types.rs index 2c1e7b58..5bc7bc9a 100644 --- a/src/transport/manager/types.rs +++ b/src/transport/manager/types.rs @@ -406,9 +406,6 @@ pub struct PeerContext { /// Peer state. pub state: PeerState, - /// Secondary connection, if it's open. - pub secondary_connection: Option, - /// Known addresses of peer. pub addresses: AddressStore, } @@ -417,7 +414,6 @@ impl Default for PeerContext { fn default() -> Self { Self { state: PeerState::Disconnected { dial_record: None }, - secondary_connection: None, addresses: AddressStore::new(), } } From 28aa17888eff9d5156ba1997b0755bfff1cae063 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 20 Sep 2024 16:43:47 +0300 Subject: [PATCH 33/66] manager/addresses: Configure address capacity from tests Signed-off-by: Alexandru Vasile --- src/transport/manager/address.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/transport/manager/address.rs b/src/transport/manager/address.rs index b9906326..36c9a725 100644 --- a/src/transport/manager/address.rs +++ b/src/transport/manager/address.rs @@ -116,6 +116,8 @@ impl Ord for AddressRecord { pub struct AddressStore { /// Addresses available. pub addresses: HashMap, + + max_capacity: usize, } impl FromIterator for AddressStore { @@ -163,6 +165,7 @@ impl AddressStore { pub fn new() -> Self { Self { addresses: HashMap::with_capacity(MAX_ADDRESSES), + max_capacity: MAX_ADDRESSES, } } @@ -190,7 +193,7 @@ impl AddressStore { // - if the store is at capacity, the worst address will be evicted. // - an address that is not dialed yet (with score zero) will be preferred over an address // that already failed (with negative score). - if num_addresses > MAX_ADDRESSES { + if num_addresses > self.max_capacity { // No need to keep track of negative addresses if we are at capacity. if record.score < 0 { return; @@ -320,7 +323,8 @@ mod tests { let mut prev: Option = None; for address in taken { - assert!(!store.addresses.contains_key(&address)); + // Addresses are still in the store. + assert!(store.addresses.contains_key(&address)); let record = store.addresses.get(&address).unwrap().clone(); @@ -345,7 +349,6 @@ mod tests { let taken = store.addresses(8usize); assert_eq!(taken.len(), 3); - assert!(store.is_empty()); let mut prev: Option = None; for record in taken { From 424941014cdf7ac5e2ebf17f0fe8b04f3073f352 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 20 Sep 2024 16:47:33 +0300 Subject: [PATCH 34/66] manager/address: Move scores to the address from manager Signed-off-by: Alexandru Vasile --- src/transport/manager/address.rs | 36 ++++++++++++++++++++++++++++++++ src/transport/manager/mod.rs | 16 +------------- 2 files changed, 37 insertions(+), 15 deletions(-) diff --git a/src/transport/manager/address.rs b/src/transport/manager/address.rs index 36c9a725..d8620dc0 100644 --- a/src/transport/manager/address.rs +++ b/src/transport/manager/address.rs @@ -28,6 +28,21 @@ use multihash::Multihash; /// Maximum number of addresses tracked for a peer. const MAX_ADDRESSES: usize = 64; +/// Scores for address records. +pub mod scores { + /// Score indicating that the connection was successfully established. + pub const CONNECTION_ESTABLISHED: i32 = 100i32; + + /// Score for a connection with a peer using a different ID than expected. + pub const DIFFERENT_PEER_ID: i32 = 50i32; + + /// Score for failing to connect due to an invalid or unreachable address. + pub const CONNECTION_FAILURE: i32 = -100i32; + + /// Score for a connection attempt that failed due to a timeout. + pub const TIMEOUT_FAILURE: i32 = -50i32; +} + #[allow(clippy::derived_hash_with_manual_eq)] #[derive(Debug, Clone, Hash)] pub struct AddressRecord { @@ -299,6 +314,27 @@ mod tests { ) } + #[test] + fn insert_record() { + let mut store = AddressStore::new(); + let mut rng = rand::thread_rng(); + + let mut record = tcp_address_record(&mut rng); + record.score = 10; + + store.insert(record.clone()); + + assert_eq!(store.addresses.len(), 1); + assert_eq!(store.addresses.get(record.address()).unwrap(), &record); + + // This time the record is updated. + store.insert(record.clone()); + + assert_eq!(store.addresses.len(), 1); + let store_record = store.addresses.get(record.address()).unwrap(); + assert_eq!(store_record.score, record.score * 2); + } + #[test] fn take_multiple_records() { let mut store = AddressStore::new(); diff --git a/src/transport/manager/mod.rs b/src/transport/manager/mod.rs index f5ef6734..6bb8f3d0 100644 --- a/src/transport/manager/mod.rs +++ b/src/transport/manager/mod.rs @@ -37,6 +37,7 @@ use crate::{ BandwidthSink, PeerId, }; +use address::scores; use futures::{Stream, StreamExt}; use indexmap::IndexMap; use multiaddr::{Multiaddr, Protocol}; @@ -72,21 +73,6 @@ pub(crate) mod handle; /// Logging target for the file. const LOG_TARGET: &str = "litep2p::transport-manager"; -/// Scores for address records. -mod scores { - /// Score indicating that the connection was successfully established. - pub const CONNECTION_ESTABLISHED: i32 = 100i32; - - /// Score for a connection with a peer using a different ID than expected. - pub const DIFFERENT_PEER_ID: i32 = 50i32; - - /// Score for failing to connect due to an invalid or unreachable address. - pub const CONNECTION_FAILURE: i32 = -100i32; - - /// Score for a connection attempt that failed due to a timeout. - pub const TIMEOUT_FAILURE: i32 = -50i32; -} - /// The connection established result. #[derive(Debug, Clone, Copy, Eq, PartialEq)] enum ConnectionEstablishedResult { From 42adc4a5c214dd52ac38d128322f82251bf79bea Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 20 Sep 2024 16:53:46 +0300 Subject: [PATCH 35/66] manager/address: Evict entries with score below threshold Signed-off-by: Alexandru Vasile --- src/transport/manager/address.rs | 28 +++++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/src/transport/manager/address.rs b/src/transport/manager/address.rs index d8620dc0..a27fb7ae 100644 --- a/src/transport/manager/address.rs +++ b/src/transport/manager/address.rs @@ -20,7 +20,7 @@ use crate::PeerId; -use std::collections::HashMap; +use std::collections::{hash_map::Entry, HashMap}; use multiaddr::{Multiaddr, Protocol}; use multihash::Multihash; @@ -43,6 +43,9 @@ pub mod scores { pub const TIMEOUT_FAILURE: i32 = -50i32; } +/// Remove the address from the store if the score is below this threshold. +const REMOVE_THRESHOLD: i32 = scores::CONNECTION_FAILURE * 2; + #[allow(clippy::derived_hash_with_manual_eq)] #[derive(Debug, Clone, Hash)] pub struct AddressRecord { @@ -196,8 +199,11 @@ impl AddressStore { pub fn insert(&mut self, record: AddressRecord) { let num_addresses = self.addresses.len(); - if let Some(occupied) = self.addresses.get_mut(record.address()) { - occupied.update_score(record.score); + if let Entry::Occupied(mut occupied) = self.addresses.entry(record.address.clone()) { + occupied.get_mut().update_score(record.score); + if occupied.get().score <= REMOVE_THRESHOLD { + occupied.remove(); + } return; } @@ -335,6 +341,22 @@ mod tests { assert_eq!(store_record.score, record.score * 2); } + #[test] + fn evict_below_threshold() { + let mut store = AddressStore::new(); + let mut rng = rand::thread_rng(); + + let mut record = tcp_address_record(&mut rng); + record.score = scores::CONNECTION_FAILURE; + store.insert(record.clone()); + + assert_eq!(store.addresses.len(), 1); + + store.insert(record.clone()); + + assert_eq!(store.addresses.len(), 0); + } + #[test] fn take_multiple_records() { let mut store = AddressStore::new(); From 91d1e90634da66aca93387da76802f07f620ff16 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 20 Sep 2024 17:00:01 +0300 Subject: [PATCH 36/66] manager/addresses/tests: Evict on capacity Signed-off-by: Alexandru Vasile --- src/transport/manager/address.rs | 49 +++++++++++++++++++++++++++++--- 1 file changed, 45 insertions(+), 4 deletions(-) diff --git a/src/transport/manager/address.rs b/src/transport/manager/address.rs index a27fb7ae..e54602d8 100644 --- a/src/transport/manager/address.rs +++ b/src/transport/manager/address.rs @@ -214,7 +214,7 @@ impl AddressStore { // - if the store is at capacity, the worst address will be evicted. // - an address that is not dialed yet (with score zero) will be preferred over an address // that already failed (with negative score). - if num_addresses > self.max_capacity { + if num_addresses >= self.max_capacity { // No need to keep track of negative addresses if we are at capacity. if record.score < 0 { return; @@ -230,6 +230,11 @@ impl AddressStore { self.addresses.remove(min_record.address()); } + // There's no need to keep track of this address if the score is below the threshold. + if record.score <= REMOVE_THRESHOLD { + return; + } + // Insert the record. self.addresses.insert(record.address.clone(), record); } @@ -263,7 +268,7 @@ mod tests { ), rng.gen_range(1..=65535), )); - let score: i32 = rng.gen(); + let score: i32 = rng.gen_range(10..=200); AddressRecord::new( &peer, @@ -285,7 +290,7 @@ mod tests { ), rng.gen_range(1..=65535), )); - let score: i32 = rng.gen(); + let score: i32 = rng.gen_range(10..=200); AddressRecord::new( &peer, @@ -308,7 +313,7 @@ mod tests { ), rng.gen_range(1..=65535), )); - let score: i32 = rng.gen(); + let score: i32 = rng.gen_range(10..=200); AddressRecord::new( &peer, @@ -357,6 +362,42 @@ mod tests { assert_eq!(store.addresses.len(), 0); } + #[test] + fn evict_on_capacity() { + let mut store = AddressStore { + addresses: HashMap::new(), + max_capacity: 2, + }; + + let mut rng = rand::thread_rng(); + let mut first_record = tcp_address_record(&mut rng); + first_record.score = scores::CONNECTION_ESTABLISHED; + let mut second_record = ws_address_record(&mut rng); + second_record.score = 0; + + store.insert(first_record.clone()); + store.insert(second_record.clone()); + + assert_eq!(store.addresses.len(), 2); + + // We have better addresses, ignore this one. + let mut third_record = quic_address_record(&mut rng); + third_record.score = scores::CONNECTION_FAILURE; + store.insert(third_record.clone()); + assert_eq!(store.addresses.len(), 2); + assert!(store.addresses.contains_key(first_record.address())); + assert!(store.addresses.contains_key(second_record.address())); + + // Evict the address with the lowest score. + let mut fourth_record = quic_address_record(&mut rng); + fourth_record.score = scores::DIFFERENT_PEER_ID; + store.insert(fourth_record.clone()); + + assert_eq!(store.addresses.len(), 2); + assert!(store.addresses.contains_key(first_record.address())); + assert!(store.addresses.contains_key(fourth_record.address())); + } + #[test] fn take_multiple_records() { let mut store = AddressStore::new(); From 8248ba778eea1e8e3f6fe0782126d13350f4262c Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 20 Sep 2024 17:14:34 +0300 Subject: [PATCH 37/66] manager/tests: Test no longer panics on invalid peerIDs Signed-off-by: Alexandru Vasile --- src/transport/manager/mod.rs | 21 --------------------- 1 file changed, 21 deletions(-) diff --git a/src/transport/manager/mod.rs b/src/transport/manager/mod.rs index 6bb8f3d0..ae543d6a 100644 --- a/src/transport/manager/mod.rs +++ b/src/transport/manager/mod.rs @@ -2563,27 +2563,6 @@ mod tests { manager.on_dial_failure(ConnectionId::random()).unwrap(); } - #[tokio::test] - #[cfg(debug_assertions)] - #[should_panic] - async fn dial_failure_for_unknow_peer() { - let _ = tracing_subscriber::fmt() - .with_env_filter(tracing_subscriber::EnvFilter::from_default_env()) - .try_init(); - - let (mut manager, _handle) = TransportManager::new( - Keypair::generate(), - HashSet::new(), - BandwidthSink::new(), - 8usize, - ConnectionLimitsConfig::default(), - ); - let connection_id = ConnectionId::random(); - let peer = PeerId::random(); - manager.pending_connections.insert(connection_id, peer); - manager.on_dial_failure(connection_id).unwrap(); - } - #[tokio::test] #[cfg(debug_assertions)] #[should_panic] From a698a00673c346a64b90cb8a9792ba2158ea457d Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 20 Sep 2024 17:17:29 +0300 Subject: [PATCH 38/66] manager/tests: Persist dial addresses even if dial is not initiatied Signed-off-by: Alexandru Vasile --- src/transport/manager/mod.rs | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/src/transport/manager/mod.rs b/src/transport/manager/mod.rs index ae543d6a..5e760764 100644 --- a/src/transport/manager/mod.rs +++ b/src/transport/manager/mod.rs @@ -3529,7 +3529,7 @@ mod tests { } #[tokio::test] - async fn do_not_overwrite_dial_addresses() { + async fn persist_dial_addresses() { let _ = tracing_subscriber::fmt() .with_env_filter(tracing_subscriber::EnvFilter::from_default_env()) .try_init(); @@ -3573,8 +3573,11 @@ mod tests { state => panic!("invalid state: {state:?}"), } - // The address is not saved yet. - assert!(!peer_context.addresses.addresses(usize::MAX).contains(&dial_address)); + // The address is saved for future dials. + assert_eq!( + peer_context.addresses.addresses.get(&dial_address).unwrap().score(), + 0 + ); } let second_address = Multiaddr::empty() @@ -3598,8 +3601,15 @@ mod tests { state => panic!("invalid state: {state:?}"), } - assert!(!peer_context.addresses.addresses(usize::MAX).contains(&dial_address)); - assert!(!peer_context.addresses.addresses(usize::MAX).contains(&second_address)); + // The address is still saved, even if a second dial is not initiated. + assert_eq!( + peer_context.addresses.addresses.get(&dial_address).unwrap().score(), + 0 + ); + assert_eq!( + peer_context.addresses.addresses.get(&second_address).unwrap().score(), + 0 + ); } } From a273531df9de1794eb5510604fd7e62ce7873b38 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 20 Sep 2024 17:19:28 +0300 Subject: [PATCH 39/66] manager/tests: Check dial attempt populated address Signed-off-by: Alexandru Vasile --- src/transport/manager/mod.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/transport/manager/mod.rs b/src/transport/manager/mod.rs index 5e760764..b0ae9c44 100644 --- a/src/transport/manager/mod.rs +++ b/src/transport/manager/mod.rs @@ -3149,7 +3149,9 @@ mod tests { state: PeerState::Connected { record, secondary }, addresses, } => { - assert!(addresses.is_empty()); + // Saved from the dial attempt. + assert_eq!(addresses.addresses.get(&dial_address).unwrap().score(), 0); + assert!(secondary.is_none()); assert_eq!(record.address, dial_address); assert_eq!(record.connection_id, connection_id); From 354f40a8e9a9ef68d88a292b8b21767f4e313d97 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 20 Sep 2024 17:30:46 +0300 Subject: [PATCH 40/66] manager/tests: Check track of addresses Signed-off-by: Alexandru Vasile --- src/transport/manager/mod.rs | 30 +++++++++++++++++++++++------- 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/src/transport/manager/mod.rs b/src/transport/manager/mod.rs index b0ae9c44..9c0a9cd2 100644 --- a/src/transport/manager/mod.rs +++ b/src/transport/manager/mod.rs @@ -773,6 +773,8 @@ impl TransportManager { peer: PeerId, endpoint: &Endpoint, ) -> crate::Result { + self.update_address_on_connection_established(peer, &endpoint); + if let Some(dialed_peer) = self.pending_connections.remove(&endpoint.connection_id()) { if dialed_peer != peer { tracing::warn!( @@ -1173,7 +1175,6 @@ impl TransportManager { } TransportEvent::ConnectionEstablished { peer, endpoint } => { self.opening_errors.remove(&endpoint.connection_id()); - self.update_address_on_connection_established(peer, &endpoint); match self.on_connection_established(peer, &endpoint) { Err(error) => { @@ -2265,7 +2266,7 @@ mod tests { } } - // second connection is established, verify that the seconary connection is tracked + // second connection is established, verify that the secondary connection is tracked let emit_event = manager .on_connection_established( peer, @@ -2308,7 +2309,7 @@ mod tests { context.addresses.addresses.get(&address2).unwrap().score(), scores::CONNECTION_ESTABLISHED ); - assert_eq!(record.connection_id, ConnectionId::from(0usize)); + assert_eq!(record.connection_id, ConnectionId::from(1usize)); } state => panic!("invalid state: {state:?}"), } @@ -2456,7 +2457,7 @@ mod tests { let emit_event = manager .on_connection_established( peer, - &Endpoint::listener(address1, ConnectionId::from(0usize)), + &Endpoint::listener(address1.clone(), ConnectionId::from(0usize)), ) .unwrap(); assert!(std::matches!( @@ -2464,6 +2465,13 @@ mod tests { ConnectionEstablishedResult::Accept )); + // The address1 should be ignored because it is an inbound connection + // initiated from an ephemeral port. + let peers = manager.peers.read(); + let context = peers.get(&peer).unwrap(); + assert!(!context.addresses.addresses.contains_key(&address1)); + drop(peers); + // verify that the peer state is `Connected` with no seconary connection { let peers = manager.peers.read(); @@ -2489,6 +2497,12 @@ mod tests { ConnectionEstablishedResult::Accept )); + // Ensure we keep track of this address. + let peers = manager.peers.read(); + let context = peers.get(&peer).unwrap(); + assert!(context.addresses.addresses.contains_key(&address2)); + drop(peers); + let peers = manager.peers.read(); let context = peers.get(&peer).unwrap(); @@ -2517,7 +2531,9 @@ mod tests { let peers = manager.peers.read(); let context = peers.get(&peer).unwrap(); - assert!(context.addresses.addresses(usize::MAX).contains(&address3)); + // The tertiary connection should be ignored because it is an inbound connection + // initiated from an ephemeral port. + assert!(!context.addresses.addresses.contains_key(&address3)); drop(peers); // close the tertiary connection that was ignored @@ -3224,7 +3240,7 @@ mod tests { assert_eq!(result, ConnectionEstablishedResult::Reject); // Close one connection. - let _ = manager.on_connection_closed(peer, first_connection_id).unwrap(); + assert!(manager.on_connection_closed(peer, first_connection_id).is_none()); // The second peer can establish 2 inbounds now. let result = manager @@ -3315,7 +3331,7 @@ mod tests { )); // Close one connection. - let _ = manager.on_connection_closed(peer, first_connection_id).unwrap(); + assert!(manager.on_connection_closed(peer, first_connection_id).is_none()); // We can now dial again. manager.dial_address(first_addr.clone()).await.unwrap(); From 1eea6788dbc34d3d8a66a736e261a78472079d3b Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 20 Sep 2024 17:40:53 +0300 Subject: [PATCH 41/66] manager: Keep connection record on secondary upgrades Signed-off-by: Alexandru Vasile --- src/transport/manager/mod.rs | 10 ++++++---- src/transport/manager/types.rs | 10 +++++----- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/src/transport/manager/mod.rs b/src/transport/manager/mod.rs index 9c0a9cd2..ffae5002 100644 --- a/src/transport/manager/mod.rs +++ b/src/transport/manager/mod.rs @@ -2106,7 +2106,8 @@ mod tests { .. } => { assert_eq!(secondary_connection.address, address2); - assert!(peer.addresses.addresses.contains_key(&address2)); + // Endpoint::listener addresses are not tracked. + assert!(!peer.addresses.addresses.contains_key(&address2)); assert_eq!( !peer.addresses.addresses.get(&address3).unwrap().score(), scores::CONNECTION_ESTABLISHED @@ -2356,7 +2357,7 @@ mod tests { ConnectionEstablishedResult::Accept )); - // verify that the peer state is `Connected` with no seconary connection + // verify that the peer state is `Connected` with no secondary connection { let peers = manager.peers.read(); let peer = peers.get(&peer).unwrap(); @@ -2369,7 +2370,7 @@ mod tests { } } - // second connection is established, verify that the seconary connection is tracked + // second connection is established, verify that the secondary connection is tracked let emit_event = manager .on_connection_established( peer, @@ -2408,7 +2409,8 @@ mod tests { secondary: None, record, } => { - assert!(context.addresses.addresses.contains_key(&address1)); + assert!(!context.addresses.addresses.contains_key(&address1)); + assert!(context.addresses.addresses.contains_key(&address2)); assert_eq!(record.connection_id, ConnectionId::from(1usize)); } state => panic!("invalid state: {state:?}"), diff --git a/src/transport/manager/types.rs b/src/transport/manager/types.rs index 5bc7bc9a..98a2350e 100644 --- a/src/transport/manager/types.rs +++ b/src/transport/manager/types.rs @@ -181,12 +181,12 @@ impl PeerState { match self { // Transform the dial record into a secondary connection. Self::Connected { + record, secondary: Some(SecondaryOrDialing::Dialing(dial_record)), - .. } => if dial_record.connection_id == connection.connection_id { *self = Self::Connected { - record: connection.clone(), + record: record.clone(), secondary: Some(SecondaryOrDialing::Secondary(connection)), }; @@ -194,10 +194,11 @@ impl PeerState { }, // There's place for a secondary connection. Self::Connected { - secondary: None, .. + record, + secondary: None, } => { *self = Self::Connected { - record: connection.clone(), + record: record.clone(), secondary: Some(SecondaryOrDialing::Secondary(connection)), }; @@ -359,7 +360,6 @@ impl<'a> DisconnectedState<'a> { /// - dialing state for outbound connections. /// - established outbound connections via [`PeerState::Connected`]. /// - established inbound connections via `PeerContext::secondary_connection`. -#[allow(clippy::derived_hash_with_manual_eq)] #[derive(Debug, Clone, Hash, PartialEq)] pub struct ConnectionRecord { /// Address of the connection. From b684142409ae2143a97dc5ecdd63668cd497b0c8 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 20 Sep 2024 17:47:10 +0300 Subject: [PATCH 42/66] manager: Handle dial initiation result friendlier Signed-off-by: Alexandru Vasile --- src/transport/manager/mod.rs | 18 ++++++++++-------- src/transport/manager/types.rs | 23 +++++++++++++++-------- 2 files changed, 25 insertions(+), 16 deletions(-) diff --git a/src/transport/manager/mod.rs b/src/transport/manager/mod.rs index ffae5002..c69f96eb 100644 --- a/src/transport/manager/mod.rs +++ b/src/transport/manager/mod.rs @@ -29,7 +29,7 @@ use crate::{ manager::{ address::AddressRecord, handle::InnerTransportManagerCommand, - types::{ConnectionRecord, PeerContext, PeerState}, + types::{ConnectionRecord, InitiateDialResult, PeerContext, PeerState}, }, Endpoint, Transport, TransportEvent, }, @@ -450,9 +450,9 @@ impl TransportManager { let context = peers.entry(peer).or_insert_with(|| PeerContext::default()); let disconnected_state = match context.state.initiate_dial() { - Ok(disconnected_peer) => disconnected_peer, - // Dialing from an invalid state, or another dial is in progress. - Err(immediate_result) => return immediate_result, + InitiateDialResult::AlreadyConnected => return Err(Error::AlreadyConnected), + InitiateDialResult::DialingInProgress => return Ok(()), + InitiateDialResult::Disconnected(disconnected_state) => disconnected_state, }; // The addresses are sorted by score and contain the remote peer ID. @@ -586,11 +586,13 @@ impl TransportManager { // Keep the provided record around for possible future dials. context.addresses.insert(address_record.clone()); - match context.state.initiate_dial() { - Ok(disconnected_peer) => disconnected_peer.dial_record(dial_record), - // Dialing from an invalid state, or another dial is in progress. - Err(immediate_result) => return immediate_result, + let disconnected_state = match context.state.initiate_dial() { + InitiateDialResult::AlreadyConnected => return Err(Error::AlreadyConnected), + InitiateDialResult::DialingInProgress => return Ok(()), + InitiateDialResult::Disconnected(disconnected_state) => disconnected_state, }; + + disconnected_state.dial_record(dial_record); } self.transports diff --git a/src/transport/manager/types.rs b/src/transport/manager/types.rs index 98a2350e..7a0f3f3f 100644 --- a/src/transport/manager/types.rs +++ b/src/transport/manager/types.rs @@ -21,7 +21,7 @@ use crate::{ transport::{manager::address::AddressStore, Endpoint}, types::ConnectionId, - Error, PeerId, + PeerId, }; use multiaddr::{Multiaddr, Protocol}; @@ -105,7 +105,15 @@ pub enum SecondaryOrDialing { Dialing(ConnectionRecord), } -pub type InitiateDialError = Result<(), Error>; +/// Result of initiating a dial. +pub enum InitiateDialResult<'a> { + /// The peer is already connected. + AlreadyConnected, + /// The dialing state is already in progress. + DialingInProgress, + /// The peer is disconnected, start dialing. + Disconnected(DisconnectedState<'a>), +} impl PeerState { /// Provides a disconnected state object if the peer can initiate a dial. @@ -113,23 +121,22 @@ impl PeerState { /// From the disconnected state, the peer can be dialed on a single address or multiple /// addresses. The provided state leverages the type system to ensure the peer /// can transition gracefully to the next state. - pub fn initiate_dial(&mut self) -> Result { + pub fn initiate_dial(&mut self) -> InitiateDialResult { match self { // The peer is already connected, no need to dial again. - Self::Connected { .. } => { - return Err(Err(Error::AlreadyConnected)); - } + Self::Connected { .. } => return InitiateDialResult::AlreadyConnected, // The dialing state is already in progress, an event will be emitted later. Self::Dialing { .. } | Self::Opening { .. } | Self::Disconnected { dial_record: Some(_), } => { - return Err(Ok(())); + return InitiateDialResult::DialingInProgress; } // The peer is disconnected, start dialing. - Self::Disconnected { dial_record: None } => return Ok(DisconnectedState::new(self)), + Self::Disconnected { dial_record: None } => + return InitiateDialResult::Disconnected(DisconnectedState::new(self)), } } From 461eb65ec117342b60f2d14f26640dbe031b12a2 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 20 Sep 2024 18:10:59 +0300 Subject: [PATCH 43/66] manager: Simplify dial on single and multiaddresses Signed-off-by: Alexandru Vasile --- src/transport/manager/mod.rs | 43 +++++++----- src/transport/manager/types.rs | 119 ++++++++++++++++----------------- 2 files changed, 85 insertions(+), 77 deletions(-) diff --git a/src/transport/manager/mod.rs b/src/transport/manager/mod.rs index c69f96eb..77af5c2b 100644 --- a/src/transport/manager/mod.rs +++ b/src/transport/manager/mod.rs @@ -29,7 +29,7 @@ use crate::{ manager::{ address::AddressRecord, handle::InnerTransportManagerCommand, - types::{ConnectionRecord, InitiateDialResult, PeerContext, PeerState}, + types::{ConnectionRecord, PeerContext, PeerState, StateDialResult}, }, Endpoint, Transport, TransportEvent, }, @@ -449,10 +449,11 @@ impl TransportManager { let context = peers.entry(peer).or_insert_with(|| PeerContext::default()); - let disconnected_state = match context.state.initiate_dial() { - InitiateDialResult::AlreadyConnected => return Err(Error::AlreadyConnected), - InitiateDialResult::DialingInProgress => return Ok(()), - InitiateDialResult::Disconnected(disconnected_state) => disconnected_state, + // Check if dialing is possible before allocating addresses. + match context.state.can_dial() { + StateDialResult::AlreadyConnected => return Err(Error::AlreadyConnected), + StateDialResult::DialingInProgress => return Ok(()), + StateDialResult::Ok => {} }; // The addresses are sorted by score and contain the remote peer ID. @@ -471,11 +472,14 @@ impl TransportManager { ); let transports = Self::open_addresses(&dial_addresses); - disconnected_state.dial_addresses( + + // Dialing addresses will succeed because the `context.state.can_dial()` returned `Ok`. + let result = context.state.dial_addresses( connection_id, dial_addresses.iter().cloned().collect(), transports.keys().cloned().collect(), ); + assert_eq!(result, StateDialResult::Ok); for (transport, addresses) in transports { if addresses.is_empty() { @@ -586,13 +590,11 @@ impl TransportManager { // Keep the provided record around for possible future dials. context.addresses.insert(address_record.clone()); - let disconnected_state = match context.state.initiate_dial() { - InitiateDialResult::AlreadyConnected => return Err(Error::AlreadyConnected), - InitiateDialResult::DialingInProgress => return Ok(()), - InitiateDialResult::Disconnected(disconnected_state) => disconnected_state, + match context.state.dial_single_address(dial_record) { + StateDialResult::AlreadyConnected => return Err(Error::AlreadyConnected), + StateDialResult::DialingInProgress => return Ok(()), + StateDialResult::Ok => {} }; - - disconnected_state.dial_record(dial_record); } self.transports @@ -2049,7 +2051,7 @@ mod tests { let established_result = manager .on_connection_established( peer, - &Endpoint::listener(address1, ConnectionId::from(0usize)), + &Endpoint::dialer(address1.clone(), ConnectionId::from(0usize)), ) .unwrap(); assert_eq!(established_result, ConnectionEstablishedResult::Accept); @@ -2110,8 +2112,9 @@ mod tests { assert_eq!(secondary_connection.address, address2); // Endpoint::listener addresses are not tracked. assert!(!peer.addresses.addresses.contains_key(&address2)); + assert!(!peer.addresses.addresses.contains_key(&address3)); assert_eq!( - !peer.addresses.addresses.get(&address3).unwrap().score(), + peer.addresses.addresses.get(&address1).unwrap().score(), scores::CONNECTION_ESTABLISHED ); } @@ -2263,8 +2266,13 @@ mod tests { match &peer.state { PeerState::Connected { - secondary: None, .. - } => {} + record, + secondary: None, + .. + } => { + // Primary connection is established. + assert_eq!(record.connection_id, ConnectionId::from(0usize)); + } state => panic!("invalid state: {state:?}"), } } @@ -2312,7 +2320,8 @@ mod tests { context.addresses.addresses.get(&address2).unwrap().score(), scores::CONNECTION_ESTABLISHED ); - assert_eq!(record.connection_id, ConnectionId::from(1usize)); + // Primary remains opened. + assert_eq!(record.connection_id, ConnectionId::from(0usize)); } state => panic!("invalid state: {state:?}"), } diff --git a/src/transport/manager/types.rs b/src/transport/manager/types.rs index 7a0f3f3f..99adb1aa 100644 --- a/src/transport/manager/types.rs +++ b/src/transport/manager/types.rs @@ -106,37 +106,83 @@ pub enum SecondaryOrDialing { } /// Result of initiating a dial. -pub enum InitiateDialResult<'a> { +#[derive(Debug, Clone, PartialEq)] +pub enum StateDialResult { /// The peer is already connected. AlreadyConnected, /// The dialing state is already in progress. DialingInProgress, /// The peer is disconnected, start dialing. - Disconnected(DisconnectedState<'a>), + Ok, } impl PeerState { - /// Provides a disconnected state object if the peer can initiate a dial. - /// - /// From the disconnected state, the peer can be dialed on a single address or multiple - /// addresses. The provided state leverages the type system to ensure the peer - /// can transition gracefully to the next state. - pub fn initiate_dial(&mut self) -> InitiateDialResult { + /// Check if the peer can be dialed. + pub fn can_dial(&self) -> StateDialResult { match self { // The peer is already connected, no need to dial again. - Self::Connected { .. } => return InitiateDialResult::AlreadyConnected, + Self::Connected { .. } => return StateDialResult::AlreadyConnected, // The dialing state is already in progress, an event will be emitted later. Self::Dialing { .. } | Self::Opening { .. } | Self::Disconnected { dial_record: Some(_), } => { - return InitiateDialResult::DialingInProgress; + return StateDialResult::DialingInProgress; } - // The peer is disconnected, start dialing. - Self::Disconnected { dial_record: None } => - return InitiateDialResult::Disconnected(DisconnectedState::new(self)), + Self::Disconnected { dial_record: None } => StateDialResult::Ok, + } + } + + /// Dial the peer on a single address. + pub fn dial_single_address(&mut self, dial_record: ConnectionRecord) -> StateDialResult { + let check = self.can_dial(); + if check != StateDialResult::Ok { + return check; + } + + match self { + Self::Disconnected { dial_record: None } => { + *self = PeerState::Dialing { dial_record }; + return StateDialResult::Ok; + } + state => panic!( + "unexpected state: {:?} validated by Self::can_dial; qed", + state + ), + } + } + + /// Dial the peer on multiple addresses. + /// + /// # Transitions + /// + /// [`PeerState::Disconnected`] -> [`PeerState::Opening`] + pub fn dial_addresses( + &mut self, + connection_id: ConnectionId, + addresses: HashSet, + transports: HashSet, + ) -> StateDialResult { + let check = self.can_dial(); + if check != StateDialResult::Ok { + return check; + } + + match self { + Self::Disconnected { dial_record: None } => { + *self = PeerState::Opening { + addresses, + connection_id, + transports, + }; + return StateDialResult::Ok; + } + state => panic!( + "unexpected state: {:?} validated by Self::can_dial; qed", + state + ), } } @@ -308,53 +354,6 @@ impl PeerState { } } -pub struct DisconnectedState<'a> { - state: &'a mut PeerState, -} - -impl<'a> DisconnectedState<'a> { - /// Constructs a new [`DisconnectedState`]. - /// - /// # Panics - /// - /// Panics if the state is not [`PeerState::Disconnected`]. - fn new(state: &'a mut PeerState) -> Self { - assert!(matches!( - state, - PeerState::Disconnected { dial_record: None } - )); - - Self { state } - } - - /// Dial the peer on a single address. - /// - /// # Transitions - /// - /// [`PeerState::Disconnected`] -> [`PeerState::Dialing`] - pub fn dial_record(self, dial_record: ConnectionRecord) { - *self.state = PeerState::Dialing { dial_record }; - } - - /// Dial the peer on multiple addresses. - /// - /// # Transitions - /// - /// [`PeerState::Disconnected`] -> [`PeerState::Opening`] - pub fn dial_addresses( - self, - connection_id: ConnectionId, - addresses: HashSet, - transports: HashSet, - ) { - *self.state = PeerState::Opening { - addresses, - connection_id, - transports, - }; - } -} - /// The connection record keeps track of the connection ID and the address of the connection. /// /// The connection ID is used to track the connection in the transport layer. From a1dfe2ec3161778cbe4a967adf9e23a52b3ae7d5 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 20 Sep 2024 18:14:52 +0300 Subject: [PATCH 44/66] manager/handle: Use state transitions Signed-off-by: Alexandru Vasile --- src/transport/manager/handle.rs | 47 +++++++++++---------------------- 1 file changed, 16 insertions(+), 31 deletions(-) diff --git a/src/transport/manager/handle.rs b/src/transport/manager/handle.rs index 1a367bbb..abefa97c 100644 --- a/src/transport/manager/handle.rs +++ b/src/transport/manager/handle.rs @@ -26,7 +26,7 @@ use crate::{ protocol::ProtocolSet, transport::manager::{ address::AddressRecord, - types::{PeerContext, PeerState, SupportedTransport}, + types::{PeerContext, StateDialResult, SupportedTransport}, ProtocolContext, TransportManagerEvent, LOG_TARGET, }, types::{protocol::ProtocolName, ConnectionId}, @@ -242,36 +242,21 @@ impl TransportManagerHandle { } { - match self.peers.read().get(peer) { - Some(PeerContext { - state: PeerState::Connected { .. }, - .. - }) => return Err(ImmediateDialError::AlreadyConnected), - Some(PeerContext { - state: PeerState::Disconnected { dial_record }, - addresses, - .. - }) => { - if addresses.is_empty() { - return Err(ImmediateDialError::NoAddressAvailable); - } - - // peer is already being dialed, don't dial again until the first dial concluded - if dial_record.is_some() { - tracing::debug!( - target: LOG_TARGET, - ?peer, - ?dial_record, - "peer is aready being dialed", - ); - return Ok(()); - } - } - Some(PeerContext { - state: PeerState::Dialing { .. } | PeerState::Opening { .. }, - .. - }) => return Ok(()), - None => return Err(ImmediateDialError::NoAddressAvailable), + let peers = self.peers.read(); + let Some(PeerContext { state, addresses }) = peers.get(peer) else { + return Err(ImmediateDialError::NoAddressAvailable); + }; + + match state.can_dial() { + StateDialResult::AlreadyConnected => + return Err(ImmediateDialError::AlreadyConnected), + StateDialResult::DialingInProgress => return Ok(()), + StateDialResult::Ok => {} + }; + + // Check if we have enough addresses to dial. + if addresses.is_empty() { + return Err(ImmediateDialError::NoAddressAvailable); } } From 6d455d06dcc5739f7183eb132d8bbc93daa12db0 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 20 Sep 2024 18:44:19 +0300 Subject: [PATCH 45/66] manager/peer_state: Add documentation Signed-off-by: Alexandru Vasile --- src/transport/manager/types.rs | 63 +++++++++++++++++++++++++++------- 1 file changed, 50 insertions(+), 13 deletions(-) diff --git a/src/transport/manager/types.rs b/src/transport/manager/types.rs index 99adb1aa..405e886f 100644 --- a/src/transport/manager/types.rs +++ b/src/transport/manager/types.rs @@ -48,7 +48,55 @@ pub enum SupportedTransport { WebSocket, } -/// Peer state. +/// The peer state that tracks connections and dialing attempts. +/// +/// # State Machine +/// +/// ## [`PeerState::Disconnected`] +/// +/// Initially, the peer is in the [`PeerState::Disconnected`] state without a +/// [`PeerState::Disconnected::dial_record`]. This means the peer is fully disconnected. +/// +/// Next states: +/// - [`PeerState::Disconnected`] -> [`PeerState::Dialing`] (via [`PeerState::dial_single_address`]) +/// - [`PeerState::Disconnected`] -> [`PeerState::Opening`] (via [`PeerState::dial_addresses`]) +/// +/// ## [`PeerState::Dialing`] +/// +/// The peer can transition to the [`PeerState::Dialing`] state when a dialing attempt is +/// initiated. This only happens when the peer is dialed on a single address via +/// [`PeerState::dial_single_address`]. +/// +/// The dialing state implies the peer is reached on the socket address provided, as well as +/// negotiating noise and yamux protocols. +/// +/// Next states: +/// - [`PeerState::Dialing`] -> [`PeerState::Connected`] (via +/// [`PeerState::on_connection_established`]) +/// - [`PeerState::Dialing`] -> [`PeerState::Disconnected`] (via [`PeerState::on_dial_failure`]) +/// +/// ## [`PeerState::Opening`] +/// +/// The peer can transition to the [`PeerState::Opening`] state when a dialing attempt is +/// initiated on multiple addresses via [`PeerState::dial_addresses`]. This takes into account +/// the parallelism factor (8 maximum) of the dialing attempts. +/// +/// The opening state holds information about which protocol is being dialed to properly report back +/// errors. +/// +/// The opening state is similar to the dial state, however the peer is only reached on a socket +/// address. The noise and yamux protocols are not negotiated yet. This state transitions to +/// [`PeerState::Dialing`] for the final part of the negotiation. Please note that it would be +/// wasteful to negotiate the noise and yamux protocols on all addresses, since only one +/// connection is kept around. +/// +/// This is something we'll reconsider in the future if we encounter issues. +/// +/// Next states: +/// - [`PeerState::Opening`] -> [`PeerState::Dialing`] (via transport manager +/// `on_connection_opened`) +/// - [`PeerState::Opening`] -> [`PeerState::Disconnected`] (via transport manager +/// `on_connection_opened` if negotiation cannot be started or via `on_open_failure`) #[derive(Debug, Clone, PartialEq)] pub enum PeerState { /// `Litep2p` is connected to peer. @@ -155,10 +203,6 @@ impl PeerState { } /// Dial the peer on multiple addresses. - /// - /// # Transitions - /// - /// [`PeerState::Disconnected`] -> [`PeerState::Opening`] pub fn dial_addresses( &mut self, connection_id: ConnectionId, @@ -188,19 +232,16 @@ impl PeerState { /// Handle dial failure. /// - /// Returns `true` if the dial record was cleared, false otherwise. - /// /// # Transitions /// - [`PeerState::Dialing`] (with record) -> [`PeerState::Disconnected`] /// - [`PeerState::Connected`] (with dial record) -> [`PeerState::Connected`] /// - [`PeerState::Disconnected`] (with dial record) -> [`PeerState::Disconnected`] - pub fn on_dial_failure(&mut self, connection_id: ConnectionId) -> bool { + pub fn on_dial_failure(&mut self, connection_id: ConnectionId) { match self { // Clear the dial record if the connection ID matches. Self::Dialing { dial_record } => if dial_record.connection_id == connection_id { *self = Self::Disconnected { dial_record: None }; - return true; }, Self::Connected { @@ -212,7 +253,6 @@ impl PeerState { record: record.clone(), secondary: None, }; - return true; }, Self::Disconnected { @@ -220,13 +260,10 @@ impl PeerState { } => if dial_record.connection_id == connection_id { *self = Self::Disconnected { dial_record: None }; - return true; }, _ => (), }; - - return false; } /// Returns `true` if the connection should be accepted by the transport manager. From 808793c343b2f7536d5cfcf29feb00d9ab6b090b Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 20 Sep 2024 18:44:29 +0300 Subject: [PATCH 46/66] manager: Document possible race Signed-off-by: Alexandru Vasile --- src/transport/manager/handle.rs | 5 ++++- src/transport/manager/mod.rs | 8 ++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/src/transport/manager/handle.rs b/src/transport/manager/handle.rs index abefa97c..00ac641e 100644 --- a/src/transport/manager/handle.rs +++ b/src/transport/manager/handle.rs @@ -316,7 +316,10 @@ impl TransportHandle { #[cfg(test)] mod tests { - use crate::transport::manager::{address::AddressStore, types::ConnectionRecord}; + use crate::transport::manager::{ + address::AddressStore, + types::{ConnectionRecord, PeerState}, + }; use super::*; use multihash::Multihash; diff --git a/src/transport/manager/mod.rs b/src/transport/manager/mod.rs index 77af5c2b..fbec02dc 100644 --- a/src/transport/manager/mod.rs +++ b/src/transport/manager/mod.rs @@ -843,6 +843,14 @@ impl TransportManager { // since an inbound connection was removed, the outbound connection can be // removed from pending dials + // + // TODO: This may race in the following scenario: + // + // T0: we open address X on protocol TCP + // T1: remote peer opens a connection with us + // T2: address X is dialed and event is propagated from TCP to transport manager + // T3: `on_connection_established` is called for T1 and pending connections cleared + // T4: event from T2 is delivered. self.pending_connections.remove(&connection_id); } From dc6073d8f9605cf926630724c576642418ae7049 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 20 Sep 2024 19:23:19 +0300 Subject: [PATCH 47/66] manager/peer_state: Ensure disconnects are reported properly Signed-off-by: Alexandru Vasile --- src/transport/manager/types.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/transport/manager/types.rs b/src/transport/manager/types.rs index 405e886f..ff6746f3 100644 --- a/src/transport/manager/types.rs +++ b/src/transport/manager/types.rs @@ -359,12 +359,12 @@ impl PeerState { dial_record: Some(dial_record.clone()), }; - // This is the only case where the connection transitions from - // [`PeerState::Connected`] to [`PeerState::Disconnected`]. return true; } None => { *self = Self::Disconnected { dial_record: None }; + + return true; } }; From 0c36972de7a7574d641048ac00e0cb868b1dd11e Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 20 Sep 2024 19:24:55 +0300 Subject: [PATCH 48/66] transport/tests: Connections are reported properly now Signed-off-by: Alexandru Vasile --- src/transport/manager/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/transport/manager/mod.rs b/src/transport/manager/mod.rs index fbec02dc..a30f2b50 100644 --- a/src/transport/manager/mod.rs +++ b/src/transport/manager/mod.rs @@ -3352,7 +3352,7 @@ mod tests { )); // Close one connection. - assert!(manager.on_connection_closed(peer, first_connection_id).is_none()); + assert!(manager.on_connection_closed(peer, first_connection_id).is_some()); // We can now dial again. manager.dial_address(first_addr.clone()).await.unwrap(); From ecff792197b1a8f681468204899ed5f4391951d6 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Mon, 23 Sep 2024 18:47:44 +0300 Subject: [PATCH 49/66] transport/manager: Move error to score to the address store Signed-off-by: Alexandru Vasile --- src/transport/manager/address.rs | 21 +++++++++++++- src/transport/manager/mod.rs | 50 ++++++++++++-------------------- 2 files changed, 38 insertions(+), 33 deletions(-) diff --git a/src/transport/manager/address.rs b/src/transport/manager/address.rs index e54602d8..155d2d08 100644 --- a/src/transport/manager/address.rs +++ b/src/transport/manager/address.rs @@ -18,7 +18,10 @@ // FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER // DEALINGS IN THE SOFTWARE. -use crate::PeerId; +use crate::{ + error::{DialError, NegotiationError}, + PeerId, +}; use std::collections::{hash_map::Entry, HashMap}; @@ -187,6 +190,22 @@ impl AddressStore { } } + /// Get the score for a given error. + pub fn error_score(error: &DialError) -> i32 { + match error { + DialError::Timeout => scores::CONNECTION_ESTABLISHED, + DialError::AddressError(_) => scores::CONNECTION_FAILURE, + DialError::DnsError(_) => scores::CONNECTION_FAILURE, + DialError::NegotiationError(negotiation_error) => match negotiation_error { + NegotiationError::PeerIdMismatch(_, _) => scores::DIFFERENT_PEER_ID, + // Timeout during the negotiation phase. + NegotiationError::Timeout => scores::TIMEOUT_FAILURE, + // Treat other errors as connection failures. + _ => scores::CONNECTION_FAILURE, + }, + } + } + /// Check if [`AddressStore`] is empty. pub fn is_empty(&self) -> bool { self.addresses.is_empty() diff --git a/src/transport/manager/mod.rs b/src/transport/manager/mod.rs index a30f2b50..f181f69d 100644 --- a/src/transport/manager/mod.rs +++ b/src/transport/manager/mod.rs @@ -37,7 +37,7 @@ use crate::{ BandwidthSink, PeerId, }; -use address::scores; +use address::{scores, AddressStore}; use futures::{Stream, StreamExt}; use indexmap::IndexMap; use multiaddr::{Multiaddr, Protocol}; @@ -612,39 +612,25 @@ impl TransportManager { fn update_address_on_dial_failure(&mut self, mut address: Multiaddr, error: &DialError) { let mut peers = self.peers.write(); - let score = match error { - DialError::Timeout => scores::CONNECTION_ESTABLISHED, - DialError::AddressError(_) => scores::CONNECTION_FAILURE, - DialError::DnsError(_) => scores::CONNECTION_FAILURE, - DialError::NegotiationError(negotiation_error) => match negotiation_error { - // Check if the address corresponds to a different peer ID than the one we're - // dialing. This can happen if the node operation restarts the node. - // - // In this case the address is reachable, however the peer ID is different. - // Keep track of this address for future dials. - // - // Note: this is happening quite often in practice and is the primary reason - // of negotiation failures. - NegotiationError::PeerIdMismatch(_, provided) => { - let context = peers.entry(*provided).or_insert_with(|| PeerContext::default()); + let score = AddressStore::error_score(error); - if !std::matches!(address.iter().last(), Some(Protocol::P2p(_))) { - address.pop(); - } - context.addresses.insert(AddressRecord::new( - &provided, - address.clone(), - scores::DIFFERENT_PEER_ID, - )); + // Check if the address corresponds to a different peer ID than the one we're + // dialing. This can happen if the node operation restarts the node. + // + // In this case the address is reachable, however the peer ID is different. + // Keep track of this address for future dials. + // + // Note: this is happening quite often in practice and is the primary reason + if let DialError::NegotiationError(NegotiationError::PeerIdMismatch(_, provided)) = error { + let context = peers.entry(*provided).or_insert_with(|| PeerContext::default()); - return; - } - // Timeout during the negotiation phase. - NegotiationError::Timeout => scores::TIMEOUT_FAILURE, - // Treat other errors as connection failures. - _ => scores::CONNECTION_FAILURE, - }, - }; + if !std::matches!(address.iter().last(), Some(Protocol::P2p(_))) { + address.pop(); + } + context.addresses.insert(AddressRecord::new(&provided, address.clone(), score)); + + return; + } // Extract the peer ID at this point to give `NegotiationError::PeerIdMismatch` a chance to // propagate. From 8a95ba8a033dce49fef7e4e201d5b3e438bee2b3 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Mon, 23 Sep 2024 18:51:25 +0300 Subject: [PATCH 50/66] transport/maanager: Update address score on open failure Signed-off-by: Alexandru Vasile --- src/transport/manager/mod.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/transport/manager/mod.rs b/src/transport/manager/mod.rs index f181f69d..8bee1bc5 100644 --- a/src/transport/manager/mod.rs +++ b/src/transport/manager/mod.rs @@ -1238,6 +1238,10 @@ impl TransportManager { } } TransportEvent::OpenFailure { connection_id, errors } => { + for (address, error) in &errors { + self.update_address_on_dial_failure(address.clone(), error); + } + match self.on_open_failure(transport, connection_id) { Err(error) => tracing::debug!( target: LOG_TARGET, From cd8ffb174b87b6b98503acd79ba7e41cb0d0194b Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Mon, 23 Sep 2024 19:11:35 +0300 Subject: [PATCH 51/66] peer_state: Move open failure transitions to dedicated module Signed-off-by: Alexandru Vasile --- src/transport/manager/mod.rs | 89 ++++++++++------------------------ src/transport/manager/types.rs | 17 +++++++ 2 files changed, 43 insertions(+), 63 deletions(-) diff --git a/src/transport/manager/mod.rs b/src/transport/manager/mod.rs index 8bee1bc5..e1d15717 100644 --- a/src/transport/manager/mod.rs +++ b/src/transport/manager/mod.rs @@ -975,78 +975,41 @@ impl TransportManager { }; let mut peers = self.peers.write(); - let context = peers.get_mut(&peer).ok_or_else(|| { + let context = peers.entry(peer).or_insert_with(|| PeerContext::default()); + + let previous_state = context.state.clone(); + let last_transport = context.state.on_open_failure(transport); + + if context.state == previous_state { tracing::warn!( target: LOG_TARGET, ?peer, ?connection_id, - "open failure but peer doesn't exist", + ?transport, + state = ?context.state, + "invalid state for a open failure", ); - debug_assert!(false); - Error::InvalidState - })?; - - match std::mem::replace( - &mut context.state, - PeerState::Disconnected { dial_record: None }, - ) { - PeerState::Opening { - addresses, - connection_id, - mut transports, - } => { - tracing::trace!( - target: LOG_TARGET, - ?peer, - ?connection_id, - ?transport, - "open failure for peer", - ); - transports.remove(&transport); - - if transports.is_empty() { - for address in addresses { - context.addresses.insert(AddressRecord::new( - &peer, - address, - scores::CONNECTION_FAILURE, - )); - } - - tracing::trace!( - target: LOG_TARGET, - ?peer, - ?connection_id, - "open failure for last transport", - ); - - return Ok(Some(peer)); - } - - self.pending_connections.insert(connection_id, peer); - context.state = PeerState::Opening { - addresses, - connection_id, - transports, - }; + return Err(Error::InvalidState); + } - Ok(None) - } - state => { - tracing::warn!( - target: LOG_TARGET, - ?peer, - ?connection_id, - ?state, - "open failure but `PeerState` is not `Opening`", - ); - context.state = state; + tracing::trace!( + target: LOG_TARGET, + ?peer, + ?connection_id, + ?transport, + ?previous_state, + state = ?context.state, + "on open failure transition completed" + ); - debug_assert!(false); - Err(Error::InvalidState) - } + if last_transport { + tracing::trace!(target: LOG_TARGET, ?peer, ?connection_id, "open failure for last transport"); + // Provide the peer to notify the open failure. + return Ok(Some(peer)); } + + Ok(None) } /// Poll next event from [`crate::transport::manager::TransportManager`]. diff --git a/src/transport/manager/types.rs b/src/transport/manager/types.rs index ff6746f3..9a9257f0 100644 --- a/src/transport/manager/types.rs +++ b/src/transport/manager/types.rs @@ -389,6 +389,23 @@ impl PeerState { false } + + /// Returns `true` if the last transport failed to open. + pub fn on_open_failure(&mut self, transport: SupportedTransport) -> bool { + match self { + Self::Opening { transports, .. } => { + transports.remove(&transport); + + if transports.is_empty() { + *self = Self::Disconnected { dial_record: None }; + return true; + } + + return false; + } + _ => false, + } + } } /// The connection record keeps track of the connection ID and the address of the connection. From bda30dfe76186161765ef9838406bbef96902838 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Mon, 23 Sep 2024 20:06:23 +0300 Subject: [PATCH 52/66] transport/manager: Move to dedicated module Signed-off-by: Alexandru Vasile --- src/transport/manager/handle.rs | 5 +- src/transport/manager/mod.rs | 6 +- src/transport/manager/peer_state.rs | 444 ++++++++++++++++++++++++++++ src/transport/manager/types.rs | 423 +------------------------- 4 files changed, 452 insertions(+), 426 deletions(-) create mode 100644 src/transport/manager/peer_state.rs diff --git a/src/transport/manager/handle.rs b/src/transport/manager/handle.rs index 00ac641e..ce3c0173 100644 --- a/src/transport/manager/handle.rs +++ b/src/transport/manager/handle.rs @@ -26,7 +26,8 @@ use crate::{ protocol::ProtocolSet, transport::manager::{ address::AddressRecord, - types::{PeerContext, StateDialResult, SupportedTransport}, + peer_state::StateDialResult, + types::{PeerContext, SupportedTransport}, ProtocolContext, TransportManagerEvent, LOG_TARGET, }, types::{protocol::ProtocolName, ConnectionId}, @@ -318,7 +319,7 @@ impl TransportHandle { mod tests { use crate::transport::manager::{ address::AddressStore, - types::{ConnectionRecord, PeerState}, + peer_state::{ConnectionRecord, PeerState}, }; use super::*; diff --git a/src/transport/manager/mod.rs b/src/transport/manager/mod.rs index e1d15717..a294a279 100644 --- a/src/transport/manager/mod.rs +++ b/src/transport/manager/mod.rs @@ -29,7 +29,8 @@ use crate::{ manager::{ address::AddressRecord, handle::InnerTransportManagerCommand, - types::{ConnectionRecord, PeerContext, PeerState, StateDialResult}, + peer_state::{ConnectionRecord, PeerState, StateDialResult}, + types::PeerContext, }, Endpoint, Transport, TransportEvent, }, @@ -61,6 +62,7 @@ pub use types::SupportedTransport; mod address; pub mod limits; +mod peer_state; mod types; pub(crate) mod handle; @@ -1301,7 +1303,7 @@ impl TransportManager { #[cfg(test)] mod tests { - use crate::transport::manager::{address::AddressStore, types::SecondaryOrDialing}; + use crate::transport::manager::{address::AddressStore, peer_state::SecondaryOrDialing}; use limits::ConnectionLimitsConfig; use multihash::Multihash; diff --git a/src/transport/manager/peer_state.rs b/src/transport/manager/peer_state.rs new file mode 100644 index 00000000..4d973bb3 --- /dev/null +++ b/src/transport/manager/peer_state.rs @@ -0,0 +1,444 @@ +// Copyright 2024 litep2p developers +// +// Permission is hereby granted, free of charge, to any person obtaining a +// copy of this software and associated documentation files (the "Software"), +// to deal in the Software without restriction, including without limitation +// the rights to use, copy, modify, merge, publish, distribute, sublicense, +// and/or sell copies of the Software, and to permit persons to whom the +// Software is furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS +// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING +// FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER +// DEALINGS IN THE SOFTWARE. + +//! Peer state management. + +use crate::{ + transport::{manager::SupportedTransport, Endpoint}, + types::ConnectionId, + PeerId, +}; + +use multiaddr::{Multiaddr, Protocol}; +use multihash::Multihash; + +use std::collections::HashSet; + +/// The peer state that tracks connections and dialing attempts. +/// +/// # State Machine +/// +/// ## [`PeerState::Disconnected`] +/// +/// Initially, the peer is in the [`PeerState::Disconnected`] state without a +/// [`PeerState::Disconnected::dial_record`]. This means the peer is fully disconnected. +/// +/// Next states: +/// - [`PeerState::Disconnected`] -> [`PeerState::Dialing`] (via [`PeerState::dial_single_address`]) +/// - [`PeerState::Disconnected`] -> [`PeerState::Opening`] (via [`PeerState::dial_addresses`]) +/// +/// ## [`PeerState::Dialing`] +/// +/// The peer can transition to the [`PeerState::Dialing`] state when a dialing attempt is +/// initiated. This only happens when the peer is dialed on a single address via +/// [`PeerState::dial_single_address`]. +/// +/// The dialing state implies the peer is reached on the socket address provided, as well as +/// negotiating noise and yamux protocols. +/// +/// Next states: +/// - [`PeerState::Dialing`] -> [`PeerState::Connected`] (via +/// [`PeerState::on_connection_established`]) +/// - [`PeerState::Dialing`] -> [`PeerState::Disconnected`] (via [`PeerState::on_dial_failure`]) +/// +/// ## [`PeerState::Opening`] +/// +/// The peer can transition to the [`PeerState::Opening`] state when a dialing attempt is +/// initiated on multiple addresses via [`PeerState::dial_addresses`]. This takes into account +/// the parallelism factor (8 maximum) of the dialing attempts. +/// +/// The opening state holds information about which protocol is being dialed to properly report back +/// errors. +/// +/// The opening state is similar to the dial state, however the peer is only reached on a socket +/// address. The noise and yamux protocols are not negotiated yet. This state transitions to +/// [`PeerState::Dialing`] for the final part of the negotiation. Please note that it would be +/// wasteful to negotiate the noise and yamux protocols on all addresses, since only one +/// connection is kept around. +/// +/// This is something we'll reconsider in the future if we encounter issues. +/// +/// Next states: +/// - [`PeerState::Opening`] -> [`PeerState::Dialing`] (via transport manager +/// `on_connection_opened`) +/// - [`PeerState::Opening`] -> [`PeerState::Disconnected`] (via transport manager +/// `on_connection_opened` if negotiation cannot be started or via `on_open_failure`) +#[derive(Debug, Clone, PartialEq)] +pub enum PeerState { + /// `Litep2p` is connected to peer. + Connected { + /// The established record of the connection. + record: ConnectionRecord, + + /// Secondary record, this can either be a dial record or an established connection. + /// + /// While the local node was dialing a remote peer, the remote peer might've dialed + /// the local node and connection was established successfully. This dial address + /// is stored for processing later when the dial attempt concluded as either + /// successful/failed. + secondary: Option, + }, + + /// Connection to peer is opening over one or more addresses. + Opening { + /// Address records used for dialing. + addresses: HashSet, + + /// Connection ID. + connection_id: ConnectionId, + + /// Active transports. + transports: HashSet, + }, + + /// Peer is being dialed. + Dialing { + /// Address record. + dial_record: ConnectionRecord, + }, + + /// `Litep2p` is not connected to peer. + Disconnected { + /// Dial address, if it exists. + /// + /// While the local node was dialing a remote peer, the remote peer might've dialed + /// the local node and connection was established successfully. The connection might've + /// been closed before the dial concluded which means that + /// [`crate::transport::manager::TransportManager`] must be prepared to handle the dial + /// failure even after the connection has been closed. + dial_record: Option, + }, +} + +/// The state of the secondary connection. +#[derive(Debug, Clone, PartialEq)] +pub enum SecondaryOrDialing { + /// The secondary connection is established. + Secondary(ConnectionRecord), + /// The primary connection is established, but the secondary connection is still dialing. + Dialing(ConnectionRecord), +} + +/// Result of initiating a dial. +#[derive(Debug, Clone, PartialEq)] +pub enum StateDialResult { + /// The peer is already connected. + AlreadyConnected, + /// The dialing state is already in progress. + DialingInProgress, + /// The peer is disconnected, start dialing. + Ok, +} + +impl PeerState { + /// Check if the peer can be dialed. + pub fn can_dial(&self) -> StateDialResult { + match self { + // The peer is already connected, no need to dial again. + Self::Connected { .. } => return StateDialResult::AlreadyConnected, + // The dialing state is already in progress, an event will be emitted later. + Self::Dialing { .. } + | Self::Opening { .. } + | Self::Disconnected { + dial_record: Some(_), + } => { + return StateDialResult::DialingInProgress; + } + + Self::Disconnected { dial_record: None } => StateDialResult::Ok, + } + } + + /// Dial the peer on a single address. + pub fn dial_single_address(&mut self, dial_record: ConnectionRecord) -> StateDialResult { + let check = self.can_dial(); + if check != StateDialResult::Ok { + return check; + } + + match self { + Self::Disconnected { dial_record: None } => { + *self = PeerState::Dialing { dial_record }; + return StateDialResult::Ok; + } + state => panic!( + "unexpected state: {:?} validated by Self::can_dial; qed", + state + ), + } + } + + /// Dial the peer on multiple addresses. + pub fn dial_addresses( + &mut self, + connection_id: ConnectionId, + addresses: HashSet, + transports: HashSet, + ) -> StateDialResult { + let check = self.can_dial(); + if check != StateDialResult::Ok { + return check; + } + + match self { + Self::Disconnected { dial_record: None } => { + *self = PeerState::Opening { + addresses, + connection_id, + transports, + }; + return StateDialResult::Ok; + } + state => panic!( + "unexpected state: {:?} validated by Self::can_dial; qed", + state + ), + } + } + + /// Handle dial failure. + /// + /// # Transitions + /// - [`PeerState::Dialing`] (with record) -> [`PeerState::Disconnected`] + /// - [`PeerState::Connected`] (with dial record) -> [`PeerState::Connected`] + /// - [`PeerState::Disconnected`] (with dial record) -> [`PeerState::Disconnected`] + pub fn on_dial_failure(&mut self, connection_id: ConnectionId) { + match self { + // Clear the dial record if the connection ID matches. + Self::Dialing { dial_record } => + if dial_record.connection_id == connection_id { + *self = Self::Disconnected { dial_record: None }; + }, + + Self::Connected { + record, + secondary: Some(SecondaryOrDialing::Dialing(dial_record)), + } => + if dial_record.connection_id == connection_id { + *self = Self::Connected { + record: record.clone(), + secondary: None, + }; + }, + + Self::Disconnected { + dial_record: Some(dial_record), + } => + if dial_record.connection_id == connection_id { + *self = Self::Disconnected { dial_record: None }; + }, + + _ => (), + }; + } + + /// Returns `true` if the connection should be accepted by the transport manager. + pub fn on_connection_established(&mut self, connection: ConnectionRecord) -> bool { + match self { + // Transform the dial record into a secondary connection. + Self::Connected { + record, + secondary: Some(SecondaryOrDialing::Dialing(dial_record)), + } => + if dial_record.connection_id == connection.connection_id { + *self = Self::Connected { + record: record.clone(), + secondary: Some(SecondaryOrDialing::Secondary(connection)), + }; + + return true; + }, + // There's place for a secondary connection. + Self::Connected { + record, + secondary: None, + } => { + *self = Self::Connected { + record: record.clone(), + secondary: Some(SecondaryOrDialing::Secondary(connection)), + }; + + return true; + } + + // Convert the dial record into a primary connection or preserve it. + Self::Dialing { dial_record } + | Self::Disconnected { + dial_record: Some(dial_record), + } => + if dial_record.connection_id == connection.connection_id { + *self = Self::Connected { + record: connection.clone(), + secondary: None, + }; + return true; + } else { + *self = Self::Connected { + record: connection, + secondary: Some(SecondaryOrDialing::Dialing(dial_record.clone())), + }; + return true; + }, + + Self::Disconnected { dial_record: None } => { + *self = Self::Connected { + record: connection, + secondary: None, + }; + + return true; + } + + // Accept the incoming connection. + Self::Opening { .. } => { + *self = Self::Connected { + record: connection, + secondary: None, + }; + + return true; + } + + _ => {} + }; + + return false; + } + + /// Returns `true` if the connection was closed. + pub fn on_connection_closed(&mut self, connection_id: ConnectionId) -> bool { + match self { + Self::Connected { record, secondary } => { + // Primary connection closed. + if record.connection_id == connection_id { + match secondary { + // Promote secondary connection to primary. + Some(SecondaryOrDialing::Secondary(secondary)) => { + *self = Self::Connected { + record: secondary.clone(), + secondary: None, + }; + } + // Preserve the dial record. + Some(SecondaryOrDialing::Dialing(dial_record)) => { + *self = Self::Disconnected { + dial_record: Some(dial_record.clone()), + }; + + return true; + } + None => { + *self = Self::Disconnected { dial_record: None }; + + return true; + } + }; + + return false; + } + + match secondary { + // Secondary connection closed. + Some(SecondaryOrDialing::Secondary(secondary)) + if secondary.connection_id == connection_id => + { + *self = Self::Connected { + record: record.clone(), + secondary: None, + }; + } + _ => (), + } + } + _ => (), + } + + false + } + + /// Returns `true` if the last transport failed to open. + pub fn on_open_failure(&mut self, transport: SupportedTransport) -> bool { + match self { + Self::Opening { transports, .. } => { + transports.remove(&transport); + + if transports.is_empty() { + *self = Self::Disconnected { dial_record: None }; + return true; + } + + return false; + } + _ => false, + } + } +} + +/// The connection record keeps track of the connection ID and the address of the connection. +/// +/// The connection ID is used to track the connection in the transport layer. +/// While the address is used to keep a healthy view of the network for dialing purposes. +/// +/// # Note +/// +/// The structure is used to keep track of: +/// +/// - dialing state for outbound connections. +/// - established outbound connections via [`PeerState::Connected`]. +/// - established inbound connections via `PeerContext::secondary_connection`. +#[derive(Debug, Clone, Hash, PartialEq)] +pub struct ConnectionRecord { + /// Address of the connection. + /// + /// The address must contain the peer ID extension `/p2p/`. + pub address: Multiaddr, + + /// Connection ID resulted from dialing. + pub connection_id: ConnectionId, +} + +impl ConnectionRecord { + /// Construct a new connection record. + pub fn new(peer: PeerId, address: Multiaddr, connection_id: ConnectionId) -> Self { + Self { + address: Self::ensure_peer_id(peer, address), + connection_id, + } + } + + /// Create a new connection record from the peer ID and the endpoint. + pub fn from_endpoint(peer: PeerId, endpoint: &Endpoint) -> Self { + Self { + address: Self::ensure_peer_id(peer, endpoint.address().clone()), + connection_id: endpoint.connection_id(), + } + } + + /// Ensures the peer ID is present in the address. + fn ensure_peer_id(peer: PeerId, address: Multiaddr) -> Multiaddr { + if !std::matches!(address.iter().last(), Some(Protocol::P2p(_))) { + address.with(Protocol::P2p( + Multihash::from_bytes(&peer.to_bytes()).expect("valid peer id"), + )) + } else { + address + } + } +} diff --git a/src/transport/manager/types.rs b/src/transport/manager/types.rs index 9a9257f0..15eb2c50 100644 --- a/src/transport/manager/types.rs +++ b/src/transport/manager/types.rs @@ -18,16 +18,7 @@ // FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER // DEALINGS IN THE SOFTWARE. -use crate::{ - transport::{manager::address::AddressStore, Endpoint}, - types::ConnectionId, - PeerId, -}; - -use multiaddr::{Multiaddr, Protocol}; -use multihash::Multihash; - -use std::collections::HashSet; +use crate::transport::manager::{address::AddressStore, peer_state::PeerState}; /// Supported protocols. #[derive(Debug, Copy, Clone, Hash, PartialEq, Eq)] @@ -48,418 +39,6 @@ pub enum SupportedTransport { WebSocket, } -/// The peer state that tracks connections and dialing attempts. -/// -/// # State Machine -/// -/// ## [`PeerState::Disconnected`] -/// -/// Initially, the peer is in the [`PeerState::Disconnected`] state without a -/// [`PeerState::Disconnected::dial_record`]. This means the peer is fully disconnected. -/// -/// Next states: -/// - [`PeerState::Disconnected`] -> [`PeerState::Dialing`] (via [`PeerState::dial_single_address`]) -/// - [`PeerState::Disconnected`] -> [`PeerState::Opening`] (via [`PeerState::dial_addresses`]) -/// -/// ## [`PeerState::Dialing`] -/// -/// The peer can transition to the [`PeerState::Dialing`] state when a dialing attempt is -/// initiated. This only happens when the peer is dialed on a single address via -/// [`PeerState::dial_single_address`]. -/// -/// The dialing state implies the peer is reached on the socket address provided, as well as -/// negotiating noise and yamux protocols. -/// -/// Next states: -/// - [`PeerState::Dialing`] -> [`PeerState::Connected`] (via -/// [`PeerState::on_connection_established`]) -/// - [`PeerState::Dialing`] -> [`PeerState::Disconnected`] (via [`PeerState::on_dial_failure`]) -/// -/// ## [`PeerState::Opening`] -/// -/// The peer can transition to the [`PeerState::Opening`] state when a dialing attempt is -/// initiated on multiple addresses via [`PeerState::dial_addresses`]. This takes into account -/// the parallelism factor (8 maximum) of the dialing attempts. -/// -/// The opening state holds information about which protocol is being dialed to properly report back -/// errors. -/// -/// The opening state is similar to the dial state, however the peer is only reached on a socket -/// address. The noise and yamux protocols are not negotiated yet. This state transitions to -/// [`PeerState::Dialing`] for the final part of the negotiation. Please note that it would be -/// wasteful to negotiate the noise and yamux protocols on all addresses, since only one -/// connection is kept around. -/// -/// This is something we'll reconsider in the future if we encounter issues. -/// -/// Next states: -/// - [`PeerState::Opening`] -> [`PeerState::Dialing`] (via transport manager -/// `on_connection_opened`) -/// - [`PeerState::Opening`] -> [`PeerState::Disconnected`] (via transport manager -/// `on_connection_opened` if negotiation cannot be started or via `on_open_failure`) -#[derive(Debug, Clone, PartialEq)] -pub enum PeerState { - /// `Litep2p` is connected to peer. - Connected { - /// The established record of the connection. - record: ConnectionRecord, - - /// Secondary record, this can either be a dial record or an established connection. - /// - /// While the local node was dialing a remote peer, the remote peer might've dialed - /// the local node and connection was established successfully. This dial address - /// is stored for processing later when the dial attempt concluded as either - /// successful/failed. - secondary: Option, - }, - - /// Connection to peer is opening over one or more addresses. - Opening { - /// Address records used for dialing. - addresses: HashSet, - - /// Connection ID. - connection_id: ConnectionId, - - /// Active transports. - transports: HashSet, - }, - - /// Peer is being dialed. - Dialing { - /// Address record. - dial_record: ConnectionRecord, - }, - - /// `Litep2p` is not connected to peer. - Disconnected { - /// Dial address, if it exists. - /// - /// While the local node was dialing a remote peer, the remote peer might've dialed - /// the local node and connection was established successfully. The connection might've - /// been closed before the dial concluded which means that - /// [`crate::transport::manager::TransportManager`] must be prepared to handle the dial - /// failure even after the connection has been closed. - dial_record: Option, - }, -} - -/// The state of the secondary connection. -#[derive(Debug, Clone, PartialEq)] -pub enum SecondaryOrDialing { - /// The secondary connection is established. - Secondary(ConnectionRecord), - /// The primary connection is established, but the secondary connection is still dialing. - Dialing(ConnectionRecord), -} - -/// Result of initiating a dial. -#[derive(Debug, Clone, PartialEq)] -pub enum StateDialResult { - /// The peer is already connected. - AlreadyConnected, - /// The dialing state is already in progress. - DialingInProgress, - /// The peer is disconnected, start dialing. - Ok, -} - -impl PeerState { - /// Check if the peer can be dialed. - pub fn can_dial(&self) -> StateDialResult { - match self { - // The peer is already connected, no need to dial again. - Self::Connected { .. } => return StateDialResult::AlreadyConnected, - // The dialing state is already in progress, an event will be emitted later. - Self::Dialing { .. } - | Self::Opening { .. } - | Self::Disconnected { - dial_record: Some(_), - } => { - return StateDialResult::DialingInProgress; - } - - Self::Disconnected { dial_record: None } => StateDialResult::Ok, - } - } - - /// Dial the peer on a single address. - pub fn dial_single_address(&mut self, dial_record: ConnectionRecord) -> StateDialResult { - let check = self.can_dial(); - if check != StateDialResult::Ok { - return check; - } - - match self { - Self::Disconnected { dial_record: None } => { - *self = PeerState::Dialing { dial_record }; - return StateDialResult::Ok; - } - state => panic!( - "unexpected state: {:?} validated by Self::can_dial; qed", - state - ), - } - } - - /// Dial the peer on multiple addresses. - pub fn dial_addresses( - &mut self, - connection_id: ConnectionId, - addresses: HashSet, - transports: HashSet, - ) -> StateDialResult { - let check = self.can_dial(); - if check != StateDialResult::Ok { - return check; - } - - match self { - Self::Disconnected { dial_record: None } => { - *self = PeerState::Opening { - addresses, - connection_id, - transports, - }; - return StateDialResult::Ok; - } - state => panic!( - "unexpected state: {:?} validated by Self::can_dial; qed", - state - ), - } - } - - /// Handle dial failure. - /// - /// # Transitions - /// - [`PeerState::Dialing`] (with record) -> [`PeerState::Disconnected`] - /// - [`PeerState::Connected`] (with dial record) -> [`PeerState::Connected`] - /// - [`PeerState::Disconnected`] (with dial record) -> [`PeerState::Disconnected`] - pub fn on_dial_failure(&mut self, connection_id: ConnectionId) { - match self { - // Clear the dial record if the connection ID matches. - Self::Dialing { dial_record } => - if dial_record.connection_id == connection_id { - *self = Self::Disconnected { dial_record: None }; - }, - - Self::Connected { - record, - secondary: Some(SecondaryOrDialing::Dialing(dial_record)), - } => - if dial_record.connection_id == connection_id { - *self = Self::Connected { - record: record.clone(), - secondary: None, - }; - }, - - Self::Disconnected { - dial_record: Some(dial_record), - } => - if dial_record.connection_id == connection_id { - *self = Self::Disconnected { dial_record: None }; - }, - - _ => (), - }; - } - - /// Returns `true` if the connection should be accepted by the transport manager. - pub fn on_connection_established(&mut self, connection: ConnectionRecord) -> bool { - match self { - // Transform the dial record into a secondary connection. - Self::Connected { - record, - secondary: Some(SecondaryOrDialing::Dialing(dial_record)), - } => - if dial_record.connection_id == connection.connection_id { - *self = Self::Connected { - record: record.clone(), - secondary: Some(SecondaryOrDialing::Secondary(connection)), - }; - - return true; - }, - // There's place for a secondary connection. - Self::Connected { - record, - secondary: None, - } => { - *self = Self::Connected { - record: record.clone(), - secondary: Some(SecondaryOrDialing::Secondary(connection)), - }; - - return true; - } - - // Convert the dial record into a primary connection or preserve it. - Self::Dialing { dial_record } - | Self::Disconnected { - dial_record: Some(dial_record), - } => - if dial_record.connection_id == connection.connection_id { - *self = Self::Connected { - record: connection.clone(), - secondary: None, - }; - return true; - } else { - *self = Self::Connected { - record: connection, - secondary: Some(SecondaryOrDialing::Dialing(dial_record.clone())), - }; - return true; - }, - - Self::Disconnected { dial_record: None } => { - *self = Self::Connected { - record: connection, - secondary: None, - }; - - return true; - } - - // Accept the incoming connection. - Self::Opening { .. } => { - *self = Self::Connected { - record: connection, - secondary: None, - }; - - return true; - } - - _ => {} - }; - - return false; - } - - /// Returns `true` if the connection was closed. - pub fn on_connection_closed(&mut self, connection_id: ConnectionId) -> bool { - match self { - Self::Connected { record, secondary } => { - // Primary connection closed. - if record.connection_id == connection_id { - match secondary { - // Promote secondary connection to primary. - Some(SecondaryOrDialing::Secondary(secondary)) => { - *self = Self::Connected { - record: secondary.clone(), - secondary: None, - }; - } - // Preserve the dial record. - Some(SecondaryOrDialing::Dialing(dial_record)) => { - *self = Self::Disconnected { - dial_record: Some(dial_record.clone()), - }; - - return true; - } - None => { - *self = Self::Disconnected { dial_record: None }; - - return true; - } - }; - - return false; - } - - match secondary { - // Secondary connection closed. - Some(SecondaryOrDialing::Secondary(secondary)) - if secondary.connection_id == connection_id => - { - *self = Self::Connected { - record: record.clone(), - secondary: None, - }; - } - _ => (), - } - } - _ => (), - } - - false - } - - /// Returns `true` if the last transport failed to open. - pub fn on_open_failure(&mut self, transport: SupportedTransport) -> bool { - match self { - Self::Opening { transports, .. } => { - transports.remove(&transport); - - if transports.is_empty() { - *self = Self::Disconnected { dial_record: None }; - return true; - } - - return false; - } - _ => false, - } - } -} - -/// The connection record keeps track of the connection ID and the address of the connection. -/// -/// The connection ID is used to track the connection in the transport layer. -/// While the address is used to keep a healthy view of the network for dialing purposes. -/// -/// # Note -/// -/// The structure is used to keep track of: -/// -/// - dialing state for outbound connections. -/// - established outbound connections via [`PeerState::Connected`]. -/// - established inbound connections via `PeerContext::secondary_connection`. -#[derive(Debug, Clone, Hash, PartialEq)] -pub struct ConnectionRecord { - /// Address of the connection. - /// - /// The address must contain the peer ID extension `/p2p/`. - pub address: Multiaddr, - - /// Connection ID resulted from dialing. - pub connection_id: ConnectionId, -} - -impl ConnectionRecord { - /// Construct a new connection record. - pub fn new(peer: PeerId, address: Multiaddr, connection_id: ConnectionId) -> Self { - Self { - address: Self::ensure_peer_id(peer, address), - connection_id, - } - } - - /// Create a new connection record from the peer ID and the endpoint. - pub fn from_endpoint(peer: PeerId, endpoint: &Endpoint) -> Self { - Self { - address: Self::ensure_peer_id(peer, endpoint.address().clone()), - connection_id: endpoint.connection_id(), - } - } - - /// Ensures the peer ID is present in the address. - fn ensure_peer_id(peer: PeerId, address: Multiaddr) -> Multiaddr { - if !std::matches!(address.iter().last(), Some(Protocol::P2p(_))) { - address.with(Protocol::P2p( - Multihash::from_bytes(&peer.to_bytes()).expect("valid peer id"), - )) - } else { - address - } - } -} - /// Peer context. #[derive(Debug)] pub struct PeerContext { From b2c51dbfe077656444172f793c4a90ecda94cbf1 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Mon, 23 Sep 2024 20:11:21 +0300 Subject: [PATCH 53/66] peer_state/tests: Check can dial states Signed-off-by: Alexandru Vasile --- src/transport/manager/peer_state.rs | 40 +++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/src/transport/manager/peer_state.rs b/src/transport/manager/peer_state.rs index 4d973bb3..5400dca0 100644 --- a/src/transport/manager/peer_state.rs +++ b/src/transport/manager/peer_state.rs @@ -442,3 +442,43 @@ impl ConnectionRecord { } } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn can_dial() { + let state = PeerState::Disconnected { dial_record: None }; + assert_eq!(state.can_dial(), StateDialResult::Ok); + + let record = ConnectionRecord::new( + PeerId::random(), + "/ip4/1.1.1.1/tcp/80".parse().unwrap(), + ConnectionId::from(0), + ); + + let state = PeerState::Disconnected { + dial_record: Some(record.clone()), + }; + assert_eq!(state.can_dial(), StateDialResult::DialingInProgress); + + let state = PeerState::Dialing { + dial_record: record.clone(), + }; + assert_eq!(state.can_dial(), StateDialResult::DialingInProgress); + + let state = PeerState::Opening { + addresses: Default::default(), + connection_id: ConnectionId::from(0), + transports: Default::default(), + }; + assert_eq!(state.can_dial(), StateDialResult::DialingInProgress); + + let state = PeerState::Connected { + record, + secondary: None, + }; + assert_eq!(state.can_dial(), StateDialResult::AlreadyConnected); + } +} From a6d86849562d48d5a54639b1cc2e309121313c45 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Mon, 23 Sep 2024 20:14:23 +0300 Subject: [PATCH 54/66] peer_state/tests: Check disconnected -> dialing Signed-off-by: Alexandru Vasile --- src/transport/manager/peer_state.rs | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/src/transport/manager/peer_state.rs b/src/transport/manager/peer_state.rs index 5400dca0..90132c4a 100644 --- a/src/transport/manager/peer_state.rs +++ b/src/transport/manager/peer_state.rs @@ -448,7 +448,7 @@ mod tests { use super::*; #[test] - fn can_dial() { + fn state_can_dial() { let state = PeerState::Disconnected { dial_record: None }; assert_eq!(state.can_dial(), StateDialResult::Ok); @@ -481,4 +481,25 @@ mod tests { }; assert_eq!(state.can_dial(), StateDialResult::AlreadyConnected); } + + #[test] + fn state_dial_single_address() { + let record = ConnectionRecord::new( + PeerId::random(), + "/ip4/1.1.1.1/tcp/80".parse().unwrap(), + ConnectionId::from(0), + ); + + let mut state = PeerState::Disconnected { dial_record: None }; + assert_eq!( + state.dial_single_address(record.clone()), + StateDialResult::Ok + ); + assert_eq!( + state, + PeerState::Dialing { + dial_record: record + } + ); + } } From 5959e4e3cc406f0ef73fdc4b9709b12c5ab108f7 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Mon, 23 Sep 2024 20:17:09 +0300 Subject: [PATCH 55/66] peer_state/tests: Check Disconnected -> Opening Signed-off-by: Alexandru Vasile --- src/transport/manager/peer_state.rs | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/src/transport/manager/peer_state.rs b/src/transport/manager/peer_state.rs index 90132c4a..d941c713 100644 --- a/src/transport/manager/peer_state.rs +++ b/src/transport/manager/peer_state.rs @@ -502,4 +502,25 @@ mod tests { } ); } + + #[test] + fn state_dial_addresses() { + let mut state = PeerState::Disconnected { dial_record: None }; + assert_eq!( + state.dial_addresses( + ConnectionId::from(0), + Default::default(), + Default::default() + ), + StateDialResult::Ok + ); + assert_eq!( + state, + PeerState::Opening { + addresses: Default::default(), + connection_id: ConnectionId::from(0), + transports: Default::default() + } + ); + } } From 6a613d8835fb3a2d07387df13ba1f3bfc7e36f56 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Mon, 23 Sep 2024 20:24:33 +0300 Subject: [PATCH 56/66] peer_state/tests: Check on dial failure Signed-off-by: Alexandru Vasile --- src/transport/manager/peer_state.rs | 79 +++++++++++++++++++++++++++++ 1 file changed, 79 insertions(+) diff --git a/src/transport/manager/peer_state.rs b/src/transport/manager/peer_state.rs index d941c713..c2bb5fc9 100644 --- a/src/transport/manager/peer_state.rs +++ b/src/transport/manager/peer_state.rs @@ -523,4 +523,83 @@ mod tests { } ); } + + #[test] + fn check_dial_failure() { + let record = ConnectionRecord::new( + PeerId::random(), + "/ip4/1.1.1.1/tcp/80".parse().unwrap(), + ConnectionId::from(0), + ); + + // Check from the dialing state. + { + let mut state = PeerState::Dialing { + dial_record: record.clone(), + }; + let previous_state = state.clone(); + // Check with different connection ID. + state.on_dial_failure(ConnectionId::from(1)); + assert_eq!(state, previous_state); + + // Check with the same connection ID. + state.on_dial_failure(ConnectionId::from(0)); + assert_eq!(state, PeerState::Disconnected { dial_record: None }); + } + + // Check from the connected state without dialing state. + { + let mut state = PeerState::Connected { + record: record.clone(), + secondary: None, + }; + let previous_state = state.clone(); + // Check with different connection ID. + state.on_dial_failure(ConnectionId::from(1)); + assert_eq!(state, previous_state); + + // Check with the same connection ID. + // The connection ID is checked against dialing records, not established connections. + state.on_dial_failure(ConnectionId::from(0)); + assert_eq!(state, previous_state); + } + + // Check from the connected state with dialing state. + { + let mut state = PeerState::Connected { + record: record.clone(), + secondary: Some(SecondaryOrDialing::Dialing(record.clone())), + }; + let previous_state = state.clone(); + // Check with different connection ID. + state.on_dial_failure(ConnectionId::from(1)); + assert_eq!(state, previous_state); + + // Check with the same connection ID. + // Dial record is cleared. + state.on_dial_failure(ConnectionId::from(0)); + assert_eq!( + state, + PeerState::Connected { + record: record.clone(), + secondary: None, + } + ); + } + + // Check from the disconnected state. + { + let mut state = PeerState::Disconnected { + dial_record: Some(record.clone()), + }; + let previous_state = state.clone(); + // Check with different connection ID. + state.on_dial_failure(ConnectionId::from(1)); + assert_eq!(state, previous_state); + + // Check with the same connection ID. + state.on_dial_failure(ConnectionId::from(0)); + assert_eq!(state, PeerState::Disconnected { dial_record: None }); + } + } } From 966cd18512afdeeae96339e8d8b2d98d407586d8 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 24 Sep 2024 11:12:25 +0300 Subject: [PATCH 57/66] peer_state/tests: Check connection established Signed-off-by: Alexandru Vasile --- src/transport/manager/peer_state.rs | 118 ++++++++++++++++++++++++++++ 1 file changed, 118 insertions(+) diff --git a/src/transport/manager/peer_state.rs b/src/transport/manager/peer_state.rs index c2bb5fc9..52ae198e 100644 --- a/src/transport/manager/peer_state.rs +++ b/src/transport/manager/peer_state.rs @@ -602,4 +602,122 @@ mod tests { assert_eq!(state, PeerState::Disconnected { dial_record: None }); } } + + #[test] + fn check_connection_established() { + let record = ConnectionRecord::new( + PeerId::random(), + "/ip4/1.1.1.1/tcp/80".parse().unwrap(), + ConnectionId::from(0), + ); + let second_record = ConnectionRecord::new( + PeerId::random(), + "/ip4/1.1.1.1/tcp/80".parse().unwrap(), + ConnectionId::from(1), + ); + + // Check from the connected state without secondary connection. + { + let mut state = PeerState::Connected { + record: record.clone(), + secondary: None, + }; + // Secondary is established. + assert!(state.on_connection_established(record.clone())); + assert_eq!( + state, + PeerState::Connected { + record: record.clone(), + secondary: Some(SecondaryOrDialing::Secondary(record.clone())), + } + ); + } + + // Check from the connected state with secondary dialing connection. + { + let mut state = PeerState::Connected { + record: record.clone(), + secondary: Some(SecondaryOrDialing::Dialing(record.clone())), + }; + // Promote the secondary connection. + assert!(state.on_connection_established(record.clone())); + assert_eq!( + state, + PeerState::Connected { + record: record.clone(), + secondary: Some(SecondaryOrDialing::Secondary(record.clone())), + } + ); + } + + // Check from the connected state with secondary established connection. + { + let mut state = PeerState::Connected { + record: record.clone(), + secondary: Some(SecondaryOrDialing::Secondary(record.clone())), + }; + // No state to advance. + assert!(!state.on_connection_established(record.clone())); + } + + // Opening state is completely wiped out. + { + let mut state = PeerState::Opening { + addresses: Default::default(), + connection_id: ConnectionId::from(0), + transports: Default::default(), + }; + assert!(state.on_connection_established(record.clone())); + assert_eq!( + state, + PeerState::Connected { + record: record.clone(), + secondary: None, + } + ); + } + + // Disconnected state with dial record. + { + let mut state = PeerState::Disconnected { + dial_record: Some(record.clone()), + }; + assert!(state.on_connection_established(record.clone())); + assert_eq!( + state, + PeerState::Connected { + record: record.clone(), + secondary: None, + } + ); + } + + // Disconnected with different dial record. + { + let mut state = PeerState::Disconnected { + dial_record: Some(record.clone()), + }; + assert!(state.on_connection_established(second_record.clone())); + assert_eq!( + state, + PeerState::Connected { + record: second_record.clone(), + secondary: Some(SecondaryOrDialing::Dialing(record.clone())) + } + ); + } + + // Disconnected without dial record. + { + let mut state = PeerState::Disconnected { dial_record: None }; + assert!(state.on_connection_established(record.clone())); + assert_eq!( + state, + PeerState::Connected { + record: record.clone(), + secondary: None, + } + ); + } + } } From 67515813056cba7f945929bf1374b081af4e7a2b Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 24 Sep 2024 11:15:35 +0300 Subject: [PATCH 58/66] peer_state/tests: Check connection established for Dialing Signed-off-by: Alexandru Vasile --- src/transport/manager/peer_state.rs | 30 +++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/src/transport/manager/peer_state.rs b/src/transport/manager/peer_state.rs index 52ae198e..0e3e4f2b 100644 --- a/src/transport/manager/peer_state.rs +++ b/src/transport/manager/peer_state.rs @@ -719,5 +719,35 @@ mod tests { } ); } + + // Dialing with different dial record. + { + let mut state = PeerState::Dialing { + dial_record: record.clone(), + }; + assert!(state.on_connection_established(second_record.clone())); + assert_eq!( + state, + PeerState::Connected { + record: second_record.clone(), + secondary: Some(SecondaryOrDialing::Dialing(record.clone())) + } + ); + } + + // Dialing with the same dial record. + { + let mut state = PeerState::Dialing { + dial_record: record.clone(), + }; + assert!(state.on_connection_established(record.clone())); + assert_eq!( + state, + PeerState::Connected { + record: record.clone(), + secondary: None, + } + ); + } } } From fc9a8484e1d84bc166128563af0ee27900dc4ce9 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 24 Sep 2024 11:22:00 +0300 Subject: [PATCH 59/66] peer_state/tests: Check connection closed Signed-off-by: Alexandru Vasile --- src/transport/manager/peer_state.rs | 56 +++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/src/transport/manager/peer_state.rs b/src/transport/manager/peer_state.rs index 0e3e4f2b..87ca6310 100644 --- a/src/transport/manager/peer_state.rs +++ b/src/transport/manager/peer_state.rs @@ -750,4 +750,60 @@ mod tests { ); } } + + #[test] + fn check_connection_closed() { + let record = ConnectionRecord::new( + PeerId::random(), + "/ip4/1.1.1.1/tcp/80".parse().unwrap(), + ConnectionId::from(0), + ); + let second_record = ConnectionRecord::new( + PeerId::random(), + "/ip4/1.1.1.1/tcp/80".parse().unwrap(), + ConnectionId::from(1), + ); + + // Primary is closed + { + let mut state = PeerState::Connected { + record: record.clone(), + secondary: None, + }; + assert!(state.on_connection_closed(ConnectionId::from(0))); + assert_eq!(state, PeerState::Disconnected { dial_record: None }); + } + + // Primary is closed with secondary promoted + { + let mut state = PeerState::Connected { + record: record.clone(), + secondary: Some(SecondaryOrDialing::Secondary(second_record.clone())), + }; + // Peer is still connected. + assert!(!state.on_connection_closed(ConnectionId::from(0))); + assert_eq!( + state, + PeerState::Connected { + record: second_record.clone(), + secondary: None, + } + ); + } + + // Primary is closed with secondary dial record + { + let mut state = PeerState::Connected { + record: record.clone(), + secondary: Some(SecondaryOrDialing::Dialing(second_record.clone())), + }; + assert!(state.on_connection_closed(ConnectionId::from(0))); + assert_eq!( + state, + PeerState::Disconnected { + dial_record: Some(second_record.clone()) + } + ); + } + } } From fa037388fe83e8ba5b288637e459561eafe241b8 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 24 Sep 2024 11:24:32 +0300 Subject: [PATCH 60/66] peer_state/tests: Check open failure state Signed-off-by: Alexandru Vasile --- src/transport/manager/peer_state.rs | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/transport/manager/peer_state.rs b/src/transport/manager/peer_state.rs index 87ca6310..51469ad7 100644 --- a/src/transport/manager/peer_state.rs +++ b/src/transport/manager/peer_state.rs @@ -806,4 +806,17 @@ mod tests { ); } } + + #[test] + fn check_open_failure() { + let mut state = PeerState::Opening { + addresses: Default::default(), + connection_id: ConnectionId::from(0), + transports: [SupportedTransport::Tcp].into_iter().collect(), + }; + + // This is the last protocol + assert!(state.on_open_failure(SupportedTransport::Tcp)); + assert_eq!(state, PeerState::Disconnected { dial_record: None }); + } } From dd3714fb49757b171620917ef46c3e0ebb6e47d7 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 24 Sep 2024 11:29:32 +0300 Subject: [PATCH 61/66] peer_state/tests: Check full lifecycle of a connection Signed-off-by: Alexandru Vasile --- src/transport/manager/peer_state.rs | 62 +++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/src/transport/manager/peer_state.rs b/src/transport/manager/peer_state.rs index 51469ad7..d5dddb20 100644 --- a/src/transport/manager/peer_state.rs +++ b/src/transport/manager/peer_state.rs @@ -819,4 +819,66 @@ mod tests { assert!(state.on_open_failure(SupportedTransport::Tcp)); assert_eq!(state, PeerState::Disconnected { dial_record: None }); } + + #[test] + fn check_full_lifecycle() { + let record = ConnectionRecord::new( + PeerId::random(), + "/ip4/1.1.1.1/tcp/80".parse().unwrap(), + ConnectionId::from(0), + ); + + let mut state = PeerState::Disconnected { dial_record: None }; + // Dialing. + assert_eq!( + state.dial_single_address(record.clone()), + StateDialResult::Ok + ); + assert_eq!( + state, + PeerState::Dialing { + dial_record: record.clone() + } + ); + + // Dialing failed. + state.on_dial_failure(ConnectionId::from(0)); + assert_eq!(state, PeerState::Disconnected { dial_record: None }); + + // Opening. + assert_eq!( + state.dial_addresses( + ConnectionId::from(0), + Default::default(), + Default::default() + ), + StateDialResult::Ok + ); + + // Open failure. + assert!(state.on_open_failure(SupportedTransport::Tcp)); + assert_eq!(state, PeerState::Disconnected { dial_record: None }); + + // Dial again. + assert_eq!( + state.dial_single_address(record.clone()), + StateDialResult::Ok + ); + assert_eq!( + state, + PeerState::Dialing { + dial_record: record.clone() + } + ); + + // Successful dial. + assert!(state.on_connection_established(record.clone())); + assert_eq!( + state, + PeerState::Connected { + record: record.clone(), + secondary: None + } + ); + } } From 6050e9632fb3e8d27f66a0c6849ada6c4ded462d Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 24 Sep 2024 13:41:55 +0300 Subject: [PATCH 62/66] manager: Remove pending connection state after success Signed-off-by: Alexandru Vasile --- src/transport/manager/mod.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/transport/manager/mod.rs b/src/transport/manager/mod.rs index a294a279..ecf5cac1 100644 --- a/src/transport/manager/mod.rs +++ b/src/transport/manager/mod.rs @@ -967,7 +967,7 @@ impl TransportManager { transport: SupportedTransport, connection_id: ConnectionId, ) -> crate::Result> { - let Some(peer) = self.pending_connections.remove(&connection_id) else { + let Some(peer) = self.pending_connections.get(&connection_id).copied() else { tracing::warn!( target: LOG_TARGET, ?connection_id, @@ -1007,6 +1007,8 @@ impl TransportManager { if last_transport { tracing::trace!(target: LOG_TARGET, ?peer, ?connection_id, "open failure for last transport"); + // Remove the pending connection. + self.pending_connections.remove(&connection_id); // Provide the peer to notify the open failure. return Ok(Some(peer)); } From 15554377af9e66cce3e82b3bdaffd27e01eec0f2 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 24 Sep 2024 13:54:45 +0300 Subject: [PATCH 63/66] peer_state: Handle connection opening Signed-off-by: Alexandru Vasile --- src/transport/manager/mod.rs | 131 +++++++++++++--------------- src/transport/manager/peer_state.rs | 19 ++++ 2 files changed, 78 insertions(+), 72 deletions(-) diff --git a/src/transport/manager/mod.rs b/src/transport/manager/mod.rs index ecf5cac1..88d91359 100644 --- a/src/transport/manager/mod.rs +++ b/src/transport/manager/mod.rs @@ -869,93 +869,80 @@ impl TransportManager { let mut peers = self.peers.write(); let context = peers.entry(peer).or_insert_with(|| PeerContext::default()); + let previous_state = context.state.clone(); - match std::mem::replace( - &mut context.state, - PeerState::Disconnected { dial_record: None }, - ) { - PeerState::Opening { - connection_id, - transports, - .. - } => { + let record = ConnectionRecord::new(peer, address.clone(), connection_id); + let state_advanced = context.state.on_connection_opened(record); + if !state_advanced { + tracing::warn!( + target: LOG_TARGET, + ?peer, + ?connection_id, + state = ?context.state, + "connection opened but `PeerState` is not `Opening`", + ); + return Err(Error::InvalidState); + } + + // State advanced from `Opening` to `Dialing`. + let PeerState::Opening { + connection_id, + transports, + .. + } = previous_state + else { + tracing::warn!( + target: LOG_TARGET, + ?peer, + ?connection_id, + state = ?context.state, + "State missmatch in opening expected by peer state transition", + ); + return Err(Error::InvalidState); + }; + + // Cancel open attempts for other transports as connection already exists. + for transport in transports.iter() { + self.transports + .get_mut(transport) + .expect("transport to exist") + .cancel(connection_id); + } + + let negotiation = self + .transports + .get_mut(&transport) + .expect("transport to exist") + .negotiate(connection_id); + + match negotiation { + Ok(()) => { tracing::trace!( target: LOG_TARGET, ?peer, ?connection_id, - ?address, ?transport, - "connection opened to peer", + "negotiation started" ); - // cancel open attempts for other transports as connection already exists - for transport in transports.iter() { - self.transports - .get_mut(transport) - .expect("transport to exist") - .cancel(connection_id); - } + self.pending_connections.insert(connection_id, peer); + context.addresses.insert(AddressRecord::new( + &peer, + address, + scores::CONNECTION_ESTABLISHED, + )); - // negotiate the connection - match self - .transports - .get_mut(&transport) - .expect("transport to exist") - .negotiate(connection_id) - { - Ok(()) => { - tracing::trace!( - target: LOG_TARGET, - ?peer, - ?connection_id, - ?transport, - "negotiation started" - ); - - self.pending_connections.insert(connection_id, peer); - - context.state = PeerState::Dialing { - dial_record: ConnectionRecord::new( - peer, - address.clone(), - connection_id, - ), - }; - - context.addresses.insert(AddressRecord::new( - &peer, - address, - scores::CONNECTION_ESTABLISHED, - )); - - Ok(()) - } - Err(error) => { - tracing::warn!( - target: LOG_TARGET, - ?peer, - ?connection_id, - ?error, - "failed to negotiate connection", - ); - context.state = PeerState::Disconnected { dial_record: None }; - - debug_assert!(false); - Err(Error::InvalidState) - } - } + Ok(()) } - state => { + Err(err) => { tracing::warn!( target: LOG_TARGET, ?peer, ?connection_id, - ?state, - "connection opened but `PeerState` is not `Opening`", + ?err, + "failed to negotiate connection", ); - context.state = state; - - debug_assert!(false); + context.state = PeerState::Disconnected { dial_record: None }; Err(Error::InvalidState) } } diff --git a/src/transport/manager/peer_state.rs b/src/transport/manager/peer_state.rs index d5dddb20..1e4904a1 100644 --- a/src/transport/manager/peer_state.rs +++ b/src/transport/manager/peer_state.rs @@ -389,6 +389,25 @@ impl PeerState { _ => false, } } + + /// Returns `true` if the connection was opened. + pub fn on_connection_opened(&mut self, record: ConnectionRecord) -> bool { + match self { + Self::Opening { .. } => { + // TODO: Litep2p did not check previously if the + // connection record is valid or not, in terms of having + // the same connection ID and the address part of the + // address set. + + *self = Self::Dialing { + dial_record: record.clone(), + }; + + true + } + _ => false, + } + } } /// The connection record keeps track of the connection ID and the address of the connection. From 3c8058ead2a20459d920f2f81ffdee292af0f89b Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 24 Sep 2024 14:03:10 +0300 Subject: [PATCH 64/66] peer_state/tests: Check opening of connections Signed-off-by: Alexandru Vasile --- src/transport/manager/peer_state.rs | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/transport/manager/peer_state.rs b/src/transport/manager/peer_state.rs index 1e4904a1..eb0341f3 100644 --- a/src/transport/manager/peer_state.rs +++ b/src/transport/manager/peer_state.rs @@ -839,6 +839,23 @@ mod tests { assert_eq!(state, PeerState::Disconnected { dial_record: None }); } + #[test] + fn check_open_connection() { + let record = ConnectionRecord::new( + PeerId::random(), + "/ip4/1.1.1.1/tcp/80".parse().unwrap(), + ConnectionId::from(0), + ); + + let mut state = PeerState::Opening { + addresses: Default::default(), + connection_id: ConnectionId::from(0), + transports: [SupportedTransport::Tcp].into_iter().collect(), + }; + + assert!(state.on_connection_opened(record.clone())); + } + #[test] fn check_full_lifecycle() { let record = ConnectionRecord::new( From de5354b36019f1f4a5272c6cd6d865cf23550753 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 24 Sep 2024 14:05:33 +0300 Subject: [PATCH 65/66] manager: Better tracking of addresses for opening Signed-off-by: Alexandru Vasile --- src/transport/manager/mod.rs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/transport/manager/mod.rs b/src/transport/manager/mod.rs index 88d91359..be4e88d0 100644 --- a/src/transport/manager/mod.rs +++ b/src/transport/manager/mod.rs @@ -869,8 +869,15 @@ impl TransportManager { let mut peers = self.peers.write(); let context = peers.entry(peer).or_insert_with(|| PeerContext::default()); - let previous_state = context.state.clone(); + // Keep track of the address. + context.addresses.insert(AddressRecord::new( + &peer, + address.clone(), + scores::CONNECTION_ESTABLISHED, + )); + + let previous_state = context.state.clone(); let record = ConnectionRecord::new(peer, address.clone(), connection_id); let state_advanced = context.state.on_connection_opened(record); if !state_advanced { @@ -926,11 +933,6 @@ impl TransportManager { ); self.pending_connections.insert(connection_id, peer); - context.addresses.insert(AddressRecord::new( - &peer, - address, - scores::CONNECTION_ESTABLISHED, - )); Ok(()) } From 9869283434663595341e4846384dd3c2c1569a09 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 24 Sep 2024 14:09:10 +0300 Subject: [PATCH 66/66] peer_state: Return bool when dial failure was handled Signed-off-by: Alexandru Vasile --- src/transport/manager/mod.rs | 4 +--- src/transport/manager/peer_state.rs | 10 +++++++++- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/transport/manager/mod.rs b/src/transport/manager/mod.rs index be4e88d0..1aba50f1 100644 --- a/src/transport/manager/mod.rs +++ b/src/transport/manager/mod.rs @@ -666,11 +666,9 @@ impl TransportManager { let mut peers = self.peers.write(); let context = peers.entry(peer).or_insert_with(|| PeerContext::default()); - let previous_state = context.state.clone(); - context.state.on_dial_failure(connection_id); - if context.state == previous_state { + if !context.state.on_dial_failure(connection_id) { tracing::warn!( target: LOG_TARGET, ?peer, diff --git a/src/transport/manager/peer_state.rs b/src/transport/manager/peer_state.rs index eb0341f3..923bdf98 100644 --- a/src/transport/manager/peer_state.rs +++ b/src/transport/manager/peer_state.rs @@ -216,15 +216,19 @@ impl PeerState { /// Handle dial failure. /// /// # Transitions + /// /// - [`PeerState::Dialing`] (with record) -> [`PeerState::Disconnected`] /// - [`PeerState::Connected`] (with dial record) -> [`PeerState::Connected`] /// - [`PeerState::Disconnected`] (with dial record) -> [`PeerState::Disconnected`] - pub fn on_dial_failure(&mut self, connection_id: ConnectionId) { + /// + /// Returns `true` if the connection was handled. + pub fn on_dial_failure(&mut self, connection_id: ConnectionId) -> bool { match self { // Clear the dial record if the connection ID matches. Self::Dialing { dial_record } => if dial_record.connection_id == connection_id { *self = Self::Disconnected { dial_record: None }; + return true; }, Self::Connected { @@ -236,6 +240,7 @@ impl PeerState { record: record.clone(), secondary: None, }; + return true; }, Self::Disconnected { @@ -243,10 +248,13 @@ impl PeerState { } => if dial_record.connection_id == connection_id { *self = Self::Disconnected { dial_record: None }; + return true; }, _ => (), }; + + false } /// Returns `true` if the connection should be accepted by the transport manager.