From 165155326cd5e27a47029c5820d637d3cace4598 Mon Sep 17 00:00:00 2001 From: Roman Useinov Date: Thu, 9 Feb 2023 12:47:39 +0100 Subject: [PATCH] [Fix] Try-state feature-gated for BagsList (#13296) * [Fix] Try-state feature-gated for BagsList * fix comment * fix try_state remote-tests * feature-gate try-state remote test for bags-list * remove try-state from a migration * more SortedListProvider fixes * more fixes * more fixes to allow do_try_state usage in other crates * do-try-state for fuzz * more fixes * more fixes * remove feature-flag * do-try-state * fix review comments * Update frame/bags-list/src/mock.rs Co-authored-by: Anton --------- Co-authored-by: parity-processbot <> Co-authored-by: Anton --- frame/bags-list/Cargo.toml | 2 +- frame/bags-list/fuzzer/src/main.rs | 2 +- frame/bags-list/remote-tests/Cargo.toml | 2 +- frame/bags-list/remote-tests/src/try_state.rs | 5 ++-- frame/bags-list/src/lib.rs | 10 +++++++- frame/bags-list/src/list/mod.rs | 17 +++++++------ frame/bags-list/src/list/tests.rs | 25 ++++++++++--------- frame/bags-list/src/mock.rs | 2 +- frame/election-provider-support/Cargo.toml | 1 + frame/election-provider-support/src/lib.rs | 3 ++- frame/staking/Cargo.toml | 2 +- frame/staking/src/migrations.rs | 1 - frame/staking/src/pallet/impls.rs | 4 +++ 13 files changed, 46 insertions(+), 30 deletions(-) diff --git a/frame/bags-list/Cargo.toml b/frame/bags-list/Cargo.toml index e3a4965f6c086..379698b1d39e1 100644 --- a/frame/bags-list/Cargo.toml +++ b/frame/bags-list/Cargo.toml @@ -75,4 +75,4 @@ fuzz = [ "sp-tracing", "frame-election-provider-support/fuzz", ] -try-runtime = [ "frame-support/try-runtime" ] +try-runtime = [ "frame-support/try-runtime", "frame-election-provider-support/try-runtime" ] diff --git a/frame/bags-list/fuzzer/src/main.rs b/frame/bags-list/fuzzer/src/main.rs index 9f7ca464cc2b8..c78e2a13076d5 100644 --- a/frame/bags-list/fuzzer/src/main.rs +++ b/frame/bags-list/fuzzer/src/main.rs @@ -88,7 +88,7 @@ fn main() { }, } - assert!(BagsList::try_state().is_ok()); + assert!(BagsList::do_try_state().is_ok()); }) }); } diff --git a/frame/bags-list/remote-tests/Cargo.toml b/frame/bags-list/remote-tests/Cargo.toml index 9fb6d0154b302..6e951b43a4aeb 100644 --- a/frame/bags-list/remote-tests/Cargo.toml +++ b/frame/bags-list/remote-tests/Cargo.toml @@ -15,7 +15,7 @@ targets = ["x86_64-unknown-linux-gnu"] [dependencies] # frame pallet-staking = { path = "../../staking", version = "4.0.0-dev" } -pallet-bags-list = { path = "../../bags-list", version = "4.0.0-dev" } +pallet-bags-list = { path = "../../bags-list", version = "4.0.0-dev", features = ["fuzz"] } frame-election-provider-support = { path = "../../election-provider-support", version = "4.0.0-dev" } frame-system = { path = "../../system", version = "4.0.0-dev" } frame-support = { path = "../../support", version = "4.0.0-dev" } diff --git a/frame/bags-list/remote-tests/src/try_state.rs b/frame/bags-list/remote-tests/src/try_state.rs index 514c80d72ab67..9ed877a43afe1 100644 --- a/frame/bags-list/remote-tests/src/try_state.rs +++ b/frame/bags-list/remote-tests/src/try_state.rs @@ -16,7 +16,6 @@ //! Test to execute the sanity-check of the voter bag. -use frame_election_provider_support::SortedListProvider; use frame_support::{ storage::generator::StorageMap, traits::{Get, PalletInfoAccess}, @@ -51,7 +50,9 @@ pub async fn execute( ext.execute_with(|| { sp_core::crypto::set_default_ss58_version(Runtime::SS58Prefix::get().try_into().unwrap()); - pallet_bags_list::Pallet::::try_state().unwrap(); + + pallet_bags_list::Pallet::::do_try_state().unwrap(); + log::info!(target: crate::LOG_TARGET, "executed bags-list sanity check with no errors."); crate::display_and_check_bags::(currency_unit, currency_name); diff --git a/frame/bags-list/src/lib.rs b/frame/bags-list/src/lib.rs index 14f8a613eb798..f04c685ae386c 100644 --- a/frame/bags-list/src/lib.rs +++ b/frame/bags-list/src/lib.rs @@ -274,6 +274,13 @@ pub mod pallet { } } +#[cfg(any(test, feature = "try-runtime", feature = "fuzz"))] +impl, I: 'static> Pallet { + pub fn do_try_state() -> Result<(), &'static str> { + List::::do_try_state() + } +} + impl, I: 'static> Pallet { /// Move an account from one bag to another, depositing an event on success. /// @@ -348,8 +355,9 @@ impl, I: 'static> SortedListProvider for Pallet List::::unsafe_regenerate(all, score_of) } + #[cfg(feature = "try-runtime")] fn try_state() -> Result<(), &'static str> { - List::::try_state() + Self::do_try_state() } fn unsafe_clear() { diff --git a/frame/bags-list/src/list/mod.rs b/frame/bags-list/src/list/mod.rs index 272526ad1a636..4082a5324091b 100644 --- a/frame/bags-list/src/list/mod.rs +++ b/frame/bags-list/src/list/mod.rs @@ -220,9 +220,6 @@ impl, I: 'static> List { crate::ListBags::::remove(removed_bag); } - #[cfg(feature = "std")] - debug_assert_eq!(Self::try_state(), Ok(())); - num_affected } @@ -514,7 +511,8 @@ impl, I: 'static> List { /// * length of this list is in sync with `ListNodes::count()`, /// * and sanity-checks all bags and nodes. This will cascade down all the checks and makes sure /// all bags and nodes are checked per *any* update to `List`. - pub(crate) fn try_state() -> Result<(), &'static str> { + #[cfg(any(test, feature = "try-runtime", feature = "fuzz"))] + pub(crate) fn do_try_state() -> Result<(), &'static str> { let mut seen_in_list = BTreeSet::new(); ensure!( Self::iter().map(|node| node.id).all(|id| seen_in_list.insert(id)), @@ -542,7 +540,7 @@ impl, I: 'static> List { thresholds.into_iter().filter_map(|t| Bag::::get(t)) }; - let _ = active_bags.clone().try_for_each(|b| b.try_state())?; + let _ = active_bags.clone().try_for_each(|b| b.do_try_state())?; let nodes_in_bags_count = active_bags.clone().fold(0u32, |acc, cur| acc + cur.iter().count() as u32); @@ -553,7 +551,7 @@ impl, I: 'static> List { // check that all nodes are sane. We check the `ListNodes` storage item directly in case we // have some "stale" nodes that are not in a bag. for (_id, node) in crate::ListNodes::::iter() { - node.try_state()? + node.do_try_state()? } Ok(()) @@ -751,7 +749,8 @@ impl, I: 'static> Bag { /// * Ensures head has no prev. /// * Ensures tail has no next. /// * Ensures there are no loops, traversal from head to tail is correct. - fn try_state(&self) -> Result<(), &'static str> { + #[cfg(any(test, feature = "try-runtime", feature = "fuzz"))] + fn do_try_state(&self) -> Result<(), &'static str> { frame_support::ensure!( self.head() .map(|head| head.prev().is_none()) @@ -790,6 +789,7 @@ impl, I: 'static> Bag { } /// Check if the bag contains a node with `id`. + #[cfg(any(test, feature = "try-runtime", feature = "fuzz"))] fn contains(&self, id: &T::AccountId) -> bool { self.iter().any(|n| n.id() == id) } @@ -894,7 +894,8 @@ impl, I: 'static> Node { self.bag_upper } - fn try_state(&self) -> Result<(), &'static str> { + #[cfg(any(test, feature = "try-runtime", feature = "fuzz"))] + fn do_try_state(&self) -> Result<(), &'static str> { let expected_bag = Bag::::get(self.bag_upper).ok_or("bag not found for node")?; let id = self.id(); diff --git a/frame/bags-list/src/list/tests.rs b/frame/bags-list/src/list/tests.rs index 966ea1a74c71c..3c4aa7c86634d 100644 --- a/frame/bags-list/src/list/tests.rs +++ b/frame/bags-list/src/list/tests.rs @@ -137,6 +137,7 @@ fn migrate_works() { BagThresholds::set(NEW_THRESHOLDS); // and we call List::::migrate(old_thresholds); + assert_eq!(List::::do_try_state(), Ok(())); // then assert_eq!( @@ -352,13 +353,13 @@ mod list { #[test] fn try_state_works() { ExtBuilder::default().build_and_execute_no_post_check(|| { - assert_ok!(List::::try_state()); + assert_ok!(List::::do_try_state()); }); // make sure there are no duplicates. ExtBuilder::default().build_and_execute_no_post_check(|| { Bag::::get(10).unwrap().insert_unchecked(2, 10); - assert_eq!(List::::try_state(), Err("duplicate identified")); + assert_eq!(List::::do_try_state(), Err("duplicate identified")); }); // ensure count is in sync with `ListNodes::count()`. @@ -372,7 +373,7 @@ mod list { CounterForListNodes::::mutate(|counter| *counter += 1); assert_eq!(crate::ListNodes::::count(), 5); - assert_eq!(List::::try_state(), Err("iter_count != stored_count")); + assert_eq!(List::::do_try_state(), Err("iter_count != stored_count")); }); } @@ -804,7 +805,7 @@ mod bags { // then assert_eq!(bag_as_ids(&bag_1000), vec![2, 3, 13, 14]); - assert_ok!(bag_1000.try_state()); + assert_ok!(bag_1000.do_try_state()); // and the node isn't mutated when its removed assert_eq!(node_4, node_4_pre_remove); @@ -814,7 +815,7 @@ mod bags { // then assert_eq!(bag_as_ids(&bag_1000), vec![3, 13, 14]); - assert_ok!(bag_1000.try_state()); + assert_ok!(bag_1000.do_try_state()); // when removing a tail that is not pointing at the head let node_14 = Node::::get(&14).unwrap(); @@ -822,7 +823,7 @@ mod bags { // then assert_eq!(bag_as_ids(&bag_1000), vec![3, 13]); - assert_ok!(bag_1000.try_state()); + assert_ok!(bag_1000.do_try_state()); // when removing a tail that is pointing at the head let node_13 = Node::::get(&13).unwrap(); @@ -830,7 +831,7 @@ mod bags { // then assert_eq!(bag_as_ids(&bag_1000), vec![3]); - assert_ok!(bag_1000.try_state()); + assert_ok!(bag_1000.do_try_state()); // when removing a node that is both the head & tail let node_3 = Node::::get(&3).unwrap(); @@ -846,7 +847,7 @@ mod bags { // then assert_eq!(bag_as_ids(&bag_10), vec![1, 12]); - assert_ok!(bag_10.try_state()); + assert_ok!(bag_10.do_try_state()); // when removing a head that is pointing at the tail let node_1 = Node::::get(&1).unwrap(); @@ -854,7 +855,7 @@ mod bags { // then assert_eq!(bag_as_ids(&bag_10), vec![12]); - assert_ok!(bag_10.try_state()); + assert_ok!(bag_10.do_try_state()); // and since we updated the bag's head/tail, we need to write this storage so we // can correctly `get` it again in later checks bag_10.put(); @@ -865,7 +866,7 @@ mod bags { // then assert_eq!(bag_as_ids(&bag_2000), vec![15, 17, 18, 19]); - assert_ok!(bag_2000.try_state()); + assert_ok!(bag_2000.do_try_state()); // when removing a node that is pointing at tail, but not head let node_18 = Node::::get(&18).unwrap(); @@ -873,7 +874,7 @@ mod bags { // then assert_eq!(bag_as_ids(&bag_2000), vec![15, 17, 19]); - assert_ok!(bag_2000.try_state()); + assert_ok!(bag_2000.do_try_state()); // finally, when reading from storage, the state of all bags is as expected assert_eq!( @@ -905,7 +906,7 @@ mod bags { // .. and the bag it was removed from let bag_1000 = Bag::::get(1_000).unwrap(); // is sane - assert_ok!(bag_1000.try_state()); + assert_ok!(bag_1000.do_try_state()); // and has the correct head and tail. assert_eq!(bag_1000.head, Some(3)); assert_eq!(bag_1000.tail, Some(4)); diff --git a/frame/bags-list/src/mock.rs b/frame/bags-list/src/mock.rs index 8cc96a988e72a..32f6bb09e4772 100644 --- a/frame/bags-list/src/mock.rs +++ b/frame/bags-list/src/mock.rs @@ -148,7 +148,7 @@ impl ExtBuilder { pub fn build_and_execute(self, test: impl FnOnce() -> ()) { self.build().execute_with(|| { test(); - List::::try_state().expect("Try-state post condition failed") + List::::do_try_state().expect("do_try_state post condition failed") }) } diff --git a/frame/election-provider-support/Cargo.toml b/frame/election-provider-support/Cargo.toml index c7f47e721d1aa..114caee793f1a 100644 --- a/frame/election-provider-support/Cargo.toml +++ b/frame/election-provider-support/Cargo.toml @@ -43,3 +43,4 @@ std = [ "sp-std/std", ] runtime-benchmarks = [] +try-runtime = [] diff --git a/frame/election-provider-support/src/lib.rs b/frame/election-provider-support/src/lib.rs index a0f4dfb974c8d..14018949e6da3 100644 --- a/frame/election-provider-support/src/lib.rs +++ b/frame/election-provider-support/src/lib.rs @@ -562,7 +562,8 @@ pub trait SortedListProvider { /// unbounded amount of storage accesses. fn unsafe_clear(); - /// Check internal state of list. Only meant for debugging. + /// Check internal state of the list. Only meant for debugging. + #[cfg(feature = "try-runtime")] fn try_state() -> Result<(), &'static str>; /// If `who` changes by the returned amount they are guaranteed to have a worst case change diff --git a/frame/staking/Cargo.toml b/frame/staking/Cargo.toml index 3d5cf1161e8e5..79c0bb5c2a32d 100644 --- a/frame/staking/Cargo.toml +++ b/frame/staking/Cargo.toml @@ -74,4 +74,4 @@ runtime-benchmarks = [ "rand_chacha", "sp-staking/runtime-benchmarks", ] -try-runtime = ["frame-support/try-runtime"] +try-runtime = ["frame-support/try-runtime", "frame-election-provider-support/try-runtime"] diff --git a/frame/staking/src/migrations.rs b/frame/staking/src/migrations.rs index 6253c3feed17d..a8d360c098137 100644 --- a/frame/staking/src/migrations.rs +++ b/frame/staking/src/migrations.rs @@ -386,7 +386,6 @@ pub mod v8 { Nominators::::iter().map(|(id, _)| id), Pallet::::weight_of_fn(), ); - debug_assert_eq!(T::VoterList::try_state(), Ok(())); StorageVersion::::put(ObsoleteReleases::V8_0_0); crate::log!( diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index e59c2c5a0620e..6a35e2b861565 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -1451,9 +1451,11 @@ impl SortedListProvider for UseValidatorsMap { // nothing to do upon regenerate. 0 } + #[cfg(feature = "try-runtime")] fn try_state() -> Result<(), &'static str> { Ok(()) } + fn unsafe_clear() { #[allow(deprecated)] Validators::::remove_all(); @@ -1525,6 +1527,8 @@ impl SortedListProvider for UseNominatorsAndValidatorsM // nothing to do upon regenerate. 0 } + + #[cfg(feature = "try-runtime")] fn try_state() -> Result<(), &'static str> { Ok(()) }