From 8993a755c764627e1568e7fabb4013936e81b872 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= <123550+andresilva@users.noreply.github.com> Date: Thu, 13 Aug 2020 19:38:14 +0100 Subject: [PATCH] network: don't log re-discovered addresses (#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 <44339188+mattrutherford@users.noreply.github.com> Co-authored-by: mattrutherford <44339188+mattrutherford@users.noreply.github.com> --- client/network/src/discovery.rs | 29 +++++++++-- client/network/src/protocol.rs | 4 +- client/network/src/protocol/util.rs | 76 ----------------------------- client/network/src/utils.rs | 74 +++++++++++++++++++++++++--- 4 files changed, 94 insertions(+), 89 deletions(-) delete mode 100644 client/network/src/protocol/util.rs diff --git a/client/network/src/discovery.rs b/client/network/src/discovery.rs index 7cb977e8e1ab3..e349b08c41dc3 100644 --- a/client/network/src/discovery.rs +++ b/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, } 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) } diff --git a/client/network/src/protocol.rs b/client/network/src/protocol.rs index f1ce6d2b5608e..c1c9ef02ea60b 100644 --- a/client/network/src/protocol.rs +++ b/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; diff --git a/client/network/src/protocol/util.rs b/client/network/src/protocol/util.rs deleted file mode 100644 index 9ba9bf6ae89c1..0000000000000 --- a/client/network/src/protocol/util.rs +++ /dev/null @@ -1,76 +0,0 @@ -// Copyright 2019-2020 Parity Technologies (UK) Ltd. -// This file is part of Substrate. - -// Substrate is free software: you can redistribute it and/or modify -// it under the terms of the GNU General Public License as published by -// the Free Software Foundation, either version 3 of the License, or -// (at your option) any later version. - -// Substrate is distributed in the hope that it will be useful, -// but WITHOUT ANY WARRANTY; without even the implied warranty of -// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -// GNU General Public License for more details. - -// You should have received a copy of the GNU General Public License -// along with Substrate. If not, see . - -use linked_hash_set::LinkedHashSet; -use std::{hash::Hash, num::NonZeroUsize}; - -/// Wrapper around `LinkedHashSet` which grows bounded. -/// -/// In the limit, for each element inserted the oldest existing element will be removed. -#[derive(Debug, Clone)] -pub(crate) struct LruHashSet { - set: LinkedHashSet, - limit: NonZeroUsize -} - -impl LruHashSet { - /// Create a new `LruHashSet` with the given (exclusive) limit. - pub(crate) 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(crate) 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::::new(three); - - // First element. - assert!(set.insert(1)); - assert_eq!(vec![&1], set.set.iter().collect::>()); - - // Second element. - assert!(set.insert(2)); - assert_eq!(vec![&1, &2], set.set.iter().collect::>()); - - // Inserting the same element updates its LRU position. - assert!(!set.insert(1)); - assert_eq!(vec![&2, &1], set.set.iter().collect::>()); - - // 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::>()); - } -} diff --git a/client/network/src/utils.rs b/client/network/src/utils.rs index f13505d0124db..490e2ced38266 100644 --- a/client/network/src/utils.rs +++ b/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 . -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 + 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 { + set: LinkedHashSet, + limit: NonZeroUsize, +} + +impl LruHashSet { + /// 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::::new(three); + + // First element. + assert!(set.insert(1)); + assert_eq!(vec![&1], set.set.iter().collect::>()); + + // Second element. + assert!(set.insert(2)); + assert_eq!(vec![&1, &2], set.set.iter().collect::>()); + + // Inserting the same element updates its LRU position. + assert!(!set.insert(1)); + assert_eq!(vec![&2, &1], set.set.iter().collect::>()); -pub fn interval(duration: Duration) -> impl Stream + 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::>()); + } }