Reject close of active vote accounts by willhickey · Pull Request #22651 · solana-labs/solana

willhickey · 2022-01-21T20:35:29Z

Problem

Withdrawing the rent exempt reserve from a vote account will close the account. This shouldn't be allowed for active vote accounts.

Summary of Changes

Attempted withdraws of the rent exempt reserve check whether the vote account received any credits in the most recent completed epoch. If they did, the withdraw is rejected with an error.

Note: This change is feature gated, and PR 21639 also created a feature in the same function, so the unit test matrix has ballooned and includes a lot of duplicated code. The duplication could be factored out but I choose to leave most of it unrolled to simplify cleanup once both features are activated.

Feature Gate Issue: #24488

codecov · 2022-01-24T23:04:38Z

Codecov Report

Merging #22651 (88b95e3) into master (aea8f0d) will decrease coverage by 0.0%.
The diff coverage is 97.4%.

@@            Coverage Diff            @@
##           master   #22651     +/-   ##
=========================================
- Coverage    81.5%    81.5%   -0.1%     
=========================================
  Files         558      558             
  Lines      149898   150122    +224     
=========================================
+ Hits       122266   122447    +181     
- Misses      27632    27675     +43

CriesofCarrots

Looking good, just one substantive question from me.

Test duplication lgtm. Will be tons easier to clean when either/both features are removed.

willhickey · 2022-01-25T20:11:21Z

@CriesofCarrots Thanks for the feedback!

I'm making changes now. Does the checked math rule apply to tests and test helper functions too?

CriesofCarrots · 2022-01-25T20:21:11Z

Does the checked math rule apply to tests and test helper functions too?

Nope, just the program code itself. Feel free to do whatever's easiest or easiest to read for tests

…e previous epoch

Co-authored-by: Tyera Eulberg <teulberg@gmail.com>

… that has credits in vote_state

CriesofCarrots

lgtm! Thanks for all the iterations and polish.
@t-nelson , can you give this a quick sanity check? 🙏

Pull request has been modified.

CriesofCarrots

oh, nit: we should probably update the error msgs. One possibility suggested below; wordsmithing welcome.

Co-authored-by: Tyera Eulberg <teulberg@gmail.com>

t-nelson

looks great! I love the test coverage. All I have is one logical simplification suggestion and a comment from Tyera's review.

If vote_state.epoch_credits.is_empty(), it should mean the vote account hasn't received any credits ever.
Yes, the VoteState is what is stored in a Vote account on chain, so part of the accounts state on all validators.
There's probably a small race condition here if (a) a validator has submitted it's first votes which haven't landed yet and (b) empties its vote account, but that shouldn't matter for the protection we're trying to add.

Since stake can't be delegated to a validator that hasn't voted, I don't think the race even exists in any meaningful way

Co-authored-by: Trent Nelson <trent.a.b.nelson@gmail.com>

Pull request has been modified.

willhickey · 2022-02-02T17:11:08Z

@t-nelson I hadn't encountered zip before... thanks for the handy new tool!

Should I backport this to 1.9 or just merge it to master?

CriesofCarrots · 2022-02-02T18:41:02Z

Should I backport this to 1.9 or just merge it to master?

I say v1.9 for sure. @t-nelson, do you foresee any more v1.8 releases? If so, I'd like to have this there too.

t-nelson · 2022-02-02T19:10:30Z

Yeah we're definitely going to see more 1.8.x. Let's take it all the way back

CriesofCarrots

* 10461 Reject close of vote accounts unless it earned no credits in the previous epoch. This is checked by comparing current epoch (from clock sysvar) with the most recent epoch with credits in vote state. (cherry picked from commit 75563f6) # Conflicts: # programs/vote/src/vote_processor.rs # programs/vote/src/vote_state/mod.rs # runtime/src/bank.rs # sdk/program/src/instruction.rs # sdk/program/src/program_error.rs # storage-proto/proto/transaction_by_addr.proto # storage-proto/src/convert.rs

* 10461 Reject close of vote accounts unless it earned no credits in the previous epoch. This is checked by comparing current epoch (from clock sysvar) with the most recent epoch with credits in vote state. (cherry picked from commit 75563f6) # Conflicts: # programs/vote/src/vote_processor.rs # sdk/src/feature_set.rs

