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

From: ianswett <[email-masked]>
Date: Sun, 02 Aug 2020 13:26:36 -0700

@ianswett commented on this pull request.

Sorry for the excessive review delay, it took me a while to fully swap this topic in.

I believe recovery needs a pseudocode update before merge this.

I think @martinthomson may have a different direction on the TLS changes, so it'd be good to figure those out.

> @@ -518,14 +518,20 @@ or when Initial or Handshake keys are discarded. This ensures the PTO is always
 set based on the latest RTT information and for the last sent packet in the
 correct packet number space.
 
+When setting the PTO timer, the ApplicationData packet number space (Section
+4.1.1 of {{QUIC-TLS}}) MUST be ignored until the handshake is confirmed. Not
+arming the PTO timer on ApplicationData prevents an endpoint from retransmitting
+data in packets when the peer might not yet have the keys necessary to process
+them or when the endpoint does not yet have the keys necessary to process their
+acknowledgements. For instance, this can happen when a client retransmits data

```suggestion
acknowledgements. For instance, this could happen if a client retransmitted data
```

> @@ -518,14 +518,20 @@ or when Initial or Handshake keys are discarded. This ensures the PTO is always
 set based on the latest RTT information and for the last sent packet in the
 correct packet number space.
 
+When setting the PTO timer, the ApplicationData packet number space (Section
+4.1.1 of {{QUIC-TLS}}) MUST be ignored until the handshake is confirmed. Not
+arming the PTO timer on ApplicationData prevents an endpoint from retransmitting
+data in packets when the peer might not yet have the keys necessary to process
+them or when the endpoint does not yet have the keys necessary to process their
+acknowledgements. For instance, this can happen when a client retransmits data
+sent in 0-RTT packets to the server before confirming that the server is able to
+decrypt 0-RTT packets. Or it can happen when a server retransmits data sent in

```suggestion
decrypt 0-RTT packets. Or it could happen if a server retransmitted data sent in
```

>  When ack-eliciting packets in multiple packet number spaces are in flight,
-the timer MUST be set for the packet number space with the earliest timeout,
-with one exception. The ApplicationData packet number space (Section 4.1.1
-of {{QUIC-TLS}}) MUST be ignored until the handshake completes. Not arming
-the PTO for ApplicationData prevents a client from retransmitting a 0-RTT
-packet on a PTO expiration before confirming that the server is able to
-decrypt 0-RTT packets, and prevents a server from sending a 1-RTT packet on
-a PTO expiration before it has the keys to process an acknowledgement.
+the timer MUST be set to the earlier value of the Initial and Handshake packet
+number spaces.

To be clear, the confirmed/complete aspect is a behavior change, which means for example that the client only arms PTO for a single PN space at once.

But this updated text looks good, assuming we go with that design change.

-- 
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-459675656

---

From: Martin Thomson <[email-masked]>
Date: Mon, 03 Aug 2020 17:51:34 -0700

The text here (aside from needing a rebase) is OK for TLS.  It's the asynchronous certificate validation stuff that bothers me and that can be done as the next step (a step for which it would be good to have this PR merged).

-- 
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#issuecomment-668311940

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 04 Aug 2020 13:24:24 -0700

@janaiyengar pushed 1 commit.

7cff551ff5536dea56d3dc3937a7a5b6ebe44693  Apply suggestions from code review

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3874/files/2e36c1e926183e37f3dca7271a05348fe07b09e9..7cff551ff5536dea56d3dc3937a7a5b6ebe44693

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 04 Aug 2020 14:41:45 -0700

2032e7cb3fe6ebaf69199ecf2ded6c60571c0325  Resolution from #3821 (pseudocode missing)

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3874/files/7cff551ff5536dea56d3dc3937a7a5b6ebe44693..2032e7cb3fe6ebaf69199ecf2ded6c60571c0325

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 04 Aug 2020 14:43:49 -0700

@janaiyengar commented on this pull request.

I've incorporated changes discussed in #3821 (and those agreed on the mailing list). I haven't added pseudocode yet, will do that next. I also haven't squared these changes with #3980; will do that as well.

-- 
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-461195635

---

From: ianswett <[email-masked]>
Date: Tue, 04 Aug 2020 15:05:02 -0700

Thanks for adding more to the PR, some initial comments.

> @@ -338,27 +338,41 @@ mean variation.
 The calculation of smoothed_rtt uses path latency after adjusting RTT samples
 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}}.
-For packets sent in the ApplicationData packet number space, a peer limits
-any delay in sending an acknowledgement for an ack-eliciting packet to no
-greater than the value it advertised in the max_ack_delay transport parameter.
-Consequently, when a peer reports an Ack Delay that is greater than its
-max_ack_delay, the delay is attributed to reasons out of the peer's control,
-such as scheduler 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.
+
+When 0-RTT or 1-RTT ack-eliciting packets are received, a peer does not delay
+acknowledging them any longer than the period it advertised in the max_ack_delay
+transport parameter; see Section 18.2 of {{QUIC-TRANSPORT}}.
+
+However, an endpoint SHOULD use ignore max_ack_delay until the handshake is

```suggestion
However, an endpoint SHOULD ignore max_ack_delay until the handshake is
```

