network: don't log re-discovered addresses (paritytech#6881)

* network: move LruHashSet to network crate utils

* network: don't log re-discovered external addresses

* Update client/network/src/utils.rs

Co-authored-by: mattrutherford <[email protected]>

Co-authored-by: mattrutherford <[email protected]>

client/network/src/discovery.rs

@@ -46,6 +46,7 @@
 //!
 
 use crate::config::ProtocolId;
+use crate::utils::LruHashSet;
 use futures::prelude::*;
 use futures_timer::Delay;
 use ip_network::IpNetwork;
@@ -63,10 +64,15 @@ use libp2p::swarm::toggle::Toggle;
 use libp2p::mdns::{Mdns, MdnsEvent};
 use libp2p::multiaddr::Protocol;
 use log::{debug, info, trace, warn};
-use std::{cmp, collections::{HashMap, HashSet, VecDeque}, io, time::Duration};
+use std::{cmp, collections::{HashMap, HashSet, VecDeque}, io, num::NonZeroUsize, time::Duration};
 use std::task::{Context, Poll};
 use sp_core::hexdisplay::HexDisplay;
 
+/// Maximum number of known external addresses that we will cache.
+/// This only affects whether we will log whenever we (re-)discover
+/// a given address.
+const MAX_KNOWN_EXTERNAL_ADDRESSES: usize = 32;
+
 /// `DiscoveryBehaviour` configuration.
 ///
 /// Note: In order to discover nodes or load and store values via Kademlia one has to add at least
@@ -190,7 +196,11 @@ impl DiscoveryConfig {
 			} else {
 				None.into()
 			},
-			allow_non_globals_in_dht: self.allow_non_globals_in_dht
+			allow_non_globals_in_dht: self.allow_non_globals_in_dht,
+			known_external_addresses: LruHashSet::new(
+				NonZeroUsize::new(MAX_KNOWN_EXTERNAL_ADDRESSES)
+					.expect("value is a constant; constant is non-zero; qed.")
+			),
 		}
 	}
 }
@@ -221,7 +231,9 @@ pub struct DiscoveryBehaviour {
 	/// Number of active connections over which we interrupt the discovery process.
 	discovery_only_if_under_num: u64,
 	/// Should non-global addresses be added to the DHT?
-	allow_non_globals_in_dht: bool
+	allow_non_globals_in_dht: bool,
+	/// A cache of discovered external addresses. Only used for logging purposes.
+	known_external_addresses: LruHashSet<Multiaddr>,
 }
 
 impl DiscoveryBehaviour {
@@ -507,7 +519,16 @@ impl NetworkBehaviour for DiscoveryBehaviour {
 	fn inject_new_external_addr(&mut self, addr: &Multiaddr) {
 		let new_addr = addr.clone()
 			.with(Protocol::P2p(self.local_peer_id.clone().into()));
-		info!(target: "sub-libp2p", "🔍 Discovered new external address for our node: {}", new_addr);
+
+		// 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()) {
+			info!(target: "sub-libp2p",
+				"🔍 Discovered new external address for our node: {}",
+				new_addr,
+			);
+		}
+
 		for k in self.kademlias.values_mut() {
 			NetworkBehaviour::inject_new_external_addr(k, addr)
 		}

client/network/src/protocol.rs

@@ -21,7 +21,7 @@ use crate::{
 	chain::{Client, FinalityProofProvider},
 	config::{BoxFinalityProofRequestBuilder, ProtocolId, TransactionPool, TransactionImportFuture, TransactionImport},
 	error,
-	utils::interval
+	utils::{interval, LruHashSet},
 };
 
 use bytes::{Bytes, BytesMut};
@@ -60,11 +60,9 @@ use std::fmt::Write;
 use std::{cmp, io, num::NonZeroUsize, pin::Pin, task::Poll, time};
 use log::{log, Level, trace, debug, warn, error};
 use sc_client_api::{ChangesProof, StorageProof};
-use util::LruHashSet;
 use wasm_timer::Instant;
 
 mod generic_proto;
-mod util;
 
 pub mod message;
 pub mod event;

client/network/src/utils.rs

@@ -14,12 +14,74 @@
 // You should have received a copy of the GNU General Public License
 // along with Substrate.  If not, see <http://www.gnu.org/licenses/>.
 
-use std::time::Duration;
-use futures::{FutureExt, Stream, StreamExt, stream::unfold};
+use futures::{stream::unfold, FutureExt, Stream, StreamExt};
 use futures_timer::Delay;
+use linked_hash_set::LinkedHashSet;
+use std::time::Duration;
+use std::{hash::Hash, num::NonZeroUsize};
+
+/// Creates a stream that returns a new value every `duration`.
+pub fn interval(duration: Duration) -> impl Stream<Item = ()> + Unpin {
+	unfold((), move |_| Delay::new(duration).map(|_| Some(((), ())))).map(drop)
+}
+
+/// Wrapper around `LinkedHashSet` with bounded growth.
+///
+/// In the limit, for each element inserted the oldest existing element will be removed.
+#[derive(Debug, Clone)]
+pub struct LruHashSet<T: Hash + Eq> {
+	set: LinkedHashSet<T>,
+	limit: NonZeroUsize,
+}
+
+impl<T: Hash + Eq> LruHashSet<T> {
+	/// Create a new `LruHashSet` with the given (exclusive) limit.
+	pub fn new(limit: NonZeroUsize) -> Self {
+		Self {
+			set: LinkedHashSet::new(),
+			limit,
+		}
+	}
+
+	/// Insert element into the set.
+	///
+	/// Returns `true` if this is a new element to the set, `false` otherwise.
+	/// Maintains the limit of the set by removing the oldest entry if necessary.
+	/// Inserting the same element will update its LRU position.
+	pub fn insert(&mut self, e: T) -> bool {
+		if self.set.insert(e) {
+			if self.set.len() == usize::from(self.limit) {
+				self.set.pop_front(); // remove oldest entry
+			}
+			return true;
+		}
+		false
+	}
+}
+
+#[cfg(test)]
+mod tests {
+	use super::*;
+
+	#[test]
+	fn maintains_limit() {
+		let three = NonZeroUsize::new(3).unwrap();
+		let mut set = LruHashSet::<u8>::new(three);
+
+		// First element.
+		assert!(set.insert(1));
+		assert_eq!(vec![&1], set.set.iter().collect::<Vec<_>>());
+
+		// Second element.
+		assert!(set.insert(2));
+		assert_eq!(vec![&1, &2], set.set.iter().collect::<Vec<_>>());
+
+		// Inserting the same element updates its LRU position.
+		assert!(!set.insert(1));
+		assert_eq!(vec![&2, &1], set.set.iter().collect::<Vec<_>>());
 
-pub fn interval(duration: Duration) -> impl Stream<Item=()> + Unpin {
-	unfold((), move |_| {
-		Delay::new(duration).map(|_| Some(((), ())))
-	}).map(drop)
+		// We reached the limit. The next element forces the oldest one out.
+		assert!(set.insert(3));
+		assert_eq!(vec![&1, &3], set.set.iter().collect::<Vec<_>>());
+	}
 }