diff --git a/prdoc/pr_5998.prdoc b/prdoc/pr_5998.prdoc new file mode 100644 index 000000000000..e3279051ca6a --- /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 diff --git a/substrate/client/network/src/litep2p/discovery.rs b/substrate/client/network/src/litep2p/discovery.rs index 6ff05e6af327..14268c4f897f 100644 --- a/substrate/client/network/src/litep2p/discovery.rs +++ b/substrate/client/network/src/litep2p/discovery.rs @@ -115,7 +115,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 address. address: Multiaddr, }, @@ -405,7 +414,17 @@ impl Discovery { } /// Check if `address` can be considered a new external address. +<<<<<<< HEAD fn is_new_external_address(&mut self, address: &Multiaddr) -> 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) { +>>>>>>> 38aa4b7 (litep2p/discovery: Fix memory leak in `litep2p.public_addresses()` (#5998)) log::trace!(target: LOG_TARGET, "verify new external address: {address}"); // is the address one of our known addresses @@ -416,23 +435,48 @@ 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) { Some(confirmations) => { *confirmations += 1usize; +<<<<<<< HEAD if *confirmations >= MIN_ADDRESS_CONFIRMATIONS { return true } }, None => { self.address_confirmations.insert(address.clone(), 1usize); +======= + if confirmations.len() >= MIN_ADDRESS_CONFIRMATIONS { + 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, peers)| { + if peers.len() >= MIN_ADDRESS_CONFIRMATIONS { + return Some(address) + } else { + None + } + }) + }) + .flatten() + .flatten(); + + self.address_confirmations.insert(address.clone(), Default::default()); + + return (false, oldest) +>>>>>>> 38aa4b7 (litep2p/discovery: Fix memory leak in `litep2p.public_addresses()` (#5998)) }, } - false + (false, None) } } @@ -540,7 +584,25 @@ impl Stream for Discovery { supported_protocols, observed_address, })) => { +<<<<<<< HEAD if this.is_new_external_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 { +>>>>>>> 38aa4b7 (litep2p/discovery: Fix memory leak in `litep2p.public_addresses()` (#5998)) this.pending_events.push_back(DiscoveryEvent::ExternalAddressDiscovered { address: observed_address.clone(), }); diff --git a/substrate/client/network/src/litep2p/mod.rs b/substrate/client/network/src/litep2p/mod.rs index 34ca5b716101..dfa38df65cb1 100644 --- a/substrate/client/network/src/litep2p/mod.rs +++ b/substrate/client/network/src/litep2p/mod.rs @@ -918,6 +918,25 @@ impl NetworkBackend for Litep2pNetworkBac log::info!(target: LOG_TARGET, "discovered new external address for our node: {address}"); } } + Some(DiscoveryEvent::ExternalAddressExpired{ address }) => { + let local_peer_id = self.litep2p.local_peer_id(); + + // 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())) + } 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,