> -part of path delay and incorporated into the smoothed_rtt estimate.
+
+When 0-RTT or 1-RTT ack-eliciting packets are received, a peer does not delay
+acknowledging them any longer than the period it advertised in the max_ack_delay
+transport parameter; see Section 18.2 of {{QUIC-TRANSPORT}}.
+
+However, an endpoint SHOULD use ignore max_ack_delay until the handshake is
+confirmed (Section 4.1.2 of {{QUIC-TLS}}). Prior to handshake confirmation, the
+peer's reported acknowledgement delays might exceed the peer's max_ack_delay
+because it might buffer undecryptable packets and acknowledge them when the
+requisite keys become available to it. Since these delays, when they occur, are
+measurable and limited to the handshake, the endpoint can use the delays 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

Handshake confirmed only works for 1-RTT packets.  But Handshake packets suffer from the same problem as 1-RTT packets.

-- 
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-461204128

---

From: Martin Thomson <[email-masked]>
Date: Tue, 04 Aug 2020 15:16:10 -0700

@martinthomson commented on this pull request.

More coming

```suggestion
When 0-RTT or 1-RTT ack-eliciting packets are received, a peer MUST NOT delay
```

>    Initial and Handshake packet number space.
 
-- 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.
+- MUST ignore the peer's max_ack_delay until the handshake is confirmed,

```suggestion
- SHOULD ignore the peer's max_ack_delay until the handshake is confirmed,
```

 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
+- MUST ignore the ACK Delay field of the ACK frame for packets sent in the

```suggestion
- MAY ignore the ACK Delay field of the ACK frame for packets sent in the
```

We need to add: ...except during the handshake, if packets cannot be processed when received due to lack of packet protection keys.

-- 
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-461210174

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 04 Aug 2020 15:26:06 -0700

c8f0905556c4b2f4c660100b707c06e500de417e  agreed changes from editors meeting

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3874/files/2032e7cb3fe6ebaf69199ecf2ded6c60571c0325..c8f0905556c4b2f4c660100b707c06e500de417e

---

From: ianswett <[email-masked]>
Date: Tue, 04 Aug 2020 15:29:24 -0700

> @@ -1212,9 +1235,7 @@ OnAckReceived(ack, pn_space):
       IncludesAckEliciting(newly_acked_packets)):
     latest_rtt =
       now - sent_packets[pn_space][ack.largest_acked].time_sent
-    ack_delay = 0
-    if (pn_space == ApplicationData):
-      ack_delay = ack.ack_delay
+    ack_delay = ack.ack_delay
     UpdateRtt(ack_delay)

```suggestion
    UpdateRtt(ack.ack_delay)
```

-- 
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-461220103

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 04 Aug 2020 17:36:27 -0700

728dc375f6b4cdcf8ad977e578a542f02cd9245c  mt's comments

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3874/files/7f22ad84730e22190bb61aceec94145bb8eca93a..728dc375f6b4cdcf8ad977e578a542f02cd9245c

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 04 Aug 2020 17:37:23 -0700

I've updated the PR, PTAL.

-- 
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-461264665

---

From: Martin Thomson <[email-masked]>
Date: Tue, 04 Aug 2020 17:47:59 -0700

@martinthomson approved this pull request.

Yes, this is much better.  Shame that it's so big, but that's unavoidable, I think.

-- 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.
+- MAY ignore the peer's max_ack_delay until the handshake is confirmed,

Do we want to say SHOULD here instead, given that buffering happens?

It means updating pseudocode, but the change is small.

> @@ -1232,10 +1255,7 @@ OnAckReceived(ack, pn_space):
       IncludesAckEliciting(newly_acked_packets)):
     latest_rtt =
       now() - newly_acked_packets.largest().time_sent
-    ack_delay = 0
-    if (pn_space == ApplicationData):
-      ack_delay = ack.ack_delay
-    UpdateRtt(ack_delay)
+    UpdateRtt(ack.ack_delay)

We should change line ~1285 down there vv to:

```
  if handshake confirmed:
    ack_delay = min(ack_delay, max_ack_delay)
```

-- 
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-461266854

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 04 Aug 2020 18:04:37 -0700

102907894de1ddc026a1b32a86d4e65c2cfb550a  kazuho corrections

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3874/files/728dc375f6b4cdcf8ad977e578a542f02cd9245c..102907894de1ddc026a1b32a86d4e65c2cfb550a

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 04 Aug 2020 18:07:04 -0700

I think SHOULD is good.

-- 
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#discussion_r465413170

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 04 Aug 2020 18:08:45 -0700

@ianswett may have an opinion here. I'll wait for it.

-- 
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#discussion_r465413607

---

From: Kazuho Oku <[email-masked]>
Date: Tue, 04 Aug 2020 18:15:01 -0700

@kazuho commented on this pull request.

+1 to changing it to SHOULD. As others have pointed out, there are cases that the delay is inevitable (e.g., due to a client doing OCSP cert validation).

-- 
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#discussion_r465415283

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 04 Aug 2020 18:18:26 -0700

@janaiyengar pushed 2 commits.

