-
Notifications
You must be signed in to change notification settings - Fork 2k
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
stake-pool: Add / remove validators from the reserve #3714
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
762f6a5
to
7c853f4
Compare
i spent most of today reviewing existing staking and stake pool documentation, since i didnt have any familiarity with that already. kinda information overloaded now but should be ready to do this next week |
d3771d8
to
7f08946
Compare
HaoranYi
pushed a commit
to HaoranYi/solana-program-library
that referenced
this pull request
Jul 19, 2023
* stake-pool: Create validator stake accounts from reserve * Update accounting to take minimum into account * Refactor withdraw tests * Add ability to remove validators during withdraw stake * Add seed to validator stake account address derivation * Update CLI with new instruction formats * Update documentation and helper scripts for new funding * Update Python bindings * Try to fix flakey test * Support token-2022 * Condense tests for CI * Reduce huge pool limit post-rebase * Avoid draining the whole reserve * Restrict account extensions on allow-list * Update comments to say "lamports" * Fixup code comments and variable names * Fix clippy in tests
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
Currently, stake pool stakers add validators to the pool using a minimum delegation of ~0.003 SOL from their funds, rather than using the reserve. This was an easy decision at the outset, before there was even a reserve. The staker can recover those funds by removing validators from the pool.
When the minimum delegation jumps from 1 lamport to 1 SOL, the stake pool program must respect that minimum in order to keep stakes usable. This means that practically speaking, the pool is stealing 0.999 SOL from the depositors for each validator added to the pool. That stinks.
Solution
The solution is very simple on the outset, but involves some pretty big changes in the accounting of stake pools and states of the system. The overall gist is to add and remove validators from the pool reserve rather than some external funder. To make this work, we need the following:
This is all well and good. But it opens up a new attack -- a malicious manager can lock depositor funds by creating loads of new stake accounts from the reserve, since they can't withdraw below the minimum delegation. Which means we have to:
This opens up another attack vector! Validator stake accounts exist on a specific PDA which uses the validator vote account and the stake pool address as seeds. When a stake account is deleted, it actually sticks around for the duration of the transaction, and anyone can resurrect it if they want. Since now anyone can remove a stake pool's accounts under certain circumstances, a malicious user can resurrect the deleted stake account and make it impossible for that pool to ever re-add that validator. That means we have to:
u64
into twou32
s. This is safe to do and doesn't break current pools, only the instruction is breaking.After that, make sure this change is reflected everywhere:
Since the 1 SOL minimum delegation breaks stake pools, the choice here is to make some breaking changes for the benefit of depositors.
Note that this change will also be audited since it touches some sensitive code and could open up other unknown attack vectors. For example, it's still possible for a malicious pool owner to keep user funds in limbo by constantly rebalancing the minimum amount between validators.
Note 2: the commit history is intentional, and can be reviewed piece by piece. I had to go backwards for two little things that didn't work as expected.
Bonus: added token-2022 support!