## Description
This moves the FrameStats into the PathStats, allowing us to check
what frames where sent on which paths. It then makes the
ConnectionStats always be computed to be the sum of all the PathStats.
This has as a nice side effect that some stats no longer need to be
recorded twice. Which was also error-prone.
The PathStats are now boxed in the PathEvent because that variant was
now way bigger than any of the other variants.
It also fixes a few missed fields in the stats code and uses more
defensive code so that won't happen again.
## Breaking Changes
Probably none? I'm only adding some fields to PathStats on the public
API, while the fields on ConnectionStats remain the same.
## Notes & open questions
I need this to be able to write tests that assert frames on specific
paths. But I also think that generally this makes more sense. The
previous version of the stats was thrown together without much thought
I think (by me).
I've cleaned up some cargo.toml mistakes that accumulated by using
tooling that doesn't follow the sorting/and locations of where things
live.
## Description
This was some kind of hack to be able to use wireshark on tests in
proto. But we have qlog now, which is probably much better for this.
Also, we have tests, e.g. in #522 and proptests, that would not work
with this because they use multiple fake addresses but we only ever
bind one socket.
## Breaking Changes
n/a
## Notes & open questions
n/a
## Description
Off-path NAT traversal probes (PATH_CHALLENGE) and off-path responses
(PATH_RESPONSE) are sent to a different destination than path_id's
network path. ACKs for these packet numbers won't arrive on path_id, so
tracking them in path_id's in_flight permanently inflates the congestion
state.
This caused the relay path to become congestion-blocked after repeated
NAT traversal probing, preventing REACH_OUT and ADD_ADDRESS frames from
being sent — breaking holepunching after network changes.
Use builder.finish() (no tracking) instead of finish_and_track(),
matching the existing send_prev_path_challenge() which already does this
correctly.
## Notes
- Is this the right solution?
- This does fix actual tests, the full combined fixes now pass the
`switch_uplink_ipv6` and `switch_uplink` tests from patchbay
## Description
We need to accept in-flight acks from an abandoned path, otherwise we
will be retransmitting data that we know the peer already received.
Only ignore the ACKs if we already discarded the path state, in this
case we'll already have marked all the data for retransmission when
the path state was discarded.
## Breaking Changes
n/a
## Notes & open questions
n/a
When `sendmsg` returns ENETUNREACH, EHOSTUNREACH, EADDRNOTAVAIL, or
ENETDOWN, propagate the error as `NetworkUnreachable` instead of
silently returning `Ok(())`.
Previously, all non-WouldBlock send errors were swallowed. This caused
the connection driver to believe packets were sent successfully, leading
to a PTO retransmit busy-loop: the driver would generate loss probes,
"send" them (error swallowed), rearm PTO, and immediately poll again
with `keep_going = true`. On a `current_thread` tokio runtime this
starved all other tasks — timers, application futures, and relay
connections all froze.
The fix follows picoquic's approach: network-unreachable errors are
classified separately from transient errors, propagated to the QUIC
stack, and the connection driver drops the transmit and stops spinning.
The QUIC timer system handles path idle timeout and demotion naturally.
Other send errors (ECONNREFUSED, EPERM, etc.) continue to be swallowed
as before, since QUIC retransmission handles recovery for those cases.
## Description
I keep getting mildly confused about the timer names. This tries to
name them consistently: they all say what happend when the timer
expires. The remaining timer names are from the specs itself AFAIK, so
I left them.
## Breaking Changes
n/a
## Notes & open questions
This is entirely opinionated, feel free to push back.
Because of that review request is an AND.
## Description
This updates noq and its dependencies to rand 0.10 and getrandom 0.4.
## Breaking Changes
I don't think there are any of rand's APIs exposed in noq or noq-proto.
So no breaking changes.
## Notes & open questions
~~Blocked on https://github.com/tomtomwombat/fastbloom/pull/27 being
released.~~ We can disable the rand feature for fastbloom instead.
`proptest` still depends on rand 0.9:
```
$ cargo tree -i rand@0.9
rand v0.9.2
└── proptest v1.9.0
[dev-dependencies]
└── noq-proto v0.16.0 (/home/philipp/program/work/noq/noq-proto)
├── noq v0.17.0 (/home/philipp/program/work/noq/noq)
│ ├── bench v0.2.0 (/home/philipp/program/work/noq/bench)
│ └── perf v0.2.0 (/home/philipp/program/work/noq/perf)
└── perf v0.2.0 (/home/philipp/program/work/noq/perf)
```
But it's only a dev-dependency, so won't get pulled in for people
depending on noq.
## Description
In favor of having a more stable API for noq, the definition of a
network path
needs some changes.
The changes, in short, are that the flow label is always cleaned, and
the scope
id is kept in cases where it might be necessary.
This is possibly the best compromise between RFC9000's address
definition
(§1.2) and the reality of requiring some ipv6 extra fields in the
address for
adecuate routing for the future goal of being able to handle link-local
inclusive topologies, while ensuring comparison between network paths is
meaningful. This last part is necessary to properly handle connection
wide path
and address validation.
## Breaking Changes
`FourTuple`'s fields are no longer public, use `FourTuple::from_remote`
and
`FourTuple::new` to create a `FourTuple` and the `FourTuple::remote` and
`FourTuple::local_ip` accessors instead of direct field access.
## Notes & open questions
n/a
## Description
If there is no other validated path then we allow sending PATH_ABANDON
on the path to be abandoned itself.
Closes#399.
## Breaking Changes
n/a
## Notes & open questions
Please review the logic changes carefully, I had to modify all
conditions and we need to be sure I only added a condition without
modifying previous logic.
I have removed the test because this is blocking other work. I will
PR the test separately. The test succeeds by checking the logs
manually, so the change is correct. There are however changes needed
to be able to assert that in the test.
---------
Co-authored-by: Philipp Krüger <philipp.krueger1@gmail.com>
## Description
On multipath we can not use the is-validating check to know if the
packet belongs to this 4-tuple. However the path generation was
introduced exactly to identify the 4-tuple a packet was sent on, so
use that instead. As a bonus it is much more explicit in intent.
## Breaking Changes
n/a
## Notes & open questions
n/a
* it's the weekend, this is useful in my head
so let's check in totally broken code.
* more
* wip
* wip
* wip
* try more stuff
* moar
* more wip
* can't send anything when there's nothing to send
This stopped us from going to the next space to coalesce the Initial
with the Handshake packet on the server-side. And probably lots of
other stuff
* wip
* and check for loss probes that need to be sent
* some doc updates
* remove SendableFrames::validation, it was redundant
Turns out this was exactly the same case as
SendableFrames::space_id_only.
* Add a test that checks we send on an available path
* simplify the packet scheduling logic a little
* Simplify, immediate close during handshake is broken though
* wip
* Guess we do need a new field for this
Slightly sad to have to make an explicit exception for this, at least
currently.
* clippy
* may_send_data has been made smarter, don't need this
* wip
* tweak logging
* proptests should maybe log errors?
* drive to idle for a 1000 iterations, 100 is a bit small
* turns out we don't need this and i find it a bit confusing
* remove stuff split off into #494
* Remove changes split off into 495
* typo
* style
* ww
* remove this old thing
* tweak
* needs_loss_probe is a variable above, it's not that bad
* collapse these two cases. it's a hard call though
* well doh, the doc comment says why
* wordsmithing, because 90% of packet scheduling is about that
* put this back, split off into another pr
* fix doc comment
* Typos from code review
Co-authored-by: Philipp Krüger <philipp.krueger1@gmail.com>
* Do not allocate and move scheduling info to separate function
Two bits of PR review:
- Move the scheduling to a separate function.
- Avoid an allocation, but this has some tradeoffs.
- I now have two loops that look for the PathId by doing `next_path_id
= self.path.keys().find(|i| **i > path_id).copied();`. It might be
possible to fold the MTU discovery in the main poll loop, MTU packets
would get a slightly higher priority but probably not really harmful
overall.
- I now need to do the computation for
`have_validated_status_available_space many more times.
I'm not sure how much the compiler manages to remove all of that. Is
it smart enough to figure out that
`have_validate_status_available_space` won't change between the calls
and does it move it out? Does it make the iteration as fast as the
previous version?
On the other hand, we now have some situations where we don't have to
compute the scheduling information, and no longer need to compute it
for all paths if we don't send on the last path.
What do you think, which version is better (though I also adopted
@matheus23's feedback about splitting it off to a function, but that
doesn't affect this really. It does make the diff a little bit more
though)?
As an aside, in working out of how scheduling should work it was
really helpful to have to extremely explicit as a bunch of data that's
computed up-front. But it's fair that now we know this is how it
should work that we can implement it in the most optimal way.
* Small attempt at making this clearer
* fix docs
* refactor(proto): ordering of REACH_OUT and OBSERVED_ADDR building
The ordering of these frames was a bit too eager.
REACH_OUT is important timing-wise. But it is not more important than
HANDSHAKE_DONE, PING, IMMEDIATE_ACK, ACK, ACK_FREQUENCY and should
anyway not be sent on a path that also needs
PATH_CHALLENGE. PATH_RESPONSE could be the one exception but it is
also small.
If one of those frames do end up in the same packet as REACH_OUT there
will still be place for the REACH_OUT frame. The CRYPTO frame is left
after it, because after the handshake that is only carrying auxiliarry
non-time-sensitive information, and REACH_OUT is also only possible in
the data space kind.
OBSERVED_ADDRESS is definitely not that high priority, it might need
to move back even further.
---------
Co-authored-by: Philipp Krüger <philipp.krueger1@gmail.com>
* it's the weekend, this is useful in my head
so let's check in totally broken code.
* more
* wip
* wip
* wip
* try more stuff
* moar
* more wip
* can't send anything when there's nothing to send
This stopped us from going to the next space to coalesce the Initial
with the Handshake packet on the server-side. And probably lots of
other stuff
* wip
* and check for loss probes that need to be sent
* some doc updates
* remove SendableFrames::validation, it was redundant
Turns out this was exactly the same case as
SendableFrames::space_id_only.
* Add a test that checks we send on an available path
* simplify the packet scheduling logic a little
* Simplify, immediate close during handshake is broken though
* wip
* Guess we do need a new field for this
Slightly sad to have to make an explicit exception for this, at least
currently.
* clippy
* may_send_data has been made smarter, don't need this
* wip
* tweak logging
* proptests should maybe log errors?
* drive to idle for a 1000 iterations, 100 is a bit small
* turns out we don't need this and i find it a bit confusing
* remove stuff split off into #494
* Remove changes split off into 495
* typo
* style
* ww
* remove this old thing
* tweak
* needs_loss_probe is a variable above, it's not that bad
* collapse these two cases. it's a hard call though
* well doh, the doc comment says why
* wordsmithing, because 90% of packet scheduling is about that
* put this back, split off into another pr
* fix doc comment
* Typos from code review
Co-authored-by: Philipp Krüger <philipp.krueger1@gmail.com>
* Do not allocate and move scheduling info to separate function
Two bits of PR review:
- Move the scheduling to a separate function.
- Avoid an allocation, but this has some tradeoffs.
- I now have two loops that look for the PathId by doing `next_path_id
= self.path.keys().find(|i| **i > path_id).copied();`. It might be
possible to fold the MTU discovery in the main poll loop, MTU packets
would get a slightly higher priority but probably not really harmful
overall.
- I now need to do the computation for
`have_validated_status_available_space many more times.
I'm not sure how much the compiler manages to remove all of that. Is
it smart enough to figure out that
`have_validate_status_available_space` won't change between the calls
and does it move it out? Does it make the iteration as fast as the
previous version?
On the other hand, we now have some situations where we don't have to
compute the scheduling information, and no longer need to compute it
for all paths if we don't send on the last path.
What do you think, which version is better (though I also adopted
@matheus23's feedback about splitting it off to a function, but that
doesn't affect this really. It does make the diff a little bit more
though)?
As an aside, in working out of how scheduling should work it was
really helpful to have to extremely explicit as a bunch of data that's
computed up-front. But it's fair that now we know this is how it
should work that we can implement it in the most optimal way.
* Small attempt at making this clearer
* fix docs
* Rename field based on more feedback
* fix docs
---------
Co-authored-by: Philipp Krüger <philipp.krueger1@gmail.com>
* Allow configuring the runtime type in the bulk bench
Also switch the default to the multithreaded runtime. I think that is more
representative of actual use.
* fmt
* clippy
---------
Co-authored-by: Floris Bruynooghe <flub@n0.computer>
* feat(proto): issue CIDs in order of ascending path ID
We used to issue CIDs in sequence order, but not in reverse order of
path ID. This is not the order in which you need to have CIDs. This
fixes this to always issue them in order of ascending path ID and then
ascending order of sequence ID.
By introducing the newtype to do this, we also ensure that this order
is respected even when retransmits come into play. The newtype keeps
its sorting invariance when retransmits are merged back in.
Another benefit of the newtype is that this order is now enforced in a
specific place. Before it was implicit on the reliance between how the
endpoint ID generated the CIDs, how it then sent them to the
connection and how the connection stored and consumed them. It was
very implicit.
WRT to the cost of doing all the ordered inserts: CIDs are issued
relatively infrequently and usually not in huge numbers. Even
considering we want to increase those numbers in the future I think
using a simple Vec as storage is a decent choice for numbers of a few
100 CIDs that can be expected at most.
Fixes#137.
* fix aws-lc-rs tests, hopefully
* lol
* fix test
* Use cmp::Reverse instead of manual cmp
Co-authored-by: Philipp Krüger <philipp.krueger1@gmail.com>
* Sort less for the sorting gods
This optimises how we sort:
- Sort is still not manually implemented, as tempting it is to
implement insertion sort. I'm a believer of not manually
implementing algorithms.
- The major downside is that all functions need to be aware of the
invariants. Everyone needs to manipulate the fields.
---------
Co-authored-by: Philipp Krüger <philipp.krueger1@gmail.com>
* ci(docs): Check internal docs as well
We also want to check that the internal docs are all correct, so we do
not get broken links etc. We have a lot of internal docs, internal
docs are great!
* turns out that syntax is not supported
* fixup all the doc errors
* fix format
* naming nitpicking, fewer changes
We also have a `connection::send_buffer:proptests` module, which did
not match the nextest naming for the proptest profile. So those tests
were being run in a normal test run.
Because switching that module name to be "proptest" is problematic
since the crate is also called "proptest" and that creates naming
conflicts with the `user super::*` I opted to change the
tests/proptest module to become proptests instead.
* ci: Add ESP32-C3 build check for noq, noq-proto, and noq-udp
Cross-compile check using riscv32imc-esp-espidf target with
nightly + build-std. No ESP-IDF SDK needed for cargo check.
* test: Deliberately break ESP32 build to verify CI catches it
Use unix.rs instead of posix_minimal.rs on espidf to confirm the
esp32_check CI job fails as expected. Revert after verifying.
* Revert "test: Deliberately break ESP32 build to verify CI catches it"
This reverts commit 45f096edc8.
* Correct ci step name
* Use sccache for new ci checks.
* Rename fallback.rs to posix_minimal.rs
Use it for platforms that are unix, but don't support advanced stuff like
CMSG, GRO, GSO.
Also fix some compile errors in the former fallback.rs
* Test noq-udp with the new posix_minimal configuration
* Eliminate warnings for posix_minimal config.
* shut up clippy
* chore: Attempt at drafting a readme
* spelling
* Fix crypto wording
* typo
* remove support for version 2
seems that needs a little more work
* add modern stuff folks demand
* qlog typo
* Try and be slightly less confusing
* WIP
* Prettify parsing code
* Fix error in docs
* clippy
* Some optimizations:
- use a lazy iterator over the ALPNs
- fast path for when the ALPNs are in the first crypto frame
Also some tests for the rarely used assembly stuff.
* Fix doc comments regarding allocations
* Add a newtype wrapper for the decrypted initial packet.
We can then add all the various convenience methods to it, not to the main Incoming.
So even if we have a lot of stuff on it we don't pollute the incoming API.
Also we cache the decryption.
* Extend comments about decrypt
* Switch to `aes-gcm` crate for retry token logic when using rustls
* Split `rustls-ring` and `rustls-aws-lc-rs` features into `rustls`, `ring` and `aws-lc-rs` features.
* Better document the features.
* `cargo make format`
* Fix outdated feature references
* Avoid double-defining `configured_provider`
* File an issue about suddenly working PQC handshakes now breaking a test
* Fix rebase
* Only enable `aes-gcm` dependency when needed
* Add features for backwards compatibility
* Add some links to the spec's retry packet integrity section
Drop the connection state lock before cloning ConnectionRef in the existing-path branch of open_path_ensure, and add a multipath regression test that verifies ensuring an existing path resolves quickly and returns consistent path details.
Floris Bruynooghe