-
Notifications
You must be signed in to change notification settings - Fork 738
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
[Merged by Bors] - Complete match for has_context_bytes
#3972
[Merged by Bors] - Complete match for has_context_bytes
#3972
Conversation
This reverts commit 89290c4.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch!
_ => return false, | ||
match self.message_name { | ||
Protocol::BlocksByRange | Protocol::BlocksByRoot => { | ||
!matches!(self.version, Version::V1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, it might be better if we explicitly match Version::V2
instead of excluding V1. If there's a V3 in the future, can't say for sure if that would have a similar mechanism for context bytes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added this and similar for the light client endpoint, but the fact that we're now going to handle a light client protocol + version combination that doesn't exist let me to thinking about this more broadly: #3980
bors r+ |
## Issue Addressed - Add a complete match for `Protocol` here. - The incomplete match was causing us not to append context bytes to the light client protocols - This is the relevant part of the spec and it looks like context bytes are defined https://github.com/ethereum/consensus-specs/blob/dev/specs/altair/light-client/p2p-interface.md#getlightclientbootstrap Disclaimer: I have no idea if people are using it but it shouldn't have been working so not sure why it wasn't caught Co-authored-by: realbigsean <seananderson33@gmail.com>
Build failed: |
I haven't re-bors'd this because we're finalizing the v3.5.0 release today (and this isn't tagged for it) and I'm not sure if this requires some testing or not. |
bors r+ |
## Issue Addressed - Add a complete match for `Protocol` here. - The incomplete match was causing us not to append context bytes to the light client protocols - This is the relevant part of the spec and it looks like context bytes are defined https://github.com/ethereum/consensus-specs/blob/dev/specs/altair/light-client/p2p-interface.md#getlightclientbootstrap Disclaimer: I have no idea if people are using it but it shouldn't have been working so not sure why it wasn't caught Co-authored-by: realbigsean <seananderson33@gmail.com>
Build failed (retrying...): |
## Issue Addressed - Add a complete match for `Protocol` here. - The incomplete match was causing us not to append context bytes to the light client protocols - This is the relevant part of the spec and it looks like context bytes are defined https://github.com/ethereum/consensus-specs/blob/dev/specs/altair/light-client/p2p-interface.md#getlightclientbootstrap Disclaimer: I have no idea if people are using it but it shouldn't have been working so not sure why it wasn't caught Co-authored-by: realbigsean <seananderson33@gmail.com>
I think this needs updating for Capella (and BlobsByRange). |
bors r- |
Canceled. |
…ght-client-protocol-match
I think no updates are required since #4019 was merged, so I just merged with unstable and looks like CI passed |
bors r+ |
## Issue Addressed - Add a complete match for `Protocol` here. - The incomplete match was causing us not to append context bytes to the light client protocols - This is the relevant part of the spec and it looks like context bytes are defined https://github.com/ethereum/consensus-specs/blob/dev/specs/altair/light-client/p2p-interface.md#getlightclientbootstrap Disclaimer: I have no idea if people are using it but it shouldn't have been working so not sure why it wasn't caught Co-authored-by: realbigsean <seananderson33@gmail.com>
has_context_bytes
has_context_bytes
## Issue Addressed - Add a complete match for `Protocol` here. - The incomplete match was causing us not to append context bytes to the light client protocols - This is the relevant part of the spec and it looks like context bytes are defined https://github.com/ethereum/consensus-specs/blob/dev/specs/altair/light-client/p2p-interface.md#getlightclientbootstrap Disclaimer: I have no idea if people are using it but it shouldn't have been working so not sure why it wasn't caught Co-authored-by: realbigsean <seananderson33@gmail.com>
Issue Addressed
Protocol
here.Disclaimer: I have no idea if people are using it but it shouldn't have been working so not sure why it wasn't caught