Subject: [quicwg/base-drafts] Fix Recovery Pseudocode (#2907)

From: ianswett <[email-masked]>
Date: Wed, 17 Jul 2019 15:48:07 -0700

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

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

-- Commit Summary --

  * Update draft-ietf-quic-recovery.md

-- File Changes --

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

-- Patch Links --

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

---

From: Martin Thomson <[email-masked]>
Date: Thu, 18 Jul 2019 04:20:07 -0700

When do you use parentheses? 

-- 
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/2907#issuecomment-512775188

---

From: ianswett <[email-masked]>
Date: Thu, 18 Jul 2019 05:05:00 -0700

It appears most conditionals have parentheses currently, except for 1177.  Are you suggesting to add them there or something else?

-- 
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/2907#issuecomment-512787400

---

From: Martin Thomson <[email-masked]>
Date: Fri, 19 Jul 2019 06:26:53 -0700

Pick one.  Be consistent.

-- 
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/2907#issuecomment-513228034

---

From: ianswett <[email-masked]>
Date: Fri, 19 Jul 2019 06:31:35 -0700

@ianswett pushed 1 commit.

d1d9bc9d5bf77b460a14cefc94938753996c010d  Consistent ifs

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

---

From: ianswett <[email-masked]>
Date: Fri, 19 Jul 2019 06:31:51 -0700

SG, done.  I also fixed some missing ':'

-- 
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/2907#issuecomment-513229642

---

From: martinduke <[email-masked]>
Date: Sat, 20 Jul 2019 14:56:15 -0700

martinduke requested changes on this pull request.

> @@ -1188,9 +1188,10 @@ SetLossDetectionTimer():
     return
 
   // Don't arm timer if there are no ack-eliciting packets
-  // in flight and the handshake is complete.
-  if (endpoint is client with 1-RTT keys &&
-      no ack-eliciting packets in flight):
+  // in flight and the endpoint is a server or has
+  // completed the handshake.
+  if (no ack-eliciting packets in flight &&
+      (endpoint is server or has 1-RTT keys)):

endpoint is server or has received hs ack or an ack of an 1-RTT packet (not a 1-RTT packet, this could be an ack of 0RTT)

-- 
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/2907#pullrequestreview-264512654

---

From: martinduke <[email-masked]>
Date: Sat, 20 Jul 2019 15:05:21 -0700

martinduke commented on this pull request.

     return

Sorry, just add "has received HS ack" and we're good. There's a case where the server flight doesn't fit in a cwnd and the extra text may save us a crypto 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/2907#discussion_r305594861

---

From: ianswett <[email-masked]>
Date: Sat, 20 Jul 2019 15:33:52 -0700

26ecb8f304b977f753cb9afedd25e843600146f3  Martin Duke's suggestion

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2907/files/d1d9bc9d5bf77b460a14cefc94938753996c010d..26ecb8f304b977f753cb9afedd25e843600146f3

---

From: Martin Thomson <[email-masked]>
Date: Sat, 20 Jul 2019 18:06:35 -0700

martinthomson commented on this pull request.

>    if (no ack-eliciting packets in flight &&
-      (endpoint is server or has 1-RTT keys)):
+      (endpoint is server or has 1-RTT keys
+       or has received Handshake ACK)):

we should be consistent about || vs or and && vs and

-- 
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/2907#pullrequestreview-264516309

---

From: ianswett <[email-masked]>
Date: Sat, 20 Jul 2019 18:14:44 -0700

b9779401d24a9d1c9c98088dd3dce72df04a7730  Remove ||

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2907/files/26ecb8f304b977f753cb9afedd25e843600146f3..b9779401d24a9d1c9c98088dd3dce72df04a7730

---

From: Martin Thomson <[email-masked]>
Date: Sat, 20 Jul 2019 18:54:05 -0700

> @@ -1175,7 +1175,7 @@ GetEarliestLossTime():
   space = Initial
   for pn_space in [ Handshake, ApplicationData ]:
     if (loss_time[pn_space] != 0 &&
-        (time == 0 || loss_time[pn_space] < time)):
+        (time == 0 or loss_time[pn_space] < time)):

so && for and and "or" for || ?

-- 
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/2907#pullrequestreview-264517063

---

From: ianswett <[email-masked]>
Date: Sun, 21 Jul 2019 04:29:32 -0700

ianswett commented on this pull request.

   space = Initial

Prior to this PR, most of the pseudocode was && and || I realize, so I'm going back to 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/2907#discussion_r305613331

---

From: ianswett <[email-masked]>
Date: Sun, 21 Jul 2019 04:30:58 -0700

f079e9dc3664d2abe4e5a0b1b615c826fbd9be15  Use || and && consistently

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

---

From: martinduke <[email-masked]>
Date: Sun, 21 Jul 2019 12:36:43 -0700

martinduke approved this pull request.

> @@ -581,13 +581,13 @@ 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.
+MUST ensure that the retransmission timer is set if the client has not received
+an ACK in a Handshake packet or does not yet have 1-RTT keys.  If the probe

or -> and

-- 
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/2907#pullrequestreview-264551273

---

From: Jana Iyengar <[email-masked]>
Date: Sun, 21 Jul 2019 12:55:56 -0700

janaiyengar commented on this pull request.

I think this should be 'or'.

-- 
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/2907#discussion_r305633369

---

From: Jana Iyengar <[email-masked]>
Date: Sun, 21 Jul 2019 12:59:42 -0700

> @@ -1188,9 +1188,13 @@ SetLossDetectionTimer():
     return
 
   // Don't arm timer if there are no ack-eliciting packets
-  // in flight and the handshake is complete.
-  if (endpoint is client with 1-RTT keys &&
-      no ack-eliciting packets in flight):
+  // in flight and the endpoint is a server.  Do arm the
+  // timer until the client has received a Handshake ACK
+  // or completed the handshake.

```suggestion
  // or has completed 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/2907#pullrequestreview-264552221

---

From: Jana Iyengar <[email-masked]>
Date: Sun, 21 Jul 2019 13:05:44 -0700

You'll need to rebase to get the changes from #2912 in 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/2907#issuecomment-513583482

---

From: Jana Iyengar <[email-masked]>
Date: Sun, 21 Jul 2019 13:06:22 -0700

This looks good to me, module rebasing and working out the conflicts.

-- 
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/2907#pullrequestreview-264552410

---

From: ianswett <[email-masked]>
Date: Sun, 21 Jul 2019 14:40:31 -0700

08ea0f58a7562c1c091e7678e20654bb283cbc07  Merge branch 'master' into ianswett-fix-loss-detection

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2907/files/f079e9dc3664d2abe4e5a0b1b615c826fbd9be15..08ea0f58a7562c1c091e7678e20654bb283cbc07

---

From: ianswett <[email-masked]>
Date: Sun, 21 Jul 2019 14:41:33 -0700

59c3af3f32313b54f31eacd6d64df1ad19c1210c  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/2907/files/08ea0f58a7562c1c091e7678e20654bb283cbc07..59c3af3f32313b54f31eacd6d64df1ad19c1210c