Suggestions for #9081 (Store voters in unsorted bags)

bin/node/runtime/voter-bags/src/main.rs

@@ -62,7 +62,7 @@ struct Opt {
 }
 
 fn main() -> Result<(), std::io::Error> {
-	let Opt {n_bags, output, runtime } = Opt::from_args();
+	let Opt { n_bags, output, runtime } = Opt::from_args();
 	let mut ext = sp_io::TestExternalities::new_empty();
 	ext.execute_with(|| runtime.generate_thresholds()(n_bags, &output))
 }

frame/staking/src/benchmarking.rs

@@ -671,11 +671,15 @@ benchmarks! {
 		// Clean up any existing state.
 		clear_validators_and_nominators::<T>();
 
+		let thresholds = T::VoterBagThresholds::get();
+
 		// stash controls the node account
-		let (stash, controller) = make_validator(USER_SEED, 100)?;
+		let bag0_thresh = thresholds[0];
+		let (stash, controller) = make_validator(USER_SEED, bag0_thresh as u32)?;
 
-		// create another validator with 3x the stake
-		let (other_stash, _) = make_validator(USER_SEED + 1, 300)?;
+		// create another validator with more stake
+		let bag2_thresh = thresholds[2];
+		let (other_stash, _) = make_validator(USER_SEED + 1, bag2_thresh as u32)?;
 
 		// update the stash account's value/weight
 		//

frame/staking/src/lib.rs

@@ -780,6 +780,7 @@ pub mod migrations {
 			log!(info, "Migrating staking to Releases::V8_0_0");
 
 			let migrated = VoterList::<T>::regenerate();
+			debug_assert_eq!(VoterList::<T>::sanity_check(), Ok(()));
 
 			StorageVersion::<T>::put(Releases::V8_0_0);
 			log!(
@@ -1313,7 +1314,7 @@ pub mod pallet {
 
 	/// How many voters are registered.
 	#[pallet::storage]
-	pub(crate) type VoterCount<T> = StorageValue<_, u32, ValueQuery>;
+	pub(crate) type CounterForVoters<T> = StorageValue<_, u32, ValueQuery>;
 
 	/// Which bag currently contains a particular voter.
 	///
@@ -1397,32 +1398,62 @@ pub mod pallet {
 			MinNominatorBond::<T>::put(self.min_nominator_bond);
 			MinValidatorBond::<T>::put(self.min_validator_bond);
 
+			let mut num_voters: u32 = 0;
 			for &(ref stash, ref controller, balance, ref status) in &self.stakers {
+				log!(
+					trace,
+					"inserting genesis staker: {:?} => {:?} => {:?}",
+					stash,
+					balance,
+					status
+				);
 				assert!(
 					T::Currency::free_balance(&stash) >= balance,
 					"Stash does not have enough balance to bond."
 				);
-				let _ = <Pallet<T>>::bond(
+
+				if let Err(why) = <Pallet<T>>::bond(
 					T::Origin::from(Some(stash.clone()).into()),
 					T::Lookup::unlookup(controller.clone()),
 					balance,
 					RewardDestination::Staked,
-				);
-				let _ = match status {
+				) {
+					// TODO: later on, fix all the tests that trigger these warnings, and
+					// make these assertions. Genesis stakers should all be correct!
+					log!(warn, "failed to bond staker at genesis: {:?}.", why);
+					continue;
+				}
+				match status {
 					StakerStatus::Validator => {
-						<Pallet<T>>::validate(
+						if let Err(why) = <Pallet<T>>::validate(
 							T::Origin::from(Some(controller.clone()).into()),
 							Default::default(),
-						)
+						) {
+							log!(warn, "failed to validate staker at genesis: {:?}.", why);
+						} else {
+							num_voters +=1 ;
+						}
 					},
 					StakerStatus::Nominator(votes) => {
-						<Pallet<T>>::nominate(
+						if let Err(why) = <Pallet<T>>::nominate(
 							T::Origin::from(Some(controller.clone()).into()),
 							votes.iter().map(|l| T::Lookup::unlookup(l.clone())).collect(),
-						)
-					}, _ => Ok(())
+						) {
+							log!(warn, "failed to nominate staker at genesis: {:?}.", why);
+						} else {
+							num_voters += 1;
+						}
+					}
+					_ => ()
 				};
 			}
+
+			// all voters are inserted sanely.
+			assert_eq!(
+				CounterForVoters::<T>::get(),
+				num_voters,
+				"not all genesis stakers were inserted into bags, something is wrong."
+			);
 		}
 	}
 
@@ -3096,7 +3127,7 @@ impl<T: Config> Pallet<T> {
 		}
 		Nominators::<T>::insert(who, nominations);
 		VoterList::<T>::insert_as(who, VoterType::Nominator);
-		debug_assert!(VoterCount::<T>::get() == CounterForNominators::<T>::get() + CounterForValidators::<T>::get());
+		debug_assert_eq!(VoterList::<T>::sanity_check(), Ok(()));
 	}
 
 	/// This function will remove a nominator from the `Nominators` storage map,
@@ -3112,7 +3143,7 @@ impl<T: Config> Pallet<T> {
 			Nominators::<T>::remove(who);
 			CounterForNominators::<T>::mutate(|x| x.saturating_dec());
 			VoterList::<T>::remove(who);
-			debug_assert!(VoterCount::<T>::get() == CounterForNominators::<T>::get() + CounterForValidators::<T>::get());
+			debug_assert_eq!(VoterList::<T>::sanity_check(), Ok(()));
 			true
 		} else {
 			false
@@ -3133,7 +3164,7 @@ impl<T: Config> Pallet<T> {
 		}
 		Validators::<T>::insert(who, prefs);
 		VoterList::<T>::insert_as(who, VoterType::Validator);
-		debug_assert!(VoterCount::<T>::get() == CounterForNominators::<T>::get() + CounterForValidators::<T>::get());
+		debug_assert_eq!(VoterList::<T>::sanity_check(), Ok(()));
 	}
 
 	/// This function will remove a validator from the `Validators` storage map,
@@ -3149,7 +3180,7 @@ impl<T: Config> Pallet<T> {
 			Validators::<T>::remove(who);
 			CounterForValidators::<T>::mutate(|x| x.saturating_dec());
 			VoterList::<T>::remove(who);
-			debug_assert!(VoterCount::<T>::get() == CounterForNominators::<T>::get() + CounterForValidators::<T>::get());
+			debug_assert_eq!(VoterList::<T>::sanity_check(), Ok(()));
 			true
 		} else {
 			false

frame/staking/src/mock.rs

@@ -17,14 +17,9 @@
 
 //! Test utilities
 
-// This module needs to exist when the `make-bags` feature is enabled so that we can generate the
-// appropriate thresholds, but we don't care if it's mostly unused in that case.
-#![cfg_attr(feature = "make-bags", allow(unused))]
-
-mod voter_bags;
-
-use crate::*;
 use crate as staking;
+use crate::*;
+use frame_election_provider_support::onchain;
 use frame_support::{
 	assert_ok, parameter_types,
 	traits::{Currency, FindAuthor, Get, OnInitialize, OneSessionHandler},
@@ -39,7 +34,6 @@ use sp_runtime::{
 };
 use sp_staking::offence::{OffenceDetails, OnOffenceHandler};
 use std::{cell::RefCell, collections::HashSet};
-use frame_election_provider_support::onchain;
 
 pub const INIT_TIMESTAMP: u64 = 30_000;
 pub const BLOCK_TIME: u64 = 1000;
@@ -249,8 +243,11 @@ impl onchain::Config for Test {
 	type DataProvider = Staking;
 }
 
+/// Thresholds used for bags.
+const THRESHOLDS: [VoteWeight; 9] = [10, 20, 30, 40, 50, 60, 1_000, 2_000, 10_000];
+
 parameter_types! {
-	pub const VoterBagThresholds: &'static [VoteWeight] = &voter_bags::THRESHOLDS;
+	pub const VoterBagThresholds: &'static [VoteWeight] = &THRESHOLDS;
 }
 
 impl Config for Test {
@@ -387,14 +384,9 @@ impl ExtBuilder {
 	}
 	fn build(self) -> sp_io::TestExternalities {
 		sp_tracing::try_init_simple();
-		let mut storage = frame_system::GenesisConfig::default()
-			.build_storage::<Test>()
-			.unwrap();
-		let balance_factor = if ExistentialDeposit::get() > 1 {
-			256
-		} else {
-			1
-		};
+
+		let mut storage = frame_system::GenesisConfig::default().build_storage::<Test>().unwrap();
+		let balance_factor = if ExistentialDeposit::get() > 1 { 256 } else { 1 };
 
 		let num_validators = self.num_validators.unwrap_or(self.validator_count);
 		// Check that the number of validators is sensible.
@@ -511,6 +503,9 @@ fn check_count() {
 	let validator_count = Validators::<Test>::iter().count() as u32;
 	assert_eq!(nominator_count, CounterForNominators::<Test>::get());
 	assert_eq!(validator_count, CounterForValidators::<Test>::get());
+
+	let voters_count = CounterForVoters::<Test>::get();
+	assert_eq!(voters_count, nominator_count + validator_count);
 }
 
 fn check_ledgers() {
@@ -839,3 +834,14 @@ pub(crate) fn staking_events() -> Vec<staking::Event<Test>> {
 pub(crate) fn balances(who: &AccountId) -> (Balance, Balance) {
 	(Balances::free_balance(who), Balances::reserved_balance(who))
 }
+
+use crate::voter_bags::Bag;
+/// Returns the nodes of all non-empty bags.
+pub(crate) fn get_bags() -> Vec<(VoteWeight, Vec<AccountId>)> {
+	VoterBagThresholds::get().into_iter().filter_map(|t| {
+		Bag::<Test>::get(*t).map(|bag| (
+			*t,
+			bag.iter().map(|n| n.voter().id).collect::<Vec<_>>()
+		))
+	}).collect::<Vec<_>>()
+}

frame/staking/src/tests.rs

@@ -148,16 +148,13 @@ fn basic_setup_works() {
 		// New era is not being forced
 		assert_eq!(Staking::force_era(), Forcing::NotForcing);
 
-		// genesis accounts must exist in the proper bags
-		let weight_of = Staking::weight_of_fn();
-		// for these stash ids, see
-		// https://github.com/paritytech/substrate/
-		//   blob/631d4cdbcad438248c2597213918d8207d85bf6e/frame/staking/src/mock.rs#L435-L441
-		for genesis_stash_account_id in [11, 21, 31, 101] {
-			let node = crate::voter_bags::Node::<Test>::from_id(&genesis_stash_account_id)
-				.expect(&format!("node was created for account {}", genesis_stash_account_id));
-			assert!(!node.is_misplaced(&weight_of));
-		}
+		// check the bags
+		assert_eq!(CounterForVoters::<Test>::get(), 4);
+
+		assert_eq!(
+			get_bags(),
+			vec![(10, vec![31]), (1000, vec![11, 21, 101])],
+		);
 	});
 }
 
@@ -3871,6 +3868,18 @@ fn on_finalize_weight_is_nonzero() {
 	})
 }
 
+// end-to-end nodes of the voter bags operation.
+mod voter_bags {
+
+	#[test]
+	fn rebag_works() {
+		todo!()
+	}
+}
+/*
+// TODO: this needs some love, retire it in favour of the one above. Use the mock data, don't make
+// it complicated with data setup, use the simplest data possible, instead check multiple
+// edge-cases.
 #[test]
 fn test_rebag() {
 	use crate::{
@@ -3901,8 +3910,8 @@ fn test_rebag() {
 	ExtBuilder::default().build_and_execute(|| {
 		// We want to have two validators: one, `stash`, is the one we will rebag.
 		// The other, `other_stash`, exists only so that the destination bag is not empty.
-		let stash = make_validator(0, 100).unwrap();
-		let other_stash = make_validator(1, 300).unwrap();
+		let stash = make_validator(0, 2000).unwrap();
+		let other_stash = make_validator(1, 9000).unwrap();
 
 		// verify preconditions
 		let weight_of = Staking::weight_of_fn();
@@ -3942,6 +3951,7 @@ fn test_rebag() {
 		assert!(!node.is_misplaced(&weight_of), "node must be in proper place after rebag");
 	});
 }
+*/
 
 mod election_data_provider {
 	use super::*;