beacon/engine, eth/catalyst, miner: 4788 CL/EL protocol updates #27872

holiman · 2023-08-07T11:18:09Z

This PR makes EIP-4788 work in the engine API and miner. It also fixes some bugs related to 4844 block processing and mining. Changes in detail:

Header.BeaconRoot has been renamed to ParentBeaconRoot.
The engine API now implements forkchoiceUpdatedV3
newPayloadV3 method has been updated with the parentBeaconBlockRoot parameter
the beacon root is now applied to new blocks in miner
For EIP-4844, block creation now updates the blobGasUsed field of the header

This is a follow-up to #27849, adding the beaconroot to the messages passed between CL and EL. I split this up into a separate PR since it is backwards-incompatible, and will start rejecting cancun-updates from the CL which do not contain a beaconroot.

This also adds the beaconroot to the miner.

Note, there's one sentence in the 4788 eip which I do not fully understand (cc @ralexstokes

When verifying a block, execution clients MUST ensure the root value in the block header matches the one provided by the consensus client.

Exactly when should what be verified against what?

ralexstokes · 2023-08-09T03:16:13Z

Exactly when should what be verified against what?

the intent is to make sure that the root coming from the engine API is the one that matches the EL block header

I think w/ the latest engine API updates, the EL block header will be constructed (if necessary) using the engine API data so it will be default be valid

the other set of cases where the roots need to be checked is syncing

holiman · 2023-08-23T07:50:13Z

This is now ready for review, cc @lightclient @fjl

fjl · 2023-08-23T21:55:28Z

I'll put #27993 in first, because it makes it easier to get the new parameter into the core of the miner.

fjl · 2023-08-24T00:28:00Z

miner/worker.go

@@ -935,6 +939,18 @@ func (w *worker) prepareWork(genParams *generateParams) (*environment, error) {
 		log.Error("Failed to create sealing context", "err", err)
 		return nil, err
 	}
