From ce15823f8d76498ece9da0c93d05c3f8a3ae4497 Mon Sep 17 00:00:00 2001 From: Potuz Date: Thu, 31 Mar 2022 21:08:01 -0300 Subject: [PATCH] Save head after FCU (#10466) * Update head after FCU * rename function Co-authored-by: terence tsao --- beacon-chain/blockchain/head.go | 30 ++++++------ beacon-chain/blockchain/head_test.go | 47 ++++++++++++++++++- beacon-chain/blockchain/process_block.go | 6 ++- .../blockchain/receive_attestation.go | 13 +++-- .../blockchain/receive_attestation_test.go | 6 +-- .../shared/common/forkchoice/runner.go | 2 +- 6 files changed, 76 insertions(+), 28 deletions(-) diff --git a/beacon-chain/blockchain/head.go b/beacon-chain/blockchain/head.go index 9e7b2dc4d..bc6df210d 100644 --- a/beacon-chain/blockchain/head.go +++ b/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 diff --git a/beacon-chain/blockchain/head_test.go b/beacon-chain/blockchain/head_test.go index e51cb3669..b0440909f 100644 --- a/beacon-chain/blockchain/head_test.go +++ b/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(ðpb.Checkpoint{Root: r[:]}) service.store.SetFinalizedCheckpt(ðpb.Checkpoint{}) service.store.SetBestJustifiedCheckpt(ðpb.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 := ðpb.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()) +} diff --git a/beacon-chain/blockchain/process_block.go b/beacon-chain/blockchain/process_block.go index e1b655d4a..7702787da 100644 --- a/beacon-chain/blockchain/process_block.go +++ b/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 diff --git a/beacon-chain/blockchain/receive_attestation.go b/beacon-chain/blockchain/receive_attestation.go index 035637b8d..e0bddd9cc 100644 --- a/beacon-chain/blockchain/receive_attestation.go +++ b/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. diff --git a/beacon-chain/blockchain/receive_attestation_test.go b/beacon-chain/blockchain/receive_attestation_test.go index 70d3b23b9..c1643228b 100644 --- a/beacon-chain/blockchain/receive_attestation_test.go +++ b/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) } diff --git a/testing/spectest/shared/common/forkchoice/runner.go b/testing/spectest/shared/common/forkchoice/runner.go index cbb303785..882034c4e 100644 --- a/testing/spectest/shared/common/forkchoice/runner.go +++ b/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)