consensus/misc/eip4844: more changes for blob gas calculation #31128
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fjl
requested review from
MariusVanDerWijden,
holiman,
rjl493456442,
s1na and
lightclient
as code owners
lightclient
reviewed
Feb 4, 2025
| @@ -34,6 +34,9 @@ var ( | |||
| // if the current block contains no transactions, the excessBlobGas is updated | |||
| // accordingly. | |||
| func VerifyEIP4844Header(config *params.ChainConfig, parent, header *types.Header) error { | |||
| if header.Number.Uint64() != parent.Number.Uint64()+1 { | |||
| panic("bad header pair") | |||
| } | |||
There was a problem hiding this comment.
Why number? Why not check the parent hash matches up or timestamp is 12 greater?
There was a problem hiding this comment.
The hash would be more correct, but it is way more expensive. I'm adding this check because it's cheap. Timestamp could be checked but then we'd need to update it if there will ever be a change in slot length.
There was a problem hiding this comment.
This check is just supposed to catch cases where we pass the headers in the wrong order.
rjl493456442
approved these changes
Feb 5, 2025
tokeyg
pushed a commit
to tokeyg/go-ethereum
that referenced
this pull request
Feb 6, 2025
…um#31128) This PR changes the signature of `CalcExcessBlobGas` to take in just the header timestamp instead of the whole object. It also adds a sanity check for the parent->child block order to `VerifyEIP4844Header`.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As noted in the review of #31101, it's a bit weird to pass two headers to
CalcExcessBlobGas. We only use the second header to look up the fork timestamp, so I'm changing the function here to pass that timestamp. Most callers are not affected much, but it does make a difference in places like in tracetest which had to commit the genesis state and create a block just to get this header.Also adding a small sanity check for the parent<->child relationship in
VerifyEIP4844Header.