Refactor and use MINIMUM_STAKE_DELEGATION constant #22663

brooksprumo · 2022-01-22T01:29:23Z

Problem

The minimum stake delegation constant is not used outside of tests.

Summary of Changes

Use the constant in initialize(), delegate(), split(), and withdraw()
Refactor code to be safe and obvious

Notes

No behavior is changed in this PR
This PR builds on the tests added in Add MINIMUM_STAKE_DELEGATION constant #23259 and Add test to check destination for stake splitting #23303 to ensure no behavior is changed
Subsequent tasks for minimum stake delegation are being tracked in Raise stake minimum delegation #22559
I recommend ignoring whitespace when looking at the diff here on github

codecov · 2022-01-22T04:01:46Z

Codecov Report

Merging #22663 (7683a5e) into master (eff085f) will increase coverage by 0.0%.
The diff coverage is 88.7%.

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

@@           Coverage Diff            @@
##           master   #22663    +/-   ##
========================================
  Coverage    81.3%    81.4%            
========================================
  Files         572      573     +1     
  Lines      155876   156739   +863     
========================================
+ Hits       126815   127638   +823     
- Misses      29061    29101    +40

brooksprumo · 2022-01-22T04:05:31Z

... or should I name this minimum stake delegation?

(Or minimum stakable balance?)

jstarry · 2022-01-22T04:32:19Z

We want to capture a "minimum stake delegation" and that constant needs to be reflected when delegating stake and splitting stake.

brooksprumo · 2022-01-22T14:40:15Z

@jstarry Constant has been renamed to MINIMUM_STAKE_DELEGATION.

jstarry · 2022-01-22T15:16:50Z

@jstarry Constant has been renamed to MINIMUM_STAKE_DELEGATION.

Cool but I don't think it makes sense to use this constant the way you're using it for withdraw. Why not use it for delegations and splitting stake like I suggested?

brooksprumo · 2022-01-22T18:56:15Z

Cool but I don't think it makes sense to use this constant the way you're using it for withdraw. Why not use it for delegations and splitting stake like I suggested?

Ok, will do. I was trying to keep this PR as small as possible, then making further changes in subsequent PRs. I'll pull those in here.

brooksprumo · 2022-01-31T20:46:31Z

@joncinque @jstarry This PR is now ready for a re-review!

runtime/src/bank.rs

joncinque

Great work! This is really close, just a few small comments,

programs/stake/src/stake_state.rs

joncinque

looks good to me! perhaps @CriesofCarrots or @ryoqun could also take a look since this code is pretty crucial, and I believe they did the initial work around here

Pull request has been modified.

CriesofCarrots

I'm requesting a couple comments, but the logic looks correct to my eye. I do think we should get @ryoqun 's eyes+approval also 🙏

programs/stake/src/stake_state.rs

Pull request has been modified.

t-nelson · 2022-02-04T04:45:26Z

Can you update the title and description? There are clearly logical changes here and I don't want to assess them without understanding the motivation

brooksprumo · 2022-02-04T14:46:28Z

@t-nelson

Can you update the title and description? There are clearly logical changes here and I don't want to assess them without understanding the motivation

Ok, I've updated the title and description.

In theory there actually are no changes to the current behavior. This PR really is future-proofing and future-prepping changes to the minimum stake delegation. I added this bit to the description:

Additionally, minimum delegation is not checked beyond initialize(), so if the minimum is ever raised there may be inconsistent/unexpected behavior with pre-existing accounts that have a delegation smaller than the new minimum.

Does that feel sufficient? If not I'll happily give it another go to explain it more/more clearly.

brooksprumo · 2022-02-07T15:39:27Z

@ryoqun Would you be able to take a look at this PR too? Thanks in advance!

joncinque

Looks good to me, just one comment which might be important for enforcing MINIMUM_STAKE_DELEGATION on active source stakes

programs/stake/src/stake_state.rs

brooksprumo · 2022-03-03T18:24:45Z

@ryoqun @joncinque Tests have been added and merged, and this PR rebased. Good for another round of reviews. Thanks in advance!

joncinque

I'm really sorry that this keeps getting dragged out, but it's really really close!

joncinque · 2022-03-03T21:50:33Z

programs/stake/src/stake_state.rs

+        destination_rent_exempt_reserve.saturating_add(MINIMUM_STAKE_DELEGATION);
+    let destination_balance_deficit =
+        destination_minimum_balance.saturating_sub(destination_account.lamports()?);
+    let minimum_split_amount = std::cmp::max(MINIMUM_STAKE_DELEGATION, destination_balance_deficit);


Sorry, I don't think I made the contra to this part clear. There could be a different strange behavior with this, where anytime I split an Initialized stake into an account that already has destination_minimum_balance, I can't split anything less than MINIMUM_STAKE_DELEGATION, which is fine for now, but once the number is big, it means I'll have to do a withdraw + transfer, which is probably fine, but something to keep in mind.

The best of all behaviors would be to take the max only if source_account is a Stake, which can be expressed through an additional is_stake flag or something. Does that make sense?

Ah, OK. I glossed over that in your other comment, sorry! I've just pushed another commit that hopefully fixes this.

I put the changes inside the match within split(), since I wanted to try and keep validate_split_amount() to be state-agnostic. I also did not add a check if splitting 100%, since I'm making the assumption that the source account meets the minimum requirements.

(But note, what happens if the MINIMUM_STAKE_DELEGATION is raised and there's an existing stake account that tries to split 100%? Should that succeed? Should that fail? I'm not sure. Luckily that doesn't need to be answered immediately for this PR.)

(But note, then what happens if the MINIMUM_STAKE_DELEGATION is raised and there's an existing stake account that tries to split 100%. Should that succeed? Should that fail? I'm not sure. Luckily that doesn't need to be answered immediately for this PR.)

Hehe indeed not for this PR, but my thinking was that only merge / deactivate / withdraw would be supported for existing stakes underneath MINIMUM_STAKE_DELEGATION. You could even talk me out of "merge"

joncinque

Looks good to me, thanks for your patience!

brooksprumo · 2022-03-04T04:30:21Z

@ryoqun I will wait for your review before merging, unless you say otherwise. (No rush, just wanted to make sure I was explicit!)

programs/stake/src/stake_state.rs

ryoqun · 2022-03-04T15:10:40Z

(sorry for dragging this pr so long for extended review cycles from me...)

Pull request has been modified.

ryoqun · 2022-03-05T03:08:44Z

programs/stake/src/stake_state.rs

+    // If the source account is already staked, the destination will end up staked as well.  Verify
+    // the destination account's delegation amount is at least MINIMUM_STAKE_DELEGATION.
+    //
+    // The *delegation* requirements are different than the *balance* requirements.  If the
+    // destination account is prefunded with a balance of `rent exempt reserve + minimum stake
+    // delegation - 1`, the minimum split amount to satisfy the *balance* requirements is 1
+    // lamport.  And since *only* the split amount is immediately staked in the destination
+    // account, the split amount must be at least the minimum stake delegation.  So if the minimum
+    // stake delegation was 10 lamports, then a split amount of 1 lamport would not meet the
+    // *delegation* requirements.


i love this crisp and lucid explanation. :)

ryoqun

@brooksprumo lgtm
thanks for your patience with my perfectionism and stubbornness. i think this pr is in flawless state now. excellent work.

by the way, what do you think about shipping this? I'd suggest 2 more things to solidify our confidence of those changes:

run locally built solana-validator for several epochs against mainnet-beta/testnet/devnet (should see no bank hash mismatch then)
request neodyme and others to informally review this (now that this is mergeable anytime, it's applicable to bounties here https://github.com/solana-labs/solana/blob/master/SECURITY.md; no need to risk our mainnet-beta; see Credits auto rewind on vote recreation #22546 (comment) for reference and i ping you on discord as well)

(I'm being paranoia here because this is editing super critical code.)

bennofs

Looks good to me, just two very picky suggestions, which are more of a personal opinion so feel free to ignore if you don't agree :)

bennofs · 2022-03-05T10:21:09Z

programs/stake/src/stake_state.rs

+    account: &KeyedAccount,
+    meta: &Meta,
+) -> Result<ValidatedDelegatedInfo, InstructionError> {
+    let stake_amount = account.lamports()?.saturating_sub(meta.rent_exempt_reserve); // can't stake the rent


The function is called validate_delegated_amount but uses saturating_sub which won't throw an error if the balance is less than rent exempt reserve. Is it possible to use checked_sub instead of the saturating one and throw an error when the sub fails, or are there already stake accounts on chain which would fail in that case?

The current behavior is intentional. (1) In the case where an account's balance is below the rent exempt minimum, then it has zero delegate-able lamports. (2) Adding new errors and/or changing what conditions return errors will change the program's behavior, which would break consensus without a feature-gate.

(In an initial iteration of this PR I had checks within this function to error in these cases, but I learned that wouldn't be a good idea to add—for the above reasons.)

OK, makes sense

bennofs · 2022-03-05T10:33:53Z

programs/stake/src/stake_state.rs

+    } else {
+        // all clear!
+        // nothing to do here
+    }


this is very picky but you could write this as:

let source_enough_lamports = source_remaining_lamports >= source_minimum_balance; || source_remaining_balance == 0 if !source_enough_lamports { return Err(InstructionError::InsufficientFunds); }

which to me more directly expresses the condition stated in the comment

Noted. I think I'm going to leave the code as-is so that I don't have to run (and babysit) CI again. Let me know if that's not OK.

Yeah that's totally okay

brooksprumo · 2022-03-05T16:47:02Z

@ryoqun

by the way, what do you think about shipping this? I'd suggest 2 more things to solidify our confidence of those changes:

run locally built solana-validator for several epochs against mainnet-beta/testnet/devnet (should see no bank hash mismatch then)

Sounds like a good idea to me; I'll get on this. To be clear, I should run validators on all three networks?

request neodyme and others to informally review this

Also sounds like a good idea!

ryoqun · 2022-03-07T12:29:55Z

@ryoqun

by the way, what do you think about shipping this? I'd suggest 2 more things to solidify our confidence of those changes:

run locally built solana-validator for several epochs against mainnet-beta/testnet/devnet (should see no bank hash mismatch then)

Sounds like a good idea to me; I'll get on this. To be clear, I should run validators on all three networks?

I'd recommend all of them. i know that sounds tedious but the tediousness is 1000x better than the mess of bug at mainent-beta, imo. also, you can spawn 3 gcp instances at the same time and just run your binary there and just wait. I assume this isn't too much of hassle. Previously, I found a bug of stake program only exhibited at testnet's live dataset. (devnet and testnet usually have very odd data; nice exercise for your hot-pressed code). Also this pr's nature is negative feature activation. i mean, its somewhat large code changes must not deviate from the current behavior in every corner of cases. so, it needs dacent track record before reaching to mainnet-beta regardless of urgency (assuming you think this worth backport for upcoming actual change).

on the other hand, merging and v1.10-backporting can comes first to avoid merge conflicts or if it's convenient for you (only pend v1.9-backporting), so that the above general principle is honored. :)

request neodyme and others to informally review this

Also sounds like a good idea!

👍 seems we got it already: ref: #22663 (review) (@bennofs thx!)

brooksprumo · 2022-03-16T15:56:35Z

Ok, I've let this PR run on mainnet-beta, testnet, and devnet for multiple epochs. I'm going to merge this in!

mainnet-beta ran for ~6 days until running into this unrelated consensus issue
testnet ran for 218 hours (just over 9 days) without issue
devnet ran for 164 hours (just under 7 days) without issue

ryoqun · 2022-03-17T05:36:30Z

Ok, I've let this PR run on mainnet-beta, testnet, and devnet for multiple epochs. I'm going to merge this in!

* mainnet-beta ran for ~6 days until running into [this unrelated consensus issue](https://github.com/solana-labs/solana/issues/23682)

* testnet ran for 218 hours (just over 9 days) without issue

* devnet ran for 164 hours (just under 7 days) without issue

congrats! :) it's really my joy to see this pr to be merged finally. :)

brooksprumo marked this pull request as ready for review January 22, 2022 04:04

brooksprumo requested review from joncinque and jstarry January 22, 2022 04:04

brooksprumo changed the title ~~Add MINIMUM_STAKE_BALANCE constant~~ Add MINIMUM_STAKE_DELEGATION constant Jan 22, 2022

brooksprumo mentioned this pull request Jan 29, 2022

Raise stake minimum delegation #22559

Open

18 tasks

brooksprumo force-pushed the minimum-stake-constant branch 2 times, most recently from bae5a1a to a5063fe Compare January 31, 2022 16:51

brooksprumo commented Jan 31, 2022

View reviewed changes

runtime/src/bank.rs Outdated Show resolved Hide resolved

joncinque reviewed Feb 1, 2022

View reviewed changes

brooksprumo requested a review from joncinque February 1, 2022 15:19

joncinque previously approved these changes Feb 1, 2022

View reviewed changes

brooksprumo requested review from CriesofCarrots and ryoqun February 1, 2022 15:27

brooksprumo force-pushed the minimum-stake-constant branch from 0fd962b to e66ae1e Compare February 2, 2022 23:53

CriesofCarrots previously approved these changes Feb 3, 2022

View reviewed changes

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

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

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

CriesofCarrots previously approved these changes Feb 4, 2022

View reviewed changes

brooksprumo changed the title ~~Add MINIMUM_STAKE_DELEGATION constant~~ Enforce MINIMUM_STAKE_DELEGATION Feb 4, 2022

brooksprumo changed the title ~~Enforce MINIMUM_STAKE_DELEGATION~~ Add and enforce MINIMUM_STAKE_DELEGATION Feb 4, 2022

joncinque reviewed Mar 2, 2022

View reviewed changes

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

brooksprumo mentioned this pull request Mar 2, 2022

Test stake split: destination delegation is at least the minimum #23456

Merged

brooksprumo force-pushed the minimum-stake-constant branch from b26cf9d to 1a2384d Compare March 3, 2022 18:01

Refactor and use MINIMUM_STAKE_DELEGATION

74ba897

brooksprumo force-pushed the minimum-stake-constant branch from 1a2384d to 74ba897 Compare March 3, 2022 18:17

brooksprumo requested a review from joncinque March 3, 2022 18:23

brooksprumo mentioned this pull request Mar 3, 2022

Stake tests respect MINIMUM_STAKE_DELEGATION #23426

Merged

joncinque reviewed Mar 3, 2022

View reviewed changes

pr: fix split with staked source and prefunded destination

7683a5e

brooksprumo requested a review from joncinque March 3, 2022 23:45

joncinque previously approved these changes Mar 3, 2022

View reviewed changes

ryoqun reviewed Mar 4, 2022

View reviewed changes

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

ryoqun reviewed Mar 4, 2022

View reviewed changes

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

ryoqun reviewed Mar 4, 2022

View reviewed changes

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

pr

c225718

clippy

d67fc2f

brooksprumo requested a review from ryoqun March 4, 2022 16:48

ryoqun reviewed Mar 5, 2022

View reviewed changes

ryoqun approved these changes Mar 5, 2022

View reviewed changes

bennofs approved these changes Mar 5, 2022

View reviewed changes

brooksprumo merged commit 74bb527 into solana-labs:master Mar 16, 2022

brooksprumo deleted the minimum-stake-constant branch March 16, 2022 15:57

Refactor and use MINIMUM_STAKE_DELEGATION constant #22663

Refactor and use MINIMUM_STAKE_DELEGATION constant #22663

Conversation

brooksprumo commented Jan 22, 2022 • edited Loading

Problem

Summary of Changes

Notes

codecov bot commented Jan 22, 2022 • edited Loading

Codecov Report

brooksprumo commented Jan 22, 2022 • edited Loading

jstarry commented Jan 22, 2022

brooksprumo commented Jan 22, 2022

jstarry commented Jan 22, 2022

brooksprumo commented Jan 22, 2022 • edited Loading

brooksprumo commented Jan 31, 2022

joncinque left a comment

Choose a reason for hiding this comment

joncinque left a comment

Choose a reason for hiding this comment

CriesofCarrots left a comment

Choose a reason for hiding this comment

t-nelson commented Feb 4, 2022

brooksprumo commented Feb 4, 2022

brooksprumo commented Feb 7, 2022

joncinque left a comment

Choose a reason for hiding this comment

brooksprumo commented Mar 3, 2022

joncinque left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brooksprumo Mar 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joncinque left a comment

Choose a reason for hiding this comment

brooksprumo commented Mar 4, 2022

ryoqun commented Mar 4, 2022

Choose a reason for hiding this comment

ryoqun left a comment • edited Loading

Choose a reason for hiding this comment

bennofs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brooksprumo commented Mar 5, 2022

ryoqun commented Mar 7, 2022

brooksprumo commented Mar 16, 2022

ryoqun commented Mar 17, 2022

brooksprumo commented Jan 22, 2022 •

edited

Loading

codecov bot commented Jan 22, 2022 •

edited

Loading

brooksprumo commented Jan 22, 2022 •

edited

Loading

brooksprumo commented Jan 22, 2022 •

edited

Loading

brooksprumo Mar 3, 2022 •

edited

Loading

ryoqun left a comment •

edited

Loading