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

Populate EpochRewards with correct data #763

Merged

Conversation

CriesofCarrots
Copy link

@CriesofCarrots CriesofCarrots commented Apr 11, 2024

Problem

Bank::begin_partitioned_rewards() creates the EpochRewards sysvar with 3 default values, even though the data is all available at or after calculation.

Summary of Changes

Populate the remainder of EpochRewards with real data. Namely, these fields: parent_blockhash, num_partitions, and total_points.
Exposing total_points requires an unfortunate amount of helper-struct plumbing, with the current calculation architecture. We could look at refactoring this, perhaps as part of the recalculation work.

✅ Rebased Sadly, because this touches the same dependency lists, this will have some dumb conflicts with #760. I'm happy to merge in either order.

@codecov-commenter
Copy link

codecov-commenter commented Apr 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.9%. Comparing base (09241ae) to head (d62bb56).
Report is 8 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #763   +/-   ##
=======================================
  Coverage    81.9%    81.9%           
=======================================
  Files         851      851           
  Lines      231504   231526   +22     
=======================================
+ Hits       189731   189781   +50     
+ Misses      41773    41745   -28     

@CriesofCarrots
Copy link
Author

Rebased

Copy link

@jstarry jstarry left a comment

Choose a reason for hiding this comment

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

One comment for now, will do another pass to look over test coverage later

runtime/src/bank/partitioned_epoch_rewards/sysvar.rs Outdated Show resolved Hide resolved
@CriesofCarrots CriesofCarrots force-pushed the simd-118-correct-sysvar branch from 41b79a1 to f38764f Compare April 13, 2024 05:55
@CriesofCarrots
Copy link
Author

No changes in rebase aside from new last commit

jstarry
jstarry previously approved these changes Apr 13, 2024
Copy link

@jstarry jstarry left a comment

Choose a reason for hiding this comment

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

Took another pass and it looks good! Just the last comment about the parent blockhash, though what you have is also correct

@CriesofCarrots CriesofCarrots merged commit 180a186 into anza-xyz:master Apr 15, 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, still catching up, but this looks good to me!

michaelschem pushed a commit to michaelschem/agave that referenced this pull request Apr 20, 2024
* Populate EpochRewards::parent_blockhash correctly

* Populate EpochRewards::num_partitions correctly

* Add new internal struct to be extended

* Add CalculateValidatorRewardsResult::total_points

* Add PartitionedRewardsCalculation::total_points

* Add CalculateRewardsAndDistributeVoteRewardsResult::total_points

* Populate EpochRewards::total_points correctly

* Nit: reorder fields to match struct definition

* Use Self::last_blockhash() to get parent_blockhash
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