Fix stake split rent-exempt adjustment by CriesofCarrots · Pull Request #13357 · solana-labs/solana

CriesofCarrots · 2020-11-03T02:03:43Z

Problem

Stake split doesn't properly handle the stake deduction from a Delegated original account when the new account has a balance less than the rent-exempt reserve. The original stake needs to be reduced by the entire balance transferred, except in the case of splitting the entire source stake, when it should simply be zeroed.

Stake split also errors on an attempt to split all of a source account to a new account that holds a balance. This is inconsistent with the behavior for a partial split, where an existing balance is acceptable and used to cover the rent-exempt reserve (seetest_split_with_rent()).

Summary of Changes

Fix math

Before/after examples added in comments.

This is WIP as I need to coordinate gating with @t-nelson
cc: @ryoqun @mvines

CriesofCarrots · 2020-11-03T02:04:12Z

Splitting partial amount into empty account (default cli behavior)

Before:

$ solana stake-account stake.json
Balance: 0.1 SOL
Rent Exempt Reserve: 0.00228288 SOL
Delegated Stake: 0.09771712 SOL
Active Stake: 0.09771712 SOL
Delegated Vote Account Address: 2FPim6EJBTW3uwr36fAhK8BEGB22BjJuxkPbtMYAeVUv

$ solana split-stake stake.json split.json 0.05
Signature: 4u1xhY2NqvsoGQHqh65QvJpT3AVyBoyv2K7Bx6h3JbuJ8NLwMFAeHKCZdhSi8wNm5huYMvmphqHn1nYzs39SLYRw

$ solana stake-account stake.json
Balance: 0.05 SOL
Rent Exempt Reserve: 0.00228288 SOL
Delegated Stake: 0.05 SOL <--- Notice stake is not less the rent-exempt-reserve
Active Stake: 0.05 SOL
Delegated Vote Account Address: 2FPim6EJBTW3uwr36fAhK8BEGB22BjJuxkPbtMYAeVUv

$ solana stake-account split.json`
Balance: 0.05 SOL
Rent Exempt Reserve: 0.00228288 SOL
Delegated Stake: 0.04771712 SOL
Active Stake: 0.04771712 SOL
Delegated Vote Account Address: 2FPim6EJBTW3uwr36fAhK8BEGB22BjJuxkPbtMYAeVUv

After:

$ solana stake-account stake.json
Balance: 0.1 SOL
Rent Exempt Reserve: 0.00228288 SOL
Delegated Stake: 0.09771712 SOL
Active Stake: 0.09771712 SOL
Delegated Vote Account Address: GGohSWd9vwLDqfu32nXuzYM7B6W8N7baRSo5ph9CJsG9

$ solana split-stake stake.json split.json 0.05
Signature: 4oEubJWX4WcjDQaN2KtN2Li4TsZD3WRDeWfGGAg26FzWATKEbJt8Nc84zhfMDx78pY1oxgnGhQMsWDhB75MJTTSK

$ solana stake-account stake.json
Balance: 0.05 SOL
Rent Exempt Reserve: 0.00228288 SOL
Delegated Stake: 0.04771712 SOL
Active Stake: 0.04771712 SOL
Delegated Vote Account Address: GGohSWd9vwLDqfu32nXuzYM7B6W8N7baRSo5ph9CJsG9

$ solana stake-account split.json
Balance: 0.05 SOL
Rent Exempt Reserve: 0.00228288 SOL
Delegated Stake: 0.04771712 SOL
Active Stake: 0.04771712 SOL
Delegated Vote Account Address: GGohSWd9vwLDqfu32nXuzYM7B6W8N7baRSo5ph9CJsG9

CriesofCarrots · 2020-11-03T02:08:22Z

Splitting complete stake account into account prefunded with rent-exempt reserve of 0.00228288 SOL (I had to edit cli to use the following create-account instruction):

system_instruction::create_account(
  authorized_pubkey, // Sending 0, so any signer will suffice
  split_stake_pubkey,
  2282880,
  std::mem::size_of::<StakeState>() as u64,
  &id(),
)

Before:

$ solana stake-account stake.json
Balance: 0.1 SOL
Rent Exempt Reserve: 0.00228288 SOL
Delegated Stake: 0.09771712 SOL
Active Stake: 0.09771712 SOL
Delegated Vote Account Address: JCcavabWfWZc9ykWb68Vjvpy6oH6nYWGrkwFdEnk4Z95

$ solana split-stake stake.json split.json 0.1
Error: RPC response error -32002: Transaction simulation failed: Error processing Instruction 1: custom program error: 0x4

After:

$ solana stake-account /Users/tyeraeulberg/.config/solana/stake.json`
Balance: 0.1 SOL
Rent Exempt Reserve: 0.00228288 SOL
Delegated Stake: 0.09771712 SOL
Active Stake: 0.09771712 SOL
Delegated Vote Account Address: FmM9tLoFrqAAS6K9MrMxDWT2o1RzpE5cv388iYJSMAm2
Stake Authority: Bbqg1M4YVVfbhEzwA9SpC9FhsaG83YMTYoR4a8oTDLX
Withdraw Authority: Bbqg1M4YVVfbhEzwA9SpC9FhsaG83YMTYoR4a8oTDLX

$ solana split-stake stake.json split.json 0.1
Signature: rVtvcTHJX6VYdYBPKyiX9rssVAB9SUSK6jNU28HSmwTMf9HJXVsrKRtrSCfVjbeam8V34CNFR8BmkDwkNPRHFVq

$ solana stake-account stake.json
Error: AccountNotFound: pubkey=CYRJWqiSjLitBAcRxPvWpgX3s5TvmN2SuRY3eEYypFvT

$ solana stake-account split.json
Balance: 0.10228288 SOL
Rent Exempt Reserve: 0.00228288 SOL
Delegated Stake: 0.09771712 SOL
Active Stake: 0.09771712 SOL
Delegated Vote Account Address: FmM9tLoFrqAAS6K9MrMxDWT2o1RzpE5cv388iYJSMAm2

CriesofCarrots · 2020-11-03T02:19:36Z

@rwalker-com , would you be able to take a look at this and see if I'm missing anything? 🙏

rwalker-com

lgtm. all I had to do was read tests :-)

codecov · 2020-11-03T03:20:33Z

Codecov Report

Merging #13357 (12fa28f) into master (fe1e08b) will increase coverage by 0.0%.
The diff coverage is 99.6%.

@@           Coverage Diff            @@
##           master   #13357    +/-   ##
========================================
  Coverage    82.0%    82.1%            
========================================
  Files         378      378            
  Lines       90042    90286   +244     
========================================
+ Hits        73912    74182   +270     
+ Misses      16130    16104    -26

t-nelson

LGTM! Just a couple comment typoes

Pull request has been modified.

ryoqun · 2020-11-03T22:39:34Z

+    let lamports_per_byte_year =
+        source_meta.rent_exempt_reserve / (source_data_len + ACCOUNT_STORAGE_OVERHEAD);
+    lamports_per_byte_year * (split_data_len + ACCOUNT_STORAGE_OVERHEAD)


Also, let's cross-ref this out-of-wild rent calculation was unavoidable at Rent in rent.rs. :) So that future engineer working on rent can be wary of this.

Where would you like to see that comment in rent.rs?

ryoqun · 2020-11-04T13:32:55Z

