p2p: peer discouragement was added; peer eviction tests were refactor…

…ed; ban tests were rewritten; PeermanagerConfig was moved to a separate file; some cleanup

dns-server/src/crawler_p2p/crawler/mod.rs

@@ -39,38 +39,30 @@ use common::{chain::ChainConfig, primitives::time::Time};
 use crypto::random::{seq::IteratorRandom, Rng};
 use logging::log;
 use p2p::{
-    config::{BanDuration, BanThreshold},
     error::P2pError,
     net::types::PeerInfo,
     peer_manager::{ADDR_RATE_BUCKET_SIZE, ADDR_RATE_INITIAL_SIZE, MAX_ADDR_RATE_PER_SECOND},
     types::{bannable_address::BannableAddress, peer_id::PeerId, socket_address::SocketAddress},
     utils::rate_limiter::RateLimiter,
 };
+use utils::make_config_setting;
 
 use crate::crawler_p2p::crawler::address_data::AddressStateTransitionTo;
 
 use self::address_data::{AddressData, AddressState};
 
 /// How many outbound connection attempts can be made per heartbeat
 const MAX_CONNECTS_PER_HEARTBEAT: usize = 25;
-const DEFAULT_BAN_THRESHOLD: u32 = 100;
-const DEFAULT_BAN_DURATION: Duration = Duration::from_secs(60 * 60 * 24);
 
-#[derive(Clone)]
+make_config_setting!(BanThreshold, u32, 100);
+make_config_setting!(BanDuration, Duration, Duration::from_secs(60 * 60 * 24));
+
+#[derive(Default, Clone)]
 pub struct CrawlerConfig {
     pub ban_threshold: BanThreshold,
     pub ban_duration: BanDuration,
 }
 
-impl Default for CrawlerConfig {
-    fn default() -> Self {
-        Self {
-            ban_threshold: BanThreshold::from(DEFAULT_BAN_THRESHOLD),
-            ban_duration: BanDuration::from(DEFAULT_BAN_DURATION),
-        }
-    }
-}
-
 /// The `Crawler` is the component that communicates with Mintlayer peers using p2p,
 /// and based on the results, commands the DNS server to add/remove ip addresses.
 /// The `Crawler` emits events that communicate whether addresses were reached or,

dns-server/src/crawler_p2p/crawler/tests/mod.rs

