cleanup a few things

frame/bags-list/src/list/tests.rs

@@ -451,7 +451,10 @@ mod list {
 			List::<Runtime>::insert_at_unchecked(node_1, node_42);
 
 			// then
-			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![42, 1]), (1_000, vec![2, 3, 4])]);
+			assert_eq!(
+				List::<Runtime>::get_bags(),
+				vec![(10, vec![42, 1]), (1_000, vec![2, 3, 4])]
+			);
 		})
 	}
 
@@ -478,7 +481,10 @@ mod list {
 			List::<Runtime>::insert_at_unchecked(node_2, node_42);
 
 			// then
-			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![42, 2, 3, 4])]);
+			assert_eq!(
+				List::<Runtime>::get_bags(),
+				vec![(10, vec![1]), (1_000, vec![42, 2, 3, 4])]
+			);
 		})
 	}
 
@@ -505,7 +511,10 @@ mod list {
 			List::<Runtime>::insert_at_unchecked(node_3, node_42);
 
 			// then
-			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 42, 3, 4])]);
+			assert_eq!(
+				List::<Runtime>::get_bags(),
+				vec![(10, vec![1]), (1_000, vec![2, 42, 3, 4])]
+			);
 		})
 	}
 
@@ -532,7 +541,10 @@ mod list {
 			List::<Runtime>::insert_at_unchecked(node_4, node_42);
 
 			// then
-			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 42, 4])]);
+			assert_eq!(
+				List::<Runtime>::get_bags(),
+				vec![(10, vec![1]), (1_000, vec![2, 3, 42, 4])]
+			);
 		})
 	}
 }

frame/election-provider-support/src/lib.rs

@@ -173,7 +173,7 @@ use sp_std::{fmt::Debug, prelude::*};
 
 /// Re-export the solution generation macro.
 pub use frame_election_provider_solution_type::generate_solution_type;
-use frame_support::{traits::Get, BoundedVec, RuntimeDebug, weights::Weight};
+use frame_support::{traits::Get, weights::Weight, BoundedVec, RuntimeDebug};
 /// Re-export some type as they are used in the interface.
 pub use sp_arithmetic::PerThing;
 pub use sp_npos_elections::{

frame/staking/src/mock.rs

@@ -37,7 +37,7 @@ use sp_runtime::{
 	traits::{IdentityLookup, Zero},
 };
 use sp_staking::offence::{DisableStrategy, OffenceDetails, OnOffenceHandler};
-use std::{cell::RefCell, cmp::Ordering};
+use std::{cell::RefCell};
 
 pub const INIT_TIMESTAMP: u64 = 30_000;
 pub const BLOCK_TIME: u64 = 1000;
@@ -288,59 +288,60 @@ impl<T: Config> sp_staking::OnStakerSlash<AccountId, Balance> for OnStakerSlashM
 	}
 }
 
-pub struct TargetBagListCompat;
-impl SortedListProvider<AccountId> for TargetBagListCompat {
-	type Error = <TargetBagsList as SortedListProvider<AccountId>>::Error;
-	type Score = <TargetBagsList as SortedListProvider<AccountId>>::Score;
-
-	fn iter() -> Box<dyn Iterator<Item = AccountId>> {
-		let mut all = TargetBagsList::iter()
-			.map(|x| (x, TargetBagsList::get_score(&x).unwrap_or_default()))
-			.collect::<Vec<_>>();
-		all.sort_by(|a, b| match a.1.partial_cmp(&b.1).unwrap() {
-			Ordering::Equal => b.0.partial_cmp(&a.0).unwrap(),
-			x @ _ => x,
-		});
-		Box::new(all.into_iter().map(|(x, _)| x))
-	}
-	fn iter_from(start: &AccountId) -> Result<Box<dyn Iterator<Item = AccountId>>, Self::Error> {
-		TargetBagsList::iter_from(start)
-	}
-	fn count() -> u32 {
-		TargetBagsList::count()
-	}
-	fn contains(id: &AccountId) -> bool {
-		TargetBagsList::contains(id)
-	}
-	fn on_insert(id: AccountId, weight: Self::Score) -> Result<(), Self::Error> {
-		TargetBagsList::on_insert(id, weight)
-	}
-	fn on_update(id: &AccountId, weight: Self::Score) -> Result<(), Self::Error> {
-		TargetBagsList::on_update(id, weight)
-	}
-	fn get_score(id: &AccountId) -> Result<Self::Score, Self::Error> {
-		TargetBagsList::get_score(id)
-	}
-	fn on_remove(id: &AccountId) -> Result<(), Self::Error> {
-		TargetBagsList::on_remove(id)
-	}
-	fn unsafe_regenerate(
-		all: impl IntoIterator<Item = AccountId>,
-		weight_of: Box<dyn Fn(&AccountId) -> Self::Score>,
-	) -> u32 {
-		TargetBagsList::unsafe_regenerate(all, weight_of)
-	}
-	fn unsafe_clear() {
-		TargetBagsList::unsafe_clear();
-	}
-	fn sanity_check() -> Result<(), &'static str> {
-		TargetBagsList::sanity_check()
-	}
-	#[cfg(feature = "runtime-benchmarks")]
-	fn score_update_worst_case(_who: &AccountId, _is_increase: bool) -> Self::Score {
-		Balance::MAX
-	}
-}
+// pub struct TargetBagsListCompat;
+// impl SortedListProvider<AccountId> for TargetBagsListCompat {
+// 	type Error = <TargetBagsList as SortedListProvider<AccountId>>::Error;
+// 	type Score = <TargetBagsList as SortedListProvider<AccountId>>::Score;
+
+// 	fn iter() -> Box<dyn Iterator<Item = AccountId>> {
+// 		let mut all = TargetBagsList::iter()
+// 			.map(|x| (x, TargetBagsList::get_score(&x).unwrap_or_default()))
+// 			.collect::<Vec<_>>();
+// 		dbg!(&all);
+// 		all.sort_by(|a, b| match a.1.partial_cmp(&b.1).unwrap() {
+// 			Ordering::Equal => b.0.partial_cmp(&a.0).unwrap(),
+// 			x @ _ => x,
+// 		});
+// 		Box::new(all.into_iter().map(|(x, _)| x))
+// 	}
+// 	fn iter_from(start: &AccountId) -> Result<Box<dyn Iterator<Item = AccountId>>, Self::Error> {
+// 		TargetBagsList::iter_from(start)
+// 	}
+// 	fn count() -> u32 {
+// 		TargetBagsList::count()
+// 	}
+// 	fn contains(id: &AccountId) -> bool {
+// 		TargetBagsList::contains(id)
+// 	}
+// 	fn on_insert(id: AccountId, weight: Self::Score) -> Result<(), Self::Error> {
+// 		TargetBagsList::on_insert(id, weight)
+// 	}
+// 	fn on_update(id: &AccountId, weight: Self::Score) -> Result<(), Self::Error> {
+// 		TargetBagsList::on_update(id, weight)
+// 	}
+// 	fn get_score(id: &AccountId) -> Result<Self::Score, Self::Error> {
+// 		TargetBagsList::get_score(id)
+// 	}
+// 	fn on_remove(id: &AccountId) -> Result<(), Self::Error> {
+// 		TargetBagsList::on_remove(id)
+// 	}
+// 	fn unsafe_regenerate(
+// 		all: impl IntoIterator<Item = AccountId>,
+// 		weight_of: Box<dyn Fn(&AccountId) -> Self::Score>,
+// 	) -> u32 {
+// 		TargetBagsList::unsafe_regenerate(all, weight_of)
+// 	}
+// 	fn unsafe_clear() {
+// 		TargetBagsList::unsafe_clear();
+// 	}
+// 	fn sanity_check() -> Result<(), &'static str> {
+// 		TargetBagsList::sanity_check()
+// 	}
+// 	#[cfg(feature = "runtime-benchmarks")]
+// 	fn score_update_worst_case(_who: &AccountId, _is_increase: bool) -> Self::Score {
+// 		Balance::MAX
+// 	}
+// }
 
 impl crate::pallet::pallet::Config for Test {
 	type MaxNominations = MaxNominations;
@@ -365,7 +366,7 @@ impl crate::pallet::pallet::Config for Test {
 	type GenesisElectionProvider = Self::ElectionProvider;
 	// NOTE: consider a macro and use `UseNominatorsAndValidatorsMap<Self>` as well.
 	type VoterList = VoterBagsList;
-	type TargetList = TargetBagListCompat;
+	type TargetList = TargetBagsList;
 	type MaxUnlockingChunks = ConstU32<32>;
 	type OnStakerSlash = OnStakerSlashMock<Test>;
 	type BenchmarkingConfig = TestBenchmarkingConfig;

frame/staking/src/pallet/impls.rs

@@ -252,8 +252,7 @@ impl<T: Config> Pallet<T> {
 			let _ = T::VoterList::on_update(&ledger.stash, to_vote(ledger.active))
 				.defensive_proof("any nominator should have an entry in the voter list.");
 
-			#[cfg(debug_assertions)]
-			Self::sanity_check_list_providers();
+			debug_assert!(Self::sanity_check_list_providers().is_ok());
 		}
 
 		// if this ledger belonged to a validator..
@@ -263,8 +262,7 @@ impl<T: Config> Pallet<T> {
 			let _ = T::VoterList::on_update(&ledger.stash, to_vote(ledger.active))
 				.defensive_proof("any validator should have an entry in the voter list.");
 
-			#[cfg(debug_assertions)]
-			Self::sanity_check_list_providers();
+			debug_assert!(Self::sanity_check_list_providers().is_ok());
 		}
 	}
 
@@ -276,8 +274,7 @@ impl<T: Config> Pallet<T> {
 			Self::deposit_event(Event::<T>::Chilled(stash.clone()));
 		}
 
-		#[cfg(debug_assertions)]
-		Self::sanity_check_approval_stakes();
+		debug_assert!(Self::sanity_check_list_providers().is_ok())
 	}
 
 	/// Actually make a payment to a staker. This uses the currency's reward function
@@ -628,8 +625,7 @@ impl<T: Config> Pallet<T> {
 		Self::do_remove_validator(stash);
 		Self::do_remove_nominator(stash);
 
-		#[cfg(debug_assertions)]
-		Self::sanity_check_approval_stakes();
+		debug_assert!(Self::sanity_check_list_providers().is_ok());
 
 		<Bonded<T>>::remove(stash);
 		<Ledger<T>>::remove(&controller);
@@ -813,15 +809,31 @@ impl<T: Config> Pallet<T> {
 	///
 	/// This function is self-weighing as [`DispatchClass::Mandatory`].
 	pub fn get_npos_targets(maybe_max_len: Option<usize>) -> Vec<T::AccountId> {
-		// TODO: what comes out here might not be a validator.
-		let targets = T::TargetList::iter()
-			.take(maybe_max_len.unwrap_or_else(Bounded::max_value))
-			.collect::<Vec<_>>();
+		let max_allowed_len = maybe_max_len.unwrap_or_else(|| T::TargetList::count() as usize);
+		let mut all_targets = Vec::<T::AccountId>::with_capacity(max_allowed_len);
+		let mut targets_seen = 0;
 
-		Self::register_weight(T::WeightInfo::get_npos_targets(targets.len() as u32));
-		log!(info, "generated {} npos targets", targets.len());
+		let mut targets_iter = T::TargetList::iter();
+		while all_targets.len() < max_allowed_len &&
+			targets_seen < (NPOS_MAX_ITERATIONS_COEFFICIENT * max_allowed_len as u32)
+		{
+			let target = match targets_iter.next() {
+				Some(target) => {
+					targets_seen.saturating_inc();
+					target
+				},
+				None => break,
+			};
+
+			if Validators::<T>::contains_key(&target) {
+				all_targets.push(target);
+			}
+		}
+
+		Self::register_weight(T::WeightInfo::get_npos_targets(all_targets.len() as u32));
+		log!(info, "generated {} npos targets", all_targets.len());
 
-		targets
+		all_targets
 	}
 
 	/// This function will add a nominator to the `Nominators` storage map,
@@ -865,9 +877,7 @@ impl<T: Config> Pallet<T> {
 		});
 
 		Nominators::<T>::insert(stash, nominations);
-
-		#[cfg(debug_assertions)]
-		Self::sanity_check_list_providers();
+		debug_assert!(Self::sanity_check_list_providers().is_ok())
 	}
 
 	/// This function will remove a nominator from the `Nominators` storage map,
@@ -895,9 +905,7 @@ impl<T: Config> Pallet<T> {
 			false
 		};
 
-		#[cfg(debug_assertions)]
-		Self::sanity_check_list_providers();
-
+		debug_assert!(Self::sanity_check_list_providers().is_ok());
 		outcome
 	}
 
@@ -921,9 +929,7 @@ impl<T: Config> Pallet<T> {
 		}
 
 		Validators::<T>::insert(who, prefs);
-
-		#[cfg(debug_assertions)]
-		Self::sanity_check_list_providers();
+		debug_assert!(Self::sanity_check_list_providers().is_ok());
 	}
 
 	/// This function will remove a validator from the `Validators` storage map.
@@ -946,8 +952,7 @@ impl<T: Config> Pallet<T> {
 			false
 		};
 
-		#[cfg(debug_assertions)]
-		Self::sanity_check_list_providers();
+		debug_assert!(Self::sanity_check_list_providers().is_ok());
 
 		outcome
 	}
@@ -964,33 +969,37 @@ impl<T: Config> Pallet<T> {
 
 	/// Perform all checks related to both [`Config::TargetList`] and [`Config::VoterList`].
 	#[cfg(any(debug_assertions, feature = "std"))]
-	pub(crate) fn sanity_check_list_providers() {
-		debug_assert_eq!(
-			Nominators::<T>::count() + Validators::<T>::count(),
-			T::VoterList::count()
+	pub(crate) fn sanity_check_list_providers() -> Result<(), &'static str> {
+		ensure!(
+			Nominators::<T>::count() + Validators::<T>::count() == T::VoterList::count(),
+			"invalid voter list count",
 		);
-
-		debug_assert_eq!(T::VoterList::sanity_check(), Ok(()));
-		debug_assert_eq!(T::TargetList::sanity_check(), Ok(()));
 		// note that we cannot say much about the count of the target list.
+
+		let _ = T::VoterList::sanity_check()?;
+		let _ = T::TargetList::sanity_check()?;
+
+		Ok(())
 	}
 
 	#[cfg(any(debug_assertions, feature = "std"))]
-	pub(crate) fn sanity_check_approval_stakes() {
+	pub(crate) fn sanity_check_approval_stakes() -> Result<(), &'static str> {
 		// additionally, if asked for the full check, we ensure that the TargetList is indeed
 		// composed of the approval stakes.
 		use crate::migrations::v10::InjectValidatorsApprovalStakeIntoTargetList as TargetListMigration;
 		let approval_stakes = TargetListMigration::<T>::build_approval_stakes();
-		approval_stakes.iter().for_each(|(v, a)| {
-			debug_assert_eq!(
-				T::TargetList::get_score(v).unwrap_or_default(),
-				*a,
-				"staker {:?}, score in TargetList {:?}, computed: {:?}",
-				v,
-				T::TargetList::get_score(v).unwrap_or_default(),
-				a
-			)
-		});
+		approval_stakes
+			.iter()
+			.map(|(v, a)| {
+				let known = T::TargetList::get_score(v).unwrap_or_default();
+				if known != *a {
+					log!(error, "wrong approval computed for {:?}: {:?} != {:?}", v, known, *a);
+					Err("bad approval")
+				} else {
+					Ok(())
+				}
+			})
+			.collect::<Result<_, _>>()
 	}
 }