From 322209f3ef7fb9d8c0d8b7264e984230a3f26ebd Mon Sep 17 00:00:00 2001 From: driftluo Date: Thu, 26 Aug 2021 10:47:45 +0800 Subject: [PATCH] chore: impl review comments --- network/src/peer_store/mod.rs | 3 ++ network/src/peer_store/peer_store_impl.rs | 34 +++++++++++------------ network/src/peer_store/types.rs | 4 +-- network/src/services/outbound_peer.rs | 4 +-- network/src/tests/peer_store.rs | 8 +++--- util/src/strings.rs | 2 +- 6 files changed, 29 insertions(+), 26 deletions(-) diff --git a/network/src/peer_store/mod.rs b/network/src/peer_store/mod.rs index 5d8b09536e4..a93541d3f26 100644 --- a/network/src/peer_store/mod.rs +++ b/network/src/peer_store/mod.rs @@ -24,6 +24,9 @@ pub(crate) const ADDR_COUNT_LIMIT: usize = 16384; const ADDR_TIMEOUT_MS: u64 = 7 * 24 * 3600 * 1000; /// The timeout that peer's address should be added to the feeler list again pub(crate) const ADDR_TRY_TIMEOUT_MS: u64 = 3 * 24 * 3600 * 1000; +/// When obtaining the list of selectable nodes for identify, +/// the node that has just been disconnected needs to be excluded +pub(crate) const DIAL_INTERVAL: u64 = 15 * 1000; const ADDR_MAX_RETRIES: u32 = 3; const ADDR_MAX_FAILURES: u32 = 10; diff --git a/network/src/peer_store/peer_store_impl.rs b/network/src/peer_store/peer_store_impl.rs index 2d4c61af3c8..924a2dc6fb0 100644 --- a/network/src/peer_store/peer_store_impl.rs +++ b/network/src/peer_store/peer_store_impl.rs @@ -7,7 +7,7 @@ use crate::{ ban_list::BanList, types::{ip_to_network, AddrInfo, BannedAddr, PeerInfo}, Behaviour, Multiaddr, PeerScoreConfig, ReportResult, Status, ADDR_COUNT_LIMIT, - ADDR_TIMEOUT_MS, ADDR_TRY_TIMEOUT_MS, + ADDR_TIMEOUT_MS, ADDR_TRY_TIMEOUT_MS, DIAL_INTERVAL, }, PeerId, SessionType, }; @@ -61,10 +61,10 @@ impl PeerStore { /// Add discovered peer address /// this method will assume peer and addr is untrust since we have not connected to it. pub fn add_addr(&mut self, addr: Multiaddr) -> Result<()> { - self.check_purge()?; if self.ban_list.is_addr_banned(&addr) { return Ok(()); } + self.check_purge()?; let score = self.score_config.default_score; self.addr_manager.add(AddrInfo::new(addr, 0, score)); Ok(()) @@ -136,8 +136,9 @@ impl PeerStore { extract_peer_id(&peer_addr.addr) .map(|peer_id| !peers.contains_key(&peer_id)) .unwrap_or_default() - && peer_addr - .connected(|t| t > addr_expired_ms && t <= now_ms.saturating_sub(15_000)) + && peer_addr.connected(|t| { + t > addr_expired_ms && t <= now_ms.saturating_sub(DIAL_INTERVAL) + }) }) } @@ -158,7 +159,7 @@ impl PeerStore { .map(|peer_id| !peers.contains_key(&peer_id)) .unwrap_or_default() && !peer_addr.tried_in_last_minute(now_ms) - && !peer_addr.connected(|t| t >= addr_expired_ms) + && !peer_addr.connected(|t| t > addr_expired_ms) }) } @@ -176,7 +177,7 @@ impl PeerStore { extract_peer_id(&peer_addr.addr) .map(|peer_id| peers.contains_key(&peer_id)) .unwrap_or_default() - || peer_addr.connected(|t| t >= addr_expired_ms) + || peer_addr.connected(|t| t > addr_expired_ms) }) } @@ -236,17 +237,17 @@ impl PeerStore { // 2.3. In the network segment with more than 4 peer, randomly evict 2 peer let now_ms = faketime::unix_time_as_millis(); - let candidate_peers: Vec<_> = { - // find candidate peers by terrible condition - let ban_score = self.score_config.ban_score; - let mut peers = Vec::default(); - for addr in self.addr_manager.addrs_iter() { - if !addr.is_connectable(now_ms) || addr.score <= ban_score { - peers.push(addr.addr.clone()) + let candidate_peers: Vec<_> = self + .addr_manager + .addrs_iter() + .filter_map(|addr| { + if !addr.is_connectable(now_ms) { + Some(addr.addr.clone()) + } else { + None } - } - peers - }; + }) + .collect(); for key in candidate_peers.iter() { self.addr_manager.remove(&key); @@ -254,7 +255,6 @@ impl PeerStore { if candidate_peers.is_empty() { let candidate_peers: Vec<_> = { - // find candidate peers by terrible condition let mut peers_by_network_group: HashMap> = HashMap::default(); for addr in self.addr_manager.addrs_iter() { peers_by_network_group diff --git a/network/src/peer_store/types.rs b/network/src/peer_store/types.rs index 924f70a045a..407c449e942 100644 --- a/network/src/peer_store/types.rs +++ b/network/src/peer_store/types.rs @@ -71,7 +71,7 @@ impl AddrInfo { self.last_tried_at_ms >= now_ms.saturating_sub(60_000) } - /// Whether terrible peer + /// Whether connectable peer pub fn is_connectable(&self, now_ms: u64) -> bool { // do not remove addr tried in last minute if self.tried_in_last_minute(now_ms) { @@ -81,7 +81,7 @@ impl AddrInfo { if self.last_connected_at_ms == 0 && self.attempts_count >= ADDR_MAX_RETRIES { return false; } - // consider addr is terrible if failed too many times + // consider addr is not connectable if failed too many times if now_ms.saturating_sub(self.last_connected_at_ms) > ADDR_TIMEOUT_MS && (self.attempts_count >= ADDR_MAX_FAILURES) { diff --git a/network/src/services/outbound_peer.rs b/network/src/services/outbound_peer.rs index 2a97494f782..d23d2aec033 100644 --- a/network/src/services/outbound_peer.rs +++ b/network/src/services/outbound_peer.rs @@ -139,12 +139,12 @@ impl Future for OutboundPeerService { } } while self.interval.as_mut().unwrap().poll_tick(cx).is_ready() { + // keep whitelist peer on connected + self.try_dial_whitelist(); // ensure feeler work at any time self.dial_feeler(); // keep outbound peer is enough self.try_dial_peers(); - // keep whitelist peer on connected - self.try_dial_whitelist(); // try dial observed addrs self.try_dial_observed(); } diff --git a/network/src/tests/peer_store.rs b/network/src/tests/peer_store.rs index e4b4efc12e6..a6443f54287 100644 --- a/network/src/tests/peer_store.rs +++ b/network/src/tests/peer_store.rs @@ -70,7 +70,7 @@ fn test_attempt_ban() { .mut_addr_manager() .get_mut(&addr) .unwrap() - .last_connected_at_ms = faketime::unix_time_as_millis(); + .mark_connected(faketime::unix_time_as_millis()); faketime::write_millis(&faketime_file, 100_000).expect("write millis"); @@ -95,7 +95,7 @@ fn test_fetch_addrs_to_attempt() { .mut_addr_manager() .get_mut(&addr) .unwrap() - .last_connected_at_ms = faketime::unix_time_as_millis(); + .mark_connected(faketime::unix_time_as_millis()); faketime::write_millis(&faketime_file, 100_000).expect("write millis"); assert_eq!(peer_store.fetch_addrs_to_attempt(2).len(), 1); @@ -153,7 +153,7 @@ fn test_fetch_addrs_to_attempt_in_last_minutes() { .mut_addr_manager() .get_mut(&addr) .unwrap() - .last_connected_at_ms = now; + .mark_connected(now); faketime::write_millis(&faketime_file, 200_000).expect("write millis"); assert_eq!(peer_store.fetch_addrs_to_attempt(1).len(), 1); @@ -224,7 +224,7 @@ fn test_fetch_random_addrs() { .mut_addr_manager() .get_mut(&addr3) .unwrap() - .last_connected_at_ms = 0; + .mark_connected(0); assert_eq!(peer_store.fetch_random_addrs(3).len(), 3); peer_store.remove_disconnected_peer(&addr3); assert_eq!(peer_store.fetch_random_addrs(3).len(), 2); diff --git a/util/src/strings.rs b/util/src/strings.rs index be51674a621..21cc9919c67 100644 --- a/util/src/strings.rs +++ b/util/src/strings.rs @@ -23,7 +23,7 @@ pub fn check_if_identifier_is_valid(ident: &str) -> Result<(), String> { } match Regex::new(IDENT_PATTERN) { Ok(re) => { - if !re.is_match(&ident) { + if !re.is_match(ident) { return Err(format!( "invaild identifier \"{}\", the identifier pattern is \"{}\"", ident, IDENT_PATTERN