feat: make store begin_write callers return Result and update callers

[d4m014]

🗒️ Description / Motivation

This PR updates all begin_write() callers in crates/storage/src/store.rs to return Result<T, Error>. Other backend methods (begin_read, etc) callers will be updated and come in follow-up PRs.

What Changed

• 8 files touched
• bin/ethlambda/src/main.rs
• crates/blockchain/src/lib.rs
• crates/blockchain/src/reaggregate.rs
• crates/blockchain/src/store.rs
• crates/net/p2p/src/req_resp/handlers.rs
• crates/net/rpc/src/lib.rs
• crates/storage/src/store.rs
• bin/ethlambda/src/checkpoint_sync.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 updates 7 public/private Store methods (insert_signed_block, insert_pending_block, insert_state, update_checkpoints, prune_live_chain, prune_old_states, prune_old_block_signatures) to return Result<T, Error> as a stepping-stone toward full error propagation (tracked in #306). All callers across 6 files are updated to .expect() the new results.

• The updated functions return Result but still use .expect() internally for every fallible backend call (begin_write, put_batch, commit), so the Err path is currently unreachable — any storage failure panics before Ok or Err is returned. This is the intended intermediate state per the PR description.
• set_time and set_safe_target are left with -> () signatures while their callee set_metadata was also changed to -> Result<(), Error>, creating an asymmetry among public write methods that will need to be addressed in a follow-up.
• One test site uses .expect("") (empty message), and fetch_initial_state in main.rs uses .expect() inside a function that already returns and propagates Result.

Confidence Score: 4/5

The changes are mechanical and the runtime behavior is unchanged — storage failures still terminate via panic, as before. Safe to merge as an intermediate refactor step.

The actual runtime behavior is identical to before: all storage write failures cause a panic through the still-present internal .expect() calls. The new Result return types don't yet carry real error information to callers. The asymmetry between set_time/set_safe_target (still -> ()) and the other updated write methods, the empty .expect("") in a test, and the missed propagation in fetch_initial_state are all worth addressing before or alongside the follow-up PRs that complete this refactor.

crates/storage/src/store.rs for the empty expect message and the set_time/set_safe_target asymmetry; bin/ethlambda/src/main.rs for the inconsistent error handling in fetch_initial_state.

Important Files Changed

Filename	Overview
crates/storage/src/store.rs	Core change: 7 public/private methods updated to return Result<T, Error>, but all still use .expect() internally — errors panic rather than propagate. Includes an empty .expect("") message in a test and asymmetric treatment of set_time/set_safe_target.
bin/ethlambda/src/main.rs	Uses .expect() on the new insert_signed_block Result inside fetch_initial_state, which already returns Result and handles other errors via map_err — missed opportunity for consistent error propagation.
crates/blockchain/src/lib.rs	Updated insert_pending_block call to handle new Result return type with .expect(), consistent with the stepping-stone approach of the PR.
crates/blockchain/src/store.rs	Updated six call sites (update_checkpoints, insert_signed_block, insert_state) in production and test code to handle new Result return types with .expect(); changes are mechanical and correct.
crates/blockchain/src/reaggregate.rs	Test-only change: update_checkpoints call in a unit test now handles the Result return type with a descriptive .expect().
crates/net/p2p/src/req_resp/handlers.rs	Test-only change: five insert_signed_block / update_checkpoints call sites updated with .expect() to handle new Result return types.
crates/net/rpc/src/lib.rs	Test-only change: insert_signed_block and update_checkpoints calls updated with .expect() to handle new Result return types.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant Caller as Caller (blockchain, rpc, main)
    participant PubFn as Store public fn (insert_signed_block etc.)
    participant Backend as DB Backend (begin_write / commit)

    Caller->>PubFn: call (now returns Result)
    PubFn->>Backend: begin_write().expect("write batch")
    alt Backend error
        Backend-->>PubFn: Err — .expect() panics
        PubFn--xCaller: (never reached)
    else Success
        Backend-->>PubFn: Ok(batch)
        PubFn->>Backend: batch.commit().expect("commit")
        Backend-->>PubFn: Ok(())
        PubFn-->>Caller: Ok(())
        Caller->>Caller: .expect("...") on Ok — no-op
    end

    Note over PubFn,Backend: Err variant currently unreachable. Follow-up PRs will replace .expect() with ? to enable true propagation.

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 Caller as Caller (blockchain, rpc, main)
    participant PubFn as Store public fn (insert_signed_block etc.)
    participant Backend as DB Backend (begin_write / commit)

    Caller->>PubFn: call (now returns Result)
    PubFn->>Backend: begin_write().expect("write batch")
    alt Backend error
        Backend-->>PubFn: Err — .expect() panics
        PubFn--xCaller: (never reached)
    else Success
        Backend-->>PubFn: Ok(batch)
        PubFn->>Backend: batch.commit().expect("commit")
        Backend-->>PubFn: Ok(())
        PubFn-->>Caller: Ok(())
        Caller->>Caller: .expect("...") on Ok — no-op
    end

    Note over PubFn,Backend: Err variant currently unreachable. Follow-up PRs will replace .expect() with ? to enable true propagation.

Loading

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:1779-1781
Empty `.expect` message — when this panics in a test, the output gives no hint about what failed. All other `.expect` calls in this PR include a descriptive string.

```suggestion
        store
            .update_checkpoints(ForkCheckpoints::head_only(head_root))
            .expect("update_checkpoints should succeed");
```

### Issue 2 of 3
crates/storage/src/store.rs:708-731
**`set_time` / `set_safe_target` left with void return types**

`set_metadata` is updated to `-> Result<(), Error>` in this PR, which means `set_time` and `set_safe_target` now absorb that `Result` with `.expect()` while keeping a `-> ()` signature — yet sibling methods that also call `begin_write` (`insert_signed_block`, `update_checkpoints`, `insert_state`) are updated to return `Result`. This asymmetry in the public `Store` API means callers of `set_time`/`set_safe_target` have no way to observe or propagate storage failures, while callers of the other methods do. If the intent is to update all public write methods uniformly, `set_time` and `set_safe_target` should also be updated to `-> Result<(), Error>` in this PR.

### Issue 3 of 3
bin/ethlambda/src/main.rs:739-742
**Missed error propagation in a function that already returns `Result`**

`fetch_initial_state` already returns `Result<Store, CheckpointSyncError>` and properly propagates the `get_forkchoice_store` error via `map_err`. The new `insert_signed_block` call, however, uses `.expect()` instead of mapping the storage error into `CheckpointSyncError` and returning it. A storage write failure during checkpoint sync will now panic the task rather than surfacing a recoverable `Err` to the caller, which is inconsistent with the surrounding error handling style.

_{Reviews (1): Last reviewed commit: "feat: make store begin_write callers ret..." | Re-trigger Greptile}

[MegaRedHand]

LGTM