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

Spin bit: Not implemented == disabled #4691

Merged
merged 3 commits into from
Jan 11, 2021
Merged

Spin bit: Not implemented == disabled #4691

merged 3 commits into from
Jan 11, 2021

Conversation

MikeBishop
Copy link
Contributor

Fixes #4666.

@MikeBishop MikeBishop added -transport iesg An issue raised during IESG review. labels Jan 7, 2021
@@ -5113,6 +5113,8 @@ discussed in {{?QUIC-MANAGEABILITY=I-D.ietf-quic-manageability}}.

The spin bit is an OPTIONAL feature of this version of QUIC. A QUIC stack that
chooses to support the spin bit MUST implement it as specified in this section.
A QUIC stack that chooses not to support the spin bit MUST behave as if it were
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to add this sentence?

Copy link
Member

Choose a reason for hiding this comment

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

I have a simpler change:

The spin bit is an OPTIONAL feature of this version of QUIC. An endpoint that does not support this feature MUST disable it, as defined below.

Copy link
Contributor

Choose a reason for hiding this comment

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

Use of "QUIC stack" is also odd. I like @martinthomson 's simpler suggestion.

@@ -5126,7 +5128,9 @@ approximately one in eight network paths.
When the spin bit is disabled, endpoints MAY set the spin bit to any value, and
MUST ignore any incoming value. It is RECOMMENDED that endpoints set the spin
bit to a random value either chosen independently for each packet or chosen
independently for each connection ID.
independently for each connection ID. One way to accomplish this is to adjust
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a useful addition.

Copy link
Member

Choose a reason for hiding this comment

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

No. No. No. This does not work. We discussed this. It breaks the AEAD.

@@ -5113,6 +5113,8 @@ discussed in {{?QUIC-MANAGEABILITY=I-D.ietf-quic-manageability}}.

The spin bit is an OPTIONAL feature of this version of QUIC. A QUIC stack that
chooses to support the spin bit MUST implement it as specified in this section.
A QUIC stack that chooses not to support the spin bit MUST behave as if it were
Copy link
Member

Choose a reason for hiding this comment

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

I have a simpler change:

The spin bit is an OPTIONAL feature of this version of QUIC. An endpoint that does not support this feature MUST disable it, as defined below.

@@ -5126,7 +5128,9 @@ approximately one in eight network paths.
When the spin bit is disabled, endpoints MAY set the spin bit to any value, and
MUST ignore any incoming value. It is RECOMMENDED that endpoints set the spin
bit to a random value either chosen independently for each packet or chosen
independently for each connection ID.
independently for each connection ID. One way to accomplish this is to adjust
Copy link
Member

Choose a reason for hiding this comment

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

No. No. No. This does not work. We discussed this. It breaks the AEAD.

@martinthomson martinthomson dismissed their stale review January 11, 2021 02:59

I've hijacked the PR with my preferred outcome

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.

@martinthomson -- this is good now. Thanks!

@janaiyengar janaiyengar merged commit 10e1298 into master Jan 11, 2021
@janaiyengar janaiyengar deleted the transport/wilton1 branch January 11, 2021 03:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-transport iesg An issue raised during IESG review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Robert Wilton's Transport Discuss 1
4 participants