Skip to content

Conversation

@CriesofCarrots
Copy link

@CriesofCarrots CriesofCarrots commented Mar 29, 2024

Problem

Code for partitioned epoch rewards is peppered throughout bank.rs. It is very hard to trace the flows, and figure out which structs and methods are needed for calculation vs distribution, or for sysvar handling.

Summary of Changes

This is the first of 5 reorg/refactoring PRs.
There are no functional changes in this PR, although some pub declarations may look different due to module nesting.
This is best reviewed by commit.

Eventually, partitioned_epoch_rewards module will look like this:

partitioned_epoch_rewards >
    calculation.rs
    compare.rs
    distribution.rs
    mod.rs
    sysvar.rs

The complete change is visible in a draft PR

@CriesofCarrots
Copy link
Author

frozen_abi change should be fine. Diff:

<             element solana_runtime::bank::EpochRewardStatus
---
>             element solana_runtime::bank::partitioned_epoch_rewards::EpochRewardStatus
2084c2084
<                     variant(0) Active(solana_runtime::bank::StartBlockHeightAndRewards) (newtype)
---
>                     variant(0) Active(solana_runtime::bank::partitioned_epoch_rewards::StartBlockHeightAndRewards) (newtype)

@codecov-commenter
Copy link

codecov-commenter commented Mar 30, 2024

Codecov Report

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

Project coverage is 81.8%. Comparing base (79e316e) to head (4d668bb).
Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #511   +/-   ##
=======================================
  Coverage    81.8%    81.8%           
=======================================
  Files         843      844    +1     
  Lines      228586   228589    +3     
=======================================
+ Hits       187124   187138   +14     
+ Misses      41462    41451   -11     

@CriesofCarrots CriesofCarrots changed the title Reorganize partitioned epoch rewards runtime code, 1 of 4 Reorganize partitioned epoch rewards runtime code, 1 of 5 Apr 1, 2024
@CriesofCarrots
Copy link
Author

CriesofCarrots commented Apr 1, 2024

Sorry, dumb accidental push. No changes since passed CI on Friday.
(edit) I went ahead and rebased on master to make sure there are no staleness issues, since I stupidly restarted CI anyway

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!


pub(crate) type StakeRewards = Vec<StakeReward>;

impl Bank {

Choose a reason for hiding this comment

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

I'll be honest, I don't love the impl Bank blocks in multiple files, because I'm used to just having one file open and jumping to function definitions within the same file. When possible, I prefer encapsulating the additional functionality in new types.

But in this case, where there's a lot of the Bank required, encapsulation would seriously complicate the scope of the refactor.

The only other options I came up with end up looking very similar (or worse) to multiple impl blocks:

  • define functions that take the Bank and anything else required, but that ends up pretty much the same, and maybe worse because more fields might need to be exposed
  • define a trait and have Bank implement it, but that's also worse since those functions become public

All that to say: it looks like the best option. And the fee distribution code has a separate impl Bank block, so there's some precedent. I'll just get better at navigating code 😅

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for your thoughts on this and sharing some alternative approaches! I definitely understand your perspective, but given how enormously sprawling Bank has become, I do think the benefits of this organizational approach outweigh the drawbacks (...and yeah, Justin and I have done some of this for other Bank functionality already 💦 )

@CriesofCarrots CriesofCarrots merged commit c29a239 into anza-xyz:master Apr 2, 2024
OliverNChalk pushed a commit to OliverNChalk/agave that referenced this pull request Nov 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

No open projects

Development

Successfully merging this pull request may close these issues.

3 participants