fix(peering): stop re-dialling an active peer on an alternate path

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

@@ -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

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,

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!(