Skip to content

Commit 817cabb

Browse files
terencechainterencechain
committed
Potuz's first round feedback
1 parent 8e3e182 commit 817cabb

File tree

17 files changed

+199
-200
lines changed

17 files changed

+199
-200
lines changed

‎beacon-chain/blockchain/chain_info.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ type ForkchoiceFetcher interface {
4141
Ancestor(context.Context, []byte, primitives.Slot) ([]byte, error)
4242
CachedHeadRoot() [32]byte
4343
GetProposerHead() [32]byte
44-
SafeHeadPayloadBlockHash() [32]byte
44+
SafeBlockHash() [32]byte
4545
SetForkChoiceGenesisTime(uint64)
4646
UpdateHead(context.Context, primitives.Slot)
4747
HighestReceivedBlockSlot() primitives.Slot

‎beacon-chain/blockchain/chain_info_forkchoice.go‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -90,11 +90,10 @@ func (s *Service) UnrealizedJustifiedPayloadBlockHash() [32]byte {
9090
return s.cfg.ForkChoiceStore.UnrealizedJustifiedPayloadBlockHash()
9191
}
9292

93-
// SafeHeadPayloadBlockHash returns safe head payload block hash from forkchoice.
94-
func (s *Service) SafeHeadPayloadBlockHash() [32]byte {
93+
func (s *Service) SafeBlockHash() [32]byte {
9594
s.cfg.ForkChoiceStore.RLock()
9695
defer s.cfg.ForkChoiceStore.RUnlock()
97-
return s.cfg.ForkChoiceStore.SafeHeadPayloadBlockHash()
96+
return s.cfg.ForkChoiceStore.SafeBlockHash()
9897
}
9998

10099
// FinalizedBlockHash returns finalized payload block hash from forkchoice.

‎beacon-chain/blockchain/execution_engine.go‎

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -63,12 +63,7 @@ func (s *Service) notifyForkchoiceUpdate(ctx context.Context, arg *fcuConfig) (*
6363
return nil, nil
6464
}
6565
finalizedHash := s.cfg.ForkChoiceStore.FinalizedPayloadBlockHash()
66-
safeBlockHash := [32]byte{}
67-
if features.Get().SafeHeadFCU {
68-
safeBlockHash = s.cfg.ForkChoiceStore.SafeHeadPayloadBlockHash()
69-
} else {
70-
safeBlockHash = s.cfg.ForkChoiceStore.UnrealizedJustifiedPayloadBlockHash()
71-
}
66+
safeBlockHash := s.cfg.ForkChoiceStore.SafeBlockHash()
7267
fcs := &enginev1.ForkchoiceState{
7368
HeadBlockHash: headPayload.BlockHash(),
7469
SafeBlockHash: safeBlockHash[:],

‎beacon-chain/blockchain/testing/mock.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -627,8 +627,8 @@ func (s *ChainService) CachedHeadRoot() [32]byte {
627627
return [32]byte{}
628628
}
629629

630-
// SafeHeadPayloadBlockHash mocks the same method in the chain service
631-
func (s *ChainService) SafeHeadPayloadBlockHash() [32]byte {
630+
// SafeBlockHash mocks the same method in the chain service
631+
func (s *ChainService) SafeBlockHash() [32]byte {
632632
return [32]byte{}
633633
}
634634

‎beacon-chain/forkchoice/doubly-linked-tree/BUILD.bazel‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ go_library(
2727
"//beacon-chain/forkchoice:go_default_library",
2828
"//beacon-chain/forkchoice/types:go_default_library",
2929
"//beacon-chain/state:go_default_library",
30+
"//config/features:go_default_library",
3031
"//config/fieldparams:go_default_library",
3132
"//config/params:go_default_library",
3233
"//consensus-types/blocks:go_default_library",

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

Lines changed: 29 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"github.com/OffchainLabs/prysm/v6/beacon-chain/forkchoice"
99
forkchoicetypes "github.com/OffchainLabs/prysm/v6/beacon-chain/forkchoice/types"
1010
"github.com/OffchainLabs/prysm/v6/beacon-chain/state"
11+
"github.com/OffchainLabs/prysm/v6/config/features"
1112
fieldparams "github.com/OffchainLabs/prysm/v6/config/fieldparams"
1213
"github.com/OffchainLabs/prysm/v6/config/params"
1314
consensus_blocks "github.com/OffchainLabs/prysm/v6/consensus-types/blocks"
@@ -65,7 +66,7 @@ func (f *ForkChoice) Head(
6566
return [32]byte{}, errors.Wrap(err, "could not apply proposer boost score")
6667
}
6768

68-
if err := f.store.treeRootNode.applyWeightChanges(ctx, f.store.proposerBoostRoot, f.store.previousProposerBoostScore); err != nil {
69+
if err := f.store.treeRootNode.applyWeightChanges(ctx); err != nil {
6970
return [32]byte{}, errors.Wrap(err, "could not apply weight changes")
7071
}
7172

@@ -76,18 +77,31 @@ func (f *ForkChoice) Head(
7677
if err != nil {
7778
log.WithError(err).Error("could not compute seconds since slot start")
7879
}
79-
if err := f.store.treeRootNode.updateBestDescendant(ctx, jc.Epoch, fc.Epoch, currentSlot, secondsSinceSlotStart, f.store.committeeWeight); err != nil {
80+
if err := f.store.treeRootNode.updateBestDescendant(ctx, &updateDescendantArgs{
81+
justifiedEpoch: jc.Epoch,
82+
finalizedEpoch: fc.Epoch,
83+
currentSlot: currentSlot,
84+
secondsSinceSlotStart: secondsSinceSlotStart,
85+
committeeWeight: f.store.committeeWeight,
86+
pbRoot: f.store.proposerBoostRoot,
87+
pbValue: f.store.previousProposerBoostScore,
88+
}); err != nil {
8089
return [32]byte{}, errors.Wrap(err, "could not update best descendant")
8190
}
82-
if err := f.UpdateSafeHead(ctx); err != nil {
91+
h, err := f.store.head(ctx)
92+
if err != nil {
93+
return [32]byte{}, errors.Wrap(err, "could not get head")
94+
}
95+
96+
if err := f.updateSafeHead(ctx); err != nil {
8397
log.WithError(err).Error("could not update safe head")
8498
}
8599

86-
return f.store.head(ctx)
100+
return h, nil
87101
}
88102

89-
// UpdateSafeHead updates the safe head in the fork choice store.
90-
func (f *ForkChoice) UpdateSafeHead(
103+
// updateSafeHead updates the safe head in the fork choice store.
104+
func (f *ForkChoice) updateSafeHead(
91105
ctx context.Context,
92106
) error {
93107
oldSafeHeadRoot := f.store.safeHeadRoot
@@ -139,9 +153,8 @@ func (f *ForkChoice) logSafeHead(ctx context.Context, newSafeHeadRoot [32]byte,
139153
return
140154
}
141155

156+
// The safe head has reorged. This is bad!
142157
if oldSafeHeadRoot != [32]byte{} && commonRoot != oldSafeHeadRoot {
143-
// The safe head has reorged.
144-
// Calculate reorg metrics.
145158
oldSafeHeadNode, ok := f.store.nodeByRoot[oldSafeHeadRoot]
146159
if !ok || oldSafeHeadNode == nil {
147160
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,
156169
"commonAncestorRoot": fmt.Sprintf("%#x", commonRoot),
157170
"distance": dis,
158171
"depth": dep,
159-
}).Debug("Safe head reorg occurred")
160-
// Update reorg metrics.
172+
}).Error("Safe head reorg occurred")
173+
161174
safeHeadReorgDistance.Observe(float64(dis))
162175
safeHeadReorgDepth.Observe(float64(dep))
163176
safeHeadReorgCount.Inc()
@@ -624,8 +637,12 @@ func (f *ForkChoice) UnrealizedJustifiedPayloadBlockHash() [32]byte {
624637
return node.payloadHash
625638
}
626639

627-
// SafeHeadPayloadBlockHash returns the hash of the payload at the safe head
628-
func (f *ForkChoice) SafeHeadPayloadBlockHash() [32]byte {
640+
// SafeBlockHash returns the hash of the payload at the safe head
641+
func (f *ForkChoice) SafeBlockHash() [32]byte {
642+
if !features.Get().EnableFastConfirmation {
643+
return f.UnrealizedJustifiedPayloadBlockHash()
644+
}
645+
629646
safeHeadRoot := f.store.safeHeadRoot
630647
node, ok := f.store.nodeByRoot[safeHeadRoot]
631648
if !ok || node == nil {

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

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -227,11 +227,17 @@ func TestForkChoice_IsCanonicalReorg(t *testing.T) {
227227
require.NoError(t, f.InsertNode(ctx, st, roblock))
228228

229229
f.store.nodeByRoot[[32]byte{'3'}].balance = 10
230-
require.NoError(t, f.store.treeRootNode.applyWeightChanges(ctx, params.BeaconConfig().ZeroHash, 0))
230+
require.NoError(t, f.store.treeRootNode.applyWeightChanges(ctx))
231231
require.Equal(t, uint64(10), f.store.nodeByRoot[[32]byte{'1'}].weight)
232232
require.Equal(t, uint64(0), f.store.nodeByRoot[[32]byte{'2'}].weight)
233233

234-
require.NoError(t, f.store.treeRootNode.updateBestDescendant(ctx, 1, 1, 6, 0, f.store.committeeWeight))
234+
require.NoError(t, f.store.treeRootNode.updateBestDescendant(ctx, &updateDescendantArgs{
235+
justifiedEpoch: 1,
236+
finalizedEpoch: 1,
237+
currentSlot: 6,
238+
secondsSinceSlotStart: 0,
239+
committeeWeight: f.store.committeeWeight,
240+
}))
235241
require.DeepEqual(t, [32]byte{'3'}, f.store.treeRootNode.bestDescendant.root)
236242

237243
r1 := [32]byte{'1'}
@@ -943,13 +949,14 @@ func TestForkChoiceSafeHead(t *testing.T) {
943949
require.Equal(t, primitives.Slot(32), slotsPerEpoch)
944950

945951
driftGenesisTime(f, primitives.Slot(11), 0)
946-
st, blkRoot, err := prepareForkchoiceState(ctx, 1, indexToHash(1), params.BeaconConfig().ZeroHash, params.BeaconConfig().ZeroHash, 0, 0)
952+
st, b, err := prepareForkchoiceState(ctx, 1, indexToHash(1), params.BeaconConfig().ZeroHash, params.BeaconConfig().ZeroHash, 0, 0)
947953
require.NoError(t, err)
948-
require.NoError(t, f.InsertNode(ctx, st, blkRoot))
954+
require.NoError(t, f.InsertNode(ctx, st, b))
949955
for i := 2; i < 10; i++ {
950-
st, blkRoot, err = prepareForkchoiceState(ctx, primitives.Slot(i), indexToHash(uint64(i)), indexToHash(uint64(i-1)), params.BeaconConfig().ZeroHash, 0, 0)
956+
st, b, err = prepareForkchoiceState(ctx, primitives.Slot(i), indexToHash(uint64(i)), indexToHash(uint64(i-1)), params.BeaconConfig().ZeroHash, 0, 0)
951957
require.NoError(t, err)
952-
require.NoError(t, f.InsertNode(ctx, st, blkRoot))
958+
t.Logf("Inserting node %d", b.Block().Slot())
959+
require.NoError(t, f.InsertNode(ctx, st, b))
953960
}
954961

955962
tests := []struct {

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

Lines changed: 34 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@ package doublylinkedtree
33
import (
44
"bytes"
55
"context"
6-
"fmt"
76

87
"github.com/OffchainLabs/prysm/v6/config/params"
98
forkchoice2 "github.com/OffchainLabs/prysm/v6/consensus-types/forkchoice"
@@ -16,35 +15,36 @@ import (
1615
// process attestations for the current slot
1716
const ProcessAttestationsThreshold = 10
1817

18+
type updateDescendantArgs struct {
19+
justifiedEpoch primitives.Epoch
20+
finalizedEpoch primitives.Epoch
21+
currentSlot primitives.Slot
22+
secondsSinceSlotStart uint64
23+
committeeWeight uint64
24+
pbRoot [32]byte
25+
pbValue uint64
26+
}
27+
1928
// applyWeightChanges recursively traverses a tree of nodes to update each node's total weight and
2029
// weight without proposer boost by summing the balance of the node and its children.
2130
// If the node matches a specific root (`pbRoot`), it subtracts a given boost value (`pbValue`) from the weight without boost,
2231
// ensuring the balance is sufficient. It also handles context cancellation and errors during recursion.
23-
func (n *Node) applyWeightChanges(ctx context.Context, pbRoot [32]byte, pbValue uint64) error {
32+
func (n *Node) applyWeightChanges(ctx context.Context) error {
2433
// Recursively calling the children to sum their weights.
2534
childrenWeight := uint64(0)
26-
childrenWeightWithoutBoost := uint64(0)
2735
for _, child := range n.children {
2836
if ctx.Err() != nil {
2937
return ctx.Err()
3038
}
31-
if err := child.applyWeightChanges(ctx, pbRoot, pbValue); err != nil {
39+
if err := child.applyWeightChanges(ctx); err != nil {
3240
return err
3341
}
3442
childrenWeight += child.weight
35-
childrenWeightWithoutBoost += child.weightWithoutBoost
3643
}
3744
if n.root == params.BeaconConfig().ZeroHash {
3845
return nil
3946
}
4047
n.weight = n.balance + childrenWeight
41-
n.weightWithoutBoost = n.balance + childrenWeightWithoutBoost
42-
if n.root == pbRoot {
43-
if n.balance < pbValue {
44-
return fmt.Errorf("node balance %d is less than proposer boost value %d", n.balance, pbValue)
45-
}
46-
n.weightWithoutBoost -= pbValue
47-
}
4848
return nil
4949
}
5050

@@ -86,9 +86,7 @@ func (n *Node) maxWeight(endSlot primitives.Slot, committeeWeight uint64) uint64
8686

8787
// updateBestDescendant updates the best descendant of this node and its
8888
// children.
89-
func (n *Node) updateBestDescendant(ctx context.Context,
90-
justifiedEpoch primitives.Epoch, finalizedEpoch primitives.Epoch,
91-
currentSlot primitives.Slot, secondsSinceSlotStart uint64, committeeWeight uint64) error {
89+
func (n *Node) updateBestDescendant(ctx context.Context, args *updateDescendantArgs) error {
9290
if ctx.Err() != nil {
9391
return ctx.Err()
9492
}
@@ -105,13 +103,11 @@ func (n *Node) updateBestDescendant(ctx context.Context,
105103
if child == nil {
106104
return errors.Wrap(ErrNilNode, "could not update best descendant")
107105
}
108-
if err := child.updateBestDescendant(ctx,
109-
justifiedEpoch, finalizedEpoch,
110-
currentSlot, secondsSinceSlotStart, committeeWeight); err != nil {
106+
if err := child.updateBestDescendant(ctx, args); err != nil {
111107
return err
112108
}
113-
currentEpoch := slots.ToEpoch(currentSlot)
114-
childLeadsToViableHead := child.leadsToViableHead(justifiedEpoch, currentEpoch)
109+
currentEpoch := slots.ToEpoch(args.currentSlot)
110+
childLeadsToViableHead := child.leadsToViableHead(args.justifiedEpoch, currentEpoch)
115111
if childLeadsToViableHead && !hasViableDescendant {
116112
// The child leads to a viable head, but the current
117113
// parent's best child doesn't.
@@ -140,14 +136,14 @@ func (n *Node) updateBestDescendant(ctx context.Context,
140136
// The best descendant is more than 1 hop away.
141137
n.bestDescendant = bestChild.bestDescendant
142138
}
143-
// Compute safe head only during the first interval of the slot
144-
if secondsSinceSlotStart < params.BeaconConfig().SecondsPerSlot/params.BeaconConfig().IntervalsPerSlot {
139+
140+
if args.secondsSinceSlotStart < params.BeaconConfig().SecondsPerSlot/params.BeaconConfig().IntervalsPerSlot {
145141
prevSlot := primitives.Slot(0)
146-
if currentSlot > 1 {
147-
prevSlot = currentSlot - 1
142+
if args.currentSlot > 1 {
143+
prevSlot = args.currentSlot - 1
148144
}
149145

150-
if bestChild.confirmed(prevSlot, committeeWeight) {
146+
if bestChild.confirmed(prevSlot, args.committeeWeight, args.pbRoot, args.pbValue) {
151147
n.bestConfirmedDescendant = bestChild.bestConfirmedDescendant
152148
if n.bestConfirmedDescendant == nil {
153149
n.bestConfirmedDescendant = bestChild
@@ -267,7 +263,7 @@ func (n *Node) nodeTreeDump(ctx context.Context, nodes []*forkchoice2.Node) ([]*
267263
}
268264

269265
// confirmed returns true if the node satisfies the confirmation rule.
270-
func (n *Node) confirmed(slot primitives.Slot, committeeWeight uint64) bool {
266+
func (n *Node) confirmed(slot primitives.Slot, committeeWeight uint64, pbRoot [32]byte, pbValue uint64) bool {
271267
if n.slot > slot {
272268
return false
273269
}
@@ -276,5 +272,16 @@ func (n *Node) confirmed(slot primitives.Slot, committeeWeight uint64) bool {
276272
maxWeight := n.maxWeight(slot, committeeWeight)
277273
threshold := (maxWeight + pbWeight) / 2
278274

279-
return n.weightWithoutBoost > threshold
275+
nodeWeight := n.weight
276+
if n.root == pbRoot {
277+
nodeWeight -= pbValue
278+
}
279+
if n.bestDescendant != nil && n.bestDescendant.root == pbRoot {
280+
if nodeWeight < pbValue {
281+
return false
282+
}
283+
nodeWeight -= pbValue
284+
}
285+
286+
return nodeWeight > threshold
280287
}

0 commit comments

Comments
 (0)