daemon: blackholed peers stay on the relay; the next packet no longer overwrites a cached peer key

CHANGELOG.md

@@ -471,6 +471,25 @@ Detailed per-release notes are on the
 - **`pilotctl send-message` fails when the receiver does not acknowledge the
   message.** Every receiver answers a stored message with an ACK; with none,
   the command used to exit 0.
+- **A peer whose direct path is dead stays on the relay.** After the
+  blackhole heuristic moved a peer to the relay, the relay probe loop put
+  it back on the direct path within 15 s: to let direct win again it has to
+  unpin the peer, and it did that with a call that clears the relay flag
+  too. Traffic then went to the dead address until three more silent sends
+  tripped the heuristic again, so the daemon spent roughly 75 s of every
+  90 s sending into the void and logged `direct path silent, flipping to
+  relay` on a loop. Seen for over half an hour against a peer whose
+  registry endpoint was stale. The probe loop now removes only the pin; the
+  peer leaves the relay when direct packets from it actually arrive.
+- **The cached key of a peer is no longer overwritten by the next packet.**
+  The key-exchange handler cached a peer's Ed25519 key as a slice of the
+  socket's reused receive buffer, so the following packet replaced the
+  cached key with its own bytes. The next key exchange from that peer then
+  mismatched the cache and made the daemon look the key up in the registry
+  from the packet read loop, holding up every other packet for the length
+  of that round trip; the log line `auth key exchange: peer pubkey updated
+  from registry` (97 in one six-hour session) was this, not peers changing
+  keys. The key is now copied.
 - **`-advertise-endpoint` survives a re-registration.** When the daemon
   re-registered (registry reconnect, transport watchdog recovery) it sent
   the tunnel socket's local address instead of the advertised endpoint, so

pkg/daemon/daemon.go

@@ -6384,8 +6384,16 @@ func (d *Daemon) tryDirectUpgrade(nodeID uint32) {
 	// A prior blackhole flip may have PINNED the peer to relay; ClearRelayOnDirect
 	// will not promote a pinned peer. Unpin it — we've confirmed above it is not
 	// registry relay-only, so direct is allowed to win on its merits.
+	//
+	// Only the pin goes. The peer stays on the relay until direct packets
+	// from it actually arrive. Clearing the relay flag here as well put
+	// every blackholed peer back on its dead direct path within one
+	// RelayProbeInterval of the flip, where it stayed until the blackhole
+	// heuristic tripped again (three more silent sends): about 75 s of
+	// every 90 s spent sending into the void, for as long as the direct
+	// path was down.
 	if d.tunnels.IsRelayPinned(nodeID) {
-		d.tunnels.SetRelayPeerPinned(nodeID, false)
+		d.tunnels.UnpinRelayPeer(nodeID)
 	}
 
 	// Coordinate the punch (opens the conntrack pinhole on both NATs).

pkg/daemon/keyexchange/handle.go

@@ -32,7 +32,11 @@ func (m *Manager) HandleAuthFrame(data []byte, from *net.UDPAddr, fromRelay bool
 
 	peerNodeID := binary.BigEndian.Uint32(data[0:4])
 	peerX25519PubKey := data[4:36]
-	peerEd25519PubKey := ed25519.PublicKey(data[36:68])
+	// Copied, not sliced: data is the socket's reused receive buffer, and
+	// this key outlives the frame (peer-key cache, post-install event).
+	// A slice would be overwritten by the next packet read.
+	peerEd25519PubKey := make(ed25519.PublicKey, ed25519.PublicKeySize)
+	copy(peerEd25519PubKey, data[36:68])
 	signature := data[68:132]
 
 	// Encryption (X25519 priv) must be enabled.

pkg/daemon/keyexchange/keyexchange.go

@@ -490,9 +490,13 @@ func (m *Manager) PeerPubKeyCached(nodeID uint32) (ed25519.PublicKey, bool) {
 
 // SetPeerPubKey installs a cache entry directly. Used by handle paths
 // after verifying a packet-carried Ed25519 pubkey against the registry.
+// The key is copied, so a caller holding a slice of a packet buffer
+// cannot leave the cache pointing into memory the next read overwrites.
 func (m *Manager) SetPeerPubKey(nodeID uint32, pk ed25519.PublicKey) {
+	cp := make(ed25519.PublicKey, len(pk))
+	copy(cp, pk)
 	m.pubKeysMu.Lock()
-	m.peerPubKeys[nodeID] = pk
+	m.peerPubKeys[nodeID] = cp
 	m.pubKeysMu.Unlock()
 }
 

pkg/daemon/keyexchange/zz_peer_key_buffer_alias_test.go

@@ -0,0 +1,77 @@
+// SPDX-License-Identifier: AGPL-3.0-or-later
+
+package keyexchange_test
+
+import (
+	"bytes"
+	"net"
+	"testing"
+
+	"github.com/pilot-protocol/pilotprotocol/pkg/daemon/keyexchange"
+)
+
+// The tunnel read loop hands HandleAuthFrame a slice of its one reused
+// receive buffer, valid only until the next packet is read. The peer's
+// Ed25519 key was cached as a sub-slice of that buffer, so the next
+// packet overwrote the cached key in place. Every consumer of the cache
+// then saw garbage: the next PILA from the peer mismatched and forced a
+// registry lookup on the read loop, and the key-bound trust check
+// (Daemon.handshakeTrusts) compared the trust record against bytes of an
+// unrelated packet.
+func TestCachedPeerKeySurvivesReceiveBufferReuse(t *testing.T) {
+	t.Parallel()
+
+	a := newPeer(t, 100)
+	b := newPeer(t, 200)
+	crossWireVerifyFuncs(a, b)
+	a.mgr.SetSender(func(uint32, *net.UDPAddr, []byte) error { return nil })
+	b.mgr.SetSender(func(uint32, *net.UDPAddr, []byte) error { return nil })
+
+	bFrame := b.mgr.BuildAuthFrame()
+	if bFrame == nil {
+		t.Fatal("B.BuildAuthFrame returned nil")
+	}
+
+	// Stand-in for the socket's receive buffer: the frame is read into
+	// it, handled, and the same bytes are then reused for the next packet.
+	recvBuf := make([]byte, 1500)
+	n := copy(recvBuf, bFrame)
+	want := append([]byte(nil), recvBuf[4+36:4+68]...)
+
+	from := &net.UDPAddr{IP: net.IPv4(127, 0, 0, 1), Port: 4000}
+	if !a.mgr.HandleAuthFrame(recvBuf[4:n], from, false) {
+		t.Fatal("PILA from B rejected by A")
+	}
+
+	// The next packet lands in the same buffer.
+	for i := range recvBuf {
+		recvBuf[i] = 0xAA
+	}
+
+	got, ok := a.mgr.PeerPubKeyCached(b.id)
+	if !ok {
+		t.Fatal("no cached key for B after a verified PILA")
+	}
+	if !bytes.Equal(got, want) {
+		t.Fatalf("cached peer key changed when the receive buffer was reused:\n got %x\nwant %x", got, want)
+	}
+
+	// The post-install hook receives the key too; a retained copy must
+	// not alias the buffer either.
+	var hooked []byte
+	a.mgr.Store().Drop(b.id)
+	a.mgr.SetPostInstallHook(func(ev keyexchange.PostInstallEvent) { hooked = ev.PeerEd25519 })
+	n = copy(recvBuf, bFrame)
+	if !a.mgr.HandleAuthFrame(recvBuf[4:n], from, false) {
+		t.Fatal("second PILA from B rejected by A")
+	}
+	for i := range recvBuf {
+		recvBuf[i] = 0xBB
+	}
+	if hooked == nil {
+		t.Fatal("post-install hook did not fire")
+	}
+	if !bytes.Equal(hooked, want) {
+		t.Fatalf("key handed to the post-install hook changed when the receive buffer was reused:\n got %x\nwant %x", hooked, want)
+	}
+}

pkg/daemon/routing/relay.go

@@ -31,6 +31,18 @@ func (m *Manager) SetRelayPeerPinned(nodeID uint32, relay bool) {
 	m.mu.Unlock()
 }
 
+// UnpinRelayPeer clears the pin on a peer and leaves its relay flag as it
+// is. Traffic keeps using the relay; ClearRelayOnDirect may now move the
+// peer back to direct once DirectClearsRequired direct packets arrive.
+// SetRelayPeerPinned(id, false) is not a substitute: it clears the relay
+// flag too, which sends traffic direct before anything has shown the
+// direct path works.
+func (m *Manager) UnpinRelayPeer(nodeID uint32) {
+	m.mu.Lock()
+	delete(m.relayPinned, nodeID)
+	m.mu.Unlock()
+}
+
 // IsRelayPeer reports whether the peer is currently in relay mode.
 func (m *Manager) IsRelayPeer(nodeID uint32) bool {
 	m.mu.RLock()

pkg/daemon/tunnel.go

@@ -2176,6 +2176,12 @@ func (tm *TunnelManager) clearRelayOnDirectLocked(peerNodeID uint32, from *net.U
 	return tm.routing.ClearRelayOnDirect(peerNodeID, from)
 }
 
+// UnpinRelayPeer clears a peer's relay pin and keeps it on the relay.
+// Thin shim over routing.Manager.UnpinRelayPeer.
+func (tm *TunnelManager) UnpinRelayPeer(nodeID uint32) {
+	tm.routing.UnpinRelayPeer(nodeID)
+}
+
 // SetRelayPeerPinned is like SetRelayPeer but also marks the relay flag
 // as authoritative — ClearRelayOnDirect will never auto-flip a pinned
 // peer back to direct based on observed packet sources. Thin shim over

pkg/daemon/zz_direct_upgrade_keeps_relay_test.go

@@ -0,0 +1,130 @@
+// SPDX-License-Identifier: AGPL-3.0-or-later
+
+package daemon
+
+import (
+	"net"
+	"testing"
+	"time"
+)
+
+// A peer whose direct path went silent is flipped to the relay and
+// pinned there. relayProbeLoop then calls tryDirectUpgrade every
+// RelayProbeInterval to give direct another chance. It has to unpin the
+// peer for that, and it used SetRelayPeerPinned(id, false), which clears
+// the relay flag as well. The peer was back on its dead direct path within
+// 15 s of every flip and stayed there until three more silent sends
+// tripped the blackhole heuristic again.
+//
+// Seen live on 2026-10-01 against a peer whose registry endpoint was
+// stale: "direct path silent, flipping to relay" every ~90 s for over
+// half an hour, with no "relay→direct auto-cleared" between the flips.
+func TestDirectUpgradeUnpinsWithoutLeavingTheRelay(t *testing.T) {
+	d := New(Config{})
+	t.Cleanup(func() { d.tunnels.Close() })
+
+	const peer uint32 = 0x0002BBE4
+	peerConn := addPeerOnDaemon(t, d, peer)
+	t.Cleanup(func() { peerConn.Close() })
+
+	beacon, err := net.ListenUDP("udp", &net.UDPAddr{IP: net.ParseIP("127.0.0.1"), Port: 0})
+	if err != nil {
+		t.Fatalf("beacon listen: %v", err)
+	}
+	t.Cleanup(func() { beacon.Close() })
+	if err := d.tunnels.SetBeaconAddr(beacon.LocalAddr().String()); err != nil {
+		t.Fatalf("SetBeaconAddr: %v", err)
+	}
+
+	// The registry says the peer is directly reachable (not relay-only),
+	// so tryDirectUpgrade goes ahead without a registry round trip.
+	d.cacheResolve(peer, map[string]interface{}{
+		"real_addr": peerConn.LocalAddr().String(),
+	})
+
+	// State after a blackhole flip: on the relay, pinned.
+	d.tunnels.SetRelayPeerPinned(peer, true)
+
+	d.tryDirectUpgrade(peer)
+
+	if d.tunnels.IsRelayPinned(peer) {
+		t.Fatal("pin must be cleared so a working direct path can win")
+	}
+	if !d.tunnels.IsRelayPeer(peer) {
+		t.Fatal("peer left the relay before any direct packet arrived from it")
+	}
+
+	// The upgrade itself still works: direct packets from the peer move it
+	// back, and only they do.
+	from := peerConn.LocalAddr().(*net.UDPAddr)
+	for i := 1; i < directClearsRequired; i++ {
+		if d.tunnels.routing.ClearRelayOnDirect(peer, from) {
+			t.Fatalf("relay cleared after %d direct packets, want %d", i, directClearsRequired)
+		}
+	}
+	if !d.tunnels.routing.ClearRelayOnDirect(peer, from) {
+		t.Fatalf("relay not cleared after %d direct packets", directClearsRequired)
+	}
+	if d.tunnels.IsRelayPeer(peer) {
+		t.Fatal("peer still on the relay after the direct path was confirmed")
+	}
+}
+
+// The blackhole heuristic and the probe loop must not fight. With the
+// direct path dead, one flip keeps the peer on the relay across any
+// number of probe ticks; writes do not flip it again.
+func TestBlackholedPeerStaysOnRelayAcrossProbeTicks(t *testing.T) {
+	d := New(Config{})
+	t.Cleanup(func() { d.tunnels.Close() })
+
+	const peer uint32 = 0x0002BBE5
+	peerConn := addPeerOnDaemon(t, d, peer)
+	t.Cleanup(func() { peerConn.Close() })
+
+	beacon, err := net.ListenUDP("udp", &net.UDPAddr{IP: net.ParseIP("127.0.0.1"), Port: 0})
+	if err != nil {
+		t.Fatalf("beacon listen: %v", err)
+	}
+	t.Cleanup(func() { beacon.Close() })
+	if err := d.tunnels.SetBeaconAddr(beacon.LocalAddr().String()); err != nil {
+		t.Fatalf("SetBeaconAddr: %v", err)
+	}
+	d.cacheResolve(peer, map[string]interface{}{
+		"real_addr": peerConn.LocalAddr().String(),
+	})
+
+	// Nothing has come from the peer directly for a long time.
+	d.tunnels.routing.RecordDirectRecv(peer, time.Now().Add(-10*time.Minute))
+	addr := peerConn.LocalAddr().(*net.UDPAddr)
+	frame := []byte("PILS-not-a-real-frame")
+
+	flips := 0
+	write := func() {
+		was := d.tunnels.IsRelayPeer(peer)
+		_ = d.tunnels.writeFrame(peer, addr, frame)
+		if !was && d.tunnels.IsRelayPeer(peer) {
+			flips++
+		}
+	}
+
+	for i := 0; i < blackholeMissesRequired; i++ {
+		write()
+	}
+	if flips != 1 || !d.tunnels.IsRelayPeer(peer) {
+		t.Fatalf("setup: want one flip to the relay after %d silent sends, got %d (relay=%v)",
+			blackholeMissesRequired, flips, d.tunnels.IsRelayPeer(peer))
+	}
+
+	for tick := 0; tick < 3; tick++ {
+		d.tryDirectUpgrade(peer)
+		for i := 0; i < blackholeMissesRequired+1; i++ {
+			write()
+		}
+	}
+	if flips != 1 {
+		t.Fatalf("peer flipped to the relay %d times; the probe loop keeps putting it back on the dead direct path", flips)
+	}
+	if !d.tunnels.IsRelayPeer(peer) {
+		t.Fatal("peer is not on the relay although its direct path never answered")
+	}
+}