DoublyLinkedTree double locks (#11269)

* DoublyLinkedTree double locks

* fix updateCheckpoints

* add comment

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

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

@@ -183,11 +183,15 @@ func (f *ForkChoice) updateCheckpoints(ctx context.Context, jc, fc *ethpb.Checkp
 				return err
 			}
 			jcRoot := bytesutil.ToBytes32(jc.Root)
+			// Releasing here the checkpoints lock because
+			// AncestorRoot acquires a lock on nodes and that can
+			// cause a double lock.
+			f.store.checkpointsLock.Unlock()
 			root, err := f.AncestorRoot(ctx, jcRoot, jSlot)
 			if err != nil {
-				f.store.checkpointsLock.Unlock()
 				return err
 			}
+			f.store.checkpointsLock.Lock()
 			if root == currentRoot {
 				f.store.prevJustifiedCheckpoint = f.store.justifiedCheckpoint
 				f.store.justifiedCheckpoint = &forkchoicetypes.Checkpoint{Epoch: jc.Epoch,
@@ -296,7 +300,8 @@ func (f *ForkChoice) AncestorRoot(ctx context.Context, root [32]byte, slot types
 }
 
 // updateBalances updates the balances that directly voted for each block taking into account the
-// validators' latest votes. This function requires a lock in Store.nodesLock.
+// validators' latest votes. This function requires a lock in Store.nodesLock
+// and votesLock
 func (f *ForkChoice) updateBalances(newBalances []uint64) error {
 	for index, vote := range f.votes {
 		// Skip if validator has been slashed
@@ -424,6 +429,9 @@ func (f *ForkChoice) SetOptimisticToInvalid(ctx context.Context, root, parentRoo
 // store-tracked list. Votes from these validators are not accounted for
 // in forkchoice.
 func (f *ForkChoice) InsertSlashedIndex(_ context.Context, index types.ValidatorIndex) {
+	f.votesLock.RLock()
+	defer f.votesLock.RUnlock()
+
 	f.store.nodesLock.Lock()
 	defer f.store.nodesLock.Unlock()
 	// return early if the index was already included:
@@ -433,8 +441,6 @@ func (f *ForkChoice) InsertSlashedIndex(_ context.Context, index types.Validator
 	f.store.slashedIndices[index] = true
 
 	// Subtract last vote from this equivocating validator
-	f.votesLock.RLock()
-	defer f.votesLock.RUnlock()
 
 	if index >= types.ValidatorIndex(len(f.balances)) {
 		return

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

@@ -40,7 +40,8 @@ func (n *Node) applyWeightChanges(ctx context.Context) error {
 	return nil
 }
 
-// updateBestDescendant updates the best descendant of this node and its children.
+// updateBestDescendant updates the best descendant of this node and its
+// children. This function assumes the caller has a lock on Store.nodesLock
 func (n *Node) updateBestDescendant(ctx context.Context, justifiedEpoch, finalizedEpoch types.Epoch) error {
 	if ctx.Err() != nil {
 		return ctx.Err()

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

@@ -41,15 +41,14 @@ func (f *ForkChoice) NewSlot(ctx context.Context, slot types.Slot) error {
 	}
 
 	// Update store.justified_checkpoint if a better checkpoint on the store.finalized_checkpoint chain
-	f.store.checkpointsLock.Lock()
-
+	f.store.checkpointsLock.RLock()
 	bjcp := f.store.bestJustifiedCheckpoint
 	jcp := f.store.justifiedCheckpoint
 	fcp := f.store.finalizedCheckpoint
+	f.store.checkpointsLock.RUnlock()
 	if bjcp.Epoch > jcp.Epoch {
 		finalizedSlot, err := slots.EpochStart(fcp.Epoch)
 		if err != nil {
-			f.store.checkpointsLock.Unlock()
 			return err
 		}
 
@@ -59,15 +58,15 @@ func (f *ForkChoice) NewSlot(ctx context.Context, slot types.Slot) error {
 		// loop call here.
 		r, err := f.AncestorRoot(ctx, bjcp.Root, finalizedSlot)
 		if err != nil {
-			f.store.checkpointsLock.Unlock()
 			return err
 		}
 		if r == fcp.Root {
+			f.store.checkpointsLock.Lock()
 			f.store.prevJustifiedCheckpoint = jcp
 			f.store.justifiedCheckpoint = bjcp
+			f.store.checkpointsLock.Unlock()
 		}
 	}
-	f.store.checkpointsLock.Unlock()
 	if !features.Get().DisablePullTips {
 		f.updateUnrealizedCheckpoints()
 	}

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

@@ -170,8 +170,11 @@ func (s *Store) insert(ctx context.Context,
 		}
 
 		// Update best descendants
-		if err := s.treeRootNode.updateBestDescendant(ctx,
-			s.justifiedCheckpoint.Epoch, s.finalizedCheckpoint.Epoch); err != nil {
+		s.checkpointsLock.RLock()
+		jEpoch := s.justifiedCheckpoint.Epoch
+		fEpoch := s.finalizedCheckpoint.Epoch
+		s.checkpointsLock.RUnlock()
+		if err := s.treeRootNode.updateBestDescendant(ctx, jEpoch, fEpoch); err != nil {
 			return n, err
 		}
 	}