engine: return -38003 for FCUv2 payloadAttributes mismatch (#19779)

This PR updates `engine_forkchoiceUpdatedV2` to return `-38003: Invalid
payload attributes` when the wrong `payloadAttributes` version is used.

  In particular, FCUv2 payload-attribute version mismatches such as:
  - missing `withdrawals` at or after Shanghai
  - unexpected `withdrawals` before Shanghai

should be treated as `Invalid payload attributes`, not `Invalid params`.

  ## Why

This change aligns the client with the latest Engine API spec update in:
  - ethereum/execution-apis#761

It also follows the implementation discussion and prior client-side
change in:
  - ethereum/go-ethereum#33918

The spec was clarified so that FCUv2 now behaves consistently with newer
forkchoiceUpdated versions for payloadAttributes structure/version
  mismatches.

  ## What changed

- Updated FCUv2 payload attributes validation to return `-38003` for
payloadAttributes version mismatches.
  - Added/updated regression coverage for the affected FCUv2 cases.

  ## Hive impact

This fixes the Hive `engine-withdrawals` failure caused by returning the
wrong error code for FCUv2 payloadAttributes mismatches.

  Relevant Hive failure:
-
https://hive.ethpandaops.io/#/test/generic/1773130326-4e173a80b2b6f0634fd0139743cbe0de

After this change, the client returns the expected error code for the
affected FCUv2 cases.

If my understanding or interpretation of the spec change is incorrect,
please let me know and I can adjust the implementation accordingly.

---------

Co-authored-by: muzry.li <muzry.li1@ambergroup.io>

Author: 2 people

.github/workflows/test-hive.yml

@@ -74,7 +74,7 @@ jobs:
         with:
           repository: ethereum/hive
           # version hive and update periodically/on-demand to prevent upstream changes in Hive affecting us with red CI
-          ref: 0ee187ce394720a5902c135324ac7de4240cbb37
+          ref: 88786d9744dabb08bdaec8d8b53142cde6e6b79a
           path: hive
 
       - name: Setup go env and cache

execution/engineapi/engine_server.go

@@ -178,13 +178,20 @@ func (e *EngineServer) Start(
 }
 
 func (s *EngineServer) checkWithdrawalsPresence(time uint64, withdrawals types.Withdrawals) error {
-	if !s.config.IsShanghai(time) && withdrawals != nil {
+	if s.isWithdrawalsPresenceValid(time, withdrawals) {
+		return nil
+	}
+	if !s.config.IsShanghai(time) {
 		return &rpc.InvalidParamsError{Message: "withdrawals before Shanghai"}
 	}
-	if s.config.IsShanghai(time) && withdrawals == nil {
-		return &rpc.InvalidParamsError{Message: "missing withdrawals list"}
+	return &rpc.InvalidParamsError{Message: "missing withdrawals list"}
+}
+
+func (s *EngineServer) isWithdrawalsPresenceValid(time uint64, withdrawals types.Withdrawals) bool {
+	if !s.config.IsShanghai(time) {
+		return withdrawals == nil
 	}
-	return nil
+	return withdrawals != nil
 }
 
 func (s *EngineServer) checkRequestsPresence(version clparams.StateVersion, executionRequests []hexutil.Bytes) error {
@@ -718,6 +725,9 @@ func (s *EngineServer) forkchoiceUpdated(ctx context.Context, forkchoiceState *e
 	if s.config.IsCancun(timestamp) && version < clparams.DenebVersion { // Not V3 after cancun
 		return nil, &rpc.UnsupportedForkError{Message: "Unsupported fork"}
 	}
+	if version >= clparams.CapellaVersion && !s.isWithdrawalsPresenceValid(timestamp, payloadAttributes.Withdrawals) {
+		return nil, &engine_helpers.InvalidPayloadAttributesErr
+	}
 
 	if !s.proposing {
 		return nil, errors.New("execution layer not running as a proposer. enable proposer by taking out the --proposer.disable flag on startup")

execution/engineapi/testing_api_test.go

@@ -24,7 +24,9 @@ import (
 	"github.com/holiman/uint256"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"google.golang.org/protobuf/types/known/emptypb"
 
+	"github.com/erigontech/erigon/cl/clparams"
 	"github.com/erigontech/erigon/common"
 	"github.com/erigontech/erigon/common/hexutil"
 	"github.com/erigontech/erigon/common/log/v3"
@@ -50,6 +52,7 @@ type stubExecutionServer struct {
 	getHeaderFunc         func(ctx context.Context, in *executionproto.GetSegmentRequest) (*executionproto.GetHeaderResponse, error)
 	assembleBlockFunc     func(ctx context.Context, in *executionproto.AssembleBlockRequest) (*executionproto.AssembleBlockResponse, error)
 	getAssembledBlockFunc func(ctx context.Context, in *executionproto.GetAssembledBlockRequest) (*executionproto.GetAssembledBlockResponse, error)
+	getForkChoiceFunc     func(ctx context.Context, in *emptypb.Empty) (*executionproto.ForkChoice, error)
 }
 
 func (s *stubExecutionServer) GetHeader(ctx context.Context, in *executionproto.GetSegmentRequest) (*executionproto.GetHeaderResponse, error) {
@@ -73,6 +76,13 @@ func (s *stubExecutionServer) GetAssembledBlock(ctx context.Context, in *executi
 	return &executionproto.GetAssembledBlockResponse{}, nil
 }
 
+func (s *stubExecutionServer) GetForkChoice(ctx context.Context, in *emptypb.Empty) (*executionproto.ForkChoice, error) {
+	if s.getForkChoiceFunc != nil {
+		return s.getForkChoiceFunc(ctx, in)
+	}
+	return &executionproto.ForkChoice{}, nil
+}
+
 // ---------------------------------------------------------------------------
 // Test helpers
 // ---------------------------------------------------------------------------
@@ -595,6 +605,86 @@ func TestBuildBlockV1(t *testing.T) {
 	})
 }
 
+func TestForkchoiceUpdatedV2PayloadAttributesWithdrawalsValidation(t *testing.T) {
+	t.Parallel()
+
+	t.Run("missing withdrawals for Shanghai returns invalid payload attributes", func(t *testing.T) {
+		forkchoiceState := &engine_types.ForkChoiceState{
+			HeadHash:           common.Hash{0x1},
+			SafeBlockHash:      common.Hash{0x2},
+			FinalizedBlockHash: common.Hash{0x3},
+		}
+		srv := NewEngineServer(
+			log.New(),
+			preCancunChainConfig(),
+			direct.NewExecutionClientDirect(&stubExecutionServer{
+				getForkChoiceFunc: func(context.Context, *emptypb.Empty) (*executionproto.ForkChoice, error) {
+					return &executionproto.ForkChoice{
+						HeadBlockHash:      gointerfaces.ConvertHashToH256(forkchoiceState.HeadHash),
+						SafeBlockHash:      gointerfaces.ConvertHashToH256(forkchoiceState.SafeBlockHash),
+						FinalizedBlockHash: gointerfaces.ConvertHashToH256(forkchoiceState.FinalizedBlockHash),
+					}, nil
+				},
+			}),
+			nil,
+			false,
+			true,
+			true,
+			nil,
+			0,
+			0,
+		)
+
+		resp, err := srv.forkchoiceUpdated(context.Background(), forkchoiceState, &engine_types.PayloadAttributes{
+			Timestamp:             hexutil.Uint64(1001),
+			PrevRandao:            common.Hash{0xaa},
+			SuggestedFeeRecipient: common.HexToAddress("0x1111111111111111111111111111111111111111"),
+			Withdrawals:           nil,
+		}, clparams.CapellaVersion)
+		require.Nil(t, resp)
+		require.Error(t, err)
+		require.Equal(t, -38003, err.(rpc.Error).ErrorCode())
+	})
+
+	t.Run("withdrawals before Shanghai returns invalid payload attributes", func(t *testing.T) {
+		forkchoiceState := &engine_types.ForkChoiceState{
+			HeadHash:           common.Hash{0x4},
+			SafeBlockHash:      common.Hash{0x5},
+			FinalizedBlockHash: common.Hash{0x6},
+		}
+		srv := NewEngineServer(
+			log.New(),
+			preShanghaiChainConfig(),
+			direct.NewExecutionClientDirect(&stubExecutionServer{
+				getForkChoiceFunc: func(context.Context, *emptypb.Empty) (*executionproto.ForkChoice, error) {
+					return &executionproto.ForkChoice{
+						HeadBlockHash:      gointerfaces.ConvertHashToH256(forkchoiceState.HeadHash),
+						SafeBlockHash:      gointerfaces.ConvertHashToH256(forkchoiceState.SafeBlockHash),
+						FinalizedBlockHash: gointerfaces.ConvertHashToH256(forkchoiceState.FinalizedBlockHash),
+					}, nil
+				},
+			}),
+			nil,
+			false,
+			true,
+			true,
+			nil,
+			0,
+			0,
+		)
+
+		resp, err := srv.forkchoiceUpdated(context.Background(), forkchoiceState, &engine_types.PayloadAttributes{
+			Timestamp:             hexutil.Uint64(1001),
+			PrevRandao:            common.Hash{0xaa},
+			SuggestedFeeRecipient: common.HexToAddress("0x1111111111111111111111111111111111111111"),
+			Withdrawals:           make([]*types.Withdrawal, 0),
+		}, clparams.CapellaVersion)
+		require.Nil(t, resp)
+		require.Error(t, err)
+		require.Equal(t, -38003, err.(rpc.Error).ErrorCode())
+	})
+}
+
 func ptrUint64(v uint64) *uint64 {
 	return &v
 }