Subject: Re: [quicwg/base-drafts] Packet number encoding appendix (#4315)

From: Kazuho Oku <[email-masked]>
Date: Tue, 03 Nov 2020 15:14:52 -0800

@kazuho commented on this pull request.

> @@ -4382,17 +4382,10 @@ the difference between the largest acknowledged packet and packet number being
 sent.  A peer receiving the packet will then correctly decode the packet number,
 unless the packet is delayed in transit such that it arrives after many
 higher-numbered packets have been received.  An endpoint SHOULD use a large
-enough packet number encoding to allow the packet number to be recovered even
-if the packet arrives after packets that are sent afterwards.
-
-As a result, the size of the packet number encoding is at least one bit more
-than the base-2 logarithm of the number of contiguous unacknowledged packet
-numbers, including the new packet.

I think my point is that it would be more helpful to the readers to have this paragraph here, than just having the super-abstract guidance stated in the paragraph above. I tend to think that the specification should be parsable without reading the pseudo-code.

That said, this is a discussion about where the practical guidance should be stated, and therefore my preference is not a strong at all.

-- 
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/4315#discussion_r517010456

---

From: Martin Thomson <[email-masked]>
Date: Tue, 03 Nov 2020 15:19:57 -0800

@martinthomson commented on this pull request.

> +~~~
+EncodePacketNumber(full_pn, largest_acked):
+
+  // The number of bits must be at least one more
+  // than the base-2 logarithm of the number of contiguous
+  // unacknowledged packet numbers, including the new packet.
+  if largest_acked is None:
+    num_unacked = full_pn + 1
+  else:
+    num_unacked = full_pn - largest_acked
+
+  min_bits = log(num_unacked, 2) + 1
+  num_bytes = ceil(min_bits / 8)
+
+  // Return the value after truncating the full packet number
+  // to num_bytes least-significant bytes

```suggestion
  // to num_bytes least-significant bytes.
```

> +EncodePacketNumber(full_pn, largest_acked):
+
+  // The number of bits must be at least one more
+  // than the base-2 logarithm of the number of contiguous
+  // unacknowledged packet numbers, including the new packet.
+  if largest_acked is None:
+    num_unacked = full_pn + 1
+  else:
+    num_unacked = full_pn - largest_acked
+
+  min_bits = log(num_unacked, 2) + 1
+  num_bytes = ceil(min_bits / 8)
+
+  // Return the value after truncating the full packet number
+  // to num_bytes least-significant bytes
+  return truncate(full_pn, num_bytes)

```suggestion
  return encode(full_pn, num_bytes)
```

> +  // Return the value after truncating the full packet number
+  // to num_bytes least-significant bytes

```suggestion
  // Encode the integer value and truncating to the 
  // num_bytes least-significant bytes.
```

-- 
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/4315#pullrequestreview-522943291

---

From: Mike Bishop <[email-masked]>
Date: Tue, 03 Nov 2020 15:21:40 -0800

@MikeBishop pushed 1 commit.

e2f4433901ab807d1412729d52ad2f830b33d625  Truncatencode

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4315/files/9608dc4f7ee751fd906876d76992dea6ef03b493..e2f4433901ab807d1412729d52ad2f830b33d625

---

From: Mike Bishop <[email-masked]>
Date: Tue, 03 Nov 2020 15:28:26 -0800

8cd6bc1e70dce9e0e089d50dcdca30a6c16ce4c7  Restore the comment in the original text

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4315/files/e2f4433901ab807d1412729d52ad2f830b33d625..8cd6bc1e70dce9e0e089d50dcdca30a6c16ce4c7

---

From: Martin Thomson <[email-masked]>
Date: Tue, 03 Nov 2020 15:30:47 -0800

Merged #4315 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/4315#event-3955196809

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 03 Nov 2020 15:30:52 -0800

@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/4315#pullrequestreview-522948118