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

Simd 118: use vote state from EpochStakes in calculation #1138

Merged

Conversation

CriesofCarrots
Copy link

@CriesofCarrots CriesofCarrots commented May 1, 2024

Problem

Stake-reward calculation methods assume that the caller is interested in existing VoteState (specifically, the last epoch in the VoteState::epoch_credits list). This is only true when rewards are calculated at the epoch-boundary block. To support recalculation in later blocks, calculation needs to have access to the VoteState such as it was at the epoch boundary.

The VoteState itself contains previous epoch-credit information, and #1102 was a way to expose this data to calculation methods. However, that approach requires that vote accounts never be destroyed during the period in which recalculation might occur, which would probably necessitate a change to the Vote Program.

Meanwhile, EpochStakes cached in the bank and snapshots already preserves vote account state from the previous epoch.

Summary of Changes

Use the vote accounts from EpochStakes in partitioned epoch rewards calculation.
Note: this works for epoch-boundary calculation because EpochStakes is updated before calculation:

agave/runtime/src/bank.rs

Lines 1423 to 1435 in be80b92

// Save a snapshot of stakes for use in consensus and stake weighted networking
let leader_schedule_epoch = self.epoch_schedule.get_leader_schedule_epoch(slot);
let (_, update_epoch_stakes_time) = measure!(
self.update_epoch_stakes(leader_schedule_epoch),
"update_epoch_stakes",
);
let mut rewards_metrics = RewardsMetrics::default();
// After saving a snapshot of stakes, apply stake rewards and commission
let (_, update_rewards_with_thread_pool_time) = measure!(
{
if self.is_partitioned_rewards_code_enabled() {
self.begin_partitioned_rewards(

Currently includes a check that the operative stakes (from StakesCache) are identical to those from the EpochStakes. This isn't important for the existing implementation, where calculation is still all in the epoch-boundary block and EpochStakes and StakesCache are therefore equivalent, but will be helpful for recalculation. If you'd like me to punt this bit to the next PR, which adds recalculation, let me know. Removed

Closes #1102

@CriesofCarrots CriesofCarrots force-pushed the simd-118-epoch-stakes branch from c7f69d9 to 516a54b Compare May 1, 2024 18:44
@codecov-commenter
Copy link

codecov-commenter commented May 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.1%. Comparing base (79ae84e) to head (42d5463).
Report is 5 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##           master    #1138     +/-   ##
=========================================
- Coverage    82.1%    82.1%   -0.1%     
=========================================
  Files         880      880             
  Lines      235632   235653     +21     
=========================================
+ Hits       193649   193654      +5     
- Misses      41983    41999     +16     

@CriesofCarrots
Copy link
Author

CriesofCarrots commented May 1, 2024

@joncinque , thanks for all your time on #1102 and sorry for this whiplash 😕 Justin and I talked some on that PR and a little bit in DM and think this is a better direction than using the StakesCache vote accounts for recalculation. He is also investigating updating EpochStakes to store the full stake_state::Stake, which would enable using that as the source of stake accounts for calculation as well.
Happy to sync up with you if it would be helpful to have more detail or context!

@CriesofCarrots CriesofCarrots requested a review from jstarry May 2, 2024 16:30
@CriesofCarrots CriesofCarrots merged commit 2d24030 into anza-xyz:master May 3, 2024
38 checks passed
Copy link

@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.

Sorry for the lateness on this, but looks good to me

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

Successfully merging this pull request may close these issues.

4 participants