beacon/engine,eth/catalyst: Fix engine API checks and exec payload creation

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.

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
 }

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,98 +46,95 @@ 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"),
 		},
 		{
 			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,
 		},
 	}