-
Notifications
You must be signed in to change notification settings - Fork 336
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
Deferred staking rewards #1035
Conversation
There was a problem hiding this 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)
pallets/parachain-staking/src/lib.rs
Outdated
*/ | ||
|
||
/* | ||
* TODO: we lose this optimization entirely because we payout per-delegator |
There was a problem hiding this comment.
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
One problem that this PR introduces is that the payouts for round Note that these extrinsics require |
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). |
I don't believe a migration is necessary:
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:
New logic:
|
I agree with @notlesh that no migration is necessary. |
Co-authored-by: Amar Singh <asinghchrony@protonmail.com>
There was a problem hiding this 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!
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 aDoubleStorageMap
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:
set_blocks_per_round()
andset_total_selected
to prevent a round from starting when the previous round has not been fully paid outon_initialize()
logicon_initialize()
calculates weight)