From 01a04cc051e7c504700951cdae6591cb169dda99 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Sun, 11 Jul 2021 14:21:44 -0700 Subject: [PATCH 01/26] Add some debug asserts to node::get and remove_node --- frame/staking/src/voter_bags.rs | 37 +++++++++++++++++++++------------ 1 file changed, 24 insertions(+), 13 deletions(-) diff --git a/frame/staking/src/voter_bags.rs b/frame/staking/src/voter_bags.rs index 34efce5c59f02..e668fa50bb8b4 100644 --- a/frame/staking/src/voter_bags.rs +++ b/frame/staking/src/voter_bags.rs @@ -21,20 +21,22 @@ //! - It's efficient to iterate over the top* N voters by stake, where the precise ordering of //! voters doesn't particularly matter. -use crate::{ - AccountIdOf, Config, Nominations, Nominators, Pallet, Validators, VoteWeight, VoterBagFor, - VotingDataOf, slashing::SlashingSpans, -}; -use codec::{Encode, Decode}; -use frame_support::{DefaultNoBound, traits::Get}; +use codec::{Decode, Encode}; +use frame_support::{traits::Get, DefaultNoBound}; use sp_runtime::SaturatedConversion; use sp_std::{ boxed::Box, + cmp::PartialEq, collections::{btree_map::BTreeMap, btree_set::BTreeSet}, iter, marker::PhantomData, }; +use crate::{ + slashing::SlashingSpans, AccountIdOf, Config, Nominations, Nominators, Pallet, Validators, + VoteWeight, VoterBagFor, VotingDataOf, +}; + /// [`Voter`] parametrized by [`Config`] instead of by `AccountId`. pub type VoterOf = Voter>; @@ -460,17 +462,25 @@ impl Bag { /// Storage note: this modifies storage, but only for adjacent nodes. You still need to call /// `self.put()` and `node.put()` after use. fn remove_node(&mut self, node: &Node) { - // TODO: we could merge this function here. - // node.excise(); + // Excise `node`. if let Some(mut prev) = node.prev() { - prev.next = self.next.clone(); + debug_assert!(prev != *node, "node.prev circularly points at node"); + + prev.next = node.next.clone(); + debug_assert!(prev.next().unwrap().prev().unwrap() == *node); + prev.put(); } if let Some(mut next) = node.next() { - next.prev = self.prev.clone(); + // if there is a node.next, we know their should be a node.prev + debug_assert!(next != *node, "node.next circularly points at node"); + debug_assert!(node.prev().unwrap() != next, "node.prev circularly points at node.next"); + + next.prev = node.prev.clone(); + debug_assert!(next.prev().unwrap().next().unwrap() == next); + next.put(); } - // IDEA: debug_assert! prev.next.prev == self // clear the bag head/tail pointers as necessary if self.head.as_ref() == Some(&node.voter.id) { @@ -484,7 +494,7 @@ impl Bag { /// A Node is the fundamental element comprising the doubly-linked lists which for each bag. #[derive(Encode, Decode)] -#[cfg_attr(feature = "std", derive(frame_support::DebugNoBound))] +#[cfg_attr(feature = "std", derive(frame_support::DebugNoBound, PartialEq))] pub struct Node { voter: Voter>, prev: Option>, @@ -498,7 +508,7 @@ pub struct Node { impl Node { /// Get a node by bag idx and account id. pub fn get(bag_upper: VoteWeight, account_id: &AccountIdOf) -> Option> { - // debug_assert!(bag_upper is in Threshold) + debug_assert!(T::VoterBagThresholds::get().contains(&bag_upper)); crate::VoterNodes::::try_get(account_id).ok().map(|mut node| { node.bag_upper = bag_upper; node @@ -566,6 +576,7 @@ impl Node { /// Remove this node from the linked list. /// /// Modifies storage, but only modifies the adjacent nodes. Does not modify `self` or any bag. + #[allow(dead_code)] // TODO: do we keep? (equivalent code in `fn remove_node`) fn excise(&self) { if let Some(mut prev) = self.prev() { prev.next = self.next.clone(); From a37e59d223b7613dbef02f7d729aa18b4a09e94c Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Sun, 11 Jul 2021 17:39:14 -0700 Subject: [PATCH 02/26] Improve the debug asserts in remove_node --- frame/staking/src/mock/voter_bags.rs | 2 +- frame/staking/src/tests.rs | 2 +- frame/staking/src/voter_bags.rs | 36 ++++++++++++++++++++++------ 3 files changed, 31 insertions(+), 9 deletions(-) diff --git a/frame/staking/src/mock/voter_bags.rs b/frame/staking/src/mock/voter_bags.rs index 6177c615bc59a..832fe6721ca0f 100644 --- a/frame/staking/src/mock/voter_bags.rs +++ b/frame/staking/src/mock/voter_bags.rs @@ -31,7 +31,7 @@ pub const EXISTENTIAL_WEIGHT: u64 = 1; pub const CONSTANT_RATIO: f64 = 2.0000000000000000; /// Upper thresholds delimiting the bag list. -pub const THRESHOLDS: [u64; 65] = [5, 15, 25, 50, 100, 1000, 2000, 3000] +// pub const THRESHOLDS: [u64; 65] = [5, 15, 25, 50, 100, 1000, 2000, 3000]; // TODO: switch to this pub const THRESHOLDS: [u64; 65] = [ 1, 2, diff --git a/frame/staking/src/tests.rs b/frame/staking/src/tests.rs index 864fcb0c35683..b7d67e2c93bb2 100644 --- a/frame/staking/src/tests.rs +++ b/frame/staking/src/tests.rs @@ -153,7 +153,7 @@ fn basic_setup_works() { // for these stash ids, see // https://github.com/paritytech/substrate/ // blob/631d4cdbcad438248c2597213918d8207d85bf6e/frame/staking/src/mock.rs#L435-L441 - for genesis_stash_account_id in [11, 21, 31, 101] { + for genesis_stash_account_id in &[11, 21, 31, 101] { let node = crate::voter_bags::Node::::from_id(&genesis_stash_account_id) .expect(&format!("node was created for account {}", genesis_stash_account_id)); assert!(!node.is_misplaced(&weight_of)); diff --git a/frame/staking/src/voter_bags.rs b/frame/staking/src/voter_bags.rs index e668fa50bb8b4..5df263ac391b0 100644 --- a/frame/staking/src/voter_bags.rs +++ b/frame/staking/src/voter_bags.rs @@ -26,7 +26,6 @@ use frame_support::{traits::Get, DefaultNoBound}; use sp_runtime::SaturatedConversion; use sp_std::{ boxed::Box, - cmp::PartialEq, collections::{btree_map::BTreeMap, btree_set::BTreeSet}, iter, marker::PhantomData, @@ -464,20 +463,43 @@ impl Bag { fn remove_node(&mut self, node: &Node) { // Excise `node`. if let Some(mut prev) = node.prev() { - debug_assert!(prev != *node, "node.prev circularly points at node"); + debug_assert!(prev.voter.id != node.voter.id, "node.prev circularly points at node"); + debug_assert!( + self.head.as_ref() != Some(&node.voter.id), + "node is the head, but has Some prev" + ); prev.next = node.next.clone(); - debug_assert!(prev.next().unwrap().prev().unwrap() == *node); + debug_assert!( + prev.next().and_then(|prev_next| + // prev.next.prev should should point at node prior to being reassigned + Some(prev_next.prev().unwrap().voter.id == node.voter.id) + ) + // unless prev.next is None, in which case node has to be the tail + .unwrap_or(self.tail.as_ref() == Some(&node.voter.id)), + "prev.next.prev should should point at node prior to being reassigned (if node is not the tail)" + ); prev.put(); } if let Some(mut next) = node.next() { // if there is a node.next, we know their should be a node.prev - debug_assert!(next != *node, "node.next circularly points at node"); - debug_assert!(node.prev().unwrap() != next, "node.prev circularly points at node.next"); + debug_assert!(next.voter.id != node.voter.id, "node.next circularly points at node"); + debug_assert!( + self.tail.as_ref() != Some(&node.voter.id), + "node is the tail, but has Some next" + ); next.prev = node.prev.clone(); - debug_assert!(next.prev().unwrap().next().unwrap() == next); + debug_assert!( + next.prev().and_then(|next_prev| + // next.prev.next should point at next after being reassigned + Some(next_prev.next().unwrap().voter.id == next.voter.id) + ) + // unless next.prev is None, in which case node has to be the head + .unwrap_or_else(|| self.head.as_ref() == Some(&node.voter.id)), + "next.prev.next should point at next after being reassigned (if node is not the head)" + ); next.put(); } @@ -494,7 +516,7 @@ impl Bag { /// A Node is the fundamental element comprising the doubly-linked lists which for each bag. #[derive(Encode, Decode)] -#[cfg_attr(feature = "std", derive(frame_support::DebugNoBound, PartialEq))] +#[cfg_attr(feature = "std", derive(frame_support::DebugNoBound))] pub struct Node { voter: Voter>, prev: Option>, From f047cec84fe0510d6254f57e4483ff07859fdf93 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Sun, 11 Jul 2021 18:17:43 -0700 Subject: [PATCH 03/26] improve debug asserts --- frame/staking/src/voter_bags.rs | 29 ++++++++++++++++++++--------- 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/frame/staking/src/voter_bags.rs b/frame/staking/src/voter_bags.rs index 5df263ac391b0..edbc966a95938 100644 --- a/frame/staking/src/voter_bags.rs +++ b/frame/staking/src/voter_bags.rs @@ -439,9 +439,9 @@ impl Bag { node.put(); // update the previous tail - if let Some(mut tail) = self.tail() { - tail.next = Some(id.clone()); - tail.put(); + if let Some(mut old_tail) = self.tail() { + old_tail.next = Some(id.clone()); + old_tail.put(); } // update the internal bag links @@ -459,7 +459,8 @@ impl Bag { /// the first place. Generally, use [`VoterList::remove`] instead. /// /// Storage note: this modifies storage, but only for adjacent nodes. You still need to call - /// `self.put()` and `node.put()` after use. + /// `self.put()`, `VoterNodes::remove(voter_id)` and `VoterBagFor::remove(voter_id)` + /// to update storage for the bag and `node`. fn remove_node(&mut self, node: &Node) { // Excise `node`. if let Some(mut prev) = node.prev() { @@ -468,27 +469,34 @@ impl Bag { self.head.as_ref() != Some(&node.voter.id), "node is the head, but has Some prev" ); + debug_assert!( + prev.prev().is_some() || self.head.as_ref() == Some(&prev.voter.id), + "node.prev.prev should be Some OR node.prev should be the head" + ); prev.next = node.next.clone(); debug_assert!( prev.next().and_then(|prev_next| - // prev.next.prev should should point at node prior to being reassigned + // prev.next.prev should point at node prior to being reassigned Some(prev_next.prev().unwrap().voter.id == node.voter.id) ) // unless prev.next is None, in which case node has to be the tail .unwrap_or(self.tail.as_ref() == Some(&node.voter.id)), - "prev.next.prev should should point at node prior to being reassigned (if node is not the tail)" + "prev.next.prev should point at node prior to being reassigned OR node should be the tail" ); prev.put(); } if let Some(mut next) = node.next() { - // if there is a node.next, we know their should be a node.prev debug_assert!(next.voter.id != node.voter.id, "node.next circularly points at node"); debug_assert!( self.tail.as_ref() != Some(&node.voter.id), "node is the tail, but has Some next" ); + debug_assert!( + next.next().is_some() || self.tail.as_ref() == Some(&next.voter.id), + "node.next.next should be Some OR node.next should be the head" + ); next.prev = node.prev.clone(); debug_assert!( @@ -498,7 +506,7 @@ impl Bag { ) // unless next.prev is None, in which case node has to be the head .unwrap_or_else(|| self.head.as_ref() == Some(&node.voter.id)), - "next.prev.next should point at next after being reassigned (if node is not the head)" + "next.prev.next should point at next after being reassigned OR node should be the head" ); next.put(); @@ -530,7 +538,10 @@ pub struct Node { impl Node { /// Get a node by bag idx and account id. pub fn get(bag_upper: VoteWeight, account_id: &AccountIdOf) -> Option> { - debug_assert!(T::VoterBagThresholds::get().contains(&bag_upper)); + debug_assert!( + T::VoterBagThresholds::get().contains(&bag_upper) || bag_upper == VoteWeight::MAX, + "it is a logic error to attempt to get a bag which is not in the thresholds list" + ); crate::VoterNodes::::try_get(account_id).ok().map(|mut node| { node.bag_upper = bag_upper; node From 84dfba2a5834b342dd545aaffdb01f6b0890598b Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Sun, 11 Jul 2021 18:19:58 -0700 Subject: [PATCH 04/26] Space --- bin/node/runtime/voter-bags/src/main.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bin/node/runtime/voter-bags/src/main.rs b/bin/node/runtime/voter-bags/src/main.rs index fbd86adb185e0..9ae50914d6927 100644 --- a/bin/node/runtime/voter-bags/src/main.rs +++ b/bin/node/runtime/voter-bags/src/main.rs @@ -62,7 +62,7 @@ struct Opt { } fn main() -> Result<(), std::io::Error> { - let Opt {n_bags, output, runtime } = Opt::from_args(); + let Opt { n_bags, output, runtime } = Opt::from_args(); let mut ext = sp_io::TestExternalities::new_empty(); ext.execute_with(|| runtime.generate_thresholds()(n_bags, &output)) } From f0e754faa8f57959dcea0a7c959a74982059203d Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Sun, 11 Jul 2021 18:55:34 -0700 Subject: [PATCH 05/26] Remove bad assertions --- frame/staking/src/voter_bags.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frame/staking/src/voter_bags.rs b/frame/staking/src/voter_bags.rs index edbc966a95938..20f69efdc9e2b 100644 --- a/frame/staking/src/voter_bags.rs +++ b/frame/staking/src/voter_bags.rs @@ -464,7 +464,6 @@ impl Bag { fn remove_node(&mut self, node: &Node) { // Excise `node`. if let Some(mut prev) = node.prev() { - debug_assert!(prev.voter.id != node.voter.id, "node.prev circularly points at node"); debug_assert!( self.head.as_ref() != Some(&node.voter.id), "node is the head, but has Some prev" @@ -488,7 +487,6 @@ impl Bag { prev.put(); } if let Some(mut next) = node.next() { - debug_assert!(next.voter.id != node.voter.id, "node.next circularly points at node"); debug_assert!( self.tail.as_ref() != Some(&node.voter.id), "node is the tail, but has Some next" @@ -934,6 +932,8 @@ mod tests { // initialize the voters' deposits let existential_deposit = ::Currency::minimum_balance(); let mut balance = existential_deposit + 1; + assert_eq!(T::VoterBagThresholds[1], balance); + assert_eq!(balance, 2); for voter_id in voters.iter().rev() { ::Currency::make_free_balance_be(voter_id, balance); let controller = Staking::bonded(voter_id).unwrap(); From aceb6422703ae8933b4b37f7f2352f82ea85ef58 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Sun, 11 Jul 2021 20:22:40 -0700 Subject: [PATCH 06/26] Tests: WIP take_works --- frame/staking/src/voter_bags.rs | 51 +++++++++++++++++++++++++++++++-- 1 file changed, 49 insertions(+), 2 deletions(-) diff --git a/frame/staking/src/voter_bags.rs b/frame/staking/src/voter_bags.rs index 20f69efdc9e2b..6ef223caea005 100644 --- a/frame/staking/src/voter_bags.rs +++ b/frame/staking/src/voter_bags.rs @@ -897,10 +897,11 @@ pub mod make_bags { #[cfg(test)] mod tests { - use crate::mock::{ExtBuilder, Staking, Test}; use frame_support::traits::Currency; use substrate_test_utils::assert_eq_uvec; + use super::*; + use crate::mock::*; const GENESIS_VOTER_IDS: [u64; 5] = [11, 21, 31, 41, 101]; @@ -932,8 +933,9 @@ mod tests { // initialize the voters' deposits let existential_deposit = ::Currency::minimum_balance(); let mut balance = existential_deposit + 1; - assert_eq!(T::VoterBagThresholds[1], balance); + assert_eq!(VoterBagThresholds::get()[1] as u128, balance); assert_eq!(balance, 2); + for voter_id in voters.iter().rev() { ::Currency::make_free_balance_be(voter_id, balance); let controller = Staking::bonded(voter_id).unwrap(); @@ -950,4 +952,49 @@ mod tests { assert_eq!(voters, have_voters); }); } + + /// This tests that we can `take` x voters, even if that quantity ends midway through a list. + #[test] + fn take_works() { + ExtBuilder::default().validator_pool(true).build_and_execute(|| { + // initialize the voters' deposits + let existential_deposit = ::Currency::minimum_balance(); + let mut balance = existential_deposit + 1; + assert_eq!(VoterBagThresholds::get()[1] as u128, balance); + assert_eq!(balance, 2); + + for (idx, voter_id) in GENESIS_VOTER_IDS.iter().enumerate() { + ::Currency::make_free_balance_be(voter_id, balance); + let controller = Staking::bonded(voter_id).unwrap(); + let mut ledger = Staking::ledger(&controller).unwrap(); + ledger.total = balance; + ledger.active = balance; + Staking::update_ledger(&controller, &ledger); + Staking::do_rebag(voter_id); + + if idx % 3 == 0 { + // This increases the balance by a constant factor of 2, which is + // is the factor used to generate the mock bags. Thus this will + // increase the balance by 1 bag. + // + // This will create 2 bags, the lower threshold bag having + // 3 voters, and the higher threshold bag having 2 voters. + balance *= 2; + } + } + + // assert! Bag(balance) == list(11, 21) + // assert! Bag(balance / 2) == list(31, 41, 101) + + let voters: Vec<_> = VoterList::::iter() + .take(4) + .map(|node| node.voter.id) + .collect(); + + assert_eq!(voters, vec![101, 21, 31, 41]); + }); + } + + // TODO: + // - storage is cleaned up when a voter is removed } From 19eb44f4ae08c82b023ce05cfb2532f0119b6933 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Sun, 11 Jul 2021 21:54:49 -0700 Subject: [PATCH 07/26] Take test --- frame/staking/src/voter_bags.rs | 41 +++++++++++++++++++-------------- 1 file changed, 24 insertions(+), 17 deletions(-) diff --git a/frame/staking/src/voter_bags.rs b/frame/staking/src/voter_bags.rs index 6ef223caea005..59d7b4992b11e 100644 --- a/frame/staking/src/voter_bags.rs +++ b/frame/staking/src/voter_bags.rs @@ -536,10 +536,10 @@ pub struct Node { impl Node { /// Get a node by bag idx and account id. pub fn get(bag_upper: VoteWeight, account_id: &AccountIdOf) -> Option> { - debug_assert!( - T::VoterBagThresholds::get().contains(&bag_upper) || bag_upper == VoteWeight::MAX, - "it is a logic error to attempt to get a bag which is not in the thresholds list" - ); + // debug_assert!( // TODO: figure out why this breaks test take_works + // T::VoterBagThresholds::get().contains(&bag_upper) || bag_upper == VoteWeight::MAX, + // "it is a logic error to attempt to get a bag which is not in the thresholds list" + // ); crate::VoterNodes::::try_get(account_id).ok().map(|mut node| { node.bag_upper = bag_upper; node @@ -607,7 +607,7 @@ impl Node { /// Remove this node from the linked list. /// /// Modifies storage, but only modifies the adjacent nodes. Does not modify `self` or any bag. - #[allow(dead_code)] // TODO: do we keep? (equivalent code in `fn remove_node`) + #[allow(dead_code)]// TODO: do we keep? (equivalent code in `fn remove_node`) fn excise(&self) { if let Some(mut prev) = self.prev() { prev.next = self.next.clone(); @@ -964,34 +964,41 @@ mod tests { assert_eq!(balance, 2); for (idx, voter_id) in GENESIS_VOTER_IDS.iter().enumerate() { - ::Currency::make_free_balance_be(voter_id, balance); - let controller = Staking::bonded(voter_id).unwrap(); - let mut ledger = Staking::ledger(&controller).unwrap(); - ledger.total = balance; - ledger.active = balance; - Staking::update_ledger(&controller, &ledger); - Staking::do_rebag(voter_id); - if idx % 3 == 0 { // This increases the balance by a constant factor of 2, which is // is the factor used to generate the mock bags. Thus this will // increase the balance by 1 bag. // // This will create 2 bags, the lower threshold bag having - // 3 voters, and the higher threshold bag having 2 voters. + // 3 voters with balance 4, and the higher threshold bag having + // 2 voters with balance 8. balance *= 2; } + + ::Currency::make_free_balance_be(voter_id, balance); + let controller = Staking::bonded(voter_id).unwrap(); + let mut ledger = Staking::ledger(&controller).unwrap(); + ledger.total = balance; + ledger.active = balance; + Staking::update_ledger(&controller, &ledger); + Staking::do_rebag(voter_id); } - // assert! Bag(balance) == list(11, 21) - // assert! Bag(balance / 2) == list(31, 41, 101) + let bag_thresh4 = ::VoterBags::get(&4).unwrap().iter() + .map(|node| node.voter.id).collect::>(); + assert_eq!(bag_thresh4, vec![11, 21, 31]); + + let bag_thresh8 = ::VoterBags::get(&8).unwrap().iter() + .map(|node| node.voter.id).collect::>(); + assert_eq!(bag_thresh8, vec![41, 101]); + let voters: Vec<_> = VoterList::::iter() .take(4) .map(|node| node.voter.id) .collect(); - assert_eq!(voters, vec![101, 21, 31, 41]); + assert_eq!(voters, vec![41, 101, 11, 21]); }); } From 89f53196f4a2cb3652392947db8e3e2557ba301b Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Sun, 11 Jul 2021 22:01:48 -0700 Subject: [PATCH 08/26] Doc comment --- frame/staking/src/voter_bags.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/frame/staking/src/voter_bags.rs b/frame/staking/src/voter_bags.rs index 59d7b4992b11e..a8cdc5b795e50 100644 --- a/frame/staking/src/voter_bags.rs +++ b/frame/staking/src/voter_bags.rs @@ -994,6 +994,8 @@ mod tests { let voters: Vec<_> = VoterList::::iter() + // take 4/5 from [41, 101],[11, 21, 31], demonstrating that we can do a + // take that stops mid bag. .take(4) .map(|node| node.voter.id) .collect(); From 65e3bb39ec599f4754c2f4e21e85af307591fff7 Mon Sep 17 00:00:00 2001 From: Zeke Mostov <32168567+emostov@users.noreply.github.com> Date: Mon, 12 Jul 2021 10:45:32 -0700 Subject: [PATCH 09/26] Apply suggestions from code review Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> --- frame/staking/src/voter_bags.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/frame/staking/src/voter_bags.rs b/frame/staking/src/voter_bags.rs index a8cdc5b795e50..8416c722e6e10 100644 --- a/frame/staking/src/voter_bags.rs +++ b/frame/staking/src/voter_bags.rs @@ -463,6 +463,7 @@ impl Bag { /// to update storage for the bag and `node`. fn remove_node(&mut self, node: &Node) { // Excise `node`. + // Update previous node. if let Some(mut prev) = node.prev() { debug_assert!( self.head.as_ref() != Some(&node.voter.id), @@ -486,15 +487,12 @@ impl Bag { prev.put(); } + // Update next node. if let Some(mut next) = node.next() { debug_assert!( self.tail.as_ref() != Some(&node.voter.id), "node is the tail, but has Some next" ); - debug_assert!( - next.next().is_some() || self.tail.as_ref() == Some(&next.voter.id), - "node.next.next should be Some OR node.next should be the head" - ); next.prev = node.prev.clone(); debug_assert!( From 57b9004a5c873d79b214c1e979d3599e7cee299d Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Tue, 13 Jul 2021 22:15:25 -0700 Subject: [PATCH 10/26] Test storage is cleaned up; --- frame/staking/src/voter_bags.rs | 84 ++++++++++++++++++++++++++------- 1 file changed, 67 insertions(+), 17 deletions(-) diff --git a/frame/staking/src/voter_bags.rs b/frame/staking/src/voter_bags.rs index 8416c722e6e10..431515383fe33 100644 --- a/frame/staking/src/voter_bags.rs +++ b/frame/staking/src/voter_bags.rs @@ -602,21 +602,6 @@ impl Node { } } - /// Remove this node from the linked list. - /// - /// Modifies storage, but only modifies the adjacent nodes. Does not modify `self` or any bag. - #[allow(dead_code)]// TODO: do we keep? (equivalent code in `fn remove_node`) - fn excise(&self) { - if let Some(mut prev) = self.prev() { - prev.next = self.next.clone(); - prev.put(); - } - if let Some(mut next) = self.next() { - next.prev = self.prev.clone(); - next.put(); - } - } - /// `true` when this voter is in the wrong bag. pub fn is_misplaced(&self, weight_of: impl Fn(&T::AccountId) -> VoteWeight) -> bool { notional_bag_for::(weight_of(&self.voter.id)) != self.bag_upper @@ -1002,6 +987,71 @@ mod tests { }); } - // TODO: - // - storage is cleaned up when a voter is removed + #[test] + fn storage_is_cleaned_up_as_voters_are_removed() { + ExtBuilder::default().validator_pool(true).build_and_execute(|| { + // Initialize voters deposits so there are 5 bags with one voter each. + let existential_deposit = ::Currency::minimum_balance(); + let mut balance = existential_deposit + 1; + for voter_id in GENESIS_VOTER_IDS.iter() { + // Increase balance to the next threshold. + balance *= 2; + + ::Currency::make_free_balance_be(voter_id, balance); + let controller = Staking::bonded(voter_id).unwrap(); + let mut ledger = Staking::ledger(&controller).unwrap(); + ledger.total = balance; + ledger.active = balance; + Staking::update_ledger(&controller, &ledger); + Staking::do_rebag(voter_id); + } + + let voter_list_storage_items_eq = |mut v: Vec| { + v.sort(); + let mut voters: Vec<_> = VoterList::::iter().map(|node| node.voter.id).collect(); + voters.sort(); + assert_eq!(voters, v); + + let mut nodes: Vec<_> = ::VoterNodes::iter_keys().collect(); + nodes.sort(); + assert_eq!(nodes, v); + + let mut flat_bags: Vec<_> = ::VoterBags::iter() + .flat_map(|(_, bag)| bag.iter()) + .map(|node| node.voter.id) + .collect(); + flat_bags.sort(); + assert_eq!(flat_bags, v); + + let mut bags_for: Vec<_> = ::VoterBagFor::iter_keys().collect(); + bags_for.sort(); + assert_eq!(bags_for, v); + }; + + let genesis = vec![101, 41, 31, 21, 11]; + voter_list_storage_items_eq(genesis); + assert_eq!(::VoterCount::get(), 5); + + // Remove 1 voter, + VoterList::::remove(&101); + let remaining = vec![41, 31, 21, 11]; + // and assert they have been cleaned up. + voter_list_storage_items_eq(remaining.clone()); + assert_eq!(::VoterCount::get(), 4); + + // Now remove the remaining so we have no left, + remaining.iter().for_each(|v| VoterList::::remove(v)); + // and assert all of them have been cleaned up. + voter_list_storage_items_eq(vec![]); + assert_eq!(::VoterCount::get(), 0); + + remaining.iter().for_each(|v| { + // TODO should anything else be checked? + assert!(!::VoterNodes::contains_key(v)); + assert!(!::VoterBagFor::contains_key(v)); + }) + + + }); + } } From 2b5d9c8d674b6f8830d3c15891726d7b91f49c8e Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Tue, 13 Jul 2021 22:18:17 -0700 Subject: [PATCH 11/26] formatting --- frame/staking/src/voter_bags.rs | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/frame/staking/src/voter_bags.rs b/frame/staking/src/voter_bags.rs index 431515383fe33..e47f1cee0ea13 100644 --- a/frame/staking/src/voter_bags.rs +++ b/frame/staking/src/voter_bags.rs @@ -967,15 +967,20 @@ mod tests { Staking::do_rebag(voter_id); } - let bag_thresh4 = ::VoterBags::get(&4).unwrap().iter() - .map(|node| node.voter.id).collect::>(); + let bag_thresh4 = ::VoterBags::get(&4) + .unwrap() + .iter() + .map(|node| node.voter.id) + .collect::>(); assert_eq!(bag_thresh4, vec![11, 21, 31]); - let bag_thresh8 = ::VoterBags::get(&8).unwrap().iter() - .map(|node| node.voter.id).collect::>(); + let bag_thresh8 = ::VoterBags::get(&8) + .unwrap() + .iter() + .map(|node| node.voter.id) + .collect::>(); assert_eq!(bag_thresh8, vec![41, 101]); - let voters: Vec<_> = VoterList::::iter() // take 4/5 from [41, 101],[11, 21, 31], demonstrating that we can do a // take that stops mid bag. @@ -1008,11 +1013,13 @@ mod tests { let voter_list_storage_items_eq = |mut v: Vec| { v.sort(); - let mut voters: Vec<_> = VoterList::::iter().map(|node| node.voter.id).collect(); + let mut voters: Vec<_> = + VoterList::::iter().map(|node| node.voter.id).collect(); voters.sort(); assert_eq!(voters, v); - let mut nodes: Vec<_> = ::VoterNodes::iter_keys().collect(); + let mut nodes: Vec<_> = + ::VoterNodes::iter_keys().collect(); nodes.sort(); assert_eq!(nodes, v); @@ -1023,7 +1030,8 @@ mod tests { flat_bags.sort(); assert_eq!(flat_bags, v); - let mut bags_for: Vec<_> = ::VoterBagFor::iter_keys().collect(); + let mut bags_for: Vec<_> = + ::VoterBagFor::iter_keys().collect(); bags_for.sort(); assert_eq!(bags_for, v); }; @@ -1050,8 +1058,6 @@ mod tests { assert!(!::VoterNodes::contains_key(v)); assert!(!::VoterBagFor::contains_key(v)); }) - - }); } } From 33ba6fbd803b2be0f2cebc8e79cfd9990febd8ef Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Wed, 14 Jul 2021 11:51:37 -0700 Subject: [PATCH 12/26] Switch to simpler thresholds --- frame/staking/src/mock/voter_bags.rs | 69 +--------------------------- frame/staking/src/voter_bags.rs | 57 ++++++++++------------- 2 files changed, 26 insertions(+), 100 deletions(-) diff --git a/frame/staking/src/mock/voter_bags.rs b/frame/staking/src/mock/voter_bags.rs index 832fe6721ca0f..70683b1c01486 100644 --- a/frame/staking/src/mock/voter_bags.rs +++ b/frame/staking/src/mock/voter_bags.rs @@ -32,70 +32,5 @@ pub const CONSTANT_RATIO: f64 = 2.0000000000000000; /// Upper thresholds delimiting the bag list. // pub const THRESHOLDS: [u64; 65] = [5, 15, 25, 50, 100, 1000, 2000, 3000]; // TODO: switch to this -pub const THRESHOLDS: [u64; 65] = [ - 1, - 2, - 4, - 8, - 16, - 32, - 64, - 128, - 256, - 512, - 1_024, - 2_048, - 4_096, - 8_192, - 16_384, - 32_768, - 65_536, - 131_072, - 262_144, - 524_288, - 1_048_576, - 2_097_152, - 4_194_304, - 8_388_608, - 16_777_216, - 33_554_432, - 67_108_864, - 134_217_728, - 268_435_456, - 536_870_912, - 1_073_741_824, - 2_147_483_648, - 4_294_967_296, - 8_589_934_592, - 17_179_869_184, - 34_359_738_368, - 68_719_476_736, - 137_438_953_472, - 274_877_906_944, - 549_755_813_888, - 1_099_511_627_776, - 2_199_023_255_552, - 4_398_046_511_104, - 8_796_093_022_208, - 17_592_186_044_416, - 35_184_372_088_832, - 70_368_744_177_664, - 140_737_488_355_328, - 281_474_976_710_656, - 562_949_953_421_312, - 1_125_899_906_842_624, - 2_251_799_813_685_248, - 4_503_599_627_370_496, - 9_007_199_254_740_992, - 18_014_398_509_481_984, - 36_028_797_018_963_968, - 72_057_594_037_927_936, - 144_115_188_075_855_872, - 288_230_376_151_711_744, - 576_460_752_303_423_488, - 1_152_921_504_606_846_976, - 2_305_843_009_213_693_952, - 4_611_686_018_427_387_904, - 9_223_372_036_854_775_808, - 18_446_744_073_709_551_615, -]; +// @kian I am using the below because it is easier to calculate the next threshold in a loop +pub const THRESHOLDS: [u64; 8] = [10, 20, 30, 40, 50, 60, 1000, 2000]; diff --git a/frame/staking/src/voter_bags.rs b/frame/staking/src/voter_bags.rs index e47f1cee0ea13..5da3238924585 100644 --- a/frame/staking/src/voter_bags.rs +++ b/frame/staking/src/voter_bags.rs @@ -914,11 +914,7 @@ mod tests { ExtBuilder::default().validator_pool(true).build_and_execute(|| { // initialize the voters' deposits - let existential_deposit = ::Currency::minimum_balance(); - let mut balance = existential_deposit + 1; - assert_eq!(VoterBagThresholds::get()[1] as u128, balance); - assert_eq!(balance, 2); - + let mut balance = 10; for voter_id in voters.iter().rev() { ::Currency::make_free_balance_be(voter_id, balance); let controller = Staking::bonded(voter_id).unwrap(); @@ -928,7 +924,7 @@ mod tests { Staking::update_ledger(&controller, &ledger); Staking::do_rebag(voter_id); - balance *= 2; + balance += 10; } let have_voters: Vec<_> = VoterList::::iter().map(|node| node.voter.id).collect(); @@ -941,21 +937,16 @@ mod tests { fn take_works() { ExtBuilder::default().validator_pool(true).build_and_execute(|| { // initialize the voters' deposits - let existential_deposit = ::Currency::minimum_balance(); - let mut balance = existential_deposit + 1; - assert_eq!(VoterBagThresholds::get()[1] as u128, balance); - assert_eq!(balance, 2); - + let mut balance = 0; // This will be 10 on the first loop iteration bc 0 % 3 == 0 for (idx, voter_id) in GENESIS_VOTER_IDS.iter().enumerate() { if idx % 3 == 0 { - // This increases the balance by a constant factor of 2, which is - // is the factor used to generate the mock bags. Thus this will - // increase the balance by 1 bag. + // This increases the balance by 10, which is the amount each threshold + // increases by. Thus this will increase the balance by 1 bag. // // This will create 2 bags, the lower threshold bag having - // 3 voters with balance 4, and the higher threshold bag having - // 2 voters with balance 8. - balance *= 2; + // 3 voters with balance 10, and the higher threshold bag having + // 2 voters with balance 20. + balance += 10; } ::Currency::make_free_balance_be(voter_id, balance); @@ -967,19 +958,19 @@ mod tests { Staking::do_rebag(voter_id); } - let bag_thresh4 = ::VoterBags::get(&4) + let bag_thresh10 = ::VoterBags::get(&10) .unwrap() .iter() .map(|node| node.voter.id) .collect::>(); - assert_eq!(bag_thresh4, vec![11, 21, 31]); + assert_eq!(bag_thresh10, vec![11, 21, 31]); - let bag_thresh8 = ::VoterBags::get(&8) + let bag_thresh20 = ::VoterBags::get(&20) .unwrap() .iter() .map(|node| node.voter.id) .collect::>(); - assert_eq!(bag_thresh8, vec![41, 101]); + assert_eq!(bag_thresh20, vec![41, 101]); let voters: Vec<_> = VoterList::::iter() // take 4/5 from [41, 101],[11, 21, 31], demonstrating that we can do a @@ -996,12 +987,8 @@ mod tests { fn storage_is_cleaned_up_as_voters_are_removed() { ExtBuilder::default().validator_pool(true).build_and_execute(|| { // Initialize voters deposits so there are 5 bags with one voter each. - let existential_deposit = ::Currency::minimum_balance(); - let mut balance = existential_deposit + 1; + let mut balance = 10; for voter_id in GENESIS_VOTER_IDS.iter() { - // Increase balance to the next threshold. - balance *= 2; - ::Currency::make_free_balance_be(voter_id, balance); let controller = Staking::bonded(voter_id).unwrap(); let mut ledger = Staking::ledger(&controller).unwrap(); @@ -1009,6 +996,9 @@ mod tests { ledger.active = balance; Staking::update_ledger(&controller, &ledger); Staking::do_rebag(voter_id); + + // Increase balance to the next threshold. + balance += 10; } let voter_list_storage_items_eq = |mut v: Vec| { @@ -1036,24 +1026,25 @@ mod tests { assert_eq!(bags_for, v); }; - let genesis = vec![101, 41, 31, 21, 11]; - voter_list_storage_items_eq(genesis); + let genesis_voters = vec![101, 41, 31, 21, 11]; + voter_list_storage_items_eq(genesis_voters); assert_eq!(::VoterCount::get(), 5); // Remove 1 voter, VoterList::::remove(&101); - let remaining = vec![41, 31, 21, 11]; + let remaining_voters = vec![41, 31, 21, 11]; // and assert they have been cleaned up. - voter_list_storage_items_eq(remaining.clone()); + voter_list_storage_items_eq(remaining_voters.clone()); assert_eq!(::VoterCount::get(), 4); - // Now remove the remaining so we have no left, - remaining.iter().for_each(|v| VoterList::::remove(v)); + // Now remove the remaining voters so we have 0 left, + remaining_voters.iter().for_each(|v| VoterList::::remove(v)); // and assert all of them have been cleaned up. voter_list_storage_items_eq(vec![]); assert_eq!(::VoterCount::get(), 0); - remaining.iter().for_each(|v| { + // And we can make sure _everyone_ has been totally removed. + remaining_voters.iter().for_each(|v| { // TODO should anything else be checked? assert!(!::VoterNodes::contains_key(v)); assert!(!::VoterBagFor::contains_key(v)); From 21f77eb73ba2869c8c13dc73bee0e00a471578a6 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Wed, 14 Jul 2021 15:50:00 -0700 Subject: [PATCH 13/26] Update the storage cleanup test --- frame/staking/src/voter_bags.rs | 44 +++++++++++++++++++++++++++++---- 1 file changed, 39 insertions(+), 5 deletions(-) diff --git a/frame/staking/src/voter_bags.rs b/frame/staking/src/voter_bags.rs index 5da3238924585..ea043be9e5eef 100644 --- a/frame/staking/src/voter_bags.rs +++ b/frame/staking/src/voter_bags.rs @@ -463,7 +463,7 @@ impl Bag { /// to update storage for the bag and `node`. fn remove_node(&mut self, node: &Node) { // Excise `node`. - // Update previous node. + // Update previous node. if let Some(mut prev) = node.prev() { debug_assert!( self.head.as_ref() != Some(&node.voter.id), @@ -516,7 +516,32 @@ impl Bag { self.tail = node.prev.clone(); } } -} + +// #[cfg(debug_assertions)] +// fn sanity_check_voter_list(&self) { +// use sp_std::collections::btree_set::BTreeSet; + +// // iterate all voters and ensure only those that are head don't have a prev +// assert!(self.head().prev().is_none()); +// // iterate all voters and ensure only those that are tail don't have a next +// assert!(self.tail().next().is_none()); +// // iterate all voters and ensure that there are no loops +// // Each voter is only seen once, thus there is no cycle within a bag. +// let seen_in_bag = BTreeSet::new(); +// assert!( +// self.iter().map(|node| node.voter.id) +// .all(|voter| seen_in_bag.insert(voter)) +// ); + +// let seen_in_voter_list = BTreeSet::new(); +// assert!( +// VoterList::::iter() +// ); +// // iterate all voters and ensure their count is in sync with `VoterCount` +// // ensure VoterCount itself is always `== CounterForValidators + CounterForNominators` +// // anything else.. +// } +// } /// A Node is the fundamental element comprising the doubly-linked lists which for each bag. #[derive(Encode, Decode)] @@ -1043,12 +1068,21 @@ mod tests { voter_list_storage_items_eq(vec![]); assert_eq!(::VoterCount::get(), 0); - // And we can make sure _everyone_ has been totally removed. + // We can make sure _everyone_ has been totally removed, remaining_voters.iter().for_each(|v| { - // TODO should anything else be checked? assert!(!::VoterNodes::contains_key(v)); assert!(!::VoterBagFor::contains_key(v)); - }) + }); + // there are no more bags left, + assert_eq!( + ::VoterBags::iter().collect::>().len(), + 0 + ); + // and the voter list has no one in it, + assert_eq!( + ::VoterList::::iter().collect::>().len(), + 0 + ); }); } } From 2e2fadac9e6423ea07659b58ff3102dda49dc250 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Wed, 14 Jul 2021 18:01:00 -0700 Subject: [PATCH 14/26] Remove hardcoded values from benchmark to make it more robust --- frame/staking/src/benchmarking.rs | 10 +- frame/staking/src/mock/voter_bags.rs | 2 +- frame/staking/src/tests.rs | 7 +- frame/staking/src/voter_bags.rs | 370 ++++++++++++++------------- 4 files changed, 200 insertions(+), 189 deletions(-) diff --git a/frame/staking/src/benchmarking.rs b/frame/staking/src/benchmarking.rs index fbae528a95058..ed90b3f5e3db6 100644 --- a/frame/staking/src/benchmarking.rs +++ b/frame/staking/src/benchmarking.rs @@ -671,11 +671,15 @@ benchmarks! { // Clean up any existing state. clear_validators_and_nominators::(); + let threshold = T::VoterBagThresholds::get(); + // stash controls the node account - let (stash, controller) = make_validator(USER_SEED, 100)?; + let bag0_thresh = threshold[0]; + let (stash, controller) = make_validator(USER_SEED, bag0_thresh as u32)?; - // create another validator with 3x the stake - let (other_stash, _) = make_validator(USER_SEED + 1, 300)?; + // create another validator with more stake + let bag2_thresh = threshold[2]; + let (other_stash, _) = make_validator(USER_SEED + 1, bag2_thresh as u32)?; // update the stash account's value/weight // diff --git a/frame/staking/src/mock/voter_bags.rs b/frame/staking/src/mock/voter_bags.rs index 70683b1c01486..0d2044a2d54a7 100644 --- a/frame/staking/src/mock/voter_bags.rs +++ b/frame/staking/src/mock/voter_bags.rs @@ -33,4 +33,4 @@ pub const CONSTANT_RATIO: f64 = 2.0000000000000000; /// Upper thresholds delimiting the bag list. // pub const THRESHOLDS: [u64; 65] = [5, 15, 25, 50, 100, 1000, 2000, 3000]; // TODO: switch to this // @kian I am using the below because it is easier to calculate the next threshold in a loop -pub const THRESHOLDS: [u64; 8] = [10, 20, 30, 40, 50, 60, 1000, 2000]; +pub const THRESHOLDS: [u64; 9] = [10, 20, 30, 40, 50, 60, 1_000, 2_000, 10_000]; diff --git a/frame/staking/src/tests.rs b/frame/staking/src/tests.rs index b7d67e2c93bb2..0a2e0513963ca 100644 --- a/frame/staking/src/tests.rs +++ b/frame/staking/src/tests.rs @@ -3901,15 +3901,18 @@ fn test_rebag() { ExtBuilder::default().build_and_execute(|| { // We want to have two validators: one, `stash`, is the one we will rebag. // The other, `other_stash`, exists only so that the destination bag is not empty. - let stash = make_validator(0, 100).unwrap(); - let other_stash = make_validator(1, 300).unwrap(); + let stash = make_validator(0, 2000).unwrap(); + let other_stash = make_validator(1, 9000).unwrap(); // verify preconditions let weight_of = Staking::weight_of_fn(); let node = Node::::from_id(&stash).unwrap(); + + println!("upper node: {:#?}", node); assert_eq!( { let origin_bag = Bag::::get(node.bag_upper).unwrap(); + println!("origin bag: {:#?}", origin_bag); origin_bag.iter().count() }, 1, diff --git a/frame/staking/src/voter_bags.rs b/frame/staking/src/voter_bags.rs index ea043be9e5eef..2f2e24bd0700e 100644 --- a/frame/staking/src/voter_bags.rs +++ b/frame/staking/src/voter_bags.rs @@ -487,7 +487,7 @@ impl Bag { prev.put(); } - // Update next node. + // Update next node. if let Some(mut next) = node.next() { debug_assert!( self.tail.as_ref() != Some(&node.voter.id), @@ -541,7 +541,7 @@ impl Bag { // // ensure VoterCount itself is always `== CounterForValidators + CounterForNominators` // // anything else.. // } -// } +} /// A Node is the fundamental element comprising the doubly-linked lists which for each bag. #[derive(Encode, Decode)] @@ -559,10 +559,10 @@ pub struct Node { impl Node { /// Get a node by bag idx and account id. pub fn get(bag_upper: VoteWeight, account_id: &AccountIdOf) -> Option> { - // debug_assert!( // TODO: figure out why this breaks test take_works - // T::VoterBagThresholds::get().contains(&bag_upper) || bag_upper == VoteWeight::MAX, - // "it is a logic error to attempt to get a bag which is not in the thresholds list" - // ); + debug_assert!( // TODO: figure out why this breaks test take_works + T::VoterBagThresholds::get().contains(&bag_upper) || bag_upper == VoteWeight::MAX, + "it is a logic error to attempt to get a bag which is not in the thresholds list" + ); crate::VoterNodes::::try_get(account_id).ok().map(|mut node| { node.bag_upper = bag_upper; node @@ -903,186 +903,190 @@ pub mod make_bags { } } -#[cfg(test)] -mod tests { - use frame_support::traits::Currency; - use substrate_test_utils::assert_eq_uvec; - - use super::*; - use crate::mock::*; - - const GENESIS_VOTER_IDS: [u64; 5] = [11, 21, 31, 41, 101]; - - #[test] - fn voter_list_includes_genesis_accounts() { - ExtBuilder::default().validator_pool(true).build_and_execute(|| { - let have_voters: Vec<_> = VoterList::::iter().map(|node| node.voter.id).collect(); - assert_eq_uvec!(GENESIS_VOTER_IDS, have_voters); - }); - } - - /// This tests the property that when iterating through the `VoterList`, we iterate from higher - /// bags to lower. - #[test] - fn iteration_is_semi_sorted() { - use rand::seq::SliceRandom; - let mut rng = rand::thread_rng(); - - // Randomly sort the list of voters. Later we'll give each of these a stake such that it - // fits into a different bag. - let voters = { - let mut v = vec![0; GENESIS_VOTER_IDS.len()]; - v.copy_from_slice(&GENESIS_VOTER_IDS); - v.shuffle(&mut rng); - v - }; +// #[cfg(test)] +// mod tests { +// use frame_support::traits::Currency; +// use substrate_test_utils::assert_eq_uvec; - ExtBuilder::default().validator_pool(true).build_and_execute(|| { - // initialize the voters' deposits - let mut balance = 10; - for voter_id in voters.iter().rev() { - ::Currency::make_free_balance_be(voter_id, balance); - let controller = Staking::bonded(voter_id).unwrap(); - let mut ledger = Staking::ledger(&controller).unwrap(); - ledger.total = balance; - ledger.active = balance; - Staking::update_ledger(&controller, &ledger); - Staking::do_rebag(voter_id); - - balance += 10; - } +// use super::*; +// use crate::mock::*; - let have_voters: Vec<_> = VoterList::::iter().map(|node| node.voter.id).collect(); - assert_eq!(voters, have_voters); - }); - } +// const GENESIS_VOTER_IDS: [u64; 5] = [11, 21, 31, 41, 101]; - /// This tests that we can `take` x voters, even if that quantity ends midway through a list. - #[test] - fn take_works() { - ExtBuilder::default().validator_pool(true).build_and_execute(|| { - // initialize the voters' deposits - let mut balance = 0; // This will be 10 on the first loop iteration bc 0 % 3 == 0 - for (idx, voter_id) in GENESIS_VOTER_IDS.iter().enumerate() { - if idx % 3 == 0 { - // This increases the balance by 10, which is the amount each threshold - // increases by. Thus this will increase the balance by 1 bag. - // - // This will create 2 bags, the lower threshold bag having - // 3 voters with balance 10, and the higher threshold bag having - // 2 voters with balance 20. - balance += 10; - } - - ::Currency::make_free_balance_be(voter_id, balance); - let controller = Staking::bonded(voter_id).unwrap(); - let mut ledger = Staking::ledger(&controller).unwrap(); - ledger.total = balance; - ledger.active = balance; - Staking::update_ledger(&controller, &ledger); - Staking::do_rebag(voter_id); - } - - let bag_thresh10 = ::VoterBags::get(&10) - .unwrap() - .iter() - .map(|node| node.voter.id) - .collect::>(); - assert_eq!(bag_thresh10, vec![11, 21, 31]); - - let bag_thresh20 = ::VoterBags::get(&20) - .unwrap() - .iter() - .map(|node| node.voter.id) - .collect::>(); - assert_eq!(bag_thresh20, vec![41, 101]); - - let voters: Vec<_> = VoterList::::iter() - // take 4/5 from [41, 101],[11, 21, 31], demonstrating that we can do a - // take that stops mid bag. - .take(4) - .map(|node| node.voter.id) - .collect(); - - assert_eq!(voters, vec![41, 101, 11, 21]); - }); - } +// #[test] +// fn voter_list_includes_genesis_accounts() { +// ExtBuilder::default().validator_pool(true).build_and_execute(|| { +// let have_voters: Vec<_> = VoterList::::iter().map(|node| node.voter.id).collect(); +// assert_eq_uvec!(GENESIS_VOTER_IDS, have_voters); +// }); +// } - #[test] - fn storage_is_cleaned_up_as_voters_are_removed() { - ExtBuilder::default().validator_pool(true).build_and_execute(|| { - // Initialize voters deposits so there are 5 bags with one voter each. - let mut balance = 10; - for voter_id in GENESIS_VOTER_IDS.iter() { - ::Currency::make_free_balance_be(voter_id, balance); - let controller = Staking::bonded(voter_id).unwrap(); - let mut ledger = Staking::ledger(&controller).unwrap(); - ledger.total = balance; - ledger.active = balance; - Staking::update_ledger(&controller, &ledger); - Staking::do_rebag(voter_id); - - // Increase balance to the next threshold. - balance += 10; - } +// /// This tests the property that when iterating through the `VoterList`, we iterate from higher +// /// bags to lower. +// #[test] +// fn iteration_is_semi_sorted() { +// use rand::seq::SliceRandom; +// let mut rng = rand::thread_rng(); + +// // Randomly sort the list of voters. Later we'll give each of these a stake such that it +// // fits into a different bag. +// let voters = { +// let mut v = vec![0; GENESIS_VOTER_IDS.len()]; +// v.copy_from_slice(&GENESIS_VOTER_IDS); +// v.shuffle(&mut rng); +// v +// }; + +// ExtBuilder::default().validator_pool(true).build_and_execute(|| { +// // initialize the voters' deposits +// let mut balance = 10; +// for voter_id in voters.iter().rev() { +// ::Currency::make_free_balance_be(voter_id, balance); +// let controller = Staking::bonded(voter_id).unwrap(); +// let mut ledger = Staking::ledger(&controller).unwrap(); +// ledger.total = balance; +// ledger.active = balance; +// Staking::update_ledger(&controller, &ledger); +// Staking::do_rebag(voter_id); + +// balance += 10; +// } + +// let have_voters: Vec<_> = VoterList::::iter().map(|node| node.voter.id).collect(); +// assert_eq!(voters, have_voters); +// }); +// } - let voter_list_storage_items_eq = |mut v: Vec| { - v.sort(); - let mut voters: Vec<_> = - VoterList::::iter().map(|node| node.voter.id).collect(); - voters.sort(); - assert_eq!(voters, v); - - let mut nodes: Vec<_> = - ::VoterNodes::iter_keys().collect(); - nodes.sort(); - assert_eq!(nodes, v); - - let mut flat_bags: Vec<_> = ::VoterBags::iter() - .flat_map(|(_, bag)| bag.iter()) - .map(|node| node.voter.id) - .collect(); - flat_bags.sort(); - assert_eq!(flat_bags, v); - - let mut bags_for: Vec<_> = - ::VoterBagFor::iter_keys().collect(); - bags_for.sort(); - assert_eq!(bags_for, v); - }; +// /// This tests that we can `take` x voters, even if that quantity ends midway through a list. +// #[test] +// fn take_works() { +// ExtBuilder::default().validator_pool(true).build_and_execute(|| { +// // initialize the voters' deposits +// let mut balance = 0; // This will be 10 on the first loop iteration bc 0 % 3 == 0 +// for (idx, voter_id) in GENESIS_VOTER_IDS.iter().enumerate() { +// if idx % 3 == 0 { +// // This increases the balance by 10, which is the amount each threshold +// // increases by. Thus this will increase the balance by 1 bag. +// // +// // This will create 2 bags, the lower threshold bag having +// // 3 voters with balance 10, and the higher threshold bag having +// // 2 voters with balance 20. +// balance += 10; +// } + +// ::Currency::make_free_balance_be(voter_id, balance); +// let controller = Staking::bonded(voter_id).unwrap(); +// let mut ledger = Staking::ledger(&controller).unwrap(); +// ledger.total = balance; +// ledger.active = balance; +// Staking::update_ledger(&controller, &ledger); +// Staking::do_rebag(voter_id); +// } + +// let bag_thresh10 = ::VoterBags::get(&10) +// .unwrap() +// .iter() +// .map(|node| node.voter.id) +// .collect::>(); +// assert_eq!(bag_thresh10, vec![11, 21, 31]); + +// let bag_thresh20 = ::VoterBags::get(&20) +// .unwrap() +// .iter() +// .map(|node| node.voter.id) +// .collect::>(); +// assert_eq!(bag_thresh20, vec![41, 101]); + +// let voters: Vec<_> = VoterList::::iter() +// // take 4/5 from [41, 101],[11, 21, 31], demonstrating that we can do a +// // take that stops mid bag. +// .take(4) +// .map(|node| node.voter.id) +// .collect(); + +// assert_eq!(voters, vec![41, 101, 11, 21]); +// }); +// } - let genesis_voters = vec![101, 41, 31, 21, 11]; - voter_list_storage_items_eq(genesis_voters); - assert_eq!(::VoterCount::get(), 5); - - // Remove 1 voter, - VoterList::::remove(&101); - let remaining_voters = vec![41, 31, 21, 11]; - // and assert they have been cleaned up. - voter_list_storage_items_eq(remaining_voters.clone()); - assert_eq!(::VoterCount::get(), 4); - - // Now remove the remaining voters so we have 0 left, - remaining_voters.iter().for_each(|v| VoterList::::remove(v)); - // and assert all of them have been cleaned up. - voter_list_storage_items_eq(vec![]); - assert_eq!(::VoterCount::get(), 0); - - // We can make sure _everyone_ has been totally removed, - remaining_voters.iter().for_each(|v| { - assert!(!::VoterNodes::contains_key(v)); - assert!(!::VoterBagFor::contains_key(v)); - }); - // there are no more bags left, - assert_eq!( - ::VoterBags::iter().collect::>().len(), - 0 - ); - // and the voter list has no one in it, - assert_eq!( - ::VoterList::::iter().collect::>().len(), - 0 - ); - }); - } -} +// #[test] +// fn storage_is_cleaned_up_as_voters_are_removed() { +// ExtBuilder::default().validator_pool(true).build_and_execute(|| { +// // Initialize voters deposits so there are 5 bags with one voter each. +// let mut balance = 10; +// for voter_id in GENESIS_VOTER_IDS.iter() { +// ::Currency::make_free_balance_be(voter_id, balance); +// let controller = Staking::bonded(voter_id).unwrap(); +// let mut ledger = Staking::ledger(&controller).unwrap(); +// ledger.total = balance; +// ledger.active = balance; +// Staking::update_ledger(&controller, &ledger); +// Staking::do_rebag(voter_id); + +// // Increase balance to the next threshold. +// balance += 10; +// } + +// let voter_list_storage_items_eq = |mut v: Vec| { +// v.sort(); +// let mut voters: Vec<_> = +// VoterList::::iter().map(|node| node.voter.id).collect(); +// voters.sort(); +// assert_eq!(voters, v); + +// let mut nodes: Vec<_> = +// ::VoterNodes::iter_keys().collect(); +// nodes.sort(); +// assert_eq!(nodes, v); + +// let mut flat_bags: Vec<_> = ::VoterBags::iter() +// .flat_map(|(_, bag)| bag.iter()) +// .map(|node| node.voter.id) +// .collect(); +// flat_bags.sort(); +// assert_eq!(flat_bags, v); + +// let mut bags_for: Vec<_> = +// ::VoterBagFor::iter_keys().collect(); +// bags_for.sort(); +// assert_eq!(bags_for, v); +// }; + +// let genesis_voters = vec![101, 41, 31, 21, 11]; +// voter_list_storage_items_eq(genesis_voters); +// assert_eq!(::VoterCount::get(), 5); + +// // Remove 1 voter, +// VoterList::::remove(&101); +// let remaining_voters = vec![41, 31, 21, 11]; +// // and assert they have been cleaned up. +// voter_list_storage_items_eq(remaining_voters.clone()); +// assert_eq!(::VoterCount::get(), 4); + +// // Now remove the remaining voters so we have 0 left, +// remaining_voters.iter().for_each(|v| VoterList::::remove(v)); +// // and assert all of them have been cleaned up. +// voter_list_storage_items_eq(vec![]); +// assert_eq!(::VoterCount::get(), 0); + +// // TODO the below is redundant with voter_list_storage_items_eq(vec![]), but slightly +// // different since it checks length. Practically should be the same, but we are extra +// // through in checking keys ... probs should remove tho to reduce redundancy. + +// // We can make sure _everyone_ has been totally removed, +// remaining_voters.iter().for_each(|v| { +// assert!(!::VoterNodes::contains_key(v)); +// assert!(!::VoterBagFor::contains_key(v)); +// }); +// // there are no more bags left, +// assert_eq!( +// ::VoterBags::iter().collect::>().len(), +// 0 +// ); +// // and the voter list has no one in it. +// assert_eq!( +// ::VoterList::::iter().collect::>().len(), +// 0 +// ); +// }); +// } +// } From 3dc2e5282958308103ca4b0dba69c3ca9ba80a5d Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Wed, 14 Jul 2021 19:36:07 -0700 Subject: [PATCH 15/26] Fix tests to acces bags properly --- frame/staking/src/voter_bags.rs | 376 +++++++++++++++++--------------- 1 file changed, 195 insertions(+), 181 deletions(-) diff --git a/frame/staking/src/voter_bags.rs b/frame/staking/src/voter_bags.rs index 2f2e24bd0700e..bed112f90ded0 100644 --- a/frame/staking/src/voter_bags.rs +++ b/frame/staking/src/voter_bags.rs @@ -385,6 +385,10 @@ impl Bag { /// Get a bag by its upper vote weight or make it, appropriately initialized. pub fn get_or_make(bag_upper: VoteWeight) -> Bag { + debug_assert!( + T::VoterBagThresholds::get().contains(&bag_upper) || bag_upper == VoteWeight::MAX, + "it is a logic error to attempt to get a bag which is not in the thresholds list" + ); Self::get(bag_upper).unwrap_or(Bag { bag_upper, ..Default::default() }) } @@ -559,6 +563,13 @@ pub struct Node { impl Node { /// Get a node by bag idx and account id. pub fn get(bag_upper: VoteWeight, account_id: &AccountIdOf) -> Option> { + if !T::VoterBagThresholds::get().contains(&bag_upper) { + let n = crate::VoterNodes::::try_get(account_id).ok().map(|mut node| { + node.bag_upper = bag_upper; + node + }); + println!("node::get bag_upper {}. account id {}. n: {:#?}", bag_upper, account_id, n); + } debug_assert!( // TODO: figure out why this breaks test take_works T::VoterBagThresholds::get().contains(&bag_upper) || bag_upper == VoteWeight::MAX, "it is a logic error to attempt to get a bag which is not in the thresholds list" @@ -903,190 +914,193 @@ pub mod make_bags { } } -// #[cfg(test)] -// mod tests { -// use frame_support::traits::Currency; -// use substrate_test_utils::assert_eq_uvec; +#[cfg(test)] +mod tests { + use frame_support::traits::Currency; + use substrate_test_utils::assert_eq_uvec; -// use super::*; -// use crate::mock::*; + use super::*; + use crate::mock::*; -// const GENESIS_VOTER_IDS: [u64; 5] = [11, 21, 31, 41, 101]; + const GENESIS_VOTER_IDS: [u64; 5] = [11, 21, 31, 41, 101]; -// #[test] -// fn voter_list_includes_genesis_accounts() { -// ExtBuilder::default().validator_pool(true).build_and_execute(|| { -// let have_voters: Vec<_> = VoterList::::iter().map(|node| node.voter.id).collect(); -// assert_eq_uvec!(GENESIS_VOTER_IDS, have_voters); -// }); -// } + #[test] + fn voter_list_includes_genesis_accounts() { + ExtBuilder::default().validator_pool(true).build_and_execute(|| { + let have_voters: Vec<_> = VoterList::::iter().map(|node| node.voter.id).collect(); + assert_eq_uvec!(GENESIS_VOTER_IDS, have_voters); + }); + } -// /// This tests the property that when iterating through the `VoterList`, we iterate from higher -// /// bags to lower. -// #[test] -// fn iteration_is_semi_sorted() { -// use rand::seq::SliceRandom; -// let mut rng = rand::thread_rng(); - -// // Randomly sort the list of voters. Later we'll give each of these a stake such that it -// // fits into a different bag. -// let voters = { -// let mut v = vec![0; GENESIS_VOTER_IDS.len()]; -// v.copy_from_slice(&GENESIS_VOTER_IDS); -// v.shuffle(&mut rng); -// v -// }; - -// ExtBuilder::default().validator_pool(true).build_and_execute(|| { -// // initialize the voters' deposits -// let mut balance = 10; -// for voter_id in voters.iter().rev() { -// ::Currency::make_free_balance_be(voter_id, balance); -// let controller = Staking::bonded(voter_id).unwrap(); -// let mut ledger = Staking::ledger(&controller).unwrap(); -// ledger.total = balance; -// ledger.active = balance; -// Staking::update_ledger(&controller, &ledger); -// Staking::do_rebag(voter_id); - -// balance += 10; -// } - -// let have_voters: Vec<_> = VoterList::::iter().map(|node| node.voter.id).collect(); -// assert_eq!(voters, have_voters); -// }); -// } + /// This tests the property that when iterating through the `VoterList`, we iterate from higher + /// bags to lower. + #[test] + fn iteration_is_semi_sorted() { + use rand::seq::SliceRandom; + let mut rng = rand::thread_rng(); + + // Randomly sort the list of voters. Later we'll give each of these a stake such that it + // fits into a different bag. + let voters = { + let mut v = vec![0; GENESIS_VOTER_IDS.len()]; + v.copy_from_slice(&GENESIS_VOTER_IDS); + v.shuffle(&mut rng); + v + }; -// /// This tests that we can `take` x voters, even if that quantity ends midway through a list. -// #[test] -// fn take_works() { -// ExtBuilder::default().validator_pool(true).build_and_execute(|| { -// // initialize the voters' deposits -// let mut balance = 0; // This will be 10 on the first loop iteration bc 0 % 3 == 0 -// for (idx, voter_id) in GENESIS_VOTER_IDS.iter().enumerate() { -// if idx % 3 == 0 { -// // This increases the balance by 10, which is the amount each threshold -// // increases by. Thus this will increase the balance by 1 bag. -// // -// // This will create 2 bags, the lower threshold bag having -// // 3 voters with balance 10, and the higher threshold bag having -// // 2 voters with balance 20. -// balance += 10; -// } - -// ::Currency::make_free_balance_be(voter_id, balance); -// let controller = Staking::bonded(voter_id).unwrap(); -// let mut ledger = Staking::ledger(&controller).unwrap(); -// ledger.total = balance; -// ledger.active = balance; -// Staking::update_ledger(&controller, &ledger); -// Staking::do_rebag(voter_id); -// } - -// let bag_thresh10 = ::VoterBags::get(&10) -// .unwrap() -// .iter() -// .map(|node| node.voter.id) -// .collect::>(); -// assert_eq!(bag_thresh10, vec![11, 21, 31]); - -// let bag_thresh20 = ::VoterBags::get(&20) -// .unwrap() -// .iter() -// .map(|node| node.voter.id) -// .collect::>(); -// assert_eq!(bag_thresh20, vec![41, 101]); - -// let voters: Vec<_> = VoterList::::iter() -// // take 4/5 from [41, 101],[11, 21, 31], demonstrating that we can do a -// // take that stops mid bag. -// .take(4) -// .map(|node| node.voter.id) -// .collect(); - -// assert_eq!(voters, vec![41, 101, 11, 21]); -// }); -// } + ExtBuilder::default().validator_pool(true).build_and_execute(|| { + // initialize the voters' deposits + let mut balance = 10; + for voter_id in voters.iter().rev() { + ::Currency::make_free_balance_be(voter_id, balance); + let controller = Staking::bonded(voter_id).unwrap(); + let mut ledger = Staking::ledger(&controller).unwrap(); + ledger.total = balance; + ledger.active = balance; + Staking::update_ledger(&controller, &ledger); + Staking::do_rebag(voter_id); + + balance += 10; + } -// #[test] -// fn storage_is_cleaned_up_as_voters_are_removed() { -// ExtBuilder::default().validator_pool(true).build_and_execute(|| { -// // Initialize voters deposits so there are 5 bags with one voter each. -// let mut balance = 10; -// for voter_id in GENESIS_VOTER_IDS.iter() { -// ::Currency::make_free_balance_be(voter_id, balance); -// let controller = Staking::bonded(voter_id).unwrap(); -// let mut ledger = Staking::ledger(&controller).unwrap(); -// ledger.total = balance; -// ledger.active = balance; -// Staking::update_ledger(&controller, &ledger); -// Staking::do_rebag(voter_id); - -// // Increase balance to the next threshold. -// balance += 10; -// } - -// let voter_list_storage_items_eq = |mut v: Vec| { -// v.sort(); -// let mut voters: Vec<_> = -// VoterList::::iter().map(|node| node.voter.id).collect(); -// voters.sort(); -// assert_eq!(voters, v); - -// let mut nodes: Vec<_> = -// ::VoterNodes::iter_keys().collect(); -// nodes.sort(); -// assert_eq!(nodes, v); - -// let mut flat_bags: Vec<_> = ::VoterBags::iter() -// .flat_map(|(_, bag)| bag.iter()) -// .map(|node| node.voter.id) -// .collect(); -// flat_bags.sort(); -// assert_eq!(flat_bags, v); - -// let mut bags_for: Vec<_> = -// ::VoterBagFor::iter_keys().collect(); -// bags_for.sort(); -// assert_eq!(bags_for, v); -// }; - -// let genesis_voters = vec![101, 41, 31, 21, 11]; -// voter_list_storage_items_eq(genesis_voters); -// assert_eq!(::VoterCount::get(), 5); - -// // Remove 1 voter, -// VoterList::::remove(&101); -// let remaining_voters = vec![41, 31, 21, 11]; -// // and assert they have been cleaned up. -// voter_list_storage_items_eq(remaining_voters.clone()); -// assert_eq!(::VoterCount::get(), 4); - -// // Now remove the remaining voters so we have 0 left, -// remaining_voters.iter().for_each(|v| VoterList::::remove(v)); -// // and assert all of them have been cleaned up. -// voter_list_storage_items_eq(vec![]); -// assert_eq!(::VoterCount::get(), 0); - -// // TODO the below is redundant with voter_list_storage_items_eq(vec![]), but slightly -// // different since it checks length. Practically should be the same, but we are extra -// // through in checking keys ... probs should remove tho to reduce redundancy. - -// // We can make sure _everyone_ has been totally removed, -// remaining_voters.iter().for_each(|v| { -// assert!(!::VoterNodes::contains_key(v)); -// assert!(!::VoterBagFor::contains_key(v)); -// }); -// // there are no more bags left, -// assert_eq!( -// ::VoterBags::iter().collect::>().len(), -// 0 -// ); -// // and the voter list has no one in it. -// assert_eq!( -// ::VoterList::::iter().collect::>().len(), -// 0 -// ); -// }); -// } -// } + let have_voters: Vec<_> = VoterList::::iter().map(|node| node.voter.id).collect(); + assert_eq!(voters, have_voters); + }); + } + + /// This tests that we can `take` x voters, even if that quantity ends midway through a list. + #[test] + fn take_works() { + ExtBuilder::default().validator_pool(true).build_and_execute(|| { + // initialize the voters' deposits + let mut balance = 0; // This will be 10 on the first loop iteration bc 0 % 3 == 0 + for (idx, voter_id) in GENESIS_VOTER_IDS.iter().enumerate() { + if idx % 3 == 0 { + // This increases the balance by 10, which is the amount each threshold + // increases by. Thus this will increase the balance by 1 bag. + // + // This will create 2 bags, the lower threshold bag having + // 3 voters with balance 10, and the higher threshold bag having + // 2 voters with balance 20. + balance += 10; + } + + ::Currency::make_free_balance_be(voter_id, balance); + let controller = Staking::bonded(voter_id).unwrap(); + let mut ledger = Staking::ledger(&controller).unwrap(); + ledger.total = balance; + ledger.active = balance; + Staking::update_ledger(&controller, &ledger); + Staking::do_rebag(voter_id); + } + + let bag_thresh10 = Bag::::get(10) + .unwrap() + .iter() + .map(|node| node.voter.id) + .collect::>(); + assert_eq!(bag_thresh10, vec![11, 21, 31]); + + let bag_thresh20 = Bag::::get(20) + .unwrap() + .iter() + .map(|node| node.voter.id) + .collect::>(); + assert_eq!(bag_thresh20, vec![41, 101]); + + let voters: Vec<_> = VoterList::::iter() + // take 4/5 from [41, 101],[11, 21, 31], demonstrating that we can do a + // take that stops mid bag. + .take(4) + .map(|node| node.voter.id) + .collect(); + + assert_eq!(voters, vec![41, 101, 11, 21]); + }); + } + + #[test] + fn storage_is_cleaned_up_as_voters_are_removed() { + ExtBuilder::default().validator_pool(true).build_and_execute(|| { + // Initialize voters deposits so there are 5 bags with one voter each. + let mut balance = 10; + for voter_id in GENESIS_VOTER_IDS.iter() { + ::Currency::make_free_balance_be(voter_id, balance); + let controller = Staking::bonded(voter_id).unwrap(); + let mut ledger = Staking::ledger(&controller).unwrap(); + ledger.total = balance; + ledger.active = balance; + Staking::update_ledger(&controller, &ledger); + Staking::do_rebag(voter_id); + + // Increase balance to the next threshold. + balance += 10; + } + + let voter_list_storage_items_eq = |mut v: Vec| { + v.sort(); + let mut voters: Vec<_> = + VoterList::::iter().map(|node| node.voter.id).collect(); + voters.sort(); + assert_eq!(voters, v); + + let mut nodes: Vec<_> = + ::VoterNodes::iter_keys().collect(); + nodes.sort(); + assert_eq!(nodes, v); + + let mut flat_bags: Vec<_> = ::VoterBags::iter() + // We always get the bag with the Bag getter because the bag_upper + // is only initialized in the getter. + .flat_map(|(key, _bag)| Bag::::get(key).unwrap().iter()) + .map(|node| node.voter.id) + .collect(); + flat_bags.sort(); + assert_eq!(flat_bags, v); + + let mut bags_for: Vec<_> = + ::VoterBagFor::iter_keys().collect(); + bags_for.sort(); + assert_eq!(bags_for, v); + }; + + let genesis_voters = vec![101, 41, 31, 21, 11]; + voter_list_storage_items_eq(genesis_voters); + assert_eq!(::VoterCount::get(), 5); + + // Remove 1 voter, + VoterList::::remove(&101); + let remaining_voters = vec![41, 31, 21, 11]; + // and assert they have been cleaned up. + voter_list_storage_items_eq(remaining_voters.clone()); + assert_eq!(::VoterCount::get(), 4); + + // Now remove the remaining voters so we have 0 left, + remaining_voters.iter().for_each(|v| VoterList::::remove(v)); + // and assert all of them have been cleaned up. + voter_list_storage_items_eq(vec![]); + assert_eq!(::VoterCount::get(), 0); + + // TODO the below is redundant with voter_list_storage_items_eq(vec![]), but slightly + // different since it checks length. Practically should be the same, but we do an extra + // by checking keys ... probs should remove tho to reduce redundancy. + + // We can make sure _everyone_ has been totally removed, + remaining_voters.iter().for_each(|v| { + assert!(!::VoterNodes::contains_key(v)); + assert!(!::VoterBagFor::contains_key(v)); + }); + // TODO bags do not get cleaned up from storages + // - is this ok? + assert_eq!( + ::VoterBags::iter().collect::>().len(), + 6 + ); + // and the voter list has no one in it. + assert_eq!( + VoterList::::iter().collect::>().len(), + 0 + ); + }); + } +} From e0dfed09523f46e0017247eb255694b1854024f6 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Thu, 15 Jul 2021 13:48:06 -0700 Subject: [PATCH 16/26] Sanity check WIP; tests failing --- frame/staking/src/voter_bags.rs | 141 +++++++++++++++----------------- 1 file changed, 65 insertions(+), 76 deletions(-) diff --git a/frame/staking/src/voter_bags.rs b/frame/staking/src/voter_bags.rs index bed112f90ded0..2670c3d6d0f30 100644 --- a/frame/staking/src/voter_bags.rs +++ b/frame/staking/src/voter_bags.rs @@ -82,6 +82,7 @@ impl VoterList { crate::VoterBagFor::::remove_all(None); crate::VoterBags::::remove_all(None); crate::VoterNodes::::remove_all(None); + sanity_check_voter_list::(); } /// Regenerate voter data from the `Nominators` and `Validators` storage items. @@ -181,10 +182,12 @@ impl VoterList { } for (_, bag) in bags { + bag.sanity_check(); bag.put(); } crate::VoterCount::::mutate(|prev_count| *prev_count = prev_count.saturating_add(count)); + sanity_check_voter_list::(); count } @@ -217,10 +220,13 @@ impl VoterList { } for (_, bag) in bags { + bag.sanity_check(); bag.put(); } crate::VoterCount::::mutate(|prev_count| *prev_count = prev_count.saturating_sub(count)); + + sanity_check_voter_list::(); } /// Update a voter's position in the voter list. @@ -228,7 +234,7 @@ impl VoterList { /// If the voter was in the correct bag, no effect. If the voter was in the incorrect bag, they /// are moved into the correct bag. /// - /// Returns `true` if the voter moved. + /// Returns `Some((old_idx, new_idx))` if the voter moved, otherwise `None`. /// /// This operation is somewhat more efficient than simply calling [`self.remove`] followed by /// [`self.insert`]. However, given large quantities of voters to move, it may be more efficient @@ -258,8 +264,11 @@ impl VoterList { node.bag_upper = new_idx; let mut bag = Bag::::get_or_make(node.bag_upper); bag.insert_node(node); + bag.sanity_check(); bag.put(); + sanity_check_voter_list::(); + (old_idx, new_idx) }) } @@ -349,6 +358,8 @@ impl VoterList { "all `bag_upper` in storage must be members of the new thresholds", ); + sanity_check_voter_list::(); + num_affected } } @@ -455,6 +466,9 @@ impl Bag { self.tail = Some(id.clone()); crate::VoterBagFor::::insert(id, self.bag_upper); + + self.sanity_check(); + sanity_check_voter_list::(); } /// Remove a voter node from this bag. @@ -466,49 +480,14 @@ impl Bag { /// `self.put()`, `VoterNodes::remove(voter_id)` and `VoterBagFor::remove(voter_id)` /// to update storage for the bag and `node`. fn remove_node(&mut self, node: &Node) { - // Excise `node`. // Update previous node. if let Some(mut prev) = node.prev() { - debug_assert!( - self.head.as_ref() != Some(&node.voter.id), - "node is the head, but has Some prev" - ); - debug_assert!( - prev.prev().is_some() || self.head.as_ref() == Some(&prev.voter.id), - "node.prev.prev should be Some OR node.prev should be the head" - ); - prev.next = node.next.clone(); - debug_assert!( - prev.next().and_then(|prev_next| - // prev.next.prev should point at node prior to being reassigned - Some(prev_next.prev().unwrap().voter.id == node.voter.id) - ) - // unless prev.next is None, in which case node has to be the tail - .unwrap_or(self.tail.as_ref() == Some(&node.voter.id)), - "prev.next.prev should point at node prior to being reassigned OR node should be the tail" - ); - prev.put(); } // Update next node. if let Some(mut next) = node.next() { - debug_assert!( - self.tail.as_ref() != Some(&node.voter.id), - "node is the tail, but has Some next" - ); - next.prev = node.prev.clone(); - debug_assert!( - next.prev().and_then(|next_prev| - // next.prev.next should point at next after being reassigned - Some(next_prev.next().unwrap().voter.id == next.voter.id) - ) - // unless next.prev is None, in which case node has to be the head - .unwrap_or_else(|| self.head.as_ref() == Some(&node.voter.id)), - "next.prev.next should point at next after being reassigned OR node should be the head" - ); - next.put(); } @@ -519,32 +498,55 @@ impl Bag { if self.tail.as_ref() == Some(&node.voter.id) { self.tail = node.prev.clone(); } + + // TODO double check these only conditionally make it in with debug builds + self.sanity_check(); + sanity_check_voter_list::(); } -// #[cfg(debug_assertions)] -// fn sanity_check_voter_list(&self) { -// use sp_std::collections::btree_set::BTreeSet; - -// // iterate all voters and ensure only those that are head don't have a prev -// assert!(self.head().prev().is_none()); -// // iterate all voters and ensure only those that are tail don't have a next -// assert!(self.tail().next().is_none()); -// // iterate all voters and ensure that there are no loops -// // Each voter is only seen once, thus there is no cycle within a bag. -// let seen_in_bag = BTreeSet::new(); -// assert!( -// self.iter().map(|node| node.voter.id) -// .all(|voter| seen_in_bag.insert(voter)) -// ); - -// let seen_in_voter_list = BTreeSet::new(); -// assert!( -// VoterList::::iter() -// ); -// // iterate all voters and ensure their count is in sync with `VoterCount` -// // ensure VoterCount itself is always `== CounterForValidators + CounterForNominators` -// // anything else.. -// } + // Sanity check this bag. + #[cfg(debug_assertions)] + fn sanity_check(&self) { + // Check: + // 1) Iterate all voters and ensure only those that are head don't have a prev. + assert!(self + .head() + .and_then(|head| Some(head.prev().is_none())) + // if there is no head, then there must not be a tail + .unwrap_or_else(|| self.tail.is_none())); + // 2) Iterate all voters and ensure only those that are tail don't have a next. + assert!(self + .tail() + .and_then(|tail| Some(tail.next().is_none())) + // if there is no head, then there must not be a tail + .unwrap_or_else(|| self.head.is_none())); + // 3) Iterate all voters and ensure that there are no loops. + let mut seen_in_bag = BTreeSet::new(); + assert!(self + .iter() + .map(|node| node.voter.id) + // each voter is only seen once, thus there is no cycle within a bag + .all(|voter| seen_in_bag.insert(voter))); + } +} + +// Sanity check the voter list. +#[cfg(debug_assertions)] +pub (crate) fn sanity_check_voter_list() { + use crate::{CounterForValidators, VoterCount, CounterForNominators}; + // Check: + // 1) Iterate all voters in list and make sure there are no duplicates. + let mut seen_in_list = BTreeSet::new(); + assert!(VoterList::::iter() + .map(|node| node.voter.id) + .all(|voter| seen_in_list.insert(voter))); + // 2) Iterate all voters and ensure their count is in sync with `VoterCount`. + assert_eq!(VoterList::::iter().collect::>().len() as u32, VoterCount::::get()); + // 3) Ensure VoterCount itself is always `== CounterForValidators + CounterForNominators` + // assert_eq!( + // VoterCount::::get(), + // CounterForValidators::::get() + CounterForNominators::::get() + // ); } /// A Node is the fundamental element comprising the doubly-linked lists which for each bag. @@ -563,14 +565,7 @@ pub struct Node { impl Node { /// Get a node by bag idx and account id. pub fn get(bag_upper: VoteWeight, account_id: &AccountIdOf) -> Option> { - if !T::VoterBagThresholds::get().contains(&bag_upper) { - let n = crate::VoterNodes::::try_get(account_id).ok().map(|mut node| { - node.bag_upper = bag_upper; - node - }); - println!("node::get bag_upper {}. account id {}. n: {:#?}", bag_upper, account_id, n); - } - debug_assert!( // TODO: figure out why this breaks test take_works + debug_assert!( T::VoterBagThresholds::get().contains(&bag_upper) || bag_upper == VoteWeight::MAX, "it is a logic error to attempt to get a bag which is not in the thresholds list" ); @@ -1092,15 +1087,9 @@ mod tests { }); // TODO bags do not get cleaned up from storages // - is this ok? - assert_eq!( - ::VoterBags::iter().collect::>().len(), - 6 - ); + assert_eq!(::VoterBags::iter().collect::>().len(), 6); // and the voter list has no one in it. - assert_eq!( - VoterList::::iter().collect::>().len(), - 0 - ); + assert_eq!(VoterList::::iter().collect::>().len(), 0); }); } } From f1f25f4a9241effaa3c5547e1368b14f7cf806b8 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Thu, 15 Jul 2021 14:41:45 -0700 Subject: [PATCH 17/26] Update sanity checks to be more correct --- frame/staking/src/voter_bags.rs | 50 +++++++++++++-------------------- 1 file changed, 19 insertions(+), 31 deletions(-) diff --git a/frame/staking/src/voter_bags.rs b/frame/staking/src/voter_bags.rs index 2670c3d6d0f30..b24397cda6877 100644 --- a/frame/staking/src/voter_bags.rs +++ b/frame/staking/src/voter_bags.rs @@ -82,7 +82,7 @@ impl VoterList { crate::VoterBagFor::::remove_all(None); crate::VoterBags::::remove_all(None); crate::VoterNodes::::remove_all(None); - sanity_check_voter_list::(); + Self::sanity_check(); } /// Regenerate voter data from the `Nominators` and `Validators` storage items. @@ -182,12 +182,11 @@ impl VoterList { } for (_, bag) in bags { - bag.sanity_check(); bag.put(); } crate::VoterCount::::mutate(|prev_count| *prev_count = prev_count.saturating_add(count)); - sanity_check_voter_list::(); + Self::sanity_check(); count } @@ -220,13 +219,12 @@ impl VoterList { } for (_, bag) in bags { - bag.sanity_check(); bag.put(); } crate::VoterCount::::mutate(|prev_count| *prev_count = prev_count.saturating_sub(count)); - sanity_check_voter_list::(); + Self::sanity_check(); } /// Update a voter's position in the voter list. @@ -264,11 +262,8 @@ impl VoterList { node.bag_upper = new_idx; let mut bag = Bag::::get_or_make(node.bag_upper); bag.insert_node(node); - bag.sanity_check(); bag.put(); - sanity_check_voter_list::(); - (old_idx, new_idx) }) } @@ -358,10 +353,25 @@ impl VoterList { "all `bag_upper` in storage must be members of the new thresholds", ); - sanity_check_voter_list::(); + Self::sanity_check(); num_affected } + + // Sanity check the voter list. + #[cfg(debug_assertions)] + fn sanity_check() { + // Check: + // 1) Iterate all voters in list and make sure there are no duplicates. + let mut seen_in_list = BTreeSet::new(); + assert!(Self::iter().map(|node| node.voter.id).all(|voter| seen_in_list.insert(voter))); + // 2) Iterate all voters and ensure their count is in sync with `VoterCount`. + assert_eq!(Self::iter().collect::>().len() as u32, crate::VoterCount::::get()); + + // NOTE: we don't check `CounterForNominators + CounterForValidators == VoterCount` + // because those are updated in lib.rs, and thus tests for just this module do + // not update them. + } } /// A Bag is a doubly-linked list of voters. @@ -468,7 +478,6 @@ impl Bag { crate::VoterBagFor::::insert(id, self.bag_upper); self.sanity_check(); - sanity_check_voter_list::(); } /// Remove a voter node from this bag. @@ -499,9 +508,7 @@ impl Bag { self.tail = node.prev.clone(); } - // TODO double check these only conditionally make it in with debug builds self.sanity_check(); - sanity_check_voter_list::(); } // Sanity check this bag. @@ -530,25 +537,6 @@ impl Bag { } } -// Sanity check the voter list. -#[cfg(debug_assertions)] -pub (crate) fn sanity_check_voter_list() { - use crate::{CounterForValidators, VoterCount, CounterForNominators}; - // Check: - // 1) Iterate all voters in list and make sure there are no duplicates. - let mut seen_in_list = BTreeSet::new(); - assert!(VoterList::::iter() - .map(|node| node.voter.id) - .all(|voter| seen_in_list.insert(voter))); - // 2) Iterate all voters and ensure their count is in sync with `VoterCount`. - assert_eq!(VoterList::::iter().collect::>().len() as u32, VoterCount::::get()); - // 3) Ensure VoterCount itself is always `== CounterForValidators + CounterForNominators` - // assert_eq!( - // VoterCount::::get(), - // CounterForValidators::::get() + CounterForNominators::::get() - // ); -} - /// A Node is the fundamental element comprising the doubly-linked lists which for each bag. #[derive(Encode, Decode)] #[cfg_attr(feature = "std", derive(frame_support::DebugNoBound))] From 90a04d0c722a6cf16ce2f9b48cce61cdfc16d57c Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Thu, 15 Jul 2021 14:53:54 -0700 Subject: [PATCH 18/26] Improve storage cleanup tests --- frame/staking/src/voter_bags.rs | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/frame/staking/src/voter_bags.rs b/frame/staking/src/voter_bags.rs index b24397cda6877..e0db54303ebb5 100644 --- a/frame/staking/src/voter_bags.rs +++ b/frame/staking/src/voter_bags.rs @@ -1064,20 +1064,14 @@ mod tests { voter_list_storage_items_eq(vec![]); assert_eq!(::VoterCount::get(), 0); - // TODO the below is redundant with voter_list_storage_items_eq(vec![]), but slightly - // different since it checks length. Practically should be the same, but we do an extra - // by checking keys ... probs should remove tho to reduce redundancy. - - // We can make sure _everyone_ has been totally removed, - remaining_voters.iter().for_each(|v| { - assert!(!::VoterNodes::contains_key(v)); - assert!(!::VoterBagFor::contains_key(v)); - }); + // TODO bags do not get cleaned up from storages // - is this ok? assert_eq!(::VoterBags::iter().collect::>().len(), 6); // and the voter list has no one in it. assert_eq!(VoterList::::iter().collect::>().len(), 0); + assert_eq!(::VoterBagFor::iter().collect::>().len(), 0); + assert_eq!(::VoterNodes::iter().collect::>().len(), 0); }); } } From c466c953717216c922ed06608e39e1086ec745ca Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Thu, 15 Jul 2021 21:22:09 -0700 Subject: [PATCH 19/26] WIP remote_ext_tests --- Cargo.lock | 5 ++ bin/node/executor/benches/bench.rs | 19 +++--- frame/staking/Cargo.toml | 7 ++ frame/staking/src/lib.rs | 10 ++- frame/staking/src/mock.rs | 2 +- frame/staking/src/remote_ext_tests.rs | 71 +++++++++++++++++++++ frame/staking/src/voter_bags.rs | 2 +- utils/frame/remote-externalities/Cargo.toml | 2 +- 8 files changed, 103 insertions(+), 15 deletions(-) create mode 100644 frame/staking/src/remote_ext_tests.rs diff --git a/Cargo.lock b/Cargo.lock index 1a915db17c878..776f96b2ac1d4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5535,6 +5535,8 @@ dependencies = [ "paste 1.0.4", "rand 0.8.4", "rand_chacha 0.2.2", + "remote-externalities", + "sc-executor", "serde", "sp-application-crypto", "sp-core", @@ -5542,11 +5544,14 @@ dependencies = [ "sp-npos-elections", "sp-runtime", "sp-staking", + "sp-state-machine", "sp-std", "sp-storage", "sp-tracing", "static_assertions", + "substrate-test-runtime", "substrate-test-utils", + "tokio 0.2.25", ] [[package]] diff --git a/bin/node/executor/benches/bench.rs b/bin/node/executor/benches/bench.rs index d21aedd1d1849..dd8b267745812 100644 --- a/bin/node/executor/benches/bench.rs +++ b/bin/node/executor/benches/bench.rs @@ -16,21 +16,22 @@ // limitations under the License. use codec::{Decode, Encode}; -use criterion::{BatchSize, Criterion, criterion_group, criterion_main}; +use criterion::{criterion_group, criterion_main, BatchSize, Criterion}; +use frame_support::Hashable; use node_executor::Executor; use node_primitives::{BlockNumber, Hash}; use node_runtime::{ - Block, BuildStorage, Call, CheckedExtrinsic, GenesisConfig, Header, UncheckedExtrinsic, + constants::currency::*, Block, BuildStorage, Call, CheckedExtrinsic, GenesisConfig, Header, + UncheckedExtrinsic, }; -use node_runtime::constants::currency::*; use node_testing::keyring::*; -use sp_core::{NativeOrEncoded, NeverNativeValue}; -use sp_core::storage::well_known_keys; -use sp_core::traits::{CodeExecutor, RuntimeCode}; -use frame_support::Hashable; -use sp_state_machine::TestExternalities as CoreTestExternalities; -use sc_executor::{NativeExecutor, RuntimeInfo, WasmExecutionMethod, Externalities}; +use sp_core::{ + storage::well_known_keys, + traits::{CodeExecutor, RuntimeCode}, + NativeOrEncoded, NeverNativeValue, +}; use sp_runtime::traits::BlakeTwo256; +use sp_state_machine::TestExternalities as CoreTestExternalities; criterion_group!(benches, bench_execute_block); criterion_main!(benches); diff --git a/frame/staking/Cargo.toml b/frame/staking/Cargo.toml index 8897262431606..e35a06f507006 100644 --- a/frame/staking/Cargo.toml +++ b/frame/staking/Cargo.toml @@ -48,12 +48,18 @@ sp-storage = { version = "3.0.0", path = "../../primitives/storage" } sp-tracing = { version = "3.0.0", path = "../../primitives/tracing" } sp-core = { version = "3.0.0", path = "../../primitives/core" } sp-npos-elections = { version = "3.0.0", path = "../../primitives/npos-elections", features = ["mocks"] } +sp-state-machine = { version = "0.9.0", path = "../../primitives/state-machine" } pallet-balances = { version = "3.0.0", path = "../balances" } pallet-timestamp = { version = "3.0.0", path = "../timestamp" } pallet-staking-reward-curve = { version = "3.0.0", path = "../staking/reward-curve" } substrate-test-utils = { version = "3.0.0", path = "../../test-utils" } frame-benchmarking = { version = "3.1.0", path = "../benchmarking" } frame-election-provider-support = { version = "3.0.0", features = ["runtime-benchmarks"], path = "../election-provider-support" } +remote-externalities = { version = "0.9", path = "../../utils/frame/remote-externalities" } +sc-executor = { version = "0.9.0", path = "../../client/executor" } +substrate-test-runtime = { version = "2.0.0", path = "../../test-utils/runtime" } + +tokio = { version = "0.2", features = ["macros", "rt-threaded"] } rand = "0.8.4" rand_chacha = { version = "0.2" } parking_lot = "0.11.1" @@ -93,3 +99,4 @@ make-bags = [ "sp-tracing", "std", ] +remote-ext = [] diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index 00458ea9f49b0..f5aac51c36998 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -274,14 +274,18 @@ #![recursion_limit = "128"] #![cfg_attr(not(feature = "std"), no_std)] +#[cfg(any(feature = "runtime-benchmarks", test))] +pub mod benchmarking; #[cfg(any(test, feature = "make-bags"))] pub mod mock; +// #[cfg(any(feature = "remote-ext"))] +#[cfg(feature = "remote-ext")] #[cfg(test)] -mod tests; +mod remote_ext_tests; #[cfg(any(feature = "runtime-benchmarks", test))] pub mod testing_utils; -#[cfg(any(feature = "runtime-benchmarks", test))] -pub mod benchmarking; +#[cfg(test)] +mod tests; pub mod inflation; pub mod slashing; diff --git a/frame/staking/src/mock.rs b/frame/staking/src/mock.rs index a584b83c539c8..8cffecc4833bb 100644 --- a/frame/staking/src/mock.rs +++ b/frame/staking/src/mock.rs @@ -92,7 +92,7 @@ pub fn is_disabled(controller: AccountId) -> bool { } type UncheckedExtrinsic = frame_system::mocking::MockUncheckedExtrinsic; -type Block = frame_system::mocking::MockBlock; +pub(crate) type Block = frame_system::mocking::MockBlock; frame_support::construct_runtime!( pub enum Test where diff --git a/frame/staking/src/remote_ext_tests.rs b/frame/staking/src/remote_ext_tests.rs new file mode 100644 index 0000000000000..a6d576901cf1f --- /dev/null +++ b/frame/staking/src/remote_ext_tests.rs @@ -0,0 +1,71 @@ +use remote_externalities::{Builder, Mode, OnlineConfig}; +// use sp_runtime::traits::BlakeTwo256; +use sc_executor::{NativeExecutor, WasmExecutionMethod}; +use sp_core::storage::well_known_keys; +use sp_runtime::traits::NumberFor; +use sp_state_machine::{ExecutionStrategy, StateMachine}; + +use crate::mock::*; + +const RPC_NODE: &str = "ws://localhost:9944"; + +// TODO: do I need this feature gat if its in lib where we require this mod? +#[cfg(feature = "remote-ext")] +#[cfg(test)] + +// pub type BlockNumber = u32; +// pub type Header = generic::Header; +// /// Block type as expected by this runtime. +// pub type Block = generic::Block; + +sc_executor::native_executor_instance!( + pub MyExecutor, + substrate_test_runtime::api::dispatch, + substrate_test_runtime::native_version, +); + +#[tokio::test] +async fn test_voter_bags_migration() { + ExtBuilder::default().build_and_execute(|| {}); + + let ext = Builder::::new() + .mode(Mode::Online(OnlineConfig { + transport: RPC_NODE.to_string().into(), + modules: vec!["Staking".to_string()], + ..Default::default() + })) + // TODO inject code from this runtime???? + .inject_hashed_key(well_known_keys::CODE) + .build() + .await + .unwrap(); + + ext.execute_with(|| { + // regenerate ... + // any other test we need to ... + }) + + // let executor = + // NativeExecutor::::new(WasmExecutionMethod::Interpreted, Some(2046), 8); + + // let mut changes = Default::default(); + + // // TODO probs don't call StateMachine here, instead + // let _encoded_result = StateMachine::<_, _, NumberFor, _>::new( + // &ext.backend, + // None, + // &mut changes, + // &executor, + // "TryRuntime_on_runtime_upgrade", + // &[], + // ext.extensions, + // &sp_state_machine::backend::BackendRuntimeCode::new(&ext.backend) + // .runtime_code() + // .unwrap(), + // sp_core::testing::TaskExecutor::new(), + // ) + // .execute(ExecutionStrategy::AlwaysWasm.into()) + // .map_err(|e| format!("failed to execute 'TryRuntime_on_runtime_upgrade': {:?}", e)) + // .unwrap(); + // // .unwrap(); +} diff --git a/frame/staking/src/voter_bags.rs b/frame/staking/src/voter_bags.rs index e0db54303ebb5..9e922b3727882 100644 --- a/frame/staking/src/voter_bags.rs +++ b/frame/staking/src/voter_bags.rs @@ -956,7 +956,7 @@ mod tests { fn take_works() { ExtBuilder::default().validator_pool(true).build_and_execute(|| { // initialize the voters' deposits - let mut balance = 0; // This will be 10 on the first loop iteration bc 0 % 3 == 0 + let mut balance = 0; // This will be 10 on the first loop iteration because 0 % 3 == 0 for (idx, voter_id) in GENESIS_VOTER_IDS.iter().enumerate() { if idx % 3 == 0 { // This increases the balance by 10, which is the amount each threshold diff --git a/utils/frame/remote-externalities/Cargo.toml b/utils/frame/remote-externalities/Cargo.toml index a7519b7e47f33..5a2065b0387fe 100644 --- a/utils/frame/remote-externalities/Cargo.toml +++ b/utils/frame/remote-externalities/Cargo.toml @@ -6,7 +6,7 @@ edition = "2018" license = "Apache-2.0" homepage = "https://substrate.dev" repository = "https://github.com/paritytech/substrate/" -description = "An externalities provided environemnt that can load itself from remote nodes or cache files" +description = "An externalities provided environment that can load itself from remote nodes or cache files" readme = "README.md" [package.metadata.docs.rs] From fd907c6e2440cdae44be9f1c61e90791d0520c6e Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Thu, 15 Jul 2021 21:23:18 -0700 Subject: [PATCH 20/26] Some notes on next steps --- frame/staking/src/remote_ext_tests.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/frame/staking/src/remote_ext_tests.rs b/frame/staking/src/remote_ext_tests.rs index a6d576901cf1f..e59383133cedc 100644 --- a/frame/staking/src/remote_ext_tests.rs +++ b/frame/staking/src/remote_ext_tests.rs @@ -41,7 +41,11 @@ async fn test_voter_bags_migration() { .unwrap(); ext.execute_with(|| { - // regenerate ... + // - check existing data ?? + // - regenerate ... + // - check regenerated data + // - stats on how each bag is occupied? + // - use real thresholds .. // any other test we need to ... }) From a626db0260e8d9e0b633d52e64989938707103bd Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Sun, 18 Jul 2021 18:39:03 -0700 Subject: [PATCH 21/26] Remove some stuff that was for remote-ext tests --- Cargo.lock | 4 -- bin/node/executor/benches/bench.rs | 19 +++--- frame/staking/Cargo.toml | 5 -- frame/staking/src/lib.rs | 4 -- frame/staking/src/remote_ext_tests.rs | 75 --------------------- frame/staking/src/voter_bags.rs | 37 +++++----- utils/frame/remote-externalities/src/lib.rs | 12 ++++ 7 files changed, 40 insertions(+), 116 deletions(-) delete mode 100644 frame/staking/src/remote_ext_tests.rs diff --git a/Cargo.lock b/Cargo.lock index 776f96b2ac1d4..195bc1a195c1e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5535,8 +5535,6 @@ dependencies = [ "paste 1.0.4", "rand 0.8.4", "rand_chacha 0.2.2", - "remote-externalities", - "sc-executor", "serde", "sp-application-crypto", "sp-core", @@ -5549,9 +5547,7 @@ dependencies = [ "sp-storage", "sp-tracing", "static_assertions", - "substrate-test-runtime", "substrate-test-utils", - "tokio 0.2.25", ] [[package]] diff --git a/bin/node/executor/benches/bench.rs b/bin/node/executor/benches/bench.rs index dd8b267745812..d21aedd1d1849 100644 --- a/bin/node/executor/benches/bench.rs +++ b/bin/node/executor/benches/bench.rs @@ -16,22 +16,21 @@ // limitations under the License. use codec::{Decode, Encode}; -use criterion::{criterion_group, criterion_main, BatchSize, Criterion}; -use frame_support::Hashable; +use criterion::{BatchSize, Criterion, criterion_group, criterion_main}; use node_executor::Executor; use node_primitives::{BlockNumber, Hash}; use node_runtime::{ - constants::currency::*, Block, BuildStorage, Call, CheckedExtrinsic, GenesisConfig, Header, - UncheckedExtrinsic, + Block, BuildStorage, Call, CheckedExtrinsic, GenesisConfig, Header, UncheckedExtrinsic, }; +use node_runtime::constants::currency::*; use node_testing::keyring::*; -use sp_core::{ - storage::well_known_keys, - traits::{CodeExecutor, RuntimeCode}, - NativeOrEncoded, NeverNativeValue, -}; -use sp_runtime::traits::BlakeTwo256; +use sp_core::{NativeOrEncoded, NeverNativeValue}; +use sp_core::storage::well_known_keys; +use sp_core::traits::{CodeExecutor, RuntimeCode}; +use frame_support::Hashable; use sp_state_machine::TestExternalities as CoreTestExternalities; +use sc_executor::{NativeExecutor, RuntimeInfo, WasmExecutionMethod, Externalities}; +use sp_runtime::traits::BlakeTwo256; criterion_group!(benches, bench_execute_block); criterion_main!(benches); diff --git a/frame/staking/Cargo.toml b/frame/staking/Cargo.toml index e35a06f507006..57553f31b79eb 100644 --- a/frame/staking/Cargo.toml +++ b/frame/staking/Cargo.toml @@ -55,11 +55,7 @@ pallet-staking-reward-curve = { version = "3.0.0", path = "../staking/reward-cur substrate-test-utils = { version = "3.0.0", path = "../../test-utils" } frame-benchmarking = { version = "3.1.0", path = "../benchmarking" } frame-election-provider-support = { version = "3.0.0", features = ["runtime-benchmarks"], path = "../election-provider-support" } -remote-externalities = { version = "0.9", path = "../../utils/frame/remote-externalities" } -sc-executor = { version = "0.9.0", path = "../../client/executor" } -substrate-test-runtime = { version = "2.0.0", path = "../../test-utils/runtime" } -tokio = { version = "0.2", features = ["macros", "rt-threaded"] } rand = "0.8.4" rand_chacha = { version = "0.2" } parking_lot = "0.11.1" @@ -99,4 +95,3 @@ make-bags = [ "sp-tracing", "std", ] -remote-ext = [] diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index f5aac51c36998..8ba3e1bdecfca 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -278,10 +278,6 @@ pub mod benchmarking; #[cfg(any(test, feature = "make-bags"))] pub mod mock; -// #[cfg(any(feature = "remote-ext"))] -#[cfg(feature = "remote-ext")] -#[cfg(test)] -mod remote_ext_tests; #[cfg(any(feature = "runtime-benchmarks", test))] pub mod testing_utils; #[cfg(test)] diff --git a/frame/staking/src/remote_ext_tests.rs b/frame/staking/src/remote_ext_tests.rs deleted file mode 100644 index e59383133cedc..0000000000000 --- a/frame/staking/src/remote_ext_tests.rs +++ /dev/null @@ -1,75 +0,0 @@ -use remote_externalities::{Builder, Mode, OnlineConfig}; -// use sp_runtime::traits::BlakeTwo256; -use sc_executor::{NativeExecutor, WasmExecutionMethod}; -use sp_core::storage::well_known_keys; -use sp_runtime::traits::NumberFor; -use sp_state_machine::{ExecutionStrategy, StateMachine}; - -use crate::mock::*; - -const RPC_NODE: &str = "ws://localhost:9944"; - -// TODO: do I need this feature gat if its in lib where we require this mod? -#[cfg(feature = "remote-ext")] -#[cfg(test)] - -// pub type BlockNumber = u32; -// pub type Header = generic::Header; -// /// Block type as expected by this runtime. -// pub type Block = generic::Block; - -sc_executor::native_executor_instance!( - pub MyExecutor, - substrate_test_runtime::api::dispatch, - substrate_test_runtime::native_version, -); - -#[tokio::test] -async fn test_voter_bags_migration() { - ExtBuilder::default().build_and_execute(|| {}); - - let ext = Builder::::new() - .mode(Mode::Online(OnlineConfig { - transport: RPC_NODE.to_string().into(), - modules: vec!["Staking".to_string()], - ..Default::default() - })) - // TODO inject code from this runtime???? - .inject_hashed_key(well_known_keys::CODE) - .build() - .await - .unwrap(); - - ext.execute_with(|| { - // - check existing data ?? - // - regenerate ... - // - check regenerated data - // - stats on how each bag is occupied? - // - use real thresholds .. - // any other test we need to ... - }) - - // let executor = - // NativeExecutor::::new(WasmExecutionMethod::Interpreted, Some(2046), 8); - - // let mut changes = Default::default(); - - // // TODO probs don't call StateMachine here, instead - // let _encoded_result = StateMachine::<_, _, NumberFor, _>::new( - // &ext.backend, - // None, - // &mut changes, - // &executor, - // "TryRuntime_on_runtime_upgrade", - // &[], - // ext.extensions, - // &sp_state_machine::backend::BackendRuntimeCode::new(&ext.backend) - // .runtime_code() - // .unwrap(), - // sp_core::testing::TaskExecutor::new(), - // ) - // .execute(ExecutionStrategy::AlwaysWasm.into()) - // .map_err(|e| format!("failed to execute 'TryRuntime_on_runtime_upgrade': {:?}", e)) - // .unwrap(); - // // .unwrap(); -} diff --git a/frame/staking/src/voter_bags.rs b/frame/staking/src/voter_bags.rs index 9e922b3727882..8afff51121216 100644 --- a/frame/staking/src/voter_bags.rs +++ b/frame/staking/src/voter_bags.rs @@ -82,7 +82,7 @@ impl VoterList { crate::VoterBagFor::::remove_all(None); crate::VoterBags::::remove_all(None); crate::VoterNodes::::remove_all(None); - Self::sanity_check(); + debug_assert!(Self::sanity_check()); } /// Regenerate voter data from the `Nominators` and `Validators` storage items. @@ -112,11 +112,11 @@ impl VoterList { crate::VoterNodes::::iter().count(), "stored length must match count of nodes", ); - debug_assert_eq!( - maybe_len.unwrap_or_default() as u32, - crate::CounterForNominators::::get() + crate::CounterForValidators::::get(), - "voter count must be sum of validator and nominator count", - ); + // debug_assert_eq!( // TODO: this case will fail in migration pre check + // maybe_len.unwrap_or_default() as u32, + // crate::CounterForNominators::::get() + crate::CounterForValidators::::get(), + // "voter count must be sum of validator and nominator count", + // ); maybe_len } @@ -186,7 +186,7 @@ impl VoterList { } crate::VoterCount::::mutate(|prev_count| *prev_count = prev_count.saturating_add(count)); - Self::sanity_check(); + debug_assert!(Self::sanity_check()); count } @@ -224,7 +224,7 @@ impl VoterList { crate::VoterCount::::mutate(|prev_count| *prev_count = prev_count.saturating_sub(count)); - Self::sanity_check(); + debug_assert!(Self::sanity_check()); } /// Update a voter's position in the voter list. @@ -352,25 +352,25 @@ impl VoterList { }, "all `bag_upper` in storage must be members of the new thresholds", ); - - Self::sanity_check(); + debug_assert!(Self::sanity_check()); num_affected } // Sanity check the voter list. - #[cfg(debug_assertions)] - fn sanity_check() { + fn sanity_check() -> bool { // Check: // 1) Iterate all voters in list and make sure there are no duplicates. let mut seen_in_list = BTreeSet::new(); assert!(Self::iter().map(|node| node.voter.id).all(|voter| seen_in_list.insert(voter))); // 2) Iterate all voters and ensure their count is in sync with `VoterCount`. - assert_eq!(Self::iter().collect::>().len() as u32, crate::VoterCount::::get()); + assert_eq!(Self::iter().collect::>().len() as u32, crate::VoterCount::::get()); // NOTE: we don't check `CounterForNominators + CounterForValidators == VoterCount` // because those are updated in lib.rs, and thus tests for just this module do // not update them. + + true // use bool so it plays friendly with debug_assert } } @@ -477,7 +477,7 @@ impl Bag { crate::VoterBagFor::::insert(id, self.bag_upper); - self.sanity_check(); + debug_assert!(self.sanity_check()); } /// Remove a voter node from this bag. @@ -508,12 +508,11 @@ impl Bag { self.tail = node.prev.clone(); } - self.sanity_check(); + debug_assert!(self.sanity_check()); } // Sanity check this bag. - #[cfg(debug_assertions)] - fn sanity_check(&self) { + fn sanity_check(&self) -> bool { // Check: // 1) Iterate all voters and ensure only those that are head don't have a prev. assert!(self @@ -534,6 +533,8 @@ impl Bag { .map(|node| node.voter.id) // each voter is only seen once, thus there is no cycle within a bag .all(|voter| seen_in_bag.insert(voter))); + + true // use bool so it plays nice with debug assert ? } } @@ -750,7 +751,7 @@ pub mod make_bags { fn path_to_header_file() -> Option { let repo = git2::Repository::open_from_env().ok()?; let workdir = repo.workdir()?; - for file_name in ["HEADER-APACHE2", "HEADER-GPL3", "HEADER", "file_header.txt"] { + for file_name in &["HEADER-APACHE2", "HEADER-GPL3", "HEADER", "file_header.txt"] { let path = workdir.join(file_name); if path.exists() { return Some(path); diff --git a/utils/frame/remote-externalities/src/lib.rs b/utils/frame/remote-externalities/src/lib.rs index 4b6738f3b915a..b54128044f180 100644 --- a/utils/frame/remote-externalities/src/lib.rs +++ b/utils/frame/remote-externalities/src/lib.rs @@ -315,6 +315,12 @@ impl Builder { key_values.push((key.clone(), value)); if key_values.len() % (10 * BATCH_SIZE) == 0 { let ratio: f64 = key_values.len() as f64 / keys_count as f64; + println!( // TODO: figure out how to get logs from remote in test cases + "progress = {:.2} [{} / {}]", + ratio, + key_values.len(), + keys_count, + ); debug!( target: LOG_TARGET, "progress = {:.2} [{} / {}]", @@ -361,6 +367,12 @@ impl Builder { for f in config.modules.iter() { let hashed_prefix = StorageKey(twox_128(f.as_bytes()).to_vec()); let module_kv = self.rpc_get_pairs_paged(hashed_prefix.clone(), at).await?; + println!( + "downloaded data for module {} (count: {} / prefix: {:?}).", + f, + module_kv.len(), + HexDisplay::from(&hashed_prefix) + ); info!( target: LOG_TARGET, "downloaded data for module {} (count: {} / prefix: {:?}).", From 2e9c6d98383f48cfba5faea7b6395545c33920f3 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Sun, 18 Jul 2021 18:47:40 -0700 Subject: [PATCH 22/26] Some more cleanup to reduce diff --- frame/staking/Cargo.toml | 2 -- frame/staking/src/tests.rs | 3 --- frame/staking/src/voter_bags.rs | 4 +++- utils/frame/remote-externalities/Cargo.toml | 2 +- 4 files changed, 4 insertions(+), 7 deletions(-) diff --git a/frame/staking/Cargo.toml b/frame/staking/Cargo.toml index 57553f31b79eb..8897262431606 100644 --- a/frame/staking/Cargo.toml +++ b/frame/staking/Cargo.toml @@ -48,14 +48,12 @@ sp-storage = { version = "3.0.0", path = "../../primitives/storage" } sp-tracing = { version = "3.0.0", path = "../../primitives/tracing" } sp-core = { version = "3.0.0", path = "../../primitives/core" } sp-npos-elections = { version = "3.0.0", path = "../../primitives/npos-elections", features = ["mocks"] } -sp-state-machine = { version = "0.9.0", path = "../../primitives/state-machine" } pallet-balances = { version = "3.0.0", path = "../balances" } pallet-timestamp = { version = "3.0.0", path = "../timestamp" } pallet-staking-reward-curve = { version = "3.0.0", path = "../staking/reward-curve" } substrate-test-utils = { version = "3.0.0", path = "../../test-utils" } frame-benchmarking = { version = "3.1.0", path = "../benchmarking" } frame-election-provider-support = { version = "3.0.0", features = ["runtime-benchmarks"], path = "../election-provider-support" } - rand = "0.8.4" rand_chacha = { version = "0.2" } parking_lot = "0.11.1" diff --git a/frame/staking/src/tests.rs b/frame/staking/src/tests.rs index 0a2e0513963ca..40bc7b55f66fc 100644 --- a/frame/staking/src/tests.rs +++ b/frame/staking/src/tests.rs @@ -3907,12 +3907,9 @@ fn test_rebag() { // verify preconditions let weight_of = Staking::weight_of_fn(); let node = Node::::from_id(&stash).unwrap(); - - println!("upper node: {:#?}", node); assert_eq!( { let origin_bag = Bag::::get(node.bag_upper).unwrap(); - println!("origin bag: {:#?}", origin_bag); origin_bag.iter().count() }, 1, diff --git a/frame/staking/src/voter_bags.rs b/frame/staking/src/voter_bags.rs index 8afff51121216..b9daaae7cd58a 100644 --- a/frame/staking/src/voter_bags.rs +++ b/frame/staking/src/voter_bags.rs @@ -944,6 +944,7 @@ mod tests { Staking::update_ledger(&controller, &ledger); Staking::do_rebag(voter_id); + // Increase balance to the next threshold. balance += 10; } @@ -1067,7 +1068,8 @@ mod tests { // TODO bags do not get cleaned up from storages - // - is this ok? + // - is this ok? I assume its ok if this is not cleaned just because voters are removed + // but it should be cleaned up if we migrate thresholds assert_eq!(::VoterBags::iter().collect::>().len(), 6); // and the voter list has no one in it. assert_eq!(VoterList::::iter().collect::>().len(), 0); diff --git a/utils/frame/remote-externalities/Cargo.toml b/utils/frame/remote-externalities/Cargo.toml index 5a2065b0387fe..a7519b7e47f33 100644 --- a/utils/frame/remote-externalities/Cargo.toml +++ b/utils/frame/remote-externalities/Cargo.toml @@ -6,7 +6,7 @@ edition = "2018" license = "Apache-2.0" homepage = "https://substrate.dev" repository = "https://github.com/paritytech/substrate/" -description = "An externalities provided environment that can load itself from remote nodes or cache files" +description = "An externalities provided environemnt that can load itself from remote nodes or cache files" readme = "README.md" [package.metadata.docs.rs] From 8deedee20e8bb87a052f6e1471d0540dcbcdf18d Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Sun, 18 Jul 2021 18:56:45 -0700 Subject: [PATCH 23/26] More :clean: --- Cargo.lock | 1 - frame/staking/src/lib.rs | 13 +++++++------ frame/staking/src/mock/voter_bags.rs | 1 - frame/staking/src/voter_bags.rs | 2 +- 4 files changed, 8 insertions(+), 9 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 195bc1a195c1e..1a915db17c878 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5542,7 +5542,6 @@ dependencies = [ "sp-npos-elections", "sp-runtime", "sp-staking", - "sp-state-machine", "sp-std", "sp-storage", "sp-tracing", diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index 8ba3e1bdecfca..95dd9f573ac63 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -274,14 +274,15 @@ #![recursion_limit = "128"] #![cfg_attr(not(feature = "std"), no_std)] -#[cfg(any(feature = "runtime-benchmarks", test))] -pub mod benchmarking; -#[cfg(any(test, feature = "make-bags"))] -pub mod mock; -#[cfg(any(feature = "runtime-benchmarks", test))] -pub mod testing_utils; +#[cfg(test)] +mod mock; #[cfg(test)] mod tests; +#[cfg(any(feature = "runtime-benchmarks", test))] +pub mod testing_utils; +#[cfg(any(feature = "runtime-benchmarks", test))] +pub mod benchmarking; + pub mod inflation; pub mod slashing; diff --git a/frame/staking/src/mock/voter_bags.rs b/frame/staking/src/mock/voter_bags.rs index 0d2044a2d54a7..5ac835afb7f78 100644 --- a/frame/staking/src/mock/voter_bags.rs +++ b/frame/staking/src/mock/voter_bags.rs @@ -32,5 +32,4 @@ pub const CONSTANT_RATIO: f64 = 2.0000000000000000; /// Upper thresholds delimiting the bag list. // pub const THRESHOLDS: [u64; 65] = [5, 15, 25, 50, 100, 1000, 2000, 3000]; // TODO: switch to this -// @kian I am using the below because it is easier to calculate the next threshold in a loop pub const THRESHOLDS: [u64; 9] = [10, 20, 30, 40, 50, 60, 1_000, 2_000, 10_000]; diff --git a/frame/staking/src/voter_bags.rs b/frame/staking/src/voter_bags.rs index b9daaae7cd58a..15fa95a1efc41 100644 --- a/frame/staking/src/voter_bags.rs +++ b/frame/staking/src/voter_bags.rs @@ -524,7 +524,7 @@ impl Bag { assert!(self .tail() .and_then(|tail| Some(tail.next().is_none())) - // if there is no head, then there must not be a tail + // if there is no tail, then there must not be a head .unwrap_or_else(|| self.head.is_none())); // 3) Iterate all voters and ensure that there are no loops. let mut seen_in_bag = BTreeSet::new(); From 8c9c736db2e2ce9d756257c9d38c2cc9e2dce2fc Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Sun, 18 Jul 2021 19:03:02 -0700 Subject: [PATCH 24/26] Mo cleanin --- frame/staking/src/lib.rs | 3 +-- frame/staking/src/mock.rs | 2 +- utils/frame/remote-externalities/src/lib.rs | 12 ------------ 3 files changed, 2 insertions(+), 15 deletions(-) diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index 95dd9f573ac63..c533fcc68d99c 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -274,7 +274,7 @@ #![recursion_limit = "128"] #![cfg_attr(not(feature = "std"), no_std)] -#[cfg(test)] +#[cfg(any(test, feature = "make-bags"))] mod mock; #[cfg(test)] mod tests; @@ -283,7 +283,6 @@ pub mod testing_utils; #[cfg(any(feature = "runtime-benchmarks", test))] pub mod benchmarking; - pub mod inflation; pub mod slashing; pub mod voter_bags; diff --git a/frame/staking/src/mock.rs b/frame/staking/src/mock.rs index 8cffecc4833bb..a584b83c539c8 100644 --- a/frame/staking/src/mock.rs +++ b/frame/staking/src/mock.rs @@ -92,7 +92,7 @@ pub fn is_disabled(controller: AccountId) -> bool { } type UncheckedExtrinsic = frame_system::mocking::MockUncheckedExtrinsic; -pub(crate) type Block = frame_system::mocking::MockBlock; +type Block = frame_system::mocking::MockBlock; frame_support::construct_runtime!( pub enum Test where diff --git a/utils/frame/remote-externalities/src/lib.rs b/utils/frame/remote-externalities/src/lib.rs index b54128044f180..4b6738f3b915a 100644 --- a/utils/frame/remote-externalities/src/lib.rs +++ b/utils/frame/remote-externalities/src/lib.rs @@ -315,12 +315,6 @@ impl Builder { key_values.push((key.clone(), value)); if key_values.len() % (10 * BATCH_SIZE) == 0 { let ratio: f64 = key_values.len() as f64 / keys_count as f64; - println!( // TODO: figure out how to get logs from remote in test cases - "progress = {:.2} [{} / {}]", - ratio, - key_values.len(), - keys_count, - ); debug!( target: LOG_TARGET, "progress = {:.2} [{} / {}]", @@ -367,12 +361,6 @@ impl Builder { for f in config.modules.iter() { let hashed_prefix = StorageKey(twox_128(f.as_bytes()).to_vec()); let module_kv = self.rpc_get_pairs_paged(hashed_prefix.clone(), at).await?; - println!( - "downloaded data for module {} (count: {} / prefix: {:?}).", - f, - module_kv.len(), - HexDisplay::from(&hashed_prefix) - ); info!( target: LOG_TARGET, "downloaded data for module {} (count: {} / prefix: {:?}).", From 46453253a5f8727367da74ee0fd0971e20271f61 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Sun, 18 Jul 2021 19:26:35 -0700 Subject: [PATCH 25/26] small fix --- frame/staking/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index c533fcc68d99c..00458ea9f49b0 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -275,7 +275,7 @@ #![cfg_attr(not(feature = "std"), no_std)] #[cfg(any(test, feature = "make-bags"))] -mod mock; +pub mod mock; #[cfg(test)] mod tests; #[cfg(any(feature = "runtime-benchmarks", test))] From 1f3c154663ae909f70e54a30203d96abb9aa1571 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Wed, 21 Jul 2021 17:41:01 +0200 Subject: [PATCH 26/26] A lot of changes from kian --- frame/staking/src/benchmarking.rs | 6 +- frame/staking/src/lib.rs | 57 +++-- frame/staking/src/mock.rs | 40 ++-- frame/staking/src/mock/voter_bags.rs | 35 --- frame/staking/src/tests.rs | 30 ++- frame/staking/src/voter_bags.rs | 332 +++++++++++++++++++++------ 6 files changed, 357 insertions(+), 143 deletions(-) delete mode 100644 frame/staking/src/mock/voter_bags.rs diff --git a/frame/staking/src/benchmarking.rs b/frame/staking/src/benchmarking.rs index ed90b3f5e3db6..3eeb739371517 100644 --- a/frame/staking/src/benchmarking.rs +++ b/frame/staking/src/benchmarking.rs @@ -671,14 +671,14 @@ benchmarks! { // Clean up any existing state. clear_validators_and_nominators::(); - let threshold = T::VoterBagThresholds::get(); + let thresholds = T::VoterBagThresholds::get(); // stash controls the node account - let bag0_thresh = threshold[0]; + let bag0_thresh = thresholds[0]; let (stash, controller) = make_validator(USER_SEED, bag0_thresh as u32)?; // create another validator with more stake - let bag2_thresh = threshold[2]; + let bag2_thresh = thresholds[2]; let (other_stash, _) = make_validator(USER_SEED + 1, bag2_thresh as u32)?; // update the stash account's value/weight diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index 00458ea9f49b0..f3eb9d68968f2 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -780,6 +780,7 @@ pub mod migrations { log!(info, "Migrating staking to Releases::V8_0_0"); let migrated = VoterList::::regenerate(); + debug_assert_eq!(VoterList::::sanity_check(), Ok(())); StorageVersion::::put(Releases::V8_0_0); log!( @@ -1313,7 +1314,7 @@ pub mod pallet { /// How many voters are registered. #[pallet::storage] - pub(crate) type VoterCount = StorageValue<_, u32, ValueQuery>; + pub(crate) type CounterForVoters = StorageValue<_, u32, ValueQuery>; /// Which bag currently contains a particular voter. /// @@ -1397,32 +1398,62 @@ pub mod pallet { MinNominatorBond::::put(self.min_nominator_bond); MinValidatorBond::::put(self.min_validator_bond); + let mut num_voters: u32 = 0; for &(ref stash, ref controller, balance, ref status) in &self.stakers { + log!( + trace, + "inserting genesis staker: {:?} => {:?} => {:?}", + stash, + balance, + status + ); assert!( T::Currency::free_balance(&stash) >= balance, "Stash does not have enough balance to bond." ); - let _ = >::bond( + + if let Err(why) = >::bond( T::Origin::from(Some(stash.clone()).into()), T::Lookup::unlookup(controller.clone()), balance, RewardDestination::Staked, - ); - let _ = match status { + ) { + // TODO: later on, fix all the tests that trigger these warnings, and + // make these assertions. Genesis stakers should all be correct! + log!(warn, "failed to bond staker at genesis: {:?}.", why); + continue; + } + match status { StakerStatus::Validator => { - >::validate( + if let Err(why) = >::validate( T::Origin::from(Some(controller.clone()).into()), Default::default(), - ) + ) { + log!(warn, "failed to validate staker at genesis: {:?}.", why); + } else { + num_voters +=1 ; + } }, StakerStatus::Nominator(votes) => { - >::nominate( + if let Err(why) = >::nominate( T::Origin::from(Some(controller.clone()).into()), votes.iter().map(|l| T::Lookup::unlookup(l.clone())).collect(), - ) - }, _ => Ok(()) + ) { + log!(warn, "failed to nominate staker at genesis: {:?}.", why); + } else { + num_voters += 1; + } + } + _ => () }; } + + // all voters are inserted sanely. + assert_eq!( + CounterForVoters::::get(), + num_voters, + "not all genesis stakers were inserted into bags, something is wrong." + ); } } @@ -3096,7 +3127,7 @@ impl Pallet { } Nominators::::insert(who, nominations); VoterList::::insert_as(who, VoterType::Nominator); - debug_assert!(VoterCount::::get() == CounterForNominators::::get() + CounterForValidators::::get()); + debug_assert_eq!(VoterList::::sanity_check(), Ok(())); } /// This function will remove a nominator from the `Nominators` storage map, @@ -3112,7 +3143,7 @@ impl Pallet { Nominators::::remove(who); CounterForNominators::::mutate(|x| x.saturating_dec()); VoterList::::remove(who); - debug_assert!(VoterCount::::get() == CounterForNominators::::get() + CounterForValidators::::get()); + debug_assert_eq!(VoterList::::sanity_check(), Ok(())); true } else { false @@ -3133,7 +3164,7 @@ impl Pallet { } Validators::::insert(who, prefs); VoterList::::insert_as(who, VoterType::Validator); - debug_assert!(VoterCount::::get() == CounterForNominators::::get() + CounterForValidators::::get()); + debug_assert_eq!(VoterList::::sanity_check(), Ok(())); } /// This function will remove a validator from the `Validators` storage map, @@ -3149,7 +3180,7 @@ impl Pallet { Validators::::remove(who); CounterForValidators::::mutate(|x| x.saturating_dec()); VoterList::::remove(who); - debug_assert!(VoterCount::::get() == CounterForNominators::::get() + CounterForValidators::::get()); + debug_assert_eq!(VoterList::::sanity_check(), Ok(())); true } else { false diff --git a/frame/staking/src/mock.rs b/frame/staking/src/mock.rs index a584b83c539c8..7c745ac5361ec 100644 --- a/frame/staking/src/mock.rs +++ b/frame/staking/src/mock.rs @@ -17,14 +17,9 @@ //! Test utilities -// This module needs to exist when the `make-bags` feature is enabled so that we can generate the -// appropriate thresholds, but we don't care if it's mostly unused in that case. -#![cfg_attr(feature = "make-bags", allow(unused))] - -mod voter_bags; - -use crate::*; use crate as staking; +use crate::*; +use frame_election_provider_support::onchain; use frame_support::{ assert_ok, parameter_types, traits::{Currency, FindAuthor, Get, OnInitialize, OneSessionHandler}, @@ -39,7 +34,6 @@ use sp_runtime::{ }; use sp_staking::offence::{OffenceDetails, OnOffenceHandler}; use std::{cell::RefCell, collections::HashSet}; -use frame_election_provider_support::onchain; pub const INIT_TIMESTAMP: u64 = 30_000; pub const BLOCK_TIME: u64 = 1000; @@ -249,8 +243,11 @@ impl onchain::Config for Test { type DataProvider = Staking; } +/// Thresholds used for bags. +const THRESHOLDS: [VoteWeight; 9] = [10, 20, 30, 40, 50, 60, 1_000, 2_000, 10_000]; + parameter_types! { - pub const VoterBagThresholds: &'static [VoteWeight] = &voter_bags::THRESHOLDS; + pub const VoterBagThresholds: &'static [VoteWeight] = &THRESHOLDS; } impl Config for Test { @@ -387,14 +384,9 @@ impl ExtBuilder { } fn build(self) -> sp_io::TestExternalities { sp_tracing::try_init_simple(); - let mut storage = frame_system::GenesisConfig::default() - .build_storage::() - .unwrap(); - let balance_factor = if ExistentialDeposit::get() > 1 { - 256 - } else { - 1 - }; + + let mut storage = frame_system::GenesisConfig::default().build_storage::().unwrap(); + let balance_factor = if ExistentialDeposit::get() > 1 { 256 } else { 1 }; let num_validators = self.num_validators.unwrap_or(self.validator_count); // Check that the number of validators is sensible. @@ -511,6 +503,9 @@ fn check_count() { let validator_count = Validators::::iter().count() as u32; assert_eq!(nominator_count, CounterForNominators::::get()); assert_eq!(validator_count, CounterForValidators::::get()); + + let voters_count = CounterForVoters::::get(); + assert_eq!(voters_count, nominator_count + validator_count); } fn check_ledgers() { @@ -839,3 +834,14 @@ pub(crate) fn staking_events() -> Vec> { pub(crate) fn balances(who: &AccountId) -> (Balance, Balance) { (Balances::free_balance(who), Balances::reserved_balance(who)) } + +use crate::voter_bags::Bag; +/// Returns the nodes of all non-empty bags. +pub(crate) fn get_bags() -> Vec<(VoteWeight, Vec)> { + VoterBagThresholds::get().into_iter().filter_map(|t| { + Bag::::get(*t).map(|bag| ( + *t, + bag.iter().map(|n| n.voter().id).collect::>() + )) + }).collect::>() +} diff --git a/frame/staking/src/mock/voter_bags.rs b/frame/staking/src/mock/voter_bags.rs deleted file mode 100644 index 5ac835afb7f78..0000000000000 --- a/frame/staking/src/mock/voter_bags.rs +++ /dev/null @@ -1,35 +0,0 @@ -// This file is part of Substrate. - -// Copyright (C) 2021 Parity Technologies (UK) Ltd. -// SPDX-License-Identifier: Apache-2.0 - -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -//! Autogenerated voter bag thresholds. -//! -//! Generated on 2021-07-05T12:08:52.871368217+00:00 -//! for the test runtime. - -/// Existential weight for this runtime. -#[cfg(any(test, feature = "std"))] -#[allow(unused)] -pub const EXISTENTIAL_WEIGHT: u64 = 1; - -/// Constant ratio between bags for this runtime. -#[cfg(any(test, feature = "std"))] -#[allow(unused)] -pub const CONSTANT_RATIO: f64 = 2.0000000000000000; - -/// Upper thresholds delimiting the bag list. -// pub const THRESHOLDS: [u64; 65] = [5, 15, 25, 50, 100, 1000, 2000, 3000]; // TODO: switch to this -pub const THRESHOLDS: [u64; 9] = [10, 20, 30, 40, 50, 60, 1_000, 2_000, 10_000]; diff --git a/frame/staking/src/tests.rs b/frame/staking/src/tests.rs index 40bc7b55f66fc..15a82e4858b30 100644 --- a/frame/staking/src/tests.rs +++ b/frame/staking/src/tests.rs @@ -148,16 +148,13 @@ fn basic_setup_works() { // New era is not being forced assert_eq!(Staking::force_era(), Forcing::NotForcing); - // genesis accounts must exist in the proper bags - let weight_of = Staking::weight_of_fn(); - // for these stash ids, see - // https://github.com/paritytech/substrate/ - // blob/631d4cdbcad438248c2597213918d8207d85bf6e/frame/staking/src/mock.rs#L435-L441 - for genesis_stash_account_id in &[11, 21, 31, 101] { - let node = crate::voter_bags::Node::::from_id(&genesis_stash_account_id) - .expect(&format!("node was created for account {}", genesis_stash_account_id)); - assert!(!node.is_misplaced(&weight_of)); - } + // check the bags + assert_eq!(CounterForVoters::::get(), 4); + + assert_eq!( + get_bags(), + vec![(10, vec![31]), (1000, vec![11, 21, 101])], + ); }); } @@ -3871,6 +3868,18 @@ fn on_finalize_weight_is_nonzero() { }) } +// end-to-end nodes of the voter bags operation. +mod voter_bags { + + #[test] + fn rebag_works() { + todo!() + } +} +/* +// TODO: this needs some love, retire it in favour of the one above. Use the mock data, don't make +// it complicated with data setup, use the simplest data possible, instead check multiple +// edge-cases. #[test] fn test_rebag() { use crate::{ @@ -3942,6 +3951,7 @@ fn test_rebag() { assert!(!node.is_misplaced(&weight_of), "node must be in proper place after rebag"); }); } +*/ mod election_data_provider { use super::*; diff --git a/frame/staking/src/voter_bags.rs b/frame/staking/src/voter_bags.rs index 15fa95a1efc41..0141f71ef156c 100644 --- a/frame/staking/src/voter_bags.rs +++ b/frame/staking/src/voter_bags.rs @@ -22,7 +22,7 @@ //! voters doesn't particularly matter. use codec::{Decode, Encode}; -use frame_support::{traits::Get, DefaultNoBound}; +use frame_support::{ensure, traits::Get, DefaultNoBound}; use sp_runtime::SaturatedConversion; use sp_std::{ boxed::Box, @@ -78,11 +78,10 @@ pub struct VoterList(PhantomData); impl VoterList { /// Remove all data associated with the voter list from storage. pub fn clear() { - crate::VoterCount::::kill(); + crate::CounterForVoters::::kill(); crate::VoterBagFor::::remove_all(None); crate::VoterBags::::remove_all(None); crate::VoterNodes::::remove_all(None); - debug_assert!(Self::sanity_check()); } /// Regenerate voter data from the `Nominators` and `Validators` storage items. @@ -106,17 +105,18 @@ impl VoterList { /// Decode the length of the voter list. pub fn decode_len() -> Option { - let maybe_len = crate::VoterCount::::try_get().ok().map(|n| n.saturated_into()); + let maybe_len = crate::CounterForVoters::::try_get().ok().map(|n| n.saturated_into()); debug_assert_eq!( maybe_len.unwrap_or_default(), crate::VoterNodes::::iter().count(), "stored length must match count of nodes", ); - // debug_assert_eq!( // TODO: this case will fail in migration pre check - // maybe_len.unwrap_or_default() as u32, - // crate::CounterForNominators::::get() + crate::CounterForValidators::::get(), - // "voter count must be sum of validator and nominator count", - // ); + debug_assert_eq!( + // TODO: this case will fail in migration pre check + maybe_len.unwrap_or_default() as u32, + crate::CounterForNominators::::get() + crate::CounterForValidators::::get(), + "voter count must be sum of validator and nominator count", + ); maybe_len } @@ -177,6 +177,7 @@ impl VoterList { for voter in voters.into_iter() { let weight = weight_of(&voter.id); let bag = notional_bag_for::(weight); + crate::log!(debug, "inserting {:?} into bag {:?}", voter, bag); bags.entry(bag).or_insert_with(|| Bag::::get_or_make(bag)).insert(voter); count += 1; } @@ -185,14 +186,15 @@ impl VoterList { bag.put(); } - crate::VoterCount::::mutate(|prev_count| *prev_count = prev_count.saturating_add(count)); - debug_assert!(Self::sanity_check()); + crate::CounterForVoters::::mutate(|prev_count| { + *prev_count = prev_count.saturating_add(count) + }); count } /// Remove a voter (by id) from the voter list. pub fn remove(voter: &AccountIdOf) { - Self::remove_many(sp_std::iter::once(voter)) + Self::remove_many(sp_std::iter::once(voter)); } /// Remove many voters (by id) from the voter list. @@ -222,9 +224,9 @@ impl VoterList { bag.put(); } - crate::VoterCount::::mutate(|prev_count| *prev_count = prev_count.saturating_sub(count)); - - debug_assert!(Self::sanity_check()); + crate::CounterForVoters::::mutate(|prev_count| { + *prev_count = prev_count.saturating_sub(count) + }); } /// Update a voter's position in the voter list. @@ -352,25 +354,52 @@ impl VoterList { }, "all `bag_upper` in storage must be members of the new thresholds", ); - debug_assert!(Self::sanity_check()); num_affected } - // Sanity check the voter list. - fn sanity_check() -> bool { - // Check: - // 1) Iterate all voters in list and make sure there are no duplicates. + /// Sanity check the voter list. + /// + /// This should be called from the call-site, whenever one of the mutating apis (e.g. `insert`) + /// is being used, after all other staking data (such as counter) has been updated. It checks + /// that: + /// + /// * Iterate all voters in list and make sure there are no duplicates. + /// * Iterate all voters and ensure their count is in sync with `CounterForVoters`. + /// * Ensure `CounterForVoters` is `CounterForValidators + CounterForNominators`. + /// * Sanity-checks all bags. This will cascade down all the checks and makes sure all bags are + /// checked per *any* update to `VoterList`. + pub(super) fn sanity_check() -> Result<(), String> { let mut seen_in_list = BTreeSet::new(); - assert!(Self::iter().map(|node| node.voter.id).all(|voter| seen_in_list.insert(voter))); - // 2) Iterate all voters and ensure their count is in sync with `VoterCount`. - assert_eq!(Self::iter().collect::>().len() as u32, crate::VoterCount::::get()); + ensure!( + Self::iter().map(|node| node.voter.id).all(|voter| seen_in_list.insert(voter)), + String::from("duplicate identified") + ); + + let iter_count = Self::iter().collect::>().len() as u32; + let stored_count = crate::CounterForVoters::::get(); + ensure!( + iter_count == stored_count, + format!("iter_count ({}) != voter_count ({})", iter_count, stored_count), + ); - // NOTE: we don't check `CounterForNominators + CounterForValidators == VoterCount` - // because those are updated in lib.rs, and thus tests for just this module do - // not update them. + let validators = crate::CounterForValidators::::get(); + let nominators = crate::CounterForNominators::::get(); + ensure!( + validators + nominators == stored_count, + format!( + "validators {} + nominators {} != voters {}", + validators, nominators, stored_count + ), + ); - true // use bool so it plays friendly with debug_assert + let _ = T::VoterBagThresholds::get() + .into_iter() + .map(|t| Bag::::get(*t).unwrap_or_default()) + .map(|b| b.sanity_check()) + .collect::>()?; + + Ok(()) } } @@ -476,8 +505,6 @@ impl Bag { self.tail = Some(id.clone()); crate::VoterBagFor::::insert(id, self.bag_upper); - - debug_assert!(self.sanity_check()); } /// Remove a voter node from this bag. @@ -507,34 +534,45 @@ impl Bag { if self.tail.as_ref() == Some(&node.voter.id) { self.tail = node.prev.clone(); } - - debug_assert!(self.sanity_check()); } - // Sanity check this bag. - fn sanity_check(&self) -> bool { - // Check: - // 1) Iterate all voters and ensure only those that are head don't have a prev. - assert!(self - .head() - .and_then(|head| Some(head.prev().is_none())) - // if there is no head, then there must not be a tail - .unwrap_or_else(|| self.tail.is_none())); - // 2) Iterate all voters and ensure only those that are tail don't have a next. - assert!(self - .tail() - .and_then(|tail| Some(tail.next().is_none())) - // if there is no tail, then there must not be a head - .unwrap_or_else(|| self.head.is_none())); - // 3) Iterate all voters and ensure that there are no loops. + /// Sanity check this bag. + /// + /// Should be called by the call-site, after each mutating operation on a bag. The call site of + /// this struct is always `VoterList`. + /// + /// * Ensures head has no prev. + /// * Ensures tail has no next. + /// * Ensures there are no loops, traversal from head to tail is correct. + fn sanity_check(&self) -> Result<(), String> { + ensure!( + self.head() + .map(|head| head.prev().is_none()) + // if there is no head, then there must not be a tail, meaning that the bag is + // empty. + .unwrap_or_else(|| self.tail.is_none()), + String::from("head has a prev") + ); + + ensure!( + self.tail() + .map(|tail| tail.next().is_none()) + // if there is no tail, then there must not be a head, meaning that the bag is + // empty. + .unwrap_or_else(|| self.head.is_none()), + String::from("tail has a next") + ); + let mut seen_in_bag = BTreeSet::new(); - assert!(self - .iter() - .map(|node| node.voter.id) - // each voter is only seen once, thus there is no cycle within a bag - .all(|voter| seen_in_bag.insert(voter))); + ensure!( + self.iter() + .map(|node| node.voter.id) + // each voter is only seen once, thus there is no cycle within a bag + .all(|voter| seen_in_bag.insert(voter)), + String::from("Duplicate found in bag.") + ); - true // use bool so it plays nice with debug assert ? + Ok(()) } } @@ -651,6 +689,12 @@ impl Node { let current_weight = weight_of(&self.voter.id); notional_bag_for::(current_weight) } + + #[cfg(any(test, feature = "runtime-benchmarks"))] + /// Get the underlying voter. + pub fn voter(&self) -> &Voter { + &self.voter + } } /// Fundamental information about a voter. @@ -898,24 +942,181 @@ pub mod make_bags { } } +// This is the highest level of abstraction provided by this module. More generic tests are here, +// among those related to `VoterList` struct. #[cfg(test)] -mod tests { +mod voter_list { use frame_support::traits::Currency; - use substrate_test_utils::assert_eq_uvec; - use super::*; use crate::mock::*; - const GENESIS_VOTER_IDS: [u64; 5] = [11, 21, 31, 41, 101]; + #[test] + fn basic_setup_works() { + // make sure ALL relevant data structures are setup correctly. + // TODO: we are not checking all of them yet. + ExtBuilder::default().build_and_execute(|| { + assert_eq!(crate::CounterForVoters::::get(), 4); + + let weight_of = Staking::weight_of_fn(); + assert_eq!(weight_of(&11), 1000); + assert_eq!(VoterBagFor::::get(11).unwrap(), 1000); + + assert_eq!(weight_of(&21), 1000); + assert_eq!(VoterBagFor::::get(21).unwrap(), 1000); + + assert_eq!(weight_of(&31), 1); + assert_eq!(VoterBagFor::::get(31).unwrap(), 10); + + assert_eq!(VoterBagFor::::get(41), None); // this staker is chilled! + + assert_eq!(weight_of(&101), 500); + assert_eq!(VoterBagFor::::get(101).unwrap(), 1000); + + // iteration of the bags would yield: + assert_eq!( + VoterList::::iter().map(|n| n.voter().id).collect::>(), + vec![11, 21, 101, 31], + // ^^ note the order of insertion in genesis! + ); + + assert_eq!( + get_bags(), + vec![(10, vec![31]), (1000, vec![11, 21, 101])], + ); + }) + } #[test] - fn voter_list_includes_genesis_accounts() { - ExtBuilder::default().validator_pool(true).build_and_execute(|| { - let have_voters: Vec<_> = VoterList::::iter().map(|node| node.voter.id).collect(); - assert_eq_uvec!(GENESIS_VOTER_IDS, have_voters); - }); + fn notional_bag_for_works() { + todo!(); + } + + #[test] + fn iteration_is_semi_sorted() { + ExtBuilder::default().build_and_execute(|| { + // add some new validators to the genesis state. + bond_validator(51, 50, 2000); + bond_validator(61, 60, 2000); + + // given + assert_eq!( + get_bags(), + vec![(10, vec![31]), (1000, vec![11, 21, 101]), (2000, vec![51, 61])], + ); + + // when + let iteration = VoterList::::iter().map(|node| node.voter.id).collect::>(); + + // then + assert_eq!(iteration, vec![ + 51, 61, // best bag + 11, 21, 101, // middle bag + 31, // last bag. + ]); + }) + } + + /// This tests that we can `take` x voters, even if that quantity ends midway through a list. + #[test] + fn take_works() { + ExtBuilder::default().build_and_execute(|| { + // add some new validators to the genesis state. + bond_validator(51, 50, 2000); + bond_validator(61, 60, 2000); + + // given + assert_eq!( + get_bags(), + vec![(10, vec![31]), (1000, vec![11, 21, 101]), (2000, vec![51, 61])], + ); + + // when + let iteration = VoterList::::iter() + .map(|node| node.voter.id) + .take(4) + .collect::>(); + + // then + assert_eq!(iteration, vec![ + 51, 61, // best bag, fully iterated + 11, 21, // middle bag, partially iterated + ]); + }) + } + + #[test] + fn storage_is_cleaned_up_as_voters_are_removed() {} + + #[test] + fn insert_works() { + todo!() + } + + #[test] + fn insert_as_works() { + // insert a new one with role + // update the status of already existing one. + todo!() + } + + #[test] + fn remove_works() { + todo!() + } + + #[test] + fn update_position_for_works() { + // alter the genesis state to require a re-bag, then ensure this fixes it. Might be similar + // `rebag_works()` + todo!(); + } +} + +#[cfg(test)] +mod bags { + #[test] + fn get_works() { + todo!() + } + + #[test] + fn insert_works() { + todo!() + } + + #[test] + fn remove_works() { + todo!() + } +} + +#[cfg(test)] +mod voter_node { + #[test] + fn get_voting_data_works() { + todo!() } + #[test] + fn is_misplaced_works() { + todo!() + } +} + +// TODO: I've created simpler versions of these tests above. We can probably remove the ones below +// now. Peter was likely not very familiar with the staking mock and he came up with these rather +// complicated test setups. Please see my versions above, we can test the same properties, easily, +// without the need to alter the stakers so much. +/* +#[cfg(test)] +mod tests { + use frame_support::traits::Currency; + + use super::*; + use crate::mock::*; + + const GENESIS_VOTER_IDS: [u64; 5] = [11, 21, 31, 41, 101]; + /// This tests the property that when iterating through the `VoterList`, we iterate from higher /// bags to lower. #[test] @@ -1051,20 +1252,20 @@ mod tests { let genesis_voters = vec![101, 41, 31, 21, 11]; voter_list_storage_items_eq(genesis_voters); - assert_eq!(::VoterCount::get(), 5); + assert_eq!(::CounterForVoters::get(), 5); // Remove 1 voter, VoterList::::remove(&101); let remaining_voters = vec![41, 31, 21, 11]; // and assert they have been cleaned up. voter_list_storage_items_eq(remaining_voters.clone()); - assert_eq!(::VoterCount::get(), 4); + assert_eq!(::CounterForVoters::get(), 4); // Now remove the remaining voters so we have 0 left, remaining_voters.iter().for_each(|v| VoterList::::remove(v)); // and assert all of them have been cleaned up. voter_list_storage_items_eq(vec![]); - assert_eq!(::VoterCount::get(), 0); + assert_eq!(::CounterForVoters::get(), 0); // TODO bags do not get cleaned up from storages @@ -1078,3 +1279,4 @@ mod tests { }); } } +*/