Save head after FCU (#10466)

* Update head after FCU

* rename function

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

beacon-chain/blockchain/head.go

@@ -24,8 +24,9 @@ import (
 	"go.opencensus.io/trace"
 )
 
-// UpdateHeadWithBalances updates the beacon state head after getting justified balanced from cache.
-func (s *Service) UpdateHeadWithBalances(ctx context.Context) error {
+// UpdateAndSaveHeadWithBalances updates the beacon state head after getting justified balanced from cache.
+// This function is only used in spec-tests, it does save the head after updating it.
+func (s *Service) UpdateAndSaveHeadWithBalances(ctx context.Context) error {
 	cp := s.store.JustifiedCheckpt()
 	if cp == nil {
 		return errors.New("no justified checkpoint")
@@ -35,8 +36,11 @@ func (s *Service) UpdateHeadWithBalances(ctx context.Context) error {
 		msg := fmt.Sprintf("could not read balances for state w/ justified checkpoint %#x", cp.Root)
 		return errors.Wrap(err, msg)
 	}
-
-	return s.updateHead(ctx, balances)
+	headRoot, err := s.updateHead(ctx, balances)
+	if err != nil {
+		return errors.Wrap(err, "could not update head")
+	}
+	return s.saveHead(ctx, headRoot)
 }
 
 // This defines the current chain service's view of head.
@@ -49,18 +53,18 @@ type head struct {
 
 // Determined the head from the fork choice service and saves its new data
 // (head root, head block, and head state) to the local service cache.
-func (s *Service) updateHead(ctx context.Context, balances []uint64) error {
+func (s *Service) updateHead(ctx context.Context, balances []uint64) ([32]byte, error) {
 	ctx, span := trace.StartSpan(ctx, "blockChain.updateHead")
 	defer span.End()
 
 	// Get head from the fork choice service.
 	f := s.store.FinalizedCheckpt()
 	if f == nil {
-		return errNilFinalizedInStore
+		return [32]byte{}, errNilFinalizedInStore
 	}
 	j := s.store.JustifiedCheckpt()
 	if j == nil {
-		return errNilJustifiedInStore
+		return [32]byte{}, errNilJustifiedInStore
 	}
 	// To get head before the first justified epoch, the fork choice will start with origin root
 	// instead of zero hashes.
@@ -76,7 +80,7 @@ func (s *Service) updateHead(ctx context.Context, balances []uint64) error {
 	if !s.cfg.ForkChoiceStore.HasNode(headStartRoot) {
 		jb, err := s.cfg.BeaconDB.Block(ctx, headStartRoot)
 		if err != nil {
-			return err
+			return [32]byte{}, err
 		}
 		if features.Get().EnableForkChoiceDoublyLinkedTree {
 			s.cfg.ForkChoiceStore = doublylinkedtree.New(j.Epoch, f.Epoch)
@@ -84,17 +88,11 @@ func (s *Service) updateHead(ctx context.Context, balances []uint64) error {
 			s.cfg.ForkChoiceStore = protoarray.New(j.Epoch, f.Epoch, bytesutil.ToBytes32(f.Root))
 		}
 		if err := s.insertBlockToForkChoiceStore(ctx, jb.Block(), headStartRoot, f, j); err != nil {
-			return err
+			return [32]byte{}, err
 		}
 	}
 
-	headRoot, err := s.cfg.ForkChoiceStore.Head(ctx, j.Epoch, headStartRoot, balances, f.Epoch)
-	if err != nil {
-		return err
-	}
-
-	// Save head to the local service cache.
-	return s.saveHead(ctx, headRoot)
+	return s.cfg.ForkChoiceStore.Head(ctx, j.Epoch, headStartRoot, balances, f.Epoch)
 }
 
 // This saves head info to the local service cache, it also saves the

beacon-chain/blockchain/head_test.go

@@ -9,6 +9,8 @@ import (
 	types "github.com/prysmaticlabs/eth2-types"
 	mock "github.com/prysmaticlabs/prysm/beacon-chain/blockchain/testing"
 	testDB "github.com/prysmaticlabs/prysm/beacon-chain/db/testing"
+	doublylinkedtree "github.com/prysmaticlabs/prysm/beacon-chain/forkchoice/doubly-linked-tree"
+	"github.com/prysmaticlabs/prysm/beacon-chain/state/stategen"
 	"github.com/prysmaticlabs/prysm/config/features"
 	"github.com/prysmaticlabs/prysm/config/params"
 	ethpbv1 "github.com/prysmaticlabs/prysm/proto/eth/v1"
@@ -154,8 +156,9 @@ func TestUpdateHead_MissingJustifiedRoot(t *testing.T) {
 	service.store.SetJustifiedCheckpt(&ethpb.Checkpoint{Root: r[:]})
 	service.store.SetFinalizedCheckpt(&ethpb.Checkpoint{})
 	service.store.SetBestJustifiedCheckpt(&ethpb.Checkpoint{})
-
-	require.NoError(t, service.updateHead(context.Background(), []uint64{}))
+	headRoot, err := service.updateHead(context.Background(), []uint64{})
+	require.NoError(t, err)
+	require.NoError(t, service.saveHead(context.Background(), headRoot))
 }
 
 func Test_notifyNewHeadEvent(t *testing.T) {
@@ -279,3 +282,43 @@ func TestSaveOrphanedAtts_CanFilter(t *testing.T) {
 	atts := b.Block.Body.Attestations
 	require.DeepNotSSZEqual(t, atts, savedAtts)
 }
+
+func TestUpdateHead_noSavedChanges(t *testing.T) {
+	ctx := context.Background()
+
+	beaconDB := testDB.SetupDB(t)
+	fcs := doublylinkedtree.New(0, 0)
+	opts := []Option{
+		WithDatabase(beaconDB),
+		WithStateGen(stategen.New(beaconDB)),
+		WithForkChoiceStore(fcs),
+	}
+
+	service, err := NewService(ctx, opts...)
+	require.NoError(t, err)
+
+	bellatrixBlk, err := wrapper.WrappedSignedBeaconBlock(util.NewBeaconBlockBellatrix())
+	require.NoError(t, err)
+	bellatrixBlkRoot, err := bellatrixBlk.Block().HashTreeRoot()
+	require.NoError(t, err)
+	require.NoError(t, beaconDB.SaveBlock(ctx, bellatrixBlk))
+	fcp := &ethpb.Checkpoint{
+		Root:  bellatrixBlkRoot[:],
+		Epoch: 1,
+	}
+	service.store.SetFinalizedCheckpt(fcp)
+	service.store.SetJustifiedCheckpt(fcp)
+	require.NoError(t, beaconDB.SaveGenesisBlockRoot(ctx, bellatrixBlkRoot))
+
+	bellatrixState, _ := util.DeterministicGenesisStateBellatrix(t, 2)
+	require.NoError(t, beaconDB.SaveState(ctx, bellatrixState, bellatrixBlkRoot))
+	service.cfg.StateGen.SaveFinalizedState(0, bellatrixBlkRoot, bellatrixState)
+
+	headRoot := service.headRoot()
+	require.Equal(t, [32]byte{}, headRoot)
+
+	newRoot, err := service.updateHead(ctx, []uint64{1, 2})
+	require.NoError(t, err)
+	require.NotEqual(t, headRoot, newRoot)
+	require.Equal(t, headRoot, service.headRoot())
+}

beacon-chain/blockchain/process_block.go

@@ -204,12 +204,16 @@ func (s *Service) onBlock(ctx context.Context, signed block.SignedBeaconBlock, b
 		msg := fmt.Sprintf("could not read balances for state w/ justified checkpoint %#x", justified.Root)
 		return errors.Wrap(err, msg)
 	}
-	if err := s.updateHead(ctx, balances); err != nil {
+	headRoot, err := s.updateHead(ctx, balances)
+	if err != nil {
 		log.WithError(err).Warn("Could not update head")
 	}
 	if _, err := s.notifyForkchoiceUpdate(ctx, s.headBlock().Block(), s.headRoot(), bytesutil.ToBytes32(finalized.Root)); err != nil {
 		return err
 	}
+	if err := s.saveHead(ctx, headRoot); err != nil {
+		return errors.Wrap(err, "could not save head")
+	}
 
 	if err := s.pruneCanonicalAttsFromPool(ctx, blockRoot, signed); err != nil {
 		return err

beacon-chain/blockchain/receive_attestation.go

@@ -161,19 +161,19 @@ func (s *Service) spawnProcessAttestationsRoutine(stateFeed *event.Feed) {
 					log.WithError(err).Errorf("Unable to get justified balances for root %v", justified.Root)
 					continue
 				}
-				prevHead := s.headRoot()
-				if err := s.updateHead(s.ctx, balances); err != nil {
+				newHeadRoot, err := s.updateHead(s.ctx, balances)
+				if err != nil {
 					log.WithError(err).Warn("Resolving fork due to new attestation")
 				}
-				s.notifyEngineIfChangedHead(prevHead)
+				s.notifyEngineIfChangedHead(s.ctx, newHeadRoot)
 			}
 		}
 	}()
 }
 
 // This calls notify Forkchoice Update in the event that the head has changed
-func (s *Service) notifyEngineIfChangedHead(prevHead [32]byte) {
-	if s.headRoot() == prevHead {
+func (s *Service) notifyEngineIfChangedHead(ctx context.Context, newHeadRoot [32]byte) {
+	if s.headRoot() == newHeadRoot {
 		return
 	}
 	finalized := s.store.FinalizedCheckpt()
@@ -189,6 +189,9 @@ func (s *Service) notifyEngineIfChangedHead(prevHead [32]byte) {
 	if err != nil {
 		log.WithError(err).Error("could not notify forkchoice update")
 	}
+	if err := s.saveHead(ctx, newHeadRoot); err != nil {
+		log.WithError(err).Error("could not save head")
+	}
 }
 
 // This processes fork choice attestations from the pool to account for validator votes and fork choice.

beacon-chain/blockchain/receive_attestation_test.go

@@ -134,12 +134,12 @@ func TestNotifyEngineIfChangedHead(t *testing.T) {
 	service, err := NewService(ctx, opts...)
 	require.NoError(t, err)
 
-	service.notifyEngineIfChangedHead(service.headRoot())
+	service.notifyEngineIfChangedHead(ctx, service.headRoot())
 	hookErr := "could not notify forkchoice update"
 	finalizedErr := "could not get finalized checkpoint"
 	require.LogsDoNotContain(t, hook, finalizedErr)
 	require.LogsDoNotContain(t, hook, hookErr)
-	service.notifyEngineIfChangedHead([32]byte{'a'})
+	service.notifyEngineIfChangedHead(ctx, [32]byte{'a'})
 	require.LogsContain(t, hook, finalizedErr)
 
 	hook.Reset()
@@ -157,7 +157,7 @@ func TestNotifyEngineIfChangedHead(t *testing.T) {
 		block: wsb,
 	}
 	service.store.SetFinalizedCheckpt(finalized)
-	service.notifyEngineIfChangedHead([32]byte{'b'})
+	service.notifyEngineIfChangedHead(ctx, [32]byte{'b'})
 	require.LogsDoNotContain(t, hook, finalizedErr)
 	require.LogsDoNotContain(t, hook, hookErr)
 }

testing/spectest/shared/common/forkchoice/runner.go

@@ -141,7 +141,7 @@ func Run(t *testing.T, config string, fork int) {
 						tdBigint.SetBytes(tdInBigEndian)
 					}
 					if step.Check != nil {
-						require.NoError(t, service.UpdateHeadWithBalances(ctx))
+						require.NoError(t, service.UpdateAndSaveHeadWithBalances(ctx))
 						c := step.Check
 						if c.Head != nil {
 							r, err := service.HeadRoot(ctx)