Cleanups to pulltips (#10984)

* Minor cleanups to pulltips

* Feedbacks

Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com>

beacon-chain/core/epoch/precompute/justification_finalization.go

@@ -1,8 +1,6 @@
 package precompute
 
 import (
-	"context"
-
 	"github.com/pkg/errors"
 	"github.com/prysmaticlabs/go-bitfield"
 	"github.com/prysmaticlabs/prysm/beacon-chain/core/helpers"
@@ -17,7 +15,7 @@ var errNilState = errors.New("nil state")
 
 // UnrealizedCheckpoints returns the justification and finalization checkpoints of the
 // given state as if it was progressed with empty slots until the next epoch.
-func UnrealizedCheckpoints(ctx context.Context, st state.BeaconState) (*ethpb.Checkpoint, *ethpb.Checkpoint, error) {
+func UnrealizedCheckpoints(st state.BeaconState) (*ethpb.Checkpoint, *ethpb.Checkpoint, error) {
 	if st == nil || st.IsNil() {
 		return nil, nil, errNilState
 	}

beacon-chain/core/epoch/precompute/justification_finalization_test.go

@@ -243,7 +243,7 @@ func TestUnrealizedCheckpoints(t *testing.T) {
 			_, _, err = altair.InitializePrecomputeValidators(context.Background(), state)
 			require.NoError(t, err)
 
-			jc, fc, err := precompute.UnrealizedCheckpoints(context.Background(), state)
+			jc, fc, err := precompute.UnrealizedCheckpoints(state)
 			require.NoError(t, err)
 			require.DeepEqual(t, test.expectedJustified, jc.Epoch)
 			require.DeepEqual(t, test.expectedFinalized, fc.Epoch)

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

@@ -150,8 +150,8 @@ func (f *ForkChoice) InsertNode(ctx context.Context, state state.BeaconState, ro
 		return err
 	}
 
-	if features.Get().PullTips {
-		jc, fc = f.store.pullTips(ctx, state, node, jc, fc)
+	if features.Get().EnablePullTips {
+		jc, fc = f.store.pullTips(state, node, jc, fc)
 	}
 	return f.updateCheckpoints(ctx, jc, fc)
 }

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

@@ -65,8 +65,8 @@ func (f *ForkChoice) NewSlot(ctx context.Context, slot types.Slot) error {
 			f.store.justifiedCheckpoint = bjcp
 		}
 	}
-	if features.Get().PullTips {
-		f.UpdateUnrealizedCheckpoints()
+	if features.Get().EnablePullTips {
+		f.updateUnrealizedCheckpoints()
 	}
 	return nil
 }

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

@@ -1,8 +1,6 @@
 package doublylinkedtree
 
 import (
-	"context"
-
 	"github.com/pkg/errors"
 	"github.com/prysmaticlabs/prysm/beacon-chain/core/epoch/precompute"
 	forkchoicetypes "github.com/prysmaticlabs/prysm/beacon-chain/forkchoice/types"
@@ -44,20 +42,19 @@ func (s *Store) setUnrealizedFinalizedEpoch(root [32]byte, epoch types.Epoch) er
 	return nil
 }
 
-// UpdateUnrealizedCheckpoints "realizes" the unrealized justified and finalized
-// epochs stored within nodes. It should be called at the beginning of each
-// epoch
-func (f *ForkChoice) UpdateUnrealizedCheckpoints() {
+// updateUnrealizedCheckpoints "realizes" the unrealized justified and finalized
+// epochs stored within nodes. It should be called at the beginning of each epoch.
+func (f *ForkChoice) updateUnrealizedCheckpoints() {
 	f.store.nodesLock.Lock()
 	defer f.store.nodesLock.Unlock()
 	for _, node := range f.store.nodeByRoot {
 		node.justifiedEpoch = node.unrealizedJustifiedEpoch
 		node.finalizedEpoch = node.unrealizedFinalizedEpoch
 		if node.justifiedEpoch > f.store.justifiedCheckpoint.Epoch {
+			f.store.justifiedCheckpoint = f.store.unrealizedJustifiedCheckpoint
 			if node.justifiedEpoch > f.store.bestJustifiedCheckpoint.Epoch {
 				f.store.bestJustifiedCheckpoint = f.store.unrealizedJustifiedCheckpoint
 			}
-			f.store.justifiedCheckpoint = f.store.unrealizedJustifiedCheckpoint
 		}
 		if node.finalizedEpoch > f.store.finalizedCheckpoint.Epoch {
 			f.store.justifiedCheckpoint = f.store.unrealizedJustifiedCheckpoint
@@ -66,33 +63,37 @@ func (f *ForkChoice) UpdateUnrealizedCheckpoints() {
 	}
 }
 
-func (s *Store) pullTips(ctx context.Context, state state.BeaconState, node *Node, jc, fc *ethpb.Checkpoint) (*ethpb.Checkpoint, *ethpb.Checkpoint) {
+func (s *Store) pullTips(state state.BeaconState, node *Node, jc, fc *ethpb.Checkpoint) (*ethpb.Checkpoint, *ethpb.Checkpoint) {
+	s.nodesLock.Lock()
+	defer s.nodesLock.Unlock()
+
+	if node.parent == nil { // Nothing to do if the parent is nil.
+		return jc, fc
+	}
+
 	s.checkpointsLock.Lock()
 	defer s.checkpointsLock.Unlock()
 
-	var uj, uf *ethpb.Checkpoint
-	currentSlot := slots.CurrentSlot(s.genesisTime)
-	currentEpoch := slots.ToEpoch(currentSlot)
+	currentEpoch := slots.ToEpoch(slots.CurrentSlot(s.genesisTime))
 	stateSlot := state.Slot()
 	stateEpoch := slots.ToEpoch(stateSlot)
-	if node.parent == nil {
-		return jc, fc
-	}
 	currJustified := node.parent.unrealizedJustifiedEpoch == currentEpoch
 	prevJustified := node.parent.unrealizedJustifiedEpoch+1 == currentEpoch
 	tooEarlyForCurr := slots.SinceEpochStarts(stateSlot)*3 < params.BeaconConfig().SlotsPerEpoch*2
+	// Exit early if it's justified or too early to be justified.
 	if currJustified || (stateEpoch == currentEpoch && prevJustified && tooEarlyForCurr) {
 		node.unrealizedJustifiedEpoch = node.parent.unrealizedJustifiedEpoch
 		node.unrealizedFinalizedEpoch = node.parent.unrealizedFinalizedEpoch
 		return jc, fc
 	}
 
-	uj, uf, err := precompute.UnrealizedCheckpoints(ctx, state)
+	uj, uf, err := precompute.UnrealizedCheckpoints(state)
 	if err != nil {
 		log.WithError(err).Debug("could not compute unrealized checkpoints")
 		uj, uf = jc, fc
 	}
-	node.unrealizedJustifiedEpoch, node.unrealizedFinalizedEpoch = uj.Epoch, uf.Epoch
+
+	// Update store's unrealized checkpoints.
 	if uj.Epoch > s.unrealizedJustifiedCheckpoint.Epoch {
 		s.unrealizedJustifiedCheckpoint = &forkchoicetypes.Checkpoint{
 			Epoch: uj.Epoch, Root: bytesutil.ToBytes32(uj.Root),
@@ -107,6 +108,8 @@ func (s *Store) pullTips(ctx context.Context, state state.BeaconState, node *Nod
 		}
 	}
 
+	// Update node's checkpoints.
+	node.unrealizedJustifiedEpoch, node.unrealizedFinalizedEpoch = uj.Epoch, uf.Epoch
 	if stateEpoch < currentEpoch {
 		jc, fc = uj, uf
 		node.justifiedEpoch = uj.Epoch

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

@@ -98,7 +98,7 @@ func TestStore_LongFork(t *testing.T) {
 	require.Equal(t, uint64(100), f.store.nodeByRoot[[32]byte{'c'}].weight)
 
 	// Update unrealized justification, c becomes head
-	f.UpdateUnrealizedCheckpoints()
+	f.updateUnrealizedCheckpoints()
 	headRoot, err = f.Head(ctx, []uint64{100})
 	require.NoError(t, err)
 	require.Equal(t, [32]byte{'c'}, headRoot)
@@ -179,7 +179,7 @@ func TestStore_NoDeadLock(t *testing.T) {
 	require.Equal(t, types.Epoch(0), f.FinalizedCheckpoint().Epoch)
 
 	// Realized Justified checkpoints, H becomes head
-	f.UpdateUnrealizedCheckpoints()
+	f.updateUnrealizedCheckpoints()
 	headRoot, err = f.Head(ctx, []uint64{100})
 	require.NoError(t, err)
 	require.Equal(t, [32]byte{'h'}, headRoot)
@@ -240,7 +240,7 @@ func TestStore_ForkNextEpoch(t *testing.T) {
 	require.NoError(t, f.InsertNode(ctx, state, blkRoot))
 	require.NoError(t, f.store.setUnrealizedJustifiedEpoch([32]byte{'d'}, 1))
 	f.store.unrealizedJustifiedCheckpoint = &forkchoicetypes.Checkpoint{Epoch: 1}
-	f.UpdateUnrealizedCheckpoints()
+	f.updateUnrealizedCheckpoints()
 	headRoot, err = f.Head(ctx, []uint64{100})
 	require.NoError(t, err)
 	require.Equal(t, [32]byte{'d'}, headRoot)
@@ -251,7 +251,7 @@ func TestStore_ForkNextEpoch(t *testing.T) {
 
 func TestStore_PullTips_Heuristics(t *testing.T) {
 	resetCfg := features.InitWithReset(&features.Flags{
-		PullTips: true,
+		EnablePullTips: true,
 	})
 	defer resetCfg()
 	ctx := context.Background()

beacon-chain/forkchoice/protoarray/on_tick.go

@@ -65,7 +65,7 @@ func (f *ForkChoice) NewSlot(ctx context.Context, slot types.Slot) error {
 			f.store.justifiedCheckpoint = bjcp
 		}
 	}
-	if features.Get().PullTips {
+	if features.Get().EnablePullTips {
 		f.UpdateUnrealizedCheckpoints()
 	}
 	return nil

beacon-chain/forkchoice/protoarray/store.go

@@ -155,8 +155,8 @@ func (f *ForkChoice) InsertNode(ctx context.Context, state state.BeaconState, ro
 		return err
 	}
 
-	if features.Get().PullTips {
-		jc, fc = f.store.pullTips(ctx, state, node, jc, fc)
+	if features.Get().EnablePullTips {
+		jc, fc = f.store.pullTips(state, node, jc, fc)
 	}
 	return f.updateCheckpoints(ctx, jc, fc)
 }

beacon-chain/forkchoice/protoarray/unrealized_justification.go

@@ -1,8 +1,6 @@
 package protoarray
 
 import (
-	"context"
-
 	"github.com/prysmaticlabs/prysm/beacon-chain/core/epoch/precompute"
 	forkchoicetypes "github.com/prysmaticlabs/prysm/beacon-chain/forkchoice/types"
 	"github.com/prysmaticlabs/prysm/beacon-chain/state"
@@ -77,16 +75,18 @@ func (f *ForkChoice) UpdateUnrealizedCheckpoints() {
 	}
 }
 
-func (s *Store) pullTips(ctx context.Context, state state.BeaconState, node *Node, jc, fc *ethpb.Checkpoint) (*ethpb.Checkpoint, *ethpb.Checkpoint) {
-	var uj, uf *ethpb.Checkpoint
+func (s *Store) pullTips(state state.BeaconState, node *Node, jc, fc *ethpb.Checkpoint) (*ethpb.Checkpoint, *ethpb.Checkpoint) {
+	s.nodesLock.Lock()
+	defer s.nodesLock.Unlock()
 
-	currentSlot := slots.CurrentSlot(s.genesisTime)
-	currentEpoch := slots.ToEpoch(currentSlot)
-	stateSlot := state.Slot()
-	stateEpoch := slots.ToEpoch(stateSlot)
-	if node.parent == NonExistentNode {
+	if node.parent == NonExistentNode { // Nothing to do if the parent is nil.
 		return jc, fc
 	}
+
+	currentEpoch := slots.ToEpoch(slots.CurrentSlot(s.genesisTime))
+	stateSlot := state.Slot()
+	stateEpoch := slots.ToEpoch(stateSlot)
+
 	parent := s.nodes[node.parent]
 	currJustified := parent.unrealizedJustifiedEpoch == currentEpoch
 	prevJustified := parent.unrealizedJustifiedEpoch+1 == currentEpoch
@@ -97,12 +97,13 @@ func (s *Store) pullTips(ctx context.Context, state state.BeaconState, node *Nod
 		return jc, fc
 	}
 
-	uj, uf, err := precompute.UnrealizedCheckpoints(ctx, state)
+	uj, uf, err := precompute.UnrealizedCheckpoints(state)
 	if err != nil {
 		log.WithError(err).Debug("could not compute unrealized checkpoints")
 		uj, uf = jc, fc
 	}
-	node.unrealizedJustifiedEpoch, node.unrealizedFinalizedEpoch = uj.Epoch, uf.Epoch
+
+	// Update store's unrealized checkpoints.
 	s.checkpointsLock.Lock()
 	if uj.Epoch > s.unrealizedJustifiedCheckpoint.Epoch {
 		s.unrealizedJustifiedCheckpoint = &forkchoicetypes.Checkpoint{
@@ -117,12 +118,15 @@ func (s *Store) pullTips(ctx context.Context, state state.BeaconState, node *Nod
 			Epoch: uf.Epoch, Root: bytesutil.ToBytes32(uf.Root),
 		}
 	}
+	s.checkpointsLock.Unlock()
 
+	// Update node's checkpoints.
+	node.unrealizedJustifiedEpoch, node.unrealizedFinalizedEpoch = uj.Epoch, uf.Epoch
 	if stateEpoch < currentEpoch {
 		jc, fc = uj, uf
 		node.justifiedEpoch = uj.Epoch
 		node.finalizedEpoch = uf.Epoch
 	}
-	s.checkpointsLock.Unlock()
+
 	return jc, fc
 }

beacon-chain/forkchoice/protoarray/unrealized_justification_test.go

@@ -252,7 +252,7 @@ func TestStore_ForkNextEpoch(t *testing.T) {
 
 func TestStore_PullTips_Heuristics(t *testing.T) {
 	resetCfg := features.InitWithReset(&features.Flags{
-		PullTips: true,
+		EnablePullTips: true,
 	})
 	defer resetCfg()
 	ctx := context.Background()

config/features/config.go

@@ -61,7 +61,7 @@ type Flags struct {
 	EnableSlashingProtectionPruning bool
 
 	EnableNativeState                bool // EnableNativeState defines whether the beacon state will be represented as a pure Go struct or a Go struct that wraps a proto struct.
-	PullTips                         bool // Experimental disable of boundary checks
+	EnablePullTips                   bool // EnablePullTips enables experimental disabling of boundary checks.
 	EnableVectorizedHTR              bool // EnableVectorizedHTR specifies whether the beacon state will use the optimized sha256 routines.
 	EnableForkChoiceDoublyLinkedTree bool // EnableForkChoiceDoublyLinkedTree specifies whether fork choice store will use a doubly linked tree.
 	EnableBatchGossipAggregation     bool // EnableBatchGossipAggregation specifies whether to further aggregate our gossip batches before verifying them.
@@ -212,9 +212,9 @@ func ConfigureBeaconChain(ctx *cli.Context) error {
 		logDisabled(disableNativeState)
 		cfg.EnableNativeState = false
 	}
-	if ctx.Bool(pullTips.Name) {
-		logEnabled(pullTips)
-		cfg.PullTips = true
+	if ctx.Bool(enablePullTips.Name) {
+		logEnabled(enablePullTips)
+		cfg.EnablePullTips = true
 	}
 	if ctx.Bool(enableVecHTR.Name) {
 		logEnabled(enableVecHTR)

config/features/flags.go

@@ -106,7 +106,7 @@ var (
 		Usage: "Disables representing the beacon state as a pure Go struct.",
 	}
 
-	pullTips = &cli.BoolFlag{
+	enablePullTips = &cli.BoolFlag{
 		Name:  "experimental-disable-boundary-checks",
 		Usage: "Experimental disable of boundary checks, useful for debugging, may cause bad votes.",
 	}
@@ -169,7 +169,7 @@ var BeaconChainFlags = append(deprecatedFlags, []cli.Flag{
 	enableSlasherFlag,
 	enableHistoricalSpaceRepresentation,
 	disableNativeState,
-	pullTips,
+	enablePullTips,
 	enableVecHTR,
 	enableForkChoiceDoublyLinkedTree,
 	enableGossipBatchAggregation,