-
Notifications
You must be signed in to change notification settings - Fork 405
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 EIP-7594 (PeerDAS) related changes #630
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: fradamt <[email protected]>
Co-authored-by: fradamt <[email protected]>
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.
I’ve added a few suggestions to slightly improve readability and clarity—mostly minor points for precision. None of them are strictly necessary, so please feel free to accept or ignore them based on your preference!
Co-authored-by: Tei Im <[email protected]>
Co-authored-by: Tei Im <[email protected]>
|
||
#### Request | ||
|
||
* method: `engine_getBlobsV2` |
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.
i am not sure what the relevance of this method is in peerdas unless this is meant to provide support for distributed block building https://notes.ethereum.org/@dankrad/BkJMU8d0R in which case the request/response format will fully change
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.
Why would it change?
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.
if EL dosen't has all the blobs this is not useful or may be am I missing something? in distributed block building scenario the mempools will only sync their custody columns and there won't be a blob to return
so this api would essentially change towards fetching the column custodies from the EL
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.
Those are ideas for the future, we are not going to be sharding the mempool in Fusaka.
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.
right, then this will be useful only if we get all blobs from the EL if my understanding is correct?
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.
That is correct, and it could be used to do distributed blob publishing, happen on nodes who have all the blobs available locally
fix camel case and other small changes
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.
have a question / request for clarification on the eth_sendRawTransaction
semantics
src/engine/osaka.md
Outdated
- [BlobsBundleV1](#blobsbundlev1) | ||
- [BlobAndProofV1](#blobandproofv1) |
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.
- [BlobsBundleV1](#blobsbundlev1) | |
- [BlobAndProofV1](#blobandproofv1) | |
- [BlobsBundleV2](#blobsbundlev2) | |
- [BlobAndProofV2](#blobandproofv2) |
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 is where the latest change is, the idea is not to bump the version of BlobsBundle and BlobAndProof message, just directly adding a new field to it, it could help simplify implementation quite a bit, and also more adhering to idiomatic protobuf conventions.
I'm working on related beacon-APIs
, builder-specs
changes along with example PR (both on prysm / geth) side to showcase this.
Let me know if you think otherwise (we should do it like before with new message version)
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 is where the latest change is, the idea is not to bump the version of BlobsBundle and BlobAndProof message, just directly adding a new field to it, it could help simplify implementation quite a bit, and also more adhering to idiomatic protobuf conventions.
I see, it's not clear to me that this makes anything simpler, it seems like bumping a version is trivial, do you mind expanding on why this would be simpler? Is there an implementation that's made complex as a result of 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.
ah I see, this suggests not even adding a new method
engine_getPayloadV4
This might be fine then
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.
Yeah, not only does this simplify EL side, also on CL side it avoids quite a lot of new APIs catered just to a new BlobsBundle / BlobAndProof V2 with an additional field, same applies to builder side as well
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.
Agree with @g11tech. I think version bump is a bit cleaner (from the CL side) than having an extra field that's always going to be empty from Fulu.
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.
How do people feel about keeping the same field name (proofs
), but bumping the API version?
From the CL side, I feel like we could potentially get the best of both worlds with this - easy validations, while keeping the structure unchanged. Separately, we also have a BlobsBundle
in the Builder API, which is currently identical to this, and we could keep the SSZ encoding unchanged if the field remains the same (max length of list does not affect SSZ encoding).
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.
IMO, I prefer bump the version. instead of modify and/or add fields to existing types.
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.
strong preference on bumping the API versions
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.
After thinking about it, also much prefer the bump, since semantics are changing.
Summary
Introduces updates to rpc and engine API definitions for EIP-7594, changes defined in ethereum/EIPs#9378
Notes for reviewers
This PR introduces backward incompatible changes to
engine_getPayloadV4
, that returns newBlobsBundleV2
with cell proofs instead of blob proofs