net/discovery: Do not propagate external addr with different peerIDs (#…

…6380)

This PR ensures that external addresses with different PeerIDs are not
propagated to the higher layer of the network code.

While at it, this ensures that libp2p only adds the `/p2p/peerid` part
to the discovered address if it does not contain it already.

This is a followup from:
- #6298

cc @paritytech/networking

---------

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Co-authored-by: Dmitry Markin <dmitry@markin.tech>

prdoc/pr_6380.prdoc

@@ -0,0 +1,11 @@
+title: Do not propagate external addr with different peerIDs
+
+doc:
+  - audience: [ Node Dev, Node Operator ]
+    description: |
+      External addresses that belong to a different peerID are no longer
+      propagated to the higher layers of the networking backends.
+
+crates:
+  - name: sc-network
+    bump: patch

substrate/client/network/src/discovery.rs

@@ -749,16 +749,28 @@ impl NetworkBehaviour for DiscoveryBehaviour {
 				self.mdns.on_swarm_event(FromSwarm::NewListenAddr(e));
 			},
 			FromSwarm::ExternalAddrConfirmed(e @ ExternalAddrConfirmed { addr }) => {
-				let new_addr = addr.clone().with(Protocol::P2p(self.local_peer_id));
+				let mut address = addr.clone();
 
-				if Self::can_add_to_dht(addr) {
+				if let Some(Protocol::P2p(peer_id)) = addr.iter().last() {
+					if peer_id != self.local_peer_id {
+						warn!(
+							target: "sub-libp2p",
+							"🔍 Discovered external address for a peer that is not us: {addr}",
+						);
+						// Ensure this address is not propagated to kademlia.
+						return
+					}
+				} else {
+					address.push(Protocol::P2p(self.local_peer_id));
+				}
+
+				if Self::can_add_to_dht(&address) {
 					// NOTE: we might re-discover the same address multiple times
 					// in which case we just want to refrain from logging.
-					if self.known_external_addresses.insert(new_addr.clone()) {
+					if self.known_external_addresses.insert(address.clone()) {
 						info!(
 						  target: "sub-libp2p",
-						  "🔍 Discovered new external address for our node: {}",
-						  new_addr,
+						  "🔍 Discovered new external address for our node: {address}",
 						);
 					}
 				}

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

@@ -162,6 +162,9 @@ pub enum DiscoveryEvent {
 
 /// Discovery.
 pub struct Discovery {
+	/// Local peer ID.
+	local_peer_id: litep2p::PeerId,
+
 	/// Ping event stream.
 	ping_event_stream: Box<dyn Stream<Item = PingEvent> + Send + Unpin>,
 
@@ -233,6 +236,7 @@ impl Discovery {
 	/// Enables `/ipfs/ping/1.0.0` and `/ipfs/identify/1.0.0` by default and starts
 	/// the mDNS peer discovery if it was enabled.
 	pub fn new<Hash: AsRef<[u8]> + Clone>(
+		local_peer_id: litep2p::PeerId,
 		config: &NetworkConfiguration,
 		genesis_hash: Hash,
 		fork_id: Option<&str>,
@@ -273,6 +277,7 @@ impl Discovery {
 
 		(
 			Self {
+				local_peer_id,
 				ping_event_stream,
 				identify_event_stream,
 				mdns_event_stream,
@@ -591,24 +596,43 @@ impl Stream for Discovery {
 				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,
-					});
-				}
+				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(),
-					});
+					if is_new {
+						this.pending_events.push_back(DiscoveryEvent::ExternalAddressDiscovered {
+							address: observed_address.clone(),
+						});
+					}
 				}
 
 				return Poll::Ready(Some(DiscoveryEvent::Identified {

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

@@ -540,6 +540,7 @@ impl<B: BlockT + 'static, H: ExHashT> NetworkBackend<B, H> for Litep2pNetworkBac
 		let listen_addresses = Arc::new(Default::default());
 		let (discovery, ping_config, identify_config, kademlia_config, maybe_mdns_config) =
 			Discovery::new(
+				local_peer_id,
 				&network_config,
 				params.genesis_hash,
 				params.fork_id.as_deref(),