Remove synced tips and use last valid hash (prysmaticlabs#10439)

* Remove synced tips

use last valid hash in removing invalid nodes.

* add test

* Remove unused code

* More unused parameters

* Fix proposer boost

* terence's review #1

* Fix conflicts

* terence's review 2

* rename argument

* terence's review #3

* rename optimistic -> status

* Minor clean up

* revert loop variable change

* do not mark lvh as valid

Co-authored-by: terence tsao <terence@prysmaticlabs.com>

beacon-chain/blockchain/chain_info.go

@@ -331,7 +331,7 @@ func (s *Service) IsOptimistic(ctx context.Context) (bool, error) {
 // IsOptimisticForRoot takes the root and slot as arguments instead of the current head
 // and returns true if it is optimistic.
 func (s *Service) IsOptimisticForRoot(ctx context.Context, root [32]byte) (bool, error) {
-	optimistic, err := s.cfg.ForkChoiceStore.IsOptimistic(ctx, root)
+	optimistic, err := s.cfg.ForkChoiceStore.IsOptimistic(root)
 	if err == nil {
 		return optimistic, nil
 	}

beacon-chain/blockchain/process_block.go

@@ -262,7 +262,7 @@ func (s *Service) onBlock(ctx context.Context, signed block.SignedBeaconBlock, b
 		if err := s.cfg.ForkChoiceStore.Prune(ctx, fRoot); err != nil {
 			return errors.Wrap(err, "could not prune proto array fork choice nodes")
 		}
-		isOptimistic, err := s.cfg.ForkChoiceStore.IsOptimistic(ctx, fRoot)
+		isOptimistic, err := s.cfg.ForkChoiceStore.IsOptimistic(fRoot)
 		if err != nil {
 			return errors.Wrap(err, "could not check if node is optimistically synced")
 		}

beacon-chain/blockchain/process_block_helpers.go

@@ -247,7 +247,7 @@ func (s *Service) updateFinalized(ctx context.Context, cp *ethpb.Checkpoint) err
 	}
 
 	fRoot := bytesutil.ToBytes32(cp.Root)
-	optimistic, err := s.cfg.ForkChoiceStore.IsOptimistic(ctx, fRoot)
+	optimistic, err := s.cfg.ForkChoiceStore.IsOptimistic(fRoot)
 	if err != nil {
 		return err
 	}

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

@@ -8,3 +8,4 @@ var errInvalidProposerBoostRoot = errors.New("invalid proposer boost root")
 var errUnknownFinalizedRoot = errors.New("unknown finalized root")
 var errUnknownJustifiedRoot = errors.New("unknown justified root")
 var errInvalidOptimisticStatus = errors.New("invalid optimistic status")
+var errUnknownPayloadHash = errors.New("unknown payload hash")

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

@@ -18,6 +18,7 @@ func New(justifiedEpoch, finalizedEpoch types.Epoch) *ForkChoice {
 		finalizedEpoch:    finalizedEpoch,
 		proposerBoostRoot: [32]byte{},
 		nodeByRoot:        make(map[[fieldparams.RootLength]byte]*Node),
+		nodeByPayload:     make(map[[fieldparams.RootLength]byte]*Node),
 		pruneThreshold:    defaultPruneThreshold,
 	}
 
@@ -168,7 +169,7 @@ func (f *ForkChoice) IsCanonical(root [32]byte) bool {
 }
 
 // IsOptimistic returns true if the given root has been optimistically synced.
-func (f *ForkChoice) IsOptimistic(_ context.Context, root [32]byte) (bool, error) {
+func (f *ForkChoice) IsOptimistic(root [32]byte) (bool, error) {
 	f.store.nodesLock.RLock()
 	defer f.store.nodesLock.RUnlock()
 
@@ -302,6 +303,6 @@ func (f *ForkChoice) ForkChoiceNodes() []*pbrpc.ForkChoiceNode {
 }
 
 // SetOptimisticToInvalid removes a block with an invalid execution payload from fork choice store
-func (f *ForkChoice) SetOptimisticToInvalid(ctx context.Context, root [fieldparams.RootLength]byte) ([][32]byte, error) {
-	return f.store.removeNode(ctx, root)
+func (f *ForkChoice) SetOptimisticToInvalid(ctx context.Context, root, payloadHash [fieldparams.RootLength]byte) ([][32]byte, error) {
+	return f.store.setOptimisticToInvalid(ctx, root, payloadHash)
 }

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

@@ -109,7 +109,7 @@ func (n *Node) leadsToViableHead(justifiedEpoch, finalizedEpoch types.Epoch) boo
 	return n.bestDescendant.viableForHead(justifiedEpoch, finalizedEpoch)
 }
 
-// setNodeAndParentValidated sets the current node and the parent as validated (i.e. non-optimistic).
+// setNodeAndParentValidated sets the current node and all the ancestors as validated (i.e. non-optimistic).
 func (n *Node) setNodeAndParentValidated(ctx context.Context) error {
 	if ctx.Err() != nil {
 		return ctx.Err()

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

@@ -180,27 +180,27 @@ func TestNode_SetFullyValidated(t *testing.T) {
 	require.NoError(t, f.InsertOptimisticBlock(ctx, 4, indexToHash(4), indexToHash(3), params.BeaconConfig().ZeroHash, 1, 1))
 	require.NoError(t, f.InsertOptimisticBlock(ctx, 5, indexToHash(5), indexToHash(1), params.BeaconConfig().ZeroHash, 1, 1))
 
-	opt, err := f.IsOptimistic(ctx, indexToHash(5))
+	opt, err := f.IsOptimistic(indexToHash(5))
 	require.NoError(t, err)
 	require.Equal(t, true, opt)
 
-	opt, err = f.IsOptimistic(ctx, indexToHash(4))
+	opt, err = f.IsOptimistic(indexToHash(4))
 	require.NoError(t, err)
 	require.Equal(t, true, opt)
 
 	require.NoError(t, f.store.nodeByRoot[indexToHash(4)].setNodeAndParentValidated(ctx))
 
 	// block 5 should still be optimistic
-	opt, err = f.IsOptimistic(ctx, indexToHash(5))
+	opt, err = f.IsOptimistic(indexToHash(5))
 	require.NoError(t, err)
 	require.Equal(t, true, opt)
 
 	// block 4 and 3 should now be valid
-	opt, err = f.IsOptimistic(ctx, indexToHash(4))
+	opt, err = f.IsOptimistic(indexToHash(4))
 	require.NoError(t, err)
 	require.Equal(t, false, opt)
 
-	opt, err = f.IsOptimistic(ctx, indexToHash(3))
+	opt, err = f.IsOptimistic(indexToHash(3))
 	require.NoError(t, err)
 	require.Equal(t, false, opt)
 }

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

@@ -2,22 +2,54 @@ package doublylinkedtree
 
 import (
 	"context"
+
+	"github.com/prysmaticlabs/prysm/config/params"
 )
 
+func (s *Store) setOptimisticToInvalid(ctx context.Context, root, payloadHash [32]byte) ([][32]byte, error) {
+	s.nodesLock.Lock()
+	invalidRoots := make([][32]byte, 0)
+	node, ok := s.nodeByRoot[root]
+	if !ok || node == nil {
+		s.nodesLock.Unlock()
+		return invalidRoots, ErrNilNode
+	}
+	// Check if last valid hash is an ancestor of the passed node.
+	lastValid, ok := s.nodeByPayload[payloadHash]
+	if !ok || lastValid == nil {
+		s.nodesLock.Unlock()
+		return invalidRoots, errUnknownPayloadHash
+	}
+	firstInvalid := node
+	for ; firstInvalid.parent != nil && firstInvalid.parent.payloadHash != payloadHash; firstInvalid = firstInvalid.parent {
+		if ctx.Err() != nil {
+			s.nodesLock.Unlock()
+			return invalidRoots, ctx.Err()
+		}
+	}
+	// If the last valid payload is in a different fork, we remove only the
+	// passed node.
+	if firstInvalid.parent == nil {
+		firstInvalid = node
+	}
+	s.nodesLock.Unlock()
+	return s.removeNode(ctx, firstInvalid)
+}
+
 // removeNode removes the node with the given root and all of its children
 // from the Fork Choice Store.
-func (s *Store) removeNode(ctx context.Context, root [32]byte) ([][32]byte, error) {
+func (s *Store) removeNode(ctx context.Context, node *Node) ([][32]byte, error) {
 	s.nodesLock.Lock()
 	defer s.nodesLock.Unlock()
 	invalidRoots := make([][32]byte, 0)
 
-	node, ok := s.nodeByRoot[root]
-	if !ok || node == nil {
+	if node == nil {
 		return invalidRoots, ErrNilNode
 	}
 	if !node.optimistic || node.parent == nil {
 		return invalidRoots, errInvalidOptimisticStatus
 	}
+
 	children := node.parent.children
 	if len(children) == 1 {
 		node.parent.children = []*Node{}
@@ -47,6 +79,16 @@ func (s *Store) removeNodeAndChildren(ctx context.Context, node *Node, invalidRo
 		}
 	}
 	invalidRoots = append(invalidRoots, node.root)
+	s.proposerBoostLock.Lock()
+	if node.root == s.proposerBoostRoot {
+		s.proposerBoostRoot = [32]byte{}
+	}
+	if node.root == s.previousProposerBoostRoot {
+		s.previousProposerBoostRoot = params.BeaconConfig().ZeroHash
+		s.previousProposerBoostScore = 0
+	}
+	s.proposerBoostLock.Unlock()
 	delete(s.nodeByRoot, node.root)
+	delete(s.nodeByPayload, node.payloadHash)
 	return invalidRoots, nil
 }

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

@@ -24,32 +24,71 @@ import (
 func TestPruneInvalid(t *testing.T) {
 	tests := []struct {
 		root             [32]byte // the root of the new INVALID block
+		payload          [32]byte // the last valid hash
 		wantedNodeNumber int
 		wantedRoots      [][32]byte
 	}{
 		{
 			[32]byte{'j'},
+			[32]byte{'B'},
 			12,
 			[][32]byte{[32]byte{'j'}},
 		},
 		{
 			[32]byte{'c'},
+			[32]byte{'B'},
 			4,
 			[][32]byte{[32]byte{'f'}, [32]byte{'e'}, [32]byte{'i'}, [32]byte{'h'}, [32]byte{'l'},
 				[32]byte{'k'}, [32]byte{'g'}, [32]byte{'d'}, [32]byte{'c'}},
 		},
 		{
 			[32]byte{'i'},
+			[32]byte{'H'},
 			12,
 			[][32]byte{[32]byte{'i'}},
 		},
 		{
 			[32]byte{'h'},
+			[32]byte{'G'},
 			11,
 			[][32]byte{[32]byte{'i'}, [32]byte{'h'}},
 		},
 		{
 			[32]byte{'g'},
+			[32]byte{'D'},
+			8,
+			[][32]byte{[32]byte{'i'}, [32]byte{'h'}, [32]byte{'l'}, [32]byte{'k'}, [32]byte{'g'}},
+		},
+		{
+			[32]byte{'i'},
+			[32]byte{'D'},
+			8,
+			[][32]byte{[32]byte{'i'}, [32]byte{'h'}, [32]byte{'l'}, [32]byte{'k'}, [32]byte{'g'}},
+		},
+		{
+			[32]byte{'f'},
+			[32]byte{'D'},
+			11,
+			[][32]byte{[32]byte{'f'}, [32]byte{'e'}},
+		},
+		{
+			[32]byte{'h'},
+			[32]byte{'C'},
+			5,
+			[][32]byte{
+				[32]byte{'f'},
+				[32]byte{'e'},
+				[32]byte{'i'},
+				[32]byte{'h'},
+				[32]byte{'l'},
+				[32]byte{'k'},
+				[32]byte{'g'},
+				[32]byte{'d'},
+			},
+		},
+		{
+			[32]byte{'g'},
+			[32]byte{'E'},
 			8,
 			[][32]byte{[32]byte{'i'}, [32]byte{'h'}, [32]byte{'l'}, [32]byte{'k'}, [32]byte{'g'}},
 		},
@@ -58,22 +97,45 @@ func TestPruneInvalid(t *testing.T) {
 		ctx := context.Background()
 		f := setup(1, 1)
 
-		require.NoError(t, f.InsertOptimisticBlock(ctx, 100, [32]byte{'a'}, params.BeaconConfig().ZeroHash, params.BeaconConfig().ZeroHash, 1, 1))
-		require.NoError(t, f.InsertOptimisticBlock(ctx, 101, [32]byte{'b'}, [32]byte{'a'}, params.BeaconConfig().ZeroHash, 1, 1))
-		require.NoError(t, f.InsertOptimisticBlock(ctx, 102, [32]byte{'c'}, [32]byte{'b'}, params.BeaconConfig().ZeroHash, 1, 1))
-		require.NoError(t, f.InsertOptimisticBlock(ctx, 102, [32]byte{'j'}, [32]byte{'b'}, params.BeaconConfig().ZeroHash, 1, 1))
-		require.NoError(t, f.InsertOptimisticBlock(ctx, 103, [32]byte{'d'}, [32]byte{'c'}, params.BeaconConfig().ZeroHash, 1, 1))
-		require.NoError(t, f.InsertOptimisticBlock(ctx, 104, [32]byte{'e'}, [32]byte{'d'}, params.BeaconConfig().ZeroHash, 1, 1))
-		require.NoError(t, f.InsertOptimisticBlock(ctx, 104, [32]byte{'g'}, [32]byte{'d'}, params.BeaconConfig().ZeroHash, 1, 1))
-		require.NoError(t, f.InsertOptimisticBlock(ctx, 105, [32]byte{'f'}, [32]byte{'e'}, params.BeaconConfig().ZeroHash, 1, 1))
-		require.NoError(t, f.InsertOptimisticBlock(ctx, 105, [32]byte{'h'}, [32]byte{'g'}, params.BeaconConfig().ZeroHash, 1, 1))
-		require.NoError(t, f.InsertOptimisticBlock(ctx, 105, [32]byte{'k'}, [32]byte{'g'}, params.BeaconConfig().ZeroHash, 1, 1))
-		require.NoError(t, f.InsertOptimisticBlock(ctx, 106, [32]byte{'i'}, [32]byte{'h'}, params.BeaconConfig().ZeroHash, 1, 1))
-		require.NoError(t, f.InsertOptimisticBlock(ctx, 106, [32]byte{'l'}, [32]byte{'k'}, params.BeaconConfig().ZeroHash, 1, 1))
+		require.NoError(t, f.InsertOptimisticBlock(ctx, 100, [32]byte{'a'}, params.BeaconConfig().ZeroHash, [32]byte{'A'}, 1, 1))
+		require.NoError(t, f.InsertOptimisticBlock(ctx, 101, [32]byte{'b'}, [32]byte{'a'}, [32]byte{'B'}, 1, 1))
+		require.NoError(t, f.InsertOptimisticBlock(ctx, 102, [32]byte{'c'}, [32]byte{'b'}, [32]byte{'C'}, 1, 1))
+		require.NoError(t, f.InsertOptimisticBlock(ctx, 102, [32]byte{'j'}, [32]byte{'b'}, [32]byte{'J'}, 1, 1))
+		require.NoError(t, f.InsertOptimisticBlock(ctx, 103, [32]byte{'d'}, [32]byte{'c'}, [32]byte{'D'}, 1, 1))
+		require.NoError(t, f.InsertOptimisticBlock(ctx, 104, [32]byte{'e'}, [32]byte{'d'}, [32]byte{'E'}, 1, 1))
+		require.NoError(t, f.InsertOptimisticBlock(ctx, 104, [32]byte{'g'}, [32]byte{'d'}, [32]byte{'G'}, 1, 1))
+		require.NoError(t, f.InsertOptimisticBlock(ctx, 105, [32]byte{'f'}, [32]byte{'e'}, [32]byte{'F'}, 1, 1))
+		require.NoError(t, f.InsertOptimisticBlock(ctx, 105, [32]byte{'h'}, [32]byte{'g'}, [32]byte{'H'}, 1, 1))
+		require.NoError(t, f.InsertOptimisticBlock(ctx, 105, [32]byte{'k'}, [32]byte{'g'}, [32]byte{'K'}, 1, 1))
+		require.NoError(t, f.InsertOptimisticBlock(ctx, 106, [32]byte{'i'}, [32]byte{'h'}, [32]byte{'I'}, 1, 1))
+		require.NoError(t, f.InsertOptimisticBlock(ctx, 106, [32]byte{'l'}, [32]byte{'k'}, [32]byte{'L'}, 1, 1))
 
-		roots, err := f.store.removeNode(context.Background(), tc.root)
+		roots, err := f.store.setOptimisticToInvalid(context.Background(), tc.root, tc.payload)
 		require.NoError(t, err)
 		require.DeepEqual(t, tc.wantedRoots, roots)
 		require.Equal(t, tc.wantedNodeNumber, f.NodeCount())
 	}
 }
+
+// This is a regression test (10445)
+func TestSetOptimisticToInvalid_ProposerBoost(t *testing.T) {
+	ctx := context.Background()
+	f := setup(1, 1)
+
+	require.NoError(t, f.InsertOptimisticBlock(ctx, 100, [32]byte{'a'}, params.BeaconConfig().ZeroHash, [32]byte{'A'}, 1, 1))
+	require.NoError(t, f.InsertOptimisticBlock(ctx, 101, [32]byte{'b'}, [32]byte{'a'}, [32]byte{'B'}, 1, 1))
+	require.NoError(t, f.InsertOptimisticBlock(ctx, 101, [32]byte{'c'}, [32]byte{'b'}, [32]byte{'C'}, 1, 1))
+	f.store.proposerBoostLock.Lock()
+	f.store.proposerBoostRoot = [32]byte{'c'}
+	f.store.previousProposerBoostScore = 10
+	f.store.previousProposerBoostRoot = [32]byte{'b'}
+	f.store.proposerBoostLock.Unlock()
+
+	_, err := f.SetOptimisticToInvalid(ctx, [32]byte{'c'}, [32]byte{'A'})
+	require.NoError(t, err)
+	f.store.proposerBoostLock.RLock()
+	require.Equal(t, uint64(0), f.store.previousProposerBoostScore)
+	require.DeepEqual(t, [32]byte{}, f.store.proposerBoostRoot)
+	require.DeepEqual(t, params.BeaconConfig().ZeroHash, f.store.previousProposerBoostRoot)
+	f.store.proposerBoostLock.RUnlock()
+}

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

@@ -121,6 +121,7 @@ func (s *Store) insert(ctx context.Context,
 		payloadHash:    payloadHash,
 	}
 
+	s.nodeByPayload[payloadHash] = n
 	s.nodeByRoot[root] = n
 	if parent != nil {
 		parent.children = append(parent.children, n)

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

@@ -107,7 +107,8 @@ func TestStore_Insert(t *testing.T) {
 	// The new node does not have a parent.
 	treeRootNode := &Node{slot: 0, root: indexToHash(0)}
 	nodeByRoot := map[[32]byte]*Node{indexToHash(0): treeRootNode}
-	s := &Store{nodeByRoot: nodeByRoot, treeRootNode: treeRootNode}
+	nodeByPayload := map[[32]byte]*Node{indexToHash(0): treeRootNode}
+	s := &Store{nodeByRoot: nodeByRoot, treeRootNode: treeRootNode, nodeByPayload: nodeByPayload}
 	payloadHash := [32]byte{'a'}
 	require.NoError(t, s.insert(context.Background(), 100, indexToHash(100), indexToHash(0), payloadHash, 1, 1))
 	assert.Equal(t, 2, len(s.nodeByRoot), "Did not insert block")

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

@@ -26,6 +26,7 @@ type Store struct {
 	treeRootNode               *Node                                  // the root node of the store tree.
 	headNode                   *Node                                  // last head Node
 	nodeByRoot                 map[[fieldparams.RootLength]byte]*Node // nodes indexed by roots.
+	nodeByPayload              map[[fieldparams.RootLength]byte]*Node // nodes indexed by payload Hash
 	nodesLock                  sync.RWMutex
 	proposerBoostLock          sync.RWMutex
 }

beacon-chain/forkchoice/interfaces.go

@@ -24,7 +24,7 @@ type ForkChoicer interface {
 type HeadRetriever interface {
 	Head(context.Context, types.Epoch, [32]byte, []uint64, types.Epoch) ([32]byte, error)
 	Tips() ([][32]byte, []types.Slot)
-	IsOptimistic(ctx context.Context, root [32]byte) (bool, error)
+	IsOptimistic(root [32]byte) (bool, error)
 }
 
 // BlockProcessor processes the block that's used for accounting fork choice.
@@ -71,5 +71,5 @@ type Getter interface {
 // Setter allows to set forkchoice information
 type Setter interface {
 	SetOptimisticToValid(context.Context, [fieldparams.RootLength]byte) error
-	SetOptimisticToInvalid(context.Context, [fieldparams.RootLength]byte) ([][32]byte, error)
+	SetOptimisticToInvalid(context.Context, [fieldparams.RootLength]byte, [fieldparams.RootLength]byte) ([][32]byte, error)
 }

beacon-chain/forkchoice/protoarray/errors.go

@@ -5,11 +5,11 @@ import "errors"
 var errUnknownFinalizedRoot = errors.New("unknown finalized root")
 var errUnknownJustifiedRoot = errors.New("unknown justified root")
 var errInvalidNodeIndex = errors.New("node index is invalid")
+var errInvalidFinalizedNode = errors.New("invalid finalized block on chain")
 var ErrUnknownNodeRoot = errors.New("unknown block root")
 var errInvalidJustifiedIndex = errors.New("justified index is invalid")
-var errInvalidBestChildIndex = errors.New("best child index is invalid")
 var errInvalidBestDescendantIndex = errors.New("best descendant index is invalid")
 var errInvalidParentDelta = errors.New("parent delta is invalid")
 var errInvalidNodeDelta = errors.New("node delta is invalid")
 var errInvalidDeltaLength = errors.New("delta length is invalid")
-var errInvalidSyncedTips = errors.New("invalid synced tips")
+var errInvalidOptimisticStatus = errors.New("invalid optimistic status")

beacon-chain/forkchoice/protoarray/helpers.go

@@ -85,18 +85,16 @@ func copyNode(node *Node) *Node {
 		return &Node{}
 	}
 
-	copiedRoot := [32]byte{}
-	copy(copiedRoot[:], node.root[:])
-
 	return &Node{
 		slot:           node.slot,
-		root:           copiedRoot,
+		root:           node.root,
 		parent:         node.parent,
 		payloadHash:    node.payloadHash,
 		justifiedEpoch: node.justifiedEpoch,
 		finalizedEpoch: node.finalizedEpoch,
 		weight:         node.weight,
 		bestChild:      node.bestChild,
 		bestDescendant: node.bestDescendant,
+		status:         node.status,
 	}
 }