Revision of `RewardDestination` to account for Split and Controller removal

Tech Talk
1

I’m posting this to flag to the community that I am exploring how to improve the RewardDestination struct of staking, which determines how your staking reward is received. I’ll review what we have currently, the problems we face & a proposed iteration to solve those problems.

Current Enum

The current structure of RewardDestination is as follows:

  • Staked: compound your rewards (auto bond them)
  • Stash: send rewards to your stash account as free balance.
  • Controller: send rewards to your controller account as free balance.
  • Account(AccountId): send rewards to an ad-hoc account as free balance.
  • None: do not receive staking rewards.

Problems

The problems we are facing here are as follows:

  • We need to remove the Controller variant if we are to fully deprecate controller accounts. Controller is index 3 of the enum, so at the least, a migration will be needed for index 3, 4, and 5. There are > 68k Payee records on Polkadot, so this would entail a lazy migration.
  • There is no splitting mechanism whereby we can compound a % of rewards, and have the rest as free balance. Such a feature has been discussed over the years.
  • If the Controller variant is removed then we realise that the Stash and Account variants are essentially the same thing.
  • If we simply add the Split variant to the existing enum, we still have the Controller variant that would eventually need to be removed, and result in an even larger migration.

Propossed New Enum

What I propose is to simplify the enum variants based on whether the rewards be compounded or be free balance. This new enum, PayeeDestination, will remove Controller and introduce Split((Perbill, AccountId)) whereby the Perbill amount of rewards will be sent to the AccountId, and the rest compounded. Over 2 runtime upgrades we can lazily migrate storage to this new enum, with the latter enabling the new Split variant.

The entire proposed enum is as follows:

pub enum PayeeDestination<AccountId> {
  /// Pay into the staking account and compound to bond.
  Compound,
  /// Pay a part back into the stash and compound to bond, and send another part to a specified account as free balance.
  Split((Perbill, AccountId)),
  /// Pay into a specified account as free balance.
  Free(AccountId),
  /// Receive no reward.
  None, 
}

We no longer have stash and controller terminology, and the terminology is simplified to account for compounding (Compound), free balance (Free) or both (Split). None remains.

Further Splitting and Exotic Routing

This proposed change assumes that the staking pallet maintains a simple mechanism for splitting rewards - it should not be responsible for housing complex logic for more exotic splitting of rewards. Instead it only cares about the ability to re-stake or not. For more capable mechanisms smart contracts or dedicated pallets can take on the role of providing better splitting support.

So given the Split option is available, users will be able to compound a chunk of their rewards back into staking, and send the other chunk as free balance to an address, perhaps a smart contract, that could futher split the reward, exchange it into other tokens, & route to other accounts / platforms / DeFi products, and so-forth. This scenario is of course not a part of this current migration, but is an eventual outcome that we can work towards.

Current Progress

If part 1 is supported and merged, this paves the way to fully transition to PayeeDestination and introduce splitting logic to set_payee and payout_stakers in a second PR.

6 Likes
2

From a weights perspective we can see that the current weights of payout_stakers comfortably fit into a block, & even doubling to roughly account for the new Split variant of the additional write operation still fits comfortably in weight limits, with a max of ~34% consumed block:

Ref time (normal class)

max: 1479890237000

bench: payout_stakers_dead_controller
ref time: 105814921360
percentage of block consumed: 7.15%

bench: current payout_stakers_alive_staked
ref time: 256643002555
percentage of block consumed: 17.34%

Proof size (normal class):

max: 13650590614545068195

payout_stakers_dead_controller: 1352580
payout_stakers_alive_staked: 1963133

These percentages of block consumed will dramatically decrease once reward payouts are multi-page. The PR that will enable multi-paging of rewards is currently pending merge at https://github.com/paritytech/substrate/pull/13498.

We can further prove updated block weights by implementing the Split mechanism in payout_stakers and re-run the benchmarks. This will be added in a following post.

1 Like
3

I am in favor of this initiative. I want to emphasize again that:

1- multi-page rewards will assist this work wrt. weight.
2- using FRAME’s new multi-block-migration system will assist this work wrt. migration.
3- further exploration can be done at the smart contract layer. The core staking pallet can indeed not grow indefinitely.

If the relay chain had some notion of smart contracts, I would have even suggested that a “system compound contract” could have gotten the job done. One staker would be able to set this account as their destination, with a percenta configured in the contract.

I encourage ink! developers to explore such ideas in hackathon and similar events.

2 Likes
4

Why does this have to be a breaking change with migration, rather than a perfectly backwards compatible one?

Just introduce a new variant, and then nothing in the ecosystem breaks.