9821f073979277e7e06919742a8bc402ae530922  s/MAY/SHOULD, and some editorializing
1fe972794aeb792c9553bccfc57b096447aea9e0  undo SHOULD

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3874/files/102907894de1ddc026a1b32a86d4e65c2cfb550a..1fe972794aeb792c9553bccfc57b096447aea9e0

---

From: Martin Thomson <[email-masked]>
Date: Tue, 04 Aug 2020 18:23:00 -0700

> @@ -337,29 +337,47 @@ samples, and rttvar is the variation in the RTT samples, estimated using a
 mean variation.
 
 The calculation of smoothed_rtt uses path latency after adjusting RTT samples
-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}}. For packets
-sent in the Application Data packet number space, a peer limits any delay in
-sending an acknowledgement for an ack-eliciting packet to no greater than the
-value it advertised in the max_ack_delay transport parameter. Consequently, when
-a peer reports an acknowledgment delay that is greater than its max_ack_delay,
-the delay is attributed to reasons out of the peer's control, such as scheduler
-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}}.
+
+A peer MUST immediately acknowledge all ack-eliciting Initial packets.

Does this belong in the transport draft too?  And maybe we can include Handshake packets by conditioning this: "acknowledge all ack-eliciting Initial and Handshake packets immediately after processing them"

> +available. When this is not the case, the peer MUST immediately acknowledge all
+ack-eliciting Handshake packets, and MUST NOT delay acknowledgement of
+ack-eliciting 0-RTT, or 1-RTT packets for any longer than the period that it
+advertised in the max_ack_delay transport parameter (Section 18.2 of
+{{QUIC-TRANSPORT}}).

With the above change I suggest, you can move this out and talk about acknowledgment within 0 (Initial/Handshake) or max_ack_delay (0-RTT/1-RTT) of processing the packet.  That leaves this paragraph as just "an endpoint might have to delay processing until it has keys".

> -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}}.
+
+A peer MUST immediately acknowledge all ack-eliciting Initial packets.
+
+Prior to handshake confirmation, a peer might not have the packet protection
+keys for Handshake, 0-RTT, or 1-RTT packets when they are received. It might
+therefore buffer them and acknowledge them when the requisite keys become
+available. When this is not the case, the peer MUST immediately acknowledge all
+ack-eliciting Handshake packets, and MUST NOT delay acknowledgement of
+ack-eliciting 0-RTT, or 1-RTT packets for any longer than the period that it
+advertised in the max_ack_delay transport parameter (Section 18.2 of
+{{QUIC-TRANSPORT}}).
+
+Since the peer might report large acknowledgement delays during the handshake,

```suggestion
As the peer might report large acknowledgement delays during the handshake,
```

-- 
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-461277231

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 04 Aug 2020 18:46:12 -0700

0000ceb45a959ffc7b568a079b8b01fe4956bbce  Add para on local delays when ack is buffered

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3874/files/1fe972794aeb792c9553bccfc57b096447aea9e0..0000ceb45a959ffc7b568a079b8b01fe4956bbce

---

From: Martin Thomson <[email-masked]>
Date: Tue, 04 Aug 2020 18:49:07 -0700

Let's be consistent with the other recommendations (i.e., SHOULD ignore max_ack_delay).

Assuming that is, that Ian agrees with Kazuho and I about this needing RECOMMENDED-strength text.

> @@ -407,6 +407,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
+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 can ignore such local delays in its round-trip time

```suggestion
such cases, an endpoint SHOULD ignore such local delays in its round-trip time
```

-- 
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-461285354

---

From: ianswett <[email-masked]>
Date: Tue, 04 Aug 2020 18:51:42 -0700

I think decryption keys is better in this context, since protection keys implies the keys to send a packet, and the server has keys to send 0.5 RTT packets, but not process them.

The section on ACK behavior is in transport, let's not duplicate select parts of it here.

> -packet in the correct packet number space. When the PTO expires and there are
-no ack-eliciting packets in flight, the PTO is set from that moment.
-
-When ack-eliciting packets in multiple packet number spaces are in flight,
-the timer MUST be set for the packet number space with the earliest timeout,
-with one exception. The Application Data packet number space (Section 4.1.1
-of {{QUIC-TLS}}) MUST be ignored until the handshake completes. Not arming
-the PTO for Application Data prevents a client from retransmitting a 0-RTT
-packet on a PTO expiration before confirming that the server is able to
-decrypt 0-RTT packets, and prevents a server from sending a 1-RTT packet on
-a PTO expiration before it has the keys to process an acknowledgement.
+When ack-eliciting packets in multiple packet number spaces are in flight, the
+timer MUST be set to the earlier value of the Initial and Handshake packet
+number spaces.
+
+An endpoint MUST NOT set its PTO timer for 0-RTT and 1-RTT packets until the

```suggestion
An endpoint MUST NOT set its PTO timer for the ApplicationData packet number space until the
```

I think that's ok, but is there a reason line 371 is a MAY instead of a SHOULD?  I can't think of any.  If it was a SHOULD, the the pseudocode should follow the SHOULDs.

GetPtoTimeAndSpace needs to be updated to use handshake confirmed as well

 mean variation.

This is already in the transport draft, so I'd be inclined to remove it, and if you want to add an informational comment about how Initial and Handshake packets are acknowledged immediately, feel free.

