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

allow use reserved balances for staking #7047

Closed
wants to merge 2 commits into from

Conversation

xlc
Copy link
Contributor

@xlc xlc commented Jan 6, 2025

Closes #6984

Allow reserved balances to be used for staking.

Previously there is an inconsistency that reserved balances cannot be used for staking, but can be transferred out if staked balances is greater than reserved amount. Effectively allow reserved balances to be used for staking.

@xlc
Copy link
Contributor Author

xlc commented Jan 6, 2025

/cmd prdoc

Copy link
Contributor

github-actions bot commented Jan 6, 2025

Sorry, only members of the organization paritytech members can run commands.

@acatangiu acatangiu added the T1-FRAME This PR/Issue is related to core FRAME, the framework. label Jan 10, 2025
@acatangiu acatangiu requested a review from a team January 10, 2025 09:29
Copy link
Contributor

@acatangiu acatangiu left a comment

Choose a reason for hiding this comment

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

PR looks good, but would like someone with more staking knowledge to also approve. cc @kianenigma

@kianenigma
Copy link
Contributor

/cmd prdoc

needs #7049

Copy link
Contributor

@acatangiu acatangiu 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 am not sure this is correct.

Looking at the documented meaning of reserved assets/balance is seems that it is used by deposits/holds?

The implementation for balances doesn't shed too much light on this, it just moves from free to reserved, but not clear to me what exactly should be the permitted operations.

Thinking high-level, if reserved is used for storage deposits, then we should NOT allow it to also be used for staking. The security deposit comes from the economic opportunity cost. Otherwise I could spam the chain for the full amount of storage deposits that I want to stake anyway with no economic cost.

@kianenigma
Copy link
Contributor

Another way to look at it is that reserved by other reasons, and this will mess with accounting in the pallet staking. This pallet always needs to make sure that there is free, fully slashable balance equal to ledger.total is available for slashing, and ergo uses free_balance as the maximum that you can stake, not total_balance.

That being said, the behavior that @xlc pointed out in the original issue is that you can effectively "trick" this intention, because locks and reserves don't overlap at all. You can do both the lock and reserve, then transfer everything that you can out, and this will effectively force the lock and reserve to overlap.

Once we move the staking pallet to #5501, this behavior still wants to be retained; staking will use a hold, and they (this time) PROPERLY won't overlap with reserves.

If you same the same test case to that branch, you can no longer transfer your funds out and trick the system.

This is also why as a part of the migration of staking to use holds, some accounts will force_withdraw small amounts of DOT in the process, to make sure reserves and holds are disjoint.

So while I understand the current system is messed up, I don't think this is a proper fix.

@xlc
Copy link
Contributor Author

xlc commented Jan 13, 2025

So I am able to transfer reserved amount out is the bug that will be fixed by #5501? I am fine with that but that also means once #5501 is in, there might be accounts with inconsistent state due to reserves and that needs to be handled somehow.

@kianenigma
Copy link
Contributor

So I am able to transfer reserved amount out is the bug that will be fixed by #5501? I am fine with that but that also means once #5501 is in, there might be accounts with inconsistent state due to reserves and that needs to be handled somehow.

Correct in both. And indeed if you see the description of the said PR there is some stats about how much DOT/KSM has to be force-unstaked to make accounting clean. Note that the DOT number is very high because of a single ledger with 20k DOT in a corrupt state (polkadot-fellows/runtimes#538). Minus that, the affected DOT/KSM is fairly small.

@kianenigma kianenigma mentioned this pull request Jan 13, 2025
@kianenigma
Copy link
Contributor

Closing as it is resolved in #7047 (comment), please re-open if need be.

@kianenigma kianenigma closed this Jan 13, 2025
@xlc xlc deleted the update-staking branch January 13, 2025 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T1-FRAME This PR/Issue is related to core FRAME, the framework.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to bond extra
4 participants