-
Notifications
You must be signed in to change notification settings - Fork 724
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
Verify inclusion proof should not be fallible #5787
Conversation
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.
Makes sense to me
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.
Looks good! I think this is a low risk change if tested (as things would just blow up if this is doesn't work) and should be good to merge.
I wrote some tests here and they pass on your branch:
dapplion#33
* Add blob sidecar inclusion test. * Fix lint
@mergify queue |
✅ The pull request has been merged automaticallyThe pull request has been merged automatically at 474c1b4 |
Issue Addressed
Adding the verify inclusion proof function for data columns I noticed that
verify_blob_sidecar_inclusion_proof
is fallible due to the need to do math with the EthSpec variables.Proposed Changes
Since the EthSpec values are non-configurable and properly chosen, it's not necessary to make
verify_blob_sidecar_inclusion_proof
fallible. A unit test in EthSpec ensures that the values are correctly chosen and then we can safely expect.