From a0fca05404326494be1afce0db0cd545b0bf317b Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 19 Nov 2024 15:35:25 +0200 Subject: [PATCH 01/27] authority-discovery: Filter out empty addresses after removing `/p2p/..` Signed-off-by: Alexandru Vasile --- .../client/authority-discovery/src/worker.rs | 18 +++++++++++++++--- .../client/network/types/src/multiaddr.rs | 5 +++++ 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/substrate/client/authority-discovery/src/worker.rs b/substrate/client/authority-discovery/src/worker.rs index ba82910efcdf..f1d3bee5d3b5 100644 --- a/substrate/client/authority-discovery/src/worker.rs +++ b/substrate/client/authority-discovery/src/worker.rs @@ -280,7 +280,9 @@ where config .public_addresses .into_iter() - .map(|address| AddressType::PublicAddress(address).without_p2p(local_peer_id)) + .filter_map(|address| { + AddressType::PublicAddress(address).without_p2p(local_peer_id) + }) .collect() }; @@ -399,6 +401,7 @@ where .filter_map(|address| { address_is_global(&address) .then(|| AddressType::GlobalListenAddress(address).without_p2p(local_peer_id)) + .flatten() }) .take(MAX_GLOBAL_LISTEN_ADDRESSES) .peekable(); @@ -411,6 +414,7 @@ where .filter_map(|address| { (publish_non_global_ips || address_is_global(&address)) .then(|| AddressType::ExternalAddress(address).without_p2p(local_peer_id)) + .flatten() }) .peekable(); @@ -998,7 +1002,9 @@ impl AddressType { /// /// In case the peer id in the address does not match the local peer id, an error is logged for /// `ExternalAddress` and `GlobalListenAddress`. - fn without_p2p(self, local_peer_id: PeerId) -> Multiaddr { + /// + /// Returns `None` if the address is empty after removing the `/p2p/..` part. + fn without_p2p(self, local_peer_id: PeerId) -> Option { // Get the address and the source str for logging. let (mut address, source) = match self { AddressType::PublicAddress(address) => (address, "public address"), @@ -1016,7 +1022,13 @@ impl AddressType { } address.pop(); } - address + + // Empty addresses cannot be published. + if address.is_empty() { + None + } else { + Some(address) + } } } diff --git a/substrate/client/network/types/src/multiaddr.rs b/substrate/client/network/types/src/multiaddr.rs index 925e24fe70d6..c394f81a2505 100644 --- a/substrate/client/network/types/src/multiaddr.rs +++ b/substrate/client/network/types/src/multiaddr.rs @@ -42,6 +42,11 @@ pub struct Multiaddr { } impl Multiaddr { + /// Returns `true` if this multiaddress is empty. + pub fn is_empty(&self) -> bool { + self.multiaddr.is_empty() + } + /// Create a new, empty multiaddress. pub fn empty() -> Self { Self { multiaddr: LiteP2pMultiaddr::empty() } From 3e3e461882ba5b267dda2b3e791afe0ce7cda907 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 19 Nov 2024 15:49:07 +0200 Subject: [PATCH 02/27] authority-discovery: Ignore empty addresses from DHT Signed-off-by: Alexandru Vasile --- substrate/client/authority-discovery/src/worker.rs | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/substrate/client/authority-discovery/src/worker.rs b/substrate/client/authority-discovery/src/worker.rs index f1d3bee5d3b5..c4c2550264aa 100644 --- a/substrate/client/authority-discovery/src/worker.rs +++ b/substrate/client/authority-discovery/src/worker.rs @@ -850,7 +850,9 @@ where // Ignore [`Multiaddr`]s without [`PeerId`] or with own addresses. let addresses: Vec = addresses .into_iter() - .filter(|a| get_peer_id(&a).filter(|p| *p != local_peer_id).is_some()) + .filter(|a| { + !address_is_empty(a) && get_peer_id(&a).filter(|p| *p != local_peer_id).is_some() + }) .collect(); let remote_peer_id = single(addresses.iter().map(|a| get_peer_id(&a))) @@ -1032,6 +1034,15 @@ impl AddressType { } } +/// Checks if an authority-discovery address is empty. +/// +/// An empty address is an address without protocols, or an address with only the `/p2p/..` +/// protocol. +fn address_is_empty(address: &Multiaddr) -> bool { + address.is_empty() || + address.iter().all(|protocol| matches!(protocol, multiaddr::Protocol::P2p(_))) +} + /// NetworkProvider provides [`Worker`] with all necessary hooks into the /// underlying Substrate networking. Using this trait abstraction instead of /// `sc_network::NetworkService` directly is necessary to unit test [`Worker`]. From 07243720cef48e12ab89260ac461e48e13b47917 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 19 Nov 2024 19:08:13 +0200 Subject: [PATCH 03/27] authority-discovery/tests: Add test for is empty address Signed-off-by: Alexandru Vasile --- .../client/authority-discovery/src/worker.rs | 25 +++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/substrate/client/authority-discovery/src/worker.rs b/substrate/client/authority-discovery/src/worker.rs index c4c2550264aa..f198a4b04b9e 100644 --- a/substrate/client/authority-discovery/src/worker.rs +++ b/substrate/client/authority-discovery/src/worker.rs @@ -1036,8 +1036,9 @@ impl AddressType { /// Checks if an authority-discovery address is empty. /// -/// An empty address is an address without protocols, or an address with only the `/p2p/..` -/// protocol. +/// An address is considered empty if: +/// - The multiaddr is empty. +/// - The multiaddr contains only the `/p2p/..` protocol. fn address_is_empty(address: &Multiaddr) -> bool { address.is_empty() || address.iter().all(|protocol| matches!(protocol, multiaddr::Protocol::P2p(_))) @@ -1222,3 +1223,23 @@ impl Worker Date: Tue, 19 Nov 2024 19:11:46 +0200 Subject: [PATCH 04/27] litep2p: Ignore empty addresses Signed-off-by: Alexandru Vasile --- substrate/client/network/src/litep2p/mod.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/substrate/client/network/src/litep2p/mod.rs b/substrate/client/network/src/litep2p/mod.rs index 10cf9f4da36d..eb71aa683aa9 100644 --- a/substrate/client/network/src/litep2p/mod.rs +++ b/substrate/client/network/src/litep2p/mod.rs @@ -748,6 +748,14 @@ impl NetworkBackend for Litep2pNetworkBac NetworkServiceCommand::AddKnownAddress { peer, address } => { let mut address: Multiaddr = address.into(); + if address.is_empty() { + log::debug!( + target: LOG_TARGET, + "Ignoring empty address for {peer:?}", + ); + continue + } + if !address.iter().any(|protocol| std::matches!(protocol, Protocol::P2p(_))) { address.push(Protocol::P2p(litep2p::PeerId::from(peer).into())); } From 5e718613293b41c0e80325193d0cb6221bd067de Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 19 Nov 2024 19:33:01 +0200 Subject: [PATCH 05/27] litep2p/discovery: Validate empty / no-IP multiaddresses Signed-off-by: Alexandru Vasile --- .../client/network/src/litep2p/discovery.rs | 68 +++++++++++++++---- 1 file changed, 54 insertions(+), 14 deletions(-) diff --git a/substrate/client/network/src/litep2p/discovery.rs b/substrate/client/network/src/litep2p/discovery.rs index 3a9454e317cc..e113801f64a6 100644 --- a/substrate/client/network/src/litep2p/discovery.rs +++ b/substrate/client/network/src/litep2p/discovery.rs @@ -488,6 +488,59 @@ impl Discovery { (false, None) } + + /// Verify the external address discovered by the identify protocol. + /// + /// This ensures that: + /// - the address is not empty + /// - the address contains a valid IP address + /// - the address is for the local peer ID + /// - the address always contains `/p2p/` protocol. + /// + /// The provided peer ID is only used for logging purposes. + fn verify_external_address(&mut self, address: Multiaddr, peer: PeerId) -> Option { + if address.is_empty() { + log::warn!( + target: LOG_TARGET, + "🔍 Discovered empty address from {peer:?}", + ); + return None; + }; + + // Expect the address to contain a valid IP address. + if std::matches!( + address.iter().next(), + Some( + Protocol::Dns(_) | + Protocol::Dns4(_) | + Protocol::Dns6(_) | + Protocol::Ip6(_) | + Protocol::Ip4(_), + ) + ) { + log::warn!( + target: LOG_TARGET, + "🔍 Discovered external address from {peer:?} does not contain a valid IP address: {address}", + ); + return None; + }; + + if let Some(Protocol::P2p(peer_id)) = address.iter().last() { + // Invalid address if the reported peer ID is not the local peer ID. + if peer_id != *self.local_peer_id.as_ref() { + log::warn!( + target: LOG_TARGET, + "🔍 Discovered external address from {peer:?} that is not us: {address}", + ); + return None + } + + return Some(address) + } + + // Ensure the address contains the local peer ID. + Some(address.with(Protocol::P2p(self.local_peer_id.into()))) + } } impl Stream for Discovery { @@ -596,20 +649,7 @@ impl Stream for Discovery { observed_address, .. })) => { - let observed_address = - if let Some(Protocol::P2p(peer_id)) = observed_address.iter().last() { - if peer_id != *this.local_peer_id.as_ref() { - log::warn!( - target: LOG_TARGET, - "Discovered external address for a peer that is not us: {observed_address}", - ); - None - } else { - Some(observed_address) - } - } else { - Some(observed_address.with(Protocol::P2p(this.local_peer_id.into()))) - }; + let observed_address = this.verify_external_address(observed_address, peer); // Ensure that an external address with a different peer ID does not have // side effects of evicting other external addresses via `ExternalAddressExpired`. From f36d749df6d1b653707b3b3ad6cbe4d55b323a0c Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 19 Nov 2024 19:59:50 +0200 Subject: [PATCH 06/27] libp2p/peer_info: Confirm address only if it is valid Signed-off-by: Alexandru Vasile --- substrate/client/network/src/peer_info.rs | 78 +++++++++++++++++++---- 1 file changed, 67 insertions(+), 11 deletions(-) diff --git a/substrate/client/network/src/peer_info.rs b/substrate/client/network/src/peer_info.rs index 21eeea6bcc0c..448700652143 100644 --- a/substrate/client/network/src/peer_info.rs +++ b/substrate/client/network/src/peer_info.rs @@ -31,6 +31,7 @@ use libp2p::{ Info as IdentifyInfo, }, identity::PublicKey, + multiaddr::Protocol, ping::{Behaviour as Ping, Config as PingConfig, Event as PingEvent}, swarm::{ behaviour::{ @@ -66,6 +67,9 @@ pub struct PeerInfoBehaviour { ping: Ping, /// Periodically identifies the remote and responds to incoming requests. identify: Identify, + /// Identity of our local node. + local_peer_id: PeerId, + /// Information that we know about all nodes. nodes_info: FnvHashMap, /// Interval at which we perform garbage collection in `nodes_info`. @@ -123,6 +127,7 @@ impl PeerInfoBehaviour { local_public_key: PublicKey, external_addresses: Arc>>, ) -> Self { + let local_peer_id = local_public_key.to_peer_id(); let identify = { let cfg = IdentifyConfig::new("/substrate/1.0".to_string(), local_public_key) .with_agent_version(user_agent) @@ -134,6 +139,7 @@ impl PeerInfoBehaviour { Self { ping: Ping::new(PingConfig::new()), identify, + local_peer_id, nodes_info: FnvHashMap::default(), garbage_collect: Box::pin(interval(GARBAGE_COLLECT_INTERVAL)), external_addresses: ExternalAddresses { addresses: external_addresses }, @@ -178,6 +184,53 @@ impl PeerInfoBehaviour { "Received pong from node we're not connected to {:?}", peer_id); } } + + /// Verify the external address discovered by the identify protocol. + /// + /// This ensures that: + /// - the address is not empty + /// - the address contains a valid IP address + /// - the address is for the local peer ID + fn verify_external_address(&mut self, address: &Multiaddr) -> bool { + if address.is_empty() { + log::warn!( + target: "sub-libp2p", + "🔍 Discovered empty address", + ); + return false; + }; + + // Expect the address to contain a valid IP address. + if std::matches!( + address.iter().next(), + Some( + Protocol::Dns(_) | + Protocol::Dns4(_) | + Protocol::Dns6(_) | + Protocol::Ip6(_) | + Protocol::Ip4(_), + ) + ) { + log::warn!( + target: "sub-libp2p", + "🔍 Discovered external address does not contain a valid IP address: {address}", + ); + return false; + }; + + if let Some(Protocol::P2p(peer_id)) = address.iter().last() { + // Invalid address if the reported peer ID is not the local peer ID. + if peer_id != self.local_peer_id { + log::warn!( + target: "sub-libp2p", + "🔍 Discovered external address that is not us: {address}", + ); + return false + } + } + + true + } } /// Gives access to the information about a node. @@ -401,17 +454,20 @@ impl NetworkBehaviour for PeerInfoBehaviour { self.external_addresses.remove(e.addr); }, FromSwarm::NewExternalAddrCandidate(e @ NewExternalAddrCandidate { addr }) => { - self.ping.on_swarm_event(FromSwarm::NewExternalAddrCandidate(e)); - self.identify.on_swarm_event(FromSwarm::NewExternalAddrCandidate(e)); - - // Manually confirm all external address candidates. - // TODO: consider adding [AutoNAT protocol](https://docs.rs/libp2p/0.52.3/libp2p/autonat/index.html) - // (must go through the polkadot protocol spec) or implemeting heuristics for - // approving external address candidates. This can be done, for example, by - // approving only addresses reported by multiple peers. - // See also https://github.com/libp2p/rust-libp2p/pull/4721 introduced - // in libp2p v0.53 for heuristics approach. - self.pending_actions.push_back(ToSwarm::ExternalAddrConfirmed(addr.clone())); + // Verify the external address is valid. + if self.verify_external_address(addr) { + self.ping.on_swarm_event(FromSwarm::NewExternalAddrCandidate(e)); + self.identify.on_swarm_event(FromSwarm::NewExternalAddrCandidate(e)); + + // Manually confirm all external address candidates. + // TODO: consider adding [AutoNAT protocol](https://docs.rs/libp2p/0.52.3/libp2p/autonat/index.html) + // (must go through the polkadot protocol spec) or implemeting heuristics for + // approving external address candidates. This can be done, for example, by + // approving only addresses reported by multiple peers. + // See also https://github.com/libp2p/rust-libp2p/pull/4721 introduced + // in libp2p v0.53 for heuristics approach. + self.pending_actions.push_back(ToSwarm::ExternalAddrConfirmed(addr.clone())); + } }, FromSwarm::ExternalAddrConfirmed(e @ ExternalAddrConfirmed { addr }) => { self.ping.on_swarm_event(FromSwarm::ExternalAddrConfirmed(e)); From 9e7c86716824c5c8b1ab22d74b3a778d32196aae Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 19 Nov 2024 21:13:04 +0200 Subject: [PATCH 07/27] Add PRdoc Signed-off-by: Alexandru Vasile --- prdoc/pr_6545.prdoc | 12 ++++++++++++ 1 file changed, 12 insertions(+) create mode 100644 prdoc/pr_6545.prdoc diff --git a/prdoc/pr_6545.prdoc b/prdoc/pr_6545.prdoc new file mode 100644 index 000000000000..406246d73ed7 --- /dev/null +++ b/prdoc/pr_6545.prdoc @@ -0,0 +1,12 @@ +title: Ignore multi-addresses without IP from DHT records + +doc: + - audience: [ Node Dev, Node Operator ] + description: | + This PR ensures that multiaddresses without an IP protocol are not published or utilized from DHT records. + +crates: + - name: sc-network + bump: patch + - name: sc-authority-discovery + bump: patch From 109bb1c1939962153847bbd5044d13d0402891b2 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Wed, 20 Nov 2024 19:04:55 +0200 Subject: [PATCH 08/27] peer_info: Downgrade warn to debug logs Signed-off-by: Alexandru Vasile --- substrate/client/network/src/peer_info.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/substrate/client/network/src/peer_info.rs b/substrate/client/network/src/peer_info.rs index 448700652143..ee596b0a2033 100644 --- a/substrate/client/network/src/peer_info.rs +++ b/substrate/client/network/src/peer_info.rs @@ -193,7 +193,7 @@ impl PeerInfoBehaviour { /// - the address is for the local peer ID fn verify_external_address(&mut self, address: &Multiaddr) -> bool { if address.is_empty() { - log::warn!( + log::debug!( target: "sub-libp2p", "🔍 Discovered empty address", ); @@ -211,7 +211,7 @@ impl PeerInfoBehaviour { Protocol::Ip4(_), ) ) { - log::warn!( + log::debug!( target: "sub-libp2p", "🔍 Discovered external address does not contain a valid IP address: {address}", ); @@ -221,7 +221,7 @@ impl PeerInfoBehaviour { if let Some(Protocol::P2p(peer_id)) = address.iter().last() { // Invalid address if the reported peer ID is not the local peer ID. if peer_id != self.local_peer_id { - log::warn!( + log::debug!( target: "sub-libp2p", "🔍 Discovered external address that is not us: {address}", ); From 281e1ebe6aaab873a9a02afb26f3fe8da55d18ac Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Thu, 21 Nov 2024 12:54:53 +0200 Subject: [PATCH 09/27] network/types: Move verification of external addr Signed-off-by: Alexandru Vasile --- .../client/network/types/src/multiaddr.rs | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/substrate/client/network/types/src/multiaddr.rs b/substrate/client/network/types/src/multiaddr.rs index c394f81a2505..6121db908d42 100644 --- a/substrate/client/network/types/src/multiaddr.rs +++ b/substrate/client/network/types/src/multiaddr.rs @@ -32,6 +32,7 @@ pub use protocol::Protocol; // Re-export the macro under shorter name under `multiaddr`. pub use crate::build_multiaddr as multiaddr; +use crate::PeerId; /// [`Multiaddr`] type used in Substrate. Converted to libp2p's `Multiaddr` /// or litep2p's `Multiaddr` when passed to the corresponding network backend. @@ -76,6 +77,41 @@ impl Multiaddr { pub fn to_vec(&self) -> Vec { self.multiaddr.to_vec() } + + /// Verify the external address is valid. + /// + /// An external address address discovered by the network is valid when: + /// - the address is not empty + /// - the address contains a valid IP address + /// - the address is for the local peer ID + pub fn verify_external_address(&self, local_peer_id: PeerId) -> Result<(), ParseError> { + if self.is_empty() { + return Err(ParseError::InvalidMultiaddr); + } + + // Expect the address to contain a valid IP address. + if !std::matches!( + self.iter().next(), + Some( + Protocol::Dns(_) | + Protocol::Dns4(_) | + Protocol::Dns6(_) | + Protocol::Ip6(_) | + Protocol::Ip4(_), + ) + ) { + return Err(ParseError::InvalidMultiaddr); + } + + if let Some(Protocol::P2p(peer_id)) = self.iter().last() { + // Invalid address if the reported peer ID is not the local peer ID. + if peer_id != *local_peer_id.as_ref() { + return Err(ParseError::InvalidMultiaddr); + } + } + + Ok(()) + } } impl Display for Multiaddr { From f09e25c81316f393884217422b8a26f83bff389c Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Thu, 21 Nov 2024 12:59:45 +0200 Subject: [PATCH 10/27] network/types: Ensure peerID functionality Signed-off-by: Alexandru Vasile --- substrate/client/network/types/src/multiaddr.rs | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/substrate/client/network/types/src/multiaddr.rs b/substrate/client/network/types/src/multiaddr.rs index 6121db908d42..e86da94f22ee 100644 --- a/substrate/client/network/types/src/multiaddr.rs +++ b/substrate/client/network/types/src/multiaddr.rs @@ -112,6 +112,23 @@ impl Multiaddr { Ok(()) } + + /// Ensure the peer ID is present in the multiaddress. + /// + /// Returns None when the peer ID of the address is different from the local peer ID. + pub fn ensure_peer_id(self, local_peer_id: PeerId) -> Option { + if let Some(Protocol::P2p(peer_id)) = self.iter().last() { + // Invalid address if the reported peer ID is not the local peer ID. + if peer_id != *local_peer_id.as_ref() { + return None + } + + return Some(self) + } + + // Ensure the address contains the local peer ID. + Some(self.with(Protocol::P2p(local_peer_id.into()))) + } } impl Display for Multiaddr { From 3906c578c96d97a8a099a4bdac4685acbe375a7c Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Thu, 21 Nov 2024 14:27:53 +0200 Subject: [PATCH 11/27] Use crate::type::Multiaddr shared functionality Signed-off-by: Alexandru Vasile --- .../client/network/src/litep2p/discovery.rs | 97 ++++++------------- substrate/client/network/src/peer_info.rs | 51 +--------- .../client/network/types/src/multiaddr.rs | 10 +- 3 files changed, 38 insertions(+), 120 deletions(-) diff --git a/substrate/client/network/src/litep2p/discovery.rs b/substrate/client/network/src/litep2p/discovery.rs index e113801f64a6..ce8e215ba0ed 100644 --- a/substrate/client/network/src/litep2p/discovery.rs +++ b/substrate/client/network/src/litep2p/discovery.rs @@ -489,57 +489,46 @@ impl Discovery { (false, None) } - /// Verify the external address discovered by the identify protocol. - /// - /// This ensures that: - /// - the address is not empty - /// - the address contains a valid IP address - /// - the address is for the local peer ID - /// - the address always contains `/p2p/` protocol. - /// - /// The provided peer ID is only used for logging purposes. - fn verify_external_address(&mut self, address: Multiaddr, peer: PeerId) -> Option { - if address.is_empty() { - log::warn!( + /// Handle the observed address from the network. + fn handle_observed_addresses(&mut self, observed_address: Multiaddr, peer: PeerId) { + // Converting to and from `sc_network_types::multiaddr::Multiaddr` is cheap considering + // it is a small wrapper over litep2p `Multiaddr`. + let observed_address: sc_network_types::multiaddr::Multiaddr = observed_address.into(); + if !observed_address.is_valid_external_address(self.local_peer_id.into()) { + log::debug!( target: LOG_TARGET, - "🔍 Discovered empty address from {peer:?}", + "Ignoring invalid external address {observed_address} from {peer:?}", ); - return None; - }; + return; + } - // Expect the address to contain a valid IP address. - if std::matches!( - address.iter().next(), - Some( - Protocol::Dns(_) | - Protocol::Dns4(_) | - Protocol::Dns6(_) | - Protocol::Ip6(_) | - Protocol::Ip4(_), - ) - ) { - log::warn!( + let Some(observed_address) = observed_address.ensure_peer_id(self.local_peer_id.into()) + else { + log::debug!( target: LOG_TARGET, - "🔍 Discovered external address from {peer:?} does not contain a valid IP address: {address}", + "Ignoring external address with different peer ID from {peer:?}", ); - return None; + return }; + let observed_address = observed_address.into(); - if let Some(Protocol::P2p(peer_id)) = address.iter().last() { - // Invalid address if the reported peer ID is not the local peer ID. - if peer_id != *self.local_peer_id.as_ref() { - log::warn!( - target: LOG_TARGET, - "🔍 Discovered external address from {peer:?} that is not us: {address}", - ); - return None - } + let (is_new, expired_address) = self.is_new_external_address(&observed_address, peer); - return Some(address) + if let Some(expired_address) = expired_address { + log::trace!( + target: LOG_TARGET, + "Removing expired external address expired={expired_address} is_new={is_new} observed={observed_address}", + ); + + self.pending_events + .push_back(DiscoveryEvent::ExternalAddressExpired { address: expired_address }); } - // Ensure the address contains the local peer ID. - Some(address.with(Protocol::P2p(self.local_peer_id.into()))) + if is_new { + self.pending_events.push_back(DiscoveryEvent::ExternalAddressDiscovered { + address: observed_address.clone(), + }); + } } } @@ -649,31 +638,7 @@ impl Stream for Discovery { observed_address, .. })) => { - let observed_address = this.verify_external_address(observed_address, peer); - - // Ensure that an external address with a different peer ID does not have - // side effects of evicting other external addresses via `ExternalAddressExpired`. - if let Some(observed_address) = observed_address { - let (is_new, expired_address) = - this.is_new_external_address(&observed_address, peer); - - if let Some(expired_address) = expired_address { - log::trace!( - target: LOG_TARGET, - "Removing expired external address expired={expired_address} is_new={is_new} observed={observed_address}", - ); - - this.pending_events.push_back(DiscoveryEvent::ExternalAddressExpired { - address: expired_address, - }); - } - - if is_new { - this.pending_events.push_back(DiscoveryEvent::ExternalAddressDiscovered { - address: observed_address.clone(), - }); - } - } + this.handle_observed_addresses(observed_address, peer); return Poll::Ready(Some(DiscoveryEvent::Identified { peer, diff --git a/substrate/client/network/src/peer_info.rs b/substrate/client/network/src/peer_info.rs index ee596b0a2033..3881dfffb86e 100644 --- a/substrate/client/network/src/peer_info.rs +++ b/substrate/client/network/src/peer_info.rs @@ -31,7 +31,6 @@ use libp2p::{ Info as IdentifyInfo, }, identity::PublicKey, - multiaddr::Protocol, ping::{Behaviour as Ping, Config as PingConfig, Event as PingEvent}, swarm::{ behaviour::{ @@ -184,53 +183,6 @@ impl PeerInfoBehaviour { "Received pong from node we're not connected to {:?}", peer_id); } } - - /// Verify the external address discovered by the identify protocol. - /// - /// This ensures that: - /// - the address is not empty - /// - the address contains a valid IP address - /// - the address is for the local peer ID - fn verify_external_address(&mut self, address: &Multiaddr) -> bool { - if address.is_empty() { - log::debug!( - target: "sub-libp2p", - "🔍 Discovered empty address", - ); - return false; - }; - - // Expect the address to contain a valid IP address. - if std::matches!( - address.iter().next(), - Some( - Protocol::Dns(_) | - Protocol::Dns4(_) | - Protocol::Dns6(_) | - Protocol::Ip6(_) | - Protocol::Ip4(_), - ) - ) { - log::debug!( - target: "sub-libp2p", - "🔍 Discovered external address does not contain a valid IP address: {address}", - ); - return false; - }; - - if let Some(Protocol::P2p(peer_id)) = address.iter().last() { - // Invalid address if the reported peer ID is not the local peer ID. - if peer_id != self.local_peer_id { - log::debug!( - target: "sub-libp2p", - "🔍 Discovered external address that is not us: {address}", - ); - return false - } - } - - true - } } /// Gives access to the information about a node. @@ -455,7 +407,8 @@ impl NetworkBehaviour for PeerInfoBehaviour { }, FromSwarm::NewExternalAddrCandidate(e @ NewExternalAddrCandidate { addr }) => { // Verify the external address is valid. - if self.verify_external_address(addr) { + let multiaddr: sc_network_types::multiaddr::Multiaddr = addr.clone().into(); + if multiaddr.is_valid_external_address(self.local_peer_id.into()) { self.ping.on_swarm_event(FromSwarm::NewExternalAddrCandidate(e)); self.identify.on_swarm_event(FromSwarm::NewExternalAddrCandidate(e)); diff --git a/substrate/client/network/types/src/multiaddr.rs b/substrate/client/network/types/src/multiaddr.rs index e86da94f22ee..b28e1a32ceac 100644 --- a/substrate/client/network/types/src/multiaddr.rs +++ b/substrate/client/network/types/src/multiaddr.rs @@ -84,9 +84,9 @@ impl Multiaddr { /// - the address is not empty /// - the address contains a valid IP address /// - the address is for the local peer ID - pub fn verify_external_address(&self, local_peer_id: PeerId) -> Result<(), ParseError> { + pub fn is_valid_external_address(&self, local_peer_id: PeerId) -> bool { if self.is_empty() { - return Err(ParseError::InvalidMultiaddr); + return false; } // Expect the address to contain a valid IP address. @@ -100,17 +100,17 @@ impl Multiaddr { Protocol::Ip4(_), ) ) { - return Err(ParseError::InvalidMultiaddr); + return false; } if let Some(Protocol::P2p(peer_id)) = self.iter().last() { // Invalid address if the reported peer ID is not the local peer ID. if peer_id != *local_peer_id.as_ref() { - return Err(ParseError::InvalidMultiaddr); + return false; } } - Ok(()) + true } /// Ensure the peer ID is present in the multiaddress. From 1dccdb3c82ff24674b8cb9483b4b524645e67407 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Thu, 21 Nov 2024 22:06:15 +0200 Subject: [PATCH 12/27] litep2p/req-resp: Always provide main protocol name in responses Signed-off-by: Alexandru Vasile --- .../network/src/litep2p/shim/request_response/mod.rs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/substrate/client/network/src/litep2p/shim/request_response/mod.rs b/substrate/client/network/src/litep2p/shim/request_response/mod.rs index bfd7a60ef9fe..146f2e4add97 100644 --- a/substrate/client/network/src/litep2p/shim/request_response/mod.rs +++ b/substrate/client/network/src/litep2p/shim/request_response/mod.rs @@ -320,7 +320,7 @@ impl RequestResponseProtocol { &mut self, peer: litep2p::PeerId, request_id: RequestId, - fallback: Option, + _fallback: Option, response: Vec, ) { match self.pending_inbound_responses.remove(&request_id) { @@ -337,10 +337,7 @@ impl RequestResponseProtocol { response.len(), ); - let _ = tx.send(Ok(( - response, - fallback.map_or_else(|| self.protocol.clone(), Into::into), - ))); + let _ = tx.send(Ok((response, self.protocol.clone()))); self.metrics.register_outbound_request_success(started.elapsed()); }, } From 9061359a5c20b65e5f8963834d0ef878c5ccca22 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 22 Nov 2024 16:59:57 +0200 Subject: [PATCH 13/27] authority-discovery: Use is_valid_external_address method Signed-off-by: Alexandru Vasile --- .../client/authority-discovery/src/worker.rs | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/substrate/client/authority-discovery/src/worker.rs b/substrate/client/authority-discovery/src/worker.rs index f198a4b04b9e..4b7d16329beb 100644 --- a/substrate/client/authority-discovery/src/worker.rs +++ b/substrate/client/authority-discovery/src/worker.rs @@ -850,8 +850,9 @@ where // Ignore [`Multiaddr`]s without [`PeerId`] or with own addresses. let addresses: Vec = addresses .into_iter() - .filter(|a| { - !address_is_empty(a) && get_peer_id(&a).filter(|p| *p != local_peer_id).is_some() + .filter(|addr| { + addr.is_valid_external_address(local_peer_id) && + get_peer_id(&addr).filter(|p| *p != local_peer_id).is_some() }) .collect(); @@ -1034,16 +1035,6 @@ impl AddressType { } } -/// Checks if an authority-discovery address is empty. -/// -/// An address is considered empty if: -/// - The multiaddr is empty. -/// - The multiaddr contains only the `/p2p/..` protocol. -fn address_is_empty(address: &Multiaddr) -> bool { - address.is_empty() || - address.iter().all(|protocol| matches!(protocol, multiaddr::Protocol::P2p(_))) -} - /// NetworkProvider provides [`Worker`] with all necessary hooks into the /// underlying Substrate networking. Using this trait abstraction instead of /// `sc_network::NetworkService` directly is necessary to unit test [`Worker`]. From 52acb47b336b77a98af7a0e0a7cf148e3bac11a7 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 22 Nov 2024 17:10:55 +0200 Subject: [PATCH 14/27] network/types: Enhance address validation by TCP / UDP protocols Signed-off-by: Alexandru Vasile --- .../client/network/types/src/multiaddr.rs | 32 ++++++++++++------- 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/substrate/client/network/types/src/multiaddr.rs b/substrate/client/network/types/src/multiaddr.rs index b28e1a32ceac..ea3ef0e44831 100644 --- a/substrate/client/network/types/src/multiaddr.rs +++ b/substrate/client/network/types/src/multiaddr.rs @@ -85,24 +85,32 @@ impl Multiaddr { /// - the address contains a valid IP address /// - the address is for the local peer ID pub fn is_valid_external_address(&self, local_peer_id: PeerId) -> bool { + // Empty addresses are not reachable. if self.is_empty() { return false; } - // Expect the address to contain a valid IP address. - if !std::matches!( - self.iter().next(), - Some( - Protocol::Dns(_) | - Protocol::Dns4(_) | - Protocol::Dns6(_) | - Protocol::Ip6(_) | - Protocol::Ip4(_), - ) - ) { - return false; + // For the address to be reachable we need an IP address with a protocol. + let mut iter = self.iter(); + match iter.next() { + Some(Protocol::Ip4(address)) => + if address.is_unspecified() { + return false; + }, + Some(Protocol::Ip6(address)) => + if address.is_unspecified() { + return false; + }, + Some(Protocol::Dns(_)) | Some(Protocol::Dns4(_)) | Some(Protocol::Dns6(_)) => {}, + _ => return false, + } + // Ensure TCP or UDP (future compatibility with QUIC) is present. + match iter.next() { + Some(Protocol::Tcp(_)) | Some(Protocol::Udp(_)) => {}, + _ => return false, } + // The last entry must be the local peer ID if present. if let Some(Protocol::P2p(peer_id)) = self.iter().last() { // Invalid address if the reported peer ID is not the local peer ID. if peer_id != *local_peer_id.as_ref() { From 6389be9cd814d7a769abdc2361294bfe83e9020e Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 22 Nov 2024 17:12:11 +0200 Subject: [PATCH 15/27] network: Rename is_valid_external_address Signed-off-by: Alexandru Vasile --- substrate/client/authority-discovery/src/worker.rs | 2 +- substrate/client/network/src/litep2p/discovery.rs | 2 +- substrate/client/network/src/peer_info.rs | 2 +- substrate/client/network/types/src/multiaddr.rs | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/substrate/client/authority-discovery/src/worker.rs b/substrate/client/authority-discovery/src/worker.rs index 4b7d16329beb..93b617662adc 100644 --- a/substrate/client/authority-discovery/src/worker.rs +++ b/substrate/client/authority-discovery/src/worker.rs @@ -851,7 +851,7 @@ where let addresses: Vec = addresses .into_iter() .filter(|addr| { - addr.is_valid_external_address(local_peer_id) && + addr.is_external_address_valid(local_peer_id) && get_peer_id(&addr).filter(|p| *p != local_peer_id).is_some() }) .collect(); diff --git a/substrate/client/network/src/litep2p/discovery.rs b/substrate/client/network/src/litep2p/discovery.rs index ce8e215ba0ed..82ed75939db1 100644 --- a/substrate/client/network/src/litep2p/discovery.rs +++ b/substrate/client/network/src/litep2p/discovery.rs @@ -494,7 +494,7 @@ impl Discovery { // Converting to and from `sc_network_types::multiaddr::Multiaddr` is cheap considering // it is a small wrapper over litep2p `Multiaddr`. let observed_address: sc_network_types::multiaddr::Multiaddr = observed_address.into(); - if !observed_address.is_valid_external_address(self.local_peer_id.into()) { + if !observed_address.is_external_address_valid(self.local_peer_id.into()) { log::debug!( target: LOG_TARGET, "Ignoring invalid external address {observed_address} from {peer:?}", diff --git a/substrate/client/network/src/peer_info.rs b/substrate/client/network/src/peer_info.rs index 3881dfffb86e..76d8b21c397a 100644 --- a/substrate/client/network/src/peer_info.rs +++ b/substrate/client/network/src/peer_info.rs @@ -408,7 +408,7 @@ impl NetworkBehaviour for PeerInfoBehaviour { FromSwarm::NewExternalAddrCandidate(e @ NewExternalAddrCandidate { addr }) => { // Verify the external address is valid. let multiaddr: sc_network_types::multiaddr::Multiaddr = addr.clone().into(); - if multiaddr.is_valid_external_address(self.local_peer_id.into()) { + if multiaddr.is_external_address_valid(self.local_peer_id.into()) { self.ping.on_swarm_event(FromSwarm::NewExternalAddrCandidate(e)); self.identify.on_swarm_event(FromSwarm::NewExternalAddrCandidate(e)); diff --git a/substrate/client/network/types/src/multiaddr.rs b/substrate/client/network/types/src/multiaddr.rs index ea3ef0e44831..e12c41719839 100644 --- a/substrate/client/network/types/src/multiaddr.rs +++ b/substrate/client/network/types/src/multiaddr.rs @@ -84,7 +84,7 @@ impl Multiaddr { /// - the address is not empty /// - the address contains a valid IP address /// - the address is for the local peer ID - pub fn is_valid_external_address(&self, local_peer_id: PeerId) -> bool { + pub fn is_external_address_valid(&self, local_peer_id: PeerId) -> bool { // Empty addresses are not reachable. if self.is_empty() { return false; From d109dd7743d7c70b073e885d9887428df54bff6c Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 22 Nov 2024 17:17:12 +0200 Subject: [PATCH 16/27] types/tests: Check empty addresses Signed-off-by: Alexandru Vasile --- .../client/network/types/src/multiaddr.rs | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/substrate/client/network/types/src/multiaddr.rs b/substrate/client/network/types/src/multiaddr.rs index e12c41719839..93dfa9c86263 100644 --- a/substrate/client/network/types/src/multiaddr.rs +++ b/substrate/client/network/types/src/multiaddr.rs @@ -350,3 +350,23 @@ macro_rules! build_multiaddr { } } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn check_external_empty_addr() { + // Plain empty address. + let empty_address = Multiaddr::empty(); + assert!(!empty_address.is_external_address_valid(PeerId::random())); + + // Address is still unusable. + let address_with_p2p = Multiaddr::from(Protocol::P2p(PeerId::random().into())); + assert!(!address_with_p2p.is_external_address_valid(PeerId::random())); + + // Address is not empty. + let valid_address: Multiaddr = "/dns/domain1.com/tcp/30333".parse().unwrap(); + assert!(valid_address.is_external_address_valid(PeerId::random())); + } +} From d6da5d444972b9048e72b3cb7f61b0cf71f3ac3f Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 22 Nov 2024 17:30:31 +0200 Subject: [PATCH 17/27] types/tests: Extensive tests to check the address is valid Signed-off-by: Alexandru Vasile --- .../client/network/types/src/multiaddr.rs | 113 +++++++++++++++++- 1 file changed, 108 insertions(+), 5 deletions(-) diff --git a/substrate/client/network/types/src/multiaddr.rs b/substrate/client/network/types/src/multiaddr.rs index 93dfa9c86263..72110f40b5e0 100644 --- a/substrate/client/network/types/src/multiaddr.rs +++ b/substrate/client/network/types/src/multiaddr.rs @@ -356,17 +356,120 @@ mod tests { use super::*; #[test] - fn check_external_empty_addr() { + fn check_is_external_address_valid() { + let peer_id = PeerId::random(); + // Plain empty address. let empty_address = Multiaddr::empty(); - assert!(!empty_address.is_external_address_valid(PeerId::random())); + assert!(!empty_address.is_external_address_valid(peer_id)); // Address is still unusable. - let address_with_p2p = Multiaddr::from(Protocol::P2p(PeerId::random().into())); - assert!(!address_with_p2p.is_external_address_valid(PeerId::random())); + // `/p2p/[random]` + let address_with_p2p = Multiaddr::from(Protocol::P2p(peer_id.into())); + assert!(!address_with_p2p.is_external_address_valid(peer_id)); // Address is not empty. let valid_address: Multiaddr = "/dns/domain1.com/tcp/30333".parse().unwrap(); - assert!(valid_address.is_external_address_valid(PeerId::random())); + assert!(valid_address.is_external_address_valid(peer_id)); + } + + #[test] + fn check_is_external_address_valid_ip() { + let peer_id = PeerId::random(); + let other_peer_id = PeerId::random(); + + // Check ip4/tcp. + let address_with_ip4_tcp = Multiaddr::from(Protocol::Ip4(Ipv4Addr::new(127, 0, 0, 1))) + .with(Protocol::Tcp(30333)) + .with(Protocol::P2p(peer_id.into())); + assert!(address_with_ip4_tcp.is_external_address_valid(peer_id)); + assert!(!address_with_ip4_tcp.is_external_address_valid(other_peer_id)); + + let address_with_ip4_tcp = + Multiaddr::from(Protocol::Ip4(Ipv4Addr::new(127, 0, 0, 1))).with(Protocol::Tcp(30333)); + assert!(address_with_ip4_tcp.is_external_address_valid(peer_id)); + assert!(address_with_ip4_tcp.is_external_address_valid(other_peer_id)); + + // Check ip4/udp. + let address_with_ip4_udp = Multiaddr::from(Protocol::Ip4(Ipv4Addr::new(127, 0, 0, 1))) + .with(Protocol::Udp(30333)) + .with(Protocol::P2p(peer_id.into())); + assert!(address_with_ip4_udp.is_external_address_valid(peer_id)); + assert!(!address_with_ip4_udp.is_external_address_valid(other_peer_id)); + + let address_with_ip4_udp = + Multiaddr::from(Protocol::Ip4(Ipv4Addr::new(127, 0, 0, 1))).with(Protocol::Udp(30333)); + assert!(address_with_ip4_udp.is_external_address_valid(peer_id)); + assert!(address_with_ip4_udp.is_external_address_valid(other_peer_id)); + + // Check ip6/tcp. + let address_with_ip6_tcp = + Multiaddr::from(Protocol::Ip6(Ipv6Addr::new(0, 0, 0, 0, 0, 0, 0, 1))) + .with(Protocol::Tcp(30333)) + .with(Protocol::P2p(peer_id.into())); + assert!(address_with_ip6_tcp.is_external_address_valid(peer_id)); + assert!(!address_with_ip6_tcp.is_external_address_valid(other_peer_id)); + + let address_with_ip6_tcp = + Multiaddr::from(Protocol::Ip6(Ipv6Addr::new(0, 0, 0, 0, 0, 0, 0, 1))) + .with(Protocol::Tcp(30333)); + assert!(address_with_ip6_tcp.is_external_address_valid(peer_id)); + assert!(address_with_ip6_tcp.is_external_address_valid(other_peer_id)); + + // Check ip6/udp. + let address_with_ip6_udp = + Multiaddr::from(Protocol::Ip6(Ipv6Addr::new(0, 0, 0, 0, 0, 0, 0, 1))) + .with(Protocol::Udp(30333)) + .with(Protocol::P2p(peer_id.into())); + assert!(address_with_ip6_udp.is_external_address_valid(peer_id)); + assert!(!address_with_ip6_udp.is_external_address_valid(other_peer_id)); + + let address_with_ip6_udp = + Multiaddr::from(Protocol::Ip6(Ipv6Addr::new(0, 0, 0, 0, 0, 0, 0, 1))) + .with(Protocol::Udp(30333)); + assert!(address_with_ip6_udp.is_external_address_valid(peer_id)); + assert!(address_with_ip6_udp.is_external_address_valid(other_peer_id)); + } + + #[test] + fn check_is_external_address_valid_dns() { + let peer_id = PeerId::random(); + let other_peer_id = PeerId::random(); + + // Check dns/tcp. + let address_with_dns = Multiaddr::from(Protocol::Dns("domain1.com".into())) + .with(Protocol::Tcp(30333)) + .with(Protocol::P2p(peer_id.into())); + assert!(address_with_dns.is_external_address_valid(peer_id)); + assert!(!address_with_dns.is_external_address_valid(other_peer_id)); + + let address_with_dns = + Multiaddr::from(Protocol::Dns("domain1.com".into())).with(Protocol::Tcp(30333)); + assert!(address_with_dns.is_external_address_valid(peer_id)); + assert!(address_with_dns.is_external_address_valid(other_peer_id)); + + // Check dns4/tcp. + let address_with_dns4 = Multiaddr::from(Protocol::Dns4("domain1.com".into())) + .with(Protocol::Tcp(30333)) + .with(Protocol::P2p(peer_id.into())); + assert!(address_with_dns4.is_external_address_valid(peer_id)); + assert!(!address_with_dns4.is_external_address_valid(other_peer_id)); + + let address_with_dns4 = + Multiaddr::from(Protocol::Dns4("domain1.com".into())).with(Protocol::Tcp(30333)); + assert!(address_with_dns4.is_external_address_valid(peer_id)); + assert!(address_with_dns4.is_external_address_valid(other_peer_id)); + + // Check dns6/tcp. + let address_with_dns6 = Multiaddr::from(Protocol::Dns6("domain1.com".into())) + .with(Protocol::Tcp(30333)) + .with(Protocol::P2p(peer_id.into())); + assert!(address_with_dns6.is_external_address_valid(peer_id)); + assert!(!address_with_dns6.is_external_address_valid(other_peer_id)); + + let address_with_dns6 = + Multiaddr::from(Protocol::Dns6("domain1.com".into())).with(Protocol::Tcp(30333)); + assert!(address_with_dns6.is_external_address_valid(peer_id)); + assert!(address_with_dns6.is_external_address_valid(other_peer_id)); } } From 6372b5f45c57a34575e3641703e62d1390c0c288 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 22 Nov 2024 17:41:42 +0200 Subject: [PATCH 18/27] litep2p: Use new multiaddr functionality Signed-off-by: Alexandru Vasile --- substrate/client/network/src/litep2p/mod.rs | 22 ++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/substrate/client/network/src/litep2p/mod.rs b/substrate/client/network/src/litep2p/mod.rs index eb71aa683aa9..2b3ca7bd4776 100644 --- a/substrate/client/network/src/litep2p/mod.rs +++ b/substrate/client/network/src/litep2p/mod.rs @@ -746,21 +746,21 @@ impl NetworkBackend for Litep2pNetworkBac }; } NetworkServiceCommand::AddKnownAddress { peer, address } => { - let mut address: Multiaddr = address.into(); - - if address.is_empty() { - log::debug!( + if !address.is_external_address_valid(peer) { + log::warn!( target: LOG_TARGET, - "Ignoring empty address for {peer:?}", + "ignoring invalid external address {address} for {peer:?}", ); continue } - - if !address.iter().any(|protocol| std::matches!(protocol, Protocol::P2p(_))) { - address.push(Protocol::P2p(litep2p::PeerId::from(peer).into())); - } - - if self.litep2p.add_known_address(peer.into(), iter::once(address.clone())) == 0usize { + let Some(address) = address.clone().ensure_peer_id(peer) else { + log::warn!( + target: LOG_TARGET, + "ignoring address ({address}) with different peer ID {peer:?}", + ); + continue + }; + if self.litep2p.add_known_address(peer.into(), iter::once(address.clone().into())) == 0usize { log::warn!( target: LOG_TARGET, "couldn't add known address ({address}) for {peer:?}, unsupported transport" From 335e0e84650a318353aebf81695ae3b49a4ab27b Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 22 Nov 2024 17:52:55 +0200 Subject: [PATCH 19/27] authority-discovery: Extra checks before publishing addresses Signed-off-by: Alexandru Vasile --- substrate/client/authority-discovery/src/worker.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/substrate/client/authority-discovery/src/worker.rs b/substrate/client/authority-discovery/src/worker.rs index 93b617662adc..3f0f0708623a 100644 --- a/substrate/client/authority-discovery/src/worker.rs +++ b/substrate/client/authority-discovery/src/worker.rs @@ -399,7 +399,7 @@ where .listen_addresses() .into_iter() .filter_map(|address| { - address_is_global(&address) + (address.is_external_address_valid(local_peer_id) && address_is_global(&address)) .then(|| AddressType::GlobalListenAddress(address).without_p2p(local_peer_id)) .flatten() }) @@ -412,9 +412,11 @@ where .external_addresses() .into_iter() .filter_map(|address| { - (publish_non_global_ips || address_is_global(&address)) - .then(|| AddressType::ExternalAddress(address).without_p2p(local_peer_id)) - .flatten() + (publish_non_global_ips || + (address_is_global(&address)) && + address.is_external_address_valid(local_peer_id)) + .then(|| AddressType::ExternalAddress(address).without_p2p(local_peer_id)) + .flatten() }) .peekable(); From 19a3c4ef4e760de8e124c8f79846e42e81205db6 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 22 Nov 2024 17:53:27 +0200 Subject: [PATCH 20/27] authority-discovery/tests: Test are moved to network/types Signed-off-by: Alexandru Vasile --- .../client/authority-discovery/src/worker.rs | 20 ------------------- 1 file changed, 20 deletions(-) diff --git a/substrate/client/authority-discovery/src/worker.rs b/substrate/client/authority-discovery/src/worker.rs index 3f0f0708623a..56cbf992f53a 100644 --- a/substrate/client/authority-discovery/src/worker.rs +++ b/substrate/client/authority-discovery/src/worker.rs @@ -1216,23 +1216,3 @@ impl Worker Date: Fri, 22 Nov 2024 17:58:26 +0200 Subject: [PATCH 21/27] network/types: Move is_global directly on the multiaddr Signed-off-by: Alexandru Vasile --- Cargo.lock | 2 +- substrate/client/authority-discovery/Cargo.toml | 1 - .../client/authority-discovery/src/worker.rs | 17 ++--------------- substrate/client/network/types/Cargo.toml | 1 + substrate/client/network/types/src/multiaddr.rs | 13 ++++++++++++- 5 files changed, 16 insertions(+), 18 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 330c2563d976..b608bc0a59ce 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -21990,7 +21990,6 @@ dependencies = [ "async-trait", "futures", "futures-timer", - "ip_network", "linked_hash_set", "log", "multihash 0.19.1", @@ -23095,6 +23094,7 @@ dependencies = [ "bs58", "bytes", "ed25519-dalek", + "ip_network", "libp2p-identity", "libp2p-kad", "litep2p", diff --git a/substrate/client/authority-discovery/Cargo.toml b/substrate/client/authority-discovery/Cargo.toml index fc88d07ef936..abd3bbaff214 100644 --- a/substrate/client/authority-discovery/Cargo.toml +++ b/substrate/client/authority-discovery/Cargo.toml @@ -23,7 +23,6 @@ prost-build = { workspace = true } codec = { workspace = true } futures = { workspace = true } futures-timer = { workspace = true } -ip_network = { workspace = true } multihash = { workspace = true } linked_hash_set = { workspace = true } log = { workspace = true, default-features = true } diff --git a/substrate/client/authority-discovery/src/worker.rs b/substrate/client/authority-discovery/src/worker.rs index 56cbf992f53a..eff315fc7e23 100644 --- a/substrate/client/authority-discovery/src/worker.rs +++ b/substrate/client/authority-discovery/src/worker.rs @@ -33,7 +33,6 @@ use futures::{channel::mpsc, future, stream::Fuse, FutureExt, Stream, StreamExt} use addr_cache::AddrCache; use codec::{Decode, Encode}; -use ip_network::IpNetwork; use linked_hash_set::LinkedHashSet; use sc_network_types::kad::{Key, PeerRecord, Record}; @@ -376,17 +375,6 @@ where let local_peer_id = self.network.local_peer_id(); let publish_non_global_ips = self.publish_non_global_ips; - // Checks that the address is global. - let address_is_global = |address: &Multiaddr| { - address.iter().all(|protocol| match protocol { - // The `ip_network` library is used because its `is_global()` method is stable, - // while `is_global()` in the standard library currently isn't. - multiaddr::Protocol::Ip4(ip) => IpNetwork::from(ip).is_global(), - multiaddr::Protocol::Ip6(ip) => IpNetwork::from(ip).is_global(), - _ => true, - }) - }; - // These are the addresses the node is listening for incoming connections, // as reported by installed protocols (tcp / websocket etc). // @@ -399,7 +387,7 @@ where .listen_addresses() .into_iter() .filter_map(|address| { - (address.is_external_address_valid(local_peer_id) && address_is_global(&address)) + (address.is_external_address_valid(local_peer_id) && address.is_global()) .then(|| AddressType::GlobalListenAddress(address).without_p2p(local_peer_id)) .flatten() }) @@ -413,8 +401,7 @@ where .into_iter() .filter_map(|address| { (publish_non_global_ips || - (address_is_global(&address)) && - address.is_external_address_valid(local_peer_id)) + (address.is_external_address_valid(local_peer_id) && address.is_global())) .then(|| AddressType::ExternalAddress(address).without_p2p(local_peer_id)) .flatten() }) diff --git a/substrate/client/network/types/Cargo.toml b/substrate/client/network/types/Cargo.toml index 7438eaeffcd2..61b9a7f98019 100644 --- a/substrate/client/network/types/Cargo.toml +++ b/substrate/client/network/types/Cargo.toml @@ -13,6 +13,7 @@ documentation = "https://docs.rs/sc-network-types" bs58 = { workspace = true, default-features = true } bytes = { version = "1.4.0", default-features = false } ed25519-dalek = { workspace = true, default-features = true } +ip_network = { workspace = true } libp2p-identity = { features = ["ed25519", "peerid", "rand"], workspace = true } libp2p-kad = { version = "0.44.6", default-features = false } litep2p = { workspace = true } diff --git a/substrate/client/network/types/src/multiaddr.rs b/substrate/client/network/types/src/multiaddr.rs index 72110f40b5e0..3775451ab23c 100644 --- a/substrate/client/network/types/src/multiaddr.rs +++ b/substrate/client/network/types/src/multiaddr.rs @@ -78,12 +78,23 @@ impl Multiaddr { self.multiaddr.to_vec() } + // Checks that the address is global. + pub fn is_global(&self) -> bool { + self.iter().all(|protocol| match protocol { + // The `ip_network` library is used because its `is_global()` method is stable, + // while `is_global()` in the standard library currently isn't. + Protocol::Ip4(ip) => ip_network::IpNetwork::from(ip).is_global(), + Protocol::Ip6(ip) => ip_network::IpNetwork::from(ip).is_global(), + _ => true, + }) + } + /// Verify the external address is valid. /// /// An external address address discovered by the network is valid when: /// - the address is not empty /// - the address contains a valid IP address - /// - the address is for the local peer ID + /// - the address /p2p/peer corresponds to the local peer ID pub fn is_external_address_valid(&self, local_peer_id: PeerId) -> bool { // Empty addresses are not reachable. if self.is_empty() { From bb5653eb04fbce40b2b7eb1f1ad62506ea2c4b0a Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Mon, 25 Nov 2024 14:20:30 +0200 Subject: [PATCH 22/27] authorit-discovery: Add more logs Signed-off-by: Alexandru Vasile --- substrate/client/authority-discovery/src/worker.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/substrate/client/authority-discovery/src/worker.rs b/substrate/client/authority-discovery/src/worker.rs index eff315fc7e23..b5e93ad61f8e 100644 --- a/substrate/client/authority-discovery/src/worker.rs +++ b/substrate/client/authority-discovery/src/worker.rs @@ -836,6 +836,13 @@ where _ => None, }; + log::debug!( + target: LOG_TARGET, + "Received DHT record for authority {:?} with addresses {:?}", + authority_id, + addresses + ); + // Ignore [`Multiaddr`]s without [`PeerId`] or with own addresses. let addresses: Vec = addresses .into_iter() From 36ff0c75a4ae5f30519381c2ad74fe833468a674 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Mon, 25 Nov 2024 16:07:20 +0200 Subject: [PATCH 23/27] types: Do not check peerID implicitely Signed-off-by: Alexandru Vasile --- substrate/client/authority-discovery/src/worker.rs | 6 +++--- substrate/client/network/src/litep2p/discovery.rs | 2 +- substrate/client/network/src/litep2p/mod.rs | 2 +- substrate/client/network/src/peer_info.rs | 4 +++- substrate/client/network/types/src/multiaddr.rs | 11 +---------- 5 files changed, 9 insertions(+), 16 deletions(-) diff --git a/substrate/client/authority-discovery/src/worker.rs b/substrate/client/authority-discovery/src/worker.rs index b5e93ad61f8e..598d4518bd27 100644 --- a/substrate/client/authority-discovery/src/worker.rs +++ b/substrate/client/authority-discovery/src/worker.rs @@ -387,7 +387,7 @@ where .listen_addresses() .into_iter() .filter_map(|address| { - (address.is_external_address_valid(local_peer_id) && address.is_global()) + (address.is_external_address_valid() && address.is_global()) .then(|| AddressType::GlobalListenAddress(address).without_p2p(local_peer_id)) .flatten() }) @@ -401,7 +401,7 @@ where .into_iter() .filter_map(|address| { (publish_non_global_ips || - (address.is_external_address_valid(local_peer_id) && address.is_global())) + (address.is_external_address_valid() && address.is_global())) .then(|| AddressType::ExternalAddress(address).without_p2p(local_peer_id)) .flatten() }) @@ -847,7 +847,7 @@ where let addresses: Vec = addresses .into_iter() .filter(|addr| { - addr.is_external_address_valid(local_peer_id) && + addr.is_external_address_valid() && get_peer_id(&addr).filter(|p| *p != local_peer_id).is_some() }) .collect(); diff --git a/substrate/client/network/src/litep2p/discovery.rs b/substrate/client/network/src/litep2p/discovery.rs index 82ed75939db1..a16aa0664f56 100644 --- a/substrate/client/network/src/litep2p/discovery.rs +++ b/substrate/client/network/src/litep2p/discovery.rs @@ -494,7 +494,7 @@ impl Discovery { // Converting to and from `sc_network_types::multiaddr::Multiaddr` is cheap considering // it is a small wrapper over litep2p `Multiaddr`. let observed_address: sc_network_types::multiaddr::Multiaddr = observed_address.into(); - if !observed_address.is_external_address_valid(self.local_peer_id.into()) { + if !observed_address.is_external_address_valid() { log::debug!( target: LOG_TARGET, "Ignoring invalid external address {observed_address} from {peer:?}", diff --git a/substrate/client/network/src/litep2p/mod.rs b/substrate/client/network/src/litep2p/mod.rs index 2b3ca7bd4776..493fb8503c82 100644 --- a/substrate/client/network/src/litep2p/mod.rs +++ b/substrate/client/network/src/litep2p/mod.rs @@ -746,7 +746,7 @@ impl NetworkBackend for Litep2pNetworkBac }; } NetworkServiceCommand::AddKnownAddress { peer, address } => { - if !address.is_external_address_valid(peer) { + if !address.is_external_address_valid() { log::warn!( target: LOG_TARGET, "ignoring invalid external address {address} for {peer:?}", diff --git a/substrate/client/network/src/peer_info.rs b/substrate/client/network/src/peer_info.rs index 76d8b21c397a..f9ee1d150883 100644 --- a/substrate/client/network/src/peer_info.rs +++ b/substrate/client/network/src/peer_info.rs @@ -408,7 +408,9 @@ impl NetworkBehaviour for PeerInfoBehaviour { FromSwarm::NewExternalAddrCandidate(e @ NewExternalAddrCandidate { addr }) => { // Verify the external address is valid. let multiaddr: sc_network_types::multiaddr::Multiaddr = addr.clone().into(); - if multiaddr.is_external_address_valid(self.local_peer_id.into()) { + if multiaddr.is_external_address_valid() && + multiaddr.ensure_peer_id(self.local_peer_id.into()).is_some() + { self.ping.on_swarm_event(FromSwarm::NewExternalAddrCandidate(e)); self.identify.on_swarm_event(FromSwarm::NewExternalAddrCandidate(e)); diff --git a/substrate/client/network/types/src/multiaddr.rs b/substrate/client/network/types/src/multiaddr.rs index 3775451ab23c..da5287beaa91 100644 --- a/substrate/client/network/types/src/multiaddr.rs +++ b/substrate/client/network/types/src/multiaddr.rs @@ -94,8 +94,7 @@ impl Multiaddr { /// An external address address discovered by the network is valid when: /// - the address is not empty /// - the address contains a valid IP address - /// - the address /p2p/peer corresponds to the local peer ID - pub fn is_external_address_valid(&self, local_peer_id: PeerId) -> bool { + pub fn is_external_address_valid(&self) -> bool { // Empty addresses are not reachable. if self.is_empty() { return false; @@ -121,14 +120,6 @@ impl Multiaddr { _ => return false, } - // The last entry must be the local peer ID if present. - if let Some(Protocol::P2p(peer_id)) = self.iter().last() { - // Invalid address if the reported peer ID is not the local peer ID. - if peer_id != *local_peer_id.as_ref() { - return false; - } - } - true } From 457b4f1ee4b84f05e8e7a2e972dd819a5058f23d Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Mon, 25 Nov 2024 16:15:16 +0200 Subject: [PATCH 24/27] types/tests: Adjust testing Signed-off-by: Alexandru Vasile --- .../client/network/types/src/multiaddr.rs | 50 +++++++------------ 1 file changed, 17 insertions(+), 33 deletions(-) diff --git a/substrate/client/network/types/src/multiaddr.rs b/substrate/client/network/types/src/multiaddr.rs index da5287beaa91..befc5656b6c1 100644 --- a/substrate/client/network/types/src/multiaddr.rs +++ b/substrate/client/network/types/src/multiaddr.rs @@ -363,115 +363,99 @@ mod tests { // Plain empty address. let empty_address = Multiaddr::empty(); - assert!(!empty_address.is_external_address_valid(peer_id)); + assert!(!empty_address.is_external_address_valid()); // Address is still unusable. // `/p2p/[random]` let address_with_p2p = Multiaddr::from(Protocol::P2p(peer_id.into())); - assert!(!address_with_p2p.is_external_address_valid(peer_id)); + assert!(!address_with_p2p.is_external_address_valid()); // Address is not empty. let valid_address: Multiaddr = "/dns/domain1.com/tcp/30333".parse().unwrap(); - assert!(valid_address.is_external_address_valid(peer_id)); + assert!(valid_address.is_external_address_valid()); } #[test] fn check_is_external_address_valid_ip() { let peer_id = PeerId::random(); - let other_peer_id = PeerId::random(); // Check ip4/tcp. let address_with_ip4_tcp = Multiaddr::from(Protocol::Ip4(Ipv4Addr::new(127, 0, 0, 1))) .with(Protocol::Tcp(30333)) .with(Protocol::P2p(peer_id.into())); - assert!(address_with_ip4_tcp.is_external_address_valid(peer_id)); - assert!(!address_with_ip4_tcp.is_external_address_valid(other_peer_id)); + assert!(address_with_ip4_tcp.is_external_address_valid()); let address_with_ip4_tcp = Multiaddr::from(Protocol::Ip4(Ipv4Addr::new(127, 0, 0, 1))).with(Protocol::Tcp(30333)); - assert!(address_with_ip4_tcp.is_external_address_valid(peer_id)); - assert!(address_with_ip4_tcp.is_external_address_valid(other_peer_id)); + assert!(address_with_ip4_tcp.is_external_address_valid()); // Check ip4/udp. let address_with_ip4_udp = Multiaddr::from(Protocol::Ip4(Ipv4Addr::new(127, 0, 0, 1))) .with(Protocol::Udp(30333)) .with(Protocol::P2p(peer_id.into())); - assert!(address_with_ip4_udp.is_external_address_valid(peer_id)); - assert!(!address_with_ip4_udp.is_external_address_valid(other_peer_id)); + assert!(address_with_ip4_udp.is_external_address_valid()); let address_with_ip4_udp = Multiaddr::from(Protocol::Ip4(Ipv4Addr::new(127, 0, 0, 1))).with(Protocol::Udp(30333)); - assert!(address_with_ip4_udp.is_external_address_valid(peer_id)); - assert!(address_with_ip4_udp.is_external_address_valid(other_peer_id)); + assert!(address_with_ip4_udp.is_external_address_valid()); // Check ip6/tcp. let address_with_ip6_tcp = Multiaddr::from(Protocol::Ip6(Ipv6Addr::new(0, 0, 0, 0, 0, 0, 0, 1))) .with(Protocol::Tcp(30333)) .with(Protocol::P2p(peer_id.into())); - assert!(address_with_ip6_tcp.is_external_address_valid(peer_id)); - assert!(!address_with_ip6_tcp.is_external_address_valid(other_peer_id)); + assert!(address_with_ip6_tcp.is_external_address_valid()); let address_with_ip6_tcp = Multiaddr::from(Protocol::Ip6(Ipv6Addr::new(0, 0, 0, 0, 0, 0, 0, 1))) .with(Protocol::Tcp(30333)); - assert!(address_with_ip6_tcp.is_external_address_valid(peer_id)); - assert!(address_with_ip6_tcp.is_external_address_valid(other_peer_id)); + assert!(address_with_ip6_tcp.is_external_address_valid()); // Check ip6/udp. let address_with_ip6_udp = Multiaddr::from(Protocol::Ip6(Ipv6Addr::new(0, 0, 0, 0, 0, 0, 0, 1))) .with(Protocol::Udp(30333)) .with(Protocol::P2p(peer_id.into())); - assert!(address_with_ip6_udp.is_external_address_valid(peer_id)); - assert!(!address_with_ip6_udp.is_external_address_valid(other_peer_id)); + assert!(address_with_ip6_udp.is_external_address_valid()); let address_with_ip6_udp = Multiaddr::from(Protocol::Ip6(Ipv6Addr::new(0, 0, 0, 0, 0, 0, 0, 1))) .with(Protocol::Udp(30333)); - assert!(address_with_ip6_udp.is_external_address_valid(peer_id)); - assert!(address_with_ip6_udp.is_external_address_valid(other_peer_id)); + assert!(address_with_ip6_udp.is_external_address_valid()); } #[test] fn check_is_external_address_valid_dns() { let peer_id = PeerId::random(); - let other_peer_id = PeerId::random(); // Check dns/tcp. let address_with_dns = Multiaddr::from(Protocol::Dns("domain1.com".into())) .with(Protocol::Tcp(30333)) .with(Protocol::P2p(peer_id.into())); - assert!(address_with_dns.is_external_address_valid(peer_id)); - assert!(!address_with_dns.is_external_address_valid(other_peer_id)); + assert!(address_with_dns.is_external_address_valid()); let address_with_dns = Multiaddr::from(Protocol::Dns("domain1.com".into())).with(Protocol::Tcp(30333)); - assert!(address_with_dns.is_external_address_valid(peer_id)); - assert!(address_with_dns.is_external_address_valid(other_peer_id)); + assert!(address_with_dns.is_external_address_valid()); // Check dns4/tcp. let address_with_dns4 = Multiaddr::from(Protocol::Dns4("domain1.com".into())) .with(Protocol::Tcp(30333)) .with(Protocol::P2p(peer_id.into())); - assert!(address_with_dns4.is_external_address_valid(peer_id)); - assert!(!address_with_dns4.is_external_address_valid(other_peer_id)); + assert!(address_with_dns4.is_external_address_valid()); let address_with_dns4 = Multiaddr::from(Protocol::Dns4("domain1.com".into())).with(Protocol::Tcp(30333)); - assert!(address_with_dns4.is_external_address_valid(peer_id)); - assert!(address_with_dns4.is_external_address_valid(other_peer_id)); + assert!(address_with_dns4.is_external_address_valid()); // Check dns6/tcp. let address_with_dns6 = Multiaddr::from(Protocol::Dns6("domain1.com".into())) .with(Protocol::Tcp(30333)) .with(Protocol::P2p(peer_id.into())); - assert!(address_with_dns6.is_external_address_valid(peer_id)); - assert!(!address_with_dns6.is_external_address_valid(other_peer_id)); + assert!(address_with_dns6.is_external_address_valid()); let address_with_dns6 = Multiaddr::from(Protocol::Dns6("domain1.com".into())).with(Protocol::Tcp(30333)); - assert!(address_with_dns6.is_external_address_valid(peer_id)); - assert!(address_with_dns6.is_external_address_valid(other_peer_id)); + assert!(address_with_dns6.is_external_address_valid()); } } From 658154b5c5256d490c2710532c06c2261194875e Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Mon, 25 Nov 2024 16:25:31 +0200 Subject: [PATCH 25/27] Add PR doc Signed-off-by: Alexandru Vasile --- prdoc/pr_6603.prdoc | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) create mode 100644 prdoc/pr_6603.prdoc diff --git a/prdoc/pr_6603.prdoc b/prdoc/pr_6603.prdoc new file mode 100644 index 000000000000..20c5e7294dfa --- /dev/null +++ b/prdoc/pr_6603.prdoc @@ -0,0 +1,16 @@ +# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0 +# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json + +title: Always provide main protocol name in litep2p responses + +doc: + - audience: [ Node Dev, Node Operator ] + description: | + This PR aligns litep2p behavior with libp2p. Previously, litep2p network backend + would provide the actual negotiated request-response protocol that produced a + response message. After this PR, only the main protocol name is reported to other + subsystems. + +crates: + - name: sc-network + bump: patch From ff8042d2715ace5382057e580ff2a4184f1dce5f Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 26 Nov 2024 14:24:34 +0200 Subject: [PATCH 26/27] cargo: Udpdate litep2p to specific branch Signed-off-by: Alexandru Vasile --- Cargo.lock | 3 +-- Cargo.toml | 2 +- substrate/client/network/src/litep2p/mod.rs | 1 + 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 6d14a119ec4f..82d180aa5592 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -10196,8 +10196,7 @@ dependencies = [ [[package]] name = "litep2p" version = "0.8.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5b67484b8ac41e1cfdf012f65fa81e88c2ef5f8a7d6dec0e2678c2d06dc04530" +source = "git+https://github.com/paritytech/litep2p.git?branch=lexnv/investigate-mismatch#eed0f755d591eb49e1b715a78aceb5a653ddf529" dependencies = [ "async-trait", "bs58", diff --git a/Cargo.toml b/Cargo.toml index 533ea4c9e878..c70861e5c4f4 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -848,7 +848,7 @@ linked-hash-map = { version = "0.5.4" } linked_hash_set = { version = "0.1.4" } linregress = { version = "0.5.1" } lite-json = { version = "0.2.0", default-features = false } -litep2p = { version = "0.8.1", features = ["websocket"] } +litep2p = { git = "https://github.com/paritytech/litep2p.git", branch = "lexnv/investigate-mismatch", features = ["websocket"] } log = { version = "0.4.22", default-features = false } macro_magic = { version = "0.5.1" } maplit = { version = "1.0.2" } diff --git a/substrate/client/network/src/litep2p/mod.rs b/substrate/client/network/src/litep2p/mod.rs index 493fb8503c82..c1c0c091eaf2 100644 --- a/substrate/client/network/src/litep2p/mod.rs +++ b/substrate/client/network/src/litep2p/mod.rs @@ -1066,6 +1066,7 @@ impl NetworkBackend for Litep2pNetworkBac NegotiationError::ParseError(_) => "parse-error", NegotiationError::IoError(_) => "io-error", NegotiationError::WebSocket(_) => "webscoket-error", + NegotiationError::BadSignature => "bad-signature", } }; From 68872d997f0e7e0b18ca4d7abc339af03cd17b13 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 26 Nov 2024 15:49:46 +0200 Subject: [PATCH 27/27] litep2p: Increment incoming connections metric Signed-off-by: Alexandru Vasile --- substrate/client/network/src/litep2p/mod.rs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/substrate/client/network/src/litep2p/mod.rs b/substrate/client/network/src/litep2p/mod.rs index c1c0c091eaf2..b5cc57ccff9f 100644 --- a/substrate/client/network/src/litep2p/mod.rs +++ b/substrate/client/network/src/litep2p/mod.rs @@ -994,7 +994,16 @@ impl NetworkBackend for Litep2pNetworkBac let direction = match endpoint { Endpoint::Dialer { .. } => "out", - Endpoint::Listener { .. } => "in", + Endpoint::Listener { .. } => { + // Increment incoming connections counter. + // + // Note: For litep2p these are represented by established connections, + // while in libp2p (legacy) these are not-yet-negotiated connections. However, + // wasting CPU cycles does not justify a slight difference in the metric. + metrics.incoming_connections_total.inc(); + + "in" + }, }; metrics.connections_opened_total.with_label_values(&[direction]).inc();