Subject: [quicwg/base-drafts] Replace handshake with crypto (#1859)

From: ianswett <[email-masked]>
Date: Mon, 15 Oct 2018 13:47:41 -0700

This replaces #1842 and tries to fix everything to consistently use crypto packets instead of handshake packets.

Fixes #1836.
You can view, comment on, or merge this pull request online at:

  https://github.com/quicwg/base-drafts/pull/1859

-- Commit Summary --

  * Replace handshake with crypto

-- File Changes --

    M draft-ietf-quic-recovery.md (51)

-- Patch Links --

https://github.com/quicwg/base-drafts/pull/1859.patch
https://github.com/quicwg/base-drafts/pull/1859.diff

-- 
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/1859

---

From: Martin Thomson <[email-masked]>
Date: Mon, 15 Oct 2018 22:20:41 +0000 (UTC)

martinthomson approved this pull request.

> @@ -302,11 +302,10 @@ Retransmission Timeout mechanisms.
 ### Crypto Handshake Timeout
 
 Data in CRYPTO frames is critical to QUIC transport and crypto negotiation, so a
-more aggressive timeout is used to retransmit it.  Below, the term "handshake
-packet" is used to refer to packets containing CRYPTO frames, not packets with
-the specific long header packet type Handshake.
+more aggressive timeout is used to retransmit it.  Below, the term "crypto
+packet" is used to refer to packets containing CRYPTO frames.

We should further qualify this: "Handshake or Initial packets containing CRYPTO frames", because CRYPTO frames after the handshake completes don't require special handling.

 When a connection has unacknowledged handshake data, the handshake timer is
-set and when it expires, all unacknowledgedd handshake data is retransmitted.
+set and when it expires, all unacknowledgedd CRYPTO data is retransmitted.

typo: dd

-The initial handshake timeout SHOULD be set to twice the initial RTT.
+The initial crypto timeout SHOULD be set to twice the initial RTT.

Can we rename this?  I initially assumed that the handshake would be abandoned when this fires.  It's a crypto retransmission timeout (a term you already use).

>        loss_detection_timer.set(
-        time_of_last_sent_handshake_packet + timeout)
+        time_of_last_sent_crypto_packet + timeout)
       return;

extra semi-colon

>        loss_detection_timer.set(
-        time_of_last_sent_handshake_packet + timeout)
+        time_of_last_sent_crypto_packet + timeout)
       return;
     else if (loss_time != 0):

No need for an else after a return

-- 
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/1859#pullrequestreview-164915515

---

From: janaiyengar <[email-masked]>
Date: Mon, 15 Oct 2018 15:55:30 -0700

janaiyengar approved this pull request.

LGTM, modulo @martinthomson 's suggestions, which are all fine with me.

+1

-- 
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/1859#pullrequestreview-164926639

---

From: ianswett <[email-masked]>
Date: Mon, 15 Oct 2018 17:08:41 -0700

ianswett commented on this pull request.