+                        // split account.
+                        (
+                            lamports - meta.rent_exempt_reserve,
+                            lamports - split_rent_exempt_reserve,


Argurably hard to notice, but I think this causes overflow, allowing user to stake around u64::max_size() (if the validator is compiled with release build) if all of following conditions are met by specifically creating crafted account state and transaction by malicious users:

lamports is identical as source.lamports().

split account is bigger than source; so split_rent_exempt_reserve > meta.rent_exempt_reserve

split account is pre-funded so that split.lamports()? + lamports >= split_rent_exempt_reserve.

I think the best thing we could here is to reject such a case? Also, I'd like to add a test for this. (I know you already adding very extensive set of tests. Sorry for more trouble).

I think I see. I'll write a test to confirm. Yes, I think rejecting makes sense.

Please see test_split_100_percent_of_source_to_larger_account_edge_case()

ryoqun · 2020-11-04T13:44:47Z

+                        // requested, less any lamports needed to cover the rent-exempt reserve
+                        (
+                            lamports,
+                            lamports - split_rent_exempt_reserve.saturating_sub(split.lamports()?),


I think similar attack is possible here like this: https://github.com/solana-labs/solana/pull/13357/files#r517344371

This case is covered by the initial lamports check, more obvious with your rewriting:
lamports < split_rent_exempt_reserve.saturating_sub(split.lamports()?)

lamports must be greater than or equal to that value

I included my test that demonstrates this: test_split_to_larger_account_edge_case()

ryoqun · 2020-11-04T13:46:45Z

Still pondering on this. But I think this getting very solid with many new tests. Thanks for great work!

ryoqun · 2020-11-04T13:55:01Z

@ryoqun , hoping for a little more guidance about where you'd like documentation in rent.rs bowing_woman . Otherwise, I think I've addressed all of your comments.

maybe, this fn:

solana/sdk/program/src/rent.rs

Line 49 in b0d1ae1

pub fn minimum_balance(&self, data_len: usize) -> u64 {

along like:

// note that stripped down version of this calcuration is reluctantly copied into calculate_split_rent_exempt_reserve in the stake program

t-nelson

Looking good! I just have a couple test hardening recommendations

CriesofCarrots · 2020-11-04T22:34:15Z

This will need rebasing on #13394 (make sure it goes in stake_state.rs, as opposed to the legacy mod)

t-nelson

LGTM!

Pull request has been modified.

…t overflow splits

* Add failing tests * Fix stake split * Calculate split rent-exempt-reserve and use * Add comment in rent.rs * Add tests for edge cases when splitting to larger accounts, and reject overflow splits * Reframe InsufficientFunds checks in terms of lamports var * Test hardening review comments (cherry picked from commit 4c5f345)

* Add failing tests * Fix stake split * Calculate split rent-exempt-reserve and use * Add comment in rent.rs * Add tests for edge cases when splitting to larger accounts, and reject overflow splits * Reframe InsufficientFunds checks in terms of lamports var * Test hardening review comments (cherry picked from commit 4c5f345) Co-authored-by: Tyera Eulberg <teulberg@gmail.com>

CriesofCarrots force-pushed the stake-split-bug branch from a07174f to eb8a8db Compare November 3, 2020 02:10

CriesofCarrots added the work in progress This isn't quite right yet label Nov 3, 2020

CriesofCarrots marked this pull request as ready for review November 3, 2020 02:20

CriesofCarrots added v1.3 labels Nov 3, 2020

CriesofCarrots requested a review from t-nelson November 3, 2020 02:20

rwalker-com approved these changes Nov 3, 2020

View reviewed changes

t-nelson mentioned this pull request Nov 3, 2020

Fix stake redelegate #13358

Merged

t-nelson previously approved these changes Nov 3, 2020

View reviewed changes

Comment thread programs/stake/src/stake_state.rs Outdated

Comment thread programs/stake/src/stake_state.rs Outdated

CriesofCarrots force-pushed the stake-split-bug branch from eb8a8db to 819b071 Compare November 3, 2020 04:51

ryoqun reviewed Nov 3, 2020

View reviewed changes

Comment thread programs/stake/src/stake_state.rs Outdated

CriesofCarrots requested review from ryoqun and t-nelson November 3, 2020 21:43

t-nelson reviewed Nov 3, 2020

View reviewed changes

Comment thread programs/stake/src/stake_state.rs Outdated

t-nelson reviewed Nov 3, 2020

View reviewed changes

Comment thread programs/stake/src/stake_state.rs

CriesofCarrots force-pushed the stake-split-bug branch from 69d4d2d to 013f40d Compare November 3, 2020 22:23

ryoqun reviewed Nov 3, 2020

View reviewed changes

Comment thread programs/stake/src/stake_state.rs

ryoqun reviewed Nov 3, 2020

View reviewed changes

CriesofCarrots force-pushed the stake-split-bug branch from 013f40d to 6f399a4 Compare November 3, 2020 22:47

ryoqun reviewed Nov 3, 2020

View reviewed changes

Comment thread programs/stake/src/stake_state.rs Outdated

ryoqun reviewed Nov 3, 2020

View reviewed changes

Comment thread programs/stake/src/stake_state.rs Outdated

ryoqun reviewed Nov 3, 2020

View reviewed changes

Comment thread programs/stake/src/stake_state.rs Outdated

CriesofCarrots force-pushed the stake-split-bug branch from 6f399a4 to 8a99ee3 Compare November 3, 2020 23:03

ryoqun reviewed Nov 3, 2020

View reviewed changes

Comment thread programs/stake/src/stake_state.rs Outdated

ryoqun reviewed Nov 3, 2020

View reviewed changes

Comment thread programs/stake/src/stake_state.rs Outdated

ryoqun reviewed Nov 4, 2020

View reviewed changes

Comment thread programs/stake/src/stake_state.rs Outdated

ryoqun reviewed Nov 4, 2020

View reviewed changes

t-nelson reviewed Nov 4, 2020

View reviewed changes

Comment thread programs/stake/src/stake_state.rs

Comment thread programs/stake/src/stake_state.rs Outdated

t-nelson previously approved these changes Nov 4, 2020

View reviewed changes

CriesofCarrots force-pushed the stake-split-bug branch from 4b344a1 to 12fa28f Compare November 6, 2020 12:55

CriesofCarrots removed the work in progress This isn't quite right yet label Nov 6, 2020

Tyera Eulberg added 7 commits November 6, 2020 12:00

Add failing tests

75a96e4

Fix stake split

f4c5c05

Calculate split rent-exempt-reserve and use

facacaf

Add comment in rent.rs

a5974f8

Add tests for edge cases when splitting to larger accounts, and rejec…

99284bb

…t overflow splits

Reframe InsufficientFunds checks in terms of lamports var

a553e52

Test hardening review comments

1ddc739

CriesofCarrots force-pushed the stake-split-bug branch from 12fa28f to 1ddc739 Compare November 6, 2020 19:16

CriesofCarrots added the automerge Merge this Pull Request automatically once CI passes label Nov 6, 2020

mergify Bot merged commit 4c5f345 into solana-labs:master Nov 6, 2020

mergify Bot mentioned this pull request Nov 6, 2020

Fix stake split rent-exempt adjustment (bp #13357) #13452

Merged

mergify Bot mentioned this pull request Nov 6, 2020

Fix stake split rent-exempt adjustment (bp #13357) #13453

Merged

ryoqun mentioned this pull request Nov 10, 2020

Rewrite stake accounts for clear migration #13461

Merged

CriesofCarrots deleted the stake-split-bug branch November 18, 2020 00:26

ryoqun mentioned this pull request Nov 22, 2020

Stake: Support merging fully-activated stake accounts #13712

Merged

Conversation

CriesofCarrots commented Nov 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Summary of Changes

Uh oh!

CriesofCarrots commented Nov 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CriesofCarrots commented Nov 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CriesofCarrots commented Nov 3, 2020

Uh oh!

rwalker-com left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Nov 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

t-nelson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryoqun Nov 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CriesofCarrots Nov 3, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryoqun Nov 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryoqun Nov 4, 2020

Choose a reason for hiding this comment

Uh oh!

CriesofCarrots Nov 4, 2020

Choose a reason for hiding this comment

Uh oh!

CriesofCarrots Nov 4, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryoqun Nov 4, 2020

Choose a reason for hiding this comment

Uh oh!

CriesofCarrots Nov 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CriesofCarrots Nov 4, 2020

Choose a reason for hiding this comment

Uh oh!

ryoqun commented Nov 4, 2020

Uh oh!

ryoqun commented Nov 4, 2020

Uh oh!

t-nelson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

CriesofCarrots commented Nov 4, 2020

Uh oh!

t-nelson left a comment

Choose a reason for hiding this comment

CriesofCarrots commented Nov 3, 2020 •

edited

Loading

CriesofCarrots commented Nov 3, 2020 •

edited

Loading

CriesofCarrots commented Nov 3, 2020 •

edited

Loading

rwalker-com left a comment •

edited

Loading

codecov Bot commented Nov 3, 2020 •

edited

Loading

ryoqun Nov 3, 2020 •

edited

Loading

ryoqun Nov 4, 2020 •

edited

Loading

CriesofCarrots Nov 4, 2020 •

edited

Loading