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

prdoc/pr_6545.prdoc

@@ -0,0 +1,33 @@
+title: Ignore multi-addresses without IP from DHT records
+doc:
+- audience: Node Dev
+  description: |-
+    Kusama has several authority-discovery records published without a proper mutlti-address format.
+
+    ```bash
+    authority="CzHcp6nEsT4NJuAbf4yCS2J8KKYpvuAe63bkWrPWhjaaT6w"
+    ..
+    addresses={.., /p2p/12D3KooWFegWJubWCeJCbWpDEAwukQmXKaWTyRgRRZ8NNdzmuxSr, ..}
+    ```
+    In the previous example, an multiaddress of form `/p2p/12D3KooWFegWJubWCeJCbWpDEAwukQmXKaWTyRgRRZ8NNdzmuxSr` cannot establish a connection to the authority.
+
+    This PR changes the following:
+    - publishing DHT records no longer includes multi addresses that are empty or just contain `/p2p/[peer]` format
+    - received DHT records are filtered in a similar manner
+    - litep2p and libp2p backends verify discovered external addresses before pushing them to other subcomponents
+
+
+    ### Next Steps
+    - [x] versi-net burnin with detailed logs
+    - [x] kusama nodes (litep2p + libp2p) to ensure the warnings are not excessive
+
+    Closes: https://github.com/paritytech/polkadot-sdk/issues/6518
+
+    cc @paritytech/networking
+crates:
+- name: sc-authority-discovery
+  bump: patch
+- name: sc-network-types
+  bump: patch
+- name: sc-network
+  bump: patch

substrate/client/authority-discovery/Cargo.toml

@@ -24,6 +24,7 @@ async-trait = { workspace = true }
 codec = { workspace = true }
 futures = { workspace = true }
 futures-timer = { workspace = true }
+multihash = { workspace = true }
 ip_network = { workspace = true }
 linked_hash_set = { workspace = true }
 log = { workspace = true, default-features = true }

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};
 
@@ -280,7 +279,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()
 		};
 
@@ -374,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).
 		//
@@ -397,8 +387,9 @@ where
 			.listen_addresses()
 			.into_iter()
 			.filter_map(|address| {
-				address_is_global(&address)
+				(address.is_external_address_valid() && address.is_global())
 					.then(|| AddressType::GlobalListenAddress(address).without_p2p(local_peer_id))
+					.flatten()
 			})
 			.take(MAX_GLOBAL_LISTEN_ADDRESSES)
 			.peekable();
@@ -409,8 +400,10 @@ 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_external_address_valid() && address.is_global()))
+				.then(|| AddressType::ExternalAddress(address).without_p2p(local_peer_id))
+				.flatten()
 			})
 			.peekable();
 
@@ -846,10 +839,20 @@ 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<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() &&
+					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)))
@@ -1001,7 +1004,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"),
@@ -1019,7 +1024,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

@@ -511,6 +511,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() {
+			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 {
@@ -633,44 +675,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

@@ -762,10 +762,12 @@ 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() {
+								log::debug!(
+									target: LOG_TARGET,
+									"ignoring invalid external address {address} for {peer:?}",
+								);
+								continue
 							}
 
 							if self.litep2p.add_known_address(peer.into(), iter::once(address.clone())) == 0usize {

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 },
@@ -406,17 +411,23 @@ 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() &&
+					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));
+
+					// 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));

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.46.2", default-features = false }
 litep2p = { workspace = true }