feat: make store begin_read() callers return Result

[d4m014]

🗒️ Description / Motivation

This PR updates all begin_read() callers in crates/storage/src/store.rs to return Result<T, Error> and updates store function callers across files.

What Changed

• 16 files touched
• bin/ethlambda/src/main.rs
• crates/blockchain/src/coverage.rs
• crates/blockchain/src/lib.rs
• crates/blockchain/src/reaggregate.rs
• crates/blockchain/src/store.rs
• crates/blockchain/tests/forkchoice_spectests.rs
• crates/net/p2p/src/req_resp/handlers.rs
• crates/net/rpc/src/base.rs
• crates/net/rpc/src/blocks.rs
• crates/net/rpc/src/fork_choice.rs
• crates/net/rpc/src/lib.rs
• crates/net/rpc/src/test_driver.rs
• crates/net/rpc/tests/test_driver_e2e.rs
• crates/storage/src/error.rs
• crates/storage/src/store.rs
• crates/net/rpc/src/genesis.rs

Related Issues / PRs

• Part of Store swallows DB errors with .expect(), propagate Result through storage layer #306

✅ 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 migrates Store metadata accessors (head, time, config, latest_justified, latest_finalized, safe_target) and several read methods (get_block_header, get_block, get_signed_block, get_state, get_live_chain, etc.) from infallible returns to Result<T, Error>, with callers across 16 files updated accordingly. Three new error variants (MissingBlockHeader, MissingBlockBody, MissingState) are added to storage::Error.