-- 
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-461282623

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 04 Aug 2020 18:51:50 -0700

I added a paragraph on local delays playing into the RTT estimate, which seems helpful. I'm happy to remove it if folks have a strong opinion that it should be.

-- 
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-461286345

---

From: ianswett <[email-masked]>
Date: Tue, 04 Aug 2020 18:52:21 -0700

Let's not duplicate existing requirements on ACK behavior in recovery, given we decided to move it to transport and that text is still getting cleaned up.

-- 
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#issuecomment-668926674

---

From: Martin Thomson <[email-masked]>
Date: Tue, 04 Aug 2020 18:54:22 -0700

MAY is in recognition of the fact that an attacker can already tilt the calculation for RTT using Initial ACK, so we allow people to ignore that information, but we don't need to recommend it.

Good catch.

 mean variation.

Sounds reasonable.

-- 
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#discussion_r465425760

---

From: Jana Iyengar <[email-masked]>
Date: Wed, 05 Aug 2020 12:16:15 -0700

808310412d66c649651e99d9e67ba8f89cd11628  Apply suggestions from code review

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3874/files/0000ceb45a959ffc7b568a079b8b01fe4956bbce..808310412d66c649651e99d9e67ba8f89cd11628

---

From: Jana Iyengar <[email-masked]>
Date: Wed, 05 Aug 2020 14:38:20 -0700

3d26a53dc4dad99851289eabfd4ff600e18c98bc  more changes

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3874/files/808310412d66c649651e99d9e67ba8f89cd11628..3d26a53dc4dad99851289eabfd4ff600e18c98bc

---

From: Jana Iyengar <[email-masked]>
Date: Wed, 05 Aug 2020 14:38:27 -0700

Done.

 mean variation.

The text in the transport draft is weak. I'm moving this there and making it stronger.

I've restructured the text a bit differently, see what you think.

We use the term "packet protection keys" for both encryption and decryption keys, but I've made the "decrypting" more explicit now.

-- 
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#discussion_r466018540

---

From: Jana Iyengar <[email-masked]>
Date: Wed, 05 Aug 2020 17:59:06 -0700

e7969995f5ce73f268746c04c89b22bc7bd152a0  fix ref

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3874/files/3d26a53dc4dad99851289eabfd4ff600e18c98bc..e7969995f5ce73f268746c04c89b22bc7bd152a0

---

From: Martin Thomson <[email-masked]>
Date: Wed, 05 Aug 2020 18:01:04 -0700

> @@ -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 acknowledgement delay from ACK frame

```suggestion
ack_delay = decoded ACK Delay field from ACK frame
```

> @@ -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 acknowledgement delay from ACK frame
+if (handshake confirmed):
+    ack_delay = min(ack_delay, max_ack_delay)

```suggestion

-- 
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-462118258

---

From: Martin Thomson <[email-masked]>
Date: Wed, 05 Aug 2020 18:02:08 -0700

> @@ -1262,8 +1284,9 @@ UpdateRtt(ack_delay):
 
   // min_rtt ignores acknowledgment delay.
   min_rtt = min(min_rtt, latest_rtt)
-  // Limit ack_delay by max_ack_delay
-  ack_delay = min(ack_delay, max_ack_delay)
+  // Limit ack_delay to max_ack_delay after handshake confirmation.

```suggestion
  // Apply max_ack_delay after handshake confirmation.
```

-- 
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-462119017

---

From: Jana Iyengar <[email-masked]>
Date: Wed, 05 Aug 2020 18:03:36 -0700

ef2a714c4105a4cce5bd11f60ca2f724123e097a  tightening

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3874/files/e7969995f5ce73f268746c04c89b22bc7bd152a0..ef2a714c4105a4cce5bd11f60ca2f724123e097a

---

From: Jana Iyengar <[email-masked]>
Date: Wed, 05 Aug 2020 18:04:54 -0700

4c41f557b5e667ea0c97d7c650b526cd5308d6c5  Apply suggestions from code review

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3874/files/ef2a714c4105a4cce5bd11f60ca2f724123e097a..4c41f557b5e667ea0c97d7c650b526cd5308d6c5

---

From: Kazuho Oku <[email-masked]>
Date: Wed, 05 Aug 2020 18:05:27 -0700

Thank you for working on this. Looks great to me. I just have one editorial preference, but that's definitely not a blocker.

> @@ -389,6 +398,13 @@ rttvar_sample = abs(smoothed_rtt - adjusted_rtt)
 
+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.

I would prefer this section moved above line 358 (i.e. "When adjusting an RTT sample using peer-reported acknowledgement delays, an endpoint:"), and this SHOULD repeated in one of the bullet points, as we do for the ack-delay due to buffering on the receiver side.

That said, I wouldn't be surprised if you chose to do that as a separate editorial path after merging this PR.

-- 
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-462119697

---

From: Jana Iyengar <[email-masked]>
Date: Wed, 05 Aug 2020 18:37:40 -0700

I'm fine with moving this further up, before the smoothed_rtt computation, but I don't think we want to include this in that list. That list is about adjusting the RTT sample based on reported ack delay, and it ends with "don't do it if the result is < min_rtt". The local buffering delay is quite different, in that it doesn't need this min_rtt protection. I'm moving it up but leaving it below the adjustment list.

-- 
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#discussion_r466094977

---

From: Jana Iyengar <[email-masked]>
Date: Wed, 05 Aug 2020 18:38:35 -0700

7f87ff781d49f02bfb3565de94525b56529ab54f  move local delay para up

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3874/files/4c41f557b5e667ea0c97d7c650b526cd5308d6c5..7f87ff781d49f02bfb3565de94525b56529ab54f

---

From: Jana Iyengar <[email-masked]>
Date: Wed, 05 Aug 2020 18:40:46 -0700

@janaiyengar approved this pull request.

Heh, apparently I can still approve this since I'm squatting on @kazuho's PR. So I'm approving this :-)

(@kazuho wanted to approve it but couldn't, so I'll approve on his behalf.)

-- 
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-462130670

---

From: ianswett <[email-masked]>
Date: Wed, 05 Aug 2020 18:44:40 -0700

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.

-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.
```

