State Diff Recorder + Guard

[ggwpez]

In migrations and tests it is sometimes desirable to know and restrict which storage keys change. Further it is nice to get a diff of all changes, if some unexpected changes happened.
For example a pallet migration would whitelist all storage keys with a specific prefix as "expected to change". Then later on when the migration finishes it is possible to assert that only exactly these keys changed. Otherwise a diff is printed and it panics.

Migration example of Pallet where OtherPallet can also change but nothing else:

let state_guard = StateDiffGuard::builder()
           .must_change(Prefix(Pallet::storage_prefix())
           .can_change(Prefix(OtherPallet::storage_prefix())
           .must_not_change(AnythingElse)
           .build();

// Run the migration with all the changes.

// Dropping will panic and print the *diff* iff there are unexpected changed.
sp_std::mem::drop(state_guard);

You could also try it without a builder and instead use single assert! statements, but then something like AnythingElse does not work anymore. The builder pattern helps here by holding a context.

cc @cor @kianenigma

[kianenigma]

Nice!

I see two general ways to go about this: either try and use a proof recorded with externalities, and try and extract the list of keys changed from the proof, although this might be a bit cumbersome. Perhaps we can use a custom proof recorded.

Or, implement alternative sp_io::storage implementations that track every key written to. But this will also have its own array of difficulties.

[ggwpez]

Or, implement alternative sp_io::storage implementations that track every key written to. But this will also have its own array of difficulties.

I think it is trivial to do via iterating sp_storage::next_key at the beginning and end of the test/migration.

[kianenigma]

Also we should have a guard that makes sure all storage is decodable. This is how I would go about it:

@@ -1737,8 +1737,24 @@ type Migrations = (
        pallet_nomination_pools::migration::v2::MigrateToV2<Runtime>,
        pallet_alliance::migration::Migration<Runtime>,
        pallet_contracts::Migration<Runtime>,
+       DecodeAll<Runtime>,
 );

+struct DecodeAll;
+impl OnRuntimeUpgrade for DecodeAll {
+       fn on_runtime_upgrade() -> Weight {
+               //
+       }
+
+       #[cfg(feature = "try-runtime")]
+       fn post_upgrade(_state: Vec<u8>) -> Result<(), &'static str> {
+               // - iterate metadata
+               // - list all storage keys
+               // - decode all of ths
+               Ok(())
+       }
+}

[ggwpez]

Also we should have a guard that makes sure all storage is decodable

This is what i created this issue for #241

[dastansam]

@ggwpez this is an interesting one, would like to work on it. I am guessing StorageNoopGuard be used as an inspiration, right?

[ggwpez]

I am guessing StorageNoopGuard be used as an inspiration, right?

Yea, but its rather small. It is a big design space, but starting small with a good foundation should be fine since it can then be extended later. Note that this is rather difficult to get right first try, but if you can do it then its great 😄
I did not mark it as mentor as i dont really know how/what to do either, so its a bit of trial and error.

[dastansam]

I am guessing StorageNoopGuard be used as an inspiration, right?

Yea, but its rather small. It is a big design space, but starting small with a good foundation should be fine since it can then be extended later. Note that this is rather difficult to get right first try, but if you can do it then its great 😄 I did not mark it as mentor as i dont really know how/what to do either, so its a bit of trial and error.

yeah, I see. I was thinking of writing something that simply checks if must change prefixes are actually changed. And then gradually tackle the AnythingElse and can_change cases, and the difference. This should be simple enough, no? Or maybe I am missing something?

[ggwpez]

yeah, I see. I was thinking of writing something that simply checks if must change prefixes are actually changed. And then gradually tackle the AnythingElse and can_change cases, and the difference. This should be simple enough, no? Or maybe I am missing something?

Yes sounds fine. One way would be to take a snapshot of the storage and then compare it. Maybe creating a storage transaction could also work 🤔 yea just try it out a bit.