• store.get_block() and Store::from_db_state() have correctness regressions: both replace ?-on-Option early-returns with .unwrap(), converting "not found → None" into a panic rather than propagating the missing-data signal.
• get_fork_choice in fork_choice.rs introduces a panic path: .map(|h| h.unwrap().proposer_index) is applied to a Result<Option<BlockHeader>>, causing a crash when get_block_header returns Ok(None).
• Several sites in blockchain/src/store.rs and reaggregate.rs use bare .unwrap() without a message, inconsistent with the expect(\"description\") convention used elsewhere in the same PR.

Confidence Score: 3/5

Three concrete panic paths were introduced that were not present before: from_db_state on an empty backend, get_block when a header is absent, and the fork-choice endpoint when a live-chain block header is momentarily missing.

The Result migration is correct in most places and the RPC layer handles the new types cleanly. However, from_db_state now panics where it previously returned None, get_block silently lost its Ok(None) path, and the fork-choice endpoint will crash on Ok(None) from get_block_header.

crates/storage/src/store.rs (from_db_state and get_block) and crates/net/rpc/src/fork_choice.rs need the most attention before merge.

Important Files Changed

Filename	Overview
crates/storage/src/store.rs	Core store methods converted to return Result, but get_block and from_db_state have correctness regressions: get_block panics instead of returning Ok(None), and from_db_state panics on empty backends.
crates/net/rpc/src/fork_choice.rs	get_fork_choice endpoint will panic when get_block_header returns Ok(None) due to .map(
crates/storage/src/error.rs	Adds MissingBlockHeader, MissingBlockBody, MissingState error variants to the Error enum; straightforward addition.
crates/blockchain/src/store.rs	All begin_read callers updated to propagate Result; heavy use of bare .unwrap() without messages in on_tick and update_safe_target is inconsistent with the expect() convention used elsewhere.
crates/blockchain/src/lib.rs	BlockChainServer updated to unwrap store Results; double .expect().unwrap() pattern for get_signed_block is verbose but correct.
crates/net/rpc/src/base.rs	RPC handlers correctly map Ok(None) and Err to appropriate HTTP status codes (404/500); clean update.
crates/net/rpc/src/blocks.rs	get_block and get_block_header handlers properly pattern-match Ok(Some), Ok(None), and Err to 200/404/500 responses.
crates/net/p2p/src/req_resp/handlers.rs	P2P handlers use ok().flatten() for signed_block lookups and Ok(Some)/break pattern for block headers — correct handling of new Result types.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["Store::get_metadata(key)"] --> B["Result<T, Error>"]
    B --> C1["store.time()"]
    B --> C2["store.config()"]
    B --> C3["store.head()"]
    B --> C4["store.safe_target()"]
    B --> C5["store.latest_justified()"]
    B --> C6["store.latest_finalized()"]
    D["store.get_block_header(root)"] --> E["Result<Option<BlockHeader>, Error>"]
    F["store.get_block(root)"] --> G["Result<Option<Block>, Error> — always Ok(Some) or panic"]
    H["Store::from_db_state(backend)"] --> K["Result<Option<Store>, Error> — panics on empty backend"]
    C3 --> |".expect()"| N["Callers (blockchain, rpc, p2p)"]
    E --> |"Ok(Some)/Ok(None)/Err"| O["RPC: 200/404/500"]
    E --> |"Ok(None) unwrap PANIC"| P["fork_choice.rs .map(|h| h.unwrap())"]

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"}}}%%
flowchart TD
    A["Store::get_metadata(key)"] --> B["Result<T, Error>"]
    B --> C1["store.time()"]
    B --> C2["store.config()"]
    B --> C3["store.head()"]
    B --> C4["store.safe_target()"]
    B --> C5["store.latest_justified()"]
    B --> C6["store.latest_finalized()"]
    D["store.get_block_header(root)"] --> E["Result<Option<BlockHeader>, Error>"]
    F["store.get_block(root)"] --> G["Result<Option<Block>, Error> — always Ok(Some) or panic"]
    H["Store::from_db_state(backend)"] --> K["Result<Option<Store>, Error> — panics on empty backend"]
    C3 --> |".expect()"| N["Callers (blockchain, rpc, p2p)"]
    E --> |"Ok(Some)/Ok(None)/Err"| O["RPC: 200/404/500"]
    E --> |"Ok(None) unwrap PANIC"| P["fork_choice.rs .map(|h| h.unwrap())"]

Loading

Prompt To Fix All With AI

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

---

### Issue 1 of 4
crates/storage/src/store.rs:602-609
**`from_db_state` panics on empty backend instead of returning `Ok(None)`**

The original code used `?` on `Option` to early-return `None` when `KEY_CONFIG` or `KEY_LATEST_FINALIZED` are absent (the "empty DB" case). The replacement `.unwrap()` panics instead. As a result, any call with a fresh `InMemoryBackend` — including the `from_db_state_returns_none_on_empty_backend` test — will panic rather than returning `Ok(None)`. The `None`-missing-key path must use an explicit `else { return Ok(None); }` guard or `ok_or`-then-`?` instead of `.unwrap()`.

### Issue 2 of 4
crates/storage/src/store.rs:1104-1118
**`get_block` can no longer return `Ok(None)` — panics when a block is missing**

The function signature advertises `Result<Option<Block>>`, implying callers can distinguish "DB error" from "block not found". However, both `.unwrap()` calls (lines 1108 and 1114) panic if the header or body is absent; the function only ever returns `Ok(Some(...))`. Any root that isn't in `BlockHeaders` will cause a panic rather than returning `Ok(None)`. The `if let Ok(Some(block)) = store.get_block(...)` guard in `coverage.rs` will never catch a missing block — it will crash instead. The fix is to use `.ok()` / `ok_or` to propagate absent keys as `Ok(None)` or a typed error, matching the original `?` semantics.

### Issue 3 of 4
crates/net/rpc/src/fork_choice.rs:59-62
**Panic on `Ok(None)` from `get_block_header`**

`store.get_block_header(root)` now returns `Result<Option<BlockHeader>, Error>`. The `.map(|h| h.unwrap().proposer_index)` closure receives the inner `Option<BlockHeader>` and calls `.unwrap()` on it, which panics when the header is `Ok(None)`. Any live-chain block whose header is momentarily absent will crash the fork-choice endpoint. Use `.ok().flatten()` to safely collapse the two layers before extracting the field.

```suggestion
            let proposer_index = store
                .get_block_header(root)
                .ok()
                .flatten()
                .map(|h| h.proposer_index)
                .unwrap_or(0);
```

### Issue 4 of 4
crates/blockchain/src/reaggregate.rs:242
Bare `.unwrap()` without a message makes panics in this path impossible to triage. Every other `Result`-unwrap in this PR uses `.expect("description")`. The pattern also appears in `on_tick` in `store.rs` (several `store.time().unwrap()` and `store.config().unwrap()` calls).

```suggestion
    let justified_slot = store.latest_justified().expect("latest justified checkpoint exists").slot;
```

_{Reviews (1): Last reviewed commit: "fix errors" | Re-trigger Greptile}

[MegaRedHand]

Left some comments

mis-click