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

From: ianswett <[email-masked]>
Date: Tue, 03 Sep 2019 15:43:40 -0700

@ianswett pushed 1 commit.

617c4a7d1b2cc2b254935e0a5c48753f00b030c9  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/b47dbbc79b26d958d54146d09c0c172cd28db5d4..617c4a7d1b2cc2b254935e0a5c48753f00b030c9

---

From: ianswett <[email-masked]>
Date: Tue, 03 Sep 2019 15:43:47 -0700

d2d1bb8075af5460de0bfd2d5e604fce502168c0  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/617c4a7d1b2cc2b254935e0a5c48753f00b030c9..d2d1bb8075af5460de0bfd2d5e604fce502168c0

---

From: ianswett <[email-masked]>
Date: Tue, 03 Sep 2019 15:48:51 -0700

fd8bdd7adecd6f5fde89e440d1ae18eb28743d7a  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/d2d1bb8075af5460de0bfd2d5e604fce502168c0..fd8bdd7adecd6f5fde89e440d1ae18eb28743d7a

---

From: ianswett <[email-masked]>
Date: Tue, 03 Sep 2019 15:50:46 -0700

9a77e26fa76c8290d1738c53f07666de75ebad47  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/fd8bdd7adecd6f5fde89e440d1ae18eb28743d7a..9a77e26fa76c8290d1738c53f07666de75ebad47

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 03 Sep 2019 15:58:17 -0700

janaiyengar commented on this pull request.

> @@ -1189,10 +1191,13 @@ SetLossDetectionTimer():
     loss_detection_timer.update(loss_time)
     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):
+  // Cancel the timer if there are no ack-eliciting packets
+  // in flight and the endpoint is a server. Do not cancel the
+  // timer until the client has received a Handshake or 1-RTT ACK.
+  if (no ack-eliciting packets in flight &&
+      (endpoint is server ||
+       has received Handshake ACK ||
+       has received 1-RTT ACK)):

Suggestion: if !(ack-eliciting packet in flight || (endpoint is client && has not received handshake ACK && has not received 1-RTT ACK))

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

---

From: ianswett <[email-masked]>
Date: Tue, 03 Sep 2019 15:58:30 -0700

8eab33dad32d29fa10b618312b995eaf7c329e12  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/9a77e26fa76c8290d1738c53f07666de75ebad47..8eab33dad32d29fa10b618312b995eaf7c329e12

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 03 Sep 2019 16:03:12 -0700

> @@ -1189,10 +1191,14 @@ SetLossDetectionTimer():

```suggestion
  // in flight, with the following exception. Do not cancel the
```

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

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 03 Sep 2019 16:03:35 -0700

+  // timer until the client has received an acknowledgement that

```suggestion
  // timer at a client until the client has received an acknowledgement 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#pullrequestreview-283304626

---

From: ianswett <[email-masked]>
Date: Tue, 03 Sep 2019 16:04:25 -0700

4df749464d69b8239d173a2a183c0a7491436448  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/8eab33dad32d29fa10b618312b995eaf7c329e12..4df749464d69b8239d173a2a183c0a7491436448

---

From: ianswett <[email-masked]>
Date: Tue, 03 Sep 2019 16:04:36 -0700

04ac7b29c94e168a4bc7589420d1038a709ed876  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/4df749464d69b8239d173a2a183c0a7491436448..04ac7b29c94e168a4bc7589420d1038a709ed876

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 03 Sep 2019 16:06:31 -0700

-  // 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):
+  // Cancel the timer if there are no ack-eliciting packets
+  // in flight, with the following exception. Do not cancel the
+  // timer at a client until the client has received an acknowledgement that

```suggestion
  // timer at a client until it has received an acknowledgement 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#pullrequestreview-283305427

---

From: ianswett <[email-masked]>
Date: Tue, 03 Sep 2019 16:07:05 -0700

9f0d6eccdc61b7d4e45d64356b69a75f01e452bc  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/04ac7b29c94e168a4bc7589420d1038a709ed876..9f0d6eccdc61b7d4e45d64356b69a75f01e452bc

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 03 Sep 2019 16:08:16 -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/2907#pullrequestreview-283305882

