Fix engine api checks and execution payload generation

turbo/engineapi/engine_server.go

@@ -5,7 +5,6 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"github.com/erigontech/erigon/consensus/misc"
 	"github.com/erigontech/erigon/eth/ethconfig"
 	"math/big"
 	"sync"
@@ -165,10 +164,9 @@ func (s *EngineServer) newPayload(ctx context.Context, req *engine_types.Executi
 		TxHash:      types.DeriveSha(types.BinaryTransactions(txs)),
 	}
 
-	// Payload must have eip-1559 params in ExtraData after Holocene
-	if s.config.IsHolocene(req.Timestamp.Uint64()) {
-		if err := misc.ValidateHoloceneExtraData(req.ExtraData); err != nil {
-			return nil, &rpc.InvalidParamsError{Message: "holocene payloads must have eip-1559 params, got none"}
+	if s.config.IsOptimism() {
+		if err := checkOptimismPayload(req, s.config); err != nil {
+			return nil, &rpc.InvalidParamsError{Message: err.Error()}
 		}
 	}
 
@@ -596,16 +594,12 @@ func (s *EngineServer) forkchoiceUpdated(ctx context.Context, forkchoiceState *e
 	}
 
 	if s.config.Optimism != nil {
-		if payloadAttributes.GasLimit == nil {
+		if err := checkOptimismPayloadAttributes(payloadAttributes, s.config); err != nil {
 			return nil, &engine_helpers.InvalidPayloadAttributesErr
 		}
+
 		if s.config.IsHolocene(payloadAttributes.Timestamp.Uint64()) {
-			if err := misc.ValidateHolocene1559Params(payloadAttributes.EIP1559Params); err != nil {
-				return nil, err
-			}
 			req.Eip_1559Params = bytes.Clone(payloadAttributes.EIP1559Params)
-		} else if len(payloadAttributes.EIP1559Params) != 0 {
-			return nil, &engine_helpers.InvalidPayloadAttributesErr
 		}
 	}
 

turbo/engineapi/engine_server_optimism.go

@@ -0,0 +1,68 @@
+package engineapi
+
+import (
+	"errors"
+	"github.com/erigontech/erigon-lib/chain"
+	"github.com/erigontech/erigon/consensus/misc"
+	"github.com/erigontech/erigon/turbo/engineapi/engine_types"
+)
+
+// checkOptimismPayload performs Optimism-specific checks on the payload data (called during [(*EngineServer).newPayload]).
+func checkOptimismPayload(params *engine_types.ExecutionPayload, cfg *chain.Config) error {
+	// (non)-nil withdrawals is already checked by Shanghai rules.
+	// Canyon - empty withdrawals
+	if cfg.IsCanyon(params.Timestamp.Uint64()) {
+		if len(params.Withdrawals) != 0 {
+			return errors.New("non-empty withdrawals post-Canyon")
+		}
+	}
+
+	// Holocene - extraData
+	if cfg.IsHolocene(params.Timestamp.Uint64()) {
+		if err := misc.ValidateHoloceneExtraData(params.ExtraData); err != nil {
+			return err
+		}
+	} else if len(params.ExtraData) > 0 { // pre-Holocene
+		return errors.New("extraData must be empty before Holocene")
+	}
+
+	// Isthmus - withdrawalsRoot
+	if cfg.IsIsthmus(params.Timestamp.Uint64()) {
+		if params.WithdrawalsRoot == nil {
+			return errors.New("nil withdrawalsRoot post-Isthmus")
+		}
+	} else if params.WithdrawalsRoot != nil { // pre-Isthmus
+		return errors.New("non-nil withdrawalsRoot pre-Isthmus")
+	}
+
+	return nil
+}
+
+// checkOptimismPayloadAttributes performs Optimism-specific checks on the payload attributes (called during [(*EngineServer).forkchoiceUpdated].
+// Will panic if payloadAttributes is nil.
+func checkOptimismPayloadAttributes(payloadAttributes *engine_types.PayloadAttributes, cfg *chain.Config) error {
+	if payloadAttributes.GasLimit == nil {
+		return errors.New("gasLimit parameter is required")
+	}
+
+	// (non)-nil withdrawals is already checked by Shanghai rules.
+	// Canyon - empty withdrawals
+	if cfg.IsCanyon(payloadAttributes.Timestamp.Uint64()) {
+		if len(payloadAttributes.Withdrawals) != 0 {
+			return errors.New("non-empty withdrawals post-Canyon")
+		}
+	}
+
+	// Holocene - extraData
+	if cfg.IsHolocene(payloadAttributes.Timestamp.Uint64()) {
+		if err := misc.ValidateHolocene1559Params(payloadAttributes.EIP1559Params); err != nil {
+			return err
+		}
+	} else if len(payloadAttributes.EIP1559Params) != 0 { // pre-Holocene
+		return errors.New("non-empty eip155Params pre-Holocene")
+	}
+
+	// Note: PayloadAttributes don't contain the Isthmus withdrawalsRoot, it's set during block assembly.
+
+	return nil
+}

turbo/engineapi/engine_server_optimism_test.go

@@ -0,0 +1,242 @@
+package engineapi
+
+import (
+	"errors"
+	"github.com/erigontech/erigon-lib/chain"
+	"github.com/erigontech/erigon-lib/common/hexutil"
+	"github.com/erigontech/erigon/core/types"
+	"github.com/erigontech/erigon/turbo/engineapi/engine_types"
+	"math/big"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func preCanyon() *chain.Config {
+	cfg := new(chain.Config)
+	// Mark as an Optimism chain so IsOptimismFoo is true when FooTime is active
+	cfg.Optimism = &chain.OptimismConfig{}
+	return cfg
+}
+
+func postCanyon() *chain.Config {
+	cfg := preCanyon()
+	cfg.CanyonTime = new(big.Int)
+	return cfg
+}
+
+func postHolocene() *chain.Config {
+	cfg := postCanyon()
+	cfg.HoloceneTime = new(big.Int)
+	return cfg
+}
+
+func postIsthmus() *chain.Config {
+	cfg := postHolocene()
+	cfg.IsthmusTime = new(big.Int)
+	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)
+var validJovianExtraData = append(append([]byte{1}, valid1559Params...), make([]byte, 8)...) // version=1, 8 bytes params, 8 byte minBaseFee
+
+func TestCheckOptimismPayload(t *testing.T) {
+	tests := []struct {
+		name     string
+		params   engine_types.ExecutionPayload
+		cfg      *chain.Config
+		expected error
+	}{
+		{
+			name: "valid payload pre-Canyon",
+			params: engine_types.ExecutionPayload{
+				ExtraData: []byte{},
+			},
+			cfg:      preCanyon(),
+			expected: nil,
+		},
+		{
+			name: "valid payload post-Canyon",
+			params: engine_types.ExecutionPayload{
+				ExtraData:   []byte{},
+				Withdrawals: emptyWithdrawals,
+			},
+			cfg: postCanyon(),
+		},
+		{
+			name: "invalid empty withdrawals post-Canyon",
+			params: engine_types.ExecutionPayload{
+				ExtraData:   []byte{},
+				Withdrawals: make([]*types.Withdrawal, 1),
+			},
+			cfg:      postCanyon(),
+			expected: errors.New("non-empty withdrawals post-Canyon"),
+		},
+		{
+			name: "non-nil withdrawalsRoot pre-Isthmus",
+			params: engine_types.ExecutionPayload{
+				ExtraData:       []byte{},
+				Withdrawals:     emptyWithdrawals,
+				WithdrawalsRoot: &types.EmptyRootHash,
+			},
+			cfg:      postCanyon(),
+			expected: errors.New("non-nil withdrawalsRoot pre-Isthmus"),
+		},
+		{
+			name: "invalid non-empty extraData pre-Holocene",
+			params: engine_types.ExecutionPayload{
+				ExtraData:   []byte{1, 2, 3},
+				Withdrawals: emptyWithdrawals,
+			},
+			cfg:      postCanyon(),
+			expected: errors.New("extraData must be empty before Holocene"),
+		},
+		{
+			name: "invalid extraData post-Holocene",
+			params: engine_types.ExecutionPayload{
+				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_types.ExecutionPayload{
+				ExtraData:   validExtraData,
+				Withdrawals: emptyWithdrawals,
+			},
+			cfg:      postHolocene(),
+			expected: nil,
+		},
+		{
+			name: "invalid non-nil withdrawalsRoot post-Holocene",
+			params: engine_types.ExecutionPayload{
+				ExtraData:       validExtraData,
+				Withdrawals:     emptyWithdrawals,
+				WithdrawalsRoot: &types.EmptyRootHash,
+			},
+			cfg:      postHolocene(),
+			expected: errors.New("non-nil withdrawalsRoot pre-Isthmus"),
+		},
+		{
+			name: "valid payload post-Isthmus",
+			params: engine_types.ExecutionPayload{
+				ExtraData:       validExtraData,
+				Withdrawals:     emptyWithdrawals,
+				WithdrawalsRoot: &types.EmptyRootHash,
+			},
+			cfg:      postIsthmus(),
+			expected: nil,
+		},
+		{
+			name: "invalid nil withdrawals root post-isthmus",
+			params: engine_types.ExecutionPayload{
+				Withdrawals: emptyWithdrawals,
+				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_types.PayloadAttributes
+		cfg               *chain.Config
+		expected          error
+		shouldPanic       bool
+	}{
+		{
+			name:              "nil payload attributes panic",
+			payloadAttributes: nil,
+			cfg:               preCanyon(),
+			shouldPanic:       true,
+		},
+		{
+			name: "valid payload attributes pre-Canyon",
+			payloadAttributes: &engine_types.PayloadAttributes{
+				GasLimit: new(hexutil.Uint64),
+			},
+			cfg:      preCanyon(),
+			expected: nil,
+		},
+		{
+			name: "invalid nil gasLimit",
+			payloadAttributes: &engine_types.PayloadAttributes{
+				GasLimit: nil,
+			},
+			cfg:      preCanyon(),
+			expected: errors.New("gasLimit parameter is required"),
+		},
+		{
+			name: "invalid non-empty withdrawals post-Canyon",
+			payloadAttributes: &engine_types.PayloadAttributes{
+				GasLimit:      new(hexutil.Uint64),
+				EIP1559Params: valid1559Params,
+				Withdrawals:   make([]*types.Withdrawal, 1),
+			},
+			cfg:      postCanyon(),
+			expected: errors.New("non-empty withdrawals post-Canyon"),
+		},
+		{
+			name: "invalid non-empty eip1559Params pre-Holocene",
+			payloadAttributes: &engine_types.PayloadAttributes{
+				GasLimit:      new(hexutil.Uint64),
+				EIP1559Params: valid1559Params,
+			},
+			cfg:      postCanyon(),
+			expected: errors.New("non-empty eip155Params pre-Holocene"),
+		},
+		{
+			name: "invalid eip1559Params post-Holocene",
+			payloadAttributes: &engine_types.PayloadAttributes{
+				GasLimit:      new(hexutil.Uint64),
+				EIP1559Params: append(valid1559Params, 77),
+			},
+			cfg:      postHolocene(),
+			expected: errors.New("holocene eip-1559 params should be 8 bytes, got 9"),
+		},
+		{
+			name: "valid payload attributes post-Holocene",
+			payloadAttributes: &engine_types.PayloadAttributes{
+				GasLimit:      new(hexutil.Uint64),
+				EIP1559Params: valid1559Params,
+			},
+			cfg:      postHolocene(),
+			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())
+				}
+			}
+		})
+	}
+}

turbo/engineapi/engine_types/jsonrpc.go

@@ -222,7 +222,9 @@ func ConvertPayloadFromRpc(payload *types2.ExecutionPayload) *ExecutionPayload {
 	}
 	if payload.WithdrawlsRoot != nil {
 		root := common.Hash(gointerfaces.ConvertH256ToHash(payload.WithdrawlsRoot))
-		res.WithdrawalsRoot = &root
+		if root != types.EmptyRootHash {
+			res.WithdrawalsRoot = &root
+		}
 	}
 	return res
 }