-
Notifications
You must be signed in to change notification settings - Fork 47
feat(forks): iterative pparams migration for robust version upgrades #802
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this 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)
crates/cardano/src/forks.rs (2)
228-251: Per‑step match table aligns with era transitions; consider making the catch‑all more explicitThe explicit arms for (0→1)…(9→10) preserve the existing era transition semantics, now applied stepwise, which looks correct relative to the documented hard‑fork sequence. The catch‑all arm
(_, target) => intra_era_hardfork(¶ms, target)is convenient for forward compatibility, but it will also silently treat any unhandled(v, next)pair (including a potential mistake in the table) as a plain intra‑era bump. For consensus‑critical code you may want to constrain this a bit—for example, only allow the catch‑all oncevis at or above the highest known version, or at least emit awarn!with(v, next)so unexpected transitions are visible in logs.
257-267: force_pparams_version can now delegate to a single migrate_pparams_version callSince
migrate_pparams_versionnow handles arbitraryfrom → toupgrades by iterating internally, thefor from in from..toloop here effectively duplicates that logic. If you don’t plan to add per‑step invariant checks in this helper, you could simplify it to a single call:pub fn force_pparams_version( - initial: &PParamsSet, - genesis: &Genesis, - from: u16, - to: u16, -) -> Result<PParamsSet, BrokenInvariant> { - let mut pparams = initial.clone(); - - for from in from..to { - pparams = migrate_pparams_version(from, from + 1, &pparams, genesis); - } - - Ok(pparams) + initial: &PParamsSet, + genesis: &Genesis, + from: u16, + to: u16, +) -> Result<PParamsSet, BrokenInvariant> { + Ok(migrate_pparams_version(from, to, initial, genesis)) }Not required for this fix, but it removes duplication and makes the intended migration path clearer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/cardano/src/forks.rs(1 hunks)
🔇 Additional comments (1)
crates/cardano/src/forks.rs (1)
222-227: Iterative loop correctly chains migrations; clarifyfrom >= tobehaviorThe
while v < to { next = v + 1; ... v = next; }flow does what you want for non‑sequential upgrades (e.g. 1→3) by forcing each intermediate step starting from a clone ofcurrent, which should address the Vector Mainnet panic. One edge case: whenfrom >= tothe loop is skipped and you just return a clonedcurrent, which is probably intended forfrom == tobut might hide a caller bug if downgrades are not supposed to happen. Consider tightening the precondition with adebug_assert!(from <= to)or an early return/error forfrom > to. A small test/property that compares a multi‑step call (e.g. 1→9) against chaining single‑step calls would also help guard this logic.Also applies to: 252-255
Description:
This PR refactors migrate_pparams_version to handle non-sequential protocol version requests (e.g., requesting a migration from v1 directly to v3) by iteratively applying the necessary intermediate hard fork transitions.
Context / The Problem:
While syncing the Vector chain, Dolos encountered a panic at slot 864000:
not implemented: don't know how to bump from version 1 to 3.
Investigation showed that while the chain history itself is sequential (v1 -> v2 -> v3), Dolos's internal state machine seemingly missed the v2 update trigger. When the v3 transition occurred, it attempted to migrate from its last known state (v1) directly to the new target (v3).
The Solution:
Instead of panicking on non-adjacent version jumps, the migration logic is now iterative.
If asked to go from v1 to v3, it will:
This makes the PParams migration logic robust against missed internal updates or potential future chains that might genuinely skip versions.
Changes:
Testing:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.