[FRAME] State Diff Guard

[dastansam]

Motivation

Adds a guard that prevents unexpected storage changes. StateDiffGuard:

• initializes map of prefixes to the mutation policy (can/not change)
• upon initialization takes the snapshot of the current state
• when dropped, iterates every storage key, gets its mutation policy, defaulting to AnythingElse if present.
• applies the mutation policy, if present

Example

Code below panics since migration is changing SomeDoubleMap

use frame_support::storage::StateDiffGuard;

pub struct UncheckedMigrateToV1<T: Config>(PhantomData<T>);

impl<T: Config> UncheckedOnRuntimeUpgrade for UncheckedMigrateToV1<T> {
    fn on_runtime_upgrade() -> Weight {
        // migration logic here
        SomeDoubleMap::remove(_);

        Weight::default()
    }

    #[cfg(feature = "try-runtime")]
    fn try_on_runtime_upgrade() -> Result<Weight, &'static str> {
        // Using StateDiffGuard to ensure storage items are correctly migrated
        let _guard = StateDiffGuard::builder()
            .must_change_if_exists(SomeMap::<T>::storage_info())
            .must_not_change(SomeDoubleMap::<T>::storage_info())
            .can_not_change(GuardSubject::AnythingElse)
            .build();

        // try runtime upgrade logic here
        let weight = Self::on_runtime_upgrade();

        // any other logic
        Ok(weight)
    }
}

part of #240

polkadot address: 16FqwPZ8GRC5U5D4Fu7W33nA55ZXzXGWHwmbnE1eT6pxuqcT

[ggwpez]

Suggested change

	whitelisted_prefixes: BTreeSet<StoragePrefix>,
	must_change_prefixes: BTreeSet<StoragePrefix>,

The name here is not really expressing that they must change. It is also possible to make this more abstract by having a Map<Prefix, Change> where Change can be { Can | Must } x { Change | NotChange } (PS: not sure if these enum values are correct - hopefully you get the idea), but i think its fine for the beginning.

[dastansam]

made some improvements here, let me know if it's in the right direction, thanks)

[paritytech-cicd-pr]

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6079245

[kianenigma]

This looks like a very good PR. @dastansam sorry for the delay, are you still around for finishing? @ggwpez @liamaharon and I can provide reviews.

[kianenigma]

What I see missing is integrating this into polkadot_sdk_docs::reference_docs::migrations. It could be as simple as a basic mention.

[dastansam]

This looks like a very good PR. @dastansam sorry for the delay, are you still around for finishing? @ggwpez @liamaharon and I can provide reviews.

thanks, for sure. Looking forward for your reviews)

[dastansam]

hey @ggwpez, thanks for the updates. I am willing to finish this off if you have any other comments?

[ggwpez]

Just wanted to improve the API a bit but I think it should be good now.

PS: Maybe still update the MR description to show a small copy&paste example, possibly one of the tests to make it understandable what is being added.

[bkchr]

Why are we reading the entire state? We should only read the state based on the prefixes we will compare later.

[dastansam]

so that we can get the keys that were not meant to be changed (i.e maybe accidentally removed). see this line

[kianenigma]

Not from the runtime, but from the node side we could probably do this more elegently by using the ProofRecording types that are used for state proofs.

I think from the runtime there is no way other than (naively) reading the entire state pre and post operation, and compare them.