From d42d685f78d7753c3da720b76d1debde36e445e2 Mon Sep 17 00:00:00 2001 From: terence tsao Date: Tue, 7 Jul 2020 19:54:55 -0700 Subject: [PATCH] Update `verifyBlkPreState` to verify only (#6506) * Verify should not return the state * Update tests * Merge branch 'master' of github.com:prysmaticlabs/prysm into refactor * Sync with master * Merge refs/heads/master into refactor * Fixed error msgs. Thanks @rauljordan * Merge branch 'master' of github.com:prysmaticlabs/prysm into refactor * Merge branch 'refactor' of github.com:prysmaticlabs/prysm into refactor * Merge refs/heads/master into refactor * Merge refs/heads/master into refactor * Merge refs/heads/master into refactor --- beacon-chain/blockchain/process_block.go | 20 ++++++++++++++---- .../blockchain/process_block_helpers.go | 21 ++++++------------- beacon-chain/blockchain/process_block_test.go | 18 +++++----------- 3 files changed, 27 insertions(+), 32 deletions(-) diff --git a/beacon-chain/blockchain/process_block.go b/beacon-chain/blockchain/process_block.go index 6c60026954..ad3b2cdfb5 100644 --- a/beacon-chain/blockchain/process_block.go +++ b/beacon-chain/blockchain/process_block.go @@ -182,10 +182,18 @@ func (s *Service) onBlockInitialSyncStateTransition(ctx context.Context, signed b := signed.Block // Retrieve incoming block's pre state. - preState, err := s.verifyBlkPreState(ctx, b) + if err := s.verifyBlkPreState(ctx, b); err != nil { + return err + } + + preState, err := s.stateGen.StateByRootInitialSync(ctx, bytesutil.ToBytes32(signed.Block.ParentRoot)) if err != nil { return err } + if preState == nil { + return fmt.Errorf("nil pre state for slot %d", b.Slot) + } + // To invalidate cache for parent root because pre state will get mutated. s.stateGen.DeleteHotStateInCache(bytesutil.ToBytes32(b.ParentRoot)) @@ -220,12 +228,16 @@ func (s *Service) onBlockBatch(ctx context.Context, blks []*ethpb.SignedBeaconBl b := blks[0].Block // Retrieve incoming block's pre state. - preState, err := s.verifyBlkPreState(ctx, b) + if err := s.verifyBlkPreState(ctx, b); err != nil { + return nil, nil, nil, err + } + preState, err := s.stateGen.StateByRootInitialSync(ctx, bytesutil.ToBytes32(b.ParentRoot)) if err != nil { return nil, nil, nil, err } - // Perform a copy to preserve copy in cache. - preState = preState.Copy() + if preState == nil { + return nil, nil, nil, fmt.Errorf("nil pre state for slot %d", b.Slot) + } jCheckpoints := make([]*ethpb.Checkpoint, len(blks)) fCheckpoints := make([]*ethpb.Checkpoint, len(blks)) diff --git a/beacon-chain/blockchain/process_block_helpers.go b/beacon-chain/blockchain/process_block_helpers.go index 9899b404f1..bcc7a46d80 100644 --- a/beacon-chain/blockchain/process_block_helpers.go +++ b/beacon-chain/blockchain/process_block_helpers.go @@ -36,12 +36,11 @@ func (s *Service) getBlockPreState(ctx context.Context, b *ethpb.BeaconBlock) (* defer span.End() // Verify incoming block has a valid pre state. - preState, err := s.verifyBlkPreState(ctx, b) - if err != nil { + if err := s.verifyBlkPreState(ctx, b); err != nil { return nil, err } - preState, err = s.stateGen.StateByRoot(ctx, bytesutil.ToBytes32(b.ParentRoot)) + preState, err := s.stateGen.StateByRoot(ctx, bytesutil.ToBytes32(b.ParentRoot)) if err != nil { return nil, errors.Wrapf(err, "could not get pre state for slot %d", b.Slot) } @@ -68,7 +67,7 @@ func (s *Service) getBlockPreState(ctx context.Context, b *ethpb.BeaconBlock) (* } // verifyBlkPreState validates input block has a valid pre-state. -func (s *Service) verifyBlkPreState(ctx context.Context, b *ethpb.BeaconBlock) (*stateTrie.BeaconState, error) { +func (s *Service) verifyBlkPreState(ctx context.Context, b *ethpb.BeaconBlock) error { ctx, span := trace.StartSpan(ctx, "chainService.verifyBlkPreState") defer span.End() @@ -77,23 +76,15 @@ func (s *Service) verifyBlkPreState(ctx context.Context, b *ethpb.BeaconBlock) ( // during initial syncing. There's no risk given a state summary object is just a // a subset of the block object. if !s.stateGen.StateSummaryExists(ctx, parentRoot) && !s.beaconDB.HasBlock(ctx, parentRoot) { - return nil, errors.New("could not reconstruct parent state") + return errors.New("could not reconstruct parent state") } if !s.stateGen.HasState(ctx, parentRoot) { if err := s.beaconDB.SaveBlocks(ctx, s.getInitSyncBlocks()); err != nil { - return nil, errors.Wrap(err, "could not save initial sync blocks") + return errors.Wrap(err, "could not save initial sync blocks") } s.clearInitSyncBlocks() } - preState, err := s.stateGen.StateByRootInitialSync(ctx, parentRoot) - if err != nil { - return nil, errors.Wrapf(err, "could not get pre state for slot %d", b.Slot) - } - if preState == nil { - return nil, errors.Wrapf(err, "nil pre state for slot %d", b.Slot) - } - - return preState, nil // No copy needed from newly hydrated state gen object. + return nil } // verifyBlkDescendant validates input block root is a descendant of the diff --git a/beacon-chain/blockchain/process_block_test.go b/beacon-chain/blockchain/process_block_test.go index b2d0362451..e032c5bb5a 100644 --- a/beacon-chain/blockchain/process_block_test.go +++ b/beacon-chain/blockchain/process_block_test.go @@ -2,13 +2,13 @@ package blockchain import ( "context" - "reflect" "strings" "testing" "time" ethpb "github.com/prysmaticlabs/ethereumapis/eth/v1alpha1" "github.com/prysmaticlabs/go-ssz" + "github.com/prysmaticlabs/prysm/beacon-chain/cache" "github.com/prysmaticlabs/prysm/beacon-chain/core/blocks" "github.com/prysmaticlabs/prysm/beacon-chain/core/state" "github.com/prysmaticlabs/prysm/beacon-chain/db" @@ -313,13 +313,9 @@ func TestCachedPreState_CanGetFromStateSummary(t *testing.T) { t.Fatal(err) } - received, err := service.verifyBlkPreState(ctx, b) - if err != nil { + if err := service.verifyBlkPreState(ctx, b); err != nil { t.Fatal(err) } - if !reflect.DeepEqual(s.InnerStateUnsafe(), received.InnerStateUnsafe()) { - t.Error("cached state not the same") - } } func TestCachedPreState_CanGetFromDB(t *testing.T) { @@ -342,7 +338,7 @@ func TestCachedPreState_CanGetFromDB(t *testing.T) { b := ðpb.BeaconBlock{Slot: 1, ParentRoot: r[:]} service.finalizedCheckpt = ðpb.Checkpoint{Root: r[:]} - _, err = service.verifyBlkPreState(ctx, b) + err = service.verifyBlkPreState(ctx, b) wanted := "could not reconstruct parent state" if err.Error() != wanted { t.Error("Did not get wanted error") @@ -359,20 +355,16 @@ func TestCachedPreState_CanGetFromDB(t *testing.T) { t.Fatal(err) } - received, err := service.verifyBlkPreState(ctx, b) - if err != nil { + if err := service.verifyBlkPreState(ctx, b); err != nil { t.Fatal(err) } - if s.Slot() != received.Slot() { - t.Error("cached state not the same") - } } func TestUpdateJustified_CouldUpdateBest(t *testing.T) { ctx := context.Background() db, _ := testDB.SetupDB(t) - cfg := &Config{BeaconDB: db} + cfg := &Config{BeaconDB: db, StateGen: stategen.New(db, cache.NewStateSummaryCache())} service, err := NewService(ctx, cfg) if err != nil { t.Fatal(err)