Skip to content

Conversation

@akaladarshi
Copy link
Collaborator

@akaladarshi akaladarshi commented Sep 25, 2025

Summary of changes

Changes introduced in this pull request:

  • Updates the snapshot links to point to latest versions (This is so CI checks can pass)

Reference issue to close (if applicable)

Part of the #6115

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 v1 snapshot downloads now target the correct v1 endpoint, improving snapshot retrieval and import reliability.
  • Tests

    • Test harness updated to use the Calibnet v1 snapshot endpoint and support parameterized migration scenarios.
  • Chores

    • Migration workflow and defaults updated (new initial version), container runtime paths aligned, and extra migration validation steps added for robustness.

@akaladarshi akaladarshi requested a review from a team as a code owner September 25, 2025 07:56
@akaladarshi akaladarshi requested review from LesnyRumcajs and sudo-shashank and removed request for a team September 25, 2025 07:56
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 25, 2025

Walkthrough

Change Calibnet snapshot source to the v1 archive URL and update the Calibnet DB migration test: parameterize initial Forest version (set to 0.30.0), adjust Docker mount path, add migration orchestration flags and post-migration checks; no public API changes.

Changes

Cohort / File(s) Summary of Changes
Calibnet snapshot URL update
scripts/tests/harness.sh, src/cli_shared/snapshot.rs
Replace Forest Calibnet snapshot base URL from https://forest-archive.chainsafe.dev/latest/calibnet/ to https://forest-archive.chainsafe.dev/latest-v1/calibnet (trailing slash removed). Snapshot download/import flow unchanged.
Calibnet DB migration script
scripts/tests/calibnet_db_migration.sh
Generalize migration test to use FOREST_INIT_VERSION (updated to 0.30.0), change docker volume mount from /home/forest/.local/share/forest to /root/.local/share/forest, add --auto-download-snapshot and --halt-after-import flags, create config.toml for old data, run current Forest to trigger migrations, perform post-migration API validation, and assert removal of the old-version directory.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant TestScript as Test Script
  participant DockerOld as Old Forest (container)
  participant FS as Host FS
  participant CurrentForest as Current Forest (container)
  participant API as Local API

  Note over TestScript: Setup old DB using FOREST_INIT_VERSION
  TestScript->>DockerOld: run old forest with mounted DB (/root/.local/share/forest)
  DockerOld-->>FS: writes old DB directory
  TestScript->>CurrentForest: run current forest with config.toml pointing to old DB\n(flags: --auto-download-snapshot, --halt-after-import)
  CurrentForest->>FS: import/upgrade DB (migration)
  CurrentForest->>API: start API for post-migration checks
  API-->>TestScript: migration status / validate data
  TestScript->>FS: assert old-version directory removed
  Note over TestScript: End of migration validation
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • sudo-shashank
  • hanabi1224

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title succinctly describes the core change of updating the snapshot links to use the latest Calibnet snapshot and clearly follows conventional commit conventions, making it easy for reviewers to understand the main intent.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch akaladarshi/fix-snapshot-script

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

@akaladarshi akaladarshi added the RPC requires calibnet RPC checks to run on CI label Sep 25, 2025
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-138: Optional: append trailing slash to latest-v1/calibnet and align all references
Both https://…/latest-v1/calibnet and with a trailing slash return 200, but adding / avoids a potential redirect and matches the mainnet/ URL.

 src/cli_shared/snapshot.rs
-const FOREST_CALIBNET_COMPRESSED: &str =
-    "https://forest-archive.chainsafe.dev/latest-v1/calibnet";
+const FOREST_CALIBNET_COMPRESSED: &str =
+    "https://forest-archive.chainsafe.dev/latest-v1/calibnet/";

Also update the URL in scripts/tests/harness.sh (line 32) and the README entry under “Forest snapshot links” to include the trailing slash.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a68b350 and 98ce74b.

📒 Files selected for processing (2)
  • scripts/tests/harness.sh (1 hunks)
  • src/cli_shared/snapshot.rs (1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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:

  • 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). (7)
  • GitHub Check: cargo-publish-dry-run
  • GitHub Check: Build MacOS
  • GitHub Check: Build Ubuntu
  • GitHub Check: All lint checks
  • GitHub Check: tests
  • GitHub Check: tests-release
  • GitHub Check: Build forest binaries on Linux AMD64
🔇 Additional comments (1)
scripts/tests/harness.sh (1)

32-32: Calibnet v1 snapshot URL switch looks good.

This aligns the harness with the new archive endpoint and matches the F3 merge flow.

Optionally verify the endpoint resolves to a .forest.car.zst with sane headers and redirects:

@akaladarshi
Copy link
Collaborator Author

@LesnyRumcajs I guess for db-migration-checks CI we will need to update the docker image to point to latest snapshot as it uses (auto-download-snapshot).

@LesnyRumcajs
Copy link
Member

@LesnyRumcajs I guess for db-migration-checks CI we will need to update the docker image to point to latest snapshot as it uses (auto-download-snapshot).

I think we can actually be more pragmatic. We don't expect anyone to run Forest pre 0.30.0 due to the recent network upgrades. Let's just bump the tag to v0.30.0 and be done with it.

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 (2)
scripts/tests/calibnet_db_migration.sh (2)

4-5: Align migration comment with bumped base version.

The header comment still states we migrate from 0.19.2, but the script now seeds with 0.30.0. Please update the wording so future readers aren’t misled.

-# This script tests the migration(s) from Forest 0.19.2 to the current version.
+# This script tests the migration(s) from Forest 0.30.0 to the current version.

16-23: Refresh inline comment to reflect FOREST_INIT_VERSION.

Same issue here: the narrative still references 0.19.2 while we now launch 0.30.0. Tweaking the comment keeps the instructions accurate.

-# Run Forest 0.19.2 with mounted db so that we can re-use it later.
+# Run Forest ${FOREST_INIT_VERSION} with a mounted db so that we can re-use it later.
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 98ce74b and 20b804f.

📒 Files selected for processing (1)
  • scripts/tests/calibnet_db_migration.sh (1 hunks)
⏰ 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). (7)
  • GitHub Check: tests-release
  • GitHub Check: tests
  • GitHub Check: cargo-publish-dry-run
  • GitHub Check: Build Ubuntu
  • GitHub Check: Build MacOS
  • GitHub Check: All lint checks
  • GitHub Check: Build forest binaries on Linux AMD64

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)
scripts/tests/calibnet_db_migration.sh (1)

14-19: Refresh baseline version references in comments.

We now seed the DB with v0.30.0, but the surrounding comments still talk about 0.19.2. Keeping them in sync avoids future confusion while debugging CI runs.

-# This script tests the migration(s) from Forest 0.19.2 to the current version.
+# This script tests the migration(s) from Forest 0.30.0 to the current version.-# Run Forest 0.19.2 with mounted db so that we can re-use it later.
+# Run Forest 0.30.0 with mounted db so that we can re-use it later.-# Assert there is no "0.19.2" directory in the database directory. This and a successful sync indicate that the database was successfully migrated.
+# Assert there is no "0.30.0" directory in the database directory. This and a successful sync indicate that the database was successfully migrated.
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 20b804f and aece8ad.

📒 Files selected for processing (1)
  • scripts/tests/calibnet_db_migration.sh (1 hunks)
⏰ 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). (7)
  • GitHub Check: Build MacOS
  • GitHub Check: cargo-publish-dry-run
  • GitHub Check: Build Ubuntu
  • GitHub Check: Build forest binaries on Linux AMD64
  • GitHub Check: All lint checks
  • GitHub Check: tests
  • GitHub Check: tests-release

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

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_db_migration.sh (1)

67-71: Version parsing will fail on release builds without ‘+’

