Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

[do not merge] Decouple rewards from ledger #13023

Closed
wants to merge 13 commits into from

Conversation

Ank4n
Copy link
Contributor

@Ank4n Ank4n commented Dec 26, 2022

Related to paritytech/polkadot-sdk#439.

This PR changes the way we prevent double claim for validator rewards. We keep track of EraRewardPoints for each validator for current era, and in subsequent eras, when validator reward is claimed, their reward points are dropped from EraRewardPoints.

Since we store 84 eras of past reward claim history, in order to avoid doing a large migration (total nominator count * history_depth), we will do a lazy migration. After this change, no new data will be written to StakingLedger.legacy_claimed_rewards (renamed from claimed_rewards). Instead for any era going forward, we will just drop the EraRewardPoints for the validator as discussed above. When distributing rewards, we check StakingLedger.legacy_claimed_rewards to make sure if any past claim has been recorded there.

After 84 eras. we can be fully confident that StakingLedger.legacy_claimed_rewards has no relevant history that we need to preserve and we can do a new migration to remove it from ledger as well as clean up the code.

Tasks

@github-actions github-actions bot added the A3-in_progress Pull request is in progress. No review needed at this stage. label Dec 26, 2022
@Ank4n Ank4n changed the title First step to decouple reward from ledger Decouple rewards from ledger Dec 30, 2022
@Ank4n Ank4n marked this pull request as ready for review January 2, 2023 12:14
@Ank4n Ank4n requested a review from kianenigma as a code owner January 2, 2023 12:14
@github-actions github-actions bot added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Jan 2, 2023
@Ank4n Ank4n added B7-runtimenoteworthy C1-low PR touches the given topic and has a low impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Jan 2, 2023
@Ank4n
Copy link
Contributor Author

Ank4n commented Jan 2, 2023

/cmd queue -c bench-bot $ pallet dev pallet_staking

@command-bot
Copy link

command-bot bot commented Jan 2, 2023

@Ank4n https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2213163 was started for your command "$PIPELINE_SCRIPTS_DIR/bench-bot.sh" pallet dev pallet_staking. Check out https://gitlab.parity.io/parity/mirrors/substrate/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment /cmd cancel 151-d187a857-8c12-450e-9b24-f00660f31e81 to cancel this command or /cmd cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Jan 2, 2023

@Ank4n Command "$PIPELINE_SCRIPTS_DIR/bench-bot.sh" pallet dev pallet_staking has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2213163 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2213163/artifacts/download.

@Ank4n Ank4n requested review from gpestana and rossbulat January 2, 2023 14:42
@kianenigma kianenigma added D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited and removed D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Jan 6, 2023
@@ -273,6 +263,26 @@ impl<T: Config> Pallet<T> {
<Ledger<T>>::insert(controller, ledger);
}

fn get_era_reward_points(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
fn get_era_reward_points(
fn temp_get_era_reward_points(

Copy link
Contributor

Choose a reason for hiding this comment

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

since this can be removed later, right?

Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

Looks mostly good, but I want to give it an in-depth review later. Also want to look into the tests again. Would be good if you revise the tests as well, as the existing ones might be mediocre at best.

@Ank4n
Copy link
Contributor Author

Ank4n commented Jan 9, 2023

While this PR will solve the existing issue of moving rewards away from StakingLedger, it will be incompatible with multi-block rewards payout PR that I am working on. I am inclined to do these changes as part of the other PR to make sure it works well with multi-block rewards.

I will wait for comments/opinions but will probably close it in couple of days.

@Ank4n Ank4n changed the title Decouple rewards from ledger [do not merge] Decouple rewards from ledger Jan 9, 2023
@gpestana gpestana self-requested a review January 9, 2023 13:35
@Ank4n Ank4n closed this Jan 11, 2023
@louismerlin louismerlin removed the D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited label May 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

New staking reward system
4 participants