A Backup path (e.g. relay, once a direct path takes over) stops
carrying new application data, but can still have in-flight packets
sent right before the switch whose PTOs keep firing with nothing sent
since to compare against. Keep-alive doesn't help: it only covers idle
gaps going forward from when its timer is armed, not data already
in-flight at the moment the path became idle. Neither reflects the
path's own health, so a Backup path could go suspect purely from being
temporarily unused, not from actually being dead.
Suspect is meant to answer "is the path we're currently relying on
still working", so it should only ever apply to the Available path.
Gate PathEvent::Suspect on that.
A path that stops acknowledging data currently just keeps accumulating
PTOs with no signal to the application or path selector, so a dead
path can keep winning path selection on stale RTT samples.
PathData::suspect_since_pn now tracks the Data-space packet number a
path was about to send when it crossed SUSPECT_PTO_THRESHOLD
consecutive PTOs on an established, validated path; Some(pn) doubles
as the suspect flag, since the two were always set and cleared
together. PathEvent::Suspect fires on that transition, and Recovered
fires once an ACK arrives that covers a packet sent at or after that
watermark.
The watermark matters because multipath QUIC lets an ACK for one path
be coalesced onto any other path's outgoing packet (PathAck names the
acked path explicitly, independent of the carrier path). A path that
has genuinely gone dead can still have older, honestly-delivered
packets whose ACK was pending and only goes out once some other path
becomes available; without the watermark such a late, pre-suspect ACK
would spuriously clear suspect on a path that will never carry
anything again.
path_stats() exposes the new PathStats::suspect for external path
selectors. The tokio-layer event dispatch now forwards both variants
to PathEvents subscribers (previously silently dropped, since no arm
existed for them).
## Description
Updates deps to latest and adjusts deny.toml entries accordingly. Pins
the aws-lc crates to the current version. New versions generate cryptic
errors
## Breaking Changes
n/a
## Notes & open questions
prompted by several dependabot commits from quinn that were not applied
in the latest sync.
These were not applied due to several reasons:
- Cherry picking `Cargo.lock` updates often gives wrong results.
`Cargo.lock` must always be generated by cargo itself.
- We still should keep our deps updated within compatible versions.
## Change checklist
- [x] Self-review.
- [x] This PR was created by a human that thought critically about the
proposed change and wrote an as clear and concise description as
they could.
- [x] This PR isn't slop, and is carefully crafted to do have the
intented effect.
- [x] `cargo make` passes locally.
## Description
Currently they start finishing at around 9 CEST. Let's move them 2h
earlier.
## Breaking Changes
n/a
## Notes & open questions
n/a
## Change checklist
- [x] Self-review.
- [x] This PR was created by a human that thought critically about the
proposed change and wrote an as clear and concise description as
they could.
- [x] This PR isn't slop, and is carefully crafted to do have the
intented effect.
- [x] `cargo make` passes locally.
## Description
Updates `CONTRIBUTING.md` to include some basic guidance to new
contributors
and instructions for maintainers for syncing quinn
## Breaking Changes
n/a
## Notes & open questions
Not meant to be super extensive. We could add more as we find what works
best
for this repo
## Change checklist
<!-- Remove any that are not relevant. -->
- [x] Self-review.
- [x] Documentation updates following the [style
guide](https://rust-lang.github.io/rfcs/1574-more-api-documentation-conventions.html#appendix-a-full-conventions-text),
if relevant.
- [x] This PR was created by a human that thought critically about the
proposed change and wrote an as clear and concise description as
they could.
- [x] This PR isn't slop, and is carefully crafted to do have the
intented effect.
- [x] `cargo make` passes locally.
## Description
Because of dualstack hosts that can be on IPv4-only networks getting
errors is very normal. It is a bit strange to emit full on warnings
for these. Telling folks that they just shouldn't be trying to connect
to such an unreachable hosts is also a bit strange. Simply trying this
is the normal thing to do.
Fixes https://github.com/n0-computer/iroh/issues/4345
## Breaking Changes
n/a
## Notes & open questions
It feels a bit odd, but I'm like 99% sure this is the right thing to do.
## Change checklist
- [x] Self-review.
- [x] This PR was created by a human that thought critically about the
proposed change and wrote an as clear and concise description as
they could.
- [x] This PR isn't slop, and is carefully crafted to do have the
intented effect.
- [x] `cargo make` passes locally.
drive_timer() used AsyncTimer::poll() to determine whether a protocol
deadline had elapsed. Under Tokio's cooperative task budget,
Sleep::poll() may return Poll::Pending for an already-expired deadline
once the task's budget is exhausted, which can happen when
process_conn_events() drains a busy channel.
As a result, handle_timeout() is not called even though the deadline has
already elapsed. For QUIC, timers such as PTO, loss detection, and idle
timeouts are correctness-critical and should not be deferred to a later
scheduling round.
Fix this by checking runtime.now() >= deadline before consulting the
async timer. The clock is not subject to cooperative budgeting. The
timer remains responsible only for registering a wakeup when the
deadline lies in the future.
(cherry picked from commit 76020ba4ab)
When the `max_incoming` queue is full (or CIDs are exhausted), quinn
replied to each Initial with CONNECTION_REFUSED. Building that reply
derives the packet's initial keys, which is computationally expensive.
A flood of Initials then forces the endpoint's packet-processing task
to do per-Initial crypto work, leaving it less time for legitimate
packets and degrading already-established connections.
(cherry picked from commit d4fc3efd5e)
Enable SO_TIMESTAMPNS on Linux and Android, parse SCM_TIMESTAMPNS
ancillary messages, and expose timestamps via RecvMeta::timestamp.
(cherry picked from commit 9849790f02)
## Description
When we open a new path we send a path challenge, if that is lost
it is retried using the normal PTO schedule for tail-loss probes.
However the opening of the entire path was only tried for 3 * PTO,
which does only leave time for 2 retries. This makes opening a path
much more brittle wrt to packet loss than establishing a new
connection.
Instead this changes this to use the same mechanism to abandon a path
as when abandoning opening a new connection: keep sending tail-loss
probes, in this case path challenges, until the interval between the
tail-loss probes is larger than the path idle timeout. At which point
the normal idle timer will fire and abandon the path.
This removes the need for a separate timer to abort validation
of a path. It also removes a user-visible event as there is one less
reason a path can be abandoned for.
Another nice effect is that the poll_transmit no longer has to
figure out why an on-path path challenge is being sent in order to
set the right timer. It only has to arm the challenge PTO.
Fixes#686Fixes#687
## Breaking Changes
none
## Notes & open questions
This replaces 3 tests with one new one. I believe this together with
the open_path_validation_fails_* tests ensures the timers are set as
intended. The previous manual triggering of path challenges had a very
tight coupling between the code under test and how to trigger it. So
much that I felt directly adopting it would just be re-implementing
the same logic again.
The new test opens a new path and ensures lost challenges are reset
appropriately, which is exactly what needs to be tested. On an
existing path we never send new challenges, so those tests were very
artificial.
## Change checklist
- [x] Self-review.
- [x] Tests if relevant.
## Description
A fix was merged, nightly shouldn't complain anymore.
## Breaking Changes
None
## Change checklist
- [x] Self-review.
- [x] This PR was created by a human that thought critically about the
proposed change and wrote an as clear and concise description as
they could.
- [x] This PR isn't slop, and is carefully crafted to do have the
intented effect.
- [x] `cargo make` passes locally.
## Description
We have a lot of these jobs running way to long into the Europe
day. 04:00 UTC is 23:00 in the latest dev timezone so should be fine.
## Breaking Changes
n/a
## Notes & open questions
n/a
## Change checklist
- [x] Self-review.
- [x] This PR was created by a human that thought critically about the
proposed change and wrote an as clear and concise description as
they could.
- [x] This PR isn't slop, and is carefully crafted to do have the
intented effect.
- [x] `cargo make` passes locally.
## Description
- Adds a regression test that triggers the `debug_assert!(max_size >=
min_size);` in `packet_builder.rs`
- Fixes the coalescing logic in `poll_transmit_path_space` to not exit
the loop & function to go to the next space when coalescing.
- Adds a bunch of useful `trace!` logs to see what the server is doing
in response.
## Breaking Changes
None
## Notes & open questions
A bit more on what was going on:
There are two loops for building datagrams: `poll_transmit_on_path`,
which goes over `Initial`, `Handshake` and `Data`, as well as
`poll_transmit_path_space`, which loops *within* the same space to
produce *separate* datagrams (e.g. for producing multiple loss probes as
a GSO batch).
The regression test generated a situation where two things were true at
the same: The `Initial` space wants to send a small `PATH_ACK` (nothing
else), and the `Handshake` space wants to send some `CRYPTO`.
Inside `poll_transmit_path_space` when we were checking for coalescing,
we call `self.has_pending_packet`, which iterates through all spaces
starting at `Initial`. It's false for `Initial`, but ends up being true
for `Handshake`, because that has `CRYPTO` pending.
However, we then keep on looping inside `poll_transmit_path_space`,
creating packets again and again in the same `space_id = Initial`,
instead of exiting out of the loop to the next space.
Eventually we fill the datagram with enough packets such that the
`debug_assert!(max_size >= min_size);` in `packet_builder.rs` triggers,
because it doesn't take into account the retry tokens in the QUIC header
for `Initial` packets (which I think it shouldn't do? because we should
never be coalescing these anyways.).
## Change checklist
- [x] Self-review.
- [x] Documentation updates following the [style
guide](https://rust-lang.github.io/rfcs/1574-more-api-documentation-conventions.html#appendix-a-full-conventions-text),
if relevant.
- [x] Tests if relevant.
- [x] All breaking changes documented.
## Description
Follow-up from #748
- Refactors how `Draining` and `Drained` endpoint events are emitted:
They're now handled within state.rs.
- `move_to_draining` and `move_to_drained` are now passed `&mut
self.endpoint_events`.
This is a pure refactor without behavioral changes.
## Breaking Changes
None
## Change checklist
- [x] Self-review.
- [x] Documentation updates following the [style
guide](https://rust-lang.github.io/rfcs/1574-more-api-documentation-conventions.html#appendix-a-full-conventions-text),
if relevant.
## Description
Copied and adjusted from iroh.
## Breaking Changes
n/a
## Notes & open questions
Move the `cargo make` to a checkbox might be a bit harsh, but honestly
I more or less am in that habit already so probably ok.
## Change checklist
- [x] Self-review.
## Description
- Fixes a bug where the `Draining` event was emitted twice when a
stateless reset token was duplicated and received twice
- Adds a regression test for the above scenario
This is one of those bugs that triggers an underflow in
`active_connections` in noq.
## Breaking Changes
None
## Notes & open questions
I'm still investigating whether there are further such cases.
## Change checklist
- [x] Self-review.
- [x] Documentation updates following the [style
guide](https://rust-lang.github.io/rfcs/1574-more-api-documentation-conventions.html#appendix-a-full-conventions-text),
if relevant.
- [x] Tests if relevant.
- [x] All breaking changes documented.
## Description
Sets the correct user config for the action (we never ported this from
iroh) and lands some fixes to the script (numerical instead of lexical
sort, plus avoids negative head counts)
## Breaking Changes
<!-- Optional, if there are any breaking changes document them,
including how to migrate older code. -->
## Notes & open questions
<!-- Any notes, remarks or open questions you have to make about the -->
<!-- PR. -->
## Change checklist
<!-- Remove any that are not relevant. -->
- [ ] Self-review.
- [ ] Documentation updates following the [style
guide](https://rust-lang.github.io/rfcs/1574-more-api-documentation-conventions.html#appendix-a-full-conventions-text),
if relevant.
- [ ] Tests if relevant.
- [ ] All breaking changes documented.
<!--
tip:
Run `cargo make` in the workspace root to check many light-weight CI
steps locally.
-->
## Description
This updates semver checks with the following behaviour:
- The baseline for the check is now the latest published release on
crates.io
- If you make a semver-breaking change you must also bump the package
version number so that the semver check will pass. Because we like
to bump the versions of all crates at the same time you can not use
pre-releases since cargo does not allow mixing those and the iroh
patchbay tests has 2nd-level dependencies on noq-udp (via netwatch).
- Adding deprecated items is allowed in minor version bumps, matching
https://semver.org/#how-should-i-handle-deprecating-functionality
- Bump noq-proto version because we previously added deprecations.
- Now the baseline is somewhat stable, enable caching.
## Breaking Changes
none
## Notes & open questions
I tested this first by not bumping the noq-proto version number and it
fails in that case. Which is due to #725.
Once this is merged I will make the semver check required in noq.
The version bumping required to make semver-checks pass could be a bit
annoying. It will also make patching iroh for noq a little bit harder.
As shown by what the patchbay check has to do now. I'm tempted to think
for now that this is worth it, but happy to think about how to tweak
this as we gain experience.
OTOH having to bump to the right version means that come to a release we
do know what the next version should be. Which is probably good.
## Change checklist
- [x] Self-review.
## Description
This uses the new CARGO_BUILD_WARNINGS in Makefile.toml so that it no
longer needs to invalidate all of rust's cache for -Dwarnings.
The code changes are new clippy fixes.
## Breaking Changes
none
## Notes & open questions
We're forcing contributors to get the last rust, but that's not that
bad I think. And cargo-make is kind of still optional.
## Change checklist
- [x] Self-review.
## Description
- Refactors the `RoutingDecision::Deliver` case to give it a `recv_time:
Instant` field. This allows `Routing` to decide when to deliver packets.
- Adds a `now: Instant` parameter to `Routing::route_client_to_server`
and `Routing::route_server_to_client` to make it possible for it to
compute the correct `RoutingDecision::recv_time`
- Adds `Routing::set_latency` to set the one-way-latency that `Routing`
now owns
- Adds a `BwLimitedRouting` routing implementation that implements a
queue that delays packets the more packets are queued and tail-drops
packets at a certain max queue length.
- Adds a `throughput` test as a quick way of testing noq-proto and its
congestion control against `BwLimitedRouting`.
## Breaking Changes
None. Test changes only.
## Notes & open questions
Started this work because it might be really useful in catching
regressions in congestion control, and to prepare work for when we send
on multiple paths simultaneously.
We can now generate neat pictures from noq-proto itself!
<img width="3750" height="1639" alt="image"
src="https://github.com/user-attachments/assets/d6e02727-40e5-42e3-b72a-2b528b7de9ea"
/>
The qlog file for this is generated in 0.2 seconds on my machine, even
though it is a simulated 10MB transfer over a 1MB/s connection, so this
simulates ~10s of actual time.
The other neat thing about it is that it is *fairly* deterministic.
There's still some sources of randomness in noq, but all of the
"measured" transfer speeds fall into a 1% range.
## Change checklist
- [x] Self-review.
- [x] Documentation updates following the [style
guide](https://rust-lang.github.io/rfcs/1574-more-api-documentation-conventions.html#appendix-a-full-conventions-text),
if relevant.
- [x] Tests if relevant.
## Description
Demotes this `error!` log to a `debug!` log instead.
Although we don't *expect* this situation to happen, it's benign when it
*does* happen, it just wastes some cycles and indicates that we have not
upheld some weak invariants somewhere.
## Breaking Changes
None
## Notes & open questions
Could also change it to a `warn!` log instead, but IMO even that is a
bit too much. There's nothing the user can do about this and it doesn't
really indicate that something dangerous is going on to be "warned"
about. The system is left in a good state and immediately heals on its
own.
## Change checklist
- [x] Self-review.
Frando