mirror of
https://github.com/jmcorgan/fips.git
synced 2026-08-10 16:43:12 +00:00
nostr: suppress retraversal of cross-FMP-version peers
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.
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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.*`).
|
||||
|
||||
@@ -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<u64> {
|
||||
@@ -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
|
||||
|
||||
@@ -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<NostrPeerFailureView> {
|
||||
self.failure_state
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
@@ -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<TransportId>,
|
||||
/// 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<TransportId, String>,
|
||||
|
||||
// === 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);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user