From bb8c7a3bd9b38ad89370d19cd68601598276942c Mon Sep 17 00:00:00 2001
From: Alexandru Vasile <60601340+lexnv@users.noreply.github.com>
Date: Thu, 7 Nov 2024 13:47:22 +0200
Subject: [PATCH] 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:
- https://github.com/paritytech/polkadot-sdk/pull/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 | 11 ++++
substrate/client/network/src/discovery.rs | 22 +++++--
.../client/network/src/litep2p/discovery.rs | 58 +++++++++++++------
substrate/client/network/src/litep2p/mod.rs | 1 +
4 files changed, 70 insertions(+), 22 deletions(-)
create mode 100644 prdoc/pr_6380.prdoc
diff --git a/prdoc/pr_6380.prdoc b/prdoc/pr_6380.prdoc
new file mode 100644
index 00000000000..72853bcf230
--- /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
diff --git a/substrate/client/network/src/discovery.rs b/substrate/client/network/src/discovery.rs
index 49e0797c126..8080bda9a57 100644
--- a/substrate/client/network/src/discovery.rs
+++ b/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}",
);
}
}
diff --git a/substrate/client/network/src/litep2p/discovery.rs b/substrate/client/network/src/litep2p/discovery.rs
index 7b2e713dffd..9043f9420e8 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<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 {
diff --git a/substrate/client/network/src/litep2p/mod.rs b/substrate/client/network/src/litep2p/mod.rs
index df4244890f9..87b99242367 100644
--- a/substrate/client/network/src/litep2p/mod.rs
+++ b/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(),
--
GitLab