diff --git a/src/discovery/nostr/runtime.rs b/src/discovery/nostr/runtime.rs index 1633bc5..f1bb485 100644 --- a/src/discovery/nostr/runtime.rs +++ b/src/discovery/nostr/runtime.rs @@ -32,6 +32,7 @@ use super::types::{ }; use crate::config::{NostrDiscoveryConfig, PeerConfig}; use crate::discovery::EstablishedTraversal; +use crate::{NodeAddr, PeerIdentity}; const ADVERT_CACHE_STALE_GRACE_MULTIPLIER: u64 = 2; @@ -50,6 +51,42 @@ fn short_id(id: &str) -> String { } } +/// Decide whether an incoming-offer responder session should be suppressed +/// in favour of our own already-running outbound initiator session. +/// +/// Two peers that each have the other as `auto_connect` simultaneously run an +/// initiator traversal *and* a responder traversal for the same peer, binding a +/// separate UDP socket per session. Each node then emits two +/// `BootstrapEvent::Established` events and `adopt_established_traversal` keeps +/// only the first on a non-deterministic race; when the two nodes' independent +/// races resolve to mismatched sessions, each side's Noise msg1 lands on a peer +/// port the peer already stopped draining and both handshakes stall (root cause +/// of ISSUE-2026-0031). +/// +/// To collapse the four-socket dance to a single, guaranteed-matching socket +/// pair, both nodes deterministically keep the session **initiated by the +/// smaller `NodeAddr`** — reusing the project's existing NodeAddr tie-breaker +/// convention (`cross_connection_winner`, the rekey dual-init resolution, and +/// the dual-cross-init adopt path in `lifecycle.rs`). +/// +/// This is evaluated on the responder path, where the session being handled is +/// *peer-initiated*. It returns `true` (suppress this responder session) only +/// when genuine duplication exists — i.e. we also have an in-flight outbound +/// initiator for this same peer (`have_active_initiator`) — and our own +/// initiator session is the preferred one (`our_addr < peer_addr`). When there +/// is no co-active initiator (the asymmetric / one-sided `auto_connect` case, +/// where only one session exists at all) it never suppresses, so connectivity +/// is preserved. The `our_addr == peer_addr` case (self / loopback) and any +/// caller that cannot derive a peer `NodeAddr` likewise fall through to "do not +/// suppress". +pub(super) fn suppress_responder_for_own_initiator( + our_addr: &NodeAddr, + peer_addr: &NodeAddr, + have_active_initiator: bool, +) -> bool { + have_active_initiator && our_addr < peer_addr +} + fn endpoint_summary(endpoints: &[OverlayEndpointAdvert]) -> String { endpoints .iter() @@ -1224,6 +1261,45 @@ impl NostrDiscovery { "traversal: offer accepted within clock-skew tolerance" ); } + // Collapse the dual-`auto_connect` four-socket dance to a single + // session. When we also have an in-flight outbound initiator for this + // same peer (genuine symmetric duplication), both nodes deterministically + // keep the session initiated by the smaller NodeAddr. If our own + // initiator session is the preferred one, decline to answer this offer: + // not answering lets the peer's redundant initiator time out, so only the + // single matching socket pair survives on both sides. Asymmetric / + // one-sided `auto_connect` (no co-active initiator) is never suppressed, + // preserving connectivity. See `suppress_responder_for_own_initiator`. + if self.active_initiators.lock().await.contains(&sender_npub) { + match ( + PeerIdentity::from_npub(&self.npub), + PeerIdentity::from_npub(&sender_npub), + ) { + (Ok(ours), Ok(theirs)) => { + if suppress_responder_for_own_initiator( + ours.node_addr(), + theirs.node_addr(), + true, + ) { + debug!( + peer = %peer_short, + session = %short_id(&offer.session_id), + "traversal: responder session suppressed, our outbound initiator wins (smaller addr)" + ); + return Ok(()); + } + } + _ => { + // Could not derive a NodeAddr for one side; fall through and + // answer rather than risk suppressing the only session. + trace!( + peer = %peer_short, + "traversal: could not derive NodeAddr for dedup, answering offer" + ); + } + } + } + self.mark_session_seen(&offer.session_id).await?; let base_socket = std::net::UdpSocket::bind(("0.0.0.0", 0))?; diff --git a/src/discovery/nostr/tests.rs b/src/discovery/nostr/tests.rs index d28201c..355666f 100644 --- a/src/discovery/nostr/tests.rs +++ b/src/discovery/nostr/tests.rs @@ -1,6 +1,6 @@ use nostr::prelude::{EventBuilder, Kind, Tag, Timestamp}; -use super::runtime::NostrDiscovery; +use super::runtime::{NostrDiscovery, suppress_responder_for_own_initiator}; use super::signal::{ FreshnessOutcome, build_signal_event, create_traversal_answer, create_traversal_offer, estimate_clock_skew, validate_offer_freshness, validate_traversal_answer_for_offer, @@ -14,6 +14,7 @@ use super::{ ADVERT_IDENTIFIER, ADVERT_KIND, ADVERT_VERSION, OverlayAdvert, OverlayEndpointAdvert, OverlayTransportKind, PunchHint, PunchPacketKind, TraversalAddress, }; +use crate::NodeAddr; #[derive(Clone, Copy, PartialEq, Eq)] enum NatType { @@ -628,3 +629,44 @@ async fn signal_events_use_current_timestamps() { assert!(created_at >= before); assert!(created_at <= after); } + +fn node_addr(first_byte: u8) -> NodeAddr { + let mut bytes = [0u8; 16]; + bytes[0] = first_byte; + NodeAddr::from_bytes(bytes) +} + +#[test] +fn responder_suppression_election() { + let smaller = node_addr(0x01); + let larger = node_addr(0x02); + + // Symmetric dual-auto_connect (co-active initiator on both sides): + // both nodes must keep the session initiated by the smaller NodeAddr. + + // Smaller-addr node handling the larger node's offer: our own outbound + // initiator (smaller) is preferred, so suppress this responder session. + assert!(suppress_responder_for_own_initiator( + &smaller, &larger, true + )); + + // Larger-addr node handling the smaller node's offer: the smaller node's + // session is preferred, so do NOT suppress — answer it. + assert!(!suppress_responder_for_own_initiator( + &larger, &smaller, true + )); + + // Asymmetric / one-sided auto_connect: no co-active initiator means only + // one session exists; never suppress, regardless of address ordering. + assert!(!suppress_responder_for_own_initiator( + &smaller, &larger, false + )); + assert!(!suppress_responder_for_own_initiator( + &larger, &smaller, false + )); + + // Self / loopback (equal addresses): never suppress. + assert!(!suppress_responder_for_own_initiator( + &smaller, &smaller, true + )); +}