From fdc127e75f374fbb38a82e95d1e2f34566823115 Mon Sep 17 00:00:00 2001 From: Arjen <18398758+Origami74@users.noreply.github.com> Date: Thu, 3 Sep 2026 09:44:37 +0100 Subject: [PATCH] fix(peering): stop re-dialling an active peer on an alternate path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A peer reachable over two interfaces was re-dialled on whichever path it was not currently using, once per 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 and tore down its session each time. That is the ordinary result of two machines sharing a LAN and a cable: each beacons on both, so each discovers the other twice. 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 the peer is the parent, which the best path usually is, every migration also switched parents, invalidated the downstream coordinate cache and re-announced to every peer, so the cost was mesh-wide while the benefit was nil. The gate already existed and already said it was for this: "skip a candidate whose path is already the current, still-fresh one (avoid churning a healthy link)". It only ever matched the *same* path, so it covered exactly the case that could not churn anything, and the alternate path — the only one that could — went straight through. Liveness is the right question, not the candidate's path: a live link should not be replaced by any path, and a dead one should be replaced by whichever answers. `active_peer_link_is_live` replaces the candidate-matching wrapper at the discovery call site accordingly. Failover is unaffected. 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 worth doing. The configured-peer refresh is deliberately untouched: it prefers alternative addresses on purpose, for NAT and multi-address peers, and that is a different question from beacon discovery re-dialling a peer already on the wire. One consequence goes with the change, named here rather than left silent. A peer held on an adopted NAT-traversal transport was re-dialled by any Ethernet or BLE beacon that named it, because such a beacon can never match that peer's current path: the addresses cannot be equal (a six-byte MAC or BLE address against an ip:port) and the transport kinds differ. A peer that was both hole-punched and locally adjacent therefore drifted onto the local path on the next discovery tick. Liveness gates that too, so it now stays on the traversed path for as long as that path answers. Nothing else repeats the migration on a timer: adopt_established_traversal refuses a peer that is already connected, so it is the way on to a bootstrap transport and not the way off. What is left is the configured-peer refresh, which keeps its bootstrap carve-out and does perform the migration, and the traversed link going quiet, which reopens every path. Upgrading a live traversed link to a local one is worth having and belongs in a change that asks for it, not in the accident this one removes. Coverage is honest but partial. The liveness predicate is unit-tested in both directions, and the path-matching those tests used as a vehicle is retargeted onto the function that still uses it. The call-site change itself is NOT covered: I restored the old same-path-only behaviour and the suite stayed green, so the tests pin the predicate rather than the decision. Driving `poll_transport_discovery` needs two bound Ethernet transports and a seeded neighbour buffer; an absent transport fails the dial anyway, so link count cannot discriminate. Verification is the live daemon that produced the measurements above. a_bootstrap_held_peer_is_never_its_own_configured_candidate pins that remaining off-ramp, which had no test anywhere: with the bootstrap carve-out in active_peer_matches_candidate deleted, the peer's own traversal address compares equal on both the address and the transport kind, has_alternative goes false, and the configured-peer refresh can no longer move it. The two predicate tests are renamed to what they assert. Neither looks at a candidate or a transport any more, so "same_path" and "discovery" named things the bodies no longer touch, and the candidate each still bound was kept alive only by a `let _ =` suppression. Both suppressions go with the bindings. Changelog entry, including the traversal consequence above. --- CHANGELOG.md | 19 ++++++++++ src/node/lifecycle/mod.rs | 67 +++++++++++++++++++-------------- src/node/tests/unit.rs | 79 +++++++++++++++++++++++++++++++-------- 3 files changed, 121 insertions(+), 44 deletions(-) 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!(