Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FRAME] Remove storage migration type #3828

Merged
merged 6 commits into from
Jun 26, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions prdoc/pr_3828.prdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0
# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json

title: "[FRAME] Remove storage migration type"

doc:
- audience: Runtime Dev
description: |
Introduce migration type to remove data associated with a specific storage of a pallet.

crates:
- name: frame-support
114 changes: 113 additions & 1 deletion substrate/frame/support/src/migrations.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@

use crate::{
defensive,
storage::transactional::with_transaction_opaque_err,
storage::{storage_prefix, transactional::with_transaction_opaque_err},
traits::{
Defensive, GetStorageVersion, NoStorageVersionSet, PalletInfoAccess, SafeMode,
StorageVersion,
Expand Down Expand Up @@ -369,6 +369,118 @@ impl<P: Get<&'static str>, DbWeight: Get<RuntimeDbWeight>> frame_support::traits
}
}

/// `RemoveStorage` is a utility struct used to remove a storage item from a specific pallet.
///
/// This struct is generic over three parameters:
/// - `P` is a type that implements the [`Get`] trait for a static string, representing the pallet's
/// name.
/// - `S` is a type that implements the [`Get`] trait for a static string, representing the storage
/// name.
/// - `DbWeight` is a type that implements the [`Get`] trait for [`RuntimeDbWeight`], providing the
/// weight for database operations.
///
/// On runtime upgrade, the `on_runtime_upgrade` function will clear the storage from the specified
/// storage, logging the number of keys removed. If the `try-runtime` feature is enabled, the
/// `pre_upgrade` and `post_upgrade` functions can be used to verify the storage removal before and
/// after the upgrade.
///
/// # Examples:
/// ```ignore
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/// construct_runtime! {
/// pub enum Runtime
/// {
/// System: frame_system = 0,
///
/// SomePallet: pallet_something = 1,
///
/// YourOtherPallets...
/// }
/// };
///
/// parameter_types! {
/// pub const SomePallet: &'static str = "SomePallet";
/// pub const StorageAccounts: &'static str = "Accounts";
/// pub const StorageAccountCount: &'static str = "AccountCount";
/// }
///
/// pub type Migrations = (
/// RemoveStorage<SomePallet, StorageAccounts, RocksDbWeight>,
/// RemoveStorage<SomePallet, StorageAccountCount, RocksDbWeight>,
/// AnyOtherMigrations...
/// );
///
/// pub type Executive = frame_executive::Executive<
/// Runtime,
/// Block,
/// frame_system::ChainContext<Runtime>,
/// Runtime,
/// Migrations
/// >;
/// ```
///
/// WARNING: `RemoveStorage` has no guard rails preventing it from bricking the chain if the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/// operation of removing storage for the given pallet would exceed the block weight limit.
///
/// If your storage has too many keys to be removed in a single block, it is advised to wait for
/// 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>>(
Copy link
Contributor Author

@muharem muharem Mar 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

PhantomData<(P, S, DbWeight)>,
);
impl<P: Get<&'static str>, S: Get<&'static str>, DbWeight: Get<RuntimeDbWeight>>
frame_support::traits::OnRuntimeUpgrade for RemoveStorage<P, S, DbWeight>
{
fn on_runtime_upgrade() -> frame_support::weights::Weight {
let hashed_prefix = storage_prefix(P::get().as_bytes(), S::get().as_bytes());
let keys_removed = match clear_prefix(&hashed_prefix, None) {
KillStorageResult::AllRemoved(value) => value,
KillStorageResult::SomeRemaining(value) => {
log::error!(
"`clear_prefix` failed to remove all keys for storage `{}` from pallet `{}`. THIS SHOULD NEVER HAPPEN! 🚨",
S::get(), P::get()
);
value
},
} as u64;

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

DbWeight::get().reads_writes(keys_removed + 1, keys_removed)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I understand, can you explain please?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see now

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

The only warning against it is:

/// 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?

Copy link
Member

@ggwpez ggwpez Apr 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

#[cfg(feature = "try-runtime")]
fn pre_upgrade() -> Result<sp_std::vec::Vec<u8>, sp_runtime::TryRuntimeError> {
use crate::storage::unhashed::contains_prefixed_key;

let hashed_prefix = storage_prefix(P::get().as_bytes(), S::get().as_bytes());
match contains_prefixed_key(&hashed_prefix) {
true => log::info!("Found `{}` `{}` keys pre-removal 👀", P::get(), S::get()),
false => log::warn!(
"Migration RemoveStorage<{}, {}> can be removed (no keys found pre-removal).",
P::get(),
S::get()
),
};
Ok(Default::default())
}

#[cfg(feature = "try-runtime")]
fn post_upgrade(_state: sp_std::vec::Vec<u8>) -> Result<(), sp_runtime::TryRuntimeError> {
use crate::storage::unhashed::contains_prefixed_key;

let hashed_prefix = storage_prefix(P::get().as_bytes(), S::get().as_bytes());
match contains_prefixed_key(&hashed_prefix) {
true => {
log::error!("`{}` `{}` has keys remaining post-removal ❗", P::get(), S::get());
return Err("Keys remaining post-removal, this should never happen 🚨".into())
},
false => log::info!("No `{}` `{}` keys found post-removal 🎉", P::get(), S::get()),
};
Ok(())
}
}

/// A migration that can proceed in multiple steps.
pub trait SteppedMigration {
/// The cursor type that stores the progress (aka. state) of this migration.
Expand Down
Loading