Skip to content

Commit

Permalink
fix: weird behaviour of dates in base node banned peers (#4037)
Browse files Browse the repository at this point in the history
Description
---
* Changed the peer display format to output `offline_at` and `banned_until` as local time instead of UTC, fixing the “time in the past” issue
* Improved handling and display of permanent bans (when invoking `ban-peer` with no params). Note that this makes _existing_ permabans still display weird times, changes affect only new permabans
* Scope of the changes does seem to only affect log messages in other applications

Motivation and Context
---
There is some weird behaviour regarding display of date times on banned peers.
- Times are shown in UTC but without the timezone, so if the user is in UTC+N the ban end time may look like it's "in the past" for some values
- The default ban time is permanent, and this makes the "banned until" display the max datetime.

How Has This Been Tested?
---
Manually tested on a base node, by banning peers with different values
  • Loading branch information
mrnaveira authored Apr 29, 2022
1 parent d01d94f commit 7097185
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 9 deletions.
10 changes: 7 additions & 3 deletions comms/core/src/peer_manager/peer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ use crate::{
peer_manager::identity_signature::IdentitySignature,
protocol::ProtocolId,
types::CommsPublicKey,
utils::datetime::safe_future_datetime_from_duration,
utils::datetime::{format_local_datetime, is_max_datetime, safe_future_datetime_from_duration},
};

bitflags! {
Expand Down Expand Up @@ -336,11 +336,15 @@ impl Display for Peer {
let status_str = {
let mut s = Vec::new();
if let Some(offline_at) = self.offline_at.as_ref() {
s.push(format!("Offline since: {}", offline_at));
s.push(format!("Offline since: {}", format_local_datetime(offline_at)));
}

if let Some(dt) = self.banned_until() {
s.push(format!("Banned until: {}", dt));
if is_max_datetime(dt) {
s.push("Banned permanently".to_string());
} else {
s.push(format!("Banned until: {}", format_local_datetime(dt)));
}
s.push(format!("Reason: {}", self.banned_reason))
}
s.join(". ")
Expand Down
19 changes: 13 additions & 6 deletions comms/core/src/utils/datetime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,15 +22,13 @@

use std::time::Duration;

use chrono::{DateTime, NaiveTime, Utc};
use chrono::{DateTime, Local, NaiveDateTime, Utc};

pub fn safe_future_datetime_from_duration(duration: Duration) -> DateTime<Utc> {
let old_duration = chrono::Duration::from_std(duration).unwrap_or_else(|_| chrono::Duration::max_value());
Utc::now().checked_add_signed(old_duration).unwrap_or_else(|| {
chrono::MAX_DATE
.and_time(NaiveTime::from_hms(0, 0, 0))
.expect("cannot fail")
})
Utc::now()
.checked_add_signed(old_duration)
.unwrap_or(chrono::MAX_DATETIME)
}

pub fn format_duration(duration: Duration) -> String {
Expand All @@ -48,6 +46,15 @@ pub fn format_duration(duration: Duration) -> String {
}
}

pub fn format_local_datetime(datetime: &NaiveDateTime) -> String {
let local_datetime = DateTime::<Local>::from_utc(*datetime, Local::now().offset().to_owned());
local_datetime.format("%Y-%m-%d %H:%M:%S").to_string()
}

pub fn is_max_datetime(datetime: &NaiveDateTime) -> bool {
chrono::MAX_DATETIME.naive_utc() == *datetime
}

#[cfg(test)]
mod test {
use super::*;
Expand Down

0 comments on commit 7097185

Please sign in to comment.