authority-discovery: Ignore multi-addresses without IP from DHT records

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

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()
 		};
 
@@ -397,8 +399,9 @@ 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()
 			})
 			.take(MAX_GLOBAL_LISTEN_ADDRESSES)
 			.peekable();
@@ -409,8 +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))
+				(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();
 
@@ -846,7 +852,10 @@ where
 		// Ignore [`Multiaddr`]s without [`PeerId`] or with own addresses.
 		let addresses: Vec<Multiaddr> = addresses
 			.into_iter()
-			.filter(|a| get_peer_id(&a).filter(|p| *p != local_peer_id).is_some())
+			.filter(|addr| {
+				addr.is_external_address_valid(local_peer_id) &&
+					get_peer_id(&addr).filter(|p| *p != local_peer_id).is_some()
+			})
 			.collect();
 
 		let remote_peer_id = single(addresses.iter().map(|a| get_peer_id(&a)))
@@ -998,7 +1007,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<Multiaddr> {
 		// Get the address and the source str for logging.
 		let (mut address, source) = match self {
 			AddressType::PublicAddress(address) => (address, "public address"),
@@ -1016,7 +1027,13 @@ impl AddressType {
 			}
 			address.pop();
 		}
-		address
+
+		// Empty addresses cannot be published.
+		if address.is_empty() {
+			None
+		} else {
+			Some(address)
+		}
 	}
 }
 

substrate/client/network/src/litep2p/discovery.rs

@@ -488,6 +488,48 @@ impl Discovery {
 
 		(false, None)
 	}
+
+	/// 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_external_address_valid(self.local_peer_id.into()) {
+			log::debug!(
+				target: LOG_TARGET,
+				"Ignoring invalid external address {observed_address} from {peer:?}",
+			);
+			return;
+		}
+
+		let Some(observed_address) = observed_address.ensure_peer_id(self.local_peer_id.into())
+		else {
+			log::debug!(
+				target: LOG_TARGET,
+				"Ignoring external address with different peer ID from {peer:?}",
+			);
+			return
+		};
+		let observed_address = observed_address.into();
+
+		let (is_new, expired_address) = self.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}",
+			);
+
+			self.pending_events
+				.push_back(DiscoveryEvent::ExternalAddressExpired { address: expired_address });
+		}
+
+		if is_new {
+			self.pending_events.push_back(DiscoveryEvent::ExternalAddressDiscovered {
+				address: observed_address.clone(),
+			});
+		}
+	}
 }
 
 impl Stream for Discovery {
@@ -596,44 +638,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())))
-					};
-
-				// 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/litep2p/mod.rs

@@ -746,13 +746,21 @@ impl<B: BlockT + 'static, H: ExHashT> NetworkBackend<B, H> for Litep2pNetworkBac
 							};
 						}
 						NetworkServiceCommand::AddKnownAddress { peer, address } => {
-							let mut address: Multiaddr = address.into();
-
-							if !address.iter().any(|protocol| std::matches!(protocol, Protocol::P2p(_))) {
-								address.push(Protocol::P2p(litep2p::PeerId::from(peer).into()));
+							if !address.is_external_address_valid(peer) {
+								log::warn!(
+									target: LOG_TARGET,
+									"ignoring invalid external address {address} for {peer:?}",
+								);
+								continue
 							}
-
-							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"

substrate/client/network/src/peer_info.rs

@@ -66,6 +66,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<PeerId, NodeInfo>,
 	/// Interval at which we perform garbage collection in `nodes_info`.
@@ -123,6 +126,7 @@ impl PeerInfoBehaviour {
 		local_public_key: PublicKey,
 		external_addresses: Arc<Mutex<HashSet<Multiaddr>>>,
 	) -> 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 +138,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 },
@@ -401,17 +406,21 @@ 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.
+				let multiaddr: sc_network_types::multiaddr::Multiaddr = addr.clone().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));
+
+					// 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));