-
Notifications
You must be signed in to change notification settings - Fork 715
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
Add explicit bounds for ssz decoding in rpc #1250
Conversation
* Update milagro_bls to new release Signed-off-by: Kirk Baird <baird.k@outlook.com> * Tidy up fake cryptos Signed-off-by: Kirk Baird <baird.k@outlook.com> * move SecretHash to bls and put plaintext back Signed-off-by: Kirk Baird <baird.k@outlook.com>
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.
This seems to build on top of #1837. I will wait for this to merge before reviewing
use types::{BeaconBlock, EthSpec, Hash256, MainnetEthSpec, Signature, SignedBeaconBlock}; | ||
|
||
lazy_static! { | ||
pub static ref SIGNED_BEACON_BLOCK_MIN: usize = SignedBeaconBlock::<MainnetEthSpec> { |
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.
@AgeManning is it okay here that I'm sorta hardcoding MainnetEthSpec
? I couldn't figure out how to keep it generic over EthSpec
.
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.
This looks fine to me.
Add a comment indicating we are doing this and that the MAX/MIN values are the same across each spec type
Issue Addressed
ethereum/consensus-specs#1800
Also contains spec updates from #1244 to avoid double work.
Proposed Changes
SignedBeaconBlock
andBlocksByRootRequest
)Note: The spec also mentions the following validation check
This PR does not contain this check as any length prefix > 10 bytes would error at the Uvi codec length decoding anyway. Adding this check explicitly will simply complicate the logic.