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)
This commit is contained in:
Tommy Verrall
2025-12-11 14:46:16 +01:00
parent 24ccba3a19
commit 7abe9a987d
10 changed files with 32 additions and 111 deletions
@@ -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<Authenticator> 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)
}
@@ -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
@@ -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}");
-12
View File
@@ -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<u16>,
/// 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<HumanDuration>,
/// 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
}
-1
View File
@@ -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,
};
+1 -18
View File
@@ -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<Wireguard> for nym_wireguard_types::Config {
@@ -1009,9 +995,6 @@ impl From<Wireguard> for nym_wireguard_types::Config {
impl From<Wireguard> 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<Wireguard> 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(),
}
}
}
@@ -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<u64>,
}
// a temporary solution until all "types" are run at the same time
@@ -1343,11 +1335,6 @@ pub async fn try_upgrade_config_v10<P: AsRef<Path>>(
.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<P: AsRef<Path>>(
};
Ok(cfg)
}
fn deserialize_optional_duration_ms<'de, D>(deserializer: D) -> Result<Option<u64>, D::Error>
where
D: Deserializer<'de>,
{
#[derive(Deserialize)]
#[serde(untagged)]
enum MaybeDuration {
Millis(u64),
Human(String),
}
let maybe_value = Option::<MaybeDuration>::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))
}
}
}
@@ -1348,7 +1348,6 @@ pub async fn try_upgrade_config_v9<P: AsRef<Path>>(
.storage_paths
.public_diffie_hellman_key_file,
},
peer_interaction_timeout_ms: None,
},
gateway_tasks: GatewayTasksConfigV10 {
storage_paths: GatewayTasksPathsV10 {
-4
View File
@@ -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 }}
-2
View File
@@ -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";