---

From: ianswett <[email-masked]>
Date: Tue, 03 Sep 2019 16:12:48 -0700

26119bd0403ead534f6bc303df56559b16e49abf  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/9f0d6eccdc61b7d4e45d64356b69a75f01e452bc..26119bd0403ead534f6bc303df56559b16e49abf

---

From: ianswett <[email-masked]>
Date: Tue, 03 Sep 2019 17:15:20 -0700

ianswett commented on this pull request.

As a code reviewer, I'd request a method like PeerAwaitingAddressValidation() and say 

if (no ack-eliciting packets in flight &&
     !PeerAwaitingAddressValidation()):

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

---

From: ianswett <[email-masked]>
Date: Tue, 03 Sep 2019 17:20:21 -0700

ec34d084545d8831abf6d5f89c09f4bc0d8be911  Add PeerAwaitingAddressValidation

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

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 03 Sep 2019 18:00:46 -0700

Thanks for the change -- looks MUCH better now.

>        time = loss_time[pn_space];
       space = pn_space
   return time, space
 
+PeerAwaitingAddressValidation():

```suggestion
IsPeerAwaitingAddressValidation():
```

>  SetLossDetectionTimer():
   loss_time, _ = GetEarliestLossTime()
   if (loss_time != 0):
     // Time threshold loss detection.
     loss_detection_timer.update(loss_time)
     return
 
+      !PeerAwaitingAddressValidation()):

```suggestion
      !IsPeerAwaitingAddressValidation()):
```

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

---

From: Martin Thomson <[email-masked]>
Date: Tue, 03 Sep 2019 18:02:27 -0700

I shouldn't have commented on the changeset.  Here's my alternative to Jana's:

```
PeerCompletedAddressValidation():
  if endpoint is server:
    return true
  return has received Handshake ACK ||
           has received 1-RTT ACK
...

  if (no ack-eliciting packets in flight &&
      PeerCompletedAddressValidation()): 
```

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

---

From: ianswett <[email-masked]>
Date: Tue, 03 Sep 2019 18:13:32 -0700

e8e778cb0cbd934132d6be4e8e316f4ef7c7b970  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/ec34d084545d8831abf6d5f89c09f4bc0d8be911..e8e778cb0cbd934132d6be4e8e316f4ef7c7b970

---

From: ianswett <[email-masked]>
Date: Tue, 03 Sep 2019 18:13:48 -0700

d0b5498f344d7e60e94cfcb9637c356f6474e9d7  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/e8e778cb0cbd934132d6be4e8e316f4ef7c7b970..d0b5498f344d7e60e94cfcb9637c356f6474e9d7

---

From: ianswett <[email-masked]>
Date: Tue, 03 Sep 2019 18:31:56 -0700

c212dfdf7e7b48481f488fe3c96e207d814f8180  Some of MT's suggestions

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

---

From: ianswett <[email-masked]>
Date: Tue, 03 Sep 2019 18:33:48 -0700

92b5fd76dc8b7dc38f217ae1a6ba633a7af0d7f3  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/c212dfdf7e7b48481f488fe3c96e207d814f8180..92b5fd76dc8b7dc38f217ae1a6ba633a7af0d7f3

---

From: ianswett <[email-masked]>
Date: Tue, 03 Sep 2019 18:38:55 -0700

d55a15304192ed4456ee6eba0b6ce2ab6dfaf0f4  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/92b5fd76dc8b7dc38f217ae1a6ba633a7af0d7f3..d55a15304192ed4456ee6eba0b6ce2ab6dfaf0f4

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 03 Sep 2019 18:47:54 -0700

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

---

From: Martin Thomson <[email-masked]>
Date: Tue, 03 Sep 2019 19:22:15 -0700

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

---

From: ianswett <[email-masked]>
Date: Wed, 04 Sep 2019 01:41:00 -0700

Merged #2907 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/2907#event-2606175541