perf(storage): prune old block signatures with a range delete

[MegaRedHand]

Motivation

Store::prune_old_block_signatures runs on every block import. It collected the keys to remove by walking BlockSignatures from the head of the table, taking entries below cutoff = tip_slot - SIGNATURE_PRUNING_RANGE.

The cutoff advances one slot at a time, so each pass deletes roughly one block's signatures while re-seeking past every tombstone the previous passes left behind. The scan's cost tracks chain height, not the handful of keys actually leaving the retention window.

Description

Adds StorageWriteBatch::delete_range(table, from, to) (half-open [from, to)) and uses it to drop the whole finalized range in one operation:

• RocksDB: WriteBatch::delete_range_cf — one range tombstone, the table is never read.
• In-memory: batch operations became an ordered Vec<(Table, PendingOp)> instead of a per-key HashMap, so a range delete interleaves with point operations the way RocksDB applies a WriteBatch. The range itself is applied with retain.

BlockSignatures keys are slot||root in big-endian slot order, so the cutoff's bare 8-byte slot prefix is an exact upper bound: every key below the cutoff sorts before it, and every key at the cutoff sorts after it (they extend it with a root).

Behavior change

prune_old_block_signatures now returns the exclusive slot it pruned below (0 = nothing pruned) instead of a key count, which a range delete cannot report without reading the table back. The log field in prune_old_data is now pruned_below_slot. Also returns early when the cutoff saturates to 0, so no empty tombstone is written on a young chain.

How to test

• cargo test -p ethlambda-storage — the three existing pruning tests still assert the surviving entries; two new cases were added to the shared backend suite (run against both backends): half-open range bounds, and a put_batch after a delete_range in the same batch winning.
• Full make lint + cargo test --workspace (including fork choice / signature / STF spec tests) pass.

[github-actions]

🤖 Kimi Code Review

The PR introduces a delete_range storage operation to optimize pruning of old block signatures using RocksDB range tombstones instead of point deletes. This is a sound performance optimization with correct semantics.

Code Quality & Correctness

1. In-memory backend semantics (crates/storage/src/backend/in_memory.rs:13-21, 108-141):
   Changing PendingOps from a HashMap to a Vec preserves the "last writer wins" batch semantics required to match RocksDB WriteBatch behavior. The DeleteRange implementation correctly handles the half-open interval [from, to) via retain.

2. Range bound calculation (crates/storage/src/store.rs:524-528, 1101-1108):
   The slot_root_key_bound function correctly generates the 8-byte slot prefix. Since keys are slot || root (8 + 32 bytes), any key for slot n sorts after the 8-byte bound of slot n but before the bound of slot n+1. Thus the range [bound(0), bound(cutoff)) precisely covers slots < cutoff.

3. RocksDB implementation (crates/storage/src/backend/rocksdb.rs:170-177):
   Correctly uses delete_range_cf. Note that range tombstones can impact iterator performance if they accumulate, but since these prunes monotonically expand the range [0, cutoff) and RocksDB compacts overlapping tombstones, this is acceptable.

Minor Observations

• Performance of in-memory range delete (crates/storage/src/backend/in_memory.rs:136-138):
  The retain call scans the entire table. This is $O(n)$ for the table size, not the range size. While acceptable for a test backend, consider documenting this asymmetry with the RocksDB backend.

• Return type semantic change (crates/storage/src/store.rs:1096):
  The change from usize (count) to u64 (exclusive upper bound) is correctly propagated to tests and logging. Verify that downstream consumers (if any) expect the slot number rather than the count.

Security & Consensus

• Consensus safety: The pruning only affects signatures for slots < tip - SIGNATURE_PRUNING_RANGE that are also finalized. These signatures are not required for fork choice or re-org safety, so the optimization does not affect consensus correctness.
• Atomicity: The range delete and the subsequent put (if any) are in the same batch, ensuring atomic application.

Verdict

Approved. The implementation correctly models RocksDB range tombstones in the in-memory backend and safely optimizes the signature pruning path. Ensure the performance characteristics of range tombstones are monitored under heavy load.

---

Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt

[github-actions]

🤖 Codex Code Review

1. crates/storage/src/store.rs:1090-1119 changes the method contract in a way that can report a prune when nothing was actually deleted. prune_old_block_signatures now returns cutoff unconditionally after issuing delete_range, but the doc says it returns 0 “when nothing was pruned.” On a repeated call after the range has already been deleted, this will still return a non-zero value and the caller at crates/storage/src/store.rs:921-925 will log "Pruned old finalized block signatures" even though the operation was a no-op. Either preserve the old semantic by checking whether anything exists below cutoff before returning non-zero, or rename/document the return value as an attempted prune boundary rather than “signatures were dropped.” Add an idempotence test for calling prune_old_block_signatures twice with the same inputs.

The rest of the change looks mechanically sound: the in-memory backend now preserves write-batch call order, which is the right model for matching RocksDB semantics, and the slot-prefix range bound is correct for slot || root big-endian keys. I did not see consensus/fork-choice/STF/XMSS/SSZ logic touched by this PR beyond the storage pruning path.

I couldn’t run the test suite in this environment because cargo needs to resolve the workspace’s leanSig git dependency and network access is blocked.

---

Automated review by OpenAI Codex · gpt-5.4 · custom prompt

[greptile-apps]

Greptile Summary

This PR replaces per-key block-signature pruning with an ordered, half-open range-delete operation.

• Adds StorageWriteBatch::delete_range for RocksDB and in-memory backends.
• Preserves write-batch operation ordering in the in-memory implementation.
• Deletes finalized BlockSignatures below the retention cutoff using big-endian slot bounds.
• Changes pruning’s return value from a deleted-key count to the exclusive cutoff slot.
• Adds shared backend tests for range boundaries and range-delete/put ordering.

Confidence Score: 5/5

The PR appears safe to merge, with no concrete correctness or security failures identified.

The production and in-memory backends implement consistent half-open, call-ordered range deletion, and the slot-prefix bounds delete exactly the signatures below the finalized cutoff while retaining cutoff-slot entries.

Important Files Changed

Filename	Overview
crates/storage/src/api/traits.rs	Adds a clearly documented half-open range-delete API with ordered batch semantics.
crates/storage/src/backend/in_memory.rs	Reworks pending operations into an ordered vector and correctly replays point and range operations in call order.
crates/storage/src/backend/rocksdb.rs	Implements range deletion through the corresponding RocksDB column-family write-batch operation.
crates/storage/src/backend/tests.rs	Adds cross-backend coverage for half-open bounds and a point put following a range deletion.
crates/storage/src/store.rs	Replaces signature-table scanning and point deletion with a correctly bounded range deletion while preserving the finalized retention window.

Sequence Diagram

sequenceDiagram
  participant Import as Block import
  participant Store
  participant Batch as StorageWriteBatch
  participant Backend as RocksDB / In-memory
  Import->>Store: prune_old_data(finalized_slot)
  Store->>Store: "cutoff = tip_slot - pruning range"
  alt cutoff is zero or above finalized slot
    Store-->>Import: 0 (nothing pruned)
  else eligible finalized range
    Store->>Batch: delete_range(BlockSignatures, slot 0, cutoff)
    Batch->>Backend: apply [0, cutoff)
    Backend-->>Batch: range deletion recorded
    Batch->>Backend: commit()
    Store-->>Import: cutoff slot
  end

Loading

_{Reviews (1): Last reviewed commit: "perf(storage): prune old block signature..." | Re-trigger Greptile}

[github-actions]

🤖 Claude Code Review

Review: perf(storage): prune old block signatures with a range delete

The core idea (replace a linear scan + point-deletes with a single RocksDB range tombstone) is sound, and the in-memory backend's move from a per-key HashMap to an ordered Vec<(Table, PendingOp)> correctly preserves call-order semantics so a put_batch after a delete_range on the same key still wins — nicely tested in test_delete_range_then_put.

However, there's a significant issue with how the range delete is invoked on every import.

1. Unconditional delete_range(0, cutoff) on every block import re-tombstones the same range forever

crates/storage/src/store.rs:1093-1119 (prune_old_block_signatures), called from prune_old_data() (store.rs:911-928), which on_block runs after every block import (crates/blockchain/src/lib.rs:975-980).

Once the chain is past SIGNATURE_PRUNING_RANGE slots old (21,600 slots ≈ 1 day at 4s/slot), cutoff is nonzero and cutoff <= finalized_slot holds continuously. On every subsequent block import, the function unconditionally does:

batch.delete_range(Table::BlockSignatures, &slot_root_key_bound(0), &slot_root_key_bound(cutoff))

i.e. it re-issues a range tombstone spanning [0, cutoff) from scratch every single call, not just the newly-eligible slice [prev_cutoff, cutoff). There's no check for whether this range was already tombstoned by the previous call. In steady state this means one new, almost-entirely-redundant range tombstone written to the BlockSignatures column family roughly every slot, forever — millions over the life of a long-running chain.

This directly undercuts the PR's own motivation. The old code's cost scaled with chain height because it read past accumulated point tombstones on every call; this version avoids the read, but now writes an ever-growing number of overlapping range tombstones instead. RocksDB's own guidance warns that many overlapping/duplicate range tombstones can degrade point-lookup and iterator performance until compaction reconciles them (and until then, contributes extra write amplification, partially offsetting the perf win this PR is chasing).

Suggested fix: persist the last-pruned cutoff (e.g. a Metadata entry or an in-memory Store field seeded from it) and either:

• skip the delete_range call entirely when cutoff hasn't advanced since the last prune, or
• narrow the deleted range to [last_pruned_cutoff, cutoff) instead of [0, cutoff).

Either way bounds the number of tombstones written to roughly one per actual advance in the pruning window, matching the "cost tracks the handful of keys leaving the window" goal stated in the PR description.

2. In-memory backend's delete_range reintroduces the height-scaling cost, just on the other backend

crates/storage/src/backend/in_memory.rs:144-146:

PendingOp::DeleteRange(from, to) => {
    table_data.retain(|key, _| key < &from || key >= &to);
}

retain is a full scan of the table on every call — the same "cost tracks chain height, not the keys leaving the window" problem the PR set out to fix, just moved to the in-memory backend. Since in-memory is test-only (per CLAUDE.md) this is low severity, but it also means the new doc comment on the trait method (crates/storage/src/api/traits.rs:44-51, "the write cost need not scale with the number of entries covered") is only true for the RocksDB implementation, not universally as the trait-level doc implies — worth a caveat there, or just noting it's a RocksDB-specific benefit.

Minor

• store.rs:924-926: since prune_old_block_signatures now always returns the (unconditionally-recomputed) cutoff rather than an actual delta, the "Pruned old finalized block signatures" info log fires on essentially every block import once the chain matures, even on calls where nothing new left the window. Not a new regression per se (frequency is similar to before in steady state), but it's now guaranteed rather than incidental — worth keeping in mind if Point 1's fix changes the log's cadence.

What looks good

• slot_root_key_bound and its use as an 8-byte bare-slot bound is correct and well-documented — relies on the same big-endian slot||root ordering already established for LiveChain/BlockSignatures.
• The early-return on cutoff == 0 (avoiding an empty tombstone on a young chain) is correct.
• New backend tests (test_delete_range, test_delete_range_then_put) exercise half-open bounds and same-batch put-after-delete-range ordering against both backends via the shared run_backend_tests harness — good coverage for the new primitive itself.
• Existing pruning tests were updated correctly to match the new return-value semantics.

---

Automated review by Claude (Anthropic) · sonnet · custom prompt