From ebf34ae712395d451eaac3c94f4c7c59d3d733ce Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Thu, 23 Jul 2026 03:23:00 +0000 Subject: [PATCH] Read the wall clock directly for Nostr traversal timestamps The traversal clock cached a Unix millisecond value and an Instant at first use, then served every later call by advancing the cached value with the monotonic elapsed time. A monotonic clock does not tick while the host is suspended, so once a machine had slept the returned value trailed real time by the sleep duration for the rest of the process lifetime, and never re-synced. Almost everything that clock feeds is an absolute timestamp. The NIP-40 expiration tags on adverts and traversal signals, and the issuedAt and expiresAt fields of offers and answers, are all computed as now plus a TTL; the freshness and cache-pruning paths compare it against a peer-authored, signed created_at. Once the host had slept longer than signal_ttl_secs, every offer was published already expired, relays dropped it, and the initiator timed out waiting for an answer with traversal broken until a restart. Read the wall clock on every call instead. This also removes a mismatch inside the traversal failure-state map, which was written here from the cached clock but written and read from the node lifecycle with the real one. Not platform-specific: monotonic clocks exclude suspended time on Linux and Windows as well, so this affected any host that suspends. A laptop is simply where a process lives long enough across a sleep to notice. The interval-shaped consumers hold up under a clock step. A forward step, which is what a resume produces, saturates the punch start delay to zero, and the attempt's own bounds are monotonic deadlines that are unaffected. A backward step lengthens that delay and can cost one punch attempt, which retries. Early eviction from the replay window cannot admit a replay under the shipped defaults, because the freshness window a replayed offer must also satisfy is strictly narrower than the replay window itself. The added test pins the contract and fires on a host that has genuinely suspended, but it is not a regression guard for this defect: nothing reachable from a unit test can simulate a suspend, so on a machine that has not slept the old implementation passes it too. Its comment says so rather than leaving a false sense of coverage. Reported in https://github.com/jmcorgan/fips/issues/128 --- CHANGELOG.md | 13 +++++++++ src/discovery/nostr/tests.rs | 39 +++++++++++++++++++++++++- src/discovery/nostr/traversal.rs | 48 +++++++++++++++++++------------- 3 files changed, 80 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d715396..b686088 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Nostr NAT traversal no longer breaks after the host suspends. The traversal + clock cached a Unix timestamp once at startup and advanced it with a + monotonic `Instant`, which does not tick while a machine is asleep, so after + a suspend the daemon's idea of the time trailed real time by the suspend + duration for the rest of the process lifetime. Every NIP-40 expiration it + computed was therefore published already in the past: relays dropped the + offers as expired, the initiator logged a signal timeout waiting for an + answer, and traversal stayed broken until the daemon was restarted. The + clock now reads the wall clock on every call. This is not macOS-specific, + though a laptop that sleeps is where it is easiest to hit; any host that + suspends or hibernates was affected. Reported in + [#128](https://github.com/jmcorgan/fips/issues/128). + - `SessionDatagram` hop-limit handling now follows IP semantics. Delivery to the addressed node is no longer TTL-gated, and a forwarder decrements before deciding rather than after, so a datagram that would leave with a TTL of zero diff --git a/src/discovery/nostr/tests.rs b/src/discovery/nostr/tests.rs index 355666f..8ec3f3a 100644 --- a/src/discovery/nostr/tests.rs +++ b/src/discovery/nostr/tests.rs @@ -7,7 +7,7 @@ use super::signal::{ }; use super::stun::{parse_stun_binding_success, parse_stun_url}; use super::traversal::{ - PunchStrategy, build_punch_packet, parse_punch_packet, plan_punch_targets, + PunchStrategy, build_punch_packet, now_ms, parse_punch_packet, plan_punch_targets, planned_remote_endpoints, session_hash, }; use super::{ @@ -670,3 +670,40 @@ fn responder_suppression_election() { &smaller, &smaller, true )); } + +#[test] +fn now_ms_tracks_the_wall_clock() { + use std::time::{SystemTime, UNIX_EPOCH}; + + fn wall_ms() -> u64 { + SystemTime::now() + .duration_since(UNIX_EPOCH) + .expect("system clock is after the Unix epoch") + .as_millis() as u64 + } + + // Bracket a sample between two independent wall-clock reads taken either + // side of it. This is the property the traversal clock has to hold for the + // NIP-40 expiration tags it computes to be in the future when published. + // + // Read this for what it is: it pins the contract (Unix epoch, milliseconds, + // tracking real time) and it fires on a host that has actually suspended, + // where the sample falls below `before` by the suspend duration. It is NOT a + // regression guard for the anchored-clock defect. Nothing reachable from a + // unit test can simulate a suspend, so on a machine that has not slept, an + // anchored implementation passes this -- deterministically when this is the + // first caller of `now_ms()` in the binary, and otherwise with a probability + // set by the fractional millisecond the anchor happened to capture. + let before = wall_ms(); + let sampled = now_ms(); + let after = wall_ms(); + + assert!( + sampled >= before, + "now_ms() is behind the wall clock: {sampled} < {before}" + ); + assert!( + sampled <= after, + "now_ms() is ahead of the wall clock: {sampled} > {after}" + ); +} diff --git a/src/discovery/nostr/traversal.rs b/src/discovery/nostr/traversal.rs index 49f875a..cbb8531 100644 --- a/src/discovery/nostr/traversal.rs +++ b/src/discovery/nostr/traversal.rs @@ -1,5 +1,5 @@ use std::net::SocketAddr; -use std::sync::{Arc, OnceLock}; +use std::sync::Arc; use std::time::{Duration, Instant, SystemTime, UNIX_EPOCH}; use tokio::net::UdpSocket; @@ -197,25 +197,35 @@ pub(super) fn nonce() -> String { format!("{}-{:016x}", now_ms(), rand::random::()) } +/// Current Unix time in milliseconds, read from the wall clock on every call. +/// +/// This deliberately does not cache a start-of-process anchor and advance it +/// with a monotonic `Instant`. A monotonic clock does not advance while the host +/// is suspended, so an anchored value trails real time by the suspend duration +/// for the remaining life of the process. Every expiry computed from it is then +/// published already in the past, the relay drops the event as expired, and +/// traversal signalling fails until the daemon is restarted. +/// +/// About half the consumers publish or serialize the value as an absolute +/// timestamp: the NIP-40 expiration tags on adverts and traversal signals, and +/// the `issuedAt`/`expiresAt` fields of offers and answers. The rest compare it +/// against timestamps on the same basis, including the peer-authored, signed +/// `created_at` of a received advert, so they need it to track real time too. +/// +/// The interval-shaped consumers survive a step in the wall clock. A forward +/// step, which is what a resume produces, saturates the punch start delay to +/// zero so punching begins immediately; the attempt's own bounds are monotonic +/// `Instant` deadlines, so its length is unaffected. A backward step lengthens +/// that delay instead and can cost a single punch attempt, which retries. Early +/// eviction from the replay window cannot admit a replay under the shipped +/// defaults, because the freshness window a replayed offer would also have to +/// satisfy (`signal_ttl_secs` plus `FRESHNESS_SKEW_TOLERANCE_MS`, 180s) is +/// strictly narrower than the replay window itself (`replay_window_secs`, 300s). pub(super) fn now_ms() -> u64 { - struct ClockAnchor { - started_at: Instant, - started_unix_ms: u64, - } - - static ANCHOR: OnceLock = OnceLock::new(); - - let anchor = ANCHOR.get_or_init(|| ClockAnchor { - started_at: Instant::now(), - started_unix_ms: SystemTime::now() - .duration_since(UNIX_EPOCH) - .map(|duration| duration.as_millis() as u64) - .unwrap_or(0), - }); - - anchor - .started_unix_ms - .saturating_add(anchor.started_at.elapsed().as_millis() as u64) + SystemTime::now() + .duration_since(UNIX_EPOCH) + .map(|duration| duration.as_millis() as u64) + .unwrap_or(0) } pub(super) fn session_hash(session_id: &str) -> [u8; 16] {