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

Update discussion of Version Negotiation #4697

Merged
merged 5 commits into from
Jan 11, 2021

Conversation

kaduk
Copy link
Contributor

@kaduk kaduk commented Jan 7, 2021

Remove a sentence (apparently) about handling version negotiation packets from the section that's supposed to be about sending them.

Clarify that the handling of version negotiation packets is meant to be specified only by standards-track documents (since it is in some sense a cross-version invariant rather than version-specific behavior). In particular, those documents will not necessarily be specifications for a future version of QUIC.

New versions of QUIC specified prior to the existence of a version negotiation mechanism are to respond to Version Negotiation packets in the same manner as v1.

There was a sentence in this section that seems to be specifying
how to handle a version negotiation packet, not how to send one,
and as such is out of place.
Attempt to more accurately reflect the expectations of the WG for
how version negotiation will be defined and what endpoint behavior
should be in the absence of a version negotiation mechanism.
In particular, this work is to be done by the IETF, not by arbitrary
future versions of QUIC, so we retain control over how it will be
developed and deployed.

Also accommodate the possibility of new versions of QUIC appearing
before version negotiation is defined.

Fixes: quicwg#4607
Copy link
Member

@martinthomson martinthomson left a comment

Choose a reason for hiding this comment

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

This seems largely OK. I do have a couple of reservations though.

Comment on lines 1437 to 1441
{{version-downgrade}}. New versions of QUIC (including
non-standards-track versions) might be specified prior to the
availability of a version negotiation mechanism. Until a version
negotiation mechanism is available, new versions of QUIC MUST respond to
Version Negotiation packets as specified above.
Copy link
Member

Choose a reason for hiding this comment

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

I don't think that we need to include these last two sentences. While we've definitely strayed into making forward-looking statements (read: predictions) about what we might do, this goes too far in my opinion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I concede it was a bit speculative to say that this reflects existing consensus. That said, there is perhaps a spectrum of options, from removing it entirely to "are expected to", "SHOULD", etc. It sounds like you prefer just removing it entirely, to confirm?

Copy link
Member

Choose a reason for hiding this comment

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

Yes. I don't think that we get much value from saying anything like this, regardless of strength, simply because it is making a prediction about work that might happen in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough. I will wait a bit before pushing an update to effectuate that, in case others want to comment.

Comment on lines 1411 to 1412
As a result, the client discards all state for the connection and does not send
any more packets on the connection.
Copy link
Member

Choose a reason for hiding this comment

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

Why remove this? It doesn't seem to be responsive to the concerns that the rest of the change addresses.

Copy link
Contributor Author

@kaduk kaduk Jan 8, 2021

Choose a reason for hiding this comment

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

It's an unrelated change and could easily be elided from the PR.
I read through the whole top-level section while working on this PR, and I thought this looked out of place (it's the "Remove a sentence (apparently) about handling version negotiation packets from the section that's supposed to be about sending them."). I could be mistaken about that, though, and we can split it out if that's preferred.

Copy link
Member

Choose a reason for hiding this comment

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

Please try to focus on targeted fixes. That makes this process a lot easier to manage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before I commit another faux pas, is it preferred to force-push or push a revert commit (or something else)?

Copy link
Member

Choose a reason for hiding this comment

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

I don't personally mind. In this repo, we have let the commit history reflect the history of the issues. So push as you like.

@kaduk
Copy link
Contributor Author

kaduk commented Jan 8, 2021

Pushed fixups for both points raised.

Copy link
Contributor

@janaiyengar janaiyengar left a comment

Choose a reason for hiding this comment

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

One suggestion, but this is good.

draft-ietf-quic-transport.md Outdated Show resolved Hide resolved
Co-authored-by: Jana Iyengar <jri.ietf@gmail.com>
@martinthomson martinthomson added -transport editorial An issue that does not affect the design of the protocol; does not require consensus. labels Jan 11, 2021
@martinthomson martinthomson merged commit dfa33c9 into quicwg:master Jan 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-transport editorial An issue that does not affect the design of the protocol; does not require consensus.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants