Skip to content
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

consensus, core, eth/downloader: add 4844 excessDataGas validations #27340

Closed
wants to merge 6 commits into from

Conversation

karalabe
Copy link
Member

This PR implements validating the excessDataGas field of Cancun headers.

The validations are significantly more complex than previous header validations because the excessDataGas field combines two notions into one: the price for data blobs and the gas used by data blobs. To validate the combo field, we need access to both the parent's excessDataGas as well as the current block's transaction list (blob count). This makes validation more convoluted in both the chain's block processor as well as the downloader, which both needs to juggle two entities (parent header, current block) from now on.

@karalabe karalabe added this to the 1.12.1 milestone May 24, 2023
@karalabe karalabe mentioned this pull request May 24, 2023
26 tasks
@karalabe karalabe changed the title consensus, core, eth/downloader: add 4844 excessDataGas validations consensus, core, eth/downloader: add 4844 excessDataGas and blob validations May 24, 2023
@karalabe karalabe changed the title consensus, core, eth/downloader: add 4844 excessDataGas and blob validations consensus, core, eth/downloader: add 4844 excessDataGa validations May 24, 2023
@karalabe karalabe changed the title consensus, core, eth/downloader: add 4844 excessDataGa validations consensus, core, eth/downloader: add 4844 excessDataGas validations May 24, 2023
Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but we need to do a couple of syncs to verify that the changes in eth/downloader are ok

consensus/misc/eip4844.go Outdated Show resolved Hide resolved
consensus/misc/eip4844.go Outdated Show resolved Hide resolved
// Verify the excessDataGas is correct based on the parent header iff the
// transaction list is empty. For non-empty blocks, validation needs to be
// done later.
if header.TxHash == types.EmptyTxsHash {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems pretty rare this code path would actually be triggered, what is benefit of fast failing here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interestingly, the downloader has an optimisation that it only requests block bodies if they are not empty. If they are empty, then the bodies aren't requested and just pre-filled to zero.

Buuut, 4844's excessDataGas field needs the bodies to be verified. Since the downloader just blindly fills teh transaction list as empty in those cases, we have two options:

  • Have the excessDataGas already validated like all other header fields (what this check does)
  • Introduce header validation pathways into the "fill empty block" mechanism of the downloader

The second thing seemed wonkier as it would need changes that touch a number of unintuitive spots in the code.

But in all honesty, this change is also not that intuitive, it just seemed nicer.

Alternatively we could do something akin to the "gasUsed" which is completely ignored during snap sync and just assumed to be correct whatever it is, but here we not only have "blobsUsed" but also "pricing" so it seemed more relevant to try and validate it.

TL;DR: These are all fallouts of that convoluted field.

Copy link
Member

@MariusVanDerWijden MariusVanDerWijden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Co-authored-by: Marius van der Wijden <m.vanderwijden@live.de>
@karalabe
Copy link
Member Author

Superseded by #27382 according to ethereum/EIPs#7062

@karalabe karalabe closed this May 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants