stake: Allow initialized stakes to be below the min delegation #24670

joncinque · 2022-04-25T23:56:47Z

Problem

The economics of stake pools will change as a result of a higher minimum delegation. First, some background on pools:

Stake pools consist of an initialized stake account for the reserve, and one delegated stake account per validator included.
A pool manager commits their funds to start up the pool, in the form of rent-exemption for accounts, and pre-funding the stake pool's reserve, which is an initialized stake account with rent-exemption + 1 lamport
A pool manager also commits their funds when adding a validator by creating a stake account with rent-exemption + 0.001 SOL. This amount can be recuperated when removing the validator.
Users can either deposit SOL directly into the reserve, or deposit a delegated stake by merging it into the corresponding stake account.
Users can either withdraw SOL from the reserve, or from one of the pool's delegated stake accounts via a split.
To calculate the value of pool tokens, the required minimums (0.001 SOL + rent exemption for delegated stakes, 1 lamport + rent exemption for the reserve) are not included, since the stake pool program does not allow accounts to go below these minimums. They are not considered usable, since users cannot withdraw them, and the manager cannot decrease stake past the minimum.

When the minimum stake delegation is raised to a larger number, the minimums when calculating the usable amount of SOL in the pool will change from 0.001 SOL to 1 SOL for delegated stakes, and from 1 lamport to 1 SOL for the reserve. In this way, existing stake pools essentially co-opt the new minimums from depositors. 1 SOL in each stake account (including the reserve) will be completely untouchable by both the pool manager and by depositors.

With a minimum delegation of 1 SOL, users with less than 1 SOL must withdraw from the reserve -- for stakes, they need to provide enough stake pool tokens to equal 1 SOL + rent-exemption.

Summary of Changes

It makes sense for delegated stake accounts to adhere to these rules, but initialized stake accounts can live apart, since they aren't present in the runtime's stakes cache.

Allow initialized stakes to be below the minimum delegation amount, while still forcing that all other stake types adhere to the minimum stake delegation. A lot of the changes were cribbed from #24603 (thanks @brooksprumo !)

With this, the whole reserve (minus rent exemption) will be usable for user withdrawals and manager delegations.

Also, for whenever stake v2 is created, with lower or no minimum delegation, their initialized state can still interoperate with initialized stake v1 accounts.

Feature Gate Issue: #24669

t-nelson

I'm confused as to why there are changes to initialize and split here. Both would allow new sub-limit delegations to be created. Aren't we just providing an escape hatch for existing sub-limit delegations here? That is, deactivate and withdraw.

joncinque · 2022-04-26T16:38:57Z

Deactivate and withdraw were required to not lock user funds in existing stake accounts, but the idea is to only require the minimum delegation amount for activating and active stakes, which will make usability much better for small stakes and stake pools.

t-nelson · 2022-04-26T22:23:31Z

Deactivate and withdraw were required to not lock user funds in existing stake accounts, but the idea is to only require the minimum delegation amount for activating and active stakes, which will make usability much better for small stakes and stake pools.

I believe this behavior (esp for split) will retain the accounts in the stakes cache and add work to the rewards calculation. Both of which are the motivation for instating a minimum stake limit.

cc/ @jstarry @behzadnouri

joncinque · 2022-04-26T22:48:47Z

The stakes cache doesn't do anything with initialized stakes since they don't have a delegation -- when trying to insert them in the stakes cache, it'll fail on deserializing:

solana/runtime/src/stake_account.rs

Line 80 in 412a5a0

return Err(Error::InvalidDelegation(stake_account.stake_state));

, which is the scope of this change.

Your point is well-taken for the future work on split + deactivate on small stakes, since those definitely need to exist in the cache to get their rewards for the deactivation epoch. For those, it might be time to finally add a new stake account type that doesn't get its rewards paid out at the epoch boundary, and add a new instruction to harvest it from the vote account! Unless it's acceptable for small deactivating stakes to exist in the cache.

brooksprumo

Looking good. I need to finish going through the test cases in stake_instruction.rs; here's a few thoughts/nits in the meantime.

programs/stake/src/stake_instruction.rs

brooksprumo

Ok, finished a first pass. Looks good to me. Comments below.

programs/stake/src/stake_instruction.rs

joncinque · 2022-04-28T17:49:23Z

Thanks for the review! This should be ready for another look

joncinque · 2022-04-28T19:44:27Z

I also had to update the frozen abi digest on BankSlotDelta, since it contains transaction::Result, and TransactionError wraps InstructionError, which has a new variant.

brooksprumo

Looks good to me! I think I'm also a little biased so it's probably valuable to have another person sign off on this as well.

codecov · 2022-04-28T22:09:41Z

Codecov Report

Merging #24670 (cc80628) into master (8ba003a) will increase coverage by 0.1%.
The diff coverage is 89.3%.

❗ Current head cc80628 differs from pull request most recent head da3efde. Consider uploading reports for the commit da3efde to get more accurate results

@@            Coverage Diff            @@
##           master   #24670     +/-   ##
=========================================
+ Coverage    81.8%    82.0%   +0.1%     
=========================================
  Files         632      598     -34     
  Lines      167499   165628   -1871     
  Branches      322        0    -322     
=========================================
- Hits       137169   135904   -1265     
+ Misses      30217    29724    -493     
+ Partials      113        0    -113

Pull request has been modified.

CriesofCarrots

The logic looks correct to me, just the one question about error type

CriesofCarrots · 2022-05-03T18:41:52Z

programs/stake/src/stake_state.rs

