## 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.
## Description
- Removes the two API calls on `Connection` that haven't been migrated
yet
- Adds some more explicit `expect`s for unnamed `unwrap`s
Closes#514
## Breaking Changes
- remove
- `noq::Connection::local_ip`
- `noq::Connection::remote_address`
## Description
Implement client-side off-path nat traversal:
- REACH_OUT frames are scheduled on a normal may_send_data path.
- NAT probes are sent off-path without opening a new path.
- On a successful probe a new path is opened.
- Paths to-be-opened are retried if they fail due to temporary errors.
Closes#568.
## Breaking Changes
While not specifically tested, I think this will still interoperate with
a 0.98 peer.
## Notes & open questions
- Paths that fail to be opened due to insufficient CIDs or MAX_PATH_ID
are retried without when those become available again, without
time-limit. This is unlikely to be a big deal right now.
- The NAT state for ClientSide and ServerSide are very similar now. But
I'd like to defer merging them further for a later refactor. This is
already enough logic change. It's a tad annoying since one side needs to
keep track of the sequence ID.
## 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
Reexport all public noq-proto types at noq level so crates that use noq
don't have to import noq-proto.
Exported types:
ClosePathError
ClosedPath
DecryptedInitial
PathError
PathEvent
PathStatus
SetPathStatusError
Fixes https://github.com/n0-computer/noq/issues/417
## 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
When sending probing packets we need to accept them and send
responses.
This currently does not try to track all 4-tuples the server should
know, we only need this to work when qnt is enabled for now so I'm
taking a shortcut and not storing that state (yet).
This also fixes updating the 4-tuple too early: it would be update
even when a packet could still be rejected. We now only update the
4-tuple once the packet is fully authenticated. It is made clear that
earlier discards are only an optimisation.
This now fully respects the various probing packet requirements of
RFC9000, Multipath and QNT in all it's confusing combinations.
Fixes#599.
## Breaking Changes
n/a
## Notes & open questions
n/a
## 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 moves the state of the server-side off-path NAT traversal probes
to not be mixed in with path challenges. This is helpful because this
is now mostly the same state as the client needs to be.
Importantly this no longer pads these nat traversal probes, they are
now only 35 bytes.
It addresses one of the fields in #591.
It also cleans up some logging for timers.
## Breaking Changes
n/a
## Notes & open questions
Note that since the server probes are not accepted by the client they
never get acknowledged. And the server keeps sending those probes until
the maximum count is reached. #599 will fix this.
Test coverage is test_simple_nat_traveral_opens_path which was added in
the
previous PR just for this purpose.
## 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>
The fast-apple-datapath feature previously declared `sendmsg_x` and
`recvmsg_x` as hard extern "C" symbols. On Apple OS versions where these
private APIs don't exist, the dynamic linker would reject the binary at
load time.
Replace the `extern "C"` block with lazy `OnceLock`-based resolvers
using `dlsym`. Both functions now return `Option<Fn>`, and the call
sites in `send_via_sendmsg_x` and `recv_via_recvmsg_x` propagate an
`Unsupported` error if the symbol is absent. This allows callers to
probe availability at runtime before enabling the fast path.
(cherry picked from commit e01f99e265)
After probing the given socket for GSO support, we need to disable this option
again to ensure we can selectively enable it via our cmsg codepaths.
(cherry picked from commit 8acb578f10)
Replaces the repeated EINTR-retry loop pattern in `send_single`,
`send_via_sendmsg_x`, `recv_via_recvmmsg`, and `recv_via_recvmsg_x`.
(cherry picked from commit 52c7ad1898)
Clarify in the documentation for `Event::ConnectionLost`, `is_closed()`,
and `is_handshaking()` that the `ConnectionLost` event is only emitted
when the connection is closed by the peer or due to an error/timeout.
When the local application calls `Connection::close()`, no
`ConnectionLost` event is emitted; instead, pending operations fail with
`ConnectionError::LocallyClosed`.
Fixes#1495
(cherry picked from commit 0adcd20531)
Instead of creating two throw-away sockets for probing GRO & GSO,
use the existing socket that gets passed into `UdpSocketState::new`.
Related: #2564
(cherry picked from commit ee8f9469b8)
Frando