From a62a0a6cf4634eae31515812486479231af76bf5 Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Wed, 6 May 2026 15:02:26 +0000 Subject: [PATCH] nostr: suppress retraversal of cross-FMP-version peers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Open-discovery NAT traversal succeeds at the UDP layer regardless of what FMP-protocol version the peer speaks. When the daemon discovers a peer running a different FMP version (e.g. a v0/v1 mix during a mid-rollout window, or a misconfigured peer in the same advert namespace), the punch sequence completes, the socket is adopted via `Node::adopt_established_traversal`, and we initiate an FMP handshake. The peer drops our msg1 at its own version-gate and we drop their msg1/msg2 at `Unknown FMP version, dropping`. Neither side advances the handshake. Today the bootstrap transport sits idle until the 31s stale- handshake timeout, drops, and the open-discovery sweep ~30s later fires the full STUN+offer+answer+punch sequence again — every minute, indefinitely, against peers the handshake literally cannot complete with. Add a `Node::bootstrap_transport_npubs` map populated alongside `bootstrap_transports` at adopt time. The rx loop reverse-maps the transport_id → npub on version-mismatch and bumps the discovery layer's `failure_state` to a long structural cooldown via the new `NostrDiscovery::record_protocol_mismatch` API. The next sweep skips the npub for `protocol_mismatch_cooldown_secs` (default 86400 = 24h, separate from the 30-min transient-failure `extended_cooldown_secs`). One-shot WARN per fresh observation. Repeat mismatches inside the cooldown window are silent (the failure_state method returns false when an existing comparable cooldown is already in place). The handshake/transport teardown chain is unchanged — the fix is specifically about preventing the *next* sweep cycle from re-traversing. Cleared on `cleanup_bootstrap_transport_if_unused` and on the adopt-failure rollback path so completed handshakes don't leave stale entries behind. Four new unit tests in `failure_state.rs` cover fresh-entry signaling, repeat-suppression inside the window, streak-pin behavior for `show_peers` rendering, and post-cooldown re-arming. --- CHANGELOG.md | 16 ++++ src/config/node.rs | 13 ++++ src/discovery/nostr/failure_state.rs | 107 +++++++++++++++++++++++++++ src/discovery/nostr/runtime.rs | 24 ++++++ src/node/handlers/rx_loop.rs | 28 +++++++ src/node/lifecycle.rs | 3 + src/node/mod.rs | 10 +++ 7 files changed, 201 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0db297c..8135752 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -403,6 +403,22 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 flows opened in that window kept getting clamped by the staler discovery-time value. The proactive mirror closes that gap with the same tighter-only semantics — never loosens the clamp. +- Nostr-discovered peers running an FMP-protocol version we cannot + speak no longer trigger an indefinite retraversal storm. Open- + discovery NAT-traversal succeeds at the UDP layer regardless of + protocol version, so the daemon would adopt the punched socket, + drop every incoming packet at `Unknown FMP version`, idle out + after 31s, and re-fire the full STUN-offer-answer-punch sequence + ~30s later — every minute, forever, against peers the handshake + literally cannot complete with. The rx loop now detects mismatched- + version packets arriving on adopted bootstrap transports, reverse- + maps to the originating npub, and applies a long structural + cooldown to the discovery layer's `failure_state` so the next + open-discovery sweep skips the peer until either side upgrades. + One-shot WARN per fresh observation; subsequent mismatches inside + the cooldown window are silent. New `protocol_mismatch_cooldown_secs` + config field under `node.discovery.nostr` (default 86400 = 24h), + separate from the transient-failure `extended_cooldown_secs`. - Auto-connect peers now reconnect after a graceful `Disconnect` notification from the remote side. `handle_disconnect` previously removed the peer without scheduling a reconnect, orphaning the diff --git a/src/config/node.rs b/src/config/node.rs index 9b95748..5eae6ef 100644 --- a/src/config/node.rs +++ b/src/config/node.rs @@ -388,6 +388,14 @@ pub struct NostrDiscoveryConfig { /// failure time) evicted when the cap is exceeded. Default: 4096. #[serde(default = "NostrDiscoveryConfig::default_failure_state_max_entries")] pub failure_state_max_entries: usize, + /// Cooldown applied after observing a fatal protocol mismatch on a + /// Nostr-adopted bootstrap transport (e.g. `Unknown FMP version` + /// from a peer running a different FMP-protocol version). Independent + /// of `extended_cooldown_secs` and much longer because the mismatch + /// is structural — re-traversing the peer is wasted effort until one + /// side upgrades. Default: 86400 (24 hours). + #[serde(default = "NostrDiscoveryConfig::default_protocol_mismatch_cooldown_secs")] + pub protocol_mismatch_cooldown_secs: u64, } impl Default for NostrDiscoveryConfig { @@ -419,6 +427,7 @@ impl Default for NostrDiscoveryConfig { extended_cooldown_secs: Self::default_extended_cooldown_secs(), warn_log_interval_secs: Self::default_warn_log_interval_secs(), failure_state_max_entries: Self::default_failure_state_max_entries(), + protocol_mismatch_cooldown_secs: Self::default_protocol_mismatch_cooldown_secs(), } } } @@ -527,6 +536,10 @@ impl NostrDiscoveryConfig { fn default_failure_state_max_entries() -> usize { 4_096 } + + fn default_protocol_mismatch_cooldown_secs() -> u64 { + 86_400 + } } /// Spanning tree (`node.tree.*`). diff --git a/src/discovery/nostr/failure_state.rs b/src/discovery/nostr/failure_state.rs index 57d2b0e..331b21f 100644 --- a/src/discovery/nostr/failure_state.rs +++ b/src/discovery/nostr/failure_state.rs @@ -176,6 +176,52 @@ impl FailureState { } } + /// Record a fatal protocol mismatch against `npub` and apply + /// `cooldown_ms` immediately (independent of the streak threshold). + /// + /// Returns `true` when this is a fresh mismatch entry (caller should + /// log a one-shot WARN) or `false` if a comparable mismatch cooldown + /// is already in place (caller should remain silent — repeat + /// observations of the same mismatch are uninteresting). + /// + /// Used when the rx loop sees an unhandshakable packet (e.g., + /// `Unknown FMP version`) on a Nostr-adopted bootstrap transport: + /// re-traversing the peer at the next sweep cycle is wasted effort + /// because the peer cannot accept our handshake until one side + /// upgrades. The cooldown is much longer than the transient-failure + /// `extended_cooldown_ms` because the mismatch is structural. + pub(super) fn record_protocol_mismatch( + &self, + npub: &str, + now_ms: u64, + cooldown_ms: u64, + ) -> bool { + let mut map = self.inner.lock().expect("failure-state mutex poisoned"); + let entry = map + .entry(npub.to_string()) + .or_insert_with(|| NpubFailureRecord::new(now_ms)); + // Treat the mismatch as crossing the streak threshold so other + // visibility paths (e.g. show_peers JSON) reflect the failed state. + entry.consecutive_failures = entry.consecutive_failures.max(self.threshold); + entry.last_failure_at_ms = now_ms; + + let cooldown_until = now_ms.saturating_add(cooldown_ms); + // "Fresh" means we weren't already inside a comparable cooldown + // window. Use the existing-cooldown's remaining time as the test + // so that an entry shifted forward by a few seconds doesn't keep + // re-triggering WARNs. + let already_suppressed = entry + .cooldown_until_ms + .is_some_and(|t| t > now_ms && t.saturating_sub(now_ms) >= cooldown_ms / 2); + entry.cooldown_until_ms = Some(cooldown_until); + + if map.len() > self.max_entries { + evict_oldest(&mut map, self.max_entries); + } + + !already_suppressed + } + /// Return cooldown_until_ms if the peer is currently in extended /// cooldown. pub(super) fn cooldown_until(&self, npub: &str, now_ms: u64) -> Option { @@ -294,6 +340,67 @@ mod tests { assert_eq!(rec.consecutive_failures, 0); } + #[test] + fn record_protocol_mismatch_fresh_entry_returns_true() { + let s = fs(); + // 24h cooldown + let cooldown_ms = 24 * 60 * 60 * 1000; + assert!( + s.record_protocol_mismatch("npub1mismatch", 1000, cooldown_ms), + "first mismatch must signal fresh — caller should WARN" + ); + assert_eq!( + s.cooldown_until("npub1mismatch", 2000), + Some(1000 + cooldown_ms), + "cooldown applied immediately" + ); + } + + #[test] + fn record_protocol_mismatch_repeat_inside_window_returns_false() { + let s = fs(); + let cooldown_ms = 24 * 60 * 60 * 1000; + s.record_protocol_mismatch("npub1mismatch", 1000, cooldown_ms); + // 30s later, same mismatch — caller should NOT re-WARN + assert!( + !s.record_protocol_mismatch("npub1mismatch", 31_000, cooldown_ms), + "second mismatch inside the existing cooldown must NOT signal fresh" + ); + // Cooldown extends forward. + assert_eq!( + s.cooldown_until("npub1mismatch", 32_000), + Some(31_000 + cooldown_ms), + ); + } + + #[test] + fn record_protocol_mismatch_pins_streak_at_threshold() { + let s = fs(); + s.record_protocol_mismatch("npub1mismatch", 1000, 60_000); + // Snapshot reflects the threshold pin so show_peers renders the + // entry as crossed-threshold. + let snap = s.snapshot(); + let (_, rec) = snap + .iter() + .find(|(n, _)| n == "npub1mismatch") + .expect("entry present"); + assert!(rec.consecutive_failures >= 3); + } + + #[test] + fn record_protocol_mismatch_after_old_cooldown_lapsed_signals_fresh() { + let s = fs(); + let cooldown_ms = 24 * 60 * 60 * 1000; + s.record_protocol_mismatch("npub1mismatch", 1000, cooldown_ms); + // Far in the future after cooldown elapsed: a *new* observation + // is fresh again so the operator gets a fresh WARN log. + let later = 1000 + cooldown_ms + 1; + assert!( + s.record_protocol_mismatch("npub1mismatch", later, cooldown_ms), + "after the cooldown window has elapsed, the next mismatch is fresh" + ); + } + #[test] fn size_cap_evicts_oldest_by_last_failure_at() { let s = fs(); // cap = 8 diff --git a/src/discovery/nostr/runtime.rs b/src/discovery/nostr/runtime.rs index 4a64300..3ad700c 100644 --- a/src/discovery/nostr/runtime.rs +++ b/src/discovery/nostr/runtime.rs @@ -215,6 +215,30 @@ impl NostrDiscovery { self.failure_state.cooldown_until(npub, now_ms) } + /// Record a fatal protocol mismatch (e.g. `Unknown FMP version` on a + /// Nostr-adopted bootstrap transport). Returns `true` if this is a + /// fresh observation worth a WARN log; `false` if the peer is already + /// inside a comparable mismatch cooldown. + /// + /// The cooldown is `protocol_mismatch_cooldown_secs` from config — + /// much longer than `extended_cooldown_secs` because mismatches are + /// structural (only resolves when one side upgrades) rather than + /// transient. + pub fn record_protocol_mismatch(&self, npub: &str, now_ms: u64) -> bool { + let cooldown_ms = self + .config + .protocol_mismatch_cooldown_secs + .saturating_mul(1000); + self.failure_state + .record_protocol_mismatch(npub, now_ms, cooldown_ms) + } + + /// Configured protocol-mismatch cooldown in seconds. Exposed so log + /// emitters can include the duration without re-reading config. + pub fn protocol_mismatch_cooldown_secs(&self) -> u64 { + self.config.protocol_mismatch_cooldown_secs + } + /// Snapshot of per-npub failure state for `show_peers` rendering. pub fn failure_state_snapshot(&self) -> Vec { self.failure_state diff --git a/src/node/handlers/rx_loop.rs b/src/node/handlers/rx_loop.rs index 082a234..3399b77 100644 --- a/src/node/handlers/rx_loop.rs +++ b/src/node/handlers/rx_loop.rs @@ -160,6 +160,34 @@ impl Node { transport_id = %packet.transport_id, "Unknown FMP version, dropping" ); + + // If the packet arrived on an adopted Nostr-NAT bootstrap + // transport, the originating peer is necessarily on a + // different FMP-protocol version than us — the discovery + // sweep would otherwise re-traverse them every cycle even + // though no msg1/msg2 exchange can ever succeed. Bump the + // discovery-layer cooldown to the long protocol-mismatch + // window and emit a single WARN per fresh observation. + if self.bootstrap_transports.contains(&packet.transport_id) + && let Some(npub) = self + .bootstrap_transport_npubs + .get(&packet.transport_id) + .cloned() + && let Some(handle) = self.nostr_discovery_handle() + { + let now_ms = Self::now_ms(); + let cooldown_secs = handle.protocol_mismatch_cooldown_secs(); + if handle.record_protocol_mismatch(&npub, now_ms) { + warn!( + peer_npub = %npub, + transport_id = %packet.transport_id, + peer_version = prefix.version, + our_version = FMP_VERSION, + cooldown_secs, + "Nostr-discovered peer speaks a different FMP version; suppressing retraversal" + ); + } + } return; } diff --git a/src/node/lifecycle.rs b/src/node/lifecycle.rs index 5d7105f..e0b654e 100644 --- a/src/node/lifecycle.rs +++ b/src/node/lifecycle.rs @@ -1852,6 +1852,8 @@ impl Node { crate::transport::TransportHandle::Udp(transport), ); self.bootstrap_transports.insert(transport_id); + self.bootstrap_transport_npubs + .insert(transport_id, traversal.peer_npub.clone()); let remote_addr = TransportAddr::from_string(&traversal.remote_addr.to_string()); if let Err(err) = self @@ -1859,6 +1861,7 @@ impl Node { .await { self.bootstrap_transports.remove(&transport_id); + self.bootstrap_transport_npubs.remove(&transport_id); if let Some(mut handle) = self.transports.remove(&transport_id) { let _ = handle.stop().await; } diff --git a/src/node/mod.rs b/src/node/mod.rs index 7744546..284db6e 100644 --- a/src/node/mod.rs +++ b/src/node/mod.rs @@ -449,6 +449,13 @@ pub struct Node { startup_open_discovery_sweep_done: bool, /// Per-peer UDP transports adopted from NAT traversal handoff. bootstrap_transports: HashSet, + /// Originating peer npub (bech32) for each adopted bootstrap + /// transport, captured at `adopt_established_traversal` time. + /// Populated alongside `bootstrap_transports`; cleared in + /// `cleanup_bootstrap_transport_if_unused`. Used by the rx loop to + /// route fatal-protocol-mismatch observations back to the + /// Nostr-discovery `failure_state` for long cooldown application. + bootstrap_transport_npubs: HashMap, // === Periodic Parent Re-evaluation === /// Timestamp of last periodic parent re-evaluation (for pacing). @@ -614,6 +621,7 @@ impl Node { nostr_discovery_started_at_ms: None, startup_open_discovery_sweep_done: false, bootstrap_transports: HashSet::new(), + bootstrap_transport_npubs: HashMap::new(), last_parent_reeval: None, last_congestion_log: None, estimated_mesh_size: None, @@ -744,6 +752,7 @@ impl Node { nostr_discovery_started_at_ms: None, startup_open_discovery_sweep_done: false, bootstrap_transports: HashSet::new(), + bootstrap_transport_npubs: HashMap::new(), last_parent_reeval: None, last_congestion_log: None, estimated_mesh_size: None, @@ -1468,6 +1477,7 @@ impl Node { ); self.bootstrap_transports.remove(&transport_id); + self.bootstrap_transport_npubs.remove(&transport_id); self.transport_drops.remove(&transport_id); self.transports.remove(&transport_id); }