refactor!: remove dead NatTraversalEndpoint hole-punch chain

[mickvandijke]

Summary

• Deletes the unused duplicate connect_with_fallback on NatTraversalEndpoint and its supporting "PATH_CHALLENGE hole-punch" chain (attempt_hole_punching, attempt_quic_hole_punching, get_candidate_pairs_for_addr, calculate_candidate_pair_priority, create_path_challenge_packet, store_successful_candidate_pair, get_successful_candidate_address) plus the orphaned successful_candidates DashMap field.
• The deleted code was reachable only from the duplicate connect_with_fallback (also removed). The production hole-punch path lives in P2pEndpoint::connect_with_fallback_inner, which is what LinkTransport::dial_addr and the saorsa-transport example bin both call. Verified no in-tree or downstream consumers across this crate, saorsa-core, and ant-node.
• Fixes a pre-existing nonminimal_bool clippy warning in the same file (!target_peer_id.is_some() → target_peer_id.is_none()).

Why this is dead code, not just unused

The deleted hole-punch implementation could not have worked even if it had been wired in:

1. attempt_quic_hole_punching bound a fresh std::net::UdpSocket to the local candidate address — always fails on a real node because Quinn already owns the port (UDP binds are exclusive).
2. The "QUIC packet" it sent was a hand-rolled byte sequence (0x40 [0,0,0,1] 0x1a <8 random>) that is not a valid encrypted QUIC packet, so any receiving Quinn endpoint silently dropped it during connection-id lookup.
3. The success branch then waited 100 ms blocking on recv_from for a "response" no compliant peer would ever send.

The #[allow(dead_code)] markers on every function in the chain disguised that nothing in the path could ever succeed, actively misleading anyone debugging hole-punch issues via grep. Comment blocks left in place point readers at the production path.

Breaking change

The ! indicates removal of a pub fn. No in-tree or downstream consumer depended on it, but the public API surface is technically narrower.

Test plan

• cargo test --lib — 1459/1459 passed (3 ignored, unchanged)
• cargo clippy --lib -- -D warnings — clean
• rustfmt --edition 2024 --check — clean
• Downstream saorsa-core test suite still green

🤖 Generated with Claude Code

Greptile Summary

Removes an unreachable NatTraversalEndpoint::connect_with_fallback duplicate and its seven supporting helper functions (plus the orphaned successful_candidates DashMap field) that were gated behind #[allow(dead_code)] and could not have worked — the inner UdpSocket bind races Quinn for the port, and the hand-rolled byte sequence is not a valid QUIC packet. The production hole-punch path in P2pEndpoint::connect_with_fallback_inner is untouched. All call-site verification confirms no in-tree consumer used the removed API.

Confidence Score: 5/5

Safe to merge — removes provably unreachable code with no in-tree callsites and adds tombstone comments.

All changed code is dead-code removal confirmed by grepping every call site. The single P2 finding (misleading log label touched by the clippy fix) does not affect correctness or runtime behaviour.

No files require special attention.

Important Files Changed

Filename	Overview
src/nat_traversal_api.rs	Removes unreachable hole-punch chain and orphaned successful_candidates field; adds tombstone comments; fixes a clippy nonminimal_bool warning.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[dial_addr / saorsa-transport bin] --> B[P2pEndpoint::connect_with_fallback]
    B --> C[P2pEndpoint::connect_with_fallback_inner]
    C --> D[PUNCH_ME_NOW coordinator flow]
    D --> E[Hole-punch attempt]
    E --> F[Connection established]
    G[NatTraversalEndpoint::connect_with_fallback REMOVED]
    G -.->|deleted chain| H[attempt_hole_punching
attempt_quic_hole_punching
get_candidate_pairs_for_addr
create_path_challenge_packet
store_successful_candidate_pair
ALL REMOVED]
    style G fill:#f99,stroke:#c33
    style H fill:#f99,stroke:#c33

Loading

Prompt To Fix All With AI

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

Comment:
**Misleading `from_addr` log label**

The `from_addr={}` format slot receives `target_peer_id.is_none()` — a `bool` — so the log line always prints `from_addr=true` or `from_addr=false` rather than an actual address. This pre-existed the PR (`!target_peer_id.is_some()` had identical semantics), but since this line is touched by the clippy fix, it's a good opportunity to rename the label to match what is actually logged.

```suggestion
            "Sending PUNCH_ME_NOW coordination request for {} to coordinator {} (wire_id={}, from_peer_id={}, peer_id_missing={})",
```

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

_{Reviews (1): Last reviewed commit: "refactor!: remove dead NatTraversalEndpo..." | Re-trigger Greptile}

Greptile also left 1 inline comment on this PR.

[greptile-apps]

Misleading from_addr log label

The from_addr={} format slot receives target_peer_id.is_none() — a bool — so the log line always prints from_addr=true or from_addr=false rather than an actual address. This pre-existed the PR (!target_peer_id.is_some() had identical semantics), but since this line is touched by the clippy fix, it's a good opportunity to rename the label to match what is actually logged.

Suggested change

	info!(
	"Sending PUNCH_ME_NOW coordination request for {} to coordinator {} (wire_id={}, from_peer_id={}, from_addr={})",
	target_addr, coordinator,
	target_addr,
	coordinator,
	hex::encode(&target_wire_id[..8]),
	target_peer_id.map(|p| hex::encode(&p[..8])).unwrap_or_else(|| "none".to_string()),
	!target_peer_id.is_some(),
	target_peer_id
	.map(|p| hex::encode(&p[..8]))
	.unwrap_or_else(|| "none".to_string()),
	target_peer_id.is_none(),
	);
	"Sending PUNCH_ME_NOW coordination request for {} to coordinator {} (wire_id={}, from_peer_id={}, peer_id_missing={})",

Prompt To Fix With AI

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

Comment:
**Misleading `from_addr` log label**

The `from_addr={}` format slot receives `target_peer_id.is_none()` — a `bool` — so the log line always prints `from_addr=true` or `from_addr=false` rather than an actual address. This pre-existed the PR (`!target_peer_id.is_some()` had identical semantics), but since this line is touched by the clippy fix, it's a good opportunity to rename the label to match what is actually logged.

```suggestion
            "Sending PUNCH_ME_NOW coordination request for {} to coordinator {} (wire_id={}, from_peer_id={}, peer_id_missing={})",
```

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