[FRAME] Remove storage migration type #3828

muharem · 2024-03-25T15:20:29Z

Introduce migration type to remove data associated with a specific storage of a pallet.

Based on existing RemovePallet migration type.

Required for #3820

muharem · 2024-03-25T15:22:51Z

substrate/frame/support/src/migrations.rs

+/// a multi-block scheduler currently under development which will allow for removal of storage
+/// items (and performing other heavy migrations) over multiple blocks
+/// (see <https://github.com/paritytech/substrate/issues/13690>).
+pub struct RemoveStorage<P: Get<&'static str>, S: Get<&'static str>, DbWeight: Get<RuntimeDbWeight>>(


it could be more general, something like ClearPrefix. And RemovePallet and RemoveStorage based on it. But it will require the introduction of few more types for resolving the prefix from static pallet and storage and and type to concat pallet and storage name to have a human readable prefix for logs.
Perhaps ClearPrefix wont be used for itself and copy/paste is better for simplicity.

kianenigma · 2024-03-26T10:52:33Z

substrate/frame/support/src/migrations.rs

+/// after the upgrade.
+///
+/// # Examples:
+/// ```ignore


I think with a few lines of change this can be made non-ignore, can you try?

I did try, requires additional deps for this pallet, example becomes very big and does not really give any assertions

kianenigma · 2024-03-26T10:53:19Z

substrate/frame/support/src/migrations.rs

+/// >;
+/// ```
+///
+/// WARNING: `RemoveStorage` has no guard rails preventing it from bricking the chain if the


This is good, please add it to other similar types like RemovePallet as well :)

I checked, there is no more general migration solution like this. RemovePallet have it, it is where I copied this from, all credits to @liamaharon

substrate/frame/support/src/migrations.rs

prdoc/pr_3828.prdoc

Co-authored-by: Liam Aharon <liam.aharon@hotmail.com> Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

ggwpez · 2024-03-26T15:54:17Z

substrate/frame/support/src/migrations.rs

+/// >;
+/// ```
+///
+/// WARNING: `RemoveStorage` has no guard rails preventing it from bricking the chain if the


Maybe a specific warning for para-chains? I think they are often not aware of such limitations.

ggwpez · 2024-03-26T15:55:00Z

substrate/frame/support/src/migrations.rs

+
+ log::info!("Removed `{}` `{}` `{}` keys 🧹", keys_removed, P::get(), S::get());
+
+ DbWeight::get().reads_writes(keys_removed + 1, keys_removed)


The weight annotation makes it unusable for parachains anyway, since it does not report proper weight.

Not sure I understand, can you explain please?

I don't see :P Can you please explain?

The fact that RuntimeDbWeight only has u64 aka. ref_time and no proof_size.

The only warning against it is:

polkadot-sdk/substrate/primitives/weights/src/lib.rs

Lines 79 to 80 in 8eeacff

/// NOTE: This is currently only measured in computational time, and will probably

/// be updated all together once proof size is accounted for.

I can't find a related tracking issue for this matter, @ggwpez do you know any?

No there is no issue. This is legacy code and reads_writes should probably be deprecated...
Only benchmarking this would solve the issue. But for long term it is probably still not usable without being a MBM.

