From 88af3f90a579dbeeb786193ab4e9f1b4f2c50be4 Mon Sep 17 00:00:00 2001 From: Potuz Date: Tue, 16 Sep 2025 20:53:19 -0300 Subject: [PATCH] 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 | 8 ++------ beacon-chain/blockchain/process_block_helpers.go | 13 ++++--------- beacon-chain/blockchain/setup_forkchoice.go | 7 +++---- .../forkchoice/doubly-linked-tree/forkchoice.go | 16 +++++++--------- .../doubly-linked-tree/forkchoice_test.go | 13 +++++++------ changelog/potuz_change_insertchain.md | 3 +++ 6 files changed, 26 insertions(+), 34 deletions(-) create mode 100644 changelog/potuz_change_insertchain.md diff --git a/beacon-chain/blockchain/process_block.go b/beacon-chain/blockchain/process_block.go index 5d08c40b58..91d9217c27 100644 --- a/beacon-chain/blockchain/process_block.go +++ b/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 { diff --git a/beacon-chain/blockchain/process_block_helpers.go b/beacon-chain/blockchain/process_block_helpers.go index 0b9a441db6..9e15c56719 100644 --- a/beacon-chain/blockchain/process_block_helpers.go +++ b/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) } diff --git a/beacon-chain/blockchain/setup_forkchoice.go b/beacon-chain/blockchain/setup_forkchoice.go index 0761fd707a..deea24d440 100644 --- a/beacon-chain/blockchain/setup_forkchoice.go +++ b/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 } diff --git a/beacon-chain/forkchoice/doubly-linked-tree/forkchoice.go b/beacon-chain/forkchoice/doubly-linked-tree/forkchoice.go index 32e0073957..500fe988bd 100644 --- a/beacon-chain/forkchoice/doubly-linked-tree/forkchoice.go +++ b/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 } } diff --git a/beacon-chain/forkchoice/doubly-linked-tree/forkchoice_test.go b/beacon-chain/forkchoice/doubly-linked-tree/forkchoice_test.go index b155d9c296..76d7319b29 100644 --- a/beacon-chain/forkchoice/doubly-linked-tree/forkchoice_test.go +++ b/beacon-chain/forkchoice/doubly-linked-tree/forkchoice_test.go @@ -630,14 +630,15 @@ func TestStore_InsertChain(t *testing.T) { FinalizedCheckpoint: ðpb.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) { diff --git a/changelog/potuz_change_insertchain.md b/changelog/potuz_change_insertchain.md new file mode 100644 index 0000000000..3c082a6f97 --- /dev/null +++ b/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.