feat: add block root slot index

[dicethedev]

🗒️ Description / Motivation

• Adds a canonical slot-to-block-root index to storage.
• BlocksByRange currently has to walk backward from the head and collect roots by slot.
• This PR makes range lookups efficient by reading canonical block roots directly by slot.

What Changed

• Added a new BlockRoots storage table:
  • key: slot / block number
  • value: canonical block root
• Registers the new table for in-memory and RocksDB backends.
• Initializes the index for anchor/checkpoint stores.
• Maintains the index when fork-choice head changes.
• Handles reorgs by deleting old canonical slot entries and inserting the new canonical branch.
• Rebuilds the index on DB restore if the persisted index is missing.
• Cleans index entries when old block data is pruned.
• Updates BlocksByRange handling to use Store::get_block_root_by_slot instead of walking parent headers.

Correctness / Behavior Guarantees

• The index points only to canonical block roots.
• Side-fork blocks do not appear in BlocksByRange responses.
• Skipped slots remain absent from the index and are skipped in responses.
• Reorgs update the index to match the new canonical chain.
• Existing persisted DBs can rebuild the index from the current head chain.
• Pruned blocks have their canonical index entries removed when applicable.

Tests Added / Run

• Added storage tests for:
  • canonical slot index updates
  • skipped slots
  • side forks
  • reorgs
  • DB restore index rebuild
  • pruning cleanup
• Existing BlocksByRange test now exercises indexed lookup.

Related Issues / PRs

• Closes Add block number to block root index #355
• Related to feat(p2p): add inbound BlocksByRange req/resp support #348
• Related to feat(p2p): use BlocksByRange for long-range sync #351

✅ Verification Checklist

• Ran make fmt — clean
• Ran make lint (clippy with -D warnings) — clean
• Ran cargo test --workspace --release — all passing

[greptile-apps]

Greptile Summary

This PR introduces a canonical slot-to-block-root index (BlockRoots table) to make BlocksByRange lookups O(count) instead of walking the full chain backward from HEAD. The index is maintained on every update_checkpoints call via an LCA-walk algorithm, rebuilt on DB restore if missing, and cleaned up during block pruning.

• store.rs: Adds encode_block_root_key, block_root_index_changes (LCA diff between old/new head), rebuild_block_root_index, and get_block_root_by_slot; wires them into init_store, update_checkpoints, from_db_state, and prune_old_blocks.
• handlers.rs: Replaces the backward head-walk in canonical_blocks_by_range with a direct per-slot index read.
• tables.rs / rocksdb.rs: Register the new BlockRoots table in both the in-memory and RocksDB backends.

Confidence Score: 3/5

The index maintenance logic and handler change are correct, but from_db_state now silently assumes KEY_HEAD is present and will panic rather than return None on a DB missing that key.

The LCA walk in block_root_index_changes is algorithmically sound and the tests cover reorgs, skipped slots, and restore paths well. However, from_db_state now calls store.head_slot() — which panics via get_metadata if KEY_HEAD is absent — without first probing for the key the way it already probes KEY_LATEST_FINALIZED. Any DB that passes the initial key checks but is missing KEY_HEAD will crash the process instead of failing gracefully. Additionally, rebuild_block_root_index takes &self while writing to the backend, deviating from the &mut self convention used everywhere else in the Store API.

crates/storage/src/store.rs — specifically the from_db_state restore guard and block_root_index_changes error handling

Important Files Changed

