From 3906c578c96d97a8a099a4bdac4685acbe375a7c Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Thu, 21 Nov 2024 14:27:53 +0200 Subject: [PATCH] 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.