Deprecate store in blockchain pkg #10903

potuz · 2022-06-17T19:10:27Z

In this PR we remove package store and let forkchoice track all justification and finalization information.

It removes several functions from the blockchain package like updateJustified and updateJustifiedInitSync. It also simplifies considerably block processing so that functions like handleBlockAfterBatchVerify are removed. Justification/Finalization update is purely done on block insertion and on on_tick within forkchoice to be more aligned with how the spec does it.

Future changes that are not covered in this PR are:

deprecation of blockchain's head structure.
tracking of justified balances within forkchoice
cleanup of all database accesses within blockchain when they should be handled purely within forkchoice. An example of this is already merged in Use forkchoice to verify finalized descendant #10905

Fixes: #10734

- TestFinalizedCheckpt_GenesisRootOk - TestCurrentJustifiedCheckpt_CanRetrieve - TestJustifiedCheckpt_GenesisRootOk - TestHeadRoot_CanRetrieve - TestHeadRoot_UseDB - TestService_ProcessAttestationsAndUpdateHead - TestService_VerifyWeakSubjectivityRoot - TestVerifyFinalizedConsistency_InconsistentRoot_ProtoArray - TestVerifyFinalizedConsistency_InconsistentRoot_DoublyLinkedTree - TestVerifyFinalizedConsistency_Ok - TestStore_OnBlock_ProtoArray - TestStore_OnBlock_DoublyLinkedTree - TestStore_OnBlockBatch_ProtoArray - TestStore_OnBlockBatch_DoublyLinkedTree - TestStore_OnBlockBatch_NotifyNewPayload - TestCachedPreState_CanGetFromStateSummary_ProtoArray - TestCachedPreState_CanGetFromStateSummary_DoublyLinkedTree

- TestStore_OnBlockBatch_PruneOK_Protoarray - TestFillForkChoiceMissingBlocks_CanSave_ProtoArray - TestFillForkChoiceMissingBlocks_CanSave_DoublyLinkedTree - TestFillForkChoiceMissingBlocks_RootsMatch_Protoarray - TestFillForkChoiceMissingBlocks_RootsMatch_DoublyLinkedTree - TestFillForkChoiceMissingBlocks_FilterFinalized_ProtoArray - TestFillForkChoiceMissingBlocks_FilterFinalized_DoublyLinkedTree - TestVerifyBlkDescendant - Test_verifyBlkFinalizedSlot_invalidBlock - TestChainService_SaveHeadNoDB

potuz · 2022-06-20T00:43:30Z

beacon-chain/blockchain/chain_info.go

-	FinalizedCheckpt() (*ethpb.Checkpoint, error)
-	CurrentJustifiedCheckpt() (*ethpb.Checkpoint, error)
-	PreviousJustifiedCheckpt() (*ethpb.Checkpoint, error)
+	FinalizedCheckpt() *ethpb.Checkpoint


We will move all these to *forkchoicetypes.Checkpoint in a future PR

potuz · 2022-06-20T00:45:58Z

beacon-chain/blockchain/init_sync_process_block.go

-func (s *Service) saveInitSyncBlock(r [32]byte, b interfaces.SignedBeaconBlock) {
+// This saves a beacon block to the initial sync blocks cache. It rate limits how many blocks
+// the cache keeps in memory (2 epochs worth of blocks) and saves them to DB when it hits this limit.
+func (s *Service) saveInitSyncBlock(ctx context.Context, r [32]byte, b interfaces.SignedBeaconBlock) error {


The logic of this function is changed: it prunes the blocks as they are being saved.

This seems like a violation of the single responsibility principle, any reason we want to couple them together like this ?

Because every single call to this function came with a direct call afterward to prune, since we want to keep the cache to a fixed size, then here's the best point to prune immediately. But I don't feel strongly about this, I can revert back to the previous behavior.

potuz · 2022-06-20T00:46:56Z

beacon-chain/blockchain/process_block.go

@@ -178,52 +182,8 @@ func (s *Service) onBlock(ctx context.Context, signed interfaces.SignedBeaconBlo
 		}()
 	}

-	// Update justified check point.


We do not need to update justification any more on store as this was already taken care by inserting the block to forkchoice.

potuz · 2022-06-20T00:47:41Z

beacon-chain/blockchain/process_block.go

-
-	balances, err := s.justifiedBalances.get(ctx, bytesutil.ToBytes32(justified.Root))
+	justified := s.ForkChoicer().JustifiedCheckpoint()
+	balances, err := s.justifiedBalances.get(ctx, justified.Root)


We are studying moving the justified balance tracking to forkchoice in a future PR

potuz · 2022-06-20T00:48:40Z

beacon-chain/blockchain/process_block.go

+			tracing.AnnotateError(span, err)
+			return err
+		}
+		if err := s.cfg.BeaconDB.SaveStateSummary(ctx, &ethpb.StateSummary{


This is all that's left of handleBlockAfterBatchVerify so it's better to inline it here.

potuz · 2022-06-20T00:50:13Z

beacon-chain/blockchain/process_block_helpers.go

-// This caches input checkpoint as justified for the service struct. It rotates current justified to previous justified,
-// caches justified checkpoint balances for fork choice and save justified checkpoint in DB.
-// This method does not have defense against fork choice bouncing attack, which is why it's only recommend to be used during initial syncing.
-func (s *Service) updateJustifiedInitSync(ctx context.Context, cp *ethpb.Checkpoint) error {


This function is deprecated and removed since anyway when we insert nodes, batched or not, we will be updating the justification info. Bouncing attack is irrelevant here since the blocks come in batches.

potuz · 2022-06-20T00:52:57Z

beacon-chain/blockchain/process_block_test.go

 	bState := st.Copy()

 	var blks []interfaces.SignedBeaconBlock
 	var blkRoots [][32]byte
-	var firstState state.BeaconState
-	for i := 1; i < 128; i++ {
+	for i := 0; i < 320; i++ {


This change is because forkchoice prunes automatically now, so we insert the minimal number of nodes so that it triggers a finalization above the treshold (which is 256 blocks, we need 64 more to account for the justified and current epochs).

potuz · 2022-06-20T00:54:27Z

beacon-chain/blockchain/process_block_test.go

-	require.NoError(t, err)
-}
-
-func TestRemoveStateSinceLastFinalized_EmptyStartSlot(t *testing.T) {


These next three tests are irrelevant now since the update logic is now tested in forkchoice.

potuz · 2022-06-20T00:56:38Z

beacon-chain/blockchain/process_block_test.go

 	err = service.fillInForkChoiceMissingBlocks(
 		context.Background(), wsb.Block(), beaconState.FinalizedCheckpoint(), beaconState.CurrentJustifiedCheckpoint())
 	require.NoError(t, err)

 	// 4 nodes from the block tree 1. B3 - B4 - B6 - B8
-	assert.Equal(t, 4, service.cfg.ForkChoiceStore.NodeCount(), "Miss match nodes")
+	// plus the origin block root
+	assert.Equal(t, 5, service.cfg.ForkChoiceStore.NodeCount(), "Miss match nodes")


The reason this number is different is because the node tree in the test setup is botched starting with a block at slot 0. I am adding artificially the genesis block to forkchoice above so that the chain being added is a descendant of it, which is now enforced in forkchoice.

potuz · 2022-06-20T00:57:35Z

beacon-chain/blockchain/process_block_test.go

@@ -1436,15 +1108,15 @@ func TestVerifyBlkDescendant(t *testing.T) {
 		WithForkChoiceStore(fcs),
 	}
 	b := util.NewBeaconBlock()
-	b.Block.Slot = 1
+	b.Block.Slot = 32


Since justification is now tracked in forkchoice, we can´t really have a justified root from epoch 0 as it should default to the genesis state.

potuz · 2022-06-20T01:23:32Z

beacon-chain/forkchoice/doubly-linked-tree/forkchoice.go

 	f.store.justifiedCheckpoint = jc
 	bj := f.store.bestJustifiedCheckpoint
-	if bj == nil || jc.Epoch > bj.Epoch {
+	if bj == nil || bj.Root == params.BeaconConfig().ZeroHash || jc.Epoch > bj.Epoch {


These setters are only used at launch, we should consider moving these checkpoints to New(). It is not done in this PR because of the extra large number of tests that would fail from such a change.

potuz · 2022-06-20T01:25:46Z

beacon-chain/forkchoice/doubly-linked-tree/forkchoice.go

@@ -546,3 +567,40 @@ func (f *ForkChoice) SetGenesisTime(genesisTime uint64) {
 func (f *ForkChoice) SetOriginRoot(root [32]byte) {
 	f.store.originRoot = root
 }
+
+// CachedHeadRoot returns the last cached head root


This method is a fast access to the last computed head. A future possible implementation would be to actually compute head when inserting a node. when processing attestations and on on_tick. Then a call to Head() would just return the cached value.

potuz · 2022-06-20T01:27:20Z

beacon-chain/forkchoice/doubly-linked-tree/forkchoice.go

+	defer f.store.nodesLock.RUnlock()
+	root := f.FinalizedCheckpoint().Root
+	node := f.store.nodeByRoot[root]
+	if node == nil {


This check is to prevent a panic, but at runtime, if the finalized checkpoint is not in forkchoice we are doomed anyway.

beacon-chain/blockchain/chain_info.go

beacon-chain/blockchain/init_sync_process_block.go

terencechain · 2022-06-22T15:19:14Z

beacon-chain/blockchain/process_block_helpers.go

-	}
-	fRoot := s.ensureRootNotZeros(bytesutil.ToBytes32(finalized.Root))
+	finalized := s.ForkChoicer().FinalizedCheckpoint()
+	fRoot := s.ensureRootNotZeros(finalized.Root)


Do we still need ensureRootNotZeros now that finalized root is derived from forkchoice

Unfortunately yes, checkpoints have to be zero root for genesis, so either we check against genesis root at Blockchain to zero it out for services that need that or we return zero from forkchoice. I wonder how much will break if we just return originRoot instead of zero from forkchoice

beacon-chain/blockchain/process_block_helpers.go

beacon-chain/blockchain/receive_block.go

terencechain · 2022-06-22T15:26:34Z

beacon-chain/forkchoice/doubly-linked-tree/forkchoice.go

+	f.store.nodesLock.RLock()
+	defer f.store.nodesLock.RUnlock()
+	root := f.FinalizedCheckpoint().Root
+	node := f.store.nodeByRoot[root]


Check ok too?

Suggested change

node := f.store.nodeByRoot[root]

node, ok := f.store.nodeByRoot[root]

I don't want to change the signature of these functions. Forkchoice is sane in handling that the root is there. Do you suggest returning some bogus root if the root is not there or do you want to return an error?

Dont need to change the signature. Just return [32]byte{}. "This should not happen"

beacon-chain/forkchoice/doubly-linked-tree/forkchoice.go

beacon-chain/forkchoice/doubly-linked-tree/on_tick.go

beacon-chain/forkchoice/protoarray/types.go

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

nisdas · 2022-06-25T00:51:27Z

beacon-chain/blockchain/chain_info.go

-	return ethpb.CopyCheckpoint(cp), nil
+func (s *Service) FinalizedCheckpt() *ethpb.Checkpoint {
+	cp := s.ForkChoicer().FinalizedCheckpoint()
+	return &ethpb.Checkpoint{Epoch: cp.Epoch, Root: bytesutil.SafeCopyBytes(cp.Root[:])}


why not just use the helper here ? ethpb.CopyCheckpoint

because ForkChoicer returns a forkchoicetypes.Checkpoint not an ethpb.Checkpoint, we'll move eventually to the former and then we'll add that helper back.

nisdas · 2022-06-25T00:51:35Z

beacon-chain/blockchain/chain_info.go

+// PreviousJustifiedCheckpt returns the current justified checkpoint from chain store.
+func (s *Service) PreviousJustifiedCheckpt() *ethpb.Checkpoint {
+	cp := s.ForkChoicer().PreviousJustifiedCheckpoint()
+	return &ethpb.Checkpoint{Epoch: cp.Epoch, Root: bytesutil.SafeCopyBytes(cp.Root[:])}


nisdas · 2022-06-25T00:53:08Z

beacon-chain/blockchain/chain_info.go

-		r := s.headRoot()
-		return r[:], nil
+	if s.head != nil && s.head.root != params.BeaconConfig().ZeroHash {
+		return s.head.root[:], nil


we should return a copy here. By taking the slice, you still reference the root in head

Thanks this is right I think.

nisdas · 2022-06-25T00:58:01Z

beacon-chain/blockchain/init_sync_process_block.go

-func (s *Service) saveInitSyncBlock(r [32]byte, b interfaces.SignedBeaconBlock) {
+// This saves a beacon block to the initial sync blocks cache. It rate limits how many blocks
+// the cache keeps in memory (2 epochs worth of blocks) and saves them to DB when it hits this limit.
+func (s *Service) saveInitSyncBlock(ctx context.Context, r [32]byte, b interfaces.SignedBeaconBlock) error {


This seems like a violation of the single responsibility principle, any reason we want to couple them together like this ?

nisdas · 2022-06-25T01:00:41Z

beacon-chain/blockchain/process_block.go

@@ -251,19 +211,19 @@ func (s *Service) onBlock(ctx context.Context, signed interfaces.SignedBeaconBlo
 	}()

 	// Save justified check point to db.
-	if postState.CurrentJustifiedCheckpoint().Epoch > currJustifiedEpoch {
+	if justified.Epoch > currJustifiedEpoch {


this seems like a big change, instead of using the post state's value we will instead use it from our forkchoice service. This doesn't seems to have anything to do with store, so why are we changing it ?

Because we want to guarantee to have DB to agree with forkchoice, the previous behavior was wrong, there are checks that one needs to make before saving a justified checkpoint, forkchoice does them, but the post-state does not. Ultimately, the one that should decide what the right justification is, is forkchoice and the rest should follow it.

Notice that below I'm passing the postState justification, that will also change in the very next PR cause it's making the next spectests fail, see
ethereum/consensus-specs#2923 for context.

nisdas · 2022-06-25T01:05:14Z

beacon-chain/blockchain/process_block.go

+			tracing.AnnotateError(span, err)
+			return err
+		}
+		if i > 0 && jCheckpoints[i].Epoch > jCheckpoints[i-1].Epoch {


in the event we only have 1 block in the whole batch, would this method not be able to updated the justified checkpoint ?

if we only have one block in a batch then the block is only inserted below in InsertNode which does the full justification update. This is only used for intermediate blocks because we don't pass the full beaconState to forkchoice.

nisdas · 2022-06-25T01:05:23Z

beacon-chain/blockchain/process_block.go

+				return err
+			}
+		}
+		if i > 0 && fCheckpoints[i].Epoch > fCheckpoints[i-1].Epoch {


same comment here

potuz added 9 commits June 17, 2022 16:08

Deprecate store WIP

e06df41

Merge remote-tracking branch 'origin/develop' into deprecate_store_4

a1dc423

fix spectests build

19d8a06

mock right interface

34aaec5

sync tests build

950c589

more tests builds

2b9595c

Merge remote-tracking branch 'origin/develop' into deprecate_store_4

79ae441

potuz dismissed a stale review via fcd384a June 19, 2022 23:39

update best justified from genesis

ea4eabe

potuz commented Jun 20, 2022

View reviewed changes

Merge remote-tracking branch 'origin/develop' into deprecate_store_4

1761b12

potuz commented Jun 20, 2022

View reviewed changes

potuz added 5 commits June 19, 2022 22:42

deal with nil head on saveHead

9736f63

initialize prev justified checkpoint

edcfebb

update finalization correctly

e37c835

update finalization logic

d0d8b48

update finalization logic

f6ea696

potuz marked this pull request as ready for review June 20, 2022 15:03

potuz requested a review from a team as a code owner June 20, 2022 15:03

Merge branch 'develop' into deprecate_store_4

18542f7

potuz requested review from rauljordan, saolyn and terencechain June 20, 2022 15:03

potuz added the Ready For Review label Jun 20, 2022

potuz added 4 commits June 21, 2022 14:14

track the wall clock on forkchoice spectests

968c0b5

Merge branch 'wall_clock_spectests' into deprecate_store_4

84a152c

Merge remote-tracking branch 'origin/develop' into deprecate_store_4

289a62b

export copies of checkpoints from blockchain package

6ad9c0c

do not use forkchoice's head on HeadRoot

58be2c9

terencechain changed the title ~~Deprecate store~~ Deprecate store in blockchain pkg Jun 22, 2022

terencechain reviewed Jun 22, 2022

View reviewed changes

potuz and others added 3 commits June 22, 2022 15:22

Remove debug remain

081024d

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

terence's review

27abeb6

Merge branch 'develop' into deprecate_store_4

64cec72

terencechain previously approved these changes Jun 22, 2022

View reviewed changes

add forkchoice types deps

7170d79

potuz dismissed stale reviews from terencechain and ghost via 7170d79 June 22, 2022 23:06

potuz and others added 6 commits June 22, 2022 20:08

wtf

61abba3

debugging

02191e6

init-sync: update justified and finalized checkpoints on db

64edcad

call updateFinalized instead of only DB

fe856f3

remove debug symbols

21b1c7b

Merge branch 'develop' into deprecate_store_4

caee5eb

terencechain previously approved these changes Jun 23, 2022

View reviewed changes

nisdas reviewed Jun 25, 2022

View reviewed changes

safe copy headroot

e111a15

potuz dismissed terencechain’s stale review via e111a15 June 25, 2022 01:36

Merge remote-tracking branch 'origin/develop' into deprecate_store_4

1588a3d

nisdas approved these changes Jun 25, 2022

View reviewed changes

potuz added the OK to merge label Jun 25, 2022

prylabs-bulldozer bot merged commit f8b4d8c into develop Jun 25, 2022

delete-merged-branch bot deleted the deprecate_store_4 branch June 25, 2022 03:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate store in blockchain pkg #10903

Deprecate store in blockchain pkg #10903

potuz commented Jun 17, 2022 •

edited

Loading

potuz Jun 20, 2022

potuz Jun 20, 2022

nisdas Jun 25, 2022

potuz Jun 25, 2022

potuz Jun 20, 2022

potuz Jun 20, 2022

potuz Jun 20, 2022

potuz Jun 20, 2022

potuz Jun 20, 2022

potuz Jun 20, 2022

potuz Jun 20, 2022

potuz Jun 20, 2022

potuz Jun 20, 2022

potuz Jun 20, 2022

potuz Jun 20, 2022

terencechain Jun 22, 2022

potuz Jun 22, 2022

terencechain Jun 22, 2022

potuz Jun 22, 2022

terencechain Jun 22, 2022

nisdas Jun 25, 2022

potuz Jun 25, 2022

nisdas Jun 25, 2022

nisdas Jun 25, 2022

potuz Jun 25, 2022

nisdas Jun 25, 2022

nisdas Jun 25, 2022

potuz Jun 25, 2022 •

edited

Loading

nisdas Jun 25, 2022

potuz Jun 25, 2022

nisdas Jun 25, 2022

	node := f.store.nodeByRoot[root]
	node, ok := f.store.nodeByRoot[root]

Deprecate store in blockchain pkg #10903

Deprecate store in blockchain pkg #10903

Conversation

potuz commented Jun 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

potuz Jun 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

potuz commented Jun 17, 2022 •

edited

Loading

potuz Jun 25, 2022 •

edited

Loading