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

stake-pool: Support stake redelegation #3856

Merged
merged 7 commits into from
Dec 24, 2022

Conversation

joncinque
Copy link
Contributor

Problem

solana-labs/solana#26294 added the ability to redelegate stake in one go, without waiting for cooldowns and warmups, allowing delegators to move stake much more easily.

While normal delegators will certainly make good use of it, stake pools will truly benefit from it, allowing managers to maintain high performance on their pool.

Solution

Add the ability to redelegate on stake pools!

The implementation aims to keep this simple, reusing the transient stake accounts used for normally rebalancing pool. The instruction processor roughly does:

  • split from source account into transient
  • redelegate from transient account into ephemeral account -- this account is only used for the duration of the transaction, and uses a PDA to avoid the additional signature cost on a normal ephemeral account. To avoid griefing possibilities, you can pass in any u64 seed for it.
  • if the destination validator has a transient account, try to merge the ephemeral account into it -- this means you can redelegate into one validator multiple times per epoch, which is pretty neat. This encourages easy movement into good validators.
  • if the destination validator has no transient account, split the entirety of the ephemeral account into the new transient account

Fixes #3749

@joncinque joncinque requested a review from mvines November 30, 2022 00:48
@mvines mvines requested a review from 2501babe November 30, 2022 17:50
@mvines
Copy link
Member

mvines commented Nov 30, 2022

@2501babe - have you had a chance to learn about the stake pool program yet?
If no, this could be a nice place to start asking question and learning.
If yes, please help review to get more eyes on this.

@joncinque - do you have a line to any ecosystem teams using the SPL stake-pool program? if possible it'd be nice to involve engineers from those teams for review here too

ephemeral_stake_seed: u64,
/// Seed used to create destination transient stake account. If there is
/// already transient stake, this must match the current seed, otherwise
/// it can be anything
Copy link
Member

Choose a reason for hiding this comment

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

Should we recommend the client generate a random u64 value?
otherwise it can be anything will cause every good lazy programmer to hard code 0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They can be reused, similar to transient stake accounts used for increase / decrease. 0 is fine for the most part, but the stake bots just increment every time to be safe.

@joncinque
Copy link
Contributor Author

do you have a line to any ecosystem teams using the SPL stake-pool program? if possible it'd be nice to involve engineers from those teams for review here too

@mvines yep, I've involved Marinade so far, and will involve Socean too. Otherwise, this will also get audited

@CMEONE
Copy link
Contributor

CMEONE commented Nov 30, 2022

do you have a line to any ecosystem teams using the SPL stake-pool program? if possible it'd be nice to involve engineers from those teams for review here too

@mvines Engineers from BlazeStake and Jito will also review the code!

@AlexanderRay does JPool want to take a look at this PR as well? I'm happy to work together again with you on adding and testing more JS bindings as well :)

@joncinque joncinque force-pushed the sp-redelegate branch 3 times, most recently from da0fd71 to f935c6d Compare December 6, 2022 01:24
@2501babe
Copy link
Member

going to finish reviewing this on monday, i stumbled on stake_allow_zero_undelegated_amount when looking at the stake program code and went down a rabbithole because of its possible relevance to the single-pool

@2501babe
Copy link
Member

i cant find anything substantive to criticize. also i made a point of tracing where all the account infos are validated and that all looks solid to me

also, for my own enlightenment... i was initially confused why we didnt reclaim the rent from the source transient account at the end of process_redelegate, and used one of the tests to confirm it actually didnt work (because redelegate marks the source as deactivated), and tracked down the StakeError::MergeTransientStake. this is because the source transient account still retains the effective stake until the next epoch, at which point it moves to the destination transient account, right? at which point, UpdateValidatorListBalance will merge the destination transient acocunt into the destination main account, and sweep the source transient account into the reserve

@joncinque
Copy link
Contributor Author

It's a confusing side-effect of the redelegate implementation, which takes advantage of the duplication of info between the Delegation struct on the stake account and its lamports. All of the lamports (minus rent) are moved from the source to the destination, but the source is still delegated and deactivating, and retains the delegated amount info. This way, the source gets properly credited with epoch rewards.

The destination also gets the effective stake amount, so the stake is actually double counted, but the lamports are only in one place. I spent a long time making sure that it wasn't exploitable, and I could only find weird corner cases. If you want to spend some time looking at the redelegate implementation, more eyes are always welcome!

And yeah, as you noticed, when the source transient stake deactivates, then the reserve can consume its rent and rewards.

@joncinque
Copy link
Contributor Author

Merging to provide an easier patch for audits. Feel free to provide post-merge comments!

@joncinque joncinque merged commit eba709b into solana-labs:master Dec 24, 2022
@joncinque joncinque deleted the sp-redelegate branch December 24, 2022 14:45
HaoranYi pushed a commit to HaoranYi/solana-program-library that referenced this pull request Jul 19, 2023
* stake-pool: Add redelegate implementation

* Remove rent account from instruction

* Update validator searching due to rebase

* Use new blockhash in test to avoid timeout in CI

* Clarify error message

* Fix instruction comment

* Refresh blockhash in failing test more often
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

Successfully merging this pull request may close these issues.

stake-pool: Integrate stake redelegation
4 participants