[QUIC] Early Editorial Review Comment draft-hamilton-quic-transport-protocol-01

[Patrick McManus <[e-mail]>]

Ryan, Jana, et al - thanks for the draft updates! Its no fun taking running
code and turning it into an IETF document :) Hopefully this review helps in
that process.

I have read fairly thoroughly through draft-hamilton-quic-transport-protocol-01
and have some initial comments. These are largely, but not completely,
editorial in nature.I have also read the companion documents, though not
yet as closely. Its possible I'm saying something that is contradicted in
their details - though I think this reflects on how we choose to arrange
the document split.

0] *There is really very little talk about addressing.* In TCP, this was
obvious - the 5 tuple identifies the connection and binds the addressing
into the transport. Not much to say. But quic awesomely separates the
connection identifier without really talking about how addressing has to
work. There is a TODO about rebinding that is obviously part of this
(github issue!), but even the original binding is underspecified (e.g.
where is the server side of the client handshake addressed to?). Its also
not clear how the STK applies to this, and that goes into security
considerations etc, etc.. There are some intuitive answers to these
questions, but we ought to document.

1] *Draft Intended Status is listed as experimenta*l. What's up with that?
Standards Track I would hope, or I'm going home now :)

2] *Use of QUIC Receiver* - is that an endpoint, or is it meant to be
broader like an intermediary who reads QUIC? its not defined.. but because
when it talks about flow control it is really talking about sending I
suspect it is a endpoint.. so I would change it to endpoint

3] Re version field "*This mechanism eliminates roundtrip latency when the
client's optimistically-chosen version is spoken by the server, and
incentivizes servers to not lag behind clients in deployment of newer
versions*." - You could also argue that it dis-incentivizes clients from
upgrading to newer versions for fear of triggering fallback :)..
Personally, I would design it the same way as documented but rationalize it
differently (i.e. we expect the common case to be a version match and want
to enable client sends first) - the document doesn't imo really benefit
from including design rationale unless it helps the reader make a better
implementation. This is a fuzzy line and a general comment - I just wanted
to get it out there.. personally I would just remove the rationale and
leave the facts - do you really want to write about every design decision
(e.g. little endian!) and have to review the rationale text in addition to
the normative text?

4]* quic version* - what version does this document describe? I would say
its time to start putting tokens in. Getting this right early will really
help with rapid evolution. h2 used npn tokens which were kind of an
inifinite space, so we just put "draft" in all the names. One thing quic
might consider is just setting the high bit for all the pre-RFC version
numbers (and bumping it one on each new draft).. then we can clear it for
the final draft and have plenty of time to forget those implementations
ever existed.

*5] "PUBLIC_RESET packets that reset a connection are currently not
authenticated."*

the word currently is weird now that we're in the IETF process. The version
of the protocol described by this draft has a PUBLIC_RESET packet that is
not authenticated. period. The next draft might say something different -
and if you think it might the 'right' thing to do would be to open a github
design issue for tracking when the group gets that far.. I guess this maps
to the DISCUSS_AND_MODIFY annotations you've got now - which seems helpful.

*6] Frames MUST fit within a single QUIC packet and MUST NOT span a
QUIC packet boundary.*

I think you only need one part of this right? Otherwise you need to define
what a QUIC packet boundary is in some way that doesn't match what it seems
like it is.

*7] 4.2.2 doesn't really describe how to find the type very well*.. I think
you could make it a simple enumeration by saying that 0x80 -> 0xFF is
stream, 0x40 -> 0x7F is ack, and the others are 8 are constants < 0x40

*8] in 4.3 label "2nd supported version (32) supported"* seems like a typo
(double supported)

*9] 5.2 is really frustrating. In general, the transport+tls+http-binding
documents make it really hard to figure out what a handshake would look
like. The decomposition strategy of the documents is weakest here - and its
a very important part.  *I'm concerned this is going to be an interop
problem. It might be signficantly easier to document if we restricted quic
to TLS. I think it is something that should be opened as a design issue for
the WG to discuss. Its really weird for a transport document to declare the
handshake protocol out of scope and not even be able to include it cleanly
by reference.

*10] in 7 "A sender bundles one or more frames in a Regular QUIC packet. A
sender MAY bundle any set of frames in a packet. All QUIC packets MUST
contain a packet number and MAY contain one or more frames.. "*
This is kind of a mess. Frames only apply to regular packets right?
Specifically version negotiation packets have ambiguous lengths if they
don't just fill the quic packet - so we don't want to say All QUIC packets
MAY contain..

*11] *i*n 4.2 the first packet sent by an endpoint MUST have a packet
number of 1. but in 7 the first packet in both directions of the connection
MUST have a packet number of 0. *I don't know how to reconcile this.

*12] 6.2 seems to let the sender skip arbitrary packet numbers..* except
packet 0/1 is required so the language should be tightened perhaps?

Optimistically,
-Patrick