MBM try-runtime support
#4251
MBM try-runtime support
#4251
Conversation
liamaharon
added
the
T1-FRAME
|
The CI pipeline was cancelled due to failure one of the required jobs. |
| @@ -460,6 +460,11 @@ impl MultiStepMigrator for MockedModeGetter { | |||
| fn step() -> Weight { | |||
| Weight::zero() | |||
| } | |||
|
|
|||
| #[cfg(feature = "try-runtime")] | |||
| fn try_mbms() -> Result<(), sp_runtime::TryRuntimeError> { | |||
There was a problem hiding this comment.
We used this approach a lot in the past, but i think it can cause some difficult to debug issues.
Maybe we can try this time to use an extension trait, like trait MultiStepMigratorTryRuntime : MultiStepMigrator that has this function, and then requiring that trait instead.
You think it could work?
There was a problem hiding this comment.
What kinds of issues are you referring to and what use case is there?
My initial reaction is I'm not sure about it, because MultiStepMigratorTryRuntime would just be MultiStepMigrator with the try-runtime feature enabled.
| @@ -649,6 +685,16 @@ impl SteppedMigrations for () { | |||
| None | |||
| } | |||
|
|
|||
| #[cfg(feature = "try-runtime")] | |||
| fn nth_pre_upgrade(_n: u32) -> Result<Vec<u8>, sp_runtime::TryRuntimeError> { | |||
| Ok(Vec::new()) | |||
There was a problem hiding this comment.
I think normally it still wraps the return value in an Option, in case that the index is out of bounds.
Why is it different here?
There was a problem hiding this comment.
Good point, I'll update it
| log::debug!("Block {}", System::<T>::block_number()); | ||
| System::<T>::set_block_number(System::<T>::block_number() + 1u32.into()); | ||
| System::<T>::on_initialize(System::<T>::block_number()); | ||
| crate::Pallet::<T>::on_initialize(System::<T>::block_number()); |
There was a problem hiding this comment.
Not sure if we need these. The step could also be called multiple times per block i think.
| <frame_system::Pallet<T>>::on_finalize(System::<T>::block_number()); | ||
|
|
||
| // Check events for `UpgradeCompleted` or `UpgradeFailed`. | ||
| for e in System::<T>::events().iter() { |
There was a problem hiding this comment.
| for e in System::<T>::events().iter() { | |
| for e in frame_system::Events::<T>::take().into_iter() { |
Should make it a bit faster to not hold old events.
| /// | ||
| /// Returns some bytes which are passed into `post_upgrade` after the migration is completed. | ||
| #[cfg(feature = "try-runtime")] | ||
| fn pre_upgrade() -> Result<Vec<u8>, sp_runtime::TryRuntimeError> { |
There was a problem hiding this comment.
Same comment here about possible extension trait instead of feature gate.
Dont know if it is really better, but could try.
There was a problem hiding this comment.
I'm not sure about creating an extension trait that just adds some feature gated methods, what's the use case?
Generally I like to keep code simple with minimal indirection, only adding complexity and indirection when there is tangible benefit.
|
I remember that you asked somewhere which approach to take. I would take the approach of building multiple blocks until the MBMs are finished. I know that this is more complicated, but this directly checks that your runtime can handle this properly and also that you stay always in the POV limits. (Didn't check the pr yet :D) |
Here's the discussion paritytech/try-runtime-cli#17 (comment) I missed that Kian also supports building proper blocks. Although more involved, I will proceed with that approach :) |
UpgradeCheckSelectenum with a struct implementing the builder pattern,TryOnRuntimeUpgradeOpts. This allows much easier control over which checks to run intry_on_runtime_upgradetry-runtimegated methodspre_upgradeandpost_upgradeonSteppedMigrationtry-runtimegated methodsnth_pre_upgradeandnth_post_upgradeonSteppedMigrationstry-runtimegated methodtry_mbmstoMultiStepMigrator, which execs thepre_upgrade, steps, andpost_upgrades of all MBMs.Executivetry_on_runtime_upgradeto calltry_mbmsif the option is selected.TODO
try-runtime-cli, and release a pre-release version that uses the newTryOnRuntimeUpgradeOpts