fix(nat): 8 NAT traversal fixes proven by Mac upload to NAT testnet

[grumbach]

Proven by live testnet upload from Mac

3/3 uploads from a macOS client (31s clock skew) succeeded to a 14-node NAT testnet across 4 regions. Without these fixes: 0/3.

Upload 1 (cold): 61s SUCCESS
Upload 2 (warm): 30s SUCCESS
Upload 3 (warm): 33s SUCCESS

The 8 fixes

1. send_ack_timeout 500ms to 5s
2. Accept loop dedup keeps newer connection (root cause of feat(nat): rotate hole-punch coordinators with capped per-attempt timeout #43 regression)
3. Reachability model: has_public_ip to scope-aware peer-verified
4. Review feedback: double-count fix, relay capability, rustdoc
5. Relay fallback rotates through all candidates
6. Relay session reuse returns socket
7. Relay session periodic cleanup
8. Quality-aware coordinator selection (RTT-weighted, load-balanced)

Incremental validation

Round 1 (send_ack): 3/3 at 44s warm
Round 2 (+dedup): 3/3 at 40s warm
Round 3 (+reachability): 3/3 at 67s warm
Round 4 (+relay+coordinator): 3/3 at 37s warm
Mac upload (all fixes): 3/3 at 30-33s warm

Total: 15/15 uploads succeeded across 5 test rounds.

Test plan

• clippy clean, fmt clean, 1493 tests pass
• 4 rounds VPS testnet: 12/12
• Mac-to-NAT-testnet: 3/3 (the real user scenario)

Greptile Summary

This PR bundles 8 targeted NAT traversal fixes validated by 15/15 successful uploads across 5 test rounds including a Mac-to-NAT-testnet scenario. The changes address a mix of timeout tuning, accept-loop deduplication regression (#43), reachability model correctness, relay fallback rotation/reuse, and coordinator quality selection. All fixes are well-scoped and supported by new tests.

• The load-balancing half of fix chore(deps): bump system-configuration from 0.6.1 to 0.7.0 #8 (quality-aware coordinator selection) is non-functional: coordination_count is initialized to 0 and never incremented anywhere in the codebase, so the load score is always 1.0 / (0+1) = 1.0 for every eligible coordinator. RTT weighting still works correctly, but the advertised load-balancing is a no-op until an increment call is wired in.

Confidence Score: 4/5

Safe to merge — all fixes are well-validated by testnet data and the single open issue (non-functional load balancing) does not affect correctness.

One P1-adjacent finding: the load-balancing component of the new quality-aware coordinator selector is silently inert because coordination_count is never incremented. This means coordinator rotation degrades to RTT-only selection rather than the RTT+load selection described in the PR. The system still works correctly with RTT weighting, but the load-balancing half of fix #8 is incomplete and could mislead future maintainers. All other fixes are correct, well-tested, and proven by live testnet data.

src/nat_traversal_api.rs — specifically the select_coordinator function (lines 5817–5870) and the missing coordination_count increment site in send_coordination_request_with_peer_id.

Vulnerabilities

No security concerns identified. The reachability model change (peer-verified scope instead of address classification) reduces the attack surface by requiring active peer confirmation before marking a node as coordinator/relay-capable, which prevents spoofed addresses from gaining coordinator trust. The accept loop fix correctly closes the old connection rather than the new one, and the relay session cleanup uses a Weak reference preventing any memory or resource leaks.

Important Files Changed

Filename	Overview
src/nat_traversal_api.rs	Accept loop dedup now keeps the newer connection (closes the old), relay session reuse returns the relay socket, coordinator selection adds RTT-weighted scoring — but coordination_count is never incremented so load balancing is inert.
src/masque/relay_server.rs	Adds spawn_cleanup_task using Weak reference so the background cleaner self-terminates when the server Arc is dropped; verified by two new tests.
src/p2p_endpoint.rs	relay_addrs population now includes all connected peers as candidates and rotates through them via transition_to_next_relay; relay capability (supports_relay) correctly gated on Client-side direct connections.
src/reachability.rs	New ReachabilityScope enum and socket_addr_scope helper replace the boolean has_public_ip; scope-aware peer-verified model is logically sound and well-tested.
src/node_status.rs	New direct_reachability_scope and has_global_address fields separate address knowledge from peer-verified reachability; can_help_traversal() test explicitly asserts global address alone is insufficient.
src/config/nat_timeouts.rs	Raises send_ack_timeout from 500ms to 5s with clear rationale; adds FAST_SEND_ACK_TIMEOUT (2.5s) for the fast profile.
src/link_transport_impl.rs	supports_relay and supports_coordination now correctly require Client-side direct connection evidence rather than any direct connection.

Sequence Diagram

sequenceDiagram
    participant Peer
    participant AcceptLoop
    participant ConnMap as DashMap[addr→conn]
    participant EmittedSet as DashSet[emitted]
    participant HandshakeTx

    Note over Peer,HandshakeTx: Fix #2 — Accept loop keeps newer connection

    Peer->>AcceptLoop: TLS Handshake (new connection)
    AcceptLoop->>ConnMap: has_live(remote_addr)?
    ConnMap-->>AcceptLoop: true (stale old conn)
    AcceptLoop->>ConnMap: get(old_conn).close("superseded")
    AcceptLoop->>EmittedSet: remove(remote_addr)
    AcceptLoop->>ConnMap: insert(remote_addr, new_conn)
    AcceptLoop->>EmittedSet: insert(remote_addr) → true
    AcceptLoop->>HandshakeTx: send(Ok((addr, new_conn)))

    Note over Peer,HandshakeTx: Fix #6 — Relay session reuse returns socket

    Peer->>AcceptLoop: establish_relay_session(relay_addr)
    AcceptLoop->>ConnMap: relay_sessions.get(relay_addr)?
    ConnMap-->>AcceptLoop: active session found
    AcceptLoop->>Peer: open_relay_stream_and_handshake(conn, addr, existing_pub_addr)
    Peer-->>AcceptLoop: (pub_addr, relay_socket) ← socket now returned

    Note over Peer,HandshakeTx: Fix #5 — Relay rotation on failure

    AcceptLoop->>Peer: try_relay_connection(target, relay[0])
    Peer-->>AcceptLoop: Error / Timeout
    AcceptLoop->>AcceptLoop: strategy.transition_to_next_relay()
    AcceptLoop->>Peer: try_relay_connection(target, relay[1])
    Peer-->>AcceptLoop: OK → success

Loading

Prompt To Fix All With AI

This is a comment left during a code review.
Path: src/nat_traversal_api.rs
Line: 5845-5848

Comment:
**Load-balancing score is always 1.0 — `coordination_count` never incremented**

`coordination_count` is initialised to `0` in every code path that creates a `BootstrapNode` (`BootstrapNode::new`, `add_connection`, the `known_peers` init loops) and there is no site in the codebase that increments it. The load score therefore evaluates to `1.0 / (0 + 1.0) = 1.0` for every candidate, making the load-balancing weight a constant additive term. Coordinator selection degrades to pure RTT-weighted selection with no actual load feedback, which is still better than the old approach — but the PR description's claim of "load-balanced" will confuse future readers.

Consider incrementing `coordination_count` when a PUNCH_ME_NOW coordination request is successfully dispatched, e.g.:

```rust
// In send_coordination_request_with_peer_id, after the frame is queued:
let mut nodes = self.bootstrap_nodes.write();
if let Some(node) = nodes.iter_mut().find(|n| n.address == coordinator) {
    node.coordination_count = node.coordination_count.saturating_add(1);
}
```

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (1): Last reviewed commit: "fix(nat): quality-aware coordinator sele..." | Re-trigger Greptile}

Greptile also left 1 inline comment on this PR.

[greptile-apps]

Load-balancing score is always 1.0 — coordination_count never incremented

coordination_count is initialised to 0 in every code path that creates a BootstrapNode (BootstrapNode::new, add_connection, the known_peers init loops) and there is no site in the codebase that increments it. The load score therefore evaluates to 1.0 / (0 + 1.0) = 1.0 for every candidate, making the load-balancing weight a constant additive term. Coordinator selection degrades to pure RTT-weighted selection with no actual load feedback, which is still better than the old approach — but the PR description's claim of "load-balanced" will confuse future readers.

Consider incrementing coordination_count when a PUNCH_ME_NOW coordination request is successfully dispatched, e.g.:

// In send_coordination_request_with_peer_id, after the frame is queued:
let mut nodes = self.bootstrap_nodes.write();
if let Some(node) = nodes.iter_mut().find(|n| n.address == coordinator) {
    node.coordination_count = node.coordination_count.saturating_add(1);
}

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/nat_traversal_api.rs
Line: 5845-5848

Comment:
**Load-balancing score is always 1.0 — `coordination_count` never incremented**

`coordination_count` is initialised to `0` in every code path that creates a `BootstrapNode` (`BootstrapNode::new`, `add_connection`, the `known_peers` init loops) and there is no site in the codebase that increments it. The load score therefore evaluates to `1.0 / (0 + 1.0) = 1.0` for every candidate, making the load-balancing weight a constant additive term. Coordinator selection degrades to pure RTT-weighted selection with no actual load feedback, which is still better than the old approach — but the PR description's claim of "load-balanced" will confuse future readers.

Consider incrementing `coordination_count` when a PUNCH_ME_NOW coordination request is successfully dispatched, e.g.:

```rust
// In send_coordination_request_with_peer_id, after the frame is queued:
let mut nodes = self.bootstrap_nodes.write();
if let Some(node) = nodes.iter_mut().find(|n| n.address == coordinator) {
    node.coordination_count = node.coordination_count.saturating_add(1);
}
```

How can I resolve this? If you propose a fix, please make it concise.

[grumbach]

Superseded by #52 which includes all 8 fixes from this PR plus TLS-key-based connection dedup and RelaySlotTable back-pressure. Our fixes were validated by Mac-to-NAT-testnet uploads; Mick's PR takes them further with structural improvements (TLS-derived identity eliminates the identity exchange timeout entirely).