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.

Sign up for GitHub

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

Jump to bottom

QUIC ack frames now store more than 255 ack ranges #906

Closed
ianswett opened this issue Nov 8, 2017 · 4 comments
Closed

QUIC ack frames now store more than 255 ack ranges #906

ianswett opened this issue Nov 8, 2017 · 4 comments
Labels
-recovery -transport editorial An issue that does not affect the design of the protocol; does not require consensus.

Comments

@ianswett
Copy link
Contributor

ianswett commented Nov 8, 2017

The existing text in the transport and recovery draft indicates QUIC ack frames can ack up to 255 ranges. Now that we're using variable length integers, we can ack more than that.

I'm not sure if that's a good thing or a bad thing, but at the least, we should update the text to be self-consistent.
@ianswett ianswett added -recovery -transport editorial An issue that does not affect the design of the protocol; does not require consensus. labels Nov 8, 2017
@RyanTheOptimist
Copy link
Contributor

More than 255 ACK ranges in a packet seems excessive to me. (Honestly 16 is probably fine). I'd be inclined to revert this field back to single byte and limit the number of ranges to 255.

@martinthomson
Copy link
Member

There is value in uniformity. No one has to use that many ranges (noting that 255 would be enough to overflow a packet anyway if abuse was the goal).

@ianswett
Copy link
Contributor Author

ianswett commented Nov 9, 2017

Just because we use varints, doesn't mean we always have to, particularly for things which are only a byte. Reading a byte is really not that difficult, so I think the uniformity value is minimal.

@ianswett
Copy link
Contributor Author

Well, let's stick with this for now, but fix the text before 08 to make it consistent?

martinthomson added a commit that referenced this issue Nov 29, 2017
@martinthomson
Remove remnants of the limit on ACK blocks
acd1d83 
Closes #906
@martinthomson martinthomson mentioned this issue Nov 29, 2017
Merged
janaiyengar pushed a commit that referenced this issue Nov 29, 2017
@martinthomson
Remove remnants of the limit on ACK blocks (#966)
85f46c2 
* Remove remnants of the limit on ACK blocks

Closes #906

* Tweakin'
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
-recovery -transport editorial An issue that does not affect the design of the protocol; does not require consensus.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@martinthomson @RyanTheOptimist @ianswett