From 239cbdc4baa6d06186b85e13079191f0d3a23cb8 Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Fri, 1 May 2026 16:54:10 +0000 Subject: [PATCH 1/5] Fix Tor onion adverts missing port in Nostr overlay discovery The Nostr overlay advert publisher serialized `transport: tor` endpoints as a bare `.onion` hostname with no port. The Tor address parser requires `:` form and rejected the bare shape with `expected host:port`. Any peer receiving a Tor-only advert went into a persistent retry-fail loop on jittered backoff until the advert aged out of the discovery cache. The bug had been latent for as long as Tor adverts have been published on Nostr, and was masked in deployments where every node also advertised a non-Tor transport (peers fell through to the working endpoint). Surfaced first on a deployment where Tor was the only advert path. Publisher now emits `.onion:` using a new `transports.tor.advertised_port` config field that defaults to 443, matching the Tor `HiddenServicePort 443 127.0.0.1:` convention. Operators whose torrc uses a non-default virtual port can override. Adds a unit test that pins the publisher/parser contract: formats the advert exactly as the publisher does and asserts `parse_tor_addr` accepts the result; asserts the bare-onion form (the bug) does not parse, catching any future regression that drops the port again. Parser is unchanged (already correct). --- CHANGELOG.md | 10 +++++++++ docs/design/fips-configuration.md | 1 + src/config/transport.rs | 17 +++++++++++++++ src/node/lifecycle.rs | 2 +- src/transport/tor/mod.rs | 35 +++++++++++++++++++++++++++++++ 5 files changed, 64 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2daed37..d7060c2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -259,6 +259,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Tor onion adverts published over Nostr overlay discovery now + include the public-facing port (`.onion:`) instead of + just the bare onion hostname. The publisher previously emitted a + bare onion that the parser refused (`expected host:port`), + producing a persistent retry-fail loop on any peer whose Tor + advert was the only entry in the discovery cache. New + `transports.tor.advertised_port` config field (default `443`, + matching the Tor `HiddenServicePort` convention) controls the + advertised port; operators with non-default virtual ports can + override. - Control socket path detection in fipsctl and fipstop now checks for the `/run/fips/` directory instead of the socket file inside it, so users not yet in the `fips` group get a clear "Permission denied" diff --git a/docs/design/fips-configuration.md b/docs/design/fips-configuration.md index f2b1613..228a9fa 100644 --- a/docs/design/fips-configuration.md +++ b/docs/design/fips-configuration.md @@ -467,6 +467,7 @@ Requires an external Tor daemon providing a SOCKS5 proxy. Three modes: | `transports.tor.max_inbound_connections` | usize | `64` | Maximum inbound connections via onion service. | | `transports.tor.directory_service.hostname_file` | string | `"/var/lib/tor/fips_onion_service/hostname"` | Path to Tor-managed hostname file containing the `.onion` address. | | `transports.tor.directory_service.bind_addr` | string | `"127.0.0.1:8443"` | Local bind address for the listener that Tor forwards inbound connections to. Must match `HiddenServicePort` target in `torrc`. | +| `transports.tor.advertised_port` | u16 | `443` | Public-facing onion port published in Nostr overlay adverts. Must match the virtual port in torrc's `HiddenServicePort 127.0.0.1:` directive — that is the port other peers will use to reach this onion. | **Named instances.** Like other transports, multiple Tor instances can be configured with named sub-keys for different SOCKS5 proxy endpoints. diff --git a/src/config/transport.rs b/src/config/transport.rs index ac2a150..af996f6 100644 --- a/src/config/transport.rs +++ b/src/config/transport.rs @@ -441,6 +441,10 @@ const DEFAULT_HOSTNAME_FILE: &str = "/var/lib/tor/fips_onion_service/hostname"; /// Default directory mode bind address. const DEFAULT_DIRECTORY_BIND_ADDR: &str = "127.0.0.1:8443"; +/// Default advertised onion port for Nostr overlay discovery. Matches the +/// Tor convention of `HiddenServicePort 443 127.0.0.1:` in torrc. +const DEFAULT_TOR_ADVERTISED_PORT: u16 = 443; + /// Tor transport instance configuration. /// /// Supports three modes: @@ -503,6 +507,13 @@ pub struct TorConfig { /// Default: false. #[serde(default, skip_serializing_if = "Option::is_none")] pub advertise_on_nostr: Option, + + /// Public-facing onion port published in Nostr overlay adverts. Must + /// match the virtual port in torrc's `HiddenServicePort + /// 127.0.0.1:` directive — that is the port other peers + /// will use to reach this onion. Default: 443. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub advertised_port: Option, } /// Directory-mode onion service configuration. @@ -595,6 +606,12 @@ impl TorConfig { pub fn advertise_on_nostr(&self) -> bool { self.advertise_on_nostr.unwrap_or(false) } + + /// Public-facing onion port published in Nostr overlay adverts. + /// Default: 443. + pub fn advertised_port(&self) -> u16 { + self.advertised_port.unwrap_or(DEFAULT_TOR_ADVERTISED_PORT) + } } // ============================================================================ diff --git a/src/node/lifecycle.rs b/src/node/lifecycle.rs index 660988a..70f1e3d 100644 --- a/src/node/lifecycle.rs +++ b/src/node/lifecycle.rs @@ -1341,7 +1341,7 @@ impl Node { if let Some(addr) = handle.onion_address() { endpoints.push(OverlayEndpointAdvert { transport: OverlayTransportKind::Tor, - addr: addr.to_string(), + addr: format!("{}:{}", addr, cfg.advertised_port()), }); } } diff --git a/src/transport/tor/mod.rs b/src/transport/tor/mod.rs index f475850..dd7355f 100644 --- a/src/transport/tor/mod.rs +++ b/src/transport/tor/mod.rs @@ -1446,6 +1446,41 @@ mod tests { assert_eq!(config.socks5_addr(), "127.0.0.1:9050"); assert_eq!(config.connect_timeout_ms(), 120000); assert_eq!(config.mtu(), 1400); + assert_eq!(config.advertised_port(), 443); + } + + #[test] + fn test_advertised_port_override() { + let config = TorConfig { + advertised_port: Some(9001), + ..Default::default() + }; + assert_eq!(config.advertised_port(), 9001); + } + + /// Pins the publisher/parser contract for Tor overlay adverts. + /// `build_overlay_advert` formats Tor endpoints as `:`; + /// `parse_tor_addr` must accept that exact form back. A bare onion + /// (no port) was the production bug — assert it does not parse. + #[test] + fn test_advert_address_round_trips_through_parser() { + let onion = "mwvj6q3pnsiaky7i6wg5s42xlfurt5uqr3qzckrlw2graa2ugcgwhiqd.onion"; + let cfg = TorConfig::default(); + let advertised = format!("{}:{}", onion, cfg.advertised_port()); + + let parsed = parse_tor_addr(&TransportAddr::from_string(&advertised)).unwrap(); + match parsed { + TorAddr::Onion(host, port) => { + assert_eq!(host, onion); + assert_eq!(port, 443); + } + other => panic!("expected Onion variant, got {:?}", other), + } + + // Sanity-check the inverse: the bare-onion form (the bug) must + // not parse, so any future regression in the publisher will be + // caught by the round-trip test above. + assert!(parse_tor_addr(&TransportAddr::from_string(onion)).is_err()); } #[tokio::test] From ab2edec2c660dfa604250948adbf18f2d67ccd8b Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Fri, 1 May 2026 18:09:50 +0000 Subject: [PATCH 2/5] Add Nostr open-discovery startup sweep with diagnostic logging MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Under `node.discovery.nostr.policy: open`, the per-tick auto-dial in `queue_open_discovery_retries` was supposed to pick up adverts cached from the relay subscription backlog at startup, but in practice only adverts arriving live (after the daemon was up) were being dialed. Backlog adverts sat in the in-memory cache until they aged out. Adds a one-shot startup sweep that runs once per daemon start, gated identically to the per-tick sweep (`enabled` && `policy == open`), after a configurable settle delay so the relay subscription backlog has time to populate the advert cache. The sweep iterates the cache with the same skip-filters as the per-tick path (statically-configured peers, already-connected, retry-pending, connecting) plus a tighter age filter: only adverts whose `created_at` is within `startup_sweep_max_age_secs` of now are queued. Two new config fields under `node.discovery.nostr`: - `startup_sweep_delay_secs` (default 5) - `startup_sweep_max_age_secs` (default 3600 = one hour) Both are only consulted when `policy == open`; under any other policy the sweep is a no-op. Adds diagnostic logging to the open-discovery sweep so operators can verify what the auto-dial path is doing on each daemon bring-up: info-level on each retry-queued enqueue (with peer short-npub and advert age), and a one-line summary on every startup sweep and on any per-tick sweep that queues at least one retry. The summary breaks down skipped candidates by reason (age, configured, self, already-connected, retry-pending, connecting, no-endpoints, invalid-npub) — currently the path was silent so there was no operator-visible signal that the cache iteration was running. Refactors the existing `queue_open_discovery_retries` body into a shared `run_open_discovery_sweep(max_age_secs, caller)` helper so the per-tick and startup paths share filter/queue logic and only differ in the age filter and log label. Surfaces `created_at` from `NostrDiscovery::cached_open_discovery_candidates` (return tuple extended) so the age filter has the data it needs. Three new unit tests in `config::node::tests` cover the new defaults, YAML override round-trip, and partial-YAML default fallback. --- CHANGELOG.md | 15 ++++ docs/design/fips-configuration.md | 2 + src/config/node.rs | 48 ++++++++++ src/discovery/nostr/runtime.rs | 10 ++- src/node/lifecycle.rs | 145 +++++++++++++++++++++++++++++- src/node/mod.rs | 13 +++ 6 files changed, 228 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d7060c2..e5e04df 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -194,6 +194,21 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 pre-`0.3.0` so a tagged release supersedes any prior dev .deb. Tagged release builds (no `-dev` in Cargo.toml) keep the clean `-1` form. Operator override via `--version` still wins +- One-shot startup advert sweep for Nostr open-discovery. On daemon + startup under `node.discovery.nostr.policy: open`, after a short + settle delay (`startup_sweep_delay_secs`, default 5s) the cached + overlay-advert table is iterated once and recent adverts (newer + than `startup_sweep_max_age_secs`, default 3600s) are queued for + outbound retry, modulo the same skip-filters as the per-tick sweep + (configured peer, already connected, retry-pending, connecting). + Closes the gap where peers learned only through relay backlog at + startup were not dialed until they republished. +- Diagnostic logging on the open-discovery sweep. Each `queued retry` + now logs at info-level with the peer short-npub and advert age, + and a one-line summary (cached count, queued count, per-reason + skip counts) is emitted on every startup sweep and on any per-tick + sweep that queues at least one retry. Operator-facing visibility + into what the auto-dial path is doing. ### Changed diff --git a/docs/design/fips-configuration.md b/docs/design/fips-configuration.md index 228a9fa..686dc82 100644 --- a/docs/design/fips-configuration.md +++ b/docs/design/fips-configuration.md @@ -205,6 +205,8 @@ without that feature ignore `udp:nat` bootstrap configuration. | `node.discovery.nostr.punch_duration_ms` | u64 | `10000` | How long to keep punching before failure | | `node.discovery.nostr.advert_ttl_secs` | u64 | `3600` | Advert TTL in seconds | | `node.discovery.nostr.advert_refresh_secs` | u64 | `1800` | How often adverts are refreshed in seconds | +| `node.discovery.nostr.startup_sweep_delay_secs` | u64 | `5` | Settle delay after Nostr discovery starts before the one-shot startup advert sweep runs (only used under `policy: open`). Allows the relay subscription backlog to populate the in-memory advert cache before the sweep fires | +| `node.discovery.nostr.startup_sweep_max_age_secs` | u64 | `3600` | Maximum advert age (`now - created_at`) considered by the one-shot startup sweep (only used under `policy: open`). Adverts older than this are skipped on startup; the per-tick sweep still considers them up to `valid_until_ms` | If `stun_servers` is omitted, the built-in default list above is used. If it is specified in YAML, the configured list fully overrides the defaults. diff --git a/src/config/node.rs b/src/config/node.rs index 619e098..93c7c40 100644 --- a/src/config/node.rs +++ b/src/config/node.rs @@ -353,6 +353,18 @@ pub struct NostrDiscoveryConfig { /// How often adverts are refreshed in seconds. #[serde(default = "NostrDiscoveryConfig::default_advert_refresh_secs")] pub advert_refresh_secs: u64, + /// Settle delay in seconds after Nostr discovery starts before the + /// one-shot startup sweep of cached adverts runs. Allows the relay + /// subscription backlog to populate the in-memory advert cache. + /// Only used under `policy: open`. Default: 5. + #[serde(default = "NostrDiscoveryConfig::default_startup_sweep_delay_secs")] + pub startup_sweep_delay_secs: u64, + /// Maximum age in seconds for cached adverts considered by the + /// one-shot startup sweep. Adverts whose `created_at` is older than + /// `now - startup_sweep_max_age_secs` are skipped. Only used under + /// `policy: open`. Default: 3600 (1 hour). + #[serde(default = "NostrDiscoveryConfig::default_startup_sweep_max_age_secs")] + pub startup_sweep_max_age_secs: u64, } impl Default for NostrDiscoveryConfig { @@ -378,6 +390,8 @@ impl Default for NostrDiscoveryConfig { punch_duration_ms: Self::default_punch_duration_ms(), advert_ttl_secs: Self::default_advert_ttl_secs(), advert_refresh_secs: Self::default_advert_refresh_secs(), + startup_sweep_delay_secs: Self::default_startup_sweep_delay_secs(), + startup_sweep_max_age_secs: Self::default_startup_sweep_max_age_secs(), } } } @@ -462,6 +476,14 @@ impl NostrDiscoveryConfig { fn default_advert_refresh_secs() -> u64 { 1_800 } + + fn default_startup_sweep_delay_secs() -> u64 { + 5 + } + + fn default_startup_sweep_max_age_secs() -> u64 { + 3_600 + } } /// Spanning tree (`node.tree.*`). @@ -1036,6 +1058,32 @@ mod tests { assert!((c.etx_threshold - 3.0).abs() < 1e-9); // default } + #[test] + fn test_nostr_discovery_startup_sweep_defaults() { + let c = NostrDiscoveryConfig::default(); + assert_eq!(c.startup_sweep_delay_secs, 5); + assert_eq!(c.startup_sweep_max_age_secs, 3_600); + } + + #[test] + fn test_nostr_discovery_startup_sweep_yaml_override() { + let yaml = "enabled: true\npolicy: open\nstartup_sweep_delay_secs: 10\nstartup_sweep_max_age_secs: 1800\n"; + let c: NostrDiscoveryConfig = serde_yaml::from_str(yaml).unwrap(); + assert!(c.enabled); + assert_eq!(c.policy, NostrDiscoveryPolicy::Open); + assert_eq!(c.startup_sweep_delay_secs, 10); + assert_eq!(c.startup_sweep_max_age_secs, 1_800); + } + + #[test] + fn test_nostr_discovery_startup_sweep_partial_yaml_uses_defaults() { + // Only override delay; max_age should fall back to default. + let yaml = "enabled: true\nstartup_sweep_delay_secs: 30\n"; + let c: NostrDiscoveryConfig = serde_yaml::from_str(yaml).unwrap(); + assert_eq!(c.startup_sweep_delay_secs, 30); + assert_eq!(c.startup_sweep_max_age_secs, 3_600); + } + #[cfg(windows)] #[test] fn test_default_socket_path_windows() { diff --git a/src/discovery/nostr/runtime.rs b/src/discovery/nostr/runtime.rs index 7aa1245..cba7b83 100644 --- a/src/discovery/nostr/runtime.rs +++ b/src/discovery/nostr/runtime.rs @@ -198,7 +198,7 @@ impl NostrDiscovery { pub async fn cached_open_discovery_candidates( &self, max: usize, - ) -> Vec<(String, Vec)> { + ) -> Vec<(String, Vec, u64)> { self.prune_advert_cache().await; let now = now_ms(); let cache = self.advert_cache.read().await; @@ -206,7 +206,13 @@ impl NostrDiscovery { .values() .filter(|entry| entry.author_npub != self.npub) .filter(|entry| entry.valid_until_ms > now) - .map(|entry| (entry.author_npub.clone(), entry.advert.endpoints.clone())) + .map(|entry| { + ( + entry.author_npub.clone(), + entry.advert.endpoints.clone(), + entry.created_at, + ) + }) .take(max) .collect() } diff --git a/src/node/lifecycle.rs b/src/node/lifecycle.rs index 70f1e3d..df1ba0d 100644 --- a/src/node/lifecycle.rs +++ b/src/node/lifecycle.rs @@ -426,6 +426,8 @@ impl Node { } } + self.maybe_run_startup_open_discovery_sweep(&bootstrap) + .await; self.queue_open_discovery_retries(&bootstrap).await; } @@ -574,6 +576,7 @@ impl Node { warn!(error = %err, "Failed to publish initial Nostr overlay advert"); } self.nostr_discovery = Some(runtime); + self.nostr_discovery_started_at_ms = Some(Self::now_ms()); info!("Nostr overlay discovery enabled"); } Err(err) => { @@ -1141,6 +1144,26 @@ impl Node { } async fn queue_open_discovery_retries(&mut self, bootstrap: &std::sync::Arc) { + self.run_open_discovery_sweep(bootstrap, None, "per-tick") + .await; + } + + /// Open-discovery cache sweep. Iterates the cached overlay adverts and + /// queues retries for non-configured, not-yet-connected peers. + /// + /// `max_age_secs`, if set, filters out adverts whose `created_at` is + /// older than `now - max_age_secs`. The per-tick sweep passes `None` + /// (relies on the cache's own `valid_until_ms` filter); the one-shot + /// startup sweep passes `Some(startup_sweep_max_age_secs)`. + /// + /// `caller` is a short label included in log lines so per-tick and + /// startup sweeps are distinguishable in operator-facing logs. + async fn run_open_discovery_sweep( + &mut self, + bootstrap: &std::sync::Arc, + max_age_secs: Option, + caller: &'static str, + ) { if !self.config.node.discovery.nostr.enabled || self.config.node.discovery.nostr.policy != crate::config::NostrDiscoveryPolicy::Open { @@ -1154,28 +1177,63 @@ impl Node { .map(|peer| peer.npub.clone()) .collect::>(); let now_ms = Self::now_ms(); + let now_secs = now_ms / 1000; let mut enqueue_budget = self.open_discovery_enqueue_budget(&configured_npubs); if enqueue_budget == 0 { + debug!( + caller = %caller, + "open-discovery sweep: enqueue budget is 0, skipping" + ); return; } - for (npub, endpoints) in bootstrap.cached_open_discovery_candidates(64).await { + let candidates = bootstrap.cached_open_discovery_candidates(64).await; + let cached_count = candidates.len(); + let mut enqueued = 0usize; + let mut skipped_age = 0usize; + let mut skipped_configured = 0usize; + let mut skipped_self = 0usize; + let mut skipped_connected = 0usize; + let mut skipped_retry_pending = 0usize; + let mut skipped_connecting = 0usize; + let mut skipped_no_endpoints = 0usize; + let mut skipped_invalid_npub = 0usize; + + for (npub, endpoints, created_at_secs) in candidates { if enqueue_budget == 0 { break; } + + if let Some(max_age) = max_age_secs + && now_secs.saturating_sub(created_at_secs) > max_age + { + skipped_age = skipped_age.saturating_add(1); + continue; + } + if configured_npubs.contains(&npub) { + skipped_configured = skipped_configured.saturating_add(1); continue; } let peer_identity = match PeerIdentity::from_npub(&npub) { Ok(identity) => identity, - Err(_) => continue, + Err(_) => { + skipped_invalid_npub = skipped_invalid_npub.saturating_add(1); + continue; + } }; let node_addr = *peer_identity.node_addr(); - if node_addr == *self.identity.node_addr() || self.peers.contains_key(&node_addr) { + if node_addr == *self.identity.node_addr() { + skipped_self = skipped_self.saturating_add(1); + continue; + } + if self.peers.contains_key(&node_addr) { + skipped_connected = skipped_connected.saturating_add(1); continue; } if self.retry_pending.contains_key(&node_addr) { + skipped_retry_pending = skipped_retry_pending.saturating_add(1); continue; } let connecting = self.connections.values().any(|conn| { @@ -1184,6 +1242,7 @@ impl Node { .unwrap_or(false) }); if connecting { + skipped_connecting = skipped_connecting.saturating_add(1); continue; } @@ -1203,6 +1262,7 @@ impl Node { priority = priority.saturating_add(1); } if addresses.is_empty() { + skipped_no_endpoints = skipped_no_endpoints.saturating_add(1); continue; } @@ -1223,8 +1283,87 @@ impl Node { state.retry_after_ms = now_ms; state.expires_at_ms = Some(self.open_discovery_retry_expires_at_ms(now_ms)); self.retry_pending.insert(node_addr, state); + info!( + caller = %caller, + peer = %peer_identity.short_npub(), + advert_age_secs = now_secs.saturating_sub(created_at_secs), + "open-discovery sweep: queued retry for cached advert" + ); enqueue_budget = enqueue_budget.saturating_sub(1); + enqueued = enqueued.saturating_add(1); } + + // Always log a one-line summary on the startup sweep so operators + // can verify it ran. Per-tick sweeps are noisier; only summarize + // when something happened. + let total_skipped = skipped_age + + skipped_configured + + skipped_self + + skipped_connected + + skipped_retry_pending + + skipped_connecting + + skipped_no_endpoints + + skipped_invalid_npub; + let should_summarize = caller == "startup" || enqueued > 0; + if should_summarize { + info!( + caller = %caller, + cached = cached_count, + queued = enqueued, + skipped_age = skipped_age, + skipped_configured = skipped_configured, + skipped_self = skipped_self, + skipped_connected = skipped_connected, + skipped_retry_pending = skipped_retry_pending, + skipped_connecting = skipped_connecting, + skipped_no_endpoints = skipped_no_endpoints, + skipped_invalid_npub = skipped_invalid_npub, + skipped_total = total_skipped, + "open-discovery sweep complete" + ); + } + } + + /// One-shot startup sweep: runs once after the configured settle + /// delay, iterating the cached overlay adverts and queueing retries + /// for any peer with a recent enough advert that we haven't already + /// configured statically or established a link to. + /// + /// Gated identically to [`run_open_discovery_sweep`]: requires + /// `node.discovery.nostr.enabled` and `policy == open`. + async fn maybe_run_startup_open_discovery_sweep( + &mut self, + bootstrap: &std::sync::Arc, + ) { + if self.startup_open_discovery_sweep_done { + return; + } + if !self.config.node.discovery.nostr.enabled + || self.config.node.discovery.nostr.policy != crate::config::NostrDiscoveryPolicy::Open + { + // Mark done so we don't keep re-checking on every tick. + self.startup_open_discovery_sweep_done = true; + return; + } + let Some(started_at_ms) = self.nostr_discovery_started_at_ms else { + return; + }; + let now_ms = Self::now_ms(); + let delay_ms = self + .config + .node + .discovery + .nostr + .startup_sweep_delay_secs + .saturating_mul(1000); + if now_ms < started_at_ms.saturating_add(delay_ms) { + return; + } + + let max_age_secs = self.config.node.discovery.nostr.startup_sweep_max_age_secs; + self.run_open_discovery_sweep(bootstrap, Some(max_age_secs), "startup") + .await; + self.startup_open_discovery_sweep_done = true; } fn available_outbound_slots(&self) -> usize { diff --git a/src/node/mod.rs b/src/node/mod.rs index e20f0f0..4da248d 100644 --- a/src/node/mod.rs +++ b/src/node/mod.rs @@ -432,6 +432,15 @@ pub struct Node { /// Optional Nostr/STUN overlay discovery coordinator for `udp:nat` peers. nostr_discovery: Option>, + /// Wall-clock ms when Nostr discovery successfully started, used to + /// schedule the one-shot startup advert sweep after a settle delay. + /// `None` until discovery comes up; remains `None` if discovery is + /// disabled or failed to start. + nostr_discovery_started_at_ms: Option, + /// Whether the one-shot startup advert sweep has run. Set to true + /// after the first sweep fires (under `policy: open`); thereafter + /// only the per-tick `queue_open_discovery_retries` continues. + startup_open_discovery_sweep_done: bool, /// Per-peer UDP transports adopted from NAT traversal handoff. bootstrap_transports: HashSet, @@ -596,6 +605,8 @@ impl Node { pending_connects: Vec::new(), retry_pending: HashMap::new(), nostr_discovery: None, + nostr_discovery_started_at_ms: None, + startup_open_discovery_sweep_done: false, bootstrap_transports: HashSet::new(), last_parent_reeval: None, last_congestion_log: None, @@ -723,6 +734,8 @@ impl Node { pending_connects: Vec::new(), retry_pending: HashMap::new(), nostr_discovery: None, + nostr_discovery_started_at_ms: None, + startup_open_discovery_sweep_done: false, bootstrap_transports: HashSet::new(), last_parent_reeval: None, last_congestion_log: None, From a41f80a7766654a9212c4666356eb4db2a1401e2 Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Sat, 2 May 2026 01:26:54 +0000 Subject: [PATCH 3/5] Tighten clippy gate to --all-targets --all-features and clean up The local ci-local.sh and the GitHub CI clippy invocations both used `cargo clippy --all -- -D warnings`, which only checks lib + bin targets. Test code, integration tests, and benches were not lint-gated. Three pre-existing clippy errors lurked in test modules as a result (two field_reassign_with_default in config tests, one items_after_test_module in stun.rs). Tighten both invocations to `cargo clippy --all-targets --all-features -- -D warnings` so the gate covers everything cargo can build, and fix the three exposed errors: - src/config/mod.rs: rewrite two test-only `Config::default()` + field-reassign sites to struct-update syntax. - src/discovery/nostr/stun.rs: move helper `random_txn_id` above the `#[cfg(test)] mod tests` block. Also adds a dedicated Clippy job to the GitHub CI workflow so the strict gate runs on every PR (the workflow had no clippy job before; clippy ran only via testing/ci-local.sh on operator machines). No behavior changes; lint hygiene + CI hardening only. --- .github/workflows/ci.yml | 22 ++++++++++++++++++++++ src/config/mod.rs | 22 +++++++++++++--------- src/discovery/nostr/stun.rs | 16 ++++++++-------- testing/ci-local.sh | 4 ++-- 4 files changed, 45 insertions(+), 19 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4decfe9..2d27397 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -36,6 +36,28 @@ jobs: components: rustfmt - run: cargo fmt --check + clippy: + name: Clippy + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - name: Install system dependencies + run: sudo apt-get update && sudo apt-get install -y libdbus-1-dev + - uses: dtolnay/rust-toolchain@stable + with: + components: clippy + - name: Cache Cargo registry + build + uses: actions/cache@v4 + with: + path: | + ~/.cargo/registry + ~/.cargo/git + target + key: ${{ runner.os }}-cargo-clippy-${{ hashFiles('**/Cargo.lock') }} + restore-keys: | + ${{ runner.os }}-cargo- + - run: cargo clippy --all-targets --all-features -- -D warnings + build: name: Build (${{ matrix.os }}) runs-on: ${{ matrix.os }} diff --git a/src/config/mod.rs b/src/config/mod.rs index 60925ea..e8e6b6f 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -1293,12 +1293,14 @@ peers: #[test] fn test_validate_peer_via_nostr_requires_nostr_enabled() { - let mut config = Config::default(); - config.peers = vec![PeerConfig { - npub: "npub1peer".to_string(), - via_nostr: true, + let mut config = Config { + peers: vec![PeerConfig { + npub: "npub1peer".to_string(), + via_nostr: true, + ..Default::default() + }], ..Default::default() - }]; + }; config.node.discovery.nostr.enabled = false; let err = config.validate().expect_err("validation should fail"); @@ -1308,11 +1310,13 @@ peers: #[test] fn test_validate_peer_addresses_required_unless_via_nostr() { // Empty addresses + via_nostr=false → error. - let mut config = Config::default(); - config.peers = vec![PeerConfig { - npub: "npub1peer".to_string(), + let mut config = Config { + peers: vec![PeerConfig { + npub: "npub1peer".to_string(), + ..Default::default() + }], ..Default::default() - }]; + }; let err = config.validate().expect_err("validation should fail"); assert!(err.to_string().contains("at least one address")); diff --git a/src/discovery/nostr/stun.rs b/src/discovery/nostr/stun.rs index b4fcd89..83f1045 100644 --- a/src/discovery/nostr/stun.rs +++ b/src/discovery/nostr/stun.rs @@ -344,6 +344,14 @@ fn push_ip(addresses: &mut Vec, ip: IpAddr) { } } +fn random_txn_id() -> [u8; 12] { + let mut txn_id = [0u8; 12]; + for byte in &mut txn_id { + *byte = rand::random::(); + } + txn_id +} + #[cfg(test)] mod tests { use super::is_private_overlay_candidate_ip; @@ -381,11 +389,3 @@ mod tests { ))); } } - -fn random_txn_id() -> [u8; 12] { - let mut txn_id = [0u8; 12]; - for byte in &mut txn_id { - *byte = rand::random::(); - } - txn_id -} diff --git a/testing/ci-local.sh b/testing/ci-local.sh index c1c9927..47d3b37 100755 --- a/testing/ci-local.sh +++ b/testing/ci-local.sh @@ -193,8 +193,8 @@ run_build() { return 1 fi - info "cargo clippy --all -- -D warnings" - if cargo clippy --all -- -D warnings 2>&1; then + info "cargo clippy --all-targets --all-features -- -D warnings" + if cargo clippy --all-targets --all-features -- -D warnings 2>&1; then record "clippy" 0 else record "clippy" 1 From 8448e3851053f42e99ba998ce40e682dbb0b8582 Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Sat, 2 May 2026 01:27:15 +0000 Subject: [PATCH 4/5] Make Node::transport_mtu() deterministic across restarts (TCP black hole fix) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Default-config TCP flows between fips peers were stalling completely (cwnd-pinned, 0 bps for 10s+) on a non-trivial fraction of restarts. Reproducible with iperf3 between any two peers. Root cause: `Node::transport_mtu()` iterated `self.transports.values()` (HashMap with default RandomState hasher) and returned `handle.mtu()` of the first one whose `is_operational()` returned true. Two stacked sources of non-determinism stacked on each other: HashMap iteration order is randomized per-process via RandomState, and async transport `.start()` completion order races each daemon restart. The returned value drives the TCP MSS clamp ceiling computed once at TUN init (src/upper/tun.rs:501-524) and stored as immutable max_mss in the reader/writer thread state. When the picker landed on a transport with MTU > 1357 (any non-UDP-1280 in the standard fleet defaults), `max_mss > 1220` (kernel's natural fips0-MTU-derived MSS), the daemon's clamp was silently a no-op, and the kernel emitted 1220-byte segments. Those wrap into 1280-byte IPv6 → 1357-byte fips datagrams that exceed UDP-1280 transports at any forwarding hop, causing silent drops with no PTB feedback to the kernel TCP stack. Fix: return min across operational transports instead of first-iterated. With UDP-1280 in the configured set (the common case), `transport_mtu = 1280`, `max_mss = 1143 < 1220`, daemon's clamp engages, MSS=1143 reaches the wire, packets fit, throughput recovers. Empirical green light from a single-UDP-config end-to-end test: iperf3-without-`-M` recovered to ~21 Mbps with no operator-side nft TCPMSS rules. Adds three unit tests: - transport_mtu_returns_min_across_operational: pin selection to smallest MTU when multiple operational transports differ. - transport_mtu_fallback_when_no_operational_transports: 1280 fallback. - transport_mtu_min_with_single_operational: trivial single-transport case. The `effective_ipv6_mtu` field reported by `fipsctl show status` was also racy (consequence of the same bug); fixed by this change as a side effect. --- src/node/mod.rs | 33 ++++++++++++------ src/node/tests/unit.rs | 79 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 102 insertions(+), 10 deletions(-) diff --git a/src/node/mod.rs b/src/node/mod.rs index 4da248d..34c5f12 100644 --- a/src/node/mod.rs +++ b/src/node/mod.rs @@ -1024,18 +1024,31 @@ impl Node { crate::upper::icmp::effective_ipv6_mtu(self.transport_mtu()) } - /// Get the transport MTU for a specific transport. + /// Get the transport MTU governing the global TUN-boundary MSS clamp. /// - /// When called without a specific transport context, returns the MTU - /// of the first operational transport, or 1280 (IPv6 minimum) as - /// fallback. This is used for initial TUN configuration where a - /// specific transport isn't yet known. + /// Returns the **minimum** MTU across all operational transports, or + /// 1280 (IPv6 minimum) as fallback. Used for initial TUN configuration + /// where a specific egress transport isn't yet known: the resulting + /// `effective_ipv6_mtu` (transport_mtu - 77) and `max_mss` + /// (effective_mtu - 60) form a conservative ceiling that fits ANY + /// configured-transport's egress, eliminating PMTU-D black holes that + /// would otherwise occur when a flow's actual egress is smaller than + /// the clamp ceiling assumed at TUN init. + /// + /// Returning the smallest (rather than the first-iterated, which used + /// to vary across HashMap iteration order + async-startup race) makes + /// the clamp deterministic across daemon restarts. + /// + /// See `ISSUE-2026-0011` for the empirical investigation. pub fn transport_mtu(&self) -> u16 { - // Prefer the MTU from the first operational transport - for handle in self.transports.values() { - if handle.is_operational() { - return handle.mtu(); - } + let min_operational = self + .transports + .values() + .filter(|h| h.is_operational()) + .map(|h| h.mtu()) + .min(); + if let Some(mtu) = min_operational { + return mtu; } // Fallback to config: try UDP first, then Ethernet if let Some((_, cfg)) = self.config.transports.udp.iter().next() { diff --git a/src/node/tests/unit.rs b/src/node/tests/unit.rs index 69a839e..ce90b2a 100644 --- a/src/node/tests/unit.rs +++ b/src/node/tests/unit.rs @@ -951,3 +951,82 @@ fn test_promote_clears_retry_pending() { "retry_pending should be cleared on successful promotion" ); } + +// ============================================================================ +// transport_mtu() — ISSUE-2026-0011 regression coverage +// ============================================================================ + +/// Helper: spawn a UdpTransport with the given mtu, started and operational. +async fn make_udp_transport_with_mtu(id: u32, mtu: u16) -> TransportHandle { + let (packet_tx, _packet_rx) = packet_channel(64); + let transport_id = TransportId::new(id); + let mut udp = UdpTransport::new( + transport_id, + Some(format!("udp{}", id)), + crate::config::UdpConfig { + bind_addr: Some("127.0.0.1:0".to_string()), + mtu: Some(mtu), + ..Default::default() + }, + packet_tx, + ); + udp.start_async().await.unwrap(); + TransportHandle::Udp(udp) +} + +#[tokio::test] +async fn test_transport_mtu_returns_min_across_operational() { + // Multiple operational transports with varied MTUs. The picker must + // return the smallest, deterministically, regardless of HashMap + // iteration order. This is the core ISSUE-2026-0011 regression test. + let mut node = make_node(); + let (packet_tx, packet_rx) = packet_channel(64); + node.packet_tx = Some(packet_tx); + node.packet_rx = Some(packet_rx); + + let udp1 = make_udp_transport_with_mtu(1, 1497).await; + let udp2 = make_udp_transport_with_mtu(2, 1280).await; + let udp3 = make_udp_transport_with_mtu(3, 1400).await; + + node.transports.insert(TransportId::new(1), udp1); + node.transports.insert(TransportId::new(2), udp2); + node.transports.insert(TransportId::new(3), udp3); + + // Expect the smallest (UDP-1280), not whichever HashMap iterates first. + assert_eq!(node.transport_mtu(), 1280); + + // effective_ipv6_mtu = 1280 - 77 = 1203, max_mss = 1203 - 60 = 1143 + // (verifies the downstream clamp value). + assert_eq!(node.effective_ipv6_mtu(), 1203); + + for transport in node.transports.values_mut() { + transport.stop().await.ok(); + } +} + +#[tokio::test] +async fn test_transport_mtu_fallback_when_no_operational_transports() { + // No transports configured at all → falls back to 1280 (IPv6 minimum). + let node = make_node(); + assert_eq!(node.transport_mtu(), 1280); +} + +#[tokio::test] +async fn test_transport_mtu_min_with_single_operational() { + // Single transport: trivially returns its MTU. Pins the picker doesn't + // accidentally drop down to a smaller fallback when one transport is + // operational. + let mut node = make_node(); + let (packet_tx, packet_rx) = packet_channel(64); + node.packet_tx = Some(packet_tx); + node.packet_rx = Some(packet_rx); + + let udp = make_udp_transport_with_mtu(1, 1452).await; + node.transports.insert(TransportId::new(1), udp); + + assert_eq!(node.transport_mtu(), 1452); + + for transport in node.transports.values_mut() { + transport.stop().await.ok(); + } +} From da5d23ccb7976a25feaa959db8a1a0a738890d37 Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Sat, 2 May 2026 01:39:52 +0000 Subject: [PATCH 5/5] Document nostr-nat ephemeral UDP transport MTU choice Adopted ephemeral UDP transports created by adopt_established_traversal() default to UdpConfig::default() (MTU=1280, IPv6 minimum) when the bootstrap runtime hands a socket without an explicit transport_config override. This is by design: NAT-traversal middlebox MTU is unpredictable and the IPv6 minimum is the only value guaranteed by spec to survive arbitrary paths. Add an explanatory comment at the call site so future readers find the rationale without spelunking through ISSUE-2026-0013, and so any future change to the inheritance behavior is a deliberate decision rather than an accidental refactor. No behavior change. --- src/node/lifecycle.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/node/lifecycle.rs b/src/node/lifecycle.rs index df1ba0d..5d1d7b4 100644 --- a/src/node/lifecycle.rs +++ b/src/node/lifecycle.rs @@ -1689,6 +1689,13 @@ impl Node { self.register_identity(peer_node_addr, peer_identity.pubkey_full()); let transport_id = self.allocate_transport_id(); + // Adopted ephemeral UDP transports use UdpConfig::default() when the + // bootstrap runtime doesn't pass an override. Default MTU resolves to + // 1280 (IPv6 minimum), which is the only value guaranteed to survive + // arbitrary NAT-traversal middlebox paths. Inheriting from the named + // [transports.udp] config (Option 3 in ISSUE-2026-0013) would track + // operator config more closely but risks regressions on hostile paths; + // accepted as-is until a concrete use case justifies the change. let mut transport = crate::transport::udp::UdpTransport::new( transport_id, traversal.transport_name.clone(),