From c3079692756b0646ff9c0a27c6e8915f723d8cb3 Mon Sep 17 00:00:00 2001 From: Sebastian Stammler Date: Wed, 20 Aug 2025 09:58:28 +0200 Subject: [PATCH] beacon/engine,eth/catalyst: Fix engine API checks and exec payload creation --- beacon/engine/types.go | 9 +- eth/catalyst/api_optimism.go | 46 ++++---- eth/catalyst/api_optimism_test.go | 168 +++++++++++++----------------- 3 files changed, 103 insertions(+), 120 deletions(-) diff --git a/beacon/engine/types.go b/beacon/engine/types.go index 5319a4d9c9..93182ea088 100644 --- a/beacon/engine/types.go +++ b/beacon/engine/types.go @@ -359,8 +359,13 @@ func BlockToExecutableData(block *types.Block, fees *big.Int, sidecars []*types. BlobGasUsed: block.BlobGasUsed(), ExcessBlobGas: block.ExcessBlobGas(), ExecutionWitness: block.ExecutionWitness(), - // OP-Stack addition: withdrawals list alone does not express the withdrawals storage-root. - WithdrawalsRoot: block.WithdrawalsRoot(), + } + + // OP-Stack: only Isthmus execution payloads must set the withdrawals root. + // They are guaranteed to not be the empty withdrawals hash, which is set pre-Isthmus (post-Canyon). + if wr := block.WithdrawalsRoot(); wr != nil && *wr != types.EmptyWithdrawalsHash { + wr := *wr + data.WithdrawalsRoot = &wr } // Add blobs. diff --git a/eth/catalyst/api_optimism.go b/eth/catalyst/api_optimism.go index 7addffefbc..b019966de7 100644 --- a/eth/catalyst/api_optimism.go +++ b/eth/catalyst/api_optimism.go @@ -5,38 +5,35 @@ import ( "github.com/ethereum/go-ethereum/beacon/engine" "github.com/ethereum/go-ethereum/consensus/misc/eip1559" - "github.com/ethereum/go-ethereum/core/types" "github.com/ethereum/go-ethereum/params" ) // checkOptimismPayload performs Optimism-specific checks on the payload data (called during [(*ConsensusAPI).newPayload]). func checkOptimismPayload(params engine.ExecutableData, cfg *params.ChainConfig) error { - // Canyon - if cfg.IsCanyon(params.Timestamp) && !cfg.IsIsthmus(params.Timestamp) { - if params.WithdrawalsRoot == nil || *params.WithdrawalsRoot != types.EmptyWithdrawalsHash { - return errors.New("withdrawalsRoot not equal to MPT root of empty list post-Canyon and pre-Isthmus") + // (non)-nil withdrawals is already checked by Shanghai rules. + // Canyon - empty withdrawals + if cfg.IsCanyon(params.Timestamp) { + if len(params.Withdrawals) != 0 { + return errors.New("non-empty withdrawals post-Canyon") } } - // Holocene + // Holocene - extraData if cfg.IsHolocene(params.Timestamp) { if err := eip1559.ValidateHoloceneExtraData(params.ExtraData); err != nil { return err } - } else { - if len(params.ExtraData) > 0 { - return errors.New("extraData must be empty before Holocene") - } + } else if len(params.ExtraData) > 0 { // pre-Holocene + return errors.New("extraData must be empty before Holocene") } - // Isthmus + // Isthmus - withdrawalsRoot if cfg.IsIsthmus(params.Timestamp) { - if params.Withdrawals == nil || len(params.Withdrawals) != 0 { - return errors.New("non-empty or nil withdrawals post-isthmus") - } if params.WithdrawalsRoot == nil { - return errors.New("nil withdrawalsRoot post-isthmus") + return errors.New("nil withdrawalsRoot post-Isthmus") } + } else if params.WithdrawalsRoot != nil { // pre-Isthmus + return errors.New("non-nil withdrawalsRoot pre-Isthmus") } return nil @@ -49,19 +46,24 @@ func checkOptimismPayloadAttributes(payloadAttributes *engine.PayloadAttributes, return errors.New("gasLimit parameter is required") } - // Holocene + // (non)-nil withdrawals is already checked by Shanghai rules. + // Canyon - empty withdrawals + if cfg.IsCanyon(payloadAttributes.Timestamp) { + if len(payloadAttributes.Withdrawals) != 0 { + return errors.New("non-empty withdrawals post-Canyon") + } + } + + // Holocene - extraData if cfg.IsHolocene(payloadAttributes.Timestamp) { if err := eip1559.ValidateHolocene1559Params(payloadAttributes.EIP1559Params); err != nil { return err } - } else if len(payloadAttributes.EIP1559Params) != 0 { - return errors.New("eip155Params not supported prior to Holocene upgrade") + } else if len(payloadAttributes.EIP1559Params) != 0 { // pre-Holocene + return errors.New("non-empty eip155Params pre-Holocene") } - // Isthmus - if cfg.IsIsthmus(payloadAttributes.Timestamp) && payloadAttributes.Withdrawals == nil || len(payloadAttributes.Withdrawals) != 0 { - return errors.New("non-empty or nil withdrawals post-isthmus") - } + // Note: PayloadAttributes don't contain the Isthmus withdrawalsRoot, it's set during block assembly. return nil } diff --git a/eth/catalyst/api_optimism_test.go b/eth/catalyst/api_optimism_test.go index 8b2252b4a6..ead528c79c 100644 --- a/eth/catalyst/api_optimism_test.go +++ b/eth/catalyst/api_optimism_test.go @@ -2,7 +2,6 @@ package catalyst import ( "errors" - "math" "testing" "github.com/ethereum/go-ethereum/beacon/engine" @@ -12,34 +11,32 @@ import ( "github.com/stretchr/testify/require" ) -func postCanyonPreIsthmus() *params.ChainConfig { +func preCanyon() *params.ChainConfig { cfg := new(params.ChainConfig) - cfg.CanyonTime = new(uint64) - future := uint64(math.MaxUint64) - cfg.IsthmusTime = &future return cfg } -func preHolocene() *params.ChainConfig { - cfg := new(params.ChainConfig) +func postCanyon() *params.ChainConfig { + cfg := preCanyon() + cfg.CanyonTime = new(uint64) return cfg } func postHolocene() *params.ChainConfig { - cfg := new(params.ChainConfig) + cfg := postCanyon() cfg.HoloceneTime = new(uint64) return cfg } func postIsthmus() *params.ChainConfig { - cfg := new(params.ChainConfig) - cfg.HoloceneTime = new(uint64) + cfg := postHolocene() cfg.IsthmusTime = new(uint64) return cfg } var valid1559Params = []byte{0, 1, 2, 3, 4, 5, 6, 7} var validExtraData = []byte{0, 1, 2, 3, 4, 5, 6, 7, 8} +var emptyWithdrawals = make([]*types.Withdrawal, 0) func TestCheckOptimismPayload(t *testing.T) { tests := []struct { @@ -49,56 +46,54 @@ func TestCheckOptimismPayload(t *testing.T) { expected error }{ { - name: "valid payload post-Canyon/pre-Isthmus", + name: "valid payload pre-Canyon", params: engine.ExecutableData{ - Timestamp: 0, - ExtraData: []byte{}, - WithdrawalsRoot: &types.EmptyWithdrawalsHash, + ExtraData: []byte{}, }, - cfg: postCanyonPreIsthmus(), + cfg: preCanyon(), + expected: nil, }, { - name: "nil withdrawalsRoot payload post-Canyon/pre-Isthmus", + name: "valid payload post-Canyon", params: engine.ExecutableData{ - Timestamp: 0, - ExtraData: []byte{}, - WithdrawalsRoot: nil, + ExtraData: []byte{}, + Withdrawals: emptyWithdrawals, }, - cfg: postCanyonPreIsthmus(), - expected: errors.New("withdrawalsRoot not equal to MPT root of empty list post-Canyon and pre-Isthmus"), - }, { - name: "incorrect withdrawalsRoot payload post-Canyon/pre-Isthmus", + cfg: postCanyon(), + }, + { + name: "invalid empty withdrawals post-Canyon", params: engine.ExecutableData{ - Timestamp: 0, - ExtraData: []byte{}, - WithdrawalsRoot: &(types.EmptyVerkleHash), + ExtraData: []byte{}, + Withdrawals: make([]*types.Withdrawal, 1), }, - cfg: postCanyonPreIsthmus(), - expected: errors.New("withdrawalsRoot not equal to MPT root of empty list post-Canyon and pre-Isthmus"), + cfg: postCanyon(), + expected: errors.New("non-empty withdrawals post-Canyon"), }, { - name: "valid payload pre-Holocene", + name: "non-nil withdrawalsRoot pre-Isthmus", params: engine.ExecutableData{ - Timestamp: 0, - ExtraData: []byte{}, + ExtraData: []byte{}, + Withdrawals: emptyWithdrawals, + WithdrawalsRoot: &types.EmptyWithdrawalsHash, }, - cfg: preHolocene(), - expected: nil, + cfg: postCanyon(), + expected: errors.New("non-nil withdrawalsRoot pre-Isthmus"), }, { - name: "invalid payload pre-Holocene with extraData", + name: "invalid non-empty extraData pre-Holocene", params: engine.ExecutableData{ - Timestamp: 0, - ExtraData: []byte{1, 2, 3}, + ExtraData: []byte{1, 2, 3}, + Withdrawals: emptyWithdrawals, }, - cfg: preHolocene(), + cfg: postCanyon(), expected: errors.New("extraData must be empty before Holocene"), }, { - name: "invalid payload pre-Holocene with extraData", + name: "invalid extraData post-Holocene", params: engine.ExecutableData{ - Timestamp: 0, - ExtraData: []byte{1, 2, 3}, + ExtraData: []byte{1, 2, 3}, + Withdrawals: emptyWithdrawals, }, cfg: postHolocene(), expected: errors.New("holocene extraData should be 9 bytes, got 3"), @@ -106,41 +101,40 @@ func TestCheckOptimismPayload(t *testing.T) { { name: "valid payload post-Holocene with extraData", params: engine.ExecutableData{ - Timestamp: 0, - ExtraData: validExtraData, + ExtraData: validExtraData, + Withdrawals: emptyWithdrawals, }, cfg: postHolocene(), expected: nil, }, { - name: "nil withdrawals post-isthmus", + name: "invalid non-nil withdrawalsRoot post-Holocene", params: engine.ExecutableData{ - Timestamp: 0, - Withdrawals: nil, - ExtraData: validExtraData, + ExtraData: validExtraData, + Withdrawals: emptyWithdrawals, + WithdrawalsRoot: &types.EmptyWithdrawalsHash, }, - cfg: postIsthmus(), - expected: errors.New("non-empty or nil withdrawals post-isthmus"), + cfg: postHolocene(), + expected: errors.New("non-nil withdrawalsRoot pre-Isthmus"), }, { - name: "non-empty withdrawals post-isthmus", + name: "valid payload post-Isthmus", params: engine.ExecutableData{ - Timestamp: 0, - Withdrawals: make([]*types.Withdrawal, 1), - ExtraData: validExtraData, + ExtraData: validExtraData, + Withdrawals: emptyWithdrawals, + WithdrawalsRoot: &types.EmptyWithdrawalsHash, }, cfg: postIsthmus(), - expected: errors.New("non-empty or nil withdrawals post-isthmus"), + expected: nil, }, { - name: "nil withdrawals root post-isthmus", + name: "invalid nil withdrawals root post-isthmus", params: engine.ExecutableData{ - Timestamp: 0, - Withdrawals: make([]*types.Withdrawal, 0), + Withdrawals: emptyWithdrawals, ExtraData: validExtraData, }, cfg: postIsthmus(), - expected: errors.New("nil withdrawalsRoot post-isthmus"), + expected: errors.New("nil withdrawalsRoot post-Isthmus"), }, } @@ -165,80 +159,62 @@ func TestCheckOptimismPayloadAttributes(t *testing.T) { shouldPanic bool }{ { - name: "nil payload attributes", + name: "nil payload attributes panic", payloadAttributes: nil, - cfg: preHolocene(), + cfg: preCanyon(), shouldPanic: true, }, { - name: "valid payload attributes pre-Holocene", + name: "valid payload attributes pre-Canyon", payloadAttributes: &engine.PayloadAttributes{ - Timestamp: 0, - GasLimit: new(uint64), + GasLimit: new(uint64), }, - cfg: preHolocene(), + cfg: preCanyon(), expected: nil, }, { - name: "invalid payload attributes pre-Holocene with gasLimit", + name: "invalid nil gasLimit", payloadAttributes: &engine.PayloadAttributes{ - Timestamp: 0, - GasLimit: nil, + GasLimit: nil, }, - cfg: preHolocene(), + cfg: preCanyon(), expected: errors.New("gasLimit parameter is required"), }, { - name: "invalid payload attributes pre-Holocene with eip1559Params", - payloadAttributes: &engine.PayloadAttributes{ - Timestamp: 0, - GasLimit: new(uint64), - EIP1559Params: valid1559Params, - }, - cfg: preHolocene(), - expected: errors.New("eip155Params not supported prior to Holocene upgrade"), - }, - { - name: "valid payload attributes post-Holocene with gasLimit", + name: "invalid non-empty withdrawals post-Canyon", payloadAttributes: &engine.PayloadAttributes{ - Timestamp: 0, GasLimit: new(uint64), EIP1559Params: valid1559Params, + Withdrawals: make([]*types.Withdrawal, 1), }, - cfg: postHolocene(), - expected: nil, + cfg: postCanyon(), + expected: errors.New("non-empty withdrawals post-Canyon"), }, { - name: "non-empty withdrawals post-isthmus", + name: "invalid non-empty eip1559Params pre-Holocene", payloadAttributes: &engine.PayloadAttributes{ - Timestamp: 0, GasLimit: new(uint64), EIP1559Params: valid1559Params, - Withdrawals: make([]*types.Withdrawal, 1), }, - cfg: postIsthmus(), - expected: errors.New("non-empty or nil withdrawals post-isthmus"), + cfg: postCanyon(), + expected: errors.New("non-empty eip155Params pre-Holocene"), }, { - name: "nil withdrawals post-isthmus", + name: "invalid eip1559Params post-Holocene", payloadAttributes: &engine.PayloadAttributes{ - Timestamp: 0, GasLimit: new(uint64), - EIP1559Params: valid1559Params, - Withdrawals: nil, + EIP1559Params: append(valid1559Params, 77), }, - cfg: postIsthmus(), - expected: errors.New("non-empty or nil withdrawals post-isthmus"), + cfg: postHolocene(), + expected: errors.New("holocene eip-1559 params should be 8 bytes, got 9"), }, { - name: "empty withdrawals post-isthmus", + name: "valid payload attributes post-Holocene", payloadAttributes: &engine.PayloadAttributes{ - Timestamp: 0, GasLimit: new(uint64), EIP1559Params: valid1559Params, - Withdrawals: make([]*types.Withdrawal, 0), }, - cfg: postIsthmus(), + cfg: postHolocene(), expected: nil, }, }