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

patch stake nominations bug #240

Merged
merged 7 commits into from
Feb 12, 2021
Merged

patch stake nominations bug #240

merged 7 commits into from
Feb 12, 2021

Conversation

4meta5
Copy link
Contributor

@4meta5 4meta5 commented Feb 11, 2021

What does it do?

CC @artkaseman Before, we were NOT using the snapshot of validator state at the time of validator selection for payout distribution. Instead, we were using the CandidateState at the time of the delayed payment to determine payout distribution. This means nominators that left before the payout distribution were not paid and, more dangerously, new nominators that joined during the delay were paid instead even though they weren't nominators at the time of the validator's block author reward.

Note that this PR also removes pallet-staking as a dependency. We may add this back if we choose to use the Exposure type when we implement slashing, but we don't need it now. I replaced it with ValidatorSnapshot which provides a snapshot of the state of the validator at the time they are selected for the round.

What important points reviewers should know?

Context

  • validators are rewarded by a point system according to how many blocks they author in a round
  • the issuance for the round is distributed among the validators based on relative point scores
  • within each validator, the due issuance is distributed to validators first (commission + due_proportion) and then to nominators second (due_proportion)
    • due_proportion is actors_stake/total_validator_stake * total_due_issuance_for_the_validator

The bug used the current CandidateState to calculate distribution of rewards earned 2 rounds prior. This means nominators that left before the payout distribution were not paid and, more dangerously, new nominators that joined during the delay were paid instead even though they weren't nominators at the time of the validator's block author reward.

The fix snapshots CandidateState at the time the account is chosen for the round so it can weight the earnings distribution based on this snapshot instead of on the current CandidateState (which could easily have changed between the time the block author was recognized and the payout was executed).

Is there something left for follow-up PRs?

What alternative implementations were considered?

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

What value does it bring to the blockchain users?

Checklist

  • Does it require a purge of the network? No
  • You bumped the runtime version if there are breaking changes in the runtime ?
  • Does it require changes in documentation/tutorials ? No

@4meta5 4meta5 marked this pull request as ready for review February 11, 2021 17:24
@4meta5 4meta5 added A0-pleasereview Pull request needs code review. B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes labels Feb 11, 2021
runtime/src/lib.rs Outdated Show resolved Hide resolved
@JoshOrndorff JoshOrndorff added A8-mergeoncegreen Pull request is reviewed well. C1-low Does not elevate a release containing this beyond "low priority". labels Feb 12, 2021
Copy link
Contributor

@JoshOrndorff JoshOrndorff left a comment

Choose a reason for hiding this comment

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

Looks good to me. I like the added test coverage and the simpler ValidatorSnapshot type.

@4meta5 4meta5 merged commit 1490dbf into master Feb 12, 2021
@4meta5 4meta5 deleted the amar-patch-stake branch February 12, 2021 15:56
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. A8-mergeoncegreen Pull request is reviewed well. B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes C1-low Does not elevate a release containing this beyond "low priority".
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants