From 8c1865d2f28beb6aa87d2d45dffa87bd2c38fac9 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Mon, 13 Jun 2022 13:17:07 +0100 Subject: [PATCH] MEL bound `state-trie-migration` (#11639) * MEL bound state-trie-migration Signed-off-by: Oliver Tale-Yazdi * wip Signed-off-by: Oliver Tale-Yazdi * Add tests Signed-off-by: Oliver Tale-Yazdi * Use sp_std Co-authored-by: cheme * Add doc Signed-off-by: Oliver Tale-Yazdi * Set MaxKeyLen default to 512 Just to be sure that it will work. There is also no real penalty from over-estimating it. Signed-off-by: Oliver Tale-Yazdi * Add more doc Signed-off-by: Oliver Tale-Yazdi * Clippy Signed-off-by: Oliver Tale-Yazdi * fmt Signed-off-by: Oliver Tale-Yazdi * Fix assert_err_with_weight macro Looks like I'm the only one using it anyway... Signed-off-by: Oliver Tale-Yazdi * Fix tests that use env macro Signed-off-by: Oliver Tale-Yazdi * Fix test Signed-off-by: Oliver Tale-Yazdi Co-authored-by: cheme --- substrate/bin/node/runtime/src/lib.rs | 2 + .../frame/state-trie-migration/src/lib.rs | 273 ++++++++++++++---- substrate/frame/support/src/lib.rs | 2 +- 3 files changed, 223 insertions(+), 54 deletions(-) diff --git a/substrate/bin/node/runtime/src/lib.rs b/substrate/bin/node/runtime/src/lib.rs index 8e854b770a..6f3df74166 100644 --- a/substrate/bin/node/runtime/src/lib.rs +++ b/substrate/bin/node/runtime/src/lib.rs @@ -1492,12 +1492,14 @@ impl pallet_whitelist::Config for Runtime { parameter_types! { pub const MigrationSignedDepositPerItem: Balance = 1 * CENTS; pub const MigrationSignedDepositBase: Balance = 20 * DOLLARS; + pub const MigrationMaxKeyLen: u32 = 512; } impl pallet_state_trie_migration::Config for Runtime { type Event = Event; type ControlOrigin = EnsureRoot; type Currency = Balances; + type MaxKeyLen = MigrationMaxKeyLen; type SignedDepositPerItem = MigrationSignedDepositPerItem; type SignedDepositBase = MigrationSignedDepositBase; // Warning: this is not advised, as it might allow the chain to be temporarily DOS-ed. diff --git a/substrate/frame/state-trie-migration/src/lib.rs b/substrate/frame/state-trie-migration/src/lib.rs index 899250cc3f..9ac128a0e3 100644 --- a/substrate/frame/state-trie-migration/src/lib.rs +++ b/substrate/frame/state-trie-migration/src/lib.rs @@ -78,12 +78,14 @@ pub mod pallet { traits::{Currency, Get}, }; use frame_system::{self, pallet_prelude::*}; - use sp_core::storage::well_known_keys::DEFAULT_CHILD_STORAGE_KEY_PREFIX; + use sp_core::{ + hexdisplay::HexDisplay, storage::well_known_keys::DEFAULT_CHILD_STORAGE_KEY_PREFIX, + }; use sp_runtime::{ self, traits::{Saturating, Zero}, }; - use sp_std::prelude::*; + use sp_std::{ops::Deref, prelude::*}; pub(crate) type BalanceOf = <::Currency as Currency<::AccountId>>::Balance; @@ -124,30 +126,43 @@ pub mod pallet { } /// The progress of either the top or child keys. - #[derive(Clone, Encode, Decode, scale_info::TypeInfo, PartialEq, Eq)] - pub enum Progress { + #[derive( + CloneNoBound, + Encode, + Decode, + scale_info::TypeInfo, + PartialEqNoBound, + EqNoBound, + MaxEncodedLen, + )] + #[scale_info(skip_type_params(MaxKeyLen))] + #[codec(mel_bound())] + pub enum Progress> { /// Yet to begin. ToStart, /// Ongoing, with the last key given. - LastKey(Vec), + LastKey(BoundedVec), /// All done. Complete, } + /// Convenience type for easier usage of [`Progress`]. + pub type ProgressOf = Progress<::MaxKeyLen>; + /// A migration task stored in state. /// /// It tracks the last top and child keys read. - #[derive(Clone, Encode, Decode, scale_info::TypeInfo, PartialEq, Eq)] + #[derive(Clone, Encode, Decode, scale_info::TypeInfo, PartialEq, Eq, MaxEncodedLen)] #[codec(mel_bound(T: Config))] #[scale_info(skip_type_params(T))] pub struct MigrationTask { /// The current top trie migration progress. - pub(crate) progress_top: Progress, + pub(crate) progress_top: ProgressOf, /// The current child trie migration progress. /// /// If `ToStart`, no further top keys are processed until the child key migration is /// `Complete`. - pub(crate) progress_child: Progress, + pub(crate) progress_child: ProgressOf, /// Dynamic counter for the number of items that we have processed in this execution from /// the top trie. @@ -186,12 +201,11 @@ pub mod pallet { pub(crate) _ph: sp_std::marker::PhantomData, } - impl sp_std::fmt::Debug for Progress { + impl> sp_std::fmt::Debug for Progress { fn fmt(&self, f: &mut sp_std::fmt::Formatter<'_>) -> sp_std::fmt::Result { match self { Progress::ToStart => f.write_str("To start"), - Progress::LastKey(key) => - write!(f, "Last: {:?}", sp_core::hexdisplay::HexDisplay::from(key)), + Progress::LastKey(key) => write!(f, "Last: {:?}", HexDisplay::from(key.deref())), Progress::Complete => f.write_str("Complete"), } } @@ -252,17 +266,20 @@ pub mod pallet { /// reading a key, we simply cannot know how many bytes it is. In other words, this should /// not be used in any environment where resources are strictly bounded (e.g. a parachain), /// but it is acceptable otherwise (relay chain, offchain workers). - pub fn migrate_until_exhaustion(&mut self, limits: MigrationLimits) { + pub fn migrate_until_exhaustion(&mut self, limits: MigrationLimits) -> DispatchResult { log!(debug, "running migrations on top of {:?} until {:?}", self, limits); if limits.item.is_zero() || limits.size.is_zero() { // handle this minor edge case, else we would call `migrate_tick` at least once. log!(warn, "limits are zero. stopping"); - return + return Ok(()) } while !self.exhausted(limits) && !self.finished() { - self.migrate_tick(); + if let Err(e) = self.migrate_tick() { + log!(error, "migrate_until_exhaustion failed: {:?}", e); + return Err(e) + } } // accumulate dynamic data into the storage items. @@ -270,19 +287,18 @@ pub mod pallet { self.child_items = self.child_items.saturating_add(self.dyn_child_items); self.top_items = self.top_items.saturating_add(self.dyn_top_items); log!(debug, "finished with {:?}", self); + Ok(()) } /// Migrate AT MOST ONE KEY. This can be either a top or a child key. /// /// This function is *the* core of this entire pallet. - fn migrate_tick(&mut self) { + fn migrate_tick(&mut self) -> DispatchResult { match (&self.progress_top, &self.progress_child) { - (Progress::ToStart, _) => { - self.migrate_top(); - }, + (Progress::ToStart, _) => self.migrate_top(), (Progress::LastKey(_), Progress::LastKey(_)) => { // we're in the middle of doing work on a child tree. - self.migrate_child(); + self.migrate_child() }, (Progress::LastKey(top_key), Progress::ToStart) => { // 3. this is the root of a child key, and we are finishing all child-keys (and @@ -293,20 +309,22 @@ pub mod pallet { if !top_key.starts_with(DEFAULT_CHILD_STORAGE_KEY_PREFIX) { // we continue the top key migrations. // continue the top key migration - self.migrate_top(); + self.migrate_top() } else { // this is the root of a child key, and we start processing child keys (and // should call `migrate_child`). - self.migrate_child(); + self.migrate_child() } }, (Progress::LastKey(_), Progress::Complete) => { // we're done with migrating a child-root. - self.migrate_top(); + self.migrate_top()?; self.progress_child = Progress::ToStart; + Ok(()) }, (Progress::Complete, _) => { // nada + Ok(()) }, } } @@ -314,24 +332,30 @@ pub mod pallet { /// Migrate the current child key, setting it to its new value, if one exists. /// /// It updates the dynamic counters. - fn migrate_child(&mut self) { + fn migrate_child(&mut self) -> DispatchResult { use sp_io::default_child_storage as child_io; let (maybe_current_child, child_root) = match (&self.progress_child, &self.progress_top) { (Progress::LastKey(last_child), Progress::LastKey(last_top)) => { let child_root = Pallet::::transform_child_key_or_halt(last_top); - let maybe_current_child = child_io::next_key(child_root, last_child); + let maybe_current_child: Option> = + if let Some(next) = child_io::next_key(child_root, last_child) { + Some(next.try_into().map_err(|_| Error::::KeyTooLong)?) + } else { + None + }; + (maybe_current_child, child_root) }, (Progress::ToStart, Progress::LastKey(last_top)) => { let child_root = Pallet::::transform_child_key_or_halt(last_top); // Start with the empty key as first key. - (Some(Vec::new()), child_root) + (Some(Default::default()), child_root) }, _ => { // defensive: there must be an ongoing top migration. frame_support::defensive!("cannot migrate child key."); - return + return Ok(()) }, }; @@ -350,21 +374,30 @@ pub mod pallet { self.progress_child = match maybe_current_child { Some(last_child) => Progress::LastKey(last_child), None => Progress::Complete, - } + }; + Ok(()) } /// Migrate the current top key, setting it to its new value, if one exists. /// /// It updates the dynamic counters. - fn migrate_top(&mut self) { + fn migrate_top(&mut self) -> DispatchResult { let maybe_current_top = match &self.progress_top { - Progress::LastKey(last_top) => sp_io::storage::next_key(last_top), + Progress::LastKey(last_top) => { + let maybe_top: Option> = + if let Some(next) = sp_io::storage::next_key(last_top) { + Some(next.try_into().map_err(|_| Error::::KeyTooLong)?) + } else { + None + }; + maybe_top + }, // Start with the empty key as first key. - Progress::ToStart => Some(Vec::new()), + Progress::ToStart => Some(Default::default()), Progress::Complete => { // defensive: there must be an ongoing top migration. frame_support::defensive!("cannot migrate top key."); - return + return Ok(()) }, }; @@ -383,12 +416,24 @@ pub mod pallet { self.progress_top = match maybe_current_top { Some(last_top) => Progress::LastKey(last_top), None => Progress::Complete, - } + }; + Ok(()) } } /// The limits of a migration. - #[derive(Clone, Copy, Encode, Decode, scale_info::TypeInfo, Default, Debug, PartialEq, Eq)] + #[derive( + Clone, + Copy, + Encode, + Decode, + scale_info::TypeInfo, + Default, + Debug, + PartialEq, + Eq, + MaxEncodedLen, + )] pub struct MigrationLimits { /// The byte size limit. pub size: u32, @@ -416,14 +461,13 @@ pub mod pallet { Slashed { who: T::AccountId, amount: BalanceOf }, /// The auto migration task finished. AutoMigrationFinished, - /// Migration got halted. + /// Migration got halted due to an error or miss-configuration. Halted, } /// The outer Pallet struct. #[pallet::pallet] #[pallet::generate_store(pub(crate) trait Store)] - #[pallet::without_storage_info] pub struct Pallet(_); /// Configurations of this pallet. @@ -441,6 +485,31 @@ pub mod pallet { /// The currency provider type. type Currency: Currency; + /// Maximal number of bytes that a key can have. + /// + /// FRAME itself does not limit the key length. + /// The concrete value must therefore depend on your storage usage. + /// A [`frame_support::storage::StorageNMap`] for example can have an arbitrary number of + /// keys which are then hashed and concatenated, resulting in arbitrarily long keys. + /// + /// Use the *state migration RPC* to retrieve the length of the longest key in your + /// storage: + /// + /// The migration will halt with a `Halted` event if this value is too small. + /// Since there is no real penalty from over-estimating, it is advised to use a large + /// value. The default is 512 byte. + /// + /// Some key lengths for reference: + /// - [`frame_support::storage::StorageValue`]: 32 byte + /// - [`frame_support::storage::StorageMap`]: 64 byte + /// - [`frame_support::storage::StorageDoubleMap`]: 96 byte + /// + /// For more info see + /// + + #[pallet::constant] + type MaxKeyLen: Get; + /// The amount of deposit collected per item in advance, for signed migrations. /// /// This should reflect the average storage value size in the worse case. @@ -481,6 +550,14 @@ pub mod pallet { pub enum Error { /// max signed limits not respected. MaxSignedLimits, + /// A key was longer than the configured maximum. + /// + /// This means that the migration halted at the current [`Progress`] and + /// can be resumed with a larger [`crate::Config::MaxKeyLen`] value. + /// Retrying with the same [`crate::Config::MaxKeyLen`] value will not work. + /// The value should only be increased to avoid a storage migration for the currently + /// stored [`crate::Progress::LastKey`]. + KeyTooLong, /// submitter does not have enough funds. NotEnoughFunds, /// bad witness data provided. @@ -563,7 +640,7 @@ pub mod pallet { } } ); - task.migrate_until_exhaustion(limits); + let migration = task.migrate_until_exhaustion(limits); // ensure that the migration witness data was correct. if real_size_upper < task.dyn_size { @@ -587,7 +664,14 @@ pub mod pallet { ); MigrationProcess::::put(task); - Ok((actual_weight, Pays::No).into()) + let post_info = PostDispatchInfo { actual_weight, pays_fee: Pays::No }; + match migration { + Ok(_) => Ok(post_info), + Err(error) => { + Self::halt(&error); + Err(DispatchErrorWithPostInfo { post_info, error }) + }, + } } /// Migrate the list of top keys by iterating each of them one by one. @@ -735,8 +819,8 @@ pub mod pallet { #[pallet::weight(T::DbWeight::get().reads_writes(1, 1))] pub fn force_set_progress( origin: OriginFor, - progress_top: Progress, - progress_child: Progress, + progress_top: ProgressOf, + progress_child: ProgressOf, ) -> DispatchResult { let _ = T::ControlOrigin::ensure_origin(origin)?; MigrationProcess::::mutate(|task| { @@ -752,7 +836,9 @@ pub mod pallet { fn on_initialize(_: BlockNumberFor) -> Weight { if let Some(limits) = Self::auto_limits() { let mut task = Self::migration_process(); - task.migrate_until_exhaustion(limits); + if let Err(e) = task.migrate_until_exhaustion(limits) { + Self::halt(&e); + } let weight = Self::dynamic_weight(task.dyn_total_items(), task.dyn_size); log!( @@ -793,8 +879,9 @@ pub mod pallet { .saturating_add(T::WeightInfo::process_top_key(size)) } - /// Put a stop to all ongoing migrations. - fn halt() { + /// Put a stop to all ongoing migrations and logs an error. + fn halt(msg: &E) { + log!(error, "migration halted due to: {:?}", msg); AutoLimits::::kill(); Self::deposit_event(Event::::Halted); } @@ -815,7 +902,7 @@ pub mod pallet { fn transform_child_key_or_halt(root: &Vec) -> &[u8] { let key = Self::transform_child_key(root); if key.is_none() { - Self::halt(); + Self::halt("bad child root key"); } key.unwrap_or_default() } @@ -858,7 +945,7 @@ mod benchmarks { continue_migrate_wrong_witness { let null = MigrationLimits::default(); let caller = frame_benchmarking::whitelisted_caller(); - let bad_witness = MigrationTask { progress_top: Progress::LastKey(vec![1u8]), ..Default::default() }; + let bad_witness = MigrationTask { progress_top: Progress::LastKey(vec![1u8].try_into().unwrap()), ..Default::default() }; }: { assert!( StateTrieMigration::::continue_migrate( @@ -1046,6 +1133,7 @@ mod mock { parameter_types! { pub const SignedDepositPerItem: u64 = 1; pub const SignedDepositBase: u64 = 5; + pub const MigrationMaxKeyLen: u32 = 512; } impl pallet_balances::Config for Test { @@ -1064,6 +1152,7 @@ mod mock { type Event = Event; type ControlOrigin = EnsureRoot; type Currency = Balances; + type MaxKeyLen = MigrationMaxKeyLen; type SignedDepositPerItem = SignedDepositPerItem; type SignedDepositBase = SignedDepositBase; type SignedFilter = EnsureSigned; @@ -1171,6 +1260,7 @@ mod mock { #[cfg(test)] mod test { use super::{mock::*, *}; + use frame_support::{bounded_vec, dispatch::*}; use sp_runtime::{traits::Bounded, StateVersion}; #[test] @@ -1185,6 +1275,80 @@ mod test { assert_ne!(root1, root2); } + #[test] + fn halts_if_top_key_too_long() { + let bad_key = vec![1u8; MigrationMaxKeyLen::get() as usize + 1]; + let bad_top_keys = vec![(bad_key.clone(), vec![])]; + + new_test_ext(StateVersion::V0, true, Some(bad_top_keys), None).execute_with(|| { + System::set_block_number(1); + assert_eq!(MigrationProcess::::get(), Default::default()); + + // Allow signed migrations. + SignedMigrationMaxLimits::::put(MigrationLimits { size: 1 << 20, item: 50 }); + + // fails if the top key is too long. + frame_support::assert_err_with_weight!( + StateTrieMigration::continue_migrate( + Origin::signed(1), + MigrationLimits { item: 50, size: 1 << 20 }, + Bounded::max_value(), + MigrationProcess::::get() + ), + Error::::KeyTooLong, + Some(2000000), + ); + // The auto migration halted. + System::assert_last_event(crate::Event::Halted {}.into()); + // Limits are killed. + assert!(AutoLimits::::get().is_none()); + + // Calling `migrate_until_exhaustion` also fails. + let mut task = StateTrieMigration::migration_process(); + let result = task.migrate_until_exhaustion( + StateTrieMigration::signed_migration_max_limits().unwrap(), + ); + assert!(result.is_err()); + }); + } + + #[test] + fn halts_if_child_key_too_long() { + let bad_key = vec![1u8; MigrationMaxKeyLen::get() as usize + 1]; + let bad_child_keys = vec![(bad_key.clone(), vec![], vec![])]; + + new_test_ext(StateVersion::V0, true, None, Some(bad_child_keys)).execute_with(|| { + System::set_block_number(1); + assert_eq!(MigrationProcess::::get(), Default::default()); + + // Allow signed migrations. + SignedMigrationMaxLimits::::put(MigrationLimits { size: 1 << 20, item: 50 }); + + // fails if the top key is too long. + frame_support::assert_err_with_weight!( + StateTrieMigration::continue_migrate( + Origin::signed(1), + MigrationLimits { item: 50, size: 1 << 20 }, + Bounded::max_value(), + MigrationProcess::::get() + ), + Error::::KeyTooLong, + Some(2000000), + ); + // The auto migration halted. + System::assert_last_event(crate::Event::Halted {}.into()); + // Limits are killed. + assert!(AutoLimits::::get().is_none()); + + // Calling `migrate_until_exhaustion` also fails. + let mut task = StateTrieMigration::migration_process(); + let result = task.migrate_until_exhaustion( + StateTrieMigration::signed_migration_max_limits().unwrap(), + ); + assert!(result.is_err()); + }); + } + #[test] fn detects_value_in_empty_top_key() { let limit = MigrationLimits { item: 1, size: 1000 }; @@ -1319,7 +1483,7 @@ mod test { MigrationLimits { item: 5, size: 100 }, 100, MigrationTask { - progress_top: Progress::LastKey(vec![1u8]), + progress_top: Progress::LastKey(bounded_vec![1u8]), ..Default::default() } ), @@ -1330,9 +1494,10 @@ mod test { while !MigrationProcess::::get().finished() { // first we compute the task to get the accurate consumption. let mut task = StateTrieMigration::migration_process(); - task.migrate_until_exhaustion( + let result = task.migrate_until_exhaustion( StateTrieMigration::signed_migration_max_limits().unwrap(), ); + assert!(result.is_ok()); frame_support::assert_ok!(StateTrieMigration::continue_migrate( Origin::signed(1), @@ -1453,6 +1618,7 @@ pub(crate) mod remote_tests { use sp_runtime::traits::{Block as BlockT, HashFor, Header as _, One}; use thousands::Separable; + #[allow(dead_code)] fn run_to_block>( n: ::BlockNumber, ) -> (H256, u64) { @@ -1474,6 +1640,7 @@ pub(crate) mod remote_tests { /// Run the entire migration, against the given `Runtime`, until completion. /// /// This will print some very useful statistics, make sure [`crate::LOG_TARGET`] is enabled. + #[allow(dead_code)] pub(crate) async fn run_with_limits< Runtime: crate::Config, Block: BlockT + serde::de::DeserializeOwned, @@ -1571,8 +1738,9 @@ mod remote_tests_local { remote_tests::run_with_limits, *, }; - use remote_externalities::{Mode, OfflineConfig, OnlineConfig}; + use remote_externalities::{Mode, OfflineConfig, OnlineConfig, SnapshotConfig}; use sp_runtime::traits::Bounded; + use std::env::var as env_var; // we only use the hash type from this, so using the mock should be fine. type Extrinsic = sp_runtime::testing::TestXt; @@ -1580,14 +1748,13 @@ mod remote_tests_local { #[tokio::test] async fn on_initialize_migration() { + let snap: SnapshotConfig = env_var("SNAP").expect("Need SNAP env var").into(); + let ws_api = env_var("WS_API").expect("Need WS_API env var").into(); + sp_tracing::try_init_simple(); let mode = Mode::OfflineOrElseOnline( - OfflineConfig { state_snapshot: env!("SNAP").to_owned().into() }, - OnlineConfig { - transport: std::env!("WS_API").to_owned().into(), - state_snapshot: Some(env!("SNAP").to_owned().into()), - ..Default::default() - }, + OfflineConfig { state_snapshot: snap.clone() }, + OnlineConfig { transport: ws_api, state_snapshot: Some(snap), ..Default::default() }, ); // item being the bottleneck diff --git a/substrate/frame/support/src/lib.rs b/substrate/frame/support/src/lib.rs index fa37fa5cda..96bc1257c7 100644 --- a/substrate/frame/support/src/lib.rs +++ b/substrate/frame/support/src/lib.rs @@ -762,7 +762,7 @@ macro_rules! assert_err_with_weight { ($call:expr, $err:expr, $weight:expr $(,)? ) => { if let Err(dispatch_err_with_post) = $call { $crate::assert_err!($call.map(|_| ()).map_err(|e| e.error), $err); - assert_eq!(dispatch_err_with_post.post_info.actual_weight, $weight.into()); + assert_eq!(dispatch_err_with_post.post_info.actual_weight, $weight); } else { panic!("expected Err(_), got Ok(_).") }