Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(s2n-quic-transport): revert "improve amplification credits on client #1818" #1906

Merged
merged 1 commit into from
Aug 11, 2023

Conversation

WesleyRosenblum
Copy link
Contributor

@WesleyRosenblum WesleyRosenblum commented Aug 10, 2023

Description of changes:

In #1818 we made two changes to improve the success of handshakes succeeding in the face of packet loss and amplification limits on the server.

These two changes were:

  1. Pad all datagrams transmitted by the client until the handshake has been confirmed, regardless of whether an initial packet was included in the datagram
  2. Force all packets transmitted during the handshake to be ack-eliciting by including a Ping frame in the packet

1. Should not be necessary since receipt of a Handshake packet on the server removes amplification limits, so padding a solitary Handshake packet would only serve to give amplification credits that would no longer be needed. From QUIC§8,1:

Connection establishment implicitly provides address validation for both endpoints. In particular, receipt of a packet protected with Handshake keys confirms that the peer successfully processed an Initial packet. Once an endpoint has successfully processed a Handshake packet from the peer, it can consider the peer address to have been validated.

2. Is addressed by #1894, which arms the PTO timer for the handshake regardless of whether the handshake space has any transmission to send (ack-eliciting or otherwise). All PTO probes are ack-eliciting.

Testing:

Will verify in Interop

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@WesleyRosenblum WesleyRosenblum merged commit a11f5d0 into main Aug 11, 2023
127 of 129 checks passed
@WesleyRosenblum WesleyRosenblum deleted the WesleyRosenblum/revert1818 branch August 11, 2023 00:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants