Change insertchain (#15688)

* Change InsertChain

InsertChain uses `ROBlock` since #14571, this allows it to insert the
last block of the chain as well. We change the semantics of InsertChain
to include all blocks and take them in increasing order.

* Fix tests

* Use slices.Reverse

beacon-chain/blockchain/process_block.go

@@ -247,7 +247,7 @@ func (s *Service) onBlockBatch(ctx context.Context, blks []consensusblocks.ROBlo
 		args := &forkchoicetypes.BlockAndCheckpoints{Block: b,
 			JustifiedCheckpoint: jCheckpoints[i],
 			FinalizedCheckpoint: fCheckpoints[i]}
-		pendingNodes[len(blks)-i-1] = args
+		pendingNodes[i] = args
 		if err := s.saveInitSyncBlock(ctx, root, b); err != nil {
 			tracing.AnnotateError(span, err)
 			return err
@@ -284,14 +284,10 @@ func (s *Service) onBlockBatch(ctx context.Context, blks []consensusblocks.ROBlo
 	if err := s.cfg.StateGen.SaveState(ctx, lastBR, preState); err != nil {
 		return err
 	}
-	// Insert all nodes but the last one to forkchoice
+	// Insert all nodes to forkchoice
 	if err := s.cfg.ForkChoiceStore.InsertChain(ctx, pendingNodes); err != nil {
 		return errors.Wrap(err, "could not insert batch to forkchoice")
 	}
-	// Insert the last block to forkchoice
-	if err := s.cfg.ForkChoiceStore.InsertNode(ctx, preState, lastB); err != nil {
-		return errors.Wrap(err, "could not insert last block in batch to forkchoice")
-	}
 	// Set their optimistic status
 	if isValidPayload {
 		if err := s.cfg.ForkChoiceStore.SetOptimisticToValid(ctx, lastBR); err != nil {

beacon-chain/blockchain/process_block_helpers.go

@@ -3,6 +3,7 @@ package blockchain
 import (
 	"context"
 	"fmt"
+	"slices"
 	"time"
 
 	"github.com/OffchainLabs/prysm/v6/beacon-chain/core/feed"
@@ -376,15 +377,8 @@ func (s *Service) fillInForkChoiceMissingBlocks(ctx context.Context, signed inte
 	if err != nil {
 		return err
 	}
-	// The first block can have a bogus root since the block is not inserted in forkchoice
-	roblock, err := consensus_blocks.NewROBlockWithRoot(signed, [32]byte{})
-	if err != nil {
-		return err
-	}
-	pendingNodes = append(pendingNodes, &forkchoicetypes.BlockAndCheckpoints{Block: roblock,
-		JustifiedCheckpoint: jCheckpoint, FinalizedCheckpoint: fCheckpoint})
+	root := signed.Block().ParentRoot()
 	// As long as parent node is not in fork choice store, and parent node is in DB.
-	root := roblock.Block().ParentRoot()
 	for !s.cfg.ForkChoiceStore.HasNode(root) && s.cfg.BeaconDB.HasBlock(ctx, root) {
 		b, err := s.getBlock(ctx, root)
 		if err != nil {
@@ -403,12 +397,13 @@ func (s *Service) fillInForkChoiceMissingBlocks(ctx context.Context, signed inte
 			FinalizedCheckpoint: fCheckpoint}
 		pendingNodes = append(pendingNodes, args)
 	}
-	if len(pendingNodes) == 1 {
+	if len(pendingNodes) == 0 {
 		return nil
 	}
 	if root != s.ensureRootNotZeros(finalized.Root) && !s.cfg.ForkChoiceStore.HasNode(root) {
 		return ErrNotDescendantOfFinalized
 	}
+	slices.Reverse(pendingNodes)
 	return s.cfg.ForkChoiceStore.InsertChain(ctx, pendingNodes)
 }
 

beacon-chain/blockchain/setup_forkchoice.go

@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"context"
 	"fmt"
+	"slices"
 
 	forkchoicetypes "github.com/OffchainLabs/prysm/v6/beacon-chain/forkchoice/types"
 	"github.com/OffchainLabs/prysm/v6/beacon-chain/state"
@@ -78,10 +79,7 @@ func (s *Service) setupForkchoiceTree(st state.BeaconState) error {
 	}
 	s.cfg.ForkChoiceStore.Lock()
 	defer s.cfg.ForkChoiceStore.Unlock()
-	if err := s.cfg.ForkChoiceStore.InsertChain(s.ctx, chain); err != nil {
-		return errors.Wrap(err, "could not insert forkchoice chain")
-	}
-	return s.cfg.ForkChoiceStore.InsertNode(s.ctx, st, chain[0].Block)
+	return s.cfg.ForkChoiceStore.InsertChain(s.ctx, chain)
 }
 
 func (s *Service) buildForkchoiceChain(ctx context.Context, head interfaces.ReadOnlySignedBeaconBlock) ([]*forkchoicetypes.BlockAndCheckpoints, error) {
@@ -115,6 +113,7 @@ func (s *Service) buildForkchoiceChain(ctx context.Context, head interfaces.Read
 			return nil, errors.New("head block is not a descendant of the finalized checkpoint")
 		}
 	}
+	slices.Reverse(chain)
 	return chain, nil
 }
 

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

@@ -463,22 +463,20 @@ func (f *ForkChoice) CommonAncestor(ctx context.Context, r1 [32]byte, r2 [32]byt
 }
 
 // InsertChain inserts all nodes corresponding to blocks in the slice
-// `blocks`. This slice must be ordered from child to parent. It includes all
-// blocks **except** the first one (that is the one with the highest slot
-// number). All blocks are assumed to be a strict chain
-// where blocks[i].Parent = blocks[i+1]. Also, we assume that the parent of the
-// last block in this list is already included in forkchoice store.
+// `blocks`. This slice must be ordered in increasing slot order and
+// each consecutive entry must be a child of the previous one.
+// The parent of the first block in this list must already be present in forkchoice.
 func (f *ForkChoice) InsertChain(ctx context.Context, chain []*forkchoicetypes.BlockAndCheckpoints) error {
 	if len(chain) == 0 {
 		return nil
 	}
-	for i := len(chain) - 1; i > 0; i-- {
+	for _, bcp := range chain {
 		if _, err := f.store.insert(ctx,
-			chain[i].Block,
-			chain[i].JustifiedCheckpoint.Epoch, chain[i].FinalizedCheckpoint.Epoch); err != nil {
+			bcp.Block,
+			bcp.JustifiedCheckpoint.Epoch, bcp.FinalizedCheckpoint.Epoch); err != nil {
 			return err
 		}
-		if err := f.updateCheckpoints(ctx, chain[i].JustifiedCheckpoint, chain[i].FinalizedCheckpoint); err != nil {
+		if err := f.updateCheckpoints(ctx, bcp.JustifiedCheckpoint, bcp.FinalizedCheckpoint); err != nil {
 			return err
 		}
 	}

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

@@ -630,14 +630,15 @@ func TestStore_InsertChain(t *testing.T) {
 			FinalizedCheckpoint: &ethpb.Checkpoint{Epoch: 1, Root: params.BeaconConfig().ZeroHash[:]},
 		})
 	}
-	args := make([]*forkchoicetypes.BlockAndCheckpoints, 10)
-	for i := 0; i < len(blks); i++ {
-		args[i] = blks[10-i-1]
-	}
-	require.NoError(t, f.InsertChain(t.Context(), args))
+	// InsertChain now expects blocks in increasing slot order
+	require.NoError(t, f.InsertChain(t.Context(), blks))
 
+	// Test partial insertion: first insert the foundation blocks, then a subset
 	f = setup(1, 1)
-	require.NoError(t, f.InsertChain(t.Context(), args[2:]))
+	// Insert first 2 blocks to establish a chain from genesis
+	require.NoError(t, f.InsertChain(t.Context(), blks[:2]))
+	// Then insert the remaining blocks
+	require.NoError(t, f.InsertChain(t.Context(), blks[2:]))
 }
 
 func TestForkChoice_UpdateCheckpoints(t *testing.T) {

changelog/potuz_change_insertchain.md

@@ -0,0 +1,3 @@
+### Ignored
+
+- Change InsertChain to use be increasingly ordered and insert the last block of the chain.