5

Because the Controller variant should be removed as controller accounts are being deprecated.

6

Completely unrelated but I really want to ask: where do we draw the line to between features should be in the core staking pallet vs something can be done by other parachains?

We already have multiple staking products offered by multiple parachains. It is also possible to implement most of those logics in a smart contract chain with XCM.

I believe the core staking pallet should be secure, simple and stable. More features, breaking features, migrations, codes, they all against secure, simple and stable.

1 Like
7

True enough, but can’t you also just remove the Controller variant, and used numbered codec indices so that the index of the variants don’t change?

8

Thanks for all the valuable feedback.

Provided that scale allows us to replace the Controller variant with Split((Perbill, AccountId)) without any migration, I’ve realised that there is a way we can introduce Split and remove Controller without introducing a new enum.

Phase 1, stop using Controller & move existing entries to Account

Move Controller Payees to Account(AccountId) via set_payee and a temporary update_payee call, which I’d call after the upgrade via a bot. This will ensure no one is using Controller by the next upgrade.

Phase 2: Replace Controller with Split((Perbill, AccountId)) & intro Split logic

Replace Controller at index 3 with Split((Perbill, AccountId)) & insert new Split logic.

The final RewardDestination enum would be:

pub enum PayeeDestination<AccountId> {
  /// Renamed Staked to Compound
  Compound,
  /// Untouched
  Stash,
  /// Replaced Controller with Split
  Split((Perbill, AccountId)),
  /// Renamed Account to Free
  Free(AccountId),
  /// Untouched
  None, 
}
9

Just so that I understand this correctly: Free(ALICE) is the same as Split((100%, ALICE)), or?
Then why do we need two variants? There could just be a fast-path for Perbill == One::one().

2 Likes
10

Adding to what Oliver said, I do not find this enum very expressive of the possible destinations.

  • Why rename RewardDestination to PayeeDestination? This doesn’t make sense; the payee does not have a destination, the reward does.
  • Why rename Staked to Compound? Saying that you “stake” your reward is very expressive of what is happening to the reward.
  • Stash and Free both seem unnecessary in the presence of Split.
  • The variants are not grammatically consistent. Compound is a verb, Split can be a verb or adjective depending on usage, so can Free (although is an adjective in this context), None is a noun. If you want to go the all-adjective route, you should use Compounded. You should definitely drop None in favor of a verb or adjective, or make the storage Option<RewardDestination>, where None implies forgoing the reward.

Going the verb route would look like:

/// Instructions from the staker about what to do with rewards.
enum RewardAction {
    /// Add the reward to the amount at stake.
    Stake,
    /// Split the reward with some going to the free balance of `AccountId`,
    /// and add the remainder to stake.
    Split((Perbill, AccountId)),
    /// Forgo the claim on this reward, such that the reward is never minted.
    Forgo,
}
1 Like
11

Why rename RewardDestination to PayeeDestination

Because in the case of a lazy migration to the original proposal another storage enum pair needs to be created, & payee desitnation is just a bettter name than reward destination.

Why rename Staked to Compound

Disagree, compound is easier to understand. I like the verb version with Stake though, that works really well.

Stash and Free both seem unnecessary

Stash is but free is explicitly telling 100% free without further encoding of tuple. Original proposal did not have Stash included, Only remains to avoid index changing

The variants are not grammatically consistent

disagree. The original proposal is with lazy migration. Revised enum includes stash raising inconsistency to avoid a migration

Forgo is fine, actually sounds better than None

12

With this logic why do we have three variations, Stash, Controller and Account(AccountId), that do exactly the same thing now? Why was it not just Account to begin with? Why was stash and controller terminology baked into this enum when it should be agnostic of it?

The answer i am expecting is less storage in favour of more call logic. This would give reason to Split and Free being spearate, in addition to it just being non-ambiguous in what they do. Of course this could be solved on a UI level with just Split too.

1 Like
13

No, it was just Staked, Stash, and Controller, see Add Beneficiary Account for Rewards · Issue #3544 · paritytech/substrate · GitHub (even included the notion of splitting).

Knowing what I know 4 years later, I indeed would not have done it that way. Just because this mistake was made in the past is not a good reason to do something wrong. Take the major change opportunity to do it correctly and not bring in mistakes from the past. Enum variants are meant to represent different things; making Split user friendly should be done on the UI level, not by misusing the data type in the protocol.

14

I fully disagree with this. The “payee” has no destination, it is the destination (i.e. the AccountId key where the reward will go).

The reward gets created in do_payout_stakers. The function needs to do something (verb) with it, or know where (reward destination) to deposit it.