feat: nv27 skeleton & base migration #6023
Conversation
WalkthroughAdds NV27 (GoldenWeek) upgrade: new Height variant and NetworkVersion V27, network HEIGHT_INFOS entries, NV27 migration module and run_migration implementation, verifier/manifest loading and per-actor migrators, a placeholder logo hook, and commented-out migration wiring in global maps. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Operator
participant ChainConfig
participant Blockstore
participant Verifier
participant StateMigration
participant StateTree
Operator->>ChainConfig: lookup manifest CID at Height::GoldenWeek
ChainConfig-->>Operator: manifest CID
Operator->>Blockstore: fetch manifest by CID
Blockstore-->>Operator: manifest bytes
Operator->>Verifier: create & load manifest
Verifier-->>Operator: verified manifest
Operator->>StateMigration: init with verifier
StateMigration->>StateTree: load state from root CID
StateMigration->>StateMigration: register per-actor migrators (nil/system)
StateMigration->>StateTree: perform migration for epoch
StateTree-->>StateMigration: new state root CID
StateMigration-->>Operator: return new state root CID
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
be2e86a to
a79b511
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
src/networks/butterflynet/mod.rs (1)
111-113: Butterfly GoldenWeek scaffold is consistent; verify bundle presence.
i64::MAXplaceholder matches other networks’ use of max‐height.- Confirm the
v16.0.1bundle is published for Butterfly (or update once NV27 actors land).Optionally extract the version into a constant to avoid repeating the literal:
- make_height!(GoldenWeek, i64::MAX, get_bundle_cid("v16.0.1")), + make_height!(GoldenWeek, i64::MAX, get_bundle_cid(GOLDEN_WEEK_BUNDLE)),const GOLDEN_WEEK_BUNDLE: &str = "v16.0.1";src/state_migration/nv27/mod.rs (1)
7-11: Drop unnecessary allow if possible.
#[allow(unused_imports)]around the re-export likely isn’t needed; remove unless CI lints require it.-#[allow(unused_imports)] pub use migration::run_migration;src/state_migration/nv27/migration.rs (3)
73-76: Drop the redundant existence check; add context to manifest load
blockstore.get(manifest_cid)is redundant sinceload_manifestwill read/validate. Remove the extra IO and attach context on failure.- blockstore.get(new_manifest_cid)?.context(format!( - "manifest for network version NV27 not found in blockstore: {new_manifest_cid}" - ))?; - - // Add migration specification verification - let verifier = Arc::new(Verifier::default()); - - let new_manifest = BuiltinActorManifest::load_manifest(blockstore, new_manifest_cid)?; + // Add migration specification verification + let verifier = Arc::new(Verifier::default()); + + let new_manifest = BuiltinActorManifest::load_manifest(blockstore, new_manifest_cid) + .with_context(|| format!("failed to load NV27 manifest {new_manifest_cid}"))?;Also applies to: 80-80
65-72: Clarify errors by naming the height explicitlyMinor copy tweak to reduce ambiguity when debugging config: mention GoldenWeek alongside NV27.
- .context("no height info for network version NV27")? + .context("no GoldenWeek (NV27) height info")? .bundle .as_ref() - .context("no bundle for network version NV27")?; + .context("no GoldenWeek (NV27) bundle")?;
24-52: Good scaffolding; consider adding a targeted testNice, minimal wiring that maps old→new codes and special-cases System. Consider a unit/integration test that asserts: (a) System actor uses the dedicated migrator, (b) all other builtins are registered with
nil_migrator.I can draft a small test harness that builds a fake manifest and inspects the migrator map if the API allows exposure, or run a dry-run migration against a tiny in-memory tree. Want me to open an issue and provide a test skeleton?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
src/networks/butterflynet/mod.rs(1 hunks)src/networks/calibnet/mod.rs(1 hunks)src/networks/devnet/mod.rs(1 hunks)src/networks/mainnet/mod.rs(1 hunks)src/networks/mod.rs(4 hunks)src/rpc/methods/state.rs(2 hunks)src/shim/version.rs(1 hunks)src/state_migration/mod.rs(4 hunks)src/state_migration/nv27/migration.rs(1 hunks)src/state_migration/nv27/mod.rs(1 hunks)src/utils/misc/logo.rs(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 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-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/networks/devnet/mod.rs
🧬 Code graph analysis (4)
src/state_migration/nv27/migration.rs (1)
src/shim/machine/manifest.rs (2)
load_v1_actor_list(132-160)load_manifest(123-131)
src/state_migration/nv27/mod.rs (1)
src/state_migration/nv27/migration.rs (1)
run_migration(56-89)
src/state_migration/mod.rs (1)
src/state_migration/nv27/migration.rs (1)
run_migration(56-89)
src/shim/version.rs (1)
src/interpreter/vm.rs (1)
new(175-271)
⏰ 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 forest binaries on Linux AMD64
- GitHub Check: tests-release
- GitHub Check: tests
- GitHub Check: cargo-publish-dry-run
- GitHub Check: Build MacOS
- GitHub Check: All lint checks
- GitHub Check: Build Ubuntu
- GitHub Check: Analyze (rust)
- GitHub Check: Analyze (go)
🔇 Additional comments (12)
src/networks/devnet/mod.rs (1)
167-172: GoldenWeek: confirm bundle version and introduce constant
- Confirm that using
"v16.0.1"for the NV27 scaffold is intentional and tracked under forest#5987.- To avoid hardcoded strings, add at the top of
src/networks/devnet/mod.rs(near other constants):and update the snippet to:const GOLDEN_WEEK_BUNDLE: &str = "v16.0.1";- make_height!( - GoldenWeek, - get_upgrade_height_from_env("FOREST_GOLDEN_WEEK_HEIGHT").unwrap_or(9999999999), - get_bundle_cid("v16.0.1") - ), + make_height!( + GoldenWeek, + get_upgrade_height_from_env("FOREST_GOLDEN_WEEK_HEIGHT").unwrap_or(9999999999), + get_bundle_cid(GOLDEN_WEEK_BUNDLE) + ),src/shim/version.rs (1)
69-70: Approve V27 additionV27 constant addition is correct and minimal; mapping for Height::GoldenWeek ⇒ NetworkVersion::V27 in src/networks/mod.rs is present.
src/rpc/methods/state.rs (1)
2962-2963: Regenerate OpenRPC schema and coordinate client updates
- Confirmed addition of
upgrade_golden_week_heighttoForkUpgradeParams(struct,TryFrom<ChainConfig>impl) and its presence inopenrpc.json.- Add a CI step to auto-regenerate OpenRPC specs upon schema changes and update downstream clients/OpenRPC docs accordingly.
src/networks/mainnet/mod.rs (1)
93-95: GoldenWeek bundle version is correct
v16.0.1 is defined in actors_bundle.rs and used consistently across all networks; retain the existing TODO for bumping when NV27 is finalized.src/utils/misc/logo.rs (1)
34-35: Wire-up for NV27 looks correct.Adding the V27 arm to route to the Golden Week logo path is consistent with prior versions.
src/networks/calibnet/mod.rs (1)
96-98: Calibnet v16.0.1 bundle metadata present — no action required.src/networks/mod.rs (1)
174-175: New Height::GoldenWeek variant added.Enum extension is straightforward and consistent with existing heights.
src/state_migration/mod.rs (1)
30-31: NV27 module registered; migrations intentionally disabled per-network.This keeps NV27 dormant until ready, which is safe. Once you flip these on, the logo path will trigger; ensure the Golden Week logo function is non-panicking.
Also applies to: 50-52, 68-70, 74-76, 88-90
src/state_migration/nv27/mod.rs (2)
14-18: Confirm NV27 system state types (v16 -> v16) are correct.Using v16 for both sides is likely a temporary stub (see TODO 5985). Please verify the intended source/target system state versions for NV27 before enabling.
20-21: System/verifier scaffolding LGTM.Macros are correctly invoked for the NV27 module.
src/state_migration/nv27/migration.rs (2)
54-62: API surface looks rightPublic
run_migrationsignature and return type are consistent with prior upgrades and keep the caller’s responsibilities clear.
85-86: I’ll inspect the definition and variants of StateTreeVersion to confirm whether V5 is still correct for NV27.
|
@LesnyRumcajs Some tests are failing |
Oh yeah, I forgot to use my brain during conflict resolution. It should be fine now. |
Summary of changes
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes #5984
Other information and links
Change checklist
Summary by CodeRabbit
New Features
Tests
Chores
Style