Keep LAN shortcut from blocking holepunch fallback

[marcus-pousette-hp]

When two peers appear to be on the same network, connect() currently tries the server's LAN address first. If that LAN ping fails, the connection aborts with HOLEPUNCH_ABORTED before the normal holepunch path gets a chance to run.

That means LAN cases with firewalls/default-deny incoming rules can fail even when the regular UDX holepunch path through the observed NAT address would work.

This changes the LAN shortcut to be opportunistic when the remote is holepunchable:

• still try the LAN ping as a fast path
• do not let a failed LAN ping abort the connection before normal holepunch
• keep the previous abort behavior when the remote is not holepunchable and there is no fallback path
• keep the server-side holepunch payload/puncher setup available for same-LAN peers

Testing

I added testing for the fallback behaivour in the PR.

But also did a LAN/hairpin repro:

I also tested the same failure shape as #210 with a Docker-based LAN/NAT setup:

• two peers on the same simulated LAN
• both peers behind the same simulated NAT
• direct LAN UDP to the server blocked
• ufw default deny incoming enabled on the peers
• hairpin NAT available through the externally observed address

On origin/main, the client failed with HOLEPUNCH_ABORTED before the normal holepunch hook ran.

On this branch, the failed LAN ping no longer stopped the connection. The client reached the normal holepunch path and successfully exchanged data with the server.

I also ran this repro inside an Ubuntu 24.04 Lima VM with rootful Docker, so the UFW/firewall behavior was tested in a Linux environment.

If needed I can provide the repro scripts used for this.

---

Ref and praise to #236 for the initial solution attempt.

Addresses the original failure mode reported in #112: when peers appear same-LAN, a failed LAN shortcut can abort with HOLEPUNCH_ABORTED before normal holepunch gets a chance to run. Closes #210

---

Co-Written with AI

[noahlevenson]

Reviewing at @marcus-pousette-hp's request...

For context, I'm familiar with this NAT traversal IP candidate optimization problem, but not yet familiar with hyperdht's implementation. First time poking around this code.

I think this change is a clever race which AFAICT solves the problem correctly.

Only thing I wasn't sure about is the condition when holepunching is mid-flight when the LAN ping succeeds, and so isDone(c) = false. But I guess c.onsocket is the guard that resolves the race there?

Zooming out: the race paths are now pretty subtle and challenging to reason about, probably a bit too subtle for my taste. But I suppose a different structure would trigger a much larger refactor, so this seems like a reasonable compromise.

[marcus-pousette-hp]

d7db4f1

I added a comment here, both isDone and c.onsocket correctly drop the other. The only real "issue" I would say here worth thinking about is that if the fallback (normal) Holepunch path wins, the LAN shortcut path will be ignored. In theory this would mean that we would take a little bit longer path than what the LAN shortcut path would offer

Context:
LAN shortcut
Uses the peer’s advertised local address, like 192.168.x.x / 10.x.x.x / loopback in the test. If both peers are truly reachable on that LAN address, this is usually the best path: fewer NAT hops, lower latency.

Normal holepunch path
Uses the externally observed/NAT address learned through relays/bootstrap nodes. If both peers share a public IP and the NAT supports hairpinning, this can still connect directly, but traffic may go through the router’s NAT hairpin path rather than straight peer-to-peer LAN addressing.

This PR makes the race and the faster wins, and there is an assumption here that if LAN shortcut is the fastest path it would win anyway here, but the shortcoming of this PR is perhaps it does not prove for example a bad scenario where the normal holepunch path would win the race, even though it is slower later when data is transmitting. My own judgement finds this unlikely, which is the reason why I went with this solution

[noahlevenson]

I think the test is correct, but I do get a little nervous whenever I see this sort of thing tested against loopback IP. I think it's just out of domain, in the sense that the fix was designed to solve the opposite case -- the case where a NATed peer's public IP is not the same as its LAN candidate.

(I think here 127.0.0.1 winds up in localAddresses which is what makes the test work, right?)

Not suggesting any changes, it was just a bit challenging to reason about whether this test was actually exercising the right paths. Compromises for testing...

[marcus-pousette-hp]

I see your concern. I added a clarifying comment in 0c06e40 about the test purpose

(I think here 127.0.0.1 winds up in localAddresses which is what makes the test work, right?)

Yeps! In this test 127.0.0.1 is intentionally used so Holepuncher.localAddresses(...) returns the loopback address and we deterministically enter the same LAN shortcut branch

[lejeunerenard]

Looks good! Good to allow holepunching even when the "lan" ping doesn't work.

Only some nits and clarifications left.