WIP: High-level restructure of transport draft #1857

janaiyengar · 2018-10-13T06:10:34Z

Bringing draft up to proposed structure in #411. We'll need to do more moving around of things and adding glue to various places to make sure the text flows freely and correctly, but I believe this PR moves major sections into the right slots (mostly).

It's easier to see the final draft, here.

martinthomson

This is a great start. Thanks for putting this together.

I took the opportunity to do a fairly thorough read-through (up until I got to the frame descriptions). So there are a mix of comments throughout. I probably strayed over the line on occasion and made some non-editorial comments as I went; as we discuss, we can open issues for those.

Overall, this is a big improvement, and I can see how it might be the basis for more improvements (some of which I noted). We shouldn't worry about getting things perfect, but at the same time I don't want to lose these comments. Most of them are fairly directly actionable.

No more time today, but I might be able to generate a PR for some of this as things stabilize more.

martinthomson · 2018-10-15T03:31:10Z

draft-ietf-quic-transport.md

-wire format, and mechanisms of the QUIC protocol for connection establishment,
-stream multiplexing, stream and connection-level flow control, connection
-migration, and data reliability.
+This document describes the core QUIC protocol, and is structured as follows:


Does a two-level list make sense here?

* Streams are the basic service abstraction that QUIC provides: ** {{streams}} describes core concepts related to streams, ** {{stream-states}} provides a reference model for stream states, and ** {{flow-control}} outlines the operation of flow control. * Connections are the context in which QUIC endpoints communicate. ** {{}} details the process for establishing connections, ** {{}} describes how endpoints migrate a connection to use a new network paths, and ** {{}} lists the options for terminating an open connection. * Packets and frames are the basic unit used by QUIC to communicate. ** {{}} describes the format of packets and frames, ** {{}} defines a models for the transmission and retransmission of information, ** {{}} describes acknowledgments, the primary feedback mechanism in QUIC, and ** {{}} contains a rules for managing the size of packets. * Details of encoding of QUIC protocol elements is described in: ** {{}} (Versions), ** {{}} (Packet Headers), ** {{}} (TP), ** {{}} (Frames), and ** {{}} (Errors).

If we do that, I would add a heading, like ## This Document here. That would also satisfy the itch that adding conventions and notations as 1.1 has, which says that having only one subsection is wrong.

I like two levels

draft-ietf-quic-transport.md

martinthomson · 2018-10-15T03:33:50Z

draft-ietf-quic-transport.md

-version negotiation and establishment of 1-RTT keys.  Short headers are minimal
-version-specific headers, which are used after version negotiation and 1-RTT
-keys are established.
+The first bi-directional stream opened by the client is stream 0.


s/bi-directional/bidirectional

martinthomson · 2018-10-15T03:35:20Z

draft-ietf-quic-transport.md


-: The most significant bit (0x80) of octet 0 (the first octet) is set to 1 for
-  long headers.
+An endpoint limits the number of concurrently active incoming streams by


Missing transitional glue: "QUIC allows for an arbitrary number of streams to operate concurrently." Necessary if you take my proposed edit above.

martinthomson · 2018-10-15T03:35:34Z

draft-ietf-quic-transport.md

-: The most significant bit (0x80) of octet 0 (the first octet) is set to 1 for
-  long headers.
+An endpoint limits the number of concurrently active incoming streams by
+adjusting the maximum stream ID.  An initial value is set in the transport


s/adjusting/limiting/

martinthomson · 2018-10-15T08:38:15Z

draft-ietf-quic-transport.md


-Disposing of connection state prior to the end 


Error codes and versions.

martinthomson · 2018-10-15T08:39:51Z

draft-ietf-quic-transport.md

+NEW_CONNECTION_ID frame to be received multiple times.  Receipt of the same
+frame multiple times MUST NOT be treated as a connection error.  A receiver can
+use the sequence number supplied in the NEW_CONNECTION_ID frame to identify new
+connection IDs from old ones.


I think that we can remove the first two sentences of this paragraph. They are vestiges of the design prior to us putting sequence numbers back in.

martinthomson · 2018-10-15T08:41:14Z

draft-ietf-quic-transport.md

-
-### Receive Stream States {#stream-recv-states}
-
-{{fig-s


We should move the remainder of this section to the acknowledgments section.

Done. I've also reworked that section, and left a comment to take a second look at that text.

martinthomson · 2018-10-15T08:41:24Z

draft-ietf-quic-transport.md

-
-### Receive Stream States {#stream-recv-states}
-
-{{fig-s


martinthomson · 2018-10-15T08:42:51Z

draft-ietf-quic-transport.md

-
-### Receive Stream States {#stream-recv-states}
-
-{{fig-s


This paragraph and the next probably belong in the packetization section.

martinduke · 2018-10-16T21:10:46Z

If people are OK with the general layout, I suggest we land this PR quickly (to unblock other PR) and then wordsmith the nits in future PRs.

martinduke

Here is a review through and including the flow control section. Hopefully more to come,.

draft-ietf-quic-transport.md

martinduke · 2018-10-16T21:29:22Z

draft-ietf-quic-transport.md

-Version:
+A receiver cannot renege on an advertisement; that is, once a receiver
+advertises a stream ID via a MAX_STREAM_ID frame, advertising a smaller maximum
+ID has no effect.  A sender MUST ignore any MAX_STREAM_ID frame that does not


s/sender/receiver

martinduke · 2018-10-16T21:31:13Z

draft-ietf-quic-transport.md


-Destination Connection ID:
+When new data is to be sent on a stream, a sender MUST set the encapsulating
+STREAM frame's offset field to the stream offset of the first byte of this new


s/byte/octet

martinduke · 2018-10-17T22:17:13Z

draft-ietf-quic-transport.md

-by the client is subject to the same restrictions as the first Initial packet.
-A client can either reuse the cryptographic handshake message or construct a
-new one at its discretion.
+A data receiver sends MAX_STREAM_DATA or MAX_DATA frames to the sender to


I think we should add a paragraph here that goes like this:

"Endpoints SHOULD communicate initial values for stream and connection flow control using transport parameters {#transport-params}. Data senders MUST use a value of zero for limits not advertised in the peer's transport parameters."

The paragraph above looks good enough to me. I don't think we need to set defaults (if this were the only thing that is read, a reader would (mostly correctly) infer that the values are always set.

I added that paragraph based on martinduke's comment, and yes, I agree that we don't need to recommend setting defaults.

martinthomson

I've tried to attach suggestions to every comment here. Let me know if that works out, it's a new GitHub feature, and a little rough. I'd appreciate if you look at the suggestions before you do, but please merge this ASAP.

martinthomson · 2018-10-18T08:15:48Z

draft-ietf-quic-transport.md

-by the client is subject to the same restrictions as the first Initial packet.
-A client can either reuse the cryptographic handshake message or construct a
-new one at its discretion.
+A data receiver sends MAX_STREAM_DATA or MAX_DATA frames to the sender to


The paragraph above looks good enough to me. I don't think we need to set defaults (if this were the only thing that is read, a reader would (mostly correctly) infer that the values are always set.

draft-ietf-quic-transport.md

martinthomson · 2018-10-18T08:56:09Z

draft-ietf-quic-transport.md


-After sending a closing frame, endpoints immediately enter the closing state.
-During the closing period, an endpoint that sends a closing frame SHOULD respond
-to any packet that it receives with another packet containing a closing frame.


Suggested change

to any packet that it receives with another packet containing a closing frame.

martinthomson · 2018-10-18T08:56:18Z

draft-ietf-quic-transport.md


-After sending a closing frame, endpoints immediately enter the closing state.
-During the closing period, an endpoint that sends a closing frame SHOULD respond
-to any packet that it receives with another packet containing a closing frame.


Suggested change

to any packet that it receives with another packet containing a closing frame.

martinthomson · 2018-10-18T08:56:25Z

draft-ietf-quic-transport.md


-After sending a closing frame, endpoints immediately enter the closing state.
-During the closing period, an endpoint that sends a closing frame SHOULD respond
-to any packet that it receives with another packet containing a closing frame.


Suggested change

to any packet that it receives with another packet containing a closing frame.

martinthomson · 2018-10-18T08:56:33Z

draft-ietf-quic-transport.md


-After sending a closing frame, endpoints immediately enter the closing state.
-During the closing period, an endpoint that sends a closing frame SHOULD respond
-to any packet that it receives with another packet containing a closing frame.


Suggested change

to any packet that it receives with another packet containing a closing frame.

martinthomson · 2018-10-18T08:57:54Z

draft-ietf-quic-transport.md


-After sending a closing frame, endpoints immediately enter the closing state.
-During the closing period, an endpoint that sends a closing frame SHOULD respond
-to any packet that it receives with another packet containing a closing frame.


Suggested change

to any packet that it receives with another packet containing a closing frame.

An endpoint can use the sequence number supplied in the NEW_CONNECTION_ID frame

to identify connection IDs that it has already received.

janaiyengar

There is one commit I'm not taking in, but everything else looks good. I really wanted to try this new feature -- it is great, except that (i) every single comment is a separate commit, which doesn't scale well, and (ii) github UI pipeline is breaking somewhere, and shows incorrect text for suggested deletions at higher line numbers. The line numbers seem fine.

janaiyengar · 2018-10-18T14:58:51Z

draft-ietf-quic-transport.md

-by the client is subject to the same restrictions as the first Initial packet.
-A client can either reuse the cryptographic handshake message or construct a
-new one at its discretion.
+A data receiver sends MAX_STREAM_DATA or MAX_DATA frames to the sender to


I added that paragraph based on martinduke's comment, and yes, I agree that we don't need to recommend setting defaults.

draft-ietf-quic-transport.md

janaiyengar · 2018-10-18T20:25:16Z

draft-ietf-quic-transport.md


-After sending a closing frame, endpoints immediately enter the closing state.
-During the closing period, an endpoint that sends a closing frame SHOULD respond
-to any packet that it receives with another packet containing a closing frame.


I'm not taking this yet - I don't think this should be removed. Let's discuss this after this PR.

janaiyengar · 2018-10-18T20:28:52Z

draft-ietf-quic-transport.md


-After sending a closing frame, endpoints immediately enter the closing state.
-During the closing period, an endpoint that sends a closing frame SHOULD respond
-to any packet that it receives with another packet containing a closing frame.


Something is broken here. This suggested patch doesn't make sense.

janaiyengar · 2018-10-18T20:32:47Z

draft-ietf-quic-transport.md


-After sending a closing frame, endpoints immediately enter the closing state.
-During the closing period, an endpoint that sends a closing frame SHOULD respond
-to any packet that it receives with another packet containing a closing frame.


Again, this suggested patch does not make sense. I think you're talking about the TODO, which I agree, and I'll remove, but the suggested patch is incorrect. Looking closer, the line number is correct, but the text it shows is broken.

janaiyengar added 3 commits October 12, 2018 14:48

Moved stream stuff up and added intro text to describe structure of doc

5ebd7ca

Moved major sections around.

c705fc1

created packets and frames section and moved things about

c4ce2c2

janaiyengar requested a review from martinthomson October 13, 2018 06:10

janaiyengar changed the title ~~First iteration over transport draft~~ WIP: High-level restructure of transport draft Oct 13, 2018

martinthomson reviewed Oct 15, 2018

View reviewed changes

martinthomson added the unicorns!!! label Oct 15, 2018

martinthomson mentioned this pull request Oct 15, 2018

Clarify coalesced packet processing #1844

Closed

janaiyengar mentioned this pull request Oct 15, 2018

Move server send limitations #1843

Closed

modified intro, made error codes first level section

438b965

Done through connection migration section

53f0bad

martinduke suggested changes Oct 17, 2018

View reviewed changes

janaiyengar added 5 commits October 17, 2018 19:33

addresses mt's comments.

9600d97

detailed structure in intro

209a269

most of martinduke comments

d927b6a

added initial values in flow control

af51da7

lint

d5a1e49

martinthomson approved these changes Oct 18, 2018

View reviewed changes

martinthomson and others added 11 commits October 18, 2018 07:59

max_stream_id

db2e9b2

parenthesize

ca49ae4

parenthesize

0368ca9

stream-id-blocked frame

1d4f7d3

s/intertwines/combines

c3426ee

unnecessary text

140a9c6

connection id para

23d95c3

connection id contd

d3cf19f

connection id contd

4c9858f

s/subsequent/additional

0dfa885

shorter sentences

a6262f9

martinthomson and others added 8 commits October 18, 2018 08:08

packet protection TODO

b90815f

packet protection TODO contd

bbb241e

s/the/a

2a00e0b

s/acks/acknowledges

2986d5b

unnecessary text

c631c31

unnecessary text

6a980bf

last of mt's comments

d6c9267

lint

e13b6b3

janaiyengar commented Oct 18, 2018

View reviewed changes

lint

dd74eda

janaiyengar merged commit b557a3a into reorganize Oct 18, 2018

martinthomson deleted the reorganize_transport branch October 26, 2018 00:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: High-level restructure of transport draft #1857

WIP: High-level restructure of transport draft #1857

janaiyengar commented Oct 13, 2018 •

edited

Loading

martinthomson left a comment

martinthomson Oct 15, 2018

ianswett Oct 16, 2018

martinthomson Oct 15, 2018

martinthomson Oct 15, 2018

martinthomson Oct 15, 2018

martinthomson Oct 15, 2018

martinthomson Oct 15, 2018

martinthomson Oct 15, 2018

janaiyengar Oct 18, 2018

martinthomson Oct 15, 2018

martinthomson Oct 15, 2018

martinduke commented Oct 16, 2018

martinduke left a comment

martinduke Oct 16, 2018

martinduke Oct 16, 2018

martinduke Oct 17, 2018

martinthomson Oct 18, 2018

janaiyengar Oct 18, 2018

martinthomson left a comment

martinthomson Oct 18, 2018

martinthomson Oct 18, 2018

martinthomson Oct 18, 2018

martinthomson Oct 18, 2018

martinthomson Oct 18, 2018

martinthomson Oct 18, 2018

janaiyengar left a comment

janaiyengar Oct 18, 2018

janaiyengar Oct 18, 2018

janaiyengar Oct 18, 2018

janaiyengar Oct 18, 2018

	to any packet that it receives with another packet containing a closing frame.
	An endpoint can use the sequence number supplied in the NEW_CONNECTION_ID frame
	to identify connection IDs that it has already received.

WIP: High-level restructure of transport draft #1857

WIP: High-level restructure of transport draft #1857

Conversation

janaiyengar commented Oct 13, 2018 • edited Loading

martinthomson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinduke commented Oct 16, 2018

martinduke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinthomson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janaiyengar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janaiyengar commented Oct 13, 2018 •

edited

Loading