From ecd6f1ded7d5a07f285d0818ecd269bba3ac55df Mon Sep 17 00:00:00 2001 From: Mykhailo Kremniov Date: Mon, 15 Jan 2024 11:28:15 +0200 Subject: [PATCH] p2p: add tests for discouragement; cleanup --- node-lib/src/options.rs | 3 +- p2p/p2p-test-utils/src/lib.rs | 2 +- p2p/src/ban_config.rs | 15 +- p2p/src/peer_manager/mod.rs | 30 +- p2p/src/peer_manager/peerdb/mod.rs | 12 +- p2p/src/peer_manager/peerdb/storage_impl.rs | 8 +- p2p/src/peer_manager/peerdb/tests.rs | 114 +++-- p2p/src/peer_manager/peers_eviction/mod.rs | 15 +- p2p/src/peer_manager/peers_eviction/tests.rs | 24 +- p2p/src/peer_manager/tests/ban.rs | 210 ++------ p2p/src/peer_manager/tests/connections.rs | 134 ++++- p2p/src/peer_manager/tests/discouragement.rs | 502 +++++++++++++++++++ p2p/src/peer_manager/tests/mod.rs | 2 + p2p/src/peer_manager/tests/utils.rs | 6 +- 14 files changed, 828 insertions(+), 249 deletions(-) create mode 100644 p2p/src/peer_manager/tests/discouragement.rs diff --git a/node-lib/src/options.rs b/node-lib/src/options.rs index 83569478fd..6844721865 100644 --- a/node-lib/src/options.rs +++ b/node-lib/src/options.rs @@ -136,7 +136,8 @@ pub struct RunOptions { #[clap(long)] pub p2p_max_inbound_connections: Option, - // 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, diff --git a/p2p/p2p-test-utils/src/lib.rs b/p2p/p2p-test-utils/src/lib.rs index e12b742ce8..9aab799d3f 100644 --- a/p2p/p2p-test-utils/src/lib.rs +++ b/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); diff --git a/p2p/src/ban_config.rs b/p2p/src/ban_config.rs index f952f078e3..e2a09652c6 100644 --- a/p2p/src/ban_config.rs +++ b/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, } diff --git a/p2p/src/peer_manager/mod.rs b/p2p/src/peer_manager/mod.rs index 68d9691d8e..c725493167 100644 --- a/p2p/src/peer_manager/mod.rs +++ b/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::>(); 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); diff --git a/p2p/src/peer_manager/peerdb/mod.rs b/p2p/src/peer_manager/peerdb/mod.rs index 585e71d22b..3c7926eaab 100644 --- a/p2p/src/peer_manager/peerdb/mod.rs +++ b/p2p/src/peer_manager/peerdb/mod.rs @@ -295,8 +295,7 @@ impl PeerDb { 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 PeerDb { 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,7 +578,7 @@ impl PeerDb { /// 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"); @@ -587,12 +586,11 @@ impl PeerDb { 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) } diff --git a/p2p/src/peer_manager/peerdb/storage_impl.rs b/p2p/src/peer_manager/peerdb/storage_impl.rs index 6ea634d8fa..c2e280f9c4 100644 --- a/p2p/src/peer_manager/peerdb/storage_impl.rs +++ b/p2p/src/peer_manager/peerdb/storage_impl.rs @@ -42,12 +42,12 @@ storage::decl_schema! { /// Table for known addresses pub DBKnownAddresses: Map, - /// 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, - /// 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, /// Table for anchor peers addresses diff --git a/p2p/src/peer_manager/peerdb/tests.rs b/p2p/src/peer_manager/peerdb/tests.rs index 74526eb4e5..7486962d03 100644 --- a/p2p/src/peer_manager/peerdb/tests.rs +++ b/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); diff --git a/p2p/src/peer_manager/peers_eviction/mod.rs b/p2p/src/peer_manager/peers_eviction/mod.rs index 8f2d095c01..5d029b4ad8 100644 --- a/p2p/src/peer_manager/peers_eviction/mod.rs +++ b/p2p/src/peer_manager/peers_eviction/mod.rs @@ -68,9 +68,8 @@ pub struct EvictionCandidate { /// expecting any blocks from it. expecting_blocks_since: Option