catalyst/api: centralize OPStack validation into helper functions

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() {
+		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()
@@ -435,17 +442,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)
-				}
+				// Validation performed above in checkOptimismPayloadAttributes
 				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"))
 			}
 		}
 		transactions := make(types.Transactions, 0, len(payloadAttributes.Transactions))
@@ -663,10 +662,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 +865,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()

eth/catalyst/api_optimism.go

@@ -0,0 +1,60 @@
+package catalyst
+
+import (
+	"errors"
+
+	"github.com/ethereum/go-ethereum/beacon/engine"
+	"github.com/ethereum/go-ethereum/consensus/misc/eip1559"
+	"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 {
+	// 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].
+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) {
+		if payloadAttributes.Withdrawals == nil || len(payloadAttributes.Withdrawals) != 0 {
+			return errors.New("non-empty or nil withdrawals post-isthmus")
+		}
+	}
+
+	return nil
+}

eth/catalyst/api_optimism_test.go

@@ -0,0 +1,211 @@
+package catalyst
+
+import (
+	"errors"
+	"testing"
+
+	"github.com/ethereum/go-ethereum/beacon/engine"
+	"github.com/ethereum/go-ethereum/core/types"
+	"github.com/ethereum/go-ethereum/params"
+	"github.com/stretchr/testify/require"
+)
+
+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 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
+	}{
+		{
+			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) {
+			err := checkOptimismPayloadAttributes(test.payloadAttributes, test.cfg)
+			if test.expected == nil {
+				require.NoError(t, err)
+			} else {
+				require.EqualError(t, err, test.expected.Error())
+			}
+		})
+	}
+}