Glue the new staking bags to the election snapshot (#9415)

* Glue the new staking bags to the election snapshot

* add CheckedRem (#9412)

* add CheckedRem

* fix

* Run fmt

* Test comment

Co-authored-by: Xiliang Chen <xlchen1291@gmail.com>
Co-authored-by: emostov <32168567+emostov@users.noreply.github.com>

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::<node_runtime::Runtime>(n_bags, &output)
-	})
+	ext.execute_with(|| generate_thresholds_module::<node_runtime::Runtime>(n_bags, &output))
 }

frame/election-provider-multi-phase/src/lib.rs

@@ -632,6 +632,15 @@ pub mod pallet {
 		#[pallet::constant]
 		type SignedDepositWeight: Get<BalanceOf<Self>>;
 
+		/// The number of snapshot voters to fetch per block.
+		///
+		/// In the future, this value will make more sense with multi-block snapshot.
+		///
+		/// Also, note the data type: If the voters are represented by a `u32` in `type
+		/// CompactSolution`, the same `u32` is used here to ensure bounds are respected.
+		#[pallet::constant]
+		type VoterSnapshotPerBlock: Get<CompactVoterIndexOf<Self>>;
+
 		/// Handler for the slashed deposits.
 		type SlashHandler: OnUnbalanced<NegativeImbalanceOf<Self>>;
 
@@ -1285,8 +1294,11 @@ impl<T: Config> Pallet<T> {
 	///
 	/// Returns `Ok(consumed_weight)` if operation is okay.
 	pub fn create_snapshot() -> Result<Weight, ElectionError> {
+		// we don't impose any limits on the targets for now, the assumption is that
+		// `T::DataProvider` will sensibly return small values to use.
 		let target_limit = <CompactTargetIndexOf<T>>::max_value().saturated_into::<usize>();
-		let voter_limit = <CompactVoterIndexOf<T>>::max_value().saturated_into::<usize>();
+		// for now we have just a single block snapshot.
+		let voter_limit = T::VoterSnapshotPerBlock::get().saturated_into::<usize>();
 
 		let (targets, w1) =
 			T::DataProvider::targets(Some(target_limit)).map_err(ElectionError::DataProvider)?;
@@ -1970,8 +1982,8 @@ mod tests {
 		})
 	}
 
-	#[test]
-	fn snapshot_creation_fails_if_too_big() {
+	fn snapshot_too_big_failure_onchain_fallback() {
+		// the `MockStaking` is designed such that if it has too many targets, it simply fails.
 		ExtBuilder::default().build_and_execute(|| {
 			Targets::set((0..(TargetIndex::max_value() as AccountId) + 1).collect::<Vec<_>>());
 
@@ -1987,6 +1999,49 @@ mod tests {
 			roll_to(29);
 			let (supports, _) = MultiPhase::elect().unwrap();
 			assert!(supports.len() > 0);
+		});
+	}
+
+	#[test]
+	fn snapshot_too_big_failure_no_fallback() {
+		// and if the backup mode is nothing, we go into the emergency mode..
+		ExtBuilder::default().fallback(FallbackStrategy::Nothing).build_and_execute(|| {
+			crate::mock::Targets::set(
+				(0..(TargetIndex::max_value() as AccountId) + 1).collect::<Vec<_>>(),
+			);
+
+			// Signed phase failed to open.
+			roll_to(15);
+			assert_eq!(MultiPhase::current_phase(), Phase::Off);
+
+			// Unsigned phase failed to open.
+			roll_to(25);
+			assert_eq!(MultiPhase::current_phase(), Phase::Off);
+
+			roll_to(29);
+			let err = MultiPhase::elect().unwrap_err();
+			assert_eq!(err, ElectionError::NoFallbackConfigured);
+			assert_eq!(MultiPhase::current_phase(), Phase::Emergency);
+		});
+	}
+
+	#[test]
+	fn snapshot_too_big_truncate() {
+		// but if there are too many voters, we simply truncate them.
+		ExtBuilder::default().build_and_execute(|| {
+			// we have 8 voters in total.
+			assert_eq!(crate::mock::Voters::get().len(), 8);
+			// but we want to take 4.
+			crate::mock::VoterSnapshotPerBlock::set(2);
+
+			// Signed phase opens just fine.
+			roll_to(15);
+			assert_eq!(MultiPhase::current_phase(), Phase::Signed);
+
+			assert_eq!(
+				MultiPhase::snapshot_metadata().unwrap(),
+				SolutionOrSnapshotSize { voters: 2, targets: 4 }
+			);
 		})
 	}
 

frame/election-provider-multi-phase/src/mock.rs

@@ -270,6 +270,7 @@ parameter_types! {
 	pub static MinerMaxWeight: Weight = BlockWeights::get().max_block;
 	pub static MinerMaxLength: u32 = 256;
 	pub static MockWeightInfo: bool = false;
+	pub static VoterSnapshotPerBlock: VoterIndex = u32::max_value();
 
 	pub static EpochLength: u64 = 30;
 }
@@ -379,6 +380,7 @@ impl crate::Config for Runtime {
 	type Fallback = Fallback;
 	type ForceOrigin = frame_system::EnsureRoot<AccountId>;
 	type CompactSolution = TestCompact;
+	type VoterSnapshotPerBlock = VoterSnapshotPerBlock;
 }
 
 impl<LocalCall> frame_system::offchain::SendTransactionTypes<LocalCall> for Runtime
@@ -410,9 +412,9 @@ impl ElectionDataProvider<AccountId, u64> for StakingMock {
 	fn voters(
 		maybe_max_len: Option<usize>,
 	) -> data_provider::Result<(Vec<(AccountId, VoteWeight, Vec<AccountId>)>, Weight)> {
-		let voters = Voters::get();
-		if maybe_max_len.map_or(false, |max_len| voters.len() > max_len) {
-			return Err("Voters too big")
+		let mut voters = Voters::get();
+		if let Some(max_len) = maybe_max_len {
+			voters.truncate(max_len)
 		}
 
 		Ok((voters, 0))

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;
@@ -3111,12 +3111,6 @@ impl<T: Config> Pallet<T> {
 		maybe_max_len: Option<usize>,
 		voter_count: usize,
 	) -> Vec<VotingDataOf<T>> {
-		debug_assert_eq!(
-			voter_count,
-			VoterList::<T>::decode_len().unwrap_or_default(),
-			"voter_count must be accurate",
-		);
-
 		let wanted_voters = maybe_max_len.unwrap_or(voter_count).min(voter_count);
 
 		let weight_of = Self::weight_of_fn();
@@ -3240,15 +3234,23 @@ impl<T: Config>
 		let nominator_count = CounterForNominators::<T>::get();
 		let validator_count = CounterForValidators::<T>::get();
 		let voter_count = nominator_count.saturating_add(validator_count) as usize;
+
+		// check a few counters one last time...
 		debug_assert!(<Nominators<T>>::iter().count() as u32 == CounterForNominators::<T>::get());
 		debug_assert!(<Validators<T>>::iter().count() as u32 == CounterForValidators::<T>::get());
+		debug_assert_eq!(
+			voter_count,
+			VoterList::<T>::decode_len().unwrap_or_default(),
+			"voter_count must be accurate",
+		);
 
 		let slashing_span_count = <SlashingSpans<T>>::iter().count();
 		let weight = T::WeightInfo::get_npos_voters(
 			nominator_count,
 			validator_count,
 			slashing_span_count as u32,
 		);
+
 		Ok((Self::get_npos_voters(maybe_max_len, voter_count), weight))
 	}
 

frame/staking/src/tests.rs

@@ -4006,9 +4006,21 @@ mod election_data_provider {
 	}
 
 	#[test]
-	fn respects_len_limits() {
-		ExtBuilder::default().build_and_execute(|| {
+	fn respects_snapshot_len_limits() {
+		ExtBuilder::default().validator_pool(true).build_and_execute(|| {
+			// sum of all validators and nominators who'd be voters.
+			assert_eq!(VoterList::<Test>::decode_len().unwrap(), 5);
+
+			// if limits is less..
 			assert_eq!(Staking::voters(Some(1)).unwrap().0.len(), 1);
+
+			// if limit is equal..
+			assert_eq!(Staking::voters(Some(5)).unwrap().0.len(), 5);
+
+			// if limit is more.
+			assert_eq!(Staking::voters(Some(55)).unwrap().0.len(), 5);
+
+			// if target limit is less, then we return an error.
 			assert_eq!(Staking::targets(Some(1)).unwrap_err(), "Target snapshot too big");
 		});
 	}
@@ -4068,12 +4080,22 @@ mod election_data_provider {
 
 	#[test]
 	#[should_panic]
-	fn count_check_works() {
+	fn count_check_prevents_validator_insert() {
 		ExtBuilder::default().build_and_execute(|| {
 			// We should never insert into the validators or nominators map directly as this will
 			// not keep track of the count. This test should panic as we verify the count is accurate
-			// after every test using the `post_checks` in `mock`.
+			// after every test using the `post_conditions` in `mock`.
 			Validators::<Test>::insert(987654321, ValidatorPrefs::default());
+		})
+	}
+
+	#[test]
+	#[should_panic]
+	fn count_check_prevents_nominator_insert() {
+		ExtBuilder::default().build_and_execute(|| {
+			// We should never insert into the validators or nominators map directly as this will
+			// not keep track of the count. This test should panic as we verify the count is accurate
+			// after every test using the `post_conditions` in `mock`.
 			Nominators::<Test>::insert(
 				987654321,
 				Nominations {

frame/staking/src/voter_bags.rs

@@ -620,15 +620,13 @@ impl<T: Config> Node<T> {
 		weight_of: impl Fn(&T::AccountId) -> VoteWeight,
 		slashing_spans: &BTreeMap<AccountIdOf<T>, SlashingSpans>,
 	) -> Option<VotingDataOf<T>> {
+		let voter_weight = weight_of(&self.voter.id);
 		match self.voter.voter_type {
-			VoterType::Validator => Some((
-				self.voter.id.clone(),
-				weight_of(&self.voter.id),
-				sp_std::vec![self.voter.id.clone()],
-			)),
+			VoterType::Validator =>
+				Some((self.voter.id.clone(), voter_weight, sp_std::vec![self.voter.id.clone()])),
 			VoterType::Nominator => {
 				let Nominations { submitted_in, mut targets, .. } =
-					Nominators::<T>::get(self.voter.id.clone())?;
+					Nominators::<T>::get(&self.voter.id)?;
 				// Filter out nomination targets which were nominated before the most recent
 				// slashing span.
 				targets.retain(|stash| {
@@ -637,8 +635,7 @@ impl<T: Config> Node<T> {
 						.map_or(true, |spans| submitted_in >= spans.last_nonzero_slash())
 				});
 
-				(!targets.is_empty())
-					.then(move || (self.voter.id.clone(), weight_of(&self.voter.id), targets))
+				(!targets.is_empty()).then(move || (self.voter.id.clone(), voter_weight, targets))
 			},
 		}
 	}

primitives/arithmetic/src/traits.rs

@@ -20,8 +20,8 @@
 use codec::HasCompact;
 pub use integer_sqrt::IntegerSquareRoot;
 pub use num_traits::{
-	checked_pow, Bounded, CheckedAdd, CheckedDiv, CheckedMul, CheckedNeg, CheckedShl, CheckedShr,
-	CheckedSub, One, Signed, Unsigned, Zero,
+	checked_pow, Bounded, CheckedAdd, CheckedDiv, CheckedMul, CheckedNeg, CheckedRem, CheckedShl,
+	CheckedShr, CheckedSub, One, Signed, Unsigned, Zero,
 };
 use sp_std::{
 	self,
@@ -55,6 +55,7 @@ pub trait BaseArithmetic:
 	+ CheckedSub
 	+ CheckedMul
 	+ CheckedDiv
+	+ CheckedRem
 	+ Saturating
 	+ PartialOrd<Self>
 	+ Ord
@@ -109,6 +110,7 @@ impl<
 			+ CheckedSub
 			+ CheckedMul
 			+ CheckedDiv
+			+ CheckedRem
 			+ Saturating
 			+ PartialOrd<Self>
 			+ Ord

primitives/npos-elections/src/lib.rs

@@ -155,7 +155,8 @@ where
 		+ Debug
 		+ Copy
 		+ Clone
-		+ Bounded;
+		+ Bounded
+		+ Encode;
 
 	/// The target type. Needs to be an index (convert to usize).
 	type Target: UniqueSaturatedInto<usize>
@@ -164,7 +165,8 @@ where
 		+ Debug
 		+ Copy
 		+ Clone
-		+ Bounded;
+		+ Bounded
+		+ Encode;
 
 	/// The weight/accuracy type of each vote.
 	type Accuracy: PerThing128;