Discouraged peers are now disconnected automatically;

Peers are no longer automatically banned;
Ban duration is now always specified explicitly by the user;
Time formatting improvements;
Wallet & rpc commands for printing reserved and discouraged addresses were added;
Printing banned and discouraged addresses now also prints the expiration time;

Cargo.toml

@@ -139,6 +139,7 @@ criterion = "0.5"
 crossterm = "0.27"
 derive_more = "0.99"
 directories = "5.0"
+humantime = "2.1"
 dyn-clone = "1.0"
 enum-iterator = "1.4"
 expect-test = "1.3"

chainstate/src/detail/mod.rs

@@ -537,7 +537,7 @@ impl<S: BlockchainStorage, V: TransactionVerificationStrategy> Chainstate<S, V>
                 bi.block_id(),
                 bi.block_height(),
                 bi.block_timestamp(),
-                bi.block_timestamp().into_time().as_standard_printable_time(),
+                bi.block_timestamp().into_time(),
             );
 
             self.update_initial_block_download_flag()

common/src/chain/transaction/printout.rs

@@ -61,10 +61,7 @@ pub fn transaction_summary(tx: &Transaction, chain_config: &ChainConfig) -> Stri
     };
     let fmt_timelock = |tl: &OutputTimeLock| match tl {
         OutputTimeLock::UntilHeight(h) => format!("OutputTimeLock::UntilHeight({h})"),
-        OutputTimeLock::UntilTime(t) => format!(
-            "OutputTimeLock::UntilTime({})",
-            t.into_time().as_standard_printable_time()
-        ),
+        OutputTimeLock::UntilTime(t) => format!("OutputTimeLock::UntilTime({})", t.into_time()),
         OutputTimeLock::ForBlockCount(n) => format!("OutputTimeLock::ForBlockCount({n} blocks)"),
         OutputTimeLock::ForSeconds(secs) => {
             format!("OutputTimeLock::ForSeconds({secs} seconds)")

common/src/primitives/time.rs

@@ -13,9 +13,13 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+use std::fmt::{Debug, Display};
 use std::sync::atomic::{AtomicU64, Ordering};
 use std::time::{Duration, SystemTime};
 
+use chrono::TimeZone;
+use serde::{Deserialize, Serialize};
+
 pub fn duration_to_int(d: &Duration) -> Result<u64, std::num::TryFromIntError> {
     let r = d.as_millis().try_into()?;
     Ok(r)
@@ -61,7 +65,7 @@ pub fn get_time() -> Time {
     }
 }
 
-#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)]
+#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize)]
 pub struct Time {
     /// Time, stored as duration since SystemTime::UNIX_EPOCH
     time: Duration,
@@ -102,13 +106,13 @@ impl Time {
         self.time.saturating_sub(t.time)
     }
 
-    pub fn as_absolute_time(&self) -> SystemTime {
-        SystemTime::UNIX_EPOCH + self.time
-    }
-
-    pub fn as_standard_printable_time(&self) -> String {
-        let datetime: chrono::DateTime<chrono::Utc> = self.as_absolute_time().into();
-        format!("{}", datetime.format("%Y-%m-%d %H:%M:%S"))
+    pub fn as_absolute_time(&self) -> Option<chrono::DateTime<chrono::Utc>> {
+        TryInto::<i64>::try_into(self.time.as_secs()).ok().and_then(|secs| {
+            // Note: chrono::DateTime supports time values up to about 262,000 years away
+            // from the common era, which is still way below i64::MAX; i.e. timestamp_opt
+            // may still return None here.
+            chrono::Utc.timestamp_opt(secs, self.time.subsec_nanos()).single()
+        })
     }
 }
 
@@ -136,6 +140,33 @@ impl std::ops::Sub<Time> for Time {
     }
 }
 
+impl Debug for Time {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        let utc_time = self.as_absolute_time();
+
+        if let Some(time) = utc_time {
+            write!(f, "{time:?}")
+        } else {
+            write!(f, "Time({:?})", self.time)
+        }
+    }
+}
+
+impl Display for Time {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        let utc_time = self.as_absolute_time();
+
+        if let Some(time) = utc_time {
+            write!(f, "{time}")
+        } else {
+            // Note: we could use humantime::format_duration here, but the output won't be
+            // very nice, e.g. for Duration::MAX it'll be:
+            // "584542046090years 7months 15days 17h 5m 3s 999ms 999us 999ns"
+            write!(f, "{:?} since Unix epoch", self.time)
+        }
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use logging::log;
@@ -189,9 +220,23 @@ mod tests {
     }
 
     #[test]
-    fn format_absolute_time() {
+    fn debug_display() {
         let t = Time::from_secs_since_epoch(1705064092);
-        let s = t.as_standard_printable_time().to_string();
-        assert_eq!(&s, "2024-01-12 12:54:52");
+        let s = format!("{t:?}");
+        assert_eq!(s, "2024-01-12T12:54:52Z");
+        let s = format!("{t}");
+        assert_eq!(s, "2024-01-12 12:54:52 UTC");
+
+        let t = Time::from_duration_since_epoch(Duration::from_millis(1705064092123));
+        let s = format!("{t:?}");
+        assert_eq!(s, "2024-01-12T12:54:52.123Z");
+        let s = format!("{t}");
+        assert_eq!(s, "2024-01-12 12:54:52.123 UTC");
+
+        let t = Time::from_duration_since_epoch(Duration::MAX);
+        let s = format!("{t:?}");
+        assert_eq!(s, "Time(18446744073709551615.999999999s)");
+        let s = format!("{t}");
+        assert_eq!(s, "18446744073709551615.999999999s since Unix epoch");
     }
 }

dns-server/src/main.rs

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

node-lib/src/config_files/mod.rs

@@ -172,8 +172,6 @@ fn p2p_config(config: P2pConfigFile, options: &RunOptions) -> P2pConfigFile {
         reserved_nodes,
         whitelisted_addresses,
         max_inbound_connections,
-        ban_threshold,
-        ban_duration,
         discouragement_threshold,
         discouragement_duration,
         max_clock_diff,
@@ -192,7 +190,9 @@ fn p2p_config(config: P2pConfigFile, options: &RunOptions) -> P2pConfigFile {
     let reserved_nodes = options.p2p_reserved_node.clone().or(reserved_nodes);
     let whitelisted_addresses = options.p2p_whitelist_addr.clone().or(whitelisted_addresses);
     let max_inbound_connections = options.p2p_max_inbound_connections.or(max_inbound_connections);
-    let ban_threshold = options.p2p_ban_threshold.or(ban_threshold);
+    let discouragement_threshold =
+        options.p2p_discouragement_threshold.or(discouragement_threshold);
+    let discouragement_duration = options.p2p_discouragement_duration.or(discouragement_duration);
     let ping_check_period = options.p2p_ping_check_period.or(ping_check_period);
     let ping_timeout = options.p2p_ping_timeout.or(ping_timeout);
     let max_clock_diff = options.p2p_max_clock_diff.or(max_clock_diff);
@@ -212,8 +212,6 @@ fn p2p_config(config: P2pConfigFile, options: &RunOptions) -> P2pConfigFile {
         reserved_nodes,
         whitelisted_addresses,
         max_inbound_connections,
-        ban_threshold,
-        ban_duration,
         discouragement_threshold,
         discouragement_duration,
         max_clock_diff,

node-lib/src/config_files/p2p.rs

@@ -73,10 +73,6 @@ pub struct P2pConfigFile {
     pub whitelisted_addresses: Option<Vec<IpAddr>>,
     /// Maximum allowed number of inbound connections.
     pub max_inbound_connections: Option<usize>,
-    /// The score threshold after which a peer is banned.
-    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.
@@ -109,8 +105,6 @@ impl From<P2pConfigFile> for P2pConfig {
             reserved_nodes,
             whitelisted_addresses,
             max_inbound_connections,
-            ban_threshold,
-            ban_duration,
             discouragement_threshold,
             discouragement_duration,
             max_clock_diff,
@@ -130,8 +124,6 @@ impl From<P2pConfigFile> for P2pConfig {
             reserved_nodes: reserved_nodes.unwrap_or_default(),
             whitelisted_addresses: whitelisted_addresses.unwrap_or_default(),
             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(),
             },

node-lib/src/options.rs

@@ -136,11 +136,13 @@ pub struct RunOptions {
     #[clap(long)]
     pub p2p_max_inbound_connections: Option<usize>,
 
-    // TODO: add all the options related to banning/discouragement thresholds and durations,
-    // for completeness.
-    /// The p2p score threshold after which a peer is banned.
+    /// The p2p score threshold after which a peer is discouraged.
     #[clap(long)]
-    pub p2p_ban_threshold: Option<u32>,
+    pub p2p_discouragement_threshold: Option<u32>,
+
+    /// The p2p discouragement duration in seconds.
+    #[clap(long)]
+    pub p2p_discouragement_duration: Option<u64>,
 
     /// The p2p timeout value in seconds.
     #[clap(long)]

node-lib/tests/cli.rs

@@ -63,7 +63,14 @@ fn create_default_config() {
         .bind_addresses
         .unwrap_or_default()
         .is_empty());
-    assert_eq!(config.p2p.clone().unwrap_or_default().ban_threshold, None);
+    assert_eq!(
+        config.p2p.clone().unwrap_or_default().discouragement_threshold,
+        None
+    );
+    assert_eq!(
+        config.p2p.clone().unwrap_or_default().discouragement_duration,
+        None
+    );
     assert_eq!(
         config.p2p.clone().unwrap_or_default().outbound_connection_timeout,
         None
@@ -97,7 +104,8 @@ fn read_config_override_values() {
     let p2p_boot_node: IpOrSocketAddress = "127.0.0.1".parse().unwrap();
     let p2p_reserved_node: IpOrSocketAddress = "127.0.0.1".parse().unwrap();
     let p2p_max_inbound_connections = 123;
-    let p2p_ban_threshold = 3;
+    let p2p_discouragement_threshold = 3;
+    let p2p_discouragement_duration = 234;
     let p2p_timeout = NonZeroU64::new(10000).unwrap();
     let p2p_ping_check_period = 30;
     let p2p_ping_timeout = NonZeroU64::new(60).unwrap();
@@ -127,7 +135,8 @@ fn read_config_override_values() {
         p2p_boot_node: Some(vec![p2p_boot_node.clone()]),
         p2p_reserved_node: Some(vec![p2p_reserved_node.clone()]),
         p2p_max_inbound_connections: Some(p2p_max_inbound_connections),
-        p2p_ban_threshold: Some(p2p_ban_threshold),
+        p2p_discouragement_threshold: Some(p2p_discouragement_threshold),
+        p2p_discouragement_duration: Some(p2p_discouragement_duration),
         p2p_outbound_connection_timeout: Some(p2p_timeout),
         p2p_ping_check_period: Some(p2p_ping_check_period),
         p2p_ping_timeout: Some(p2p_ping_timeout),
@@ -201,8 +210,12 @@ fn read_config_override_values() {
         Some(p2p_max_inbound_connections)
     );
     assert_eq!(
-        config.p2p.clone().unwrap().ban_threshold,
-        Some(p2p_ban_threshold)
+        config.p2p.clone().unwrap().discouragement_threshold,
+        Some(p2p_discouragement_threshold)
+    );
+    assert_eq!(
+        config.p2p.clone().unwrap().discouragement_duration,
+        Some(p2p_discouragement_duration)
     );
     assert_eq!(
         config.p2p.clone().unwrap().outbound_connection_timeout,

p2p/benches/benches.rs

@@ -13,7 +13,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-use std::{collections::BTreeSet, sync::Arc};
+use std::{collections::BTreeSet, sync::Arc, time::Duration};
 
 use common::chain::config::create_unit_test_config;
 use criterion::{criterion_group, criterion_main, Criterion};
@@ -35,7 +35,10 @@ pub fn peer_db(c: &mut Criterion) {
     }
 
     for _ in 0..1000 {
-        peerdb.ban(TestAddressMaker::new_random_address().as_bannable());
+        peerdb.ban(
+            TestAddressMaker::new_random_address().as_bannable(),
+            Duration::from_secs(60 * 60 * 24),
+        );
     }
 
     let outbound_addr_groups = (0..5)

p2p/src/ban_config.rs

@@ -17,37 +17,18 @@ 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.
+/// Settings related to banning in the general sense (i.e. to the handling of BanScore and
+/// potentially to manual banning as well).
 #[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.
+    /// The ban score threshold after which a peer becomes discouraged.
     pub discouragement_threshold: DiscouragementThreshold,
     /// The duration of discouragement.
     pub discouragement_duration: DiscouragementDuration,

p2p/src/error.rs

@@ -127,8 +127,6 @@ pub enum DialError {
 pub enum ConversionError {
     #[error("Invalid address: {0}")]
     InvalidAddress(String),
-    #[error("Failed to decode data: {0}")]
-    DecodeError(serialization::Error),
 }
 
 #[derive(Error, Debug, PartialEq, Eq, Clone)]
@@ -186,12 +184,6 @@ impl From<std::io::Error> for P2pError {
     }
 }
 
-impl From<serialization::Error> for P2pError {
-    fn from(err: serialization::Error) -> P2pError {
-        P2pError::ConversionError(ConversionError::DecodeError(err))
-    }
-}
-
 impl From<tokio::sync::oneshot::error::RecvError> for P2pError {
     fn from(_: tokio::sync::oneshot::error::RecvError) -> P2pError {
         P2pError::ChannelClosed
@@ -272,7 +264,6 @@ impl BanScore for ConversionError {
     fn ban_score(&self) -> u32 {
         match self {
             ConversionError::InvalidAddress(_) => 0,
-            ConversionError::DecodeError(_) => 100,
         }
     }
 }