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

Robert Wilton Invariants Discuss 1 #4468

Closed
LPardue opened this issue Jan 4, 2021 · 17 comments · Fixed by #4744
Closed

Robert Wilton Invariants Discuss 1 #4468

LPardue opened this issue Jan 4, 2021 · 17 comments · Fixed by #4744
Assignees
Labels
-invariants -transport design An issue that affects the design of the protocol; resolution requires consensus. iesg An issue raised during IESG review.

Comments

@LPardue
Copy link
Member

LPardue commented Jan 4, 2021

Robert Wilton said:

A trivial discuss that should hopefully be easy to resolve, and it is plausible
that the resolution may end up being in the QUIC transport document:

In this document, the unused bits are defined as:

               Figure 4: Version Negotiation Packet

Only the most significant bit of the first byte of a Version
Negotiation packet has any defined value. The remaining 7 bits,
labeled Unused, can be set to any value when sending and MUST be
ignored on receipt.

In the QUIC transport document, they are defined as this:

               Figure 14: Version Negotiation Packet

The value in the Unused field is selected randomly by the server.
Clients MUST ignore the value of this field. Servers SHOULD set the
most significant bit of this field (0x40) to 1 so that Version
Negotiation packets appear to have the Fixed Bit field.

I would have expected that these two should be consistent as to whether the
Fixed Bit SHOULD be set to 1 or not. Given draft-thomson-quic-bit-grease-00,
it might be better if the SHOULD is removed from QUIC transport, but I will
defer to the experts here.

@LPardue LPardue added -invariants iesg An issue raised during IESG review. labels Jan 4, 2021
@LPardue LPardue added this to the invariants-iesg milestone Jan 4, 2021
@martinthomson
Copy link
Member

I think that Rob's suggestion to remove the "SHOULD" is ideal.

@DavidSchinazi
Copy link
Contributor

Does someone remember why this SHOULD was added? The fixed bit is useful in the client-to-server direction because it allows servers to demultiplex QUIC with other protocols, but I don't remember what benefit it provided in the server-to-client direction.

@ianswett
Copy link
Contributor

ianswett commented Jan 5, 2021

I believe there 'could' be applications(ie: WebRTC) which share a port with QUIC and might want to use this bit to demux even on the client side, but that's seems like a pretty theoretical problem, so I'd be inclined to drop the SHOULD.

@marten-seemann
Copy link
Contributor

The fixed bit is useful in the client-to-server direction because it allows servers to demultiplex QUIC with other protocols, but I don't remember what benefit it provided in the server-to-client direction.

In a p2p setup, you might run QUIC clients and servers on the same port (in fact, that's what we're doing in libp2p), so demultiplexing might be important for both sides in that case.

@janaiyengar
Copy link
Contributor

Yes, this was to avoid collision with p2p protocols. As I recall, we had decided that this would be something we would recommend for v1, but not require this for future versions... and that if both endpoints agreed, then they could set the bit to something other than 1. I don't see why we should change this.

@DavidSchinazi
Copy link
Contributor

In a P2P setup where we're demultiplexing with another bootstrapping protocol over UDP, is there a need for version negotiation packets at all? I'd expect the bootstrapping protocol to convey the supported QUIC versions, and at that point you don't need version negotiation. I'm in favor of removing the SHOULD on VN packets.

@ianswett
Copy link
Contributor

ianswett commented Jan 6, 2021

My thinking is along the lines of @DavidSchinazi but I also don't care much about whether we remove this or not.

@MikeBishop
Copy link
Contributor

The VN packet is version-independent. v1 shouldn't be constraining its contents. However, implementations are free to select the value they put there any way they wish, so if they find it useful to always set that bit, they may.

@janaiyengar
Copy link
Contributor

This isn't constraining the contents of a VN packet broadly, it is constraining what a v1 sender sends in a VN packet. And, I don't think it's reasonable to require that P2P protocols shouldn't need to use VN.

My high-level point is this: IIRC, we had this SHOULD for co-existence with P2P protocols, and we went through a lot of discussion, especially with ART folks on this. Do we really want to re-litigate this point at this stage in the process?

Maybe I'm mis-remembering. If I am, and this SHOULD was there for a different reason that is no longer valid (or never was important), I'm happy to remove this.

@larseggert larseggert added this to Triage in Late Stage Processing via automation Jan 11, 2021
@martinthomson
Copy link
Member

That's right. Maybe we should have been more studious in capturing reasons.

I think that for those P2P applications that care about multiplexing, this is a non-issue. Those applications can use their signaling channel to ensure that version negotiation does not happen. If things get badly out of hand, endpoints can ensure that Version Negotiation packets are sent with the bit set to conform to other constraints.

@marten-seemann
Copy link
Contributor

I think that for those P2P applications that care about multiplexing, this is a non-issue. Those applications can use their signaling channel to ensure that version negotiation does not happen.

That applies to peers discovered during the execution of the p2p protocol. Every p2p network has some hard-coded bootstrap nodes though, for which the QUIC version might not be known in advance.

@larseggert larseggert added the design An issue that affects the design of the protocol; resolution requires consensus. label Jan 12, 2021
@project-bot project-bot bot moved this from Triage to Design Issues in Late Stage Processing Jan 12, 2021
@DavidSchinazi
Copy link
Contributor

@marten-seemann are those hard-coded bootstrap nodes QUIC clients or servers? We're only talking about receipt of VN packets here.

@martinthomson
Copy link
Member

I think that we can keep the inconsistency, but we need someone (@janaiyengar) to follow up with Rob.

@janaiyengar
Copy link
Contributor

@rgwilton: At this point, I'd rather not change what was a design decision made by the wg, so I propose close with no action to this issue (not merge #4471). Can you see if the explanation above is satisfactory to you?

@rgwilton
Copy link

To check that my understanding is correct:

  • VN packets (as per the Invariants doc) MAY set the fixed bit.
  • VN packets for QUIC version 1 (as per the transport doc) SHOULD set the fixed bit.

This behaviour is consistent, but the documents need to explain this more clearly. Particularly, because the the first sentence of section 17.2.1 states: " A Version Negotiation packet is inherently not version-specific," and yet the value set in this field might be version specific.

Hence, I would suggest that you expand the description in the transport draft to explain that it SHOULD be set for QUIC version 1, but that other versions of QUIC may relax this requirement. Noting that the transport draft already talks about future versions having different requirements on the length of connection ids.

Late Stage Processing automation moved this from Design Issues to Issue Handled Jan 15, 2021
@martinthomson
Copy link
Member

Just to close the loop here. I've added an explanation of two things to the draft that is about to go out: that this recommendation is to allow for multiplexing, and that this recommendation is not guaranteed to apply when other QUIC versions are in use. I think that is in line with what @rgwilton suggests. If you disagree, we might be able to tweak this more later, but I need to publish -34 so that the wheels of the process can continue to turn.

@larseggert larseggert moved this from Issue Handled to Design Issues in Late Stage Processing Jan 15, 2021
@rgwilton
Copy link

Just to confirm that I am happy with the committed text change. Thanks for accommodating.

@LPardue LPardue moved this from Design Issues to Issue Handled in Late Stage Processing Jan 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-invariants -transport design An issue that affects the design of the protocol; resolution requires consensus. iesg An issue raised during IESG review.
Projects
Late Stage Processing
  
Issue Handled
9 participants