Subject: Re: [quicwg/base-drafts] Transport spec diagrams for #54 (#103)

From: Martin Thomson <[email-masked]>
Date: Mon, 02 Jan 2017 22:24:45 -0800

martinthomson approved this pull request.

>  * Packet Number: The lower 8, 16, 32, or 48 bits of the packet number, based on
   the PACKET_NUMBER_SIZE flag.  Each Regular packet is assigned a packet number
   by the sender.  The first packet sent by an endpoint MUST have a packet number
   of 1.
 
-* AEAD Data: A Regular packet's header, which includes the Common Header, and
-  the Version, Diversification Nonce, and Packet Number fields, is authenticated
+* AEAD Data: A Regular packet's Common Header and fields are authenticated,

Reminder to me: this statement is incorrect.  The AAD is empty.

-* Frame Type: The Frame Type byte is an 8-bit value containing various flags.
-  This byte is formatted as the following 8 bits: 01NULLMM.
+* The `N` bit indicates whether the frame has more than 1 ack range (i.e.

nit: comma after "i.e."

-  * Delta Largest Observed (opt, repeated): (Same as above.)
+* Delta Largest Acked 1-N (opt, repeated): (Same as above.)

Maybe use an ellipsis here rather than the hyphen, which my addled brain interpreted as minus.

-The WINDOW_UPDATE frame informs the peer of an increase in an endpoint's flow
-control receive window.  The StreamID can be zero, indicating this WINDOW_UPDATE
-applies to the connection level flow control window, or non-zero, indicating
-that the specified stream should increase its flow control window.  The frame is
-as follows:
+The WINDOW_UPDATE frame (type=0x04) informs the peer of an increase in an 
+endpoint's flow control receive window. The Stream ID can be zero, indicating 
+this WINDOW_UPDATE applies to the connection level flow control window, or 
+non-zero, indicating that the specified stream should increase its flow control 
+window. The frame is as follows: 

You need to watch for trailing whitespace here.

-## PING frame
+## PING frame {#frame-ping}
 
 Endpoints can use PING frames to verify that their peers are still alive or to

(type=0x...)

 ~~~
-   +---------------------------------------------------+
-   | Type (8) | Least unacked delta (8, 16, 32, or 48) |
-   +---------------------------------------------------+
+ 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
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+|               Least Unacked Delta (8/16/32/24)              ...

This is not a faithful transcription.  s/24/48

-- 
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/103#pullrequestreview-14886730

---

From: Mike Bishop <[email-masked]>
Date: Tue, 03 Jan 2017 13:41:51 -0800

@MikeBishop pushed 1 commit.

e905622  Martin's feedback

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/103/files/0e4d537d8ac7dd8483b4891137900a6403dc143a..e9056225cf421b7d355ae096333ddebb0c823f7a

---

From: janaiyengar <[email-masked]>
Date: Tue, 03 Jan 2017 18:51:30 -0800

janaiyengar commented on this pull request.

I don't think that it's accurate to say that the Regular packet's Common Header and fields are authenticated, since the "fields" contain frames and those are encrypted. Perhaps we should start calling them Regular packet header and payload, so that the Common Header and the Regular packet's Header are authenticated, and the Regular packet's payload is encrypted?

-- 
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/103

---

From: Martin Thomson <[email-masked]>
Date: Tue, 03 Jan 2017 18:56:47 -0800

martinthomson commented on this pull request.

I think that we can merge this absent any fix here.  It's a minor point that needs to be corrected, but this PR doesn't need to fix problems it didn't create :)

---

From: janaiyengar <[email-masked]>
Date: Tue, 03 Jan 2017 22:19:32 -0800

> @@ -374,27 +398,18 @@ Check the flags in the common header
 
 ## Regular Packets
 
-Each Regular packet's header consists of a Common Header followed by fields
-specific to Regular packets, as shown below:
+Each Regular packet's payload consists of fields shown below:

I would change this to: "Each Regular packet contains additional header fields followed by payload, as shown below:"

-- 
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/103#pullrequestreview-15061021

---

From: janaiyengar <[email-masked]>
Date: Tue, 03 Jan 2017 22:20:25 -0800

In addition to the suggested change above, I would change this to: "A Regular packet's header fields, including the Common Header, are authenticated but not encrypted."

-- 
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/103#pullrequestreview-15061077

---

From: janaiyengar <[email-masked]>
Date: Tue, 03 Jan 2017 22:20:56 -0800

This PR introduces it. The text currently is quite clear about what's authenticated and what's encrypted. This new text reads better, but confuses that point. I've suggested some mods.

---

From: janaiyengar <[email-masked]>
Date: Tue, 03 Jan 2017 22:25:52 -0800

-  * The 'SS' bits encode the length of the Stream ID header field as 8, 16, 24,
-    or 32 bits.  (DISCUSS: Consider making this 8, 16, 32, 64.)
+The STREAM frame payload contains the following fields:

I don't think it makes sense to call this the payload. Why not just say "In addition to the Type byte, the STREAM frame contains the following fields"?

-- 
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/103#pullrequestreview-15061405

---

From: janaiyengar <[email-masked]>
Date: Tue, 03 Jan 2017 22:34:52 -0800

> @@ -943,47 +940,104 @@ The fields in the ACK frame are as follows:
 * Ack Delay: Time from when the largest acked, as indicated in the Largest Acked
   field, was received by this peer to when this ack was sent.
 
-* Ack Block Section:
+* Ack Block Section: Contains one or more blocks of packet numbers which have
+  been successfully received.  See {{ack-block-section}}.
+  
+* Timestamp Section: Contains zero or more timestamps reporting transit delay of

"transit delay" --> "receive times"

-- 
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/103#pullrequestreview-15061911

---

From: janaiyengar <[email-masked]>
Date: Tue, 03 Jan 2017 22:44:47 -0800

-  * Ack Block Length (opt, repeated): An unsigned packet number delta that
-    indicates the number of contiguous packets being acked starting after the
-    end of the previous gap.  Along with the previous field, this field is
-    repeated "Num Blocks" times.
+### Timestamp Section {#timestamp-section}
+
+The Timestamp Section contains between zero and 255 measurements of packet
+delay. This information can be used by a sender to better estimate the RTT of
+the connection.

The timestamps are simply packet receive times as observed by the receiver, and are not packet transit times. How about something like: "... of packet receive time deltas. This information may be used by a sender to better estimate the RTT of the connection."

-- 
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/103#pullrequestreview-15062585

---

From: janaiyengar <[email-masked]>
Date: Tue, 03 Jan 2017 22:52:16 -0800

-The fields in the BLOCKED frame are as follows:
-
-* Frame Type: The Frame Type byte is an 8-bit value that must be set to 0x05
-  indicating that this is a BLOCKED frame.
+The BLOCKED frame payload contains a single field:

I would avoid saying "payload" here -- the BLOCKED frame has no payload really.

-- 
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/103#pullrequestreview-15063044

---

From: janaiyengar <[email-masked]>
Date: Tue, 03 Jan 2017 22:53:54 -0800

-~~~
-   +--------+
-   |  0x00  |
-   +--------+
-~~~
+The PADDING frame pads a packet with 0x00 bytes. When this frame is encountered, 

For parity with other frames, perhaps add "(type = 0x00)" here

-- 
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/103#pullrequestreview-15063144

---

From: Mike Bishop <[email-masked]>
Date: Wed, 04 Jan 2017 10:10:18 -0800

MikeBishop commented on this pull request.

Well, one of the (authenticated) fields contains ciphertext, which then contains frames....  I think that's a separate terminology issue than Martin's comment, isn't it?

---

From: Mike Bishop <[email-masked]>
Date: Wed, 04 Jan 2017 10:14:11 -0800

We can phrase it either way, but I was trying to get a consistent usage that for both packets and frames, there's a type-independent envelope (which for frames is really thin) and a type-dependent payload.

---

From: Mike Bishop <[email-masked]>
Date: Wed, 04 Jan 2017 10:18:56 -0800

Same comment -- let's agree on terminology first.  Per the description at line 490, this is a payload.  If we want a different term, let's sync on that first.

---

From: janaiyengar <[email-masked]>
Date: Wed, 04 Jan 2017 17:36:23 -0800

Apologies, yes, this is a separate issue from Martin's, but it was on the same proposed text change, so I tagged along.

---

From: janaiyengar <[email-masked]>
Date: Wed, 04 Jan 2017 17:39:23 -0800

I think it is a bit confusing to call the fields in the ACK frame the payload of the ACK frame, for instance. I think of it as every frame has a Type field and the rest of the frame is type-dependent fields. STREAM frames have a payload, but that's stream data.

---

From: janaiyengar <[email-masked]>
Date: Wed, 04 Jan 2017 17:46:08 -0800

Now that I understand what you were going for, I think the use of "payload" is perfectly accurate, but adds an unnecessary level of abstraction: we'll end up referring to "fields in the BLOCKED frame" anyways. In addition, we lose the ability to use "payload" for stream data that's carried within a STREAM frame. I'd prefer that we simply called it the frame's fields instead of the frame's payload.

---

From: Mike Bishop <[email-masked]>
Date: Thu, 05 Jan 2017 14:36:39 -0800

Notes from discussion with Jana.

+~~~
+ 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
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+|[Num Blocks(8)]|      First Ack Block Length (8/16/32/48)    ...
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+|  [Gap 1 (8)]  |       [Ack Block 1 Length (8/16/32/48)]     ...
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+|  [Gap 2 (8)]  |       [Ack Block 2 Length (8/16/32/48)]     ...
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+                             ...
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+|  [Gap N (8)]  |       [Ack Block N Length (8/16/32/48)]     ...
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+~~~

Per Jana:  Move numbers from each section into the fixed structure above.

-  * Time Since Previous Timestamp (opt, repeated): An optional 16-bit unsigned
-    value specifying time delta from the previous reported timestamp.  It is
-    encoded in the same format as the Ack Delay.  Along with the previous field,
-    this field is repeated "Num Timestamps" times.
+* First Timestamp (opt): An optional 32-bit unsigned value specifying the time

Inconsistent use - First Timestamp vs. Time Since Largest Acked.  Per discussion with Jana, this doesn't need to still be 32 bits; @janaiyengar will file issue to trim it down.

-- 
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/103#pullrequestreview-15408111

---

From: Martin Thomson <[email-masked]>
Date: Thu, 05 Jan 2017 17:45:42 -0800

> A Regular packet's header fields, including the Common Header, are authenticated but not encrypted.

That would be incorrect.

---

From: Martin Thomson <[email-masked]>
Date: Thu, 05 Jan 2017 17:52:13 -0800

I should expand on that a little:

Regular (ugh, that name has to change) packets aren't always protected.  When they are, the header fields are not protected at all.  However, I believe that the values that are present do not require protection.  Outside of 0-RTT, the only fields that need to be protected are the flags field, the connection ID and the packet number.

Connection ID is easy not to authenticate, it just tells you which key to use.  Change it and you are guaranteed (or at least with astonishingly high probability) to get the wrong keys.

Packet number is input to the nonce, so that's covered.

Of the flags, the only relevant one is KEY_PHASE, which tells the receiver which key to use (see connection ID).  The packet number size bits are subsumed by the packet number.  I have no idea what the multipath bit might do.  Public reset and version don't appear on regular packets with one exception.  Version does appear on 0-RTT packets, but it serves the same purpose as KEY_PHASE there.

---

From: janaiyengar <[email-masked]>
Date: Thu, 05 Jan 2017 18:59:02 -0800

+~~~
+ 0                   1                   2                   3

I wouldn't do this just yet. I'd leave this PR to not do major changes to the ACK format, and leave that for a separate PR that reworks the ack format entirely.

---

From: Martin Thomson <[email-masked]>
Date: Thu, 05 Jan 2017 19:11:07 -0800

That makes sense to me.  Then we can talk about "transport protocol payload" and "STREAM frame payload" and we will be talking about the same thing.

---

From: Mike Bishop <[email-masked]>
Date: Fri, 06 Jan 2017 20:54:28 -0800

@MikeBishop pushed 2 commits.

2e567f4  Merge remote-tracking branch 'origin/master' into diagrams
5c8221b  Payload => Type-dependent fields

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/103/files/e9056225cf421b7d355ae096333ddebb0c823f7a..5c8221ba182b34f274a5563657539554dfe2eed9

---

From: Mike Bishop <[email-masked]>
Date: Fri, 06 Jan 2017 21:03:04 -0800

+~~~
+ 0                   1                   2                   3

@janaiyengar, I think your comment is for a different change. My other note said you'd file an issue for reducing the size of the `Time Since Largest Acked` field to match the others, and I agree that should be saved for a different PR.  This is a purely editorial change which you suggested, putting the counts up in the top section.  If you no longer want that, we can revert it.

---

From: Mike Bishop <[email-masked]>
Date: Fri, 06 Jan 2017 21:08:10 -0800

+~~~
+ 0                   1                   2                   3

...but I also had a note that I had an inconsistency between `Time Since Largest Acked` and `First Timestamp`.  The description of First Timestamp gives it as a time since the beginning of the connection, which makes more sense to be larger, and the remainders are deltas from that value.

---

From: Mike Bishop <[email-masked]>
Date: Fri, 06 Jan 2017 21:12:28 -0800

f192883  Move ACK frame block counts up a section

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/103/files/5c8221ba182b34f274a5563657539554dfe2eed9..f19288301d80e417d83f8b01335ecd56e23ee5d1

---

From: Mike Bishop <[email-masked]>
Date: Fri, 06 Jan 2017 21:15:26 -0800

At this point, I think I've incorporated Jana's feedback and it addresses #54.  @janaiyengar, @martinthomson, @larseggert, please confirm.

-- 
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/103#issuecomment-271063891

---

From: Mike Bishop <[email-masked]>
Date: Sun, 08 Jan 2017 16:19:47 -0800

@MikeBishop pushed 9 commits.

2c99f61  Don't go so wide
dabd66b  De-indent a little
86ab2c6  Take and improve suggestion on regular packet intro
6de9527  Leave definition of AEAD Data to QUIC-TLS
633ea6e  The stream frame just contains fields
aa9c908  Timestamps are packet receive times
1c96214  As with STREAM, so with BLOCKED: no payload
60028fe  Merge branch 'diagrams' into diagrams_mt
6104a58  Merge pull request #129 from quicwg/diagrams_mt

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/103/files/f19288301d80e417d83f8b01335ecd56e23ee5d1..6104a58f4e9674d4cd62d8bd9721c772e9d0ff6f

---

From: Martin Thomson <[email-masked]>
Date: Mon, 09 Jan 2017 20:41:46 -0800

@janaiyengar, I'd really like to merge this.  Is there any reason I shouldn't?

-- 
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/103#issuecomment-271484979

---

From: ianswett <[email-masked]>
Date: Thu, 12 Jan 2017 10:56:34 -0800

ianswett approved this pull request.

I think this looks really good.  One nit.

If payload means application data of some sort(or a transformed version thereof), which I think is the intent, then I'd agree with Jana, using payload here is confusing.

SGTM.

> -    delta in microseconds, from the beginning of the connection to the arrival
-    of this packet.
+* Gap To Next Block (opt, repeated): An unsigned number specifying the number
+  of contiguous missing packets from the end of the previous ack block to the
+  start of the next.
+
+* Ack Block Length (opt, repeated): An unsigned packet number delta that
+  indicates the number of contiguous packets being acked starting after the
+  end of the previous gap.  Along with the previous field, this field is
+  repeated "Num Blocks" times.
+
+### Timestamp Section {#timestamp-section}
+
+The Timestamp Section contains between zero and 255 measurements of packet
+receive times. This information can be used by a sender to better estimate the
+RTT of the connection.

I would leave the second sentence out, because I don't think improving RTT estimates are the most likely use case.

I would suggest adding "relative to the beginning of the connection." to the end of the first sentence to clarify these aren't an effort to convey a true time.

-- 
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/103#pullrequestreview-15927257

---

From: Mike Bishop <[email-masked]>
Date: Thu, 12 Jan 2017 11:19:09 -0800

6d0d6f7  Missed a 'payload'
d9d5eaf  Ian's feedback

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/103/files/6104a58f4e9674d4cd62d8bd9721c772e9d0ff6f..d9d5eaf8a1b096cdcb47229cc36cd96656e2a1de

---

From: Mike Bishop <[email-masked]>
Date: Thu, 12 Jan 2017 11:24:08 -0800

3cc43bc  Merge branch 'master' into diagrams

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/103/files/d9d5eaf8a1b096cdcb47229cc36cd96656e2a1de..3cc43bc9b3d0471cc24cdf8e5737b0ee9585e473

---

From: janaiyengar <[email-masked]>
Date: Fri, 13 Jan 2017 22:17:34 -0800

LGTM -- looks great, ship it!

-- 
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/103#issuecomment-272605370

---

From: janaiyengar <[email-masked]>
Date: Fri, 13 Jan 2017 22:27:16 -0800

Merged #103.

-- 
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/103#event-923124732