Staking ledger bonding fixes

substrate/frame/staking/src/ledger.rs

@@ -32,8 +32,8 @@
 //! state consistency.
 
 use frame_support::{
-	defensive,
-	traits::{LockableCurrency, WithdrawReasons},
+	defensive, ensure,
+	traits::{Defensive, LockableCurrency, WithdrawReasons},
 };
 use sp_staking::StakingAccount;
 use sp_std::prelude::*;
@@ -201,6 +201,32 @@ impl<T: Config> StakingLedger<T> {
 		}
 	}
 
+	/// Sets the ledger controller to its stash.
+	pub(crate) fn set_controller_to_stash(self) -> Result<(), Error<T>> {
+		let controller = self.controller.as_ref()
+            .defensive_proof("Ledger's controller field didn't exist. The controller should have been fetched using StakingLedger.")
+            .ok_or(Error::<T>::NotController)?;
+
+		ensure!(self.stash != *controller, Error::<T>::AlreadyPaired);
+
+		match Ledger::<T>::get(&self.stash) {
+			Some(other_ledger) => {
+				if other_ledger.stash != self.stash {
+					// stash is a controller of another ledger, fail with `Error::<T>::DoubleBonded`
+					// to avoid data inconsistencies.
+					return Err(Error::<T>::DoubleBonded);
+				}
+			},
+			None => (), // proceed.
+		}
+
+		<Ledger<T>>::remove(&controller);
+		<Ledger<T>>::insert(&self.stash, &self);
+		<Bonded<T>>::insert(&self.stash, &self.stash);
+
+		Ok(())
+	}
+
 	/// Clears all data related to a staking ledger and its bond in both [`Ledger`] and [`Bonded`]
 	/// storage items and updates the stash staking lock.
 	pub(crate) fn kill(stash: &T::AccountId) -> Result<(), Error<T>> {

substrate/frame/staking/src/mock.rs

@@ -786,6 +786,58 @@ pub(crate) fn bond_controller_stash(controller: AccountId, stash: AccountId) ->
 	Ok(())
 }
 
