diff --git a/eth/catalyst/api.go b/eth/catalyst/api.go index eece31cadc..677e59bdd2 100644 --- a/eth/catalyst/api.go +++ b/eth/catalyst/api.go @@ -28,7 +28,6 @@ import ( "github.com/ethereum/go-ethereum/beacon/engine" "github.com/ethereum/go-ethereum/common" "github.com/ethereum/go-ethereum/common/hexutil" - "github.com/ethereum/go-ethereum/consensus/misc/eip1559" "github.com/ethereum/go-ethereum/core" "github.com/ethereum/go-ethereum/core/rawdb" "github.com/ethereum/go-ethereum/core/stateless" @@ -319,6 +318,14 @@ func (api *ConsensusAPI) forkchoiceUpdated(update engine.ForkchoiceStateV1, payl log.Warn("Forkchoice requested update to zero hash") return engine.STATUS_INVALID, nil // TODO(karalabe): Why does someone send us this? } + + // OP-Stack diff payload attributes validation: + if cfg := api.eth.BlockChain().Config(); cfg.IsOptimism() && payloadAttributes != nil { + if err := checkOptimismPayloadAttributes(payloadAttributes, cfg); err != nil { + return engine.STATUS_INVALID, engine.InvalidPayloadAttributes.With(err) + } + } + // Stash away the last update to warn the user if the beacon client goes offline api.lastForkchoiceLock.Lock() api.lastForkchoiceUpdate = time.Now() @@ -434,19 +441,9 @@ func (api *ConsensusAPI) forkchoiceUpdated(update engine.ForkchoiceStateV1, payl if payloadAttributes != nil { var eip1559Params []byte - if api.eth.BlockChain().Config().Optimism != nil { - if payloadAttributes.GasLimit == nil { - return engine.STATUS_INVALID, engine.InvalidPayloadAttributes.With(errors.New("gasLimit parameter is required")) - } - if api.eth.BlockChain().Config().IsHolocene(payloadAttributes.Timestamp) { - if err := eip1559.ValidateHolocene1559Params(payloadAttributes.EIP1559Params); err != nil { - return engine.STATUS_INVALID, engine.InvalidPayloadAttributes.With(err) - } - eip1559Params = bytes.Clone(payloadAttributes.EIP1559Params) - } else if len(payloadAttributes.EIP1559Params) != 0 { - return engine.STATUS_INVALID, - engine.InvalidPayloadAttributes.With(errors.New("eip155Params not supported prior to Holocene upgrade")) - } + if api.eth.BlockChain().Config().IsOptimismHolocene(payloadAttributes.Timestamp) { + // Validation performed above in checkOptimismPayloadAttributes + eip1559Params = bytes.Clone(payloadAttributes.EIP1559Params) } transactions := make(types.Transactions, 0, len(payloadAttributes.Transactions)) for i, otx := range payloadAttributes.Transactions { @@ -663,10 +660,6 @@ func (api *ConsensusAPI) NewPayloadV4(params engine.ExecutableData, versionedHas return engine.PayloadStatusV1{Status: engine.INVALID}, engine.UnsupportedFork.With(errors.New("newPayloadV4 must only be called for prague payloads")) } - if api.eth.BlockChain().Config().IsIsthmus(params.Timestamp) && params.WithdrawalsRoot == nil { - return engine.PayloadStatusV1{Status: engine.INVALID}, engine.InvalidParams.With(errors.New("nil withdrawalsRoot post-isthmus")) - } - requests := convertRequests(executionRequests) if err := validateRequests(requests); err != nil { return engine.PayloadStatusV1{Status: engine.INVALID}, engine.InvalidParams.With(err) @@ -870,15 +863,11 @@ func (api *ConsensusAPI) newPayload(params engine.ExecutableData, versionedHashe // Hence, we use a lock here, to be sure that the previous call has finished before we // check whether we already have the block locally. - // OP-Stack diff: payload must have empty extraData before Holocene and hold eip-1559 params after Holocene. - if cfg := api.eth.BlockChain().Config(); cfg.IsHolocene(params.Timestamp) { - if err := eip1559.ValidateHoloceneExtraData(params.ExtraData); err != nil { + // OP-Stack diff payload validation: + if cfg := api.eth.BlockChain().Config(); cfg.IsOptimism() { + if err := checkOptimismPayload(params, cfg); err != nil { return api.invalid(err, nil), nil } - } else if cfg.IsOptimism() { - if len(params.ExtraData) > 0 { - return api.invalid(errors.New("extraData must be empty before Holocene"), nil), nil - } } api.newPayloadLock.Lock() diff --git a/eth/catalyst/api_optimism.go b/eth/catalyst/api_optimism.go new file mode 100644 index 0000000000..7addffefbc --- /dev/null +++ b/eth/catalyst/api_optimism.go @@ -0,0 +1,67 @@ +package catalyst + +import ( + "errors" + + "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") + } + } + + // Holocene + 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") + } + } + + // Isthmus + 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 nil +} + +// checkOptimismPayloadAttributes performs Optimism-specific checks on the payload attributes (called during [(*ConsensusAPI).forkChoiceUpdated]. +// Will panic if payloadAttributes is nil. +func checkOptimismPayloadAttributes(payloadAttributes *engine.PayloadAttributes, cfg *params.ChainConfig) error { + if payloadAttributes.GasLimit == nil { + return errors.New("gasLimit parameter is required") + } + + // Holocene + 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") + } + + // Isthmus + if cfg.IsIsthmus(payloadAttributes.Timestamp) && payloadAttributes.Withdrawals == nil || len(payloadAttributes.Withdrawals) != 0 { + return errors.New("non-empty or nil withdrawals post-isthmus") + } + + return nil +} diff --git a/eth/catalyst/api_optimism_test.go b/eth/catalyst/api_optimism_test.go new file mode 100644 index 0000000000..8b2252b4a6 --- /dev/null +++ b/eth/catalyst/api_optimism_test.go @@ -0,0 +1,279 @@ +package catalyst + +import ( + "errors" + "math" + "testing" + + "github.com/ethereum/go-ethereum/beacon/engine" + "github.com/ethereum/go-ethereum/common" + "github.com/ethereum/go-ethereum/core/types" + "github.com/ethereum/go-ethereum/params" + "github.com/stretchr/testify/require" +) + +func postCanyonPreIsthmus() *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) + return cfg +} + +func postHolocene() *params.ChainConfig { + cfg := new(params.ChainConfig) + cfg.HoloceneTime = new(uint64) + return cfg +} + +func postIsthmus() *params.ChainConfig { + cfg := new(params.ChainConfig) + cfg.HoloceneTime = new(uint64) + 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} + +func TestCheckOptimismPayload(t *testing.T) { + tests := []struct { + name string + params engine.ExecutableData + cfg *params.ChainConfig + expected error + }{ + { + name: "valid payload post-Canyon/pre-Isthmus", + params: engine.ExecutableData{ + Timestamp: 0, + ExtraData: []byte{}, + WithdrawalsRoot: &types.EmptyWithdrawalsHash, + }, + cfg: postCanyonPreIsthmus(), + }, + { + name: "nil withdrawalsRoot payload post-Canyon/pre-Isthmus", + params: engine.ExecutableData{ + Timestamp: 0, + ExtraData: []byte{}, + WithdrawalsRoot: nil, + }, + 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", + params: engine.ExecutableData{ + Timestamp: 0, + ExtraData: []byte{}, + WithdrawalsRoot: &(types.EmptyVerkleHash), + }, + cfg: postCanyonPreIsthmus(), + expected: errors.New("withdrawalsRoot not equal to MPT root of empty list post-Canyon and pre-Isthmus"), + }, + { + name: "valid payload pre-Holocene", + params: engine.ExecutableData{ + Timestamp: 0, + ExtraData: []byte{}, + }, + cfg: preHolocene(), + expected: nil, + }, + { + name: "invalid payload pre-Holocene with extraData", + params: engine.ExecutableData{ + Timestamp: 0, + ExtraData: []byte{1, 2, 3}, + }, + cfg: preHolocene(), + expected: errors.New("extraData must be empty before Holocene"), + }, + { + name: "invalid payload pre-Holocene with extraData", + params: engine.ExecutableData{ + Timestamp: 0, + ExtraData: []byte{1, 2, 3}, + }, + cfg: postHolocene(), + expected: errors.New("holocene extraData should be 9 bytes, got 3"), + }, + { + name: "valid payload post-Holocene with extraData", + params: engine.ExecutableData{ + Timestamp: 0, + ExtraData: validExtraData, + }, + cfg: postHolocene(), + expected: nil, + }, + { + name: "nil withdrawals post-isthmus", + params: engine.ExecutableData{ + Timestamp: 0, + Withdrawals: nil, + ExtraData: validExtraData, + }, + cfg: postIsthmus(), + expected: errors.New("non-empty or nil withdrawals post-isthmus"), + }, + { + name: "non-empty withdrawals post-isthmus", + params: engine.ExecutableData{ + Timestamp: 0, + Withdrawals: make([]*types.Withdrawal, 1), + ExtraData: validExtraData, + }, + cfg: postIsthmus(), + expected: errors.New("non-empty or nil withdrawals post-isthmus"), + }, + { + name: "nil withdrawals root post-isthmus", + params: engine.ExecutableData{ + Timestamp: 0, + Withdrawals: make([]*types.Withdrawal, 0), + ExtraData: validExtraData, + }, + cfg: postIsthmus(), + expected: errors.New("nil withdrawalsRoot post-isthmus"), + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + err := checkOptimismPayload(test.params, test.cfg) + if test.expected == nil { + require.NoError(t, err) + } else { + require.EqualError(t, err, test.expected.Error()) + } + }) + } +} + +func TestCheckOptimismPayloadAttributes(t *testing.T) { + tests := []struct { + name string + payloadAttributes *engine.PayloadAttributes + cfg *params.ChainConfig + expected error + shouldPanic bool + }{ + { + name: "nil payload attributes", + payloadAttributes: nil, + cfg: preHolocene(), + shouldPanic: true, + }, + { + name: "valid payload attributes pre-Holocene", + payloadAttributes: &engine.PayloadAttributes{ + Timestamp: 0, + GasLimit: new(uint64), + }, + cfg: preHolocene(), + expected: nil, + }, + { + name: "invalid payload attributes pre-Holocene with gasLimit", + payloadAttributes: &engine.PayloadAttributes{ + Timestamp: 0, + GasLimit: nil, + }, + cfg: preHolocene(), + 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", + payloadAttributes: &engine.PayloadAttributes{ + Timestamp: 0, + GasLimit: new(uint64), + EIP1559Params: valid1559Params, + }, + cfg: postHolocene(), + expected: nil, + }, + { + name: "non-empty withdrawals post-isthmus", + 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"), + }, + { + name: "nil withdrawals post-isthmus", + payloadAttributes: &engine.PayloadAttributes{ + Timestamp: 0, + GasLimit: new(uint64), + EIP1559Params: valid1559Params, + Withdrawals: nil, + }, + cfg: postIsthmus(), + expected: errors.New("non-empty or nil withdrawals post-isthmus"), + }, + { + name: "empty withdrawals post-isthmus", + payloadAttributes: &engine.PayloadAttributes{ + Timestamp: 0, + GasLimit: new(uint64), + EIP1559Params: valid1559Params, + Withdrawals: make([]*types.Withdrawal, 0), + }, + cfg: postIsthmus(), + expected: nil, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + if test.shouldPanic { + require.Panics(t, func() { + checkOptimismPayloadAttributes(test.payloadAttributes, test.cfg) + }) + } else { + err := checkOptimismPayloadAttributes(test.payloadAttributes, test.cfg) + if test.expected == nil { + require.NoError(t, err) + } else { + require.EqualError(t, err, test.expected.Error()) + } + } + }) + } +} + +// OP Stack test diff: verify that nil payload attributes does not cause a panic +func TestForkChoiceUpdatedNilPayloadAttributes(t *testing.T) { + genesis, blocks := generateMergeChain(10, true) + n, ethservice := startEthService(t, genesis, blocks) + defer n.Close() + api := NewConsensusAPI(ethservice) + cfg := api.eth.BlockChain().Config() + cfg.Optimism = ¶ms.OptimismConfig{} + if !cfg.IsOptimism() { + t.Fatalf("expected optimism config") + } + fcState := engine.ForkchoiceStateV1{ + HeadBlockHash: common.Hash{42}, + } + _, _ = api.forkchoiceUpdated(fcState, nil, engine.PayloadV3, false) +}