set optimistic status on notify payload (#10398)

* set optimistic status on notify payload

* pre-execution blocks are never optimistic

* add unit test

* add test for lengths

* Update optimistic_sync_test.go

Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com>
Co-authored-by: terence tsao <terence@prysmaticlabs.com>

beacon-chain/blockchain/error.go

@@ -13,4 +13,7 @@ var (
 	errInvalidNilSummary = errors.New("nil summary returned from the DB")
 	// errNilParentInDB is returned when a nil parent block is returned from the DB.
 	errNilParentInDB = errors.New("nil parent block in DB")
+	// errWrongBlockCound is returned when the wrong number of blocks or
+	// block roots is used
+	errWrongBlockCount = errors.New("wrong number of blocks or block roots")
 )

beacon-chain/blockchain/optimistic_sync.go

@@ -83,13 +83,14 @@ func (s *Service) notifyForkchoiceUpdate(ctx context.Context, headBlk block.Beac
 }
 
 // notifyForkchoiceUpdate signals execution engine on a new payload
-func (s *Service) notifyNewPayload(ctx context.Context, preStateVersion int, header *ethpb.ExecutionPayloadHeader, postState state.BeaconState, blk block.SignedBeaconBlock) error {
+func (s *Service) notifyNewPayload(ctx context.Context, preStateVersion int, header *ethpb.ExecutionPayloadHeader, postState state.BeaconState, blk block.SignedBeaconBlock, root [32]byte) error {
 	if postState == nil {
 		return errors.New("pre and post states must not be nil")
 	}
-	// Execution payload is only supported in Bellatrix and beyond.
+	// Execution payload is only supported in Bellatrix and beyond. Pre
+	// merge blocks are never optimistic
 	if isPreBellatrix(postState.Version()) {
-		return nil
+		return s.cfg.ForkChoiceStore.SetOptimisticToValid(ctx, root)
 	}
 	if err := helpers.BeaconBlockIsNil(blk); err != nil {
 		return err
@@ -100,7 +101,7 @@ func (s *Service) notifyNewPayload(ctx context.Context, preStateVersion int, hea
 		return errors.Wrap(err, "could not determine if execution is enabled")
 	}
 	if !enabled {
-		return nil
+		return s.cfg.ForkChoiceStore.SetOptimisticToValid(ctx, root)
 	}
 	payload, err := body.ExecutionPayload()
 	if err != nil {
@@ -120,6 +121,10 @@ func (s *Service) notifyNewPayload(ctx context.Context, preStateVersion int, hea
 		}
 	}
 
+	if err := s.cfg.ForkChoiceStore.SetOptimisticToValid(ctx, root); err != nil {
+		return errors.Wrap(err, "could not set optimistic status")
+	}
+
 	// During the transition event, the transition block should be verified for sanity.
 	if isPreBellatrix(preStateVersion) {
 		// Handle case where pre-state is Altair but block contains payload.

beacon-chain/blockchain/optimistic_sync_test.go

@@ -346,7 +346,9 @@ func Test_NotifyNewPayload(t *testing.T) {
 				payload, err = tt.preState.LatestExecutionPayloadHeader()
 				require.NoError(t, err)
 			}
-			err := service.notifyNewPayload(ctx, tt.preState.Version(), payload, tt.postState, tt.blk)
+			root := [32]byte{'a'}
+			require.NoError(t, service.cfg.ForkChoiceStore.InsertOptimisticBlock(ctx, 0, root, root, params.BeaconConfig().ZeroHash, 0, 0))
+			err = service.notifyNewPayload(ctx, tt.preState.Version(), payload, tt.postState, tt.blk, root)
 			if tt.errString != "" {
 				require.ErrorContains(t, tt.errString, err)
 			} else {
@@ -356,6 +358,53 @@ func Test_NotifyNewPayload(t *testing.T) {
 	}
 }
 
+func Test_NotifyNewPayload_SetOptimisticToValid(t *testing.T) {
+	cfg := params.BeaconConfig()
+	cfg.TerminalTotalDifficulty = "2"
+	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),
+	}
+	bellatrixState, _ := util.DeterministicGenesisStateBellatrix(t, 2)
+	blk := &ethpb.SignedBeaconBlockBellatrix{
+		Block: &ethpb.BeaconBlockBellatrix{
+			Body: &ethpb.BeaconBlockBodyBellatrix{
+				ExecutionPayload: &v1.ExecutionPayload{
+					ParentHash: bytesutil.PadTo([]byte{'a'}, fieldparams.RootLength),
+				},
+			},
+		},
+	}
+	bellatrixBlk, err := wrapper.WrappedSignedBeaconBlock(blk)
+	require.NoError(t, err)
+	service, err := NewService(ctx, opts...)
+	require.NoError(t, err)
+	engine := &mockEngineService{blks: map[[32]byte]*v1.ExecutionBlock{}}
+	engine.blks[[32]byte{'a'}] = &v1.ExecutionBlock{
+		ParentHash:      bytesutil.PadTo([]byte{'b'}, fieldparams.RootLength),
+		TotalDifficulty: "0x2",
+	}
+	engine.blks[[32]byte{'b'}] = &v1.ExecutionBlock{
+		ParentHash:      bytesutil.PadTo([]byte{'3'}, fieldparams.RootLength),
+		TotalDifficulty: "0x1",
+	}
+	service.cfg.ExecutionEngineCaller = engine
+	payload, err := bellatrixState.LatestExecutionPayloadHeader()
+	require.NoError(t, err)
+	root := [32]byte{'c'}
+	require.NoError(t, service.cfg.ForkChoiceStore.InsertOptimisticBlock(ctx, 1, root, [32]byte{'a'}, params.BeaconConfig().ZeroHash, 0, 0))
+	err = service.notifyNewPayload(ctx, bellatrixState.Version(), payload, bellatrixState, bellatrixBlk, root)
+	require.NoError(t, err)
+	optimistic, err := service.IsOptimisticForRoot(ctx, root)
+	require.NoError(t, err)
+	require.Equal(t, false, optimistic)
+}
+
 func Test_IsOptimisticCandidateBlock(t *testing.T) {
 	params.SetupTestConfigCleanup(t)
 	params.OverrideBeaconConfig(params.MainnetConfig())

beacon-chain/blockchain/process_block.go

@@ -107,13 +107,12 @@ func (s *Service) onBlock(ctx context.Context, signed block.SignedBeaconBlock, b
 	if err != nil {
 		return err
 	}
-	if err := s.notifyNewPayload(ctx, preStateVersion, preStateHeader, postState, signed); err != nil {
-		return errors.Wrap(err, "could not verify new payload")
-	}
-
 	if err := s.savePostStateInfo(ctx, blockRoot, signed, postState, false /* reg sync */); err != nil {
 		return err
 	}
+	if err := s.notifyNewPayload(ctx, preStateVersion, preStateHeader, postState, signed, blockRoot); err != nil {
+		return errors.Wrap(err, "could not verify new payload")
+	}
 
 	// We add a proposer score boost to fork choice for the block root if applicable, right after
 	// running a successful state transition for the block.
@@ -280,6 +279,11 @@ func (s *Service) onBlockBatch(ctx context.Context, blks []block.SignedBeaconBlo
 	if len(blks) == 0 || len(blockRoots) == 0 {
 		return nil, nil, errors.New("no blocks provided")
 	}
+
+	if len(blks) != len(blockRoots) {
+		return nil, nil, errWrongBlockCount
+	}
+
 	if err := helpers.BeaconBlockIsNil(blks[0]); err != nil {
 		return nil, nil, err
 	}
@@ -307,17 +311,10 @@ func (s *Service) onBlockBatch(ctx context.Context, blks []block.SignedBeaconBlo
 	var set *bls.SignatureBatch
 	boundaries := make(map[[32]byte]state.BeaconState)
 	for i, b := range blks {
-		preStateVersion, preStateHeader, err := getStateVersionAndPayload(preState)
-		if err != nil {
-			return nil, nil, err
-		}
 		set, preState, err = transition.ExecuteStateTransitionNoVerifyAnySig(ctx, preState, b)
 		if err != nil {
 			return nil, nil, err
 		}
-		if err := s.notifyNewPayload(ctx, preStateVersion, preStateHeader, preState, b); err != nil {
-			return nil, nil, err
-		}
 		// Save potential boundary states.
 		if slots.IsEpochStart(preState.Slot()) {
 			boundaries[blockRoots[i]] = preState.Copy()
@@ -336,6 +333,25 @@ func (s *Service) onBlockBatch(ctx context.Context, blks []block.SignedBeaconBlo
 	if !verify {
 		return nil, nil, errors.New("batch block signature verification failed")
 	}
+
+	// blocks have been verified, add them to forkchoice and call the engine
+	for i, b := range blks {
+		preStateVersion, preStateHeader, err := getStateVersionAndPayload(preState)
+		if err != nil {
+			return nil, nil, err
+		}
+
+		s.saveInitSyncBlock(blockRoots[i], b)
+		if err := s.insertBlockToForkChoiceStore(ctx, b.Block(), blockRoots[i], fCheckpoints[i], jCheckpoints[i]); err != nil {
+			return nil, nil, err
+		}
+		if err := s.notifyNewPayload(ctx, preStateVersion, preStateHeader, preState, b, blockRoots[i]); err != nil {
+			return nil, nil, err
+		}
+		if _, err := s.notifyForkchoiceUpdate(ctx, b.Block(), blockRoots[i], bytesutil.ToBytes32(fCheckpoints[i].Root)); err != nil {
+			return nil, nil, err
+		}
+	}
 	for r, st := range boundaries {
 		if err := s.cfg.StateGen.SaveState(ctx, r, st); err != nil {
 			return nil, nil, err
@@ -357,15 +373,6 @@ func (s *Service) onBlockBatch(ctx context.Context, blks []block.SignedBeaconBlo
 // their state summaries and split them off to relative hot/cold storage.
 func (s *Service) handleBlockAfterBatchVerify(ctx context.Context, signed block.SignedBeaconBlock,
 	blockRoot [32]byte, fCheckpoint, jCheckpoint *ethpb.Checkpoint) error {
-	b := signed.Block()
-
-	s.saveInitSyncBlock(blockRoot, signed)
-	if err := s.insertBlockToForkChoiceStore(ctx, b, blockRoot, fCheckpoint, jCheckpoint); err != nil {
-		return err
-	}
-	if _, err := s.notifyForkchoiceUpdate(ctx, b, blockRoot, bytesutil.ToBytes32(fCheckpoint.Root)); err != nil {
-		return err
-	}
 
 	if err := s.cfg.BeaconDB.SaveStateSummary(ctx, &ethpb.StateSummary{
 		Slot: signed.Block().Slot(),

beacon-chain/blockchain/process_block_test.go

@@ -295,6 +295,8 @@ func TestStore_OnBlockBatch_ProtoArray(t *testing.T) {
 	rBlock.Block.ParentRoot = gRoot[:]
 	require.NoError(t, beaconDB.SaveBlock(context.Background(), blks[0]))
 	require.NoError(t, service.cfg.StateGen.SaveState(ctx, blkRoots[0], firstState))
+	_, _, err = service.onBlockBatch(ctx, blks, blkRoots[1:])
+	require.ErrorIs(t, errWrongBlockCount, err)
 	_, _, err = service.onBlockBatch(ctx, blks[1:], blkRoots[1:])
 	require.NoError(t, err)
 }
@@ -347,6 +349,8 @@ func TestStore_OnBlockBatch_DoublyLinkedTree(t *testing.T) {
 	rBlock.Block.ParentRoot = gRoot[:]
 	require.NoError(t, beaconDB.SaveBlock(context.Background(), blks[0]))
 	require.NoError(t, service.cfg.StateGen.SaveState(ctx, blkRoots[0], firstState))
+	_, _, err = service.onBlockBatch(ctx, blks, blkRoots[1:])
+	require.ErrorIs(t, errWrongBlockCount, err)
 	_, _, err = service.onBlockBatch(ctx, blks[1:], blkRoots[1:])
 	require.NoError(t, err)
 }