# ISSUE - Link to the issue: #3800 # Deliverables - [x] remove deprecated calls; (d579b67) - [x] set explicit coded indexes for Error and Event enums, remove unused variants and keep the same indexes for the rest; (d579b67) - [x] remove unused Config's type parameters; (d579b67) - [x] remove irrelevant tests and adopt relevant using old api; (d579b67) - [x] remove benchmarks for removed calls; (1a3d5f1) - [x] prdoc (d579b67) - [x] remove deprecated methods from the `treasury/README.md` and add up-to-date dispatchable functions documentation (d579b67) - [x] remove deprecated weight functions (8f74134) > ### Separated to other issues > - [ ] remove storage items like Proposals and ProposalCount, that are not used anymore Adjust all treasury pallet instances within polkadot-sdk - [x] `pallet_bounty`, `tip`, `child_bounties`: openguild-labs#3 - [x] Remove deprecated treasury weight functions used in Westend and Rococo runtime `collective-westend`, `collective-rococo` Add migration for westend and rococo to clean the data from removed storage items - [ ] #3828 # Test Outcomes Successful tests by running `cargo test --features runtime-benchmarks` ``` running 38 tests test tests::__construct_runtime_integrity_test::runtime_integrity_tests ... ok test benchmarking::benchmarks::bench_check_status ... ok test benchmarking::benchmarks::bench_payout ... ok test benchmarking::benchmarks::bench_spend_local ... ok test tests::accepted_spend_proposal_enacted_on_spend_period ... ok test benchmarking::benchmarks::bench_spend ... ok test tests::accepted_spend_proposal_ignored_outside_spend_period ... ok test benchmarking::benchmarks::bench_void_spend ... ok test benchmarking::benchmarks::bench_remove_approval ... ok test tests::genesis_funding_works ... ok test tests::genesis_config_works ... ok test tests::inexistent_account_works ... ok test tests::minting_works ... ok test tests::check_status_works ... ok test tests::payout_retry_works ... ok test tests::pot_underflow_should_not_diminish ... ok test tests::remove_already_removed_approval_fails ... ok test tests::spend_local_origin_permissioning_works ... ok test tests::spend_valid_from_works ... ok test tests::spend_expires ... ok test tests::spend_works ... ok test tests::test_genesis_config_builds ... ok test tests::spend_payout_works ... ok test tests::spend_local_origin_works ... ok test tests::spend_origin_works ... ok test tests::spending_local_in_batch_respects_max_total ... ok test tests::spending_in_batch_respects_max_total ... ok test tests::try_state_proposals_invariant_2_works ... ok test tests::try_state_proposals_invariant_1_works ... ok test tests::try_state_spends_invariant_2_works ... ok test tests::try_state_spends_invariant_1_works ... ok test tests::treasury_account_doesnt_get_deleted ... ok test tests::try_state_spends_invariant_3_works ... ok test tests::unused_pot_should_diminish ... ok test tests::void_spend_works ... ok test tests::try_state_proposals_invariant_3_works ... ok test tests::max_approvals_limited ... ok test benchmarking::benchmarks::bench_on_initialize_proposals ... ok test result: ok. 38 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.08s Doc-tests pallet_treasury running 2 tests test substrate/frame/treasury/src/lib.rs - (line 52) ... ignored test substrate/frame/treasury/src/lib.rs - (line 79) ... ignored test result: ok. 0 passed; 0 failed; 2 ignored; 0 measured; 0 filtered out; finished in 0.00s ``` polkadot address: 19nSqFQorfF2HxD3oBzWM3oCh4SaCRKWt1yvmgaPYGCo71J

bkchr · 2024-06-24T19:44:48Z

@muharem what is the status of this pr?

muharem · 2024-06-25T13:11:45Z

@bkchr I was not sure about merging it because it cannot be safely applied for a parachain migration. But now I think we will need it at least for relay chains. Also since there is a warning in the doc about this concern. I think we can merge this. If fine with you, I will merge it after CIs pass

# ISSUE - Link to the issue: paritytech#3800 # Deliverables - [x] remove deprecated calls; (paritytech@d579b67) - [x] set explicit coded indexes for Error and Event enums, remove unused variants and keep the same indexes for the rest; (paritytech@d579b67) - [x] remove unused Config's type parameters; (paritytech@d579b67) - [x] remove irrelevant tests and adopt relevant using old api; (paritytech@d579b67) - [x] remove benchmarks for removed calls; (paritytech@1a3d5f1) - [x] prdoc (paritytech@d579b67) - [x] remove deprecated methods from the `treasury/README.md` and add up-to-date dispatchable functions documentation (paritytech@d579b67) - [x] remove deprecated weight functions (paritytech@8f74134) > ### Separated to other issues > - [ ] remove storage items like Proposals and ProposalCount, that are not used anymore Adjust all treasury pallet instances within polkadot-sdk - [x] `pallet_bounty`, `tip`, `child_bounties`: openguild-labs#3 - [x] Remove deprecated treasury weight functions used in Westend and Rococo runtime `collective-westend`, `collective-rococo` Add migration for westend and rococo to clean the data from removed storage items - [ ] paritytech#3828 # Test Outcomes Successful tests by running `cargo test --features runtime-benchmarks` ``` running 38 tests test tests::__construct_runtime_integrity_test::runtime_integrity_tests ... ok test benchmarking::benchmarks::bench_check_status ... ok test benchmarking::benchmarks::bench_payout ... ok test benchmarking::benchmarks::bench_spend_local ... ok test tests::accepted_spend_proposal_enacted_on_spend_period ... ok test benchmarking::benchmarks::bench_spend ... ok test tests::accepted_spend_proposal_ignored_outside_spend_period ... ok test benchmarking::benchmarks::bench_void_spend ... ok test benchmarking::benchmarks::bench_remove_approval ... ok test tests::genesis_funding_works ... ok test tests::genesis_config_works ... ok test tests::inexistent_account_works ... ok test tests::minting_works ... ok test tests::check_status_works ... ok test tests::payout_retry_works ... ok test tests::pot_underflow_should_not_diminish ... ok test tests::remove_already_removed_approval_fails ... ok test tests::spend_local_origin_permissioning_works ... ok test tests::spend_valid_from_works ... ok test tests::spend_expires ... ok test tests::spend_works ... ok test tests::test_genesis_config_builds ... ok test tests::spend_payout_works ... ok test tests::spend_local_origin_works ... ok test tests::spend_origin_works ... ok test tests::spending_local_in_batch_respects_max_total ... ok test tests::spending_in_batch_respects_max_total ... ok test tests::try_state_proposals_invariant_2_works ... ok test tests::try_state_proposals_invariant_1_works ... ok test tests::try_state_spends_invariant_2_works ... ok test tests::try_state_spends_invariant_1_works ... ok test tests::treasury_account_doesnt_get_deleted ... ok test tests::try_state_spends_invariant_3_works ... ok test tests::unused_pot_should_diminish ... ok test tests::void_spend_works ... ok test tests::try_state_proposals_invariant_3_works ... ok test tests::max_approvals_limited ... ok test benchmarking::benchmarks::bench_on_initialize_proposals ... ok test result: ok. 38 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.08s Doc-tests pallet_treasury running 2 tests test substrate/frame/treasury/src/lib.rs - (line 52) ... ignored test substrate/frame/treasury/src/lib.rs - (line 79) ... ignored test result: ok. 0 passed; 0 failed; 2 ignored; 0 measured; 0 filtered out; finished in 0.00s ``` polkadot address: 19nSqFQorfF2HxD3oBzWM3oCh4SaCRKWt1yvmgaPYGCo71J

