Add handshake retransmissions #266

ianswett · 2017-02-07T18:39:04Z

Add details about handshake retransmissions for issue #169

martinduke

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.

martinduke · 2017-02-07T21:15:07Z

draft-ietf-quic-recovery.md

@@ -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"?

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

SG, will ignore the entire packet.

martinduke · 2017-02-07T21:21:10Z

draft-ietf-quic-recovery.md

+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?

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.

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

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.

martinduke · 2017-02-07T21:27:46Z

draft-ietf-quic-recovery.md

+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

Thanks done.

martinthomson

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

martinthomson · 2017-02-08T00:02:33Z

draft-ietf-quic-recovery.md

@@ -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 [...]

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?

The extra ':' is correct -- it's Markdown formatting for a definition list.

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.

martinthomson · 2017-02-08T00:07:27Z

draft-ietf-quic-recovery.md

+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


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.

martinthomson · 2017-02-08T00:09:00Z

draft-ietf-quic-recovery.md

+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


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

martinthomson · 2017-02-08T00:09:29Z

draft-ietf-quic-recovery.md

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


s/unencryped/unencrypted

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

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

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

martinthomson · 2017-02-08T00:10:40Z

draft-ietf-quic-recovery.md

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


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

martinthomson · 2017-02-08T00:11:35Z

draft-ietf-quic-recovery.md

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

ianswett

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

ianswett · 2017-02-08T16:20:27Z

draft-ietf-quic-recovery.md

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


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?

ianswett · 2017-02-08T16:38:53Z

draft-ietf-quic-recovery.md

+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


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.

ianswett · 2017-02-08T16:38:59Z

draft-ietf-quic-recovery.md

+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


ianswett · 2017-02-09T18:55:04Z

draft-ietf-quic-recovery.md

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


Thanks done.

ianswett · 2017-02-09T18:56:26Z

draft-ietf-quic-recovery.md

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


SG, will ignore the entire packet.

ianswett · 2017-02-09T18:58:21Z

draft-ietf-quic-recovery.md

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


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

ianswett · 2017-02-09T18:59:14Z

draft-ietf-quic-recovery.md

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


martinduke

Looks good to me, except for the one issue.

martinduke · 2017-02-09T19:54:52Z

draft-ietf-quic-recovery.md

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

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.

ianswett

Minor update, PTAL.

ianswett · 2017-02-09T20:18:17Z

draft-ietf-quic-recovery.md

-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


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.

martinduke · 2017-02-09T20:24:10Z

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.

Add some text on handshake state transitions cancelling handshake frames.

ianswett · 2017-02-09T20:35:41Z

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.

MikeBishop · 2017-02-09T21:51:48Z

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, which seems to be working pretty well for me and has a pretty rich collection of plugins. You might give it a shot.

ianswett · 2017-02-14T16:33:06Z

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?

martinthomson · 2017-02-14T23:22:35Z

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

martinthomson · 2017-02-17T03:27:33Z

@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 already provided ample opportunity.

martinthomson · 2017-02-17T03:34:58Z

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, which I think is unnecessary
x vs * for multiplication

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

ianswett · 2017-02-17T13:48:41Z

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

Add handshake retransmissions

90c32c3

ianswett requested review from janaiyengar and martinduke February 7, 2017 18:39

martinduke suggested changes Feb 7, 2017

View reviewed changes

martinthomson reviewed Feb 8, 2017

View reviewed changes

martinthomson added the -recovery label Feb 9, 2017

Update draft-ietf-quic-recovery.md

f0edf20

ianswett commented Feb 9, 2017

View reviewed changes

martinduke approved these changes Feb 9, 2017

View reviewed changes

Update draft-ietf-quic-recovery.md

488f617

ianswett commented Feb 9, 2017

View reviewed changes

Update draft-ietf-quic-recovery.md

99eb478

Add some text on handshake state transitions cancelling handshake frames.

Fix Ian's trailing whitespace

ae14993

martinthomson added 2 commits February 15, 2017 11:14

Merge branch 'master' into ianswett-patch-3

f759b4b

Merge branch 'master' into ianswett-patch-3

a9d25dd

Reformat example

4f5fb74

ianswett merged commit e1002e5 into master Feb 17, 2017

martinthomson deleted the ianswett-patch-3 branch February 21, 2017 02:53

Add handshake retransmissions #266

Add handshake retransmissions #266

Conversation

ianswett commented Feb 7, 2017

martinduke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinthomson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianswett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinduke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianswett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinduke commented Feb 9, 2017

ianswett commented Feb 9, 2017

MikeBishop commented Feb 9, 2017

ianswett commented Feb 14, 2017

martinthomson commented Feb 14, 2017

martinthomson commented Feb 17, 2017 • edited Loading

martinthomson commented Feb 17, 2017 • edited Loading

ianswett commented Feb 17, 2017

martinthomson commented Feb 17, 2017 •

edited

Loading

martinthomson commented Feb 17, 2017 •

edited

Loading