Subject: Re: [quicwg/base-drafts] Merge Crypto timeout and PTO (#2806)

From: ianswett <[email-masked]>
Date: Tue, 02 Jul 2019 19:13:06 -0700

@ianswett pushed 1 commit.

ab199a4f27c55bfa67d604644d68ecbd4a04fea3  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/2806/files/fa3b5497e8019ce105433a216867dd098c365ad7..ab199a4f27c55bfa67d604644d68ecbd4a04fea3

---

From: ianswett <[email-masked]>
Date: Tue, 02 Jul 2019 19:13:15 -0700

f4a79150d02c50a31686c97fb5d3690c9ea1b309  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/2806/files/ab199a4f27c55bfa67d604644d68ecbd4a04fea3..f4a79150d02c50a31686c97fb5d3690c9ea1b309

---

From: ianswett <[email-masked]>
Date: Tue, 02 Jul 2019 19:16:33 -0700

7ce89c2709a4c26d086ee8e71bcd4684b0f49280  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/2806/files/f4a79150d02c50a31686c97fb5d3690c9ea1b309..7ce89c2709a4c26d086ee8e71bcd4684b0f49280

---

From: ianswett <[email-masked]>
Date: Tue, 02 Jul 2019 19:26:40 -0700

3a10638950defc8a0030ef53f1f06d270b6d95a3  Remove crypto timer from the pseudocode

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2806/files/7ce89c2709a4c26d086ee8e71bcd4684b0f49280..3a10638950defc8a0030ef53f1f06d270b6d95a3

---

From: ianswett <[email-masked]>
Date: Tue, 02 Jul 2019 19:41:18 -0700

50d6fffc5902cdb2fcb237d1c1690559dc7c3a54  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/2806/files/3a10638950defc8a0030ef53f1f06d270b6d95a3..50d6fffc5902cdb2fcb237d1c1690559dc7c3a54

---

From: ianswett <[email-masked]>
Date: Tue, 02 Jul 2019 19:42:44 -0700

cc80198b1448004726bcb388d82375e086acfba5  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/2806/files/50d6fffc5902cdb2fcb237d1c1690559dc7c3a54..cc80198b1448004726bcb388d82375e086acfba5

---

From: ianswett <[email-masked]>
Date: Tue, 02 Jul 2019 19:45:37 -0700

80c67ed5b7b6e8a93ed55f84358c5e91bdb72ae1  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/2806/files/cc80198b1448004726bcb388d82375e086acfba5..80c67ed5b7b6e8a93ed55f84358c5e91bdb72ae1

---

From: ianswett <[email-masked]>
Date: Tue, 02 Jul 2019 20:02:34 -0700

87682724d04ea9b8efd7106f814449a1230dfc7e  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/2806/files/80c67ed5b7b6e8a93ed55f84358c5e91bdb72ae1..87682724d04ea9b8efd7106f814449a1230dfc7e

---

From: ianswett <[email-masked]>
Date: Tue, 02 Jul 2019 20:11:05 -0700

342c7930d83eacd6fdfcd2dc392e4011ae9f410d  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/2806/files/87682724d04ea9b8efd7106f814449a1230dfc7e..342c7930d83eacd6fdfcd2dc392e4011ae9f410d

---

From: ianswett <[email-masked]>
Date: Tue, 02 Jul 2019 21:19:14 -0700

Thanks for your patience, I believe this is now ready for a full review.

-- 
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/2806#issuecomment-507931694

---

From: Martin Thomson <[email-masked]>
Date: Tue, 02 Jul 2019 21:58:34 -0700

martinthomson commented on this pull request.

 A sender computes its PTO timer every time an ack-eliciting packet is sent. A
 sender might choose to optimize this by setting the timer fewer times if it
 knows that more ack-eliciting packets will be sent within a short period of
 time.
 
+The probe timer is not set if the time threshold {{time-threshold}} loss
+detection timer is set.  The time threshold loss detection timer is expected
+to both expire earlier than the PTO and be less likely to spuriously retransmit
+data. The Initial and Handshake packet number spaces will typically contain a
+small number of packets, so losses are less likely to be detected using
+packet-threshold loss detection.
+
+## Handshakes and new paths

```suggestion
## Handshakes and New Paths
```

> @@ -1283,7 +1237,7 @@ OnLossDetectionTimeout():
        SendOneHandshakePacket()
      else:
        SendOnePaddedInitialPacket()
-    crypto_count++
+    pto_count++

```suggestion
     pto_count++
```
The alignment looks off here.

-- 
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/2806#pullrequestreview-257259977

---

From: MikkelFJ <[email-masked]>
Date: Wed, 03 Jul 2019 01:56:40 -0700

mikkelfj commented on this pull request.

> @@ -543,49 +544,46 @@ delay sending an acknowledgement.
 The PTO value MUST be set to at least kGranularity, to avoid the timer expiring
 immediately.
 
-When a PTO timer expires, the sender probes the network as described in the next
-section. The PTO period MUST be set to twice its current value. This exponential
-reduction in the sender's rate is important because the PTOs might be caused by
-loss of packets or acknowledgements due to severe congestion.
+When a PTO timer expires, the PTO period MUST be set to twice its current
+value. This exponential reduction in the sender's rate is important because
+the PTOs might be caused by loss of packets or acknowledgements due to severe
+congestion.

Would we want some upper limit or this? What happens if idle timeout is not set - just indefinite inactivity?

-- 
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/2806#pullrequestreview-257348210

---

From: Marten Seemann <[email-masked]>
Date: Wed, 03 Jul 2019 02:22:08 -0700

marten-seemann commented on this pull request.

Why would there be no idle timeout?

-- 
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/2806#discussion_r299859093

---

From: MikkelFJ <[email-masked]>
Date: Wed, 03 Jul 2019 02:28:10 -0700

There should be, and maybe that is the answer. But I think a change was made to allow idle timeout to be indefinite. So I wonder how that will affect PTO.

-- 
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/2806#discussion_r299862021

---

From: Marten Seemann <[email-masked]>
Date: Wed, 03 Jul 2019 02:38:44 -0700

I don't remember any change like that. Every peer decides on its idle timeout. So even if a peer decides to have an infinite idle timeout (why would you do that to yourself?), once you reach a PTO duration on the order of what you might consider a typical idle timeout (let's say 30 seconds), exponential backoff should take care of your concern.

-- 
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/2806#discussion_r299866909

---

From: MikkelFJ <[email-masked]>
Date: Wed, 03 Jul 2019 02:41:03 -0700

Thats what I am asking - should the PTO just grow, or would you clamp it to 30 seconds or something?

-- 
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/2806#discussion_r299867970

---

From: ianswett <[email-masked]>
Date: Mon, 08 Jul 2019 13:42:26 -0700

48c1a469f88908ae3359fa3cd547e9deba761564  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/2806/files/342c7930d83eacd6fdfcd2dc392e4011ae9f410d..48c1a469f88908ae3359fa3cd547e9deba761564

---

From: ianswett <[email-masked]>
Date: Mon, 08 Jul 2019 13:44:48 -0700

b46c1078656491e07042b177c89c02af548a4150  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/2806/files/48c1a469f88908ae3359fa3cd547e9deba761564..b46c1078656491e07042b177c89c02af548a4150

---

From: ianswett <[email-masked]>
Date: Mon, 08 Jul 2019 13:44:56 -0700

ianswett commented on this pull request.

TCP clamps to a value(typically 30 seconds), but I think it's better to recommend a dead path timeout, such as a limit on the number of successive PTOs(I think some TCP impls default to 7?).

        SendOneHandshakePacket()
      else:

You're correct, the else is off.

-- 
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/2806#discussion_r301290467

---

From: ianswett <[email-masked]>
Date: Mon, 08 Jul 2019 13:59:37 -0700

69fdaa408c50a44e48a55972360ed35d813daa2b  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/2806/files/b46c1078656491e07042b177c89c02af548a4150..69fdaa408c50a44e48a55972360ed35d813daa2b

---

From: Nick Banks <[email-masked]>
Date: Tue, 09 Jul 2019 03:45:52 -0700

