feat: add --augmented-data and --tipset-lookup to forest-cli snapshot export#7376
feat: add --augmented-data and --tipset-lookup to forest-cli snapshot export#7376hanabi1224 wants to merge 11 commits into
--augmented-data and --tipset-lookup to forest-cli snapshot export#7376Conversation
|
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:
WalkthroughSnapshot export gains optional augmented receipts/events and tipset-lookup CAR outputs. A new extended validation command checks these artifacts, while calibration checks and CLI documentation cover the new workflow. ChangesSnapshot export and validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SnapshotCLI
participant ForestChainExport
participant ValidateExtended
participant ManyCar
SnapshotCLI->>ForestChainExport: request augmented_data and tipset_lookup
ForestChainExport->>ManyCar: write base and optional CAR artifacts
SnapshotCLI->>ValidateExtended: validate base, augmented, and tipset-lookup CARs
ValidateExtended->>ManyCar: import CAR blocks
ValidateExtended->>ManyCar: load receipts, events, and HAMT checkpoints
Possibly related PRs
Suggested labels: 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 8 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); |
There was a problem hiding this comment.
This is to address #7376 (comment) I used it to get perf stats we could 1. enable it 2. remove it 3. keep it commented out, what's your preference?
There was a problem hiding this comment.
Commented out code is a pretty bad smell. The logic is trivial, so IMO it's fine to just remove it. If it later turns out to be needed, we can revive it and maybe gate it with some env variables.
| #[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?
There was a problem hiding this comment.
A lite snapshot only contains the state trees of the latest N epochs, thus we cannot find all state trees back to genesis. This command is to assert that, when the state tree presents for an epoch, the message receipts and events present for the same epoch
There was a problem hiding this comment.
Perhaps worth adding a comment in the code.
|
|
||
| $FOREST_CLI_PATH shutdown --force | ||
|
|
||
| # Check file sizes |
There was a problem hiding this comment.
@hanabi1224 I don't see any tests exercising the import of those snapshots. Do you intend to do it in a follow-up?
There was a problem hiding this comment.
forest-tool snapshot validate-extended imports and tests the snapshots. Hooking those into --auto-download really depends on how the snapshots are hosted and is beyond the scope of this PR.
Importing an augment-data snapshot into an existing node can be done via forest-tool db import.
Import a tipset lookup hamt can be implemented later similarly
hanabi1224
changed the title
--augmented-data and --tipset-lookup to forestcli snapshot export--augmented-data and --tipset-lookup to forest-cli snapshot export
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