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

Removes incorrect try-state check in staking #14186

Merged
merged 2 commits into from
Jul 24, 2023

Conversation

gpestana
Copy link
Contributor

@gpestana gpestana commented May 22, 2023

The current logic does not completely prevent the ledger active balance to remain above ED and there are code/config paths in which ledger.active may be 0. This PR removes the try-state check that is testing those invariants.

Closes #14246

@gpestana gpestana added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit T1-runtime This PR/Issue is related to the topic “runtime”. labels May 22, 2023
@gpestana gpestana requested a review from a team May 22, 2023 08:27
@gpestana gpestana self-assigned this May 22, 2023
@gpestana gpestana requested review from kianenigma and Ank4n May 22, 2023 08:27
@kianenigma
Copy link
Contributor

Has an issue been made about minStake/minNominatorStake/minValidator stake? if we had minStake, then it would have been reasonable to at least raise a warning if ledger.active is less than minStake, although even this is not great as minStake can reduce.

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.

Actually, I found the reason why we had this test: this code should prevent ledger.active from ever getting less than

if ledger.active < T::Currency::minimum_balance() {
.

Although, slashing could still violate this.

I think it is still better to first check a few of the stashes that cause this, and see what path they went through.

@gpestana
Copy link
Contributor Author

gpestana commented May 27, 2023

Actually, I found the reason why we had this test: this code should prevent ledger.active from ever getting less than (ED)

Although, slashing could still violate this.

I think that's correct. However, if a stash has been slashed to the point of the active/total balance being below ED, someone needs to call reap for the stash to be killed and removed. Otherwise, the entry with ledger.total and ledger.active will linger in the ledger.

I think it is still better to first check a few of the stashes that cause this, and see what path they went through.

As mentioned in #14246, I see two paths for ledger.active to be below ED:

  1. First chill stash and not call withdraw_unbonded after the bonding period;
  2. Slashing occurs that drops ledger.total below ED and no reap is called.

From my current understanding, the try-state consistence check that this PR is removing is not correct and should be removed.

We may want to think about a way to call withdraw_unbonded permissionlessly after a minimum of eras after the unbonding period has passed, so that we can free up space in storage (more on this in the issue above).

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.

Fair enough.

All in all, I still find this situation with our assumptions around ledger a bit confusing. I hope you take a fundamental look at it and add new reasonable checks, if any.

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.

bot merge

@kianenigma
Copy link
Contributor

bot rebase

@paritytech-processbot
Copy link

Rebased

@paritytech-processbot paritytech-processbot bot requested a review from a team July 10, 2023 14:07
@gpestana
Copy link
Contributor Author

bot merge

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. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit T1-runtime This PR/Issue is related to the topic “runtime”.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Staking] Active ledger balance may fall below ED if account chills before unbounding
3 participants