Done, but is the term "handshake complete" well-defined?  I wanted to use long header packets, but I realized that the CFIN is head of line blocking :(

-- 
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/1859#discussion_r225356198

---

From: ianswett <[email-masked]>
Date: Mon, 15 Oct 2018 17:10:18 -0700

I went with consistently using crypto handshake timeout, because I think it reads well and is consistent, but I can change it to something else if you prefer.

-- 
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/1859#discussion_r225356486

---

From: ianswett <[email-masked]>
Date: Mon, 15 Oct 2018 17:12:23 -0700

@ianswett pushed 1 commit.

602b196  Martin's comments

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1859/files/4d37bf0bd21795d9d8e732226916962a09e9f3d1..602b196f4ff9d04870b7830dda60727da8f255b7

---

From: ianswett <[email-masked]>
Date: Mon, 15 Oct 2018 17:23:00 -0700

d2ffb44  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/1859/files/602b196f4ff9d04870b7830dda60727da8f255b7..d2ffb449891235f15ea7a7153f02212a267c9ea7

---

From: Martin Thomson <[email-masked]>
Date: Mon, 15 Oct 2018 18:17:10 -0700

> @@ -302,11 +302,11 @@ Retransmission Timeout mechanisms.
 
+packet" is used to refer to packets containing CRYPTO frames that are part of
+the handshake.

Better to say "Initial and Handshake" explicitly.

-The initial handshake timeout SHOULD be set to twice the initial RTT.
+The initial crypto handshake timeout SHOULD be set to twice the initial RTT.

initial crypto **retransmission** 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/1859#pullrequestreview-164952857

---

From: ianswett <[email-masked]>
Date: Mon, 15 Oct 2018 18:22:47 -0700

859b2fe  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/1859/files/d2ffb449891235f15ea7a7153f02212a267c9ea7..859b2fef148df3d81c93a6f2c05e85aea119069c

---

From: Martin Thomson <[email-masked]>
Date: Mon, 15 Oct 2018 20:23:24 -0700

@martinthomson pushed 1 commit.

b8208a0  remove stray keypress

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1859/files/859b2fef148df3d81c93a6f2c05e85aea119069c..b8208a08b102f54ecf6da716fcdb41b3b3574cb6

---

From: ianswett <[email-masked]>
Date: Tue, 16 Oct 2018 06:37:37 -0700

4867882  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/1859/files/b8208a08b102f54ecf6da716fcdb41b3b3574cb6..4867882c5a7f639475938a007b9467e3dc7bb643

---

From: ianswett <[email-masked]>
Date: Tue, 16 Oct 2018 06:39:20 -0700

2ddd001  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/1859/files/4867882c5a7f639475938a007b9467e3dc7bb643..2ddd001c0f2eff734c79433d7eac771cca84eddc

---

From: Mike Bishop <[email-masked]>
Date: Tue, 16 Oct 2018 11:14:25 -0700

MikeBishop commented on this pull request.

This still seems to be somewhat inconsistent with the use of new names.  "Crypto handshake timeout" vs. "crypto retransmission timeout" vs. "crypto timeout period," for example.  Similarly "Crypto handshake timer" vs. "crypto retransmission timer."  I'd pick one of each and use it consistently.

> -CRYPTO data by calling RetransmitAllUnackedHandshakeData(). On each
-consecutive expiration of the handshake timer without receiving an
-acknowledgement for a new packet, the sender SHOULD double the handshake timeout
-and set a timer for this period.
+smoothed RTT value as the resumed connection's initial RTT.  If no previous RTT
+is available, or if the network changes, the initial RTT SHOULD be set to 100ms.
+When an acknowledgement is received, a new RTT is computed and the timer
+SHOULD be set for twice the newly computed smoothed RTT.
+
+When CRYPTO frames are sent in Initial or Handshake packets, the sender SHOULD
+set a timer for the crypto timeout period.  Upon timeout, the sender MUST
+retransmit all unacknowledged CRYPTO data by calling
+RetransmitAllUnackedCryptoData(). On each consecutive expiration of the
+crypto timer without receiving an acknowledgement for a new packet, the
+sender SHOULD double the crypto handshake timeout and set a timer for this
+period.
 
 When CRYPTO frames are outstanding, the TLP and RTO timers are not active unless
 the CRYPTO frames were sent at 1-RTT encryption.

Now that you have a definition for "crypto packets" above which excludes 1-RTT, you could probably simplify this by saying that "When crypto packets are outstanding, the TLS and RTO timers are not active."

> -
-If no previous RTT is available, or if the network changes, the initial RTT
-SHOULD be set to 100ms.
-
-When CRYPTO frames are sent, the sender SHOULD set a timer for the handshake
-timeout period.  Upon timeout, the sender MUST retransmit all unacknowledged
-CRYPTO data by calling RetransmitAllUnackedHandshakeData(). On each
-consecutive expiration of the handshake timer without receiving an
-acknowledgement for a new packet, the sender SHOULD double the handshake timeout
-and set a timer for this period.
+smoothed RTT value as the resumed connection's initial RTT.  If no previous RTT
+is available, or if the network changes, the initial RTT SHOULD be set to 100ms.
+When an acknowledgement is received, a new RTT is computed and the timer
+SHOULD be set for twice the newly computed smoothed RTT.
+
+When CRYPTO frames are sent in Initial or Handshake packets, the sender SHOULD

Likewise, you can condense to "When crypto packets are sent,"

> @@ -558,7 +557,7 @@ largest_sent_before_rto:
 time_of_last_sent_retransmittable_packet:
 : The time the most recent retransmittable packet was sent.
 
-time_of_last_sent_handshake_packet:
+time_of_last_sent_crypto_packet:
 : The time the most recent packet containing a CRYPTO frame was sent.

This includes 1-RTT as currently defined, but I don't think you want it to.

-- 
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/1859#pullrequestreview-165293058

---

From: martinduke <[email-masked]>
Date: Tue, 16 Oct 2018 12:32:00 -0700

martinduke approved this pull request.

 A couple of nits, but looks good to me.

TLS->TLP

> @@ -302,32 +302,31 @@ Retransmission Timeout mechanisms.
 
+packet" is used to refer to packets containing CRYPTO data sent in Initial or

Maybe add this definition to "Conventions and Definitions" above?

-- 
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/1859#pullrequestreview-165328354

---

From: martinduke <[email-masked]>
Date: Tue, 16 Oct 2018 19:43:10 +0000 (UTC)

martinduke commented on this pull request.

Other nits:
L299: "handshake retransmission" -> "crypto handshake retransmission"
L463: "handshake timeouts" -> "crypto handshake timeouts"
L785: "when there is no unacknowledged handshake data" -> "when there are no unacknowledged crypto packets"

> @@ -764,10 +763,10 @@ duration of the timer is based on the timer's mode, which is set in the packet
 and timer events further below.  The function SetLossDetectionTimer defined
 below shows how the single timer is set.
 
-#### Handshake Timer
+#### Crypto Handshake Timer

"handshake data" -> "crypto handshake data"
"handshake timer" -> "crypto handshake timer"

-- 
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/1859#pullrequestreview-165334365

---

From: ianswett <[email-masked]>
Date: Tue, 16 Oct 2018 12:49:56 -0700

7a2e233  Most of Mike's comments

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

---

From: ianswett <[email-masked]>
Date: Tue, 16 Oct 2018 12:54:35 -0700

68cfbaa  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/1859/files/7a2e233a9a2ad16e2c745c13440111f997aa7337..68cfbaa2c573c2236fdb8048ee10a7002c807616

---

From: ianswett <[email-masked]>
Date: Tue, 16 Oct 2018 14:59:46 -0700

Merged #1859 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/1859#event-1908065974