chore: impl review comments

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;
 

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,25 +237,24 @@ 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);
         }
 
         if candidate_peers.is_empty() {
             let candidate_peers: Vec<_> = {
-                // find candidate peers by terrible condition
                 let mut peers_by_network_group: HashMap<Group, Vec<_>> = HashMap::default();
                 for addr in self.addr_manager.addrs_iter() {
                     peers_by_network_group

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)
         {

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();
         }

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);

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