Working 0-RTT, stateless reset, retyping, more efficient datagrams #156
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- By passing `opt::disable_key_verification` in calls to `Endpoint::{listen,connect}(...)` the application can decide whether to use key verification for inbound or outbound connections.
- can be toggled on/off, bespoke types for handling generation and storage - improvements, better handling, callbacks
- ping binaries added to test timing - more thoughtful compile time checking
The copy and move constructors/operators are a decent amount of complexity keeping track of pointers that really isn't needed because we nearly always want these held inside a unique_ptr, and so it simplifies things to just enforce that everywhere and fix the two occurances that held in a value and then sort of mutated into a unique_ptr. This also lets us make the key data const, which gives us better safety assurances for using it as the key of a map (as we currently are). Also makes the private fields actually private, along with the constructors (to enforce all usage now go through make). Also dropped some unused comparison & hashing functions.
- added RAII gnutls_datum object, used where needed. Must be careful, can double free if the memory is owned by another object or held within ngtcp2 - misc fixes from oxen-io#147
- cmake configuration to force fmt > 10 when compiling with clang-19 - misc
- Make it run as many pings as you want over a connection (or forever,
which is now the default) with `-c`.
- Set ping interval with `-i`.
- Add mean, sd, min, max statistics after test finishes
- Make interruption (e.g. via Ctrl-C) close gracefully and print stats.
- Send pings via datagrams instead of streams as the atomicity of a
datagram makes it easier to implement multiple pings rather than
worrying about untangling stream data that could contain multiple (or
non-whole) pings.
- add a `--seed` option for key generation to allow for easier
repeatability for keys and the endpoint static secret (for stateless
reset testing).
- add a `--flakiness` option to the server to make it deliberate miss
some ping responses.
- copy friendly_duration from oxen-core for auto-ranging duration
formatting.
Sample output:
$ ./tests/ping-server --seed "abc" -f 0.1
$ ./tests/ping-client --seed def --remote-pubkey TbjGXzmRqPGBC7xoodC79UfY7IZlhGrzLA6gTZqpUR4= -c 1000 -i 0.001
[2025-01-29 22:06:10] [+0.000s] [quic:info|src/loop.cpp:197] libevent loop is started
[2025-01-29 22:06:10] [+0.000s] [quic-test:info|tests/utils.cpp:133] Generating insecure but reproducible keys from seed string 'def'
[2025-01-29 22:06:10] [+0.000s] [quic-test:info|tests/ping-client.cpp:208] Constructing endpoint on [::]:0
[2025-01-29 22:06:10] [+0.000s] [quic-test:info|tests/ping-client.cpp:254] Connecting to 127.0.0.1:5500...
[2025-01-29 22:06:10] [+0.001s] [quic:info|src/connection.cpp:1827] Successfully created new outbound connection object < RID:1 >
[2025-01-29 22:06:10] [+0.002s] [quic-test:info|tests/ping-client.cpp:193] Connection established to [::ffff:127.0.0.1]:5500 in 1.659ms
[2025-01-29 22:06:10] [+0.002s] [quic-test:info|tests/ping-client.cpp:242] Ping 0 received in 130.302µs
[2025-01-29 22:06:10] [+0.003s] [quic-test:info|tests/ping-client.cpp:242] Ping 1 received in 100.101µs
[2025-01-29 22:06:10] [+0.004s] [quic-test:info|tests/ping-client.cpp:242] Ping 2 received in 102.181µs
[2025-01-29 22:06:10] [+0.005s] [quic-test:info|tests/ping-client.cpp:242] Ping 3 received in 86.021µs
[2025-01-29 22:06:10] [+0.006s] [quic-test:info|tests/ping-client.cpp:242] Ping 4 received in 86.721µs
[2025-01-29 22:06:10] [+0.007s] [quic-test:info|tests/ping-client.cpp:242] Ping 5 received in 100.941µs
...
[2025-01-29 22:06:11] [+0.999s] [quic-test:info|tests/ping-client.cpp:242] Ping 997 received in 131.781µs
[2025-01-29 22:06:11] [+1.000s] [quic-test:info|tests/ping-client.cpp:242] Ping 998 received in 107.471µs
[2025-01-29 22:06:11] [+1.001s] [quic-test:info|tests/ping-client.cpp:242] Ping 999 received in 101.681µs
[2025-01-29 22:06:11] [+1.001s] [quic-test:info|tests/ping-client.cpp:200] Disconnected from [::ffff:127.0.0.1]:5500
[2025-01-29 22:06:11] [+1.001s] [quic:info|src/network.cpp:38] Network shutdown complete
[2025-01-29 22:06:11] [+1.002s] [quic:info|src/loop.cpp:217] Loop shutdown complete
Ping results: 1000 sent, 905 received (90.500%) in 999.178ms
RTT mean: 113.592µs, stdev: 28.335µs, min: 63.051µs, max: 226.012µs
Also adds comments to the vector and span members to make the relationship clear.
The 8 here means write 8 bytes, but we give it a pointer to a 1-byte value.
The way we were dropping connections was not right: these always need to be deferred into the event loop, because they are called from within a Connection itself where dropping is not safe. We had some called that wrapped this in a `call()`, but those weren't quite right either because call() fires immediately when already inside the event loop, as these would be. This thus makes `drop_connection` itself do the call_soon so that we can safely use it directly, and renames the existing direct, unsafe one to `_drop_connection`. This also adds a missing set_closing(): without it, a drop_connection can trigger a second attempt to close or drain the connection, which results in an erroneous double-fire of the close callback and a potential segfault from entering code multiple times that we shouldn't be.
This commit fixes various issues in the current WIP stateless reset
implementation, getting it to a state where it now works reliably, after
pouring through ngtcp2 and the RFC's instructions and requirements
around stateless reset.
- Lower max_active_cids to 4. ngtcp2 only allows changing cids when
migrating, but does not expose anything to allow us to rotate manually
(which is somewhat annoying), and so 8 seems excessive.
- Drop the option to disable stateless reset; there is no reason to not
always handle and produce stateless reset tokens because, at worst,
the stateless tokens just don't do anything if the other end doesn't
support them. Previously there were problems around them (such as an
infinite loop) but those are resolved by this PR.
- Hash stateless reset tokens rather than storing/looking them up
directly, as recommended by the RFC.
- Avoid shared_ptr in reset token lookups; it will basically make
lookups always fail because == values in different shared_ptr's do not
hash or compare equal.
- Drop reset token expiry. Connection IDs (and their reset tokens) can
be long-lived and so there is no particular expiry value that makes
sense; instead we should clean them up when they are retired or the
connection that owns them closes.
- Replace `gtls_reset_token` with a simpler type that holds only the
token. `gtls_reset_token` was also holding (and comparing) random
data, but it doesn't work properly for that purpose because the amount
of random data we require varies based on the incoming packet size
(see next item) and can be generated on the fly when needed.
- Fix infinite loop in stateless reset by following the RFC's
recommendation to always make the stateless reset packet smaller than
the packet that triggered it, and to not drop below 41 byte stateless
reset packets.
- This was the cause of high CPU usage on lokinet testnet restarts:
existing connections were infinite looping stateless reset packets
back and forth (each new stateless reset would illicit a returned
stateless reset).
- Avoid double-lookup of stateless reset tokens by having them map
directly to the ConnectionID rather than the dcid: we need to be able
to go from reset token -> connection, but don't actually care about
looking up by dcid. (We do need an ability to get any active reset
tokens for a connection, during connection destruction, but that is
easily done by storing a container of active reset tokens inside the
Connection).
- Extract stateless reset checking from accept_initial_connection.
Having it be part of accept_initial_connection caused problems:
- when accept_initial_connection returned a pointer, the code calling
it assumed that meant it was a new connection, but with stateless
reset handling embedded it could also mean it matched a stateless
reset.
- stateless reset *is* allowed to run in reverse (i.e. from client to
server) and so shouldn't be guarded inside an if-accepts-connection.
It's somewht less common, and it doesn't work for the initial DCIC
provided by the client, but it otherwise fully allowed by the RFC,
and is useful for Lokinet where "client" and "servers" are really
just both server peers.
- Removed the stateless reset test case because it was only really
testing the opt:: value (which was removed). Unfortunately it doesn't
seem trivial to do automatic stateless reset testing; testing of this
PR was done manually using the ping-server/ping-client scripts with
uncleanly killed server restarts.
- Adds a new packet delayer class to the test suite so that we can meaningfully test on the number of round-trip-times things are taking. - Adds tests of various 0-RTT situations.
jagerman
changed the title
Various stream and datagram methods take a bspan (or string_view) with a
keepalive, where the keepalive defaults to nullptr. This is error-prone
as the keep-alive is *required* to keep the span alive, and if you don't
want or need it, you should need to be explicit about that by passing an
explicit nullptr. Otherwise code like this is silently broken:
std::vector<std::byte> stuff;
// insert things into stuff
stream->send(stuff);
and that is much too dangerous for a public API to allow. With this
change that above no longer compiles and you need to change either to
move the vector into the send call (at which point we make the keepalive
for you) or provide some other object that allows libquic to guarantee
that the data will remain valid.
- Adds `--seed` argument (from ping-client/server) to speedtest and dgram-speed for reproducible pubkeys. - Generalizes zerortt flag/storage/etc. from ping client/server and adds it to speedtest and dgram-speed. - Adds `--remote-seed` to test clients to compute server pubkey on the fly, i.e. `--remote-seed abc` will derive the server pubkey for a server started with `--seed abc`, which makes it easier to test without needing to copy over the actual pubkey. - Move duplicated speedtest code into utils. - Add different ALPNs to different speed tests. - Make default ports distinct across different speed test servers. - Remove unimplemented reverse and bidir options from speedtest-client
It's a bit annoying to have to restart it for each new test. This makes it stick around.
If split datagrams arrive out of order (i.e. second part first), there were being reassembled SECONDFIRST instead of FIRSTSECOND. This fixes it, and simplifies the code around reassembly a bit. Diagnosing this required reworking the datagram code quite a bit to be able to figure out where things were going wrong, but those changes seem useful to leave in place: - instead of sending the same datagram, we now send a 1-250 offset from the byte rainbow table, and then send from offset 0 for the final packet. - Added opt-in server code (-V) to verify received packets. - Added --shutdown server option to make it exit after a verification or fidelity failure. - Add `--lookahead` option to dgram speed client to allow configuring the split packet lookahead intensity.
What we had was failing with newer ngtcp2, and was failing with existing ngtcp2 if a too-old gnutls system package was installed. This fixes it by adding a full FindGnuTLS.cmake during StaticBuild so that ngtcp2 will load from that and ignore the system one.
17 isn't available in bookworm (which is what our lint ci image is based on), but 19 is.
The stateless reset code was moved to a separate 016 test.
jagerman
force-pushed
the
0rtt-debug
branch
2 times, most recently
from
ad6eb17 to
5052b80
Compare
macOS really shows its pathetic slowness here with packets to localhost taking an few orders of magnitude longer than on Windows or Linux. But I'm probably just holding it wrong. Raspberry Pis also need the same slowdown, but given their price and speed this is far more reasonable than it is under macos.
If a request length such as "43:" got split across two packets we weren't recombining them properly because of this bad substr (introduced in this PR, not in merged code).
This updates the code to make all gnutls includes (both explicit and implicit) confined to to quic/gnutls_crypto.hpp header and the source. This makes gnutls an optional dev dependency: i.e. application code that doesn't need the gnutls_crypto bits can use libquic without requiring gnutls headers.
gcc 11 failed to properly accept the above in an initializer_list<string>; this takes raw C strings instead which still accomplishes the intended purpose.
The signal code in here doesn't work so just don't build the client until/unless someone wants to figure out how to do something similar in hell.
jagerman
force-pushed
the
0rtt-debug
branch
2 times, most recently
from
8eb6c66 to
8bad931
Compare
The wine test is currently failing for an unknown reason, but not working under wine is not currently enough to warrant not merging.
We need nettle for sha3/shake which we use for reset tokens.
The datagram-received callback should not have been set in the first place if datagrams aren't enabled. (Also this runtime check was missing a return).
This adjusts `conns` and `conn_lookup` in a couple of ways: - conns becomes a unordered_map. There is no reason we need conns to be ordered, and an unordered_map ought to yield better performance with many connections. - conn_lookup now holds a Connection shared_ptr instead of a ConnectionID so that we can bypass `conns` lookup entirely in the every-packet case: while previously we had to do two (qcid -> ConnectionID -> Connection), now we can go directly from qcid -> Connection. (And if we need the Connection ID, it's already inside the Connection).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This merges #152 and #153 along with various fixes to stateless reset and 0-RTT, addressing numerous issues. See individual commits for more details:
=nullptrto view/span method keepalives because it is too dangerous.Closes #152
Closes #153