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

Give IMMEDIATE_ACK a 1 byte codepoint #216

Merged
merged 6 commits into from
Oct 23, 2023
Merged

Give IMMEDIATE_ACK a 1 byte codepoint #216

merged 6 commits into from
Oct 23, 2023

Conversation

ianswett
Copy link
Collaborator

@ianswett ianswett commented Jul 25, 2023

Fixes #181 by making IMMEDIATE_ACK 0x1f

ACK_FREQUENCY already has a 2 byte codepoint.

Also changes the transport parameter value, because this is not backwards compatible.

@ianswett ianswett requested a review from mirjak July 25, 2023 22:35
@marten-seemann
Copy link
Contributor

I don't think this PR achieves the intended effect.
0xaf = 175
0xaccf = 44239
The 1-byte range ends at 63, the 2-byte range at 16383: https://datatracker.ietf.org/doc/html/rfc9000#name-variable-length-integer-enc. Therefore 0xaf is a 2-byte value, whereas 0xaccf is a 4-byte value.

@ianswett
Copy link
Collaborator Author

Thanks, I was having that thought, but assumed I must be mistaken.

In that case, the existing value already was a 2 byte value and we don't need to change any text in the draft, correct?

@marten-seemann
Copy link
Contributor

Not for this frame.
IMMEDIATE_ACK is still at 0xac (= 172), which is a 2-byte code point.

@ianswett
Copy link
Collaborator Author

I was just thinking that. Any suggested frame values for IMMEDIATE_ACK?

@kazuho
Copy link
Member

kazuho commented Jul 26, 2023

Assuming that there is nothing attractive as a abbreviation, maybe it would be a good idea to pick the smallest unused value?

From the pretty restricted 1-byte number space, we picked QUIC v1 frame numbers in order. Why not continue doing that?

@mirjak
Copy link
Contributor

mirjak commented Jul 26, 2023

Actually at this point we probably should set them to TBD and then let them be picked and assigned by the experts when the RFC gets published. I think we can keep the old ones for experimentation (however, we never officially registered them which we should have done). I would assume the experts would then just pick the next one in order.

@LPardue
Copy link
Member

LPardue commented Jul 26, 2023

We'll need implementation and interop before we can get to RFC. We need code points to do that. Asking for a provisional registration right now, for the value we intend to use later seems ok given the stage we are at.

@mirjak
Copy link
Contributor

mirjak commented Jul 26, 2023

The values 0x00 to 0x3f require standards Action and can only be assigned as soon as the RFC is published. Therefore I basically suggest to keep using the current values for experimentation and only assign the final values on publication.

@LPardue
Copy link
Member

LPardue commented Jul 26, 2023

Per https://datatracker.ietf.org/doc/html/rfc9000#section-22.1.4

The creation of a registry MAY identify a range of codepoints where registrations are governed by a different registration policy. For instance, the "QUIC Frame Types" registry (Section 22.4) has a stricter policy for codepoints in the range from 0 to 63.Any stricter requirements for permanent registrations do not prevent provisional registrations for affected codepoints. For instance, a provisional registration for a frame type of 61 could be requested.All registrations made by Standards Track publications MUST be permanent.

My read is that we actually allow the provisional registration in the low code point space

@mirjak
Copy link
Contributor

mirjak commented Jul 26, 2023

Ah, I only read this one:

"Permanent registrations in this registry are assigned using the Specification Required policy (Section 4.6 of [RFC8126]), except for values between 0x00 and 0x3f (in hexadecimal), inclusive, which are assigned using Standards Action or IESG Approval as defined in Sections 4.9 and 4.10 of [RFC8126]."

But I guess the text you cite above says that provisional registration are always only "Specification required". That seem to circumvent the registration policy a bit. But I guess that's what it says.

I think I both cases we should actually registered the code point(s) before submitting a new draft, especially if we want to ue the initial range.

But again given there is already a number we are squatting on, I propose we keep that for experimentation and assign the final number on publication. That ensures that we don't do any additional changes anymore that would burn that code point.

@LPardue
Copy link
Member

LPardue commented Jul 27, 2023

Sounds reasonable to me. Caveat: I am not a DE and have not yet tried this extension

@@ -212,7 +212,7 @@ ACK_FREQUENCY frame, shown below:

