diff --git a/CHANGELOG.md b/CHANGELOG.md index 4a8d7fd3..68ac4cc3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -271,6 +271,25 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 teardown, which was already benign, and stops reporting a local self-clearing condition at `warn`. +- A peer reachable over two interfaces is no longer re-dialled on the path it + is not using. Beacon discovery skipped only a candidate naming the peer's + *current* path, which is the one case that could not churn anything, so the + alternate path was dialled every discovery tick; each dial that completed + promoted and displaced a healthy incumbent, tore down the session, and, when + that peer was the parent, switched parents and re-announced mesh-wide. + Measured on real hardware, seventeen dials to one peer in fifteen minutes, + alternating wifi and cable, displacing a link reporting etx 1.0 and loss 0.0. + Discovery now asks whether the link it already holds is answering rather than + which path the candidate names. Failover is unchanged: a peer that goes quiet + for longer than `node.heartbeat_interval_secs` is dialled again on every + path, alternate included. **One behaviour goes with it.** A peer held on an + adopted NAT-traversal transport that is *also* reachable by Ethernet or BLE + beacon used to drift onto the local path on the next discovery tick, and now + stays on the traversed path for as long as that path answers. Migrating it is + still done by the configured-peer refresh (a config reload, a runtime peer + update, or `fipsctl connect`), and a traversed link that goes quiet still + releases the peer to every path. + - A heartbeat whose send failed no longer counts as one that was delivered. The peer's "last heartbeat" timestamp was stamped before the send and left alone whatever came back, so a failure suppressed the next attempt for a diff --git a/src/node/lifecycle/mod.rs b/src/node/lifecycle/mod.rs index 586e3de6..7ce30858 100644 --- a/src/node/lifecycle/mod.rs +++ b/src/node/lifecycle/mod.rs @@ -810,15 +810,37 @@ impl Node { let connected = self.peers.contains_key(&node_addr); if connected { - // Active peer: skip a candidate whose path is already the - // current, still-fresh one (avoid churning a healthy link). - let transport_name = transport.transport_type().name; - let peer_addr_candidate = - PeerAddress::new(transport_name, remote_addr.to_string()); - if self.active_peer_candidate_is_fresh_enough_to_skip( - &node_addr, - std::slice::from_ref(&peer_addr_candidate), - ) { + // Active peer: skip every candidate while the link we + // already hold is live — the current path *and* any + // alternate one. + // + // Only the same-path case used to be skipped, which left + // the stated intent ("avoid churning a healthy link") + // covering exactly the case that could not churn anything. + // A peer reachable twice — the ordinary result of two + // machines sharing a LAN and a cable, since each beacons on + // both — was therefore re-dialled on its alternate path + // every discovery tick, forever. Each dial that completed + // promoted and displaced the incumbent, so the peer's link + // migrated back and forth on a fixed cadence, tearing down + // and re-establishing its session each time. Measured on + // real hardware: seventeen dials to one peer in fifteen + // minutes, alternating wifi and cable, displacing a link + // reporting `etx = 1.0` and `loss = 0.0`. + // + // When that peer is the parent — which the best path + // usually is — every migration also switched parents, + // invalidating the downstream coordinate cache and + // re-announcing to every peer. The cost of the churn was + // therefore mesh-wide while the benefit was nil: the link + // being replaced was already perfect. + // + // Failover is unaffected. Liveness is the gate, so a peer + // that stops answering goes stale within a heartbeat + // interval and every path, alternate included, is dialled + // again. What is given up is switching away from a link + // that is working, which is not a thing worth doing. + if self.active_peer_link_is_live(&node_addr) { continue; } if self.is_connecting_to_peer_on_path( @@ -3261,14 +3283,13 @@ impl Node { candidates } - pub(in crate::node) fn active_peer_candidate_is_fresh_enough_to_skip( - &self, - peer_node_addr: &NodeAddr, - candidates: &[PeerAddress], - ) -> bool { - if !self.active_peer_matches_any_candidate(peer_node_addr, candidates) { - return false; - } + /// Whether the link we already hold to this peer is answering. + /// + /// The gate on dialling an active peer at all. Phrased as liveness rather + /// than as a property of the candidate, because the candidate's path is + /// not the question: a live link should not be replaced by *any* path, + /// and a dead one should be replaced by whichever path answers. + pub(in crate::node) fn active_peer_link_is_live(&self, peer_node_addr: &NodeAddr) -> bool { !self.active_peer_needs_same_path_refresh(peer_node_addr) } @@ -3285,17 +3306,7 @@ impl Node { peer.idle_time(Self::now_ms()) > stale_after_ms } - fn active_peer_matches_any_candidate( - &self, - peer_node_addr: &NodeAddr, - candidates: &[PeerAddress], - ) -> bool { - candidates - .iter() - .any(|candidate| self.active_peer_matches_candidate(peer_node_addr, candidate)) - } - - fn active_peer_matches_candidate( + pub(in crate::node) fn active_peer_matches_candidate( &self, peer_node_addr: &NodeAddr, candidate: &PeerAddress, diff --git a/src/node/tests/unit.rs b/src/node/tests/unit.rs index 92cf8d01..930a0f4f 100644 --- a/src/node/tests/unit.rs +++ b/src/node/tests/unit.rs @@ -1255,7 +1255,7 @@ async fn test_try_peer_addresses_skips_connecting_peer() { } #[test] -fn active_peer_same_path_discovery_skips_fresh_peer() { +fn a_peer_heard_from_within_the_heartbeat_interval_has_a_live_link() { let mut node = make_node(); let peer_full = Identity::generate(); let peer_identity = PeerIdentity::from_pubkey_full(peer_full.pubkey_full()); @@ -1265,16 +1265,14 @@ fn active_peer_same_path_discovery_skips_fresh_peer() { let mut active_peer = ActivePeer::new(peer_identity, LinkId::new(7), Node::now_ms()); active_peer.set_current_addr(transport_id, current_addr.clone()); node.peers.insert(peer_node_addr, active_peer); - let candidate = crate::config::PeerAddress::new("udp", "127.0.0.1:9"); - assert!(node.active_peer_candidate_is_fresh_enough_to_skip( - &peer_node_addr, - std::slice::from_ref(&candidate), - )); + // A link heard from just now is live, so discovery must not dial this + // peer at all — on this path or on any other. + assert!(node.active_peer_link_is_live(&peer_node_addr)); } #[test] -fn active_peer_same_path_discovery_refreshes_stale_peer() { +fn a_peer_quiet_past_the_heartbeat_interval_no_longer_has_a_live_link() { let mut node = make_node(); let peer_full = Identity::generate(); let peer_identity = PeerIdentity::from_pubkey_full(peer_full.pubkey_full()); @@ -1291,12 +1289,62 @@ fn active_peer_same_path_discovery_refreshes_stale_peer() { let mut active_peer = ActivePeer::new(peer_identity, LinkId::new(7), stale_at); active_peer.set_current_addr(transport_id, current_addr.clone()); node.peers.insert(peer_node_addr, active_peer); - let candidate = crate::config::PeerAddress::new("udp", "127.0.0.1:9"); - assert!(!node.active_peer_candidate_is_fresh_enough_to_skip( - &peer_node_addr, - std::slice::from_ref(&candidate), - )); + // Gone quiet past the heartbeat interval: every path is dialable again, + // which is what keeps failover working now that a live link is never + // displaced. + assert!(!node.active_peer_link_is_live(&peer_node_addr)); +} + +/// A bootstrap-held peer is never its own configured candidate. +/// +/// `adopt_established_traversal` refuses a peer that is already connected, so +/// an adopted NAT-traversal transport is the way *on* to a traversed path and +/// not the way off. Now that beacon discovery asks only whether the link it +/// holds is answering, the configured-peer refresh is the one automatic +/// off-ramp left, and it works only because a bootstrap-held peer is refused +/// as a match for its own address: a configured `udp` address can be +/// byte-identical to the traversal's remote address and the transport kinds +/// match, so without this carve-out `has_alternative` would be false and the +/// peer could never be moved off the traversed socket at all. +#[test] +fn a_bootstrap_held_peer_is_never_its_own_configured_candidate() { + let mut node = make_node(); + let peer_full = Identity::generate(); + let peer_identity = PeerIdentity::from_pubkey_full(peer_full.pubkey_full()); + let peer_node_addr = *peer_identity.node_addr(); + let npub = peer_identity.npub(); + let transport_id = TransportId::new(1); + let current_addr = TransportAddr::from_string("203.0.113.5:41234"); + + let (packet_tx, _packet_rx) = packet_channel(8); + let udp = UdpTransport::new( + transport_id, + Some("main".to_string()), + crate::config::UdpConfig { + bind_addr: Some("127.0.0.1:0".to_string()), + ..Default::default() + }, + packet_tx, + ); + node.transports + .insert(transport_id, TransportHandle::Udp(udp)); + + let mut active_peer = ActivePeer::new(peer_identity, LinkId::new(7), Node::now_ms()); + active_peer.set_current_addr(transport_id, current_addr); + node.peers.insert(peer_node_addr, active_peer); + node.supervisor + .nostr_rendezvous + .insert_bootstrap_transport(transport_id, npub); + + assert!( + !node.active_peer_matches_candidate( + &peer_node_addr, + &crate::config::PeerAddress::new("udp", "203.0.113.5:41234") + ), + "a bootstrap-held peer must not count its own traversal address as its \ + current path, or the configured-peer refresh can never migrate it off" + ); } /// An instance-qualified candidate is the peer's *current* path only when it @@ -1338,12 +1386,11 @@ async fn an_instance_qualified_candidate_matches_only_its_own_instance() { active_peer.set_current_addr(main_id, TransportAddr::from_string("127.0.0.1:9")); node.peers.insert(peer_node_addr, active_peer); + // Path matching, which still gates the *configured-peer* refresh even + // though beacon discovery now gates on liveness alone. let matches = |transport: &str| { let candidate = crate::config::PeerAddress::new(transport, "127.0.0.1:9"); - node.active_peer_candidate_is_fresh_enough_to_skip( - &peer_node_addr, - std::slice::from_ref(&candidate), - ) + node.active_peer_matches_candidate(&peer_node_addr, &candidate) }; assert!(