fix: flatten duration serialization (#1647)

* Revert "fix: fix dump to config (#1637)" This reverts commit 3f750d8. * fix: flatten duration serialization to seconds * fix: rename peer manager timeouts to specify units and update serialization * fix: fix CR comments
starkware-libs · Nov 5, 2024 · ba1ac5c · ba1ac5c
1 parent 2c166ff
commit ba1ac5c
Show file tree

Hide file tree

Showing 7 changed files with 54 additions and 50 deletions.
diff --git a/config/mempool/default_config.json b/config/mempool/default_config.json
@@ -734,7 +734,7 @@
     "privacy": "Public",
     "value": 5
   },
-  "mempool_p2p_config.network_config.discovery_config.bootstrap_dial_retry_config.max_delay": {
+  "mempool_p2p_config.network_config.discovery_config.bootstrap_dial_retry_config.max_delay_seconds": {
     "description": "The maximum delay in seconds for the exponential backoff strategy.",
     "privacy": "Public",
     "value": 5
@@ -749,15 +749,15 @@
     "privacy": "Public",
     "value": 120
   },
-  "mempool_p2p_config.network_config.peer_manager_config.malicious_timeout": {
-    "description": "The duration a peer is blacklisted after being marked as malicious.",
+  "mempool_p2p_config.network_config.peer_manager_config.malicious_timeout_seconds": {
+    "description": "The duration in seconds a peer is blacklisted after being marked as malicious.",
     "privacy": "Public",
     "value": 31536000
   },
-  "mempool_p2p_config.network_config.peer_manager_config.unstable_timeout": {
-    "description": "The duration a peer blacklisted after being reported as unstable.",
+  "mempool_p2p_config.network_config.peer_manager_config.unstable_timeout_millis": {
+    "description": "The duration in milliseconds a peer blacklisted after being reported as unstable.",
     "privacy": "Public",
-    "value": 1
+    "value": 1000
   },
   "mempool_p2p_config.network_config.quic_port": {
     "description": "The port that the node listens on for incoming quic connections.",

diff --git a/config/papyrus/default_config.json b/config/papyrus/default_config.json
@@ -184,7 +184,7 @@
     "privacy": "Public",
     "value": 5
   },
-  "network.discovery_config.bootstrap_dial_retry_config.max_delay": {
+  "network.discovery_config.bootstrap_dial_retry_config.max_delay_seconds": {
     "description": "The maximum delay in seconds for the exponential backoff strategy.",
     "privacy": "Public",
     "value": 5
@@ -199,15 +199,15 @@
     "privacy": "Public",
     "value": 120
   },
-  "network.peer_manager_config.malicious_timeout": {
-    "description": "The duration a peer is blacklisted after being marked as malicious.",
+  "network.peer_manager_config.malicious_timeout_seconds": {
+    "description": "The duration in seconds a peer is blacklisted after being marked as malicious.",
     "privacy": "Public",
     "value": 31536000
   },
-  "network.peer_manager_config.unstable_timeout": {
-    "description": "The duration a peer blacklisted after being reported as unstable.",
+  "network.peer_manager_config.unstable_timeout_millis": {
+    "description": "The duration in milliseconds a peer blacklisted after being reported as unstable.",
     "privacy": "Public",
-    "value": 1
+    "value": 1000
   },
   "network.quic_port": {
     "description": "The port that the node listens on for incoming quic connections.",

diff --git a/crates/papyrus_network/src/discovery/discovery_test.rs b/crates/papyrus_network/src/discovery/discovery_test.rs
@@ -36,7 +36,7 @@ const BOOTSTRAP_DIAL_SLEEP: Duration = Duration::from_secs(1);
 const CONFIG: DiscoveryConfig = DiscoveryConfig {
     bootstrap_dial_retry_config: RetryConfig {
         base_delay_millis: BOOTSTRAP_DIAL_SLEEP.as_millis() as u64,
-        max_delay: BOOTSTRAP_DIAL_SLEEP.as_secs(),
+        max_delay_seconds: BOOTSTRAP_DIAL_SLEEP,
         factor: 1,
     },
     heartbeat_interval: Duration::ZERO,

diff --git a/crates/papyrus_network/src/discovery/mod.rs b/crates/papyrus_network/src/discovery/mod.rs
@@ -28,7 +28,10 @@ use libp2p::swarm::{
     ToSwarm,
 };
 use libp2p::{Multiaddr, PeerId};
-use papyrus_config::converters::deserialize_milliseconds_to_duration;
+use papyrus_config::converters::{
+    deserialize_milliseconds_to_duration,
+    deserialize_seconds_to_duration,
+};
 use papyrus_config::dumping::{append_sub_config_name, ser_param, SerializeConfig};
 use papyrus_config::{ParamPath, ParamPrivacyInput, SerializedParam};
 use serde::{Deserialize, Serialize};
@@ -221,13 +224,14 @@ impl SerializeConfig for DiscoveryConfig {
 #[derive(Copy, Clone, Debug, Serialize, Deserialize, PartialEq)]
 pub struct RetryConfig {
     pub base_delay_millis: u64,
-    pub max_delay: u64,
+    #[serde(deserialize_with = "deserialize_seconds_to_duration")]
+    pub max_delay_seconds: Duration,
     pub factor: u64,
 }
 
 impl Default for RetryConfig {
     fn default() -> Self {
-        Self { base_delay_millis: 2, max_delay: 5, factor: 5 }
+        Self { base_delay_millis: 2, max_delay_seconds: Duration::from_secs(5), factor: 5 }
     }
 }
 
@@ -241,8 +245,8 @@ impl SerializeConfig for RetryConfig {
                 ParamPrivacyInput::Public,
             ),
             ser_param(
-                "max_delay",
-                &self.max_delay,
+                "max_delay_seconds",
+                &self.max_delay_seconds.as_secs(),
                 "The maximum delay in seconds for the exponential backoff strategy.",
                 ParamPrivacyInput::Public,
             ),
@@ -259,7 +263,7 @@ impl SerializeConfig for RetryConfig {
 impl RetryConfig {
     fn strategy(&self) -> ExponentialBackoff {
         ExponentialBackoff::from_millis(self.base_delay_millis)
-            .max_delay(Duration::from_secs(self.max_delay))
+            .max_delay(self.max_delay_seconds)
             .factor(self.factor)
     }
 }

diff --git a/crates/papyrus_network/src/peer_manager/mod.rs b/crates/papyrus_network/src/peer_manager/mod.rs
@@ -6,6 +6,10 @@ use futures::FutureExt;
 use libp2p::swarm::dial_opts::DialOpts;
 use libp2p::swarm::ToSwarm;
 use libp2p::PeerId;
+use papyrus_config::converters::{
+    deserialize_milliseconds_to_duration,
+    deserialize_seconds_to_duration,
+};
 use papyrus_config::dumping::{ser_param, SerializeConfig};
 use papyrus_config::{ParamPath, ParamPrivacyInput, SerializedParam};
 use peer::Peer;
@@ -52,8 +56,10 @@ pub struct PeerManager {
 
 #[derive(Debug, Clone, Deserialize, Serialize, PartialEq)]
 pub struct PeerManagerConfig {
-    malicious_timeout: u64,
-    unstable_timeout: u64,
+    #[serde(deserialize_with = "deserialize_seconds_to_duration")]
+    malicious_timeout_seconds: Duration,
+    #[serde(deserialize_with = "deserialize_milliseconds_to_duration")]
+    unstable_timeout_millis: Duration,
 }
 
 #[derive(thiserror::Error, Debug)]
@@ -70,8 +76,8 @@ impl Default for PeerManagerConfig {
     fn default() -> Self {
         Self {
             // 1 year.
-            malicious_timeout: 3600 * 24 * 365,
-            unstable_timeout: 1,
+            malicious_timeout_seconds: Duration::from_secs(3600 * 24 * 365),
+            unstable_timeout_millis: Duration::from_millis(1000),
         }
     }
 }
@@ -80,15 +86,15 @@ impl SerializeConfig for PeerManagerConfig {
     fn dump(&self) -> BTreeMap<ParamPath, SerializedParam> {
         BTreeMap::from([
             ser_param(
-                "malicious_timeout",
-                &self.malicious_timeout,
-                "The duration a peer is blacklisted after being marked as malicious.",
+                "malicious_timeout_seconds",
+                &self.malicious_timeout_seconds.as_secs(),
+                "The duration in seconds a peer is blacklisted after being marked as malicious.",
                 ParamPrivacyInput::Public,
             ),
             ser_param(
-                "unstable_timeout",
-                &self.unstable_timeout,
-                "The duration a peer blacklisted after being reported as unstable.",
+                "unstable_timeout_millis",
+                &self.unstable_timeout_millis.as_millis(),
+                "The duration in milliseconds a peer blacklisted after being reported as unstable.",
                 ParamPrivacyInput::Public,
             ),
         ])
@@ -216,12 +222,12 @@ impl PeerManager {
                 ReputationModifier::Misconduct { misconduct_score } => {
                     peer.report(misconduct_score);
                     if peer.is_malicious() {
-                        peer.blacklist_peer(Duration::from_secs(self.config.malicious_timeout));
+                        peer.blacklist_peer(self.config.malicious_timeout_seconds);
                         peer.reset_misconduct_score();
                     }
                 }
                 ReputationModifier::Unstable => {
-                    peer.blacklist_peer(Duration::from_secs(self.config.unstable_timeout));
+                    peer.blacklist_peer(self.config.unstable_timeout_millis);
                 }
             }
             Ok(())

diff --git a/crates/papyrus_network/src/peer_manager/test.rs b/crates/papyrus_network/src/peer_manager/test.rs
@@ -153,10 +153,11 @@ async fn peer_assignment_no_peers() {
 
 #[tokio::test]
 async fn peer_assignment_no_unblocked_peers() {
-    const BLOCKED_UNTIL: u64 = 5;
-    const TIMEOUT: u64 = 1;
+    const BLOCKED_UNTIL: Duration = Duration::from_secs(5);
+    const TIMEOUT: Duration = Duration::from_secs(1);
     // Create a new peer manager
-    let config = PeerManagerConfig { malicious_timeout: TIMEOUT, unstable_timeout: TIMEOUT };
+    let config =
+        PeerManagerConfig { malicious_timeout_seconds: TIMEOUT, unstable_timeout_millis: TIMEOUT };
     let mut peer_manager: PeerManager = PeerManager::new(config.clone());
 
     // Create a session
@@ -172,10 +173,7 @@ async fn peer_assignment_no_unblocked_peers() {
     peer_manager.report_peer(peer_id, ReputationModifier::Unstable).unwrap();
 
     // Consume the peer blacklisted event
-    let event = tokio::time::timeout(Duration::from_secs(TIMEOUT), peer_manager.next())
-        .await
-        .unwrap()
-        .unwrap();
+    let event = tokio::time::timeout(TIMEOUT, peer_manager.next()).await.unwrap().unwrap();
     assert_matches!(
         event,
         ToSwarm::GenerateEvent(ToOtherBehaviourEvent::PeerBlacklisted { peer_id: event_peer_id })
@@ -188,14 +186,11 @@ async fn peer_assignment_no_unblocked_peers() {
 
     // Simulate that BLOCKED_UNTIL has passed.
     tokio::time::pause();
-    tokio::time::advance(Duration::from_secs(BLOCKED_UNTIL)).await;
+    tokio::time::advance(BLOCKED_UNTIL).await;
     tokio::time::resume();
 
     // After BLOCKED_UNTIL has passed, the peer manager can assign the session.
-    let event = tokio::time::timeout(Duration::from_secs(TIMEOUT), peer_manager.next())
-        .await
-        .unwrap()
-        .unwrap();
+    let event = tokio::time::timeout(TIMEOUT, peer_manager.next()).await.unwrap().unwrap();
     assert_matches!(
         event,
         ToSwarm::GenerateEvent(ToOtherBehaviourEvent::SessionAssigned {

diff --git a/...rus_node/src/config/snapshots/papyrus_node__config__config_test__dump_default_config.snap b/...rus_node/src/config/snapshots/papyrus_node__config__config_test__dump_default_config.snap
@@ -1,6 +1,5 @@
 ---
 source: crates/papyrus_node/src/config/config_test.rs
-assertion_line: 101
 expression: dumped_default_config
 ---
 {
@@ -211,7 +210,7 @@ expression: dumped_default_config
     },
     "privacy": "Public"
   },
-  "network.discovery_config.bootstrap_dial_retry_config.max_delay": {
+  "network.discovery_config.bootstrap_dial_retry_config.max_delay_seconds": {
     "description": "The maximum delay in seconds for the exponential backoff strategy.",
     "value": {
       "$serde_json::private::Number": "5"
@@ -232,17 +231,17 @@ expression: dumped_default_config
     },
     "privacy": "Public"
   },
-  "network.peer_manager_config.malicious_timeout": {
-    "description": "The duration a peer is blacklisted after being marked as malicious.",
+  "network.peer_manager_config.malicious_timeout_seconds": {
+    "description": "The duration in seconds a peer is blacklisted after being marked as malicious.",
     "value": {
       "$serde_json::private::Number": "31536000"
     },
     "privacy": "Public"
   },
-  "network.peer_manager_config.unstable_timeout": {
-    "description": "The duration a peer blacklisted after being reported as unstable.",
+  "network.peer_manager_config.unstable_timeout_millis": {
+    "description": "The duration in milliseconds a peer blacklisted after being reported as unstable.",
     "value": {
-      "$serde_json::private::Number": "1"
+      "$serde_json::private::Number": "1000"
     },
     "privacy": "Public"
   },