From 7abe9a987dfdaefe0c44bb640282271b4de08715 Mon Sep 17 00:00:00 2001 From: Tommy Verrall Date: Thu, 11 Dec 2025 14:46:16 +0100 Subject: [PATCH] PR comments addressed - Changed peer_interaction_timeout from u64 milliseconds to Duration type - Made timeout non-configurable (always uses safe 5s default) to prevent users from setting unsafe values - Removed timeout from CLI args, env vars, and config template - Added suspend/resume scenario documentation - Simplified code by removing unnecessary tuple - Reverted old_config migration code (to be handled in separate PR) --- .../authenticator/config/mod.rs | 16 ++++--- .../authenticator/mixnet_listener.rs | 43 +++++++++---------- .../authenticator/mod.rs | 4 +- nym-node/src/cli/helpers.rs | 12 ------ nym-node/src/config/helpers.rs | 1 - nym-node/src/config/mod.rs | 19 +------- .../src/config/old_configs/old_config_v10.rs | 41 ------------------ .../src/config/old_configs/old_config_v9.rs | 1 - nym-node/src/config/template.rs | 4 -- nym-node/src/env.rs | 2 - 10 files changed, 32 insertions(+), 111 deletions(-) diff --git a/gateway/src/node/internal_service_providers/authenticator/config/mod.rs b/gateway/src/node/internal_service_providers/authenticator/config/mod.rs index 404e770631..b05be7d6de 100644 --- a/gateway/src/node/internal_service_providers/authenticator/config/mod.rs +++ b/gateway/src/node/internal_service_providers/authenticator/config/mod.rs @@ -7,6 +7,7 @@ use nym_network_defaults::{ }; use serde::{Deserialize, Serialize}; use std::net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddr}; +use std::time::Duration; pub use nym_client_core::config::Config as BaseClientConfig; pub use persistence::AuthenticatorPaths; @@ -26,7 +27,6 @@ pub struct Config { impl Config { pub fn validate(&self) -> bool { - // no other sections have explicit requirements (yet) self.base.validate() } } @@ -58,9 +58,11 @@ pub struct Authenticator { /// The maximum value for IPv6 is 128 pub private_network_prefix_v6: u8, - /// Timeout (in milliseconds) to wait for responses from the peer controller before failing. - /// Helps the authenticator recover from suspend/resume scenarios where peer RPCs hang. - pub peer_interaction_timeout_ms: u64, + /// Timeout to wait for responses from the peer controller before failing. + /// Helps the authenticator recover from suspend/resume scenarios where the peer controller + /// process/task can get stuck and never respond to oneshot RPC responses, which previously + /// caused the authenticator to block forever waiting on the oneshot channel. + pub peer_interaction_timeout: Duration, } impl Default for Authenticator { @@ -72,7 +74,7 @@ impl Default for Authenticator { tunnel_announced_port: WG_TUNNEL_PORT, private_network_prefix_v4: WG_TUN_DEVICE_NETMASK_V4, private_network_prefix_v6: WG_TUN_DEVICE_NETMASK_V6, - peer_interaction_timeout_ms: default_peer_interaction_timeout_ms(), + peer_interaction_timeout: default_peer_interaction_timeout(), } } } @@ -91,6 +93,6 @@ impl From for nym_wireguard_types::Config { } } -pub const fn default_peer_interaction_timeout_ms() -> u64 { - 5_000 +pub fn default_peer_interaction_timeout() -> Duration { + Duration::from_millis(5_000) } diff --git a/gateway/src/node/internal_service_providers/authenticator/mixnet_listener.rs b/gateway/src/node/internal_service_providers/authenticator/mixnet_listener.rs index 3913452cb8..77a7d38d06 100644 --- a/gateway/src/node/internal_service_providers/authenticator/mixnet_listener.rs +++ b/gateway/src/node/internal_service_providers/authenticator/mixnet_listener.rs @@ -394,30 +394,27 @@ impl MixnetListener { return Ok((bytes, reply_to)); } - let (registration_data, private_ips) = { - let private_ip = self - .registered_and_free - .free_private_network_ips - .iter_mut() - .filter(|r| r.1.is_none()) - .choose(&mut thread_rng()) - .ok_or(AuthenticatorError::NoFreeIp)?; - let private_ips = *private_ip.0; - // mark it as used, even though it's not final - *private_ip.1 = Some(SystemTime::now()); + let private_ip = self + .registered_and_free + .free_private_network_ips + .iter_mut() + .filter(|r| r.1.is_none()) + .choose(&mut thread_rng()) + .ok_or(AuthenticatorError::NoFreeIp)?; + let private_ips = *private_ip.0; + // mark it as used, even though it's not final + *private_ip.1 = Some(SystemTime::now()); - let gateway_data = GatewayClient::new( - self.keypair().private_key(), - remote_public.inner(), - private_ips, - nonce, - ); - let registration_data = latest::registration::RegistrationData { - nonce, - gateway_data: gateway_data.clone(), - wg_port: self.config.authenticator.tunnel_announced_port, - }; - (registration_data, private_ips) + let gateway_data = GatewayClient::new( + self.keypair().private_key(), + remote_public.inner(), + private_ips, + nonce, + ); + let registration_data = latest::registration::RegistrationData { + nonce, + gateway_data: gateway_data.clone(), + wg_port: self.config.authenticator.tunnel_announced_port, }; self.registered_and_free diff --git a/gateway/src/node/internal_service_providers/authenticator/mod.rs b/gateway/src/node/internal_service_providers/authenticator/mod.rs index 506dd11de8..bac338400c 100644 --- a/gateway/src/node/internal_service_providers/authenticator/mod.rs +++ b/gateway/src/node/internal_service_providers/authenticator/mod.rs @@ -151,7 +151,7 @@ impl Authenticator { } }) .collect(); - let peer_timeout = std::cmp::max(1, self.config.authenticator.peer_interaction_timeout_ms); + let peer_timeout = self.config.authenticator.peer_interaction_timeout; let mixnet_listener = crate::node::internal_service_providers::authenticator::mixnet_listener::MixnetListener::new( self.config, free_private_network_ips, @@ -159,7 +159,7 @@ impl Authenticator { mixnet_client, self.upgrade_mode_state, self.ecash_verifier, - std::time::Duration::from_millis(peer_timeout), + peer_timeout, ); tracing::info!("The address of this client is: {self_address}"); diff --git a/nym-node/src/cli/helpers.rs b/nym-node/src/cli/helpers.rs index 15e3fc9ee9..08ccef0857 100644 --- a/nym-node/src/cli/helpers.rs +++ b/nym-node/src/cli/helpers.rs @@ -9,7 +9,6 @@ use crate::error::NymNodeError; use celes::Country; use clap::Args; use clap::builder::ArgPredicate; -use humantime::Duration as HumanDuration; use nym_crypto::asymmetric::ed25519; use std::net::{IpAddr, SocketAddr}; use std::path::{Path, PathBuf}; @@ -287,13 +286,6 @@ pub(crate) struct WireguardArgs { )] pub(crate) wireguard_tunnel_announced_port: Option, - /// Timeout to wait for responses from the peer controller before aborting the operation. - #[clap( - long, - env = NYMNODE_WG_PEER_INTERACTION_TIMEOUT_MS_ARG - )] - pub(crate) wireguard_peer_interaction_timeout: Option, - /// The prefix denoting the maximum number of the clients that can be connected via Wireguard. /// The maximum value for IPv4 is 32 and for IPv6 is 128 #[clap( @@ -325,10 +317,6 @@ impl WireguardArgs { section.announced_tunnel_port = announced_tunnel_port } - if let Some(timeout) = self.wireguard_peer_interaction_timeout { - section.peer_interaction_timeout_ms = timeout.into(); - } - if let Some(private_network_prefix) = self.wireguard_private_network_prefix { section.private_network_prefix_v4 = private_network_prefix } diff --git a/nym-node/src/config/helpers.rs b/nym-node/src/config/helpers.rs index fd84a58cb3..9605302aa2 100644 --- a/nym-node/src/config/helpers.rs +++ b/nym-node/src/config/helpers.rs @@ -213,7 +213,6 @@ pub fn gateway_tasks_config(config: &Config) -> GatewayTasksConfig { private_network_prefix_v4: config.wireguard.private_network_prefix_v4, private_network_prefix_v6: config.wireguard.private_network_prefix_v6, storage_paths: config.wireguard.storage_paths.clone(), - peer_interaction_timeout_ms: config.wireguard.peer_interaction_timeout_ms, }, custom_mixnet_path: None, }; diff --git a/nym-node/src/config/mod.rs b/nym-node/src/config/mod.rs index 4dfe605263..16225fb2cc 100644 --- a/nym-node/src/config/mod.rs +++ b/nym-node/src/config/mod.rs @@ -23,7 +23,6 @@ use nym_config::{ }; use nym_gateway::nym_authenticator; use serde::{Deserialize, Serialize}; -use std::convert::TryFrom; use std::env; use std::fmt::{Display, Formatter}; use std::net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddr}; @@ -962,14 +961,6 @@ pub struct Wireguard { /// Paths for wireguard keys, client registries, etc. pub storage_paths: persistence::WireguardPaths, - - /// Timeout that limits how long the authenticator waits for the peer controller to reply. - /// Accepts standard humantime values, e.g. `5000ms`, `5s`, `1m`. - #[serde( - default = "Wireguard::default_peer_interaction_timeout", - with = "humantime_serde" - )] - pub peer_interaction_timeout_ms: Duration, } impl Wireguard { @@ -984,13 +975,8 @@ impl Wireguard { private_network_prefix_v4: WG_TUN_DEVICE_NETMASK_V4, private_network_prefix_v6: WG_TUN_DEVICE_NETMASK_V6, storage_paths: persistence::WireguardPaths::new(data_dir), - peer_interaction_timeout_ms: Self::default_peer_interaction_timeout(), } } - - pub fn default_peer_interaction_timeout() -> Duration { - Duration::from_millis(nym_authenticator::config::default_peer_interaction_timeout_ms()) - } } impl From for nym_wireguard_types::Config { @@ -1009,9 +995,6 @@ impl From for nym_wireguard_types::Config { impl From for nym_authenticator::config::Authenticator { fn from(value: Wireguard) -> Self { - let timeout_ms_u128 = value.peer_interaction_timeout_ms.as_millis(); - let timeout_ms = u64::try_from(timeout_ms_u128).unwrap_or(u64::MAX).max(1); - nym_authenticator::config::Authenticator { bind_address: value.bind_address, private_ipv4: value.private_ipv4, @@ -1019,7 +1002,7 @@ impl From for nym_authenticator::config::Authenticator { tunnel_announced_port: value.announced_tunnel_port, private_network_prefix_v4: value.private_network_prefix_v4, private_network_prefix_v6: value.private_network_prefix_v6, - peer_interaction_timeout_ms: timeout_ms, + peer_interaction_timeout: nym_authenticator::config::default_peer_interaction_timeout(), } } } diff --git a/nym-node/src/config/old_configs/old_config_v10.rs b/nym-node/src/config/old_configs/old_config_v10.rs index ec7db9bfe8..7fa7c9a269 100644 --- a/nym-node/src/config/old_configs/old_config_v10.rs +++ b/nym-node/src/config/old_configs/old_config_v10.rs @@ -20,7 +20,6 @@ use crate::config::{ use crate::error::NymNodeError; use celes::Country; use clap::ValueEnum; -use humantime::parse_duration; use nym_bin_common::logging::LoggingSettings; use nym_client_core_config_types::DebugConfig as ClientDebugConfig; use nym_config::defaults::{DEFAULT_VERLOC_LISTENING_PORT, WG_METADATA_PORT}; @@ -30,9 +29,7 @@ use nym_config::{ read_config_from_toml_file, serde_helpers::{de_maybe_port, de_maybe_stringified}, }; -use serde::de::{Deserializer, Error as SerdeDeError}; use serde::{Deserialize, Serialize}; -use std::convert::TryFrom; use std::net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddr}; use std::path::{Path, PathBuf}; use std::time::Duration; @@ -79,11 +76,6 @@ pub struct WireguardV10 { /// Paths for wireguard keys, client registries, etc. pub storage_paths: WireguardPathsV10, - - /// Optional override for the peer interaction timeout expressed in milliseconds. - /// Accepts either a plain integer value or a humantime string such as "5s". - #[serde(default, deserialize_with = "deserialize_optional_duration_ms")] - pub peer_interaction_timeout_ms: Option, } // a temporary solution until all "types" are run at the same time @@ -1343,11 +1335,6 @@ pub async fn try_upgrade_config_v10>( .storage_paths .public_diffie_hellman_key_file, }, - peer_interaction_timeout_ms: old_cfg - .wireguard - .peer_interaction_timeout_ms - .map(Duration::from_millis) - .unwrap_or_else(Wireguard::default_peer_interaction_timeout), }, gateway_tasks: GatewayTasksConfig { storage_paths: GatewayTasksPaths { @@ -1587,31 +1574,3 @@ pub async fn try_upgrade_config_v10>( }; Ok(cfg) } - -fn deserialize_optional_duration_ms<'de, D>(deserializer: D) -> Result, D::Error> -where - D: Deserializer<'de>, -{ - #[derive(Deserialize)] - #[serde(untagged)] - enum MaybeDuration { - Millis(u64), - Human(String), - } - - let maybe_value = Option::::deserialize(deserializer)?; - let Some(value) = maybe_value else { - return Ok(None); - }; - - match value { - MaybeDuration::Millis(ms) => Ok(Some(ms)), - MaybeDuration::Human(text) => { - let duration = - parse_duration(&text).map_err(|err| D::Error::custom(err.to_string()))?; - let millis = u64::try_from(duration.as_millis()) - .map_err(|_| D::Error::custom("duration too large"))?; - Ok(Some(millis)) - } - } -} diff --git a/nym-node/src/config/old_configs/old_config_v9.rs b/nym-node/src/config/old_configs/old_config_v9.rs index c50b0ea493..d14b89f31a 100644 --- a/nym-node/src/config/old_configs/old_config_v9.rs +++ b/nym-node/src/config/old_configs/old_config_v9.rs @@ -1348,7 +1348,6 @@ pub async fn try_upgrade_config_v9>( .storage_paths .public_diffie_hellman_key_file, }, - peer_interaction_timeout_ms: None, }, gateway_tasks: GatewayTasksConfigV10 { storage_paths: GatewayTasksPathsV10 { diff --git a/nym-node/src/config/template.rs b/nym-node/src/config/template.rs index 624be956cf..8177119780 100644 --- a/nym-node/src/config/template.rs +++ b/nym-node/src/config/template.rs @@ -151,10 +151,6 @@ announced_tunnel_port = {{ wireguard.announced_tunnel_port }} # Useful in the instances where the node is behind a proxy. announced_metadata_port = {{ wireguard.announced_metadata_port }} -# Timeout to wait for responses from the peer controller before aborting the operation. -# Accepts standard humantime duration strings such as `200ms`, `5s`. -peer_interaction_timeout_ms = '{{ wireguard.peer_interaction_timeout_ms }}' - # The prefix denoting the maximum number of the clients that can be connected via Wireguard using IPv4. # The maximum value for IPv4 is 32 private_network_prefix_v4 = {{ wireguard.private_network_prefix_v4 }} diff --git a/nym-node/src/env.rs b/nym-node/src/env.rs index 670eb37cae..1564d087a4 100644 --- a/nym-node/src/env.rs +++ b/nym-node/src/env.rs @@ -47,8 +47,6 @@ pub mod vars { pub const NYMNODE_WG_BIND_ADDRESS_ARG: &str = "NYMNODE_WG_BIND_ADDRESS"; pub const NYMNODE_WG_ANNOUNCED_PORT_ARG: &str = "NYMNODE_WG_ANNOUNCED_PORT"; pub const NYMNODE_WG_PRIVATE_NETWORK_PREFIX_ARG: &str = "NYMNODE_WG_PRIVATE_NETWORK_PREFIX"; - pub const NYMNODE_WG_PEER_INTERACTION_TIMEOUT_MS_ARG: &str = - "NYMNODE_WG_PEER_INTERACTION_TIMEOUT_MS"; // verloc: pub const NYMNODE_VERLOC_BIND_ADDRESS_ARG: &str = "NYMNODE_VERLOC_BIND_ADDRESS";