fix: Drop of Initial space only after sending 1st Handshake #2118
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
This is what RFC9000 says, and it helps with servers that expect ACKs in the Initial packet number space to happen. It also eliminates a difference in behavior we had when a client received an Initial packet containing an ACK and a CRYPTO frame vs. a CRYPTO frame and an ACK, where that CRYPTO frame caused the Initial packet number space to be dropped. Broken out of mozilla#1998
larseggert
requested review from
KershawChang,
martinthomson and
mxinden
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2118 +/- ##
==========================================
- Coverage 95.38% 95.35% -0.03%
==========================================
Files 112 112
Lines 36593 36590 -3
==========================================
- Hits 34903 34892 -11
- Misses 1690 1698 +8 ☔ View full report in Codecov by Sentry. |
Failed Interop TestsQUIC Interop Runner, client vs. server 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 c6d5502. coalesce_acked_from_zero 1+1 entries: Change within noise threshold. time: [99.927 ns 100.28 ns 100.64 ns]
change: [+0.0982% +0.7185% +1.2427%] (p = 0.01 < 0.05)
coalesce_acked_from_zero 3+1 entries: Change within noise threshold. time: [118.78 ns 119.15 ns 119.55 ns]
change: [+0.9611% +1.3874% +1.8044%] (p = 0.00 < 0.05)
coalesce_acked_from_zero 10+1 entries: Change within noise threshold. time: [118.32 ns 118.76 ns 119.30 ns]
change: [+0.8059% +1.2969% +1.7302%] (p = 0.00 < 0.05)
coalesce_acked_from_zero 1000+1 entries: No change in performance detected. time: [98.203 ns 100.74 ns 106.57 ns]
change: [+0.7503% +6.3497% +16.564%] (p = 0.19 > 0.05)
RxStreamOrderer::inbound_frame(): Change within noise threshold. time: [111.51 ms 111.56 ms 111.61 ms]
change: [+0.0434% +0.1140% +0.1834%] (p = 0.00 < 0.05)
SentPackets::take_ranges: No change in performance detected. time: [5.5640 µs 5.7315 µs 5.8999 µs]
change: [-3.2969% +5.6470% +20.364%] (p = 0.50 > 0.05)
transfer/pacing-false/varying-seeds: Change within noise threshold. time: [26.794 ms 27.953 ms 29.116 ms]
change: [+0.3304% +6.3231% +12.918%] (p = 0.05 < 0.05)
transfer/pacing-true/varying-seeds: 💔 Performance has regressed. time: [36.540 ms 38.108 ms 39.699 ms]
change: [+4.2156% +10.920% +18.569%] (p = 0.00 < 0.05)
transfer/pacing-false/same-seed: 💚 Performance has improved. time: [9.5426 ms 9.9066 ms 10.286 ms]
change: [-63.561% -61.898% -60.157%] (p = 0.00 < 0.05)
transfer/pacing-true/same-seed: 💚 Performance has improved. time: [28.572 ms 30.175 ms 31.783 ms]
change: [-32.273% -27.242% -21.923%] (p = 0.00 < 0.05)
1-conn/1-100mb-resp/mtu-1504 (aka. Download)/client: 💚 Performance has improved. time: [870.36 ms 880.74 ms 891.26 ms]
thrpt: [112.20 MiB/s 113.54 MiB/s 114.89 MiB/s]
change:
time: [-4.1859% -2.6810% -1.0336%] (p = 0.00 < 0.05)
thrpt: [+1.0444% +2.7548% +4.3688%]
1-conn/10_000-parallel-1b-resp/mtu-1504 (aka. RPS)/client: No change in performance detected. time: [320.15 ms 323.25 ms 326.39 ms]
thrpt: [30.638 Kelem/s 30.936 Kelem/s 31.235 Kelem/s]
change:
time: [-1.7931% -0.5204% +0.7818%] (p = 0.46 > 0.05)
thrpt: [-0.7758% +0.5232% +1.8258%]
1-conn/1-1b-resp/mtu-1504 (aka. HPS)/client: No change in performance detected. time: [33.666 ms 33.845 ms 34.032 ms]
thrpt: [29.384 elem/s 29.546 elem/s 29.704 elem/s]
change:
time: [-0.6877% +0.1886% +1.0155%] (p = 0.67 > 0.05)
thrpt: [-1.0053% -0.1882% +0.6924%]
1-conn/1-100mb-resp/mtu-1504 (aka. Upload)/client: No change in performance detected. time: [1.6595 s 1.6799 s 1.7006 s]
thrpt: [58.802 MiB/s 59.529 MiB/s 60.257 MiB/s]
change:
time: [-0.8913% +1.0230% +2.8378%] (p = 0.28 > 0.05)
thrpt: [-2.7595% -1.0127% +0.8994%]
Client/server transfer resultsTransfer of 33554432 bytes over loopback.
|
mxinden
approved these changes
Sep 19, 2024
There was a problem hiding this comment.
This makes sense to me.
Can you link to the concrete section of RFC 9000 you are referring to?
I can only find the following in RFC 9001 4.9.1:
The successful use of Handshake packets indicates that no more Initial packets need to be exchanged, as these keys can only be produced after receiving all CRYPTO frames from Initial packets. Thus, a client MUST discard Initial keys when it first sends a Handshake packet and a server MUST discard Initial keys when it first successfully processes a Handshake packet. Endpoints MUST NOT send Initial packets after this point.
|
That is the section in question. |
|
@martinthomson I'd appreciate a review, since the code I am touching is pretty complex. |
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 is what RFC9000 says, and it helps with servers that expect ACKs in the Initial packet number space to happen. It also eliminates a difference in behavior we had when a client received an Initial packet containing an ACK and a CRYPTO frame vs. a CRYPTO frame and an ACK, where that CRYPTO frame caused the Initial packet number space to be dropped.
Broken out of #1998