Add StakeInstruction::Redelegate

[mvines]

The stake deactivation penalty makes active stake too sticky and causes stake pools to perform poorly.
#24762 proposes a new stake account format to deal with this problem, but that's a big change. What if we could get most of the way there with something more incremental in the v1.11 timeframe.

Enter the StakeInstruction::Redelegate instruction. It works by initializing a new stake account to allow for the re-delegated stake to warm up in parallel to the stake cooling down "virtually" in the original stake account. At the end of the warm-up/cool-down, the original stake account holds rent exempt lamports that can be withdrawn.

This approach does cause the redelegated stake to be double counted towards the per-epoch max warm-up/cool-down rate of 25%. One implication here is that re-delegating more than 12.5% of the active stake would trigger multi-epoch warm-ups/cool-downs instead of the usual 25% for normal activations+deactivations. This doesn't fundamentally seem like it would be a problem though.

TODO:

• Add cli support (solana redelegate-stake <existing_stake_account> <new_vote_account> <new_stake_account>)
• Tests
• Land prerequisite PR: rpc: getStakeActivation now correctly returns the inactivate lamports for deactivat{ed,ing} stake account in all cases #26595

[mvines]

Preliminary cli support added, piggybacking on the existing delegate-stake command, and redelegation appears to work well with a solana-test-validator.

The cli feels clumsy though and a more explicit redelegate-stake command will be better, even though this means a little bit more copy pasta in the cli implementation

[joncinque]

Just a really quick pass on the program logic. I like the overall approach, but I'm worried about multi-epoch movements and slashing if it's eventually implemented. I'll think about other ways we can make this work.

[mvines]

Just a really quick pass on the program logic. I like the overall approach, but I'm worried about multi-epoch movements and slashing if it's eventually implemented. I'll think about other ways we can make this work.

Thanks for taking a look.

Slashing I think also should be ok. That is, assuming that a slashing regime is never implemented that puts the staker's principal at risk (which I feel would be ridiculous). Even after a redelegation, the original and now deactivating stake account is still eligible for epoch rewards until it fully deactivates. One may imagine a particularly brutal regime that slashes epoch rewards for validator transgressions committed in that epoch, and the approach in this PR wouldn't inhibit such tyranny.

[codecov]

Codecov Report

Merging #26294 (5ef8488) into master (1165a7f) will increase coverage by 0.0%.
The diff coverage is 68.0%.

❗ Current head 5ef8488 differs from pull request most recent head 85587b9. Consider uploading reports for the commit 85587b9 to get more accurate results

@@            Coverage Diff            @@
##           master   #26294     +/-   ##
=========================================
  Coverage    81.9%    81.9%             
=========================================
  Files         631      632      +1     
  Lines      174252   175287   +1035     
=========================================
+ Hits       142728   143707    +979     
- Misses      31524    31580     +56     

[joncinque]

That is, assuming that a slashing regime is never implemented that puts the staker's principal at risk (which I feel would be ridiculous).

It's a really interesting question, since Ethereum actually takes the stake due to bad behavior. I came across this really interesting article regarding Hedera, which may be a closer analogue to how Solana works, with on-chain votes and lockouts: https://hedera.com/blog/why-is-there-no-slashing-in-hederas-proof-of-stake

In that case, it may be sufficient to destake or zero out rewards as the punishment. I haven't studied the question enough to be sure, and we should probably get an idea from @carllin to make sure this lines up with his design for slashing.

[mvines]

In that case, it may be sufficient to destake or zero out rewards as the punishment.

A forced destaking of any public validator is basically an extinction-level event for them. For nodes running on self-stake only, sure they could restake themselves but losing a couple epochs-worth of rewards is also pretty painful if they continue to violate whatever slashing rules the future may bring.

This kind of punishment would be very suitable for nodes that are violating voting lockouts IMO, and I can't think if anything worse that a node could currently perform against the network. Other lesser misbehaviors, like transmitting duplicate blocks, can simply be handled like transaction spam or other malformed data - drop it as soon as possible and move on

[joncinque]

In general, transferring lamports into the source account will make some weird possibilities, since those lamports effectively become deactivating immediately, but nothing that can be exploited (yet).

Going through the instructions, making sure that the source account can't be messed with:

• Merge is all good since deactivating stakes cannot be merged
• Split always checks the lamports, so it's safe. There could be some weirdness if you transfer lamports into the account and then split, but it won't ever be inconsistent.
• Withdraw won't work properly if you transfer lamports into the source account, since it checks that you aren't withdrawing below rent-exemption + delegation.

With this pass done, I'll start on the tooling / runtime. Looking good!

[joncinque]

Really just some small things. I took a look through the runtime as well, and everywhere it's correctly using the delegation.stake(...) function to figure out the effective stake, and never just using the full delegation number.

The deactivated stake through RPC is the only potentially sketchy bit, and your test showed that thankfully.

Once that's figured out, I think this is ready for a larger group!

[brooksprumo]

Are there tests somewhere that check that the total delegated stake is correct/expected (not just per account)? Same question w.r.t. warm-up/cool-down.

Edit to add context:

The total stake delegation is checked/known to do various things (like set leader schedule). I assume there are tests that perform some delegations/deactivations and ensure the total stake amount is correct. I would think that adding in some calls to redelegate would be valuable.

[joncinque]

@joncinque Do stake pools rely on activating/deactivating numbers for anything? Either logic or analytics/metrics?

Stake pools can't use redelegated stakes yet, so no immediate impact. Your point is well taken though -- we will need to start using account.lamports instead of stake.delegation in a couple of places.

[mvines]

@joncinque Do stake pools rely on activating/deactivating numbers for anything? Either logic or analytics/metrics?

Stake pools can't use redelegated stakes yet, so no immediate impact. Your point is well taken though -- we will need to start using account.lamports instead of stake.delegation in a couple of places.

Are there any cases you see where the current use of stake.delegation has an adverse impact if this PR goes live?

[joncinque]

Again, no immediate impact, since stake pools won't redelegate right away. When they do, however, we may need to update these two places:

https://github.com/solana-labs/solana-program-library/blob/76ad511ed0dbebcb3778e2296faa54fa10fd98f8/stake-pool/program/src/processor.rs#L1616-L1618

https://github.com/solana-labs/solana-program-library/blob/76ad511ed0dbebcb3778e2296faa54fa10fd98f8/stake-pool/program/src/processor.rs#L1644-L1646

But these might even be ok -- I'm imagining that every stake pool will continue to have the one transient account, and now also have a redelegation stake account. And we'll just have to be careful to not use delegation.stake when accounting for those.

[mvines]

Are there tests somewhere that check that the total delegated stake is correct/expected (not just per account)? Same question w.r.t. warm-up/cool-down.

I looked around the current tests and my memories and both came up empty. There are various unit tests for different parts of the stake management but I don't see system integration-level tests like what you mention. local-cluster would be the natural dumping ground for such a thing, and I didn't see anything relevant there in particular.

I do agree that if such tests existed then adding redelegate to them would be nice. 😬

[brooksprumo]

Are there tests somewhere that check that the total delegated stake is correct/expected (not just per account)? Same question w.r.t. warm-up/cool-down.

I looked around the current tests and my memories and both came up empty. There are various unit tests for different parts of the stake management but I don't see system integration-level tests like what you mention. local-cluster would be the natural dumping ground for such a thing, and I didn't see anything relevant there in particular.

I do agree that if such tests existed then adding redelegate to them would be nice. 😬

Gotcha. Think this is something valuable to have? If yes, does creating a new issue to track it seem good?

[brooksprumo]

Looks good to me! Probably worthwhile for @t-nelson to also still review before merging too.

[brooksprumo]

minor: I wish we had some additional type safety around these indices, if only to prevent accidental param mixups. Not something that needs to be addressed in this PR; rather a preference I have and wanted to see what others thought.

[mvines]

I agree, I don't like this style myself but didn't want to diverge from what's already customary in this file.

Pull request has been modified.

[CriesofCarrots]

lgtm!

Pull request has been modified.

[Cerbah]

Hello @mvines and @joncinque

In this other PR it's mentioned that:

"As a side note, you could use the "redelegate" command more than once per epoch, but the only one that matters for rewards accrual is the chosen validator at the end of the epoch."

Is this already implemented in this PR? Can a stakepool use the redelegate command more than once per epoch if there is a need?

[joncinque]

@Cerbah there's a few things being conflated here. Nothing in the proposal PR you linked has been implemented, it was some ideas put in an issue.

The stake program and the stake pool program are separate. This implementation is for redelegation in the stake program. The stake pool program is not using this yet. After redelegate, yes, you can choose to delegate the new stake to another validator before activation.

You can read more about the instruction and implementation in the PR: https://github.com/solana-labs/solana/pull/26294/files#diff-08e1ae1c3e3ac70c2e12aaeaae612655ff7d4422c2b15e2ac22a0b854cff8561R271-R291

[Cerbah]

@joncinque thank you for this answer, sorry for the confusing question.

The idea is that Marinade wants to make use of this update to the stake program, and does not run on the stake pool program. We'll check the implementation details to finalize our design, thanks a lot.