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 p2p reqresp with explicit handling of reqresp size bounds #1800

Merged
merged 3 commits into from
May 11, 2020

Conversation

protolambda
Copy link
Collaborator

@protolambda protolambda commented May 9, 2020

Update p2p doc to:

  • Reflect ability to check much stricter SSZ size bounds (as part of the encoding strategy)
  • That the parts of req/resps should be validated according to encoding strategy
  • Encoding strategy describes SSZ bounds checking in more detail. Previously already checking min/max somewhat, but not enforcing it as well.
  • Include FAQ item for SSZ type size bounds
  • Restrict varint bytes to max 10, to avoid unlimited varint attack
  • Misc. consistency, clarification and de-duplication things.

Related, now that we are looking more critically at the req-resp payload format/sizing:

I see we have both:


response_chunk  ::= <result> | <encoding-dependent-header> | <encoded-payload>
  +--------+--------+--------+--------+--------+--------+
  | result |   header (opt)  |     encoded_response     |
  +--------+--------+--------+--------+--------+--------+

Can we maybe remove the second one? It's nice ascii, but this and the sudden introduction of "A response therefore has the form ..." after going in-depth a lot already feels double to me. And the optional header is confusing with the earlier BNF. The header can be empty if the encoding strategy doesn't need any header data, that's all.


Edit: above duplicate section has been removed. The BNF-like diagram already has this exact information. And result being one byte is explained as part of the responding side.

Force push edit: bounds are now described as type size bounds instead of type bounds

Copy link
Contributor

@djrtwo djrtwo left a comment

Choose a reason for hiding this comment

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

looks goo. just some minor fixes

specs/phase0/p2p-interface.md Outdated Show resolved Hide resolved
specs/phase0/p2p-interface.md Outdated Show resolved Hide resolved
Copy link
Contributor

@djrtwo djrtwo left a comment

Choose a reason for hiding this comment

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

I handled the minor PR feedback (typo and markdown list formatting)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants