Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Commit

Permalink
[Fix] Try-state feature-gated for BagsList (#13296)
Browse files Browse the repository at this point in the history
* [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 <anton.kalyaev@gmail.com>

---------

Co-authored-by: parity-processbot <>
Co-authored-by: Anton <anton.kalyaev@gmail.com>
  • Loading branch information
2 people authored and gpestana committed Feb 14, 2023
1 parent 8511c9b commit 1651553
Show file tree
Hide file tree
Showing 13 changed files with 46 additions and 30 deletions.
2 changes: 1 addition & 1 deletion frame/bags-list/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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" ]
2 changes: 1 addition & 1 deletion frame/bags-list/fuzzer/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ fn main() {
},
}

assert!(BagsList::try_state().is_ok());
assert!(BagsList::do_try_state().is_ok());
})
});
}
2 changes: 1 addition & 1 deletion frame/bags-list/remote-tests/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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" }
Expand Down
5 changes: 3 additions & 2 deletions frame/bags-list/remote-tests/src/try_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand Down Expand Up @@ -51,7 +50,9 @@ pub async fn execute<Runtime, Block>(

ext.execute_with(|| {
sp_core::crypto::set_default_ss58_version(Runtime::SS58Prefix::get().try_into().unwrap());
pallet_bags_list::Pallet::<Runtime, pallet_bags_list::Instance1>::try_state().unwrap();

pallet_bags_list::Pallet::<Runtime, pallet_bags_list::Instance1>::do_try_state().unwrap();

log::info!(target: crate::LOG_TARGET, "executed bags-list sanity check with no errors.");

crate::display_and_check_bags::<Runtime>(currency_unit, currency_name);
Expand Down
10 changes: 9 additions & 1 deletion frame/bags-list/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,13 @@ pub mod pallet {
}
}

#[cfg(any(test, feature = "try-runtime", feature = "fuzz"))]
impl<T: Config<I>, I: 'static> Pallet<T, I> {
pub fn do_try_state() -> Result<(), &'static str> {
List::<T, I>::do_try_state()
}
}

impl<T: Config<I>, I: 'static> Pallet<T, I> {
/// Move an account from one bag to another, depositing an event on success.
///
Expand Down Expand Up @@ -348,8 +355,9 @@ impl<T: Config<I>, I: 'static> SortedListProvider<T::AccountId> for Pallet<T, I>
List::<T, I>::unsafe_regenerate(all, score_of)
}

#[cfg(feature = "try-runtime")]
fn try_state() -> Result<(), &'static str> {
List::<T, I>::try_state()
Self::do_try_state()
}

fn unsafe_clear() {
Expand Down
17 changes: 9 additions & 8 deletions frame/bags-list/src/list/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -220,9 +220,6 @@ impl<T: Config<I>, I: 'static> List<T, I> {
crate::ListBags::<T, I>::remove(removed_bag);
}

#[cfg(feature = "std")]
debug_assert_eq!(Self::try_state(), Ok(()));

num_affected
}

Expand Down Expand Up @@ -514,7 +511,8 @@ impl<T: Config<I>, I: 'static> List<T, I> {
/// * 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)),
Expand Down Expand Up @@ -542,7 +540,7 @@ impl<T: Config<I>, I: 'static> List<T, I> {
thresholds.into_iter().filter_map(|t| Bag::<T, I>::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);
Expand All @@ -553,7 +551,7 @@ impl<T: Config<I>, I: 'static> List<T, I> {
// 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::<T, I>::iter() {
node.try_state()?
node.do_try_state()?
}

Ok(())
Expand Down Expand Up @@ -751,7 +749,8 @@ impl<T: Config<I>, I: 'static> Bag<T, I> {
/// * 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())
Expand Down Expand Up @@ -790,6 +789,7 @@ impl<T: Config<I>, I: 'static> Bag<T, I> {
}

/// 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)
}
Expand Down Expand Up @@ -894,7 +894,8 @@ impl<T: Config<I>, I: 'static> Node<T, I> {
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::<T, I>::get(self.bag_upper).ok_or("bag not found for node")?;

let id = self.id();
Expand Down
25 changes: 13 additions & 12 deletions frame/bags-list/src/list/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,7 @@ fn migrate_works() {
BagThresholds::set(NEW_THRESHOLDS);
// and we call
List::<Runtime>::migrate(old_thresholds);
assert_eq!(List::<Runtime>::do_try_state(), Ok(()));

// then
assert_eq!(
Expand Down Expand Up @@ -352,13 +353,13 @@ mod list {
#[test]
fn try_state_works() {
ExtBuilder::default().build_and_execute_no_post_check(|| {
assert_ok!(List::<Runtime>::try_state());
assert_ok!(List::<Runtime>::do_try_state());
});

// make sure there are no duplicates.
ExtBuilder::default().build_and_execute_no_post_check(|| {
Bag::<Runtime>::get(10).unwrap().insert_unchecked(2, 10);
assert_eq!(List::<Runtime>::try_state(), Err("duplicate identified"));
assert_eq!(List::<Runtime>::do_try_state(), Err("duplicate identified"));
});

// ensure count is in sync with `ListNodes::count()`.
Expand All @@ -372,7 +373,7 @@ mod list {
CounterForListNodes::<Runtime>::mutate(|counter| *counter += 1);
assert_eq!(crate::ListNodes::<Runtime>::count(), 5);

assert_eq!(List::<Runtime>::try_state(), Err("iter_count != stored_count"));
assert_eq!(List::<Runtime>::do_try_state(), Err("iter_count != stored_count"));
});
}

Expand Down Expand Up @@ -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);

Expand All @@ -814,23 +815,23 @@ 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::<Runtime>::get(&14).unwrap();
bag_1000.remove_node_unchecked(&node_14);

// 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::<Runtime>::get(&13).unwrap();
bag_1000.remove_node_unchecked(&node_13);

// 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::<Runtime>::get(&3).unwrap();
Expand All @@ -846,15 +847,15 @@ 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::<Runtime>::get(&1).unwrap();
bag_10.remove_node_unchecked(&node_1);

// 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();
Expand All @@ -865,15 +866,15 @@ 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::<Runtime>::get(&18).unwrap();
bag_2000.remove_node_unchecked(&node_18);

// 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!(
Expand Down Expand Up @@ -905,7 +906,7 @@ mod bags {
// .. and the bag it was removed from
let bag_1000 = Bag::<Runtime>::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));
Expand Down
2 changes: 1 addition & 1 deletion frame/bags-list/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ impl ExtBuilder {
pub fn build_and_execute(self, test: impl FnOnce() -> ()) {
self.build().execute_with(|| {
test();
List::<Runtime>::try_state().expect("Try-state post condition failed")
List::<Runtime>::do_try_state().expect("do_try_state post condition failed")
})
}

Expand Down
1 change: 1 addition & 0 deletions frame/election-provider-support/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -43,3 +43,4 @@ std = [
"sp-std/std",
]
runtime-benchmarks = []
try-runtime = []
3 changes: 2 additions & 1 deletion frame/election-provider-support/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -562,7 +562,8 @@ pub trait SortedListProvider<AccountId> {
/// 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
Expand Down
2 changes: 1 addition & 1 deletion frame/staking/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"]
1 change: 0 additions & 1 deletion frame/staking/src/migrations.rs
Original file line number Diff line number Diff line change
Expand Up @@ -386,7 +386,6 @@ pub mod v8 {
Nominators::<T>::iter().map(|(id, _)| id),
Pallet::<T>::weight_of_fn(),
);
debug_assert_eq!(T::VoterList::try_state(), Ok(()));

StorageVersion::<T>::put(ObsoleteReleases::V8_0_0);
crate::log!(
Expand Down
4 changes: 4 additions & 0 deletions frame/staking/src/pallet/impls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1451,9 +1451,11 @@ impl<T: Config> SortedListProvider<T::AccountId> for UseValidatorsMap<T> {
// nothing to do upon regenerate.
0
}
#[cfg(feature = "try-runtime")]
fn try_state() -> Result<(), &'static str> {
Ok(())
}

fn unsafe_clear() {
#[allow(deprecated)]
Validators::<T>::remove_all();
Expand Down Expand Up @@ -1525,6 +1527,8 @@ impl<T: Config> SortedListProvider<T::AccountId> for UseNominatorsAndValidatorsM
// nothing to do upon regenerate.
0
}

#[cfg(feature = "try-runtime")]
fn try_state() -> Result<(), &'static str> {
Ok(())
}
Expand Down

0 comments on commit 1651553

Please sign in to comment.