From 11d81588d7d930c514f139cc655c2030762f7468 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Thu, 22 Jul 2021 16:21:57 -0700 Subject: [PATCH 01/28] impl notional_bag_for_works --- bin/node/runtime/voter-bags/src/main.rs | 6 ++---- frame/staking/src/lib.rs | 4 ++-- frame/staking/src/voter_bags.rs | 17 +++++++++++++++-- 3 files changed, 19 insertions(+), 8 deletions(-) diff --git a/bin/node/runtime/voter-bags/src/main.rs b/bin/node/runtime/voter-bags/src/main.rs index 1340285c29a1a..a92af37fb5bf8 100644 --- a/bin/node/runtime/voter-bags/src/main.rs +++ b/bin/node/runtime/voter-bags/src/main.rs @@ -17,7 +17,7 @@ //! Make the set of voting bag thresholds to be used in `voter_bags.rs`. -use pallet_staking::{voter_bags::make_bags::generate_thresholds_module}; +use pallet_staking::voter_bags::make_bags::generate_thresholds_module; use std::path::PathBuf; use structopt::StructOpt; @@ -34,7 +34,5 @@ struct Opt { fn main() -> Result<(), std::io::Error> { let Opt { n_bags, output } = Opt::from_args(); let mut ext = sp_io::TestExternalities::new_empty(); - ext.execute_with(|| { - generate_thresholds_module::(n_bags, &output) - }) + ext.execute_with(|| generate_thresholds_module::(n_bags, &output)) } diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index 4ac2b7404d0f3..6073635732aab 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -273,10 +273,10 @@ #![cfg_attr(not(feature = "std"), no_std)] -#[cfg(any(feature = "runtime-benchmarks", test))] -pub mod testing_utils; #[cfg(any(feature = "runtime-benchmarks", test))] pub mod benchmarking; +#[cfg(any(feature = "runtime-benchmarks", test))] +pub mod testing_utils; #[cfg(test)] pub(crate) mod mock; diff --git a/frame/staking/src/voter_bags.rs b/frame/staking/src/voter_bags.rs index d5bae70b29f46..ca31c37da2a3f 100644 --- a/frame/staking/src/voter_bags.rs +++ b/frame/staking/src/voter_bags.rs @@ -673,8 +673,8 @@ impl Node { notional_bag_for::(current_weight) } - #[cfg(any(test, feature = "runtime-benchmarks"))] /// Get the underlying voter. + #[cfg(any(test, feature = "runtime-benchmarks"))] pub fn voter(&self) -> &Voter { &self.voter } @@ -963,7 +963,20 @@ mod voter_list { #[test] fn notional_bag_for_works() { - todo!(); + // under a threshold gives the next threshold. + assert_eq!(notional_bag_for::(0), 10); + assert_eq!(notional_bag_for::(9), 10); + assert_eq!(notional_bag_for::(11), 20); + + // at a threshold gives the threshold. + assert_eq!(notional_bag_for::(10), 10); + + let max_explicit_threshold = *::VoterBagThresholds::get().last().unwrap(); + assert_eq!(max_explicit_threshold, 10_000); + // if the max explicit threshold is less than VoteWeight::MAX, + assert!(VoteWeight::MAX > max_explicit_threshold); + // anything above it will have belong to the threshold VoteWeight::MAX. + assert_eq!(notional_bag_for::(max_explicit_threshold + 1), VoteWeight::MAX); } #[test] From 25a16dc5f8da02d63838b9de9941bf215e4eb221 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Thu, 22 Jul 2021 20:20:08 -0700 Subject: [PATCH 02/28] Add tests: insert_as_works & insert_works --- frame/staking/src/mock.rs | 30 ++++-- frame/staking/src/voter_bags.rs | 174 +++++++++++++------------------- 2 files changed, 91 insertions(+), 113 deletions(-) diff --git a/frame/staking/src/mock.rs b/frame/staking/src/mock.rs index c16d49f7dce16..5abbf14f46a75 100644 --- a/frame/staking/src/mock.rs +++ b/frame/staking/src/mock.rs @@ -503,13 +503,21 @@ fn post_conditions() { } fn check_count() { - let nominator_count = Nominators::::iter().count() as u32; - let validator_count = Validators::::iter().count() as u32; - assert_eq!(nominator_count, CounterForNominators::::get()); - assert_eq!(validator_count, CounterForValidators::::get()); + // @kianenigma + // TODO: checking this is good for tests of top level staking api, but when + // unit testing parts of voter_bags we can't be expecting this to update properly + // (unless we manually do it). + // Instead I think the debug_asserts should be enough? + // Otherwise maybe we can have a `build_and_execute_without_check_count`, but that + // is just more code. - let voters_count = CounterForVoters::::get(); - assert_eq!(voters_count, nominator_count + validator_count); + // let nominator_count = Nominators::::iter().count() as u32; + // 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() { @@ -602,10 +610,14 @@ pub(crate) fn current_era() -> EraIndex { Staking::current_era().unwrap() } -pub(crate) fn bond_validator(stash: AccountId, ctrl: AccountId, val: Balance) { +pub(crate) fn bond(stash: AccountId, ctrl: AccountId, val: Balance) { let _ = Balances::make_free_balance_be(&stash, val); let _ = Balances::make_free_balance_be(&ctrl, val); assert_ok!(Staking::bond(Origin::signed(stash), ctrl, val, RewardDestination::Controller)); +} + +pub(crate) fn bond_validator(stash: AccountId, ctrl: AccountId, val: Balance) { + bond(stash, ctrl, val); assert_ok!(Staking::validate(Origin::signed(ctrl), ValidatorPrefs::default())); } @@ -615,9 +627,7 @@ pub(crate) fn bond_nominator( val: Balance, target: Vec, ) { - let _ = Balances::make_free_balance_be(&stash, val); - let _ = Balances::make_free_balance_be(&ctrl, val); - assert_ok!(Staking::bond(Origin::signed(stash), ctrl, val, RewardDestination::Controller)); + bond(stash, ctrl, val); assert_ok!(Staking::nominate(Origin::signed(ctrl), target)); } diff --git a/frame/staking/src/voter_bags.rs b/frame/staking/src/voter_bags.rs index ca31c37da2a3f..35786cc34543d 100644 --- a/frame/staking/src/voter_bags.rs +++ b/frame/staking/src/voter_bags.rs @@ -968,7 +968,7 @@ mod voter_list { assert_eq!(notional_bag_for::(9), 10); assert_eq!(notional_bag_for::(11), 20); - // at a threshold gives the threshold. + // at a threshold gives that threshold. assert_eq!(notional_bag_for::(10), 10); let max_explicit_threshold = *::VoterBagThresholds::get().last().unwrap(); @@ -1037,18 +1037,83 @@ mod voter_list { } #[test] - fn storage_is_cleaned_up_as_voters_are_removed() {} + fn storage_is_cleaned_up_as_voters_are_removed() { + todo!() + } #[test] fn insert_works() { - todo!() + ExtBuilder::default().build_and_execute(|| { + assert_eq!( + VoterList::::iter().map(|n| n.voter().id).collect::>(), + vec![11, 21, 101, 31] + ); + assert_eq!(get_bags(), vec![(10, vec![31]), (1_000, vec![11, 21, 101])]); + + // Insert into an existing bag: + // when + bond(42, 43, 999); + VoterList::::insert(Voter::<_>::nominator(42), Pallet::::weight_of_fn()); + + // then + assert_eq!( + VoterList::::iter().map(|n| n.voter().id).collect::>(), + vec![11, 21, 101, 42, 31] + ); + assert_eq!(get_bags(), vec![(10, vec![31]), (1_000, vec![11, 21, 101, 42])]); + + // TODO maybe this is overkill since its not api behavior driven testing and instead + // should be tested on the bags abstraction level + // Insert into a non-existent bag: + // when + bond(422, 433, 1_001); + VoterList::::insert(Voter::<_>::nominator(422), Pallet::::weight_of_fn()); + + // then + assert_eq!( + VoterList::::iter().map(|n| n.voter().id).collect::>(), + vec![422, 11, 21, 101, 88, 31] + ); + assert_eq!( + get_bags(), + vec![(10, vec![31]), (1_000, vec![11, 21, 101, 88]), (2_000, vec![422])] + ); + }); } #[test] fn insert_as_works() { - // insert a new one with role - // update the status of already existing one. - todo!() + ExtBuilder::default().build_and_execute(|| { + let actual = + VoterList::::iter().map(|node| node.voter().clone()).collect::>(); + let mut expected: Vec> = vec![ + Voter::<_>::validator(11), + Voter::<_>::validator(21), + Voter::<_>::nominator(101), + Voter::<_>::validator(31), + ]; + assert_eq!(actual, expected); + + // Insert a new account with role: + // when + VoterList::::insert_as(&42, VoterType::Nominator); + + // then + let actual = + VoterList::::iter().map(|node| node.voter().clone()).collect::>(); + expected.push(Voter::<_>::nominator(42)); + assert_eq!(actual, expected); + + // Update the status of already existing one: + // when + VoterList::::insert_as(&42, VoterType::Validator); + + // then + let actual = + VoterList::::iter().map(|node| node.voter().clone()).collect::>(); + expected[4] = Voter::<_>::validator(42); + assert_eq!(actual, expected); + }); } #[test] @@ -1100,103 +1165,6 @@ mod voter_node { // 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] - 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); - - // Increase balance to the next threshold. - balance += 10; - } - - 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 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 - // 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(|| { From 9b55e10c58e208cda477a4e5794c2149c08fc853 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Thu, 22 Jul 2021 20:36:43 -0700 Subject: [PATCH 03/28] Impl test: remove_works --- frame/staking/src/voter_bags.rs | 48 +++++++++++++++++++++++++++++++-- 1 file changed, 46 insertions(+), 2 deletions(-) diff --git a/frame/staking/src/voter_bags.rs b/frame/staking/src/voter_bags.rs index 35786cc34543d..d8e89340703a2 100644 --- a/frame/staking/src/voter_bags.rs +++ b/frame/staking/src/voter_bags.rs @@ -1044,10 +1044,13 @@ mod voter_list { #[test] fn insert_works() { ExtBuilder::default().build_and_execute(|| { + // given assert_eq!( VoterList::::iter().map(|n| n.voter().id).collect::>(), vec![11, 21, 101, 31] ); + // TODO maybe checking the actual bags here is overkill since this is aimed at VoterList + // api and not bags? (same Q for other tests here) assert_eq!(get_bags(), vec![(10, vec![31]), (1_000, vec![11, 21, 101])]); // Insert into an existing bag: @@ -1063,7 +1066,7 @@ mod voter_list { assert_eq!(get_bags(), vec![(10, vec![31]), (1_000, vec![11, 21, 101, 42])]); // TODO maybe this is overkill since its not api behavior driven testing and instead - // should be tested on the bags abstraction level + // should be tested on the bags abstraction level? (same Q for other tests here) // Insert into a non-existent bag: // when bond(422, 433, 1_001); @@ -1084,6 +1087,7 @@ mod voter_list { #[test] fn insert_as_works() { ExtBuilder::default().build_and_execute(|| { + // give let actual = VoterList::::iter().map(|node| node.voter().clone()).collect::>(); let mut expected: Vec> = vec![ @@ -1118,7 +1122,47 @@ mod voter_list { #[test] fn remove_works() { - todo!() + ExtBuilder::default().build_and_execute(|| { + // given + assert_eq!( + VoterList::::iter().map(|n| n.voter().id).collect::>(), + vec![11, 21, 101, 31] + ); + assert_eq!(get_bags(), vec![(10, vec![31]), (1_000, vec![11, 21, 101])]); + + // Removing non-existent voter does not change anything: + // when + VoterList::::remove(&42); + + // then + assert_eq!( + VoterList::::iter().map(|n| n.voter().id).collect::>(), + vec![11, 21, 101, 31] + ); + assert_eq!(get_bags(), vec![(10, vec![31]), (1_000, vec![11, 21, 101])]); + + // Remove from a bag with multiple nodes: + // when + VoterList::::remove(&11); + + // then + assert_eq!( + VoterList::::iter().map(|n| n.voter().id).collect::>(), + vec![21, 101, 31] + ); + assert_eq!(get_bags(), vec![(10, vec![31]), (1_000, vec![21, 101])]); + + + // Remove from a bag with only one node: + VoterList::::remove(&31); + + // then + assert_eq!( + VoterList::::iter().map(|n| n.voter().id).collect::>(), + vec![21, 101] + ); + assert_eq!(get_bags(), vec![(10, vec![]), (1_000, vec![21, 101])]); + }); } #[test] From f3222d736cf1ac4d5da38f399d4c44e963b94220 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Thu, 22 Jul 2021 20:58:41 -0700 Subject: [PATCH 04/28] Trivial cleaning --- frame/staking/src/mock.rs | 4 ++-- frame/staking/src/voter_bags.rs | 15 +++++++-------- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/frame/staking/src/mock.rs b/frame/staking/src/mock.rs index 5abbf14f46a75..45bf86df91a81 100644 --- a/frame/staking/src/mock.rs +++ b/frame/staking/src/mock.rs @@ -505,8 +505,8 @@ fn post_conditions() { fn check_count() { // @kianenigma // TODO: checking this is good for tests of top level staking api, but when - // unit testing parts of voter_bags we can't be expecting this to update properly - // (unless we manually do it). + // unit testing parts of voter_bags we can't expect this to update properly + // (unless we manually do it in the test, which might be ok?). // Instead I think the debug_asserts should be enough? // Otherwise maybe we can have a `build_and_execute_without_check_count`, but that // is just more code. diff --git a/frame/staking/src/voter_bags.rs b/frame/staking/src/voter_bags.rs index d8e89340703a2..b322a0f9ad179 100644 --- a/frame/staking/src/voter_bags.rs +++ b/frame/staking/src/voter_bags.rs @@ -975,7 +975,7 @@ mod voter_list { assert_eq!(max_explicit_threshold, 10_000); // if the max explicit threshold is less than VoteWeight::MAX, assert!(VoteWeight::MAX > max_explicit_threshold); - // anything above it will have belong to the threshold VoteWeight::MAX. + // anything above it will belong to the VoteWeight::MAX bag. assert_eq!(notional_bag_for::(max_explicit_threshold + 1), VoteWeight::MAX); } @@ -1065,7 +1065,7 @@ mod voter_list { ); assert_eq!(get_bags(), vec![(10, vec![31]), (1_000, vec![11, 21, 101, 42])]); - // TODO maybe this is overkill since its not api behavior driven testing and instead + // TODO maybe this scenario is overkill and instead // should be tested on the bags abstraction level? (same Q for other tests here) // Insert into a non-existent bag: // when @@ -1087,7 +1087,7 @@ mod voter_list { #[test] fn insert_as_works() { ExtBuilder::default().build_and_execute(|| { - // give + // given let actual = VoterList::::iter().map(|node| node.voter().clone()).collect::>(); let mut expected: Vec> = vec![ @@ -1098,7 +1098,7 @@ mod voter_list { ]; assert_eq!(actual, expected); - // Insert a new account with role: + // Insert a new voter with role: // when VoterList::::insert_as(&42, VoterType::Nominator); @@ -1108,7 +1108,7 @@ mod voter_list { expected.push(Voter::<_>::nominator(42)); assert_eq!(actual, expected); - // Update the status of already existing one: + // Update the voter type of an already existing voter: // when VoterList::::insert_as(&42, VoterType::Validator); @@ -1130,11 +1130,11 @@ mod voter_list { ); assert_eq!(get_bags(), vec![(10, vec![31]), (1_000, vec![11, 21, 101])]); - // Removing non-existent voter does not change anything: + // Remove a non-existent voter: // when VoterList::::remove(&42); - // then + // then nothing changes assert_eq!( VoterList::::iter().map(|n| n.voter().id).collect::>(), vec![11, 21, 101, 31] @@ -1152,7 +1152,6 @@ mod voter_list { ); assert_eq!(get_bags(), vec![(10, vec![31]), (1_000, vec![21, 101])]); - // Remove from a bag with only one node: VoterList::::remove(&31); From a45e761360d1eb2b7332cc5f06c452e467bcf9e4 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Fri, 23 Jul 2021 13:38:37 -0700 Subject: [PATCH 05/28] Add test: update_position_for_works --- frame/staking/src/mock.rs | 40 +++++---- frame/staking/src/voter_bags.rs | 144 +++++++++++++++++++++++--------- 2 files changed, 128 insertions(+), 56 deletions(-) diff --git a/frame/staking/src/mock.rs b/frame/staking/src/mock.rs index 45bf86df91a81..0910bb9dcc43d 100644 --- a/frame/staking/src/mock.rs +++ b/frame/staking/src/mock.rs @@ -493,31 +493,33 @@ impl ExtBuilder { ext.execute_with(test); ext.execute_with(post_conditions); } + /// WARNING: This should only be use for testing `VoterList` api or lower. + pub fn build_and_execute_without_check_count(self, test: impl FnOnce() -> ()) { + let mut ext = self.build(); + ext.execute_with(test); + ext.execute_with(post_conditions_without_check_count); + } } fn post_conditions() { + post_conditions_without_check_count(); + check_count(); +} + +fn post_conditions_without_check_count() { check_nominators(); check_exposures(); check_ledgers(); - check_count(); } fn check_count() { - // @kianenigma - // TODO: checking this is good for tests of top level staking api, but when - // unit testing parts of voter_bags we can't expect this to update properly - // (unless we manually do it in the test, which might be ok?). - // Instead I think the debug_asserts should be enough? - // Otherwise maybe we can have a `build_and_execute_without_check_count`, but that - // is just more code. - - // let nominator_count = Nominators::::iter().count() as u32; - // let validator_count = Validators::::iter().count() as u32; - // assert_eq!(nominator_count, CounterForNominators::::get()); - // assert_eq!(validator_count, CounterForValidators::::get()); + let nominator_count = Nominators::::iter().count() as u32; + 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); + let voters_count = CounterForVoters::::get(); + assert_eq!(voters_count, nominator_count + validator_count); } fn check_ledgers() { @@ -843,3 +845,11 @@ pub(crate) fn get_bags() -> Vec<(VoteWeight, Vec)> { }) .collect::>() } + +pub(crate) fn get_voter_list_as_ids() -> Vec { + VoterList::::iter().map(|n| n.voter().id).collect::>() +} + +pub(crate) fn get_voter_list_as_voters() -> Vec> { + VoterList::::iter().map(|node| node.voter().clone()).collect::>() +} diff --git a/frame/staking/src/voter_bags.rs b/frame/staking/src/voter_bags.rs index f831c6becfd7f..20c9cd3896474 100644 --- a/frame/staking/src/voter_bags.rs +++ b/frame/staking/src/voter_bags.rs @@ -260,7 +260,12 @@ impl VoterList { let new_idx = notional_bag_for::(weight_of(&node.voter.id)); node.bag_upper = new_idx; let mut bag = Bag::::get_or_make(node.bag_upper); + println!("update_position_fr bag BEFORe insert: {:#?}", bag); + bag.insert_node(node); + + // TODO remove this + println!("update_position_fr bag AFTER insert: {:#?}", bag); bag.put(); (old_idx, new_idx) @@ -471,8 +476,14 @@ impl Bag { fn insert_node(&mut self, mut node: Node) { let id = node.voter.id.clone(); + // if let Some(tail_id) = self.tail { + // debug_assert!(id != tail_id, "Should never insert a node into a bag where it already exists") + // + // } else { + node.prev = self.tail.clone(); node.prev = self.tail.clone(); node.next = None; + println!("insert_node before node.put() (id {:#?}) {:#?}", node.voter.id, node); node.put(); // update the previous tail @@ -924,6 +935,7 @@ pub mod make_bags { mod voter_list { use super::*; use crate::mock::*; + use frame_support::{assert_ok, assert_storage_noop, traits::Currency}; #[test] fn basic_setup_works() { @@ -1040,12 +1052,9 @@ mod voter_list { #[test] fn insert_works() { - ExtBuilder::default().build_and_execute(|| { + ExtBuilder::default().build_and_execute_without_check_count(|| { // given - assert_eq!( - VoterList::::iter().map(|n| n.voter().id).collect::>(), - vec![11, 21, 101, 31] - ); + assert_eq!(get_voter_list_as_ids(), vec![11, 21, 101, 31]); // TODO maybe checking the actual bags here is overkill since this is aimed at VoterList // api and not bags? (same Q for other tests here) assert_eq!(get_bags(), vec![(10, vec![31]), (1_000, vec![11, 21, 101])]); @@ -1056,37 +1065,28 @@ mod voter_list { VoterList::::insert(Voter::<_>::nominator(42), Pallet::::weight_of_fn()); // then - assert_eq!( - VoterList::::iter().map(|n| n.voter().id).collect::>(), - vec![11, 21, 101, 42, 31] - ); + assert_eq!(get_voter_list_as_ids(), vec![11, 21, 101, 42, 31]); assert_eq!(get_bags(), vec![(10, vec![31]), (1_000, vec![11, 21, 101, 42])]); - // TODO maybe this scenario is overkill and instead - // should be tested on the bags abstraction level? (same Q for other tests here) // Insert into a non-existent bag: // when bond(422, 433, 1_001); VoterList::::insert(Voter::<_>::nominator(422), Pallet::::weight_of_fn()); // then - assert_eq!( - VoterList::::iter().map(|n| n.voter().id).collect::>(), - vec![422, 11, 21, 101, 88, 31] - ); + assert_eq!(get_voter_list_as_ids(), vec![422, 11, 21, 101, 42, 31]); assert_eq!( get_bags(), - vec![(10, vec![31]), (1_000, vec![11, 21, 101, 88]), (2_000, vec![422])] + vec![(10, vec![31]), (1_000, vec![11, 21, 101, 42]), (2_000, vec![422])] ); }); } #[test] fn insert_as_works() { - ExtBuilder::default().build_and_execute(|| { + ExtBuilder::default().build_and_execute_without_check_count(|| { // given - let actual = - VoterList::::iter().map(|node| node.voter().clone()).collect::>(); + let actual = get_voter_list_as_voters(); let mut expected: Vec> = vec![ Voter::<_>::validator(11), Voter::<_>::validator(21), @@ -1100,8 +1100,7 @@ mod voter_list { VoterList::::insert_as(&42, VoterType::Nominator); // then - let actual = - VoterList::::iter().map(|node| node.voter().clone()).collect::>(); + let actual = get_voter_list_as_voters(); expected.push(Voter::<_>::nominator(42)); assert_eq!(actual, expected); @@ -1110,8 +1109,7 @@ mod voter_list { VoterList::::insert_as(&42, VoterType::Validator); // then - let actual = - VoterList::::iter().map(|node| node.voter().clone()).collect::>(); + let actual = get_voter_list_as_voters(); expected[4] = Voter::<_>::validator(42); assert_eq!(actual, expected); }); @@ -1119,12 +1117,9 @@ mod voter_list { #[test] fn remove_works() { - ExtBuilder::default().build_and_execute(|| { + ExtBuilder::default().build_and_execute_without_check_count(|| { // given - assert_eq!( - VoterList::::iter().map(|n| n.voter().id).collect::>(), - vec![11, 21, 101, 31] - ); + assert_eq!(get_voter_list_as_ids(), vec![11, 21, 101, 31]); assert_eq!(get_bags(), vec![(10, vec![31]), (1_000, vec![11, 21, 101])]); // Remove a non-existent voter: @@ -1132,10 +1127,7 @@ mod voter_list { VoterList::::remove(&42); // then nothing changes - assert_eq!( - VoterList::::iter().map(|n| n.voter().id).collect::>(), - vec![11, 21, 101, 31] - ); + assert_eq!(get_voter_list_as_ids(), vec![11, 21, 101, 31]); assert_eq!(get_bags(), vec![(10, vec![31]), (1_000, vec![11, 21, 101])]); // Remove from a bag with multiple nodes: @@ -1143,21 +1135,17 @@ mod voter_list { VoterList::::remove(&11); // then - assert_eq!( - VoterList::::iter().map(|n| n.voter().id).collect::>(), - vec![21, 101, 31] - ); + assert_eq!(get_voter_list_as_ids(), vec![21, 101, 31]); assert_eq!(get_bags(), vec![(10, vec![31]), (1_000, vec![21, 101])]); // Remove from a bag with only one node: VoterList::::remove(&31); // then - assert_eq!( - VoterList::::iter().map(|n| n.voter().id).collect::>(), - vec![21, 101] - ); + assert_eq!(get_voter_list_as_ids(), vec![21, 101]); assert_eq!(get_bags(), vec![(10, vec![]), (1_000, vec![21, 101])]); + + // TODO check storage is cleaned up }); } @@ -1165,7 +1153,81 @@ mod voter_list { 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!(); + + ExtBuilder::default().build_and_execute_without_check_count(|| { + // given a correctly placed account 31 + assert_eq!(get_voter_list_as_ids(), vec![11, 21, 101, 31]); + assert_eq!(get_bags(), vec![(10, vec![31]), (1_000, vec![11, 21, 101])]); + + let weight_of = Staking::weight_of_fn(); + + let node_31 = Node::::from_id(&31).unwrap(); + assert!(!node_31.is_misplaced(&weight_of)); + assert_eq!(weight_of(&31), 1); + + // when account 31 bonds extra and needs to be moved to a non-existing higher bag + // (we can't call bond_extra, because that implicitly calls update_position_for) + Balances::make_free_balance_be(&31, 11); + let controller = Staking::bonded(&31).unwrap(); + let mut ledger = Staking::ledger(&controller).unwrap(); + ledger.total = 11; + ledger.active = 11; + Staking::update_ledger(&controller, &ledger); + + assert!(node_31.is_misplaced(&weight_of)); + assert_eq!(weight_of(&31), 11); + + // then updating position moves it to the correct bag + assert_eq!(VoterList::::update_position_for(node_31, &weight_of), Some((10, 20))); + assert_eq!(get_bags(), vec![(10, vec![]), (20, vec![31]), (1_000, vec![11, 21, 101])]); + assert_eq!(get_voter_list_as_ids(), vec![11, 21, 101, 31]); + + // and if you try and update the position with no change in active stake nothing changes + let node_31 = Node::::from_id(&31).unwrap(); + assert_storage_noop!(assert_eq!( + VoterList::::update_position_for(node_31, &weight_of), + None, + )); + + // when account 31 bonds extra and needs to be moved to an existent higher bag + Balances::make_free_balance_be(&31, 1_000); + let mut ledger = Staking::ledger(&controller).unwrap(); + ledger.total = 61; + ledger.active = 61; + Staking::update_ledger(&controller, &ledger); + + // then updating positions moves it to the correct bag + let node_31 = Node::::from_id(&31).unwrap(); + assert_eq!( + VoterList::::update_position_for(node_31, &weight_of), + Some((20, 1_000)) + ); + assert_eq!( + get_bags(), + vec![(10, vec![]), (20, vec![]), (1_000, vec![11, 21, 101, 31])] + ); + assert_eq!(get_voter_list_as_ids(), vec![11, 21, 101, 31]); + + // when account 31 bonds extra but should not change bags + let mut ledger = Staking::ledger(&controller).unwrap(); + ledger.total = 1_000; + ledger.active = 1_000; + Staking::update_ledger(&controller, &ledger); + + // then nothing changes + let node_31 = Node::::from_id(&31).unwrap(); + assert_storage_noop!(assert_eq!( + VoterList::::update_position_for(node_31, &weight_of), + None, + )); + }); + } + + fn update_position_failure_edge_case() { + // let weight_of = Staking::weight_of_fn(); + + // Balances::make_free_balance_be(&31, 11); + // assert_ok!(Staking::bond_extra(Origin::signed(31), 11)); } } From fb4bbe305f45ab9e465005373623144a681d512a Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Fri, 23 Jul 2021 14:22:21 -0700 Subject: [PATCH 06/28] Write out edge case; probably can delete later --- frame/staking/src/voter_bags.rs | 70 ++++++++++++++++++++++++--------- 1 file changed, 52 insertions(+), 18 deletions(-) diff --git a/frame/staking/src/voter_bags.rs b/frame/staking/src/voter_bags.rs index 20c9cd3896474..554b62ff19dea 100644 --- a/frame/staking/src/voter_bags.rs +++ b/frame/staking/src/voter_bags.rs @@ -260,12 +260,7 @@ impl VoterList { let new_idx = notional_bag_for::(weight_of(&node.voter.id)); node.bag_upper = new_idx; let mut bag = Bag::::get_or_make(node.bag_upper); - println!("update_position_fr bag BEFORe insert: {:#?}", bag); - bag.insert_node(node); - - // TODO remove this - println!("update_position_fr bag AFTER insert: {:#?}", bag); bag.put(); (old_idx, new_idx) @@ -476,14 +471,8 @@ impl Bag { fn insert_node(&mut self, mut node: Node) { let id = node.voter.id.clone(); - // if let Some(tail_id) = self.tail { - // debug_assert!(id != tail_id, "Should never insert a node into a bag where it already exists") - // - // } else { - node.prev = self.tail.clone(); node.prev = self.tail.clone(); node.next = None; - println!("insert_node before node.put() (id {:#?}) {:#?}", node.voter.id, node); node.put(); // update the previous tail @@ -1151,9 +1140,6 @@ mod voter_list { #[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()` - ExtBuilder::default().build_and_execute_without_check_count(|| { // given a correctly placed account 31 assert_eq!(get_voter_list_as_ids(), vec![11, 21, 101, 31]); @@ -1223,11 +1209,59 @@ mod voter_list { }); } - fn update_position_failure_edge_case() { - // let weight_of = Staking::weight_of_fn(); + // TODO: should probs remove this; Just wanted to document an edge case + // I ran into where I read in a node, called bond_extra, and then called + // update_position_for(node). The underlying issue is we insert a node with + // the same ID twice. + // When we insert we set node.prev = tail.id + // and then old_tail.next = node.id. Which creates a cycle since old_tail + // was the same node + // long story short, don't insert the same id into a bag twice + #[test] + fn node_reference_invalid_after_implicit_update_position() { + ExtBuilder::default().build_and_execute_without_check_count(|| { + let weight_of = Staking::weight_of_fn(); - // Balances::make_free_balance_be(&31, 11); - // assert_ok!(Staking::bond_extra(Origin::signed(31), 11)); + // starts out in the correct place. + let node_31 = Node::::from_id(&31).unwrap(); + assert!(!node_31.is_misplaced(&weight_of)); + + Balances::make_free_balance_be(&31, 11); + // when we call bond_extra + assert_ok!(Staking::bond_extra(Origin::signed(31), 11)); + + // then now the reference is outdated + assert!(node_31.is_misplaced(&weight_of)); + // but we can expect the list is correct + assert_eq!( + get_bags(), + vec![(10, vec![]), (20, vec![31]), (1_000, vec![11, 21, 101])] + ); + assert_eq!(get_voter_list_as_ids(), vec![11, 21, 101, 31]); + + assert_eq!( + VoterList::::update_position_for(node_31, &weight_of), + Some((10, 20)) + ); + + let bag = Bag::::get(20).unwrap(); + // there is a cycle, + // the bags head/tail are the same node, and that node has its own + // id for prev & next so when we iterate we will infinitely keep + // reading the same node from storage. + assert_eq!( + bag.head().unwrap().voter().id, + bag.head().unwrap().next().unwrap().voter().id + ); + assert_eq!( + bag.head().unwrap().voter().id, + bag.head().unwrap().prev().unwrap().voter().id + ); + assert_eq!( + bag.head().unwrap().voter().id, + bag.tail().unwrap().voter().id, + ) + }); } } From b9c9d56914a4579c8496f3e1d79a12e2bc71827c Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Fri, 23 Jul 2021 15:24:10 -0700 Subject: [PATCH 07/28] Add test: bags::get_works --- frame/staking/src/voter_bags.rs | 73 +++++++++++++++++++++------------ 1 file changed, 47 insertions(+), 26 deletions(-) diff --git a/frame/staking/src/voter_bags.rs b/frame/staking/src/voter_bags.rs index 554b62ff19dea..be686d81ddf20 100644 --- a/frame/staking/src/voter_bags.rs +++ b/frame/staking/src/voter_bags.rs @@ -399,7 +399,7 @@ impl VoterList { /// iteration so that there's no incentive to churn voter positioning to improve the chances of /// appearing within the voter set. #[derive(DefaultNoBound, Encode, Decode)] -#[cfg_attr(feature = "std", derive(frame_support::DebugNoBound))] +#[cfg_attr(feature = "std", derive(frame_support::DebugNoBound, PartialEq))] pub struct Bag { head: Option>, tail: Option>, @@ -1042,11 +1042,7 @@ mod voter_list { #[test] fn insert_works() { ExtBuilder::default().build_and_execute_without_check_count(|| { - // given - assert_eq!(get_voter_list_as_ids(), vec![11, 21, 101, 31]); - // TODO maybe checking the actual bags here is overkill since this is aimed at VoterList - // api and not bags? (same Q for other tests here) - assert_eq!(get_bags(), vec![(10, vec![31]), (1_000, vec![11, 21, 101])]); + // given basic_setup_works // Insert into an existing bag: // when @@ -1107,9 +1103,7 @@ mod voter_list { #[test] fn remove_works() { ExtBuilder::default().build_and_execute_without_check_count(|| { - // given - assert_eq!(get_voter_list_as_ids(), vec![11, 21, 101, 31]); - assert_eq!(get_bags(), vec![(10, vec![31]), (1_000, vec![11, 21, 101])]); + // given basic_setup_works // Remove a non-existent voter: // when @@ -1141,12 +1135,9 @@ mod voter_list { #[test] fn update_position_for_works() { ExtBuilder::default().build_and_execute_without_check_count(|| { - // given a correctly placed account 31 - assert_eq!(get_voter_list_as_ids(), vec![11, 21, 101, 31]); - assert_eq!(get_bags(), vec![(10, vec![31]), (1_000, vec![11, 21, 101])]); - let weight_of = Staking::weight_of_fn(); + // given basic_setup_works with a correctly placed account 31 let node_31 = Node::::from_id(&31).unwrap(); assert!(!node_31.is_misplaced(&weight_of)); assert_eq!(weight_of(&31), 1); @@ -1233,16 +1224,10 @@ mod voter_list { // then now the reference is outdated assert!(node_31.is_misplaced(&weight_of)); // but we can expect the list is correct - assert_eq!( - get_bags(), - vec![(10, vec![]), (20, vec![31]), (1_000, vec![11, 21, 101])] - ); + assert_eq!(get_bags(), vec![(10, vec![]), (20, vec![31]), (1_000, vec![11, 21, 101])]); assert_eq!(get_voter_list_as_ids(), vec![11, 21, 101, 31]); - assert_eq!( - VoterList::::update_position_for(node_31, &weight_of), - Some((10, 20)) - ); + assert_eq!(VoterList::::update_position_for(node_31, &weight_of), Some((10, 20))); let bag = Bag::::get(20).unwrap(); // there is a cycle, @@ -1257,19 +1242,55 @@ mod voter_list { bag.head().unwrap().voter().id, bag.head().unwrap().prev().unwrap().voter().id ); - assert_eq!( - bag.head().unwrap().voter().id, - bag.tail().unwrap().voter().id, - ) + assert_eq!(bag.head().unwrap().voter().id, bag.tail().unwrap().voter().id,) }); } } #[cfg(test)] mod bags { + use super::*; + use crate::mock::*; + use frame_support::{assert_ok, assert_storage_noop, traits::Currency}; + #[test] fn get_works() { - todo!() + ExtBuilder::default().build_and_execute_without_check_count(|| { + let check_bag = |bag_upper, head, tail, ids| { + assert_storage_noop!(Bag::::get(bag_upper)); + let bag = Bag::::get(bag_upper).unwrap(); + let bag_ids = bag.iter().map(|n| n.voter().id).collect::>(); + assert_eq!(bag, Bag:: { head, tail, bag_upper }); + assert_eq!(bag_ids, ids); + }; + + // given uppers of bags that exist. + let existing_bag_uppers = vec![10, 1_000]; + + // we can fetch them + check_bag(existing_bag_uppers[0], Some(31), Some(31), vec![31]); + // (getting the same bag twice has the same results) + check_bag(existing_bag_uppers[0], Some(31), Some(31), vec![31]); + check_bag(existing_bag_uppers[1], Some(11), Some(101), vec![11, 21, 101]); + + // and all other uppers don't get bags. + ::VoterBagThresholds::get() + .iter() + .chain(iter::once(&VoteWeight::MAX)) + .filter(|bag_upper| !existing_bag_uppers.contains(bag_upper)) + .for_each(|bag_upper| { + assert_storage_noop!(assert_eq!(Bag::::get(*bag_upper), None)); + }); + }); + } + + #[test] + #[should_panic] + fn get_panics_with_a_bad_threshold() { + // NOTE: panic is only expected with debug compilation + ExtBuilder::default().build_and_execute_without_check_count(|| { + Bag::::get(11); + }); } #[test] From 1c41c1bf28e903f4241c6d9e818b2e5910521977 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Fri, 23 Jul 2021 19:41:14 -0700 Subject: [PATCH 08/28] Add test: remove_node_happy_path_works --- frame/staking/src/mock.rs | 4 + frame/staking/src/voter_bags.rs | 206 ++++++++++++++++++++++++++++++-- 2 files changed, 197 insertions(+), 13 deletions(-) diff --git a/frame/staking/src/mock.rs b/frame/staking/src/mock.rs index 0910bb9dcc43d..3e159a2c42ed5 100644 --- a/frame/staking/src/mock.rs +++ b/frame/staking/src/mock.rs @@ -846,6 +846,10 @@ pub(crate) fn get_bags() -> Vec<(VoteWeight, Vec)> { .collect::>() } +pub(crate) fn bag_as_ids(bag: &Bag) -> Vec { + bag.iter().map(|n| n.voter().id).collect::>() +} + pub(crate) fn get_voter_list_as_ids() -> Vec { VoterList::::iter().map(|n| n.voter().id).collect::>() } diff --git a/frame/staking/src/voter_bags.rs b/frame/staking/src/voter_bags.rs index be686d81ddf20..7a706283c13cc 100644 --- a/frame/staking/src/voter_bags.rs +++ b/frame/staking/src/voter_bags.rs @@ -399,7 +399,8 @@ impl VoterList { /// iteration so that there's no incentive to churn voter positioning to improve the chances of /// appearing within the voter set. #[derive(DefaultNoBound, Encode, Decode)] -#[cfg_attr(feature = "std", derive(frame_support::DebugNoBound, PartialEq))] +#[cfg_attr(feature = "std", derive(frame_support::DebugNoBound))] +#[cfg_attr(test, derive(PartialEq))] pub struct Bag { head: Option>, tail: Option>, @@ -562,6 +563,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(test, derive(PartialEq, Clone))] pub struct Node { voter: Voter>, prev: Option>, @@ -1042,8 +1044,6 @@ mod voter_list { #[test] fn insert_works() { ExtBuilder::default().build_and_execute_without_check_count(|| { - // given basic_setup_works - // Insert into an existing bag: // when bond(42, 43, 999); @@ -1103,8 +1103,6 @@ mod voter_list { #[test] fn remove_works() { ExtBuilder::default().build_and_execute_without_check_count(|| { - // given basic_setup_works - // Remove a non-existent voter: // when VoterList::::remove(&42); @@ -1137,10 +1135,9 @@ mod voter_list { ExtBuilder::default().build_and_execute_without_check_count(|| { let weight_of = Staking::weight_of_fn(); - // given basic_setup_works with a correctly placed account 31 + // given a correctly placed account 31 let node_31 = Node::::from_id(&31).unwrap(); assert!(!node_31.is_misplaced(&weight_of)); - assert_eq!(weight_of(&31), 1); // when account 31 bonds extra and needs to be moved to a non-existing higher bag // (we can't call bond_extra, because that implicitly calls update_position_for) @@ -1207,7 +1204,8 @@ mod voter_list { // When we insert we set node.prev = tail.id // and then old_tail.next = node.id. Which creates a cycle since old_tail // was the same node - // long story short, don't insert the same id into a bag twice + // tl;dr, don't insert the same id into a bag twice, especially if its tail + // cover this explicitly in insert_node_bad_paths_documented #[test] fn node_reference_invalid_after_implicit_update_position() { ExtBuilder::default().build_and_execute_without_check_count(|| { @@ -1251,7 +1249,7 @@ mod voter_list { mod bags { use super::*; use crate::mock::*; - use frame_support::{assert_ok, assert_storage_noop, traits::Currency}; + use frame_support::{assert_storage_noop, assert_ok}; #[test] fn get_works() { @@ -1294,13 +1292,195 @@ mod bags { } #[test] - fn insert_works() { - todo!() + fn insert_node_happy_paths_works() { + ExtBuilder::default().build_and_execute_without_check_count(|| { + let node = |voter, bag_upper| Node:: { voter, prev: None, next: None, bag_upper }; + + // when inserting into a bag with 1 node + let mut bag_10 = Bag::::get(10).unwrap(); + // (note: 42 has no balance or ledger: bags api does not care) + bag_10.insert_node(node(Voter::<_>::nominator(42), bag_10.bag_upper)); + // then + assert_eq!(bag_as_ids(&bag_10), vec![31, 42]); + + // when inserting into a bag with 3 nodes + let mut bag_1000 = Bag::::get(1_000).unwrap(); + bag_1000.insert_node(node(Voter::<_>::nominator(52), bag_1000.bag_upper)); + // then + assert_eq!(bag_as_ids(&bag_1000), vec![11, 21, 101, 52]); + + // when inserting into a new bag + let mut bag_20 = Bag::::get_or_make(20); + bag_20.insert_node(node(Voter::<_>::nominator(62), bag_20.bag_upper)); + // then + assert_eq!(bag_as_ids(&bag_20), vec![20]); + + // when inserting a node pointing to the accounts not in the bag + let voter_62 = Voter::<_>::validator(62); + let node_62 = Node:: { voter: voter_62.clone(), prev: Some(21), next: Some(101), bag_upper: 20 }; + bag_20.insert_node(node_62); + // then ids are in order + assert_eq!(bag_as_ids(&bag_20), vec![20, 62]); + // and when the node is re-fetched all the info is correct + assert_eq!( + Node::::get(20, &62).unwrap(), + Node:: { voter: voter_62, prev: Some(20), next: None, bag_upper: 20 } + ); + }); } + // Document improper ways `insert_node` may be getting used. #[test] - fn remove_works() { - todo!() + fn insert_node_bad_paths_documented() { + ExtBuilder::default().build_and_execute_without_check_count(|| { + let node = |voter, prev, next, bag_upper| Node:: { voter, prev, next, bag_upper }; + // insert some validators into genesis state + bond_validator(51, 50, 2_000); + bond_validator(61, 60, 2_000); + bond_validator(71, 70, 2_000); + + // when inserting a node with both prev & next pointing at an account in the bag + // in the bag and a incorrect bag_upper + let mut bag_1000 = Bag::::get(1_000).unwrap(); + let voter_42 = Voter::<_>::nominator(42); + bag_1000.insert_node(node(voter_42.clone(), Some(11), Some(11), 0)); + // then the ids are in the correct order + assert_eq!(bag_as_ids(&bag_1000), vec![11, 21, 101, 42]); + // and when the node is re-fetched all the info is correct + assert_eq!( + Node::::get(1_000, &42).unwrap(), + node(voter_42, Some(101), None, bag_1000.bag_upper) + ); + + // given 21 is a validator in bag_1000 (and not a tail node) + let bag_1000_voter = bag_1000.iter().map(|node| node.voter().clone()).collect::>(); + assert_eq!(bag_1000_voter[1], Voter::<_>::validator(21)); + // when inserting a node with id 21 but as a nominator + let voter_21_nom = Voter::<_>::nominator(21); + bag_1000.insert_node(node(voter_21_nom.clone(), None, None, bag_1000.bag_upper)); + // then all the nodes after the duplicate are lost (because it is set as the tail) + assert_eq!(bag_as_ids(&bag_1000), vec![11, 21]); + // and the re-fetched node is a nominator with an **incorrect** prev pointer. + assert_eq!( + Node::::get(1_000, &21).unwrap(), + node(voter_21_nom, Some(42), None, bag_1000.bag_upper) + ); + + // TODO: this exact issue could be avoided at this level of abstraction if we check if + // an id is already the bags tail before inserting + + + // given a bag we are confident is in valid state + let mut bag_2000 = Bag::::get(2_000).unwrap(); + assert_eq!(bag_as_ids(&bag_2000), vec![51, 61, 71]); + // when inserting a duplicate id that is already the tail + assert_eq!(bag_2000.tail, Some(71)); + let voter_71 = Voter::<_>::validator(71); + bag_2000.insert_node(node(voter_71.clone(), None, None, bag_2000.bag_upper)); + // then a cycle is created + assert_eq!(bag_2000.tail, Some(71)); + assert_eq!( + Node::::get(2_000, &71).unwrap(), + node(voter_71, Some(71), Some(71), bag_2000.bag_upper) + ); + }) + } + + #[test] + fn remove_node_happy_path_works() { + ExtBuilder::default().build_and_execute_without_check_count(|| { + // add some validators to genesis state + bond_validator(51, 50, 1_000); + bond_validator(61, 60, 1_000); + bond_validator(71, 70, 10); + bond_validator(81, 80, 10); + bond_validator(91, 90, 2_000); + bond_validator(161, 160, 2_000); + bond_validator(171, 170, 2_000); + bond_validator(181, 180, 2_000); + bond_validator(191, 190, 2_000); + + // given + let mut bag_1000 = Bag::::get(1_000).unwrap(); + assert_eq!(bag_as_ids(&bag_1000), vec![11, 21, 101, 51, 61]); + let mut bag_10 = Bag::::get(10).unwrap(); + assert_eq!(bag_as_ids(&bag_10), vec![31, 71, 81]); + let mut bag_2000 = Bag::::get(2_000).unwrap(); + assert_eq!(bag_as_ids(&bag_2000), vec![91, 161, 171, 181, 191]); + + // remove node from center that is not pointing at head or tail + let node_101 = Node::::get(bag_1000.bag_upper, &101).unwrap(); + let node_101_pre_remove = node_101.clone(); + bag_1000.remove_node(&node_101); + assert_eq!(bag_as_ids(&bag_1000), vec![11, 21, 51, 61]); + // node isn't mutated when its removed + assert_eq!(node_101, node_101_pre_remove); + assert_ok!(bag_1000.sanity_check()); + + // remove head when its not pointing at tail + let node_11 = Node::::get(bag_1000.bag_upper, &11).unwrap(); + bag_1000.remove_node(&node_11); + assert_eq!(bag_as_ids(&bag_1000), vec![21, 51, 61]); + assert_eq!(bag_1000.head, Some(21)); assert_eq!(bag_1000.tail, Some(61)); + assert_ok!(bag_1000.sanity_check()); + + // remove tail when its not pointing at head + let node_61 = Node::::get(bag_1000.bag_upper, &61).unwrap(); + bag_1000.remove_node(&node_61); + assert_eq!(bag_as_ids(&bag_1000), vec![21, 51]); + assert_eq!(bag_1000.head, Some(21)); assert_eq!(bag_1000.tail, Some(51)); + assert_ok!(bag_1000.sanity_check()); + + // remove tail when its pointing at head + let node_51 = Node::::get(bag_1000.bag_upper, &51).unwrap(); + bag_1000.remove_node(&node_51); + assert_eq!(bag_as_ids(&bag_1000), vec![21]); + assert_eq!(bag_1000.head, Some(21)); assert_eq!(bag_1000.tail, Some(21)); + assert_ok!(bag_1000.sanity_check()); + + // remove node that is head & tail + let node_21 = Node::::get(bag_1000.bag_upper, &21).unwrap(); + bag_1000.remove_node(&node_21); + assert_eq!(bag_as_ids(&bag_1000), Vec::::new()); + assert_eq!(bag_1000.head, None); assert_eq!(bag_1000.tail, None); + assert_ok!(bag_1000.sanity_check()); + + // remove node that is pointing at head and tail + let node_71 = Node::::get(bag_10.bag_upper, &71).unwrap(); + bag_10.remove_node(&node_71); + assert_eq!(bag_as_ids(&bag_10), vec![31, 81]); + assert_eq!(bag_10.head, Some(31)); assert_eq!(bag_10.tail, Some(81)); + assert_ok!(bag_10.sanity_check()); + + // remove head when pointing at tail + let node_31 = Node::::get(bag_10.bag_upper, &31).unwrap(); + bag_10.remove_node(&node_31); + assert_eq!(bag_as_ids(&bag_10), vec![81]); + assert_eq!(bag_10.head, Some(81)); assert_eq!(bag_10.tail, Some(81)); + assert_ok!(bag_10.sanity_check()); + + // remove node that is pointing at head, but not tail + let node_161 = Node::::get(bag_2000.bag_upper, &161).unwrap(); + bag_2000.remove_node(&node_161); + assert_eq!(bag_as_ids(&bag_2000), vec![91, 171, 181, 191]); + assert_eq!(bag_2000.head, Some(91)); assert_eq!(bag_2000.tail, Some(191)); + assert_ok!(bag_2000.sanity_check()); + + // remove node that is pointing at tail, but not head + let node_181 = Node::::get(bag_2000.bag_upper, &181).unwrap(); + bag_2000.remove_node(&node_181); + assert_eq!(bag_as_ids(&bag_2000), vec![91, 171, 191]); + assert_eq!(bag_2000.head, Some(91)); assert_eq!(bag_2000.tail, Some(191)); + assert_ok!(bag_2000.sanity_check()); + + }); + } + + #[test] + fn remove_node_bad_paths_documented() { + // removing node with prev, next in other bags will mess up other bags + // removing node that is bag but has the wrong upper wont do anything + // removing a node whose id is not in the bag does not do anything } } From e59b09365c58d31d4d967f0bc3ed3eb581aac68a Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Fri, 23 Jul 2021 20:43:02 -0700 Subject: [PATCH 09/28] Add test: remove_node_bad_paths_documented --- frame/staking/src/voter_bags.rs | 73 +++++++++++++++++++++++++-------- 1 file changed, 55 insertions(+), 18 deletions(-) diff --git a/frame/staking/src/voter_bags.rs b/frame/staking/src/voter_bags.rs index 7a706283c13cc..e2b8a6fb6758d 100644 --- a/frame/staking/src/voter_bags.rs +++ b/frame/staking/src/voter_bags.rs @@ -1249,7 +1249,7 @@ mod voter_list { mod bags { use super::*; use crate::mock::*; - use frame_support::{assert_storage_noop, assert_ok}; + use frame_support::{assert_ok, assert_storage_noop}; #[test] fn get_works() { @@ -1317,7 +1317,12 @@ mod bags { // when inserting a node pointing to the accounts not in the bag let voter_62 = Voter::<_>::validator(62); - let node_62 = Node:: { voter: voter_62.clone(), prev: Some(21), next: Some(101), bag_upper: 20 }; + let node_62 = Node:: { + voter: voter_62.clone(), + prev: Some(21), + next: Some(101), + bag_upper: 20, + }; bag_20.insert_node(node_62); // then ids are in order assert_eq!(bag_as_ids(&bag_20), vec![20, 62]); @@ -1353,7 +1358,8 @@ mod bags { ); // given 21 is a validator in bag_1000 (and not a tail node) - let bag_1000_voter = bag_1000.iter().map(|node| node.voter().clone()).collect::>(); + let bag_1000_voter = + bag_1000.iter().map(|node| node.voter().clone()).collect::>(); assert_eq!(bag_1000_voter[1], Voter::<_>::validator(21)); // when inserting a node with id 21 but as a nominator let voter_21_nom = Voter::<_>::nominator(21); @@ -1369,7 +1375,6 @@ mod bags { // TODO: this exact issue could be avoided at this level of abstraction if we check if // an id is already the bags tail before inserting - // given a bag we are confident is in valid state let mut bag_2000 = Bag::::get(2_000).unwrap(); assert_eq!(bag_as_ids(&bag_2000), vec![51, 61, 71]); @@ -1387,7 +1392,7 @@ mod bags { } #[test] - fn remove_node_happy_path_works() { + fn remove_node_happy_paths_works() { ExtBuilder::default().build_and_execute_without_check_count(|| { // add some validators to genesis state bond_validator(51, 50, 1_000); @@ -1406,7 +1411,7 @@ mod bags { let mut bag_10 = Bag::::get(10).unwrap(); assert_eq!(bag_as_ids(&bag_10), vec![31, 71, 81]); let mut bag_2000 = Bag::::get(2_000).unwrap(); - assert_eq!(bag_as_ids(&bag_2000), vec![91, 161, 171, 181, 191]); + assert_eq!(bag_as_ids(&bag_2000), vec![91, 161, 171, 181, 191]); // remove node from center that is not pointing at head or tail let node_101 = Node::::get(bag_1000.bag_upper, &101).unwrap(); @@ -1421,66 +1426,98 @@ mod bags { let node_11 = Node::::get(bag_1000.bag_upper, &11).unwrap(); bag_1000.remove_node(&node_11); assert_eq!(bag_as_ids(&bag_1000), vec![21, 51, 61]); - assert_eq!(bag_1000.head, Some(21)); assert_eq!(bag_1000.tail, Some(61)); assert_ok!(bag_1000.sanity_check()); // remove tail when its not pointing at head let node_61 = Node::::get(bag_1000.bag_upper, &61).unwrap(); bag_1000.remove_node(&node_61); assert_eq!(bag_as_ids(&bag_1000), vec![21, 51]); - assert_eq!(bag_1000.head, Some(21)); assert_eq!(bag_1000.tail, Some(51)); assert_ok!(bag_1000.sanity_check()); // remove tail when its pointing at head let node_51 = Node::::get(bag_1000.bag_upper, &51).unwrap(); bag_1000.remove_node(&node_51); assert_eq!(bag_as_ids(&bag_1000), vec![21]); - assert_eq!(bag_1000.head, Some(21)); assert_eq!(bag_1000.tail, Some(21)); assert_ok!(bag_1000.sanity_check()); // remove node that is head & tail let node_21 = Node::::get(bag_1000.bag_upper, &21).unwrap(); bag_1000.remove_node(&node_21); assert_eq!(bag_as_ids(&bag_1000), Vec::::new()); - assert_eq!(bag_1000.head, None); assert_eq!(bag_1000.tail, None); assert_ok!(bag_1000.sanity_check()); // remove node that is pointing at head and tail let node_71 = Node::::get(bag_10.bag_upper, &71).unwrap(); bag_10.remove_node(&node_71); assert_eq!(bag_as_ids(&bag_10), vec![31, 81]); - assert_eq!(bag_10.head, Some(31)); assert_eq!(bag_10.tail, Some(81)); assert_ok!(bag_10.sanity_check()); // remove head when pointing at tail let node_31 = Node::::get(bag_10.bag_upper, &31).unwrap(); bag_10.remove_node(&node_31); assert_eq!(bag_as_ids(&bag_10), vec![81]); - assert_eq!(bag_10.head, Some(81)); assert_eq!(bag_10.tail, Some(81)); assert_ok!(bag_10.sanity_check()); // remove node that is pointing at head, but not tail let node_161 = Node::::get(bag_2000.bag_upper, &161).unwrap(); bag_2000.remove_node(&node_161); assert_eq!(bag_as_ids(&bag_2000), vec![91, 171, 181, 191]); - assert_eq!(bag_2000.head, Some(91)); assert_eq!(bag_2000.tail, Some(191)); assert_ok!(bag_2000.sanity_check()); // remove node that is pointing at tail, but not head let node_181 = Node::::get(bag_2000.bag_upper, &181).unwrap(); bag_2000.remove_node(&node_181); assert_eq!(bag_as_ids(&bag_2000), vec![91, 171, 191]); - assert_eq!(bag_2000.head, Some(91)); assert_eq!(bag_2000.tail, Some(191)); assert_ok!(bag_2000.sanity_check()); - }); } #[test] fn remove_node_bad_paths_documented() { - // removing node with prev, next in other bags will mess up other bags - // removing node that is bag but has the wrong upper wont do anything - // removing a node whose id is not in the bag does not do anything + ExtBuilder::default().build_and_execute_without_check_count(|| { + // removing node that is in the bag but has the wrong upper works + let bad_upper_node_11 = Node:: { + voter: Voter::<_>::validator(11), + prev: None, + next: Some(21), + bag_upper: 10, // should be 1_000 + }; + let mut bag_1000 = Bag::::get(1_000).unwrap(); + bag_1000.remove_node(&bad_upper_node_11); + bag_1000.put(); + + assert_eq!(get_bags(), vec![(10, vec![31]), (1_000, vec![21, 101])]); + let bag_1000 = Bag::::get(1_000).unwrap(); + assert_ok!(bag_1000.sanity_check()); + assert_eq!(bag_1000.head, Some(21)); + assert_eq!(bag_1000.tail, Some(101)); + }); + + ExtBuilder::default().build_and_execute_without_check_count(|| { + // removing a node that is in an other bag, will mess up the + // other bag. + + let node_101 = Node::::get(1_000, &101).unwrap(); + let mut bag_10 = Bag::::get(10).unwrap(); + bag_10.remove_node(&node_101); // node_101 is in bag 1_000 + bag_10.put(); + + // the node was removed from its actual bag, bag_1000. + assert_eq!(get_bags(), vec![(10, vec![31]), (1_000, vec![11, 21])]); + + // the bag removed was called on is ok. + let bag_10 = Bag::::get(10).unwrap(); + assert_eq!(bag_10.tail, Some(31)); + assert_eq!(bag_10.head, Some(31)); + + // but the bag that actually had the node is in an invalid state + let bag_1000 = Bag::::get(1_000).unwrap(); + assert_ok!(bag_1000.sanity_check()); + let bag_1000 = Bag::::get(1_000).unwrap(); + // because it still has the removed node as its tail. + assert_eq!(bag_1000.tail, Some(101)); + assert_eq!(bag_1000.head, Some(11)); + }); } } From 83ec65653bbe3c11f6d266fe6ee7c9c5b327a631 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Fri, 23 Jul 2021 22:19:09 -0700 Subject: [PATCH 10/28] WIP: voting_data_works --- frame/staking/src/voter_bags.rs | 81 +++++++++++++++++++++++++++++++-- 1 file changed, 76 insertions(+), 5 deletions(-) diff --git a/frame/staking/src/voter_bags.rs b/frame/staking/src/voter_bags.rs index e2b8a6fb6758d..50b19a705dc9e 100644 --- a/frame/staking/src/voter_bags.rs +++ b/frame/staking/src/voter_bags.rs @@ -1512,25 +1512,96 @@ mod bags { // but the bag that actually had the node is in an invalid state let bag_1000 = Bag::::get(1_000).unwrap(); - assert_ok!(bag_1000.sanity_check()); - let bag_1000 = Bag::::get(1_000).unwrap(); // because it still has the removed node as its tail. assert_eq!(bag_1000.tail, Some(101)); assert_eq!(bag_1000.head, Some(11)); + assert_ok!(bag_1000.sanity_check()); }); } } #[cfg(test)] mod voter_node { + use super::*; + use crate::mock::*; + use frame_support::{assert_ok, assert_storage_noop, traits::Currency}; + #[test] - fn get_voting_data_works() { - todo!() + fn voting_data_works() { + ExtBuilder::default().build_and_execute_without_check_count(|| { + let weight_of = Staking::weight_of_fn(); + + // add nominator with 2/3 slashed targets + bond_nominator(42, 43, 1_000, vec![31, 21, 11]); + + // given + assert_eq!(get_voter_list_as_voters(), vec![ + Voter::<_>::validator(11), + Voter::<_>::validator(21), + Voter::<_>::nominator(101), + Voter::<_>::nominator(42), + Voter::<_>::validator(31), + ]); + + let slashing_spans = ::SlashingSpans::iter().collect::>(); + assert_eq!(slashing_spans.keys().len(), 0); // no pre-existing slashing spans + + let node_31 = Node::::get(10, &31).unwrap(); + assert_eq!( + node_31.voting_data(&weight_of, &slashing_spans).unwrap(), + (31, 1, vec![31]) + ); + + // getting data for a nominator with 0 slashed targets + let node_42 = Node::::get(10, &42).unwrap(); + assert_eq!( + node_42.voting_data(&weight_of, &slashing_spans).unwrap(), + (42, 1_000, vec![31, 21, 11]) + ); + + // when validator 31 gets a slash, + add_slash(&31); + let mut slashing_spans = ::SlashingSpans::iter().collect::>(); + assert!(slashing_spans.contains_key(&31)); + // then its node no longer exists + assert_eq!(get_voter_list_as_voters(), vec![ + Voter::<_>::validator(11), + Voter::<_>::validator(21), + Voter::<_>::nominator(101), + Voter::<_>::nominator(42), + ]); + // and its nominator no longer has it as a target + let node_42 = Node::::get(10, &42).unwrap(); + assert_eq!( + node_42.voting_data(&weight_of, &slashing_spans).unwrap(), + (42, 1_000, vec![21, 11]) + ) + }); } #[test] fn is_misplaced_works() { - todo!() + ExtBuilder::default().build_and_execute_without_check_count(|| { + let weight_of = Staking::weight_of_fn(); + let node_31 = Node::::get(10, &31).unwrap(); + + // a node is properly placed if its slashable balance is in range + // of the threshold of the bag its in. + assert_eq!(Staking::slashable_balance_of(&31), 1); + assert!(!node_31.is_misplaced(&weight_of)); + + // and will become misplaced if its slashable balance does not + // correspond to the bag it is in. + Balances::make_free_balance_be(&31, 11); + let controller = Staking::bonded(&31).unwrap(); + let mut ledger = Staking::ledger(&controller).unwrap(); + ledger.total = 11; + ledger.active = 11; + Staking::update_ledger(&controller, &ledger); + + assert_eq!(Staking::slashable_balance_of(&31), 11); + assert!(node_31.is_misplaced(&weight_of)); + }); } } From 29af5c435b8546f70dc11760c837fcaf185d80ec Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Fri, 23 Jul 2021 23:20:36 -0700 Subject: [PATCH 11/28] done --- frame/staking/src/voter_bags.rs | 49 +++++++++++++++++++++------------ 1 file changed, 32 insertions(+), 17 deletions(-) diff --git a/frame/staking/src/voter_bags.rs b/frame/staking/src/voter_bags.rs index 50b19a705dc9e..8eeb2e61ca819 100644 --- a/frame/staking/src/voter_bags.rs +++ b/frame/staking/src/voter_bags.rs @@ -1531,8 +1531,8 @@ mod voter_node { ExtBuilder::default().build_and_execute_without_check_count(|| { let weight_of = Staking::weight_of_fn(); - // add nominator with 2/3 slashed targets - bond_nominator(42, 43, 1_000, vec![31, 21, 11]); + // add nominator with no targets + bond_nominator(42, 43, 1_000, vec![11]); // given assert_eq!(get_voter_list_as_voters(), vec![ @@ -1542,40 +1542,55 @@ mod voter_node { Voter::<_>::nominator(42), Voter::<_>::validator(31), ]); + assert_eq!(active_era(), 0); let slashing_spans = ::SlashingSpans::iter().collect::>(); assert_eq!(slashing_spans.keys().len(), 0); // no pre-existing slashing spans - let node_31 = Node::::get(10, &31).unwrap(); + let node_11 = Node::::get(10, &11).unwrap(); assert_eq!( - node_31.voting_data(&weight_of, &slashing_spans).unwrap(), - (31, 1, vec![31]) + node_11.voting_data(&weight_of, &slashing_spans).unwrap(), + (11, 1_000, vec![11]) ); - // getting data for a nominator with 0 slashed targets + // getting data for a nominators with 0 slashed targets + let node_101 = Node::::get(1_000, &101).unwrap(); + assert_eq!( + node_101.voting_data(&weight_of, &slashing_spans).unwrap(), + (101, 500, vec![11, 21]) + ); let node_42 = Node::::get(10, &42).unwrap(); assert_eq!( node_42.voting_data(&weight_of, &slashing_spans).unwrap(), - (42, 1_000, vec![31, 21, 11]) + (42, 1_000, vec![11]) ); - // when validator 31 gets a slash, - add_slash(&31); - let mut slashing_spans = ::SlashingSpans::iter().collect::>(); - assert!(slashing_spans.contains_key(&31)); + run_to_block(2_500); + assert_eq!(active_era(), 166); + // when a validator gets a slash, + add_slash(&11); + let slashing_spans = ::SlashingSpans::iter().collect::>(); + + assert_eq!(slashing_spans.keys().cloned().collect::>(), vec![11, 42, 101]); // then its node no longer exists assert_eq!(get_voter_list_as_voters(), vec![ - Voter::<_>::validator(11), Voter::<_>::validator(21), Voter::<_>::nominator(101), Voter::<_>::nominator(42), + Voter::<_>::validator(31), ]); - // and its nominator no longer has it as a target - let node_42 = Node::::get(10, &42).unwrap(); + // and its nominators no longer have it as a target + let node_101 = Node::::get(10, &101).unwrap(); assert_eq!( - node_42.voting_data(&weight_of, &slashing_spans).unwrap(), - (42, 1_000, vec![21, 11]) - ) + node_101.voting_data(&weight_of, &slashing_spans).unwrap(), + (101, 475, vec![21]) + ); + + let node_42 = Node::::get(10, &42); + assert_eq!( + node_42.voting_data(&weight_of, &slashing_spans), + None + ); }); } From 2f82fb18fa357eabc17f1930be4d0cb0a2233239 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Sun, 25 Jul 2021 14:21:34 -0700 Subject: [PATCH 12/28] Improve test voting_data_works --- frame/staking/src/voter_bags.rs | 52 +++++++++++++++++++-------------- 1 file changed, 30 insertions(+), 22 deletions(-) diff --git a/frame/staking/src/voter_bags.rs b/frame/staking/src/voter_bags.rs index 8eeb2e61ca819..3aba664a38853 100644 --- a/frame/staking/src/voter_bags.rs +++ b/frame/staking/src/voter_bags.rs @@ -1524,7 +1524,7 @@ mod bags { mod voter_node { use super::*; use crate::mock::*; - use frame_support::{assert_ok, assert_storage_noop, traits::Currency}; + use frame_support::traits::Currency; #[test] fn voting_data_works() { @@ -1535,16 +1535,20 @@ mod voter_node { bond_nominator(42, 43, 1_000, vec![11]); // given - assert_eq!(get_voter_list_as_voters(), vec![ - Voter::<_>::validator(11), - Voter::<_>::validator(21), - Voter::<_>::nominator(101), - Voter::<_>::nominator(42), - Voter::<_>::validator(31), - ]); + assert_eq!( + get_voter_list_as_voters(), + vec![ + Voter::<_>::validator(11), + Voter::<_>::validator(21), + Voter::<_>::nominator(101), + Voter::<_>::nominator(42), + Voter::<_>::validator(31), + ] + ); assert_eq!(active_era(), 0); - let slashing_spans = ::SlashingSpans::iter().collect::>(); + let slashing_spans = + ::SlashingSpans::iter().collect::>(); assert_eq!(slashing_spans.keys().len(), 0); // no pre-existing slashing spans let node_11 = Node::::get(10, &11).unwrap(); @@ -1565,31 +1569,35 @@ mod voter_node { (42, 1_000, vec![11]) ); - run_to_block(2_500); - assert_eq!(active_era(), 166); + run_to_block(20); + assert_eq!(active_era(), 1); // when a validator gets a slash, add_slash(&11); - let slashing_spans = ::SlashingSpans::iter().collect::>(); + let slashing_spans = + ::SlashingSpans::iter().collect::>(); assert_eq!(slashing_spans.keys().cloned().collect::>(), vec![11, 42, 101]); // then its node no longer exists - assert_eq!(get_voter_list_as_voters(), vec![ - Voter::<_>::validator(21), - Voter::<_>::nominator(101), - Voter::<_>::nominator(42), - Voter::<_>::validator(31), - ]); + assert_eq!( + get_voter_list_as_voters(), + vec![ + Voter::<_>::validator(21), + Voter::<_>::nominator(101), + Voter::<_>::nominator(42), + Voter::<_>::validator(31), + ] + ); // and its nominators no longer have it as a target let node_101 = Node::::get(10, &101).unwrap(); assert_eq!( - node_101.voting_data(&weight_of, &slashing_spans).unwrap(), - (101, 475, vec![21]) + node_101.voting_data(&weight_of, &slashing_spans), + Some((101, 475, vec![21])), ); - let node_42 = Node::::get(10, &42); + let node_42 = Node::::get(10, &42).unwrap(); assert_eq!( node_42.voting_data(&weight_of, &slashing_spans), - None + None, // no voting data since all targets have been slashed since nominating ); }); } From 8c995f3c74e1b7a6fd4d4beb3a3756d65b05eff6 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Sun, 25 Jul 2021 14:22:57 -0700 Subject: [PATCH 13/28] Add comment --- frame/staking/src/voter_bags.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/staking/src/voter_bags.rs b/frame/staking/src/voter_bags.rs index 3aba664a38853..f92ce963625aa 100644 --- a/frame/staking/src/voter_bags.rs +++ b/frame/staking/src/voter_bags.rs @@ -1597,7 +1597,7 @@ mod voter_node { let node_42 = Node::::get(10, &42).unwrap(); assert_eq!( node_42.voting_data(&weight_of, &slashing_spans), - None, // no voting data since all targets have been slashed since nominating + None, // no voting data since its target(s) have been slashed since nominating ); }); } From c847c23cb6369fae6050f49d15e15697420e830c Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Mon, 26 Jul 2021 15:23:46 -0700 Subject: [PATCH 14/28] Fill out test basic_setup_works --- frame/staking/src/voter_bags.rs | 41 +++++++++++++++++++++++++++++++-- 1 file changed, 39 insertions(+), 2 deletions(-) diff --git a/frame/staking/src/voter_bags.rs b/frame/staking/src/voter_bags.rs index f92ce963625aa..cc8c245c871a7 100644 --- a/frame/staking/src/voter_bags.rs +++ b/frame/staking/src/voter_bags.rs @@ -930,25 +930,62 @@ mod voter_list { #[test] fn basic_setup_works() { + use crate::{ + CounterForNominators, CounterForValidators, CounterForVoters, VoterBags, VoterNodes, + }; + let node = |voter, prev, next| Node:: { voter, prev, next, bag_upper: 0 }; + // 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); + assert_eq!(CounterForVoters::::get(), 4); + assert_eq!(VoterBagFor::::iter().count(), 4); + assert_eq!(VoterNodes::::iter().count(), 4); + assert_eq!(VoterBags::::iter().count(), 2); + assert_eq!(CounterForValidators::::get(), 3); + assert_eq!(CounterForNominators::::get(), 1); + + assert_eq!( + VoterBags::::get(10).unwrap(), + Bag:: { head: Some(31), tail: Some(31), bag_upper: 0 } + ); + assert_eq!( + VoterBags::::get(1_000).unwrap(), + Bag:: { head: Some(11), tail: Some(101), bag_upper: 0 } + ); let weight_of = Staking::weight_of_fn(); + assert_eq!(weight_of(&11), 1000); assert_eq!(VoterBagFor::::get(11).unwrap(), 1000); + assert_eq!( + VoterNodes::::get(11).unwrap(), + node(Voter::validator(11), None, Some(21)) + ); assert_eq!(weight_of(&21), 1000); assert_eq!(VoterBagFor::::get(21).unwrap(), 1000); + assert_eq!( + VoterNodes::::get(21).unwrap(), + node(Voter::validator(21), Some(11), Some(101)) + ); assert_eq!(weight_of(&31), 1); assert_eq!(VoterBagFor::::get(31).unwrap(), 10); + assert_eq!( + VoterNodes::::get(31).unwrap(), + node(Voter::validator(31), None, None) + ); + assert_eq!(weight_of(&41), 1000); assert_eq!(VoterBagFor::::get(41), None); // this staker is chilled! + assert_eq!(VoterNodes::::get(41), None); assert_eq!(weight_of(&101), 500); assert_eq!(VoterBagFor::::get(101).unwrap(), 1000); + assert_eq!( + VoterNodes::::get(101).unwrap(), + node(Voter::nominator(101), Some(21), None) + ); // iteration of the bags would yield: assert_eq!( From d0bd4b5a8a0504a24c91627456c44f9539c7584d Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Mon, 26 Jul 2021 15:28:23 -0700 Subject: [PATCH 15/28] Update: iteration_is_semi_sorted --- frame/staking/src/voter_bags.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/frame/staking/src/voter_bags.rs b/frame/staking/src/voter_bags.rs index cc8c245c871a7..66b9854f94d56 100644 --- a/frame/staking/src/voter_bags.rs +++ b/frame/staking/src/voter_bags.rs @@ -1041,6 +1041,20 @@ mod voter_list { 31, // last bag. ] ); + + // when adding a voter that has a higher weight than pre-existing voters in the bag + bond_validator(71, 70, 10); + + // then + assert_eq!( + iteration, + vec![ + 51, 61, // best bag + 11, 21, 101, // middle bag + 31, 71, // last bag; the new voter is last, because it is order of insertion + ] + ); + }) } From f1eb10297e0a3e319470d7eeb3c5677ae5ff708a Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Mon, 26 Jul 2021 15:54:47 -0700 Subject: [PATCH 16/28] Improve remove_works --- frame/staking/src/voter_bags.rs | 43 ++++++++++++++++++--------------- 1 file changed, 24 insertions(+), 19 deletions(-) diff --git a/frame/staking/src/voter_bags.rs b/frame/staking/src/voter_bags.rs index 66b9854f94d56..080aba2e58041 100644 --- a/frame/staking/src/voter_bags.rs +++ b/frame/staking/src/voter_bags.rs @@ -1051,10 +1051,10 @@ mod voter_list { vec![ 51, 61, // best bag 11, 21, 101, // middle bag - 31, 71, // last bag; the new voter is last, because it is order of insertion + 31, + 71, // last bag; the new voter is last, because it is order of insertion ] ); - }) } @@ -1095,17 +1095,15 @@ mod voter_list { #[test] fn insert_works() { ExtBuilder::default().build_and_execute_without_check_count(|| { - // Insert into an existing bag: - // when - bond(42, 43, 999); + // when inserting into an existing bag + bond(42, 43, 1_000); VoterList::::insert(Voter::<_>::nominator(42), Pallet::::weight_of_fn()); // then assert_eq!(get_voter_list_as_ids(), vec![11, 21, 101, 42, 31]); assert_eq!(get_bags(), vec![(10, vec![31]), (1_000, vec![11, 21, 101, 42])]); - // Insert into a non-existent bag: - // when + // when inserting into a non-existent bag bond(422, 433, 1_001); VoterList::::insert(Voter::<_>::nominator(422), Pallet::::weight_of_fn()); @@ -1131,8 +1129,7 @@ mod voter_list { ]; assert_eq!(actual, expected); - // Insert a new voter with role: - // when + // when inserting a new voter VoterList::::insert_as(&42, VoterType::Nominator); // then @@ -1140,8 +1137,7 @@ mod voter_list { expected.push(Voter::<_>::nominator(42)); assert_eq!(actual, expected); - // Update the voter type of an already existing voter: - // when + // when updating the voter type of an already existing voter VoterList::::insert_as(&42, VoterType::Validator); // then @@ -1153,31 +1149,40 @@ mod voter_list { #[test] fn remove_works() { + use crate::VoterNodes; ExtBuilder::default().build_and_execute_without_check_count(|| { - // Remove a non-existent voter: - // when + // when removing a non-existent voter VoterList::::remove(&42); + assert!(!VoterBagFor::::contains_key(42)); + assert!(!VoterNodes::::contains_key(42)); // then nothing changes assert_eq!(get_voter_list_as_ids(), vec![11, 21, 101, 31]); assert_eq!(get_bags(), vec![(10, vec![31]), (1_000, vec![11, 21, 101])]); - // Remove from a bag with multiple nodes: - // when + // when removing a node from a bag with multiple nodes VoterList::::remove(&11); // then assert_eq!(get_voter_list_as_ids(), vec![21, 101, 31]); assert_eq!(get_bags(), vec![(10, vec![31]), (1_000, vec![21, 101])]); + assert!(!VoterBagFor::::contains_key(11)); + assert!(!VoterNodes::::contains_key(11)); - // Remove from a bag with only one node: + // when removing a node from a bag with only one node: VoterList::::remove(&31); // then assert_eq!(get_voter_list_as_ids(), vec![21, 101]); - assert_eq!(get_bags(), vec![(10, vec![]), (1_000, vec![21, 101])]); - - // TODO check storage is cleaned up + assert_eq!( + get_bags(), + vec![ + (10, vec![]), // the bag itself is not cleaned up from storage, which is ok + (1_000, vec![21, 101]) + ] + ); + assert!(!VoterBagFor::::contains_key(31)); + assert!(!VoterNodes::::contains_key(31)); }); } From 0e4429f6b8070f3ce3d2d9275e15f391fb765e17 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Mon, 26 Jul 2021 16:07:49 -0700 Subject: [PATCH 17/28] Update update_position_for_works; create set_ledger_and_free_balance --- frame/staking/src/mock.rs | 11 +++++++++++ frame/staking/src/voter_bags.rs | 20 ++++---------------- 2 files changed, 15 insertions(+), 16 deletions(-) diff --git a/frame/staking/src/mock.rs b/frame/staking/src/mock.rs index 3e159a2c42ed5..9863fe7bb712c 100644 --- a/frame/staking/src/mock.rs +++ b/frame/staking/src/mock.rs @@ -857,3 +857,14 @@ pub(crate) fn get_voter_list_as_ids() -> Vec { pub(crate) fn get_voter_list_as_voters() -> Vec> { VoterList::::iter().map(|node| node.voter().clone()).collect::>() } + +// Useful for when you want to change the effectively bonded value but you don't want to use +// the bond extrinsics because they implicitly rebag. +pub (crate) fn set_ledger_and_free_balance(account: &AccountId, value: Balance) { + Balances::make_free_balance_be(account, value); + let controller = Staking::bonded(account).unwrap(); + let mut ledger = Staking::ledger(&controller).unwrap(); + ledger.total = value; + ledger.active = value; + Staking::update_ledger(&controller, &ledger); +} \ No newline at end of file diff --git a/frame/staking/src/voter_bags.rs b/frame/staking/src/voter_bags.rs index 080aba2e58041..bb59a89788dcf 100644 --- a/frame/staking/src/voter_bags.rs +++ b/frame/staking/src/voter_bags.rs @@ -1197,12 +1197,7 @@ mod voter_list { // when account 31 bonds extra and needs to be moved to a non-existing higher bag // (we can't call bond_extra, because that implicitly calls update_position_for) - Balances::make_free_balance_be(&31, 11); - let controller = Staking::bonded(&31).unwrap(); - let mut ledger = Staking::ledger(&controller).unwrap(); - ledger.total = 11; - ledger.active = 11; - Staking::update_ledger(&controller, &ledger); + set_ledger_and_free_balance(&31, 11); assert!(node_31.is_misplaced(&weight_of)); assert_eq!(weight_of(&31), 11); @@ -1219,12 +1214,8 @@ mod voter_list { None, )); - // when account 31 bonds extra and needs to be moved to an existent higher bag - Balances::make_free_balance_be(&31, 1_000); - let mut ledger = Staking::ledger(&controller).unwrap(); - ledger.total = 61; - ledger.active = 61; - Staking::update_ledger(&controller, &ledger); + // when account 31 bonds extra and needs to be moved to an existing higher bag + set_ledger_and_free_balance(&31, 61); // then updating positions moves it to the correct bag let node_31 = Node::::from_id(&31).unwrap(); @@ -1239,10 +1230,7 @@ mod voter_list { assert_eq!(get_voter_list_as_ids(), vec![11, 21, 101, 31]); // when account 31 bonds extra but should not change bags - let mut ledger = Staking::ledger(&controller).unwrap(); - ledger.total = 1_000; - ledger.active = 1_000; - Staking::update_ledger(&controller, &ledger); + set_ledger_and_free_balance(&31, 1_000); // then nothing changes let node_31 = Node::::from_id(&31).unwrap(); From 7a96f3745415e0afa0cac81eb23eafd14f9b23a6 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Mon, 26 Jul 2021 16:21:06 -0700 Subject: [PATCH 18/28] Improve get_works --- frame/staking/src/voter_bags.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/frame/staking/src/voter_bags.rs b/frame/staking/src/voter_bags.rs index bb59a89788dcf..e9c645edcc5de 100644 --- a/frame/staking/src/voter_bags.rs +++ b/frame/staking/src/voter_bags.rs @@ -1297,11 +1297,14 @@ mod bags { #[test] fn get_works() { + use crate::VoterBags; ExtBuilder::default().build_and_execute_without_check_count(|| { let check_bag = |bag_upper, head, tail, ids| { assert_storage_noop!(Bag::::get(bag_upper)); + let bag = Bag::::get(bag_upper).unwrap(); let bag_ids = bag.iter().map(|n| n.voter().id).collect::>(); + assert_eq!(bag, Bag:: { head, tail, bag_upper }); assert_eq!(bag_ids, ids); }; @@ -1322,7 +1325,14 @@ mod bags { .filter(|bag_upper| !existing_bag_uppers.contains(bag_upper)) .for_each(|bag_upper| { assert_storage_noop!(assert_eq!(Bag::::get(*bag_upper), None)); + assert!(!VoterBags::::contains_key(*bag_upper)); }); + + // when we make a pre-existing bag empty + VoterList::::remove(&31); + + // then + check_bag(existing_bag_uppers[0], None, None, vec![]); }); } From d8bdcd6839127dfab8e18473f2940b1ce9cf83b1 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Mon, 26 Jul 2021 18:04:51 -0700 Subject: [PATCH 19/28] Improve storage clean up checks in remove test --- frame/staking/src/mock.rs | 4 +- frame/staking/src/voter_bags.rs | 248 ++++++++++++++------------------ 2 files changed, 111 insertions(+), 141 deletions(-) diff --git a/frame/staking/src/mock.rs b/frame/staking/src/mock.rs index 9863fe7bb712c..a08b2e671042d 100644 --- a/frame/staking/src/mock.rs +++ b/frame/staking/src/mock.rs @@ -860,11 +860,11 @@ pub(crate) fn get_voter_list_as_voters() -> Vec> { // Useful for when you want to change the effectively bonded value but you don't want to use // the bond extrinsics because they implicitly rebag. -pub (crate) fn set_ledger_and_free_balance(account: &AccountId, value: Balance) { +pub(crate) fn set_ledger_and_free_balance(account: &AccountId, value: Balance) { Balances::make_free_balance_be(account, value); let controller = Staking::bonded(account).unwrap(); let mut ledger = Staking::ledger(&controller).unwrap(); ledger.total = value; ledger.active = value; Staking::update_ledger(&controller, &ledger); -} \ No newline at end of file +} diff --git a/frame/staking/src/voter_bags.rs b/frame/staking/src/voter_bags.rs index e9c645edcc5de..33f5e0d610729 100644 --- a/frame/staking/src/voter_bags.rs +++ b/frame/staking/src/voter_bags.rs @@ -1087,11 +1087,6 @@ mod voter_list { }) } - #[test] - fn storage_is_cleaned_up_as_voters_are_removed() { - todo!() - } - #[test] fn insert_works() { ExtBuilder::default().build_and_execute_without_check_count(|| { @@ -1149,7 +1144,18 @@ mod voter_list { #[test] fn remove_works() { - use crate::VoterNodes; + use crate::{CounterForVoters, VoterNodes}; + + let check_storage = |id, counter, voters, bags| { + assert!(!VoterBagFor::::contains_key(id)); + assert!(!VoterNodes::::contains_key(id)); + assert_eq!(CounterForVoters::::get(), counter); + assert_eq!(VoterBagFor::::iter().count() as u32, counter); + assert_eq!(VoterNodes::::iter().count() as u32, counter); + assert_eq!(get_voter_list_as_ids(), voters); + assert_eq!(get_bags(), bags); + }; + ExtBuilder::default().build_and_execute_without_check_count(|| { // when removing a non-existent voter VoterList::::remove(&42); @@ -1159,30 +1165,59 @@ mod voter_list { // then nothing changes assert_eq!(get_voter_list_as_ids(), vec![11, 21, 101, 31]); assert_eq!(get_bags(), vec![(10, vec![31]), (1_000, vec![11, 21, 101])]); + assert_eq!(CounterForVoters::::get(), 4); // when removing a node from a bag with multiple nodes VoterList::::remove(&11); // then assert_eq!(get_voter_list_as_ids(), vec![21, 101, 31]); - assert_eq!(get_bags(), vec![(10, vec![31]), (1_000, vec![21, 101])]); - assert!(!VoterBagFor::::contains_key(11)); - assert!(!VoterNodes::::contains_key(11)); + check_storage( + 11, + 3, + vec![21, 101, 31], // voter list + vec![(10, vec![31]), (1_000, vec![21, 101])], // bags + ); // when removing a node from a bag with only one node: VoterList::::remove(&31); // then assert_eq!(get_voter_list_as_ids(), vec![21, 101]); - assert_eq!( - get_bags(), + check_storage( + 31, + 2, + vec![21, 101], // voter list vec![ + // bags (10, vec![]), // the bag itself is not cleaned up from storage, which is ok - (1_000, vec![21, 101]) - ] + (1_000, vec![21, 101]), + ], ); - assert!(!VoterBagFor::::contains_key(31)); - assert!(!VoterNodes::::contains_key(31)); + + // remove remaining voters to make sure storage cleans up as expected + VoterList::::remove(&21); + check_storage( + 21, + 1, + vec![101], // voter list + vec![(10, vec![]), (1_000, vec![101])], // bags + ); + + VoterList::::remove(&101); + check_storage( + 101, + 0, + Vec::::new(), // voter list + vec![(10, vec![]), (1_000, vec![])], // bags + ); + + // bags are not deleted via removals + assert_eq!(crate::VoterBags::::iter().count(), 2); + // nominator and validator counters are not updated at this level of the api + assert_eq!(crate::CounterForValidators::::get(), 3); + assert_eq!(crate::CounterForNominators::::get(), 1); + }); } @@ -1352,38 +1387,45 @@ mod bags { // when inserting into a bag with 1 node let mut bag_10 = Bag::::get(10).unwrap(); - // (note: 42 has no balance or ledger: bags api does not care) - bag_10.insert_node(node(Voter::<_>::nominator(42), bag_10.bag_upper)); + // (note: bags api does not care about balance or ledger) + bag_10.insert_node(node(Voter::nominator(42), bag_10.bag_upper)); // then assert_eq!(bag_as_ids(&bag_10), vec![31, 42]); // when inserting into a bag with 3 nodes let mut bag_1000 = Bag::::get(1_000).unwrap(); - bag_1000.insert_node(node(Voter::<_>::nominator(52), bag_1000.bag_upper)); + bag_1000.insert_node(node(Voter::nominator(52), bag_1000.bag_upper)); // then assert_eq!(bag_as_ids(&bag_1000), vec![11, 21, 101, 52]); // when inserting into a new bag let mut bag_20 = Bag::::get_or_make(20); - bag_20.insert_node(node(Voter::<_>::nominator(62), bag_20.bag_upper)); + bag_20.insert_node(node(Voter::nominator(71), bag_20.bag_upper)); // then - assert_eq!(bag_as_ids(&bag_20), vec![20]); + assert_eq!(bag_as_ids(&bag_20), vec![71]); // when inserting a node pointing to the accounts not in the bag - let voter_62 = Voter::<_>::validator(62); - let node_62 = Node:: { - voter: voter_62.clone(), + let voter_61 = Voter::validator(61); + let node_61 = Node:: { + voter: voter_61.clone(), prev: Some(21), next: Some(101), bag_upper: 20, }; - bag_20.insert_node(node_62); + bag_20.insert_node(node_61); // then ids are in order - assert_eq!(bag_as_ids(&bag_20), vec![20, 62]); + assert_eq!(bag_as_ids(&bag_20), vec![71, 61]); // and when the node is re-fetched all the info is correct assert_eq!( - Node::::get(20, &62).unwrap(), - Node:: { voter: voter_62, prev: Some(20), next: None, bag_upper: 20 } + Node::::get(20, &61).unwrap(), + Node:: { voter: voter_61, prev: Some(71), next: None, bag_upper: 20 } + ); + + // state of all bags is as expected + bag_20.put(); // need to put this bag so its in the storage map + assert_eq!( + get_bags(), + vec![(10, vec![31, 42]), (20, vec![71, 61]), (1_000, vec![11, 21, 101, 52])] ); }); } @@ -1398,10 +1440,16 @@ mod bags { bond_validator(61, 60, 2_000); bond_validator(71, 70, 2_000); + // given + assert_eq!( + get_bags(), + vec![(10, vec![31]), (1000, vec![11, 21, 101]), (2000, vec![51, 61, 71])], + ); + // when inserting a node with both prev & next pointing at an account in the bag - // in the bag and a incorrect bag_upper + // and an incorrect bag_upper let mut bag_1000 = Bag::::get(1_000).unwrap(); - let voter_42 = Voter::<_>::nominator(42); + let voter_42 = Voter::nominator(42); bag_1000.insert_node(node(voter_42.clone(), Some(11), Some(11), 0)); // then the ids are in the correct order assert_eq!(bag_as_ids(&bag_1000), vec![11, 21, 101, 42]); @@ -1415,7 +1463,7 @@ mod bags { let bag_1000_voter = bag_1000.iter().map(|node| node.voter().clone()).collect::>(); assert_eq!(bag_1000_voter[1], Voter::<_>::validator(21)); - // when inserting a node with id 21 but as a nominator + // when inserting a node with duplicate id 21 but as a nominator let voter_21_nom = Voter::<_>::nominator(21); bag_1000.insert_node(node(voter_21_nom.clone(), None, None, bag_1000.bag_upper)); // then all the nodes after the duplicate are lost (because it is set as the tail) @@ -1459,22 +1507,23 @@ mod bags { bond_validator(181, 180, 2_000); bond_validator(191, 190, 2_000); - // given - let mut bag_1000 = Bag::::get(1_000).unwrap(); - assert_eq!(bag_as_ids(&bag_1000), vec![11, 21, 101, 51, 61]); let mut bag_10 = Bag::::get(10).unwrap(); - assert_eq!(bag_as_ids(&bag_10), vec![31, 71, 81]); + let mut bag_1000 = Bag::::get(1_000).unwrap(); let mut bag_2000 = Bag::::get(2_000).unwrap(); + + // given + assert_eq!(bag_as_ids(&bag_10), vec![31, 71, 81]); + assert_eq!(bag_as_ids(&bag_1000), vec![11, 21, 101, 51, 61]); assert_eq!(bag_as_ids(&bag_2000), vec![91, 161, 171, 181, 191]); - // remove node from center that is not pointing at head or tail + // remove node that is not pointing at head or tail let node_101 = Node::::get(bag_1000.bag_upper, &101).unwrap(); let node_101_pre_remove = node_101.clone(); bag_1000.remove_node(&node_101); assert_eq!(bag_as_ids(&bag_1000), vec![11, 21, 51, 61]); + assert_ok!(bag_1000.sanity_check()); // node isn't mutated when its removed assert_eq!(node_101, node_101_pre_remove); - assert_ok!(bag_1000.sanity_check()); // remove head when its not pointing at tail let node_11 = Node::::get(bag_1000.bag_upper, &11).unwrap(); @@ -1523,13 +1572,20 @@ mod bags { bag_2000.remove_node(&node_181); assert_eq!(bag_as_ids(&bag_2000), vec![91, 171, 191]); assert_ok!(bag_2000.sanity_check()); + + // state of all bags is as expected + assert_eq!( + get_bags(), + vec![(10, vec![81]), (1_000, vec![]), (2_000, vec![91, 171, 191])] + ); }); } #[test] fn remove_node_bad_paths_documented() { ExtBuilder::default().build_and_execute_without_check_count(|| { - // removing node that is in the bag but has the wrong upper works + // removing a node that is in the bag but has the wrong upper works. + let bad_upper_node_11 = Node:: { voter: Voter::<_>::validator(11), prev: None, @@ -1548,7 +1604,7 @@ mod bags { }); ExtBuilder::default().build_and_execute_without_check_count(|| { - // removing a node that is in an other bag, will mess up the + // removing a node that is in another bag, will mess up the // other bag. let node_101 = Node::::get(1_000, &101).unwrap(); @@ -1564,7 +1620,7 @@ mod bags { assert_eq!(bag_10.tail, Some(31)); assert_eq!(bag_10.head, Some(31)); - // but the bag that actually had the node is in an invalid state + // but the bag that the node belonged to is in an invalid state let bag_1000 = Bag::::get(1_000).unwrap(); // because it still has the removed node as its tail. assert_eq!(bag_1000.tail, Some(101)); @@ -1578,7 +1634,6 @@ mod bags { mod voter_node { use super::*; use crate::mock::*; - use frame_support::traits::Currency; #[test] fn voting_data_works() { @@ -1592,11 +1647,11 @@ mod voter_node { assert_eq!( get_voter_list_as_voters(), vec![ - Voter::<_>::validator(11), - Voter::<_>::validator(21), - Voter::<_>::nominator(101), - Voter::<_>::nominator(42), - Voter::<_>::validator(31), + Voter::validator(11), + Voter::validator(21), + Voter::nominator(101), + Voter::nominator(42), + Voter::validator(31), ] ); assert_eq!(active_era(), 0); @@ -1623,8 +1678,9 @@ mod voter_node { (42, 1_000, vec![11]) ); - run_to_block(20); - assert_eq!(active_era(), 1); + // roll ahead an era so any slashes will be after the previous nominations + start_active_era(1); + // when a validator gets a slash, add_slash(&11); let slashing_spans = @@ -1635,10 +1691,10 @@ mod voter_node { assert_eq!( get_voter_list_as_voters(), vec![ - Voter::<_>::validator(21), - Voter::<_>::nominator(101), - Voter::<_>::nominator(42), - Voter::<_>::validator(31), + Voter::validator(21), + Voter::nominator(101), + Voter::nominator(42), + Voter::validator(31), ] ); // and its nominators no longer have it as a target @@ -1651,7 +1707,7 @@ mod voter_node { let node_42 = Node::::get(10, &42).unwrap(); assert_eq!( node_42.voting_data(&weight_of, &slashing_spans), - None, // no voting data since its target(s) have been slashed since nominating + None, // no voting data since its 1 target has been slashed since nominating ); }); } @@ -1669,96 +1725,10 @@ mod voter_node { // and will become misplaced if its slashable balance does not // correspond to the bag it is in. - Balances::make_free_balance_be(&31, 11); - let controller = Staking::bonded(&31).unwrap(); - let mut ledger = Staking::ledger(&controller).unwrap(); - ledger.total = 11; - ledger.active = 11; - Staking::update_ledger(&controller, &ledger); + set_ledger_and_free_balance(&31, 11); assert_eq!(Staking::slashable_balance_of(&31), 11); assert!(node_31.is_misplaced(&weight_of)); }); } } - -// 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. -/* - #[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!(::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!(::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!(::CounterForVoters::get(), 0); - - - // TODO bags do not get cleaned up from storages - // - 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); - assert_eq!(::VoterBagFor::iter().collect::>().len(), 0); - assert_eq!(::VoterNodes::iter().collect::>().len(), 0); - }); - } -} -*/ From 2522d7537fda2d8fdd00ffbcdab182f0a9f7a323 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Mon, 26 Jul 2021 20:29:20 -0700 Subject: [PATCH 20/28] Test: impl rebag_works + insert_and_remove_works --- frame/staking/src/tests.rs | 68 ++++++++++++++++++++++++++++++++- frame/staking/src/voter_bags.rs | 3 +- 2 files changed, 68 insertions(+), 3 deletions(-) diff --git a/frame/staking/src/tests.rs b/frame/staking/src/tests.rs index 4ae38611492e5..e06c6534ccf70 100644 --- a/frame/staking/src/tests.rs +++ b/frame/staking/src/tests.rs @@ -3827,10 +3827,76 @@ fn on_finalize_weight_is_nonzero() { // end-to-end nodes of the voter bags operation. mod voter_bags { + use crate::{mock::*, ValidatorPrefs}; + use frame_support::{assert_ok, traits::Currency}; + use super::Origin; + + #[test] + fn insert_and_remove_works() { + // we test insert/remove indirectly via `validate`, `nominate`, and chill + + // given + assert_eq!(get_bags(), vec![(10, vec![31]), (1000, vec![11, 21, 101])]); + + // `bond` + bond(42, 43, 2_000); + // does not insert the voter + assert_eq!(get_bags(), vec![(10, vec![31]), (1000, vec![11, 21, 101])]); + + // `validate` + assert_ok!(Staking::validate(Origin::signed(43).into(), ValidatorPrefs::default())); + // moves the voter into a bag + assert_eq!(get_bags(), vec![(10, vec![31]), (1000, vec![11, 21, 101]), (2000, vec![42])]); + + // `nominate`-ing, but not changing active stake (which implicitly calls remove) + assert_ok!(Staking::nominate(Origin::signed(43), vec![11])); + // does not change the voters position + assert_eq!(get_bags(), vec![(10, vec![31]), (1000, vec![11, 21, 101]), (2000, vec![42])]); + + // `chill` + assert_ok!(Staking::chill(Origin::signed(43))); + // removes the voter + assert_eq!(get_bags(), vec![(10, vec![31]), (1000, vec![11, 21, 101]), (2000, vec![])]); + } #[test] fn rebag_works() { - todo!() + ExtBuilder::default().build_and_execute(|| { + // add a nominator to genesis state + bond_nominator(42, 43, 20, vec![11]); + Balances::make_free_balance_be(&42, 2_000); + + // given + assert_eq!(get_bags(), vec![(10, vec![31]), (20, vec![42]), (1000, vec![11, 21, 101])]); + + // increase stake and implicitly rebag with `bond_extra` to the level of non-existent bag + assert_ok!(Staking::bond_extra(Origin::signed(42), 1_980)); // 20 + 1_980 = 2_000 + assert_eq!(get_bags(), vec![(10, vec![31]), (20, vec![]), (1000, vec![11, 21, 101]), (2000, vec![42])]); + + // decrease stake within the range of the current bag + assert_ok!(Staking::unbond(Origin::signed(43), 999)); // 2000 - 999 = 1001 + assert_ok!(Staking::rebag(Origin::signed(43), 42)); + // does not change bags + assert_eq!(get_bags(), vec![(10, vec![31]), (20, vec![]), (1000, vec![11, 21, 101]), (2000, vec![42])]); + + // reduce stake to the level of a non-existent bag + assert_ok!(Staking::unbond(Origin::signed(43), 971)); // 1001 - 971 = 30 + assert_ok!(Staking::rebag(Origin::signed(43), 42)); + // creates the bag and moves the voter into it + assert_eq!( + get_bags(), + vec![(10, vec![31]), (20, vec![]), (30, vec![42]), (1000, vec![11, 21, 101]), (2000, vec![])] + ); + + // increase stake by `rebond`-ing to the level of a pre-existing bag + assert_ok!(Staking::rebond(Origin::signed(43), 31)); // 30 + 41 = 61 + assert_ok!(Staking::rebag(Origin::signed(43), 42)); + // moves the voter to that bag + assert_eq!( + get_bags(), + vec![(10, vec![31]), (20, vec![]), (30, vec![]), (1000, vec![11, 21, 101, 42]), (2000, vec![])] + ); + }); } } /* diff --git a/frame/staking/src/voter_bags.rs b/frame/staking/src/voter_bags.rs index 33f5e0d610729..0f2e2f9c21ab1 100644 --- a/frame/staking/src/voter_bags.rs +++ b/frame/staking/src/voter_bags.rs @@ -994,7 +994,7 @@ mod voter_list { // ^^ note the order of insertion in genesis! ); - assert_eq!(get_bags(), vec![(10, vec![31]), (1000, vec![11, 21, 101])],); + assert_eq!(get_bags(), vec![(10, vec![31]), (1000, vec![11, 21, 101])]); }) } @@ -1217,7 +1217,6 @@ mod voter_list { // nominator and validator counters are not updated at this level of the api assert_eq!(crate::CounterForValidators::::get(), 3); assert_eq!(crate::CounterForNominators::::get(), 1); - }); } From de378b509c9e9c26fab7bc7ab1606e0eeeb82f53 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Mon, 26 Jul 2021 20:29:53 -0700 Subject: [PATCH 21/28] forgot file - Test: impl rebag_works + insert_and_remove_works --- frame/staking/src/tests.rs | 149 ++++++++++++------------------------- 1 file changed, 49 insertions(+), 100 deletions(-) diff --git a/frame/staking/src/tests.rs b/frame/staking/src/tests.rs index e06c6534ccf70..bbf105d6908bd 100644 --- a/frame/staking/src/tests.rs +++ b/frame/staking/src/tests.rs @@ -3827,36 +3827,43 @@ fn on_finalize_weight_is_nonzero() { // end-to-end nodes of the voter bags operation. mod voter_bags { + use super::Origin; use crate::{mock::*, ValidatorPrefs}; use frame_support::{assert_ok, traits::Currency}; - use super::Origin; #[test] fn insert_and_remove_works() { // we test insert/remove indirectly via `validate`, `nominate`, and chill + ExtBuilder::default().build_and_execute(|| { + // given + assert_eq!(get_bags(), vec![(10, vec![31]), (1000, vec![11, 21, 101])]); - // given - assert_eq!(get_bags(), vec![(10, vec![31]), (1000, vec![11, 21, 101])]); - - // `bond` - bond(42, 43, 2_000); - // does not insert the voter - assert_eq!(get_bags(), vec![(10, vec![31]), (1000, vec![11, 21, 101])]); + // `bond` + bond(42, 43, 2_000); + // does not insert the voter + assert_eq!(get_bags(), vec![(10, vec![31]), (1000, vec![11, 21, 101])]); - // `validate` - assert_ok!(Staking::validate(Origin::signed(43).into(), ValidatorPrefs::default())); - // moves the voter into a bag - assert_eq!(get_bags(), vec![(10, vec![31]), (1000, vec![11, 21, 101]), (2000, vec![42])]); + // `validate` + assert_ok!(Staking::validate(Origin::signed(43).into(), ValidatorPrefs::default())); + // moves the voter into a bag + assert_eq!( + get_bags(), + vec![(10, vec![31]), (1000, vec![11, 21, 101]), (2000, vec![42])] + ); - // `nominate`-ing, but not changing active stake (which implicitly calls remove) - assert_ok!(Staking::nominate(Origin::signed(43), vec![11])); - // does not change the voters position - assert_eq!(get_bags(), vec![(10, vec![31]), (1000, vec![11, 21, 101]), (2000, vec![42])]); + // `nominate`-ing, but not changing active stake (which implicitly calls remove) + assert_ok!(Staking::nominate(Origin::signed(43), vec![11])); + // does not change the voters position + assert_eq!( + get_bags(), + vec![(10, vec![31]), (1000, vec![11, 21, 101]), (2000, vec![42])] + ); - // `chill` - assert_ok!(Staking::chill(Origin::signed(43))); - // removes the voter - assert_eq!(get_bags(), vec![(10, vec![31]), (1000, vec![11, 21, 101]), (2000, vec![])]); + // `chill` + assert_ok!(Staking::chill(Origin::signed(43))); + // removes the voter + assert_eq!(get_bags(), vec![(10, vec![31]), (1000, vec![11, 21, 101]), (2000, vec![])]); + }); } #[test] @@ -3871,13 +3878,19 @@ mod voter_bags { // increase stake and implicitly rebag with `bond_extra` to the level of non-existent bag assert_ok!(Staking::bond_extra(Origin::signed(42), 1_980)); // 20 + 1_980 = 2_000 - assert_eq!(get_bags(), vec![(10, vec![31]), (20, vec![]), (1000, vec![11, 21, 101]), (2000, vec![42])]); + assert_eq!( + get_bags(), + vec![(10, vec![31]), (20, vec![]), (1000, vec![11, 21, 101]), (2000, vec![42])] + ); // decrease stake within the range of the current bag assert_ok!(Staking::unbond(Origin::signed(43), 999)); // 2000 - 999 = 1001 assert_ok!(Staking::rebag(Origin::signed(43), 42)); // does not change bags - assert_eq!(get_bags(), vec![(10, vec![31]), (20, vec![]), (1000, vec![11, 21, 101]), (2000, vec![42])]); + assert_eq!( + get_bags(), + vec![(10, vec![31]), (20, vec![]), (1000, vec![11, 21, 101]), (2000, vec![42])] + ); // reduce stake to the level of a non-existent bag assert_ok!(Staking::unbond(Origin::signed(43), 971)); // 1001 - 971 = 30 @@ -3885,7 +3898,13 @@ mod voter_bags { // creates the bag and moves the voter into it assert_eq!( get_bags(), - vec![(10, vec![31]), (20, vec![]), (30, vec![42]), (1000, vec![11, 21, 101]), (2000, vec![])] + vec![ + (10, vec![31]), + (20, vec![]), + (30, vec![42]), + (1000, vec![11, 21, 101]), + (2000, vec![]) + ] ); // increase stake by `rebond`-ing to the level of a pre-existing bag @@ -3894,87 +3913,17 @@ mod voter_bags { // moves the voter to that bag assert_eq!( get_bags(), - vec![(10, vec![31]), (20, vec![]), (30, vec![]), (1000, vec![11, 21, 101, 42]), (2000, vec![])] + vec![ + (10, vec![31]), + (20, vec![]), + (30, vec![]), + (1000, vec![11, 21, 101, 42]), + (2000, vec![]) + ] ); }); } } -/* -// 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::{ - testing_utils::create_stash_controller, - voter_bags::{Bag, Node}, - }; - use frame_system::RawOrigin; - - /// Make a validator and return its stash - fn make_validator(n: u32, balance_factor: u32) -> Result, &'static str> { - let (stash, controller) = create_stash_controller::(n, balance_factor, Default::default()).unwrap(); - - // Bond the full value of the stash - // - // By default, `create_stash_controller` only bonds 10% of the stash. However, we're going - // to want to edit one account's bonded value to match another's, so it's simpler if 100% of - // the balance is bonded. - let balance = ::Currency::free_balance(&stash); - Staking::bond_extra(RawOrigin::Signed(stash.clone()).into(), balance).unwrap(); - Staking::validate( - RawOrigin::Signed(controller.clone()).into(), - ValidatorPrefs::default(), - ).unwrap(); - - Ok(stash) - } - - 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, 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(); - assert_eq!( - { - let origin_bag = Bag::::get(node.bag_upper).unwrap(); - origin_bag.iter().count() - }, - 1, - "stash should be the only node in origin bag", - ); - let other_node = Node::::from_id(&other_stash).unwrap(); - assert!(!other_node.is_misplaced(&weight_of), "other stash balance never changed"); - assert_ne!( - { - let destination_bag = Bag::::get(other_node.bag_upper); - destination_bag.iter().count() - }, - 0, - "destination bag should not be empty", - ); - - // Update `stash`'s value to match `other_stash`, and bond extra to update its weight. - // - // This implicitly calls rebag, so the user stays in the best bag they qualify for. - let new_balance = ::Currency::free_balance(&other_stash); - ::Currency::make_free_balance_be(&stash, new_balance); - Staking::bond_extra( - RawOrigin::Signed(stash.clone()).into(), - new_balance, - ).unwrap(); - - // node should no longer be misplaced - // note that we refresh the node, in case the storage value has changed - let node = Node::::from_id(&stash).unwrap(); - assert!(!node.is_misplaced(&weight_of), "node must be in proper place after rebag"); - }); -} -*/ mod election_data_provider { use super::*; From e7fdad2aa55c5e71ee209a11baf8b3014248f260 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Tue, 27 Jul 2021 10:30:22 +0200 Subject: [PATCH 22/28] Small tweak --- frame/staking/src/voter_bags.rs | 34 ++++++++++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-) diff --git a/frame/staking/src/voter_bags.rs b/frame/staking/src/voter_bags.rs index 0f2e2f9c21ab1..3c034dcb81a39 100644 --- a/frame/staking/src/voter_bags.rs +++ b/frame/staking/src/voter_bags.rs @@ -245,6 +245,7 @@ impl VoterList { // clear the old bag head/tail pointers as necessary if let Some(mut bag) = Bag::::get(node.bag_upper) { + dbg!(&bag); bag.remove_node(&node); bag.put(); } else { @@ -431,9 +432,18 @@ impl Bag { Self::get(bag_upper).unwrap_or(Bag { bag_upper, ..Default::default() }) } + /// `True` if self is empty. + pub fn is_empty(&self) -> bool { + self.head.is_none() && self.tail.is_none() + } + /// Put the bag back into storage. pub fn put(self) { - crate::VoterBags::::insert(self.bag_upper, self); + if self.is_empty() { + crate::VoterBags::::remove(self.bag_upper); + } else { + crate::VoterBags::::insert(self.bag_upper, self); + } } /// Get the head node in this bag. @@ -1016,6 +1026,28 @@ mod voter_list { assert_eq!(notional_bag_for::(max_explicit_threshold + 1), VoteWeight::MAX); } + #[test] + fn remove_last_voter_in_bags_cleans_bag() { + ExtBuilder::default().build_and_execute(|| { + // given + assert_eq!(get_bags(), vec![(10, vec![31]), (1000, vec![11, 21, 101])]); + + // give 31 more stake to bump it to a new bag. + Balances::make_free_balance_be(&31, 10000); + assert_ok!(Staking::bond_extra(Origin::signed(31), 10000 - 10)); + + // then the bag with bound 10 is wiped from storage. + assert_eq!(get_bags(), vec![(1000, vec![11, 21, 101]), (10_000, vec![31])]); + + // and can be recreated again as needed + bond_validator(77, 777, 10); + assert_eq!( + get_bags(), + vec![(10, vec![77]), (1000, vec![11, 21, 101]), (10_000, vec![31])] + ); + }); + } + #[test] fn iteration_is_semi_sorted() { ExtBuilder::default().build_and_execute(|| { From 724c17ef1bf4005a033c04ee1e5d8e4d79240714 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Tue, 27 Jul 2021 11:16:05 -0700 Subject: [PATCH 23/28] Update voter_bags test to reflect unused bags are removed --- frame/staking/src/tests.rs | 25 ++------- frame/staking/src/voter_bags.rs | 92 +++++++-------------------------- 2 files changed, 25 insertions(+), 92 deletions(-) diff --git a/frame/staking/src/tests.rs b/frame/staking/src/tests.rs index bbf105d6908bd..0ea5576319ad0 100644 --- a/frame/staking/src/tests.rs +++ b/frame/staking/src/tests.rs @@ -3862,7 +3862,7 @@ mod voter_bags { // `chill` assert_ok!(Staking::chill(Origin::signed(43))); // removes the voter - assert_eq!(get_bags(), vec![(10, vec![31]), (1000, vec![11, 21, 101]), (2000, vec![])]); + assert_eq!(get_bags(), vec![(10, vec![31]), (1000, vec![11, 21, 101])]); }); } @@ -3880,7 +3880,7 @@ mod voter_bags { assert_ok!(Staking::bond_extra(Origin::signed(42), 1_980)); // 20 + 1_980 = 2_000 assert_eq!( get_bags(), - vec![(10, vec![31]), (20, vec![]), (1000, vec![11, 21, 101]), (2000, vec![42])] + vec![(10, vec![31]), (1000, vec![11, 21, 101]), (2000, vec![42])] ); // decrease stake within the range of the current bag @@ -3889,7 +3889,7 @@ mod voter_bags { // does not change bags assert_eq!( get_bags(), - vec![(10, vec![31]), (20, vec![]), (1000, vec![11, 21, 101]), (2000, vec![42])] + vec![(10, vec![31]), (1000, vec![11, 21, 101]), (2000, vec![42])] ); // reduce stake to the level of a non-existent bag @@ -3898,29 +3898,14 @@ mod voter_bags { // creates the bag and moves the voter into it assert_eq!( get_bags(), - vec![ - (10, vec![31]), - (20, vec![]), - (30, vec![42]), - (1000, vec![11, 21, 101]), - (2000, vec![]) - ] + vec![(10, vec![31]), (30, vec![42]), (1000, vec![11, 21, 101]),] ); // increase stake by `rebond`-ing to the level of a pre-existing bag assert_ok!(Staking::rebond(Origin::signed(43), 31)); // 30 + 41 = 61 assert_ok!(Staking::rebag(Origin::signed(43), 42)); // moves the voter to that bag - assert_eq!( - get_bags(), - vec![ - (10, vec![31]), - (20, vec![]), - (30, vec![]), - (1000, vec![11, 21, 101, 42]), - (2000, vec![]) - ] - ); + assert_eq!(get_bags(), vec![(10, vec![31]), (1000, vec![11, 21, 101, 42]),]); }); } } diff --git a/frame/staking/src/voter_bags.rs b/frame/staking/src/voter_bags.rs index 3c034dcb81a39..fd61834e1c0ea 100644 --- a/frame/staking/src/voter_bags.rs +++ b/frame/staking/src/voter_bags.rs @@ -245,7 +245,6 @@ impl VoterList { // clear the old bag head/tail pointers as necessary if let Some(mut bag) = Bag::::get(node.bag_upper) { - dbg!(&bag); bag.remove_node(&node); bag.put(); } else { @@ -1061,12 +1060,9 @@ mod voter_list { 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, + get_voter_list_as_ids(), vec![ 51, 61, // best bag 11, 21, 101, // middle bag @@ -1079,7 +1075,7 @@ mod voter_list { // then assert_eq!( - iteration, + get_voter_list_as_ids(), vec![ 51, 61, // best bag 11, 21, 101, // middle bag @@ -1176,7 +1172,7 @@ mod voter_list { #[test] fn remove_works() { - use crate::{CounterForVoters, VoterNodes}; + use crate::{CounterForVoters, VoterNodes, VoterBags}; let check_storage = |id, counter, voters, bags| { assert!(!VoterBagFor::::contains_key(id)); @@ -1219,33 +1215,31 @@ mod voter_list { check_storage( 31, 2, - vec![21, 101], // voter list - vec![ - // bags - (10, vec![]), // the bag itself is not cleaned up from storage, which is ok - (1_000, vec![21, 101]), - ], + vec![21, 101], // voter list + vec![(1_000, vec![21, 101])], // bags ); + assert!(!VoterBags::::contains_key(10)); // bag 10 is removed // remove remaining voters to make sure storage cleans up as expected VoterList::::remove(&21); check_storage( 21, 1, - vec![101], // voter list - vec![(10, vec![]), (1_000, vec![101])], // bags + vec![101], // voter list + vec![(1_000, vec![101])], // bags ); VoterList::::remove(&101); check_storage( 101, 0, - Vec::::new(), // voter list - vec![(10, vec![]), (1_000, vec![])], // bags + Vec::::new(), // voter list + vec![], // bags ); + assert!(!VoterBags::::contains_key(1_000)); // bag 1_000 is removed - // bags are not deleted via removals - assert_eq!(crate::VoterBags::::iter().count(), 2); + // bags are deleted via removals + assert_eq!(VoterBags::::iter().count(), 0); // nominator and validator counters are not updated at this level of the api assert_eq!(crate::CounterForValidators::::get(), 3); assert_eq!(crate::CounterForNominators::::get(), 1); @@ -1270,7 +1264,7 @@ mod voter_list { // then updating position moves it to the correct bag assert_eq!(VoterList::::update_position_for(node_31, &weight_of), Some((10, 20))); - assert_eq!(get_bags(), vec![(10, vec![]), (20, vec![31]), (1_000, vec![11, 21, 101])]); + assert_eq!(get_bags(), vec![(20, vec![31]), (1_000, vec![11, 21, 101])]); assert_eq!(get_voter_list_as_ids(), vec![11, 21, 101, 31]); // and if you try and update the position with no change in active stake nothing changes @@ -1291,7 +1285,7 @@ mod voter_list { ); assert_eq!( get_bags(), - vec![(10, vec![]), (20, vec![]), (1_000, vec![11, 21, 101, 31])] + vec![(1_000, vec![11, 21, 101, 31])] ); assert_eq!(get_voter_list_as_ids(), vec![11, 21, 101, 31]); @@ -1306,53 +1300,6 @@ mod voter_list { )); }); } - - // TODO: should probs remove this; Just wanted to document an edge case - // I ran into where I read in a node, called bond_extra, and then called - // update_position_for(node). The underlying issue is we insert a node with - // the same ID twice. - // When we insert we set node.prev = tail.id - // and then old_tail.next = node.id. Which creates a cycle since old_tail - // was the same node - // tl;dr, don't insert the same id into a bag twice, especially if its tail - // cover this explicitly in insert_node_bad_paths_documented - #[test] - fn node_reference_invalid_after_implicit_update_position() { - ExtBuilder::default().build_and_execute_without_check_count(|| { - let weight_of = Staking::weight_of_fn(); - - // starts out in the correct place. - let node_31 = Node::::from_id(&31).unwrap(); - assert!(!node_31.is_misplaced(&weight_of)); - - Balances::make_free_balance_be(&31, 11); - // when we call bond_extra - assert_ok!(Staking::bond_extra(Origin::signed(31), 11)); - - // then now the reference is outdated - assert!(node_31.is_misplaced(&weight_of)); - // but we can expect the list is correct - assert_eq!(get_bags(), vec![(10, vec![]), (20, vec![31]), (1_000, vec![11, 21, 101])]); - assert_eq!(get_voter_list_as_ids(), vec![11, 21, 101, 31]); - - assert_eq!(VoterList::::update_position_for(node_31, &weight_of), Some((10, 20))); - - let bag = Bag::::get(20).unwrap(); - // there is a cycle, - // the bags head/tail are the same node, and that node has its own - // id for prev & next so when we iterate we will infinitely keep - // reading the same node from storage. - assert_eq!( - bag.head().unwrap().voter().id, - bag.head().unwrap().next().unwrap().voter().id - ); - assert_eq!( - bag.head().unwrap().voter().id, - bag.head().unwrap().prev().unwrap().voter().id - ); - assert_eq!(bag.head().unwrap().voter().id, bag.tail().unwrap().voter().id,) - }); - } } #[cfg(test)] @@ -1398,7 +1345,7 @@ mod bags { VoterList::::remove(&31); // then - check_bag(existing_bag_uppers[0], None, None, vec![]); + assert_eq!(Bag::::get(existing_bag_uppers[0]), None) }); } @@ -1577,8 +1524,8 @@ mod bags { // remove node that is head & tail let node_21 = Node::::get(bag_1000.bag_upper, &21).unwrap(); bag_1000.remove_node(&node_21); - assert_eq!(bag_as_ids(&bag_1000), Vec::::new()); - assert_ok!(bag_1000.sanity_check()); + bag_1000.put(); // put into storage so get returns the updated bag + assert_eq!(Bag::::get(1_000), None); // remove node that is pointing at head and tail let node_71 = Node::::get(bag_10.bag_upper, &71).unwrap(); @@ -1591,6 +1538,7 @@ mod bags { bag_10.remove_node(&node_31); assert_eq!(bag_as_ids(&bag_10), vec![81]); assert_ok!(bag_10.sanity_check()); + bag_10.put(); // since we updated the bag's head/tail, we need to write this storage // remove node that is pointing at head, but not tail let node_161 = Node::::get(bag_2000.bag_upper, &161).unwrap(); @@ -1607,7 +1555,7 @@ mod bags { // state of all bags is as expected assert_eq!( get_bags(), - vec![(10, vec![81]), (1_000, vec![]), (2_000, vec![91, 171, 191])] + vec![(10, vec![81]), (2_000, vec![91, 171, 191])] ); }); } From 887035169fabb7778e806c8a6f36fa6c092fe5e2 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Tue, 27 Jul 2021 11:34:30 -0700 Subject: [PATCH 24/28] Unbond & Rebond: do_rebag --- frame/staking/src/lib.rs | 2 ++ frame/staking/src/tests.rs | 3 --- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index 3574c34602bbb..94e962b571fe6 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -1781,6 +1781,7 @@ pub mod pallet { let era = Self::current_era().unwrap_or(0) + T::BondingDuration::get(); ledger.unlocking.push(UnlockChunk { value, era }); Self::update_ledger(&controller, &ledger); + Self::do_rebag(&ledger.stash); Self::deposit_event(Event::::Unbonded(ledger.stash, value)); } Ok(()) @@ -2273,6 +2274,7 @@ pub mod pallet { Self::deposit_event(Event::::Bonded(ledger.stash.clone(), value)); Self::update_ledger(&controller, &ledger); + Self::do_rebag(&ledger.stash); Ok(Some( 35 * WEIGHT_PER_MICROS + 50 * WEIGHT_PER_NANOS * (ledger.unlocking.len() as Weight) + diff --git a/frame/staking/src/tests.rs b/frame/staking/src/tests.rs index 0ea5576319ad0..4db46a1bf7624 100644 --- a/frame/staking/src/tests.rs +++ b/frame/staking/src/tests.rs @@ -3885,7 +3885,6 @@ mod voter_bags { // decrease stake within the range of the current bag assert_ok!(Staking::unbond(Origin::signed(43), 999)); // 2000 - 999 = 1001 - assert_ok!(Staking::rebag(Origin::signed(43), 42)); // does not change bags assert_eq!( get_bags(), @@ -3894,7 +3893,6 @@ mod voter_bags { // reduce stake to the level of a non-existent bag assert_ok!(Staking::unbond(Origin::signed(43), 971)); // 1001 - 971 = 30 - assert_ok!(Staking::rebag(Origin::signed(43), 42)); // creates the bag and moves the voter into it assert_eq!( get_bags(), @@ -3903,7 +3901,6 @@ mod voter_bags { // increase stake by `rebond`-ing to the level of a pre-existing bag assert_ok!(Staking::rebond(Origin::signed(43), 31)); // 30 + 41 = 61 - assert_ok!(Staking::rebag(Origin::signed(43), 42)); // moves the voter to that bag assert_eq!(get_bags(), vec![(10, vec![31]), (1000, vec![11, 21, 101, 42]),]); }); From ec7b7cc223ffeb634ae7cd357a1500fdcfd4a1e7 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Tue, 27 Jul 2021 13:32:02 -0700 Subject: [PATCH 25/28] Prevent infinite loops with duplicate tail insert --- frame/staking/src/tests.rs | 6 +-- frame/staking/src/voter_bags.rs | 92 +++++++++++++++++++-------------- 2 files changed, 57 insertions(+), 41 deletions(-) diff --git a/frame/staking/src/tests.rs b/frame/staking/src/tests.rs index 4db46a1bf7624..b881f68b29b3c 100644 --- a/frame/staking/src/tests.rs +++ b/frame/staking/src/tests.rs @@ -3885,7 +3885,7 @@ mod voter_bags { // decrease stake within the range of the current bag assert_ok!(Staking::unbond(Origin::signed(43), 999)); // 2000 - 999 = 1001 - // does not change bags + // does not change bags assert_eq!( get_bags(), vec![(10, vec![31]), (1000, vec![11, 21, 101]), (2000, vec![42])] @@ -3893,7 +3893,7 @@ mod voter_bags { // reduce stake to the level of a non-existent bag assert_ok!(Staking::unbond(Origin::signed(43), 971)); // 1001 - 971 = 30 - // creates the bag and moves the voter into it + // creates the bag and moves the voter into it assert_eq!( get_bags(), vec![(10, vec![31]), (30, vec![42]), (1000, vec![11, 21, 101]),] @@ -3901,7 +3901,7 @@ mod voter_bags { // increase stake by `rebond`-ing to the level of a pre-existing bag assert_ok!(Staking::rebond(Origin::signed(43), 31)); // 30 + 41 = 61 - // moves the voter to that bag + // moves the voter to that bag assert_eq!(get_bags(), vec![(10, vec![31]), (1000, vec![11, 21, 101, 42]),]); }); } diff --git a/frame/staking/src/voter_bags.rs b/frame/staking/src/voter_bags.rs index fd61834e1c0ea..04d1943164bfb 100644 --- a/frame/staking/src/voter_bags.rs +++ b/frame/staking/src/voter_bags.rs @@ -479,6 +479,15 @@ impl Bag { /// Storage note: this modifies storage, but only for the node. You still need to call /// `self.put()` after use. fn insert_node(&mut self, mut node: Node) { + if let Some(tail) = &self.tail { + if *tail == node.voter.id { + // this should never happen, but this check prevents a worst case infinite loop + debug_assert!(false, "system logic error: inserting a node who has the id of tail"); + crate::log!(warn, "system logic error: inserting a node who has the id of tail"); + return + }; + } + let id = node.voter.id.clone(); node.prev = self.tail.clone(); @@ -1172,7 +1181,7 @@ mod voter_list { #[test] fn remove_works() { - use crate::{CounterForVoters, VoterNodes, VoterBags}; + use crate::{CounterForVoters, VoterBags, VoterNodes}; let check_storage = |id, counter, voters, bags| { assert!(!VoterBagFor::::contains_key(id)); @@ -1283,10 +1292,7 @@ mod voter_list { VoterList::::update_position_for(node_31, &weight_of), Some((20, 1_000)) ); - assert_eq!( - get_bags(), - vec![(1_000, vec![11, 21, 101, 31])] - ); + assert_eq!(get_bags(), vec![(1_000, vec![11, 21, 101, 31])]); assert_eq!(get_voter_list_as_ids(), vec![11, 21, 101, 31]); // when account 31 bonds extra but should not change bags @@ -1411,24 +1417,14 @@ mod bags { // Document improper ways `insert_node` may be getting used. #[test] fn insert_node_bad_paths_documented() { + let node = |voter, prev, next, bag_upper| Node:: { voter, prev, next, bag_upper }; ExtBuilder::default().build_and_execute_without_check_count(|| { - let node = |voter, prev, next, bag_upper| Node:: { voter, prev, next, bag_upper }; - // insert some validators into genesis state - bond_validator(51, 50, 2_000); - bond_validator(61, 60, 2_000); - bond_validator(71, 70, 2_000); - - // given - assert_eq!( - get_bags(), - vec![(10, vec![31]), (1000, vec![11, 21, 101]), (2000, vec![51, 61, 71])], - ); - // when inserting a node with both prev & next pointing at an account in the bag // and an incorrect bag_upper let mut bag_1000 = Bag::::get(1_000).unwrap(); let voter_42 = Voter::nominator(42); bag_1000.insert_node(node(voter_42.clone(), Some(11), Some(11), 0)); + // then the ids are in the correct order assert_eq!(bag_as_ids(&bag_1000), vec![11, 21, 101, 42]); // and when the node is re-fetched all the info is correct @@ -1440,10 +1436,12 @@ mod bags { // given 21 is a validator in bag_1000 (and not a tail node) let bag_1000_voter = bag_1000.iter().map(|node| node.voter().clone()).collect::>(); - assert_eq!(bag_1000_voter[1], Voter::<_>::validator(21)); + assert_eq!(bag_1000_voter[1], Voter::validator(21)); + // when inserting a node with duplicate id 21 but as a nominator - let voter_21_nom = Voter::<_>::nominator(21); + let voter_21_nom = Voter::nominator(21); bag_1000.insert_node(node(voter_21_nom.clone(), None, None, bag_1000.bag_upper)); + // then all the nodes after the duplicate are lost (because it is set as the tail) assert_eq!(bag_as_ids(&bag_1000), vec![11, 21]); // and the re-fetched node is a nominator with an **incorrect** prev pointer. @@ -1451,24 +1449,45 @@ mod bags { Node::::get(1_000, &21).unwrap(), node(voter_21_nom, Some(42), None, bag_1000.bag_upper) ); + }); - // TODO: this exact issue could be avoided at this level of abstraction if we check if - // an id is already the bags tail before inserting - - // given a bag we are confident is in valid state - let mut bag_2000 = Bag::::get(2_000).unwrap(); - assert_eq!(bag_as_ids(&bag_2000), vec![51, 61, 71]); - // when inserting a duplicate id that is already the tail - assert_eq!(bag_2000.tail, Some(71)); - let voter_71 = Voter::<_>::validator(71); - bag_2000.insert_node(node(voter_71.clone(), None, None, bag_2000.bag_upper)); - // then a cycle is created - assert_eq!(bag_2000.tail, Some(71)); + ExtBuilder::default().build_and_execute_without_check_count(|| { + // when inserting a duplicate id of the head + let mut bag_1000 = Bag::::get(1_000).unwrap(); + let voter_11 = Voter::validator(11); + bag_1000.insert_node(node(voter_11.clone(), None, None, 0)); + // then all nodes after the head are lost + assert_eq!(bag_as_ids(&bag_1000), vec![11]); + // and the re-fetched node assert_eq!( - Node::::get(2_000, &71).unwrap(), - node(voter_71, Some(71), Some(71), bag_2000.bag_upper) + Node::::get(1_000, &11).unwrap(), + node(voter_11, Some(101), None, bag_1000.bag_upper) ); - }) + + assert_eq!( + bag_1000, + Bag { + head: Some(11), tail: Some(11), bag_upper: 1_000 + } + ) + }); + } + + #[test] + #[should_panic = "system logic error: inserting a node who has the id of tail"] + fn insert_node_duplicate_tail_panics_with_debug_assert() { + ExtBuilder::default().build_and_execute_without_check_count(|| { + let node = |voter, prev, next, bag_upper| Node:: { voter, prev, next, bag_upper }; + + // given + assert_eq!(get_bags(), vec![(10, vec![31]), (1000, vec![11, 21, 101])],); + let mut bag_1000 = Bag::::get(1_000).unwrap(); + + // when inserting a duplicate id that is already the tail + assert_eq!(bag_1000.tail, Some(101)); + let voter_101 = Voter::validator(101); + bag_1000.insert_node(node(voter_101, None, None, bag_1000.bag_upper)); // panics + }); } #[test] @@ -1553,10 +1572,7 @@ mod bags { assert_ok!(bag_2000.sanity_check()); // state of all bags is as expected - assert_eq!( - get_bags(), - vec![(10, vec![81]), (2_000, vec![91, 171, 191])] - ); + assert_eq!(get_bags(), vec![(10, vec![81]), (2_000, vec![91, 171, 191])]); }); } From d59ea90205a78fa1f0661ea75bd5b148e5d5a9b5 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Tue, 27 Jul 2021 17:37:24 -0700 Subject: [PATCH 26/28] Check iter.count on voter list in pre-migrate --- 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 0afc5643cf011..6121d3a3affa8 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -766,7 +766,7 @@ pub mod migrations { pub fn pre_migrate() -> Result<(), &'static str> { ensure!(StorageVersion::::get() == Releases::V7_0_0, "must upgrade linearly"); ensure!( - VoterList::::decode_len().unwrap_or_default() == 0, + VoterList::::iter().count() == 0, "voter list already exists" ); Ok(()) From 33cae7f1938c10682db5972966506523e7b82358 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Tue, 27 Jul 2021 17:38:20 -0700 Subject: [PATCH 27/28] undo strang fmt comment stuff --- frame/staking/src/tests.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/frame/staking/src/tests.rs b/frame/staking/src/tests.rs index b881f68b29b3c..4db46a1bf7624 100644 --- a/frame/staking/src/tests.rs +++ b/frame/staking/src/tests.rs @@ -3885,7 +3885,7 @@ mod voter_bags { // decrease stake within the range of the current bag assert_ok!(Staking::unbond(Origin::signed(43), 999)); // 2000 - 999 = 1001 - // does not change bags + // does not change bags assert_eq!( get_bags(), vec![(10, vec![31]), (1000, vec![11, 21, 101]), (2000, vec![42])] @@ -3893,7 +3893,7 @@ mod voter_bags { // reduce stake to the level of a non-existent bag assert_ok!(Staking::unbond(Origin::signed(43), 971)); // 1001 - 971 = 30 - // creates the bag and moves the voter into it + // creates the bag and moves the voter into it assert_eq!( get_bags(), vec![(10, vec![31]), (30, vec![42]), (1000, vec![11, 21, 101]),] @@ -3901,7 +3901,7 @@ mod voter_bags { // increase stake by `rebond`-ing to the level of a pre-existing bag assert_ok!(Staking::rebond(Origin::signed(43), 31)); // 30 + 41 = 61 - // moves the voter to that bag + // moves the voter to that bag assert_eq!(get_bags(), vec![(10, vec![31]), (1000, vec![11, 21, 101, 42]),]); }); } From 605640b1e1a46ec487fc7ad570da478166c1c1f3 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Wed, 28 Jul 2021 09:28:17 -0700 Subject: [PATCH 28/28] Add in todo --- frame/staking/src/tests.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/frame/staking/src/tests.rs b/frame/staking/src/tests.rs index 4db46a1bf7624..11432e6d68e3d 100644 --- a/frame/staking/src/tests.rs +++ b/frame/staking/src/tests.rs @@ -3903,6 +3903,8 @@ mod voter_bags { assert_ok!(Staking::rebond(Origin::signed(43), 31)); // 30 + 41 = 61 // moves the voter to that bag assert_eq!(get_bags(), vec![(10, vec![31]), (1000, vec![11, 21, 101, 42]),]); + + // TODO test rebag directly }); } }