-
Notifications
You must be signed in to change notification settings - Fork 616
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 count between CL and EL #1899
Conversation
pub fn from_parent_and_target( | ||
parent_excess_blob_gas: u64, | ||
parent_blob_gas_used: u64, | ||
target_blob_gas_per_block: u64, | ||
) -> Self { |
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 believe this should use parent's target_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.
I don't think so, target_blob_per_block
is what is received from CL when you want to build a new block, if this is something that we fetch from parent block there wouldn't be extensions to Engine API https://eips.ethereum.org/EIPS/eip-7742
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 value is set to the current target blob count. The Engine API is modified along with this EIP to provide the target_blobs_per_block with each payload and implementations can use this value to correctly set the block header field.
from EIP
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, it's received from CL to populate the current block
but to calculate the excess blob gas I'd expect us to use the parent's target as the excess is calculated relative to the parent's usage
however I'm unsure about this
seems that in the current wip PR for geth the current target blob count is used https://github.com/lightclient/go-ethereum/pull/67/files#diff-1e0f75e568cb72ba19ee3b24dfd7f0ed05c9980e8fef61b14a95c4e09b80b08cR64
however, in one of the recent spec update PRs parent's target was used https://github.com/ethereum/EIPs/pull/8994/files#diff-fd7433dcf9e3427288829925409a7164a9b7b663a8d608c43a4d3c0c592ac4e2R89
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.
spec-tests repo version is also using parent's blob count https://github.com/ethereum/execution-spec-tests/blob/77552cc5c2fb6bcda1b207eec81f8e635fc37885/tests/prague/eip7742_uncouple_blob_count/spec.py#L139
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.
Good resource, will ask in discord for clarification, maybe it is a parent
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.
Tests are made with current value: https://discord.com/channels/595666850260713488/892088344438255616/1314676451089580032
Will merge this as is as the change is only in the variable name in Revm.
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.
Besu uses parent, it feels a little bit nicer, so will rename it.
The only meaningful change is that the
calc_excess_blob_gas
func now takestarget_blob_gas_per_block
as input.Check to make sure that tx blob num is smaller than the block blob num is removed, this needs to be checked on block level.