-- 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.

 ~~~
-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
```

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.

```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

---

From: Kazuho Oku <[email-masked]>
Date: Wed, 05 Aug 2020 18:46:33 -0700

Looks great!

-- MUST NOT apply the adjustment if the resulting RTT sample is smaller than the
-  min_rtt.  This limits the underestimation that a misreporting peer can cause
-  to the smoothed_rtt.
+- MUST use the lesser of the acknowledgement delay and the peer's max_ack_delay
+  after the handshake is confirmed,
+
+- MUST NOT apply the adjustments above if the resulting round-trip time sample
+  is smaller than the min_rtt.  This limits the underestimation of the
+  smoothed_rtt because of a misreporting peer.
+
+Additionaly, 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

```suggestion
such cases, an endpoint SHOULD ignore such local delays in its RTT
```

"RTT" is the term we use, I think?

-- 
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-462132366

---

From: Jana Iyengar <[email-masked]>
Date: Wed, 05 Aug 2020 18:47:14 -0700

-- 
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#discussion_r466097605

---

From: ianswett <[email-masked]>
Date: Wed, 05 Aug 2020 18:50:04 -0700

Sorry, read it backwards.  Nevermind.

-- 
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#discussion_r466098283

---

From: Jana Iyengar <[email-masked]>
Date: Wed, 05 Aug 2020 18:53:23 -0700

6635adb440aeeab3a023dc60aa735679026d1599  Ian's comments

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3874/files/7f87ff781d49f02bfb3565de94525b56529ab54f..6635adb440aeeab3a023dc60aa735679026d1599

---

From: Jana Iyengar <[email-masked]>
Date: Wed, 05 Aug 2020 18:53:41 -0700

Thanks, @ianswett, changes made.

Gah! That MAY should've been fixed. Fixed now.

 ~~~

it's setting a variable, so spelling out that it is a decoded field seems more explicit.

-- 
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-462133363

---

From: Jana Iyengar <[email-masked]>
Date: Wed, 05 Aug 2020 22:21:21 -0700

ddb4b86561e175e9670b6804bc890fd947491f2e  Update draft-ietf-quic-recovery.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3874/files/6635adb440aeeab3a023dc60aa735679026d1599..ddb4b86561e175e9670b6804bc890fd947491f2e

---

From: Jana Iyengar <[email-masked]>
Date: Wed, 05 Aug 2020 22:22:17 -0700

0b9b2e1e3e9b65773a94c6e9b6c8cf12fbd34d95  RTT

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3874/files/ddb4b86561e175e9670b6804bc890fd947491f2e..0b9b2e1e3e9b65773a94c6e9b6c8cf12fbd34d95

---

From: ianswett <[email-masked]>
Date: Thu, 06 Aug 2020 06:54:17 -0700

@ianswett approved this pull request.

I think we should stop using intentional delays and instead use only introduced delays.  Intentional seems potentially confusing.

> @@ -3688,11 +3694,13 @@ to adjust for any intentional delays, which is important for getting a better
 estimate of the path RTT when acknowledgments are delayed.  A packet might be

I think we should start using 'introduced delays' instead of 'intentionally introduced delays' because now we have two cases where the delay isn't really intentional: alarms which wake up late and lack of availability of decryption keys.

 ~~~

I think the most correct clear option is "decoded acknowledgement delay from the ACK Delay field in the ACK frame", but that's quite long.

-- 
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-462538207

---

From: Marten Seemann <[email-masked]>
Date: Thu, 06 Aug 2020 07:26:48 -0700

@marten-seemann approved this pull request.

 estimate of the path RTT when acknowledgments are delayed.  A packet might be
 held in the OS kernel or elsewhere on the host before being processed.  An
 endpoint MUST NOT include delays that it does not control when populating the
-ACK Delay field in an ACK frame.
+ACK Delay field in an ACK frame, but endpoints SHOULD include buffering delays
+caused by unavailability of decryption keys.

Do we need to explain why this is a SHOULD? We could say that if a packet containing CRYPTO data from the previous PN was lost, the buffering delay will be > 1 RTT. That should be a very good motivation to follow this SHOULD.

