-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Update EIP-4844: Refactor Validity Conditions #6583
Update EIP-4844: Refactor Validity Conditions #6583
Conversation
✅ All reviewers have approved. |
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 refactor! we need to make sure the actual substantive changes here are highlighted. maybe list them very explicitly in the PR header comment.
I left a number of nits, some of which are not directly related to your changes
EIPS/eip-4844.md
Outdated
the `TransactionNetworkPayload` version of the transaction also includes `blobs` and `kzgs` (commitments list). | ||
The execution layer verifies the wrapper validity against the inner `TransactionPayload` after signature verification as: | ||
|
||
- All hashes in `blob_versioned_hashes` must start with the byte `BLOB_COMMITMENT_VERSION_KZG` |
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 suggest unifying the naming of BLOB_COMMITMENT_VERSION_KZG
(in EIP) with VERSIONED_HASH_VERSION_KZG
(in CL specs)
I think we can close this. Resolved via #6863 |
f5b2761
to
685af00
Compare
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 work! I think we should definitely specify the blob limit this way
I left one comment that would help clarify although it could be obvious how the new checks interact with the old checks in which case we can disregard
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.
remove dash in EL and CL
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 after Alex's feedback is addressed
updated the PR description to reflect all changes made. |
e9420f9
to
531784c
Compare
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.
All Reviewers Have Approved; Performing Automatic Merge...
Add a separate execution-layer validation section with the block validity rule changes.
Detailed list of changes:
calc_excess_data_gas()
There is still an open question as to how to handle the relationship between data gas limit and CL max blobs per block limit, however this is considered out of scope for this PR.