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

Rewrite stake accounts for clear migration #13461

Merged
merged 9 commits into from
Nov 19, 2020

Conversation

ryoqun
Copy link
Member

@ryoqun ryoqun commented Nov 7, 2020

Problem

this is very much still draft. this is done job!

bugs (those were fixed by #13357 and #13358) created many kinds of bad stake accounts.

Summary of Changes

todo: add test. done

Fixes #

runtime/src/bank.rs Outdated Show resolved Hide resolved
@ryoqun ryoqun added CI Pull Request is ready to enter CI and removed CI Pull Request is ready to enter CI labels Nov 7, 2020
@ryoqun ryoqun added the CI Pull Request is ready to enter CI label Nov 7, 2020
@solana-grimes solana-grimes removed the CI Pull Request is ready to enter CI label Nov 7, 2020
@codecov
Copy link

codecov bot commented Nov 8, 2020

Codecov Report

Merging #13461 (929e217) into master (acc40b1) will increase coverage by 0.0%.
The diff coverage is 98.9%.

@@           Coverage Diff            @@
##           master   #13461    +/-   ##
========================================
  Coverage    82.2%    82.2%            
========================================
  Files         378      378            
  Lines       91258    91433   +175     
========================================
+ Hits        75065    75244   +179     
+ Misses      16193    16189     -4     

runtime/src/bank.rs Outdated Show resolved Hide resolved
runtime/src/genesis_utils.rs Outdated Show resolved Hide resolved
@ryoqun
Copy link
Member Author

ryoqun commented Nov 16, 2020

  • add another feature gate specifically for this.

runtime/src/genesis_utils.rs Outdated Show resolved Hide resolved
runtime/src/genesis_utils.rs Outdated Show resolved Hide resolved
runtime/src/bank.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
programs/stake/src/stake_state.rs Outdated Show resolved Hide resolved
programs/stake/src/stake_state.rs Outdated Show resolved Hide resolved
runtime/src/bank.rs Outdated Show resolved Hide resolved
@ryoqun
Copy link
Member Author

ryoqun commented Nov 17, 2020

  • add another feature gate specifically for this.

@CriesofCarrots Also, I noticed that this calculation could be off without rewriting all rent_exempt_reserve and only enabling stake_program_v2. Because stake account size and rent exempt reserve isn't guaranteed to be in sync at the moment unfortunately, maybe?:

let split_rent_exempt_reserve = calculate_split_rent_exempt_reserve(
meta.rent_exempt_reserve,
self.data_len()? as u64,
split.data_len()? as u64,
);

@ryoqun ryoqun marked this pull request as ready for review November 18, 2020 16:53
@ryoqun ryoqun changed the title Reduce overage stake by rewritng stake accounts Rewritng stake accounts for clear migration Nov 18, 2020
@ryoqun ryoqun changed the title Rewritng stake accounts for clear migration Rewrite stake accounts for clear migration Nov 18, 2020
Copy link
Contributor

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

Fantastic, this seems really close! A couple nits, and I just have a couple quick questions about the feature activation (probably I'm missing something).

ledger-tool/src/main.rs Outdated Show resolved Hide resolved
runtime/src/bank.rs Outdated Show resolved Hide resolved
runtime/src/bank.rs Outdated Show resolved Hide resolved
runtime/src/bank.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

Thanks for all the care taken on this. Lgtm!

@ryoqun
Copy link
Member Author

ryoqun commented Nov 19, 2020

finally! I've finished to test this locally extensively with mainnet-beta snapshot. Merging this shortly.

@ryoqun ryoqun merged commit 43d5e47 into solana-labs:master Nov 19, 2020
mergify bot pushed a commit that referenced this pull request Nov 19, 2020
* Reduce overage stake by rewritng stake accounts

* Write tests and finish implemention

* Create and use new feature gate

* Clean up logging

* Fix typo

* Simplify enable_rewrite_stake

* Fix typo...

* Even simplify gating

* Add metrics

(cherry picked from commit 43d5e47)
mergify bot added a commit that referenced this pull request Nov 19, 2020
* Reduce overage stake by rewritng stake accounts

* Write tests and finish implemention

* Create and use new feature gate

* Clean up logging

* Fix typo

* Simplify enable_rewrite_stake

* Fix typo...

* Even simplify gating

* Add metrics

(cherry picked from commit 43d5e47)

Co-authored-by: Ryo Onodera <ryoqun@gmail.com>
@Lichtso
Copy link
Contributor

Lichtso commented May 24, 2023

Can we remove the function Bank::reconfigure_token2_native_mint() now?

@mvines
Copy link
Member

mvines commented May 25, 2023

Can we remove the function Bank::reconfigure_token2_native_mint() now?

Did you mean to necropost here? Short answer is maybe! Worth exploring

@Lichtso
Copy link
Contributor

Lichtso commented May 25, 2023

Yes, I meant to post. Just saw the function added in this PR still being around as I am working on the feature set transition for the LoadedPrograms cache.

@ryoqun
Copy link
Member Author

ryoqun commented May 26, 2023

Yes, I meant to post. Just saw the function added in this PR still being around as I am working on the feature set transition for the LoadedPrograms cache.

@Lichtso hmm? i think reconfigure_token2_native_mint is added by a different pr: #11966 (comment), not this pr..

anyway, i also think it's okay to remove that function. (thanks for feature cleanup prs by the way)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants