## 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)
Add runtime dispatch so callers explicitly opt in
to the `sendmsg_x`/`recvmsg_x` fast datapath
instead of using it unconditionally.
(cherry picked from commit a0ec97a50b)
Suppress table output meant to provide human-friendly visualization
when using the `--json` CLI option and its argument is `-`, that is,
when the JSON output targets stdout.
Fixes#2544.
(cherry picked from commit f853e5e082)
Separate the fast path (`msghdr_x`-based) and slow path (`msghdr`-based)
implementations to prepare for runtime dispatch between them.
Broken out of #2463 as suggested by @djc.
(cherry picked from commit 2964782b4f)
Avoids locking the State mutex on every Clone/Drop of a ConnectionRef
or EndpointRef: the count now lives on the lockless Shared struct as
an AtomicUsize bumped/decremented with Relaxed ordering.
Also folds in the two related upstream stream-cleanup changes and
ports all four regression tests so the port has coverage for the
drop/stop paths.
Ports (adapted to noq's double-Arc ConnectionRef and WeakConnectionHandle
machinery):
- quinn-rs/quinn#2495 @ 404db1bc9, 4b7a03949, 475b55bad
(ref-count move; rightward-drift cleanup; RecvStream::drop early return
+ stream_drop_removes_blocked_reader test)
- quinn-rs/quinn#2609 @ 37625fe2d
(fix: fetch_sub returns prior value, use >1 / ==1 semantics)
- quinn-rs/quinn#2541 @ 803c814
(RecvStream::stop clears blocked_readers + recv_stream_cancel_stop_drop test)
- quinn-rs/quinn @ 07ce61cc2
(dropped_endpoint_cleans_up / dropped_connection_cleans_up tests)
When we store a type-erased, boxed value internally, accepting that
value directly allows users to avoid double-boxing. Probably not hugely
important in this case, but we've been adopting this pattern everywhere
else and we should be consistent.
(cherry picked from commit 06f7f7df1b)
Frando