Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make active stake consistent in split #33295

Merged
merged 18 commits into from
Sep 20, 2023

Conversation

CriesofCarrots
Copy link
Contributor

Problem

Stake split still contains some brittle logic.

Summary of Changes

Update tests to assert that active stake is consistent across split
Add feature to make rent-exempt handling consistent

@CriesofCarrots CriesofCarrots added the v1.16 PRs that should be backported to v1.16 label Sep 18, 2023
@CriesofCarrots CriesofCarrots force-pushed the rent-exempt-split branch 2 times, most recently from 5d54c37 to e201022 Compare September 18, 2023 18:11
@codecov
Copy link

codecov bot commented Sep 18, 2023

Codecov Report

Merging #33295 (e6a8808) into master (7a8a492) will increase coverage by 0.0%.
Report is 1 commits behind head on master.
The diff coverage is 96.2%.

@@           Coverage Diff            @@
##           master   #33295    +/-   ##
========================================
  Coverage    81.9%    82.0%            
========================================
  Files         795      796     +1     
  Lines      215226   215640   +414     
========================================
+ Hits       176404   176838   +434     
+ Misses      38822    38802    -20     

programs/stake/src/stake_state.rs Outdated Show resolved Hide resolved
programs/stake/src/stake_instruction.rs Outdated Show resolved Hide resolved
programs/stake/src/stake_instruction.rs Outdated Show resolved Hide resolved
joncinque
joncinque previously approved these changes Sep 19, 2023
Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! I'll keep at the stake pool changes in order to support this, then re-enable the downstream job

Copy link
Contributor

@t-nelson t-nelson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@CriesofCarrots CriesofCarrots added the automerge Merge this Pull Request automatically once CI passes label Sep 20, 2023
@mergify mergify bot merged commit bca41ed into solana-labs:master Sep 20, 2023
24 checks passed
mergify bot pushed a commit that referenced this pull request Sep 20, 2023
* Add feature gate

* Add helper fn

* Require split destination to be rent-exempt if it is active

* Update cli to prefund split accounts

* cli: require rent param with sign-only

* Update tokens to prefund split accounts

* Update split tests with sysvar accounts

* Fix test_split_to_account_with_rent_exempt_reserve

* Fix test_staked_split_destination_minimum_balance

* Fix test_split_more_than_staked

* Fix test_split_minimum_stake_delegation and remove misleading StakeState::Initialized case

* Fix test_split_from_larger_sized_account

* Add test for pre-/post-activation behavior splitting some or all of stake account

* Assert active stake

* Fix runtime test

* Ignore stake-pool downstream

* Review comments

* Feature gate sysvar reads

(cherry picked from commit bca41ed)

# Conflicts:
#	cli/tests/stake.rs
#	programs/stake/src/stake_instruction.rs
#	programs/stake/src/stake_state.rs
#	runtime/tests/stake.rs
#	sdk/src/feature_set.rs
CriesofCarrots added a commit that referenced this pull request Sep 20, 2023
…3319)

* Make active stake consistent in split (#33295)

* Add feature gate

* Add helper fn

* Require split destination to be rent-exempt if it is active

* Update cli to prefund split accounts

* cli: require rent param with sign-only

* Update tokens to prefund split accounts

* Update split tests with sysvar accounts

* Fix test_split_to_account_with_rent_exempt_reserve

* Fix test_staked_split_destination_minimum_balance

* Fix test_split_more_than_staked

* Fix test_split_minimum_stake_delegation and remove misleading StakeState::Initialized case

* Fix test_split_from_larger_sized_account

* Add test for pre-/post-activation behavior splitting some or all of stake account

* Assert active stake

* Fix runtime test

* Ignore stake-pool downstream

* Review comments

* Feature gate sysvar reads

(cherry picked from commit bca41ed)

# Conflicts:
#	cli/tests/stake.rs
#	programs/stake/src/stake_instruction.rs
#	programs/stake/src/stake_state.rs
#	runtime/tests/stake.rs
#	sdk/src/feature_set.rs

* Fix conflicts

---------

Co-authored-by: Tyera <tyera@solana.com>
@janlegner
Copy link
Contributor

janlegner commented Sep 26, 2023

This PR has significant impact on how stake authority can be used. E.g. when stake authority is given to 3rd party to operate on the stake accounts, that 3rd party now has pay for rent (which it has no way of claiming back) and cost of such stake management is quite high. Consider for example Marinade Native - having thousands of stake accounts and splitting and re-balancing them was possible specifically because the rent could have been taken from the existing stake account. Now the cost to split the account would be quite high.

Staker funding the split account cannot get hold of the funds later - as opposed to being a withdrawer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge this Pull Request automatically once CI passes v1.16 PRs that should be backported to v1.16
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants