-
Notifications
You must be signed in to change notification settings - Fork 818
remove solana-stake-program
#8834
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
|
The Firedancer team maintains a line-for-line reimplementation of the |
71abb0a to
c1732d2
Compare
00fe45a to
1225ef4
Compare
solana-stake-program
625b89d to
b50c9cf
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #8834 +/- ##
=======================================
Coverage 83.2% 83.2%
=======================================
Files 864 860 -4
Lines 375287 375284 -3
=======================================
+ Hits 312389 312390 +1
+ Misses 62898 62894 -4 🚀 New features to boost your workflow:
|
|
added both of you but this only needs one full review, i was thinking @grod220 could go over the detail since this relates to his de-floating (sinking?) work on stake-interface, while @joncinque might want to glance over and confirm there are no bigger-picture concerns eg with genesis or downstream projects |
joncinque
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 on my side! Thanks for doing all the cleanup
| #[cfg(feature = "dev-context-only-utils")] | ||
| fn create_stake_account( |
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.
We've had to duplicate a function across accounts-db and runtime in the past, so I'm fine with it here
| EpochSchedule, DEFAULT_LEADER_SCHEDULE_SLOT_OFFSET, MINIMUM_SLOTS_PER_EPOCH, | ||
| }, | ||
| solana_keypair::Keypair, | ||
| solana_runtime::bank::Bank, |
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.
Weird, this wasn't caught by clippy before?
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 looks like clippy doesnt detect useless test imports like it does code imports only used in tests. i went to add a useless test import in svm/ to double check it wasnt a weird build thing and turns out it already has at least one 😅
grod220
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.
LGTM, great job!
b50c9cf to
bebd047
Compare
|
oh i remember why we hated the merge queue last time we tried it, it doesnt let you rewrite the final commit message. happily it looks like re-reviews arent needed for a rebase with the same base |
monorepo so that stake tests can pass.
monorepo so that stake tests can pass.
Problem
#7203 removed all usage of the native stake program itself in the agave codebase, switching the builtin to bpf stake, pulling the core bpf repo for genesis, and using a bpf stake binary for tests. #7645 deleted the native stake program implementation, in other words virtually all stake program code
to keep the enormous diff of #7645 trivial, we left behind a few utility functions that were used in other parts of the codebase. this pr removes
solana-stake-programentirelySummary of Changes
points.rswas already copied to runtime by someone other than myself. this is deleted with no additionsconfig.rsandepoch_rewards.rsonly exist to set up these accounts for genesis. this functionality is moved toruntime::genesis_utilssince it is lowest on the dependency tree compared togenesisandgenesis-utilsstake_state.rsdeserialization wrappers are mostly deleted and replaced with standard deserialize calls. a few are moved where they are used frequently and locallystake_state::create_account()is moved to a newruntime::stake_utilsmodule. this is used in many places, the logic is very annoying, and it is only required for tests and cli so it shouldnt be added to stake interface. it is also copy-pasted toaccounts-dbbehind dcou, the one place that cannot import runtimeget_minimum_delegation()is also added toruntime::stake_utils. this is a trivial function that accepts a bool and returns 1 or 1*10^9. the main reason to keep it is that when we implement minimum delegation (under a new feature flag), this marks the places in agave that care about itsolana-stake-programtosolana-stake-interfacesolana-stake-programin its entiretyUpgrade Path
after this lands, we will be free to reuse the
solana-stake-programname on crates io for the bpf stake programour timing here is actually impeccable. native stake reexports stake interface, so removing it is a major breaking change for downstream. but the entire package was marked unstable in agave 3.x, and master is now on 4.0 alpha. this means we can land this pr now with no issues
bpf stake is presently on 1.0, with a version bump (to 2.0 or 1.1 im not sure) planned. after this pr lands, we must skip bpf stake at least to 4.0 and then we can push to crates io. if any 4.x agave release hits crates io before this pr is merged, we should skip bpf stake to 5.0