insert block to forkchoice after notifyNewPayload (#10453)

* insert block to forkchoice after notifyNewPayload

* Add optimistic candidate check

* Terence's review

* Apply suggestions from code review

Co-authored-by: terence tsao <terence@prysmaticlabs.com>

beacon-chain/blockchain/error.go

@@ -16,4 +16,6 @@ var (
 	// errWrongBlockCount is returned when the wrong number of blocks or
 	// block roots is used
 	errWrongBlockCount = errors.New("wrong number of blocks or block roots")
+	// block is not a valid optimistic candidate block
+	errNotOptimisticCandidate = errors.New("block is not suitable for optimistic sync")
 )

beacon-chain/blockchain/optimistic_sync.go

@@ -81,31 +81,32 @@ func (s *Service) notifyForkchoiceUpdate(ctx context.Context, headBlk block.Beac
 	return payloadID, nil
 }
 
-// notifyForkchoiceUpdate signals execution engine on a new payload
+// 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, root [32]byte) error {
+	preStateHeader, postStateHeader *ethpb.ExecutionPayloadHeader, blk block.SignedBeaconBlock) (bool, error) {
 	ctx, span := trace.StartSpan(ctx, "blockChain.notifyNewPayload")
 	defer span.End()
 
 	// Execution payload is only supported in Bellatrix and beyond. Pre
 	// merge blocks are never optimistic
 	if blocks.IsPreBellatrixVersion(postStateVersion) {
-		return s.cfg.ForkChoiceStore.SetOptimisticToValid(ctx, root)
+		return true, nil
 	}
 	if err := helpers.BeaconBlockIsNil(blk); err != nil {
-		return err
+		return false, err
 	}
 	body := blk.Block().Body()
 	enabled, err := blocks.IsExecutionEnabledUsingHeader(postStateHeader, body)
 	if err != nil {
-		return errors.Wrap(err, "could not determine if execution is enabled")
+		return false, errors.Wrap(err, "could not determine if execution is enabled")
 	}
 	if !enabled {
-		return s.cfg.ForkChoiceStore.SetOptimisticToValid(ctx, root)
+		return true, nil
 	}
 	payload, err := body.ExecutionPayload()
 	if err != nil {
-		return errors.Wrap(err, "could not get execution payload")
+		return false, errors.Wrap(err, "could not get execution payload")
 	}
 	_, err = s.cfg.ExecutionEngineCaller.NewPayload(ctx, payload)
 	if err != nil {
@@ -115,30 +116,26 @@ func (s *Service) notifyNewPayload(ctx context.Context, preStateVersion, postSta
 				"slot":      blk.Block().Slot(),
 				"blockHash": fmt.Sprintf("%#x", bytesutil.Trunc(payload.BlockHash)),
 			}).Info("Called new payload with optimistic block")
-			return nil
+			return false, nil
 		default:
-			return errors.Wrap(err, "could not validate execution payload from execution engine")
+			return false, errors.Wrap(err, "could not validate execution payload from execution engine")
 		}
 	}
 
-	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 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 s.validateMergeBlock(ctx, blk)
+		return true, s.validateMergeBlock(ctx, blk)
 	}
 	atTransition, err := blocks.IsMergeTransitionBlockUsingPreStatePayloadHeader(preStateHeader, body)
 	if err != nil {
-		return errors.Wrap(err, "could not check if merge block is terminal")
+		return true, errors.Wrap(err, "could not check if merge block is terminal")
 	}
 	if !atTransition {
-		return nil
+		return true, nil
 	}
-	return s.validateMergeBlock(ctx, blk)
+	return true, s.validateMergeBlock(ctx, blk)
 }
 
 // optimisticCandidateBlock returns true if this block can be optimistically synced.
