* fix: Don't process timers during Closing or Draining
This doesn't seem to cause any packets to go out, but is a bit wasteful
(and also confused me when I saw timers fire in the logs...)
* Suggestion from @martinthomson
* fix: qlog `reference_time` should be in msec
https://datatracker.ietf.org/doc/html/draft-ietf-quic-qlog-main-schema#section-7.1
* Update neqo-common/src/qlog.rs
Co-authored-by: Max Inden <mail@max-inden.de>
Signed-off-by: Lars Eggert <lars@eggert.org>
* Undo
---------
Signed-off-by: Lars Eggert <lars@eggert.org>
Co-authored-by: Max Inden <mail@max-inden.de>
* fix: Send ACKs during handshake after they are lost
This is not yet complete. It works for the first RTX of a lost server
Initial, which now contains the same data as the original
transmission. A third RTX only contains pings. Something is off about
our probe logic. Will continue investigating, but want to see what
the bench and QNS impacts of this change are.
* Improvements
* Remove debug output
* Suggestion from @mxinden
* doc comment
* Check if `L1`/`C1` improve if this is also done in the Handshake space
* Update comment
* ACK more aggressively during the handshake
* Undo
* Update neqo-transport/src/tracking.rs
Signed-off-by: Lars Eggert <lars@eggert.org>
* Update neqo-transport/src/recovery/mod.rs
Signed-off-by: Lars Eggert <lars@eggert.org>
---------
Signed-off-by: Lars Eggert <lars@eggert.org>
* fix: Check for error when looking up packet number space
The crash signatures don't really give much of an indication of where
exactly the issue is. Am kinda guessing it's this `unwrap` of the
packet number space which was based on unverified network input
(via `PacketNumberSpace::from`).
Possible fix for #1363.
* Fewer `unwrap`s with `AckTracker::get_mut`
* Log error
* fix: Handle H3 grease stream types
Doesn't work yet, since it stops reading from the stream afterwards.
Fixes#1991
* Add test and fix bug
* Handle non-HEADERS first frames
* Update neqo-http3/src/stream_type_reader.rs
Co-authored-by: Martin Thomson <mt@lowentropy.net>
Signed-off-by: Lars Eggert <lars@eggert.org>
* Suggestion from @martinthomson
* In debug builds, add a grease frame before the headers.
* Revert "In debug builds, add a grease frame before the headers."
This is breaking too many tests.
This reverts commit 7b9ea7ff7e.
* Simplify
* HFrameType is a struct now
* Bump coverage
---------
Signed-off-by: Lars Eggert <lars@eggert.org>
Co-authored-by: Martin Thomson <mt@lowentropy.net>
* feat: Use `EnumMap` for `AckTracker`
Hopefully addresses @martinthomson's `TODO` about this.
* Minimize diff
* Fix
* More
* Add test
* Suggestions from @martinthomson
* More
* fix: Check whether CIDs are empty
WIP
Fixes#1429
* Update neqo-transport/src/path.rs
Co-authored-by: Martin Thomson <mt@lowentropy.net>
Signed-off-by: Lars Eggert <lars@eggert.org>
* Suggestion from @martinthomson
* Update neqo-transport/src/qlog.rs
Co-authored-by: Max Inden <mail@max-inden.de>
Signed-off-by: Lars Eggert <lars@eggert.org>
* Suggestion from @mxinden
@mxinden, is `take()` the way to go here?
* Log error
* Fix test
* Simplify test
---------
Signed-off-by: Lars Eggert <lars@eggert.org>
Co-authored-by: Martin Thomson <mt@lowentropy.net>
Co-authored-by: Max Inden <mail@max-inden.de>
* refactor: consolidate NeqoQlog instantiation
Previously `neqo_transport::Server`, `neqo_bin::Client` and
[`neqo_glue`](https://searchfox.org/mozilla-central/rev/7279a1df13a819be254fd4649e07c4ff93e4bd45/netwerk/socket/neqo_glue/src/lib.rs#283-315)
had each their own but very similar way of instantiating `NeqoQlog`.
This commit consolidates the instantiation logic into a single function in
`neqo_common`.
* Add simple test for client and server to write qlog file
* clippy
* Replace : with _ in filename on Windows
---------
Co-authored-by: Lars Eggert <lars@eggert.org>
* fix(transport): don't pace below timer granularity
Neqo assumes a timer granularity of 1ms:
0eb9174d7a/neqo-transport/src/rtt.rs (L25-L27)
but `neqo_transport::Pacer::next()` might return values `< GRANULARITY`.
0eb9174d7a/neqo-transport/src/pace.rs (L71-L90)
Under the assumption that a timer implementation rounds small values up to its
granularity (e.g. 1ms), packets can be delayed significantly more than intended
by `Pacer`.
With this commit `Pacer` does not delay packets that would previously be delayed
by less than `GRANULARITY`. The downside is loss in pacing granularity.
See also:
- google/quiche
- 60aec87316/quiche/quic/core/congestion_control/pacing_sender.cc (L167)
- 60aec87316/quiche/quic/core/quic_constants.h (L304)
- quic-go
- d1f9af4cc6/internal/protocol/params.go (L137)
- `kGranularity` in RFC 9002 https://datatracker.ietf.org/doc/html/rfc9002#name-constants-of-interest
* Address suggestions
* Add test
* Fix path_forwarding_attack test
Pacing on new path is now below granularity and thus packet on new path is send
immediately. Calling `skip_pacing` will instead fast forward to the PTO of the
old path to expire, thus leading to an unexpected probe packet on the old path.
```
thread 'connection::tests::migration::path_forwarding_attack' panicked at test-fixture/src/assertions.rs:153:5:
assertion `left == right` failed
left: [fe80::1]:443
right: 192.0.2.1:443
```
This commit simply removes the no longer needed `skip_pacing` step, thus
reverting to the previous behavior.
* clippy
---------
Co-authored-by: Lars Eggert <lars@eggert.org>
* test(udp): assert ignoring of empty datagram
* fix(udp): ignore empty datagram
When receiving an emtpy datagram `meta.len` and `meta.stride` would be `0`.
Chunking the receive buffer via `.chunks(0)` would panic with:
```
chunk size must be non-zero
```
See also panic docs on `slice::chunks`:
https://doc.rust-lang.org/std/primitive.slice.html#method.chunks
With this commit `recv_inner` ignores the empty datagram.
In addition, under the assumption that an empty datagram is not a faulty event,
`recv_inner` attempts another receive call on the socket. This ensures that the
socket eventually returns `WouldBlock` and is thus registered for the next
wake-up with the corresponding event loop (e.g. tokio`).
In the `path_forwarding_attack` unit test
``` rust
/// This simulates an attack where a valid packet is forwarded on
/// a different path. This shows how both paths are probed and the
/// server eventually returns to the original path.
fn path_forwarding_attack() {
```
data is send on two paths, a IPv6 path (aka. "old") and an IPv4 path (aka. "new").
Towards the end of the test it asserts one path, but mentions the other in the
doc comment.
This commit fixes the mix-up in the doc comment.
* neqo-crypto: use SSL_PeerCertificateChainDER
The SSL_PeerCertificateChainDER function was added in NSS 3.103 to allow
an application to retrieve the peer's presented certificate chain without
constructing CERTCertificates. This is expected to improve performance,
as constructing a CERTCertificate will typically involve querying the
NSS certificate database.
* make null_safe_slice generic over pointer type
* handle len == 0 case in null_safe_slice
* use slice::Iter as SliceIter
* use into_iter in test for better code coverage
---------
Co-authored-by: Lars Eggert <lars@eggert.org>
Co-authored-by: Martin Thomson <mt@lowentropy.net>
Discussed yesterday with @KershawChang. @martinthomson, are you OK with this? @mxinden is pretty actively reviewing, so it might unblock getting PRs merged when both of you are busy.
Signed-off-by: Lars Eggert <lars@eggert.org>
* test(transport): handshake delay with ecn blackhole
This commit adds a test where a client connects to a server over a connection
dropping all ECN marked datagrams (ECN blackhole) in both directions, asserting
43 RTT to detect ECN blackhole, disable ECN and eventually establish connection.
* Bail out of ECN validation after three failed Initials
* Update neqo-transport/src/ecn.rs
Co-authored-by: Max Inden <mail@max-inden.de>
Signed-off-by: Lars Eggert <lars@eggert.org>
* Update neqo-transport/src/ecn.rs
Co-authored-by: Max Inden <mail@max-inden.de>
Signed-off-by: Lars Eggert <lars@eggert.org>
* Update neqo-transport/src/ecn.rs
Co-authored-by: Max Inden <mail@max-inden.de>
Signed-off-by: Lars Eggert <lars@eggert.org>
* Update neqo-transport/src/ecn.rs
Co-authored-by: Max Inden <mail@max-inden.de>
Signed-off-by: Lars Eggert <lars@eggert.org>
* Update neqo-transport/src/ecn.rs
Co-authored-by: Max Inden <mail@max-inden.de>
Signed-off-by: Lars Eggert <lars@eggert.org>
* Fixes
* clean-up tests
---------
Signed-off-by: Lars Eggert <lars@eggert.org>
Co-authored-by: Lars Eggert <lars@eggert.org>
* fix(benches/transfer): remove throughput
The `neqo-transport/benches/transfer.rs` benchmarks use the
`test-fixture/src/sim` simulator. The simulator can travel in time, i.e. it
simulates time.
The _wall-clock time_ of a single benchmark run is not the amount of time it took to
transfer `TRANSFER_AMOUNT`. The _simulated time_ is the amount of time it took
to transfer `TRANSFER_AMOUNT`.
`criterion` will use the _wall-clock time_, not the _simulated time_ to
calculate the throughput based on `TRANSFER_AMOUNT`. The resulting throughput
number is not meaningful.
This commit removes the call to `group.throughput`, thus removing the misleading
`criterion` throughput reporting.
* Add comment
---------
Co-authored-by: Lars Eggert <lars@eggert.org>
In the following regex there are 6 capture groups.
```
perl -p -0777 -e 's/(.*?)\n(.*?)(((No change|Change within|Performance has).*?)(\nFound .*?)?)?\n\n/<details><summary>$1: $4<\/summary><pre>\n$2$4$6<\/pre><\/details>\n/gs' |\
```
Capture group 5 is nested within capture group 4:
```
((No change|Change within|Performance has).*?)
```
Capture group 5 is already printed as part of capture group 4 in the first line.
This commit drops capture group 5.
Lars Eggert
Martin Thomson
dependabot[bot]
Max Inden
John Schanck