+endpoint SHOULD ignore max_ack_delay until the handshake is confirmed (Section

It's not only smoothed_rtt, but also rttvar.
```suggestion
inflating the RTT estimate.
```

+
+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 SHOULD 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 the peer's max_ack_delay are attributed to
+unintentional but potentially repeating delays, such as scheduler latency at the
+peer or loss of previous acknowledgements. Therefore, these extra delays are
+considered effectively part of path delay and incorporated into the smoothed_rtt
+estimate.

Not sure where to put this, but do we need to recommend something along these lines:
If you set an ack timer for `x ms`, you should advertise a max_ack_delay for `x ms + timer_granularity` in your transport parameters?

-- 
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-462562593

---

From: Nick Banks <[email-masked]>
Date: Thu, 06 Aug 2020 07:39:05 -0700

@nibanks commented on this pull request.

+

I thought we already had something like this, but I agree with the sentiment. MsQuic does exactly this.

-- 
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#discussion_r466461296

---

From: Nick Banks <[email-masked]>
Date: Thu, 06 Aug 2020 07:42:36 -0700

~~~

Maybe something like `ack_frame.ack_delay`?

-- 
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#discussion_r466463705

---

From: Jana Iyengar <[email-masked]>
Date: Thu, 06 Aug 2020 13:47:43 -0700

+

Safeguards are always up to the implementation... this includes any for the idle timeout as well. At any rate, the sender is protected by the rttvar.

I'd be open to an editorial PR that suggests this in the right place.

-- 
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#discussion_r466673512

---

From: Jana Iyengar <[email-masked]>
Date: Thu, 06 Aug 2020 13:48:08 -0700

936c4a0f080564afc3ab0e5c2d8732f14101a27b  more comments

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3874/files/0b9b2e1e3e9b65773a94c6e9b6c8cf12fbd34d95..936c4a0f080564afc3ab0e5c2d8732f14101a27b

---

From: ianswett <[email-masked]>
Date: Tue, 11 Aug 2020 14:59:52 -0700

```suggestion
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}}), so the
```

```suggestion
4.1.2 of {{QUIC-TLS}}). Since these acknowledgement delays, when they
```

```suggestion
can use them without limiting them to the max_ack_delay to avoid unnecessarily
```

+- MUST NOT apply the adjustments above if the resulting RTT sample is smaller

```suggestion
- MUST NOT subtract the acknowledgement delay if the resulting RTT sample is smaller
```

```suggestion
Additionaly, an endpoint might be unable to process acknowledgements when
the corresponding decryption keys are not immediately available. For example, a
```

-- MUST NOT apply the adjustment if the resulting RTT sample is smaller than the
-  min_rtt.  This limits the underestimation that a misreporting peer can cause
-  to the smoothed_rtt.
+- MUST use the lesser of the acknowledgement delay and the peer's max_ack_delay
+  after the handshake is confirmed,
+
+- MUST NOT apply the adjustments above if the resulting RTT sample is smaller
+  than the min_rtt.  This limits the underestimation of the smoothed_rtt because
+  of a misreporting peer.
+
+Additionaly, 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 RTT sample until

```suggestion
such cases, an endpoint SHOULD subtract such local delays from its RTT sample until
```

-The PTO value MUST be set to at least kGranularity, to avoid the timer expiring
+When the PTO is armed for Initial or Handshake packet number spaces, the
+max_ack_delay in the PTO period computation is set to 0, since the peer is

This seems potentially at odds with the text above about ignoring max_ack_delay until the handshake is confirmed?

+

I agree with @nibanks that I thought we already had text for that, though it'd be in transport.

-- 
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-465443983

---

From: ianswett <[email-masked]>
Date: Tue, 11 Aug 2020 15:16:06 -0700

A few more comments, but this is looking good.

> @@ -1225,17 +1249,15 @@ OnAckReceived(ack, pn_space):
   if (newly_acked_packets.empty()):
     return
 
-  // If the largest acknowledged is newly acked and
-  // at least one ack-eliciting was newly acked, update the RTT.
+  // Update the RTT if the largest acknowledged is newly acked
+  // and at least one ack-eliciting was newly acked.
+

```suggestion
```

> @@ -499,9 +499,12 @@ handshake, new data is requested from TLS after providing received data.
 ### Encryption Level Changes
 
 As keys at a given encryption level become available to TLS, TLS indicates to
-QUIC that reading or writing keys at that encryption level are available.  These
-events are not asynchronous; they always occur immediately after TLS is provided
-with new handshake bytes, or after TLS produces handshake bytes.
+QUIC that reading or writing keys at that encryption level are available.  While
+generating these keys, an endpoint SHOULD buffer received packets marked as
+protected by the keys being generated, and process them once those keys become
+available.  If the keys are generated asynchronously, an endpoint MAY continue

I think we expect implementations to do this typically, unless there's a reason not to?

```suggestion
available.  If the keys are generated asynchronously, an endpoint SHOULD continue
```

> @@ -3685,14 +3691,18 @@ packet with the largest packet number is received and the time an acknowledgment
 is sent.  The endpoint encodes this acknowledgement delay in the ACK Delay field
 of an ACK frame; see {{frame-ack}}.  This allows the receiver of the ACK frame
 to adjust for any intentional delays, which is important for getting a better
-estimate of the path RTT when acknowledgments are delayed.  A packet might be
-held in the OS kernel or elsewhere on the host before being processed.  An
-endpoint MUST NOT include delays that it does not control when populating the
-ACK Delay field in an ACK frame.
-
-Since the acknowledgement delay is not used for Initial and Handshake
-packets, the ACK Delay field in acknowledgements for those packet types
-SHOULD be set to 0.
+estimate of the path RTT when acknowledgments are delayed.
+
+A packet might be held in the OS kernel or elsewhere on the host before being

I'd still like to strike the word 'intentionally' from above, since I don't think it's helpful.

-- 
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-465459308

---

From: Jana Iyengar <[email-masked]>
Date: Thu, 13 Aug 2020 11:06:17 -0700

I don't think this needs a recommendation. It's an optimization, and endpoints can do as they choose.

-- 
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#discussion_r470148157

---

From: Jana Iyengar <[email-masked]>
Date: Thu, 13 Aug 2020 11:08:51 -0700

I'm leaving it there. I don't want to make any more editorial changes to existing text in this PR.

-- 
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#discussion_r470149526

---

From: Jana Iyengar <[email-masked]>
Date: Thu, 13 Aug 2020 11:12:55 -0700

If you remove the "large" qualifier, the sentence seems to suggest that ack delays are limited to the handshake. Leaving this out.

-- 
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#discussion_r470152108

---

From: Kazuho Oku <[email-masked]>
Date: Thu, 13 Aug 2020 18:11:28 -0700

Aside from "intentionally" being existing text, I would argue that we have to keep that word. As we have discussed elsewhere (e.g., #3970), any delay that is *not* intentionally introduced has to be accounted as path delay. As an example, [a delay caused by the timer waking up late](https://github.com/quicwg/base-drafts/pull/3874/#discussion_r466428681) should ideally be considered as part of the path delay (and hence be *not* included in the reported ack delay).

-- 
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#discussion_r470357378

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 18 Aug 2020 12:57:35 -0700

This is a useful distinction -- endpoints that are unable to process acks because keys aren't there can simply drop those packets too. That won't reflect in the delay.

-- 
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#discussion_r472444616

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 18 Aug 2020 13:02:05 -0700

e7d423e78f6a6778b685688f4d2d56f4813881f4  Ian's comments

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3874/files/936c4a0f080564afc3ab0e5c2d8732f14101a27b..e7d423e78f6a6778b685688f4d2d56f4813881f4

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 18 Aug 2020 13:05:40 -0700

I've changed the text to be a bit clearer, but this text is still ok and useful for Initial packets and useful if the implementation doesn't follow the SHOULD above or harmless for Handshake packets.

-- 
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#discussion_r472449848

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 18 Aug 2020 13:07:29 -0700

721ee0008702eb34ae1c1fb2bf67b84d762e12a9  s/ack immediately/not delay intentionally

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3874/files/e7d423e78f6a6778b685688f4d2d56f4813881f4..721ee0008702eb34ae1c1fb2bf67b84d762e12a9

---

From: ianswett <[email-masked]>
Date: Tue, 18 Aug 2020 14:41:40 -0700

> @@ -532,7 +532,7 @@ delay sending an acknowledgement.
 
 When the PTO is armed for Initial or Handshake packet number spaces, the
 max_ack_delay in the PTO period computation is set to 0, since the peer is
-expected to acknowledge these packets immediately; see 13.2.1 of
+expected to not delay these packets intentionally; see 13.2.1 of

I think the old text is better, I'll need to re-read the entire text to understand my comment.

```suggestion
expected to acknowledge these packets immediately; see 13.2.1 of
```

-- 
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-469850580

---

From: ianswett <[email-masked]>
Date: Tue, 18 Aug 2020 14:57:12 -0700

After a re-read, I think I was thinking this was a problem for packets in ApplicationData, but after re-thinking this, I don't think there's an issue there.  I posted this 1 minute before our editors meeting, so I suspect I was a bit rushed.

-- 
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#discussion_r472516191

---

From: ianswett <[email-masked]>
Date: Tue, 18 Aug 2020 15:08:27 -0700

-- 
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-469864228

---

From: Jana Iyengar <[email-masked]>
Date: Wed, 19 Aug 2020 22:29:55 -0700

952952a241916edbca9cd1a56cd633fc596c756d  Martin's concern in #4035

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3874/files/721ee0008702eb34ae1c1fb2bf67b84d762e12a9..952952a241916edbca9cd1a56cd633fc596c756d

---

From: Kazuho Oku <[email-masked]>
Date: Wed, 19 Aug 2020 22:43:55 -0700

> @@ -348,6 +348,11 @@ acknowledgement delays are likely to be non-repeating and limited to the
 handshake. The endpoint can therefore use them without limiting them to the
 max_ack_delay, avoiding unnecessary inflation of the RTT estimate.
 
+Ignoring max_ack_delay at the beginning of the connection can lead to a
+substantially inflated smoothed_rtt. Therefore, prior to handshake confirmation,

I do not think that this sentence is correct. Ignoring `max_ack_delay` is a way to prevent inflation of SRTT. The problem here is that ignoring `max_ack_delay` is not enough, if `min_rtt` is not established.

> +The calculation of smoothed_rtt uses RTT samples after adjusting them 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}}.
+
+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}}). To
+account for this, the endpoint SHOULD ignore max_ack_delay until the handshake
+is confirmed (Section 4.1.2 of {{QUIC-TLS}}). When they occur, these large
+acknowledgement delays are likely to be non-repeating and limited to the
+handshake. The endpoint can therefore use them without limiting them to the
+max_ack_delay, avoiding unnecessary inflation of the RTT estimate.
+
+Ignoring max_ack_delay at the beginning of the connection can lead to a
+substantially inflated smoothed_rtt. Therefore, prior to handshake confirmation,
+an endpoint MAY ignore RTT samples if subtracting the acknowledgement delay
+causes the sample to be less than the min_rtt.

