feat: Randomize CI packet number #2499
Conversation
WIP. Two potential issues: 1. Did I mess up the validation in `CryptoDxState::continuation()`? 2. See the `FIXME` in `Version::confirm_version()`. Fixes mozilla#2462 CC @omansfeld
Failed Interop TestsQUIC Interop Runner, client vs. server, differences relative to fd50a23. neqo-latest as client
neqo-latest as server
All resultsSucceeded Interop TestsQUIC Interop Runner, client vs. server neqo-latest as client
neqo-latest as server
Unsupported Interop TestsQUIC Interop Runner, client vs. server neqo-latest as client
neqo-latest as server
|
Benchmark resultsPerformance differences relative to eb3e2a6. decode 4096 bytes, mask ff: No change in performance detected. time: [11.865 µs 11.911 µs 11.962 µs]
change: [+0.0023% +0.3730% +0.7267%] (p = 0.05 > 0.05)
decode 1048576 bytes, mask ff: No change in performance detected. time: [3.0668 ms 3.0760 ms 3.0868 ms]
change: [-0.2914% +0.1283% +0.5644%] (p = 0.55 > 0.05)
decode 4096 bytes, mask 7f: No change in performance detected. time: [19.767 µs 19.813 µs 19.865 µs]
change: [-0.2894% +0.1264% +0.6193%] (p = 0.60 > 0.05)
decode 1048576 bytes, mask 7f: No change in performance detected. time: [5.1412 ms 5.1520 ms 5.1634 ms]
change: [-0.3776% -0.0581% +0.2623%] (p = 0.73 > 0.05)
decode 4096 bytes, mask 3f: No change in performance detected. time: [6.8760 µs 6.9108 µs 6.9562 µs]
change: [-0.1045% +0.3173% +0.8251%] (p = 0.19 > 0.05)
decode 1048576 bytes, mask 3f: No change in performance detected. time: [1.7551 ms 1.7607 ms 1.7676 ms]
change: [-0.4074% +0.0636% +0.6146%] (p = 0.86 > 0.05)
1 streams of 1 bytes/multistream: No change in performance detected. time: [68.798 µs 69.360 µs 70.371 µs]
change: [-1.2242% +0.8016% +2.9149%] (p = 0.53 > 0.05)
1000 streams of 1 bytes/multistream: Change within noise threshold. time: [24.728 ms 24.767 ms 24.806 ms]
change: [+0.7068% +0.9091% +1.1247%] (p = 0.00 < 0.05)
10000 streams of 1 bytes/multistream: 💔 Performance has regressed. time: [1.6641 s 1.6660 s 1.6679 s]
change: [+1.0095% +1.1630% +1.3195%] (p = 0.00 < 0.05)
1 streams of 1000 bytes/multistream: No change in performance detected. time: [70.279 µs 70.851 µs 71.889 µs]
change: [-3.7524% -1.1530% +1.1388%] (p = 0.43 > 0.05)
100 streams of 1000 bytes/multistream: 💔 Performance has regressed. time: [3.3342 ms 3.3406 ms 3.3474 ms]
change: [+3.5920% +3.9093% +4.2248%] (p = 0.00 < 0.05)
1000 streams of 1000 bytes/multistream: 💔 Performance has regressed. time: [145.70 ms 145.78 ms 145.88 ms]
change: [+6.2013% +6.2904% +6.3845%] (p = 0.00 < 0.05)
coalesce_acked_from_zero 1+1 entries: No change in performance detected. time: [92.503 ns 92.815 ns 93.133 ns]
change: [-0.1145% +2.4447% +8.9192%] (p = 0.30 > 0.05)
coalesce_acked_from_zero 3+1 entries: No change in performance detected. time: [110.04 ns 110.34 ns 110.68 ns]
change: [-0.0131% +1.5025% +4.2996%] (p = 0.23 > 0.05)
coalesce_acked_from_zero 10+1 entries: No change in performance detected. time: [110.19 ns 110.77 ns 111.43 ns]
change: [-0.0848% +1.3116% +3.4977%] (p = 0.18 > 0.05)
coalesce_acked_from_zero 1000+1 entries: No change in performance detected. time: [91.468 ns 91.504 ns 91.545 ns]
change: [-1.2043% -0.1532% +0.8703%] (p = 0.79 > 0.05)
RxStreamOrderer::inbound_frame(): Change within noise threshold. time: [115.32 ms 115.37 ms 115.41 ms]
change: [-0.1802% -0.1216% -0.0643%] (p = 0.00 < 0.05)
SentPackets::take_ranges: No change in performance detected. time: [5.3141 µs 5.4993 µs 5.6994 µs]
change: [-16.718% -4.6892% +4.2003%] (p = 0.60 > 0.05)
transfer/pacing-false/varying-seeds: Change within noise threshold. time: [34.386 ms 34.451 ms 34.517 ms]
change: [-1.0133% -0.7358% -0.4605%] (p = 0.00 < 0.05)
transfer/pacing-true/varying-seeds: Change within noise threshold. time: [34.410 ms 34.469 ms 34.527 ms]
change: [-1.0390% -0.7966% -0.5473%] (p = 0.00 < 0.05)
transfer/pacing-false/same-seed: Change within noise threshold. time: [34.795 ms 34.856 ms 34.916 ms]
change: [+0.3264% +0.5567% +0.7844%] (p = 0.00 < 0.05)
transfer/pacing-true/same-seed: Change within noise threshold. time: [34.547 ms 34.602 ms 34.657 ms]
change: [-2.0059% -1.7627% -1.5087%] (p = 0.00 < 0.05)
1-conn/1-100mb-resp/mtu-1504 (aka. Download)/client: Change within noise threshold. time: [2.1900 s 2.1975 s 2.2053 s]
thrpt: [45.345 MiB/s 45.505 MiB/s 45.663 MiB/s]
change:
time: [-1.6458% -1.0290% -0.4638%] (p = 0.00 < 0.05)
thrpt: [+0.4660% +1.0397% +1.6733%]
1-conn/10_000-parallel-1b-resp/mtu-1504 (aka. RPS)/client: No change in performance detected. time: [387.29 ms 389.31 ms 391.36 ms]
thrpt: [25.552 Kelem/s 25.686 Kelem/s 25.821 Kelem/s]
change:
time: [-0.9330% -0.1693% +0.6008%] (p = 0.66 > 0.05)
thrpt: [-0.5972% +0.1696% +0.9418%]
1-conn/1-1b-resp/mtu-1504 (aka. HPS)/client: 💔 Performance has regressed. time: [29.233 ms 30.064 ms 30.882 ms]
thrpt: [32.382 elem/s 33.263 elem/s 34.208 elem/s]
change:
time: [+1.6212% +5.5274% +9.5398%] (p = 0.01 < 0.05)
thrpt: [-8.7090% -5.2379% -1.5953%]
1-conn/1-100mb-resp/mtu-1504 (aka. Upload)/client: 💔 Performance has regressed. time: [3.5594 s 3.5820 s 3.6069 s]
thrpt: [27.725 MiB/s 27.918 MiB/s 28.095 MiB/s]
change:
time: [+10.109% +11.075% +12.085%] (p = 0.00 < 0.05)
thrpt: [-10.782% -9.9709% -9.1805%]
Client/server transfer resultsPerformance differences relative to eb3e2a6. Transfer of 33554432 bytes over loopback, 30 runs. All unit-less numbers are in milliseconds.
|
Signed-off-by: Lars Eggert <[email protected]>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2499 +/- ##
==========================================
- Coverage 94.92% 94.91% -0.02%
==========================================
Files 115 115
Lines 34370 34465 +95
Branches 34370 34465 +95
==========================================
+ Hits 32625 32711 +86
- Misses 1736 1747 +11
+ Partials 9 7 -2
|
larseggert
requested review from
KershawChang,
martinthomson and
mxinden
as code owners
neqo-transport/src/crypto.rs
Outdated
| @@ -979,15 +988,21 @@ impl CryptoStates { | |||
| Role::Server => (SERVER_INITIAL_LABEL, CLIENT_INITIAL_LABEL), | |||
| }; | |||
|
|
|||
| let min_pn = if randomize_ci_pn { | |||
| packet::Number::from((random::<1>()[0] >> 4) + 1) | |||
There was a problem hiding this comment.
Why skip 0? Isn't it a good idea to include that in the range? If you want lower odds: (r >> 4) + (r & 0xf) returns a value of 0 pretty rarely. Though I tend to like (r >> 3) + (r & 0x7) for keeping it in one byte.
Also, what logic are you using to decide that 16 is the largest value? Obviously, if we bump this up to 64, we'll have to contend with the whole thing where the packet number needs to be longer. But that seems harmless. We use largest_acknowledged, so it's probably fine to use any starting number. And we can afford to spend a byte on this.
Given that we are safe in including any value here, could we instead do let r = random::<2>(); r[0] & r[1] ? That has a 7/16 chance of slipping into the next byte, but no more than that. It also pushes more toward having smaller values overall.
There was a problem hiding this comment.
I skipped zero, so that it would be easy to do a test for it (which I then failed to do.)
The logic behind 16 was simply to pick a number that gave us some small randomization without eating up too much of the 0-255 range. Am very open to doing something different.
There was a problem hiding this comment.
There is now a test.
There was a problem hiding this comment.
You should make it more random, I think. It doesn't hurt if this uses more of the range. Even beyond 63. Though I'd aim for something that did less than 63 most times, either way.
There was a problem hiding this comment.
I tried your last suggestion, but we then need to fix the reorder_handshake test, which currently fails when we get two-byte packet numbers. Going with
packet::Number::from((r >> 3) + (r & 0x7)) + 1There was a problem hiding this comment.
I can't easily see how reorder_handshake fails. If the failure happens after two-bytes, just disable the initial packet number tweak for that test.
There was a problem hiding this comment.
thread 'connection::tests::handshake::reorder_handshake' (12105956) panicked at neqo-transport/src/connection/tests/handshake.rs:566:5:
assertion `left == right` failed
left: 1
right: 3
I.e., it's only receiving one packet by then.
I can make the change, would then do
let r = random::<2>();
(r[0] & r[1]) + 1There was a problem hiding this comment.
@martinthomson so, about that failing test, I wonder if we actually have a bug somewhere. What seems to happen with two-byte packet numbers is that Handshake keys are not available after the
client.process_input(s_initial_2, now);line (neqo-transport/src/connection/tests/handshake.rs:564), whereas they are available (and saved packets are hence processed) with single-byte packet numbers. Odd...
neqo-transport/src/crypto.rs
Outdated
| @@ -979,15 +988,21 @@ impl CryptoStates { | |||
| Role::Server => (SERVER_INITIAL_LABEL, CLIENT_INITIAL_LABEL), | |||
| }; | |||
|
|
|||
| let min_pn = if randomize_ci_pn { | |||
| packet::Number::from((random::<1>()[0] >> 4) + 1) | |||
There was a problem hiding this comment.
You should make it more random, I think. It doesn't hurt if this uses more of the range. Even beyond 63. Though I'd aim for something that did less than 63 most times, either way.
Client/server transfer resultsPerformance differences relative to f1df423. Transfer of 33554432 bytes over loopback, min. 100 runs. All unit-less numbers are in milliseconds.
Download data for |
Benchmark resultsPerformance differences relative to f1df423. 1-conn/1-100mb-resp/mtu-1504 (aka. Download)/client: Change within noise threshold. time: [206.80 ms 207.31 ms 207.95 ms]
thrpt: [480.88 MiB/s 482.38 MiB/s 483.57 MiB/s]
change:
time: [+0.5946% +0.9179% +1.2714%] (p = 0.00 < 0.05)
thrpt: [−1.2554% −0.9096% −0.5911%]
1-conn/10_000-parallel-1b-resp/mtu-1504 (aka. RPS)/client: Change within noise threshold. time: [303.15 ms 304.53 ms 305.92 ms]
thrpt: [32.689 Kelem/s 32.837 Kelem/s 32.987 Kelem/s]
change:
time: [−1.7236% −1.0745% −0.4304%] (p = 0.00 < 0.05)
thrpt: [+0.4322% +1.0862% +1.7539%]
1-conn/1-1b-resp/mtu-1504 (aka. HPS)/client: No change in performance detected. time: [28.132 ms 28.247 ms 28.376 ms]
thrpt: [35.241 B/s 35.402 B/s 35.546 B/s]
change:
time: [−0.8420% −0.0444% +0.6253%] (p = 0.92 > 0.05)
thrpt: [−0.6214% +0.0444% +0.8492%]
1-conn/1-100mb-req/mtu-1504 (aka. Upload)/client: 💔 Performance has regressed. time: [208.69 ms 208.97 ms 209.31 ms]
thrpt: [477.76 MiB/s 478.54 MiB/s 479.19 MiB/s]
change:
time: [+2.1152% +2.4115% +2.6711%] (p = 0.00 < 0.05)
thrpt: [−2.6016% −2.3547% −2.0714%]
decode 4096 bytes, mask ff: No change in performance detected. time: [11.624 µs 11.666 µs 11.712 µs]
change: [−1.0150% −0.4196% +0.1159%] (p = 0.16 > 0.05)
decode 1048576 bytes, mask ff: No change in performance detected. time: [3.0068 ms 3.0162 ms 3.0274 ms]
change: [−0.7423% −0.1456% +0.4175%] (p = 0.64 > 0.05)
decode 4096 bytes, mask 7f: No change in performance detected. time: [19.378 µs 19.431 µs 19.488 µs]
change: [−0.4198% +0.0597% +0.5116%] (p = 0.81 > 0.05)
decode 1048576 bytes, mask 7f: No change in performance detected. time: [5.0857 ms 5.0971 ms 5.1099 ms]
change: [−0.4551% −0.0635% +0.3313%] (p = 0.76 > 0.05)
decode 4096 bytes, mask 3f: No change in performance detected. time: [5.5307 µs 5.5586 µs 5.5928 µs]
change: [−0.5146% +0.0795% +0.7571%] (p = 0.82 > 0.05)
decode 1048576 bytes, mask 3f: No change in performance detected. time: [1.7579 ms 1.7594 ms 1.7624 ms]
change: [−0.0063% +0.0848% +0.3157%] (p = 0.31 > 0.05)
coalesce_acked_from_zero 1+1 entries: No change in performance detected. time: [88.477 ns 88.796 ns 89.104 ns]
change: [−0.7817% −0.3812% +0.0541%] (p = 0.06 > 0.05)
coalesce_acked_from_zero 3+1 entries: Change within noise threshold. time: [105.65 ns 105.96 ns 106.29 ns]
change: [−1.0977% −0.7066% −0.3470%] (p = 0.00 < 0.05)
coalesce_acked_from_zero 10+1 entries: No change in performance detected. time: [105.20 ns 105.69 ns 106.27 ns]
change: [−1.4938% −0.5767% +0.3480%] (p = 0.22 > 0.05)
coalesce_acked_from_zero 1000+1 entries: No change in performance detected. time: [89.176 ns 92.788 ns 100.92 ns]
change: [−1.5064% +1.8440% +7.2353%] (p = 0.65 > 0.05)
RxStreamOrderer::inbound_frame(): Change within noise threshold. time: [107.99 ms 108.08 ms 108.18 ms]
change: [−0.6893% −0.4848% −0.3110%] (p = 0.00 < 0.05)
sent::Packets::take_ranges: :green_heart: Performance has improved. time: [5.0716 µs 5.1701 µs 5.2758 µs]
change: [−42.641% −36.626% −25.438%] (p = 0.00 < 0.05)
transfer/pacing-false/varying-seeds: Change within noise threshold. time: [36.989 ms 37.074 ms 37.170 ms]
change: [+0.0643% +0.4117% +0.7689%] (p = 0.02 < 0.05)
transfer/pacing-true/varying-seeds: Change within noise threshold. time: [37.832 ms 37.939 ms 38.051 ms]
change: [+0.3177% +0.7468% +1.1725%] (p = 0.00 < 0.05)
transfer/pacing-false/same-seed: Change within noise threshold. time: [36.727 ms 36.801 ms 36.891 ms]
change: [+0.4516% +0.7648% +1.1022%] (p = 0.00 < 0.05)
transfer/pacing-true/same-seed: No change in performance detected. time: [38.263 ms 38.357 ms 38.456 ms]
change: [−0.5487% −0.1922% +0.1659%] (p = 0.29 > 0.05)
Download data for |
|
Nice way to find a boundary condition, @larseggert. Our tests operate with a 1232-byte MTU. In this particular test, the packet was being filled completely and precisely. The ServerHello is 1178 bytes. In a CRYPTO frame, that's 1182 bytes. Add an ACK frame with a one-byte largest acknowledged and a gap for the dropped packet (7 bytes), the header (27) and an auth tag (16) and you get EXACTLY 1232 bytes. In this case, we're adding one byte to the size of the ACK frame because the largest acknowledged needs two bytes to encode. So the CRYPTO frame needs to have its last byte cut off to fit.
The challenge here is that we have lost ONE packet, but we need to transmit TWO packets to repair the loss properly. But our PTO code only sends two packets for Application packets, whereas it only sends ONE for Initial and Handshake packets. That is because we want to avoid pointless PING-only packets when repairing the handshake. So we limited handshake PTOs to a single packet. That means that the PTO code moves on to sending a Handshake packet, rather than sending the last byte of that CRYPTO frame. If we had sent TWO packets, we could maybe have made the PTO code pay attention to the number of packets that were marked as needing retransmission, but in this very narrow case, we need to repair the loss of one packet with two. The alternative is to add some smarts to check for outstanding CRYPTO data when sending a second PTO, rather than capping the number of PTOs to 1. A hacky workaround here might be to increase the MTU. Though that just defers the problem for later. |
This is mostly the work of @larseggert, I'm just repackaging it. This will target his branch with any changes. This randomizes the starting packet number the client uses for the Initial packet number space. We don't randomize this on the server, since otherwise we'd need even more changes to the tests to account for that. Fixes mozilla#2462. Closes mozilla#2499.
This randomizes the starting packet number the client uses for the Initial packet number space.
We don't randomize this on the server, since otherwise we'd need even more changes to the tests to account for that.
Fixes #2462
CC @omansfeld