Skip to content

refactor: replace generic db with enum#7051

Merged
hanabi1224 merged 10 commits into
mainfrom
hm/db-enum-final
May 14, 2026
Merged

refactor: replace generic db with enum#7051
hanabi1224 merged 10 commits into
mainfrom
hm/db-enum-final

Conversation

@hanabi1224
Copy link
Copy Markdown
Contributor

@hanabi1224 hanabi1224 commented May 13, 2026

Summary of changes

(On top of #7044)

Changes introduced in this pull request:

  • replace generic db with enum in ChainIndex, ChainStore and StateManager

Reference issue to close (if applicable)

Closes

Other information and links

Change checklist

  • I have performed a self-review of my own code,
  • I have made corresponding changes to the documentation. All new code adheres to the team's documentation standards,
  • I have added tests that prove my fix is effective or that my feature works (if possible),
  • I have made sure the CHANGELOG is up-to-date. All user-facing changes should be reflected in this document.

Outside contributions

  • I have read and agree to the CONTRIBUTING document.
  • I have read and agree to the AI Policy document. I understand that failure to comply with the guidelines will lead to rejection of the pull request.

Summary by CodeRabbit

  • Refactor

    • Unified database handling into a single concrete DB abstraction and simplified constructors across the system.
    • De-genericized core services, RPCs, and subsystems to a consistent, non-generic API surface.
    • Added lightweight shallow-clone semantics for cheaper cloning of shared components.
  • Style

    • Introduced a crate prelude to centralize common imports and reduce boilerplate.

Review Change Stack

@hanabi1224 hanabi1224 added the RPC requires calibnet RPC checks to run on CI label May 13, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 13, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 0b435cc0-1913-4e1f-8e9d-d762b81b8690

📥 Commits

Reviewing files that changed from the base of the PR and between 449f4c5 and 46b7671.

📒 Files selected for processing (2)
  • src/rpc/methods/eth.rs
  • src/rpc/mod.rs
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/rpc/mod.rs
  • src/rpc/methods/eth.rs

I can’t reliably rebuild and emit a hidden review stack that includes every rangeId exactly once for this very large PR in a single response. Please either:

  • Split the PR (or the rangeId list) into smaller cohorts (recommended), or
  • Ask me to produce the hidden artifact in multiple sequential parts (e.g., “part 1: db/* + chain/store/”, then “part 2: state_manager/ + rpc/*”, etc.), and I will generate each part with correct, non-duplicated range assignments.

Which option do you prefer?

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch hm/db-enum-final
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch hm/db-enum-final

@hanabi1224 hanabi1224 marked this pull request as ready for review May 13, 2026 23:47
@hanabi1224 hanabi1224 requested a review from a team as a code owner May 13, 2026 23:47
@hanabi1224 hanabi1224 requested review from LesnyRumcajs and sudo-shashank and removed request for a team May 13, 2026 23:47
Comment thread src/daemon/context.rs
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
src/rpc/methods/net.rs (1)

286-295: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Avoid a global cache for net_version.

Like eth_chainId, this string is derived from ctx.chain_config(), but OnceLock makes it process-global. If more than one RPC context with different chain configs exists in the same process, this can return the wrong network version.

Suggested fix
-        // `eth_chain_id` is fixed for the process lifetime; cache the decimal form.
-        static CACHED: OnceLock<Arc<str>> = OnceLock::new();
-        Ok(CACHED
-            .get_or_init(|| Arc::<str>::from(ctx.chain_config().eth_chain_id.to_string()))
-            .clone())
+        Ok(Arc::<str>::from(
+            ctx.chain_config().eth_chain_id.to_string(),
+        ))
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/rpc/methods/net.rs` around lines 286 - 295, The current handler uses a
process-global static CACHED OnceLock which can return the wrong net_version
when multiple RPC contexts with different chain configs exist; remove the static
OnceLock (CACHED) and instead derive the string from ctx per-invocation (e.g.,
create and return an Arc<str> from ctx.chain_config().eth_chain_id.to_string()),
or use a cache tied to the Ctx instance if one exists, ensuring the cached value
is specific to ctx rather than process-global; update the async fn handle
implementation and remove references to CACHED accordingly.
src/rpc/methods/eth.rs (1)

856-865: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Don't cache eth_chainId in a process-global OnceLock.

This value comes from ctx.chain_config(), but the cache is shared across every RPC context in the process. The first context to initialize it wins, so later contexts can return the wrong chain ID.

Suggested fix
-        // `eth_chain_id` is fixed for the process lifetime; cache the hex form.
-        static CACHED: OnceLock<Arc<str>> = OnceLock::new();
-        Ok(CACHED
-            .get_or_init(|| Arc::<str>::from(format!("{:`#x`}", ctx.chain_config().eth_chain_id)))
-            .clone())
+        Ok(Arc::<str>::from(format!(
+            "{:`#x`}",
+            ctx.chain_config().eth_chain_id
+        )))
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/rpc/methods/eth.rs` around lines 856 - 865, The handler currently uses a
process-global static CACHED OnceLock (symbol CACHED) to store the hex chain id,
which can return the wrong value for different RPC contexts; remove the static
OnceLock usage in async fn handle and instead compute the hex string from
ctx.chain_config().eth_chain_id on each call (or read/return a cached value from
the per-request context if Ctx provides per-context storage), and return that
per-call Arc<str>/String so the chain id is correct for the current ctx; update
references in handle (and remove CACHED) accordingly.
🧹 Nitpick comments (1)
src/fil_cns/weight.rs (1)

22-27: 🏗️ Heavy lift

Use anyhow::Result and preserve error context in weight.

This helper still returns Result<BigInt, String> and converts upstream errors with to_string(), which drops context. Prefer anyhow::Result<BigInt> and .context(...) on fallible calls.

As per coding guidelines "Use anyhow::Result<T> for most operations and add context with .context() when errors occur".

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/fil_cns/weight.rs` around lines 22 - 27, Change the signature of weight
to return anyhow::Result<BigInt> and stop converting errors to String; instead
wrap fallible calls like StateTree::new_from_tipset(db, ts) and
state_tree.get_actor_state() with .context(...) to preserve context (e.g.,
".context(\"creating StateTree from tipset\")" and ".context(\"getting power
actor state\")"), and return the propagated anyhow errors from the function.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/db/parity_db.rs`:
- Around line 394-401: subscribe_write_ops currently has a race where two
concurrent first-time callers can both create a channel and one will overwrite
the other; fix it by using a double-checked upgrade pattern: read-lock
self.write_ops_broadcast_tx and if Some -> return tx.subscribe(); otherwise drop
the read lock, acquire a write lock on self.write_ops_broadcast_tx, check again
if Some (another task may have created it) and if so return tx.subscribe(); only
if still None create the tokio::sync::broadcast::channel(65536), store the
sender into *self.write_ops_broadcast_tx.write() and then return tx.subscribe();
ensure you reference the same field name self.write_ops_broadcast_tx and call
tx.subscribe() for the receiver.

In `@src/rpc/methods/state.rs`:
- Around line 1108-1111: The call to get_vm_circulating_supply_detailed is
passing Arc::new(ctx.db()), producing an Arc<&DbImpl> which is incorrect and
inconsistent with other call sites; replace the Arc construction with a direct
reference to the concrete DB handle (use ctx.db() directly or &ctx.db_owned() to
match other sites) so
genesis_info.get_vm_circulating_supply_detailed(ts.epoch(), &ctx.db(),
ts.parent_state()) (or &ctx.db_owned()) is used instead of Arc::new(...),
removing the unnecessary Arc allocation and ensuring the function receives the
expected DB reference.

In `@src/tool/subcommands/api_cmd/generate_test_snapshot.rs`:
- Line 121: The code calls ChainStore::new(...).unwrap() when creating
chain_store which panics on error; replace the unwrap with the try operator so
the initialization error is propagated (e.g., let chain_store =
Arc::new(ChainStore::new(db.clone(), chain_config, genesis_header)?);) and
update the enclosing function signature to return a Result (or convert the error
via map_err to the function's error type) so callers can handle failures instead
of aborting; reference ChainStore::new, chain_store, db, chain_config, and
genesis_header when making these changes.

---

Outside diff comments:
In `@src/rpc/methods/eth.rs`:
- Around line 856-865: The handler currently uses a process-global static CACHED
OnceLock (symbol CACHED) to store the hex chain id, which can return the wrong
value for different RPC contexts; remove the static OnceLock usage in async fn
handle and instead compute the hex string from ctx.chain_config().eth_chain_id
on each call (or read/return a cached value from the per-request context if Ctx
provides per-context storage), and return that per-call Arc<str>/String so the
chain id is correct for the current ctx; update references in handle (and remove
CACHED) accordingly.

In `@src/rpc/methods/net.rs`:
- Around line 286-295: The current handler uses a process-global static CACHED
OnceLock which can return the wrong net_version when multiple RPC contexts with
different chain configs exist; remove the static OnceLock (CACHED) and instead
derive the string from ctx per-invocation (e.g., create and return an Arc<str>
from ctx.chain_config().eth_chain_id.to_string()), or use a cache tied to the
Ctx instance if one exists, ensuring the cached value is specific to ctx rather
than process-global; update the async fn handle implementation and remove
references to CACHED accordingly.

---

Nitpick comments:
In `@src/fil_cns/weight.rs`:
- Around line 22-27: Change the signature of weight to return
anyhow::Result<BigInt> and stop converting errors to String; instead wrap
fallible calls like StateTree::new_from_tipset(db, ts) and
state_tree.get_actor_state() with .context(...) to preserve context (e.g.,
".context(\"creating StateTree from tipset\")" and ".context(\"getting power
actor state\")"), and return the propagated anyhow errors from the function.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 47b67245-3c34-4281-92de-17551b58ec01

📥 Commits

Reviewing files that changed from the base of the PR and between ff926a4 and 698f691.

📒 Files selected for processing (103)
  • src/blocks/chain4u.rs
  • src/chain/mod.rs
  • src/chain/store/chain_store.rs
  • src/chain/store/index.rs
  • src/chain/store/tipset_tracker.rs
  • src/chain/tests.rs
  • src/chain_sync/chain_follower.rs
  • src/chain_sync/network_context.rs
  • src/chain_sync/sync_status.rs
  • src/chain_sync/tipset_syncer.rs
  • src/chain_sync/validation.rs
  • src/daemon/context.rs
  • src/daemon/db_util.rs
  • src/daemon/mod.rs
  • src/db/car/many.rs
  • src/db/db_impl.rs
  • src/db/either.rs
  • src/db/gc/snapshot.rs
  • src/db/mod.rs
  • src/db/parity_db.rs
  • src/db/parity_db/gc.rs
  • src/db/ttl/mod.rs
  • src/dev/subcommands/export_state_tree_cmd.rs
  • src/dev/subcommands/export_tipset_lookup_cmd.rs
  • src/dev/subcommands/state_cmd.rs
  • src/fil_cns/mod.rs
  • src/fil_cns/validation.rs
  • src/fil_cns/weight.rs
  • src/interpreter/fvm2.rs
  • src/interpreter/fvm3.rs
  • src/interpreter/fvm4.rs
  • src/interpreter/vm.rs
  • src/libp2p/chain_exchange/provider.rs
  • src/libp2p/service.rs
  • src/libp2p_bitswap/request_manager.rs
  • src/libp2p_bitswap/store.rs
  • src/message_pool/msgpool/msg_pool.rs
  • src/message_pool/msgpool/provider.rs
  • src/rpc/methods/auth.rs
  • src/rpc/methods/beacon.rs
  • src/rpc/methods/chain.rs
  • src/rpc/methods/common.rs
  • src/rpc/methods/eth.rs
  • src/rpc/methods/eth/filter/mod.rs
  • src/rpc/methods/eth/pubsub.rs
  • src/rpc/methods/eth/tipset_resolver.rs
  • src/rpc/methods/eth/trace/mod.rs
  • src/rpc/methods/eth/trace/parity.rs
  • src/rpc/methods/eth/trace/state_diff.rs
  • src/rpc/methods/eth/trace/test_helpers.rs
  • src/rpc/methods/f3.rs
  • src/rpc/methods/gas.rs
  • src/rpc/methods/market.rs
  • src/rpc/methods/miner.rs
  • src/rpc/methods/misc.rs
  • src/rpc/methods/mpool.rs
  • src/rpc/methods/msig.rs
  • src/rpc/methods/net.rs
  • src/rpc/methods/node.rs
  • src/rpc/methods/state.rs
  • src/rpc/methods/sync.rs
  • src/rpc/methods/wallet.rs
  • src/rpc/mod.rs
  • src/rpc/reflect/mod.rs
  • src/shim/state_tree.rs
  • src/state_manager/actor_queries.rs
  • src/state_manager/address_resolution.rs
  • src/state_manager/chain_rand.rs
  • src/state_manager/circulating_supply.rs
  • src/state_manager/execution.rs
  • src/state_manager/message_search.rs
  • src/state_manager/message_simulation.rs
  • src/state_manager/mining.rs
  • src/state_manager/mod.rs
  • src/state_manager/state_computation.rs
  • src/state_manager/utils.rs
  • src/state_migration/mod.rs
  • src/state_migration/nv17/migration.rs
  • src/state_migration/nv17/miner.rs
  • src/state_migration/nv17/verifreg_market.rs
  • src/state_migration/nv18/eam.rs
  • src/state_migration/nv18/migration.rs
  • src/state_migration/nv19/migration.rs
  • src/state_migration/nv21/migration.rs
  • src/state_migration/nv21/miner.rs
  • src/state_migration/nv21fix/migration.rs
  • src/state_migration/nv21fix2/migration.rs
  • src/state_migration/nv22/migration.rs
  • src/state_migration/nv22fix/migration.rs
  • src/state_migration/nv23/migration.rs
  • src/state_migration/nv24/migration.rs
  • src/state_migration/nv25/migration.rs
  • src/state_migration/nv26fix/migration.rs
  • src/state_migration/nv27/migration.rs
  • src/state_migration/nv28/migration.rs
  • src/statediff/mod.rs
  • src/tool/offline_server/server.rs
  • src/tool/subcommands/api_cmd/api_compare_tests.rs
  • src/tool/subcommands/api_cmd/generate_test_snapshot.rs
  • src/tool/subcommands/api_cmd/test_snapshot.rs
  • src/tool/subcommands/archive_cmd.rs
  • src/tool/subcommands/index_cmd.rs
  • src/tool/subcommands/snapshot_cmd.rs

Comment thread src/db/parity_db.rs Outdated
Comment thread src/rpc/methods/state.rs
Comment thread src/tool/subcommands/api_cmd/generate_test_snapshot.rs Outdated
@codecov
Copy link
Copy Markdown

codecov Bot commented May 14, 2026

Codecov Report

❌ Patch coverage is 55.28634% with 609 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.16%. Comparing base (501aa30) to head (46b7671).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/rpc/methods/state.rs 57.93% 36 Missing and 25 partials ⚠️
src/chain_sync/chain_follower.rs 29.72% 51 Missing and 1 partial ⚠️
src/rpc/methods/eth.rs 68.24% 43 Missing and 4 partials ⚠️
src/db/db_impl.rs 34.32% 38 Missing and 6 partials ⚠️
src/rpc/methods/f3.rs 0.00% 26 Missing ⚠️
src/rpc/methods/chain.rs 71.91% 18 Missing and 7 partials ⚠️
src/daemon/mod.rs 42.85% 19 Missing and 1 partial ⚠️
src/tool/subcommands/archive_cmd.rs 43.33% 16 Missing and 1 partial ⚠️
src/rpc/methods/net.rs 0.00% 14 Missing ⚠️
src/rpc/methods/mpool.rs 7.14% 13 Missing ⚠️
... and 68 more
Additional details and impacted files
Files with missing lines Coverage Δ
src/beacon/drand.rs 75.26% <ø> (ø)
src/blocks/chain4u.rs 93.24% <100.00%> (+1.03%) ⬆️
src/blocks/tipset.rs 84.88% <ø> (ø)
src/chain/snapshot_format.rs 44.11% <ø> (ø)
src/chain/store/index.rs 89.47% <100.00%> (ø)
src/chain/store/tipset_tracker.rs 68.88% <100.00%> (+2.62%) ⬆️
src/chain_sync/bad_block_cache.rs 79.48% <100.00%> (+3.01%) ⬆️
src/chain_sync/network_context.rs 30.35% <ø> (ø)
src/cli/humantoken.rs 99.34% <ø> (ø)
src/cli/subcommands/chain_cmd/list.rs 0.00% <ø> (ø)
... and 110 more

... and 4 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 501aa30...46b7671. Read the comment docs.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
src/tool/subcommands/api_cmd/test_snapshot.rs (1)

145-146: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Propagate StateManager::new failures instead of panicking.

This helper already returns anyhow::Result, so unwrap() turns a recoverable snapshot/bootstrap error into an avoidable panic.

Proposed fix
-    let state_manager = StateManager::new(chain_store.shallow_clone()).unwrap();
+    let state_manager = StateManager::new(chain_store.shallow_clone())?;

As per coding guidelines "Avoid unwrap() in production code; use ? or expect() with descriptive messages instead".

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/tool/subcommands/api_cmd/test_snapshot.rs` around lines 145 - 146, The
code calls StateManager::new(...).unwrap(), which panics on failure; change this
to propagate the error instead—replace the unwrap() on
StateManager::new(chain_store.shallow_clone()) with the try operator (?) (or use
.with_context(...) and ? for a descriptive message) so the helper's
anyhow::Result returns the error instead of aborting; locate the state_manager
binding and adjust its initialization to use ? and optional context to clarify
the failure.
🧹 Nitpick comments (4)
src/statediff/mod.rs (1)

66-67: ⚡ Quick win

Add context to StateTree::new_from_root failures.

Both fallible calls currently return raw errors; attach root-specific context to make failures actionable.

Suggested patch
+use anyhow::Context as _;
...
-    let state_tree = StateTree::new_from_root(bs, root)?;
+    let state_tree = StateTree::new_from_root(bs, root)
+        .with_context(|| format!("failed to load state tree from root {root}"))?;
...
-    let state_tree = StateTree::new_from_root(bs, root)?;
+    let state_tree = StateTree::new_from_root(bs, root)
+        .with_context(|| format!("failed to load calculated state tree from root {root}"))?;

As per coding guidelines, "Use anyhow::Result<T> for most operations and add context with .context() when errors occur".

Also applies to: 90-90

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/statediff/mod.rs` around lines 66 - 67, StateTree::new_from_root failures
are returned as raw errors; wrap both calls to StateTree::new_from_root with
anyhow context to include the root value (and any other relevant info) for
actionable diagnostics. Locate the calls to StateTree::new_from_root (the one
using variables bs and root and the second occurrence mentioned) and replace the
plain ? propagation with .context(format!("creating StateTree from root {}",
root))? (or similar context message) so errors include root-specific context.
src/libp2p/service.rs (1)

333-337: 💤 Low value

Consider deferring db_owned() creation to when it's actually needed.

db_owned() is called for every network message, but it's only used for BitswapRequest messages. For other message types (PubsubMessage, HelloRequest, ChainExchangeRequest, JSONRPCRequest), the db handle is created and immediately dropped.

If db_owned() involves non-trivial work, consider passing &self.cs and letting the handler call db_owned() only in the BitswapRequest arm.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/libp2p/service.rs` around lines 333 - 337, The current call passes
self.cs.db_owned() for every message but only BitswapRequest uses the DB; change
the call site to pass &self.cs (not db_owned()) into the handler (the place
invoking bitswap_request_manager.shallow_clone(), message,
&self.network_sender_out, &self.peer_manager) and update the handler logic so
that only the BitswapRequest branch calls cs.db_owned() to obtain the DB handle;
keep usage of bitswap_request_manager and other message arms (PubsubMessage,
HelloRequest, ChainExchangeRequest, JSONRPCRequest) unchanged so they don't
allocate the DB handle unnecessarily.
src/daemon/context.rs (1)

246-253: ⚡ Quick win

Add stage-specific context to state-manager initialization.

If daemon startup fails here, the current bare ?s won't tell you whether the failure came from ChainStore::new or StateManager::new. Wrapping both with .context(...) will make bootstrap failures much easier to triage.

♻️ Proposed fix
-    let chain_store = ChainStore::new(
-        db.shallow_clone(),
-        chain_config.shallow_clone(),
-        genesis_header,
-    )?;
+    let chain_store = ChainStore::new(
+        db.shallow_clone(),
+        chain_config.shallow_clone(),
+        genesis_header,
+    )
+    .context("failed to construct chain store")?;
 
     // Initialize StateManager
-    let state_manager = StateManager::new(chain_store)?;
+    let state_manager =
+        StateManager::new(chain_store).context("failed to construct state manager")?;
     Ok(state_manager)

As per coding guidelines "Use anyhow::Result for most operations and add context with .context() when errors occur".

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/daemon/context.rs` around lines 246 - 253, The error here is ambiguous
because failures from ChainStore::new and StateManager::new are propagated with
bare ?; change both calls to add context so errors identify the stage: wrap
ChainStore::new(...) with .context("initializing ChainStore")? and wrap
StateManager::new(chain_store)? with .context("initializing StateManager")? (or
similar descriptive strings) so bootstrap failures show which step failed; refer
to ChainStore::new and StateManager::new when making the changes and preserve
existing variable names like chain_store.
src/daemon/db_util.rs (1)

338-346: ⚡ Quick win

Attach epoch/tipset context to backfill failures.

These calls run inside a potentially long backfill loop. If any of them fail, the current error chain won't say which tipset caused the failure, which makes resume/debugging painful.

♻️ Proposed fix
-    state_manager.load_executed_tipset(ts).await?;
+    state_manager
+        .load_executed_tipset(ts)
+        .await
+        .with_context(|| format!("failed to load executed tipset for epoch {} ({})", epoch, tsk))?;
 
     delegated_messages.append(
         &mut state_manager
             .chain_store()
-            .headers_delegated_messages(ts.block_headers().iter())?,
+            .headers_delegated_messages(ts.block_headers().iter())
+            .with_context(|| {
+                format!("failed to collect delegated messages for epoch {} ({})", epoch, tsk)
+            })?,
     );
     tracing::trace!("Indexing tipset @{}: {}", epoch, &tsk);
-    tsk.save(state_manager.db())?;
+    tsk.save(state_manager.db())
+        .with_context(|| format!("failed to persist tipset key for epoch {} ({})", epoch, tsk))?;

As per coding guidelines "Use anyhow::Result for most operations and add context with .context() when errors occur".

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/daemon/db_util.rs` around lines 338 - 346, The backfill calls
(state_manager.load_executed_tipset, chain_store().headers_delegated_messages,
and tsk.save) lack contextual error information; update each await?/call to use
anyhow's .context() (or .with_context()) to attach the epoch and tipset
identifier (e.g., &tsk or tsk.key().to_string()) so failures report which
tipset/epoch failed during backfill, e.g., add context like format!("backfill
failed at epoch {} tipset {}", epoch, tsk).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/state_migration/nv26fix/migration.rs`:
- Around line 27-29: Wrap the fallible calls in migration.rs with contextualized
errors using .context(...) so failures are informative: add .context("creating
StateTree from root") to the StateTree::new_from_root(store, state)? call, add
.context("fetching system actor from state tree") to
state_tree.get_required_actor(&Address::SYSTEM_ACTOR)?, add .context("loading
SystemStateOld from store") to
store.get_cbor_required::<SystemStateOld>(&system_actor.state)?, and similarly
add context messages to the fallible calls around lines 81–87 (the other
get_required_actor / get_cbor_required / store accessors used there) to describe
which actor/state load failed; ensure each .context string names the operation
and the actor or CID being loaded.

---

Duplicate comments:
In `@src/tool/subcommands/api_cmd/test_snapshot.rs`:
- Around line 145-146: The code calls StateManager::new(...).unwrap(), which
panics on failure; change this to propagate the error instead—replace the
unwrap() on StateManager::new(chain_store.shallow_clone()) with the try operator
(?) (or use .with_context(...) and ? for a descriptive message) so the helper's
anyhow::Result returns the error instead of aborting; locate the state_manager
binding and adjust its initialization to use ? and optional context to clarify
the failure.

---

Nitpick comments:
In `@src/daemon/context.rs`:
- Around line 246-253: The error here is ambiguous because failures from
ChainStore::new and StateManager::new are propagated with bare ?; change both
calls to add context so errors identify the stage: wrap ChainStore::new(...)
with .context("initializing ChainStore")? and wrap
StateManager::new(chain_store)? with .context("initializing StateManager")? (or
similar descriptive strings) so bootstrap failures show which step failed; refer
to ChainStore::new and StateManager::new when making the changes and preserve
existing variable names like chain_store.

In `@src/daemon/db_util.rs`:
- Around line 338-346: The backfill calls (state_manager.load_executed_tipset,
chain_store().headers_delegated_messages, and tsk.save) lack contextual error
information; update each await?/call to use anyhow's .context() (or
.with_context()) to attach the epoch and tipset identifier (e.g., &tsk or
tsk.key().to_string()) so failures report which tipset/epoch failed during
backfill, e.g., add context like format!("backfill failed at epoch {} tipset
{}", epoch, tsk).

In `@src/libp2p/service.rs`:
- Around line 333-337: The current call passes self.cs.db_owned() for every
message but only BitswapRequest uses the DB; change the call site to pass
&self.cs (not db_owned()) into the handler (the place invoking
bitswap_request_manager.shallow_clone(), message, &self.network_sender_out,
&self.peer_manager) and update the handler logic so that only the BitswapRequest
branch calls cs.db_owned() to obtain the DB handle; keep usage of
bitswap_request_manager and other message arms (PubsubMessage, HelloRequest,
ChainExchangeRequest, JSONRPCRequest) unchanged so they don't allocate the DB
handle unnecessarily.

In `@src/statediff/mod.rs`:
- Around line 66-67: StateTree::new_from_root failures are returned as raw
errors; wrap both calls to StateTree::new_from_root with anyhow context to
include the root value (and any other relevant info) for actionable diagnostics.
Locate the calls to StateTree::new_from_root (the one using variables bs and
root and the second occurrence mentioned) and replace the plain ? propagation
with .context(format!("creating StateTree from root {}", root))? (or similar
context message) so errors include root-specific context.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: e20a837b-af75-48ed-a4f3-419a00f2ecab

📥 Commits

Reviewing files that changed from the base of the PR and between 698f691 and 78ebe28.

📒 Files selected for processing (120)
  • src/beacon/drand.rs
  • src/blocks/chain4u.rs
  • src/blocks/tipset.rs
  • src/chain/mod.rs
  • src/chain/snapshot_format.rs
  • src/chain/store/chain_store.rs
  • src/chain/store/index.rs
  • src/chain/store/tipset_tracker.rs
  • src/chain/tests.rs
  • src/chain_sync/bad_block_cache.rs
  • src/chain_sync/chain_follower.rs
  • src/chain_sync/network_context.rs
  • src/chain_sync/sync_status.rs
  • src/chain_sync/tipset_syncer.rs
  • src/chain_sync/validation.rs
  • src/cli/humantoken.rs
  • src/cli/subcommands/chain_cmd/list.rs
  • src/cli/subcommands/f3_cmd.rs
  • src/daemon/context.rs
  • src/daemon/db_util.rs
  • src/daemon/mod.rs
  • src/db/blockstore_with_write_buffer.rs
  • src/db/car/any.rs
  • src/db/car/many.rs
  • src/db/car/mod.rs
  • src/db/db_impl.rs
  • src/db/db_mode.rs
  • src/db/gc/snapshot.rs
  • src/db/memory.rs
  • src/db/mod.rs
  • src/db/parity_db.rs
  • src/db/parity_db/gc.rs
  • src/db/ttl/mod.rs
  • src/dev/subcommands/export_state_tree_cmd.rs
  • src/dev/subcommands/export_tipset_lookup_cmd.rs
  • src/dev/subcommands/state_cmd.rs
  • src/fil_cns/mod.rs
  • src/fil_cns/validation.rs
  • src/fil_cns/weight.rs
  • src/interpreter/fvm2.rs
  • src/interpreter/fvm3.rs
  • src/interpreter/fvm4.rs
  • src/interpreter/vm.rs
  • src/lib.rs
  • src/libp2p/chain_exchange/provider.rs
  • src/libp2p/service.rs
  • src/libp2p_bitswap/request_manager.rs
  • src/message_pool/msgpool/mod.rs
  • src/message_pool/msgpool/msg_pool.rs
  • src/message_pool/msgpool/pending_store.rs
  • src/message_pool/msgpool/provider.rs
  • src/rpc/auth_layer.rs
  • src/rpc/methods/auth.rs
  • src/rpc/methods/beacon.rs
  • src/rpc/methods/chain.rs
  • src/rpc/methods/common.rs
  • src/rpc/methods/eth.rs
  • src/rpc/methods/eth/filter/mod.rs
  • src/rpc/methods/eth/pubsub.rs
  • src/rpc/methods/eth/tipset_resolver.rs
  • src/rpc/methods/eth/trace/mod.rs
  • src/rpc/methods/eth/trace/parity.rs
  • src/rpc/methods/eth/trace/state_diff.rs
  • src/rpc/methods/eth/trace/test_helpers.rs
  • src/rpc/methods/f3.rs
  • src/rpc/methods/gas.rs
  • src/rpc/methods/market.rs
  • src/rpc/methods/miner.rs
  • src/rpc/methods/misc.rs
  • src/rpc/methods/mpool.rs
  • src/rpc/methods/msig.rs
  • src/rpc/methods/net.rs
  • src/rpc/methods/node.rs
  • src/rpc/methods/state.rs
  • src/rpc/methods/sync.rs
  • src/rpc/methods/wallet.rs
  • src/rpc/mod.rs
  • src/rpc/reflect/mod.rs
  • src/shim/machine/manifest.rs
  • src/shim/state_tree.rs
  • src/state_manager/actor_queries.rs
  • src/state_manager/address_resolution.rs
  • src/state_manager/cache.rs
  • src/state_manager/chain_rand.rs
  • src/state_manager/circulating_supply.rs
  • src/state_manager/execution.rs
  • src/state_manager/message_search.rs
  • src/state_manager/message_simulation.rs
  • src/state_manager/mining.rs
  • src/state_manager/mod.rs
  • src/state_manager/state_computation.rs
  • src/state_manager/utils.rs
  • src/state_migration/mod.rs
  • src/state_migration/nv17/migration.rs
  • src/state_migration/nv17/miner.rs
  • src/state_migration/nv17/verifreg_market.rs
  • src/state_migration/nv18/eam.rs
  • src/state_migration/nv18/migration.rs
  • src/state_migration/nv19/migration.rs
  • src/state_migration/nv21/migration.rs
  • src/state_migration/nv21/miner.rs
  • src/state_migration/nv21fix/migration.rs
  • src/state_migration/nv21fix2/migration.rs
  • src/state_migration/nv22/migration.rs
  • src/state_migration/nv22fix/migration.rs
  • src/state_migration/nv23/migration.rs
  • src/state_migration/nv24/migration.rs
  • src/state_migration/nv25/migration.rs
  • src/state_migration/nv26fix/migration.rs
  • src/state_migration/nv27/migration.rs
  • src/state_migration/nv28/migration.rs
  • src/statediff/mod.rs
  • src/tool/offline_server/server.rs
  • src/tool/subcommands/api_cmd/api_compare_tests.rs
  • src/tool/subcommands/api_cmd/generate_test_snapshot.rs
  • src/tool/subcommands/api_cmd/test_snapshot.rs
  • src/tool/subcommands/archive_cmd.rs
  • src/tool/subcommands/index_cmd.rs
  • src/utils/cache/lru.rs
  • src/utils/db/car_util.rs
✅ Files skipped from review due to trivial changes (15)
  • src/shim/machine/manifest.rs
  • src/chain/tests.rs
  • src/lib.rs
  • src/utils/cache/lru.rs
  • src/db/blockstore_with_write_buffer.rs
  • src/rpc/methods/eth/trace/state_diff.rs
  • src/rpc/methods/market.rs
  • src/message_pool/msgpool/mod.rs
  • src/state_migration/nv18/eam.rs
  • src/chain/snapshot_format.rs
  • src/state_migration/nv17/verifreg_market.rs
  • src/db/car/any.rs
  • src/cli/subcommands/chain_cmd/list.rs
  • src/message_pool/msgpool/pending_store.rs
  • src/utils/db/car_util.rs
🚧 Files skipped from review as they are similar to previous changes (70)
  • src/dev/subcommands/export_tipset_lookup_cmd.rs
  • src/db/car/many.rs
  • src/rpc/methods/misc.rs
  • src/db/parity_db/gc.rs
  • src/rpc/methods/eth/trace/parity.rs
  • src/tool/subcommands/index_cmd.rs
  • src/rpc/methods/eth/trace/test_helpers.rs
  • src/rpc/methods/auth.rs
  • src/state_migration/nv21fix/migration.rs
  • src/dev/subcommands/export_state_tree_cmd.rs
  • src/db/ttl/mod.rs
  • src/state_migration/nv17/miner.rs
  • src/rpc/methods/eth/tipset_resolver.rs
  • src/chain_sync/sync_status.rs
  • src/message_pool/msgpool/msg_pool.rs
  • src/state_migration/nv28/migration.rs
  • src/rpc/methods/eth/pubsub.rs
  • src/chain_sync/validation.rs
  • src/state_migration/mod.rs
  • src/rpc/methods/node.rs
  • src/state_migration/nv25/migration.rs
  • src/state_migration/nv24/migration.rs
  • src/state_migration/nv22fix/migration.rs
  • src/rpc/mod.rs
  • src/state_migration/nv23/migration.rs
  • src/state_manager/chain_rand.rs
  • src/fil_cns/weight.rs
  • src/shim/state_tree.rs
  • src/state_migration/nv18/migration.rs
  • src/chain/store/index.rs
  • src/db/mod.rs
  • src/state_migration/nv21/migration.rs
  • src/state_migration/nv27/migration.rs
  • src/chain/mod.rs
  • src/rpc/methods/sync.rs
  • src/state_manager/address_resolution.rs
  • src/rpc/methods/msig.rs
  • src/db/gc/snapshot.rs
  • src/state_migration/nv17/migration.rs
  • src/rpc/reflect/mod.rs
  • src/interpreter/fvm2.rs
  • src/state_migration/nv21fix2/migration.rs
  • src/chain_sync/network_context.rs
  • src/blocks/chain4u.rs
  • src/state_migration/nv19/migration.rs
  • src/daemon/mod.rs
  • src/tool/subcommands/api_cmd/api_compare_tests.rs
  • src/rpc/methods/mpool.rs
  • src/state_manager/actor_queries.rs
  • src/state_manager/state_computation.rs
  • src/rpc/methods/eth/filter/mod.rs
  • src/chain_sync/tipset_syncer.rs
  • src/message_pool/msgpool/provider.rs
  • src/rpc/methods/net.rs
  • src/state_manager/circulating_supply.rs
  • src/tool/subcommands/archive_cmd.rs
  • src/rpc/methods/common.rs
  • src/interpreter/fvm3.rs
  • src/rpc/methods/wallet.rs
  • src/interpreter/vm.rs
  • src/tool/offline_server/server.rs
  • src/rpc/methods/gas.rs
  • src/db/db_impl.rs
  • src/chain/store/chain_store.rs
  • src/libp2p/chain_exchange/provider.rs
  • src/rpc/methods/f3.rs
  • src/interpreter/fvm4.rs
  • src/rpc/methods/state.rs
  • src/rpc/methods/eth.rs
  • src/rpc/methods/chain.rs

Comment thread src/state_migration/nv26fix/migration.rs
LesnyRumcajs
LesnyRumcajs previously approved these changes May 14, 2026
@hanabi1224 hanabi1224 enabled auto-merge May 14, 2026 08:47
@hanabi1224 hanabi1224 disabled auto-merge May 14, 2026 08:47
@hanabi1224 hanabi1224 enabled auto-merge May 14, 2026 09:35
@hanabi1224 hanabi1224 added this pull request to the merge queue May 14, 2026
Merged via the queue into main with commit 8088a29 May 14, 2026
34 checks passed
@hanabi1224 hanabi1224 deleted the hm/db-enum-final branch May 14, 2026 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

RPC requires calibnet RPC checks to run on CI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants