The hint's is_path_recoverable() needs the original local_ip to check
if the path's interface is still available. Previously, local_ip was
cleared to None before reading network_path, causing the hint to always
return true (None => assumed recoverable). This meant paths through a
switched-away interface were incorrectly marked as recoverable instead
of being closed and replaced.
Increase MAX_OFF_PATH_PROBE_ATTEMPTS from 3 to 10 and set minimum
retry interval to max(PTO, 100ms). With asymmetric link delays, the
peer's outgoing packet may take 50-100ms to create its NAT mapping.
PTO-only retries (~6ms interval) exhaust all attempts before the
mapping exists.
Off-path NAT traversal probes had IPv4-mapped IPv6 destinations
(e.g. [::ffff:10.0.0.4]) because map_to_local_socket_family maps
to IPv6 when the connection has any IPv6 path. The transport layer
routes V6 addresses to the IPv6 socket, which has a different port
than the IPv4 socket. This breaks NAT hole punching because the
probe arrives from the wrong source port.
Canonicalize the destination in the Transmit so IPv4-mapped IPv6
addresses become plain IPv4, routing to the correct socket.
Off-path probes were fire-and-forget: sent once per address per round
with no retry. This broke simultaneous-open NAT traversal because the
first probe is typically dropped (the peer's NAT mapping doesn't exist
yet when the probe arrives).
Now off-path probes are retransmitted up to 3 times (once per PTO),
matching on-path PATH_CHALLENGE retry behavior:
- ServerState tracks attempt count per probe address
- New OffPathProbeRetry connection timer fires after each PTO
- queue_retries() re-enables sent probes for retransmission
- When CIDs are exhausted from initial probes, retries fall back to
the active CID (acceptable since the same destination already saw
a CID from us)
This matches picoquic's retry behavior and the old iroh DISCO protocol
which sent pings repeatedly for NAT hole punching.
Related: #410, iroh#3931
Off-path NAT traversal probes are currently fire-and-forget: the server
sends each probe once and never retries. This breaks simultaneous-open
NAT traversal because the first probe is typically dropped (the peer's
NAT mapping doesn't exist yet).
The test verifies that off-path probes are retransmitted up to 3 times
(once per PTO), matching on-path PATH_CHALLENGE retry behavior. This
matches picoquic's approach and the old iroh DISCO protocol's retry
behavior.
Related: #410, iroh#3931
Update the test to work with the PTO cap: increase idle timeout to 60s
(gap with 10 blackhole steps spans ~30s simulated time), reduce blackhole
steps from 50 to 10, and check for stream data arrival instead of idle
state (keep-alive timers prevent idle).
After a connectivity gap, PTO backs off exponentially without bound
(30ms * 2^10 = ~30s). This makes recovery impossibly slow — the server
can't retransmit data until the next PTO fires, often exceeding the
path idle timeout.
Cap PTO duration at 2s post-handshake, matching picoquic's
PICOQUIC_LARGE_RETRANSMIT_TIMER. During handshake, natural backoff is
preserved to avoid flooding unreachable peers.
Not from the RFC (which specifies unbounded exponential backoff), but a
widely-used implementation choice that enables practical gap recovery.
Reproduces the netsim interface_switch failure: server uploads data to
client, the path goes silent for several seconds (simulating relay
WebSocket reconnection), then resumes. The connection should recover
via keep-alive PINGs but doesn't.
Root cause: keep-alive PINGs keep the connection alive (preventing idle
timeout) but don't reset the server's PTO backoff. The server has
in-flight stream data with exponentially backed-off PTO from the gap.
The client's PINGs get ACKed, but the server never retransmits its
stream data because the next PTO is minutes away.
The fix needs to reset PTO on the side that receives a PING after a
period of silence (indicating the path recovered), not just the side
that called handle_network_change.
When handle_network_change marks a path as "recoverable", the PTO
backoff counter (pto_count) may be high from failed retransmits during
the transport disconnect. This causes the next PTO to be exponentially
backed off (seconds/minutes), preventing timely retransmission even
after the transport recovers.
Reset pto_count to 0 and re-arm the loss detection timer so PTO fires
promptly. This triggers loss probes that retransmit in-flight data once
the transport is back.
Intentionally preserves the congestion controller and RTT estimate:
for a truly recoverable path (e.g. relay reconnecting), these learned
values are still valid. Resetting them would cause unnecessary
slow-start on a path that was performing well before the disruption.
set_path_max_idle_timeout now calls reset_idle_timeout to immediately
re-arm the PathIdle timer with the new value. Previously it only stored
the new timeout, and the already-running timer continued with the old
deadline until data was next received.
This caused relay paths to time out even when the idle timeout was
extended to 15s during network changes — the timer was set with the
old 6.5s value before the extension, and since the relay was
disconnected (no data flowing), reset_idle_timeout was never called.
When the server sends off-path PATH_CHALLENGEs for NAT traversal, each
probe consumes a reserved CID from the sending path's queue. If the
probes fail (no PATH_RESPONSE), those CIDs were never retired, permanently
reducing the available CID budget. After enough failed rounds, remaining()
drops below 2 and no more probes can be sent — holepunching is broken.
This was the root cause of holepunch failure after 5G→WiFi switch:
initial 5G rounds consumed all probe CIDs on failed attempts, and after
switching to WiFi the server couldn't probe the client's new addresses.
Fix: when a new NAT traversal round's REACH_OUT arrives, clear the
previous round's unconfirmed off-path challenges and rotate the CID
queue (via next()) to retire the consumed CIDs. The peer then issues
replacement CIDs via NEW_CONNECTION_ID, restoring the probe budget.
Includes:
- ServerState::current_round() accessor
- PathData::has_off_path_challenges() / clear_off_path_challenges()
- Failing test: off_path_probe_cids_recovered_after_timeout
Move ADD_ADDRESS and REMOVE_ADDRESS frame scheduling before
NEW_CONNECTION_ID in populate_packet. Previously ADD_ADDRESS was
scheduled dead last, after STREAM frames. With high max_path counts
(e.g. 12), the first data packet could contain 35+ PATH_NEW_CONNECTION_ID
frames that filled the entire packet, pushing ADD_ADDRESS to a later
packet.
In real-world testing, this caused the server's public IP to be
advertised 10 seconds late — the client only learned the private LAN
IP initially and couldn't holepunch to the public IP until the
connection was nearly over.
ADD_ADDRESS frames are small (~10 bytes) and time-critical: the remote
can't probe addresses it doesn't know about, even if it has CIDs.
Sending addresses before CIDs ensures the remote learns WHERE to
connect before getting the credentials to do so.
Includes regression test: add_address_not_delayed_by_cid_frames
Two tests confirming no data blackout during network change:
1. Single-path replacement (open_first mode): replacement path inherits
validated status from the still-alive old path. Stream data sent after
network change arrives at the server without waiting for PATH_CHALLENGE
round-trip. Verified by sending known bytes and reading exact match.
2. Mixed paths (relay + direct): data flows on the recoverable relay path
while the non-recoverable direct path's replacement validates. Verified
by sending known bytes and reading exact match on the server.
These confirm noq avoids the "data blackout" gap identified in the
picoquic comparison, using a different mechanism: open_first ordering
ensures validation inheritance, and recoverable paths carry data during
transition.
PR #444 (packet scheduling): verify unvalidated paths don't carry
stream data — only PATH_CHALLENGE/RESPONSE. Stream data should flow
exclusively on validated Available paths.
PR #509 (PATH_ABANDON on self): verify PATH_ABANDON is sent on the
abandoned path itself when no other validated path exists. Also verify
PATH_ABANDON is delivered to remote after network change replaces the
only validated path.
Add tests covering network change scenarios related to iroh#3931
(holepunch after network changes):
- Replacement path to unreachable remote (NAT mapping change)
- Replacement path with fresh local address
- Recoverable path liveness probe behavior
- Recoverable dead path abandoned after PTO
- All-non-recoverable path replacement
- Single multipath path replacement
- Replacement path validation failure
These document the current behavior of handle_network_change and serve
as regression tests. The core iroh#3931 fix (re-initiating NAT traversal
after network change) is at the iroh layer, not noq-proto.
The NoViablePath grace timer (added in fd562f184 for #397) could fire
after the connection was already closing/drained — e.g. when the
application calls close() before the timer expires. This caused a panic
on the invalid state transition Drained -> closed.
Also adds:
- Regression tests for #398/#400 last-validated-path abandon scenarios
- Fix proptest error string to match new NO_VIABLE_PATH message
- Proptest regression seeds for the discovered panic
Previously, receiving PATH_ABANDON for the only remaining path was
rejected with `ClosePathError::LastOpenPath`, killing the connection
via `TransportError::NO_VIABLE_PATH` without accepting the abandon or
sending a reciprocal PATH_ABANDON.
Per draft-ietf-quic-multipath-21 Section 3.4, PATH_ABANDON is
non-optional: the endpoint MUST accept it and send a reciprocal. When
it's the last path, the spec says SHOULD send CONNECTION_CLOSE, but a
client MAY open a new path instead.
Changes:
- `close_path_inner` now accepts both locally- and remote-initiated
abandon of the last path (previously blocked both)
- Remote CIDs are preserved for last-path abandon so PATH_ABANDON can
be sent on the abandoned path itself
- New `ConnTimer::NoViablePath` grace timer (~1 PTO) starts when the
last path is abandoned. If a new path is opened within the grace
period (cancelled in `ensure_path`), the connection survives.
Otherwise CONNECTION_CLOSE is sent when the timer fires.
- Frame handler no longer returns transport error for last-path abandon
- Updated `path_close_last_path` test: closing last path now succeeds
and triggers connection close
The ordering of these frames was a bit too eager.
REACH_OUT is important timing-wise. But it is not more important than
HANDSHAKE_DONE, PING, IMMEDIATE_ACK, ACK, ACK_FREQUENCY and should
anyway not be sent on a path that also needs
PATH_CHALLENGE. PATH_RESPONSE could be the one exception but it is
also small.
If one of those frames do end up in the same packet as REACH_OUT there
will still be place for the REACH_OUT frame. The CRYPTO frame is left
after it, because after the handshake that is only carrying auxiliarry
non-time-sensitive information, and REACH_OUT is also only possible in
the data space kind.
OBSERVED_ADDRESS is definitely not that high priority, it might need
to move back even further.
Two bits of PR review:
- Move the scheduling to a separate function.
- Avoid an allocation, but this has some tradeoffs.
- I now have two loops that look for the PathId by doing `next_path_id
= self.path.keys().find(|i| **i > path_id).copied();`. It might be
possible to fold the MTU discovery in the main poll loop, MTU packets
would get a slightly higher priority but probably not really harmful
overall.
- I now need to do the computation for
`have_validated_status_available_space many more times.
I'm not sure how much the compiler manages to remove all of that. Is
it smart enough to figure out that
`have_validate_status_available_space` won't change between the calls
and does it move it out? Does it make the iteration as fast as the
previous version?
On the other hand, we now have some situations where we don't have to
compute the scheduling information, and no longer need to compute it
for all paths if we don't send on the last path.
What do you think, which version is better (though I also adopted
@matheus23's feedback about splitting it off to a function, but that
doesn't affect this really. It does make the diff a little bit more
though)?
As an aside, in working out of how scheduling should work it was
really helpful to have to extremely explicit as a bunch of data that's
computed up-front. But it's fair that now we know this is how it
should work that we can implement it in the most optimal way.
* feat(proto): issue CIDs in order of ascending path ID
We used to issue CIDs in sequence order, but not in reverse order of
path ID. This is not the order in which you need to have CIDs. This
fixes this to always issue them in order of ascending path ID and then
ascending order of sequence ID.
By introducing the newtype to do this, we also ensure that this order
is respected even when retransmits come into play. The newtype keeps
its sorting invariance when retransmits are merged back in.
Another benefit of the newtype is that this order is now enforced in a
specific place. Before it was implicit on the reliance between how the
endpoint ID generated the CIDs, how it then sent them to the
connection and how the connection stored and consumed them. It was
very implicit.
WRT to the cost of doing all the ordered inserts: CIDs are issued
relatively infrequently and usually not in huge numbers. Even
considering we want to increase those numbers in the future I think
using a simple Vec as storage is a decent choice for numbers of a few
100 CIDs that can be expected at most.
Fixes#137.
* fix aws-lc-rs tests, hopefully
* lol
* fix test
* Use cmp::Reverse instead of manual cmp
Co-authored-by: Philipp Krüger <philipp.krueger1@gmail.com>
* Sort less for the sorting gods
This optimises how we sort:
- Sort is still not manually implemented, as tempting it is to
implement insertion sort. I'm a believer of not manually
implementing algorithms.
- The major downside is that all functions need to be aware of the
invariants. Everyone needs to manipulate the fields.
---------
Co-authored-by: Philipp Krüger <philipp.krueger1@gmail.com>
* ci(docs): Check internal docs as well
We also want to check that the internal docs are all correct, so we do
not get broken links etc. We have a lot of internal docs, internal
docs are great!
* turns out that syntax is not supported
* fixup all the doc errors
* fix format
* naming nitpicking, fewer changes
dignifiedquire