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

Suggestions for #9081 (Store voters in unsorted bags) #9328

Merged
emostov merged 26 commits into from
Jul 21, 2021
Merged

Suggestions for #9081 (Store voters in unsorted bags) #9328

Changes from 1 commit
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
01a04cc
Add some debug asserts to node::get and remove_node
emostov Jul 11, 2021
a37e59d
Improve the debug asserts in remove_node
emostov Jul 12, 2021
f047cec
improve debug asserts
emostov Jul 12, 2021
84dfba2
Space
emostov Jul 12, 2021
f0e754f
Remove bad assertions
emostov Jul 12, 2021
aceb642
Tests: WIP take_works
emostov Jul 12, 2021
19eb44f
Take test
emostov Jul 12, 2021
89f5319
Doc comment
emostov Jul 12, 2021
65e3bb3
Apply suggestions from code review
emostov Jul 12, 2021
57b9004
Test storage is cleaned up;
emostov Jul 14, 2021
2b5d9c8
formatting
emostov Jul 14, 2021
33ba6fb
Switch to simpler thresholds
emostov Jul 14, 2021
21f77eb
Update the storage cleanup test
emostov Jul 14, 2021
2e2fada
Remove hardcoded values from benchmark to make it more robust
emostov Jul 15, 2021
3dc2e52
Fix tests to acces bags properly
emostov Jul 15, 2021
e0dfed0
Sanity check WIP; tests failing
emostov Jul 15, 2021
f1f25f4
Update sanity checks to be more correct
emostov Jul 15, 2021
90a04d0
Improve storage cleanup tests
emostov Jul 15, 2021
c466c95
WIP remote_ext_tests
emostov Jul 16, 2021
fd907c6
Some notes on next steps
emostov Jul 16, 2021
a626db0
Remove some stuff that was for remote-ext tests
emostov Jul 19, 2021
2e9c6d9
Some more cleanup to reduce diff
emostov Jul 19, 2021
8deedee
More :clean:
emostov Jul 19, 2021
8c9c736
Mo cleanin
emostov Jul 19, 2021
4645325
small fix
emostov Jul 19, 2021
1f3c154
A lot of changes from kian
kianenigma Jul 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Fix tests to acces bags properly
  • Loading branch information
@emostov
emostov committed Jul 15, 2021
commit 3dc2e5282958308103ca4b0dba69c3ca9ba80a5d
376 changes: 195 additions & 181 deletions frame/staking/src/voter_bags.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,10 @@ impl<T: Config> Bag<T> {

/// Get a bag by its upper vote weight or make it, appropriately initialized.
pub fn get_or_make(bag_upper: VoteWeight) -> Bag<T> {
debug_assert!(
T::VoterBagThresholds::get().contains(&bag_upper) || bag_upper == VoteWeight::MAX,
"it is a logic error to attempt to get a bag which is not in the thresholds list"
);
Self::get(bag_upper).unwrap_or(Bag { bag_upper, ..Default::default() })
}

Expand Down Expand Up @@ -559,6 +563,13 @@ pub struct Node<T: Config> {
impl<T: Config> Node<T> {
/// Get a node by bag idx and account id.
pub fn get(bag_upper: VoteWeight, account_id: &AccountIdOf<T>) -> Option<Node<T>> {
if !T::VoterBagThresholds::get().contains(&bag_upper) {
let n = crate::VoterNodes::<T>::try_get(account_id).ok().map(|mut node| {
node.bag_upper = bag_upper;
node
});
println!("node::get bag_upper {}. account id {}. n: {:#?}", bag_upper, account_id, n);
}
debug_assert!( // TODO: figure out why this breaks test take_works
T::VoterBagThresholds::get().contains(&bag_upper) || bag_upper == VoteWeight::MAX,
"it is a logic error to attempt to get a bag which is not in the thresholds list"
Expand Down Expand Up @@ -903,190 +914,193 @@ pub mod make_bags {
}
}

// #[cfg(test)]
// mod tests {
// use frame_support::traits::Currency;
// use substrate_test_utils::assert_eq_uvec;
#[cfg(test)]
mod tests {
use frame_support::traits::Currency;
use substrate_test_utils::assert_eq_uvec;

// use super::*;
// use crate::mock::*;
use super::*;
use crate::mock::*;

// const GENESIS_VOTER_IDS: [u64; 5] = [11, 21, 31, 41, 101];
const GENESIS_VOTER_IDS: [u64; 5] = [11, 21, 31, 41, 101];

// #[test]
// fn voter_list_includes_genesis_accounts() {
// ExtBuilder::default().validator_pool(true).build_and_execute(|| {
// let have_voters: Vec<_> = VoterList::<Test>::iter().map(|node| node.voter.id).collect();
// assert_eq_uvec!(GENESIS_VOTER_IDS, have_voters);
// });
// }
#[test]
fn voter_list_includes_genesis_accounts() {
ExtBuilder::default().validator_pool(true).build_and_execute(|| {
let have_voters: Vec<_> = VoterList::<Test>::iter().map(|node| node.voter.id).collect();
assert_eq_uvec!(GENESIS_VOTER_IDS, have_voters);
});
}

// /// This tests the property that when iterating through the `VoterList`, we iterate from higher
// /// bags to lower.
// #[test]
// fn iteration_is_semi_sorted() {
// use rand::seq::SliceRandom;
// let mut rng = rand::thread_rng();

// // Randomly sort the list of voters. Later we'll give each of these a stake such that it
// // fits into a different bag.
// let voters = {
// let mut v = vec![0; GENESIS_VOTER_IDS.len()];
// v.copy_from_slice(&GENESIS_VOTER_IDS);
// v.shuffle(&mut rng);
// v
// };

// ExtBuilder::default().validator_pool(true).build_and_execute(|| {
// // initialize the voters' deposits
// let mut balance = 10;
// for voter_id in voters.iter().rev() {
// <Test as Config>::Currency::make_free_balance_be(voter_id, balance);
// let controller = Staking::bonded(voter_id).unwrap();
// let mut ledger = Staking::ledger(&controller).unwrap();
// ledger.total = balance;
// ledger.active = balance;
// Staking::update_ledger(&controller, &ledger);
// Staking::do_rebag(voter_id);

// balance += 10;
// }

// let have_voters: Vec<_> = VoterList::<Test>::iter().map(|node| node.voter.id).collect();
// assert_eq!(voters, have_voters);
// });
// }
/// This tests the property that when iterating through the `VoterList`, we iterate from higher
/// bags to lower.
#[test]
fn iteration_is_semi_sorted() {
use rand::seq::SliceRandom;
let mut rng = rand::thread_rng();

// Randomly sort the list of voters. Later we'll give each of these a stake such that it
// fits into a different bag.
let voters = {
let mut v = vec![0; GENESIS_VOTER_IDS.len()];
v.copy_from_slice(&GENESIS_VOTER_IDS);
v.shuffle(&mut rng);
v
};

// /// This tests that we can `take` x voters, even if that quantity ends midway through a list.
// #[test]
// fn take_works() {
// ExtBuilder::default().validator_pool(true).build_and_execute(|| {
// // initialize the voters' deposits
// let mut balance = 0; // This will be 10 on the first loop iteration bc 0 % 3 == 0
// for (idx, voter_id) in GENESIS_VOTER_IDS.iter().enumerate() {
// if idx % 3 == 0 {
// // This increases the balance by 10, which is the amount each threshold
// // increases by. Thus this will increase the balance by 1 bag.
// //
// // This will create 2 bags, the lower threshold bag having
// // 3 voters with balance 10, and the higher threshold bag having
// // 2 voters with balance 20.
// balance += 10;
// }

// <Test as Config>::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 = <Staking as crate::Store>::VoterBags::get(&10)
// .unwrap()
// .iter()
// .map(|node| node.voter.id)
// .collect::<Vec<_>>();
// assert_eq!(bag_thresh10, vec![11, 21, 31]);

// let bag_thresh20 = <Staking as crate::Store>::VoterBags::get(&20)
// .unwrap()
// .iter()
// .map(|node| node.voter.id)
// .collect::<Vec<_>>();
// assert_eq!(bag_thresh20, vec![41, 101]);

// let voters: Vec<_> = VoterList::<Test>::iter()
// // take 4/5 from [41, 101],[11, 21, 31], demonstrating that we can do a
// // take that stops mid bag.
// .take(4)
// .map(|node| node.voter.id)
// .collect();

// assert_eq!(voters, vec![41, 101, 11, 21]);
// });
// }
ExtBuilder::default().validator_pool(true).build_and_execute(|| {
// initialize the voters' deposits
let mut balance = 10;
for voter_id in voters.iter().rev() {
<Test as Config>::Currency::make_free_balance_be(voter_id, balance);
let controller = Staking::bonded(voter_id).unwrap();
let mut ledger = Staking::ledger(&controller).unwrap();
ledger.total = balance;
ledger.active = balance;
Staking::update_ledger(&controller, &ledger);
Staking::do_rebag(voter_id);

balance += 10;
}

// #[test]
// fn storage_is_cleaned_up_as_voters_are_removed() {
// ExtBuilder::default().validator_pool(true).build_and_execute(|| {
// // Initialize voters deposits so there are 5 bags with one voter each.
// let mut balance = 10;
// for voter_id in GENESIS_VOTER_IDS.iter() {
// <Test as Config>::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<u64>| {
// v.sort();
// let mut voters: Vec<_> =
// VoterList::<Test>::iter().map(|node| node.voter.id).collect();
// voters.sort();
// assert_eq!(voters, v);

// let mut nodes: Vec<_> =
// <Staking as crate::Store>::VoterNodes::iter_keys().collect();
// nodes.sort();
// assert_eq!(nodes, v);

// let mut flat_bags: Vec<_> = <Staking as crate::Store>::VoterBags::iter()
// .flat_map(|(_, bag)| bag.iter())
// .map(|node| node.voter.id)
// .collect();
// flat_bags.sort();
// assert_eq!(flat_bags, v);

// let mut bags_for: Vec<_> =
// <Staking as crate::Store>::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!(<Staking as crate::Store>::VoterCount::get(), 5);

// // Remove 1 voter,
// VoterList::<Test>::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!(<Staking as crate::Store>::VoterCount::get(), 4);

// // Now remove the remaining voters so we have 0 left,
// remaining_voters.iter().for_each(|v| VoterList::<Test>::remove(v));
// // and assert all of them have been cleaned up.
// voter_list_storage_items_eq(vec![]);
// assert_eq!(<Staking as crate::Store>::VoterCount::get(), 0);

// // TODO the below is redundant with voter_list_storage_items_eq(vec![]), but slightly
// // different since it checks length. Practically should be the same, but we are extra
// // through in checking keys ... probs should remove tho to reduce redundancy.

// // We can make sure _everyone_ has been totally removed,
// remaining_voters.iter().for_each(|v| {
// assert!(!<Staking as crate::Store>::VoterNodes::contains_key(v));
// assert!(!<Staking as crate::Store>::VoterBagFor::contains_key(v));
// });
// // there are no more bags left,
// assert_eq!(
// <Staking as crate::Store>::VoterBags::iter().collect::<Vec<_>>().len(),
// 0
// );
// // and the voter list has no one in it.
// assert_eq!(
// <Staking as crate::Store>::VoterList::<Test>::iter().collect::<Vec<_>>().len(),
// 0
// );
// });
// }
// }
let have_voters: Vec<_> = VoterList::<Test>::iter().map(|node| node.voter.id).collect();
assert_eq!(voters, have_voters);
});
}

/// This tests that we can `take` x voters, even if that quantity ends midway through a list.
#[test]
fn take_works() {
ExtBuilder::default().validator_pool(true).build_and_execute(|| {
// initialize the voters' deposits
let mut balance = 0; // This will be 10 on the first loop iteration bc 0 % 3 == 0
for (idx, voter_id) in GENESIS_VOTER_IDS.iter().enumerate() {
Copy link
Contributor

@kianenigma kianenigma Jul 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are we doing this? GENESIS_VOTER_IDS should already be in the correct bag.

Copy link
Contributor Author

@emostov emostov Jul 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am thinking its easier to read the test when you can see up front how they are distributed in the bags. I need to double check how the genesis voters are distributed, but the scheme here guarantees that the last node taken is mid-bag, which is an edge case we talked about explicitly testing

Copy link
Contributor

@kianenigma kianenigma Jul 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your point, but when you have 100+ tests, it is quite verbose to setup things manually for each tests. Instead, it is more reasonable to assume some fixed genesis stakers (and the programmer should simply have this in their mind) and write all tests preferably based on that.

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;
}

<Test as Config>::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::<Test>::get(10)
.unwrap()
.iter()
.map(|node| node.voter.id)
.collect::<Vec<_>>();
assert_eq!(bag_thresh10, vec![11, 21, 31]);

let bag_thresh20 = Bag::<Test>::get(20)
.unwrap()
.iter()
.map(|node| node.voter.id)
.collect::<Vec<_>>();
assert_eq!(bag_thresh20, vec![41, 101]);

let voters: Vec<_> = VoterList::<Test>::iter()
// take 4/5 from [41, 101],[11, 21, 31], demonstrating that we can do a
// take that stops mid bag.
.take(4)
.map(|node| node.voter.id)
.collect();

assert_eq!(voters, vec![41, 101, 11, 21]);
});
}

#[test]
fn storage_is_cleaned_up_as_voters_are_removed() {
ExtBuilder::default().validator_pool(true).build_and_execute(|| {
// Initialize voters deposits so there are 5 bags with one voter each.
let mut balance = 10;
for voter_id in GENESIS_VOTER_IDS.iter() {
<Test as Config>::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<u64>| {
v.sort();
let mut voters: Vec<_> =
VoterList::<Test>::iter().map(|node| node.voter.id).collect();
voters.sort();
assert_eq!(voters, v);

let mut nodes: Vec<_> =
<Staking as crate::Store>::VoterNodes::iter_keys().collect();
nodes.sort();
assert_eq!(nodes, v);

let mut flat_bags: Vec<_> = <Staking as crate::Store>::VoterBags::iter()
// We always get the bag with the Bag<T> getter because the bag_upper
// is only initialized in the getter.
.flat_map(|(key, _bag)| Bag::<Test>::get(key).unwrap().iter())
.map(|node| node.voter.id)
.collect();
flat_bags.sort();
assert_eq!(flat_bags, v);

let mut bags_for: Vec<_> =
<Staking as crate::Store>::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!(<Staking as crate::Store>::VoterCount::get(), 5);

// Remove 1 voter,
VoterList::<Test>::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!(<Staking as crate::Store>::VoterCount::get(), 4);

// Now remove the remaining voters so we have 0 left,
remaining_voters.iter().for_each(|v| VoterList::<Test>::remove(v));
// and assert all of them have been cleaned up.
voter_list_storage_items_eq(vec![]);
assert_eq!(<Staking as crate::Store>::VoterCount::get(), 0);

// TODO the below is redundant with voter_list_storage_items_eq(vec![]), but slightly
// different since it checks length. Practically should be the same, but we do an extra
// by checking keys ... probs should remove tho to reduce redundancy.

// We can make sure _everyone_ has been totally removed,
remaining_voters.iter().for_each(|v| {
assert!(!<Staking as crate::Store>::VoterNodes::contains_key(v));
assert!(!<Staking as crate::Store>::VoterBagFor::contains_key(v));
});
// TODO bags do not get cleaned up from storages
// - is this ok?
assert_eq!(
<Staking as crate::Store>::VoterBags::iter().collect::<Vec<_>>().len(),
6
);
// and the voter list has no one in it.
assert_eq!(
VoterList::<Test>::iter().collect::<Vec<_>>().len(),
0
);
});
}
}