I'm not sure if I follow this sentence. I thought that adjusted_rtt can never become smaller than min_rtt, and that would continue to be the case with this PR?

-- 
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-471258517

---

From: Jana Iyengar <[email-masked]>
Date: Wed, 19 Aug 2020 23:32:56 -0700

99d976fe50d0e4a1a8dc740e0fefbb81b59d4a9d  kazuho's comments
2a50ecd6accd6b3376891dd94a332015c3eef6d3  kazuho's comments

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3874/files/952952a241916edbca9cd1a56cd633fc596c756d..2a50ecd6accd6b3376891dd94a332015c3eef6d3

---

From: Kazuho Oku <[email-masked]>
Date: Wed, 19 Aug 2020 23:42:42 -0700

@janaiyengar Thank you for the changes. The updated text looks good to me.
* it explains how ack-delay can entirely be ignored,
* it talks about ack-delay, rather than max_ack_delay (ack-delay of 24ms can still be substantially large compared to actual RTT), and
* the advice uses "can", implying that there could be other ways of working around the problem.

-- 
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-471287642

---

From: ianswett <[email-masked]>
Date: Thu, 20 Aug 2020 06:51:43 -0700

> @@ -348,6 +348,13 @@ acknowledgement delays are likely to be non-repeating and limited to the
 
