Re: [quicwg/base-drafts] Add an appendix related to packet number decoding. (#1493)

[Martin Thomson <[e-mail]>]

martinthomson commented on this pull request.

Thanks!  We've been meaning to do this for a while now.

> @@ -4778,6 +4780,36 @@ from 0xFF00 to 0xFFFF are reserved for Private Use {{!RFC8126}}.
 
 --- back
 
+# Sample code for packet number decoding {#sample-packet-number-decoding}
+
+The following pseudo-code shows how an implementation can decode packet
+numbers after packet protection has been removed.
+
+~~~
+rcv_nxt        = 0xaa82f30e + 1
+pktn           = 0x9b3
+pktn_nbits     = 14
+pktn_len       = 1 << pktn_nbits
+pktn_win       = pktn_len / 2          // 0x2000
+
+// The incoming packet number is between:
+//     [rcv_nxt - pktn_win ; rcv_nxt + pktn_win[

I would instead say the incoming packet number is greater than (rcv_next - pktn_win) and less than or equal to (rcv_next + pktn_win).  The use of '[' here implies a different interpretation, and it's not so obvious.

> @@ -4778,6 +4780,36 @@ from 0xFF00 to 0xFFFF are reserved for Private Use {{!RFC8126}}.
 
 --- back
 
+# Sample code for packet number decoding {#sample-packet-number-decoding}
+
+The following pseudo-code shows how an implementation can decode packet
+numbers after packet protection has been removed.
+
+~~~
+rcv_nxt        = 0xaa82f30e + 1

Maybe start with a function declaration:
InterpretPacketNumber(largest_received_pn, truncated_pn, truncated_pn_bits):

then you can include comments on each line that show the result of the step, as you do for pktn_win.

> +# Sample code for packet number decoding {#sample-packet-number-decoding}
+
+The following pseudo-code shows how an implementation can decode packet
+numbers after packet protection has been removed.
+
+~~~
+rcv_nxt        = 0xaa82f30e + 1
+pktn           = 0x9b3
+pktn_nbits     = 14
+pktn_len       = 1 << pktn_nbits
+pktn_win       = pktn_len / 2          // 0x2000
+
+// The incoming packet number is between:
+//     [rcv_nxt - pktn_win ; rcv_nxt + pktn_win[
+//
+// However, we can't just perform the exclusive-or of rcv_nxt

Exclusive-or might be misleading.  Also, we're concerned about being outside the window on both sides.

Maybe, "However, we can't just remove the trailing bits from rcv_nxt and add the truncated packet number because that might be outside the window."

> +rcv_nxt        = 0xaa82f30e + 1
+pktn           = 0x9b3
+pktn_nbits     = 14
+pktn_len       = 1 << pktn_nbits
+pktn_win       = pktn_len / 2          // 0x2000
+
+// The incoming packet number is between:
+//     [rcv_nxt - pktn_win ; rcv_nxt + pktn_win[
+//
+// However, we can't just perform the exclusive-or of rcv_nxt
+// (with the appropriate bits removed)  with pktn because that
+// would yield 0xaa82c9b3 which is lower than rcv_nxt - pktn_win.
+//
+// We need to find a packet number that fits the window,
+// so we add the window to the expected packet number and check
+// for overflow.

Here we find the top of the window and use that as the basis for calculating the value.  Then, the value can only be too large.

> +pktn           = 0x9b3
+pktn_nbits     = 14
+pktn_len       = 1 << pktn_nbits
+pktn_win       = pktn_len / 2          // 0x2000
+
+// The incoming packet number is between:
+//     [rcv_nxt - pktn_win ; rcv_nxt + pktn_win[
+//
+// However, we can't just perform the exclusive-or of rcv_nxt
+// (with the appropriate bits removed)  with pktn because that
+// would yield 0xaa82c9b3 which is lower than rcv_nxt - pktn_win.
+//
+// We need to find a packet number that fits the window,
+// so we add the window to the expected packet number and check
+// for overflow.
+rcv_nxt_masked  = (rcv_nxt + pktn_win) & ~(pktn_len - 1)

I would add a temporary for rcv_max = rcv_nxt + pktn_win so that you can a) explain it better, and b) use it in two places.

> +pktn_win       = pktn_len / 2          // 0x2000
+
+// The incoming packet number is between:
+//     [rcv_nxt - pktn_win ; rcv_nxt + pktn_win[
+//
+// However, we can't just perform the exclusive-or of rcv_nxt
+// (with the appropriate bits removed)  with pktn because that
+// would yield 0xaa82c9b3 which is lower than rcv_nxt - pktn_win.
+//
+// We need to find a packet number that fits the window,
+// so we add the window to the expected packet number and check
+// for overflow.
+rcv_nxt_masked  = (rcv_nxt + pktn_win) & ~(pktn_len - 1)
+decoded_pktn    = rcv_nxt_masked + pktn
+
+// Note that we also check for underflow.

"Note the extra check for underflow when the next packet number is near zero."

-- 
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/1493#pullrequestreview-132656969