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

Size limit for Version Negotiation #754

Merged
merged 2 commits into from
Sep 8, 2017
Merged

Size limit for Version Negotiation #754

merged 2 commits into from
Sep 8, 2017

Conversation

martinthomson
Copy link
Member

As discussed, the limits are largely practical, and there should be enough
incentive to avoid inflation, but it pays to be a little more precise when
defining protocols or you can get odd downstream effects. I've chosen to limit
the size to the CI. That's 1200 currently (or more) and should be plenty og
space for many versions.

Closes #585.

As discussed, the limits are largely practical, and there should be enough
incentive to avoid inflation, but it pays to be a little more precise when
defining protocols or you can get odd downstream effects.  I've chosen to limit
the size to the CI.  That's 1200 currently (or more) and should be plenty og
space for many versions.

Closes #585.
@martinthomson martinthomson added -transport editorial An issue that does not affect the design of the protocol; does not require consensus. labels Aug 31, 2017
Copy link
Contributor

@MikeBishop MikeBishop left a comment

Choose a reason for hiding this comment

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

SGTM.

@@ -887,6 +887,13 @@ server discards the incoming packet and responds with a Version Negotiation
packet ({{packet-version}}). This includes a list of versions that the server
will accept.

There is no limit to the size of the Version Negotiation packet, other than the
number of versions that the server supports and the estimated path MTU. To
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the point of this change is to impose a limit, this first sentence seems contradictory. I'd just drop it.

@martinthomson martinthomson merged commit d6e7119 into master Sep 8, 2017
@martinthomson martinthomson deleted the vn-mtu branch October 12, 2017 23:52
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.

None yet

3 participants