+Note however that a large acknowledgement delay can result in a substantially

```suggestion
A large acknowledgement delay can result in a substantially
```

-- 
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-471635223

---

From: ianswett <[email-masked]>
Date: Thu, 20 Aug 2020 06:55:17 -0700

+Note however that a large acknowledgement delay can result in a substantially
+inflated smoothed_rtt, if there is either an error in the peer's reporting of
+the acknowledgement delay or in the endpoint's min_rtt estimate.  Therefore,
+prior to handshake confirmation, an endpoint can ignore RTT samples if adjusting

Is this better as a MAY?  We never say you can ignore RTT samples in any other circumstance, so this feels a bit new.
```suggestion
prior to handshake confirmation, an endpoint MAY ignore RTT samples if adjusting
```

-- 
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-471638792

---

From: martinduke <[email-masked]>
Date: Fri, 28 Aug 2020 12:59:36 -0700

@martinduke approved this pull request.

+prior to handshake confirmation, an endpoint can ignore RTT samples if adjusting
+the RTT sample for acknowledgement delay causes the sample to be less than the
+min_rtt.
+
+After the handshake is confirmed, any acknowledgement delays reported by the
+peer that are greater than the peer's max_ack_delay are attributed to
+unintentional but potentially repeating delays, such as scheduler latency at the
+peer or loss of previous acknowledgements. Therefore, these extra delays are
+considered effectively part of path delay and incorporated into the RTT
+estimate.
+
+Therefore, when adjusting an RTT sample using peer-reported acknowledgement
+delays, an endpoint:
+
+- MAY ignore the acknowledgement delay for Initial packets,

If I understand correctly,  this leads to a higher current_rtt measurement. It would be nice if the paragraph above included a motivation for that.

-- 
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-477982122

---

From: Jana Iyengar <[email-masked]>
Date: Fri, 28 Aug 2020 13:03:59 -0700

95988d742bfdeef43e32027563e3de1f608d28e2  Update draft-ietf-quic-recovery.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3874/files/2a50ecd6accd6b3376891dd94a332015c3eef6d3..95988d742bfdeef43e32027563e3de1f608d28e2

---

From: Jana Iyengar <[email-masked]>
Date: Fri, 28 Aug 2020 13:13:03 -0700

cae02dc83b371f4a5f63d62636bca4cf5920ba2d  reference for why Initial packets won't be delayed

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3874/files/95988d742bfdeef43e32027563e3de1f608d28e2..cae02dc83b371f4a5f63d62636bca4cf5920ba2d

---

From: Jana Iyengar <[email-masked]>
Date: Fri, 28 Aug 2020 13:13:34 -0700

This shouldn't because the peer is expected to ack these immediately. Added text and a reference.

-- 
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#discussion_r479514010

---

From: Jana Iyengar <[email-masked]>
Date: Fri, 28 Aug 2020 13:14:17 -0700

This connects it to the previous para, which is useful I think.

-- 
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#discussion_r479514291