From 2e2e24098cec7b1c18d3a8057ba39ec18346e519 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Wed, 6 Nov 2024 10:42:06 +0200 Subject: [PATCH 1/6] libp2p/discovery: Do not propagate external addr with different peerIDs Signed-off-by: Alexandru Vasile --- substrate/client/network/src/discovery.rs | 24 +++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/substrate/client/network/src/discovery.rs b/substrate/client/network/src/discovery.rs index 49e0797c126c..532be62bcaa2 100644 --- a/substrate/client/network/src/discovery.rs +++ b/substrate/client/network/src/discovery.rs @@ -74,7 +74,7 @@ use libp2p::{ PeerId, }; use linked_hash_set::LinkedHashSet; -use log::{debug, info, trace, warn}; +use log::{debug, error, info, trace, warn}; use sp_core::hexdisplay::HexDisplay; use std::{ cmp, @@ -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 { + error!( + 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}", ); } } From b65c042d8cb82bfa9ac439f11828d3e4575aaa48 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Wed, 6 Nov 2024 10:58:06 +0200 Subject: [PATCH 2/6] litep2p/discovery: Do not propagate external addr with different peerIDs Signed-off-by: Alexandru Vasile --- .../client/network/src/litep2p/discovery.rs | 58 +++++++++++++------ substrate/client/network/src/litep2p/mod.rs | 1 + 2 files changed, 42 insertions(+), 17 deletions(-) diff --git a/substrate/client/network/src/litep2p/discovery.rs b/substrate/client/network/src/litep2p/discovery.rs index 7b2e713dffd2..12c0b9bff319 100644 --- a/substrate/client/network/src/litep2p/discovery.rs +++ b/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 + 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 + 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::error!( + 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 { diff --git a/substrate/client/network/src/litep2p/mod.rs b/substrate/client/network/src/litep2p/mod.rs index df4244890f96..87b992423674 100644 --- a/substrate/client/network/src/litep2p/mod.rs +++ b/substrate/client/network/src/litep2p/mod.rs @@ -540,6 +540,7 @@ impl NetworkBackend 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(), From d3b4206f52d102b2069f6416f974f03af5de31ff Mon Sep 17 00:00:00 2001 From: Alexandru Vasile <60601340+lexnv@users.noreply.github.com> Date: Wed, 6 Nov 2024 12:30:26 +0200 Subject: [PATCH 3/6] Update substrate/client/network/src/discovery.rs Co-authored-by: Dmitry Markin --- substrate/client/network/src/discovery.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/client/network/src/discovery.rs b/substrate/client/network/src/discovery.rs index 532be62bcaa2..2e49e4cfb04d 100644 --- a/substrate/client/network/src/discovery.rs +++ b/substrate/client/network/src/discovery.rs @@ -753,7 +753,7 @@ impl NetworkBehaviour for DiscoveryBehaviour { if let Some(Protocol::P2p(peer_id)) = addr.iter().last() { if peer_id != self.local_peer_id { - error!( + warn!( target: "sub-libp2p", "🔍 Discovered external address for a peer that is not us: {addr}", ); From 867f9bbcaeeae3a87e08eb0008ae4462d6c012d2 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile <60601340+lexnv@users.noreply.github.com> Date: Wed, 6 Nov 2024 12:30:30 +0200 Subject: [PATCH 4/6] Update substrate/client/network/src/litep2p/discovery.rs Co-authored-by: Dmitry Markin --- substrate/client/network/src/litep2p/discovery.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/client/network/src/litep2p/discovery.rs b/substrate/client/network/src/litep2p/discovery.rs index 12c0b9bff319..9043f9420e8d 100644 --- a/substrate/client/network/src/litep2p/discovery.rs +++ b/substrate/client/network/src/litep2p/discovery.rs @@ -599,7 +599,7 @@ impl Stream for Discovery { let observed_address = if let Some(Protocol::P2p(peer_id)) = observed_address.iter().last() { if peer_id != *this.local_peer_id.as_ref() { - log::error!( + log::warn!( target: LOG_TARGET, "Discovered external address for a peer that is not us: {observed_address}", ); From e50d295a10d31526b77edaa421eafdfe2bdc9f14 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile <60601340+lexnv@users.noreply.github.com> Date: Wed, 6 Nov 2024 12:30:39 +0200 Subject: [PATCH 5/6] Update substrate/client/network/src/discovery.rs Co-authored-by: Dmitry Markin --- substrate/client/network/src/discovery.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/client/network/src/discovery.rs b/substrate/client/network/src/discovery.rs index 2e49e4cfb04d..8080bda9a574 100644 --- a/substrate/client/network/src/discovery.rs +++ b/substrate/client/network/src/discovery.rs @@ -74,7 +74,7 @@ use libp2p::{ PeerId, }; use linked_hash_set::LinkedHashSet; -use log::{debug, error, info, trace, warn}; +use log::{debug, info, trace, warn}; use sp_core::hexdisplay::HexDisplay; use std::{ cmp, From 5380433dec621cd80b3469df416d7c2a743a7020 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Wed, 6 Nov 2024 12:33:17 +0200 Subject: [PATCH 6/6] Add prdoc Signed-off-by: Alexandru Vasile --- prdoc/pr_6380.prdoc | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 prdoc/pr_6380.prdoc diff --git a/prdoc/pr_6380.prdoc b/prdoc/pr_6380.prdoc new file mode 100644 index 000000000000..72853bcf230c --- /dev/null +++ b/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