@@ -148,10 +145,6 @@ func (s *Service) notifyNewPayload(ctx context.Context, preStateVersion, postSta
 //    if is_execution_block(opt_store.blocks[block.parent_root]):
 //        return True
 //
-//    justified_root = opt_store.block_states[opt_store.head_block_root].current_justified_checkpoint.root
-//    if is_execution_block(opt_store.blocks[justified_root]):
-//        return True
-//
 //    if block.slot + SAFE_SLOTS_TO_IMPORT_OPTIMISTICALLY <= current_slot:
 //        return True
 //
@@ -173,17 +166,5 @@ func (s *Service) optimisticCandidateBlock(ctx context.Context, blk block.Beacon
 	if err != nil {
 		return false, err
 	}
-	if parentIsExecutionBlock {
-		return true, nil
-	}
-
-	j := s.store.JustifiedCheckpt()
-	if j == nil {
-		return false, errNilJustifiedInStore
-	}
-	jBlock, err := s.cfg.BeaconDB.Block(ctx, bytesutil.ToBytes32(j.Root))
-	if err != nil {
-		return false, err
-	}
-	return blocks.IsExecutionBlock(jBlock.Block().Body())
+	return parentIsExecutionBlock, nil
 }

beacon-chain/blockchain/optimistic_sync_test.go

@@ -202,49 +202,56 @@ func Test_NotifyNewPayload(t *testing.T) {
 	require.NoError(t, err)
 
 	tests := []struct {
-		name          string
-		preState      state.BeaconState
-		postState     state.BeaconState
-		blk           block.SignedBeaconBlock
-		newPayloadErr error
-		errString     string
+		name           string
+		preState       state.BeaconState
+		postState      state.BeaconState
+		isValidPayload bool
+		blk            block.SignedBeaconBlock
+		newPayloadErr  error
+		errString      string
 	}{
 		{
-			name:      "phase 0 post state",
-			postState: phase0State,
-			preState:  phase0State,
+			name:           "phase 0 post state",
+			postState:      phase0State,
+			preState:       phase0State,
+			isValidPayload: true,
 		},
 		{
-			name:      "altair post state",
-			postState: altairState,
-			preState:  altairState,
+			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",
+			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: engine.ErrAcceptedSyncingPayloadStatus,
+			name:           "new payload with optimistic block",
+			postState:      bellatrixState,
+			preState:       bellatrixState,
+			blk:            bellatrixBlk,
+			newPayloadErr:  engine.ErrAcceptedSyncingPayloadStatus,
+			isValidPayload: false,
 		},
 		{
-			name:          "new payload with invalid block",
-			postState:     bellatrixState,
-			preState:      bellatrixState,
-			blk:           bellatrixBlk,
-			newPayloadErr: engine.ErrInvalidPayloadStatus,
-			errString:     "could not validate execution payload from execution engine: payload status is INVALID",
+			name:           "new payload with invalid block",
+			postState:      bellatrixState,
+			preState:       bellatrixState,
+			blk:            bellatrixBlk,
+			newPayloadErr:  engine.ErrInvalidPayloadStatus,
+			errString:      "could not validate execution payload from execution engine: payload status is INVALID",
+			isValidPayload: false,
 		},
 		{
-			name:      "altair pre state, altair block",
-			postState: bellatrixState,
-			preState:  altairState,
-			blk:       altairBlk,
+			name:           "altair pre state, altair block",
+			postState:      bellatrixState,
+			preState:       altairState,
+			blk:            altairBlk,
+			isValidPayload: true,
 		},
 		{
 			name:      "altair pre state, happy case",
@@ -264,13 +271,15 @@ func Test_NotifyNewPayload(t *testing.T) {
 				require.NoError(t, err)
 				return b
 			}(),
+			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",
+			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",
@@ -297,13 +306,15 @@ func Test_NotifyNewPayload(t *testing.T) {
 				require.NoError(t, err)
 				return b
 			}(),
+			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",
+			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",
@@ -323,6 +334,7 @@ func Test_NotifyNewPayload(t *testing.T) {
 				require.NoError(t, err)
 				return b
 			}(),
+			isValidPayload: true,
 		},
 	}
 	for _, tt := range tests {
@@ -346,11 +358,12 @@ func Test_NotifyNewPayload(t *testing.T) {
 			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)
-			err = service.notifyNewPayload(ctx, tt.preState.Version(), postVersion, payload, postHeader, tt.blk, root)
+			isValidPayload, err := service.notifyNewPayload(ctx, tt.preState.Version(), postVersion, payload, postHeader, tt.blk)
 			if tt.errString != "" {
 				require.ErrorContains(t, tt.errString, err)
 			} else {
 				require.NoError(t, err)
+				require.Equal(t, tt.isValidPayload, isValidPayload)
 			}
 		})
 	}
@@ -394,15 +407,11 @@ func Test_NotifyNewPayload_SetOptimisticToValid(t *testing.T) {
 	service.cfg.ExecutionEngineCaller = e
 	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))
 	postVersion, postHeader, err := getStateVersionAndPayload(bellatrixState)
 	require.NoError(t, err)
-	err = service.notifyNewPayload(ctx, bellatrixState.Version(), postVersion, payload, postHeader, bellatrixBlk, root)
+	validated, err := service.notifyNewPayload(ctx, bellatrixState.Version(), postVersion, payload, postHeader, bellatrixBlk)
 	require.NoError(t, err)
-	optimistic, err := service.IsOptimisticForRoot(ctx, root)
-	require.NoError(t, err)
-	require.Equal(t, false, optimistic)
+	require.Equal(t, true, validated)
 }
 
 func Test_IsOptimisticCandidateBlock(t *testing.T) {
@@ -496,34 +505,6 @@ func Test_IsOptimisticCandidateBlock(t *testing.T) {
 			}(t),
 			want: false,
 		},
