Subject: [quicwg/base-drafts] Alternative packet format presentation (#3573)

From: Martin Thomson <[email-masked]>
Date: Wed, 08 Apr 2020 18:50:37 -0700

This is not an editorial decision, this is a request for feedback on whether this form of presentation makes more sense.  So far we have mixed feedback.
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * Test for an alternative format
  * Some more formalism
  * Refinements
  * One fewer line
  * Follow own conventions
  * Refine format syntax more
  * Apply the new format throughout
  * Use length instead of len now that there is room

-- File Changes --

    M draft-ietf-quic-http.md (109)
    M draft-ietf-quic-invariants.md (75)
    M draft-ietf-quic-tls.md (98)
    M draft-ietf-quic-transport.md (699)

-- Patch Links --

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

---

From: ianswett <[email-masked]>
Date: Thu, 09 Apr 2020 06:58:28 -0700

@ianswett approved this pull request.

I'd prefer to differentiate Type from Stream Type in H3, but otherwise LGTM.

> @@ -1035,11 +1035,9 @@ integer at the start of the stream. The format and structure of data that
 follows this integer is determined by the stream type.
 
 ~~~~~~~~~~ drawing
- 0                   1                   2                   3
- 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-|                        Stream Type (i)                      ...
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+Unidirectional Stream Header {
+  Type (i),

```suggestion
  Stream Type (i),
```
To avoid changing the name or be confused with the 'Type' used in all the frames.

> @@ -1124,13 +1122,10 @@ Only servers can push; if a server receives a client-initiated push stream, this
 MUST be treated as a connection error of type H3_STREAM_CREATION_ERROR.
 
 ~~~~~~~~~~ drawing
- 0                   1                   2                   3
- 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-|                           0x01 (i)                          ...
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-|                          Push ID (i)                        ...
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+Push Stream Header {
+  Type (i) = 0x01,

```suggestion
  Stream Type (i) = 0x01,
```

-- 
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/3573#pullrequestreview-390813431

---

From: Mike Bishop <[email-masked]>
Date: Thu, 09 Apr 2020 07:00:14 -0700

@MikeBishop commented on this pull request.

But...!  But...!  *Muh ASCII art!*

More seriously, this is probably a net improvement for readability.  There's value in conforming to existing paradigms where possible, but we'd already had to define custom extensions to the notation from RFC2360, so this is probably no worse.

I'm really not a fan of the trailing comma in the lists, but that's neither here nor there.

> +  Protected Payload (0..24), # Skipped
+  Protected Payload (128),   # Sampled

```suggestion
  Protected Payload (0..24), # Skipped Part
  Protected Payload (128),   # Sampled Part
```
Inconsistent with the below.  Resolve either direction, but I don't see a reason to make them different.

> +  Protected Payload (128),   # Sampled
+  Protected Payload (..)     # Remainder
+}
+
+Short Header Packet {
+  Header Form (1) = 0,
+  Fixed Bit (1) = 1,
+  Spin Bit (1),
+  Reserved Bits (2),         # Protected
+  Key Phase (1),             # Protected
+  Packet Number Length (2),  # Protected
+  Destination Connection ID (0..160),
+  Packet Number (8..32),     # Protected
+  Protected Payload (0..24), # Skipped Part
+  Protected Payload (128),   # Sampled Part
+  Protected Payload (..)     # Remainder

```suggestion
  Protected Payload (..),    # Remainder
```
I'm not really a fan of the trailing commas, but for consistency sake, you missed this one.

> +  a mimumum of zero bits and B can be omitted to indicate no set upper limit;
+  values always end on an octet boundary

```suggestion
  a mimumum of zero bits and B can be omitted to indicate no set upper limit;
  values in this format always end on an octet boundary
```
Just to be clear, since not all values end on an octet boundary.  I do think separate sentences would be more readable, but then it would be inconsistent with the others.

> +
+x (?) = C:
+: Indicates that x has a fixed value of C
+
+x (?) = C..D:
+: Indicates that x has a value in the range from C to D, inclusive
+
+\[x (E)\]:
+: Indicates that x is optional (and has length of E)
+
+x (E) ...:
+: Indicates that x is repeated zero or more times (and that each instance is
+  length E)
+
+By convention, individual fields reference a complex field by using the name of
+that the complex field.

```suggestion
the complex field.
```

> +  Variable-Length Field (8..24),
+  Field With Minimum Length (16..),

```suggestion
  Arbitrary-Length Field (..),
  Variable-Length Field (8..24),
  Field With Minimum Length (16..),
```

Omitting both ends is used in the document and consistent with the definition, but not clearly spelled out or used in an example.

> @@ -3926,11 +3938,11 @@ DCID Len:
 
 Destination Connection ID:
 
-: The Destination Connection ID field follows the DCID Len and is between 0 and
-  20 bytes in length. {{negotiating-connection-ids}} describes the use of this
-  field in more detail.
+: The Destination Connection ID field follows the DCID Length and is between 0

```suggestion
: The Destination Connection ID field follows the DCID Length field and is between 0
```

> @@ -3942,7 +3954,7 @@ SCID Len:
 
 Source Connection ID:
 
-: The Source Connection ID field follows the SCID Len and is between 0 and 20
+: The Source Connection ID field follows the SCID Length and is between 0 and 20

```suggestion
: The Source Connection ID field follows the SCID Length field and is between 0 and 20
```

-|                     Packet Number (8/16/24/32)              ...
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-|                     Protected Payload (*)                   ...
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+Short Header Packet {
+  Header Form (1) = 0,
+  Fixed Bit (1) = 1,
+  Spin Bit (1),
+  Reserved Bits (2),
+  Key Phase (1),
+  Packet Number Length (2),
+  Destination Connection ID (0..160),
+  Packet Number (8..32),
+  Packet Payload (..),
+}
 ~~~~~
 {: #fig-short-header title="Short Header Packet Format"}

GitHub won't let me put a comment down there (too far from a change), but you can remove the (S), (R), (K), (P) notations from the field definitions, since they're not needed to correlate with the diagram.

> @@ -5354,13 +5257,11 @@ with error STREAM_STATE_ERROR.
 The MAX_STREAM_DATA frame is shown in {{fig-max-stream-data}}.
 
 ~~~
- 0                   1                   2                   3
- 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-|                        Stream ID (i)                        ...
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-|                    Maximum Stream Data (i)                  ...
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+MAX_STREAM_DATA Frame {
+  Type (i) = 0x11,
+  Stream ID (i),
+  Maximum Data (i),

Orthogonal to this PR, but perhaps we should align the "Maximum Data" and "Data Limit" terms which reference the same limit in different frames?

-- 
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/3573#pullrequestreview-390801974

---

From: Rich Salz <[email-masked]>
Date: Thu, 09 Apr 2020 08:20:12 -0700

Did you consider TLS presentation language?  It would be nice to not have n+1 formats.

-- 
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/3573#issuecomment-611586596

---

From: Mike Bishop <[email-masked]>
Date: Thu, 09 Apr 2020 08:40:15 -0700

@richsalz, TLS presentation language implies length prefixes in a _lot_ of places.  Plus we already ripped out TLS presentation language for definition of the transport parameters.  Bringing it back for definition of packet layouts would be... a bit weird.

-- 
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/3573#issuecomment-611597283

---

From: Rich Salz <[email-masked]>
Date: Thu, 09 Apr 2020 08:44:09 -0700

As long as it was considered and found to be unsuitable, that's fine with me.

-- 
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/3573#issuecomment-611599329

---

From: Jana Iyengar <[email-masked]>
Date: Thu, 09 Apr 2020 19:35:00 -0700

@janaiyengar approved this pull request.

I really like this format.

-\[x\]:
-: Indicates that x is optional
+Complex fields are named and then followed by a list of fields surrounded by a
+pair of matching braces. Each field in this list is separated by commas.
+
+Individual fields include length information, plus indications about fixed
+value, optionality, or repetitions. Individual fields use the following
+notational conventions:

```suggestion
notational conventions, with all lengths in bits:
```

-- 
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/3573#pullrequestreview-391234740

---

From: Martin Thomson <[email-masked]>
Date: Mon, 13 Apr 2020 17:48:25 -0700

@martinthomson pushed 1 commit.

7d096198ad50b812645c4ea7eaf27fdcf7c2183e  Batch of code review suggestions

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3573/files/f0e90441c9c73b22a29b5b12b2816c4d76f48fbb..7d096198ad50b812645c4ea7eaf27fdcf7c2183e

---

From: Martin Thomson <[email-masked]>
Date: Mon, 13 Apr 2020 17:52:11 -0700

@martinthomson pushed 2 commits.

953d93e0feb839717c2881b7e901011af3f54ca6  Rewrap
93f3cd4461ca858bd7943f0bf6b10779c3384166  Remove parenthesized letters from bit notation

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3573/files/7d096198ad50b812645c4ea7eaf27fdcf7c2183e..93f3cd4461ca858bd7943f0bf6b10779c3384166

---

From: ianswett <[email-masked]>
Date: Mon, 13 Apr 2020 18:34:45 -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/3573#pullrequestreview-392545467

---

From: Lucas Pardue <[email-masked]>
Date: Wed, 15 Apr 2020 11:00:37 -0700

@LPardue requested changes on this pull request.

Overall I like this. The terser presentation allows you to define the full H3 frame, which I really like. Strictly, those figures now present  the whole frame so I've made the suggestion to drop the `Payload` qualifier.

Also see comment about the 0 or more SETTINGS parameters, which needs a fixup.

> @@ -1229,11 +1220,11 @@ frame is received on a control stream, the recipient MUST respond with a
 connection error ({{errors}}) of type H3_FRAME_UNEXPECTED.
 
 ~~~~~~~~~~ drawing
- 0                   1                   2                   3
- 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-|                         Payload (*)                         ...
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+DATA Frame {
+  Type (i) = 0x0,
+  Length (i),
+  Data (..),
+}
 ~~~~~~~~~~
 {: #fig-data title="DATA Frame Payload"}

```suggestion
{: #fig-data title="DATA Frame"}
```

> @@ -1243,11 +1234,11 @@ The HEADERS frame (type=0x1) is used to carry an HTTP field section, encoded
 using QPACK. See [QPACK] for more details.
 
 ~~~~~~~~~~  drawing
- 0                   1                   2                   3
- 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-|                   Encoded Field Section (*)                 ...
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+HEADERS Frame {
+  Type (i) = 0x1,
+  Length (i),
+  Encoded Field Section (..),
+}
 ~~~~~~~~~~
 {: #fig-headers title="HEADERS Frame Payload"}

```suggestion
{: #fig-headers title="HEADERS Frame"}
```

> @@ -1284,11 +1275,11 @@ frame on a stream other than the control stream MUST be treated as a connection
 error of type H3_FRAME_UNEXPECTED.
 
 ~~~~~~~~~~  drawing
- 0                   1                   2                   3
- 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-|                          Push ID (i)                        ...
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+CANCEL_PUSH Frame {
+  Type (i) = 0x3,
+  Length (i),
+  Push ID (..),
+}
 ~~~~~~~~~~
 {: #fig-cancel-push title="CANCEL_PUSH Frame Payload"}

```suggestion
{: #fig-cancel-push title="CANCEL_PUSH Frame"}
```

> +SETTINGS Frame {
+  Type (i) = 0x4,
+  Length (i),
+  Identifier (i),
+  Value (i),
+}

This is wrong, sorry. The text discusses that the frame contains 0 or more parameters and the old drawing presents the format of the parameter, not the frame. I think what you want to do is present  the pair of (Identifier, Value) with cardinality, and then update the fig title to sat "SETTINGS Frame".

> @@ -1436,13 +1426,12 @@ The PUSH_PROMISE frame (type=0x5) is used to carry a promised request header
 field section from server to client on a request stream, as in HTTP/2.
 
 ~~~~~~~~~~  drawing
- 0                   1                   2                   3
- 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-|                          Push ID (i)                        ...
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-|                   Encoded Field Section (*)                 ...
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+PUSH_PROMISE Frame {
+  Type (i) = 0x5,
+  Length (i),
+  Push ID (i),
+  Encoded Field Section (..),
+}
 ~~~~~~~~~~
 {: #fig-push-promise title="PUSH_PROMISE Frame Payload"}

```suggestion
{: #fig-push-promise title="PUSH_PROMISE Frame"}
```

> @@ -1496,11 +1485,11 @@ requests and pushes.  This enables administrative actions, like server
 maintenance.  GOAWAY by itself does not close a connection.
 
 ~~~~~~~~~~  drawing
- 0                   1                   2                   3
- 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-|                  Stream ID/Push ID (i)                      ...
-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+GOAWAY Frame {
+  Type (i) = 0x7,
+  Length (i),
+  Stream ID/Push ID (..),
+}
 ~~~~~~~~~~
 {: #fig-goaway title="GOAWAY Frame Payload"}

```suggestion
{: #fig-goaway title="GOAWAY Frame"}
```

-- 
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/3573#pullrequestreview-394000888

---

From: Martin Thomson <[email-masked]>
Date: Wed, 15 Apr 2020 16:22:31 -0700

82bcaff60bf2d6725da5d1c1f628fb510ebdae7f  Notpayload

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3573/files/4c0abcff58fa163d3a506c592a89f6e8b41789d0..82bcaff60bf2d6725da5d1c1f628fb510ebdae7f

---

From: Martin Thomson <[email-masked]>
Date: Wed, 15 Apr 2020 16:24:51 -0700

f746d7f956d321e1fdb5decae711bd3360a6ac46  Define the SETTINGS frame properly

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3573/files/82bcaff60bf2d6725da5d1c1f628fb510ebdae7f..f746d7f956d321e1fdb5decae711bd3360a6ac46

---

From: Lucas Pardue <[email-masked]>
Date: Wed, 15 Apr 2020 16:30:11 -0700

@LPardue 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/3573#pullrequestreview-394202843

---

From: ekr <[email-masked]>
Date: Fri, 17 Apr 2020 13:01:34 -0700

I definitely like this general structure better. 

As far as the question of whether we should import the TLS language, you would need some extensions, but I suspect that you could get pretty far with the following three:

uintX == unsigned integer of size X
uintv == varint
opaque x[lower..upper] == opaque vector without a header (TLS uses <> to mean that it has a header) and opaque x[y..] means "to end of the container and minimum size y]

I believe we've already done uintv for MLS and @bifurcation has been thinking about a generalized an a "TLS spec language" 

-- 
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/3573#issuecomment-615438704

---

From: Martin Thomson <[email-masked]>
Date: Sun, 19 Apr 2020 22:24:19 -0700

4d4701548ea1d9f18140e89a1114c003b4af79ed  Consistent naming for _BLOCKED fields
e05726eb06f6ea4b94991b71305cfd6c4feff9e5  Merge pull request #3584 from quicwg/blocked-frame-limit-names

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

---

From: Martin Thomson <[email-masked]>
Date: Tue, 21 Apr 2020 15:07:50 -0700

Merged #3573 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/3573#event-3258082412