* Reject close of active vote accounts (#22651) * 10461 Reject close of vote accounts unless it earned no credits in the previous epoch. This is checked by comparing current epoch (from clock sysvar) with the most recent epoch with credits in vote state. (cherry picked from commit 75563f6) # Conflicts: # programs/vote/src/vote_processor.rs # sdk/src/feature_set.rs * Resolve merge conflicts Co-authored-by: Will Hickey <csu_hickey@yahoo.com> Co-authored-by: Will Hickey <will.hickey@solana.com>

* Reject close of active vote accounts (#22651) * 10461 Reject close of vote accounts unless it earned no credits in the previous epoch. This is checked by comparing current epoch (from clock sysvar) with the most recent epoch with credits in vote state. (cherry picked from commit 75563f6) # Conflicts: # programs/vote/src/vote_processor.rs # programs/vote/src/vote_state/mod.rs # runtime/src/bank.rs # sdk/program/src/instruction.rs # sdk/program/src/program_error.rs # storage-proto/proto/transaction_by_addr.proto # storage-proto/src/convert.rs * Resolve merge conflicts * lint * Clippy cleanup * Add import to test module * remove vote processor * Update test_abi_digest hash * cleanup Co-authored-by: Will Hickey <csu_hickey@yahoo.com> Co-authored-by: Will Hickey <will.hickey@solana.com>

* 10461 Reject close of vote accounts unless it earned no credits in the previous epoch. This is checked by comparing current epoch (from clock sysvar) with the most recent epoch with credits in vote state.

willhickey requested review from CriesofCarrots, jstarry and t-nelson January 21, 2022 20:56

CriesofCarrots reviewed Jan 25, 2022

View reviewed changes

Comment thread programs/vote/src/vote_state/mod.rs Outdated

Comment thread programs/vote/src/vote_state/mod.rs Outdated

Comment thread programs/vote/src/vote_state/mod.rs Outdated

CriesofCarrots reviewed Jan 25, 2022

View reviewed changes

Comment thread programs/vote/src/vote_state/mod.rs Outdated

Will Hickey and others added 12 commits January 25, 2022 22:44

10461 Reject close of vote accounts unless it earned no credits in th…

4662399

…e previous epoch

lint

6c7c151

lint

f54e0ec

Update bank::BankSlotDelta_frozen_abi::test_abi_digest

39350cf

Suggestion: name tuple elements. use checked math.

bc684d3

Co-authored-by: Tyera Eulberg <teulberg@gmail.com>

Suggestion: Fix incorrect comment.

0cad77e

Co-authored-by: Tyera Eulberg <teulberg@gmail.com>

Review feedback. Check 2nd to last epoch credits.

ea2b33f

clippy lint

f430854

lint

4461c49

lint

5d04b2a

lint

e5de27e

fix missing } from merge conflict

ceb88e5

CriesofCarrots reviewed Jan 26, 2022

View reviewed changes

Comment thread programs/vote/src/vote_state/mod.rs Outdated

Use clock to determine current epoch and compare to most recent epoch…

1feb4ae

… that has credits in vote_state

willhickey commented Jan 27, 2022

View reviewed changes

Comment thread programs/vote/src/vote_state/mod.rs Outdated

willhickey commented Jan 27, 2022

View reviewed changes

Comment thread programs/vote/src/vote_processor.rs Outdated

CriesofCarrots reviewed Jan 27, 2022

View reviewed changes

Comment thread programs/vote/src/vote_processor.rs Outdated

Comment thread programs/vote/src/vote_state/mod.rs Outdated

Comment thread programs/vote/src/vote_state/mod.rs Outdated

Comment thread programs/vote/src/vote_state/mod.rs Outdated

Sysvar read and use Option<&Clock> param

4f812a8

CriesofCarrots previously approved these changes Jan 28, 2022

View reviewed changes

Cleanup. Remove commented code

abae8f6

CriesofCarrots reviewed Jan 28, 2022

View reviewed changes

Comment thread sdk/program/src/instruction.rs Outdated

Comment thread sdk/program/src/program_error.rs Outdated

willhickey and others added 2 commits January 28, 2022 13:43

Review suggestion: Update error message

ffea66d

Co-authored-by: Tyera Eulberg <teulberg@gmail.com>

Review suggestion: Update error message

6c9295e

Co-authored-by: Tyera Eulberg <teulberg@gmail.com>

t-nelson previously approved these changes Feb 1, 2022

View reviewed changes

Comment thread programs/vote/src/vote_state/mod.rs Outdated

Review suggestion: logic simplification.

88b95e3

Co-authored-by: Trent Nelson <trent.a.b.nelson@gmail.com>

CriesofCarrots added v1.8 labels Feb 2, 2022

CriesofCarrots approved these changes Feb 2, 2022

View reviewed changes

willhickey merged commit 75563f6 into solana-labs:master Feb 2, 2022

mergify Bot mentioned this pull request Feb 2, 2022

Reject close of active vote accounts (backport #22651) #22895

Merged

mergify Bot mentioned this pull request Feb 2, 2022

Reject close of active vote accounts (backport #22651) #22896

Merged

mvines mentioned this pull request Feb 3, 2022

Deleting and recreating vote account causes calculate_stake_points_and_credits to exit early #22512

Closed

willhickey linked an issue Feb 14, 2022 that may be closed by this pull request

Withdrawing from vote account can delete it while still validating #10461

Closed

CriesofCarrots mentioned this pull request Apr 8, 2022

Withdraw all from vote account caused validator node stop working #24183

Closed

willhickey mentioned this pull request Apr 19, 2022

Feature Gate: Disallow closing active vote accounts #24488

Closed

jstarry added the feature-gate Pull Request adds or modifies a runtime feature gate label Jun 29, 2022

Conversation

willhickey commented Jan 21, 2022 • edited by jstarry Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Summary of Changes

Uh oh!

codecov Bot commented Jan 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

CriesofCarrots left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

willhickey commented Jan 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CriesofCarrots commented Jan 25, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CriesofCarrots left a comment

Choose a reason for hiding this comment

Uh oh!

CriesofCarrots left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

t-nelson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

willhickey commented Feb 2, 2022

Uh oh!

CriesofCarrots commented Feb 2, 2022

Uh oh!

t-nelson commented Feb 2, 2022

Uh oh!

CriesofCarrots left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

willhickey commented Jan 21, 2022 •

edited by jstarry

Loading

codecov Bot commented Jan 24, 2022 •

edited

Loading

willhickey commented Jan 25, 2022 •

edited

Loading