-
Notifications
You must be signed in to change notification settings - Fork 975
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
EIP-7742: Uncouple blob limits per block across CL and EL #3800
base: dev
Are you sure you want to change the base?
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.
strong lgtm from lodestar and ethereumjs team
0ed74fb
to
71f2912
Compare
From my perspective, this looks like the best option with which CL can run There is another constant |
I think I would add the target as well. Otherwise we have to set the target to be 1/2 of the limit, but being able to configure the target independently gives us more flexibility in the future. |
An alternative that may be more elegant and lets us remove some fields from the header: Only add the current blob gas price (to both execution header and the execution API). The consensus layer already has all the information to compute the correct blob gas price. |
I would favour maxBlobGasLimit (with target half of limit and price calc moved to something like this: https://github.com/ethereum/EIPs/pull/8343/files#diff-41459deea2791d37cd24d9139a73beacd8e7bad2aaaa194db237754a0b8951d6R56) and letting EL compute price using parent's excess blob gas. CL telling blob gas price and effectively moving gas calc to the CL breaks separation of concerns. Its (CL price) also not that ePBS compatible (it can still be done with extra bookkeeping) where payloads can be "missed" with beacon payloads still being part of chain (because of decoupling between beacon block and payload) and also with some (draft) EIPs which allows for gas compensation for missed blocks so that constant throughput can be targetted. |
If I am wondering if it is important for EL to know the Regardless of the |
It seems to me that |
specs/electra/beacon-chain.md
Outdated
# [Modified in Electra] | ||
if not self.notify_new_payload(execution_payload, | ||
parent_beacon_block_root, | ||
MAX_BLOBS_PER_BLOCK): |
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.
We already have the following checks in the process_execution_payload
:
# [New in Deneb:EIP4844] Verify commitments are under limit
assert len(body.blob_kzg_commitments) <= MAX_BLOBS_PER_BLOCK
# Verify the execution payload is valid
# [Modified in Deneb:EIP4844] Pass `versioned_hashes` to Execution Engine
# [Modified in Deneb:EIP4788] Pass `parent_beacon_block_root` to Execution Engine
versioned_hashes = [kzg_commitment_to_versioned_hash(commitment) for commitment in body.blob_kzg_commitments]
assert execution_engine.verify_and_notify_new_payload(
NewPayloadRequest(
execution_payload=payload,
versioned_hashes=versioned_hashes,
parent_beacon_block_root=state.latest_block_header.parent_root,
)
)
So the MAX_BLOBS_PER_BLOCK
is already implicitly validated by the CL by the mean of matching the actual versioned_hashes with the expected ones. Why the payload validation process requires MAX_BLOBS_PER_BLOCK
to be sent to EL explicitly?
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, ive been thinking about this wrt to how we want to handle this feature
as you point out, the current check for "max blob gas per block" in the EL is redundant when the CL and EL are moving in lock-step via the call to verify_and_notify_new_payload
we still have a concern here around optimistic sync and I think the simplest thing would be to include the blob limits in the EL header so that an execution layer block is self-contained
I intend to bring up on the CL call Thursday, I think we need to agree on the exact semantics of optimistic sync as I could imagine two cases:
- Make EL blocks self-contained, for verification purposes during syncing
- Have the CL drive the EL via this check it will already be doing and skip changing the EL block as all (beyond just removing the EL validation to ensure the max/target number of blobs)
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.
Versioned hashes check is similar to block hash validation (both checks the block structure), and the spec says that this check must be run instantly even during the sync process as it does not involve the world state. CL clients either send every payload to EL or run versioned hashes and block hash validations on their own if EL is syncing. So this check is executed over each payload, and the MAX_BLOBS_PER_BLOCK
shouldn’t be checked by the EL explicitly. So the block building would be the only place where this parameter must be passed to 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.
yep! I ended up reviewing this as well and it seems the maximum check on the EL is currently redundant
I think we remove the check for max from the EL and send over the target
I'll work on tidying up this PR and make an EIP to add the target value to the block header
updated the top comment description with the latest thinking here, along with a link to a WIP EIP; please see comment and the WIP EIP for the latest thinking around this feature |
1aa7488
to
c891283
Compare
c891283
to
093f5f3
Compare
This is the CL part of a change to remove the hard-coded blob count(s) from the execution layer.
Motivation here: https://hackmd.io/@ralexstokes/rJhC_vkSR#Uncertainty-around-deployment-timeline-and-cross-layer-concerns
EIP is here: ethereum/EIPs#8735
Engine API PR here: ethereum/execution-apis#574
This PR targets Pectra, and I will update to a spec "feature" if we decide against Pectra inclusion.