Re: [quicwg/base-drafts] Allow endpoints to generate traffic keys asynchronously (#3874)

[ianswett <[e-mail]>]

@ianswett commented on this pull request.

This PR is pretty wide-reaching and not isolated to traffic key generation I think we should re-title it before landing to avoid confusion.

>  
-- MUST use the lesser of the value reported in ACK Delay field of the ACK frame
-  and the peer's max_ack_delay transport parameter.
+- SHOULD ignore the peer's max_ack_delay until the handshake is confirmed,

You have "MAY ignore max_ack_delay until the handshake is confirmed" above.

> -latency at the peer or loss of previous ACK frames.  Any delays beyond the
-peer's max_ack_delay are therefore considered effectively part of path delay and
-incorporated into the smoothed_rtt estimate.
+for acknowledgement delays. These delays are computed using the ACK Delay
+field of the ACK frame as described in Section 19.3 of {{QUIC-TRANSPORT}}.
+
+As the peer might report acknowledgement delays that are larger than the peer's
+max_ack_delay during the handshake (Section 13.2.1 of {{QUIC-TRANSPORT}}), the
+endpoint MAY ignore max_ack_delay until the handshake is confirmed (Section
+4.1.2 of {{QUIC-TLS}}). Since these large acknowledgement delays, when they
+occur, are likely to be non-repeating and limited to the handshake, the endpoint
+can use them without limiting them to the max_ack_delay and avoid unnecessarily
+inflating the smoothed_rtt estimate.
+
+After the handshake is confirmed, any acknowledgement delays reported by the
+peer that are greater than its max_ack_delay are attributed to unintentional but

```suggestion
peer that are greater than the peer's max_ack_delay are attributed to unintentional but
```

> +previous acknowledgements. Any delays beyond the peer's max_ack_delay are
+therefore considered effectively part of path delay and incorporated into the
+smoothed_rtt estimate.

```suggestion
previous acknowledgements. Therefore, these extra delays are considered
effectively part of path delay and incorporated into the smoothed_rtt and
rttvar.
```

>  
 When adjusting an RTT sample using peer-reported acknowledgement delays, an
 endpoint:
 
-- MUST ignore the ACK Delay field of the ACK frame for packets sent in the
-  Initial and Handshake packet number space.
+- MAY ignore the acknowledgement delay for Initial packets,

Given we say you should acknowledge these immediately, why is this a MAY?  I'm not aware of any issues this solves vs a MUST or SHOULD.

> @@ -379,8 +387,9 @@ default values.
 On subsequent RTT samples, smoothed_rtt and rttvar evolve as follows:
 
 ~~~
-ack_delay = acknowledgement delay from ACK frame
-ack_delay = min(ack_delay, max_ack_delay)
+ack_delay = decoded ACK Delay field from ACK frame

I think the old text was clearer in this case.
```suggestion
ack_delay = acknowledgement delay from ACK frame
```

> @@ -389,6 +398,13 @@ rttvar_sample = abs(smoothed_rtt - adjusted_rtt)
 rttvar = 3/4 * rttvar + 1/4 * rttvar_sample
 ~~~
 
+An endpoint might postpone the processing of acknowledgements when the

I agree this would be better above.  Also, I think we should make this tighter, since it only applies ACKs of 0-RTT and 0.5-RTT packets.

> +An endpoint might postpone the processing of acknowledgements when the
+corresponding decryption keys are not immediately available. For example, a
+client might receive an acknowledgement for a 0-RTT packet that it cannot
+decrypt because 1-RTT packet protection keys are not yet available to it. In
+such cases, an endpoint SHOULD ignore such local delays in its round-trip time
+sample until the handshake is confirmed.

```suggestion
An endpoint might receive packets in ApplicationData containing
acknowledgements which cannot be processed immediately due to a
lack of 1-RTT decryption keys. For example, a client might receive an
acknowledgement for a 0-RTT packet that it cannot decrypt because
1-RTT packet protection keys are not yet available to it. In such cases,
an endpoint SHOULD subtract the time the packet was buffered and
unable to be decrypted from the round-trip time sample.
```

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3874#pullrequestreview-462127379