Filename	Overview
crates/storage/src/store.rs	Core of the PR — adds BlockRoots index with init, LCA-walk update-on-head-change, rebuild-on-restore, and prune cleanup. The from_db_state guard can panic if KEY_HEAD is missing; block_root_index_changes uses expect() on header lookups; rebuild_block_root_index is a mutating method with a &self signature.
crates/net/p2p/src/req_resp/handlers.rs	Replaces the backward head-walk in canonical_blocks_by_range with a direct indexed slot lookup using get_block_root_by_slot; removes the HashMap and related imports. Logic is correct and the early-exit guard is preserved.
crates/storage/src/api/tables.rs	Adds BlockRoots variant to the Table enum and ALL_TABLES constant; name string is consistent with other tables.
crates/storage/src/backend/rocksdb.rs	Registers the new block_roots column family in the RocksDB backend; change is minimal and correct.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant FC as Fork Choice
    participant Store
    participant BlockRoots as BlockRoots Table
    participant BlockHeaders as BlockHeaders Table

    Note over FC,BlockHeaders: Head update (normal extension)
    FC->>Store: update_checkpoints(new_head)
    Store->>Store: block_root_index_changes(old_head, new_head)
    Store->>BlockHeaders: get_block_header(new_head) [walk back to LCA]
    BlockHeaders-->>Store: headers
    Store->>BlockRoots: delete_batch(old canonical slots)
    Store->>BlockRoots: put_batch(new canonical slots)
    Store-->>FC: committed

    Note over FC,BlockHeaders: DB restore
    FC->>Store: from_db_state(backend)
    Store->>BlockRoots: get(head_slot) check index
    alt index stale or missing
        Store->>Store: rebuild_block_root_index()
        Store->>BlockHeaders: walk from head to anchor
        Store->>BlockRoots: delete_batch(old) + put_batch(rebuilt)
    end
    Store-->>FC: Some(store)

    Note over FC,BlockHeaders: BlocksByRange P2P request
    FC->>Store: get_block_root_by_slot(slot) x count
    Store->>BlockRoots: get(slot_key)
    BlockRoots-->>Store: block_root
    Store-->>FC: signed blocks

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant FC as Fork Choice
    participant Store
    participant BlockRoots as BlockRoots Table
    participant BlockHeaders as BlockHeaders Table

    Note over FC,BlockHeaders: Head update (normal extension)
    FC->>Store: update_checkpoints(new_head)
    Store->>Store: block_root_index_changes(old_head, new_head)
    Store->>BlockHeaders: get_block_header(new_head) [walk back to LCA]
    BlockHeaders-->>Store: headers
    Store->>BlockRoots: delete_batch(old canonical slots)
    Store->>BlockRoots: put_batch(new canonical slots)
    Store-->>FC: committed

    Note over FC,BlockHeaders: DB restore
    FC->>Store: from_db_state(backend)
    Store->>BlockRoots: get(head_slot) check index
    alt index stale or missing
        Store->>Store: rebuild_block_root_index()
        Store->>BlockHeaders: walk from head to anchor
        Store->>BlockRoots: delete_batch(old) + put_batch(rebuilt)
    end
    Store-->>FC: Some(store)

    Note over FC,BlockHeaders: BlocksByRange P2P request
    FC->>Store: get_block_root_by_slot(slot) x count
    Store->>BlockRoots: get(slot_key)
    BlockRoots-->>Store: block_root
    Store-->>FC: signed blocks

Loading

Comments Outside Diff (3)

1. crates/storage/src/store.rs, line 577-579 (link)

   from_db_state can panic if KEY_HEAD is absent

   from_db_state is designed to return None gracefully when the DB is empty or incompatible — it probes KEY_CONFIG and KEY_LATEST_FINALIZED with ? to do so. But it now calls store.head_slot() → store.head() → get_metadata(KEY_HEAD), and get_metadata uses an inner expect("metadata key exists") that panics on a missing key. If KEY_HEAD is absent (corrupted write, partial migration, or a hand-crafted fixture like the one fixed in the test at line 1905), the process panics instead of returning None. The probe for KEY_HEAD should be added alongside the existing probe for KEY_LATEST_FINALIZED.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: crates/storage/src/store.rs
   Line: 577-579
   
   Comment:
   **`from_db_state` can panic if `KEY_HEAD` is absent**
   
   `from_db_state` is designed to return `None` gracefully when the DB is empty or incompatible — it probes `KEY_CONFIG` and `KEY_LATEST_FINALIZED` with `?` to do so. But it now calls `store.head_slot()` → `store.head()` → `get_metadata(KEY_HEAD)`, and `get_metadata` uses an inner `expect("metadata key exists")` that panics on a missing key. If `KEY_HEAD` is absent (corrupted write, partial migration, or a hand-crafted fixture like the one fixed in the test at line 1905), the process panics instead of returning `None`. The probe for `KEY_HEAD` should be added alongside the existing probe for `KEY_LATEST_FINALIZED`.
   
   How can I resolve this? If you propose a fix, please make it concise.

