From b9703bc122d90c7dabae4f2775044ed5403f9f6b Mon Sep 17 00:00:00 2001 From: Kasey Kirkham Date: Tue, 4 Mar 2025 16:22:27 -0600 Subject: [PATCH] clean up block-slot-indices on block deletion --- beacon-chain/db/kv/blocks.go | 71 ++++++++++++++++++++----- beacon-chain/db/kv/blocks_test.go | 17 ++++++ beacon-chain/db/kv/utils.go | 25 +++++++++ beacon-chain/db/kv/utils_test.go | 80 +++++++++++++++++++++++++++++ changelog/kasey_delete-block-idx.md | 2 + 5 files changed, 183 insertions(+), 12 deletions(-) create mode 100644 changelog/kasey_delete-block-idx.md diff --git a/beacon-chain/db/kv/blocks.go b/beacon-chain/db/kv/blocks.go index e99f5ed91dbf..2b638678b3a9 100644 --- a/beacon-chain/db/kv/blocks.go +++ b/beacon-chain/db/kv/blocks.go @@ -30,22 +30,32 @@ var errInvalidSlotRange = errors.New("invalid end slot and start slot provided") func (s *Store) Block(ctx context.Context, blockRoot [32]byte) (interfaces.ReadOnlySignedBeaconBlock, error) { ctx, span := trace.StartSpan(ctx, "BeaconDB.Block") defer span.End() - // Return block from cache if it exists. + blk, err := s.getBlock(ctx, blockRoot, nil) + if errors.Is(err, ErrNotFound) { + return nil, nil + } + return blk, err +} + +func (s *Store) getBlock(ctx context.Context, blockRoot [32]byte, tx *bolt.Tx) (interfaces.ReadOnlySignedBeaconBlock, error) { if v, ok := s.blockCache.Get(string(blockRoot[:])); v != nil && ok { return v.(interfaces.ReadOnlySignedBeaconBlock), nil } - var blk interfaces.ReadOnlySignedBeaconBlock - err := s.db.View(func(tx *bolt.Tx) error { - bkt := tx.Bucket(blocksBucket) - enc := bkt.Get(blockRoot[:]) - if enc == nil { - return nil - } + // This method allows the caller to pass in its tx if one is already open. + // Or if a nil value is used, a transaction will be managed intenally. + if tx == nil { var err error - blk, err = unmarshalBlock(ctx, enc) - return err - }) - return blk, err + tx, err = s.db.Begin(false) + if err != nil { + return nil, err + } + defer func() { + if err := tx.Rollback(); err != nil { + log.WithError(err).Error("could not rollback read-only getBlock transaction") + } + }() + } + return unmarshalBlock(ctx, tx.Bucket(blocksBucket).Get(blockRoot[:])) } // OriginCheckpointBlockRoot returns the value written to the db in SaveOriginCheckpointBlockRoot @@ -227,6 +237,18 @@ func (s *Store) DeleteBlock(ctx context.Context, root [32]byte) error { return ErrDeleteJustifiedAndFinalized } + // Look up the block to find its slot; needed to remove the slot index entry. + blk, err := s.getBlock(ctx, root, tx) + if err != nil { + // getBlock can return ErrNotFound, in which case we won't even try to delete it. + if errors.Is(err, ErrNotFound) { + return nil + } + return err + } + if err := s.deleteSlotIndexEntry(tx, blk.Block().Slot(), root); err != nil { + return err + } if err := s.deleteBlock(tx, root[:]); err != nil { return err } @@ -899,6 +921,9 @@ func createBlockIndicesFromFilters(ctx context.Context, f *filters.QueryFilter) // unmarshal block from marshaled proto beacon block bytes to versioned beacon block struct type. func unmarshalBlock(_ context.Context, enc []byte) (interfaces.ReadOnlySignedBeaconBlock, error) { + if len(enc) == 0 { + return nil, errors.Wrap(ErrNotFound, "empty block bytes in db") + } var err error enc, err = snappy.Decode(nil, enc) if err != nil { @@ -1050,6 +1075,28 @@ func (s *Store) deleteBlock(tx *bolt.Tx, root []byte) error { return nil } +func (s *Store) deleteSlotIndexEntry(tx *bolt.Tx, slot primitives.Slot, root [32]byte) error { + key := bytesutil.SlotToBytesBigEndian(slot) + bkt := tx.Bucket(blockSlotIndicesBucket) + packed := bkt.Get(key) + if len(packed) == 0 { + return nil + } + updated, err := removeRoot(packed, root) + if err != nil { + return errors.Wrap(err, "could not remove root from slot index with invalid packing") + } + // If there are no other roots in the key, just delete it. + if len(updated) == 0 { + if err := bkt.Delete(key); err != nil { + return errors.Wrap(err, "could not delete slot index") + } + return nil + } + // Update the key with the root removed. + return bkt.Put(key, updated) +} + func (s *Store) deleteValidatorHashes(tx *bolt.Tx, root []byte) error { ok, err := s.isStateValidatorMigrationOver() if err != nil { diff --git a/beacon-chain/db/kv/blocks_test.go b/beacon-chain/db/kv/blocks_test.go index 80362ffd964a..a6de256cdb01 100644 --- a/beacon-chain/db/kv/blocks_test.go +++ b/beacon-chain/db/kv/blocks_test.go @@ -196,9 +196,13 @@ func TestStore_BlocksCRUD(t *testing.T) { blockRoot, err := blk.Block().HashTreeRoot() require.NoError(t, err) + _, err = db.getBlock(ctx, blockRoot, nil) + require.ErrorIs(t, err, ErrNotFound) retrievedBlock, err := db.Block(ctx, blockRoot) require.NoError(t, err) assert.DeepEqual(t, nil, retrievedBlock, "Expected nil block") + _, err = db.getBlock(ctx, blockRoot, nil) + require.ErrorIs(t, err, ErrNotFound) require.NoError(t, db.SaveBlock(ctx, blk)) assert.Equal(t, true, db.HasBlock(ctx, blockRoot), "Expected block to exist in the db") @@ -214,6 +218,19 @@ func TestStore_BlocksCRUD(t *testing.T) { retrievedPb, err := retrievedBlock.Proto() require.NoError(t, err) assert.Equal(t, true, proto.Equal(wantedPb, retrievedPb), "Wanted: %v, received: %v", wanted, retrievedBlock) + // Check that the block is in the slot->block index + found, roots, err := db.BlockRootsBySlot(ctx, blk.Block().Slot()) + require.NoError(t, err) + require.Equal(t, true, found) + require.Equal(t, 1, len(roots)) + require.Equal(t, blockRoot, roots[0]) + // Delete the block, then check that it is no longer in the index. + require.NoError(t, db.DeleteBlock(ctx, blockRoot)) + require.NoError(t, err) + found, roots, err = db.BlockRootsBySlot(ctx, blk.Block().Slot()) + require.NoError(t, err) + require.Equal(t, false, found) + require.Equal(t, 0, len(roots)) }) } } diff --git a/beacon-chain/db/kv/utils.go b/beacon-chain/db/kv/utils.go index 522b3860cd5c..e4837ce9b13f 100644 --- a/beacon-chain/db/kv/utils.go +++ b/beacon-chain/db/kv/utils.go @@ -114,3 +114,28 @@ func splitRoots(b []byte) ([][32]byte, error) { } return rl, nil } + +func removeRoot(roots []byte, root [32]byte) ([]byte, error) { + if len(roots) == 0 { + return []byte{}, nil + } + if len(roots) == 32 && bytes.Equal(roots, root[:]) { + return []byte{}, nil + } + if len(roots)%32 != 0 { + return nil, errors.Wrapf(errMisalignedRootList, "root list len=%d", len(roots)) + } + rEnd := 0 + for s := 0; s <= len(roots)-32; s += 32 { + end := s + 32 + if !bytes.Equal(roots[s:end], root[:]) { + continue + } + rEnd = end + break + } + if rEnd == 0 { + return roots, nil + } + return append(roots[:rEnd-32], roots[rEnd:]...), nil +} diff --git a/beacon-chain/db/kv/utils_test.go b/beacon-chain/db/kv/utils_test.go index d78290aaa4bc..7394553f68fd 100644 --- a/beacon-chain/db/kv/utils_test.go +++ b/beacon-chain/db/kv/utils_test.go @@ -1,6 +1,7 @@ package kv import ( + "bytes" "context" "crypto/rand" "testing" @@ -195,3 +196,82 @@ func TestSplitRoots(t *testing.T) { }) } } + +func tPad(p ...[]byte) []byte { + r := make([]byte, 32*len(p)) + for i, b := range p { + copy(r[i*32:], b) + } + return r +} + +func TestRemoveRoot(t *testing.T) { + cases := []struct { + name string + roots []byte + root [32]byte + expect []byte + err error + }{ + { + name: "empty", + roots: []byte{}, + root: [32]byte{0xde, 0xad, 0xbe, 0xef}, + expect: []byte{}, + }, + { + name: "single", + roots: tPad([]byte{0xde, 0xad, 0xbe, 0xef}), + root: [32]byte{0xde, 0xad, 0xbe, 0xef}, + expect: []byte{}, + }, + { + name: "single, different", + roots: tPad([]byte{0xde, 0xad, 0xbe, 0xef}), + root: [32]byte{0xde, 0xad, 0xbe, 0xee}, + expect: tPad([]byte{0xde, 0xad, 0xbe, 0xef}), + }, + { + name: "multi", + roots: tPad([]byte{0xde, 0xad, 0xbe, 0xef}, []byte{0xac, 0x1d, 0xfa, 0xce}), + root: [32]byte{0xac, 0x1d, 0xfa, 0xce}, + expect: tPad([]byte{0xde, 0xad, 0xbe, 0xef}), + }, + { + name: "multi, reordered", + roots: tPad([]byte{0xac, 0x1d, 0xfa, 0xce}, []byte{0xde, 0xad, 0xbe, 0xef}), + root: [32]byte{0xac, 0x1d, 0xfa, 0xce}, + expect: tPad([]byte{0xde, 0xad, 0xbe, 0xef}), + }, + { + name: "multi, 3", + roots: tPad([]byte{0xac, 0x1d, 0xfa, 0xce}, []byte{0xbe, 0xef, 0xca, 0xb5}, []byte{0xde, 0xad, 0xbe, 0xef}), + root: [32]byte{0xac, 0x1d, 0xfa, 0xce}, + expect: tPad([]byte{0xbe, 0xef, 0xca, 0xb5}, []byte{0xde, 0xad, 0xbe, 0xef}), + }, + { + name: "multi, different", + roots: tPad([]byte{0xde, 0xad, 0xbe, 0xef}, []byte{0xac, 0x1d, 0xfa, 0xce}), + root: [32]byte{0xac, 0x1d, 0xbe, 0xa7}, + expect: tPad([]byte{0xde, 0xad, 0xbe, 0xef}, []byte{0xac, 0x1d, 0xfa, 0xce}), + }, + { + name: "misaligned", + roots: make([]byte, 61), + root: [32]byte{0xac, 0x1d, 0xbe, 0xa7}, + err: errMisalignedRootList, + }, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + r, err := removeRoot(c.roots, c.root) + if c.err != nil { + require.ErrorIs(t, err, c.err) + return + } + require.NoError(t, err) + require.Equal(t, len(c.expect), len(r)) + require.Equal(t, true, bytes.Equal(c.expect, r)) + }) + } +} diff --git a/changelog/kasey_delete-block-idx.md b/changelog/kasey_delete-block-idx.md new file mode 100644 index 000000000000..17127a0c00b4 --- /dev/null +++ b/changelog/kasey_delete-block-idx.md @@ -0,0 +1,2 @@ +### Fixed +- Ensure that deleting a block from the database clears its entry in the slot->root db index.