Update EIP-4844: Refactor Validity Conditions#6583
Conversation
|
✅ All reviewers have approved. |
djrtwo
left a comment
There was a problem hiding this comment.
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.
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
ralexstokes
left a comment
There was a problem hiding this comment.
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
lightclient
left a comment
There was a problem hiding this comment.
LGTM after Alex's feedback is addressed
|
updated the PR description to reflect all changes made. |
e9420f9 to
531784c
Compare
eth-bot
left a comment
There was a problem hiding this comment.
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.