-		{
-			name: "shallow block, execution enabled justified chkpt",
-			blk: func(tt *testing.T) block.BeaconBlock {
-				blk := util.NewBeaconBlockBellatrix()
-				blk.Block.Slot = 200
-				blk.Block.ParentRoot = parentRoot[:]
-				wr, err := wrapper.WrappedBeaconBlock(blk.Block)
-				require.NoError(tt, err)
-				return wr
-			}(t),
-			justified: func(tt *testing.T) block.SignedBeaconBlock {
-				blk := util.NewBeaconBlockBellatrix()
-				blk.Block.Slot = 32
-				blk.Block.ParentRoot = parentRoot[:]
-				blk.Block.Body.ExecutionPayload.ParentHash = bytesutil.PadTo([]byte{'a'}, fieldparams.RootLength)
-				blk.Block.Body.ExecutionPayload.FeeRecipient = bytesutil.PadTo([]byte{'a'}, fieldparams.FeeRecipientLength)
-				blk.Block.Body.ExecutionPayload.StateRoot = bytesutil.PadTo([]byte{'a'}, fieldparams.RootLength)
-				blk.Block.Body.ExecutionPayload.ReceiptsRoot = bytesutil.PadTo([]byte{'a'}, fieldparams.RootLength)
-				blk.Block.Body.ExecutionPayload.LogsBloom = bytesutil.PadTo([]byte{'a'}, fieldparams.LogsBloomLength)
-				blk.Block.Body.ExecutionPayload.PrevRandao = bytesutil.PadTo([]byte{'a'}, fieldparams.RootLength)
-				blk.Block.Body.ExecutionPayload.BaseFeePerGas = bytesutil.PadTo([]byte{'a'}, fieldparams.RootLength)
-				blk.Block.Body.ExecutionPayload.BlockHash = bytesutil.PadTo([]byte{'a'}, fieldparams.RootLength)
-				wr, err := wrapper.WrappedSignedBeaconBlock(blk)
-				require.NoError(tt, err)
-				return wr
-			}(t),
-			want: true,
-		},
 	}
 	for _, tt := range tests {
 		jRoot, err := tt.justified.Block().HashTreeRoot()

beacon-chain/blockchain/process_block.go

@@ -109,16 +109,32 @@ func (s *Service) onBlock(ctx context.Context, signed block.SignedBeaconBlock, b
 	if err != nil {
 		return err
 	}
-	if err := s.savePostStateInfo(ctx, blockRoot, signed, postState, false /* reg sync */); err != nil {
-		return err
-	}
 	postStateVersion, postStateHeader, err := getStateVersionAndPayload(postState)
 	if err != nil {
 		return err
 	}
-	if err := s.notifyNewPayload(ctx, preStateVersion, postStateVersion, preStateHeader, postStateHeader, signed, blockRoot); err != nil {
+	isValidPayload, err := s.notifyNewPayload(ctx, preStateVersion, postStateVersion, preStateHeader, postStateHeader, signed)
+	if err != nil {
 		return errors.Wrap(err, "could not verify new payload")
 	}
+	if !isValidPayload {
+		candidate, err := s.optimisticCandidateBlock(ctx, b)
+		if err != nil {
+			return errors.Wrap(err, "could not check if block is optimistic candidate")
+		}
+		if !candidate {
+			return errNotOptimisticCandidate
+		}
+	}
+
+	if err := s.savePostStateInfo(ctx, blockRoot, signed, postState, false /* reg sync */); err != nil {
+		return err
+	}
+	if isValidPayload {
+		if err := s.cfg.ForkChoiceStore.SetOptimisticToValid(ctx, blockRoot); err != nil {
+			return errors.Wrap(err, "could not set optimistic block to valid")
+		}
+	}
 
 	// 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.
@@ -376,16 +392,33 @@ 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 {
 		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,
+		isValidPayload, err := s.notifyNewPayload(ctx,
 			preVersionAndHeaders[i].version,
 			postVersionAndHeaders[i].version,
 			preVersionAndHeaders[i].header,
-			postVersionAndHeaders[i].header, b, blockRoots[i]); err != nil {
+			postVersionAndHeaders[i].header, b)
+
+		if err != nil {
 			return nil, nil, err
 		}
+		if !isValidPayload {
+			candidate, err := s.optimisticCandidateBlock(ctx, b.Block())
+			if err != nil {
+				return nil, nil, errors.Wrap(err, "could not check if block is optimistic candidate")
+			}
+			if !candidate {
+				return nil, nil, errNotOptimisticCandidate
+			}
+		}
+
+		if err := s.insertBlockToForkChoiceStore(ctx, b.Block(), blockRoots[i], fCheckpoints[i], jCheckpoints[i]); err != nil {
+			return nil, nil, err
+		}
+		if isValidPayload {
+			if err := s.cfg.ForkChoiceStore.SetOptimisticToValid(ctx, blockRoots[i]); err != nil {
+				return nil, nil, errors.Wrap(err, "could not set optimistic block to valid")
+			}
+		}
 
 		if _, err := s.notifyForkchoiceUpdate(ctx, b.Block(), blockRoots[i], bytesutil.ToBytes32(fCheckpoints[i].Root)); err != nil {
 			return nil, nil, err