dead-end with restricted nominations

frame/staking/src/migrations.rs

@@ -74,20 +74,21 @@ impl<T: Config> InjectValidatorsApprovalStakeIntoTargetList<T> {
 	pub(crate) fn build_approval_stakes() -> BTreeMap<T::AccountId, VoteWeight> {
 		let mut approval_stakes = BTreeMap::<T::AccountId, VoteWeight>::new();
 
-		NominatorsHelper::<T>::iter_all().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);
-			}
-		});
-
 		Validators::<T>::iter().for_each(|(v, _)| {
 			let stake = Pallet::<T>::weight_of(&v);
 			let current = approval_stakes.entry(v).or_default();
 			*current = current.saturating_add(stake);
 		});
 
+		NominatorsHelper::<T>::iter_all().for_each(|(who, nomination)| {
+			let stake = Pallet::<T>::weight_of(&who);
+			for target in nomination.targets {
+				if let Some(current) = approval_stakes.get_mut(&target) {
+					*current = current.saturating_add(stake);
+				}
+			}
+		});
+
 		approval_stakes
 	}
 }

frame/staking/src/mock.rs

@@ -820,13 +820,6 @@ pub(crate) fn reward_all_elected() {
 	<Pallet<Test>>::reward_by_ids(rewards)
 }
 
-pub(crate) fn validator_controllers() -> Vec<AccountId> {
-	Session::validators()
-		.into_iter()
-		.map(|s| Staking::bonded(&s).expect("no controller for validator"))
-		.collect()
-}
-
 pub(crate) fn on_offence_in_era(
 	offenders: &[OffenceDetails<
 		AccountId,
@@ -925,3 +918,22 @@ pub(crate) fn staking_events() -> Vec<crate::Event<Test>> {
 pub(crate) fn balances(who: &AccountId) -> (Balance, Balance) {
 	(Balances::free_balance(who), Balances::reserved_balance(who))
 }
+
+pub(crate) fn validator_ids() -> Vec<AccountId> {
+	Validators::<Test>::iter().map(|(v, _)| v).collect::<Vec<_>>()
+}
+
+pub(crate) fn nominator_ids() -> Vec<AccountId> {
+	Nominators::<Test>::iter().map(|(n, _)| n).collect::<Vec<_>>()
+}
+
+pub(crate) fn nominator_targets(who: AccountId) -> Vec<AccountId> {
+	Nominators::<Test>::get(&who).map(|n| n.targets).unwrap().into_inner()
+}
+
+pub(crate) fn validator_controllers() -> Vec<AccountId> {
+	Session::validators()
+		.into_iter()
+		.map(|s| Staking::bonded(&s).expect("no controller for validator"))
+		.collect()
+}

frame/staking/src/pallet/impls.rs

@@ -212,22 +212,18 @@ impl<T: Config> Pallet<T> {
 		<Ledger<T>>::insert(controller, ledger);
 
 		let total_issuance = T::Currency::total_issuance();
-		let apply_change = |who: &T::AccountId| {
+		let to_vote = |x| T::CurrencyToVote::to_vote(x, total_issuance);
+
+		let update_target_list = |who: &T::AccountId| {
 			use sp_std::cmp::Ordering;
 			match ledger.active.cmp(&prev_active) {
 				Ordering::Greater => {
-					let _ = T::TargetList::on_increase(
-						who,
-						T::CurrencyToVote::to_vote(ledger.active - prev_active, total_issuance),
-					)
-					.defensive();
+					let _ = T::TargetList::on_increase(who, to_vote(ledger.active - prev_active))
+						.defensive();
 				},
 				Ordering::Less => {
-					let _ = T::TargetList::on_decrease(
-						who,
-						T::CurrencyToVote::to_vote(prev_active - ledger.active, total_issuance),
-					)
-					.defensive();
+					let _ = T::TargetList::on_decrease(who, to_vote(prev_active - ledger.active))
+						.defensive();
 				},
 				Ordering::Equal => (),
 			};
@@ -237,16 +233,26 @@ impl<T: Config> Pallet<T> {
 		if let Some(targets) =
 			NominatorsHelper::<T>::get_any(&ledger.stash).map(|nomination| nomination.targets)
 		{
+			// update the target list.
 			for target in targets {
-				apply_change(&target);
+				update_target_list(&target);
 			}
+
+			// update the voter list.
+			let _ = T::VoterList::on_update(&ledger.stash, to_vote(ledger.active))
+				.defensive_proof("any nominator should an entry in the voter list.");
+
 			#[cfg(debug_assertions)]
 			Self::sanity_check_list_providers();
 		}
 
 		// if this ledger belonged to a validator..
 		if Validators::<T>::contains_key(&ledger.stash) {
-			apply_change(&ledger.stash);
+			update_target_list(&ledger.stash);
+
+			let _ = T::VoterList::on_update(&ledger.stash, to_vote(ledger.active))
+				.defensive_proof("any validator should an entry in the voter list.");
+
 			#[cfg(debug_assertions)]
 			Self::sanity_check_list_providers();
 		}
@@ -259,6 +265,9 @@ impl<T: Config> Pallet<T> {
 		if chilled_as_validator || chilled_as_nominator {
 			Self::deposit_event(Event::<T>::Chilled(stash.clone()));
 		}
+
+		#[cfg(debug_assertions)]
+		Self::sanity_check_approval_stakes();
 	}
 
 	/// Actually make a payment to a staker. This uses the currency's reward function
@@ -610,6 +619,9 @@ impl<T: Config> Pallet<T> {
 		Self::do_remove_validator(stash);
 		Self::do_remove_nominator(stash);
 
+		#[cfg(debug_assertions)]
+		Self::sanity_check_approval_stakes();
+
 		<Bonded<T>>::remove(stash);
 		<Ledger<T>>::remove(&controller);
 		<Payee<T>>::remove(stash);
@@ -808,13 +820,42 @@ impl<T: Config> Pallet<T> {
 	/// NOTE: you must ALWAYS use this function to add nominator or update their targets. Any access
 	/// to `Nominators` or `VoterList` outside of this function is almost certainly
 	/// wrong.
-	pub fn do_add_nominator(who: &T::AccountId, nominations: Nominations<T>) {
-		if !NominatorsHelper::<T>::contains_any(who) {
+	pub fn do_add_nominator(
+		stash: &T::AccountId,
+		old: Vec<T::AccountId>,
+		nominations: Nominations<T>,
+	) {
+		if !NominatorsHelper::<T>::contains_any(stash) {
 			// maybe update sorted list.
-			let _ = T::VoterList::on_insert(who.clone(), Self::weight_of(who))
-				.defensive_unwrap_or_default();
+			let _ = T::VoterList::on_insert(stash.clone(), Self::weight_of(stash)).defensive();
 		}
-		Nominators::<T>::insert(who, nominations);
+
+		let incoming = nominations.targets.iter().filter(|x| !old.contains(x)).collect::<Vec<_>>();
+		let outgoing =
+			old.into_iter().filter(|x| !nominations.targets.contains(x)).collect::<Vec<_>>();
+
+		// TODO: edge case: some wanker nominating themselves? should only be possible if they are a
+		// validator and now they nominate themselves.
+		// 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 weight = Self::weight_of(stash);
+		incoming.into_iter().for_each(|i| {
+			if T::TargetList::contains(i) {
+				let _ = T::TargetList::on_increase(i, weight).defensive();
+			} else {
+				defensive!("no incoming target can not have an entry in the target-list");
+			}
+		});
+		outgoing.into_iter().for_each(|o| {
+			if T::TargetList::contains(&o) {
+				let _ = T::TargetList::on_decrease(&o, weight);
+			} else {
+				// probably the validator has already been chilled and their target list entry
+				// removed.
+			}
+		});
+
+		Nominators::<T>::insert(stash, nominations);
 
 		#[cfg(debug_assertions)]
 		Self::sanity_check_list_providers();
@@ -861,8 +902,7 @@ impl<T: Config> Pallet<T> {
 	pub fn do_add_validator(who: &T::AccountId, prefs: ValidatorPrefs) {
 		if !Validators::<T>::contains_key(who) {
 			// maybe update sorted list.
-			let _ = T::VoterList::on_insert(who.clone(), Self::weight_of(who))
-				.defensive_unwrap_or_default();
+			let _ = T::VoterList::on_insert(who.clone(), Self::weight_of(who)).defensive();
 
 			if T::TargetList::contains(who) {
 				let _ = T::TargetList::on_increase(who, Self::weight_of(who)).defensive();
@@ -890,7 +930,8 @@ impl<T: Config> Pallet<T> {
 		let outcome = if Validators::<T>::contains_key(who) {
 			Validators::<T>::remove(who);
 			let _ = T::VoterList::on_remove(who).defensive();
-			let _ = T::TargetList::on_decrease(who, Self::weight_of(who)).defensive();
+			// NOTE: simply remove, not decrease.
+			let _ = T::TargetList::on_remove(who).defensive();
 			true
 		} else {
 			false
@@ -1024,7 +1065,11 @@ impl<T: Config> ElectionDataProvider for Pallet<T> {
 			},
 		);
 
-		Self::do_add_nominator(&voter, Nominations { targets, submitted_in: 0, suppressed: false });
+		Self::do_add_nominator(
+			&voter,
+			Default::default(),
+			Nominations { targets, submitted_in: 0, suppressed: false },
+		);
 	}
 
 	#[cfg(feature = "runtime-benchmarks")]
@@ -1101,6 +1146,7 @@ impl<T: Config> ElectionDataProvider for Pallet<T> {
 			);
 			Self::do_add_nominator(
 				&v,
+				Default::default(),
 				Nominations { targets: t.try_into().unwrap(), submitted_in: 0, suppressed: false },
 			);
 		});

frame/staking/src/pallet/mod.rs

@@ -878,16 +878,7 @@ pub mod pallet {
 					Error::<T>::InsufficientBond
 				);
 
-				// NOTE: ledger must be updated prior to calling `Self::weight_of`.
 				Self::update_ledger(&controller, &ledger);
-
-				// update this staker in the sorted list, if they exist in it.
-				if T::VoterList::contains(&stash) {
-					let _ =
-						T::VoterList::on_update(&stash, Self::weight_of(&ledger.stash)).defensive();
-					debug_assert_eq!(T::VoterList::sanity_check(), Ok(()));
-				}
-
 				Self::deposit_event(Event::<T>::Bonded(stash.clone(), extra));
 			}
 			Ok(())
@@ -962,16 +953,8 @@ pub mod pallet {
 						.try_push(UnlockChunk { value, era })
 						.map_err(|_| Error::<T>::NoMoreChunks)?;
 				};
-				// NOTE: ledger must be updated prior to calling `Self::weight_of`.
-				Self::update_ledger(&controller, &ledger);
-
-				// TODO: we do some of the update in update ledger and some here... not good.
-				// update this staker in the sorted list, if they exist in it.
-				if T::VoterList::contains(&ledger.stash) {
-					let _ = T::VoterList::on_update(&ledger.stash, Self::weight_of(&ledger.stash))
-						.defensive();
-				}
 
+				Self::update_ledger(&controller, &ledger);
 				Self::deposit_event(Event::<T>::Unbonded(ledger.stash, value));
 			}
 			Ok(())
@@ -1066,6 +1049,10 @@ pub mod pallet {
 
 			Self::do_remove_nominator(stash);
 			Self::do_add_validator(stash, prefs);
+
+			#[cfg(debug_assertions)]
+			Self::sanity_check_approval_stakes();
+
 			Ok(())
 		}
 
@@ -1113,7 +1100,9 @@ pub mod pallet {
 				.map(|t| T::Lookup::lookup(t).map_err(DispatchError::from))
 				.map(|n| {
 					n.and_then(|n| {
-						if old.contains(&n) || !Validators::<T>::get(&n).blocked {
+						if Validators::<T>::contains_key(&n) &&
+							(old.contains(&n) || !Validators::<T>::get(&n).blocked)
+						{
 							Ok(n)
 						} else {
 							Err(Error::<T>::BadTarget.into())
@@ -1133,27 +1122,6 @@ pub mod pallet {
 				Error::<T>::DuplicateTarget
 			);
 
-			let incoming = targets.iter().cloned().filter(|x| !old.contains(x)).collect::<Vec<_>>();
-			let outgoing = old.iter().cloned().filter(|x| !targets.contains(x)).collect::<Vec<_>>();
-
-			// 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 weight = Self::weight_of(stash);
-			incoming.into_iter().for_each(|i| {
-				if T::TargetList::contains(&i) {
-					let _ = T::TargetList::on_increase(&i, weight).defensive();
-				} else {
-					let _ = T::TargetList::on_insert(i, weight).defensive();
-				}
-			});
-			outgoing.into_iter().for_each(|o| {
-				if T::TargetList::contains(&o) {
-					let _ = T::TargetList::on_decrease(&o, weight);
-				} else {
-					frame_support::defensive!("this validator must have, at some point in the past, been inserted into `TargetList`");
-				}
-			});
-
 			let nominations = Nominations {
 				targets,
 				// Initial nominations are considered submitted at era 0. See `Nominations` doc.
@@ -1162,7 +1130,7 @@ pub mod pallet {
 			};
 
 			Self::do_remove_validator(stash);
-			Self::do_add_nominator(stash, nominations);
+			Self::do_add_nominator(stash, old, nominations);
 
 			// NOTE: we need to do this after all validators and nominators have been updated in the
 			// previous two function calls.
@@ -1488,7 +1456,8 @@ pub mod pallet {
 			Self::update_ledger(&controller, &ledger);
 
 			if T::VoterList::contains(&ledger.stash) {
-				T::VoterList::on_update(&ledger.stash, Self::weight_of(&ledger.stash)).defensive();
+				let _ = T::VoterList::on_update(&ledger.stash, Self::weight_of(&ledger.stash))
+					.defensive();
 			}
 
 			let removed_chunks = 1u32 // for the case where the last iterated chunk is not removed