-
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: Support stake redelegation #3856
Conversation
@2501babe - have you had a chance to learn about the stake pool program yet? @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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
@mvines yep, I've involved Marinade so far, and will involve Socean too. Otherwise, this will also get audited |
@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 :) |
da0fd71
to
f935c6d
Compare
going to finish reviewing this on monday, i stumbled on |
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 |
It's a confusing side-effect of the redelegate implementation, which takes advantage of the duplication of info between the 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. |
1059074
to
4d81404
Compare
Merging to provide an easier patch for audits. Feel free to provide post-merge comments! |
* 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
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:
Fixes #3749