solana icon indicating copy to clipboard operation
solana copied to clipboard

Add rpc support for partitioned rewards

Open CriesofCarrots opened this issue 1 year ago • 8 comments

Problem

Since https://github.com/solana-labs/solana/pull/34624, we can pull the PartitionData PDA to create the hasher to locate specific rewards.

Summary of Changes

  • Add branch for partitioned rewards in get_inflation_rewards
  • Remove feature deactivation in solana-test-validator

CriesofCarrots avatar Jan 12 '24 20:01 CriesofCarrots

Codecov Report

Attention: 113 lines in your changes are missing coverage. Please review.

Comparison is base (b04765f) 81.6% compared to head (89fbcbb) 81.6%. Report is 7 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##           master   #34773     +/-   ##
=========================================
- Coverage    81.6%    81.6%   -0.1%     
=========================================
  Files         828      830      +2     
  Lines      224339   224721    +382     
=========================================
+ Hits       183274   183482    +208     
- Misses      41065    41239    +174     

codecov[bot] avatar Jan 12 '24 22:01 codecov[bot]

While I want to complete MNB-representative testing before merge, could you reviewers start looking at this code? 🙏

CriesofCarrots avatar Jan 16 '24 17:01 CriesofCarrots

I went through the code. The algorithm logic looks good to me! Just one comment.

HaoranYi avatar Jan 16 '24 19:01 HaoranYi

Just one comment.

I don't see a comment @HaoranYi

CriesofCarrots avatar Jan 16 '24 20:01 CriesofCarrots

Sorry. Forget to click "submit". Now submitted.

HaoranYi avatar Jan 16 '24 20:01 HaoranYi

"get all of epoch E's rewards" use case

We don't have a method that does this currently, except for I suppose getBlock on the first block of the epoch. So I would lean toward a client-side implementation. We could add something to solana_rpc_client or solana_cli as a reference implementation, if you like.

CriesofCarrots avatar Jan 17 '24 22:01 CriesofCarrots

CI failure is that h2 audit error. Will rebase when fix is available.

CriesofCarrots avatar Jan 17 '24 22:01 CriesofCarrots

except for I suppose getBlock on the first block of the epoch

yeah i should've mentioned that this implicit implementation is what i was talking about. client-side is fine (preferred even), just kinda annoying that the runtime and client code is so divergent that we'll basically be fully replicoding this logic

t-nelson avatar Jan 18 '24 03:01 t-nelson

I tested this on a solana-test-validator and a client spinning up 50k-100k stake accounts. I rekeyed the feature to test pre- and post-activation. Caught 2 bugs: https://github.com/solana-labs/solana/pull/34913, and and off-by-one error here It's now testing well.

I'd like to run one test with mnb-levels of stake accounts (we're working on getting that set up on pop-sim), but in the meantime, I think this is ready for final review.

CriesofCarrots avatar Jan 23 '24 21:01 CriesofCarrots

Rebased to pick up #34934 and removed commit touching EpochRewardsHasher

CriesofCarrots avatar Jan 24 '24 23:01 CriesofCarrots

No changes; just rebased to pick up the local-cluster fix

CriesofCarrots avatar Jan 25 '24 18:01 CriesofCarrots

Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis.

mergify[bot] avatar Jan 25 '24 19:01 mergify[bot]