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

Adds migration for ParachainStaking AtStake to support auto-compound #1878

Merged
merged 7 commits into from Oct 19, 2022

Conversation

nbaztec
Copy link
Contributor

@nbaztec nbaztec commented Oct 18, 2022

What does it do?

This PR adds the following migrations:

  • RemovePaidRoundsFromAtStake - to remove any stale AtStake entries relating to already paid-out rounds that had candidates that didn't produce any blocks.
  • MigrateAtStakeAutoCompound - migrates the snapshots for unpaid rounds for AtStake instead of the on-the-fly-migration via MigratedAtStake storage.

The fix for stale AtStake entries relating to already paid-out rounds that had candidates that didn't produce any blocks, is also included in this PR since it is a pre-requisite to avoid having to run RemovePaidRoundsFromAtStake over and over again.

What important points reviewers should know?

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?

@nbaztec nbaztec added B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Oct 18, 2022
@nbaztec nbaztec marked this pull request as ready for review October 18, 2022 15:48
@nbaztec nbaztec changed the title add migration for AtStake add migration for AtStake stale entries + autocompound, fix for removing candidates without blocks from rewarded rounds Oct 19, 2022
@librelois librelois mentioned this pull request Oct 19, 2022
24 tasks

// remove up to 1000 candidates that did not produce any blocks for
// the given round
let _ = <AtStake<T>>::clear_prefix(paid_for_round, 1000, None);
Copy link
Contributor

Choose a reason for hiding this comment

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

We could use the value from TopSelected but maybe we don't want an extra read. Alternatively, a constant for this would be good.

If we make our rounds longer, this problem should mostly go away (except for very poorly performing collators...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I struggled to find a constant for this value. But I don't get how increasing the round length would fix this issue, since we only read the points first, then the AtStake

Copy link
Contributor

@notlesh notlesh left a comment

Choose a reason for hiding this comment

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

LGTM. I worry about the PoV, but I also trust our testing :)

@crystalin crystalin 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 Oct 19, 2022
@nbaztec nbaztec merged commit e6fe3cb into master Oct 19, 2022
@nbaztec nbaztec deleted the nish-auto-compound-migrate branch October 19, 2022 15:17
@crystalin crystalin changed the title add migration for AtStake stale entries + autocompound, fix for removing candidates without blocks from rewarded rounds Adds migration for ParachainStaking AtStake to support auto-compound Oct 20, 2022
@notlesh notlesh added D1-audited👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D9-needsaudit👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Dec 14, 2022
imstar15 pushed a commit to OAK-Foundation/moonbeam that referenced this pull request May 16, 2023
…ing candidates without blocks from rewarded rounds (moonbeam-foundation#1878)

* add migration for AtStake

* add validations

* add migrations to remove the stale AtStake entries before autoCompound migrations

* better logs

* fix AtStake not cleaned up for candidates not producing blocks

* Allow fork test to start from the expected round index

Co-authored-by: Crystalin <alan@purestake.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants