Staking::{bond, set_controller} to set controllers to stash only. (#14039)

* update set_controller

* clone

* bond uses `stash`

* remove controller from bond(), chill_other test works

* remove ctlr from testing_utils &  dead ctlr -> dead payee

* mvs controllers to stashes for 3 tests

* migrate mock bond fns & fix 1 test

* mvs controllers to stashes for 7 tests

* mvs controllers to stashes for 9 tests

* remove double_controlling_should_fail

* remove double_staking_should_fail

* mvs controllers to stashes for 10 tests

* mvs controllers to stashes for 2 tests

* remove payout_creates_controller

* mvs controllers to stashes for 27 tests

* remove println!

* fix rewards_should_work

* fix test_payout_stakers

* fix bond benchmark

* clone

* rm unused import

* rm unused var

* rm controller from create_offender

* fix GenesisConfig stakers

* fix controllers in consensus pallets

* fix unqiue controller in chain_spec

* fmt

* fix create_offender

* fix set_controller benchmark

* add TODO

* create_unique_stash_controller

* staking benchmarks working

* fmt

* fix args

* rm println

* import

* import

* fix fast unstake tests

* fix staking-tests-e2e

* fix root-offenses

* fmt

* differentiate controller to stash

* bring back change_controller_works w. unique ctrl

* bring back double_staking_should_fail

* double_controlling_attempt_should_fail

* bring back payout_creates_controller

* add commnet to controller balances

* + set_controller call description

* fmt

* rm clones

* fmt

* clippy fixes

* fmt

* update README

* small fixes

* use controller_to_be_deprecated

* .comment

* comment

* bump zombienet version

* ci

---------

Co-authored-by: parity-processbot <>
Co-authored-by: Javier Viola <javier@parity.io>

substrate/frame/staking/src/benchmarking.rs

@@ -71,7 +71,8 @@ pub fn add_slashing_spans<T: Config>(who: &T::AccountId, spans: u32) {
 pub fn create_validator_with_nominators<T: Config>(
 	n: u32,
 	upper_bound: u32,
-	dead: bool,
+	dead_controller: bool,
+	unique_controller: bool,
 	destination: RewardDestination<T::AccountId>,
 ) -> Result<(T::AccountId, Vec<(T::AccountId, T::AccountId)>), &'static str> {
 	// Clean up any existing state.
@@ -79,7 +80,12 @@ pub fn create_validator_with_nominators<T: Config>(
 	let mut points_total = 0;
 	let mut points_individual = Vec::new();
 
-	let (v_stash, v_controller) = create_stash_controller::<T>(0, 100, destination.clone())?;
+	let (v_stash, v_controller) = if unique_controller {
+		create_unique_stash_controller::<T>(0, 100, destination.clone(), false)?
+	} else {
+		create_stash_controller::<T>(0, 100, destination.clone())?
+	};
+
 	let validator_prefs =
 		ValidatorPrefs { commission: Perbill::from_percent(50), ..Default::default() };
 	Staking::<T>::validate(RawOrigin::Signed(v_controller).into(), validator_prefs)?;
@@ -93,10 +99,10 @@ pub fn create_validator_with_nominators<T: Config>(
 
 	// Give the validator n nominators, but keep total users in the system the same.
 	for i in 0..upper_bound {
-		let (n_stash, n_controller) = if !dead {
+		let (n_stash, n_controller) = if !dead_controller {
 			create_stash_controller::<T>(u32::MAX - i, 100, destination.clone())?
 		} else {
-			create_stash_and_dead_controller::<T>(u32::MAX - i, 100, destination.clone())?
+			create_unique_stash_controller::<T>(u32::MAX - i, 100, destination.clone(), true)?
 		};
 		if i < n {
 			Staking::<T>::nominate(
@@ -217,15 +223,13 @@ const USER_SEED: u32 = 999666;
 benchmarks! {
 	bond {
 		let stash = create_funded_user::<T>("stash", USER_SEED, 100);
-		let controller = create_funded_user::<T>("controller", USER_SEED, 100);
-		let controller_lookup = T::Lookup::unlookup(controller.clone());
 		let reward_destination = RewardDestination::Staked;
 		let amount = T::Currency::minimum_balance() * 10u32.into();
 		whitelist_account!(stash);
-	}: _(RawOrigin::Signed(stash.clone()), controller_lookup, amount, reward_destination)
+	}: _(RawOrigin::Signed(stash.clone()), amount, reward_destination)
 	verify {
-		assert!(Bonded::<T>::contains_key(stash));
-		assert!(Ledger::<T>::contains_key(controller));
+		assert!(Bonded::<T>::contains_key(stash.clone()));
+		assert!(Ledger::<T>::contains_key(stash));
 	}
 
 	bond_extra {
@@ -470,13 +474,16 @@ benchmarks! {
 	}
 
 	set_controller {
-		let (stash, _) = create_stash_controller::<T>(USER_SEED, 100, Default::default())?;
-		let new_controller = create_funded_user::<T>("new_controller", USER_SEED, 100);
-		let new_controller_lookup = T::Lookup::unlookup(new_controller.clone());
+		let (stash, ctlr) = create_unique_stash_controller::<T>(9000, 100, Default::default(), false)?;
+		// ensure `ctlr` is the currently stored controller.
+		assert!(!Ledger::<T>::contains_key(&stash));
+		assert!(Ledger::<T>::contains_key(&ctlr));
+		assert_eq!(Bonded::<T>::get(&stash), Some(ctlr.clone()));
+
 		whitelist_account!(stash);
-	}: _(RawOrigin::Signed(stash), new_controller_lookup)
+	}: _(RawOrigin::Signed(stash.clone()))
 	verify {
-		assert!(Ledger::<T>::contains_key(&new_controller));
+		assert!(Ledger::<T>::contains_key(&stash));
 	}
 
 	set_validator_count {
@@ -551,6 +558,7 @@ benchmarks! {
 			n,
 			T::MaxNominatorRewardedPerValidator::get() as u32,
 			true,
+			true,
 			RewardDestination::Controller,
 		)?;
 
@@ -584,6 +592,7 @@ benchmarks! {
 			n,
 			T::MaxNominatorRewardedPerValidator::get() as u32,
 			false,
+			true,
 			RewardDestination::Staked,
 		)?;
 
@@ -978,6 +987,7 @@ mod tests {
 				n,
 				<<Test as Config>::MaxNominatorRewardedPerValidator as Get<_>>::get(),
 				false,
+				false,
 				RewardDestination::Staked,
 			)
 			.unwrap();
@@ -1007,6 +1017,7 @@ mod tests {
 				n,
 				<<Test as Config>::MaxNominatorRewardedPerValidator as Get<_>>::get(),
 				false,
+				false,
 				RewardDestination::Staked,
 			)
 			.unwrap();

substrate/frame/staking/src/lib.rs

@@ -67,7 +67,7 @@
 //!
 //! An account pair can become bonded using the [`bond`](Call::bond) call.
 //!
-//! Stash accounts can change their associated controller using the
+//! Stash accounts can update their associated controller back to the stash account using the
 //! [`set_controller`](Call::set_controller) call.
 //!
 //! There are three possible roles that any staked account pair can be in: `Validator`, `Nominator`

substrate/frame/staking/src/mock.rs

@@ -432,7 +432,7 @@ impl ExtBuilder {
 				(2, 20 * self.balance_factor),
 				(3, 300 * self.balance_factor),
 				(4, 400 * self.balance_factor),
-				// controllers
+				// controllers (still used in some tests. Soon to be deprecated).
 				(10, self.balance_factor),
 				(20, self.balance_factor),
 				(30, self.balance_factor),
@@ -465,18 +465,18 @@ impl ExtBuilder {
 			stakers = vec![
 				// (stash, ctrl, stake, status)
 				// these two will be elected in the default test where we elect 2.
-				(11, 10, self.balance_factor * 1000, StakerStatus::<AccountId>::Validator),
-				(21, 20, self.balance_factor * 1000, StakerStatus::<AccountId>::Validator),
+				(11, 11, self.balance_factor * 1000, StakerStatus::<AccountId>::Validator),
+				(21, 21, self.balance_factor * 1000, StakerStatus::<AccountId>::Validator),
 				// a loser validator
-				(31, 30, self.balance_factor * 500, StakerStatus::<AccountId>::Validator),
+				(31, 31, self.balance_factor * 500, StakerStatus::<AccountId>::Validator),
 				// an idle validator
-				(41, 40, self.balance_factor * 1000, StakerStatus::<AccountId>::Idle),
+				(41, 41, self.balance_factor * 1000, StakerStatus::<AccountId>::Idle),
 			];
 			// optionally add a nominator
 			if self.nominate {
 				stakers.push((
 					101,
-					100,
+					101,
 					self.balance_factor * 500,
 					StakerStatus::<AccountId>::Nominator(vec![11, 21]),
 				))
@@ -563,35 +563,24 @@ pub(crate) fn current_era() -> EraIndex {
 	Staking::current_era().unwrap()
 }
 
-pub(crate) fn bond(stash: AccountId, ctrl: AccountId, val: Balance) {
-	let _ = Balances::make_free_balance_be(&stash, val);
-	let _ = Balances::make_free_balance_be(&ctrl, val);
-	assert_ok!(Staking::bond(
-		RuntimeOrigin::signed(stash),
-		ctrl,
-		val,
-		RewardDestination::Controller
-	));
+pub(crate) fn bond(who: AccountId, val: Balance) {
+	let _ = Balances::make_free_balance_be(&who, val);
+	assert_ok!(Staking::bond(RuntimeOrigin::signed(who), val, RewardDestination::Controller));
 }
 
-pub(crate) fn bond_validator(stash: AccountId, ctrl: AccountId, val: Balance) {
-	bond(stash, ctrl, val);
-	assert_ok!(Staking::validate(RuntimeOrigin::signed(ctrl), ValidatorPrefs::default()));
+pub(crate) fn bond_validator(who: AccountId, val: Balance) {
+	bond(who, val);
+	assert_ok!(Staking::validate(RuntimeOrigin::signed(who), ValidatorPrefs::default()));
 	assert_ok!(Session::set_keys(
-		RuntimeOrigin::signed(ctrl),
-		SessionKeys { other: ctrl.into() },
+		RuntimeOrigin::signed(who),
+		SessionKeys { other: who.into() },
 		vec![]
 	));
 }
 
-pub(crate) fn bond_nominator(
-	stash: AccountId,
-	ctrl: AccountId,
-	val: Balance,
-	target: Vec<AccountId>,
-) {
-	bond(stash, ctrl, val);
-	assert_ok!(Staking::nominate(RuntimeOrigin::signed(ctrl), target));
+pub(crate) fn bond_nominator(who: AccountId, val: Balance, target: Vec<AccountId>) {
+	bond(who, val);
+	assert_ok!(Staking::nominate(RuntimeOrigin::signed(who), target));
 }
 
 /// Progress to the given block, triggering session and era changes as we progress.

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

@@ -1651,7 +1651,6 @@ impl<T: Config> StakingInterface for Pallet<T> {
 	) -> DispatchResult {
 		Self::bond(
 			RawOrigin::Signed(who.clone()).into(),
-			T::Lookup::unlookup(who.clone()),
 			value,
 			RewardDestination::Account(payee.clone()),
 		)

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

@@ -645,7 +645,6 @@ pub mod pallet {
 				);
 				frame_support::assert_ok!(<Pallet<T>>::bond(
 					T::RuntimeOrigin::from(Some(stash.clone()).into()),
-					T::Lookup::unlookup(controller.clone()),
 					balance,
 					RewardDestination::Staked,
 				));
@@ -850,19 +849,17 @@ pub mod pallet {
 		#[pallet::weight(T::WeightInfo::bond())]
 		pub fn bond(
 			origin: OriginFor<T>,
-			controller: AccountIdLookupOf<T>,
 			#[pallet::compact] value: BalanceOf<T>,
 			payee: RewardDestination<T::AccountId>,
 		) -> DispatchResult {
 			let stash = ensure_signed(origin)?;
+			let controller_to_be_deprecated = stash.clone();
 
 			if <Bonded<T>>::contains_key(&stash) {
 				return Err(Error::<T>::AlreadyBonded.into())
 			}
 
-			let controller = T::Lookup::lookup(controller)?;
-
-			if <Ledger<T>>::contains_key(&controller) {
+			if <Ledger<T>>::contains_key(&controller_to_be_deprecated) {
 				return Err(Error::<T>::AlreadyPaired.into())
 			}
 
@@ -875,7 +872,7 @@ pub mod pallet {
 
 			// You're auto-bonded forever, here. We might improve this by only bonding when
 			// you actually validate/nominate and remove once you unbond __everything__.
-			<Bonded<T>>::insert(&stash, &controller);
+			<Bonded<T>>::insert(&stash, &stash);
 			<Payee<T>>::insert(&stash, payee);
 
 			let current_era = CurrentEra::<T>::get().unwrap_or(0);
@@ -886,7 +883,7 @@ pub mod pallet {
 			let value = value.min(stash_balance);
 			Self::deposit_event(Event::<T>::Bonded { stash: stash.clone(), amount: value });
 			let item = StakingLedger {
-				stash,
+				stash: stash.clone(),
 				total: value,
 				active: value,
 				unlocking: Default::default(),
@@ -897,7 +894,7 @@ pub mod pallet {
 					// satisfied.
 					.defensive_map_err(|_| Error::<T>::BoundNotMet)?,
 			};
-			Self::update_ledger(&controller, &item);
+			Self::update_ledger(&controller_to_be_deprecated, &item);
 			Ok(())
 		}
 
@@ -1237,7 +1234,10 @@ pub mod pallet {
 			Ok(())
 		}
 
-		/// (Re-)set the controller of a stash.
+		/// (Re-)sets the controller of a stash to the stash itself. This function previously
+		/// accepted a `controller` argument to set the controller to an account other than the
+		/// stash itself. This functionality has now been removed, now only setting the controller
+		/// to the stash, if it is not already.
 		///
 		/// Effects will be felt instantly (as soon as this function is completed successfully).
 		///
@@ -1250,20 +1250,17 @@ pub mod pallet {
 		/// - Writes are limited to the `origin` account key.
 		#[pallet::call_index(8)]
 		#[pallet::weight(T::WeightInfo::set_controller())]
-		pub fn set_controller(
-			origin: OriginFor<T>,
-			controller: AccountIdLookupOf<T>,
-		) -> DispatchResult {
+		pub fn set_controller(origin: OriginFor<T>) -> DispatchResult {
 			let stash = ensure_signed(origin)?;
 			let old_controller = Self::bonded(&stash).ok_or(Error::<T>::NotStash)?;
-			let controller = T::Lookup::lookup(controller)?;
-			if <Ledger<T>>::contains_key(&controller) {
+
+			if <Ledger<T>>::contains_key(&stash) {
 				return Err(Error::<T>::AlreadyPaired.into())
 			}
-			if controller != old_controller {
-				<Bonded<T>>::insert(&stash, &controller);
+			if old_controller != stash {
+				<Bonded<T>>::insert(&stash, &stash);
 				if let Some(l) = <Ledger<T>>::take(&old_controller) {
-					<Ledger<T>>::insert(&controller, l);
+					<Ledger<T>>::insert(&stash, l);
 				}
 			}
 			Ok(())

substrate/frame/staking/src/testing_utils.rs

@@ -76,16 +76,36 @@ pub fn create_stash_controller<T: Config>(
 	balance_factor: u32,
 	destination: RewardDestination<T::AccountId>,
 ) -> Result<(T::AccountId, T::AccountId), &'static str> {
-	let stash = create_funded_user::<T>("stash", n, balance_factor);
-	let controller = create_funded_user::<T>("controller", n, balance_factor);
-	let controller_lookup = T::Lookup::unlookup(controller.clone());
+	let staker = create_funded_user::<T>("stash", n, balance_factor);
 	let amount = T::Currency::minimum_balance() * (balance_factor / 10).max(1).into();
-	Staking::<T>::bond(
-		RawOrigin::Signed(stash.clone()).into(),
-		controller_lookup,
-		amount,
-		destination,
-	)?;
+	Staking::<T>::bond(RawOrigin::Signed(staker.clone()).into(), amount, destination)?;
+	Ok((staker.clone(), staker))
+}
+
+/// Create a unique stash and controller pair.
+pub fn create_unique_stash_controller<T: Config>(
+	n: u32,
+	balance_factor: u32,
+	destination: RewardDestination<T::AccountId>,
+	dead_controller: bool,
+) -> Result<(T::AccountId, T::AccountId), &'static str> {
+	let stash = create_funded_user::<T>("stash", n, balance_factor);
+
+	let controller = if dead_controller {
+		create_funded_user::<T>("controller", n, 0)
+	} else {
+		create_funded_user::<T>("controller", n, balance_factor)
+	};
+	let amount = T::Currency::minimum_balance() * (balance_factor / 10).max(1).into();
+	Staking::<T>::bond(RawOrigin::Signed(stash.clone()).into(), amount, destination)?;
+
+	// update ledger to be a *different* controller to stash
+	if let Some(l) = Ledger::<T>::take(&stash) {
+		<Ledger<T>>::insert(&controller, l);
+	}
+	// update bonded account to be unique controller
+	<Bonded<T>>::insert(&stash, &controller);
+
 	Ok((stash, controller))
 }
 
@@ -95,38 +115,27 @@ pub fn create_stash_controller_with_balance<T: Config>(
 	balance: crate::BalanceOf<T>,
 	destination: RewardDestination<T::AccountId>,
 ) -> Result<(T::AccountId, T::AccountId), &'static str> {
-	let stash = create_funded_user_with_balance::<T>("stash", n, balance);
-	let controller = create_funded_user_with_balance::<T>("controller", n, balance);
-	let controller_lookup = T::Lookup::unlookup(controller.clone());
-
-	Staking::<T>::bond(
-		RawOrigin::Signed(stash.clone()).into(),
-		controller_lookup,
-		balance,
-		destination,
-	)?;
-	Ok((stash, controller))
+	let staker = create_funded_user_with_balance::<T>("stash", n, balance);
+	Staking::<T>::bond(RawOrigin::Signed(staker.clone()).into(), balance, destination)?;
+	Ok((staker.clone(), staker))
 }
 
-/// Create a stash and controller pair, where the controller is dead, and payouts go to controller.
-/// This is used to test worst case payout scenarios.
-pub fn create_stash_and_dead_controller<T: Config>(
+/// Create a stash and controller pair, where payouts go to a dead payee account. This is used to
+/// test worst case payout scenarios.
+pub fn create_stash_and_dead_payee<T: Config>(
 	n: u32,
 	balance_factor: u32,
-	destination: RewardDestination<T::AccountId>,
 ) -> Result<(T::AccountId, T::AccountId), &'static str> {
-	let stash = create_funded_user::<T>("stash", n, balance_factor);
-	// controller has no funds
-	let controller = create_funded_user::<T>("controller", n, 0);
-	let controller_lookup = T::Lookup::unlookup(controller.clone());
+	let staker = create_funded_user::<T>("stash", n, 0);
+	// payee has no funds
+	let payee = create_funded_user::<T>("payee", n, 0);
 	let amount = T::Currency::minimum_balance() * (balance_factor / 10).max(1).into();
 	Staking::<T>::bond(
-		RawOrigin::Signed(stash.clone()).into(),
-		controller_lookup,
+		RawOrigin::Signed(staker.clone()).into(),
 		amount,
-		destination,
+		RewardDestination::Account(payee),
 	)?;
-	Ok((stash, controller))
+	Ok((staker.clone(), staker))
 }
 
 /// create `max` validators.