Forkchoice duplicate store #10840

potuz · 2022-06-07T20:25:22Z

This PR is the second step towards fixing #10734. In this PR we let forkchoice update its justification information when inserting a new node. Moving away from the dependence on the store package for this. This PR also makes forkchoice package track the best justification checkpoint.

potuz · 2022-06-14T19:19:38Z

beacon-chain/blockchain/head.go

@@ -63,58 +59,6 @@ type head struct {
 	state state.BeaconState            // current head state.
 }

-// 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) ([32]byte, error) {


This function is deprecated, it only simply calls to forkchoice.Head, as forkchoice now tracks the justified checkpoints and it is guaranteed to have them already (so the call to fill in missing blocks is irrelevant)

potuz · 2022-06-14T19:20:09Z

beacon-chain/blockchain/head_test.go

@@ -158,26 +162,6 @@ func TestCacheJustifiedStateBalances_CanCache(t *testing.T) {
 	require.DeepEqual(t, balances, state.Balances(), "Incorrect justified balances")
 }

-func TestUpdateHead_MissingJustifiedRoot(t *testing.T) {


This test is irrelevant since forkchoice will not fail to have it's justified checkpoint

potuz · 2022-06-14T19:21:24Z

beacon-chain/blockchain/process_block.go

@@ -221,23 +221,14 @@ func (s *Service) onBlock(ctx context.Context, signed interfaces.SignedBeaconBlo
 			return err
 		}
 		s.store.SetJustifiedCheckptAndPayloadHash(postState.CurrentJustifiedCheckpoint(), h)
-		// Update Forkchoice checkpoints


We do not need to update on onBlock since the call to InsertNode above already updated the checkpoints within forkchoice.

potuz · 2022-06-14T19:23:35Z

beacon-chain/blockchain/service.go

 	if err := s.cfg.ForkChoiceStore.SetOptimisticToValid(ctx, genesisBlkRoot); err != nil {
-		log.Fatalf("Could not set optimistic status of genesis block to false: %v", err)
+		return errors.Wrap(err, "Could not set optimistic status of genesis block to false")


Better to error here since the node will stop anyway

potuz · 2022-06-14T19:25:45Z

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

@@ -59,7 +59,8 @@ func TestFFGUpdates_OneBranch(t *testing.T) {
 	//            2 <- head
 	//            |
 	//            3
-	f.store.justifiedCheckpoint = &forkchoicetypes.Checkpoint{Root: indexToHash(2), Epoch: 1}
+	f.store.justifiedCheckpoint = &forkchoicetypes.Checkpoint{Root: indexToHash(1), Epoch: 1}


These changes on these tests seem scary, but they are mostly due to the impossibility of some of the tests setups: before these changes, we had to carefully check that head will return a different value if a different justified checkpoint was passed. Currently, forkchoice already tracks the right justified checkpoint, so we need to trick it into having different checkpoints to obtain the previous results.

potuz · 2022-06-14T19:26:50Z

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

+}
+
+// updateCheckpoints update the checkpoints when inserting a new node.
+func (f *ForkChoice) updateCheckpoints(ctx context.Context, jc, fc *ethpb.Checkpoint) error {


The current implementation in the blockchain package checks for a block in DB, but that is unnecessary to have as the current justified checkpoint is strictly a forkchoice feature.

potuz · 2022-06-14T19:27:50Z

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

@@ -484,3 +537,13 @@ func (f *ForkChoice) InsertOptimisticChain(ctx context.Context, chain []*forkcho
 	}
 	return nil
 }
+
+// sets the genesisTime tracked by forkchoice


forkchoice needs to track genesisTime to ascertain the current slot number. This is needed for the balancing attack mitigation.

potuz · 2022-06-14T19:28:39Z

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

+}
+
+// sets the genesis Block root
+func (f *ForkChoice) SetOriginRoot(root [32]byte) {


Forkchoice needs to keep track of the OriginRoot because it needs to deal with some checkpoint roots being zero, when they should be the genesis block hash.

potuz · 2022-06-14T19:29:24Z

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

@@ -82,32 +82,6 @@ func TestNoVote_CanFindHead(t *testing.T) {
 	require.NoError(t, f.InsertNode(ctx, state, blkRoot))
 	r, err = f.Head(context.Background(), balances)
 	require.NoError(t, err)
-	assert.Equal(t, indexToHash(4), r, "Incorrect head for with justified epoch at 1")


This is an example of a test that was irrelevant.

beacon-chain/blockchain/init_sync_process_block.go

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

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

terencechain · 2022-06-15T19:56:00Z

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

@@ -29,9 +30,11 @@ type Store struct {
 	nodeByRoot                 map[[fieldparams.RootLength]byte]*Node // nodes indexed by roots.
 	nodeByPayload              map[[fieldparams.RootLength]byte]*Node // nodes indexed by payload Hash
 	slashedIndices             map[types.ValidatorIndex]bool          // the list of equivocating validator indices
+	originRoot                 [fieldparams.RootLength]byte           // The genesis Block root


Is this really just the genesis block root? or it could be check point sync origin root too?

Only genesis root, it's used to deal with zero hashes on checkpoints

Co-authored-by: terencechain <[email protected]>

rauljordan · 2022-06-15T22:34:36Z

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

+		// zeroHash. In this case it should be the root of forkchoice
+		// tree.
+		if s.justifiedCheckpoint.Epoch == params.BeaconConfig().GenesisEpoch {
+			justifiedNode = s.treeRootNode


isnt returning zero hash here sufficient?

This will use the node with root = zerohash as the justified node. Head still needs to be computed

rauljordan · 2022-06-15T22:35:09Z

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

@@ -79,7 +79,7 @@ func TestForkChoice_HasNode(t *testing.T) {
 func TestStore_Head_UnknownJustifiedRoot(t *testing.T) {
 	f := setup(0, 0)

-	f.store.justifiedCheckpoint.Root = [32]byte{'a'}
+	f.store.justifiedCheckpoint = &forkchoicetypes.Checkpoint{Epoch: 1, Root: [32]byte{'a'}}


Why not epoch 0? Is it inconsequential for this test, or does it fail if 0?

Epoch zero is special in that I completely disregard the root and use zerohash if the justified epoch is genesis since the only possible justified checkpoint can be the genesis block root

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

terencechain

Lgtm otherwise!

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

terencechain · 2022-06-16T16:39:14Z

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

+		if slots.SinceEpochStarts(currentSlot) < params.BeaconConfig().SafeSlotsToUpdateJustified {
+			f.store.justifiedCheckpoint = &forkchoicetypes.Checkpoint{Epoch: jc.Epoch,
+				Root: bytesutil.ToBytes32(jc.Root)}
+		} else {
+			currentJcp := f.store.justifiedCheckpoint
+			currentRoot := currentJcp.Root
+			if currentRoot == params.BeaconConfig().ZeroHash {
+				currentRoot = f.store.originRoot
+			}
+			jSlot, err := slots.EpochStart(currentJcp.Epoch)
+			if err != nil {
+				return err
+			}
+			jcRoot := bytesutil.ToBytes32(jc.Root)
+			root, err := f.AncestorRoot(ctx, jcRoot, jSlot)
+			if err != nil {
+				return err
+			}
+			if root == currentRoot {
+				f.store.justifiedCheckpoint = &forkchoicetypes.Checkpoint{Epoch: jc.Epoch,
+					Root: jcRoot}
+			}
+		}
+	}


A little weird to read for translating spec and shouldUpdateCurrentJustified to this if else condition but I don't mind it. It'll work

if compute_slots_since_epoch_start(get_current_slot(store)) < SAFE_SLOTS_TO_UPDATE_JUSTIFIED: return True justified_slot = compute_start_slot_at_epoch(store.justified_checkpoint.epoch) if not get_ancestor(store, new_justified_checkpoint.root, justified_slot) == store.justified_checkpoint.root: return False return True

terencechain · 2022-06-16T16:40:19Z

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

@@ -397,6 +447,10 @@ func (f *ForkChoice) UpdateJustifiedCheckpoint(jc *forkchoicetypes.Checkpoint) e
 	f.store.checkpointsLock.Lock()
 	defer f.store.checkpointsLock.Unlock()
 	f.store.justifiedCheckpoint = jc
+	bj := f.store.bestJustifiedCheckpoint


Do we still need UpdateJustifiedCheckpoint when everything is done?

It's not entirely clear yet. In principle it can be gone, and I am hoping that it completely disappears, but I haven't yet cleaned up init sync, where we don't really have the state cause we do transition in batches

beacon-chain/forkchoice/protoarray/types.go

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

beacon-chain/forkchoice/protoarray/store.go

Co-authored-by: terencechain <[email protected]>

potuz requested a review from a team as a code owner June 7, 2022 20:25

potuz requested review from kasey, terencechain and rkapka June 7, 2022 20:25

potuz force-pushed the forkchoice_duplicate_store branch from 1aa0738 to 98b6fb9 Compare June 8, 2022 13:45

potuz dismissed a stale review via 850a1b9 June 14, 2022 18:45

potuz added 5 commits June 14, 2022 15:47

forkchoice tests

b2652cb

blockchain tests

08c1ee6

no fatal errors

92c18d4

use ZeroHash for genesis

5054c3c

deal with zerohashes at genesis

d9d5133

potuz force-pushed the forkchoice_duplicate_store branch from 850a1b9 to d9d5133 Compare June 14, 2022 18:49

potuz commented Jun 14, 2022

View reviewed changes

potuz added 2 commits June 14, 2022 16:37

avoid nil best justified checkpoint on new store

0012891

unit tests

6c0d684

potuz changed the title ~~[WIP] Forkchoice duplicate store~~ Forkchoice duplicate store Jun 15, 2022

potuz added the Ready For Review A pull request ready for code review label Jun 15, 2022

terencechain requested changes Jun 15, 2022

View reviewed changes

potuz and others added 2 commits June 15, 2022 17:39

Debug log

7429784

Co-authored-by: terencechain <[email protected]>

Terence's review

52a4887

rauljordan reviewed Jun 15, 2022

View reviewed changes

potuz added 4 commits June 15, 2022 20:49

Merge branch 'develop' into forkchoice_duplicate_store

bcf32cb

Merge branch 'develop' into forkchoice_duplicate_store

1ab5138

Merge branch 'develop' into forkchoice_duplicate_store

0d98521

Merge branch 'develop' into forkchoice_duplicate_store

0f8b054

terencechain mentioned this pull request Jun 16, 2022

Apply Proposer Boost from forkchoice #10893

Merged

potuz mentioned this pull request Jun 16, 2022

Prune within forkchoice #10896

Merged

terencechain reviewed Jun 16, 2022

View reviewed changes

update capitalization

052abde

Co-authored-by: terencechain <[email protected]>

potuz dismissed a stale review via 052abde June 16, 2022 16:56

potuz and others added 3 commits June 16, 2022 13:56

update capitalization

7026477

Co-authored-by: terencechain <[email protected]>

update capitalization

9bf686c

Co-authored-by: terencechain <[email protected]>

update capitalization

2862767

Co-authored-by: terencechain <[email protected]>

terencechain approved these changes Jun 16, 2022

View reviewed changes

Merge branch 'develop' into forkchoice_duplicate_store

642f3d6

potuz added the OK to merge label Jun 16, 2022

prylabs-bulldozer bot merged commit e439f4a into develop Jun 16, 2022

delete-merged-branch bot deleted the forkchoice_duplicate_store branch June 16, 2022 18:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Forkchoice duplicate store #10840

Forkchoice duplicate store #10840

potuz commented Jun 7, 2022 •

edited

Loading

potuz Jun 14, 2022

potuz Jun 14, 2022

potuz Jun 14, 2022

potuz Jun 14, 2022

potuz Jun 14, 2022

potuz Jun 14, 2022

potuz Jun 14, 2022

potuz Jun 14, 2022

potuz Jun 14, 2022

terencechain Jun 15, 2022

potuz Jun 15, 2022

rauljordan Jun 15, 2022

potuz Jun 15, 2022

rauljordan Jun 15, 2022

potuz Jun 15, 2022

terencechain left a comment

terencechain Jun 16, 2022

terencechain Jun 16, 2022

potuz Jun 16, 2022 •

edited

Loading

Forkchoice duplicate store #10840

Forkchoice duplicate store #10840

Conversation

potuz commented Jun 7, 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

terencechain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

potuz Jun 16, 2022 • edited Loading

Choose a reason for hiding this comment

potuz commented Jun 7, 2022 •

edited

Loading

potuz Jun 16, 2022 •

edited

Loading