p2p: add tests for discouragement; cleanup

node-lib/src/options.rs

@@ -136,7 +136,8 @@ pub struct RunOptions {
     #[clap(long)]
     pub p2p_max_inbound_connections: Option<usize>,
 
-    // FIXME: ban duration? same options for discouragement?
+    // TODO: add all the options related to banning/discouragement thresholds and durations,
+    // for completeness.
     /// The p2p score threshold after which a peer is banned.
     #[clap(long)]
     pub p2p_ban_threshold: Option<u32>,

p2p/p2p-test-utils/src/lib.rs

@@ -142,7 +142,7 @@ impl P2pBasicTestTimeGetter {
 }
 
 /// A timeout for blocking calls.
-pub const LONG_TIMEOUT: Duration = Duration::from_secs(30); // FIXME
+pub const LONG_TIMEOUT: Duration = Duration::from_secs(600);
 /// A short timeout for events that shouldn't occur.
 pub const SHORT_TIMEOUT: Duration = Duration::from_millis(500);
 

p2p/src/ban_config.rs

@@ -13,8 +13,6 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-// FIXME: rename to AutoBan? Do we need auto banning? Should manual banning be indefinite or use a separate duration setting?
-
 use std::time::Duration;
 
 use utils::make_config_setting;
@@ -27,8 +25,13 @@ make_config_setting!(BanThreshold, u32, 100);
 // to network splitting; e.g. if in a future version some previously incorrect behavior becomes
 // valid, the network may be split between old and new nodes, because the old ones would ban peers
 // that exhibit such behavior. Also, 30 min doesn't make much sense for manual banning. So, we
-// should probably get rid of automatic banning and return the old duration (ideally though, the
-// manual ban duration should be passed explicitly by the user via rpc).
+// should probably get rid of automatic banning and return the old duration for the manual banning.
+// But if we decide to keep the auto-ban functionality:
+// a) the config names should be changed to AutoBanDuration, AutoBanThreshold;
+// b) manual banning should have a separate ManualBanDuration; or event better, the ban duration
+// should be specified by the user via RPC and the config should become ManualBanDefaultDuration.
+// c) there should be the ability to turn auto-banning off, e.g. by setting the threshold to 0;
+// by default, auto banning should be turned off.
 make_config_setting!(BanDuration, Duration, Duration::from_secs(60 * 30));
 make_config_setting!(DiscouragementThreshold, u32, 100);
 make_config_setting!(
@@ -42,10 +45,10 @@ make_config_setting!(
 pub struct BanConfig {
     /// The score threshold after which a peer is banned.
     pub ban_threshold: BanThreshold,
-    /// Duration of bans in seconds.
+    /// The duration of a ban.
     pub ban_duration: BanDuration,
     /// The score threshold after which a peer becomes discouraged.
     pub discouragement_threshold: DiscouragementThreshold,
-    /// Duration of discouragements in seconds.
+    /// The duration of discouragement.
     pub discouragement_duration: DiscouragementDuration,
 }

p2p/src/peer_manager/mod.rs

@@ -357,9 +357,7 @@ where
     /// `peer_id` must be from the connected peer.
     fn announce_address(&mut self, peer_id: PeerId, address: SocketAddress) {
         let peer = self.peers.get_mut(&peer_id).expect("peer must be known");
-        if !peer.announced_addresses.contains(&address)
-            && !self.peerdb.is_address_banned_or_discouraged(&address.as_bannable())
-        {
+        if !peer.announced_addresses.contains(&address) {
             Self::send_peer_message(
                 &mut self.peer_connectivity_handle,
                 peer_id,
@@ -456,7 +454,7 @@ where
     /// Adjust peer score after a failed handshake.
     ///
     /// Note that currently intermediate scores are not stored in the peer db, so this call will
-    /// only make any effect if the passed score is bigger than the ban threshold.
+    /// only make any effect if the passed score is bigger than the threshold.
     fn adjust_peer_score_on_failed_handshake(&mut self, peer_address: SocketAddress, score: u32) {
         let whitelisted_node =
             self.pending_outbound_connects
@@ -503,7 +501,7 @@ where
             .collect::<Vec<_>>();
 
         log::info!(
-            "Banning address {:?}, the following peers will be disconnected: {:?}",
+            "Banning {:?}, the following peers will be disconnected: {:?}",
             address,
             to_disconnect
         );
@@ -1291,17 +1289,17 @@ where
 
             peer.announced_addresses.insert(&address, &mut make_pseudo_rng());
 
-            // FIXME: check for ban etc here
-
             self.peerdb.peer_discovered(address);
 
-            let peer_ids = self
-                .subscribed_to_peer_addresses
-                .iter()
-                .cloned()
-                .choose_multiple(&mut make_pseudo_rng(), PEER_ADDRESS_RESEND_COUNT);
-            for new_peer_id in peer_ids {
-                self.announce_address(new_peer_id, address);
+            if !self.peerdb.is_address_banned_or_discouraged(&address.as_bannable()) {
+                let peer_ids = self
+                    .subscribed_to_peer_addresses
+                    .iter()
+                    .cloned()
+                    .choose_multiple(&mut make_pseudo_rng(), PEER_ADDRESS_RESEND_COUNT);
+                for new_peer_id in peer_ids {
+                    self.announce_address(new_peer_id, address);
+                }
             }
         }
     }
@@ -1337,6 +1335,10 @@ where
                     })
                     .choose_multiple(&mut make_pseudo_rng(), max_addr_count)
             })
+            // Note: some of the addresses may have become banned or discouraged after they've been
+            // cached. It's not clear whether it's better to filter them out here, which will
+            // reveal to peers what addresses we've ban or discouraged, or keep them as is.
+            // But it's probably not that important.
             .clone();
 
         assert!(addresses.len() <= max_addr_count);

p2p/src/peer_manager/peerdb/mod.rs

@@ -295,8 +295,7 @@ impl<S: PeerDbStorage> PeerDb<S> {
                 Some(addr_data) => {
                     addr_data.connect_now(now)
                         && !addr_data.reserved()
-                        && !self.banned_addresses.contains_key(&addr.as_bannable())
-                        && !self.discouraged_addresses.contains_key(&addr.as_bannable())
+                        && !self.is_address_banned_or_discouraged(&addr.as_bannable())
                 }
                 None => {
                     debug_assert!(false, "Address {addr} not found in self.addresses");
@@ -560,14 +559,14 @@ impl<S: PeerDbStorage> PeerDb<S> {
         update_db(&self.storage, |tx| {
             tx.add_banned_address(&address, ban_till)
         })
-        .expect("adding banned address is expected to succeed (ban_peer)");
+        .expect("adding banned address is expected to succeed");
 
         self.banned_addresses.insert(address, ban_till);
     }
 
     pub fn unban(&mut self, address: &BannableAddress) {
         update_db(&self.storage, |tx| tx.del_banned_address(address))
-            .expect("adding banned address is expected to succeed (ban_peer)");
+            .expect("removing banned address is expected to succeed");
 
         self.banned_addresses.remove(address);
     }
@@ -579,20 +578,19 @@ impl<S: PeerDbStorage> PeerDb<S> {
 
     /// Changes the address state to discouraged
     pub fn discourage(&mut self, address: BannableAddress) {
-        // FIXME: prolong discouragement + tests
+        // TODO: do we need to prolong the discouragement if the peer continues misbehaving?
         let discourage_till = (self.time_getter.get_time()
             + *self.p2p_config.ban_config.discouragement_duration)
             .expect("Discouragement duration is expected to be valid");
 
         update_db(&self.storage, |tx| {
             tx.add_discouraged_address(&address, discourage_till)
         })
-        .expect("adding discouraged address is expected to succeed (ban_peer)");
+        .expect("adding discouraged address is expected to succeed");
 
         self.discouraged_addresses.insert(address, discourage_till);
     }
 
-    // FIXME: some "single" namer to use here and in the eviction candidate?
     pub fn is_address_banned_or_discouraged(&self, address: &BannableAddress) -> bool {
         self.is_address_banned(address) || self.is_address_discouraged(address)
     }

p2p/src/peer_manager/peerdb/storage_impl.rs

@@ -42,12 +42,12 @@ storage::decl_schema! {
         /// Table for known addresses
         pub DBKnownAddresses: Map<String, KnownAddressState>,
 
-        /// Table for banned addresses vs the time when they can be unbanned
-        /// (Duration is timestamp since UNIX Epoch)
+        /// Table for banned addresses vs the time when they should be unbanned
+        /// (Duration is a timestamp since UNIX Epoch)
         pub DBBannedAddresses: Map<String, Duration>,
 
-        /// Table for discouraged addresses vs the time when the discouragement must expire
-        /// (Duration is timestamp since UNIX Epoch)
+        /// Table for discouraged addresses vs the time when the discouragement should expire
+        /// (Duration is a timestamp since UNIX Epoch)
         pub DBDiscouragedAddresses: Map<String, Duration>,
 
         /// Table for anchor peers addresses

p2p/src/peer_manager/peerdb/tests.rs

@@ -24,15 +24,12 @@ use itertools::Itertools;
 use rstest::rstest;
 
 use ::test_utils::random::{make_seedable_rng, Seed};
-use common::{
-    chain::config::create_unit_test_config, primitives::user_agent::mintlayer_core_user_agent,
-};
+use common::chain::config::create_unit_test_config;
 use crypto::random::Rng;
 use p2p_test_utils::P2pBasicTestTimeGetter;
 
 use crate::{
     ban_config::BanConfig,
-    config::P2pConfig,
     peer_manager::{
         peerdb::{
             address_data::{self, PURGE_REACHABLE_FAIL_COUNT, PURGE_UNREACHABLE_TIME},
@@ -42,8 +39,8 @@ use crate::{
         peerdb_common::Transactional,
     },
     testing_utils::{
-        peerdb_inmemory_store, test_p2p_config, test_p2p_config_with_peer_db_config,
-        TestAddressMaker,
+        peerdb_inmemory_store, test_p2p_config, test_p2p_config_with_ban_config,
+        test_p2p_config_with_peer_db_config, TestAddressMaker,
     },
 };
 
@@ -65,33 +62,13 @@ fn unban_peer() {
     let chain_config = create_unit_test_config();
     let mut peerdb = PeerDb::<_>::new(
         &chain_config,
-        Arc::new(P2pConfig {
-            ban_config: BanConfig {
-                ban_duration: Duration::from_secs(60).into(),
-
-                ban_threshold: Default::default(),
-                discouragement_threshold: Default::default(),
-                discouragement_duration: Default::default(),
-            },
-
-            bind_addresses: Default::default(),
-            socks5_proxy: None,
-            disable_noise: Default::default(),
-            boot_nodes: Default::default(),
-            reserved_nodes: Default::default(),
-            whitelisted_addresses: Default::default(),
-            outbound_connection_timeout: Default::default(),
-            ping_check_period: Default::default(),
-            ping_timeout: Default::default(),
-            peer_handshake_timeout: Default::default(),
-            max_clock_diff: Default::default(),
-            node_type: Default::default(),
-            allow_discover_private_ips: Default::default(),
-            user_agent: mintlayer_core_user_agent(),
-            sync_stalling_timeout: Default::default(),
-            peer_manager_config: Default::default(),
-            protocol_config: Default::default(),
-        }),
+        Arc::new(test_p2p_config_with_ban_config(BanConfig {
+            ban_duration: Duration::from_secs(60).into(),
+            discouragement_duration: Duration::from_secs(600).into(),
+
+            ban_threshold: Default::default(),
+            discouragement_threshold: Default::default(),
+        })),
         time_getter.get_time_getter(),
         db_store,
     )
@@ -100,15 +77,84 @@ fn unban_peer() {
     let address = TestAddressMaker::new_random_address();
     peerdb.ban(address.as_bannable());
 
+    // The address is banned.
     assert!(peerdb.is_address_banned(&address.as_bannable()));
     let banned_addresses = peerdb.storage.transaction_ro().unwrap().get_banned_addresses().unwrap();
     assert_eq!(banned_addresses.len(), 1);
+    assert_eq!(banned_addresses[0].0, address.as_bannable());
+
+    // But not discouraged.
+    assert!(!peerdb.is_address_discouraged(&address.as_bannable()));
+    let discouraged_addresses =
+        peerdb.storage.transaction_ro().unwrap().get_discouraged_addresses().unwrap();
+    assert_eq!(discouraged_addresses.len(), 0);
+
+    time_getter.advance_time(Duration::from_secs(120));
+
+    // Banned addresses are updated in the `heartbeat` function
+    peerdb.heartbeat();
+
+    // The address is no longer banned.
+    assert!(!peerdb.is_address_banned(&address.as_bannable()));
+    let banned_addresses = peerdb.storage.transaction_ro().unwrap().get_banned_addresses().unwrap();
+    assert_eq!(banned_addresses.len(), 0);
+
+    // And still not discouraged.
+    assert!(!peerdb.is_address_discouraged(&address.as_bannable()));
+    let discouraged_addresses =
+        peerdb.storage.transaction_ro().unwrap().get_discouraged_addresses().unwrap();
+    assert_eq!(discouraged_addresses.len(), 0);
+
+    assert_addr_consistency(&peerdb);
+}
+
+#[tracing::instrument]
+#[test]
+fn undiscourage_peer() {
+    let db_store = peerdb_inmemory_store();
+    let time_getter = P2pBasicTestTimeGetter::new();
+    let chain_config = create_unit_test_config();
+    let mut peerdb = PeerDb::<_>::new(
+        &chain_config,
+        Arc::new(test_p2p_config_with_ban_config(BanConfig {
+            discouragement_duration: Duration::from_secs(60).into(),
+            ban_duration: Duration::from_secs(600).into(),
+
+            ban_threshold: Default::default(),
+            discouragement_threshold: Default::default(),
+        })),
+        time_getter.get_time_getter(),
+        db_store,
+    )
+    .unwrap();
+
+    let address = TestAddressMaker::new_random_address();
+    peerdb.discourage(address.as_bannable());
+
+    // The address is discouraged.
+    assert!(peerdb.is_address_discouraged(&address.as_bannable()));
+    let discouraged_addresses =
+        peerdb.storage.transaction_ro().unwrap().get_discouraged_addresses().unwrap();
+    assert_eq!(discouraged_addresses.len(), 1);
+    assert_eq!(discouraged_addresses[0].0, address.as_bannable());
+
+    // But not banned.
+    assert!(!peerdb.is_address_banned(&address.as_bannable()));
+    let banned_addresses = peerdb.storage.transaction_ro().unwrap().get_banned_addresses().unwrap();
+    assert_eq!(banned_addresses.len(), 0);
 
     time_getter.advance_time(Duration::from_secs(120));
 
-    // Banned addresses updated in the `heartbeat` function
+    // Discouraged addresses are updated in the `heartbeat` function
     peerdb.heartbeat();
 
+    // The address is no longer discouraged.
+    assert!(!peerdb.is_address_discouraged(&address.as_bannable()));
+    let discouraged_addresses =
+        peerdb.storage.transaction_ro().unwrap().get_discouraged_addresses().unwrap();
+    assert_eq!(discouraged_addresses.len(), 0);
+
+    // And still not banned.
     assert!(!peerdb.is_address_banned(&address.as_bannable()));
     let banned_addresses = peerdb.storage.transaction_ro().unwrap().get_banned_addresses().unwrap();
     assert_eq!(banned_addresses.len(), 0);

p2p/src/peer_manager/peers_eviction/mod.rs

@@ -68,9 +68,8 @@ pub struct EvictionCandidate {
     /// expecting any blocks from it.
     expecting_blocks_since: Option<Time>,
 
-    /// Whether the address is discouraged (in the general sense, banned addresses are included
-    /// too).
-    is_discouraged: bool,
+    /// Whether the address is banned or discouraged.
+    is_banned_or_discouraged: bool,
 }
 
 pub struct RandomState(u64, u64);
@@ -92,7 +91,7 @@ impl EvictionCandidate {
         peer: &PeerContext,
         random_state: &RandomState,
         now: Time,
-        is_discouraged: bool,
+        is_banned_or_discouraged: bool,
     ) -> Self {
         EvictionCandidate {
             age: now.saturating_sub(peer.created_at),
@@ -107,7 +106,7 @@ impl EvictionCandidate {
 
             expecting_blocks_since: peer.block_sync_status.expecting_blocks_since,
 
-            is_discouraged,
+            is_banned_or_discouraged,
         }
     }
 }
@@ -206,7 +205,8 @@ pub fn select_for_eviction_inbound(
         return None;
     }
 
-    if let Some(candidate) = candidates.iter().filter(|ec| ec.is_discouraged).choose(rng) {
+    if let Some(candidate) = candidates.iter().filter(|ec| ec.is_banned_or_discouraged).choose(rng)
+    {
         return Some(candidate.peer_id);
     }
 
@@ -306,7 +306,8 @@ fn select_for_eviction_outbound(
         return None;
     }
 
-    if let Some(candidate) = candidates.iter().filter(|ec| ec.is_discouraged).choose(rng) {
+    if let Some(candidate) = candidates.iter().filter(|ec| ec.is_banned_or_discouraged).choose(rng)
+    {
         return Some(candidate.peer_id);
     }