Use crate::type::Multiaddr shared functionality

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

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/<local_peer_id>` protocol.
-	///
-	/// The provided peer ID is only used for logging purposes.
-	fn verify_external_address(&mut self, address: Multiaddr, peer: PeerId) -> Option<Multiaddr> {
-		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,

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));
 

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.