nibanks approved this pull request.

-- 
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/2806#pullrequestreview-259418179

---

From: ianswett <[email-masked]>
Date: Tue, 09 Jul 2019 15:32:35 -0700

f399b5a9af41bf36794b45b90da5f399640912d1  Changed to MUST

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2806/files/69fdaa408c50a44e48a55972360ed35d813daa2b..f399b5a9af41bf36794b45b90da5f399640912d1

---

From: ianswett <[email-masked]>
Date: Tue, 09 Jul 2019 15:35:36 -0700

6ab3dcdc421ad62b1fa8aab2c1de34e0b5cf5414  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/2806/files/f399b5a9af41bf36794b45b90da5f399640912d1..6ab3dcdc421ad62b1fa8aab2c1de34e0b5cf5414

---

From: ianswett <[email-masked]>
Date: Tue, 09 Jul 2019 15:52:55 -0700

2dff033c8e9e6fcc99c0eb56a9069ad83a26891b  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/2806/files/6ab3dcdc421ad62b1fa8aab2c1de34e0b5cf5414..2dff033c8e9e6fcc99c0eb56a9069ad83a26891b

---

From: ianswett <[email-masked]>
Date: Tue, 09 Jul 2019 15:54:44 -0700

21c79243e265d1455dd921988f4e07c53b4d874c  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/2806/files/2dff033c8e9e6fcc99c0eb56a9069ad83a26891b..21c79243e265d1455dd921988f4e07c53b4d874c

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 09 Jul 2019 16:00:14 -0700

janaiyengar commented on this pull request.

> +The initial probe timeout for a new connection or new path SHOULD be
+set to twice the initial RTT.  Resumed connections over the same network
+SHOULD use the previous connection's final smoothed RTT value as the resumed
+connection's initial RTT.  If no previous RTT is available, the initial RTT
+SHOULD be set to 500ms, resulting in a 1 second initial timeout as recommended
+in {{?RFC6298}}.
+
+A connection MAY use the delay between sending a PATH_CHALLENGE and receiving
+a PATH_RESPONSE to seed initial_rtt for a new path, but the delay SHOULD NOT
+be considered an RTT sample.
+
+Until the server has validated the client's address on the path, the amount of
+data it can send is limited, as specified in Section 8.1 of {{QUIC-TRANSPORT}}.
+If not all data can be retransmitted, then data at Initial and Handshake
+encryption should be retransmitted before any ApplicationData data. If no data
+can be sent, then the PTO alarm should not be armed until data has been

```suggestion
can be sent, then the PTO alarm MUST NOT be armed until data has been
```

-- 
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/2806#pullrequestreview-259792135

---

From: ianswett <[email-masked]>
Date: Tue, 09 Jul 2019 16:00:29 -0700

d4570113c0a67a997fd92c0694e7252e7cd5cad6  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/2806/files/21c79243e265d1455dd921988f4e07c53b4d874c..d4570113c0a67a997fd92c0694e7252e7cd5cad6

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 09 Jul 2019 16:03:38 -0700

Maybe it's worth adding a sentence here to say that "The life of a connection that is experiencing consecutive PTOs is limited by the endpoint's idle timeout."

-- 
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/2806#discussion_r301828150

---

From: ianswett <[email-masked]>
Date: Tue, 09 Jul 2019 16:04:51 -0700

fcccfff8962a9f069466d8227ead6c1ad1e82356  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/2806/files/d4570113c0a67a997fd92c0694e7252e7cd5cad6..fcccfff8962a9f069466d8227ead6c1ad1e82356

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 09 Jul 2019 16:05:06 -0700

janaiyengar approved this pull request.

-- 
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/2806#pullrequestreview-259801590

---

From: ianswett <[email-masked]>
Date: Tue, 09 Jul 2019 16:07:26 -0700

fb1f327239d8bc3a8c886c482336a85d0e0a73f1  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/2806/files/fcccfff8962a9f069466d8227ead6c1ad1e82356..fb1f327239d8bc3a8c886c482336a85d0e0a73f1

---

