Skip to content

Conversation

@LesnyRumcajs
Copy link
Member

@LesnyRumcajs LesnyRumcajs commented Oct 30, 2025

Summary of changes

Changes introduced in this pull request:

  • v1 snapshots are getting decommissioned (sooner or later) - use correct link for the Forest tooling downloads plus update docs

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.

Summary by CodeRabbit

  • Bug Fixes

    • Calibnet snapshot download URL updated to point to the current "latest" snapshot path.
  • Documentation

    • Snapshot docs simplified: reference FRC-108 for default v2 and provide streamlined download instructions with a single v2 note and two links (mainnet, calibnet).
  • Tests / Chores

    • Test/export script retry delay increased to reduce rapid polling during export status checks.

@LesnyRumcajs LesnyRumcajs requested a review from a team as a code owner October 30, 2025 13:19
@LesnyRumcajs LesnyRumcajs requested review from elmattic and hanabi1224 and removed request for a team October 30, 2025 13:19
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 30, 2025

Walkthrough

Updates docs to reference FRC‑108 and a single download section, changes Calibnet snapshot URL constant to the "latest/calibnet/" path, and increases a test script sleep interval from 0.1s to 0.5s. (≤50 words)

Changes

Cohort / File(s) Summary
Documentation simplification
docs/docs/users/filecoin_services.md
Removes separate v1/v2 instructions and explicit default note; adds concise FRC‑108 reference and a simplified download section with two links (mainnet-latest, calibnet-latest).
Snapshot URL constant update
src/cli_shared/snapshot.rs
Changes FOREST_CALIBNET_COMPRESSED constant from https://forest-archive.chainsafe.dev/latest-v1/calibnet to https://forest-archive.chainsafe.dev/latest/calibnet/.
Test script timing tweak
scripts/tests/calibnet_export_check.sh
Increases sleep_interval from 0.1 to 0.5, affecting retry/backoff delays in export status polling and cancellation checks.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Small, localized edits across docs, a constant, and a test script.
  • Pay attention to:
    • Exact URL string and trailing slash in FOREST_CALIBNET_COMPRESSED.
    • Tests, CI scripts, or fixtures that reference latest-v1 vs latest.
    • Potential timing impacts of increased sleep interval in calibnet_export_check.sh.

Possibly related PRs

Suggested reviewers

  • hanabi1224
  • elmattic
  • akaladarshi

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The pull request title "chore: use default links of v2 snapshots" directly aligns with the main objectives and changes in this PR. The changeset updates documentation, code constants, and test scripts to transition from v1 snapshot links to v2/default snapshot links as v1 snapshots are being decommissioned. The title is concise, specific, and clearly conveys the primary change across all modified files without being vague or misleading. A teammate reviewing the repository history would understand that this PR addresses the migration to v2 snapshot links.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix-forest-snapshot-link

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3508037 and 6d1fbf0.

📒 Files selected for processing (1)
  • scripts/tests/calibnet_export_check.sh (1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: LesnyRumcajs
PR: ChainSafe/forest#5907
File: src/rpc/methods/state.rs:523-570
Timestamp: 2025-08-06T15:44:33.467Z
Learning: LesnyRumcajs prefers to rely on BufWriter's Drop implementation for automatic flushing rather than explicit flush() calls in Forest codebase.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
  • GitHub Check: Build forest binaries on Linux AMD64
  • GitHub Check: All lint checks
  • GitHub Check: Check
  • GitHub Check: Build Ubuntu
  • GitHub Check: cargo-publish-dry-run
  • GitHub Check: tests
  • GitHub Check: tests-release
  • GitHub Check: Deploy to Cloudflare Pages
🔇 Additional comments (1)
scripts/tests/calibnet_export_check.sh (1)

15-15: Clarify the sleep interval increase rationale.

The sleep interval was increased 5× from 0.1s to 0.5s, which extends test execution time significantly (potentially adding ~10s across the two retry loops). While this may be necessary for v2 endpoint response characteristics, the change lacks justification in code comments or the PR description.

Please verify:

  1. Is the increase necessary due to v2 endpoint being slower?
  2. Is 0.5s empirically sufficient, or is it a precautionary measure?
  3. Should the reason be documented inline?

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
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: 0

🧹 Nitpick comments (1)
src/cli_shared/snapshot.rs (1)

135-136: Consider adding trailing slash for consistency.

There's a minor inconsistency: FOREST_MAINNET_COMPRESSED has a trailing slash (/latest/mainnet/) while FOREST_CALIBNET_COMPRESSED does not (/latest/calibnet). For consistency and to avoid potential differences in HTTP redirect behavior, consider adding a trailing slash to the calibnet URL.

Apply this diff to make the URLs consistent:

-    const FOREST_CALIBNET_COMPRESSED: &str = "https://forest-archive.chainsafe.dev/latest/calibnet";
+    const FOREST_CALIBNET_COMPRESSED: &str = "https://forest-archive.chainsafe.dev/latest/calibnet/";
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 91f4ed1 and da04d3d.

📒 Files selected for processing (2)
  • docs/docs/users/filecoin_services.md (1 hunks)
  • src/cli_shared/snapshot.rs (1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: LesnyRumcajs
PR: ChainSafe/forest#5907
File: src/rpc/methods/state.rs:523-570
Timestamp: 2025-08-06T15:44:33.467Z
Learning: LesnyRumcajs prefers to rely on BufWriter's Drop implementation for automatic flushing rather than explicit flush() calls in Forest codebase.
📚 Learning: 2025-09-02T14:23:53.808Z
Learnt from: akaladarshi
PR: ChainSafe/forest#5923
File: src/tool/subcommands/api_cmd/test_snapshots.txt:206-252
Timestamp: 2025-09-02T14:23:53.808Z
Learning: In the Forest project, .rpcsnap.json.zst snapshot files listed in src/tool/subcommands/api_cmd/test_snapshots.txt are not stored in the repository itself but are downloaded from a DigitalOcean (DO) bucket when needed. The manifest file serves as an index/catalog of available snapshots.

Applied to files:

  • docs/docs/users/filecoin_services.md
📚 Learning: 2025-08-25T14:17:09.129Z
Learnt from: hanabi1224
PR: ChainSafe/forest#5978
File: .github/workflows/unit-tests.yml:0-0
Timestamp: 2025-08-25T14:17:09.129Z
Learning: hanabi1224's download_file_with_cache function in src/utils/net/download_file.rs preserves URL path structure in local cache directories by using cache_dir.join(url.path().strip_prefix('/').unwrap_or_else(|| url.path())), so snapshots from https://forest-snapshots.fra1.cdn.digitaloceanspaces.com/rpc_test/ are cached locally at ~/.cache/forest/test/rpc-snapshots/rpc_test/ (including the rpc_test subdirectory from the URL path).

Applied to files:

  • docs/docs/users/filecoin_services.md
  • src/cli_shared/snapshot.rs
📚 Learning: 2025-08-25T14:17:09.129Z
Learnt from: hanabi1224
PR: ChainSafe/forest#5978
File: .github/workflows/unit-tests.yml:0-0
Timestamp: 2025-08-25T14:17:09.129Z
Learning: hanabi1224's download_file_with_cache function preserves URL path structure in local cache directories, so snapshots from https://forest-snapshots.fra1.cdn.digitaloceanspaces.com/rpc_test/ are cached locally at ~/.cache/forest/test/rpc-snapshots/rpc_test (including the rpc_test subdirectory from the URL path).

Applied to files:

  • docs/docs/users/filecoin_services.md
📚 Learning: 2025-09-16T12:55:26.955Z
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.

Applied to files:

  • src/cli_shared/snapshot.rs
📚 Learning: 2025-08-08T12:10:45.218Z
Learnt from: hanabi1224
PR: ChainSafe/forest#5867
File: src/ipld/util.rs:553-558
Timestamp: 2025-08-08T12:10:45.218Z
Learning: Forest project targets Rust stable >=1.89; features stabilized in 1.88 like let-chains are acceptable in this codebase.

Applied to files:

  • src/cli_shared/snapshot.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
  • GitHub Check: Build Ubuntu
  • GitHub Check: cargo-publish-dry-run
  • GitHub Check: Build MacOS
  • GitHub Check: All lint checks
  • GitHub Check: tests
  • GitHub Check: tests-release
  • GitHub Check: Deploy to Cloudflare Pages
  • GitHub Check: Build forest binaries on Linux AMD64
  • GitHub Check: Check
🔇 Additional comments (2)
docs/docs/users/filecoin_services.md (1)

74-77: LGTM! Clear documentation update.

The documentation correctly references FRC-108 and provides simplified download instructions with the updated v2 snapshot paths. This aligns well with the code changes in src/cli_shared/snapshot.rs.

src/cli_shared/snapshot.rs (1)

136-136: LGTM! Correct migration from v1 to v2 snapshot path.

The URL update from latest-v1/calibnet to latest/calibnet correctly aligns with the v2 snapshot migration as documented in FRC-108.

@LesnyRumcajs LesnyRumcajs force-pushed the fix-forest-snapshot-link branch from da04d3d to 7bfc0d2 Compare October 30, 2025 13:23
@LesnyRumcajs LesnyRumcajs disabled auto-merge October 30, 2025 14:48
@LesnyRumcajs LesnyRumcajs added this pull request to the merge queue Oct 30, 2025
Merged via the queue into main with commit 4712a9f Oct 30, 2025
51 checks passed
@LesnyRumcajs LesnyRumcajs deleted the fix-forest-snapshot-link branch October 30, 2025 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants