Kusama state version switch and migration. #7015
Conversation
cheme
added
A0-please_review
cheme
added
E1-database_migration
| use sp_std::prelude::*; | ||
|
|
||
| /// Initialize an automatic migration process. | ||
| pub struct InitMigrate; |
There was a problem hiding this comment.
You need to add it to the list of migrations.
|
Do we have confirmation this is fully compatible with all the ecosystem tooling? e.g smoldot and alternative polkadot clients |
|
Everything that can run Rococo/Westend is already compatible with this change as state version V1 is already running there. Smoldot is also ready AFAIK. |
Yes without privileged account (could be an idea to be able to emergency switch off but that would mean giving a specific power to some account/owner, a bit awkward in my opinion). |
|
In what case would you think that we need an emergency stop? |
Bug, but not much value in it (as can only stop if the blocks are still processing). |
|
@cheme please merge master. |
| type SignedFilter = frame_support::traits::NeverEnsureOrigin<AccountId>; | ||
|
|
||
| // Use same weights as substrate ones. | ||
| type WeightInfo = pallet_state_trie_migration::weights::SubstrateWeight<Runtime>; |
There was a problem hiding this comment.
Would be more appropriate to benchmark it properly in this runtime.
There was a problem hiding this comment.
yes, not sure how much change it will be to do so (in theory there is no reason to see significant change: only wasm bin size may interfere a tiny bit maybe).
| type Currency = Balances; | ||
| type SignedDepositPerItem = MigrationSignedDepositPerItem; | ||
| type SignedDepositBase = MigrationSignedDepositBase; | ||
| type ControlOrigin = EnsureRoot<AccountId>; |
There was a problem hiding this comment.
Sure? it would be hard to acquire.
There was a problem hiding this comment.
I am not very comfortable with having a privilege account.
Actually I recently reconsider bringing back the inherent version of the migration (the main issue was the code was a bit everywhere and less cleanly separated, but looking backward, it would have really feel more comfortable to me).
runtime/kusama/src/lib.rs
Outdated
| if MigrationProcess::<Runtime>::get() == Default::default() && | ||
| AutoLimits::<Runtime>::get().is_none() | ||
| { | ||
| AutoLimits::<Runtime>::put(Some(MigrationLimits { item: 160, size: 204800 })); |
There was a problem hiding this comment.
Some comment about how this number is chosen would be good.
runtime/kusama/src/lib.rs
Outdated
| migrations::V0941, | ||
| migrations::V0942, | ||
| migrations::Unreleased, | ||
| init_state_migration::InitMigrate, |
There was a problem hiding this comment.
Is there a reason why not to move that into migrations::Unreleased,?
migrations::Unreleased migrations do run, we just don't know yet in which of the upcoming Version it will be included.
At release time, merged Unreleased migrations will moved to a new VXXXX migration set.
There was a problem hiding this comment.
👍 I will do, I just merged to master a bit too fast.
(currently I am trying to get better limit but encounter some limitation with try runtime, but will certainly get new limits soon).
cheme
changed the title
runtime/kusama/src/lib.rs
Outdated
| pub type Migrations = | ||
| (migrations::V0940, migrations::V0941, | ||
| migrations::V0942, migrations::Unreleased); |
There was a problem hiding this comment.
I am no fan of what rustfmt does here.
The list you had before is much more readable.
I would suggest adding #[rustfmt_skip] for this Migrations type as well as for Unreleased and keep one migration / version per line. That also helps with your comment about having this single migration in a release.
runtime/kusama/src/lib.rs
Outdated
| pub type Unreleased = (init_state_migration::InitMigrate,); | ||
| } |
There was a problem hiding this comment.
Your comment:
It may be pertinent to avoid adding this in a runtime upgrade that already have others migration.
would be suited here so whoever does the move from Unreleased to VNext is aware of it.
We can then drop the comment.
|
@bkchr @kianenigma I did change the limits (see https://forum.polkadot.network/t/state-trie-migration/852/11). |
| @@ -2405,7 +2405,11 @@ mod init_state_migration { | |||
| if MigrationProcess::<Runtime>::get() == Default::default() && | |||
| AutoLimits::<Runtime>::get().is_none() | |||
| { | |||
| AutoLimits::<Runtime>::put(Some(MigrationLimits { item: 160, size: 204800 })); | |||
| // We use limits to target 600ko proofs per block and | |||
| // avg 800_000_000_000 of weight per block. | |||
There was a problem hiding this comment.
Can lower to 600 (using 3200 instead of 4800 move to 510 blocks the migration time so not taking really longer).
Could make sense to calculate better weight as I believe it is an overestimate (iterating on data since iterator improvement may be cheaper than random data access).
There was a problem hiding this comment.
800ms sounds good! For some reason I thought we measure ref time in nanoseconds, but ty @crystalin
|
@cheme what is the status here? |
|
Forgot to merge the PR :( , it was totally ready iirc, will rebase and merge. |
|
bot merge |
|
Waiting for commit status. |
|
Merge cancelled due to error. Error: Statuses failed for 9f4c073 |
|
bot merge |
|
Error: Statuses failed for 18cc436 |
|
bot merge |
|
ok, migration is pending in unreleased. @chevdor is it enought to get included in next runtime build? |
yes |
Ok, if for any reason it ends up not being included in next runtime, then it would be better if: polkadot/runtime/kusama/src/lib.rs Line 143 in 032d3eb stays at 0 (it breaks warpsync). |
|
This pull request has been mentioned on Polkadot Forum. There might be relevant details there: https://forum.polkadot.network/t/state-trie-migration/852/13 |
This PR adds state-migration pallet to kusuma to allow switch to trie state version 1.
It should use the same configuration as the ones previously used in westend (#6336)
It may be pertinent to avoid adding this in a runtime upgrade that already have others migration.