Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change Client Initial size to 1200 flat #685

Merged
merged 4 commits into from
Aug 11, 2017
Merged

Change Client Initial size to 1200 flat #685

merged 4 commits into from
Aug 11, 2017

Conversation

martinthomson
Copy link
Member

Closes #684.

@martinthomson martinthomson added -transport design An issue that affects the design of the protocol; resolution requires consensus. labels Jul 15, 2017
Copy link
Collaborator

@ekr ekr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 million


The initial client packet SHOULD be padded to exactly these values unless the
The initial client packet SHOULD be padded to exactly 1200 octets unless the
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make clear that this is just the QUIC packet, not the headers.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -541,7 +541,7 @@ older than 1.3 is negotiated.

QUIC requires that the initial handshake packet from a client fit within the
payload of a single packet. The size limits on QUIC packets mean that a record
containing a ClientHello needs to fit within 1197 octets.
containing a ClientHello needs to fit within 1165 octets.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be clear, this 1165 number may change if we increase or decrease the amount of framing overhead QUIC adds. Are we ok about that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK with that. As long as we stop changing the overhead when we ship the spec.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One q: How are you arriving at 1165 if the 1200 doesn't include IP + UDP? 1(type byte) + 8(conn id) + 4(packet number) + stream frame type byte(1) + stream id(1 byte) + ?

the QUIC header and integrity check, but not the UDP or IP header. Sending a
packet of this size ensures that the network path supports an MTU of this size
and helps reduce the amplitude of amplification attacks caused by server
responses toward an unverified client address.

Servers MUST ignore an initial plaintext packet from a client if its total size
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be nice to come up with a consistent way of referring to this "total size" which does not include IP+UDP.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"QUIC packet size" or "packet size" should do.

Copy link
Contributor

@ianswett ianswett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As with the definition of bytes_in_flight for recovery, it's important to have a consistent definition of packet size. It looks like we're aiming for "size of packet without including IP + UDP headers", which seems great, but the current text may be ambiguous?

@@ -541,7 +541,7 @@ older than 1.3 is negotiated.

QUIC requires that the initial handshake packet from a client fit within the
payload of a single packet. The size limits on QUIC packets mean that a record
containing a ClientHello needs to fit within 1197 octets.
containing a ClientHello needs to fit within 1165 octets.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One q: How are you arriving at 1165 if the 1200 doesn't include IP + UDP? 1(type byte) + 8(conn id) + 4(packet number) + stream frame type byte(1) + stream id(1 byte) + ?

@@ -1140,7 +1140,7 @@ max_packet_size (0x0005):
the endpoint is willing to receive, encoded as an unsigned 16-bit integer.
This indicates that packets larger than this limit will be dropped. The
default for this parameter is the maximum permitted UDP payload of 65527.
Values below 1252 are invalid. This limit only applies to protected packets
Values below 1200 are invalid. This limit only applies to protected packets
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we somehow clarify this size is also without UDP + IP headers?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Payload size?

@martinthomson
Copy link
Member Author

@ianswett,

One q: How are you arriving at 1165 if the 1200 doesn't include IP + UDP? 1(type byte) + 8(conn id) + 4(packet number) + stream frame type byte(1) + stream id(1 byte) + ?

1200- all of that(15) - version(4) - fnv-1a(8), which I realize now is 29 and not 35 because I calculated the original number with FNV-1a 128.

Copy link
Contributor

@ianswett ianswett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-transport design An issue that affects the design of the protocol; resolution requires consensus.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants