Subject: [quicwg/base-drafts] Add handshake retransmissions (#266)

From: ianswett <[email-masked]>
Date: Tue, 07 Feb 2017 10:39:05 -0800

Add details about handshake retransmissions for issue #169 
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * Add handshake retransmissions

-- File Changes --

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

-- Patch Links --

https://github.com/quicwg/base-drafts/pull/266.patch
https://github.com/quicwg/base-drafts/pull/266.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/266

---

From: martinduke <[email-masked]>
Date: Tue, 07 Feb 2017 14:57:27 -0800

martinduke requested changes on this pull request.

I think the logic is fine, though there are some fairly significant editorial comments.

I don't believe this fully addresses #169, although from Tokyo I gather that some of the things I was worried about are actually application-layer actions.

> @@ -414,6 +445,15 @@ acknowledged.  DetectLostPackets is called every time there is a new largest
 packet or if the loss detection alarm fires the previous largest acked packet is
 supplied.
 
+### Handshake Packets
+
+The receiver MUST not trust an unencryped ack for encrypted packets.  The receiver

what does it mean to "trust" an ack? Perhaps " the receiver MUST ignore any acknowledgment of an encrypted packet that occurs in an ACK frame in an unencrypted packet"?

> @@ -392,6 +357,72 @@ Pseudocode for OnPacketAcked follows:
      sent_packets.remove(acked_packet);
 ~~~
 
+## Setting the Loss Detection Alarm
+
+QUIC loss detection uses a single alarm for all timer-based loss detection.  The
+duration of the alarm is based on the alarm's mode, which is set in the packet
+and timer events further below.  The function SetLossDetectionAlarm defined
+below shows how the single timer is set based on the alarm mode.
+
+### Handshake Packets
+
+The initial client hello has no prior RTT sample.  A client SHOULD remember
+the previous RTT it observed when resumption is attempted and use that for an
+initial RTT value.  If no previous RTT is available, the initial RTT defaults
+to 200ms.  Once an RTT measurement is taken, it MUST replace initial_rtt.
+
+The client hello is retransmitted via a timer, similar to a tail loss probe, but

Some issues with the client and server paragraphs:
- I think all the language about tail loss probe and delayed ack confuses it.
- I'm afraid "client hello" is handshake specific
- Separating out client and server behavior is also unnecessary, as the psuedocode is the same
- This doesn't totally reflect the pseudo code. 

So how about striking these two paragraphs and replacing with the following:
"Endpoints MUST retransmit handshake packets if not acknowledged(1) within a time limit. This time limit will be the largest of the rtt value and MinTLPTimeout. For each previous handshake timeout on this connection, the timeout doubles."

"Endpoints MUST(?) also retransmit handshake packets if they receive a packet from earlier in the handshake, indicating loss of the local packet."

(1) I don't remember, must we ack the client hello, or is the server hello sufficient?

> +
+The initial client hello has no prior RTT sample.  A client SHOULD remember
+the previous RTT it observed when resumption is attempted and use that for an
+initial RTT value.  If no previous RTT is available, the initial RTT defaults
+to 200ms.  Once an RTT measurement is taken, it MUST replace initial_rtt.
+
+The client hello is retransmitted via a timer, similar to a tail loss probe, but
+it is expected client hellos will be acked immediately or a handshake packet will
+be sent immediately, so delayed acks do not need to be compensated for.  As such,
+it is always retransmitted at 2x the initial_rtt or smoothed_rtt.
+
+Server handshake packets may be sent based on a similar timer, or triggered by
+receipt of another client hello.  
+
+Version negotiation packets are always stateless, and MUST be sent once per
+client hello, and may be sent in response to 0RTT packets.

...per handshake packet that uses an unsupported QUIC version,...

s/may/MAY

-- 
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/266#pullrequestreview-20621785

---

From: Martin Thomson <[email-masked]>
Date: Tue, 07 Feb 2017 16:12:12 -0800

martinthomson commented on this pull request.

You really need to stop using the github editor for doing this stuff.  It shows.

> @@ -213,14 +213,17 @@ or negotiated in order to better suit a variety of environments.
   a packet lost.  In fraction of an RTT.
 
 * kMinTLPTimeout: 10ms
- Minimum time in the future a tail loss probe alarm may be set for.
+  Minimum time in the future a tail loss probe alarm may be set for.

Editorial: this might look OK in markdown, but it turns into a real mess in the text version.  I think that you want something like the following:

```
kMinTLPTimeout (default 10ms):
 : Minimum time in the future [...]
```

An ACK for the ClientHello isn't needed, but it does contain some useful information (if you don't care about provenance, that is).

@martinduke, your use of "timeout" in that proposed text is problematic I think.  It could refer to the handshake timing out, but I think that you simply intended to retransmit on a doubling interval (i.e., 200ms, 400ms, 600ms, etc...).

I wonder if some amount of randomization on that retransmission is necessary to avoid synchronizing retransmissions across a large cohort of clients that are simultaneously disconnected, or whether variations in RTT will be enough to deal with that.

Odd to be talking about TLP when you don't have text on TLP yet.

+### Handshake Packets
+

s/unencryped/unencrypted

More generally, use "protected" rather than "encrypted' please.  We have both confidentiality and integrity protection and both are relevant.

+### Handshake Packets
+

@martinduke is right to ask, note that not just the acknowledgment, but the entire packet is now suspect.  As we discussed elsewhere, I think that the entire packet needs to go away if it contains suspicious content.

> @@ -431,7 +471,7 @@ Pseudocode for DetectLostPackets follows:
                 acked_packet.packet_number - reordering_threshold)
          lost_packets.insert(unacked_packet.packet_number);
      return lost_packets;
-~~~
+~~~ 

trailing space 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/266#pullrequestreview-20653838

---

From: martinduke <[email-masked]>
Date: Tue, 07 Feb 2017 16:41:37 -0800

martinduke commented on this pull request.

@martinthomson Yeah, maybe "For each previous handshake restransmission on this connection, the time limit doubles".

The way I wrote this actually implies that the timeout doubling doesn't actually reset, i.e. chlo times out twice, causing the timeout to double. Then the Finished is lost, and remains at the doubled length. I'm not sure if I meant to do that, but be aware of it.

---

From: ianswett <[email-masked]>
Date: Thu, 09 Feb 2017 10:59:45 -0800

@ianswett pushed 1 commit.

f0edf20  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/266/files/90c32c3ce444bf088be8cfeb2a77fa60367893dc..f0edf20b2157641148f02c72f4c85a071e2c0665

---

From: ianswett <[email-masked]>
Date: Thu, 09 Feb 2017 11:00:10 -0800

ianswett commented on this pull request.

Thanks, updated with one follow up comment on protected terminology.

 * kMinTLPTimeout: 10ms

I tweaked it to say default as you said, but the extra : looked odd in markdown and text, so I didn't add it.  

Is there an example formatting you were trying to achieve I should copy?

* I'll remove the TLP/delayed ack wording.  It was there for motivation, but seems to be confusing more than helping.
* Changed client hello to initial flight
* Client vs server does matter in the case of stateless rejects, but I'll word that in as a special case where the connection 'terminates immediately' once the reject is sent.

I don't believe any of the TCP specs worry about randomization of handshake timers, so I don't think it's a large issue in practice.  

Removed.

Thanks done.

+### Handshake Packets
+

SG, will ignore the entire packet.

+### Handshake Packets
+

So that'd be unprotected packets?  Or is there a better term?

Done

-- 
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/266#pullrequestreview-20800622

---

From: martinduke <[email-masked]>
Date: Thu, 09 Feb 2017 11:58:07 -0800

martinduke approved this pull request.

Looks good to me, except for the one issue.

>  the previous RTT it observed when resumption is attempted and use that for an
 initial RTT value.  If no previous RTT is available, the initial RTT defaults
 to 200ms.  Once an RTT measurement is taken, it MUST replace initial_rtt.
 
-The client hello is retransmitted via a timer, similar to a tail loss probe, but
-it is expected client hellos will be acked immediately or a handshake packet will
-be sent immediately, so delayed acks do not need to be compensated for.  As such,
-it is always retransmitted at 2x the initial_rtt or smoothed_rtt.
+Endpoints MUST retransmit handshake packets if not acknowledged(1) within a

The (1) was my reference to a footnote in my comment. I think "not acknowledged" has to extend to be "not acknowledged and the next handshake message didn't arrive" but I'm not sure how to word 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/266#pullrequestreview-21104017

---

From: ianswett <[email-masked]>
Date: Thu, 09 Feb 2017 12:15:13 -0800

488f617  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/266/files/f0edf20b2157641148f02c72f4c85a071e2c0665..488f6170dd38c17b8d6174acb7392fae221580af

---

From: ianswett <[email-masked]>
Date: Thu, 09 Feb 2017 12:18:28 -0800

Minor update, PTAL.

Thanks, I removed it and tweaked the text about exponential backoff again.  Currently in the pseudocode(and the real code), all timers have their exponential backoff reset when an ack is received.  The idea being, if you're getting some packets through, based the retransmission time on RTT.  If you're not getting any data through, exponentially backoff because your RTT estimate may be wrong, and to avoid overloading the network.

-- 
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/266#pullrequestreview-21109157

---

From: martinduke <[email-masked]>
Date: Thu, 09 Feb 2017 12:24:11 -0800

So I'm still concerned that, as written, this requires to retransmit the client hello because we got a server hello instead of an ack. Unless I've missed something, and we're requiring ACKs, which would make the protocol quite a bit cleaner.

