This means the merge queue queue will trigger starting the patchbay
checks. And those checks are currently required to merge, so without
it the merge queue can not merge PRs.
## Description
This now includes the error_code that is part of the frame.
## Breaking Changes
none
## Notes & open questions
A small thing i still had lying around in a stash. Might as well
submit this separately.
## Change checklist
- [x] Self-review.
---------
Co-authored-by: Diva Martínez <26765164+divagant-martian@users.noreply.github.com>
## Description
We really can not afford these panics for broken
invariants. Especially not in auxiliary code like qlog.
## Breaking Changes
none
## Notes & open questions
none
## Change checklist
- [x] Self-review.
## Description
When the server sends a tail-loss probe in the Initial space it was not
padded to 1200-bytes. However a tail-loss probe is by-definition
ack-eliciting, so it MUST be padded to 1200 bytes.
Because of how space_can_send works it does not give us accurate
information for tail-loss probes. Which is why this bug occurred. We do
want to eventually get rid of space_can_send in #507 which would be a
more systematic fix, hence I'm fine with just the simple fix of catering
for this exception here.
## Breaking Changes
none
## Notes & open questions
I believe this will effectively fix#690. This is the reason that a
packet of an invalid size gets produced.
That issue has a secondary problem that the data scheduled for the 2nd
tail-loss probe than does not fit all in the 2nd tail-loss probe. This
will also be fixed as a side effect of the first tail-loss probe being
padded so the 2nd bug won't trigger.
That 2nd bug of not tracking the exact size needs is also an already
known bug that so far doesn't cause too much real-life trouble. It's
other symptom is tail-loss probes sending an entire GSO batch of empty
packets.
If you inspect the output of the test carefully there are still some
inefficient and minor bugs happening after "continue connection
establishment". Though none of them seem to be very critical right away.
## Change checklist
- [x] Self-review.
- [x] Tests if relevant.
## Description
This updates `n0-qlog` to `0.2.0`, which merges in changes from upstream
`qlog` and brings us up-to-date to the latest draft standards. See
https://github.com/n0-computer/n0-qlog/pull/16 for all changes.
## Change checklist
- [x] Self-review.
## Description
The fix in #695 was only gating emitting the event to the
application. However the entire frame should be ignored: the timers
are already all cancelled when the path is
abandoned (Connection::abandon_path). And this branch *could*
accidentally set a new timer if there are still other path challenges
without a response on the path. Which would then lead to spurious
firing of timers for an abandoned path.
## Breaking Changes
none
## Notes & open questions
I'm choosing to do this in the match branches so that the received
challenge is removed from the `PathData::on_path_challenges_unconfirmed`
field. This probably no longer matters, but it seems tidier than
entirely ignoring the frame.
Follow up to
https://github.com/n0-computer/noq/pull/695/files#r3386765693
I'm hoping the existing test is sufficient. Testing this particular
anomaly would be very tricky as we can't yet inject a custom packet
when we want it.
## Change checklist
- [x] Self-review.
## Description
Philipp's monkeys generated a case in which path challenges are sent on
the
second datagram of a batch, where the first one is smaller than the
required
`MIN_INITIAL_SIZE`. This causes a misalignment of the batch since these
frames
must be sent on expanded datagrams.
## Breaking Changes
n/a
## Notes & open questions
Regression test is in #694
## Change checklist
- [x] Self-review.
## Description
This change makes sure that we never emit a `PathEvent::Established` for
a path that has already been abandoned.
It may happen that a PATH_RESPONSE frame which validates a path is
received after a PATH_ABANDON frame was received for the same path. In
this case, before this fix we used to emit a `PathEvent::Established`
*after* a `PathEvent::Abandoned`. This ordering makes no sense for
applications, and it makes state tracking e.g. for iroh difficult and
error-prone.
This PR fixes this by not emitting `PathEvent::Established` for paths
that are already abandoned. Comes with a regression test that fails
without the fix.
## 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] Tests if relevant.
---------
Co-authored-by: Diva Martínez <26765164+divagant-martian@users.noreply.github.com>
## Description
This is simply a LOC reduction PR. This is a change the quinn
maintainers
wanted and I finally got to do it for when we decide to upstream this.
Simple
refactor, nothing fancy. Just less lines of code.
## Breaking Changes
## Notes & open questions
## 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
This stores the abandoned paths in an ArrayRangeSet, which results
that the memory is bounded by the number of allowed concurrent
multipath paths.
## Breaking Changes
## Notes & open questions
Mainly for comparison with #688.
- ~~AbandonedPaths::len is kind of sad, needing to construct a full
iter. Could be optimised to iterate over the ranges only and doing
the sums of each range end - start. That's probably reasonable.~~
done now.
- ~~All the casts to u32 are a bit unfortunate, but they are correct I
think.~~ They are gone now that the ArrayRangeSet can contain a u32.
- ~~The ARRAY_RANGE_SET_INLINE_CAPACITY (2) is not really a suitable
value for this use. It'll almost always allocate in practice. This
could probably be fixed by making it a const generic so we can pick
a different value for the use here.
Doing this might give this version an advantage?~~
This is now done.
- ~~Making the ArrayRangeSet generic over u32 (or PathId) or u64 would
save space in the inline vec. But some traits range depends on are
not public or unstable and it's a bit tricky.~~
done
Question is, which version would we prefer? I've not benchmarked these
against each other, it's hard to say which would be better and
probably doesn't really matter. Memory and allocation wise I don't
think it makes much difference, I think only CPU might be interesting
but suspect it doesn't matter much.
It is kind of nice that the compaction only needs to exist once. I
think this version is more complex but also it's already written. So
I'm undecided.
I'll still add tests, both versions need them anyway.
## 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.
---------
Co-authored-by: Diva Martínez <26765164+divagant-martian@users.noreply.github.com>
## Description
It seems this was missed when this was ported from the upstream
PR. This is different from #657 which was doing this unconditionally,
regardless when the packet needed to be accounted for congestion
control or whether it was ack-eliciting.
I did not continue the tests. It's something extremely specific and
artificial that is being tested. It's like testing everything with a
mock, I'm not sure the value it provides is worth all the stuff that
is setup. Ideally we figure out a better way to test that congestion
controllers actually behave correctly sometime.
## Breaking Changes
none
## Notes & open questions
Replaces #657, I considered taking it over. But this way I can't
approve my own PR.
## Change checklist
- [x] Self-review.
## Description
We should be able to traverse NATs with a combination of a "very easy"
and "hard" NAT. Adjust the SimpleFirewallRouting to allow emulating
that behaviour.
Verified by reverting c20a684349 and
then test_hard_nat_server_opens_path fails.
## Breaking Changes
none
## Notes & open questions
Look how simple this was. #675 was a red herring.
## 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.
---------
Co-authored-by: Philipp Krüger <philipp.krueger1@gmail.com>
## Description
This introduces a ConnPairBuilder to construct the ConnPair and
switches everything over to use it. The individual ConnPair
construction methods were becoming unwieldy.
There are probably a few style arguments to be had, but the current
code doesn't look to bad. We can keep tweaking this.
## Breaking Changes
none
## Notes & open questions
split off from #675 and taken a bit further.
## 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.
---------
Co-authored-by: Diva Martínez <26765164+divagant-martian@users.noreply.github.com>
## Description
This really surprised me that this wasn't calling me out on dead
code. I don't think this is very helpful.
ConnPair has loads of dead code, I assume if we'd convert all the Pair
usage to ConnPair they would go away so that is kind of intentional?
So I allowed it just there for now.
## Breaking Changes
none
## Notes & open questions
none
## Change checklist
- [x] Self-review.
## Description
This removes all the star-imports from the tests, making all the
imports much more explicit.
I guess tests/mod.rs' `use super::*` is the equivalent of the usual
test layout, but them not being in the same file makes it really hard
to follow imports. And implicitly relying on imports of 3rd party
crates via star imports is not very nice. So I think it being in a
separate file justifies not using the `use super::*`.
## Breaking Changes
none
## Notes & open questions
I know it is opiniated. I stumbled upon this because I was looking why
dead code was not being flagged in tests. Stand by for another PR...
## Change checklist
- [x] Self-review.
## Description
This implements a simple backoff algorithm for resending the
PATH_CHALLENGEs that are sent on-path, very similar to the backoff
mechanism in `pto_time_and_space`.
Fixes the proptest failure in #609
## Notes & open questions
This is one way to "fix" on-path path challenges.
I did look at what a version would look like that perhaps tried to use
`Retransmits` or the `LossDetection` timer, but (1) `Retransmits` is
used across all paths and so far only used for data that can be
transmitted over any valid path (speaking of the data space-kind only),
and the `LossDetection` timer is specifically about detecting loss via
acknowledgements, not about detecting loss from missing
`PATH_RESPONSE`s. It's possible that a `PATH_CHALLENGE` is ACKed, but
needs to be retransmitted, because the ACK was sent over a different
path than the `PATH_RESPONSE`, and the `PATH_RESPONSE` got lost.
We *could* look into a system that resends both `PATH_CHALLENGE` as well
as `PATH_RESPONSE`, but then you'd need to retransmit the
`PATH_RESPONSE` using the same token, which would invalidate RTT
estimation based on path challenges.
All in all, keeping our own timer and roughly duplicating the backoff
algorithm from `pto_time_and_space` seems like the most reasonable move
forward.
## 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
This changes the `ManyToManyRouting::route_client_to_server` and
`ManyToManyRouting::route_server_to_client` functions to take
`Transmit::src_ip` into account.
`Transmit::src_ip` is set when noq-proto wants to pin the traffic sent
to a specific interface. In that case, `src_ip` will be the IP address
of the local interface that's to be used to send to the remote.
If the routing table changes in between noq-proto assigning a local IP
for a path (and thus choosing a `src_ip`) and sending again - the
`src_ip` may be different from what the `ManyToManyRouting` table would
use otherwise.
In those cases, `src_ip` should force staying on the same 4-tuple.
We also check that the route even exists. It's possible that the
`ManyToManyRouting` table is modified in a way that will cut the network
path on a certain 4-tuple. In that case, we need to return
`RoutingDecision::Drop`.
Thus the logic is the following:
- Look for all local interfaces that are connected to given destination
socket address (this may be multiple)
- Filter all these local interfaces for the interfaces with an IP
address matching our `src_ip`.
- Choose the first local interface among those interfaces as our route.
That local interface address will then be the remote that the other side
will see for the incoming `Transmit`.
This is just some effort in making the `ManyToManyRouting` table a
little less weird. Otherwise it would represent a weird OS that
completely ignores the `src_ip` setting.
## Change checklist
<!-- Remove any that are not relevant. -->
- [x] Self-review.
## Description
When we receive a successful probe response from an unknown remote
that only means the remote managed to challenge us from that
remote. It is not because this remote was not advertised in an
ADD_ADDRESS frame that it should be ignored.
This now successfully opens paths if the server is behind a
Desitnation Endpoint Dependent NAT.
Replaces #647
## Breaking Changes
n/a
## Notes & open questions
I *really* wanted to have tests for this in proto, but they will come
later. In the meantime I'll point the patchbay tests from
https://github.com/n0-computer/iroh/pull/4254 to this PR which will
test this.
## 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
The RoutingTable is in charge of which addresses a TestEndpoint
has. So now all decisions are made there remove the addr field and
clean up any remaining usages. This should avoid confusion in the
future.
## Breaking Changes
n/a
## Notes & open questions
Maybe I'm finally done with cleaning up existing test infra. I'm
medium enthusiastic about this whole thing. I think it's an
improvement but there's definitely some things that look more complex
now. And some of the tests still do just plain weird stuff. Like when
opening a 2nd path we should now probably have a router that has a 2nd
path. But it was originally written without any router so cheated by
disconnecting the first path and relying on not reaching the
keep-alive interval or idle-timeout of the first path. So those are
still further improvements. But I need to work towards writing my
test, so I'll stop here.
## Change checklist
- [x] Self-review.
- [ ] Tests if relevant.
## Description
Based on #643
Removes `identity_hash` from the public API of `noq_proto`.
## Breaking Changes
* changed: `noq_proto::PathId` no longer
implements`identity_hash::IdentityHashable`
## Notes & open questions
Not sure if it's worth it?
## Change checklist
- [x] Self-review.
- [x] All breaking changes documented.
---------
Co-authored-by: Philipp Krüger <philipp.krueger1@gmail.com>
## Description
This is for future-proofing the API.
Closes#642
## Breaking Changes
- `enum PathEvent` and all of its cases are now marked as
`#[non_exhaustive]`
## Notes & open questions
I'm ignoring the `_ =>` case, as anything else would mean we would "do
something" in case e.g. noq is bound against a newer noq-proto.
In `#[cfg(test)]` however, I panic, as in that case the version we
depend on must be up to date.
## Change checklist
<!-- Remove any that are not relevant. -->
- [x] Self-review.
## Description
Subsumes #660Fixes#654
- Separates `peer_may_probe` from `peer_may_migrate` (taken from #660 )
- Adds a test that failed before the above change was applied
- Makes `peer_may_migrate` return `false` for the client side to avoid
triggering migration probing on clients
- Adds a proptest regression we've observed a bunch of times that
reproduced the above issue but can't be fixed without also depending on
#660 without breaking other tests.
## 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] Tests if relevant.
---------
Co-authored-by: Floris Bruynooghe <flub@n0.computer>
## Description
This replaces the `Pair::routes = None` with a basic routing table so
that there always is a routing table set.
As part of this it also replaces the PairRoutingTable trait with the
RoutingTable enum. This makes it a bit easier for the proptests to
invoke the correct variant.
## Breaking Changes
none
## Notes & open questions
none
## Change checklist
- [ ] 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
Adds an API for waiting for the start of the draining period in
`Endpoint::wait_all_draining` instead of waiting for draining to have
finished with `Endpoint::wait_idle`.
This allows dropping the `Endpoint` once all connections became inactive
and there is no need to wait for all connections to have drained.
This is all motivated by [this
paragraph](https://datatracker.ietf.org/doc/html/rfc9000#section-10.2-6)
in the QUIC spec:
> Endpoints that have some alternative means to ensure that
late-arriving packets do not induce a response, such as those that are
able to close the UDP socket, MAY end these states earlier to allow for
faster resource recovery. Servers that retain an open socket for
accepting new connections SHOULD NOT end the closing or draining state
early.
And finally, we're not replacing `Endpoint::wait_idle` and instead keep
it around as some tests require waiting for all `Connection`s to be
dropped before proceeding, which is only guaranteed by `wait_idle` and
not `wait_all_draining`.
## Breaking Changes
- Only an addition: `Endpoint::wait_all_draining` was added.
## 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] Tests if relevant.
- [x] All breaking changes documented.
## Description
This is only a mechanical change for now, but the routing decision now
separates choosing the IPs involved from dropping or not. This will
allow us to make more complex routing decisions, like delivering but
having the local_ip set to None.
## Breaking Changes
none
## Notes & open questions
This is the very first part of the road to implementing real NATs in
the proto tests. Next concrete step is to always make the routing
table required which is now possible. I may end up doing that as a 2nd
commit or as a followup PR depending on fast the review proceeds.
## Change checklist
- [x] Self-review.
## Description
Migration only gets triggered if the remote address changes. Not if
the local_ip changes. This check was too strict, which resulted in the
notification for an observed address being suprpressed if the local IP
changes, because it would assume it was emitted later as part of the
migration. Yet the migration would never run because the remote
address did not change.
This is basically correcting the wrong fix that was applied in #635.
## Breaking Changes
n/a
## Notes & open questions
No direct test, this was visible as flakyness in
address_discovery_rebind_retransmission. Which is now no longer
flaky. The test, nor it's flakyness, did not rely on MTU discovery so
that's disabled now to keep things a little cleaner.
## Change checklist
- [x] Self-review.
- [x] Tests if relevant.
Port of quinn-rs/quinn#2601.
`StreamsState::new` used to eagerly insert `(id, None)` placeholder
entries in both `send` and `recv` FxHashMaps for every remote stream id
in `0..max_remote[dir]`. With `max_concurrent_*_streams = 10_000` (e.g.
for MoQ relay nodes that burn through short-lived streams), hashbrown
rounded each map's bucket array up to ~65K buckets, costing ~0.5-2 MB of
bucket memory per Connection before any stream data was sent.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
## Description
This changes `noq::Connection::open_path` and
`noq::Connection::open_path_ensure` to take a `Impl Into<FourTuple>`
instead of a `SocketAddr`. This lets callers specify the local IP for
the new path, which previously could only be discovered after the path
was opened. Call sites can stay as-is because `FourTuple` impls
`From<SocketAddr>`, but it is now possible to also set the local IP if
desired.
The proto-side API is unchanged; both `noq-proto::Connection::open_path`
and `noq-proto::Connection::open_path_ensure` already took a
`FourTuple`.
## Breaking Changes
* changed: `noq::Connection::open_path` now takes a `impl
Into<FourTuple>` argument. Existing call-sites can stay as-is because
`FourTuple` impls `From<SocketAddr>`.
* changed `noq::Connection::open_path_ensure` now takes a `impl
Into<FourTuple>` argument. Existing call-sites can stay as-is because
`FourTuple` impls `From<SocketAddr>`.
## Change checklist
- [x] Self-review.
- [x] All breaking changes documented.
## Description
The "Build & test wasm32" check has been flaky in recent times.
The pattern seems to be, when it fails, then the `cargo binstall
wasm-bindgen-cli` command outputs:
```
INFO wasm-bindgen-cli v0.2.121 is already installed, use --force to override
INFO Done in 2.399616ms
```
Whereas when it succeeds, it's always
```
INFO resolve: Resolving package: 'wasm-bindgen-cli@=0.2.121'
WARN The package wasm-bindgen-cli v0.2.121 (x86_64-unknown-linux-musl) has been downloaded from github.com
INFO This will install the following binaries:
INFO - wasm-bindgen => /root/.cargo/bin/wasm-bindgen
INFO - wasm-bindgen-test-runner => /root/.cargo/bin/wasm-bindgen-test-runner
INFO - wasm2es6js => /root/.cargo/bin/wasm2es6js
INFO Installing binaries...
INFO Done in 2.149248106s
```
Here are some recent runs, all after #664 was merged:
- Runner name: 'hetz-ci-linux-x64-2': Fail
(https://github.com/n0-computer/noq/actions/runs/26168869453/job/76980594957)
- Runner name: 'hetz-ci-linux-x64-5': Success
(https://github.com/n0-computer/noq/actions/runs/26165233426/job/76967593091)
- Runner name: 'hetz-ci-linux-x64-1': Success
(https://github.com/n0-computer/noq/actions/runs/26164884966/job/76966183329)
- Runner name: 'hetz-ci-linux-x64-1': Fail
(https://github.com/n0-computer/noq/actions/runs/26167126491/job/76974283055)
- Runner name: 'hetz-ci-linux-x64-1': Fail
(https://github.com/n0-computer/noq/actions/runs/26167190559/job/76974514178?pr=662)
# Notes
It's unfortunate that we loose caching this way (and make ourselves more
vulnerable to github releases being flaky!!!), but this is mostly to
unblock us in the short term...
## Description
This removes `PathAbandonReason::NatTraversalRoundEnded`, it is never
constructed after the recent refactors.
## Breaking Changes
* Removed: `PathAbandonReason::NatTraversalRoundEnded`, no longer needed
## Change checklist
- [x] Self-review.
- [x] All breaking changes documented.
## Description
This adds a `cargo make` task and CI job to ensure that no foreign
crates appear in the public API apart from those explicitly
allow-listed.
## Change checklist
- [x] Self-review.
## Description
The event only is emitted when the path is usable for application
data, having it called Opened is confusing as that does not match
multipath terminology. Maybe in the future there is a need for an
event when the path is really opened, according to multipath, and then
we'd have to come up with a different event name.
Established also matches how we consider the state of the connection:
once the handshake is *completed*, it is considered established. So I
think this is a good match.
## Breaking Changes
- `PathEvent::Opened` -> `PathEvent::Established`
## Notes & open questions
Yes, this is an API breaking change in an RC release.
## 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] All breaking changes documented.
## Description
Remove Chunk usage in ordered read API
Make read_chunk return just a Bytes. Also rename read_chunks to
read_chunks_many (it already returns Bytes).
Add a bytes_read fn for the rare case where you do need the offset
despite being in ordered read mode.
Not sure if people agree, but there was an inconsistency before between
read_chunk (returns a Chunk, including offset) and read_chunks (fills a
bunch of Bytes, no offset).
Also it doesn't seem useful to have Chunk at all for ordered streams.
You usually don't care about the offset unless you are reading unordered
streams. So I would like to confine Chunk usage to only the (now
separate) unordered read API.
## Breaking Changes
noq::RecvStream::read_chunk returns a Bytes.
noq::RecvStream::read_chunks renamed to read_many_chunks.
## Notes & open questions
Note: while the [other related
PR](https://github.com/n0-computer/noq/pull/536) is just renaming, this
one is I think actually removing some weirdness.
Why does read_chunk give you an offset but read_chunks does not. And
there is no way to get the offset if you need it if you use read_chunks.
---------
Co-authored-by: Floris Bruynooghe <flub@n0.computer>
## Description
Remove the Written struct from the public API.
write_many_chunks had a somewhat weird API. On the one hand it was
mutating the passed buffers to zero out fully written buffers and split
the half-written buffer, but then on the other hand it expected the
caller to remove the empty buffers from the buffer array for the next
call.
With the change we only return the total number of written bytes and
remove the buffers from the slice ourselves. That makes the typical use
simpler.
In the rare case where you do need the number of fully written buffers
you can still get it.
## Breaking Changes
noq::SendStream::write_bytes_many: renamed to write_many_chunks, and
returns just the size.
noq::Written: removed
## Notes & open questions
<!-- Any notes, remarks or open questions you have to make about the PR.
-->
---------
Co-authored-by: Floris Bruynooghe <flub@devork.be>
## Description
Mirror the recent `ConnectionRef`/`EndpointRef` refactor (c1d7ed27) for
path reference counts. Cloning a `Path` or `WeakPathHandle` and calling
`Path::weak_handle`/`WeakPathHandle::upgrade` no longer lock the
connection state to bump a counter. Instead they `fetch_add` an atomic.
The state mutex is still taken on initial construction and on the last
drop, since `PathRef` entries are allocated lazily per `PathId` and we
need to clear `path_refs` plus any cached `final_path_stats` when the
count hits zero.
## Breaking Changes
None.
## Change checklist
- [x] Self-review.
## Description
I ran `cargo-check-external-types` on the noq crates. It all looks
reasonable. `noq-proto` has `ring` and `aws-lc-rs` in its public API
though through a `From<ring::error::Unspecified> for CryptoError` impl
(same for aws-lcs-rs, depending on feature flags). This means that from
a strict POV that ring couldn't be updated without a semver breaking
change.
This PR removes the From impl, instead using `map_err` at the few call
sites.
## Breaking Changes
* changed: `noq_proto::crypto::CryptoError` no longer implements
`From<ring::error::Unspecified>` and
`From<aws_lcs_rs::error::Unspecified>`
## Notes & open questions
Remaining foreign types in public API are: `bytes`, `futures_core`,
`rustls`, `rustls_pki_types`, `tokio` - all reasonable. Then we also
have `identity_hash` (marker trait, but don't think there would be any
need to update that crate during 1.0 so fine) and `arbitrary` (gated
behind non-default `arbitrary` feature). I think we're good!
## Change checklist
- [x] Self-review.
- [x] All breaking changes documented.
## Description
Make `negotiated_key_exchange_group` always available. This is useful if
you configured post quantum key exchange and want to make sure that you
actually got one, but also in general. It just costs 32 bit in size, and
getting it is very cheap.
## Breaking Changes
`crypto::rustls::HandshakeData`: public struct gets a new field
negotiated_key_exchange_group.
## Notes & open questions
Note: we could choose a shorter name?
Note: I made the struct non_exhaustive in case we want to add more.
Needed for https://github.com/n0-computer/iroh/issues/4195
## 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
draft-ietf-quic-qlog-main-schema-13.txt: All timestamps and
time-related values (e.g. offsets) in qlog are logged as float64 in
the millisecond resolution.
## Breaking Changes
n/a
## Notes & open questions
Port of https://github.com/quinn-rs/quinn/pull/2631
## Change checklist
- [x] Self-review.
## Description
This makes `noq::Path::set_status` return the previous `PathStatus`, so
that callers can check whether the status changed without having to call
`status` again (which would aquire another lock needlessly).
## Breaking Changes
* changed: `noq::Path::set_status` now returns `Result<PathStatus,
SetPathStatusError>`, where `PathStatus` is the previous path status
## 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] All breaking changes documented.
<!--
tip:
Run `cargo make` in the workspace root to check many light-weight CI
steps locally.
-->
## Description
This improves the NAT probing intervals: it now starts at 33ms by
default and does an exponential backoff capping at 2s intervals. There
are now 7 probe attempts spanning a period 4 seconds.
Fixes#569
## Breaking Changes
n/a
## Notes & open questions
4s is a bit longer than I initially thought, but I think it's
fine. Maybe we could remove 1 or even 2 further attempts. Removing 1
gets you to 2s though. Alternatively the interval cap could be lowered
a bit and the initial interval a tiny bit higher. But really I'm only
writing this down for nice git history, I think the current tradeoff
is a good start.
## 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
When we are NAT probing and we are sending a PATH_RESPONSE as a
client, we include a PATH_CHALLENGE. This ensures that if the peer got
through the firewall first that the client immediately gets through as
well and can open the path. Speeding up NAT traversal.
## Breaking Changes
n/a
## Notes & open questions
Closes#570.
Also fixes an oversight from before: we were still consuming CIDs for
off-path responses. For now we want to not do that.
## Change checklist
- [x] Self-review.
---------
Co-authored-by: Philipp Krüger <philipp.krueger1@gmail.com>
## Description
Replace the (ConnectionError, ConnectionStats) tuple with a named Closed
struct that exposes the close reason, the aggregate connection stats and
per-path stats for every path the connection knew about at close time.
Path stats are sourced from the proto layer for paths still tracked at
close time and from the cached final stats for already-discarded paths.
## Breaking Changes
* changed: the `OnClosed` future returned `noq::Connection::on_closed`
now resolves to a struct `Closed` instead of a tuple `(ConnectionError,
ConnectionStats)`. `Closed` has public fields `reason`, `stats`, and
`path_stats`.
## Notes & open questions
I gave the `Closed` struct public fields plus `#[non_exhaustive]`. Could
also be accessor methods and private fields, but then the path stats and
the connection error would have to be cloned out to get hold of owned
variants, or we'd have to have both getters and `into_` methods. I think
the public fields are fine here, but can also change it.
## 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] Tests if relevant.
- [x] All breaking changes documented.
## Description
When the local IP is not yet set on a network path we need to do our
network path comparisons with `is_probably_same_path` so that we accept
this difference. We can not accept the local_ip earlier because for
probing packets it should not be set. This requires a few fixed in
various places where network paths are compared.
Additionally when doing these network path changes we are not allowed to
depend on the PathData existing, it only gets created later for new
paths.
## Breaking Changes
n/a
## Notes & open questions
Testing this is difficult, it relies on the strange way that transports
are setup in iroh. Which is not even a notion that noq has at this time.
I'll think about it but not sure I can come up with a test. I've created
https://github.com/n0-computer/noq/issues/637 to follow up on this.
## Change checklist
- [x] Self-review.
Floris Bruynooghe