-
Notifications
You must be signed in to change notification settings - Fork 141
Remove StateTrieMigration pallet - V0→V1 migration complete #1040
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
Update value of MigrationMaxKeyLen to proceed with migration via bot on both KAH and PAH
Set ControlOrigin to MigController
Migration limit could be set either by fellowship or MigController
The state trie migration from V0 to V1 format has been completed on: - Polkadot relay chain (2,192,421 top items + 10,423 child items) - Asset Hub Polkadot (5,448,606 top items) - Asset Hub Kusama (288,016 top items) This PR removes the pallet-state-trie-migration from these runtimes and adds RemovePallet migrations to clean up the storage. Closes polkadot-fellows#74 (for Polkadot ecosystem chains)
1a650d3 to
0d02e8e
Compare
|
Review required! Latest push from author must always be reviewed |
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.
Found 3 runtime-safety issues: (1) unreachable!() introduced in runtime conversion paths (panic risk), (2) potential SCALE decoding / runtime failure risk if any StateTrieMigration hold reasons still exist in on-chain Balances::Holds, and (3) minor CHANGELOG formatting noise (blank lines/whitespace).
Suggestions that couldn't be attached to a specific line
relay/polkadot/src/ah_migration/phase1.rs:137, 156-157
Removal of the StateTrieMigration match arms from call filtering and (more importantly) from RuntimeHoldReason portable conversion increases the risk of a runtime-breaking upgrade if any on-chain Balances::Holds entries (or other storage/XCM payloads) still encode the StateTrieMigration hold reason. RemovePallet only clears the pallet's own storage prefix; it does not clear holds stored under pallet_balances. Actionable mitigation: (a) add/confirm a dedicated runtime migration that scans pallet_balances::Holds and removes any StateTrieMigration holds (or asserts none exist in try-runtime pre/post upgrade), and/or (b) keep the enum variant for at least one release after removing the pallet to maintain decoding compatibility.
| PortableHoldReason::StateTrieMigration(_) => | ||
| unreachable!("StateTrieMigration pallet removed - no holds should exist"), |
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.
PortableHoldReason::StateTrieMigration(_) => unreachable!(...) will panic if that variant is ever encountered. Even if "should never happen", a panic in runtime code is consensus-critical (can abort block execution). Replace this with non-panicking handling: e.g., change the conversion to TryFrom returning Result<RuntimeHoldReason, Error> and make callers handle/propagate the error, or (safer for backwards compatibility) keep a RuntimeHoldReason::StateTrieMigration variant (even if the pallet is removed) so old data/messages can still decode without panicking.
| PortableHoldReason::StateTrieMigration(_) => | ||
| unreachable!("StateTrieMigration pallet removed - no holds should exist"), |
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.
Same issue as Asset Hub Kusama: unreachable!() in a runtime conversion is a panic path. If any stale/malicious/old-format data causes PortableHoldReason::StateTrieMigration to be processed, the runtime can panic. Prefer a non-panicking conversion (TryFrom) or preserve a compatible enum variant and map it safely.
| ### Removed | ||
|
|
||
| - Remove StateTrieMigration pallet from Polkadot relay chain, Asset Hub Polkadot, and Asset Hub Kusama - V0→V1 state trie migration complete ([polkadot-fellows/runtimes#1040](https://github.com/polkadot-fellows/runtimes/pull/1040)) | ||
|
|
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.
The new entry under ### Removed includes extra blank lines (and appears to introduce whitespace-only lines). This tends to trip markdown formatting/linting and makes the changelog noisier. Remove the empty lines and ensure the bullet is directly under the heading (no whitespace-only lines).
| StateTrieMigrationPalletName, | ||
| <Runtime as frame_system::Config>::DbWeight, | ||
| >, | ||
| KickOffAhm<Runtime>, |
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.
can you also remove this one (KickOffAhm)?
| PortableHoldReason::StateTrieMigration(_) => | ||
| unreachable!("StateTrieMigration pallet removed - no holds should exist"), |
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.
why is there still an enum variant for StateTrieMigration without the pallet? can we remove completely?
and even if not, please remove the unreachable!() - it's a footgun if we later add back the pallet and miss updating this
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.
What prevents us from removing the entire ah_migration.mod? Then we do not have to care about this issue.
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.
why is there still an enum variant for
StateTrieMigrationwithout the pallet?
@nkpar we need to clean it up here:
runtimes/pallets/rc-migrator/src/types.rs
Line 654 in b31ce90
| StateTrieMigration(pallet_state_trie_migration::HoldReason), |
not sure how this enum is used, but better add there indexes:
pub enum PortableHoldReason {
+ #[codec(index = 0)]
Preimage(pallet_preimage::HoldReason),
+ #[codec(index = 1)]
Staking(pallet_staking::HoldReason),
- #[codec(index = 2)]
- StateTrieMigration(pallet_state_trie_migration::HoldReason),
+ #[codec(index = 3)]
DelegatedStaking(pallet_delegated_staking::HoldReason),
+ #[codec(index = 4)]
Session(pallet_session::HoldReason),
+ #[codec(index = 5)]
XcmPallet(pallet_xcm::HoldReason),
}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.
| PortableHoldReason::StateTrieMigration(state_trie_migration) => | ||
| StateTrieMigration(state_trie_migration), | ||
| PortableHoldReason::StateTrieMigration(_) => | ||
| unreachable!("StateTrieMigration pallet removed - no holds should exist"), |
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.
same comment as for AHK, don't like the unreachable
| "pallet-staking-async", | ||
| "pallet-staking-async-rc-client", | ||
| "pallet-staking-runtime-api", | ||
| "pallet-state-trie-migration", |
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.
@nkpar I would also remove pallet-state-trie-migration = { workspace = true } from:
runtimes/pallets/ah-migrator/Cargo.toml
Line 39 in b31ce90
| pallet-state-trie-migration = { workspace = true } |
runtimes/pallets/rc-migrator/Cargo.toml
Line 47 in b31ce90
| pallet-state-trie-migration = { workspace = true } |
and from the main Cargo.toml:
Line 174 in b31ce90
| pallet-state-trie-migration = { version = "49.0.0", default-features = false } |
kianenigma
left a 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.
Existing comments should be addressed, otherwise a great cleanup to, wishing to see more of such PRs to remove legacy stuff.
Summary
The state trie migration from V0 to V1 format has been completed on all Polkadot ecosystem chains:
This PR removes the
pallet-state-trie-migrationfrom these runtimes and addsRemovePalletmigrations to clean up the storage.Changes
For each runtime:
pallet-state-trie-migrationdependency fromCargo.tomlimpl pallet_state_trie_migration::Config)construct_runtime!macroRemovePallet<StateTrieMigrationPalletName, DbWeight>migrationMigControlleraccounts and tests (Asset Hubs only)Reference
Test Plan
Closes #74 (for Polkadot ecosystem chains)