Skip to content

Deprecate RewardDestination::Controller #2380

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rossbulat merged 49 commits into from
Nov 22, 2023
Merged

Deprecate RewardDestination::Controller #2380

Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
49 commits
Select commit Hold shift + click to select a range
4369870
use `DeprecatedController`, payout same as stash
Nov 17, 2023
a5537d6
add update_payee call
Nov 17, 2023
016d225
add migration test
Nov 17, 2023
446fe8f
get ci working
Nov 17, 2023
46a5062
rm unused payout_stakers_dead_controller
Nov 17, 2023
7dcef6d
stop using `DeprecatedController` variant
Nov 17, 2023
039d618
Merge branch 'master' into rb-deprecate-set-payee-controller
Nov 17, 2023
0119204
fix benchmarks
Nov 17, 2023
2e14496
Merge branch 'rb-deprecate-set-payee-controller' of https://github.co…
Nov 17, 2023
18638c2
tidy up comments
Nov 17, 2023
2b98d99
annotate with deprecated
Nov 17, 2023
096ac39
revert to `Controller`
Nov 17, 2023
7c07d93
Merge branch 'master' into rb-deprecate-set-payee-controller
Nov 17, 2023
8edebd5
add update_payee benchamrk
Nov 17, 2023
7d8021c
Merge branch 'rb-deprecate-set-payee-controller' of https://github.co…
Nov 17, 2023
af75c2f
".git/.scripts/commands/bench/bench.sh" --subcommand=pallet --runtime…
Nov 17, 2023
5614c1c
replace weight call
Nov 17, 2023
797f68c
allow deprecatef in bench
Nov 17, 2023
ce4fdd3
fmt
Nov 17, 2023
e34d97b
add runtime weight
Nov 17, 2023
5caba0c
Merge branch 'master' into rb-deprecate-set-payee-controller
Nov 17, 2023
d652e2e
Merge branch 'master' into rb-deprecate-set-payee-controller
Nov 18, 2023
f0e3759
add error
Nov 19, 2023
34d7fd9
Merge branch 'rb-deprecate-set-payee-controller' of https://github.co…
Nov 19, 2023
11de5a5
Update substrate/frame/staking/src/pallet/mod.rs
Nov 19, 2023
fef57b3
Merge branch 'master' into rb-deprecate-set-payee-controller
Nov 19, 2023
aaf5233
Update substrate/frame/staking/src/pallet/mod.rs
Nov 20, 2023
a83b9bd
rm controller_deprecated
Nov 20, 2023
30a6b5c
Merge branch 'master' into rb-deprecate-set-payee-controller
Nov 20, 2023
45e048f
Merge branch 'master' into rb-deprecate-set-payee-controller
Nov 20, 2023
4bb35d3
add deprecation removal date
Nov 21, 2023
0f95d05
Merge branch 'rb-deprecate-set-payee-controller' of https://github.co…
Nov 21, 2023
06799f2
Merge branch 'master' into rb-deprecate-set-payee-controller
Nov 21, 2023
98860f0
keep Controller payout until post-migration
Nov 21, 2023
2c5cd1f
Merge branch 'rb-deprecate-set-payee-controller' of https://github.co…
Nov 21, 2023
c94ab24
revert
Nov 21, 2023
78d5e38
Merge branch 'master' into rb-deprecate-set-payee-controller
Nov 21, 2023
89a1ed1
return error on not controllererror
Nov 21, 2023
c0299b8
Merge branch 'rb-deprecate-set-payee-controller' of https://github.co…
Nov 21, 2023
e9fe18a
Merge branch 'master' into rb-deprecate-set-payee-controller
Nov 21, 2023
02f5f55
use deprecated!
Nov 21, 2023
5363cc3
Merge branch 'rb-deprecate-set-payee-controller' of https://github.co…
Nov 21, 2023
cec0430
Merge branch 'master' into rb-deprecate-set-payee-controller
Nov 21, 2023
a39745f
".git/.scripts/commands/bench/bench.sh" --subcommand=runtime --runtim…
Nov 21, 2023
c8f1d01
Merge branch 'master' into rb-deprecate-set-payee-controller
Nov 22, 2023
90d4ee1
polishes
Nov 22, 2023
568c676
Merge branch 'rb-deprecate-set-payee-controller' of https://github.co…
Nov 22, 2023
a1fdb2c
rm getter
Nov 22, 2023
ee44468
don't use default
Nov 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 15 additions & 42 deletions polkadot/runtime/westend/src/weights/pallet_staking.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,21 @@ impl<T: frame_system::Config> pallet_staking::WeightInfo for WeightInfo<T> {
.saturating_add(T::DbWeight::get().reads(2))
.saturating_add(T::DbWeight::get().writes(1))
}
/// Storage: `Staking::Ledger` (r:1 w:0)
/// Proof: `Staking::Ledger` (`max_values`: None, `max_size`: Some(1091), added: 3566, mode: `MaxEncodedLen`)
/// Storage: `Staking::Payee` (r:1 w:1)
/// Proof: `Staking::Payee` (`max_values`: None, `max_size`: Some(73), added: 2548, mode: `MaxEncodedLen`)
/// Storage: `Staking::Bonded` (r:1 w:0)
/// Proof: `Staking::Bonded` (`max_values`: None, `max_size`: Some(72), added: 2547, mode: `MaxEncodedLen`)
fn update_payee() -> Weight {
// Proof Size summary in bytes:
// Measured: `969`
// Estimated: `4556`
// Minimum execution time: 20_545_000 picoseconds.
Weight::from_parts(21_395_000, 4556)
.saturating_add(T::DbWeight::get().reads(3_u64))
.saturating_add(T::DbWeight::get().writes(1_u64))
}
/// Storage: `Staking::Bonded` (r:1 w:1)
/// Proof: `Staking::Bonded` (`max_values`: None, `max_size`: Some(72), added: 2547, mode: `MaxEncodedLen`)
/// Storage: `Staking::Ledger` (r:1 w:2)
Expand Down Expand Up @@ -442,48 +457,6 @@ impl<T: frame_system::Config> pallet_staking::WeightInfo for WeightInfo<T> {
.saturating_add(T::DbWeight::get().reads(1))
.saturating_add(T::DbWeight::get().writes(1))
}
/// Storage: `Staking::CurrentEra` (r:1 w:0)
/// Proof: `Staking::CurrentEra` (`max_values`: Some(1), `max_size`: Some(4), added: 499, mode: `MaxEncodedLen`)
/// Storage: `Staking::ErasStakersOverview` (r:1 w:0)
/// Proof: `Staking::ErasStakersOverview` (`max_values`: None, `max_size`: Some(92), added: 2567, mode: `MaxEncodedLen`)
/// Storage: `Staking::ErasValidatorReward` (r:1 w:0)
/// Proof: `Staking::ErasValidatorReward` (`max_values`: None, `max_size`: Some(28), added: 2503, mode: `MaxEncodedLen`)
/// Storage: `Staking::Bonded` (r:65 w:0)
/// Proof: `Staking::Bonded` (`max_values`: None, `max_size`: Some(72), added: 2547, mode: `MaxEncodedLen`)
/// Storage: `Staking::Ledger` (r:1 w:1)
/// Proof: `Staking::Ledger` (`max_values`: None, `max_size`: Some(1091), added: 3566, mode: `MaxEncodedLen`)
/// Storage: `Balances::Locks` (r:1 w:1)
/// Proof: `Balances::Locks` (`max_values`: None, `max_size`: Some(1299), added: 3774, mode: `MaxEncodedLen`)
/// Storage: `Balances::Freezes` (r:1 w:0)
/// Proof: `Balances::Freezes` (`max_values`: None, `max_size`: Some(67), added: 2542, mode: `MaxEncodedLen`)
/// Storage: `System::Account` (r:66 w:66)
/// Proof: `System::Account` (`max_values`: None, `max_size`: Some(128), added: 2603, mode: `MaxEncodedLen`)
/// Storage: `Staking::ClaimedRewards` (r:1 w:1)
/// Proof: `Staking::ClaimedRewards` (`max_values`: None, `max_size`: None, mode: `Measured`)
/// Storage: `Staking::ErasStakersPaged` (r:1 w:0)
/// Proof: `Staking::ErasStakersPaged` (`max_values`: None, `max_size`: None, mode: `Measured`)
/// Storage: `Staking::ErasRewardPoints` (r:1 w:0)
/// Proof: `Staking::ErasRewardPoints` (`max_values`: None, `max_size`: None, mode: `Measured`)
/// Storage: `Staking::ErasValidatorPrefs` (r:1 w:0)
/// Proof: `Staking::ErasValidatorPrefs` (`max_values`: None, `max_size`: Some(57), added: 2532, mode: `MaxEncodedLen`)
/// Storage: `Staking::Payee` (r:65 w:0)
/// Proof: `Staking::Payee` (`max_values`: None, `max_size`: Some(73), added: 2548, mode: `MaxEncodedLen`)
/// The range of component `n` is `[0, 64]`.
fn payout_stakers_dead_controller(n: u32, ) -> Weight {
// Proof Size summary in bytes:
// Measured: `6895 + n * (156 ±0)`
// Estimated: `9802 + n * (2603 ±0)`
// Minimum execution time: 114_338_000 picoseconds.
Weight::from_parts(138_518_124, 0)
.saturating_add(Weight::from_parts(0, 9802))
// Standard Error: 53_621
.saturating_add(Weight::from_parts(25_676_781, 0).saturating_mul(n.into()))
.saturating_add(T::DbWeight::get().reads(14))
.saturating_add(T::DbWeight::get().reads((3_u64).saturating_mul(n.into())))
.saturating_add(T::DbWeight::get().writes(5))
.saturating_add(T::DbWeight::get().writes((1_u64).saturating_mul(n.into())))
.saturating_add(Weight::from_parts(0, 2603).saturating_mul(n.into()))
}
/// Storage: `Staking::Bonded` (r:65 w:0)
/// Proof: `Staking::Bonded` (`max_values`: None, `max_size`: Some(72), added: 2547, mode: `MaxEncodedLen`)
/// Storage: `Staking::Ledger` (r:65 w:65)
Expand Down
53 changes: 17 additions & 36 deletions substrate/frame/staking/src/benchmarking.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,12 @@ const MAX_SLASHES: u32 = 1000;
type MaxValidators<T> = <<T as Config>::BenchmarkingConfig as BenchmarkingConfig>::MaxValidators;
type MaxNominators<T> = <<T as Config>::BenchmarkingConfig as BenchmarkingConfig>::MaxNominators;

// Getter for deprecated `RewardDestination::Controller` variant.
fn controller_rewards_destination<T: Config>() -> RewardDestination<T::AccountId> {
#[allow(deprecated)]
RewardDestination::Controller
}

// Add slashing spans to a user account. Not relevant for actual use, only to benchmark
// read and write operations.
pub fn add_slashing_spans<T: Config>(who: &T::AccountId, spans: u32) {
Expand Down Expand Up @@ -467,9 +473,18 @@ benchmarks! {
let (stash, controller) = create_stash_controller::<T>(USER_SEED, 100, Default::default())?;
assert_eq!(Payee::<T>::get(&stash), RewardDestination::Staked);
whitelist_account!(controller);
}: _(RawOrigin::Signed(controller), RewardDestination::Controller)
}: _(RawOrigin::Signed(controller.clone()), RewardDestination::Account(controller.clone()))
verify {
assert_eq!(Payee::<T>::get(&stash), RewardDestination::Account(controller));
}

update_payee {
let (stash, controller) = create_stash_controller::<T>(USER_SEED, 100, Default::default())?;
Payee::<T>::insert(&stash, controller_rewards_destination::<T>());
whitelist_account!(controller);
}: _(RawOrigin::Signed(controller.clone()), controller.clone())
verify {
assert_eq!(Payee::<T>::get(&stash), RewardDestination::Controller);
assert_eq!(Payee::<T>::get(&stash), RewardDestination::Account(controller));
}

set_controller {
Expand Down Expand Up @@ -551,40 +566,6 @@ benchmarks! {
assert_eq!(UnappliedSlashes::<T>::get(&era).len(), (MAX_SLASHES - s) as usize);
}

payout_stakers_dead_controller {
let n in 0 .. T::MaxExposurePageSize::get() as u32;
let (validator, nominators) = create_validator_with_nominators::<T>(
n,
T::MaxExposurePageSize::get() as u32,
true,
true,
RewardDestination::Controller,
)?;

let current_era = CurrentEra::<T>::get().unwrap();
// set the commission for this particular era as well.
<ErasValidatorPrefs<T>>::insert(current_era, validator.clone(), <Staking<T>>::validators(&validator));

let caller = whitelisted_caller();
let validator_controller = <Bonded<T>>::get(&validator).unwrap();
let balance_before = T::Currency::free_balance(&validator_controller);
for (_, controller) in &nominators {
let balance = T::Currency::free_balance(controller);
ensure!(balance.is_zero(), "Controller has balance, but should be dead.");
}
}: payout_stakers_by_page(RawOrigin::Signed(caller), validator, current_era, 0)
verify {
let balance_after = T::Currency::free_balance(&validator_controller);
ensure!(
balance_before < balance_after,
"Balance of validator controller should have increased after payout.",
);
for (_, controller) in &nominators {
let balance = T::Currency::free_balance(controller);
ensure!(!balance.is_zero(), "Payout not given to controller.");
}
}

payout_stakers_alive_staked {
let n in 0 .. T::MaxExposurePageSize::get() as u32;
let (validator, nominators) = create_validator_with_nominators::<T>(
Expand Down
6 changes: 4 additions & 2 deletions substrate/frame/staking/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,9 +240,9 @@
//! [`Payee`] storage item (see
//! [`set_payee`](Call::set_payee)), to be one of the following:
//!
//! - Controller account, (obviously) not increasing the staked value.
//! - Stash account, not increasing the staked value.
//! - Stash account, also increasing the staked value.
//! - Any other account, sent as free balance.
//!
//! ### Additional Fund Management Operations
//!
Expand Down Expand Up @@ -404,7 +404,9 @@ pub enum RewardDestination<AccountId> {
Staked,
/// Pay into the stash account, not increasing the amount at stake.
Stash,
/// Pay into the controller account.
#[deprecated(
note = "Controller accounts are deprecated. This variant now behaves the same as `Stash` variant"
)]
Controller,
/// Pay into a specified account.
Account(AccountId),
Expand Down
2 changes: 1 addition & 1 deletion substrate/frame/staking/src/mock.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -594,7 +594,7 @@ pub(crate) fn current_era() -> EraIndex {

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));
assert_ok!(Staking::bond(RuntimeOrigin::signed(who), val, RewardDestination::Stash));
}