From: Ryan Hamilton <[email-masked]>
Date: Thu, 11 Jul 2019 09:43:53 -0700

RyanAtGoogle approved this pull request.

Wow, this looks so much cleaner.

> +data it can send is limited, as specified in Section 8.1 of {{QUIC-TRANSPORT}}.
+Data at Initial encryption MUST be retransmitted before Handshake data and
+data at Handshake encryption MUST be retransmitted before any ApplicationData
+data.  If no data can be sent, then the PTO alarm MUST NOT be armed until
+data has been received from the client.
+
+Because the server could be blocked until more packets are received, the client
+MUST ensure that the retransmission timer is set if the client does not yet
+have 1-RTT keys.  If the probe timer expires before the client has 1-RTT keys,
+it is possible that the client may not have any crypto data to retransmit.
+However, the client MUST send a new packet, containing only PADDING frames if
+necessary, to allow the server to continue sending data. If Handshake keys
+are available to the client, it MUST send a Handshake packet, and otherwise
+it MUST send an Initial packet in a UDP datagram of at least 1200 bytes.
+
+Because Initial packets only containing PADDING do not elicit an

nit: "only containing" => "containing only"

> +data.  If no data can be sent, then the PTO alarm MUST NOT be armed until
+data has been received from the client.
+
+Because the server could be blocked until more packets are received, the client
+MUST ensure that the retransmission timer is set if the client does not yet
+have 1-RTT keys.  If the probe timer expires before the client has 1-RTT keys,
+it is possible that the client may not have any crypto data to retransmit.
+However, the client MUST send a new packet, containing only PADDING frames if
+necessary, to allow the server to continue sending data. If Handshake keys
+are available to the client, it MUST send a Handshake packet, and otherwise
+it MUST send an Initial packet in a UDP datagram of at least 1200 bytes.
+
+Because Initial packets only containing PADDING do not elicit an
+acknowledgement, they may never be acknowledged, but they are removed from
+bytes in flight when the client gets Handshake keys and the Initial keys are
+discarded.

Because of this, I wonder if it might make sense to include a PING along with an otherwise PADDING only packet?

>  ## Probe Timeout {#pto}
 
-A Probe Timeout (PTO) triggers a probe packet when ack-eliciting data is in
-flight but an acknowledgement is not received within the expected period of
-time.  A PTO enables a connection to recover from loss of tail packets or acks.
-The PTO algorithm used in QUIC implements the reliability functions of Tail Loss
-Probe {{?TLP=I-D.dukkipati-tcpm-tcp-loss-probe}} {{?RACK}}, RTO {{?RFC5681}} and
+A Probe Timeout (PTO) triggers sending one or two probe packets when

This is probably obvious but it doesn't seem to explicitly say what causes 1 vs 2 packet to be sent. Perhaps this is clear elsewhere? I wonder if it might make sense to say something like "two packets, if there is sufficient data available, and one otherwise". But maybe that's not really 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/2806#pullrequestreview-260823749

---

From: ianswett <[email-masked]>
Date: Sat, 13 Jul 2019 04:18:39 -0700

d3adee48beb3b2ad53336c7bdb8ab1f835c8ecc8  Ryan's suggestion

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

---

From: ianswett <[email-masked]>
Date: Sat, 13 Jul 2019 04:23:01 -0700

Thanks Ryan, I took your editorial suggestion, I can file an issue for the last comment.

It's 1 or 2 because some impls only do 1 packet PTO(TLP).  I could recommend two packets during the handshake, in order to speed handshake completion?

It actually should be datagrams, but I'm inclined to change that in a follow-up, rather that burying it in this change?

True, we recommend that elsewhere.  I think we should change that in a follow-up though, since it's not directly related to this 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/2806#pullrequestreview-261537548

---

From: ianswett <[email-masked]>
Date: Sat, 13 Jul 2019 04:31:50 -0700

#2895 filed

-- 
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/2806#discussion_r303202308

---

From: ianswett <[email-masked]>
Date: Mon, 15 Jul 2019 20:00:12 -0700

Merged #2806 into master.

-- 
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/2806#event-2484757629