[FRAME] State Diff Guard #4204

dastansam · 2024-04-18T23:28:33Z

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

substrate/frame/support/src/storage/state_diff_guard.rs

ggwpez · 2024-04-19T12:44:00Z

substrate/frame/support/src/storage/state_diff_guard.rs

+#[derive(Default, Debug)]
+pub struct StateDiffGuard {
+	// Storage prefixes that are expected to change.
+	whitelisted_prefixes: BTreeSet<StoragePrefix>,


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.

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

paritytech-cicd-pr · 2024-04-29T00:46:04Z

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 · 2024-07-10T08:59:07Z

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 · 2024-07-10T09:00:31Z

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

dastansam · 2024-07-10T09:00:51Z

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)

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

dastansam · 2025-01-10T12:34:25Z

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

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

ggwpez · 2025-01-10T13:05:58Z

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 · 2024-07-17T12:55:03Z

substrate/frame/support/src/storage/state_diff_guard.rs

+		let check_passed = self.apply_mutation_policy();
+
+		// No need to double panic, eg. inside a test assertion failure.
+		if sp_std::thread::panicking() {


Suggested change

if sp_std::thread::panicking() {

#[cfg(feature = "std")]

if std::thread::panicking() {

You did not apply this.

substrate/frame/support/src/storage/state_diff_guard.rs

bkchr · 2025-01-10T13:42:41Z

substrate/frame/support/src/storage/state_diff_guard.rs

+		let mut state = BTreeMap::new();
+
+		let mut previous_key = Vec::new();
+		while let Some(next) = sp_io::storage::next_key(&previous_key) {


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

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

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.

dastansam · 2025-01-17T16:00:49Z

@bkchr one more review, please

bkchr

Given the current way it is implemented, we can never use this with any production chain. The time it would take to run the migrations would grow by a lot if every migration would read the entire state.

IMO if we really want to have this, we need to change the internals. The user needs to declare which storage entries are allowed to change. At the start we would read these keys from the state. (Could still hit a big map, but we can not really go better with what we have at hand)

After the operation is finished, we could do the checks that the keys that should have changed, indeed have changed and also record the current keys/values. Now we could do a trick and run the migration and the checking in a storage transaction. After the migration and the checking succeeded, we would record the storage root and revert the transaction. Then we would apply the changed keys again and check that the storage root matches. If the storage root doesn't match, something else was touched. Problem would be that we would not know which key changed.

bkchr · 2025-01-17T19:34:20Z

substrate/frame/support/src/storage/state_diff_guard.rs

+//! In the example above, the guard will panic if any storage entry that doesn't match `SomeMap` or
+//! `SomeDoubleMap` prefixes is changed.
+
+use alloc::{collections::btree_map::BTreeMap, vec::Vec};


Suggested change

use alloc::{collections::btree_map::BTreeMap, vec::Vec};

use alloc::{collections::btree_map::BTreeMap, vec::Vec, vec};

bkchr · 2025-01-17T19:36:41Z

substrate/frame/support/src/storage/state_diff_guard.rs

+//!
+//! # Example
+//!
+//! Use the `StateDiffGuard` in a migration:


IMO this can not be used in any migration with a reasonable state, because of the behavior of reading the entire state.

This would take ages.

bkchr · 2025-01-17T19:40:27Z

substrate/frame/support/src/storage/state_diff_guard.rs

+		let check_passed = self.apply_mutation_policy();
+
+		// No need to double panic, eg. inside a test assertion failure.
+		if sp_std::thread::panicking() {


You did not apply this.

dastansam and others added 3 commits April 19, 2024 01:28

add StateDiffGuard initial impl

39863fa

Add prdoc

c7fad3f

Merge branch 'master' into feat/state-diff-guard

064c27f

dastansam changed the title ~~add StateDiffGuard initial impl~~ migrations: State difference guard and recorder Apr 18, 2024

dastansam changed the title ~~migrations: State difference guard and recorder~~ migrations: state difference guard and recorder Apr 19, 2024

Fix checks

730d9a9

dastansam force-pushed the feat/state-diff-guard branch from f3cc891 to 730d9a9 Compare April 19, 2024 00:30

Some improvements

ddf1711

dastansam force-pushed the feat/state-diff-guard branch from 8e68898 to ddf1711 Compare April 19, 2024 12:04

ggwpez reviewed Apr 19, 2024

View reviewed changes

substrate/frame/support/src/storage/state_diff_guard.rs Outdated Show resolved Hide resolved

ggwpez reviewed Apr 19, 2024

View reviewed changes

dastansam force-pushed the feat/state-diff-guard branch from e93ecb2 to d1e0384 Compare April 29, 2024 00:37

Introduce mutation policy, make guard more generic

12e6f44

dastansam force-pushed the feat/state-diff-guard branch from d1e0384 to 12e6f44 Compare April 29, 2024 00:52

dastansam requested a review from ggwpez April 29, 2024 00:52

dastansam marked this pull request as ready for review April 29, 2024 00:52

dastansam requested a review from a team as a code owner April 29, 2024 00:52

ggwpez added 4 commits January 10, 2025 12:45

Less intendation

95d45fb

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

Merge remote-tracking branch 'origin/master' into feat/state-diff-guard

f0fb645

fmt

b8472e7

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

Fix bugs and add tests

cfeb384

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

ggwpez added 4 commits January 10, 2025 13:54

Improve API

5491b3b

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

fixes

6931ad8

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

prdoc

fb921e4

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

dont change StorageInfo

d9874c3

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

ggwpez approved these changes Jan 10, 2025

View reviewed changes

ggwpez added the T1-FRAME This PR/Issue is related to core FRAME, the framework. label Jan 10, 2025

ggwpez changed the title ~~migrations: state difference guard and recorder~~ [FRAME] State Diff Guard Jan 10, 2025

bkchr reviewed Jan 10, 2025

View reviewed changes

Resolve comments

113dd27

kianenigma approved these changes Jan 10, 2025

View reviewed changes

dastansam requested a review from bkchr January 15, 2025 18:55

bkchr reviewed Jan 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FRAME] State Diff Guard #4204

[FRAME] State Diff Guard #4204

dastansam commented Apr 18, 2024 •

edited

Loading

ggwpez Apr 19, 2024 •

edited

Loading

dastansam Apr 29, 2024

paritytech-cicd-pr commented Apr 29, 2024

kianenigma commented Jul 10, 2024

kianenigma commented Jul 10, 2024

dastansam commented Jul 10, 2024

dastansam commented Jan 10, 2025

ggwpez commented Jan 10, 2025 •

edited

Loading

bkchr Jul 17, 2024

bkchr Jan 17, 2025

bkchr Jan 10, 2025

dastansam Jan 10, 2025 •

edited

Loading

kianenigma Jan 10, 2025

dastansam commented Jan 17, 2025

bkchr left a comment

bkchr Jan 17, 2025

bkchr Jan 17, 2025

bkchr Jan 17, 2025

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

	if sp_std::thread::panicking() {
	#[cfg(feature = "std")]
	if std::thread::panicking() {

	use alloc::{collections::btree_map::BTreeMap, vec::Vec};
	use alloc::{collections::btree_map::BTreeMap, vec::Vec, vec};

[FRAME] State Diff Guard #4204

Are you sure you want to change the base?

[FRAME] State Diff Guard #4204

Conversation

dastansam commented Apr 18, 2024 • edited Loading

Motivation

Example

ggwpez Apr 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paritytech-cicd-pr commented Apr 29, 2024

kianenigma commented Jul 10, 2024

kianenigma commented Jul 10, 2024

dastansam commented Jul 10, 2024

dastansam commented Jan 10, 2025

ggwpez commented Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dastansam Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dastansam commented Jan 17, 2025

bkchr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dastansam commented Apr 18, 2024 •

edited

Loading

ggwpez Apr 19, 2024 •

edited

Loading

ggwpez commented Jan 10, 2025 •

edited

Loading

dastansam Jan 10, 2025 •

edited

Loading