-
Notifications
You must be signed in to change notification settings - Fork 689
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
Changes from 1 commit
94181f9
72f5c46
189c6c0
39be16d
dedce62
7065e64
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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, | ||||||
|
@@ -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 | ||||||
/// 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 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is good, please add it to other similar types like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I checked, there is no more general migration solution like this. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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>>( | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it could be more general, something like |
||||||
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) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure I understand, can you explain please? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see now There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see :P Can you please explain? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The fact that The only warning against it is: polkadot-sdk/substrate/primitives/weights/src/lib.rs Lines 79 to 80 in 8eeacff
I can't find a related tracking issue for this matter, @ggwpez do you know any? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No there is no issue. This is legacy code and |
||||||
} | ||||||
|
||||||
#[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(sp_std::vec::Vec::new()) | ||||||
muharem marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
} | ||||||
|
||||||
#[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. | ||||||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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