~~~
ACK_FREQUENCY Frame {
Type (i) = 0xaf,
Type (i) = 0xaccf,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Type (i) = 0xaccf,
Type (i) = TBD,

@@ -221,7 +221,7 @@ ACK_FREQUENCY Frame {
~~~

Following the common frame format described in {{Section 12.4 of
QUIC-TRANSPORT}}, ACK_FREQUENCY frames have a type of 0xaf, and contain the
QUIC-TRANSPORT}}, ACK_FREQUENCY frames have a type of 0xaccf, and contain the
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
QUIC-TRANSPORT}}, ACK_FREQUENCY frames have a type of 0xaccf, and contain the
QUIC-TRANSPORT}}, ACK_FREQUENCY frames have a type of TBD1, and contain the

@@ -576,7 +576,7 @@ registry under the "QUIC Protocol" heading.

Value | Frame Name | Specification
-----------|---------------------|-----------------
0xaf | ACK_FREQUENCY | {{ack-frequency-frame}}
0xaccf | ACK_FREQUENCY | {{ack-frequency-frame}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
0xaccf | ACK_FREQUENCY | {{ack-frequency-frame}}
TBD1 | ACK_FREQUENCY | {{ack-frequency-frame}}

@@ -576,7 +576,7 @@ registry under the "QUIC Protocol" heading.

Value | Frame Name | Specification
-----------|---------------------|-----------------
0xaf | ACK_FREQUENCY | {{ack-frequency-frame}}
0xaccf | ACK_FREQUENCY | {{ack-frequency-frame}}
0xac | IMMEDIATE_ACK | {{immediate-ack-frame}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
0xac | IMMEDIATE_ACK | {{immediate-ack-frame}}
TBD2 | IMMEDIATE_ACK | {{immediate-ack-frame}}

draft-ietf-quic-ack-frequency.md Show resolved Hide resolved
@mirjak
Copy link
Contributor

mirjak commented Aug 28, 2023

@ianswett should we discuss this further?

@ianswett
Copy link
Collaborator Author

ianswett commented Sep 2, 2023

I'm happy to close this and then register permanent codepoints for the final draft.

@ianswett
Copy link
Collaborator Author

ianswett commented Sep 9, 2023

I've updated the PR to reflect the values I believe we should request from IANA.

@mirjak
Copy link
Contributor

mirjak commented Sep 18, 2023

Codepoint in those ranges need standard action based on RFC9000. This those values can only be assigned when publication is approved. We should not put them in the draft now.

@LPardue
Copy link
Member

LPardue commented Sep 18, 2023

I'm confused because the PR title doesn't match the content, so not 100% clear what the intent is.

Regardless, we can in theory request early-assignment per https://datatracker.ietf.org/doc/html/rfc7120. Perhaps that's a bit too early since we want some more interop but let the chairs know if you want to pursue it.

@ianswett ianswett changed the title Change 0xaf to 0xaccf for the Ack Frequency Frame Give IMMEDIATE_ACK a 1 byte codepoint Sep 18, 2023
@ianswett
Copy link
Collaborator Author

Title was out of date and has been updated.

My only reason for preferring to ask for the codepoints now would be to avoid the complexity of 1 vs 2 byte IMMEDIATE_ACK frames for those who haven't yet shipped the 2 byte version, which includes me. Otherwise, I think it's fine to wait.

That being said, given you have to negotiate the extension to have these frame types be valid, the only issue is if another extension tries using the same codepoint without registering it and one wants to use both extensions at once. But that does seem to be the point of provisional registrations with IANA, so maybe we should register it now?

@mirjak
Copy link
Contributor

mirjak commented Sep 18, 2023

The two options are we either a) ask for provisional registration now (which we should have done for the all other codepoints anyway) and wait for draft submission until assigned. Or b) we submit now with the old values and start last call right away in order to get this done quickly.

@mirjak
Copy link
Contributor

mirjak commented Oct 16, 2023

I've put in a provisional registration request to IANA for 0x1f and 0xaf frame types. I'm also requesting provisional registration for the new transport parameter 0xff04de1b.

Should we anyway also registered the old values (transport parameter 0xff04de1a and frame type 0xac) or are we sure these have never been deployed?

Also I want to note that while it is true that frames are only valid if an extension is negotiated, by creating this registry we basically have we only have one number space for all extensions and I don't think we intended to have multiple entries for the same value in the registry...? It is true that it is easier to re-use provisional assignment because you have to negotiate a transport parameter, however, not sure if that was really the intention with this process.

Note provisional allocation and request permanent allocation to IANA when document is approved for publication.
@mirjak
Copy link
Contributor

mirjak commented Oct 16, 2023

I updated this PR to request permanent allocation on approval for publication. I assume we want a new, shorter transport parameter for the final RFC, right?

@ianswett
Copy link
Collaborator Author

I think we'll want a new, shorter TP for the RFC, yes.

@mirjak mirjak merged commit 5df318c into main Oct 23, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Final code point size
5 participants