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

Move back to draft 07 style type byte #995

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
8 participants
@ianswett
Contributor

ianswett commented Dec 6, 2017

Un-does most of the changes in PR #956, to give time for more discussion and avoid causing implementers potentially unnecessary work.

Move back to draft 07 style type byte
Un-does most of the changes in PR #956, to give time for rough consensus to be reached before changing the type byte.
@ekr

This comment has been minimized.

Collaborator

ekr commented Dec 6, 2017

@martinthomson @ianswett are you planning to issue a -09 with this change? Should we use that for ID3?

@ianswett

This comment has been minimized.

Contributor

ianswett commented Dec 6, 2017

@ekr If this lands, then I believe the answer is yes(along with complete change logs). But that's subject to @martinthomson and @janaiyengar

@janaiyengar

Please wait for @martinthomson's response, but LGTM. (I'm happy to bring this change back in once we agree on list -- it's a small enough text change.)

@janaiyengar

This comment has been minimized.

Contributor

janaiyengar commented Dec 6, 2017

@ekr: I'd like for @martinthomson to take a look at this PR, and if it lands, then we'll issue a draft-09.

@MikeBishop

I'm ambivalent here -- it does sound like the discussion isn't finished, though.

@larseggert

This comment has been minimized.

Member

larseggert commented Dec 6, 2017

So I did implement the current -08 scheme already... :-(

@csperkins

This comment has been minimized.

Contributor

csperkins commented Dec 7, 2017

The discussion hasn't finished, but I thought there was consensus that some form of multiplexing was needed. This change looks like a step backwards.

@marten-seemann

This comment has been minimized.

Contributor

marten-seemann commented Dec 7, 2017

I agree that it's better to first get this right, and then change all the implementations. There's no benefit from changing the same code over and over again.

@janaiyengar

This comment has been minimized.

Contributor

janaiyengar commented Dec 7, 2017

@csperkins There was no formal consensus, but that's not the bar for getting changes into the draft. That said, we're still discussing the form and what we want to mux with, so the discussion is clearly not necessarily going to yield this outcome. In the meanwhile, implementations are required to use these new types for interop, and will likely have to change it again when we figure out the "right" answer. Given this is not the agreed upon answer yet, what does this PR get you?

@huitema

This comment has been minimized.

Contributor

huitema commented Dec 8, 2017

I have implemented the changes in Picoquic. Going back to the previous format would not help me one bit. Besides, the PR is not going back to the previous format -- it still does changes like merging server clear text and client clear text in a single code, removing the code point for renegotiation, and shifting all the other long header types by one. So it does not actually save much work for someone going from -07 to -08.

Useless, negative in fact.

@huitema

Please don't do that. It will cause trouble for those implementers who already have draft 08 done, and will not actually save that much work for the other ones. Even with that change, the format of the long header will change, due to removal of renegotiation. So you don't win much...

@marten-seemann

This comment has been minimized.

Contributor

marten-seemann commented Dec 8, 2017

@huitema: Client and Server Cleartext were merged into a single codepoint a long time before #956.

This PR is very helpful for people who have a deployment of gQUIC, since it's easy to demultiplex the old format with gQUIC, which is a big pain with #956. I don't know about the others maintaining a gQUIC deployments, but I don't see the point of going through the trouble to implement a new demultiplexing scheme for this change, just to change it again once we agree on yet another type byte format.

@ianswett

This comment has been minimized.

Contributor

ianswett commented Dec 8, 2017

@huitema At this point, I think there are enough people who have implemented 08 that this should only be landed once we have a plan for greasing the type byte.

@marten-seemann Given we're not yet greasing, I think you can use the trick of looking at the nonce bit on the server side to differentiate GQUIC from IETF QUIC(No client to server packet ever has the nonce bit set to true in GQUIC). We haven't done this yet in GQUIC, but I don't anticipate this will be a huge change?

@ianswett

This comment has been minimized.

Contributor

ianswett commented Dec 8, 2017

Slight tweak to my last comment. Apparently @janaiyengar asked implementers their preference on the slack channel. I'm happy to let implementers decide.

@janaiyengar

This comment has been minimized.

Contributor

janaiyengar commented Dec 12, 2017

Based on implementer input on the slack channel, the general sense seems to be that folks have implemented the changes this PR reverts, so we aren't going to merge this PR. Please note that there's active discussion on the question of packet types, which continues on #426. Please share further thoughts there.

@MikeBishop

This comment has been minimized.

Contributor

MikeBishop commented Dec 18, 2017

Given that we've agreed not to do this, I'm closing it.

@MikeBishop MikeBishop closed this Dec 18, 2017

@MikeBishop MikeBishop deleted the ianswett-07typebyte branch Jan 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment