feat: add --augmented-data and --tipset-lookup to forestcli snapshot export#7376
feat: add --augmented-data and --tipset-lookup to forestcli snapshot export#7376hanabi1224 wants to merge 7 commits into
--augmented-data and --tipset-lookup to forestcli snapshot export#7376Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (3)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughSnapshot export now accepts augmented-data and tipset-lookup options, generates additional CAR files through RPC, exposes extended validation for those files, and updates calibration checks to exercise the new outputs. ChangesSnapshot export and validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SnapshotCLI
participant ForestChainExport
participant ReceiptsEventsExporter
participant ForestCAR
SnapshotCLI->>ForestChainExport: request augmented_data and tipset_lookup
ForestChainExport->>ForestCAR: persist standard snapshot
ForestChainExport->>ReceiptsEventsExporter: export receipts and events
ReceiptsEventsExporter->>ForestCAR: write receipts/events CAR
ForestChainExport->>ForestCAR: write tipset-lookup CAR
SnapshotCLI->>ForestCAR: run extended validation
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 (1)
src/rpc/methods/chain.rs (1)
445-519: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftStage every requested CAR before publishing final paths.
The base snapshot is persisted first, while auxiliary CARs are written directly to their final paths. A later error or cancellation therefore leaves a completed base file and potentially truncated auxiliary files even though the RPC reports failure. Write all outputs to temporary paths, then persist them only after every export succeeds.
🤖 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/chain.rs` around lines 445 - 519, Update the export flow around the base snapshot persistence, tipset lookup export, and augmented-data export so every requested CAR is written to a temporary path first. Defer persisting or publishing the base and auxiliary outputs until all exports complete successfully, then atomically persist each temporary file to its final path; ensure errors or cancellation leave no completed or truncated final auxiliary files.
🧹 Nitpick comments (1)
src/chain/mod.rs (1)
265-270: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the new public snapshot APIs.
Add rustdoc describing their purpose and important semantics.
src/chain/mod.rs#L265-L270: document depth boundaries, output contents, and placeholder CAR root.src/rpc/methods/chain.rs#L1634-L1643: document the receipts/events export parameter schema.src/ipld/util.rs#L580-L586: document the stream’s role and expected generic parameters.As per coding guidelines, “Document public functions and structs with doc comments.”
🤖 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/mod.rs` around lines 265 - 270, Document the public snapshot APIs with rustdoc: in src/chain/mod.rs lines 265-270, describe export depth boundaries, output contents, and the placeholder CAR root; in src/rpc/methods/chain.rs lines 1634-1643, document the receipts/events export parameter schema; and in src/ipld/util.rs lines 580-586, describe the stream’s role and expected generic parameters.Source: Coding guidelines
🤖 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 `@scripts/tests/calibnet_export_check.sh`:
- Around line 94-101: Update the snapshot validation command in the calibration
check to invoke the harness-configured FOREST_TOOL_PATH instead of the literal
forest-tool executable, preserving the existing validate-extended arguments and
flow.
In `@src/rpc/mod.rs`:
- Line 101: Prevent the internal testing RPC from being available in production:
update the ForestChainExportReceiptsEvents registration in src/rpc/mod.rs lines
101-101 to gate or remove it, and restrict the corresponding method in
src/rpc/methods/chain.rs lines 276-282 to test/feature builds rather than
production API paths.
In `@src/tool/subcommands/snapshot_cmd.rs`:
- Around line 282-293: Update the checkpoint validation loop around
heaviest_tipset and hamt.get to derive each expected checkpoint tipset from the
base chain, require a HAMT entry for every checkpoint, and compare the stored
TipsetKey with the base tipset’s key. Reject missing entries or mismatches
before counting the checkpoint as validated, while preserving the existing load
validation and error context.
---
Outside diff comments:
In `@src/rpc/methods/chain.rs`:
- Around line 445-519: Update the export flow around the base snapshot
persistence, tipset lookup export, and augmented-data export so every requested
CAR is written to a temporary path first. Defer persisting or publishing the
base and auxiliary outputs until all exports complete successfully, then
atomically persist each temporary file to its final path; ensure errors or
cancellation leave no completed or truncated final auxiliary files.
---
Nitpick comments:
In `@src/chain/mod.rs`:
- Around line 265-270: Document the public snapshot APIs with rustdoc: in
src/chain/mod.rs lines 265-270, describe export depth boundaries, output
contents, and the placeholder CAR root; in src/rpc/methods/chain.rs lines
1634-1643, document the receipts/events export parameter schema; and in
src/ipld/util.rs lines 580-586, describe the stream’s role and expected generic
parameters.
🪄 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: f0b8604c-8331-4b98-bdbf-d67800bf11be
📒 Files selected for processing (8)
scripts/tests/calibnet_export_check.shsrc/chain/mod.rssrc/cli/subcommands/snapshot_cmd.rssrc/db/car/forest.rssrc/ipld/util.rssrc/rpc/methods/chain.rssrc/rpc/mod.rssrc/tool/subcommands/snapshot_cmd.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
filecoin-project/lotus(manual)
Codecov Report❌ Patch coverage is Additional details and impacted files
... and 7 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
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)
scripts/tests/calibnet_export_check.sh (1)
64-66: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winWait for export status instead of sleeping for a fixed five seconds.
On fast nodes, the export can finish before the second export attempt, causing this test to fail even though the implementation is correct. Poll
snapshot export-statuswith the existing retry/timeout pattern before asserting that concurrent exports are rejected.🤖 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 `@scripts/tests/calibnet_export_check.sh` around lines 64 - 66, Replace the fixed sleep after launching the first export in the snapshot export test with polling via snapshot export-status, using the script’s existing retry and timeout pattern. Continue only once the export is confirmed active, then preserve the concurrent-export rejection assertion.
🤖 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 `@scripts/tests/calibnet_export_check.sh`:
- Around line 64-66: Replace the fixed sleep after launching the first export in
the snapshot export test with polling via snapshot export-status, using the
script’s existing retry and timeout pattern. Continue only once the export is
confirmed active, then preserve the concurrent-export rejection assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 868017cd-d7a4-4034-90b5-17feb4768ca3
⛔ Files ignored due to path filters (3)
src/rpc/snapshots/forest__rpc__tests__rpc__v0.snapis excluded by!**/*.snapsrc/rpc/snapshots/forest__rpc__tests__rpc__v1.snapis excluded by!**/*.snapsrc/rpc/snapshots/forest__rpc__tests__rpc__v2.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
scripts/tests/calibnet_export_check.shsrc/tool/subcommands/snapshot_cmd.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 (1)
- src/tool/subcommands/snapshot_cmd.rs
220b4a0 to
042f4c0
Compare
|
no green checkmark, no review! |
| $callback!($crate::rpc::chain::ForestChainExportDiff); | ||
| $callback!($crate::rpc::chain::ForestChainExportStatus); | ||
| $callback!($crate::rpc::chain::ForestChainExportCancel); | ||
| // $callback!($crate::rpc::chain::ForestChainExportReceiptsEvents); |
| #[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] | ||
| pub struct ForestChainExportReceiptsEventsParams { | ||
| pub epoch: ChainEpoch, | ||
| pub recent_roots: i64, |
There was a problem hiding this comment.
unsigned? I guess it can't be negative?
| anyhow::ensure!( | ||
| augmented.is_some() || tipset_lookup.is_some(), | ||
| "either `--augmented` or `--tipset-lookup` needs to be specified" | ||
| ); |
There was a problem hiding this comment.
Not a huge deal, but AFAIK this could be specified via clap derive sorcery.
| if !store.has(ts.parent_state())? { | ||
| break; |
There was a problem hiding this comment.
I don't get it. If it can't find the parent state, then it will break... but the command itself will succeed, no?
2 participants
Stats:
Summary of changes
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit
augmented-dataandtipset-lookupoutputs to snapshot export workflows.validate-extended) to verify receipts/events and tipset-lookup contents from a base CAR plus optional auxiliary CARs.