-
Notifications
You must be signed in to change notification settings - Fork 378
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
Define engine_getBlobsV1
#559
Conversation
* Fix spell check * Fix typo and formatting
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.
A few nits, otherwise, looks good to me!
Co-authored-by: Mikhail Kalinin <noblesse.knight@gmail.com>
Co-authored-by: Mikhail Kalinin <noblesse.knight@gmail.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.
LGTM!
Would it be possible to query by tx hash instead of blob version hashes? EL generally store by tx hash. We can add another mapping but it will complicate things a bit. Or pairs of txhash+blobversionhashes. EDIT: decided on ACCD that CL doesn't have the tx hashes. |
Is there a reason this should be in the engine API vs the regular eth api? I lean towards the latter. |
@lightclient i disagree. i dont see why this should move to eth API. it will only be used by CL clients. I dont see any other usage. Additionally, it is way better to keep all methods used by CL in the engine API. i hate the fact that they have to use eth API for other things (like eth_getLogs etc) |
This is my analysis of an actual case that could have been avoided by a good adoption of this PR I think we should prioritize this |
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.
lgtm
Engine API specification for a new optional
getBlobsV1
method.The purpose of this method is to allow consensus clients to fetch blobs from the EL blob pool, so that:
Co-authored with @jimmygchen