From 99a29e985660444e8bd3121dd90a0fecf5571e08 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Wed, 9 Oct 2024 18:43:48 +0300 Subject: [PATCH 01/13] litep2p/discovery: Replace oldest address with new address Signed-off-by: Alexandru Vasile --- .../client/network/src/litep2p/discovery.rs | 37 ++++++++++++++++--- 1 file changed, 32 insertions(+), 5 deletions(-) diff --git a/substrate/client/network/src/litep2p/discovery.rs b/substrate/client/network/src/litep2p/discovery.rs index 5fe944cadc0b..70101033331f 100644 --- a/substrate/client/network/src/litep2p/discovery.rs +++ b/substrate/client/network/src/litep2p/discovery.rs @@ -423,7 +423,13 @@ impl Discovery { } /// Check if `address` can be considered a new external address. - fn is_new_external_address(&mut self, address: &Multiaddr, peer: PeerId) -> bool { + /// + /// If this address replaces an older address, the expired address is returned. + fn is_new_external_address( + &mut self, + address: &Multiaddr, + peer: PeerId, + ) -> (bool, Option) { log::trace!(target: LOG_TARGET, "verify new external address: {address}"); // is the address one of our known addresses @@ -434,7 +440,7 @@ impl Discovery { .chain(self.public_addresses.iter()) .any(|known_address| Discovery::is_known_address(&known_address, &address)) { - return true + return (true, None) } match self.address_confirmations.get(address) { @@ -442,15 +448,22 @@ impl Discovery { confirmations.insert(peer); if confirmations.len() >= MIN_ADDRESS_CONFIRMATIONS { - return true + return (true, None) } }, None => { + let oldest = (self.address_confirmations.len() >= + self.address_confirmations.limiter().max_length() as usize) + .then(|| self.address_confirmations.pop_oldest().map(|(address, _)| address)) + .flatten(); + self.address_confirmations.insert(address.clone(), Default::default()); + + return (false, oldest) }, } - false + (false, None) } } @@ -557,7 +570,21 @@ impl Stream for Discovery { observed_address, .. })) => { - if this.is_new_external_address(&observed_address, peer) { + let (is_new, expired_address) = + this.is_new_external_address(&observed_address, peer.clone()); + + if let Some(expired_address) = expired_address { + log::trace!( + target: LOG_TARGET, + "replacing expired external address {expired_address} with {observed_address}", + ); + + this.pending_events.push_back(DiscoveryEvent::ExternalAddressDiscovered { + address: expired_address, + }); + } + + if is_new { this.pending_events.push_back(DiscoveryEvent::ExternalAddressDiscovered { address: observed_address.clone(), }); From 06b7c8e04ad32629683dec3d6fd4721c4d41beb3 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Wed, 9 Oct 2024 18:52:31 +0300 Subject: [PATCH 02/13] litep2p/discovery: Handle expired addresses and remove from litep2p obj Signed-off-by: Alexandru Vasile --- .../client/network/src/litep2p/discovery.rs | 13 ++++++++++-- substrate/client/network/src/litep2p/mod.rs | 20 +++++++++++++++++++ 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/substrate/client/network/src/litep2p/discovery.rs b/substrate/client/network/src/litep2p/discovery.rs index 70101033331f..75e0de8f1258 100644 --- a/substrate/client/network/src/litep2p/discovery.rs +++ b/substrate/client/network/src/litep2p/discovery.rs @@ -116,7 +116,16 @@ pub enum DiscoveryEvent { /// New external address discovered. ExternalAddressDiscovered { - /// Discovered addresses. + /// Discovered address. + address: Multiaddr, + }, + + /// The external address has expired. + /// + /// This happens when the internal buffers exceed the maximum number of external addresses, + /// and this address is the oldest one. + ExternalAddressExpired { + /// Expired addresse. address: Multiaddr, }, @@ -579,7 +588,7 @@ impl Stream for Discovery { "replacing expired external address {expired_address} with {observed_address}", ); - this.pending_events.push_back(DiscoveryEvent::ExternalAddressDiscovered { + this.pending_events.push_back(DiscoveryEvent::ExternalAddressExpired { address: expired_address, }); } diff --git a/substrate/client/network/src/litep2p/mod.rs b/substrate/client/network/src/litep2p/mod.rs index 277f0759729c..5100686aa03d 100644 --- a/substrate/client/network/src/litep2p/mod.rs +++ b/substrate/client/network/src/litep2p/mod.rs @@ -935,6 +935,26 @@ impl NetworkBackend for Litep2pNetworkBac }, } } + Some(DiscoveryEvent::ExternalAddressExpired{ address }) => { + let local_peer_id = self.litep2p.local_peer_id().clone(); + + if let Some(multihash) = litep2p::types::multihash::Multihash::from_bytes(&local_peer_id.to_bytes()).ok() { + let address = if !std::matches!(address.iter().last(), Some(Protocol::P2p(_))) { + address.with(Protocol::P2p(multihash)) + } else { + address + }; + + if self.litep2p.public_addresses().remove_address(&address) { + log::info!(target: LOG_TARGET, "πŸ” Expired external address for our node: {address}"); + } else { + log::warn!( + target: LOG_TARGET, + "πŸ” Failed to remove expired external address {address:?}" + ); + } + } + } Some(DiscoveryEvent::Ping { peer, rtt }) => { log::trace!( target: LOG_TARGET, From b5d1ccc326480a76d31511d99f5ce4bb96992c1d Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Wed, 9 Oct 2024 18:54:43 +0300 Subject: [PATCH 03/13] litep2p/discovery: Adjust indentation Signed-off-by: Alexandru Vasile --- substrate/client/network/src/litep2p/mod.rs | 31 +++++++++++---------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/substrate/client/network/src/litep2p/mod.rs b/substrate/client/network/src/litep2p/mod.rs index 5100686aa03d..cef99bd3d684 100644 --- a/substrate/client/network/src/litep2p/mod.rs +++ b/substrate/client/network/src/litep2p/mod.rs @@ -938,21 +938,24 @@ impl NetworkBackend for Litep2pNetworkBac Some(DiscoveryEvent::ExternalAddressExpired{ address }) => { let local_peer_id = self.litep2p.local_peer_id().clone(); - if let Some(multihash) = litep2p::types::multihash::Multihash::from_bytes(&local_peer_id.to_bytes()).ok() { - let address = if !std::matches!(address.iter().last(), Some(Protocol::P2p(_))) { - address.with(Protocol::P2p(multihash)) - } else { - address - }; + // Litep2p requires the peer ID to be present in the address. + let Some(multihash) = litep2p::types::multihash::Multihash::from_bytes(&local_peer_id.to_bytes()).ok() else { + continue + }; - if self.litep2p.public_addresses().remove_address(&address) { - log::info!(target: LOG_TARGET, "πŸ” Expired external address for our node: {address}"); - } else { - log::warn!( - target: LOG_TARGET, - "πŸ” Failed to remove expired external address {address:?}" - ); - } + let address = if !std::matches!(address.iter().last(), Some(Protocol::P2p(_))) { + address.with(Protocol::P2p(multihash)) + } else { + address + }; + + if self.litep2p.public_addresses().remove_address(&address) { + log::info!(target: LOG_TARGET, "πŸ” Expired external address for our node: {address}"); + } else { + log::warn!( + target: LOG_TARGET, + "πŸ” Failed to remove expired external address {address:?}" + ); } } Some(DiscoveryEvent::Ping { peer, rtt }) => { From 967195df4a5912466a0f8965301c46e452d86f7e Mon Sep 17 00:00:00 2001 From: Alexandru Vasile <60601340+lexnv@users.noreply.github.com> Date: Thu, 10 Oct 2024 11:48:24 +0300 Subject: [PATCH 04/13] Update substrate/client/network/src/litep2p/discovery.rs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bastian KΓΆcher --- 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 75e0de8f1258..e75ddd58e233 100644 --- a/substrate/client/network/src/litep2p/discovery.rs +++ b/substrate/client/network/src/litep2p/discovery.rs @@ -125,7 +125,7 @@ pub enum DiscoveryEvent { /// This happens when the internal buffers exceed the maximum number of external addresses, /// and this address is the oldest one. ExternalAddressExpired { - /// Expired addresse. + /// Expired address. address: Multiaddr, }, From ce4ea3f2ca6440a9adbfc795a4a261c82304c999 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Thu, 10 Oct 2024 15:04:57 +0300 Subject: [PATCH 05/13] Add prdoc Signed-off-by: Alexandru Vasile --- prdoc/pr_5998.prdoc | 15 +++++++++++++++ 1 file changed, 15 insertions(+) create mode 100644 prdoc/pr_5998.prdoc diff --git a/prdoc/pr_5998.prdoc b/prdoc/pr_5998.prdoc new file mode 100644 index 000000000000..d8846c2c4730 --- /dev/null +++ b/prdoc/pr_5998.prdoc @@ -0,0 +1,15 @@ +# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0 +# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json + +title: Fix memory leak in litep2p public addresses + +doc: + - audience: [ Node Dev, Node Operator ] + description: | + This PR bounds the number of public addresses of litep2p to 32 entries. + This ensures we do not increase the number of addresses over time, and that the DHT + authority records will not exceed the upper size limit. + +crates: + - name: sc-network + bump: patch \ No newline at end of file From f4d4ca79fc2d491d2e341dd4bb56f4abcc6d6cae Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Thu, 10 Oct 2024 15:10:23 +0300 Subject: [PATCH 06/13] litep2p/discovery: Enhance trace logs Signed-off-by: Alexandru Vasile --- 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 e75ddd58e233..a9f4ee37c4ca 100644 --- a/substrate/client/network/src/litep2p/discovery.rs +++ b/substrate/client/network/src/litep2p/discovery.rs @@ -585,7 +585,7 @@ impl Stream for Discovery { if let Some(expired_address) = expired_address { log::trace!( target: LOG_TARGET, - "replacing expired external address {expired_address} with {observed_address}", + "Removing expired external address expired={expired_address} is_new={is_new} observed={observed_address}", ); this.pending_events.push_back(DiscoveryEvent::ExternalAddressExpired { From c4308a033e2a088728938a0588279c914faf8870 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 11 Oct 2024 08:43:40 +0000 Subject: [PATCH 07/13] litep2p/discovery: Fix clippy Signed-off-by: Alexandru Vasile --- substrate/client/network/src/litep2p/discovery.rs | 2 +- substrate/client/network/src/litep2p/mod.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/substrate/client/network/src/litep2p/discovery.rs b/substrate/client/network/src/litep2p/discovery.rs index a9f4ee37c4ca..d23f0a5ccb6b 100644 --- a/substrate/client/network/src/litep2p/discovery.rs +++ b/substrate/client/network/src/litep2p/discovery.rs @@ -580,7 +580,7 @@ impl Stream for Discovery { .. })) => { let (is_new, expired_address) = - this.is_new_external_address(&observed_address, peer.clone()); + this.is_new_external_address(&observed_address, peer); if let Some(expired_address) = expired_address { log::trace!( diff --git a/substrate/client/network/src/litep2p/mod.rs b/substrate/client/network/src/litep2p/mod.rs index cef99bd3d684..141d1d4f0eac 100644 --- a/substrate/client/network/src/litep2p/mod.rs +++ b/substrate/client/network/src/litep2p/mod.rs @@ -936,7 +936,7 @@ impl NetworkBackend for Litep2pNetworkBac } } Some(DiscoveryEvent::ExternalAddressExpired{ address }) => { - let local_peer_id = self.litep2p.local_peer_id().clone(); + let local_peer_id = self.litep2p.local_peer_id(); // Litep2p requires the peer ID to be present in the address. let Some(multihash) = litep2p::types::multihash::Multihash::from_bytes(&local_peer_id.to_bytes()).ok() else { From 15435335305516c24aa1fc58be72b9fa74cb8744 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile <60601340+lexnv@users.noreply.github.com> Date: Fri, 11 Oct 2024 14:56:51 +0300 Subject: [PATCH 08/13] Update substrate/client/network/src/litep2p/mod.rs Co-authored-by: Dmitry Markin --- substrate/client/network/src/litep2p/mod.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/substrate/client/network/src/litep2p/mod.rs b/substrate/client/network/src/litep2p/mod.rs index 141d1d4f0eac..7655f0d5a933 100644 --- a/substrate/client/network/src/litep2p/mod.rs +++ b/substrate/client/network/src/litep2p/mod.rs @@ -939,12 +939,8 @@ impl NetworkBackend for Litep2pNetworkBac let local_peer_id = self.litep2p.local_peer_id(); // Litep2p requires the peer ID to be present in the address. - let Some(multihash) = litep2p::types::multihash::Multihash::from_bytes(&local_peer_id.to_bytes()).ok() else { - continue - }; - let address = if !std::matches!(address.iter().last(), Some(Protocol::P2p(_))) { - address.with(Protocol::P2p(multihash)) + address.with(Protocol::P2p(local_peer_id.as_ref().clone())) } else { address }; From 65a34d37fe792fbf74a97ca978a2bf0cd3a92feb Mon Sep 17 00:00:00 2001 From: Alexandru Vasile <60601340+lexnv@users.noreply.github.com> Date: Fri, 11 Oct 2024 14:56:57 +0300 Subject: [PATCH 09/13] Update prdoc/pr_5998.prdoc Co-authored-by: Dmitry Markin --- prdoc/pr_5998.prdoc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/prdoc/pr_5998.prdoc b/prdoc/pr_5998.prdoc index d8846c2c4730..90590e9fa088 100644 --- a/prdoc/pr_5998.prdoc +++ b/prdoc/pr_5998.prdoc @@ -12,4 +12,5 @@ doc: crates: - name: sc-network - bump: patch \ No newline at end of file + bump: patch + \ No newline at end of file From ec241e6609661fe7341ba9bc0ba8b37836b4af98 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 11 Oct 2024 12:45:29 +0000 Subject: [PATCH 10/13] litep2p Fix clippy Signed-off-by: Alexandru Vasile --- substrate/client/network/src/litep2p/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/client/network/src/litep2p/mod.rs b/substrate/client/network/src/litep2p/mod.rs index 7655f0d5a933..e03ef18a7f8b 100644 --- a/substrate/client/network/src/litep2p/mod.rs +++ b/substrate/client/network/src/litep2p/mod.rs @@ -940,7 +940,7 @@ impl NetworkBackend for Litep2pNetworkBac // Litep2p requires the peer ID to be present in the address. let address = if !std::matches!(address.iter().last(), Some(Protocol::P2p(_))) { - address.with(Protocol::P2p(local_peer_id.as_ref().clone())) + address.with(Protocol::P2p(local_peer_id.as_ref())) } else { address }; From b484bc72b38d6845c3d0a395142d55e861a65e48 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 11 Oct 2024 12:46:32 +0000 Subject: [PATCH 11/13] Fix empty line Signed-off-by: Alexandru Vasile --- prdoc/pr_5998.prdoc | 1 - 1 file changed, 1 deletion(-) diff --git a/prdoc/pr_5998.prdoc b/prdoc/pr_5998.prdoc index 90590e9fa088..e3279051ca6a 100644 --- a/prdoc/pr_5998.prdoc +++ b/prdoc/pr_5998.prdoc @@ -13,4 +13,3 @@ doc: crates: - name: sc-network bump: patch - \ No newline at end of file From 7365773d7a8b8d3dee0814865f607f46d348ae93 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 15 Oct 2024 18:55:46 +0300 Subject: [PATCH 12/13] litep2p/discovery: Do not expire addresses not confirmed yet Signed-off-by: Alexandru Vasile --- substrate/client/network/src/litep2p/discovery.rs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/substrate/client/network/src/litep2p/discovery.rs b/substrate/client/network/src/litep2p/discovery.rs index d23f0a5ccb6b..13cf8a4c6ee0 100644 --- a/substrate/client/network/src/litep2p/discovery.rs +++ b/substrate/client/network/src/litep2p/discovery.rs @@ -463,7 +463,16 @@ impl Discovery { None => { let oldest = (self.address_confirmations.len() >= self.address_confirmations.limiter().max_length() as usize) - .then(|| self.address_confirmations.pop_oldest().map(|(address, _)| address)) + .then(|| { + self.address_confirmations.pop_oldest().map(|(address, peers)| { + if peers.len() >= MIN_ADDRESS_CONFIRMATIONS { + return Some(address) + } else { + None + } + }) + }) + .flatten() .flatten(); self.address_confirmations.insert(address.clone(), Default::default()); From f77498e84cd9e030448c7f74ec604cf9ca4be2ae Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 15 Oct 2024 18:56:23 +0300 Subject: [PATCH 13/13] litep2p/mod: Fix clippy Signed-off-by: Alexandru Vasile --- substrate/client/network/src/litep2p/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/client/network/src/litep2p/mod.rs b/substrate/client/network/src/litep2p/mod.rs index e03ef18a7f8b..df4244890f96 100644 --- a/substrate/client/network/src/litep2p/mod.rs +++ b/substrate/client/network/src/litep2p/mod.rs @@ -940,7 +940,7 @@ impl NetworkBackend for Litep2pNetworkBac // Litep2p requires the peer ID to be present in the address. let address = if !std::matches!(address.iter().last(), Some(Protocol::P2p(_))) { - address.with(Protocol::P2p(local_peer_id.as_ref())) + address.with(Protocol::P2p(*local_peer_id.as_ref())) } else { address };