@@ -27,7 +27,7 @@ use common::{
     primitives::{time::Time, user_agent::mintlayer_core_user_agent},
 };
 use p2p::{
-    config::{BanDuration, BanThreshold, NodeType},
+    config::NodeType,
     error::{DialError, P2pError, ProtocolError},
     net::types::PeerInfo,
     testing_utils::TEST_PROTOCOL_VERSION,
@@ -312,8 +312,8 @@ fn ban_misbehaved_peer(#[case] seed: Seed) {
     let chain_config = common::chain::config::create_mainnet();
     let mut crawler = test_crawler(
         CrawlerConfig {
-            ban_duration: BanDuration::new(BAN_DURATION),
-            ban_threshold: BanThreshold::new(ban_threshold),
+            ban_duration: BAN_DURATION.into(),
+            ban_threshold: ban_threshold.into(),
         },
         BTreeSet::new(),
         BTreeMap::new(),
@@ -442,8 +442,8 @@ fn ban_misbehaved_peers_with_same_address(#[case] seed: Seed) {
     let chain_config = common::chain::config::create_mainnet();
     let mut crawler = test_crawler(
         CrawlerConfig {
-            ban_duration: BanDuration::new(BAN_DURATION),
-            ban_threshold: BanThreshold::new(ban_threshold),
+            ban_duration: BAN_DURATION.into(),
+            ban_threshold: ban_threshold.into(),
         },
         BTreeSet::new(),
         BTreeMap::new(),
@@ -589,8 +589,8 @@ fn ban_on_misbehavior_during_handshake(#[case] seed: Seed) {
     let chain_config = common::chain::config::create_mainnet();
     let mut crawler = test_crawler(
         CrawlerConfig {
-            ban_duration: BanDuration::new(BAN_DURATION),
-            ban_threshold: BanThreshold::new(ban_threshold),
+            ban_duration: BAN_DURATION.into(),
+            ban_threshold: ban_threshold.into(),
         },
         BTreeSet::new(),
         BTreeMap::new(),
@@ -665,8 +665,8 @@ fn no_ban_on_connection_error(#[case] seed: Seed) {
     let chain_config = common::chain::config::create_mainnet();
     let mut crawler = test_crawler(
         CrawlerConfig {
-            ban_duration: BanDuration::new(BAN_DURATION),
-            ban_threshold: BanThreshold::new(ban_threshold),
+            ban_duration: BAN_DURATION.into(),
+            ban_threshold: ban_threshold.into(),
         },
         BTreeSet::new(),
         BTreeMap::new(),
@@ -712,8 +712,8 @@ const BAN_THRESHOLD: u32 = 100;
 
 fn make_config() -> CrawlerConfig {
     CrawlerConfig {
-        ban_duration: BanDuration::new(BAN_DURATION),
-        ban_threshold: BanThreshold::new(BAN_THRESHOLD),
+        ban_duration: BAN_DURATION.into(),
+        ban_threshold: BAN_THRESHOLD.into(),
     }
 }
 

dns-server/src/crawler_p2p/crawler_manager/tests/mock_manager.rs

@@ -35,7 +35,7 @@ use common::{
     time_getter::TimeGetter,
 };
 use p2p::{
-    config::{BanDuration, BanThreshold, NodeType, P2pConfig},
+    config::{NodeType, P2pConfig},
     error::{DialError, P2pError},
     message::{AnnounceAddrRequest, PeerManagerMessage},
     net::{
@@ -295,8 +295,8 @@ pub fn test_crawler(
         default_p2p_port: 3031,
     };
     let crawler_config = CrawlerConfig {
-        ban_duration: BanDuration::default(),
-        ban_threshold: BanThreshold::default(),
+        ban_duration: Default::default(),
+        ban_threshold: Default::default(),
     };
 
     let state = MockStateRef {

dns-server/src/crawler_p2p/crawler_manager/tests/mod.rs

@@ -19,16 +19,18 @@ use std::time::Duration;
 
 use chainstate::ban_score::BanScore;
 use p2p::{
-    config::{BanDuration, BanThreshold},
     error::{P2pError, ProtocolError},
     types::socket_address::SocketAddress,
 };
 use p2p_test_utils::{expect_no_recv, expect_recv};
 
 use crate::{
-    crawler_p2p::crawler_manager::tests::mock_manager::{
-        advance_time, assert_banned_addresses, assert_known_addresses, test_crawler,
-        ErraticNodeConnectError,
+    crawler_p2p::{
+        crawler::{BanDuration, BanThreshold},
+        crawler_manager::tests::mock_manager::{
+            advance_time, assert_banned_addresses, assert_known_addresses, test_crawler,
+            ErraticNodeConnectError,
+        },
     },
     dns_server::DnsServerCommand,
 };

dns-server/src/main.rs

@@ -58,8 +58,9 @@ async fn run(config: Arc<DnsServerConfig>) -> Result<Never, error::DnsServerErro
         boot_nodes: Vec::new(),
         reserved_nodes: Vec::new(),
         whitelisted_addresses: Default::default(),
-        ban_threshold: Default::default(),
-        ban_duration: Default::default(),
+        // Note: this ban config (as well as any other non-backend setting here) won't have
+        // any effect on the dns server.
+        ban_config: Default::default(),
         outbound_connection_timeout: Default::default(),
         ping_check_period: Default::default(),
         ping_timeout: Default::default(),

node-lib/src/config_files/mod.rs

@@ -174,6 +174,8 @@ fn p2p_config(config: P2pConfigFile, options: &RunOptions) -> P2pConfigFile {
         max_inbound_connections,
         ban_threshold,
         ban_duration,
+        discouragement_threshold,
+        discouragement_duration,
         max_clock_diff,
         outbound_connection_timeout,
         ping_check_period,
@@ -212,6 +214,8 @@ fn p2p_config(config: P2pConfigFile, options: &RunOptions) -> P2pConfigFile {
         max_inbound_connections,
         ban_threshold,
         ban_duration,
+        discouragement_threshold,
+        discouragement_duration,
         max_clock_diff,
         outbound_connection_timeout,
         ping_check_period,

node-lib/src/config_files/p2p.rs

@@ -19,8 +19,9 @@ use common::primitives::user_agent::mintlayer_core_user_agent;
 use serde::{Deserialize, Serialize};
 
 use p2p::{
+    ban_config::BanConfig,
     config::{NodeType, P2pConfig},
-    peer_manager::PeerManagerConfig,
+    peer_manager::config::PeerManagerConfig,
     types::ip_or_socket_address::IpOrSocketAddress,
 };
 
@@ -76,6 +77,10 @@ pub struct P2pConfigFile {
     pub ban_threshold: Option<u32>,
     /// Duration of bans in seconds.
     pub ban_duration: Option<u64>,
+    /// The score threshold after which a peer becomes discouraged.
+    pub discouragement_threshold: Option<u32>,
+    /// Duration of discouragement in seconds.
+    pub discouragement_duration: Option<u64>,
     /// Maximum acceptable time difference between this node and the remote peer (in seconds).
     /// If a large difference is detected, the peer will be disconnected.
     pub max_clock_diff: Option<u64>,
@@ -106,6 +111,8 @@ impl From<P2pConfigFile> for P2pConfig {
             max_inbound_connections,
             ban_threshold,
             ban_duration,
+            discouragement_threshold,
+            discouragement_duration,
             max_clock_diff,
             outbound_connection_timeout,
             ping_check_period,
@@ -122,8 +129,12 @@ impl From<P2pConfigFile> for P2pConfig {
             boot_nodes: boot_nodes.unwrap_or_default(),
             reserved_nodes: reserved_nodes.unwrap_or_default(),
             whitelisted_addresses: whitelisted_addresses.unwrap_or_default(),
-            ban_threshold: ban_threshold.into(),
-            ban_duration: ban_duration.map(Duration::from_secs).into(),
+            ban_config: BanConfig {
+                ban_threshold: ban_threshold.into(),
+                ban_duration: ban_duration.map(Duration::from_secs).into(),
+                discouragement_threshold: discouragement_threshold.into(),
+                discouragement_duration: discouragement_duration.map(Duration::from_secs).into(),
+            },
             max_clock_diff: max_clock_diff.map(Duration::from_secs).into(),
             outbound_connection_timeout: outbound_connection_timeout
                 .map(|t| Duration::from_secs(t.into()))

node-lib/src/options.rs

@@ -136,7 +136,9 @@ pub struct RunOptions {
     #[clap(long)]
     pub p2p_max_inbound_connections: Option<usize>,
 
-    /// The p2p score threshold after which a peer is baned.
+    // 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/backend-test-suite/src/block_announcement.rs

@@ -179,8 +179,7 @@ where
         boot_nodes: Vec::new(),
         reserved_nodes: Vec::new(),
         whitelisted_addresses: Default::default(),
-        ban_threshold: Default::default(),
-        ban_duration: Default::default(),
+        ban_config: Default::default(),
         outbound_connection_timeout: Default::default(),
         ping_check_period: Default::default(),
         ping_timeout: Default::default(),

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

@@ -221,6 +221,7 @@ macro_rules! expect_no_recv {
     };
 }
 
+/// Wait until the specified value is received from the channel.
 pub async fn wait_for_recv<T: Eq>(receiver: &mut mpsc::UnboundedReceiver<T>, value: &T) {
     let wait_loop = async {
         loop {
@@ -232,3 +233,18 @@ pub async fn wait_for_recv<T: Eq>(receiver: &mut mpsc::UnboundedReceiver<T>, val
 
     expect_future_val!(wait_loop);
 }
+
+/// Wait until the sender stops putting messages into the channel.
+pub async fn wait_for_no_recv<T>(receiver: &mut mpsc::UnboundedReceiver<T>) {
+    let wait_loop = async {
+        loop {
+            let wait_result =
+                tokio::time::timeout(Duration::from_millis(100), receiver.recv()).await;
+            if wait_result.is_err() {
+                break;
+            }
+        }
+    };
+
+    expect_future_val!(wait_loop);
+}

p2p/src/ban_config.rs

@@ -0,0 +1,54 @@
+// Copyright (c) 2021-2024 RBB S.r.l
+// [email protected]
+// SPDX-License-Identifier: MIT
+// Licensed under the MIT License;
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// https://github.com/mintlayer/mintlayer-core/blob/master/LICENSE
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+use std::time::Duration;
+
+use utils::make_config_setting;
+
+make_config_setting!(BanThreshold, u32, 100);
+// TODO: initially, the ban duration was 24h, but later it was changed to 30 min because automatic
+// banning can be dangerous. E.g. it's possible for an attacker to force the node to use malicious
+// Tor exit nodes by making it ban all honest ones. Using a smaller interval seemed less dangerous,
+// so we chose 30 min. But even automatic disconnection of seemingly misbehaving peers may lead
+// 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 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!(
+    DiscouragementDuration,
+    Duration,
+    Duration::from_secs(60 * 60 * 24)
+);
+
+/// Settings related to banning and discouragement.
+#[derive(Default, Debug, Clone)]
+pub struct BanConfig {
+    /// The score threshold after which a peer is banned.
+    pub ban_threshold: BanThreshold,
+    /// The duration of a ban.
+    pub ban_duration: BanDuration,
+    /// The score threshold after which a peer becomes discouraged.
+    pub discouragement_threshold: DiscouragementThreshold,
+    /// The duration of discouragement.
+    pub discouragement_duration: DiscouragementDuration,
+}

p2p/src/config.rs

@@ -21,15 +21,12 @@ use p2p_types::ip_or_socket_address::IpOrSocketAddress;
 use utils::make_config_setting;
 
 use crate::{
+    ban_config::BanConfig,
     net::types::services::{Service, Services},
-    peer_manager::PeerManagerConfig,
+    peer_manager::config::PeerManagerConfig,
     protocol::ProtocolConfig,
 };
 
-make_config_setting!(BanThreshold, u32, 100);
-// BanDuration is only 30 mins as a compromise between banning for a longer period of time
-// and not banning at all with alternative approaches such as discouragement
-make_config_setting!(BanDuration, Duration, Duration::from_secs(60 * 30));
 make_config_setting!(OutboundConnectionTimeout, Duration, Duration::from_secs(10));
 make_config_setting!(NodeTypeSetting, NodeType, NodeType::Full);
 make_config_setting!(AllowDiscoverPrivateIps, bool, false);
@@ -85,10 +82,8 @@ pub struct P2pConfig {
     pub reserved_nodes: Vec<IpOrSocketAddress>,
     /// Optional list of whitelisted addresses. Such addresses cannot be automatically banned.
     pub whitelisted_addresses: Vec<IpAddr>,
-    /// The score threshold after which a peer is banned.
-    pub ban_threshold: BanThreshold,
-    /// Duration of bans in seconds.
-    pub ban_duration: BanDuration,
+    /// Settings related to banning and discouragement.
+    pub ban_config: BanConfig,
     /// The outbound connection timeout value in seconds.
     pub outbound_connection_timeout: OutboundConnectionTimeout,
     /// How often send ping requests to peers
@@ -108,7 +103,7 @@ pub struct P2pConfig {
     pub user_agent: UserAgent,
     /// A timeout after which a peer is disconnected.
     pub sync_stalling_timeout: SyncStallingTimeout,
-    /// Various limits used internally by the peer manager; these should only be overridden in tests.
+    /// Various settings used internally by the peer manager.
     pub peer_manager_config: PeerManagerConfig,
     /// Various limits related to the protocol; these should only be overridden in tests.
     pub protocol_config: ProtocolConfig,

p2p/src/error.rs

@@ -88,6 +88,8 @@ pub enum PeerError {
     },
     #[error("Address {0} is banned")]
     BannedAddress(String),
+    #[error("Address {0} is discouraged")]
+    DiscouragedAddress(String),
     #[error("PeerManager has too many peers")]
     TooManyPeers,
     #[error("Connection to address {0} already pending")]

p2p/src/lib.rs

@@ -13,6 +13,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+pub mod ban_config;
 pub mod config;
 pub mod error;
 pub mod interface;