-- 
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/266#issuecomment-278762108

---

From: ianswett <[email-masked]>
Date: Thu, 09 Feb 2017 12:32:38 -0800

99eb478  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/266/files/488f6170dd38c17b8d6174acb7392fae221580af..99eb478be80e68ea6f751a053bacd888509936b1

---

From: ianswett <[email-masked]>
Date: Thu, 09 Feb 2017 12:35:42 -0800

Good point, that's a bit awkward.  You're right, it'd be best if we could just rely on ack frames.  I added some text on crypto state transitions, but I'm not sure I like it.  For now, we could just require they be properly acked, since we can include acks in crypto 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/266#issuecomment-278765360

---

From: Mike Bishop <[email-masked]>
Date: Thu, 09 Feb 2017 13:50:05 -0800

MikeBishop commented on this pull request.

 * kMinTLPTimeout: 10ms

The extra ':' is correct -- it's Markdown formatting for a [definition list](https://kramdown.gettalong.org/syntax.html#definition-lists).

---

From: Mike Bishop <[email-masked]>
Date: Thu, 09 Feb 2017 13:51:49 -0800

As Martin said, you need to find something better than the GitHub editor, or you'll keep having funky alignment and broken builds due to whitespace.  I've recently switched to [Atom](http://www.atom.io), which seems to be working pretty well for me and has a pretty rich collection of plugins.  You might give it a shot.

-- 
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/266#issuecomment-278785574

---

From: Martin Thomson <[email-masked]>
Date: Thu, 09 Feb 2017 15:55:37 -0800

* kMinTLPTimeout: 10ms

Note that the '*' at the start of the definition title line prevents kramdown from detecting it as a definition list.  You will note its absence in my example.

You also need to reformat the whole list consistent or, as you say, it looks odd.

---

From: Martin Thomson <[email-masked]>
Date: Thu, 09 Feb 2017 15:56:25 -0800

+### Handshake Packets
+

That is right.  "unprotected" isn't great, but I don't have anything better.

---

From: Mike Bishop <[email-masked]>
Date: Mon, 13 Feb 2017 16:50:07 -0800

@MikeBishop pushed 1 commit.

ae14993  Fix Ian's trailing whitespace

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/266/files/99eb478be80e68ea6f751a053bacd888509936b1..ae14993a65ef726f67127af6432a2a394d35575e

---

From: ianswett <[email-masked]>
Date: Tue, 14 Feb 2017 08:33:07 -0800

Are there any other web editors that are better?  Alternately, can we just add a presubmit that strips whitespace and wraps lines?

Also, how are you seeing the trailing whitespace and long lines?  

-- 
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/266#issuecomment-279759477

---

From: Martin Thomson <[email-masked]>
Date: Tue, 14 Feb 2017 15:22:36 -0800

@ianswett, when you look at the diff that you produce, it shows both.  Long lines are harder to detect because github doesn't have column markers, but you can see really long lines.

-- 
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/266#issuecomment-279868868

---

From: Martin Thomson <[email-masked]>
Date: Tue, 14 Feb 2017 16:14:06 -0800

@martinthomson pushed 1 commit.

f759b4b  Merge branch 'master' into ianswett-patch-3

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

---

From: Martin Thomson <[email-masked]>
Date: Thu, 16 Feb 2017 19:25:03 -0800

a9d25dd  Merge branch 'master' into ianswett-patch-3

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

---

From: Martin Thomson <[email-masked]>
Date: Thu, 16 Feb 2017 19:27:34 -0800

@ianswett, I don't know what you are waiting for before merging this.  This is only rotting sitting here like this.  If it is a review from @janaiyengar, I'd suggest that you have provide ample opportunity.

-- 
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/266#issuecomment-280539904

---

From: Martin Thomson <[email-masked]>
Date: Thu, 16 Feb 2017 19:33:38 -0800

4f5fb74  Reformat example

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

---

From: Martin Thomson <[email-masked]>
Date: Thu, 16 Feb 2017 19:34:59 -0800

I've corrected the errors that were in this patch by folding a bunch of lines.

Ian, you are quite inconsistent with a few things in the diagrams
* the use of semi-colons at the end of statements
* `x` vs `*` for multiplication

That shouldn't block this PR, but it's probably worth fixing separately.

-- 
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/266#issuecomment-280540916

---

From: ianswett <[email-masked]>
Date: Fri, 17 Feb 2017 05:48:42 -0800

I was waiting for Jana, but yes, at this point, I'll merge and fix all the issues which have come up since then.

-- 
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/266#issuecomment-280653826

---

From: ianswett <[email-masked]>
Date: Fri, 17 Feb 2017 05:49:00 -0800

Merged #266.

-- 
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/266#event-966903321