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: add recalculation method and use when booting from snapshot #1159

Merged

Conversation

CriesofCarrots
Copy link

@CriesofCarrots CriesofCarrots commented May 3, 2024

Problem

As per SIMD-0118, partitioned rewards should be recalculated on boot from snapshot. Partition and initial calculation info is available in the EpochRewards sysvar, and as of #1138, Bank::get_epoch_reward_calculate_param_info provides vote-account credits_observed from the EpochStakes cache. This is everything needed to recalculate undistributed stake-reward partitions.

Summary of Changes

Add Bank::recalculate_partitioned_rewards method. Recalculated partitions could differ from the original calculation in two ways:

  1. Block partitions that have already been rewarded will be empty. This is because stake-account state is pulled from the StakesCache, which is modified as distribution occurs. (Specifically, stake credits_observed are updated to the new amount, so subsequent calculations determine no distribution needed.) Since only upcoming partitions need action (distribution), this is fine.
  2. Rewards within a particular partition may appear in a different order. The runtime makes no guarantees about reward order, either in actual account updates or in block metadata storage, so this is fine.

Use recalculate_partitioned_rewards in Bank::new_from_fields to populate Bank::epoch_reward_status

@CriesofCarrots CriesofCarrots force-pushed the simd-118-recalc-from-snap branch 2 times, most recently from d411cdd to a547820 Compare May 3, 2024 20:58
@CriesofCarrots CriesofCarrots marked this pull request as ready for review May 3, 2024 21:01
@codecov-commenter
Copy link

codecov-commenter commented May 3, 2024

Codecov Report

Attention: Patch coverage is 98.81890% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 82.1%. Comparing base (4ae2ca1) to head (f138560).
Report is 12 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff            @@
##           master    #1159    +/-   ##
========================================
  Coverage    82.1%    82.1%            
========================================
  Files         886      886            
  Lines      236439   236581   +142     
========================================
+ Hits       194252   194379   +127     
- Misses      42187    42202    +15     

point_value,
thread_pool,
reward_calc_tracer,
&mut RewardsMetrics::default(), // This is required, but not reporting anything at the moment
Copy link

Choose a reason for hiding this comment

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

Seems useful to report these metrics, are you planning to add this later?

Copy link
Author

Choose a reason for hiding this comment

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

Yep. I want to take a look at the metrics separately with the whole implementation in mind. The reporting cadence will need to change, and I don't think all the fields will still be relevant.

runtime/src/bank/partitioned_epoch_rewards/calculation.rs Outdated Show resolved Hide resolved
runtime/src/bank/partitioned_epoch_rewards/calculation.rs Outdated Show resolved Hide resolved
runtime/src/bank/partitioned_epoch_rewards/calculation.rs Outdated Show resolved Hide resolved
runtime/src/bank/partitioned_epoch_rewards/calculation.rs Outdated Show resolved Hide resolved
runtime/src/bank.rs Show resolved Hide resolved
@CriesofCarrots CriesofCarrots force-pushed the simd-118-recalc-from-snap branch from a547820 to dc3d2b2 Compare May 7, 2024 01:03
@CriesofCarrots
Copy link
Author

Rebased to pick up tip of master; changes all in the new, last 4 commits

@CriesofCarrots CriesofCarrots requested a review from jstarry May 7, 2024 01:05
jstarry
jstarry previously approved these changes May 7, 2024
joncinque
joncinque previously approved these changes May 7, 2024
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.

Looks great! Just a couple of tiny things, can be addressed separately if more convenient

@CriesofCarrots CriesofCarrots dismissed stale reviews from joncinque and jstarry via 1e8d4d4 May 7, 2024 23:14
@CriesofCarrots CriesofCarrots force-pushed the simd-118-recalc-from-snap branch from 1e8d4d4 to f138560 Compare May 7, 2024 23:16
@CriesofCarrots
Copy link
Author

Looks great! Just a couple of tiny things, can be addressed separately if more convenient

Thanks! Went ahead and pushed those here as the last commit. Rebasing to ensure nothing weird upstream.

@CriesofCarrots CriesofCarrots requested a review from joncinque May 7, 2024 23:16
@CriesofCarrots CriesofCarrots merged commit 9b1bae0 into anza-xyz:master May 8, 2024
38 checks passed
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