+	if w.chainConfig.IsCancun(header.Number, header.Time) {
+		if header.BeaconRoot == nil {


I think we need to decide if this is the right place for this check. We could check it earlier, i.e. just require that the beaconRoot be submitted. However, checking it here has the advantage that the consensus engine can override it. This will mostly be useful for downstream projects, who might want to set this there.

So the options are ...

Leave it as is,

Remove the check (potentially getting a nil deref on core.ProcessBeaconBlockRoot(*header.BeaconRoot,

Change the check to use common.Hash{} if it is nil,

Change the check to not ProcessBeaconBlockRoot at all, if it is nil

But perhaps you were thinking of something else?

I mean, whatever we somewhere else, if we deref a pointer here, we should check it first, so IMO the only question is if we barge ahead or back out.

Out of the options I listed, I'd prefer 1, possibly 4 for flexibility, but that's the kind of flexibility which can bite us later on.

I decided to go with (4), for now.

fjl · 2023-08-25T14:15:34Z

@holiman PTAL!

holiman

LGTM

…i surface miner, beacon/engine: process systemtx, update json tag

…thereum#27872) This PR makes EIP-4788 work in the engine API and miner. It also fixes some bugs related to EIP-4844 block processing and mining. Changes in detail: - Header.BeaconRoot has been renamed to ParentBeaconRoot. - The engine API now implements forkchoiceUpdatedV3 - newPayloadV3 method has been updated with the parentBeaconBlockRoot parameter - beacon root is now applied to new blocks in miner - For EIP-4844, block creation now updates the blobGasUsed field of the header

…pdates (ethereum#27872)" This reverts commit 8398b70.

… param checks (#28230) This PR introduces a few changes with respect to payload verification in fcu and new payload requests: * First of all, it undoes the `verifyPayloadAttributes(..)` simplification I attempted in #27872. * Adds timestamp validation to fcu payload attributes [as required](https://github.com/ethereum/execution-apis/blob/main/src/engine/cancun.md#specification-1) (section 2) by the Engine API spec. * For the new payload methods, I also update the verification of the executable data. For `newPayloadV2`, it does not currently ensure that cancun values are `nil`. Which could make it possible to submit cancun payloads through it. * On `newPayloadV3` the same types of checks are added. All shanghai and cancun related fields in the executable data must be non-nil, with the addition that the timestamp is _only_ with cancun. * Finally it updates a newly failing catalyst test to call the correct fcu and new payload methods depending on the fork.

… param checks (ethereum#28230) This PR introduces a few changes with respect to payload verification in fcu and new payload requests: * First of all, it undoes the `verifyPayloadAttributes(..)` simplification I attempted in ethereum#27872. * Adds timestamp validation to fcu payload attributes [as required](https://github.com/ethereum/execution-apis/blob/main/src/engine/cancun.md#specification-1) (section 2) by the Engine API spec. * For the new payload methods, I also update the verification of the executable data. For `newPayloadV2`, it does not currently ensure that cancun values are `nil`. Which could make it possible to submit cancun payloads through it. * On `newPayloadV3` the same types of checks are added. All shanghai and cancun related fields in the executable data must be non-nil, with the addition that the timestamp is _only_ with cancun. * Finally it updates a newly failing catalyst test to call the correct fcu and new payload methods depending on the fork.

beacon/engine, eth/catalyst, miner: EIP-4788 CL/EL protocol updates (ethereum#27872) Conflicts: eth/catalyst/api.go callSite to IsShanghai moved. New callsirte also calls IsCancun. I solved the conflict by assuming IsCancun will also accept ArbOS, without modifying IsCancun itself - this will be done in the next commit to keep things separate and easier to review. miner/worker.go callsite to ApplyTransaction moved, we have a change that adds a new (here ignored) value

holiman changed the title ~~4788 part2~~ 4788 CL/EL protocol updates Aug 7, 2023

holiman added the cancun label Aug 14, 2023

holiman mentioned this pull request Aug 14, 2023

core, params, beacon/engine: implement EIP 4788 BeaconRoot #27849

Merged

holiman force-pushed the 4788_part2 branch 3 times, most recently from 14e9932 to 03a9b09 Compare August 17, 2023 06:43

holiman mentioned this pull request Aug 17, 2023

cancun-devnet-8 tracker #27925

Closed

holiman force-pushed the 4788_part2 branch 2 times, most recently from 06b0626 to 445fce3 Compare August 22, 2023 12:44

holiman marked this pull request as ready for review August 22, 2023 12:54

holiman requested a review from gballet as a code owner August 22, 2023 12:54

holiman changed the title ~~4788 CL/EL protocol updates~~ beacon/engine, miner: 4788 CL/EL protocol updates Aug 22, 2023

fjl force-pushed the 4788_part2 branch from 445fce3 to f0d666a Compare August 23, 2023 23:09

fjl reviewed Aug 24, 2023

View reviewed changes

holiman commented Aug 25, 2023

View reviewed changes

holiman and others added 7 commits August 25, 2023 18:10

beacon/engine, eth/catalyst, miner: add beaconroot to miner and CL ap…

14eb33e

…i surface miner, beacon/engine: process systemtx, update json tag

eth/catalyst: add forkchoiceUpdatedV3

a66a284

miner: improve blob transaction logic

9f2a435

eth/catalyst: add newPayloadV3

010d228

eth/catalyst: remove print

3f2219e

miner: simplify check for BeaconRoot

48bebc9

miner: assign header fields in usual order

0d2ec8e

fjl force-pushed the 4788_part2 branch from b0bcca8 to 0d2ec8e Compare August 25, 2023 16:10

fjl added 3 commits August 25, 2023 22:27

core: temporarily change genesis block beacon root for tests

bd7d2fd

core: put back beaconRoot in genesis

e923d73

internal/ethapi: return parentBeaconBlockRoot

f1bd467

fjl requested review from karalabe and rjl493456442 as code owners August 25, 2023 20:31

lightclient and others added 4 commits August 25, 2023 17:01

core: add blob gas info to receipt during apply tx

3be5755

eth/catalyst: fix validity check for new payload v3

ade495d

core: explain why genesis has no parentBeaconBlockRoot

d51660b

all: rename BeaconRoot -> ParentBeaconRoot

22adccb

fjl changed the title ~~beacon/engine, miner: 4788 CL/EL protocol updates~~ beacon/engine, eth/catalyst, miner: 4788 CL/EL protocol updates Aug 26, 2023

eth/catalyst: fix error in test

8f98b56

fjl merged commit 6aa88cc into ethereum:master Aug 26, 2023

fjl added this to the 1.13.0 milestone Aug 26, 2023

karalabe added the EIP-4844 label Sep 12, 2023

lightclient mentioned this pull request Sep 29, 2023

eth/catalyst: add timestamp checks to fcu and new payload and improve param checks #28230

Merged

holiman deleted the 4788_part2 branch October 11, 2023 07:26

devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023

Revert "beacon/engine, eth/catalyst, miner: EIP-4788 CL/EL protocol u…

7daafba

…pdates (ethereum#27872)" This reverts commit 8398b70.

devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023

Revert "beacon/engine, eth/catalyst, miner: EIP-4788 CL/EL protocol u…

b929da0

…pdates (ethereum#27872)" This reverts commit 8398b70.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

beacon/engine, eth/catalyst, miner: 4788 CL/EL protocol updates #27872

beacon/engine, eth/catalyst, miner: 4788 CL/EL protocol updates #27872

holiman commented Aug 7, 2023 •

edited by fjl

Loading

ralexstokes commented Aug 9, 2023

holiman commented Aug 23, 2023

fjl commented Aug 23, 2023

fjl Aug 24, 2023

holiman Aug 24, 2023

fjl Aug 25, 2023 •

edited

Loading

fjl commented Aug 25, 2023

holiman left a comment

beacon/engine, eth/catalyst, miner: 4788 CL/EL protocol updates #27872

beacon/engine, eth/catalyst, miner: 4788 CL/EL protocol updates #27872

Conversation

holiman commented Aug 7, 2023 • edited by fjl Loading

ralexstokes commented Aug 9, 2023

holiman commented Aug 23, 2023

fjl commented Aug 23, 2023

fjl Aug 24, 2023

Choose a reason for hiding this comment

holiman Aug 24, 2023

Choose a reason for hiding this comment

fjl Aug 25, 2023 • edited Loading

Choose a reason for hiding this comment

fjl commented Aug 25, 2023

holiman left a comment

Choose a reason for hiding this comment

holiman commented Aug 7, 2023 •

edited by fjl

Loading

fjl Aug 25, 2023 •

edited

Loading