+pub(crate) fn setup_double_bonded_ledgers() {
+	assert_ok!(Staking::bond(RuntimeOrigin::signed(1), 10, RewardDestination::Staked));
+	assert_ok!(Staking::bond(RuntimeOrigin::signed(2), 20, RewardDestination::Staked));
+	assert_ok!(Staking::bond(RuntimeOrigin::signed(3), 20, RewardDestination::Staked));
+	// not relevant to the test case, but ensures try-runtime checks pass.
+	let _ = [1, 2, 3]
+		.iter()
+		.map(|s| Payee::<Test>::insert(s, RewardDestination::Staked))
+		.collect::<Vec<_>>();
+
+	// we want to test the case where a controller can also be a stash of another ledger.
+	// for that, we change the controller/stash bonding so that:
+	// * 2 becomes controller of 1.
+	// * 3 becomes controller of 2.
+	// * 4 becomes controller of 3.
+	let ledger_1 = Ledger::<Test>::get(1).unwrap();
+	let ledger_2 = Ledger::<Test>::get(2).unwrap();
+	let ledger_3 = Ledger::<Test>::get(3).unwrap();
+
+	// 4 becomes controller of 3.
+	Bonded::<Test>::mutate(3, |controller| *controller = Some(4));
+	Ledger::<Test>::insert(4, ledger_3);
+
+	// 3 becomes controller of 2.
+	Bonded::<Test>::mutate(2, |controller| *controller = Some(3));
+	Ledger::<Test>::insert(3, ledger_2);
+
+	// 2 becomes controller of 1
+	Bonded::<Test>::mutate(1, |controller| *controller = Some(2));
+	Ledger::<Test>::insert(2, ledger_1);
+	// 1 is not controller anymore.
+	Ledger::<Test>::remove(1);
+
+	// checks. now we have:
+	// * 3 ledgers
+	assert_eq!(Ledger::<Test>::iter().count(), 3);
+	// * stash 1 has controller 2.
+	assert_eq!(Bonded::<Test>::get(1), Some(2));
+	assert_eq!(StakingLedger::<Test>::paired_account(StakingAccount::Stash(1)), Some(2));
+	assert_eq!(Ledger::<Test>::get(2).unwrap().stash, 1);
+
+	// * stash 2 has controller 3.
+	assert_eq!(Bonded::<Test>::get(2), Some(3));
+	assert_eq!(StakingLedger::<Test>::paired_account(StakingAccount::Stash(2)), Some(3));
+	assert_eq!(Ledger::<Test>::get(3).unwrap().stash, 2);
+
+	// * stash 3 has controller 4.
+	assert_eq!(Bonded::<Test>::get(3), Some(4));
+	assert_eq!(StakingLedger::<Test>::paired_account(StakingAccount::Stash(3)), Some(4));
+	assert_eq!(Ledger::<Test>::get(4).unwrap().stash, 3);
+}
+
 #[macro_export]
 macro_rules! assert_session_era {
 	($session:expr, $era:expr) => {

substrate/frame/staking/src/pallet/impls.rs

@@ -1836,6 +1836,7 @@ impl<T: Config> Pallet<T> {
 			"VoterList contains non-staker"
 		);
 
+		Self::check_bonded_consistency()?;
 		Self::check_payees()?;
 		Self::check_nominators()?;
 		Self::check_exposures()?;
@@ -1844,9 +1845,58 @@ impl<T: Config> Pallet<T> {
 		Self::check_count()
 	}
 
+	/// Invariants:
+	/// * A bonded (stash, controller) pair should have only one associated ledger. I.e. if the
+	///   ledger is bonded by stash, the controller account must not bond a different ledger. (note:
+	///   only warn, do not fail this check).
+	/// * A bonded (stash, controller) pair must have an associated ledger.
+	fn check_bonded_consistency() -> Result<(), TryRuntimeError> {
+		use sp_std::collections::btree_map::BTreeMap;
+
+		let mut count_double = 0;
+		let mut count_none = 0;
+		// sanity check to ensure that each controller in Bonded storage is associated with only one
+		// ledger.
+		let mut controllers: BTreeMap<T::AccountId, ()> = BTreeMap::new();
+
+		for (stash, controller) in <Bonded<T>>::iter() {
+			ensure!(
+				!controllers.contains_key(&controller),
+				"controller associated with more than 1 ledger"
+			);
+			controllers.insert(controller.clone(), ());
+
+			match (<Ledger<T>>::get(&stash), <Ledger<T>>::get(&controller)) {
+				(Some(_), Some(_)) =>
+				// if stash == controller, it means that the ledger has migrated to
+				// post-controller. If no migration happened, we expect that the (stash,
+				// controller) pair has only one associated ledger.
+					if stash != controller {
+						count_double += 1;
+					},
+				(None, None) => {
+					count_none += 1;
+				},
+				_ => {},
+			};
+		}
+
+		if count_double == 0 {
+			log!(warn, "single tuple of (stash, controller) pair bonds more than one ledger");
+		};
+
+		ensure!(
+			count_none == 0,
+			"inconsistent bonded state: (stash, controller) pair missing associated ledger",
+		);
+
+		Ok(())
+	}
+
 	/// Invariants:
 	/// * A bonded ledger should always have an assigned `Payee`.
 	/// * The number of entries in `Payee` and of bonded staking ledgers *must* match.
+	/// * The stash account in the ledger must match that of the bonded acount.
 	fn check_payees() -> Result<(), TryRuntimeError> {
 		for (stash, _) in Bonded::<T>::iter() {
 			ensure!(Payee::<T>::get(&stash).is_some(), "bonded ledger does not have payee set");
@@ -1861,6 +1911,11 @@ impl<T: Config> Pallet<T> {
 		Ok(())
 	}
 
+	/// Invariants:
+	/// * Number of voters in `VoterList` match that of the number of Nominators and Validators in
+	/// the system (validator is both voter and target).
+	/// * Number of targets in `TargetList` matches the number of validators in the system.
+	/// * Current validator count is bounded by the election provider's max winners.
 	fn check_count() -> Result<(), TryRuntimeError> {
 		ensure!(
 			<T as Config>::VoterList::count() ==
@@ -1879,6 +1934,11 @@ impl<T: Config> Pallet<T> {
 		Ok(())
 	}
 
+	/// Invariants:
+	/// * `ledger.controller` is not stored in the storage (but populated at retrieval).
+	/// * Stake consistency: ledger.total == ledger.active + sum(ledger.unlocking).
+	/// * The controller keyeing the ledger and the ledger stash matches the state of the `Bonded`
+	/// storage.
 	fn check_ledgers() -> Result<(), TryRuntimeError> {
 		Bonded::<T>::iter()
 			.map(|(stash, ctrl)| {
@@ -1896,8 +1956,10 @@ impl<T: Config> Pallet<T> {
 		Ok(())
 	}
 
+	/// Invariants:
+	/// * For each era exposed validator, check if the exposure total is sane (exposure.total  =
+	/// exposure.own + exposure.own).
 	fn check_exposures() -> Result<(), TryRuntimeError> {
-		// a check per validator to ensure the exposure struct is always sane.
 		let era = Self::active_era().unwrap().index;
 		ErasStakers::<T>::iter_prefix_values(era)
 			.map(|expo| {
@@ -1915,6 +1977,10 @@ impl<T: Config> Pallet<T> {
 			.collect::<Result<(), TryRuntimeError>>()
 	}
 
+	/// Invariants:
+	/// * For each paged era exposed validator, check if the exposure total is sane (exposure.total
+	/// = exposure.own + exposure.own).
+	/// * Paged exposures metadata (`ErasStakersOverview`) matches the paged exposures state.
 	fn check_paged_exposures() -> Result<(), TryRuntimeError> {
 		use sp_staking::PagedExposureMetadata;
 		use sp_std::collections::btree_map::BTreeMap;
@@ -1979,6 +2045,8 @@ impl<T: Config> Pallet<T> {
 			.collect::<Result<(), TryRuntimeError>>()
 	}
 
+	/// Invariants:
+	/// * Checks that each nominator has its entire stake correctly distributed.
 	fn check_nominators() -> Result<(), TryRuntimeError> {
 		// a check per nominator to ensure their entire stake is correctly distributed. Will only
 		// kick-in if the nomination was submitted before the current era.

substrate/frame/staking/src/pallet/mod.rs

@@ -791,6 +791,8 @@ pub mod pallet {
 		SnapshotTargetsSizeExceeded { size: u32 },
 		/// A new force era mode was set.
 		ForceEra { mode: Forcing },
+		/// Report of a controller batch deprecation.
+		ControllerBatchDeprecated { failures: u32 },
 	}
 
 	#[pallet::error]
@@ -853,6 +855,9 @@ pub mod pallet {
 		BoundNotMet,
 		/// Used when attempting to use deprecated controller account logic.
 		ControllerDeprecated,
+		/// Account is double bonded, i.e. a stash is also a controller for another ledger or a
+		/// controller is a stash for another ledger.
+		DoubleBonded,
 	}
 
 	#[pallet::hooks]
@@ -1332,8 +1337,6 @@ pub mod pallet {
 		pub fn set_controller(origin: OriginFor<T>) -> DispatchResult {
 			let stash = ensure_signed(origin)?;
 
-			// The bonded map and ledger are mutated directly as this extrinsic is related to a
-			// (temporary) passive migration.
 			Self::ledger(StakingAccount::Stash(stash.clone())).map(|ledger| {
 				let controller = ledger.controller()
                     .defensive_proof("Ledger's controller field didn't exist. The controller should have been fetched using StakingLedger.")
@@ -1343,9 +1346,8 @@ pub mod pallet {
 					// Stash is already its own controller.
 					return Err(Error::<T>::AlreadyPaired.into())
 				}
-				<Ledger<T>>::remove(controller);
-				<Bonded<T>>::insert(&stash, &stash);
-				<Ledger<T>>::insert(&stash, ledger);
+
+				let _ = ledger.set_controller_to_stash()?;
 				Ok(())
 			})?
 		}
@@ -1960,7 +1962,7 @@ pub mod pallet {
 						};
 
 						if ledger.stash != *controller && !payee_deprecated {
-							Some((controller.clone(), ledger))
+							Some(ledger)
 						} else {
 							None
 						}
@@ -1969,13 +1971,12 @@ pub mod pallet {
 				.collect();
 
 			// Update unique pairs.
-			for (controller, ledger) in filtered_batch_with_ledger {
-				let stash = ledger.stash.clone();
-
-				<Bonded<T>>::insert(&stash, &stash);
-				<Ledger<T>>::remove(controller);
-				<Ledger<T>>::insert(stash, ledger);
+			let mut failures = 0;
+			for ledger in filtered_batch_with_ledger {
+				let _ = ledger.clone().set_controller_to_stash().map_err(|_| failures += 1);
 			}
+			Self::deposit_event(Event::<T>::ControllerBatchDeprecated { failures });
+
 			Ok(Some(T::WeightInfo::deprecate_controller_batch(controllers.len() as u32)).into())
 		}
 	}

substrate/frame/staking/src/tests.rs

@@ -7161,4 +7161,78 @@ mod ledger {
 			assert_eq!(ledger_updated.stash, stash);
 		})
 	}
+
+	#[test]
+	fn deprecate_controller_batch_with_double_bonded_ok() {
+		ExtBuilder::default().has_stakers(false).nominate(false).build_and_execute(|| {
+			setup_double_bonded_ledgers();
+
+			// now let's deprecate all the controllers for all the existing ledgers.
+			let bounded_controllers: BoundedVec<
+				_,
+				<Test as Config>::MaxControllersInDeprecationBatch,
+			> = BoundedVec::try_from(vec![1, 2, 3, 4]).unwrap();
+
+			assert_ok!(Staking::deprecate_controller_batch(
+				RuntimeOrigin::root(),
+				bounded_controllers
+			));
+
+			assert_eq!(
+				*staking_events().last().unwrap(),
+				Event::ControllerBatchDeprecated { failures: 0 }
+			);
+		})
+	}
+
+	#[test]
+	fn deprecate_controller_batch_with_double_bonded_failures() {
+		ExtBuilder::default().has_stakers(false).nominate(false).build_and_execute(|| {
+			setup_double_bonded_ledgers();
+
+			// now let's deprecate all the controllers for all the existing ledgers.
+			let bounded_controllers: BoundedVec<
+				_,
+				<Test as Config>::MaxControllersInDeprecationBatch,
+			> = BoundedVec::try_from(vec![4, 3, 2, 1]).unwrap();
+
+			assert_ok!(Staking::deprecate_controller_batch(
+				RuntimeOrigin::root(),
+				bounded_controllers
+			));
+
+			assert_eq!(
+				*staking_events().last().unwrap(),
+				Event::ControllerBatchDeprecated { failures: 2 }
+			);
+		})
+	}
+
+	#[test]
+	fn set_controller_with_double_bonded_ok() {
+		ExtBuilder::default().has_stakers(false).nominate(false).build_and_execute(|| {
+			setup_double_bonded_ledgers();
+
+			assert_ok!(Staking::set_controller(RuntimeOrigin::signed(1)));
+			assert_ok!(Staking::set_controller(RuntimeOrigin::signed(2)));
+			assert_ok!(Staking::set_controller(RuntimeOrigin::signed(3)));
+		})
+	}
+
+	#[test]
+	fn set_controller_with_double_bonded_fail() {
+		ExtBuilder::default().has_stakers(false).nominate(false).build_and_execute(|| {
+			setup_double_bonded_ledgers();
+
+			assert_noop!(
+				Staking::set_controller(RuntimeOrigin::signed(3)),
+				Error::<Test>::DoubleBonded
+			);
+			assert_noop!(
+				Staking::set_controller(RuntimeOrigin::signed(2)),
+				Error::<Test>::DoubleBonded
+			);
+			assert_ok!(Staking::set_controller(RuntimeOrigin::signed(1)));
+		})
+	}
 }