2. crates/storage/src/store.rs, line 849-892 (link)

   block_root_index_changes panics on missing headers during a reorg

   Both expect("old canonical block header exists") and expect("new canonical block header exists") will panic if a header cannot be fetched. While BLOCKS_TO_KEEP = 21_600 makes a reorg crossing the prune boundary essentially impossible on mainnet, the same function is exercised during DB restore rebuilds. A more defensive approach would be to break out of the loop on a missing header (similar to the let Some(header) = ... else { break } pattern used in rebuild_block_root_index), leaving any deeper chain entries unchanged rather than panicking.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: crates/storage/src/store.rs
   Line: 849-892
   
   Comment:
   **`block_root_index_changes` panics on missing headers during a reorg**
   
   Both `expect("old canonical block header exists")` and `expect("new canonical block header exists")` will panic if a header cannot be fetched. While `BLOCKS_TO_KEEP = 21_600` makes a reorg crossing the prune boundary essentially impossible on mainnet, the same function is exercised during DB restore rebuilds. A more defensive approach would be to break out of the loop on a missing header (similar to the `let Some(header) = ... else { break }` pattern used in `rebuild_block_root_index`), leaving any deeper chain entries unchanged rather than panicking.
   
   How can I resolve this? If you propose a fix, please make it concise.

3. crates/storage/src/store.rs, line 895-923 (link)

   rebuild_block_root_index takes &self but performs a write

   Every other mutating method on Store (update_checkpoints, prune_old_blocks, insert_signed_block, etc.) takes &mut self. rebuild_block_root_index writes to the backend through the inner Arc while only holding &self, which is inconsistent and sidesteps Rust's normal exclusivity guarantee. This is safe in the narrow call site in from_db_state (no other holders exist yet), but makes the method's signature silently lie about its mutation semantics. Changing it to &mut self would be consistent with the rest of the API.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: crates/storage/src/store.rs
   Line: 895-923
   
   Comment:
   **`rebuild_block_root_index` takes `&self` but performs a write**
   
   Every other mutating method on `Store` (`update_checkpoints`, `prune_old_blocks`, `insert_signed_block`, etc.) takes `&mut self`. `rebuild_block_root_index` writes to the backend through the inner `Arc` while only holding `&self`, which is inconsistent and sidesteps Rust's normal exclusivity guarantee. This is safe in the narrow call site in `from_db_state` (no other holders exist yet), but makes the method's signature silently lie about its mutation semantics. Changing it to `&mut self` would be consistent with the rest of the API.
   
   How can I resolve this? If you propose a fix, please make it concise.

   Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Prompt To Fix All With AI

Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 3
crates/storage/src/store.rs:577-579
**`from_db_state` can panic if `KEY_HEAD` is absent**

`from_db_state` is designed to return `None` gracefully when the DB is empty or incompatible — it probes `KEY_CONFIG` and `KEY_LATEST_FINALIZED` with `?` to do so. But it now calls `store.head_slot()` → `store.head()` → `get_metadata(KEY_HEAD)`, and `get_metadata` uses an inner `expect("metadata key exists")` that panics on a missing key. If `KEY_HEAD` is absent (corrupted write, partial migration, or a hand-crafted fixture like the one fixed in the test at line 1905), the process panics instead of returning `None`. The probe for `KEY_HEAD` should be added alongside the existing probe for `KEY_LATEST_FINALIZED`.

### Issue 2 of 3
crates/storage/src/store.rs:849-892
**`block_root_index_changes` panics on missing headers during a reorg**

Both `expect("old canonical block header exists")` and `expect("new canonical block header exists")` will panic if a header cannot be fetched. While `BLOCKS_TO_KEEP = 21_600` makes a reorg crossing the prune boundary essentially impossible on mainnet, the same function is exercised during DB restore rebuilds. A more defensive approach would be to break out of the loop on a missing header (similar to the `let Some(header) = ... else { break }` pattern used in `rebuild_block_root_index`), leaving any deeper chain entries unchanged rather than panicking.

### Issue 3 of 3
crates/storage/src/store.rs:895-923
**`rebuild_block_root_index` takes `&self` but performs a write**

Every other mutating method on `Store` (`update_checkpoints`, `prune_old_blocks`, `insert_signed_block`, etc.) takes `&mut self`. `rebuild_block_root_index` writes to the backend through the inner `Arc` while only holding `&self`, which is inconsistent and sidesteps Rust's normal exclusivity guarantee. This is safe in the narrow call site in `from_db_state` (no other holders exist yet), but makes the method's signature silently lie about its mutation semantics. Changing it to `&mut self` would be consistent with the rest of the API.

_{Reviews (1): Last reviewed commit: "Merge branch 'main' into feat/block-root..." | Re-trigger Greptile}

[pablodeymo]

can you solve this conflicts?

[dicethedev]

can you solve this conflicts?

You can review again

[MegaRedHand]

Left some comments