sed -E 's/.* (.*)\+.*/\1/' only works when forest --version contains a +… suffix. On plain releases it yields the full string (e.g., “forest 0.31.0”) and breaks the path check.

-# Get current Forest version
-CURRENT_VERSION=$(forest --version | sed -E 's/.* (.*)\+.*/\1/')
+# Get current Forest version (strip any +metadata suffix)
+CURRENT_VERSION=$(forest --version | awk '{print $NF}' | cut -d+ -f1)
🧹 Nitpick comments (4)
scripts/tests/calibnet_db_migration.sh (4)

14-14: Allow overriding FOREST_INIT_VERSION via env

Hardcoding makes the job less flexible. Default to 0.30.0 but let CI override.

-FOREST_INIT_VERSION="0.30.0"
+FOREST_INIT_VERSION="${FOREST_INIT_VERSION:-0.30.0}"

16-23: Volume path may differ across images; mount both root and non-root locations

Older images may run as user “forest” with HOME=/home/forest while newer run as root. Dual‑mount avoids path drift and removes ambiguity.

 docker run --init --rm --name forest-${FOREST_INIT_VERSION} \
-  --volume "${DATA_DIR}":/root/.local/share/forest \
+  --volume "${DATA_DIR}":/root/.local/share/forest \
+  --volume "${DATA_DIR}":/home/forest/.local/share/forest \
   ghcr.io/chainsafe/forest:v${FOREST_INIT_VERSION} \
   --chain calibnet \
   --encrypt-keystore false \
   --auto-download-snapshot \
   --halt-after-import

Optional: add --pull=always to avoid stale tag usage.


33-35: Don’t assume sudo availability

GH-hosted runners have sudo, but self-hosted may not. Fallback to plain chown when sudo is absent.

-if [ ! -w "${DATA_DIR}/calibnet/${FOREST_INIT_VERSION}" ]; then
-  sudo chown -R "$(id -u):$(id -g)" "${DATA_DIR}/"
-fi
+if [ ! -w "${DATA_DIR}/calibnet/${FOREST_INIT_VERSION}" ]; then
+  if command -v sudo >/dev/null 2>&1; then
+    sudo chown -R "$(id -u):$(id -g)" "${DATA_DIR}/"
+  else
+    chown -R "$(id -u):$(id -g)" "${DATA_DIR}/"
+  fi
+fi

45-59: Ensure background forest is cleaned up

Add a trap to kill background processes to avoid leaks on failure.

 /usr/bin/time -v forest --chain calibnet --log-dir "$LOG_DIRECTORY" --save-token ./admin_token --config "${CONFIG_FILE}" &
+trap 'kill $(jobs -p) 2>/dev/null || true' EXIT
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between aece8ad and 58fae1e.

📒 Files selected for processing (1)
  • scripts/tests/calibnet_db_migration.sh (3 hunks)
⏰ 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). (7)
  • GitHub Check: Build MacOS
  • GitHub Check: Build Ubuntu
  • GitHub Check: cargo-publish-dry-run
  • GitHub Check: All lint checks
  • GitHub Check: tests-release
  • GitHub Check: tests
  • GitHub Check: Build forest binaries on Linux AMD64
🔇 Additional comments (1)
scripts/tests/calibnet_db_migration.sh (1)

61-65: LGTM: assert old-version dir removal

This check is a good guard to confirm migration completed and cleanup happened.

If migrations intentionally retain a tombstone/marker in older versions, consider checking for a non-empty directory instead: [ -d dir ] && [ -z "$(ls -A dir)" ].

@LesnyRumcajs LesnyRumcajs added this pull request to the merge queue Sep 25, 2025
Merged via the queue into main with commit ac8cd71 Sep 25, 2025
40 checks passed
@LesnyRumcajs LesnyRumcajs deleted the akaladarshi/fix-snapshot-script branch September 25, 2025 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

RPC requires calibnet RPC checks to run on CI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants