Potuz's first round feedback

Author: terencechain

Diff for: beacon-chain/blockchain/chain_info.go

@@ -41,7 +41,7 @@ type ForkchoiceFetcher interface {
 	Ancestor(context.Context, []byte, primitives.Slot) ([]byte, error)
 	CachedHeadRoot() [32]byte
 	GetProposerHead() [32]byte
-	SafeHeadPayloadBlockHash() [32]byte
+	SafeBlockHash() [32]byte
 	SetForkChoiceGenesisTime(uint64)
 	UpdateHead(context.Context, primitives.Slot)
 	HighestReceivedBlockSlot() primitives.Slot

Diff for: beacon-chain/blockchain/chain_info_forkchoice.go

@@ -90,11 +90,10 @@ func (s *Service) UnrealizedJustifiedPayloadBlockHash() [32]byte {
 	return s.cfg.ForkChoiceStore.UnrealizedJustifiedPayloadBlockHash()
 }
 
-// SafeHeadPayloadBlockHash returns safe head payload block hash from forkchoice.
-func (s *Service) SafeHeadPayloadBlockHash() [32]byte {
+func (s *Service) SafeBlockHash() [32]byte {
 	s.cfg.ForkChoiceStore.RLock()
 	defer s.cfg.ForkChoiceStore.RUnlock()
-	return s.cfg.ForkChoiceStore.SafeHeadPayloadBlockHash()
+	return s.cfg.ForkChoiceStore.SafeBlockHash()
 }
 
 // FinalizedBlockHash returns finalized payload block hash from forkchoice.

Diff for: beacon-chain/blockchain/execution_engine.go

@@ -63,12 +63,7 @@ func (s *Service) notifyForkchoiceUpdate(ctx context.Context, arg *fcuConfig) (*
 		return nil, nil
 	}
 	finalizedHash := s.cfg.ForkChoiceStore.FinalizedPayloadBlockHash()
-	safeBlockHash := [32]byte{}
-	if features.Get().SafeHeadFCU {
-		safeBlockHash = s.cfg.ForkChoiceStore.SafeHeadPayloadBlockHash()
-	} else {
-		safeBlockHash = s.cfg.ForkChoiceStore.UnrealizedJustifiedPayloadBlockHash()
-	}
+	safeBlockHash := s.cfg.ForkChoiceStore.SafeBlockHash()
 	fcs := &enginev1.ForkchoiceState{
 		HeadBlockHash:      headPayload.BlockHash(),
 		SafeBlockHash:      safeBlockHash[:],

Diff for: beacon-chain/blockchain/testing/mock.go

@@ -627,8 +627,8 @@ func (s *ChainService) CachedHeadRoot() [32]byte {
 	return [32]byte{}
 }
 
-// SafeHeadPayloadBlockHash mocks the same method in the chain service
-func (s *ChainService) SafeHeadPayloadBlockHash() [32]byte {
+// SafeBlockHash mocks the same method in the chain service
+func (s *ChainService) SafeBlockHash() [32]byte {
 	return [32]byte{}
 }
 

Diff for: beacon-chain/forkchoice/doubly-linked-tree/BUILD.bazel

@@ -27,6 +27,7 @@ go_library(
         "//beacon-chain/forkchoice:go_default_library",
         "//beacon-chain/forkchoice/types:go_default_library",
         "//beacon-chain/state:go_default_library",
+        "//config/features:go_default_library",
         "//config/fieldparams:go_default_library",
         "//config/params:go_default_library",
         "//consensus-types/blocks:go_default_library",

Diff for: beacon-chain/forkchoice/doubly-linked-tree/forkchoice.go

@@ -8,6 +8,7 @@ import (
 	"github.com/OffchainLabs/prysm/v6/beacon-chain/forkchoice"
 	forkchoicetypes "github.com/OffchainLabs/prysm/v6/beacon-chain/forkchoice/types"
 	"github.com/OffchainLabs/prysm/v6/beacon-chain/state"
+	"github.com/OffchainLabs/prysm/v6/config/features"
 	fieldparams "github.com/OffchainLabs/prysm/v6/config/fieldparams"
 	"github.com/OffchainLabs/prysm/v6/config/params"
 	consensus_blocks "github.com/OffchainLabs/prysm/v6/consensus-types/blocks"
@@ -65,7 +66,7 @@ func (f *ForkChoice) Head(
 		return [32]byte{}, errors.Wrap(err, "could not apply proposer boost score")
 	}
 
-	if err := f.store.treeRootNode.applyWeightChanges(ctx, f.store.proposerBoostRoot, f.store.previousProposerBoostScore); err != nil {
+	if err := f.store.treeRootNode.applyWeightChanges(ctx); err != nil {
 		return [32]byte{}, errors.Wrap(err, "could not apply weight changes")
 	}
 
@@ -76,18 +77,31 @@ func (f *ForkChoice) Head(
 	if err != nil {
 		log.WithError(err).Error("could not compute seconds since slot start")
 	}
-	if err := f.store.treeRootNode.updateBestDescendant(ctx, jc.Epoch, fc.Epoch, currentSlot, secondsSinceSlotStart, f.store.committeeWeight); err != nil {
+	if err := f.store.treeRootNode.updateBestDescendant(ctx, &updateDescendantArgs{
+		justifiedEpoch:        jc.Epoch,
+		finalizedEpoch:        fc.Epoch,
+		currentSlot:           currentSlot,
+		secondsSinceSlotStart: secondsSinceSlotStart,
+		committeeWeight:       f.store.committeeWeight,
+		pbRoot:                f.store.proposerBoostRoot,
+		pbValue:               f.store.previousProposerBoostScore,
+	}); err != nil {
 		return [32]byte{}, errors.Wrap(err, "could not update best descendant")
 	}
-	if err := f.UpdateSafeHead(ctx); err != nil {
+	h, err := f.store.head(ctx)
+	if err != nil {
+		return [32]byte{}, errors.Wrap(err, "could not get head")
+	}
+
+	if err := f.updateSafeHead(ctx); err != nil {
 		log.WithError(err).Error("could not update safe head")
 	}
 
-	return f.store.head(ctx)
+	return h, nil
 }
 
-// UpdateSafeHead updates the safe head in the fork choice store.
-func (f *ForkChoice) UpdateSafeHead(
+// updateSafeHead updates the safe head in the fork choice store.
+func (f *ForkChoice) updateSafeHead(
 	ctx context.Context,
 ) error {
 	oldSafeHeadRoot := f.store.safeHeadRoot
@@ -139,9 +153,8 @@ func (f *ForkChoice) logSafeHead(ctx context.Context, newSafeHeadRoot [32]byte,
 		return
 	}
 
+	// The safe head has reorged. This is bad!
 	if oldSafeHeadRoot != [32]byte{} && commonRoot != oldSafeHeadRoot {
-		// The safe head has reorged.
-		// Calculate reorg metrics.
 		oldSafeHeadNode, ok := f.store.nodeByRoot[oldSafeHeadRoot]
 		if !ok || oldSafeHeadNode == nil {
 			log.WithError(ErrNilNode).Error("could not find old safe head node")
@@ -156,8 +169,8 @@ func (f *ForkChoice) logSafeHead(ctx context.Context, newSafeHeadRoot [32]byte,
 			"commonAncestorRoot": fmt.Sprintf("%#x", commonRoot),
 			"distance":           dis,
 			"depth":              dep,
-		}).Debug("Safe head reorg occurred")
-		// Update reorg metrics.
+		}).Error("Safe head reorg occurred")
+
 		safeHeadReorgDistance.Observe(float64(dis))
 		safeHeadReorgDepth.Observe(float64(dep))
 		safeHeadReorgCount.Inc()
@@ -624,8 +637,12 @@ func (f *ForkChoice) UnrealizedJustifiedPayloadBlockHash() [32]byte {
 	return node.payloadHash
 }
 
-// SafeHeadPayloadBlockHash returns the hash of the payload at the safe head
-func (f *ForkChoice) SafeHeadPayloadBlockHash() [32]byte {
+// SafeBlockHash returns the hash of the payload at the safe head
+func (f *ForkChoice) SafeBlockHash() [32]byte {
+	if !features.Get().EnableFastConfirmation {
+		return f.UnrealizedJustifiedPayloadBlockHash()
+	}
+
 	safeHeadRoot := f.store.safeHeadRoot
 	node, ok := f.store.nodeByRoot[safeHeadRoot]
 	if !ok || node == nil {

Diff for: beacon-chain/forkchoice/doubly-linked-tree/forkchoice_test.go

@@ -227,11 +227,17 @@ func TestForkChoice_IsCanonicalReorg(t *testing.T) {
 	require.NoError(t, f.InsertNode(ctx, st, roblock))
 
 	f.store.nodeByRoot[[32]byte{'3'}].balance = 10
-	require.NoError(t, f.store.treeRootNode.applyWeightChanges(ctx, params.BeaconConfig().ZeroHash, 0))
+	require.NoError(t, f.store.treeRootNode.applyWeightChanges(ctx))
 	require.Equal(t, uint64(10), f.store.nodeByRoot[[32]byte{'1'}].weight)
 	require.Equal(t, uint64(0), f.store.nodeByRoot[[32]byte{'2'}].weight)
 
-	require.NoError(t, f.store.treeRootNode.updateBestDescendant(ctx, 1, 1, 6, 0, f.store.committeeWeight))
+	require.NoError(t, f.store.treeRootNode.updateBestDescendant(ctx, &updateDescendantArgs{
+		justifiedEpoch:        1,
+		finalizedEpoch:        1,
+		currentSlot:           6,
+		secondsSinceSlotStart: 0,
+		committeeWeight:       f.store.committeeWeight,
+	}))
 	require.DeepEqual(t, [32]byte{'3'}, f.store.treeRootNode.bestDescendant.root)
 
 	r1 := [32]byte{'1'}
@@ -943,13 +949,14 @@ func TestForkChoiceSafeHead(t *testing.T) {
 	require.Equal(t, primitives.Slot(32), slotsPerEpoch)
 
 	driftGenesisTime(f, primitives.Slot(11), 0)
-	st, blkRoot, err := prepareForkchoiceState(ctx, 1, indexToHash(1), params.BeaconConfig().ZeroHash, params.BeaconConfig().ZeroHash, 0, 0)
+	st, b, err := prepareForkchoiceState(ctx, 1, indexToHash(1), params.BeaconConfig().ZeroHash, params.BeaconConfig().ZeroHash, 0, 0)
 	require.NoError(t, err)
-	require.NoError(t, f.InsertNode(ctx, st, blkRoot))
+	require.NoError(t, f.InsertNode(ctx, st, b))
 	for i := 2; i < 10; i++ {
-		st, blkRoot, err = prepareForkchoiceState(ctx, primitives.Slot(i), indexToHash(uint64(i)), indexToHash(uint64(i-1)), params.BeaconConfig().ZeroHash, 0, 0)
+		st, b, err = prepareForkchoiceState(ctx, primitives.Slot(i), indexToHash(uint64(i)), indexToHash(uint64(i-1)), params.BeaconConfig().ZeroHash, 0, 0)
 		require.NoError(t, err)
-		require.NoError(t, f.InsertNode(ctx, st, blkRoot))
+		t.Logf("Inserting node %d", b.Block().Slot())
+		require.NoError(t, f.InsertNode(ctx, st, b))
 	}
 
 	tests := []struct {

Diff for: beacon-chain/forkchoice/doubly-linked-tree/node.go

@@ -16,35 +16,36 @@ import (
 // process attestations for the current slot
 const ProcessAttestationsThreshold = 10
 
+type updateDescendantArgs struct {
+	justifiedEpoch        primitives.Epoch
+	finalizedEpoch        primitives.Epoch
+	currentSlot           primitives.Slot
+	secondsSinceSlotStart uint64
+	committeeWeight       uint64
+	pbRoot                [32]byte
+	pbValue               uint64
+}
+
 // applyWeightChanges recursively traverses a tree of nodes to update each node's total weight and
 // weight without proposer boost by summing the balance of the node and its children.
 // If the node matches a specific root (`pbRoot`), it subtracts a given boost value (`pbValue`) from the weight without boost,
 // ensuring the balance is sufficient. It also handles context cancellation and errors during recursion.
-func (n *Node) applyWeightChanges(ctx context.Context, pbRoot [32]byte, pbValue uint64) error {
+func (n *Node) applyWeightChanges(ctx context.Context) error {
 	// Recursively calling the children to sum their weights.
 	childrenWeight := uint64(0)
-	childrenWeightWithoutBoost := uint64(0)
 	for _, child := range n.children {
 		if ctx.Err() != nil {
 			return ctx.Err()
 		}
-		if err := child.applyWeightChanges(ctx, pbRoot, pbValue); err != nil {
+		if err := child.applyWeightChanges(ctx); err != nil {
 			return err
 		}
 		childrenWeight += child.weight
-		childrenWeightWithoutBoost += child.weightWithoutBoost
 	}
 	if n.root == params.BeaconConfig().ZeroHash {
 		return nil
 	}
 	n.weight = n.balance + childrenWeight
-	n.weightWithoutBoost = n.balance + childrenWeightWithoutBoost
-	if n.root == pbRoot {
-		if n.balance < pbValue {
-			return fmt.Errorf("node balance %d is less than proposer boost value %d", n.balance, pbValue)
-		}
-		n.weightWithoutBoost -= pbValue
-	}
 	return nil
 }
 
@@ -86,9 +87,7 @@ func (n *Node) maxWeight(endSlot primitives.Slot, committeeWeight uint64) uint64
 
 // updateBestDescendant updates the best descendant of this node and its
 // children.
-func (n *Node) updateBestDescendant(ctx context.Context,
-	justifiedEpoch primitives.Epoch, finalizedEpoch primitives.Epoch,
-	currentSlot primitives.Slot, secondsSinceSlotStart uint64, committeeWeight uint64) error {
+func (n *Node) updateBestDescendant(ctx context.Context, args *updateDescendantArgs) error {
 	if ctx.Err() != nil {
 		return ctx.Err()
 	}
@@ -105,13 +104,11 @@ func (n *Node) updateBestDescendant(ctx context.Context,
 		if child == nil {
 			return errors.Wrap(ErrNilNode, "could not update best descendant")
 		}
-		if err := child.updateBestDescendant(ctx,
-			justifiedEpoch, finalizedEpoch,
-			currentSlot, secondsSinceSlotStart, committeeWeight); err != nil {
+		if err := child.updateBestDescendant(ctx, args); err != nil {
 			return err
 		}
-		currentEpoch := slots.ToEpoch(currentSlot)
-		childLeadsToViableHead := child.leadsToViableHead(justifiedEpoch, currentEpoch)
+		currentEpoch := slots.ToEpoch(args.currentSlot)
+		childLeadsToViableHead := child.leadsToViableHead(args.justifiedEpoch, currentEpoch)
 		if childLeadsToViableHead && !hasViableDescendant {
 			// The child leads to a viable head, but the current
 			// parent's best child doesn't.
@@ -140,14 +137,14 @@ func (n *Node) updateBestDescendant(ctx context.Context,
 			// The best descendant is more than 1 hop away.
 			n.bestDescendant = bestChild.bestDescendant
 		}
-		// Compute safe head only during the first interval of the slot
-		if secondsSinceSlotStart < params.BeaconConfig().SecondsPerSlot/params.BeaconConfig().IntervalsPerSlot {
+
+		if args.secondsSinceSlotStart < params.BeaconConfig().SecondsPerSlot/params.BeaconConfig().IntervalsPerSlot {
 			prevSlot := primitives.Slot(0)
-			if currentSlot > 1 {
-				prevSlot = currentSlot - 1
+			if args.currentSlot > 1 {
+				prevSlot = args.currentSlot - 1
 			}
 
-			if bestChild.confirmed(prevSlot, committeeWeight) {
+			if bestChild.confirmed(prevSlot, args.committeeWeight, args.pbRoot, args.pbValue) {
 				n.bestConfirmedDescendant = bestChild.bestConfirmedDescendant
 				if n.bestConfirmedDescendant == nil {
 					n.bestConfirmedDescendant = bestChild
@@ -267,7 +264,7 @@ func (n *Node) nodeTreeDump(ctx context.Context, nodes []*forkchoice2.Node) ([]*
 }
 
 // confirmed returns true if the node satisfies the confirmation rule.
-func (n *Node) confirmed(slot primitives.Slot, committeeWeight uint64) bool {
+func (n *Node) confirmed(slot primitives.Slot, committeeWeight uint64, pbRoot [32]byte, pbValue uint64) bool {
 	if n.slot > slot {
 		return false
 	}
@@ -276,5 +273,17 @@ func (n *Node) confirmed(slot primitives.Slot, committeeWeight uint64) bool {
 	maxWeight := n.maxWeight(slot, committeeWeight)
 	threshold := (maxWeight + pbWeight) / 2
 
-	return n.weightWithoutBoost > threshold
+	nodeWeight := n.weight
+	if n.root == pbRoot {
+		nodeWeight -= pbValue
+	}
+	if n.bestDescendant != nil && n.bestDescendant.root == pbRoot {
+		if nodeWeight < pbValue {
+			return false
+		}
+		nodeWeight -= pbValue
+	}
+
+	fmt.Println(nodeWeight, threshold)
+	return nodeWeight > threshold
 }