## 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.
## Description
With the current off-path nat traversal none of these limits apply, so
do not issue warnings about them.
We'll add appropriate warnings once we need them again, but there's no
point in logging stuff that does not matter.
## Breaking Changes
n/a
## Notes & open questions
n/a
## Change checklist
- [x] Self-review.
## Description
This can happen when we abandon the path for which the peer is just
retiring a CID. That's not really an issue.
## Breaking Changes
n/a
## Notes & open questions
Seen as NODES-BACKEND-31 on sentry
## Change checklist
- [x] Self-review.
## Description
- Introduces a `CanonicalIpPort` wrapper around `IpPort` that makes sure
`IpAddr::to_canonical` is called.
- Fixes a bug in `check_remote_address` so it compares the canonical
address instead of mixing the compared types.
- Makes `next_probe_addr` return an `IpPort` and `mark_probe_sent` take
an `IpPort` to save needless conversions.
## Notes & open questions
This should unblock https://github.com/n0-computer/iroh/pull/4213 and
seems to fix the test problems with ipv4-only hosts.
## 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.
## Description
This sets the -Dwarnings flags so that the commands fail when there
are warnings. The output of the full flow easily scrolls off screen so
you don't notice things will fail on CI.
## Breaking Changes
n/a
## Notes & open questions
n/a
## Change checklist
- [x] Self-review.
## Description
- Raises `MAX_PATH_RESPONSES` from 16 to 32.
16 is not enough with users potentially having 25 interfaces or more
configured. Now that we send off-path PATH_CHALLENGEs all from a single
`PathId`, we need to be able to keep around 32 PATH_RESPONSEs queued for
sending for potentially that many challenges.
I've considered other values for this such as 64 or 128, but the
worst-case memory use caused by malicious attackers is quite bad, so
I've opted for the lowest reasonable value.
- Raises `MAX_QNT_ADDRS` in the proptests from 12 to 32, to match values
that will be set in iroh: https://github.com/n0-computer/iroh/pull/4213Closes#613 (together with
https://github.com/n0-computer/iroh/pull/4213).
## 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.
## Description
There was some inconsistency in naming in the transport config between
the multipath enabling config and NAT traversal enabling config:
- `max_concurrent_multipath_paths` vs.
- `set_max_remote_nat_traversal_addresses`
This renames `set_max_remote_nat_traversal_addresses` to
`max_remote_nat_traversal_addresses`, dropping the `set_` prefix.
I'm also removing the whole `#[cfg(doc)] pub const
DEFAULT_MAX_CONCURRENT_MULTIPATHS_PATHS` dance with two `const` values
and `NonZeroUSize`. There is no previous pattern for exposing config
defaults, so I'm keep it consistent.
## Breaking Changes
- Renamed `set_max_remote_nat_traversal_addresses` to
`max_remote_nat_traversal_addresses`.
## Notes & open questions
This is based on https://github.com/n0-computer/noq/pull/620 to avoid
conflicts.
## 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] All breaking changes documented.
<!--
tip:
Run `cargo make` in the workspace root to check many light-weight CI
steps locally.
-->
## Description
Since we're using off-path PATH_CHALLENGEs for punching firewall holes
(#567), we don't need as many concurrent multipath paths.
## Breaking Changes
None, only behavioral: The default maximum multipath paths is reduced
from 12 to 8.
## Notes & open questions
Partially addresses #613
But really, we need to change the default in iroh more than the one in
noq.
I also changed the default used in the proptests. Those shouldn't break
older regression tests, none of them hit the concurrent path limit.
We might want to consider changing the whole
`DEFAULT_CONCURRENT_MULTIPATH_PATHS_WHEN_ENABLED_` constant dance, but
I'll do that in another PR.
## Change checklist
<!-- Remove any that are not relevant. -->
- [x] Self-review.
Frando