Subject: [quicwg/base-drafts] update the pseudo code to include packet number spaces (#2417)

From: Marten Seemann <[email-masked]>
Date: Mon, 04 Feb 2019 19:17:57 -0800

This is an ongoing attempt to resolve #2405.
In the current state this PR is nowhere near complete, but I hope it gets the discussion going about the necessary changes. I will update it accordingly.

So far, this PR makes the following changes:

- makes it explicit that the variables `largest_sent_packet`, `largest_acked_packet` and `sent_packets` are kept for every packet number space. By indexing these variables with their respective packet number space, we can immediately see where we need to pass a packet number space variable around,
- makes it explicit that `OnPacketAcked` and `DetectLostPackets` are packet-number-space dependent,
- I realized that `loss_time` would also apply to every packet number space, if we didn't have special treatment for crypto packets. Since we have that, we never run early retransmit for crypto packets, so `loss_time` only applies to the 1-RTT packet number space.
  - I'm not sure if is this a bug or a feature. The only way to declare crypto packets lost now is using the crypto timer. Contrary to what we say in the text, this is *less* aggressive than the logic we use for packets sent after completion of the handshake.
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * update the pseudo code to include packet number spaces

-- File Changes --

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

-- Patch Links --

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

---

From: Marten Seemann <[email-masked]>
Date: Tue, 05 Feb 2019 18:06:11 -0800

@ianswett, @janaiyengar Can you please review this? Is this going in the right direction?

-- 
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/2417#issuecomment-460876727

---

From: ianswett <[email-masked]>
Date: Wed, 06 Feb 2019 02:12:39 -0800

ianswett commented on this pull request.

Thanks for fixing this Marten, it looks good.  One suggestion.

> @@ -890,8 +891,9 @@ follows:
    min_rtt = infinite
    time_of_last_sent_ack_eliciting_packet = 0
    time_of_last_sent_crypto_packet = 0
-   largest_sent_packet = 0
-   largest_acked_packet = 0
+   for pn_space in packet number spaces:

I think we should define packet number spaces more clearly above, and possibly call it kPacketNumberSpaces.  I think it makes sense to define it as a list of 3 enum values.

-- 
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/2417#pullrequestreview-200495063

---

From: Marten Seemann <[email-masked]>
Date: Wed, 06 Feb 2019 13:57:54 +0000 (UTC)

@marten-seemann pushed 1 commit.

17161f54da4f76f94cd4271af303792afad40c71  introduce an enum for packet number spaces

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2417/files/1d8af249df68710203ce2ca376823ee61d2dd9cc..17161f54da4f76f94cd4271af303792afad40c71

---

From: Marten Seemann <[email-masked]>
Date: Wed, 06 Feb 2019 05:58:37 -0800

marten-seemann commented on this pull request.

    min_rtt = infinite

Not sure if we need that constant, since we're only looping over the spaces once (and only for initialization), but I introduced the enum.

-- 
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/2417#discussion_r254279810

---

From: Marten Seemann <[email-masked]>
Date: Wed, 06 Feb 2019 06:02:50 -0800

a5789417360ab50cc408d156d37d12dce167b746  fix line lengths

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2417/files/17161f54da4f76f94cd4271af303792afad40c71..a5789417360ab50cc408d156d37d12dce167b746

---

From: ianswett <[email-masked]>
Date: Wed, 06 Feb 2019 08:29:07 -0800

> @@ -903,20 +915,22 @@ to OnPacketSent are described in detail above in {{sent-packets-fields}}.
 Pseudocode for OnPacketSent follows:
 
 ~~~
- OnPacketSent(packet_number, ack_eliciting, in_flight,
-              is_crypto_packet, sent_bytes):
-   largest_sent_packet = packet_number
-   sent_packets[packet_number].packet_number = packet_number
-   sent_packets[packet_number].time_sent = now
-   sent_packets[packet_number].ack_eliciting = ack_eliciting
-   sent_packets[packet_number].in_flight = in_flight
+ OnPacketSent(packet_number, pn_space, ack_eliciting,

Using pn_space for the variable name and the type is confusing.  I think the enum should be kPacketNumberSpace

-- 
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/2417#pullrequestreview-200672923

---

From: Marten Seemann <[email-masked]>
Date: Wed, 06 Feb 2019 17:03:28 -0800

f45707fa254156ed571645ff83a8001a362e7357  kPacketNumberSpace

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2417/files/8280320ca7208e485af75d581f4a38d8d184a8a7..f45707fa254156ed571645ff83a8001a362e7357

---

From: Marten Seemann <[email-masked]>
Date: Wed, 06 Feb 2019 17:08:33 -0800

~~~

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/2417#discussion_r254515283

---

From: Kazuho Oku <[email-masked]>
Date: Wed, 06 Feb 2019 18:05:10 -0800

kazuho approved this pull request.

LGTM modulo the point below.

> @@ -819,6 +819,16 @@ kGranularity:
 kInitialRtt:
 : The RTT used before an RTT sample is taken. The RECOMMENDED value is 100ms.
 
+kPacketNumberSpace:
+: An enum to enumerate the three packet number spaces.
+~~~
+  enum kPacketNumberSpace {
+    Initial,
+    Handshake,
+    1-RTT,

I think the name for the PN space that governs 0- and 1-RTT packets is “application data”.

-- 
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/2417#pullrequestreview-200894627

---

From: Marten Seemann <[email-masked]>
Date: Wed, 06 Feb 2019 18:55:14 -0800

23720eb15b5abf9ceb28dcc2c2ed2c573c9a0ed7  1-RTT => Application data

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2417/files/f45707fa254156ed571645ff83a8001a362e7357..23720eb15b5abf9ceb28dcc2c2ed2c573c9a0ed7

---

From: Marten Seemann <[email-masked]>
Date: Wed, 06 Feb 2019 18:56:03 -0800

kInitialRtt:
 
+kPacketNumberSpace:

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/2417#discussion_r254532383

---

From: ianswett <[email-masked]>
Date: Thu, 07 Feb 2019 06:07:30 -0800

kInitialRtt:
 
+kPacketNumberSpace:
+    Application data,

It's an enum, so how about ApplicationData

OT: At some point there's a hope to make this pseudocode compile in Python.

-- 
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/2417#pullrequestreview-201103008

---

From: ianswett <[email-masked]>
Date: Thu, 07 Feb 2019 06:09:25 -0800

ianswett approved this pull request.

LG minus the ApplicationData comment.

-- 
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/2417#pullrequestreview-201103999

---

From: Marten Seemann <[email-masked]>
Date: Thu, 07 Feb 2019 15:23:21 +0000 (UTC)

9cd3dfef754d23334e54c779171fd13a266628d0  Application data => ApplicationData

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2417/files/23720eb15b5abf9ceb28dcc2c2ed2c573c9a0ed7..9cd3dfef754d23334e54c779171fd13a266628d0

---

From: Marten Seemann <[email-masked]>
Date: Thu, 07 Feb 2019 15:24:13 +0000 (UTC)

kInitialRtt:
 
+kPacketNumberSpace:

Python enums don't look particularly nice though: https://docs.python.org/3/library/enum.html#creating-an-enum

-- 
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/2417#discussion_r254728416

---

From: Marten Seemann <[email-masked]>
Date: Thu, 07 Feb 2019 07:24:57 -0800

kInitialRtt:
 
+kPacketNumberSpace:

I renamed the variable.

-- 
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/2417#discussion_r254728752

---

From: ianswett <[email-masked]>
Date: Thu, 07 Feb 2019 07:51:29 -0800

Good point, maybe we should pick a different language then.

On Thu, Feb 7, 2019 at 10:24 AM Marten Seemann <[email-masked]>
wrote:

> *@marten-seemann* commented on this pull request.
> ------------------------------
>
> In draft-ietf-quic-recovery.md
> <https://github.com/quicwg/base-drafts/pull/2417#discussion_r254728752>:
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/quicwg/base-drafts/pull/2417#discussion_r254728752>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ATJJcYQWGpEZU-wVBlGcVYQ9EhhelE9xks5vLEVJgaJpZM4aidmj>
> .
>

-- 
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/2417#issuecomment-461480665

---

From: Jana Iyengar <[email-masked]>
Date: Thu, 07 Feb 2019 08:24:57 -0800

janaiyengar approved this pull request.

A couple of nits, but looks good. Thanks for doing this work! Yes, we don't have ER for crypto packets, but that was always true, and this PR makes it more explicit, which is good.

> @@ -890,8 +901,9 @@ follows:
    min_rtt = infinite
+   for pn_space in [ Initial, Handshake, Applicaton data ]:

```suggestion
   for pn_space in [ Initial, Handshake, ApplicatonData ]:
```

>      loss_detection_timer.update(
-      time_of_last_sent_ack_eliciting_packet + timeout)
+      time_of_last_sent_crypto_packet + timeout)
+    return
+  if (loss_time != 0):

```suggestion

  if (loss_time != 0):
```

-- 
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/2417#pullrequestreview-201178586

---

From: Jana Iyengar <[email-masked]>
Date: Thu, 07 Feb 2019 16:26:57 +0000 (UTC)

@janaiyengar pushed 1 commit.

c9da9ee12a8e9a7d48ba8e3818ed0df5a7e16e28  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/2417/files/9cd3dfef754d23334e54c779171fd13a266628d0..c9da9ee12a8e9a7d48ba8e3818ed0df5a7e16e28

---

From: Jana Iyengar <[email-masked]>
Date: Thu, 07 Feb 2019 16:29:07 +0000 (UTC)

I've fixed the nits -- merging.

-- 
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/2417#issuecomment-461498867

---

From: Jana Iyengar <[email-masked]>
Date: Thu, 07 Feb 2019 08:29:15 -0800

Merged #2417 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/2417#event-2124575130