Introduce migration type to remove data associated with a specific storage of a pallet. Based on existing `RemovePallet` migration type. Required for paritytech#3820 --------- Co-authored-by: Liam Aharon <liam.aharon@hotmail.com> Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

remove storage migration

94181f9

muharem added the T1-FRAME This PR/Issue is related to core FRAME, the framework. label Mar 25, 2024

muharem requested a review from a team as a code owner March 25, 2024 15:20

muharem commented Mar 25, 2024

View reviewed changes

muharem requested a review from liamaharon March 25, 2024 15:23

pr doc

72f5c46

muharem mentioned this pull request Mar 25, 2024

Remove deprecated treasury pallet calls #3820

Merged

12 tasks

kianenigma reviewed Mar 26, 2024

View reviewed changes

substrate/frame/support/src/migrations.rs Outdated Show resolved Hide resolved

kianenigma approved these changes Mar 26, 2024

View reviewed changes

liamaharon approved these changes Mar 26, 2024

View reviewed changes

prdoc/pr_3828.prdoc Outdated Show resolved Hide resolved

Apply suggestions from code review

189c6c0

Co-authored-by: Liam Aharon <liam.aharon@hotmail.com> Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

ggwpez reviewed Mar 26, 2024

View reviewed changes

ggwpez approved these changes Mar 26, 2024

View reviewed changes

ggwpez changed the title ~~[FREAME] Remove storage migration type~~ [FRAME] Remove storage migration type Mar 26, 2024

ggwpez reviewed Mar 26, 2024

View reviewed changes

Merge branch 'master' into muharem-remove-storage-migration

39be16d

bkontur mentioned this pull request Apr 2, 2024

Snowbridge: Synchronize from Snowfork repository #3761

Merged

muharem added 2 commits June 25, 2024 15:04

pr doc bump

dedce62

Merge branch 'master' into muharem-remove-storage-migration

7065e64

bkchr approved these changes Jun 25, 2024

View reviewed changes

bkchr added this pull request to the merge queue Jun 25, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 25, 2024

bkchr added this pull request to the merge queue Jun 26, 2024

Merged via the queue into master with commit 20aecad Jun 26, 2024
156 of 159 checks passed

bkchr deleted the muharem-remove-storage-migration branch June 26, 2024 08:39

This was referenced Aug 21, 2024

Update polkadot-sdk from v1.11.0 to stable2407 moondance-labs/tanssi#659

Open

Update polkadot-sdk from v1.11.0 to stable2407 moonbeam-foundation/moonbeam#2912

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FRAME] Remove storage migration type #3828

[FRAME] Remove storage migration type #3828

muharem commented Mar 25, 2024 •

edited

Loading

muharem Mar 25, 2024 •

edited

Loading

kianenigma Mar 26, 2024

muharem Mar 26, 2024

kianenigma Mar 26, 2024

muharem Mar 26, 2024

ggwpez Mar 26, 2024

ggwpez Mar 26, 2024

muharem Mar 26, 2024

muharem Mar 26, 2024

liamaharon Apr 2, 2024

kianenigma Apr 2, 2024

ggwpez Apr 2, 2024 •

edited

Loading

bkchr commented Jun 24, 2024

muharem commented Jun 25, 2024


		log::info!("Removed `{}` `{}` `{}` keys 🧹", keys_removed, P::get(), S::get());

		DbWeight::get().reads_writes(keys_removed + 1, keys_removed)

	/// NOTE: This is currently only measured in computational time, and will probably
	/// be updated all together once proof size is accounted for.

[FRAME] Remove storage migration type #3828

[FRAME] Remove storage migration type #3828

Conversation

muharem commented Mar 25, 2024 • edited Loading

muharem Mar 25, 2024 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggwpez Apr 2, 2024 • edited Loading

Choose a reason for hiding this comment

bkchr commented Jun 24, 2024

muharem commented Jun 25, 2024

muharem commented Mar 25, 2024 •

edited

Loading

muharem Mar 25, 2024 •

edited

Loading

ggwpez Apr 2, 2024 •

edited

Loading