Most things work now

frame/bags-list/src/lib.rs

@@ -272,6 +272,10 @@ impl<T: Config> SortedListProvider<T::AccountId> for Pallet<T> {
 		List::<T>::insert(id, weight)
 	}
 
+	fn get_weight(id: &T::AccountId) -> Result<VoteWeight, Error> {
+		List::<T>::get_weight(id)
+	}
+
 	fn on_update(id: &T::AccountId, new_weight: VoteWeight) {
 		Pallet::<T>::do_rebag(id, new_weight);
 	}

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

@@ -41,6 +41,8 @@ use sp_std::{
 pub enum Error {
 	/// A duplicate id has been detected.
 	Duplicate,
+	/// the given id does not exists.
+	NonExistent,
 }
 
 #[cfg(test)]
@@ -215,6 +217,11 @@ impl<T: Config> List<T> {
 		crate::ListNodes::<T>::contains_key(id)
 	}
 
+	/// Get the weight of the given node,
+	pub fn get_weight(id: &T::AccountId) -> Result<VoteWeight, Error> {
+		Node::<T>::get(id).map(|node| node.weight()).ok_or(Error::NonExistent)
+	}
+
 	/// Iterate over all nodes in all bags in the list.
 	///
 	/// Full iteration can be expensive; it's recommended to limit the number of items with
@@ -269,7 +276,7 @@ impl<T: Config> List<T> {
 		let bag_weight = notional_bag_for::<T>(weight);
 		let mut bag = Bag::<T>::get_or_make(bag_weight);
 		// unchecked insertion is okay; we just got the correct `notional_bag_for`.
-		bag.insert_unchecked(id.clone());
+		bag.insert_unchecked(id.clone(), weight);
 
 		// new inserts are always the tail, so we must write the bag.
 		bag.put();
@@ -616,11 +623,11 @@ impl<T: Config> Bag<T> {
 	///
 	/// Storage note: this modifies storage, but only for the nodes. You still need to call
 	/// `self.put()` after use.
-	fn insert_unchecked(&mut self, id: T::AccountId) {
+	fn insert_unchecked(&mut self, id: T::AccountId, weight: VoteWeight) {
 		// insert_node will overwrite `prev`, `next` and `bag_upper` to the proper values. As long
 		// as this bag is the correct one, we're good. All calls to this must come after getting the
 		// correct [`notional_bag_for`].
-		self.insert_node_unchecked(Node::<T> { id, prev: None, next: None, bag_upper: 0 });
+		self.insert_node_unchecked(Node::<T> { id, weight, prev: None, next: None, bag_upper: 0 });
 	}
 
 	/// Insert a node into this bag.
@@ -756,6 +763,7 @@ pub struct Node<T: Config> {
 	prev: Option<T::AccountId>,
 	next: Option<T::AccountId>,
 	bag_upper: VoteWeight,
+	weight: VoteWeight,
 }
 
 impl<T: Config> Node<T> {
@@ -818,6 +826,11 @@ impl<T: Config> Node<T> {
 		&self.id
 	}
 
+	/// Get the current vote weight of the node.
+	pub(crate) fn weight(&self) -> VoteWeight {
+		self.weight
+	}
+
 	/// Get the underlying voter (public fo tests).
 	#[cfg(feature = "std")]
 	#[allow(dead_code)]

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

@@ -376,11 +376,35 @@ pub trait SortedListProvider<AccountId> {
 	fn contains(id: &AccountId) -> bool;
 
 	/// Hook for inserting a new id.
+	///
+	/// Implementation should return an error if duplicate item is being
 	fn on_insert(id: AccountId, weight: VoteWeight) -> Result<(), Self::Error>;
 
 	/// Hook for updating a single id.
+	///
+	/// The `new` weight is given.
+	// TODO: why not return result here?
 	fn on_update(id: &AccountId, weight: VoteWeight);
 
+	/// Get the weight of `id`.
+	fn get_weight(id: &AccountId) -> Result<VoteWeight, Self::Error>;
+
+	/// Same as `on_update`, but incorporate some increased vote weight.
+	fn on_increase(id: &AccountId, additional: VoteWeight) -> Result<(), Self::Error> {
+		let old_weight = Self::get_weight(id)?;
+		let new_weight = old_weight.saturating_add(additional);
+		Self::on_update(id, new_weight);
+		Ok(())
+	}
+
+	/// Same as `on_update`, but incorporate some decreased vote weight.
+	fn on_decrease(id: &AccountId, decreased: VoteWeight) -> Result<(), Self::Error> {
+		let old_weight = Self::get_weight(id)?;
+		let new_weight = old_weight.saturating_sub(decreased);
+		Self::on_update(id, new_weight);
+		Ok(())
+	}
+
 	/// Hook for removing am id from the list.
 	fn on_remove(id: &AccountId);
 

frame/staking/src/migrations.rs

@@ -17,14 +17,15 @@
 //! Storage migrations for the Staking pallet.
 
 use super::*;
-use frame_election_provider_support::{SortedListProvider, VoteWeightProvider};
-use frame_support::traits::OnRuntimeUpgrade;
+use frame_election_provider_support::{SortedListProvider, VoteWeight, VoteWeightProvider};
+use frame_support::traits::{Defensive, OnRuntimeUpgrade};
+use sp_std::collections::btree_map::BTreeMap;
 
 /// Migration implementation that injects all validators into sorted list.
 ///
 /// This is only useful for chains that started their `VoterList` just based on nominators.
-pub struct InjectValidatorsIntoVoterList<T>(sp_std::marker::PhantomData<T>);
-impl<T: Config> OnRuntimeUpgrade for InjectValidatorsIntoVoterList<T> {
+pub struct InjectValidatorsSelfStakeIntoVoterList<T>(sp_std::marker::PhantomData<T>);
+impl<T: Config> OnRuntimeUpgrade for InjectValidatorsSelfStakeIntoVoterList<T> {
 	fn on_runtime_upgrade() -> Weight {
 		if StorageVersion::<T>::get() == Releases::V8_0_0 {
 			for (v, _) in Validators::<T>::iter() {
@@ -67,15 +68,32 @@ impl<T: Config> OnRuntimeUpgrade for InjectValidatorsIntoVoterList<T> {
 /// Migration implementation that injects all validators into sorted list.
 ///
 /// This is only useful for chains that started their `VoterList` just based on nominators.
-pub struct InjectValidatorsIntoTargetList<T>(sp_std::marker::PhantomData<T>);
-impl<T: Config> OnRuntimeUpgrade for InjectValidatorsIntoTargetList<T> {
+pub struct InjectValidatorsApprovalStakeIntoTargetList<T>(sp_std::marker::PhantomData<T>);
+
+impl<T: Config> InjectValidatorsApprovalStakeIntoTargetList<T> {
+	pub(crate) fn build_approval_stakes() -> BTreeMap<T::AccountId, VoteWeight> {
+		let mut approval_stakes = BTreeMap::<T::AccountId, VoteWeight>::new();
+		Nominators::<T>::iter().for_each(|(who, nomination)| {
+			let stake = Pallet::<T>::weight_of(&who);
+			for target in nomination.targets {
+				let current = approval_stakes.entry(target).or_default();
+				*current = current.saturating_add(stake);
+			}
+		});
+
+		approval_stakes
+	}
+}
+
+impl<T: Config> OnRuntimeUpgrade for InjectValidatorsApprovalStakeIntoTargetList<T> {
 	fn on_runtime_upgrade() -> Weight {
 		if StorageVersion::<T>::get() == Releases::V9_0_0 {
+			// TODO: maybe write this in a multi-block fashion.
+			// TODO: TargetList should store balance, not u64.
+			let approval_stakes = Self::build_approval_stakes();
 			for (v, _) in Validators::<T>::iter() {
-				let weight = Pallet::<T>::vote_weight(&v);
-				let _ = T::TargetList::on_insert(v.clone(), weight).map_err(|err| {
-					log!(warn, "failed to insert {:?} into TargetList: {:?}", v, err)
-				});
+				let approval_stake = approval_stakes.get(&v).map(|x| *x).unwrap_or_default();
+				let _ = T::TargetList::on_insert(v.clone(), approval_stake).defensive();
 			}
 
 			StorageVersion::<T>::put(Releases::V10_0_0);

frame/staking/src/pallet/impls.rs

@@ -201,13 +201,23 @@ impl<T: Config> Pallet<T> {
 
 	/// Update the ledger for a controller.
 	///
-	/// This will also update the stash lock.
+	/// All updates to the ledger amount MUST be reported ot this function, so that the lock amount
+	/// and other bookkeeping is maintained.
 	pub(crate) fn update_ledger(
 		controller: &T::AccountId,
 		ledger: &StakingLedger<T::AccountId, BalanceOf<T>>,
 	) {
 		T::Currency::set_lock(STAKING_ID, &ledger.stash, ledger.total, WithdrawReasons::all());
 		<Ledger<T>>::insert(controller, ledger);
+
+		if let Some(targets) = Self::bonded(controller)
+			.and_then(|stash| Self::nominators(stash))
+			.map(|nomination| nomination.targets)
+		{
+			for target in targets {
+				T::TargetList::on_update(&target, Self::vote_weight(&ledger.stash))
+			}
+		}
 	}
 
 	/// Chill a stash account.
@@ -810,8 +820,6 @@ impl<T: Config> Pallet<T> {
 			// maybe update sorted list.
 			let _ = T::VoterList::on_insert(who.clone(), Self::weight_of(who))
 				.defensive_unwrap_or_default();
-			let _ = T::TargetList::on_insert(who.clone(), Self::weight_of(who))
-				.defensive_unwrap_or_default();
 		}
 		Validators::<T>::insert(who, prefs);
 
@@ -830,7 +838,6 @@ impl<T: Config> Pallet<T> {
 		let outcome = if Validators::<T>::contains_key(who) {
 			Validators::<T>::remove(who);
 			T::VoterList::on_remove(who);
-			T::TargetList::on_remove(who);
 			true
 		} else {
 			false
@@ -853,15 +860,24 @@ impl<T: Config> Pallet<T> {
 	}
 
 	/// Perform all checks related to both [`Config::TargetList`] and [`Config::VoterList`].
-	#[cfg(debug_assertions)]
-	fn sanity_check_list_providers() {
+	#[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()
 		);
 		debug_assert_eq!(Validators::<T>::count(), T::TargetList::count());
+
 		debug_assert_eq!(T::VoterList::sanity_check(), Ok(()));
-		debug_assert_eq!(T::VoterList::sanity_check(), Ok(()));
+		debug_assert_eq!(T::TargetList::sanity_check(), Ok(()));
+
+		// additionally, if asked for the full check, we ensure that the TargetList is indeed
+		// composed of the approval stakes.
+		use crate::migrations::InjectValidatorsApprovalStakeIntoTargetList as TargetListMigration;
+		let approval_stakes = TargetListMigration::<T>::build_approval_stakes();
+		debug_assert!(approval_stakes
+			.iter()
+			.all(|(v, a)| T::TargetList::get_weight(v).unwrap_or_default() == *a))
 	}
 }
 
@@ -1310,6 +1326,10 @@ impl<T: Config> SortedListProvider<T::AccountId> for UseNominatorsAndValidatorsM
 		// nothing to do on insert.
 		Ok(())
 	}
+	fn get_weight(id: &T::AccountId) -> Result<VoteWeight, Self::Error> {
+		// TODO: this is not consistent, should ideally return an error if not exists.
+		Ok(Pallet::<T>::weight_of(id))
+	}
 	fn on_update(_: &T::AccountId, _weight: VoteWeight) {
 		// nothing to do on update.
 	}
@@ -1333,9 +1353,7 @@ impl<T: Config> SortedListProvider<T::AccountId> for UseNominatorsAndValidatorsM
 	}
 }
 
-/// A simple sorted list implementation that does not require any additional pallets. Note, this
-/// does not provided validators in sorted ordered. If you desire nominators in a sorted order take
-/// a look at [`pallet-bags-list].
+// TODO: move this to mock, as it is only for testing anyway.
 pub struct UseValidatorsMap<T>(sp_std::marker::PhantomData<T>);
 impl<T: Config> SortedListProvider<T::AccountId> for UseValidatorsMap<T> {
 	type Error = ();
@@ -1354,6 +1372,15 @@ impl<T: Config> SortedListProvider<T::AccountId> for UseValidatorsMap<T> {
 		// nothing to do on insert.
 		Ok(())
 	}
+	fn get_weight(id: &T::AccountId) -> Result<VoteWeight, Self::Error> {
+		let mut approval_stake = VoteWeight::zero();
+		Nominators::<T>::iter().for_each(|(nominator, nomination)| {
+			if nomination.targets.contains(id) {
+				approval_stake += Pallet::<T>::weight_of(&nominator);
+			}
+		});
+		Ok(approval_stake)
+	}
 	fn on_update(_: &T::AccountId, _weight: VoteWeight) {
 		// nothing to do on update.
 	}

frame/staking/src/pallet/mod.rs

@@ -21,8 +21,8 @@ use frame_election_provider_support::SortedListProvider;
 use frame_support::{
 	pallet_prelude::*,
 	traits::{
-		Currency, CurrencyToVote, EnsureOrigin, EstimateNextNewSession, Get, LockIdentifier,
-		LockableCurrency, OnUnbalanced, UnixTime,
+		Currency, CurrencyToVote, Defensive, DefensiveResult, EnsureOrigin, EstimateNextNewSession,
+		Get, LockIdentifier, LockableCurrency, OnUnbalanced, UnixTime,
 	},
 	weights::Weight,
 };
@@ -568,16 +568,7 @@ pub mod pallet {
 			}
 
 			// all voters are reported to the `VoterList`.
-			assert_eq!(
-				T::VoterList::count(),
-				Nominators::<T>::count() + Validators::<T>::count(),
-				"not all genesis stakers were inserted into sorted list provider, something is wrong."
-			);
-			assert_eq!(
-				T::TargetList::count(),
-				Validators::<T>::count(),
-				"not all genesis stakers were inserted into sorted list provider, something is wrong."
-			);
+			Pallet::<T>::sanity_check_list_providers();
 		}
 	}
 
@@ -830,10 +821,6 @@ pub mod pallet {
 					T::VoterList::on_update(&stash, Self::weight_of(&ledger.stash));
 					debug_assert_eq!(T::VoterList::sanity_check(), Ok(()));
 				}
-				if T::TargetList::contains(&stash) {
-					T::TargetList::on_update(&stash, Self::weight_of(&ledger.stash));
-					debug_assert_eq!(T::VoterList::sanity_check(), Ok(()));
-				}
 
 				Self::deposit_event(Event::<T>::Bonded(stash.clone(), extra));
 			}
@@ -901,9 +888,6 @@ pub mod pallet {
 				if T::VoterList::contains(&ledger.stash) {
 					T::VoterList::on_update(&ledger.stash, Self::weight_of(&ledger.stash));
 				}
-				if T::TargetList::contains(&ledger.stash) {
-					T::TargetList::on_update(&ledger.stash, Self::weight_of(&ledger.stash));
-				}
 
 				Self::deposit_event(Event::<T>::Unbonded(ledger.stash, value));
 			}
@@ -1056,7 +1040,35 @@ pub mod pallet {
 				})
 				.collect::<Result<Vec<_>, _>>()?
 				.try_into()
-				.map_err(|_| Error::<T>::TooManyNominators)?;
+				.defensive_map_err(|_| Error::<T>::TooManyTargets)?;
+
+			ensure!(
+				targets.len() ==
+					targets
+						.iter()
+						.collect::<sp_std::collections::btree_set::BTreeSet<_>>()
+						.len(),
+				"DuplicateTargets"
+			);
+
+			let incoming = targets.iter().cloned().filter(|x| !old.contains(x)).collect::<Vec<_>>();
+			let outgoing = old.iter().cloned().filter(|x| !targets.contains(x)).collect::<Vec<_>>();
+			incoming.into_iter().for_each(|i| {
+				if T::TargetList::contains(&i) {
+					// TODO: these are all rather inefficient now based on how vote_weight is
+					// implemented, but I don't care because: https://github.com/paritytech/substrate/issues/10990
+					let _ = T::TargetList::on_increase(&i, Self::weight_of(stash)).defensive();
+				} else {
+					let _ = T::TargetList::on_insert(i, Self::weight_of(stash)).defensive();
+				}
+			});
+			outgoing.into_iter().for_each(|o| {
+				if T::TargetList::contains(&o) {
+					let _ = T::TargetList::on_decrease(&o, Self::weight_of(stash));
+				} else {
+					frame_support::defensive!("this validator must have, at some point in the past, been inserted into `TargetList`");
+				}
+			});
 
 			let nominations = Nominations {
 				targets,
@@ -1388,9 +1400,6 @@ pub mod pallet {
 			if T::VoterList::contains(&ledger.stash) {
 				T::VoterList::on_update(&ledger.stash, Self::weight_of(&ledger.stash));
 			}
-			if T::TargetList::contains(&ledger.stash) {
-				T::TargetList::on_update(&ledger.stash, Self::weight_of(&ledger.stash));
-			}
 
 			let removed_chunks = 1u32 // for the case where the last iterated chunk is not removed
 				.saturating_add(initial_unlocking)

frame/staking/src/tests.rs

@@ -4071,8 +4071,14 @@ mod election_data_provider {
 				assert_eq!(Staking::targets(Some(6)).unwrap().len(), 4);
 				assert_eq!(Staking::targets(Some(4)).unwrap().len(), 4);
 
-				// if target limit is less, then we return an error.
-				assert_eq!(Staking::targets(Some(1)).unwrap_err(), "Target snapshot too big");
+				// if target limit is less, then we still return something based on `TargetList`
+				// implementation. Currently, it should be the validators with the highest approval
+				// stake.
+				assert_eq!(
+					<Test as crate::Config>::TargetList::iter().take(1).collect::<Vec<_>>(),
+					vec![31]
+				);
+				assert_eq!(Staking::targets(Some(1)).unwrap(), vec![31]);
 			});
 	}