Skip to content
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.

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

Deferred staking rewards #1035

Merged
merged 49 commits into from
Dec 15, 2021
Merged

Deferred staking rewards #1035

merged 49 commits into from
Dec 15, 2021

Conversation

notlesh
Copy link
Contributor

@notlesh notlesh commented Nov 29, 2021

What does it do?

Changes staking payouts such that they occur in smaller chunks in blocks following a round rather than paying it all on the block where the round ends.

My current plan is to do as little as possible when the round ends. Because of the way the AtStake storage item works (that is, because it's a DoubleStorageMap and the primary key is the round index), this means we can avoid any per-delegator accounting at the end of the round. One disadvantage to this approach is that we lose the duplicated-delegation-is-still-only-one-payout optimization. On the other hand, it paves the way for easy manual payouts (if they are done per-collator and not per-delegator).

What important points reviewers should know?

Is there something left for follow-up PRs?

What alternative implementations were considered?

We could keep the per-collator accounting and the resulting optimization ("duplicated-delegation-is-still-only-one-payout"). However, this actually requires more storage writes at round-end, so it could end up being more work.

Are there relevant PRs or issues in other repositories (Substrate, Polkadot, Frontier, Cumulus)?

What value does it bring to the blockchain users?

Support for more delegators (and, as a result, lower minimum delegation requirements).

TODO:

  • Basic impl
  • Enforce constraints on in set_blocks_per_round() and set_total_selected to prevent a round from starting when the previous round has not been fully paid out
  • Add / modify events
  • Fix broken test cases (rust)
  • Write new test cases (rust)
  • Fix broken integration tests (ts)
  • Write new integration tests (ts)
  • Update benchmarks
  • Modify on_initialize() logic
  • Proper weight handling (may require changing how on_initialize() calculates weight)
  • Explore fix for predictable / unfair payout ordering (Elois created a Jira ticket to track this as future work)

@notlesh notlesh added the B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes label Nov 29, 2021
Copy link
Collaborator

@crystalin crystalin left a comment

Choose a reason for hiding this comment

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

It would be great if we can measure the PoV in the tests/benchmark (but maybe later)

*/

/*
* TODO: we lose this optimization entirely because we payout per-delegator
Copy link
Collaborator

Choose a reason for hiding this comment

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

That is right, it is one disadvantage

@4meta5 4meta5 added the I7-optimisation ⏱ An enhancement to provide better overall performance in terms of time-to-completion for a task. label Dec 1, 2021
@notlesh
Copy link
Contributor Author

notlesh commented Dec 1, 2021

One problem that this PR introduces is that the payouts for round N (AKA the paid-for round) occur in round N + RewardPaymentDelay + 1 (AKA the payout round). We want to constrain this relationship such that the number of blocks (BlocksPerRound) in the payout round is greater than the number of collators (TotalSelected) in the paid-for round. Because both of these can change via extrinsic and the constraint spans multiple blocks, it isn't sufficient to simply require BlocksPerRound > TotalSelected.

Note that these extrinsics require frame_system::ensure_root(origin), this is easily avoided.

@notlesh notlesh mentioned this pull request Dec 2, 2021
@notlesh
Copy link
Contributor Author

notlesh commented Dec 3, 2021

e7d92b5 mods this so that deferred payments begin in the same round of a round change, rather than waiting for the following block. I don't see any problem with this (our round changes are now really lightweight) and it prevents the tests from having fewer blocks-per-round than collators (in many cases, at least).

@crystalin crystalin added the A3-inprogress Pull request is in progress. No review needed at this stage. label Dec 7, 2021
@notlesh
Copy link
Contributor Author

notlesh commented Dec 13, 2021

I don't believe a migration is necessary:

  • When a new round begins, the requirements are the same in the old logic as the new logic. Namely:
    • <AtStake>, <Points>, and <AwardedPts> from RewardPaymentDelay rounds ago are available in storage
  • The new logic requires the addition of <DelayedPayouts> but will create this on round change from data that the previous logic ensures will exist (in other words, creating <DelayedPayouts> does not introduce any new requirements)
  • For purposes of understanding our runtime upgrade, the payout mechanism in the previous logic can be considered atomic; it's impossible to interrupt it or apply a runtime upgrade in the middle of it

Given this, I believe it is not possible for a runtime upgrade to introduce the new logic at any point where it would fail to make payments properly (miss payments, make extra payments, etc.)

(The following is my original, less cohesive approach to analyzing this; I'll leave it here in case it's helpful):

Here's my analysis of the previous and new logic with respect to storage and round changes.

Previous:

  • <Points> and <AwardedPts> are updated each block as collators create blocks. These are left in storage for RewardPaymentDelay rounds.
  • At round change:
    • These storage items from RewardPaymentDelay rounds ago are used to calculate payments.
    • A snapshot for <AtStake> for the upcoming round is taken
    • Payments are made for the entire round and the above storage items are removed for the paid-out-round.

New logic:

  • <Points> and <AwardedPts> are updated in the same way and are kept around for RewardPaymentDelay rounds, just as before.
  • At round change:
    • <DelayedPayouts> snapshot of issuance/commission is taken for the paid-out-round
    • <AtStake> snapshot taken for upcoming round, as before
    • <Points> and <AwardedPts> are not touched
  • Payouts made in each block following round change:
    • Read from and consume <AtStake> and <AwardedPts>
    • Require <Points> and <DelayedPayouts> to exist for the paid-out-round until payments are done

@librelois
Copy link
Collaborator

I agree with @notlesh that no migration is necessary.

Co-authored-by: Amar Singh <asinghchrony@protonmail.com>
Copy link
Contributor

@4meta5 4meta5 left a comment

Choose a reason for hiding this comment

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

Good job, thanks for addressing all review comments!

@notlesh notlesh merged commit 3fafb50 into master Dec 15, 2021
@notlesh notlesh deleted the notlesh-deferred-staking-rewards branch December 15, 2021 17:42
@notlesh notlesh added D1-audited👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Feb 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A0-pleasereview Pull request needs code review. A3-inprogress Pull request is in progress. No review needed at this stage. B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D1-audited👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited I7-optimisation ⏱ An enhancement to provide better overall performance in terms of time-to-completion for a task.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants