-
Notifications
You must be signed in to change notification settings - Fork 819
remove stake program builtin #7203
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
Conversation
a6cde57 to
f502627
Compare
|
playing whackamole with what i am supposed to change now that the stake program is migrated...
plus various test changes already committed... still todo:
more todo:
the most perplexing thing to fix in this pr was that no note we had test failures in |
a63431e to
88184af
Compare
aff7033 to
05aaaa5
Compare
16e51ec to
d09ad5a
Compare
9504c99 to
246f038
Compare
| #[test] | ||
| fn test_builtin_program_migration() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is kind of a shame to delete this whole test but i dont see how to realistically keep is since we no longer have any migrating programs. maybe make a mental note of the fact that we removed it, so we can add it back when we have a new migration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it's better to just refactor the BuiltinProgramsFilter a little bit so that, in a test environment, it can be mocked out, and we can keep the tests here and here around to make sure the scaffold works.
Might be a tall order though, considering the compile-time assertions involved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about this? we keep the important test and while it doesnt actually test anything it gives some automatic safety next time a migration is added back #7495
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that looks good but would prefer it to be part of this PR so that we can read the diff easier
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ( | ||
| solana_sdk_ids::stake::ID, | ||
| None, | ||
| include_bytes!("programs/core_bpf_stake-1.0.0.so"), | ||
| ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw im very pleased to report that all the pain of this pr was in dealing with non-ProgramTest setups that are suddenly missing the stake program. zero issues with any of the ProgramTest consumers that now have a different stake program
| let new_stake_authority = solana_pubkey::new_rand(); | ||
| let stake_authority = Keypair::new(); | ||
| let validator = Keypair::new(); | ||
| let voter = Keypair::new(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this test has a complicated diff but the tldr is it created a stake account and wanted to see pubsub updates when it was reauthorized, i just reauthorize the vote account instead for the same effect
| /// Test that lamports can be sent to stake accounts regardless of rewards period. | ||
| #[test] | ||
| fn test_program_execution_restricted_for_stake_account_in_reward_period() { | ||
| use solana_transaction_error::TransactionError::InstructionError; | ||
|
|
||
| fn test_rewards_period_system_transfer() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the logic stripped out here is replaced by solana-program/stake#78, which tests all stake program instructions (except deactivate deliquent rip) instead of just withdraw
runtime/src/bank/tests.rs
Outdated
| // insert a stake account delegated to it, since we do not have a stake program | ||
| let stake_pubkey = Pubkey::new_unique(); | ||
| let stake_account = stake_state::create_account_with_activation_epoch( | ||
| &stake_pubkey, | ||
| &vote_keypair.pubkey(), | ||
| &bank.get_account(&vote_keypair.pubkey()).unwrap(), | ||
| &Rent::default(), | ||
| stake_balance, | ||
| bank.epoch(), | ||
| ); | ||
|
|
||
| bank.store_account(&stake_pubkey, &stake_account); | ||
|
|
||
| // the stakes cache is updated by any successful transaction that touches a stake account | ||
| let transaction = | ||
| system_transaction::transfer(&mint_keypair, &stake_pubkey, 1, bank.last_blockhash()); | ||
|
|
||
| bank.process_transaction(&transaction).unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
highlighting this in case it is contentious. this is the only runtime test that uses a stake program interaction for something "real." but i believe my change is just as good in accomplishing the intent of the test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test doesn't test the right thing anymore. The stake account is first added to the stakes cache by bank.store_account(&stake_pubkey, &stake_account) above. After the transfer you're not checking that the stake account balance in the stakes cache was increased by 1 lamport.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
youre right, i wrongly believed store_account() was a backdoor for tests that just adds it wth no side effects, knowing that ProgramTestContext function set_account() is a backdoor for tests
| assert_eq!(bank.epoch(), 0); | ||
| assert_eq!( | ||
| bank.hash().to_string(), | ||
| "AyXhbqmPsC46x7MHAuW89pQcNZVrUZnAND6ABWJ24svx", | ||
| "EzyLJJki4ALhQAq5wbmiNctDhytQckGJRXnk9APKXv7r", | ||
| ); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i joked i feel like claude code just replacing the failing static value with the actual one, but per joe this is correct. the bank hash is different because we no longer have the builtin stake program account. but there is no consensus divergence because this is already the case on live clusters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, lgtm. For context: #5331 (comment)
stake-accounts/src/stake_accounts.rs
Outdated
| let core_programs = core_bpf_programs(&genesis_config.rent, |_| true); | ||
| let stake_idx = core_programs | ||
| .iter() | ||
| .position(|(key, _)| *key == stake::program::id()) | ||
| .unwrap(); | ||
| let program = core_programs[stake_idx].1.clone(); | ||
| let (programdata_address, programdata) = core_programs[stake_idx + 1].clone(); | ||
|
|
||
| genesis_config.add_account(stake::program::id(), program); | ||
| genesis_config.add_account(programdata_address, programdata); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the same as what i originally did in LocalCluster (but changed it to load all core programs). but unlike there, just throwing the program in here is totally unobjectionable, because its just testing a stake cli
fwiw these are the only two places i needed the core program binary blobs outside ProgramTest, so i didnt consider it important to break out the core bpf programs into a new package. in fact i think its better they live there: circular dependencies create a barrier to dragging these magic elves (lol) all over the code base where they dont belong
| for core_program_account in &core_bpf_programs(&Rent::default(), |_| true) { | ||
| config | ||
| .additional_accounts | ||
| .push(core_program_account.clone()); | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
im mildly concerned this is a hack, i dont know what mechanism a real cluster genesis uses to make sure it gets the core bpf programs, and if it is viable for LocalCluster to use it as well. i guess my view is "if we have some kind of test coverage of the multinode setup or similar, that ensures fresh clusters actually work, what we do in LocalCluster doesnt really matter. if LocalCluster is the only test surface we have over setting up clusters, probably we should use the formal mechanism that real clusters use to get core bpf programs"
also i assume that because it goes through the same kind of GenesisConfig setup as bank tests, LocalCluster is implicitly FeatureSet::all_enabled(), but i didnt find where this actually happens
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you need to add the Stake BPF program to the genesis setup, right? I'm assuming local cluster goes through this whole process.
https://github.com/2501babe/solana/blob/20250728_stakebpf/fetch-core-bpf.sh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah ok! this is the mechanism for cluster genesis that i was not aware of 😅 also ive updated the binary name in the stake program release as i didnt know there was a convention for it
LocalCluster definitely does not call any shell scripts or pull anything down from github, no. i believe in master it just doesnt have any of the core bpf programs, but until this pr, it never uses any of them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a reasonable way to do it though! Unless anyone has any opposition to the program-test dep here, wfm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm opposed! program-test is for testing programs.. local-cluster is for setting up a cluster with staked nodes.. there should definitely be a way to setup a test cluster without program-test in the agave repo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is splitting the core program binaries into a separate package sufficient or do you think it should clone from github like a real cluster?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not exactly sure what you mean. Could we add a dependency on solana-stake-program and use that to build the sbf binary? What are we currently doing for program test?
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #7203 +/- ##
=========================================
- Coverage 83.4% 83.4% -0.1%
=========================================
Files 814 814
Lines 366038 366031 -7
=========================================
- Hits 305516 305443 -73
- Misses 60522 60588 +66 🚀 New features to boost your workflow:
|
8d5ed9e to
4e60638
Compare
buffalojoec
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Just needs a rebase and I'll stamp from my side.
3c75de9 to
d36f10e
Compare
|
@yihau can you accept |
|
green! 🫡 |
|
@anza-xyz/fees i think we need another fees approval for this to land |
* remove stake program from `BUILTINS` and `MIGRATING_BUILTINS_COSTS` * add bpf state program to `CORE_BPF_PROGRAMS` * use bpf program in solana-test-validator, `ProgramTest`, and `LocalCluster` * add stake program repo to core bpf genesis script * break core bpf and spl binaries out into new solana-program-binaries crate * fix bank-based tests that no longer have a stake program
d36f10e to
855e90f
Compare
|
😱 New commits were pushed while the automerge label was present. |
apfitzge
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
review for fee-related changes: lgtm
|
automerge label removed due to a CI failure |
Problem
the bpf stake program is now active on all clusters and can be removed from builtins
Summary of Changes
remove it from
BUILTINS, add the bpf stake program 1.0.0 binary blob toCORE_BPF_PROGRAMS, and fix various tests and test setups to deal with this:programs/stake-tests/. these were tests forMoveStakeandMoveLamportswhich have long since been duplicated in the bpf stake repothis pr is already quite difficult to review. if everyone is happy with this idea, id like to merge this as a standalone pr without removing the stake program code, and do that in a followup pr. removing the stake program code is nontrivial because it contains a bunch of utility functions which are used in tests, during genesis, and possibly elsewhere. it also contains some of the rewards-related code still running in agave. and we may want to reexport various things to not break downstream too badly
these changes will have to be debated (particularly breaking the interface, whether we move things into new packages, etc) and itd be better to look at them in isolation