CHANGELOG.md (1)

32-33: Fix link: reference the PR, not an Issue.

6074 is a PR. Use the PR URL to match the contribution guidelines in the header.

Apply:

- - [#6074](https://github.com/ChainSafe/forest/issues/6074) Added `forest-cli snapshot export-diff` subcommand for exporting a diff snapshot.
+ - [#6074](https://github.com/ChainSafe/forest/pull/6074) Added `forest-cli snapshot export-diff` subcommand for exporting a diff snapshot.

src/ipld/util.rs (1)

113-115: Doc/param naming drift: “exclusive” bound; align comments and names.

Behavior changes look correct, but docs still refer to stateroot_limit. Update to stateroot_limit_exclusive to prevent off‑by‑one confusion.

Apply:

-/// Stream all blocks that are reachable before the `stateroot_limit` epoch in a depth-first
+/// Stream all blocks that are reachable before the `stateroot_limit_exclusive` epoch (exclusive) in a depth-first
@@
-/// * `stateroot_limit` - An epoch that signifies how far back (exclusive) we need to inspect tipsets,
+/// * `stateroot_limit_exclusive` - Epoch strictly before which we stop deep inspection,

Also good call on:

• Using > for the exclusive bound (Lines 256, 269).
• Keeping the genesis special case (Line 269).

Also applies to: 146-150, 156-159, 165-169, 180-181, 256-265, 269-280

src/cli/subcommands/snapshot_cmd.rs (2)

44-58: Parameter UX: validate from > to and positive depth.

Defaulting depth to from - to is sensible, but add guards to fail fast on invalid inputs (e.g., from <= to, negative/zero depth).

Apply:

     ExportDiff {
@@
         depth: Option<crate::chain::ChainEpochDelta>,
     },

And in the handler (see below) add checks before issuing the RPC.

---

157-226: Create output directory and validate epochs before export.

• Ensure the parent directory exists to avoid ENOENT from tempfile_in.
• Validate from > to and that effective depth > 0 before RPC.

Apply:

@@
-                let depth = depth.unwrap_or_else(|| from - to);
+                let depth = depth.unwrap_or_else(|| from - to);
+                if from <= to {
+                    anyhow::bail!("`--from` ({from}) must be greater than `--to` ({to})");
+                }
+                if depth <= 0 {
+                    anyhow::bail!("`--depth` must be positive (got {depth})");
+                }
@@
-                let output_dir = output_path.parent().context("invalid output path")?;
+                let output_dir = output_path.parent().context("invalid output path")?;
+                std::fs::create_dir_all(output_dir)
+                    .with_context(|| format!("creating output dir {}", output_dir.display()))?;

Optional: preflight fetch Forest.Version/StateNetworkName to include chain finality in the CLI error, mirroring the server‑side check.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: hanabi1224
PR: ChainSafe/forest#5930
File: build.rs:64-77
Timestamp: 2025-08-13T09:43:20.301Z
Learning: hanabi1224 prefers hard compile-time errors in build scripts rather than runtime safeguards or collision detection, believing it's better to fail fast and fix root causes of issues like malformed snapshot names.

scripts/tests/calibnet_export_diff_check.sh (1)

6-6: Good: pipefail enabled.

Thanks for adopting set -euo pipefail; this eliminates a class of silent failures in the test harness.

scripts/tests/calibnet_export_diff_check.sh (4)

12-14: Harden snapshot discovery (deterministic latest, fail fast, robust parsing).

Globbing through find "$db_path/car_db"/*.car.zst | tail -n 1 is brittle and may pick an arbitrary file. Prefer mtime-based selection with an explicit “none found” guard and more robust db_path parsing.

- db_path=$($FOREST_TOOL_PATH db stats --chain calibnet | grep "Database path:" | cut -d':' -f2- | xargs)
- snapshot=$(find "$db_path/car_db"/*.car.zst | tail -n 1)
- snapshot_epoch=$(forest_query_epoch "$snapshot")
+ db_path="$(
+   $FOREST_TOOL_PATH db stats --chain calibnet \
+     | grep -E '^Database path:' \
+     | cut -d':' -f2- \
+     | xargs
+ )"
+ # Pick newest snapshot by mtime; fail clearly if none.
+ snapshot="$(ls -1t "$db_path"/car_db/*.car.zst 2>/dev/null | head -n1 || true)"
+ if [[ -z "${snapshot:-}" ]]; then
+   echo "Error: no *.car.zst snapshots found under: $db_path/car_db" >&2
+   exit 1
+ fi
+ snapshot_epoch="$(forest_query_epoch "$snapshot")"

---

16-23: Parametrize depth and compute target epoch once; add guards.

Make depth configurable, derive --to from it, and add a simple lower-bound guard to avoid negative epochs in edge cases.

- echo "Exporting diff snapshot @ $snapshot_epoch with forest-cli snapshot export-diff"
- $FOREST_CLI_PATH snapshot export-diff --from "$snapshot_epoch" --to "$((snapshot_epoch - 900))" -d 900 -o diff1
+ DEPTH="${DEPTH:-900}"
+ TO_EPOCH="$((snapshot_epoch - DEPTH))"
+ if (( TO_EPOCH < 0 )); then
+   echo "Error: computed --to epoch ($TO_EPOCH) is negative (from=$snapshot_epoch depth=$DEPTH)" >&2
+   exit 1
+ fi
+ echo "Exporting diff snapshot @ $snapshot_epoch with forest-cli snapshot export-diff (to=$TO_EPOCH depth=$DEPTH)"
+ $FOREST_CLI_PATH snapshot export-diff --from "$snapshot_epoch" --to "$TO_EPOCH" -d "$DEPTH" -o diff1
@@
- echo "Exporting diff snapshot @ $snapshot_epoch with forest-tool archive export"
- $FOREST_TOOL_PATH archive export --epoch "$snapshot_epoch" --depth 900 --diff "$((snapshot_epoch - 900))" --diff-depth 900 -o diff2 "$snapshot"
+ echo "Exporting diff snapshot @ $snapshot_epoch with forest-tool archive export"
+ $FOREST_TOOL_PATH archive export \
+   --epoch "$snapshot_epoch" \
+   --depth "$DEPTH" \
+   --diff "$TO_EPOCH" \
+   --diff-depth "$DEPTH" \
+   -o diff2 "$snapshot"

---

24-25: Compare normalized metadata, not raw bytes (avoid compression framing noise).

Byte-for-byte diffs of compressed outputs can spuriously fail. Compare canonicalized metadata from archive info. Optionally keep strict compare behind a flag.

- echo "Comparing diff snapshots"
- diff diff1 diff2
+ echo "Comparing diff snapshots (metadata)"
+ $FOREST_TOOL_PATH archive info diff1 > info1.txt
+ $FOREST_TOOL_PATH archive info diff2 > info2.txt
+ diff -u info1.txt info2.txt
+ if [[ "${STRICT_BYTE_COMPARE:-0}" == "1" ]]; then
+   echo "Strict byte-by-byte compare enabled"
+   diff diff1 diff2
+ fi

---

6-10: Use tmpdir + trap to avoid file collisions and ensure cleanup.

Guard against parallel CI runs and leave a clean workspace.

 set -euo pipefail
 
 source "$(dirname "$0")/harness.sh"
 
 forest_init "$@"
+
+# Work in a temporary directory and clean up on exit.
+TMPDIR="$(mktemp -d -t calibnet-export-diff-XXXXXX)"
+trap 'rm -rf "$TMPDIR"' EXIT
+cd "$TMPDIR"

src/tool/subcommands/archive_cmd.rs (6)

262-275: Construct Arc once for the store; avoid temporary into() + reference.

This improves readability and avoids borrowing a temporary. It’s consistent with other call sites that already use Arc.

-                let store = ManyCar::try_from(snapshot_files)?;
-                let heaviest_tipset = store.heaviest_tipset()?;
-                do_export(
-                    &store.into(),
-                    heaviest_tipset.into(),
+                let store = Arc::new(ManyCar::try_from(snapshot_files)?);
+                let heaviest_tipset = store.heaviest_tipset()?;
+                do_export(
+                    &store,
+                    heaviest_tipset.into(),
                     output_path,
                     epoch,
                     depth,
                     diff,
                     diff_depth,
                     force,
                 )
                 .await

---

540-540: Prefer Arc::clone for clarity when cloning Arcs.

Makes intent explicit and avoids confusion with cloning the underlying store.

-    let index = ChainIndex::new(store.clone());
+    let index = ChainIndex::new(Arc::clone(store));

---

997-1005: Minor: avoid implicit into() for root in helpers.

Being explicit about Arc materialization reads clearer in these paths.

-    do_export(
-        &store,
-        root.into(),
+    let root = Arc::new(root);
+    do_export(
+        &store,
+        root,
         output_path.clone(),
         Some(epoch),
         depth,
         diff,
         diff_depth,
         force,
     )

---

1031-1038: Same root.into() nit as above in diff export helper.

-    do_export(
-        &store,
-        root.into(),
+    let root = Arc::new(root);
+    do_export(
+        &store,
+        root,
         output_path.clone(),
         Some(epoch),
         depth,
         diff,
         diff_depth,
         force,
     )

---

1171-1179: Test: construct Arc once to match production path.

Avoid borrowing a temporary; also mirrors the main code path suggested above.

-        do_export(
-            &store.into(),
-            heaviest_tipset.into(),
+        let store = Arc::new(store);
+        let heaviest_tipset = Arc::new(heaviest_tipset);
+        do_export(
+            &store,
+            heaviest_tipset,
             output_path.path().into(),
             Some(0),
             1,
             None,
             None,
             false,
         )

---

512-516: Change API to take Arc<impl Blockstore + Send + Sync + 'static> by value — update callers

Mirrors root: Arc<Tipset> and removes the odd &Arc borrow; update the callers below to pass the Arc (remove & or clone as needed):

• src/rpc/methods/chain.rs:447 — do_export(&ctx.store_owned(), start_ts, output_path)
• src/tool/subcommands/archive_cmd.rs:264 — do_export(&store.into(), heaviest_tipset.into(), output_path)
• src/tool/subcommands/archive_cmd.rs:995 — do_export(&store, root.into(), output_path.clone())
• src/tool/subcommands/archive_cmd.rs:1028 — do_export(&store, root.into(), output_path.clone())
• src/tool/subcommands/archive_cmd.rs:1170 — do_export(&store.into(), heaviest_tipset.into(), output_path.path().into())

Also search for out-of-crate uses before merging.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: hanabi1224
PR: ChainSafe/forest#5930
File: build.rs:64-77
Timestamp: 2025-08-13T09:43:20.301Z
Learning: hanabi1224 prefers hard compile-time errors in build scripts rather than runtime safeguards or collision detection, believing it's better to fail fast and fix root causes of issues like malformed snapshot names.

Learnt from: hanabi1224
PR: ChainSafe/forest#6079
File: src/chain_sync/metrics.rs:6-7
Timestamp: 2025-09-16T12:55:26.955Z
Learning: HEAD_EPOCH references in shell scripts (like scripts/tests/calibnet_eth_mapping_check.sh) that extract data from `forest-cli info show` are unrelated to Rust metrics constants with the same name and should not be flagged when metrics cleanup is performed.

src/cli/subcommands/snapshot_cmd.rs (1)

173-175: Nice: adopted prior depth defaulting suggestion.
Matches the earlier review note; thanks for folding it in.

src/cli/subcommands/snapshot_cmd.rs (5)

44-58: Clarify flag semantics in help text.
Add that --from must be >= --to, and that depth defaults to (from - to). This reduces user confusion and mismatches with filenames.

---

44-58: Optional: parity with export flags.
Consider adding --dry-run/--skip-checksum (even if checksum is a no-op here) for UI parity between export and export-diff.

---

176-185: Make output path handling resilient (no parent, missing dirs).
Currently parent() None (e.g., "diff.car.zst") errors, and missing directories aren’t created. Default to . and ensure the directory exists.

Apply this diff:

-                let output_dir = output_path.parent().context("invalid output path")?;
+                let output_dir = output_path.parent().unwrap_or_else(|| Path::new("."));
+                std::fs::create_dir_all(output_dir)
+                    .with_context(|| format!("failed to create output dir {}", output_dir.display()))?;

Note: Consider applying the same improvement in the Export path (Lines 103–105) for consistency.

---

200-213: Ensure progress task cannot outlive on early errors.
The spawned reporter keeps running if the RPC call fails before reaching the abort. The next comment proposes a minimal restructure that guarantees abort/await on all paths.

---

214-223: Always abort/await the progress task even on RPC errors.
Restructure to handle abortion before propagating errors.

Apply this diff:

-                // Manually construct RpcRequest because snapshot export could
-                // take a few hours on mainnet
-                client
-                    .call(ForestChainExportDiff::request((params,))?.with_timeout(Duration::MAX))
-                    .await?;
-
-                handle.abort();
-                pb.finish();
-                _ = handle.await;
+                // Manually construct RpcRequest because snapshot export could
+                // take a few hours on mainnet
+                let res = client
+                    .call(ForestChainExportDiff::request((params,))?.with_timeout(Duration::MAX))
+                    .await;
+                handle.abort();
+                pb.finish();
+                _ = handle.await;
+                res?;

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: hanabi1224
PR: ChainSafe/forest#5930
File: build.rs:64-77
Timestamp: 2025-08-13T09:43:20.301Z
Learning: hanabi1224 prefers hard compile-time errors in build scripts rather than runtime safeguards or collision detection, believing it's better to fail fast and fix root causes of issues like malformed snapshot names.

src/cli/subcommands/snapshot_cmd.rs (1)

173-175: Also enforce from > to (independent of depth).

With an explicit positive depth, from <= to currently passes CLI validation and will rely on server‑side behavior. Make intent explicit and fail early.

-                let depth = depth.unwrap_or_else(|| from - to);
-                anyhow::ensure!(depth > 0, "depth must be positive");
+                anyhow::ensure!(from > to, "`from` must be greater than `to`");
+                let depth = depth.unwrap_or_else(|| from - to);
+                anyhow::ensure!(depth > 0, "depth must be positive");

src/cli/subcommands/snapshot_cmd.rs (4)

44-58: Validate epochs at parse-time to fail fast.

Enforce non‑negative epochs via clap so users get immediate feedback; keep runtime checks too.

     /// Epoch to export from
-    #[arg(long)]
+    #[arg(long, value_parser = clap::value_parser!(i64).range(0..))]
     from: i64,
     /// Epoch to diff against
-    #[arg(long)]
+    #[arg(long, value_parser = clap::value_parser!(i64).range(0..))]
     to: i64,

---

163-171: DRY up chain name mapping.

The calibnet mapping is duplicated in Export and ExportDiff. Consider extracting a small helper and reusing it in both branches.

Example (outside this hunk):

fn snapshot_chain_name(raw: &str) -> &str {
    if raw == calibnet::NETWORK_GENESIS_NAME {
        calibnet::NETWORK_COMMON_NAME
    } else {
        raw
    }
}

---

183-185: Clarify handling when output_path points to a non‑existent directory.

is_dir() returns false for non‑existent paths, so a user‑intended directory might be treated as a file path. Consider documenting this or adding a --output-dir flag to disambiguate.

---

224-226: Optional: emit a checksum for parity with export.

Diff exports don’t persist a checksum; consider computing SHA‑256 client‑side post‑persist (or have RPC return it) for UX consistency.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: hanabi1224
PR: ChainSafe/forest#5930
File: build.rs:64-77
Timestamp: 2025-08-13T09:43:20.301Z
Learning: hanabi1224 prefers hard compile-time errors in build scripts rather than runtime safeguards or collision detection, believing it's better to fail fast and fix root causes of issues like malformed snapshot names.