pub(crate) fn bond_validator(who: AccountId, val: Balance) {
Expand Down
6 changes: 3 additions & 3 deletions substrate/frame/staking/src/pallet/impls.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,9 +338,9 @@ impl<T: Config> Pallet<T> {

let dest = Self::payee(StakingAccount::Stash(stash.clone()));
let maybe_imbalance = match dest {
RewardDestination::Controller => Self::bonded(stash)
.map(|controller| T::Currency::deposit_creating(&controller, amount)),
RewardDestination::Stash => T::Currency::deposit_into_existing(stash, amount).ok(),
#[allow(deprecated)]
Copy link
Copy Markdown
Contributor

@Ank4n Ank4n Nov 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we pay to stash when reward destination is controller? This means it is not migrated yet and controller and stash are different?
It should be paid just like a RD::Account right? (we could also do a migrate of reward destination here)

Copy link
Copy Markdown
Author

@rossbulat rossbulat Nov 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is being paid to stash because the goal is to remove controller accounts completely, & in that scenario they are the same as the stash account. In a world where there are no controllers the stash acts as the controller.

Copy link
Copy Markdown
Author

@rossbulat rossbulat Nov 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means it is not migrated yet and controller and stash are different?

Yes, there will be a period of time after the upgrade where the lazy migration has not processed every record yet, & if payout_stakers is called then the stash will be paid. I think this is a non issue as the lazy migration will be processed extremely quickly once upgrade is live.

we could also do a migrate of reward destination here

The lazy migration call is enough, & will be the only thing to clean up later on.

Copy link
Copy Markdown
Contributor

@Ank4n Ank4n Nov 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree its a non-issue as migration can happen pretty quickly so consider it as a nit but the logic does not seem right.

If migration has not happened, we should pay out the controller account. If migration has happened, we should never get a variant RewardDestination::Controller but RewardDestination::Account(_). Right? (sorry for being pedantic :/ )

Copy link
Copy Markdown
Author

@rossbulat rossbulat Nov 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The placement of controller with stash is based on the post-deprecation logic that stash and controller are the same account. I guess the migration could happen during do_payout_stakers but it would add a lot of additional weight to the call given the worse case where everyone is still using a controller payee.

If migration has happened, we should never get a variant RewardDestination::Controller but RewardDestination::Account(_). Right?

Yes, there will be no RewardDestination::Controller records in storage post-migration.

Copy link
Copy Markdown
Contributor

@Ank4n Ank4n Nov 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree we can avoid migration, but if this ends up happening, paying out stash would be wrong I think. We can just keep the same logic as before (i.e. payout controller account) and also add defensive check on RewardDestination::Controller variant.

Copy link
Copy Markdown
Contributor

@georgepisaltu georgepisaltu Nov 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we pay out to RewardDestination::Controller anyway instead of stash like you suggest, the whole process gets dragged out longer than it needs to get rid of this stuff. Also, with a lazy migration, in order to ensure we get to a state where nobody uses the deprecated controller mechanism anymore, we have to incentivize them somehow to do that migration or do it ourselves with a script or something, which is extra work for us. If we instead give them a whole session (before any payout_stakers is called) to migrate themselves (plenty of time IMO), they have an incentive to do it, which is control their account and rewards. Also, if they don't, they don't lose the rewards, they just go to stash which is still under their control.

I don't feel strongly about this but I prefer the original approach. It might be a bit more blunt, but I don't consider it "wrong". If you disagree, I'm happy to go with what you suggest too, as long as we accept the migration effort too.

Copy link
Copy Markdown
Contributor

@Ank4n Ank4n Nov 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the whole process gets dragged out longer than it needs to get rid of this stuff.

Why would it drag more? Migration is independent of payouts and a new nominator cannot create a controller anymore.

do it ourselves with a script or something

This is @rossbulat's plan I believe otherwise we are stuck for a long time. The migration is also free so a script would migrate them very soon just after the upgrade. So in best case we never have the defensive error.

Also, if they don't, they don't lose the rewards, they just go to stash which is still under their control.

They should never lose reward and they should receive the rewards in the account they declared as their reward destination (which is the controller account in this case). Once all accounts are migrated, we will never have this arm executed so a union of controller and stash variants is a bit ugly and better solution would be to put the deprecated controller arm and have a defensive check that fails in tests. Also note, we are migrating RewardDestination::Controller to RewardDestination::Account(old_controller), so this change should have no functional change to the logic of where rewards are paid out.

Also adding the following snipped to avoid confusion:

match dest {
    RewardDestination::Controller => Self::bonded(stash)
				.map(|controller| {
					defensive!("paying out controller as reward destination which is deprecated and should be migrated");
					// this should have never happened if we lazily migrated everything but if it 
					// does, just pay the controller account.
					T::Currency::deposit_creating(&controller, amount)
	}),
	// other arms ...
};

Copy link
Copy Markdown
Author

@rossbulat rossbulat Nov 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for expanding @Ank4n, please see the latest (02f5f55), it uses the defensive! method.

Copy link
Copy Markdown
Contributor

@Ank4n Ank4n Nov 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good thanks 👍

RewardDestination::Stash | RewardDestination::Controller =>
T::Currency::deposit_into_existing(stash, amount).ok(),
RewardDestination::Staked => Self::ledger(Stash(stash.clone()))
.and_then(|mut ledger| {
ledger.active += amount;
Expand Down
42 changes: 39 additions & 3 deletions substrate/frame/staking/src/pallet/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1283,10 +1283,19 @@ pub mod pallet {
payee: RewardDestination<T::AccountId>,
) -> DispatchResult {
let controller = ensure_signed(origin)?;
let ledger = Self::ledger(Controller(controller))?;
let ledger = Self::ledger(Controller(controller.clone()))?;

// Ensure that the `Controller` variant is not assigned.
#[allow(deprecated)]
let payee_final = if payee == RewardDestination::Controller {
RewardDestination::Account(controller)
} else {
payee
};

let _ = ledger
.set_payee(payee)
.defensive_proof("ledger was retrieved from storage, thus its bonded; qed.");
.set_payee(payee_final)
.defensive_proof("ledger was retrieved from storage, thus its bonded; qed.")?;

Ok(())
}
Expand Down Expand Up @@ -1872,6 +1881,33 @@ pub mod pallet {
ensure_signed(origin)?;
Self::do_payout_stakers_by_page(validator_stash, era, page)
}

/// Migrates an account's `RewardDestination::Controller` to
/// `RewardDestination::Account(controller)`.
///
/// Effects will be felt instantly (as soon as this function is completed successfully).
///
/// This will waive the transaction fee if the `payee` is successfully migrated.
#[pallet::call_index(27)]
#[pallet::weight(T::WeightInfo::update_payee())]
pub fn update_payee(
origin: OriginFor<T>,
controller: T::AccountId,
) -> DispatchResultWithPostInfo {
let _ = ensure_signed(origin)?;
let ledger = Self::ledger(StakingAccount::Controller(controller.clone()))?;

#[allow(deprecated)]
if Payee::<T>::get(&ledger.stash) != RewardDestination::Controller {
return Ok(Pays::Yes.into())
}

let _ = ledger
.set_payee(RewardDestination::Account(controller))
.defensive_proof("ledger was retrieved from storage, thus its bonded; qed.")?;

Ok(Pays::No.into())
}
}
}

Expand Down
Loading