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

program: Splitting a deactivated stake wrongly takes delegation into account #1

Open
joncinque opened this issue Mar 8, 2024 · 1 comment
Assignees

Comments

@joncinque
Copy link
Contributor

This is a copy of solana-labs/solana#30400

Problem

If a stake is delegated, becomes active, and then gets deactivated, the delegation is always present in its StakeState. When splitting from any stake, if it's a StakeState, we check that the delegation doesn't go under 0 at https://github.com/solana-labs/solana/blob/fba990654b5d5a16f3b5f6dfd05ed4fd6129c332/sdk/program/src/stake/state.rs#L553-L555. If that deactivated stake has loads of liquid SOL, it will fail to split, which essentially means we can't correctly split a deactivated stake.

Proposed Solution

When splitting from a deactivated stake, don't take the delegation into account.

@2501babe
Copy link
Member

2501babe commented Mar 20, 2024

noting to my future self since im implementing split tests now, i believe there are three distinct issues here:

  • a stake account is created, delegated, and deactivated. split cannot bring the source account below the minimum delegation, even though its delegation is no longer meaningful
  • a stake account is created, delegated, and deactivated. split cannot create a destination account holding less than the minimum delegation, even though that delegation is meaningless
  • a stake account is created, delegated, deactivated, and transfered into. split cannot access the excess lamports beyond the nominal delegation

after comparing the code for split and withdraw i understand the reason for this situation: split historically did not use stake_history. originally it calculated only in lamports and dodged complexity via the fact that the destination account would have the same activation and deactivation epoch as the source account. then it was changed to enforce minimum delegations without awareness of true activation status. then it was changed to do some calculations with delegations to enforce other invariants. then it was changed to never pay rent-exemption for the destination out of an active stake, and it gets stake_history from the sysvar cache for that specific purpose

withdraw, on the other hand, has always accepted stake_history as an input account and was designed around it. so the first thing it does is use stake_history to figure out what the real delegation is

there is a low-impact fix here to skip some of the error checks related to delegations when !source_is_active. but i think a better (albeit scarier) fix would be to rewrite all the calculation logic starting from the fact that we have stake_history to give us the real delegation

the reason to do the scarier thing is that validate_split_amount and the block after it starting let (remaining_stake_delta, split_stake_amount) = is some of the most complicated calculation logic in the program and it could probably be simplified substantially, since we now have stake_history, and since it has accumulated new logic over the years that may have overlapping intents. as such im proposing finishing a bug-compat bpf stake program first, and then tackling this issue as if it were a new change to a completed program

(and i will be starting the fix by writing out a graphviz for the state machine before attempting to implement it)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants