fix: optimize logic of looking up genesis from a tipset#7290
Conversation
hanabi1224
added
RPC
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughGenesis handling now uses stored ChangesGenesis tipset, chain state, and export flow
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Client
participant Tipset
participant ChainStore
participant ChainIndex
participant StateComputation
Client->>Tipset: genesis(store)
Tipset->>ChainStore: store / return genesis tipset
ChainStore->>ChainIndex: new(store, genesis)
StateComputation->>ChainIndex: genesis()
ChainIndex-->>StateComputation: cached genesis tipset
StateComputation->>StateComputation: derive genesis timestamp
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
hanabi1224
requested review from
LesnyRumcajs and
akaladarshi
and removed request for
a team
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/chain/store/chain_store.rs (1)
115-131: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winValidate that cached genesis is actually epoch 0.
ChainIndexnow trusts the stored genesis for height-0 lookups, butChainStore::newaccepts anyTipset. The updated test currently buildsgen_tsfrom a header withepoch: 1, which would make epoch-0 lookups return an epoch-1 tipset if used through the index.Proposed fix
let db = db.into(); let genesis = genesis.into(); + anyhow::ensure!( + genesis.epoch() == 0, + "genesis tipset must be at epoch 0, got {}", + genesis.epoch() + );- epoch: 1, + epoch: 0,Also applies to: 735-738
🤖 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/chain/store/chain_store.rs` around lines 115 - 131, Ensure the genesis tipset passed into ChainStore::new is actually epoch 0 before it gets cached and handed to ChainIndex::new, since the index now relies on that stored genesis for height-0 lookups. Update the test fixture that builds gen_ts so it uses a header at epoch 0 instead of epoch 1, or add validation in ChainStore::new to reject non-zero genesis tipsets. Use the ChainStore::new and ChainIndex::new symbols to locate the affected flow.
🧹 Nitpick comments (4)
src/blocks/tipset.rs (1)
430-437: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd context to the new fallible genesis lookup paths.
The
spawn_blocking(...).await?andstore.get_cbor(&genesis_cid)?errors lose the operation/CID context, which makes failed genesis resolution harder to diagnose.Proposed fix
- tokio::task::spawn_blocking(move || this.genesis_blocking(&store)).await? + tokio::task::spawn_blocking(move || this.genesis_blocking(&store)) + .await + .context("genesis lookup blocking task failed")?let genesis_block: CachingBlockHeader = store - .get_cbor(&genesis_cid)? + .get_cbor(&genesis_cid) + .with_context(|| format!("failed to load genesis block {genesis_cid}"))? .context("Genesis block missing from database")?;As per coding guidelines,
**/*.rs: add context with.context()when errors occur.Also applies to: 467-470
🤖 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/blocks/tipset.rs` around lines 430 - 437, The new fallible genesis lookup paths in Tipset::genesis and genesis_blocking lose useful operation and CID context when they fail. Update the spawn_blocking(...).await? path to add context around the async blocking lookup, and add .context() to the store.get_cbor(&genesis_cid)? failure so the genesis CID and operation are included. Use the existing Tipset::genesis and genesis_blocking symbols to locate the changes and keep the error messages specific to genesis resolution.Source: Coding guidelines
src/chain/store/index.rs (1)
180-180: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the height-zero contract docs.
Line 180 no longer performs a genesis lookup, so the doc above this method should stop saying height-zero can fail because “genesis lookup fails.”
🤖 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/chain/store/index.rs` at line 180, The doc above the height-zero contract method is now outdated because `self.genesis.shallow_clone()` no longer implies a genesis lookup can fail. Update the documentation for the relevant method in `Store` so it no longer mentions “genesis lookup fails” for height zero, and make sure the wording matches the current `genesis`-based behavior in `src/chain/store/index.rs`.src/tool/subcommands/archive_cmd.rs (2)
553-576: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider guarding against a mismatched caller-supplied genesis.
do_exportnow trusts anySome(genesis)passed in without checking it actually belongs toroot's chain (e.g.genesis.epoch() == 0). A future caller passing a genesis from a different chain/store would silently produce a wrongnetwork/finality/output-path derivation and could corrupt an exported snapshot. Only the current caller (ForestChainExportDiff::handle) is self-consistent, but nothing enforces this contract going forward.🛡️ Proposed cheap guard
let genesis = if let Some(genesis) = genesis { + debug_assert_eq!( + genesis.epoch(), + 0, + "caller-supplied genesis must be an epoch-0 tipset" + ); genesis } else {🤖 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/archive_cmd.rs` around lines 553 - 576, Guard do_export against a caller-supplied genesis that does not match the root chain by validating the optional genesis before using it, such as confirming it is the actual chain genesis (for example via genesis.epoch() or equivalent Tipset checks) and rejecting mismatches with an error. Keep the existing fallback path that derives genesis from ts.genesis(store.shallow_clone()), and ensure the NetworkChain::from_genesis_or_devnet_placeholder derivation only runs on a verified genesis.
1059-1064: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
sync_bucket's export loops repeat the slow genesis walk on every iteration instead of reusing the genesis already resolved.
sync_bucketalready resolves the genesis tipset once (to getgenesis_timestampat line ~1135-1139), butexport_lite_snapshot/export_diff_snapshotare calledNoneforgenesis(lines 1062, 1099) inside per-epoch loops (steps_in_range(...)). Each call therefore forcesdo_exportto redo the "slow"ts.genesis(store.shallow_clone()).await?chain-walk once per exported epoch — directly re-introducing the genesis-lookup cost this PR is meant to eliminate, just in a different (batch/tooling) code path.Thread the genesis
Tipsetthatsync_bucketalready computed throughexport_lite_snapshot/export_diff_snapshotintodo_exportinstead of re-deriving it per epoch.♻️ Proposed refactor sketch
-async fn export_lite_snapshot<DB>( - store: DB, - root: Tipset, - network: &str, - genesis_timestamp: u64, - epoch: ChainEpoch, -) -> anyhow::Result<PathBuf> +async fn export_lite_snapshot<DB>( + store: DB, + root: Tipset, + network: &str, + genesis: Tipset, + epoch: ChainEpoch, +) -> anyhow::Result<PathBuf> where DB: Blockstore + ShallowClone + Into<DbImpl> + Unpin + Send + Sync + 'static, { - let output_path: PathBuf = format_lite_snapshot(network, genesis_timestamp, epoch)?.into(); + let output_path: PathBuf = + format_lite_snapshot(network, genesis.min_ticket_block().timestamp, epoch)?.into(); ... do_export( &store, root, - None, + Some(genesis), output_path.clone(), ...And in
sync_bucket, keep the resolvedgenesisTipset (not just its timestamp) and passgenesis.clone()into each call site instead ofgenesis_timestampalone. Apply the same change toexport_diff_snapshot.Also applies to: 1096-1101, 1116-1207
🤖 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/archive_cmd.rs` around lines 1059 - 1064, `sync_bucket` is still re-running the slow genesis lookup inside the export loops because `export_lite_snapshot` and `export_diff_snapshot` pass `None` for `genesis` into `do_export`. Reuse the already-resolved genesis `Tipset` from `sync_bucket` (the one used to derive `genesis_timestamp`) and thread it through both export helpers so `do_export` receives that value instead of calling `ts.genesis(store.shallow_clone()).await?` again for every epoch.
🤖 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.
Outside diff comments:
In `@src/chain/store/chain_store.rs`:
- Around line 115-131: Ensure the genesis tipset passed into ChainStore::new is
actually epoch 0 before it gets cached and handed to ChainIndex::new, since the
index now relies on that stored genesis for height-0 lookups. Update the test
fixture that builds gen_ts so it uses a header at epoch 0 instead of epoch 1, or
add validation in ChainStore::new to reject non-zero genesis tipsets. Use the
ChainStore::new and ChainIndex::new symbols to locate the affected flow.
---
Nitpick comments:
In `@src/blocks/tipset.rs`:
- Around line 430-437: The new fallible genesis lookup paths in Tipset::genesis
and genesis_blocking lose useful operation and CID context when they fail.
Update the spawn_blocking(...).await? path to add context around the async
blocking lookup, and add .context() to the store.get_cbor(&genesis_cid)? failure
so the genesis CID and operation are included. Use the existing Tipset::genesis
and genesis_blocking symbols to locate the changes and keep the error messages
specific to genesis resolution.
In `@src/chain/store/index.rs`:
- Line 180: The doc above the height-zero contract method is now outdated
because `self.genesis.shallow_clone()` no longer implies a genesis lookup can
fail. Update the documentation for the relevant method in `Store` so it no
longer mentions “genesis lookup fails” for height zero, and make sure the
wording matches the current `genesis`-based behavior in
`src/chain/store/index.rs`.
In `@src/tool/subcommands/archive_cmd.rs`:
- Around line 553-576: Guard do_export against a caller-supplied genesis that
does not match the root chain by validating the optional genesis before using
it, such as confirming it is the actual chain genesis (for example via
genesis.epoch() or equivalent Tipset checks) and rejecting mismatches with an
error. Keep the existing fallback path that derives genesis from
ts.genesis(store.shallow_clone()), and ensure the
NetworkChain::from_genesis_or_devnet_placeholder derivation only runs on a
verified genesis.
- Around line 1059-1064: `sync_bucket` is still re-running the slow genesis
lookup inside the export loops because `export_lite_snapshot` and
`export_diff_snapshot` pass `None` for `genesis` into `do_export`. Reuse the
already-resolved genesis `Tipset` from `sync_bucket` (the one used to derive
`genesis_timestamp`) and thread it through both export helpers so `do_export`
receives that value instead of calling
`ts.genesis(store.shallow_clone()).await?` again for every epoch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d64fced2-3c58-4ef2-994a-7dde4b9f04bd
📒 Files selected for processing (14)
src/blocks/tipset.rssrc/chain/store/chain_store.rssrc/chain/store/index.rssrc/chain_sync/chain_follower.rssrc/daemon/mod.rssrc/db/car/many.rssrc/rpc/methods/chain.rssrc/state_manager/execution.rssrc/state_manager/state_computation.rssrc/tool/offline_server/server.rssrc/tool/subcommands/api_cmd/api_compare_tests.rssrc/tool/subcommands/archive_cmd.rssrc/tool/subcommands/benchmark_cmd.rssrc/tool/subcommands/snapshot_cmd.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
filecoin-project/lotus(manual)
💤 Files with no reviewable changes (1)
- src/state_manager/execution.rs
hanabi1224
force-pushed
the
hm/optimize-genesis-loading
branch
from
2cd9c6e to
0179c05
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/tool/subcommands/api_cmd/api_compare_tests.rs (1)
508-509: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winIgnored parity tests won't catch epoch-0 regressions in default CI runs.
These new tests target the exact scenario from issue
#7291(epoch-0 lookups), but.ignore("Lotus times out")skips them in default test runs. Since Lotus is the side that times out,RpcTest::identity(which requires both Forest and Lotus to agree) can never usefully validate this path — the assertion is effectively disabled rather than deferred. UsingRpcTest::basicinstead (verifying only that Forest itself returns successfully/quickly) would give continuous regression coverage for the genesis-lookup performance fix without depending on Lotus's behavior.♻️ Suggested direction
- RpcTest::identity(ChainGetTipSetByHeight::request((0, Default::default())).unwrap()) - .ignore("Lotus times out"), + RpcTest::basic(ChainGetTipSetByHeight::request((0, Default::default())).unwrap()),Also applies to: 1402-1419
🤖 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/api_compare_tests.rs` around lines 508 - 509, The epoch-0 parity checks in RpcTest::identity are being skipped, so they won’t protect against regressions in default CI. Update the affected tests in api_compare_tests.rs to use RpcTest::basic for the ChainGetTipSetByHeight request path instead of identity, and remove the Lotus-timeout ignore so the Forest-side lookup remains continuously validated; apply the same change to the other epoch-0 cases referenced in the diff.
🤖 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.
Nitpick comments:
In `@src/tool/subcommands/api_cmd/api_compare_tests.rs`:
- Around line 508-509: The epoch-0 parity checks in RpcTest::identity are being
skipped, so they won’t protect against regressions in default CI. Update the
affected tests in api_compare_tests.rs to use RpcTest::basic for the
ChainGetTipSetByHeight request path instead of identity, and remove the
Lotus-timeout ignore so the Forest-side lookup remains continuously validated;
apply the same change to the other epoch-0 cases referenced in the diff.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bf714423-b1e8-4029-bfa8-af3b65be4ab4
📒 Files selected for processing (3)
src/chain/store/chain_store.rssrc/chain/store/index.rssrc/tool/subcommands/api_cmd/api_compare_tests.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
filecoin-project/lotus(manual)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/chain/store/index.rs
- src/chain/store/chain_store.rs
hanabi1224
force-pushed
the
hm/optimize-genesis-loading
branch
from
d72c525 to
cd33bd0
Compare
hanabi1224
force-pushed
the
hm/optimize-genesis-loading
branch
from
cd33bd0 to
99df92e
Compare
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/tool/subcommands/api_cmd/test_snapshot.rs (1)
288-330: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFragile substring replace for eth/filecoin name normalization.
.replace("eth.", "eth_")is a global, unanchored substring replace rather than a prefix check. It currently works because captured names contain only a single underscore (namespace + PascalCase method), but it's non-idiomatic and could silently mis-normalize any future method/namespace name that happens to contain the literal substring"eth."anywhere (not just at the start).Prefer explicitly anchoring the check to the start of the string:
♻️ Proposed fix using prefix-anchored normalization
- .replace("_", ".") - .to_lowercase() - .replace("eth.", "eth_") + .replace("_", ".") + .to_lowercase(); + let name = if let Some(rest) = name.strip_prefix("eth.") { + format!("eth_{rest}") + } else { + name + };Since this pattern deviates from idiomatic string handling, worth double-checking against actual entries in
test_snapshots.txt/test_snapshots_ignored.txtto confirm no method or namespace name contains an embedded"eth."sequence beyond the intended prefix.🤖 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 288 - 330, The name normalization in the snapshot coverage logic is too fragile because the `replace("eth.", "eth_")` call in the `covered` mapping is a global substring replacement. Update the normalization in `test_snapshot.rs` to use an explicit prefix-based check for the `eth` namespace when transforming the captured `name`, so only the intended leading namespace separator is rewritten. Keep the rest of the `print_uncovered`/coverage matching flow unchanged, and verify the entries read from `test_snapshots.txt` and `test_snapshots_ignored.txt` still normalize to the same values.
🤖 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.
Nitpick comments:
In `@src/tool/subcommands/api_cmd/test_snapshot.rs`:
- Around line 288-330: The name normalization in the snapshot coverage logic is
too fragile because the `replace("eth.", "eth_")` call in the `covered` mapping
is a global substring replacement. Update the normalization in
`test_snapshot.rs` to use an explicit prefix-based check for the `eth` namespace
when transforming the captured `name`, so only the intended leading namespace
separator is rewritten. Keep the rest of the `print_uncovered`/coverage matching
flow unchanged, and verify the entries read from `test_snapshots.txt` and
`test_snapshots_ignored.txt` still normalize to the same values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cccf36f2-534a-4ff2-9ca2-3df6f6659122
📒 Files selected for processing (4)
src/chain/store/chain_store.rssrc/tool/subcommands/api_cmd.rssrc/tool/subcommands/api_cmd/test_snapshot.rssrc/tool/subcommands/api_cmd/test_snapshots.txt
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
filecoin-project/lotus(manual)
✅ Files skipped from review due to trivial changes (1)
- src/tool/subcommands/api_cmd/test_snapshots.txt
🚧 Files skipped from review as they are similar to previous changes (2)
- src/tool/subcommands/api_cmd.rs
- src/chain/store/chain_store.rs
|
@LesnyRumcajs all green now |
2 participants
Summary of changes
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes #7291
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit