Move validate merge transition block outside of notify new payload (#10526)

* Move validate merge transition block

* Update process_block_test.go

* Conflict, use swith err

* Update execution_engine.go

Co-authored-by: Raul Jordan <raul@prysmaticlabs.com>

beacon-chain/blockchain/execution_engine.go

@@ -108,8 +108,8 @@ func (s *Service) notifyForkchoiceUpdate(ctx context.Context, headState state.Be
 
 // notifyForkchoiceUpdate signals execution engine on a new payload.
 // It returns true if the EL has returned VALID for the block
-func (s *Service) notifyNewPayload(ctx context.Context, preStateVersion, postStateVersion int,
-	preStateHeader, postStateHeader *ethpb.ExecutionPayloadHeader, blk block.SignedBeaconBlock) (bool, error) {
+func (s *Service) notifyNewPayload(ctx context.Context, postStateVersion int,
+	postStateHeader *ethpb.ExecutionPayloadHeader, blk block.SignedBeaconBlock) (bool, error) {
 	ctx, span := trace.StartSpan(ctx, "blockChain.notifyNewPayload")
 	defer span.End()
 
@@ -134,48 +134,34 @@ func (s *Service) notifyNewPayload(ctx context.Context, preStateVersion, postSta
 		return false, errors.Wrap(err, "could not get execution payload")
 	}
 	lastValidHash, err := s.cfg.ExecutionEngineCaller.NewPayload(ctx, payload)
-	if err != nil {
-		switch err {
-		case powchain.ErrAcceptedSyncingPayloadStatus:
-			newPayloadOptimisticNodeCount.Inc()
-			log.WithFields(logrus.Fields{
-				"slot":             blk.Block().Slot(),
-				"payloadBlockHash": fmt.Sprintf("%#x", bytesutil.Trunc(payload.BlockHash)),
-			}).Info("Called new payload with optimistic block")
-			return false, nil
-		case powchain.ErrInvalidPayloadStatus:
-			newPayloadInvalidNodeCount.Inc()
-			root, err := blk.Block().HashTreeRoot()
-			if err != nil {
-				return false, err
-			}
-			invalidRoots, err := s.ForkChoicer().SetOptimisticToInvalid(ctx, root, bytesutil.ToBytes32(lastValidHash))
-			if err != nil {
-				return false, err
-			}
-			if err := s.removeInvalidBlockAndState(ctx, invalidRoots); err != nil {
-				return false, err
-			}
-			return false, errors.New("could not validate an INVALID payload from execution engine")
-		default:
-			return false, errors.Wrap(err, "could not validate execution payload from execution engine")
-		}
-	}
-	newPayloadValidNodeCount.Inc()
-	// During the transition event, the transition block should be verified for sanity.
-	if blocks.IsPreBellatrixVersion(preStateVersion) {
-		// Handle case where pre-state is Altair but block contains payload.
-		// To reach here, the block must have contained a valid payload.
-		return true, s.validateMergeBlock(ctx, blk)
-	}
-	atTransition, err := blocks.IsMergeTransitionBlockUsingPreStatePayloadHeader(preStateHeader, body)
-	if err != nil {
-		return true, errors.Wrap(err, "could not check if merge block is terminal")
-	}
-	if !atTransition {
+	switch err {
+	case nil:
+		newPayloadValidNodeCount.Inc()
 		return true, nil
+	case powchain.ErrAcceptedSyncingPayloadStatus:
+		newPayloadOptimisticNodeCount.Inc()
+		log.WithFields(logrus.Fields{
+			"slot":             blk.Block().Slot(),
+			"payloadBlockHash": fmt.Sprintf("%#x", bytesutil.Trunc(payload.BlockHash)),
+		}).Info("Called new payload with optimistic block")
+		return false, nil
+	case powchain.ErrInvalidPayloadStatus:
+		newPayloadInvalidNodeCount.Inc()
+		root, err := blk.Block().HashTreeRoot()
+		if err != nil {
+			return false, err
+		}
+		invalidRoots, err := s.ForkChoicer().SetOptimisticToInvalid(ctx, root, bytesutil.ToBytes32(lastValidHash))
+		if err != nil {
+			return false, err
+		}
+		if err := s.removeInvalidBlockAndState(ctx, invalidRoots); err != nil {
+			return false, err
+		}
+		return false, errors.New("could not validate an INVALID payload from execution engine")
+	default:
+		return false, errors.Wrap(err, "could not validate execution payload from execution engine")
 	}
-	return true, s.validateMergeBlock(ctx, blk)
 }
 
 // optimisticCandidateBlock returns true if this block can be optimistically synced.

beacon-chain/blockchain/execution_engine_test.go

@@ -22,7 +22,6 @@ import (
 	ethpb "github.com/prysmaticlabs/prysm/proto/prysm/v1alpha1"
 	"github.com/prysmaticlabs/prysm/proto/prysm/v1alpha1/block"
 	"github.com/prysmaticlabs/prysm/proto/prysm/v1alpha1/wrapper"
-	"github.com/prysmaticlabs/prysm/runtime/version"
 	"github.com/prysmaticlabs/prysm/testing/assert"
 	"github.com/prysmaticlabs/prysm/testing/require"
 	"github.com/prysmaticlabs/prysm/testing/util"
@@ -229,7 +228,6 @@ func Test_NotifyNewPayload(t *testing.T) {
 
 	tests := []struct {
 		name           string
-		preState       state.BeaconState
 		postState      state.BeaconState
 		isValidPayload bool
 		blk            block.SignedBeaconBlock
@@ -239,26 +237,22 @@ func Test_NotifyNewPayload(t *testing.T) {
 		{
 			name:           "phase 0 post state",
 			postState:      phase0State,
-			preState:       phase0State,
 			isValidPayload: true,
 		},
 		{
 			name:           "altair post state",
 			postState:      altairState,
-			preState:       altairState,
 			isValidPayload: true,
 		},
 		{
 			name:           "nil beacon block",
 			postState:      bellatrixState,
-			preState:       bellatrixState,
 			errString:      "signed beacon block can't be nil",
 			isValidPayload: false,
 		},
 		{
 			name:           "new payload with optimistic block",
 			postState:      bellatrixState,
-			preState:       bellatrixState,
 			blk:            bellatrixBlk,
 			newPayloadErr:  powchain.ErrAcceptedSyncingPayloadStatus,
 			isValidPayload: false,
@@ -266,7 +260,6 @@ func Test_NotifyNewPayload(t *testing.T) {
 		{
 			name:           "new payload with invalid block",
 			postState:      bellatrixState,
-			preState:       bellatrixState,
 			blk:            bellatrixBlk,
 			newPayloadErr:  powchain.ErrInvalidPayloadStatus,
 			errString:      "could not validate an INVALID payload from execution engine",
@@ -275,14 +268,12 @@ func Test_NotifyNewPayload(t *testing.T) {
 		{
 			name:           "altair pre state, altair block",
 			postState:      bellatrixState,
-			preState:       altairState,
 			blk:            altairBlk,
 			isValidPayload: true,
 		},
 		{
 			name:      "altair pre state, happy case",
 			postState: bellatrixState,
-			preState:  altairState,
 			blk: func() block.SignedBeaconBlock {
 				blk := &ethpb.SignedBeaconBlockBellatrix{
 					Block: &ethpb.BeaconBlockBellatrix{
@@ -299,18 +290,9 @@ func Test_NotifyNewPayload(t *testing.T) {
 			}(),
 			isValidPayload: true,
 		},
-		{
-			name:           "could not get merge block",
-			postState:      bellatrixState,
-			preState:       bellatrixState,
-			blk:            bellatrixBlk,
-			errString:      "could not get merge block parent hash and total difficulty",
-			isValidPayload: false,
-		},
 		{
 			name:      "not at merge transition",
 			postState: bellatrixState,
-			preState:  bellatrixState,
 			blk: func() block.SignedBeaconBlock {
 				blk := &ethpb.SignedBeaconBlockBellatrix{
 					Block: &ethpb.BeaconBlockBellatrix{
@@ -334,18 +316,9 @@ func Test_NotifyNewPayload(t *testing.T) {
 			}(),
 			isValidPayload: true,
 		},
-		{
-			name:           "could not get merge block",
-			postState:      bellatrixState,
-			preState:       bellatrixState,
-			blk:            bellatrixBlk,
-			errString:      "could not get merge block parent hash and total difficulty",
-			isValidPayload: false,
-		},
 		{
 			name:      "happy case",
 			postState: bellatrixState,
-			preState:  bellatrixState,
 			blk: func() block.SignedBeaconBlock {
 				blk := &ethpb.SignedBeaconBlockBellatrix{
 					Block: &ethpb.BeaconBlockBellatrix{
@@ -375,16 +348,11 @@ func Test_NotifyNewPayload(t *testing.T) {
 				TotalDifficulty: "0x1",
 			}
 			service.cfg.ExecutionEngineCaller = e
-			var payload *ethpb.ExecutionPayloadHeader
-			if tt.preState.Version() == version.Bellatrix {
-				payload, err = tt.preState.LatestExecutionPayloadHeader()
-				require.NoError(t, err)
-			}
 			root := [32]byte{'a'}
 			require.NoError(t, service.cfg.ForkChoiceStore.InsertOptimisticBlock(ctx, 0, root, root, params.BeaconConfig().ZeroHash, 0, 0))
 			postVersion, postHeader, err := getStateVersionAndPayload(tt.postState)
 			require.NoError(t, err)
-			isValidPayload, err := service.notifyNewPayload(ctx, tt.preState.Version(), postVersion, payload, postHeader, tt.blk)
+			isValidPayload, err := service.notifyNewPayload(ctx, postVersion, postHeader, tt.blk)
 			if tt.errString != "" {
 				require.ErrorContains(t, tt.errString, err)
 			} else {
@@ -431,11 +399,9 @@ func Test_NotifyNewPayload_SetOptimisticToValid(t *testing.T) {
 		TotalDifficulty: "0x1",
 	}
 	service.cfg.ExecutionEngineCaller = e
-	payload, err := bellatrixState.LatestExecutionPayloadHeader()
-	require.NoError(t, err)
 	postVersion, postHeader, err := getStateVersionAndPayload(bellatrixState)
 	require.NoError(t, err)
-	validated, err := service.notifyNewPayload(ctx, bellatrixState.Version(), postVersion, payload, postHeader, bellatrixBlk)
+	validated, err := service.notifyNewPayload(ctx, postVersion, postHeader, bellatrixBlk)
 	require.NoError(t, err)
 	require.Equal(t, true, validated)
 }

beacon-chain/blockchain/process_block.go

@@ -113,11 +113,15 @@ func (s *Service) onBlock(ctx context.Context, signed block.SignedBeaconBlock, b
 	if err != nil {
 		return err
 	}
-	isValidPayload, err := s.notifyNewPayload(ctx, preStateVersion, postStateVersion, preStateHeader, postStateHeader, signed)
+	isValidPayload, err := s.notifyNewPayload(ctx, postStateVersion, postStateHeader, signed)
 	if err != nil {
 		return errors.Wrap(err, "could not verify new payload")
 	}
-	if !isValidPayload {
+	if isValidPayload {
+		if err := s.validateMergeTransitionBlock(ctx, preStateVersion, preStateHeader, signed); err != nil {
+			return err
+		}
+	} else {
 		candidate, err := s.optimisticCandidateBlock(ctx, b)
 		if err != nil {
 			return errors.Wrap(err, "could not check if block is optimistic candidate")
@@ -407,15 +411,17 @@ func (s *Service) onBlockBatch(ctx context.Context, blks []block.SignedBeaconBlo
 	// blocks have been verified, add them to forkchoice and call the engine
 	for i, b := range blks {
 		isValidPayload, err := s.notifyNewPayload(ctx,
-			preVersionAndHeaders[i].version,
 			postVersionAndHeaders[i].version,
-			preVersionAndHeaders[i].header,
 			postVersionAndHeaders[i].header, b)
-
 		if err != nil {
 			return nil, nil, err
 		}
-		if !isValidPayload {
+		if isValidPayload {
+			if err := s.validateMergeTransitionBlock(ctx, preVersionAndHeaders[i].version,
+				preVersionAndHeaders[i].header, b); err != nil {
+				return nil, nil, err
+			}
+		} else {
 			candidate, err := s.optimisticCandidateBlock(ctx, b.Block())
 			if err != nil {
 				return nil, nil, errors.Wrap(err, "could not check if block is optimistic candidate")
@@ -642,3 +648,36 @@ func (s *Service) pruneCanonicalAttsFromPool(ctx context.Context, r [32]byte, b
 	}
 	return nil
 }
+
+// validateMergeTransitionBlock validates the merge transition block.
+func (s *Service) validateMergeTransitionBlock(ctx context.Context, stateVersion int, stateHeader *ethpb.ExecutionPayloadHeader, blk block.SignedBeaconBlock) error {
+	// Skip validation if block is older than Bellatrix.
+	if blocks.IsPreBellatrixVersion(blk.Block().Version()) {
+		return nil
+	}
+
+	// Skip validation if block has an empty payload.
+	payload, err := blk.Block().Body().ExecutionPayload()
+	if err != nil {
+		return err
+	}
+	if blocks.IsEmptyPayload(payload) {
+		return nil
+	}
+
+	// Handle case where pre-state is Altair but block contains payload.
+	// To reach here, the block must have contained a valid payload.
+	if blocks.IsPreBellatrixVersion(stateVersion) {
+		return s.validateMergeBlock(ctx, blk)
+	}
+
+	// Skip validation if the block is not a merge transition block.
+	atTransition, err := blocks.IsMergeTransitionBlockUsingPreStatePayloadHeader(stateHeader, blk.Block().Body())
+	if err != nil {
+		return errors.Wrap(err, "could not check if merge block is terminal")
+	}
+	if !atTransition {
+		return nil
+	}
+	return s.validateMergeBlock(ctx, blk)
+}

beacon-chain/blockchain/process_block_test.go

@@ -11,6 +11,7 @@ import (
 	"github.com/pkg/errors"
 	types "github.com/prysmaticlabs/eth2-types"
 	mock "github.com/prysmaticlabs/prysm/beacon-chain/blockchain/testing"
+	"github.com/prysmaticlabs/prysm/beacon-chain/cache"
 	"github.com/prysmaticlabs/prysm/beacon-chain/cache/depositcache"
 	"github.com/prysmaticlabs/prysm/beacon-chain/core/blocks"
 	"github.com/prysmaticlabs/prysm/beacon-chain/core/transition"
@@ -19,6 +20,7 @@ import (
 	doublylinkedtree "github.com/prysmaticlabs/prysm/beacon-chain/forkchoice/doubly-linked-tree"
 	"github.com/prysmaticlabs/prysm/beacon-chain/forkchoice/protoarray"
 	forkchoicetypes "github.com/prysmaticlabs/prysm/beacon-chain/forkchoice/types"
+	mockPOW "github.com/prysmaticlabs/prysm/beacon-chain/powchain/testing"
 	"github.com/prysmaticlabs/prysm/beacon-chain/state"
 	"github.com/prysmaticlabs/prysm/beacon-chain/state/stategen"
 	v1 "github.com/prysmaticlabs/prysm/beacon-chain/state/v1"
@@ -26,6 +28,7 @@ import (
 	fieldparams "github.com/prysmaticlabs/prysm/config/fieldparams"
 	"github.com/prysmaticlabs/prysm/config/params"
 	"github.com/prysmaticlabs/prysm/encoding/bytesutil"
+	enginev1 "github.com/prysmaticlabs/prysm/proto/engine/v1"
 	ethpb "github.com/prysmaticlabs/prysm/proto/prysm/v1alpha1"
 	"github.com/prysmaticlabs/prysm/proto/prysm/v1alpha1/block"
 	"github.com/prysmaticlabs/prysm/proto/prysm/v1alpha1/wrapper"
@@ -1692,3 +1695,136 @@ func Test_getStateVersionAndPayload(t *testing.T) {
 		})
 	}
 }
+
+func Test_validateMergeTransitionBlock(t *testing.T) {
+	cfg := params.BeaconConfig()
+	cfg.TerminalTotalDifficulty = "2"
+	cfg.TerminalBlockHash = params.BeaconConfig().ZeroHash
+	params.OverrideBeaconConfig(cfg)
+
+	ctx := context.Background()
+	beaconDB := testDB.SetupDB(t)
+	fcs := protoarray.New(0, 0, [32]byte{'a'})
+	opts := []Option{
+		WithDatabase(beaconDB),
+		WithStateGen(stategen.New(beaconDB)),
+		WithForkChoiceStore(fcs),
+		WithProposerIdsCache(cache.NewProposerPayloadIDsCache()),
+	}
+	service, err := NewService(ctx, opts...)
+	require.NoError(t, err)
+
+	tests := []struct {
+		name         string
+		stateVersion int
+		header       *ethpb.ExecutionPayloadHeader
+		payload      *enginev1.ExecutionPayload
+		errString    string
+	}{
+		{
+			name:         "state older than Bellatrix, nil payload",
+			stateVersion: 1,
+			payload:      nil,
+		},
+		{
+			name:         "state older than Bellatrix, empty payload",
+			stateVersion: 1,
+			payload: &enginev1.ExecutionPayload{
+				ParentHash:    make([]byte, fieldparams.RootLength),
+				FeeRecipient:  make([]byte, fieldparams.FeeRecipientLength),
+				StateRoot:     make([]byte, fieldparams.RootLength),
+				ReceiptsRoot:  make([]byte, fieldparams.RootLength),
+				LogsBloom:     make([]byte, fieldparams.LogsBloomLength),
+				PrevRandao:    make([]byte, fieldparams.RootLength),
+				BaseFeePerGas: make([]byte, fieldparams.RootLength),
+				BlockHash:     make([]byte, fieldparams.RootLength),
+			},
+		},
+		{
+			name:         "state older than Bellatrix, non empty payload",
+			stateVersion: 1,
+			payload: &enginev1.ExecutionPayload{
+				ParentHash: bytesutil.PadTo([]byte{'a'}, fieldparams.RootLength),
+			},
+		},
+		{
+			name:         "state is Bellatrix, nil payload",
+			stateVersion: 2,
+			payload:      nil,
+		},
+		{
+			name:         "state is Bellatrix, empty payload",
+			stateVersion: 2,
+			payload: &enginev1.ExecutionPayload{
+				ParentHash:    make([]byte, fieldparams.RootLength),
+				FeeRecipient:  make([]byte, fieldparams.FeeRecipientLength),
+				StateRoot:     make([]byte, fieldparams.RootLength),
+				ReceiptsRoot:  make([]byte, fieldparams.RootLength),
+				LogsBloom:     make([]byte, fieldparams.LogsBloomLength),
+				PrevRandao:    make([]byte, fieldparams.RootLength),
+				BaseFeePerGas: make([]byte, fieldparams.RootLength),
+				BlockHash:     make([]byte, fieldparams.RootLength),
+			},
+		},
+		{
+			name:         "state is Bellatrix, non empty payload, empty header",
+			stateVersion: 2,
+			payload: &enginev1.ExecutionPayload{
+				ParentHash: bytesutil.PadTo([]byte{'a'}, fieldparams.RootLength),
+			},
+			header: &ethpb.ExecutionPayloadHeader{
+				ParentHash:       make([]byte, fieldparams.RootLength),
+				FeeRecipient:     make([]byte, fieldparams.FeeRecipientLength),
+				StateRoot:        make([]byte, fieldparams.RootLength),
+				ReceiptsRoot:     make([]byte, fieldparams.RootLength),
+				LogsBloom:        make([]byte, fieldparams.LogsBloomLength),
+				PrevRandao:       make([]byte, fieldparams.RootLength),
+				BaseFeePerGas:    make([]byte, fieldparams.RootLength),
+				BlockHash:        make([]byte, fieldparams.RootLength),
+				TransactionsRoot: make([]byte, fieldparams.RootLength),
+			},
+		},
+		{
+			name:         "state is Bellatrix, non empty payload, non empty header",
+			stateVersion: 2,
+			payload: &enginev1.ExecutionPayload{
+				ParentHash: bytesutil.PadTo([]byte{'a'}, fieldparams.RootLength),
+			},
+			header: &ethpb.ExecutionPayloadHeader{
+				BlockNumber: 1,
+			},
+		},
+		{
+			name:         "state is Bellatrix, non empty payload, nil header",
+			stateVersion: 2,
+			payload: &enginev1.ExecutionPayload{
+				ParentHash: bytesutil.PadTo([]byte{'a'}, fieldparams.RootLength),
+			},
+			errString: "nil header or block body",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			e := &mockPOW.EngineClient{BlockByHashMap: map[[32]byte]*enginev1.ExecutionBlock{}}
+			e.BlockByHashMap[[32]byte{'a'}] = &enginev1.ExecutionBlock{
+				ParentHash:      bytesutil.PadTo([]byte{'b'}, fieldparams.RootLength),
+				TotalDifficulty: "0x2",
+			}
+			e.BlockByHashMap[[32]byte{'b'}] = &enginev1.ExecutionBlock{
+				ParentHash:      bytesutil.PadTo([]byte{'3'}, fieldparams.RootLength),
+				TotalDifficulty: "0x1",
+			}
+			service.cfg.ExecutionEngineCaller = e
+			b := util.HydrateSignedBeaconBlockBellatrix(&ethpb.SignedBeaconBlockBellatrix{})
+			b.Block.Body.ExecutionPayload = tt.payload
+			blk, err := wrapper.WrappedSignedBeaconBlock(b)
+			require.NoError(t, err)
+			err = service.validateMergeTransitionBlock(ctx, tt.stateVersion, tt.header, blk)
+			if tt.errString != "" {
+				require.ErrorContains(t, tt.errString, err)
+			} else {
+				require.NoError(t, err)
+			}
+		})
+	}
+}

beacon-chain/core/blocks/payload.go

@@ -74,7 +74,7 @@ func IsExecutionBlock(body block.BeaconBlockBody) (bool, error) {
 		return false, err
 	default:
 	}
-	return !isEmptyPayload(payload), nil
+	return !IsEmptyPayload(payload), nil
 }
 
 // IsExecutionEnabled returns true if the beacon chain can begin executing.
@@ -240,7 +240,7 @@ func PayloadToHeader(payload *enginev1.ExecutionPayload) (*ethpb.ExecutionPayloa
 	}, nil
 }
 
-func isEmptyPayload(p *enginev1.ExecutionPayload) bool {
+func IsEmptyPayload(p *enginev1.ExecutionPayload) bool {
 	if p == nil {
 		return true
 	}