+    if feature_set.is_active(&stake_allow_zero_undelegated_amount::id())
+        && stake_amount < crate::get_minimum_delegation(feature_set)
+    {
+        return Err(InstructionError::InsufficientStakeDelegation);


Is there a reason this needs to be a new InstructionError variant instead of a new StakeError variant?
We did similar for ActiveVoteAccountClose, but that was because VoteError is really specific to Vote instructions, whereas StakeError covers a variety of Stake instructions.

Very good call, I'll make this a new StakeError

programs/stake/src/stake_instruction.rs

CriesofCarrots · 2022-05-03T19:10:11Z

programs/stake/src/stake_state.rs

 ) -> Result<ValidatedDelegatedInfo, InstructionError> {
    let stake_amount = account
        .get_lamports()
        .saturating_sub(meta.rent_exempt_reserve); // can't stake the rent
+    if feature_set.is_active(&stake_allow_zero_undelegated_amount::id())


Perhaps add a comment about the behavior change here?
Maybe something like this (but moar better): Previously, all StakeState::Initialize accounts were required to have a balance greater than the minimum delegation amount. After feature stake_allow_zero_undelegated_amount, StakeState::Initialize accounts may have a lower balance, so the minimum must be checked on delegation via this function.

Is this feature gate actually necessary? It's not currently possible to initialize a stake account which doesn't already hit the minimum delegation.

Never mind, I see it was possible to withdraw and then delegate again. Will make sure this behavior gets documented here: #24603

joncinque · 2022-05-03T22:49:31Z

This should be ready for another look, thanks for the careful eyes!

CriesofCarrots

lgtm!

…a-labs#24670) * stake: Allow initialized stakes to be below the min delegation * Add PR number in feature * Fixup RPC subscription test * Address feedback pt 1 * Address feedback pt 2 * Update FrozenAbi Digest * Address feedback: no new error type, more comments

joncinque added the feature-gate Pull Request adds or modifies a runtime feature gate label Apr 25, 2022

joncinque requested review from ryoqun and brooksprumo April 25, 2022 23:56

t-nelson reviewed Apr 26, 2022

View reviewed changes

joncinque force-pushed the stk-min branch from b23e671 to 4eadbcc Compare April 26, 2022 20:28

joncinque force-pushed the stk-min branch from 256a21e to 9ef43cd Compare April 27, 2022 19:57

brooksprumo reviewed Apr 28, 2022

View reviewed changes

programs/stake/src/stake_instruction.rs Outdated Show resolved Hide resolved

programs/stake/src/stake_instruction.rs Outdated Show resolved Hide resolved

programs/stake/src/stake_instruction.rs Show resolved Hide resolved

joncinque commented Apr 28, 2022

View reviewed changes

programs/stake/src/stake_instruction.rs Outdated Show resolved Hide resolved

brooksprumo reviewed Apr 28, 2022

View reviewed changes

programs/stake/src/stake_instruction.rs Outdated Show resolved Hide resolved

joncinque force-pushed the stk-min branch from 740e5b7 to c03658e Compare April 28, 2022 19:39

brooksprumo previously approved these changes Apr 28, 2022

View reviewed changes

joncinque force-pushed the stk-min branch from c03658e to cc80628 Compare April 28, 2022 23:02

joncinque mentioned this pull request Apr 29, 2022

stake-pool: Update program to work with minimum delegation solana-labs/solana-program-library#3127

Merged

CriesofCarrots reviewed May 3, 2022

View reviewed changes

joncinque added 7 commits May 4, 2022 00:45

stake: Allow initialized stakes to be below the min delegation

eaeeebd

Add PR number in feature

10c241b

Fixup RPC subscription test

c198121

Address feedback pt 1

07f3cc0

Address feedback pt 2

6a642c0

Update FrozenAbi Digest

66e8ec2

Address feedback: no new error type, more comments

da3efde

joncinque force-pushed the stk-min branch from cbd890c to da3efde Compare May 3, 2022 22:47

CriesofCarrots approved these changes May 4, 2022

View reviewed changes

joncinque merged commit 326e53b into solana-labs:master May 4, 2022

joncinque deleted the stk-min branch May 4, 2022 10:17

joncinque mentioned this pull request May 4, 2022

stake: Fix redelegation test for new behavior #24969

Closed

brooksprumo mentioned this pull request May 6, 2022

Raise stake minimum delegation to 1 SOL #24603

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stake: Allow initialized stakes to be below the min delegation #24670

stake: Allow initialized stakes to be below the min delegation #24670

joncinque commented Apr 25, 2022 •

edited

Loading

t-nelson left a comment

joncinque commented Apr 26, 2022

t-nelson commented Apr 26, 2022

joncinque commented Apr 26, 2022 •

edited

Loading

brooksprumo left a comment

brooksprumo left a comment

joncinque commented Apr 28, 2022

joncinque commented Apr 28, 2022

brooksprumo left a comment

codecov bot commented Apr 28, 2022 •

edited

Loading

CriesofCarrots left a comment

CriesofCarrots May 3, 2022 •

edited

Loading

joncinque May 3, 2022

CriesofCarrots May 3, 2022 •

edited

Loading

jstarry May 6, 2022

jstarry May 6, 2022

joncinque commented May 3, 2022

CriesofCarrots left a comment

stake: Allow initialized stakes to be below the min delegation #24670

stake: Allow initialized stakes to be below the min delegation #24670

Conversation

joncinque commented Apr 25, 2022 • edited Loading

Problem

Summary of Changes

t-nelson left a comment

Choose a reason for hiding this comment

joncinque commented Apr 26, 2022

t-nelson commented Apr 26, 2022

joncinque commented Apr 26, 2022 • edited Loading

brooksprumo left a comment

Choose a reason for hiding this comment

brooksprumo left a comment

Choose a reason for hiding this comment

joncinque commented Apr 28, 2022

joncinque commented Apr 28, 2022

brooksprumo left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 28, 2022 • edited Loading

Codecov Report

CriesofCarrots left a comment

Choose a reason for hiding this comment

CriesofCarrots May 3, 2022 • edited Loading

Choose a reason for hiding this comment

joncinque May 3, 2022

Choose a reason for hiding this comment

CriesofCarrots May 3, 2022 • edited Loading

Choose a reason for hiding this comment

jstarry May 6, 2022

Choose a reason for hiding this comment

jstarry May 6, 2022

Choose a reason for hiding this comment

joncinque commented May 3, 2022

CriesofCarrots left a comment

Choose a reason for hiding this comment

joncinque commented Apr 25, 2022 •

edited

Loading

joncinque commented Apr 26, 2022 •

edited

Loading

codecov bot commented Apr 28, 2022 •

edited

Loading

CriesofCarrots May 3, 2022 •

edited

Loading

CriesofCarrots May 3, 2022 •

edited

Loading