-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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 test_next_leader_slot_next_epoch() aware of stake minimum delegation #24660
Conversation
@@ -19,7 +19,7 @@ pub fn add_genesis_accounts(genesis_config: &mut GenesisConfig) -> u64 { | |||
/// NOTE: This is also used to calculate the minimum balance of a stake account, which is the | |||
/// rent exempt reserve _plus_ the minimum stake delegation. | |||
#[inline(always)] | |||
pub(crate) fn get_minimum_delegation(_feature_set: &FeatureSet) -> u64 { | |||
pub fn get_minimum_delegation(_feature_set: &FeatureSet) -> u64 { |
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 moving this into solana-program, so we don’t need a new dep in ledger/
When it’s moved into solana-program, we could probably remove the FeatureSet as well (since that’s only available in solana-sdk) and instead just always use 1 SOL
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.
@mvines Sorry 😢 I already merged this specific part via #24659. I like this suggestion though as it sounds like it'll simplify a number of testing/impl details. So, going forward I could:
- Revert Make rpc test_account_subscribe aware of stake minimum delegation #24659, then move
get_minimum_delegation()
, then re-do/fix these tests - Merge this PR (which will end up not having this specific change since it has already been merged), then move
get_minimum_delegation()
- Move
get_minimum_delegation()
, then fixup this PR, then redo Make rpc test_account_subscribe aware of stake minimum delegation #24659
Do you have a preference here? I feel like (2) is going to be the fastest and most straight forward. Maybe there's another option too?
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.
(2), or whatever is easiest 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.
Tracking this in #24690
Codecov Report
@@ Coverage Diff @@
## master #24660 +/- ##
===========================================
+ Coverage 70.0% 82.0% +12.0%
===========================================
Files 36 595 +559
Lines 2255 165075 +162820
Branches 322 0 -322
===========================================
+ Hits 1580 135491 +133911
- Misses 560 29584 +29024
+ Partials 115 0 -115 |
Problem
The ledger test
test_next_leader_slot_next_epoch()
fails once the stake minimum delegation is raised. See #24603 for more context.Summary of Changes
Makeget_minimum_delegation
public