From 7607b9258cc8ee5a5c685fa1db499b4d909c3001 Mon Sep 17 00:00:00 2001 From: Francisco Aguirre Date: Wed, 29 May 2024 20:57:17 +0100 Subject: [PATCH 01/18] Change `XcmDryRunApi::dry_run_extrinsic` to take a call instead (#4621) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the new `XcmDryRunApi` runtime API introduced in https://github.com/paritytech/polkadot-sdk/pull/3872. Taking an extrinsic means the frontend has to sign first to dry-run and once again to submit. This is bad UX which is solved by taking an `origin` and a `call`. This also has the benefit of being able to dry-run as any account, since it needs no signature. This is a breaking change since I changed `dry_run_extrinsic` to `dry_run_call`, however, this API is still only on testnets. The crates are bumped accordingly. As a part of this PR, I changed the name of the API from `XcmDryRunApi` to just `DryRunApi`, since it can be used for general dry-running :) Step towards https://github.com/paritytech/polkadot-sdk/issues/690. Example of calling the API with PAPI, not the best code, just testing :) ```ts // We just build a call, the arguments make it look very big though. const call = localApi.tx.XcmPallet.transfer_assets({ dest: XcmVersionedLocation.V4({ parents: 0, interior: XcmV4Junctions.X1(XcmV4Junction.Parachain(1000)) }), beneficiary: XcmVersionedLocation.V4({ parents: 0, interior: XcmV4Junctions.X1(XcmV4Junction.AccountId32({ network: undefined, id: Binary.fromBytes(encodeAccount(account.address)) })) }), weight_limit: XcmV3WeightLimit.Unlimited(), assets: XcmVersionedAssets.V4([{ id: { parents: 0, interior: XcmV4Junctions.Here() }, fun: XcmV3MultiassetFungibility.Fungible(1_000_000_000_000n) } ]), fee_asset_item: 0, }); // We call the API passing in a signed origin const result = await localApi.apis.XcmDryRunApi.dry_run_call( WestendRuntimeOriginCaller.system(DispatchRawOrigin.Signed(account.address)), call.decodedCall ); if (result.success && result.value.execution_result.success) { // We find the forwarded XCM we want. The first one going to AssetHub in this case. const xcmsToAssetHub = result.value.forwarded_xcms.find(([location, _]) => ( location.type === "V4" && location.value.parents === 0 && location.value.interior.type === "X1" && location.value.interior.value.type === "Parachain" && location.value.interior.value.value === 1000 ))!; // We can even find the delivery fees for that forwarded XCM. const deliveryFeesQuery = await localApi.apis.XcmPaymentApi.query_delivery_fees(xcmsToAssetHub[0], xcmsToAssetHub[1][0]); if (deliveryFeesQuery.success) { const amount = deliveryFeesQuery.value.type === "V4" && deliveryFeesQuery.value.value[0].fun.type === "Fungible" && deliveryFeesQuery.value.value[0].fun.value.valueOf() || 0n; // We store them in state somewhere. setDeliveryFees(formatAmount(BigInt(amount))); } } ``` --------- Co-authored-by: Bastian Köcher --- polkadot/xcm/pallet-xcm/src/lib.rs | 71 ++++++++++++++++-------------- 1 file changed, 39 insertions(+), 32 deletions(-) diff --git a/polkadot/xcm/pallet-xcm/src/lib.rs b/polkadot/xcm/pallet-xcm/src/lib.rs index 668f07c52ce3..8c0bf39b1641 100644 --- a/polkadot/xcm/pallet-xcm/src/lib.rs +++ b/polkadot/xcm/pallet-xcm/src/lib.rs @@ -836,7 +836,7 @@ pub mod pallet { if Self::request_version_notify(dest).is_ok() { // TODO: correct weights. weight_used.saturating_accrue(T::DbWeight::get().reads_writes(1, 1)); - break + break; } } } @@ -884,8 +884,9 @@ pub mod pallet { timeout, maybe_match_querier: Some(Location::here().into()), }, - VersionNotifier { origin, is_active } => - QueryStatus::VersionNotifier { origin, is_active }, + VersionNotifier { origin, is_active } => { + QueryStatus::VersionNotifier { origin, is_active } + }, Ready { response, at } => QueryStatus::Ready { response, at }, } } @@ -1689,8 +1690,9 @@ impl Pallet { fees, weight_limit, )?, - TransferType::RemoteReserve(_) => - return Err(Error::::InvalidAssetUnsupportedReserve.into()), + TransferType::RemoteReserve(_) => { + return Err(Error::::InvalidAssetUnsupportedReserve.into()) + }, }; FeesHandling::Separate { local_xcm, remote_xcm } }; @@ -2239,7 +2241,7 @@ impl Pallet { } weight_used.saturating_accrue(sv_migrate_weight); if weight_used.any_gte(weight_cutoff) { - return (weight_used, Some(stage)) + return (weight_used, Some(stage)); } } } @@ -2253,7 +2255,7 @@ impl Pallet { } weight_used.saturating_accrue(vn_migrate_weight); if weight_used.any_gte(weight_cutoff) { - return (weight_used, Some(stage)) + return (weight_used, Some(stage)); } } } @@ -2275,7 +2277,7 @@ impl Pallet { // We don't early return here since we need to be certain that we // make some progress. weight_used.saturating_accrue(vnt_already_notified_weight); - continue + continue; }, }; let response = Response::Version(xcm_version); @@ -2301,7 +2303,7 @@ impl Pallet { weight_used.saturating_accrue(vnt_notify_weight); if weight_used.any_gte(weight_cutoff) { let last = Some(iter.last_raw_key().into()); - return (weight_used, Some(NotifyCurrentTargets(last))) + return (weight_used, Some(NotifyCurrentTargets(last))); } } stage = MigrateAndNotifyOldTargets; @@ -2319,9 +2321,9 @@ impl Pallet { }); weight_used.saturating_accrue(vnt_migrate_fail_weight); if weight_used.any_gte(weight_cutoff) { - return (weight_used, Some(stage)) + return (weight_used, Some(stage)); } - continue + continue; }, }; @@ -2362,7 +2364,7 @@ impl Pallet { weight_used.saturating_accrue(vnt_notify_migrate_weight); } if weight_used.any_gte(weight_cutoff) { - return (weight_used, Some(stage)) + return (weight_used, Some(stage)); } } } @@ -2688,7 +2690,7 @@ impl Pallet { // if migration has been already scheduled, everything is ok and data will be eventually // migrated if CurrentMigration::::exists() { - return Ok(()) + return Ok(()); } // if migration has NOT been scheduled yet, we need to check all operational data @@ -2985,7 +2987,7 @@ impl VersionChangeNotifier for Pallet { impl DropAssets for Pallet { fn drop_assets(origin: &Location, assets: AssetsInHolding, _context: &XcmContext) -> Weight { if assets.is_empty() { - return Weight::zero() + return Weight::zero(); } let versioned = VersionedAssets::from(Assets::from(assets)); let hash = BlakeTwo256::hash_of(&(&origin, &versioned)); @@ -3009,11 +3011,12 @@ impl ClaimAssets for Pallet { ) -> bool { let mut versioned = VersionedAssets::from(assets.clone()); match ticket.unpack() { - (0, [GeneralIndex(i)]) => + (0, [GeneralIndex(i)]) => { versioned = match versioned.into_version(*i as u32) { Ok(v) => v, Err(()) => return false, - }, + } + }, (0, []) => (), _ => return false, }; @@ -3028,7 +3031,7 @@ impl ClaimAssets for Pallet { origin: origin.clone(), assets: versioned, }); - return true + return true; } } @@ -3039,15 +3042,17 @@ impl OnResponse for Pallet { querier: Option<&Location>, ) -> bool { match Queries::::get(query_id) { - Some(QueryStatus::Pending { responder, maybe_match_querier, .. }) => - Location::try_from(responder).map_or(false, |r| origin == &r) && - maybe_match_querier.map_or(true, |match_querier| { + Some(QueryStatus::Pending { responder, maybe_match_querier, .. }) => { + Location::try_from(responder).map_or(false, |r| origin == &r) + && maybe_match_querier.map_or(true, |match_querier| { Location::try_from(match_querier).map_or(false, |match_querier| { querier.map_or(false, |q| q == &match_querier) }) - }), - Some(QueryStatus::VersionNotifier { origin: r, .. }) => - Location::try_from(r).map_or(false, |r| origin == &r), + }) + }, + Some(QueryStatus::VersionNotifier { origin: r, .. }) => { + Location::try_from(r).map_or(false, |r| origin == &r) + }, _ => false, } } @@ -3074,7 +3079,7 @@ impl OnResponse for Pallet { query_id, expected_location: Some(o), }); - return Weight::zero() + return Weight::zero(); }, _ => { Self::deposit_event(Event::InvalidResponder { @@ -3083,7 +3088,7 @@ impl OnResponse for Pallet { expected_location: None, }); // TODO #3735: Correct weight for this. - return Weight::zero() + return Weight::zero(); }, }; // TODO #3735: Check max_weight is correct. @@ -3116,7 +3121,7 @@ impl OnResponse for Pallet { origin: origin.clone(), query_id, }); - return Weight::zero() + return Weight::zero(); }, }; if querier.map_or(true, |q| q != &match_querier) { @@ -3126,7 +3131,7 @@ impl OnResponse for Pallet { expected_querier: match_querier, maybe_actual_querier: querier.cloned(), }); - return Weight::zero() + return Weight::zero(); } } let responder = match Location::try_from(responder) { @@ -3136,7 +3141,7 @@ impl OnResponse for Pallet { origin: origin.clone(), query_id, }); - return Weight::zero() + return Weight::zero(); }, }; if origin != responder { @@ -3145,7 +3150,7 @@ impl OnResponse for Pallet { query_id, expected_location: Some(responder), }); - return Weight::zero() + return Weight::zero(); } return match maybe_notify { Some((pallet_index, call_index)) => { @@ -3167,7 +3172,7 @@ impl OnResponse for Pallet { max_budgeted_weight: max_weight, }; Self::deposit_event(e); - return Weight::zero() + return Weight::zero(); } let dispatch_origin = Origin::Response(origin.clone()).into(); match call.dispatch(dispatch_origin) { @@ -3204,7 +3209,7 @@ impl OnResponse for Pallet { Queries::::insert(query_id, QueryStatus::Ready { response, at }); Weight::zero() }, - } + }; }, _ => { let e = Event::UnexpectedResponse { origin: origin.clone(), query_id }; @@ -3313,7 +3318,9 @@ where caller.try_into().and_then(|o| match o { Origin::Xcm(ref location) if F::contains(&location.clone().try_into().map_err(|_| o.clone().into())?) => - Ok(location.clone().try_into().map_err(|_| o.clone().into())?), + { + Ok(location.clone().try_into().map_err(|_| o.clone().into())?) + }, Origin::Xcm(location) => Err(Origin::Xcm(location).into()), o => Err(o.into()), }) From 53d131cf9bc41a548cb3614699009dbd64ef5769 Mon Sep 17 00:00:00 2001 From: Antonio Antonino Date: Wed, 22 May 2024 09:58:13 +0200 Subject: [PATCH 02/18] Add new generic to Executive --- substrate/frame/executive/src/lib.rs | 56 +++++++++++++++++++++------- 1 file changed, 42 insertions(+), 14 deletions(-) diff --git a/substrate/frame/executive/src/lib.rs b/substrate/frame/executive/src/lib.rs index 3028eaf318e0..2ac022855cdc 100644 --- a/substrate/frame/executive/src/lib.rs +++ b/substrate/frame/executive/src/lib.rs @@ -86,6 +86,12 @@ //! done by setting an optional generic parameter. The custom logic will be called before //! the on runtime upgrade logic of all modules is called. //! +//! ### Custom `TryState` logic +//! +//! You can add custom logic that should be called in your runtime on a try-state test. This is +//! done by setting an optional generic parameter after the `OnRuntimeUpgrade` type. The custom logic will be called before +//! the `try-state` logic of all modules is called. +//! //! ``` //! # use sp_runtime::generic; //! # use frame_executive as executive; @@ -115,7 +121,16 @@ //! } //! } //! -//! pub type Executive = executive::Executive; +//! struct CustomTryState; +//! impl frame_support::traits::TryState for CustomTryState { +//! +//! fn try_state(_: BlockNumber, _: Select) -> Result<(), sp_runtime::TryRuntimeError> { +//! // Do whatever you want. +//! Ok(()) +//! } +//! } +//! +//! pub type Executive = executive::Executive; //! ``` #[cfg(doc)] @@ -205,6 +220,8 @@ pub type OriginOf = as Dispatchable>::RuntimeOrigin; /// used to call hooks e.g. `on_initialize`. /// - `OnRuntimeUpgrade`: Custom logic that should be called after a runtime upgrade. Modules are /// already called by `AllPalletsWithSystem`. It will be called before all modules will be called. +/// - `TryState`: Custom logic that should be called when running `try-state` tests. Modules are +/// already called by `AllPalletsWithSystem`. It will be called before all modules will be called. pub struct Executive< System, Block, @@ -212,6 +229,7 @@ pub struct Executive< UnsignedValidator, AllPalletsWithSystem, OnRuntimeUpgrade = (), + TryState = (), >( PhantomData<( System, @@ -220,6 +238,7 @@ pub struct Executive< UnsignedValidator, AllPalletsWithSystem, OnRuntimeUpgrade, + TryState, )>, ); @@ -280,8 +299,17 @@ impl< + TryState> + TryDecodeEntireStorage, COnRuntimeUpgrade: OnRuntimeUpgrade, - > Executive -where + CTryState: frame_support::traits::TryState>, + > + Executive< + System, + Block, + Context, + UnsignedValidator, + AllPalletsWithSystem, + COnRuntimeUpgrade, + CTryState, + > where Block::Extrinsic: Checkable + Codec, CheckedOf: Applyable + GetDispatchInfo, CallOf: @@ -318,7 +346,7 @@ where // Check if there are any forbidden non-inherents in the block. if mode == ExtrinsicInclusionMode::OnlyInherents && extrinsics.len() > num_inherents { - return Err("Only inherents allowed".into()) + return Err("Only inherents allowed".into()); } let try_apply_extrinsic = |uxt: Block::Extrinsic| -> ApplyExtrinsicResult { @@ -343,7 +371,7 @@ where let r = Applyable::apply::(xt, &dispatch_info, encoded_len)?; if r.is_err() && dispatch_info.class == DispatchClass::Mandatory { - return Err(InvalidTransaction::BadMandatory.into()) + return Err(InvalidTransaction::BadMandatory.into()); } >::note_applied_extrinsic(&r, dispatch_info); @@ -359,7 +387,7 @@ where e, err, ); - break + break; } } @@ -377,7 +405,7 @@ where // run the try-state checks of all pallets, ensuring they don't alter any state. let _guard = frame_support::StorageNoopGuard::default(); - , >>::try_state(*header.number(), select.clone()) .map_err(|e| { @@ -456,7 +484,7 @@ where // Check all storage invariants: if checks.try_state() { - AllPalletsWithSystem::try_state( + <(CTryState, AllPalletsWithSystem)>::try_state( frame_system::Pallet::::block_number(), TryStateSelect::All, )?; @@ -621,9 +649,9 @@ where // Check that `parent_hash` is correct. let n = *header.number(); assert!( - n > BlockNumberFor::::zero() && - >::block_hash(n - BlockNumberFor::::one()) == - *header.parent_hash(), + n > BlockNumberFor::::zero() + && >::block_hash(n - BlockNumberFor::::one()) + == *header.parent_hash(), "Parent hash should be valid.", ); @@ -719,7 +747,7 @@ where /// ongoing MBMs. fn on_idle_hook(block_number: NumberFor) { if ::MultiBlockMigrator::ongoing() { - return + return; } let weight = >::block_weight(); @@ -803,7 +831,7 @@ where // The entire block should be discarded if an inherent fails to apply. Otherwise // it may open an attack vector. if r.is_err() && dispatch_info.class == DispatchClass::Mandatory { - return Err(InvalidTransaction::BadMandatory.into()) + return Err(InvalidTransaction::BadMandatory.into()); } >::note_applied_extrinsic(&r, dispatch_info); @@ -873,7 +901,7 @@ where }; if dispatch_info.class == DispatchClass::Mandatory { - return Err(InvalidTransaction::MandatoryValidation.into()) + return Err(InvalidTransaction::MandatoryValidation.into()); } within_span! { From 85b439ab8492ef66e4a4ffd768c69d3e6e76b255 Mon Sep 17 00:00:00 2001 From: Antonio Antonino Date: Mon, 27 May 2024 11:59:03 +0200 Subject: [PATCH 03/18] frame-executive code compiling --- out.txt | 0 substrate/frame/executive/src/lib.rs | 39 +++++++++++++----- .../src/pallet/expand/pallet_struct.rs | 10 +++++ substrate/frame/support/src/traits.rs | 2 +- .../frame/support/src/traits/metadata.rs | 20 +++++++++ .../support/src/traits/try_runtime/mod.rs | 41 ++++++++++--------- 6 files changed, 80 insertions(+), 32 deletions(-) create mode 100644 out.txt diff --git a/out.txt b/out.txt new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/substrate/frame/executive/src/lib.rs b/substrate/frame/executive/src/lib.rs index 2ac022855cdc..a3df8dae56d2 100644 --- a/substrate/frame/executive/src/lib.rs +++ b/substrate/frame/executive/src/lib.rs @@ -175,7 +175,7 @@ use frame_support::{ traits::{ BeforeAllRuntimeMigrations, EnsureInherentsAreFirst, ExecuteBlock, OffchainWorker, OnFinalize, OnIdle, OnInitialize, OnPoll, OnRuntimeUpgrade, PostInherents, - PostTransactions, PreInherents, + PostTransactions, PreInherents, StaticPartialEq, }, weights::{Weight, WeightMeter}, }; @@ -258,9 +258,17 @@ impl< + OffchainWorker> + OnPoll>, COnRuntimeUpgrade: OnRuntimeUpgrade, + CTryState, > ExecuteBlock - for Executive -where + for Executive< + System, + Block, + Context, + UnsignedValidator, + AllPalletsWithSystem, + COnRuntimeUpgrade, + CTryState, + > where Block::Extrinsic: Checkable + Codec, CheckedOf: Applyable + GetDispatchInfo, CallOf: @@ -297,9 +305,10 @@ impl< + OffchainWorker> + OnPoll> + TryState> - + TryDecodeEntireStorage, + + TryDecodeEntireStorage + + StaticPartialEq<[u8]>, COnRuntimeUpgrade: OnRuntimeUpgrade, - CTryState: frame_support::traits::TryState>, + CTryState: frame_support::traits::TryState> + StaticPartialEq<[u8]>, > Executive< System, @@ -484,10 +493,9 @@ impl< // Check all storage invariants: if checks.try_state() { - <(CTryState, AllPalletsWithSystem)>::try_state( - frame_system::Pallet::::block_number(), - TryStateSelect::All, - )?; + <(CTryState, AllPalletsWithSystem) as frame_support::traits::TryState< + BlockNumberFor, + >>::try_state(frame_system::Pallet::::block_number(), TryStateSelect::All)?; } Ok(before_all_weight.saturating_add(try_on_runtime_upgrade_weight)) @@ -542,8 +550,17 @@ impl< + OffchainWorker> + OnPoll>, COnRuntimeUpgrade: OnRuntimeUpgrade, - > Executive -where + CTryState, + > + Executive< + System, + Block, + Context, + UnsignedValidator, + AllPalletsWithSystem, + COnRuntimeUpgrade, + CTryState, + > where Block::Extrinsic: Checkable + Codec, CheckedOf: Applyable + GetDispatchInfo, CallOf: diff --git a/substrate/frame/support/procedural/src/pallet/expand/pallet_struct.rs b/substrate/frame/support/procedural/src/pallet/expand/pallet_struct.rs index 7cdf6bde9de8..6b49f1717b29 100644 --- a/substrate/frame/support/procedural/src/pallet/expand/pallet_struct.rs +++ b/substrate/frame/support/procedural/src/pallet/expand/pallet_struct.rs @@ -284,6 +284,16 @@ pub fn expand_pallet_struct(def: &mut Def) -> proc_macro2::TokenStream { } } + // Implement `StaticPartialEq` for `Pallet` + impl<#type_impl_gen> #frame_support::traits::StaticPartialEq<[u8]> + for #pallet_ident<#type_use_gen> + #config_where_clause + { + fn eq(other: &[u8]) -> bool { + Self::name().as_bytes() == other + } + } + #storage_info #whitelisted_storage_keys_impl ) diff --git a/substrate/frame/support/src/traits.rs b/substrate/frame/support/src/traits.rs index a423656c394f..f49c7f2ac1aa 100644 --- a/substrate/frame/support/src/traits.rs +++ b/substrate/frame/support/src/traits.rs @@ -78,7 +78,7 @@ mod metadata; pub use metadata::{ CallMetadata, CrateVersion, GetCallIndex, GetCallMetadata, GetCallName, GetStorageVersion, NoStorageVersionSet, PalletInfo, PalletInfoAccess, PalletInfoData, PalletsInfoAccess, - StorageVersion, STORAGE_VERSION_STORAGE_KEY_POSTFIX, + StaticPartialEq, StorageVersion, STORAGE_VERSION_STORAGE_KEY_POSTFIX, }; mod hooks; diff --git a/substrate/frame/support/src/traits/metadata.rs b/substrate/frame/support/src/traits/metadata.rs index 8bda4186bc96..3ddbd26ed51c 100644 --- a/substrate/frame/support/src/traits/metadata.rs +++ b/substrate/frame/support/src/traits/metadata.rs @@ -322,6 +322,26 @@ pub trait GetStorageVersion { fn on_chain_storage_version() -> StorageVersion; } +pub trait StaticPartialEq +where + Rhs: ?Sized, +{ + fn eq(_other: &Rhs) -> bool; +} + +#[cfg_attr(all(not(feature = "tuples-96"), not(feature = "tuples-128")), impl_for_tuples(64))] +#[cfg_attr(all(feature = "tuples-96", not(feature = "tuples-128")), impl_for_tuples(96))] +#[cfg_attr(feature = "tuples-128", impl_for_tuples(128))] +impl StaticPartialEq for Tuple +where + Rhs: ?Sized, +{ + fn eq(other: &Rhs) -> bool { + for_tuples!( #( if Tuple::eq(other) { return true; } )* ); + return false; + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/substrate/frame/support/src/traits/try_runtime/mod.rs b/substrate/frame/support/src/traits/try_runtime/mod.rs index c1bf1feb19e5..b35bb0221b9d 100644 --- a/substrate/frame/support/src/traits/try_runtime/mod.rs +++ b/substrate/frame/support/src/traits/try_runtime/mod.rs @@ -36,9 +36,9 @@ pub enum Select { All, /// Run a fixed number of them in a round robin manner. RoundRobin(u32), - /// Run only pallets who's name matches the given list. + /// Run only logic whose identifier is included in the given list. /// - /// Pallet names are obtained from [`super::PalletInfoAccess`]. + /// For pallets, their identifiers are obtained from [`super::PalletInfoAccess`]. Only(Vec>), } @@ -79,7 +79,7 @@ impl sp_std::str::FromStr for Select { match s { "all" | "All" => Ok(Select::All), "none" | "None" => Ok(Select::None), - _ => + _ => { if s.starts_with("rr-") { let count = s .split_once('-') @@ -89,7 +89,8 @@ impl sp_std::str::FromStr for Select { } else { let pallets = s.split(',').map(|x| x.as_bytes().to_vec()).collect::>(); Ok(Select::Only(pallets)) - }, + } + }, } } } @@ -156,7 +157,7 @@ pub trait TryState { impl TryState for Tuple { - for_tuples!( where #( Tuple: crate::traits::PalletInfoAccess )* ); + for_tuples!( where #( Tuple: crate::traits::StaticPartialEq<[u8]> )* ); fn try_state(n: BlockNumber, targets: Select) -> Result<(), TryRuntimeError> { match targets { Select::None => Ok(()), @@ -187,7 +188,7 @@ impl TryState TryState { let try_state_fns: &[( - &'static str, + fn(&[u8]) -> bool, fn(BlockNumber, Select) -> Result<(), TryRuntimeError>, )] = &[for_tuples!( - #( (::name(), Tuple::try_state) ),* + #( (Tuple::eq, Tuple::try_state) ),* )]; let mut result = Ok(()); - pallet_names.iter().for_each(|pallet_name| { - if let Some((name, try_state_fn)) = - try_state_fns.iter().find(|(name, _)| name.as_bytes() == pallet_name) - { - result = result.and(try_state_fn(n.clone(), targets.clone())); - } else { - log::warn!( - "Pallet {:?} not found", - sp_std::str::from_utf8(pallet_name).unwrap_or_default() - ); - } - }); + // pallet_names.iter().for_each(|pallet_name| { + // if let Some((name, try_state_fn)) = + // try_state_fns.iter().find(|(name, _)| name.as_bytes() == pallet_name) + // { + // result = result.and(try_state_fn(n.clone(), targets.clone())); + // } else { + // log::warn!( + // "Pallet {:?} not found", + // sp_std::str::from_utf8(pallet_name).unwrap_or_default() + // ); + // } + // }); result }, From da5e90255889a53778973430cb1a88cc4bee399a Mon Sep 17 00:00:00 2001 From: Antonio Antonino Date: Wed, 29 May 2024 10:00:37 +0200 Subject: [PATCH 04/18] frame-support compiling --- .../src/pallet/expand/pallet_struct.rs | 10 ++-- substrate/frame/support/src/traits.rs | 2 +- .../support/src/traits/try_runtime/mod.rs | 51 +++++++++++++------ 3 files changed, 43 insertions(+), 20 deletions(-) diff --git a/substrate/frame/support/procedural/src/pallet/expand/pallet_struct.rs b/substrate/frame/support/procedural/src/pallet/expand/pallet_struct.rs index 6b49f1717b29..a694cd416d2b 100644 --- a/substrate/frame/support/procedural/src/pallet/expand/pallet_struct.rs +++ b/substrate/frame/support/procedural/src/pallet/expand/pallet_struct.rs @@ -284,13 +284,15 @@ pub fn expand_pallet_struct(def: &mut Def) -> proc_macro2::TokenStream { } } - // Implement `StaticPartialEq` for `Pallet` - impl<#type_impl_gen> #frame_support::traits::StaticPartialEq<[u8]> + // Implement `StaticPartialEq` for `Pallet` + #[cfg(feature = "try-runtime")] + impl<#type_impl_gen> #frame_support::traits::StaticPartialEq for #pallet_ident<#type_use_gen> #config_where_clause { - fn eq(other: &[u8]) -> bool { - Self::name().as_bytes() == other + fn eq(other: &frame_support::traits::TryStateIdentifier) -> bool { + use sp_std::ops::Deref; + Self::name().as_bytes() == other.deref() } } diff --git a/substrate/frame/support/src/traits.rs b/substrate/frame/support/src/traits.rs index f49c7f2ac1aa..c64e29f9c063 100644 --- a/substrate/frame/support/src/traits.rs +++ b/substrate/frame/support/src/traits.rs @@ -135,5 +135,5 @@ mod try_runtime; #[cfg(feature = "try-runtime")] pub use try_runtime::{ Select as TryStateSelect, TryDecodeEntireStorage, TryDecodeEntireStorageError, TryState, - UpgradeCheckSelect, + TryStateIdentifier, UpgradeCheckSelect, }; diff --git a/substrate/frame/support/src/traits/try_runtime/mod.rs b/substrate/frame/support/src/traits/try_runtime/mod.rs index b35bb0221b9d..e6e1e0b9618e 100644 --- a/substrate/frame/support/src/traits/try_runtime/mod.rs +++ b/substrate/frame/support/src/traits/try_runtime/mod.rs @@ -151,13 +151,32 @@ pub trait TryState { fn try_state(_: BlockNumber, _: Select) -> Result<(), TryRuntimeError>; } +/// The identifier for a `try-state` check. +/// It is used by the default `TryState` implementation on tuples for the `Select::Only` test filter. +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct TryStateIdentifier(Vec); + +impl sp_std::ops::Deref for TryStateIdentifier { + type Target = [u8]; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +impl From> for TryStateIdentifier { + fn from(value: Vec) -> Self { + Self(value) + } +} + #[cfg_attr(all(not(feature = "tuples-96"), not(feature = "tuples-128")), impl_for_tuples(64))] #[cfg_attr(all(feature = "tuples-96", not(feature = "tuples-128")), impl_for_tuples(96))] #[cfg_attr(all(feature = "tuples-128"), impl_for_tuples(128))] impl TryState for Tuple { - for_tuples!( where #( Tuple: crate::traits::StaticPartialEq<[u8]> )* ); + for_tuples!( where #( Tuple: crate::traits::StaticPartialEq )* ); fn try_state(n: BlockNumber, targets: Select) -> Result<(), TryRuntimeError> { match targets { Select::None => Ok(()), @@ -206,26 +225,28 @@ impl TryState { + Select::Only(ref try_state_identifiers) => { let try_state_fns: &[( - fn(&[u8]) -> bool, + fn(&TryStateIdentifier) -> bool, fn(BlockNumber, Select) -> Result<(), TryRuntimeError>, )] = &[for_tuples!( #( (Tuple::eq, Tuple::try_state) ),* )]; + let mut result = Ok(()); - // pallet_names.iter().for_each(|pallet_name| { - // if let Some((name, try_state_fn)) = - // try_state_fns.iter().find(|(name, _)| name.as_bytes() == pallet_name) - // { - // result = result.and(try_state_fn(n.clone(), targets.clone())); - // } else { - // log::warn!( - // "Pallet {:?} not found", - // sp_std::str::from_utf8(pallet_name).unwrap_or_default() - // ); - // } - // }); + try_state_identifiers.iter().for_each(|id| { + if let Some((_, try_state_fn)) = try_state_fns + .iter() + .find(|(eq_logic, _)| eq_logic(&TryStateIdentifier::from(id.clone()))) + { + result = result.and(try_state_fn(n.clone(), targets.clone())); + } else { + log::warn!( + "Try-state logic with identifier {:?} not found", + sp_std::str::from_utf8(id).unwrap_or_default() + ); + } + }); result }, From 871f87ae7bed1d90b6acb09d228d3b0118a0a28f Mon Sep 17 00:00:00 2001 From: Antonio Antonino Date: Wed, 29 May 2024 10:03:37 +0200 Subject: [PATCH 05/18] frame-executive compiling --- substrate/frame/executive/src/lib.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/substrate/frame/executive/src/lib.rs b/substrate/frame/executive/src/lib.rs index a3df8dae56d2..17ec6a962fba 100644 --- a/substrate/frame/executive/src/lib.rs +++ b/substrate/frame/executive/src/lib.rs @@ -175,7 +175,7 @@ use frame_support::{ traits::{ BeforeAllRuntimeMigrations, EnsureInherentsAreFirst, ExecuteBlock, OffchainWorker, OnFinalize, OnIdle, OnInitialize, OnPoll, OnRuntimeUpgrade, PostInherents, - PostTransactions, PreInherents, StaticPartialEq, + PostTransactions, PreInherents, }, weights::{Weight, WeightMeter}, }; @@ -306,9 +306,10 @@ impl< + OnPoll> + TryState> + TryDecodeEntireStorage - + StaticPartialEq<[u8]>, + + frame_support::traits::StaticPartialEq, COnRuntimeUpgrade: OnRuntimeUpgrade, - CTryState: frame_support::traits::TryState> + StaticPartialEq<[u8]>, + CTryState: frame_support::traits::TryState> + + frame_support::traits::StaticPartialEq, > Executive< System, From b962d31d4b7f4b0742f3a6faca7cca9fbd0121aa Mon Sep 17 00:00:00 2001 From: Antonio Antonino Date: Wed, 29 May 2024 11:27:52 +0200 Subject: [PATCH 06/18] frame-support compiling --- .../src/pallet/expand/pallet_struct.rs | 20 +++- substrate/frame/support/src/traits.rs | 4 +- .../support/src/traits/try_runtime/mod.rs | 105 +++++++++++------- 3 files changed, 83 insertions(+), 46 deletions(-) diff --git a/substrate/frame/support/procedural/src/pallet/expand/pallet_struct.rs b/substrate/frame/support/procedural/src/pallet/expand/pallet_struct.rs index a694cd416d2b..640e438450a3 100644 --- a/substrate/frame/support/procedural/src/pallet/expand/pallet_struct.rs +++ b/substrate/frame/support/procedural/src/pallet/expand/pallet_struct.rs @@ -284,15 +284,25 @@ pub fn expand_pallet_struct(def: &mut Def) -> proc_macro2::TokenStream { } } - // Implement `StaticPartialEq` for `Pallet` + // Implement `TryStateLogic` for `Pallet` #[cfg(feature = "try-runtime")] - impl<#type_impl_gen> #frame_support::traits::StaticPartialEq + impl<#type_impl_gen> #frame_support::traits::TryStateLogic> for #pallet_ident<#type_use_gen> #config_where_clause { - fn eq(other: &frame_support::traits::TryStateIdentifier) -> bool { - use sp_std::ops::Deref; - Self::name().as_bytes() == other.deref() + fn try_state(n: BlockNumberFor) -> Result<(), sp_runtime::TryRuntimeError> { + >>::try_state(n) + } + } + + // Implement `IdentifiableTryStateLogic` for `Pallet` + #[cfg(feature = "try-runtime")] + impl<#type_impl_gen> #frame_support::traits::IdentifiableTryStateLogic> + for #pallet_ident<#type_use_gen> + #config_where_clause + { + fn matches_id(id: &[u8]) -> bool { + ::name().as_bytes() == id } } diff --git a/substrate/frame/support/src/traits.rs b/substrate/frame/support/src/traits.rs index c64e29f9c063..eab53a5bb297 100644 --- a/substrate/frame/support/src/traits.rs +++ b/substrate/frame/support/src/traits.rs @@ -134,6 +134,6 @@ pub use tasks::Task; mod try_runtime; #[cfg(feature = "try-runtime")] pub use try_runtime::{ - Select as TryStateSelect, TryDecodeEntireStorage, TryDecodeEntireStorageError, TryState, - TryStateIdentifier, UpgradeCheckSelect, + IdentifiableTryStateLogic, Select as TryStateSelect, TryDecodeEntireStorage, + TryDecodeEntireStorageError, TryState, TryStateLogic, UpgradeCheckSelect, }; diff --git a/substrate/frame/support/src/traits/try_runtime/mod.rs b/substrate/frame/support/src/traits/try_runtime/mod.rs index e6e1e0b9618e..54845ca99b1b 100644 --- a/substrate/frame/support/src/traits/try_runtime/mod.rs +++ b/substrate/frame/support/src/traits/try_runtime/mod.rs @@ -140,6 +140,54 @@ impl core::str::FromStr for UpgradeCheckSelect { } } +pub trait TryStateLogic { + fn try_state(_: BlockNumber) -> Result<(), TryRuntimeError>; +} + +#[cfg_attr(all(not(feature = "tuples-96"), not(feature = "tuples-128")), impl_for_tuples(64))] +#[cfg_attr(all(feature = "tuples-96", not(feature = "tuples-128")), impl_for_tuples(96))] +#[cfg_attr(all(feature = "tuples-128"), impl_for_tuples(128))] +impl TryStateLogic for Tuple +where + BlockNumber: Clone + sp_std::fmt::Debug + AtLeast32BitUnsigned, +{ + fn try_state(n: BlockNumber) -> Result<(), TryRuntimeError> { + let mut errors = Vec::::new(); + + for_tuples!(#( + if let Err(err) = Tuple::try_state(n.clone()) { + errors.push(err); + } + )*); + + if !errors.is_empty() { + return Err("Detected errors while executing `try_state` checks. See logs for more \ + info." + .into()); + } + + Ok(()) + } +} + +pub trait IdentifiableTryStateLogic: TryStateLogic { + fn matches_id(_id: &[u8]) -> bool; +} + +#[cfg_attr(all(not(feature = "tuples-96"), not(feature = "tuples-128")), impl_for_tuples(64))] +#[cfg_attr(all(feature = "tuples-96", not(feature = "tuples-128")), impl_for_tuples(96))] +#[cfg_attr(all(feature = "tuples-128"), impl_for_tuples(128))] +impl IdentifiableTryStateLogic for Tuple +where + BlockNumber: Clone + sp_std::fmt::Debug + AtLeast32BitUnsigned, +{ + for_tuples!( where #( Tuple: TryStateLogic )* ); + fn matches_id(id: &[u8]) -> bool { + for_tuples!( #( if Tuple::matches_id(id) { return true; } )* ); + return false; + } +} + /// Execute some checks to ensure the internal state of a pallet is consistent. /// /// Usually, these checks should check all of the invariants that are expected to be held on all of @@ -151,43 +199,26 @@ pub trait TryState { fn try_state(_: BlockNumber, _: Select) -> Result<(), TryRuntimeError>; } -/// The identifier for a `try-state` check. -/// It is used by the default `TryState` implementation on tuples for the `Select::Only` test filter. -#[derive(Clone, Debug, Eq, PartialEq)] -pub struct TryStateIdentifier(Vec); - -impl sp_std::ops::Deref for TryStateIdentifier { - type Target = [u8]; - - fn deref(&self) -> &Self::Target { - &self.0 - } -} - -impl From> for TryStateIdentifier { - fn from(value: Vec) -> Self { - Self(value) +impl TryState for () { + fn try_state(_: BlockNumber, _: Select) -> Result<(), TryRuntimeError> { + Ok(()) } } -#[cfg_attr(all(not(feature = "tuples-96"), not(feature = "tuples-128")), impl_for_tuples(64))] -#[cfg_attr(all(feature = "tuples-96", not(feature = "tuples-128")), impl_for_tuples(96))] -#[cfg_attr(all(feature = "tuples-128"), impl_for_tuples(128))] -impl TryState - for Tuple +impl TryState for (T,) +where + BlockNumber: Clone + sp_std::fmt::Debug + AtLeast32BitUnsigned, + T: IdentifiableTryStateLogic, { - for_tuples!( where #( Tuple: crate::traits::StaticPartialEq )* ); fn try_state(n: BlockNumber, targets: Select) -> Result<(), TryRuntimeError> { match targets { Select::None => Ok(()), Select::All => { let mut errors = Vec::::new(); - for_tuples!(#( - if let Err(err) = Tuple::try_state(n.clone(), targets.clone()) { - errors.push(err); - } - )*); + if let Err(err) = T::try_state(n.clone()) { + errors.push(err); + } if !errors.is_empty() { log::error!( @@ -213,33 +244,29 @@ impl TryState { - let functions: &[fn(BlockNumber, Select) -> Result<(), TryRuntimeError>] = - &[for_tuples!(#( Tuple::try_state ),*)]; + let functions: &[fn(BlockNumber) -> Result<(), TryRuntimeError>] = &[T::try_state]; let skip = n.clone() % (functions.len() as u32).into(); let skip: u32 = skip.try_into().unwrap_or_else(|_| sp_runtime::traits::Bounded::max_value()); let mut result = Ok(()); for try_state_fn in functions.iter().cycle().skip(skip as usize).take(len as usize) { - result = result.and(try_state_fn(n.clone(), targets.clone())); + result = result.and(try_state_fn(n.clone())); } result }, Select::Only(ref try_state_identifiers) => { let try_state_fns: &[( - fn(&TryStateIdentifier) -> bool, - fn(BlockNumber, Select) -> Result<(), TryRuntimeError>, - )] = &[for_tuples!( - #( (Tuple::eq, Tuple::try_state) ),* - )]; + fn(&[u8]) -> bool, + fn(BlockNumber) -> Result<(), TryRuntimeError>, + )] = &[(T::matches_id, T::try_state)]; let mut result = Ok(()); try_state_identifiers.iter().for_each(|id| { - if let Some((_, try_state_fn)) = try_state_fns - .iter() - .find(|(eq_logic, _)| eq_logic(&TryStateIdentifier::from(id.clone()))) + if let Some((_, try_state_fn)) = + try_state_fns.iter().find(|(eq_logic, _)| eq_logic(id.as_slice())) { - result = result.and(try_state_fn(n.clone(), targets.clone())); + result = result.and(try_state_fn(n.clone())); } else { log::warn!( "Try-state logic with identifier {:?} not found", From 4760e8fd9a568b02af58d9d07954cf451d5e30bf Mon Sep 17 00:00:00 2001 From: Antonio Antonino Date: Wed, 29 May 2024 11:33:09 +0200 Subject: [PATCH 07/18] frame-executive compiling as well --- substrate/frame/executive/src/lib.rs | 27 ++++++++++--------- .../support/src/traits/try_runtime/mod.rs | 15 +++++++---- 2 files changed, 25 insertions(+), 17 deletions(-) diff --git a/substrate/frame/executive/src/lib.rs b/substrate/frame/executive/src/lib.rs index 17ec6a962fba..d41c235ac495 100644 --- a/substrate/frame/executive/src/lib.rs +++ b/substrate/frame/executive/src/lib.rs @@ -194,7 +194,10 @@ use sp_std::{marker::PhantomData, prelude::*}; #[cfg(feature = "try-runtime")] use ::{ frame_support::{ - traits::{TryDecodeEntireStorage, TryDecodeEntireStorageError, TryState}, + traits::{ + IdentifiableTryStateLogic, TryDecodeEntireStorage, TryDecodeEntireStorageError, + TryState, + }, StorageNoopGuard, }, frame_try_runtime::{TryStateSelect, UpgradeCheckSelect}, @@ -304,12 +307,10 @@ impl< + OnFinalize> + OffchainWorker> + OnPoll> - + TryState> - + TryDecodeEntireStorage - + frame_support::traits::StaticPartialEq, + + IdentifiableTryStateLogic> + + TryDecodeEntireStorage, COnRuntimeUpgrade: OnRuntimeUpgrade, - CTryState: frame_support::traits::TryState> - + frame_support::traits::StaticPartialEq, + CTryState: IdentifiableTryStateLogic>, > Executive< System, @@ -415,9 +416,10 @@ impl< // run the try-state checks of all pallets, ensuring they don't alter any state. let _guard = frame_support::StorageNoopGuard::default(); - <(CTryState, AllPalletsWithSystem) as frame_support::traits::TryState< - BlockNumberFor, - >>::try_state(*header.number(), select.clone()) + <(CTryState, AllPalletsWithSystem) as TryState>>::try_state( + *header.number(), + select.clone(), + ) .map_err(|e| { log::error!(target: LOG_TARGET, "failure: {:?}", e); e @@ -494,9 +496,10 @@ impl< // Check all storage invariants: if checks.try_state() { - <(CTryState, AllPalletsWithSystem) as frame_support::traits::TryState< - BlockNumberFor, - >>::try_state(frame_system::Pallet::::block_number(), TryStateSelect::All)?; + <(CTryState, AllPalletsWithSystem) as TryState>>::try_state( + frame_system::Pallet::::block_number(), + TryStateSelect::All, + )?; } Ok(before_all_weight.saturating_add(try_on_runtime_upgrade_weight)) diff --git a/substrate/frame/support/src/traits/try_runtime/mod.rs b/substrate/frame/support/src/traits/try_runtime/mod.rs index 54845ca99b1b..f46189283795 100644 --- a/substrate/frame/support/src/traits/try_runtime/mod.rs +++ b/substrate/frame/support/src/traits/try_runtime/mod.rs @@ -205,10 +205,11 @@ impl TryState for () { } } -impl TryState for (T,) +impl TryState for (T1, T2) where BlockNumber: Clone + sp_std::fmt::Debug + AtLeast32BitUnsigned, - T: IdentifiableTryStateLogic, + T1: IdentifiableTryStateLogic, + T2: IdentifiableTryStateLogic, { fn try_state(n: BlockNumber, targets: Select) -> Result<(), TryRuntimeError> { match targets { @@ -216,7 +217,10 @@ where Select::All => { let mut errors = Vec::::new(); - if let Err(err) = T::try_state(n.clone()) { + if let Err(err) = T1::try_state(n.clone()) { + errors.push(err); + } + if let Err(err) = T2::try_state(n.clone()) { errors.push(err); } @@ -244,7 +248,8 @@ where Ok(()) }, Select::RoundRobin(len) => { - let functions: &[fn(BlockNumber) -> Result<(), TryRuntimeError>] = &[T::try_state]; + let functions: &[fn(BlockNumber) -> Result<(), TryRuntimeError>] = + &[T1::try_state, T2::try_state]; let skip = n.clone() % (functions.len() as u32).into(); let skip: u32 = skip.try_into().unwrap_or_else(|_| sp_runtime::traits::Bounded::max_value()); @@ -259,7 +264,7 @@ where let try_state_fns: &[( fn(&[u8]) -> bool, fn(BlockNumber) -> Result<(), TryRuntimeError>, - )] = &[(T::matches_id, T::try_state)]; + )] = &[(T1::matches_id, T1::try_state), (T2::matches_id, T2::try_state)]; let mut result = Ok(()); try_state_identifiers.iter().for_each(|id| { From 4ddee80045ca678f5e84c4938f0bcfed55da7784 Mon Sep 17 00:00:00 2001 From: Antonio Antonino Date: Wed, 29 May 2024 11:33:53 +0200 Subject: [PATCH 08/18] Add comment --- substrate/frame/support/src/traits/try_runtime/mod.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/substrate/frame/support/src/traits/try_runtime/mod.rs b/substrate/frame/support/src/traits/try_runtime/mod.rs index f46189283795..bf1749e39336 100644 --- a/substrate/frame/support/src/traits/try_runtime/mod.rs +++ b/substrate/frame/support/src/traits/try_runtime/mod.rs @@ -205,6 +205,7 @@ impl TryState for () { } } +// Did not find a way to implement a trait on tuple without requiring the tuples to implement the very same trait being implemented. impl TryState for (T1, T2) where BlockNumber: Clone + sp_std::fmt::Debug + AtLeast32BitUnsigned, From 5401db633ba84dbfec45adda2a1a1460dedffe72 Mon Sep 17 00:00:00 2001 From: Antonio Antonino Date: Wed, 29 May 2024 11:56:14 +0200 Subject: [PATCH 09/18] Rename generics --- substrate/frame/executive/src/lib.rs | 9 +++-- .../support/src/traits/try_runtime/mod.rs | 34 +++++++++++-------- 2 files changed, 27 insertions(+), 16 deletions(-) diff --git a/substrate/frame/executive/src/lib.rs b/substrate/frame/executive/src/lib.rs index d41c235ac495..cee01ac08165 100644 --- a/substrate/frame/executive/src/lib.rs +++ b/substrate/frame/executive/src/lib.rs @@ -122,12 +122,17 @@ //! } //! //! struct CustomTryState; -//! impl frame_support::traits::TryState for CustomTryState { +//! impl frame_support::traits::IdentifiableTryStateLogic for CustomTryState { //! -//! fn try_state(_: BlockNumber, _: Select) -> Result<(), sp_runtime::TryRuntimeError> { +//! fn try_state(_: BlockNumber) -> Result<(), sp_runtime::TryRuntimeError> { //! // Do whatever you want. //! Ok(()) //! } +//! +//! fn matches_id(id: &[u8]) -> bool { +//! // Used to decide whether to include this or not when `Select::Only` is specified. +//! id == b"my_custom_try_state" +//! } //! } //! //! pub type Executive = executive::Executive; diff --git a/substrate/frame/support/src/traits/try_runtime/mod.rs b/substrate/frame/support/src/traits/try_runtime/mod.rs index bf1749e39336..21c64b0af0ba 100644 --- a/substrate/frame/support/src/traits/try_runtime/mod.rs +++ b/substrate/frame/support/src/traits/try_runtime/mod.rs @@ -140,6 +140,12 @@ impl core::str::FromStr for UpgradeCheckSelect { } } +/// Execute some checks to ensure the internal state of a pallet is consistent. +/// +/// Usually, these checks should check all of the invariants that are expected to be held on all of +/// the storage items of your pallet. +/// +/// This hook should not alter any storage. pub trait TryStateLogic { fn try_state(_: BlockNumber) -> Result<(), TryRuntimeError>; } @@ -160,16 +166,13 @@ where } )*); - if !errors.is_empty() { - return Err("Detected errors while executing `try_state` checks. See logs for more \ - info." - .into()); - } - Ok(()) } } +/// Logic executed when `try-state` filters are provided in the `try-runtime` CLI. +/// +/// Returning `true` for the provided ID will include this `try-state` logic in the overall tests performed. pub trait IdentifiableTryStateLogic: TryStateLogic { fn matches_id(_id: &[u8]) -> bool; } @@ -205,12 +208,12 @@ impl TryState for () { } } -// Did not find a way to implement a trait on tuple without requiring the tuples to implement the very same trait being implemented. -impl TryState for (T1, T2) +impl TryState + for (AdditionalHooks, AllPallets) where BlockNumber: Clone + sp_std::fmt::Debug + AtLeast32BitUnsigned, - T1: IdentifiableTryStateLogic, - T2: IdentifiableTryStateLogic, + AdditionalHooks: IdentifiableTryStateLogic, + AllPallets: IdentifiableTryStateLogic, { fn try_state(n: BlockNumber, targets: Select) -> Result<(), TryRuntimeError> { match targets { @@ -218,10 +221,10 @@ where Select::All => { let mut errors = Vec::::new(); - if let Err(err) = T1::try_state(n.clone()) { + if let Err(err) = AdditionalHooks::try_state(n.clone()) { errors.push(err); } - if let Err(err) = T2::try_state(n.clone()) { + if let Err(err) = AllPallets::try_state(n.clone()) { errors.push(err); } @@ -250,7 +253,7 @@ where }, Select::RoundRobin(len) => { let functions: &[fn(BlockNumber) -> Result<(), TryRuntimeError>] = - &[T1::try_state, T2::try_state]; + &[AdditionalHooks::try_state, AllPallets::try_state]; let skip = n.clone() % (functions.len() as u32).into(); let skip: u32 = skip.try_into().unwrap_or_else(|_| sp_runtime::traits::Bounded::max_value()); @@ -265,7 +268,10 @@ where let try_state_fns: &[( fn(&[u8]) -> bool, fn(BlockNumber) -> Result<(), TryRuntimeError>, - )] = &[(T1::matches_id, T1::try_state), (T2::matches_id, T2::try_state)]; + )] = &[ + (AdditionalHooks::matches_id, AdditionalHooks::try_state), + (AllPallets::matches_id, AllPallets::try_state), + ]; let mut result = Ok(()); try_state_identifiers.iter().for_each(|id| { From 44a545a6f4afd5b4bf874d84f1b9129deca0fa29 Mon Sep 17 00:00:00 2001 From: Antonio Antonino Date: Wed, 29 May 2024 13:34:25 +0200 Subject: [PATCH 10/18] Final cleanup before tests --- out.txt | 0 substrate/frame/support/src/traits.rs | 2 +- .../frame/support/src/traits/metadata.rs | 20 ------------------- .../support/src/traits/try_runtime/mod.rs | 13 ++++++++++++ 4 files changed, 14 insertions(+), 21 deletions(-) delete mode 100644 out.txt diff --git a/out.txt b/out.txt deleted file mode 100644 index e69de29bb2d1..000000000000 diff --git a/substrate/frame/support/src/traits.rs b/substrate/frame/support/src/traits.rs index eab53a5bb297..68dc94d1a2f2 100644 --- a/substrate/frame/support/src/traits.rs +++ b/substrate/frame/support/src/traits.rs @@ -78,7 +78,7 @@ mod metadata; pub use metadata::{ CallMetadata, CrateVersion, GetCallIndex, GetCallMetadata, GetCallName, GetStorageVersion, NoStorageVersionSet, PalletInfo, PalletInfoAccess, PalletInfoData, PalletsInfoAccess, - StaticPartialEq, StorageVersion, STORAGE_VERSION_STORAGE_KEY_POSTFIX, + StorageVersion, STORAGE_VERSION_STORAGE_KEY_POSTFIX, }; mod hooks; diff --git a/substrate/frame/support/src/traits/metadata.rs b/substrate/frame/support/src/traits/metadata.rs index 3ddbd26ed51c..8bda4186bc96 100644 --- a/substrate/frame/support/src/traits/metadata.rs +++ b/substrate/frame/support/src/traits/metadata.rs @@ -322,26 +322,6 @@ pub trait GetStorageVersion { fn on_chain_storage_version() -> StorageVersion; } -pub trait StaticPartialEq -where - Rhs: ?Sized, -{ - fn eq(_other: &Rhs) -> bool; -} - -#[cfg_attr(all(not(feature = "tuples-96"), not(feature = "tuples-128")), impl_for_tuples(64))] -#[cfg_attr(all(feature = "tuples-96", not(feature = "tuples-128")), impl_for_tuples(96))] -#[cfg_attr(feature = "tuples-128", impl_for_tuples(128))] -impl StaticPartialEq for Tuple -where - Rhs: ?Sized, -{ - fn eq(other: &Rhs) -> bool { - for_tuples!( #( if Tuple::eq(other) { return true; } )* ); - return false; - } -} - #[cfg(test)] mod tests { use super::*; diff --git a/substrate/frame/support/src/traits/try_runtime/mod.rs b/substrate/frame/support/src/traits/try_runtime/mod.rs index 21c64b0af0ba..26313f6144ac 100644 --- a/substrate/frame/support/src/traits/try_runtime/mod.rs +++ b/substrate/frame/support/src/traits/try_runtime/mod.rs @@ -202,12 +202,14 @@ pub trait TryState { fn try_state(_: BlockNumber, _: Select) -> Result<(), TryRuntimeError>; } +// Empty tuple never does anything. impl TryState for () { fn try_state(_: BlockNumber, _: Select) -> Result<(), TryRuntimeError> { Ok(()) } } +// 2-element tuple calls first and then second, optionally filtering according to input. impl TryState for (AdditionalHooks, AllPallets) where @@ -292,3 +294,14 @@ where } } } + +// 1-element tuple calls first and then empty tuple, which is a no-op +impl TryState for (T,) +where + BlockNumber: Clone + sp_std::fmt::Debug + AtLeast32BitUnsigned, + T: IdentifiableTryStateLogic, +{ + fn try_state(n: BlockNumber, targets: Select) -> Result<(), TryRuntimeError> { + <(T, ()) as TryState>::try_state(n, targets) + } +} From a8aad9e479d00cd22a2a89ba68cd4fd2f6a471c7 Mon Sep 17 00:00:00 2001 From: Antonio Antonino Date: Wed, 29 May 2024 14:19:21 +0200 Subject: [PATCH 11/18] Add tests for executive try-state --- substrate/frame/executive/src/tests.rs | 85 +++++++++++++++++++++----- substrate/primitives/io/src/lib.rs | 2 +- 2 files changed, 72 insertions(+), 15 deletions(-) diff --git a/substrate/frame/executive/src/tests.rs b/substrate/frame/executive/src/tests.rs index 69a970a89d93..9ece27de9082 100644 --- a/substrate/frame/executive/src/tests.rs +++ b/substrate/frame/executive/src/tests.rs @@ -44,6 +44,40 @@ use pallet_balances::Call as BalancesCall; const TEST_KEY: &[u8] = b":test:key:"; +#[cfg(feature = "try-runtime")] +mod try_runtime { + use frame_support::traits::{IdentifiableTryStateLogic, TryStateLogic}; + use sp_runtime::traits::AtLeast32BitUnsigned; + + // Will contain `true` when the custom runtime logic was called. + pub(super) static mut CANARY_FLAG: bool = false; + + const CUSTOM_TRY_STATE_ID: &[u8] = b"custom_try_state"; + + pub(super) struct CustomTryState; + + impl TryStateLogic for CustomTryState + where + BlockNumber: Clone + sp_std::fmt::Debug + AtLeast32BitUnsigned, + { + fn try_state(_: BlockNumber) -> Result<(), sp_runtime::TryRuntimeError> { + unsafe { + CANARY_FLAG = true; + } + Ok(()) + } + } + + impl IdentifiableTryStateLogic for CustomTryState + where + BlockNumber: Clone + sp_std::fmt::Debug + AtLeast32BitUnsigned, + { + fn matches_id(id: &[u8]) -> bool { + id == CUSTOM_TRY_STATE_ID + } + } +} + #[frame_support::pallet(dev_mode)] mod custom { use super::*; @@ -78,6 +112,18 @@ mod custom { fn offchain_worker(n: BlockNumberFor) { assert_eq!(BlockNumberFor::::from(1u32), n); } + + // Verify that `CustomTryState` has been called before. + #[cfg(feature = "try-runtime")] + fn try_state(_n: BlockNumberFor) -> Result<(), sp_runtime::TryRuntimeError> { + unsafe { + assert!( + try_runtime::CANARY_FLAG, + "Custom `try-runtime` did not run before pallet `try-runtime`." + ); + } + Ok(()) + } } #[pallet::call] @@ -268,9 +314,9 @@ mod custom2 { // Inherent call is accepted for being dispatched fn pre_dispatch(call: &Self::Call) -> Result<(), TransactionValidityError> { match call { - Call::allowed_unsigned { .. } | - Call::optional_inherent { .. } | - Call::inherent { .. } => Ok(()), + Call::allowed_unsigned { .. } + | Call::optional_inherent { .. } + | Call::inherent { .. } => Ok(()), _ => Err(UnknownTransaction::NoUnsignedValidator.into()), } } @@ -397,6 +443,11 @@ impl OnRuntimeUpgrade for CustomOnRuntimeUpgrade { } } +#[cfg(not(feature = "try-runtime"))] +type CustomOnTryState = (); +#[cfg(feature = "try-runtime")] +type CustomOnTryState = try_runtime::CustomTryState; + type Executive = super::Executive< Runtime, Block, @@ -404,6 +455,7 @@ type Executive = super::Executive< Runtime, AllPalletsWithSystem, CustomOnRuntimeUpgrade, + CustomOnTryState, >; parameter_types! { @@ -498,8 +550,8 @@ fn balance_transfer_dispatch_works() { .assimilate_storage(&mut t) .unwrap(); let xt = TestXt::new(call_transfer(2, 69), sign_extra(1, 0, 0)); - let weight = xt.get_dispatch_info().weight + - ::BlockWeights::get() + let weight = xt.get_dispatch_info().weight + + ::BlockWeights::get() .get(DispatchClass::Normal) .base_extrinsic; let fee: Balance = @@ -523,9 +575,14 @@ fn new_test_ext(balance_factor: Balance) -> sp_io::TestExternalities { ext.execute_with(|| { SystemCallbacksCalled::set(0); }); + + #[cfg(feature = "try-runtime")] + unsafe { + try_runtime::CANARY_FLAG = false; + } + ext } - fn new_test_ext_v0(balance_factor: Balance) -> sp_io::TestExternalities { let mut t = frame_system::GenesisConfig::::default().build_storage().unwrap(); pallet_balances::GenesisConfig:: { balances: vec![(1, 111 * balance_factor)] } @@ -681,8 +738,8 @@ fn block_weight_and_size_is_stored_per_tx() { let mut t = new_test_ext(1); t.execute_with(|| { // Block execution weight + on_initialize weight from custom module - let base_block_weight = Weight::from_parts(175, 0) + - ::BlockWeights::get().base_block; + let base_block_weight = Weight::from_parts(175, 0) + + ::BlockWeights::get().base_block; Executive::initialize_block(&Header::new_from_number(1)); @@ -694,8 +751,8 @@ fn block_weight_and_size_is_stored_per_tx() { assert!(Executive::apply_extrinsic(x2.clone()).unwrap().is_ok()); // default weight for `TestXt` == encoded length. - let extrinsic_weight = Weight::from_parts(len as u64, 0) + - ::BlockWeights::get() + let extrinsic_weight = Weight::from_parts(len as u64, 0) + + ::BlockWeights::get() .get(DispatchClass::Normal) .base_extrinsic; // Check we account for all extrinsic weight and their len. @@ -957,10 +1014,10 @@ fn all_weights_are_recorded_correctly() { // Weights are recorded correctly assert_eq!( frame_system::Pallet::::block_weight().total(), - custom_runtime_upgrade_weight + - runtime_upgrade_weight + - on_initialize_weight + - base_block_weight, + custom_runtime_upgrade_weight + + runtime_upgrade_weight + + on_initialize_weight + + base_block_weight, ); }); } diff --git a/substrate/primitives/io/src/lib.rs b/substrate/primitives/io/src/lib.rs index 67e822ba7e24..7e753c9e92fe 100644 --- a/substrate/primitives/io/src/lib.rs +++ b/substrate/primitives/io/src/lib.rs @@ -836,7 +836,7 @@ pub trait Crypto { use ed25519_dalek::Verifier; let Ok(public_key) = ed25519_dalek::VerifyingKey::from_bytes(&pub_key.0) else { - return false + return false; }; let sig = ed25519_dalek::Signature::from_bytes(&sig.0); From 82a4100f6aa928fc6179716282c2ef392863aa76 Mon Sep 17 00:00:00 2001 From: Antonio Antonino Date: Wed, 29 May 2024 14:25:16 +0200 Subject: [PATCH 12/18] Last cleanups --- substrate/frame/executive/src/tests.rs | 2 +- .../procedural/src/pallet/expand/hooks.rs | 355 ++++++++++-------- .../src/pallet/expand/pallet_struct.rs | 22 -- .../support/src/traits/try_runtime/mod.rs | 2 +- 4 files changed, 191 insertions(+), 190 deletions(-) diff --git a/substrate/frame/executive/src/tests.rs b/substrate/frame/executive/src/tests.rs index 9ece27de9082..378f5a88e2fb 100644 --- a/substrate/frame/executive/src/tests.rs +++ b/substrate/frame/executive/src/tests.rs @@ -49,7 +49,7 @@ mod try_runtime { use frame_support::traits::{IdentifiableTryStateLogic, TryStateLogic}; use sp_runtime::traits::AtLeast32BitUnsigned; - // Will contain `true` when the custom runtime logic was called. + // Will contain `true` when the custom runtime logic is called. pub(super) static mut CANARY_FLAG: bool = false; const CUSTOM_TRY_STATE_ID: &[u8] = b"custom_try_state"; diff --git a/substrate/frame/support/procedural/src/pallet/expand/hooks.rs b/substrate/frame/support/procedural/src/pallet/expand/hooks.rs index 3623b595268d..19d6f8caf146 100644 --- a/substrate/frame/support/procedural/src/pallet/expand/hooks.rs +++ b/substrate/frame/support/procedural/src/pallet/expand/hooks.rs @@ -18,6 +18,7 @@ use crate::pallet::Def; /// * implement the individual traits using the Hooks trait +/// * implement the `TryStateLogic` and `IdentifiableTryStateLogic` traits, that are strictly dependent on the `TryState` hook pub fn expand_hooks(def: &mut Def) -> proc_macro2::TokenStream { let (where_clause, span, has_runtime_upgrade) = match def.hooks.as_ref() { Some(hooks) => { @@ -141,200 +142,222 @@ pub fn expand_hooks(def: &mut Def) -> proc_macro2::TokenStream { }; quote::quote_spanned!(span => - #hooks_impl - - impl<#type_impl_gen> - #frame_support::traits::OnFinalize<#frame_system::pallet_prelude::BlockNumberFor::> - for #pallet_ident<#type_use_gen> #where_clause - { - fn on_finalize(n: #frame_system::pallet_prelude::BlockNumberFor::) { - #frame_support::__private::sp_tracing::enter_span!( - #frame_support::__private::sp_tracing::trace_span!("on_finalize") - ); - < - Self as #frame_support::traits::Hooks< - #frame_system::pallet_prelude::BlockNumberFor:: - > - >::on_finalize(n) + #hooks_impl + + impl<#type_impl_gen> + #frame_support::traits::OnFinalize<#frame_system::pallet_prelude::BlockNumberFor::> + for #pallet_ident<#type_use_gen> #where_clause + { + fn on_finalize(n: #frame_system::pallet_prelude::BlockNumberFor::) { + #frame_support::__private::sp_tracing::enter_span!( + #frame_support::__private::sp_tracing::trace_span!("on_finalize") + ); + < + Self as #frame_support::traits::Hooks< + #frame_system::pallet_prelude::BlockNumberFor:: + > + >::on_finalize(n) + } } - } - impl<#type_impl_gen> - #frame_support::traits::OnIdle<#frame_system::pallet_prelude::BlockNumberFor::> - for #pallet_ident<#type_use_gen> #where_clause - { - fn on_idle( - n: #frame_system::pallet_prelude::BlockNumberFor::, - remaining_weight: #frame_support::weights::Weight - ) -> #frame_support::weights::Weight { - < - Self as #frame_support::traits::Hooks< - #frame_system::pallet_prelude::BlockNumberFor:: - > - >::on_idle(n, remaining_weight) + impl<#type_impl_gen> + #frame_support::traits::OnIdle<#frame_system::pallet_prelude::BlockNumberFor::> + for #pallet_ident<#type_use_gen> #where_clause + { + fn on_idle( + n: #frame_system::pallet_prelude::BlockNumberFor::, + remaining_weight: #frame_support::weights::Weight + ) -> #frame_support::weights::Weight { + < + Self as #frame_support::traits::Hooks< + #frame_system::pallet_prelude::BlockNumberFor:: + > + >::on_idle(n, remaining_weight) + } } - } - impl<#type_impl_gen> - #frame_support::traits::OnPoll<#frame_system::pallet_prelude::BlockNumberFor::> - for #pallet_ident<#type_use_gen> #where_clause - { - fn on_poll( - n: #frame_system::pallet_prelude::BlockNumberFor::, - weight: &mut #frame_support::weights::WeightMeter - ) { - < - Self as #frame_support::traits::Hooks< - #frame_system::pallet_prelude::BlockNumberFor:: - > - >::on_poll(n, weight); + impl<#type_impl_gen> + #frame_support::traits::OnPoll<#frame_system::pallet_prelude::BlockNumberFor::> + for #pallet_ident<#type_use_gen> #where_clause + { + fn on_poll( + n: #frame_system::pallet_prelude::BlockNumberFor::, + weight: &mut #frame_support::weights::WeightMeter + ) { + < + Self as #frame_support::traits::Hooks< + #frame_system::pallet_prelude::BlockNumberFor:: + > + >::on_poll(n, weight); + } } - } - impl<#type_impl_gen> - #frame_support::traits::OnInitialize<#frame_system::pallet_prelude::BlockNumberFor::> - for #pallet_ident<#type_use_gen> #where_clause - { - fn on_initialize( - n: #frame_system::pallet_prelude::BlockNumberFor:: - ) -> #frame_support::weights::Weight { - #frame_support::__private::sp_tracing::enter_span!( - #frame_support::__private::sp_tracing::trace_span!("on_initialize") - ); - < - Self as #frame_support::traits::Hooks< - #frame_system::pallet_prelude::BlockNumberFor:: - > - >::on_initialize(n) + impl<#type_impl_gen> + #frame_support::traits::OnInitialize<#frame_system::pallet_prelude::BlockNumberFor::> + for #pallet_ident<#type_use_gen> #where_clause + { + fn on_initialize( + n: #frame_system::pallet_prelude::BlockNumberFor:: + ) -> #frame_support::weights::Weight { + #frame_support::__private::sp_tracing::enter_span!( + #frame_support::__private::sp_tracing::trace_span!("on_initialize") + ); + < + Self as #frame_support::traits::Hooks< + #frame_system::pallet_prelude::BlockNumberFor:: + > + >::on_initialize(n) + } } - } - impl<#type_impl_gen> - #frame_support::traits::BeforeAllRuntimeMigrations - for #pallet_ident<#type_use_gen> #where_clause - { - fn before_all_runtime_migrations() -> #frame_support::weights::Weight { - use #frame_support::traits::{Get, PalletInfoAccess}; - use #frame_support::__private::hashing::twox_128; - use #frame_support::storage::unhashed::contains_prefixed_key; - #frame_support::__private::sp_tracing::enter_span!( - #frame_support::__private::sp_tracing::trace_span!("before_all") - ); + impl<#type_impl_gen> + #frame_support::traits::BeforeAllRuntimeMigrations + for #pallet_ident<#type_use_gen> #where_clause + { + fn before_all_runtime_migrations() -> #frame_support::weights::Weight { + use #frame_support::traits::{Get, PalletInfoAccess}; + use #frame_support::__private::hashing::twox_128; + use #frame_support::storage::unhashed::contains_prefixed_key; + #frame_support::__private::sp_tracing::enter_span!( + #frame_support::__private::sp_tracing::trace_span!("before_all") + ); - // Check if the pallet has any keys set, including the storage version. If there are - // no keys set, the pallet was just added to the runtime and needs to have its - // version initialized. - let pallet_hashed_prefix = ::name_hash(); - let exists = contains_prefixed_key(&pallet_hashed_prefix); - if !exists { - #initialize_on_chain_storage_version - ::DbWeight::get().reads_writes(1, 1) - } else { - ::DbWeight::get().reads(1) + // Check if the pallet has any keys set, including the storage version. If there are + // no keys set, the pallet was just added to the runtime and needs to have its + // version initialized. + let pallet_hashed_prefix = ::name_hash(); + let exists = contains_prefixed_key(&pallet_hashed_prefix); + if !exists { + #initialize_on_chain_storage_version + ::DbWeight::get().reads_writes(1, 1) + } else { + ::DbWeight::get().reads(1) + } } } - } - impl<#type_impl_gen> - #frame_support::traits::OnRuntimeUpgrade - for #pallet_ident<#type_use_gen> #where_clause - { - fn on_runtime_upgrade() -> #frame_support::weights::Weight { - #frame_support::__private::sp_tracing::enter_span!( - #frame_support::__private::sp_tracing::trace_span!("on_runtime_update") - ); + impl<#type_impl_gen> + #frame_support::traits::OnRuntimeUpgrade + for #pallet_ident<#type_use_gen> #where_clause + { + fn on_runtime_upgrade() -> #frame_support::weights::Weight { + #frame_support::__private::sp_tracing::enter_span!( + #frame_support::__private::sp_tracing::trace_span!("on_runtime_update") + ); - // log info about the upgrade. - #log_runtime_upgrade + // log info about the upgrade. + #log_runtime_upgrade - < - Self as #frame_support::traits::Hooks< - #frame_system::pallet_prelude::BlockNumberFor:: - > - >::on_runtime_upgrade() - } + < + Self as #frame_support::traits::Hooks< + #frame_system::pallet_prelude::BlockNumberFor:: + > + >::on_runtime_upgrade() + } - #[cfg(feature = "try-runtime")] - fn pre_upgrade() -> Result<#frame_support::__private::sp_std::vec::Vec, #frame_support::sp_runtime::TryRuntimeError> { - < - Self - as - #frame_support::traits::Hooks<#frame_system::pallet_prelude::BlockNumberFor::> - >::pre_upgrade() - } + #[cfg(feature = "try-runtime")] + fn pre_upgrade() -> Result<#frame_support::__private::sp_std::vec::Vec, #frame_support::sp_runtime::TryRuntimeError> { + < + Self + as + #frame_support::traits::Hooks<#frame_system::pallet_prelude::BlockNumberFor::> + >::pre_upgrade() + } - #[cfg(feature = "try-runtime")] - fn post_upgrade(state: #frame_support::__private::sp_std::vec::Vec) -> Result<(), #frame_support::sp_runtime::TryRuntimeError> { - #post_storage_version_check - - < - Self - as - #frame_support::traits::Hooks<#frame_system::pallet_prelude::BlockNumberFor::> - >::post_upgrade(state) - } - } + #[cfg(feature = "try-runtime")] + fn post_upgrade(state: #frame_support::__private::sp_std::vec::Vec) -> Result<(), #frame_support::sp_runtime::TryRuntimeError> { + #post_storage_version_check - impl<#type_impl_gen> - #frame_support::traits::OffchainWorker<#frame_system::pallet_prelude::BlockNumberFor::> - for #pallet_ident<#type_use_gen> #where_clause - { - fn offchain_worker(n: #frame_system::pallet_prelude::BlockNumberFor::) { - < - Self as #frame_support::traits::Hooks< - #frame_system::pallet_prelude::BlockNumberFor:: - > - >::offchain_worker(n) + < + Self + as + #frame_support::traits::Hooks<#frame_system::pallet_prelude::BlockNumberFor::> + >::post_upgrade(state) + } } - } - // Integrity tests are only required for when `std` is enabled. - #frame_support::std_enabled! { impl<#type_impl_gen> - #frame_support::traits::IntegrityTest - for #pallet_ident<#type_use_gen> #where_clause + #frame_support::traits::OffchainWorker<#frame_system::pallet_prelude::BlockNumberFor::> + for #pallet_ident<#type_use_gen> #where_clause { - fn integrity_test() { - #frame_support::__private::sp_io::TestExternalities::default().execute_with(|| { - < - Self as #frame_support::traits::Hooks< - #frame_system::pallet_prelude::BlockNumberFor:: - > - >::integrity_test() - }); + fn offchain_worker(n: #frame_system::pallet_prelude::BlockNumberFor::) { + < + Self as #frame_support::traits::Hooks< + #frame_system::pallet_prelude::BlockNumberFor:: + > + >::offchain_worker(n) } } - } - #[cfg(feature = "try-runtime")] - impl<#type_impl_gen> - #frame_support::traits::TryState<#frame_system::pallet_prelude::BlockNumberFor::> - for #pallet_ident<#type_use_gen> #where_clause - { - fn try_state( - n: #frame_system::pallet_prelude::BlockNumberFor::, - _s: #frame_support::traits::TryStateSelect - ) -> Result<(), #frame_support::sp_runtime::TryRuntimeError> { - #frame_support::__private::log::info!( - target: #frame_support::LOG_TARGET, - "🩺 Running {:?} try-state checks", - #pallet_name, - ); - < - Self as #frame_support::traits::Hooks< - #frame_system::pallet_prelude::BlockNumberFor:: - > - >::try_state(n).map_err(|err| { - #frame_support::__private::log::error!( + // Integrity tests are only required for when `std` is enabled. + #frame_support::std_enabled! { + impl<#type_impl_gen> + #frame_support::traits::IntegrityTest + for #pallet_ident<#type_use_gen> #where_clause + { + fn integrity_test() { + #frame_support::__private::sp_io::TestExternalities::default().execute_with(|| { + < + Self as #frame_support::traits::Hooks< + #frame_system::pallet_prelude::BlockNumberFor:: + > + >::integrity_test() + }); + } + } + } + + #[cfg(feature = "try-runtime")] + impl<#type_impl_gen> + #frame_support::traits::TryState<#frame_system::pallet_prelude::BlockNumberFor::> + for #pallet_ident<#type_use_gen> #where_clause + { + fn try_state( + n: #frame_system::pallet_prelude::BlockNumberFor::, + _s: #frame_support::traits::TryStateSelect + ) -> Result<(), #frame_support::sp_runtime::TryRuntimeError> { + #frame_support::__private::log::info!( target: #frame_support::LOG_TARGET, - "❌ {:?} try_state checks failed: {:?}", + "🩺 Running {:?} try-state checks", #pallet_name, - err ); + < + Self as #frame_support::traits::Hooks< + #frame_system::pallet_prelude::BlockNumberFor:: + > + >::try_state(n).map_err(|err| { + #frame_support::__private::log::error!( + target: #frame_support::LOG_TARGET, + "❌ {:?} try_state checks failed: {:?}", + #pallet_name, + err + ); - err - }) + err + }) + } + } + + // Implement `TryStateLogic` for `Pallet` + #[cfg(feature = "try-runtime")] + impl<#type_impl_gen> #frame_support::traits::TryStateLogic<#frame_system::pallet_prelude::BlockNumberFor> + for #pallet_ident<#type_use_gen> + #where_clause + { + fn try_state(n: frame_system::pallet_prelude::BlockNumberFor) -> Result<(), #frame_support::sp_runtime::TryRuntimeError> { + >>::try_state(n, #frame_support::traits::TryStateSelect::All) + } + } + + // Implement `IdentifiableTryStateLogic` for `Pallet` + #[cfg(feature = "try-runtime")] + impl<#type_impl_gen> #frame_support::traits::IdentifiableTryStateLogic> + for #pallet_ident<#type_use_gen> + #where_clause + { + fn matches_id(id: &[u8]) -> bool { + ::name().as_bytes() == id + } } - } ) } diff --git a/substrate/frame/support/procedural/src/pallet/expand/pallet_struct.rs b/substrate/frame/support/procedural/src/pallet/expand/pallet_struct.rs index 640e438450a3..7cdf6bde9de8 100644 --- a/substrate/frame/support/procedural/src/pallet/expand/pallet_struct.rs +++ b/substrate/frame/support/procedural/src/pallet/expand/pallet_struct.rs @@ -284,28 +284,6 @@ pub fn expand_pallet_struct(def: &mut Def) -> proc_macro2::TokenStream { } } - // Implement `TryStateLogic` for `Pallet` - #[cfg(feature = "try-runtime")] - impl<#type_impl_gen> #frame_support::traits::TryStateLogic> - for #pallet_ident<#type_use_gen> - #config_where_clause - { - fn try_state(n: BlockNumberFor) -> Result<(), sp_runtime::TryRuntimeError> { - >>::try_state(n) - } - } - - // Implement `IdentifiableTryStateLogic` for `Pallet` - #[cfg(feature = "try-runtime")] - impl<#type_impl_gen> #frame_support::traits::IdentifiableTryStateLogic> - for #pallet_ident<#type_use_gen> - #config_where_clause - { - fn matches_id(id: &[u8]) -> bool { - ::name().as_bytes() == id - } - } - #storage_info #whitelisted_storage_keys_impl ) diff --git a/substrate/frame/support/src/traits/try_runtime/mod.rs b/substrate/frame/support/src/traits/try_runtime/mod.rs index 26313f6144ac..c5653eaafd29 100644 --- a/substrate/frame/support/src/traits/try_runtime/mod.rs +++ b/substrate/frame/support/src/traits/try_runtime/mod.rs @@ -295,7 +295,7 @@ where } } -// 1-element tuple calls first and then empty tuple, which is a no-op +// 1-element tuple calls first tuple element and then empty tuple, which is a no-op impl TryState for (T,) where BlockNumber: Clone + sp_std::fmt::Debug + AtLeast32BitUnsigned, From 4acd36613cdb452910ff04db7a88071b697d2106 Mon Sep 17 00:00:00 2001 From: Antonio Antonino Date: Thu, 30 May 2024 08:56:10 +0200 Subject: [PATCH 13/18] Fix CI issues --- polkadot/xcm/xcm-simulator/fuzzer/src/fuzz.rs | 20 +++++++++---------- substrate/frame/delegated-staking/src/mock.rs | 7 +++---- 2 files changed, 13 insertions(+), 14 deletions(-) diff --git a/polkadot/xcm/xcm-simulator/fuzzer/src/fuzz.rs b/polkadot/xcm/xcm-simulator/fuzzer/src/fuzz.rs index adf6cacd278b..489cecee3880 100644 --- a/polkadot/xcm/xcm-simulator/fuzzer/src/fuzz.rs +++ b/polkadot/xcm/xcm-simulator/fuzzer/src/fuzz.rs @@ -24,7 +24,7 @@ use sp_runtime::{traits::AccountIdConversion, BuildStorage}; use xcm_simulator::{decl_test_network, decl_test_parachain, decl_test_relay_chain, TestExt}; #[cfg(feature = "try-runtime")] -use frame_support::traits::{TryState, TryStateSelect::All}; +use frame_support::traits::TryStateLogic; use frame_support::{assert_ok, traits::IntegrityTest}; use xcm::{latest::prelude::*, MAX_XCM_DECODE_DEPTH}; @@ -153,13 +153,13 @@ pub type ParachainPalletXcm = pallet_xcm::Pallet; // We check XCM messages recursively for blocklisted messages fn recursively_matches_blocklisted_messages(message: &Instruction<()>) -> bool { match message { - DepositReserveAsset { xcm, .. } | - ExportMessage { xcm, .. } | - InitiateReserveWithdraw { xcm, .. } | - InitiateTeleport { xcm, .. } | - TransferReserveAsset { xcm, .. } | - SetErrorHandler(xcm) | - SetAppendix(xcm) => xcm.iter().any(recursively_matches_blocklisted_messages), + DepositReserveAsset { xcm, .. } + | ExportMessage { xcm, .. } + | InitiateReserveWithdraw { xcm, .. } + | InitiateTeleport { xcm, .. } + | TransferReserveAsset { xcm, .. } + | SetErrorHandler(xcm) + | SetAppendix(xcm) => xcm.iter().any(recursively_matches_blocklisted_messages), // The blocklisted message is the Transact instruction. m => matches!(m, Transact { .. }), } @@ -224,14 +224,14 @@ fn run_input(xcm_messages: [XcmMessage; 5]) { |execute_with| { execute_with(|| { #[cfg(feature = "try-runtime")] - parachain::AllPalletsWithSystem::try_state(Default::default(), All).unwrap(); + parachain::AllPalletsWithSystem::try_state(Default::default()).unwrap(); parachain::AllPalletsWithSystem::integrity_test(); }); }, ); Relay::execute_with(|| { #[cfg(feature = "try-runtime")] - relay_chain::AllPalletsWithSystem::try_state(Default::default(), All).unwrap(); + relay_chain::AllPalletsWithSystem::try_state(Default::default()).unwrap(); relay_chain::AllPalletsWithSystem::integrity_test(); }); } diff --git a/substrate/frame/delegated-staking/src/mock.rs b/substrate/frame/delegated-staking/src/mock.rs index 811d5739f4e9..5d78d2105982 100644 --- a/substrate/frame/delegated-staking/src/mock.rs +++ b/substrate/frame/delegated-staking/src/mock.rs @@ -242,9 +242,8 @@ impl ExtBuilder { ext.execute_with(test); ext.execute_with(|| { #[cfg(feature = "try-runtime")] - >::try_state( + >::try_state( frame_system::Pallet::::block_number(), - frame_support::traits::TryStateSelect::All, ) .unwrap(); #[cfg(not(feature = "try-runtime"))] @@ -297,8 +296,8 @@ pub(crate) fn start_era(era: sp_staking::EraIndex) { } pub(crate) fn eq_stake(who: AccountId, total: Balance, active: Balance) -> bool { - Staking::stake(&who).unwrap() == Stake { total, active } && - get_agent_ledger(&who).ledger.stakeable_balance() == total + Staking::stake(&who).unwrap() == Stake { total, active } + && get_agent_ledger(&who).ledger.stakeable_balance() == total } pub(crate) fn get_agent_ledger(agent: &AccountId) -> AgentLedgerOuter { From 716059538565eaff5aff1c22393231577bdad1f2 Mon Sep 17 00:00:00 2001 From: Antonio Antonino Date: Thu, 30 May 2024 09:33:15 +0200 Subject: [PATCH 14/18] Fix docs --- substrate/frame/executive/src/lib.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/substrate/frame/executive/src/lib.rs b/substrate/frame/executive/src/lib.rs index cee01ac08165..4735ffd9ad45 100644 --- a/substrate/frame/executive/src/lib.rs +++ b/substrate/frame/executive/src/lib.rs @@ -97,6 +97,7 @@ //! # use frame_executive as executive; //! # pub struct UncheckedExtrinsic {}; //! # pub struct Header {}; +//! # pub type BlockNumber = u32; //! # type Context = frame_system::ChainContext; //! # pub type Block = generic::Block; //! # pub type Balances = u64; @@ -122,6 +123,8 @@ //! } //! //! struct CustomTryState; +//! +//! #[cfg(feature = "try-runtime")] //! impl frame_support::traits::IdentifiableTryStateLogic for CustomTryState { //! //! fn try_state(_: BlockNumber) -> Result<(), sp_runtime::TryRuntimeError> { From 10da08b482631416fb8657bde9b07c7924776b8c Mon Sep 17 00:00:00 2001 From: Antonio Antonino Date: Thu, 30 May 2024 09:33:56 +0200 Subject: [PATCH 15/18] fmt --- polkadot/xcm/xcm-simulator/fuzzer/src/fuzz.rs | 14 +++++----- substrate/frame/executive/src/lib.rs | 10 +++---- substrate/frame/executive/src/tests.rs | 26 +++++++++---------- .../procedural/src/pallet/expand/hooks.rs | 3 ++- .../support/src/traits/try_runtime/mod.rs | 8 +++--- 5 files changed, 31 insertions(+), 30 deletions(-) diff --git a/polkadot/xcm/xcm-simulator/fuzzer/src/fuzz.rs b/polkadot/xcm/xcm-simulator/fuzzer/src/fuzz.rs index 489cecee3880..50d17655e124 100644 --- a/polkadot/xcm/xcm-simulator/fuzzer/src/fuzz.rs +++ b/polkadot/xcm/xcm-simulator/fuzzer/src/fuzz.rs @@ -153,13 +153,13 @@ pub type ParachainPalletXcm = pallet_xcm::Pallet; // We check XCM messages recursively for blocklisted messages fn recursively_matches_blocklisted_messages(message: &Instruction<()>) -> bool { match message { - DepositReserveAsset { xcm, .. } - | ExportMessage { xcm, .. } - | InitiateReserveWithdraw { xcm, .. } - | InitiateTeleport { xcm, .. } - | TransferReserveAsset { xcm, .. } - | SetErrorHandler(xcm) - | SetAppendix(xcm) => xcm.iter().any(recursively_matches_blocklisted_messages), + DepositReserveAsset { xcm, .. } | + ExportMessage { xcm, .. } | + InitiateReserveWithdraw { xcm, .. } | + InitiateTeleport { xcm, .. } | + TransferReserveAsset { xcm, .. } | + SetErrorHandler(xcm) | + SetAppendix(xcm) => xcm.iter().any(recursively_matches_blocklisted_messages), // The blocklisted message is the Transact instruction. m => matches!(m, Transact { .. }), } diff --git a/substrate/frame/executive/src/lib.rs b/substrate/frame/executive/src/lib.rs index 4735ffd9ad45..a1764526efd3 100644 --- a/substrate/frame/executive/src/lib.rs +++ b/substrate/frame/executive/src/lib.rs @@ -89,8 +89,8 @@ //! ### Custom `TryState` logic //! //! You can add custom logic that should be called in your runtime on a try-state test. This is -//! done by setting an optional generic parameter after the `OnRuntimeUpgrade` type. The custom logic will be called before -//! the `try-state` logic of all modules is called. +//! done by setting an optional generic parameter after the `OnRuntimeUpgrade` type. The custom +//! logic will be called before the `try-state` logic of all modules is called. //! //! ``` //! # use sp_runtime::generic; @@ -678,9 +678,9 @@ impl< // Check that `parent_hash` is correct. let n = *header.number(); assert!( - n > BlockNumberFor::::zero() - && >::block_hash(n - BlockNumberFor::::one()) - == *header.parent_hash(), + n > BlockNumberFor::::zero() && + >::block_hash(n - BlockNumberFor::::one()) == + *header.parent_hash(), "Parent hash should be valid.", ); diff --git a/substrate/frame/executive/src/tests.rs b/substrate/frame/executive/src/tests.rs index 378f5a88e2fb..f14ce13be481 100644 --- a/substrate/frame/executive/src/tests.rs +++ b/substrate/frame/executive/src/tests.rs @@ -314,9 +314,9 @@ mod custom2 { // Inherent call is accepted for being dispatched fn pre_dispatch(call: &Self::Call) -> Result<(), TransactionValidityError> { match call { - Call::allowed_unsigned { .. } - | Call::optional_inherent { .. } - | Call::inherent { .. } => Ok(()), + Call::allowed_unsigned { .. } | + Call::optional_inherent { .. } | + Call::inherent { .. } => Ok(()), _ => Err(UnknownTransaction::NoUnsignedValidator.into()), } } @@ -550,8 +550,8 @@ fn balance_transfer_dispatch_works() { .assimilate_storage(&mut t) .unwrap(); let xt = TestXt::new(call_transfer(2, 69), sign_extra(1, 0, 0)); - let weight = xt.get_dispatch_info().weight - + ::BlockWeights::get() + let weight = xt.get_dispatch_info().weight + + ::BlockWeights::get() .get(DispatchClass::Normal) .base_extrinsic; let fee: Balance = @@ -738,8 +738,8 @@ fn block_weight_and_size_is_stored_per_tx() { let mut t = new_test_ext(1); t.execute_with(|| { // Block execution weight + on_initialize weight from custom module - let base_block_weight = Weight::from_parts(175, 0) - + ::BlockWeights::get().base_block; + let base_block_weight = Weight::from_parts(175, 0) + + ::BlockWeights::get().base_block; Executive::initialize_block(&Header::new_from_number(1)); @@ -751,8 +751,8 @@ fn block_weight_and_size_is_stored_per_tx() { assert!(Executive::apply_extrinsic(x2.clone()).unwrap().is_ok()); // default weight for `TestXt` == encoded length. - let extrinsic_weight = Weight::from_parts(len as u64, 0) - + ::BlockWeights::get() + let extrinsic_weight = Weight::from_parts(len as u64, 0) + + ::BlockWeights::get() .get(DispatchClass::Normal) .base_extrinsic; // Check we account for all extrinsic weight and their len. @@ -1014,10 +1014,10 @@ fn all_weights_are_recorded_correctly() { // Weights are recorded correctly assert_eq!( frame_system::Pallet::::block_weight().total(), - custom_runtime_upgrade_weight - + runtime_upgrade_weight - + on_initialize_weight - + base_block_weight, + custom_runtime_upgrade_weight + + runtime_upgrade_weight + + on_initialize_weight + + base_block_weight, ); }); } diff --git a/substrate/frame/support/procedural/src/pallet/expand/hooks.rs b/substrate/frame/support/procedural/src/pallet/expand/hooks.rs index 19d6f8caf146..e412581d11a6 100644 --- a/substrate/frame/support/procedural/src/pallet/expand/hooks.rs +++ b/substrate/frame/support/procedural/src/pallet/expand/hooks.rs @@ -18,7 +18,8 @@ use crate::pallet::Def; /// * implement the individual traits using the Hooks trait -/// * implement the `TryStateLogic` and `IdentifiableTryStateLogic` traits, that are strictly dependent on the `TryState` hook +/// * implement the `TryStateLogic` and `IdentifiableTryStateLogic` traits, that are strictly +/// dependent on the `TryState` hook pub fn expand_hooks(def: &mut Def) -> proc_macro2::TokenStream { let (where_clause, span, has_runtime_upgrade) = match def.hooks.as_ref() { Some(hooks) => { diff --git a/substrate/frame/support/src/traits/try_runtime/mod.rs b/substrate/frame/support/src/traits/try_runtime/mod.rs index c5653eaafd29..5205c0cb4dc9 100644 --- a/substrate/frame/support/src/traits/try_runtime/mod.rs +++ b/substrate/frame/support/src/traits/try_runtime/mod.rs @@ -79,7 +79,7 @@ impl sp_std::str::FromStr for Select { match s { "all" | "All" => Ok(Select::All), "none" | "None" => Ok(Select::None), - _ => { + _ => if s.starts_with("rr-") { let count = s .split_once('-') @@ -89,8 +89,7 @@ impl sp_std::str::FromStr for Select { } else { let pallets = s.split(',').map(|x| x.as_bytes().to_vec()).collect::>(); Ok(Select::Only(pallets)) - } - }, + }, } } } @@ -172,7 +171,8 @@ where /// Logic executed when `try-state` filters are provided in the `try-runtime` CLI. /// -/// Returning `true` for the provided ID will include this `try-state` logic in the overall tests performed. +/// Returning `true` for the provided ID will include this `try-state` logic in the overall tests +/// performed. pub trait IdentifiableTryStateLogic: TryStateLogic { fn matches_id(_id: &[u8]) -> bool; } From 2f69a01a3d278c773b9a1d7a45162a53711353cf Mon Sep 17 00:00:00 2001 From: Antonio Antonino Date: Mon, 10 Jun 2024 14:55:16 +0200 Subject: [PATCH 16/18] Post-rebase fixes --- polkadot/xcm/pallet-xcm/src/lib.rs | 71 ++-- polkadot/xcm/xcm-simulator/fuzzer/src/fuzz.rs | 4 +- substrate/frame/delegated-staking/src/mock.rs | 4 +- substrate/frame/executive/src/lib.rs | 12 +- .../procedural/src/pallet/expand/hooks.rs | 368 +++++++++--------- .../support/src/traits/try_runtime/mod.rs | 2 +- substrate/primitives/io/src/lib.rs | 2 +- 7 files changed, 228 insertions(+), 235 deletions(-) diff --git a/polkadot/xcm/pallet-xcm/src/lib.rs b/polkadot/xcm/pallet-xcm/src/lib.rs index 8c0bf39b1641..668f07c52ce3 100644 --- a/polkadot/xcm/pallet-xcm/src/lib.rs +++ b/polkadot/xcm/pallet-xcm/src/lib.rs @@ -836,7 +836,7 @@ pub mod pallet { if Self::request_version_notify(dest).is_ok() { // TODO: correct weights. weight_used.saturating_accrue(T::DbWeight::get().reads_writes(1, 1)); - break; + break } } } @@ -884,9 +884,8 @@ pub mod pallet { timeout, maybe_match_querier: Some(Location::here().into()), }, - VersionNotifier { origin, is_active } => { - QueryStatus::VersionNotifier { origin, is_active } - }, + VersionNotifier { origin, is_active } => + QueryStatus::VersionNotifier { origin, is_active }, Ready { response, at } => QueryStatus::Ready { response, at }, } } @@ -1690,9 +1689,8 @@ impl Pallet { fees, weight_limit, )?, - TransferType::RemoteReserve(_) => { - return Err(Error::::InvalidAssetUnsupportedReserve.into()) - }, + TransferType::RemoteReserve(_) => + return Err(Error::::InvalidAssetUnsupportedReserve.into()), }; FeesHandling::Separate { local_xcm, remote_xcm } }; @@ -2241,7 +2239,7 @@ impl Pallet { } weight_used.saturating_accrue(sv_migrate_weight); if weight_used.any_gte(weight_cutoff) { - return (weight_used, Some(stage)); + return (weight_used, Some(stage)) } } } @@ -2255,7 +2253,7 @@ impl Pallet { } weight_used.saturating_accrue(vn_migrate_weight); if weight_used.any_gte(weight_cutoff) { - return (weight_used, Some(stage)); + return (weight_used, Some(stage)) } } } @@ -2277,7 +2275,7 @@ impl Pallet { // We don't early return here since we need to be certain that we // make some progress. weight_used.saturating_accrue(vnt_already_notified_weight); - continue; + continue }, }; let response = Response::Version(xcm_version); @@ -2303,7 +2301,7 @@ impl Pallet { weight_used.saturating_accrue(vnt_notify_weight); if weight_used.any_gte(weight_cutoff) { let last = Some(iter.last_raw_key().into()); - return (weight_used, Some(NotifyCurrentTargets(last))); + return (weight_used, Some(NotifyCurrentTargets(last))) } } stage = MigrateAndNotifyOldTargets; @@ -2321,9 +2319,9 @@ impl Pallet { }); weight_used.saturating_accrue(vnt_migrate_fail_weight); if weight_used.any_gte(weight_cutoff) { - return (weight_used, Some(stage)); + return (weight_used, Some(stage)) } - continue; + continue }, }; @@ -2364,7 +2362,7 @@ impl Pallet { weight_used.saturating_accrue(vnt_notify_migrate_weight); } if weight_used.any_gte(weight_cutoff) { - return (weight_used, Some(stage)); + return (weight_used, Some(stage)) } } } @@ -2690,7 +2688,7 @@ impl Pallet { // if migration has been already scheduled, everything is ok and data will be eventually // migrated if CurrentMigration::::exists() { - return Ok(()); + return Ok(()) } // if migration has NOT been scheduled yet, we need to check all operational data @@ -2987,7 +2985,7 @@ impl VersionChangeNotifier for Pallet { impl DropAssets for Pallet { fn drop_assets(origin: &Location, assets: AssetsInHolding, _context: &XcmContext) -> Weight { if assets.is_empty() { - return Weight::zero(); + return Weight::zero() } let versioned = VersionedAssets::from(Assets::from(assets)); let hash = BlakeTwo256::hash_of(&(&origin, &versioned)); @@ -3011,12 +3009,11 @@ impl ClaimAssets for Pallet { ) -> bool { let mut versioned = VersionedAssets::from(assets.clone()); match ticket.unpack() { - (0, [GeneralIndex(i)]) => { + (0, [GeneralIndex(i)]) => versioned = match versioned.into_version(*i as u32) { Ok(v) => v, Err(()) => return false, - } - }, + }, (0, []) => (), _ => return false, }; @@ -3031,7 +3028,7 @@ impl ClaimAssets for Pallet { origin: origin.clone(), assets: versioned, }); - return true; + return true } } @@ -3042,17 +3039,15 @@ impl OnResponse for Pallet { querier: Option<&Location>, ) -> bool { match Queries::::get(query_id) { - Some(QueryStatus::Pending { responder, maybe_match_querier, .. }) => { - Location::try_from(responder).map_or(false, |r| origin == &r) - && maybe_match_querier.map_or(true, |match_querier| { + Some(QueryStatus::Pending { responder, maybe_match_querier, .. }) => + Location::try_from(responder).map_or(false, |r| origin == &r) && + maybe_match_querier.map_or(true, |match_querier| { Location::try_from(match_querier).map_or(false, |match_querier| { querier.map_or(false, |q| q == &match_querier) }) - }) - }, - Some(QueryStatus::VersionNotifier { origin: r, .. }) => { - Location::try_from(r).map_or(false, |r| origin == &r) - }, + }), + Some(QueryStatus::VersionNotifier { origin: r, .. }) => + Location::try_from(r).map_or(false, |r| origin == &r), _ => false, } } @@ -3079,7 +3074,7 @@ impl OnResponse for Pallet { query_id, expected_location: Some(o), }); - return Weight::zero(); + return Weight::zero() }, _ => { Self::deposit_event(Event::InvalidResponder { @@ -3088,7 +3083,7 @@ impl OnResponse for Pallet { expected_location: None, }); // TODO #3735: Correct weight for this. - return Weight::zero(); + return Weight::zero() }, }; // TODO #3735: Check max_weight is correct. @@ -3121,7 +3116,7 @@ impl OnResponse for Pallet { origin: origin.clone(), query_id, }); - return Weight::zero(); + return Weight::zero() }, }; if querier.map_or(true, |q| q != &match_querier) { @@ -3131,7 +3126,7 @@ impl OnResponse for Pallet { expected_querier: match_querier, maybe_actual_querier: querier.cloned(), }); - return Weight::zero(); + return Weight::zero() } } let responder = match Location::try_from(responder) { @@ -3141,7 +3136,7 @@ impl OnResponse for Pallet { origin: origin.clone(), query_id, }); - return Weight::zero(); + return Weight::zero() }, }; if origin != responder { @@ -3150,7 +3145,7 @@ impl OnResponse for Pallet { query_id, expected_location: Some(responder), }); - return Weight::zero(); + return Weight::zero() } return match maybe_notify { Some((pallet_index, call_index)) => { @@ -3172,7 +3167,7 @@ impl OnResponse for Pallet { max_budgeted_weight: max_weight, }; Self::deposit_event(e); - return Weight::zero(); + return Weight::zero() } let dispatch_origin = Origin::Response(origin.clone()).into(); match call.dispatch(dispatch_origin) { @@ -3209,7 +3204,7 @@ impl OnResponse for Pallet { Queries::::insert(query_id, QueryStatus::Ready { response, at }); Weight::zero() }, - }; + } }, _ => { let e = Event::UnexpectedResponse { origin: origin.clone(), query_id }; @@ -3318,9 +3313,7 @@ where caller.try_into().and_then(|o| match o { Origin::Xcm(ref location) if F::contains(&location.clone().try_into().map_err(|_| o.clone().into())?) => - { - Ok(location.clone().try_into().map_err(|_| o.clone().into())?) - }, + Ok(location.clone().try_into().map_err(|_| o.clone().into())?), Origin::Xcm(location) => Err(Origin::Xcm(location).into()), o => Err(o.into()), }) diff --git a/polkadot/xcm/xcm-simulator/fuzzer/src/fuzz.rs b/polkadot/xcm/xcm-simulator/fuzzer/src/fuzz.rs index 50d17655e124..3d57c61d1616 100644 --- a/polkadot/xcm/xcm-simulator/fuzzer/src/fuzz.rs +++ b/polkadot/xcm/xcm-simulator/fuzzer/src/fuzz.rs @@ -224,14 +224,14 @@ fn run_input(xcm_messages: [XcmMessage; 5]) { |execute_with| { execute_with(|| { #[cfg(feature = "try-runtime")] - parachain::AllPalletsWithSystem::try_state(Default::default()).unwrap(); + parachain::AllPalletsWithSystem::try_state(Default::default(), All).unwrap(); parachain::AllPalletsWithSystem::integrity_test(); }); }, ); Relay::execute_with(|| { #[cfg(feature = "try-runtime")] - relay_chain::AllPalletsWithSystem::try_state(Default::default()).unwrap(); + relay_chain::AllPalletsWithSystem::try_state(Default::default(), All).unwrap(); relay_chain::AllPalletsWithSystem::integrity_test(); }); } diff --git a/substrate/frame/delegated-staking/src/mock.rs b/substrate/frame/delegated-staking/src/mock.rs index 5d78d2105982..6e01aff30e06 100644 --- a/substrate/frame/delegated-staking/src/mock.rs +++ b/substrate/frame/delegated-staking/src/mock.rs @@ -296,8 +296,8 @@ pub(crate) fn start_era(era: sp_staking::EraIndex) { } pub(crate) fn eq_stake(who: AccountId, total: Balance, active: Balance) -> bool { - Staking::stake(&who).unwrap() == Stake { total, active } - && get_agent_ledger(&who).ledger.stakeable_balance() == total + Staking::stake(&who).unwrap() == Stake { total, active } && + get_agent_ledger(&who).ledger.stakeable_balance() == total } pub(crate) fn get_agent_ledger(agent: &AccountId) -> AgentLedgerOuter { diff --git a/substrate/frame/executive/src/lib.rs b/substrate/frame/executive/src/lib.rs index a1764526efd3..192fb451d24e 100644 --- a/substrate/frame/executive/src/lib.rs +++ b/substrate/frame/executive/src/lib.rs @@ -365,7 +365,7 @@ impl< // Check if there are any forbidden non-inherents in the block. if mode == ExtrinsicInclusionMode::OnlyInherents && extrinsics.len() > num_inherents { - return Err("Only inherents allowed".into()); + return Err("Only inherents allowed".into()) } let try_apply_extrinsic = |uxt: Block::Extrinsic| -> ApplyExtrinsicResult { @@ -390,7 +390,7 @@ impl< let r = Applyable::apply::(xt, &dispatch_info, encoded_len)?; if r.is_err() && dispatch_info.class == DispatchClass::Mandatory { - return Err(InvalidTransaction::BadMandatory.into()); + return Err(InvalidTransaction::BadMandatory.into()) } >::note_applied_extrinsic(&r, dispatch_info); @@ -406,7 +406,7 @@ impl< e, err, ); - break; + break } } @@ -776,7 +776,7 @@ impl< /// ongoing MBMs. fn on_idle_hook(block_number: NumberFor) { if ::MultiBlockMigrator::ongoing() { - return; + return } let weight = >::block_weight(); @@ -860,7 +860,7 @@ impl< // The entire block should be discarded if an inherent fails to apply. Otherwise // it may open an attack vector. if r.is_err() && dispatch_info.class == DispatchClass::Mandatory { - return Err(InvalidTransaction::BadMandatory.into()); + return Err(InvalidTransaction::BadMandatory.into()) } >::note_applied_extrinsic(&r, dispatch_info); @@ -930,7 +930,7 @@ impl< }; if dispatch_info.class == DispatchClass::Mandatory { - return Err(InvalidTransaction::MandatoryValidation.into()); + return Err(InvalidTransaction::MandatoryValidation.into()) } within_span! { diff --git a/substrate/frame/support/procedural/src/pallet/expand/hooks.rs b/substrate/frame/support/procedural/src/pallet/expand/hooks.rs index e412581d11a6..37fde6568833 100644 --- a/substrate/frame/support/procedural/src/pallet/expand/hooks.rs +++ b/substrate/frame/support/procedural/src/pallet/expand/hooks.rs @@ -143,222 +143,222 @@ pub fn expand_hooks(def: &mut Def) -> proc_macro2::TokenStream { }; quote::quote_spanned!(span => - #hooks_impl - - impl<#type_impl_gen> - #frame_support::traits::OnFinalize<#frame_system::pallet_prelude::BlockNumberFor::> - for #pallet_ident<#type_use_gen> #where_clause - { - fn on_finalize(n: #frame_system::pallet_prelude::BlockNumberFor::) { - #frame_support::__private::sp_tracing::enter_span!( - #frame_support::__private::sp_tracing::trace_span!("on_finalize") - ); - < - Self as #frame_support::traits::Hooks< - #frame_system::pallet_prelude::BlockNumberFor:: - > - >::on_finalize(n) - } + #hooks_impl + + impl<#type_impl_gen> + #frame_support::traits::OnFinalize<#frame_system::pallet_prelude::BlockNumberFor::> + for #pallet_ident<#type_use_gen> #where_clause + { + fn on_finalize(n: #frame_system::pallet_prelude::BlockNumberFor::) { + #frame_support::__private::sp_tracing::enter_span!( + #frame_support::__private::sp_tracing::trace_span!("on_finalize") + ); + < + Self as #frame_support::traits::Hooks< + #frame_system::pallet_prelude::BlockNumberFor:: + > + >::on_finalize(n) } + } - impl<#type_impl_gen> - #frame_support::traits::OnIdle<#frame_system::pallet_prelude::BlockNumberFor::> - for #pallet_ident<#type_use_gen> #where_clause - { - fn on_idle( - n: #frame_system::pallet_prelude::BlockNumberFor::, - remaining_weight: #frame_support::weights::Weight - ) -> #frame_support::weights::Weight { - < - Self as #frame_support::traits::Hooks< - #frame_system::pallet_prelude::BlockNumberFor:: - > - >::on_idle(n, remaining_weight) - } + impl<#type_impl_gen> + #frame_support::traits::OnIdle<#frame_system::pallet_prelude::BlockNumberFor::> + for #pallet_ident<#type_use_gen> #where_clause + { + fn on_idle( + n: #frame_system::pallet_prelude::BlockNumberFor::, + remaining_weight: #frame_support::weights::Weight + ) -> #frame_support::weights::Weight { + < + Self as #frame_support::traits::Hooks< + #frame_system::pallet_prelude::BlockNumberFor:: + > + >::on_idle(n, remaining_weight) } + } - impl<#type_impl_gen> - #frame_support::traits::OnPoll<#frame_system::pallet_prelude::BlockNumberFor::> - for #pallet_ident<#type_use_gen> #where_clause - { - fn on_poll( - n: #frame_system::pallet_prelude::BlockNumberFor::, - weight: &mut #frame_support::weights::WeightMeter - ) { - < - Self as #frame_support::traits::Hooks< - #frame_system::pallet_prelude::BlockNumberFor:: - > - >::on_poll(n, weight); - } + impl<#type_impl_gen> + #frame_support::traits::OnPoll<#frame_system::pallet_prelude::BlockNumberFor::> + for #pallet_ident<#type_use_gen> #where_clause + { + fn on_poll( + n: #frame_system::pallet_prelude::BlockNumberFor::, + weight: &mut #frame_support::weights::WeightMeter + ) { + < + Self as #frame_support::traits::Hooks< + #frame_system::pallet_prelude::BlockNumberFor:: + > + >::on_poll(n, weight); } + } - impl<#type_impl_gen> - #frame_support::traits::OnInitialize<#frame_system::pallet_prelude::BlockNumberFor::> - for #pallet_ident<#type_use_gen> #where_clause - { - fn on_initialize( - n: #frame_system::pallet_prelude::BlockNumberFor:: - ) -> #frame_support::weights::Weight { - #frame_support::__private::sp_tracing::enter_span!( - #frame_support::__private::sp_tracing::trace_span!("on_initialize") - ); - < - Self as #frame_support::traits::Hooks< - #frame_system::pallet_prelude::BlockNumberFor:: - > - >::on_initialize(n) - } + impl<#type_impl_gen> + #frame_support::traits::OnInitialize<#frame_system::pallet_prelude::BlockNumberFor::> + for #pallet_ident<#type_use_gen> #where_clause + { + fn on_initialize( + n: #frame_system::pallet_prelude::BlockNumberFor:: + ) -> #frame_support::weights::Weight { + #frame_support::__private::sp_tracing::enter_span!( + #frame_support::__private::sp_tracing::trace_span!("on_initialize") + ); + < + Self as #frame_support::traits::Hooks< + #frame_system::pallet_prelude::BlockNumberFor:: + > + >::on_initialize(n) } + } - impl<#type_impl_gen> - #frame_support::traits::BeforeAllRuntimeMigrations - for #pallet_ident<#type_use_gen> #where_clause - { - fn before_all_runtime_migrations() -> #frame_support::weights::Weight { - use #frame_support::traits::{Get, PalletInfoAccess}; - use #frame_support::__private::hashing::twox_128; - use #frame_support::storage::unhashed::contains_prefixed_key; - #frame_support::__private::sp_tracing::enter_span!( - #frame_support::__private::sp_tracing::trace_span!("before_all") - ); + impl<#type_impl_gen> + #frame_support::traits::BeforeAllRuntimeMigrations + for #pallet_ident<#type_use_gen> #where_clause + { + fn before_all_runtime_migrations() -> #frame_support::weights::Weight { + use #frame_support::traits::{Get, PalletInfoAccess}; + use #frame_support::__private::hashing::twox_128; + use #frame_support::storage::unhashed::contains_prefixed_key; + #frame_support::__private::sp_tracing::enter_span!( + #frame_support::__private::sp_tracing::trace_span!("before_all") + ); - // Check if the pallet has any keys set, including the storage version. If there are - // no keys set, the pallet was just added to the runtime and needs to have its - // version initialized. - let pallet_hashed_prefix = ::name_hash(); - let exists = contains_prefixed_key(&pallet_hashed_prefix); - if !exists { - #initialize_on_chain_storage_version - ::DbWeight::get().reads_writes(1, 1) - } else { - ::DbWeight::get().reads(1) - } + // Check if the pallet has any keys set, including the storage version. If there are + // no keys set, the pallet was just added to the runtime and needs to have its + // version initialized. + let pallet_hashed_prefix = ::name_hash(); + let exists = contains_prefixed_key(&pallet_hashed_prefix); + if !exists { + #initialize_on_chain_storage_version + ::DbWeight::get().reads_writes(1, 1) + } else { + ::DbWeight::get().reads(1) } } + } - impl<#type_impl_gen> - #frame_support::traits::OnRuntimeUpgrade - for #pallet_ident<#type_use_gen> #where_clause - { - fn on_runtime_upgrade() -> #frame_support::weights::Weight { - #frame_support::__private::sp_tracing::enter_span!( - #frame_support::__private::sp_tracing::trace_span!("on_runtime_update") - ); + impl<#type_impl_gen> + #frame_support::traits::OnRuntimeUpgrade + for #pallet_ident<#type_use_gen> #where_clause + { + fn on_runtime_upgrade() -> #frame_support::weights::Weight { + #frame_support::__private::sp_tracing::enter_span!( + #frame_support::__private::sp_tracing::trace_span!("on_runtime_update") + ); - // log info about the upgrade. - #log_runtime_upgrade + // log info about the upgrade. + #log_runtime_upgrade - < - Self as #frame_support::traits::Hooks< - #frame_system::pallet_prelude::BlockNumberFor:: - > - >::on_runtime_upgrade() - } + < + Self as #frame_support::traits::Hooks< + #frame_system::pallet_prelude::BlockNumberFor:: + > + >::on_runtime_upgrade() + } - #[cfg(feature = "try-runtime")] - fn pre_upgrade() -> Result<#frame_support::__private::sp_std::vec::Vec, #frame_support::sp_runtime::TryRuntimeError> { - < - Self - as - #frame_support::traits::Hooks<#frame_system::pallet_prelude::BlockNumberFor::> - >::pre_upgrade() - } + #[cfg(feature = "try-runtime")] + fn pre_upgrade() -> Result<#frame_support::__private::sp_std::vec::Vec, #frame_support::sp_runtime::TryRuntimeError> { + < + Self + as + #frame_support::traits::Hooks<#frame_system::pallet_prelude::BlockNumberFor::> + >::pre_upgrade() + } - #[cfg(feature = "try-runtime")] - fn post_upgrade(state: #frame_support::__private::sp_std::vec::Vec) -> Result<(), #frame_support::sp_runtime::TryRuntimeError> { - #post_storage_version_check + #[cfg(feature = "try-runtime")] + fn post_upgrade(state: #frame_support::__private::sp_std::vec::Vec) -> Result<(), #frame_support::sp_runtime::TryRuntimeError> { + #post_storage_version_check + + < + Self + as + #frame_support::traits::Hooks<#frame_system::pallet_prelude::BlockNumberFor::> + >::post_upgrade(state) + } + } - < - Self - as - #frame_support::traits::Hooks<#frame_system::pallet_prelude::BlockNumberFor::> - >::post_upgrade(state) - } + impl<#type_impl_gen> + #frame_support::traits::OffchainWorker<#frame_system::pallet_prelude::BlockNumberFor::> + for #pallet_ident<#type_use_gen> #where_clause + { + fn offchain_worker(n: #frame_system::pallet_prelude::BlockNumberFor::) { + < + Self as #frame_support::traits::Hooks< + #frame_system::pallet_prelude::BlockNumberFor:: + > + >::offchain_worker(n) } + } + // Integrity tests are only required for when `std` is enabled. + #frame_support::std_enabled! { impl<#type_impl_gen> - #frame_support::traits::OffchainWorker<#frame_system::pallet_prelude::BlockNumberFor::> - for #pallet_ident<#type_use_gen> #where_clause + #frame_support::traits::IntegrityTest + for #pallet_ident<#type_use_gen> #where_clause { - fn offchain_worker(n: #frame_system::pallet_prelude::BlockNumberFor::) { - < - Self as #frame_support::traits::Hooks< - #frame_system::pallet_prelude::BlockNumberFor:: - > - >::offchain_worker(n) - } - } - - // Integrity tests are only required for when `std` is enabled. - #frame_support::std_enabled! { - impl<#type_impl_gen> - #frame_support::traits::IntegrityTest - for #pallet_ident<#type_use_gen> #where_clause - { - fn integrity_test() { - #frame_support::__private::sp_io::TestExternalities::default().execute_with(|| { - < - Self as #frame_support::traits::Hooks< - #frame_system::pallet_prelude::BlockNumberFor:: - > - >::integrity_test() - }); - } + fn integrity_test() { + #frame_support::__private::sp_io::TestExternalities::default().execute_with(|| { + < + Self as #frame_support::traits::Hooks< + #frame_system::pallet_prelude::BlockNumberFor:: + > + >::integrity_test() + }); } } + } - #[cfg(feature = "try-runtime")] - impl<#type_impl_gen> - #frame_support::traits::TryState<#frame_system::pallet_prelude::BlockNumberFor::> - for #pallet_ident<#type_use_gen> #where_clause - { - fn try_state( - n: #frame_system::pallet_prelude::BlockNumberFor::, - _s: #frame_support::traits::TryStateSelect - ) -> Result<(), #frame_support::sp_runtime::TryRuntimeError> { - #frame_support::__private::log::info!( + #[cfg(feature = "try-runtime")] + impl<#type_impl_gen> + #frame_support::traits::TryState<#frame_system::pallet_prelude::BlockNumberFor::> + for #pallet_ident<#type_use_gen> #where_clause + { + fn try_state( + n: #frame_system::pallet_prelude::BlockNumberFor::, + _s: #frame_support::traits::TryStateSelect + ) -> Result<(), #frame_support::sp_runtime::TryRuntimeError> { + #frame_support::__private::log::info!( + target: #frame_support::LOG_TARGET, + "🩺 Running {:?} try-state checks", + #pallet_name, + ); + < + Self as #frame_support::traits::Hooks< + #frame_system::pallet_prelude::BlockNumberFor:: + > + >::try_state(n).map_err(|err| { + #frame_support::__private::log::error!( target: #frame_support::LOG_TARGET, - "🩺 Running {:?} try-state checks", + "❌ {:?} try_state checks failed: {:?}", #pallet_name, + err ); - < - Self as #frame_support::traits::Hooks< - #frame_system::pallet_prelude::BlockNumberFor:: - > - >::try_state(n).map_err(|err| { - #frame_support::__private::log::error!( - target: #frame_support::LOG_TARGET, - "❌ {:?} try_state checks failed: {:?}", - #pallet_name, - err - ); - err - }) - } + err + }) } + } - // Implement `TryStateLogic` for `Pallet` - #[cfg(feature = "try-runtime")] - impl<#type_impl_gen> #frame_support::traits::TryStateLogic<#frame_system::pallet_prelude::BlockNumberFor> - for #pallet_ident<#type_use_gen> - #where_clause - { - fn try_state(n: frame_system::pallet_prelude::BlockNumberFor) -> Result<(), #frame_support::sp_runtime::TryRuntimeError> { - >>::try_state(n, #frame_support::traits::TryStateSelect::All) - } + // Implement `TryStateLogic` for `Pallet` + #[cfg(feature = "try-runtime")] + impl<#type_impl_gen> #frame_support::traits::TryStateLogic<#frame_system::pallet_prelude::BlockNumberFor> + for #pallet_ident<#type_use_gen> + #where_clause + { + fn try_state(n: frame_system::pallet_prelude::BlockNumberFor) -> Result<(), #frame_support::sp_runtime::TryRuntimeError> { + >>::try_state(n, #frame_support::traits::TryStateSelect::All) } + } - // Implement `IdentifiableTryStateLogic` for `Pallet` - #[cfg(feature = "try-runtime")] - impl<#type_impl_gen> #frame_support::traits::IdentifiableTryStateLogic> - for #pallet_ident<#type_use_gen> - #where_clause - { - fn matches_id(id: &[u8]) -> bool { - ::name().as_bytes() == id - } + // Implement `IdentifiableTryStateLogic` for `Pallet` + #[cfg(feature = "try-runtime")] + impl<#type_impl_gen> #frame_support::traits::IdentifiableTryStateLogic> + for #pallet_ident<#type_use_gen> + #where_clause + { + fn matches_id(id: &[u8]) -> bool { + ::name().as_bytes() == id } + } ) } diff --git a/substrate/frame/support/src/traits/try_runtime/mod.rs b/substrate/frame/support/src/traits/try_runtime/mod.rs index 5205c0cb4dc9..dcf323fd01de 100644 --- a/substrate/frame/support/src/traits/try_runtime/mod.rs +++ b/substrate/frame/support/src/traits/try_runtime/mod.rs @@ -248,7 +248,7 @@ where "Detected errors while executing `try_state` checks. See logs for more \ info." .into(), - ); + ) } Ok(()) diff --git a/substrate/primitives/io/src/lib.rs b/substrate/primitives/io/src/lib.rs index 7e753c9e92fe..67e822ba7e24 100644 --- a/substrate/primitives/io/src/lib.rs +++ b/substrate/primitives/io/src/lib.rs @@ -836,7 +836,7 @@ pub trait Crypto { use ed25519_dalek::Verifier; let Ok(public_key) = ed25519_dalek::VerifyingKey::from_bytes(&pub_key.0) else { - return false; + return false }; let sig = ed25519_dalek::Signature::from_bytes(&sig.0); From 56e91c7b3915a0d538a72a9ccc3ccc4f663f0106 Mon Sep 17 00:00:00 2001 From: Antonio Antonino Date: Sat, 13 Jul 2024 12:15:15 +0200 Subject: [PATCH 17/18] Address first PR comment --- substrate/frame/executive/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/executive/README.md b/substrate/frame/executive/README.md index 6151232ecaf1..44a058356751 100644 --- a/substrate/frame/executive/README.md +++ b/substrate/frame/executive/README.md @@ -53,7 +53,7 @@ generic parameter. The custom logic will be called before the on runtime upgrade struct CustomOnRuntimeUpgrade; impl frame_support::traits::OnRuntimeUpgrade for CustomOnRuntimeUpgrade { fn on_runtime_upgrade() -> frame_support::weights::Weight { - // Do whatever you want. + // Do whatever you want besides modifying storage. frame_support::weights::Weight::zero() } } From a87c5947c8c025981475c52121e5ce48c42b0eb2 Mon Sep 17 00:00:00 2001 From: Antonio Antonino Date: Mon, 15 Jul 2024 09:42:39 +0200 Subject: [PATCH 18/18] Replace global static mut with static parameter_types! --- substrate/frame/executive/src/tests.rs | 27 +++++++++++++------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/substrate/frame/executive/src/tests.rs b/substrate/frame/executive/src/tests.rs index f14ce13be481..58638502a803 100644 --- a/substrate/frame/executive/src/tests.rs +++ b/substrate/frame/executive/src/tests.rs @@ -46,11 +46,16 @@ const TEST_KEY: &[u8] = b":test:key:"; #[cfg(feature = "try-runtime")] mod try_runtime { - use frame_support::traits::{IdentifiableTryStateLogic, TryStateLogic}; + use frame_support::{ + parameter_types, + traits::{IdentifiableTryStateLogic, TryStateLogic}, + }; use sp_runtime::traits::AtLeast32BitUnsigned; // Will contain `true` when the custom runtime logic is called. - pub(super) static mut CANARY_FLAG: bool = false; + parameter_types! { + pub(super) static CanaryFlag: bool = false; + } const CUSTOM_TRY_STATE_ID: &[u8] = b"custom_try_state"; @@ -61,9 +66,7 @@ mod try_runtime { BlockNumber: Clone + sp_std::fmt::Debug + AtLeast32BitUnsigned, { fn try_state(_: BlockNumber) -> Result<(), sp_runtime::TryRuntimeError> { - unsafe { - CANARY_FLAG = true; - } + CanaryFlag::set(true); Ok(()) } } @@ -116,12 +119,10 @@ mod custom { // Verify that `CustomTryState` has been called before. #[cfg(feature = "try-runtime")] fn try_state(_n: BlockNumberFor) -> Result<(), sp_runtime::TryRuntimeError> { - unsafe { - assert!( - try_runtime::CANARY_FLAG, - "Custom `try-runtime` did not run before pallet `try-runtime`." - ); - } + assert!( + try_runtime::CanaryFlag::get(), + "Custom `try-runtime` did not run before pallet `try-runtime`." + ); Ok(()) } } @@ -577,9 +578,7 @@ fn new_test_ext(balance_factor: Balance) -> sp_io::TestExternalities { }); #[cfg(feature = "try-runtime")] - unsafe { - try_runtime::CANARY_FLAG = false; - } + try_runtime::CanaryFlag::set(false); ext }