Add rpc support for partitioned rewards #34773

CriesofCarrots · 2024-01-12T20:46:42Z

Problem

Since #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

codecov · 2024-01-12T22:41:14Z

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

CriesofCarrots · 2024-01-16T17:43:06Z

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

HaoranYi · 2024-01-16T19:38:08Z

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

CriesofCarrots · 2024-01-16T20:25:58Z

Just one comment.

I don't see a comment @HaoranYi

rpc/src/rpc.rs

HaoranYi · 2024-01-16T20:29:01Z

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

t-nelson

looking like a good start!

i imagine we'll need some way to ease the "get all of epoch E's rewards" use case. not sure if it makes more sense as

a hack for the case of addresses.is_empty()
its own method
a client-side version of similar logic (would be cool if we could maximally share code)

i'm kinda leaning toward (3) atm

rpc/src/rpc.rs

t-nelson · 2024-01-17T05:45:17Z

rpc/src/rpc.rs

+                    .unwrap_or_default()
+                    .into_iter()
+                    .filter_map(|reward| match reward.reward_type? {
+                        RewardType::Staking | RewardType::Voting => addresses


and only staking rewards here

This one can be changed, yes.

Given the dereplicode refactoring, I'm not sure this is worth doing. It's a little prickly to match only one or the other type after feature activation, but both before.

It's a little prickly to match only one or the other type after feature activation, but both before.

mmm... is it tho? split the filter_map() to a discrete filter() and map(), pass in different filter closures, works no?

Fine, I've pushed that. I don't think it gains us much, though, since after feature activation there won't be any RewardType::Staking rewards in the first block to check.

rpc/src/rpc.rs

CriesofCarrots · 2024-01-17T22:04:46Z

"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 · 2024-01-17T22:29:04Z

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

t-nelson · 2024-01-18T03:48:07Z

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

rpc/src/rpc.rs

CriesofCarrots · 2024-01-23T21:41:45Z

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: #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.

rpc/src/rpc.rs

runtime/src/epoch_rewards_hasher.rs

CriesofCarrots · 2024-01-24T23:58:36Z

Rebased to pick up #34934 and removed commit touching EpochRewardsHasher

HaoranYi

Good job! lgtm!

…h must exist for partiion PDA to exist

… catch Voting rewards

…8098)

CriesofCarrots · 2024-01-25T18:24:25Z

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

mergify · 2024-01-25T19:31:40Z

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.

* Check feature_set for enable_partitioned_epoch_reward * Keep common variable outside if case * Keep common early return out of if case, since the first_slot_in_epoch must exist for partiion PDA to exist * Get and parse epoch partition data PDA * Find partition index for all addresses * Pull relevant blocks and get rewards * Reuse ordering and reformatting * Remove feature deactivation from TestValidator * Restore rewards iteration in first block in epoch for feature case to catch Voting rewards * Add fn get_reward_map helper to dedupe code * No need to start 2nd get_block_with_limit call with first block again * Replace filter_map to parameterize RewardType filter expression * Weird thing to make clippy and compiler agree (rust-lang/rust-clippy#8098) * Use activated_slot to ensure the right approach for past rewards epochs (cherry picked from commit 22500c2)

…34960) Add rpc support for partitioned rewards (#34773) * Check feature_set for enable_partitioned_epoch_reward * Keep common variable outside if case * Keep common early return out of if case, since the first_slot_in_epoch must exist for partiion PDA to exist * Get and parse epoch partition data PDA * Find partition index for all addresses * Pull relevant blocks and get rewards * Reuse ordering and reformatting * Remove feature deactivation from TestValidator * Restore rewards iteration in first block in epoch for feature case to catch Voting rewards * Add fn get_reward_map helper to dedupe code * No need to start 2nd get_block_with_limit call with first block again * Replace filter_map to parameterize RewardType filter expression * Weird thing to make clippy and compiler agree (rust-lang/rust-clippy#8098) * Use activated_slot to ensure the right approach for past rewards epochs (cherry picked from commit 22500c2) Co-authored-by: Tyera <tyera@solana.com>

This reverts commit 22500c2.

This reverts commit 22500c2. (cherry picked from commit f01f361)

CriesofCarrots force-pushed the rpc-partitioned-rewards branch 2 times, most recently from 184dcfc to 8575572 Compare January 12, 2024 21:20

CriesofCarrots marked this pull request as ready for review January 16, 2024 17:40

CriesofCarrots requested review from HaoranYi and t-nelson January 16, 2024 17:41

HaoranYi reviewed Jan 16, 2024

View reviewed changes

rpc/src/rpc.rs Outdated Show resolved Hide resolved

CriesofCarrots force-pushed the rpc-partitioned-rewards branch from 8575572 to 569e79a Compare January 17, 2024 01:41

t-nelson reviewed Jan 17, 2024

View reviewed changes

CriesofCarrots force-pushed the rpc-partitioned-rewards branch from c2b1d6a to 771df04 Compare January 17, 2024 22:07

CriesofCarrots force-pushed the rpc-partitioned-rewards branch from 771df04 to 766a416 Compare January 18, 2024 02:02

CriesofCarrots force-pushed the rpc-partitioned-rewards branch from 8976385 to a56e364 Compare January 18, 2024 19:13

CriesofCarrots commented Jan 23, 2024

View reviewed changes

rpc/src/rpc.rs Outdated Show resolved Hide resolved

CriesofCarrots force-pushed the rpc-partitioned-rewards branch from bf01475 to 9fdb774 Compare January 23, 2024 21:35

CriesofCarrots requested review from t-nelson and HaoranYi January 24, 2024 03:14

t-nelson previously approved these changes Jan 24, 2024

View reviewed changes

rpc/src/rpc.rs Show resolved Hide resolved

rpc/src/rpc.rs Show resolved Hide resolved

rpc/src/rpc.rs Show resolved Hide resolved

runtime/src/epoch_rewards_hasher.rs Outdated Show resolved Hide resolved

CriesofCarrots mentioned this pull request Jan 24, 2024

Move EpochRewardsHasher to solana-sdk #34934

Merged

CriesofCarrots dismissed t-nelson’s stale review via f367f12 January 24, 2024 23:56

CriesofCarrots force-pushed the rpc-partitioned-rewards branch from 9fdb774 to f367f12 Compare January 24, 2024 23:56

HaoranYi approved these changes Jan 25, 2024

View reviewed changes

CriesofCarrots added 14 commits January 25, 2024 11:24

Check feature_set for enable_partitioned_epoch_reward

2c4883f

Keep common variable outside if case

6f22f90

Keep common early return out of if case, since the first_slot_in_epoc…

fdfb7f8

…h must exist for partiion PDA to exist

Get and parse epoch partition data PDA

388dd37

Find partition index for all addresses

6b52fc6

Pull relevant blocks and get rewards

e1a5d37

Reuse ordering and reformatting

edfe08b

Remove feature deactivation from TestValidator

01626c9

Restore rewards iteration in first block in epoch for feature case to…

8107e6c

… catch Voting rewards

Add fn get_reward_map helper to dedupe code

a1d2692

No need to start 2nd get_block_with_limit call with first block again

8050768

Replace filter_map to parameterize RewardType filter expression

4ad1470

Weird thing to make clippy and compiler agree (rust-lang/rust-clippy#…

0cc2104

…8098)

Use activated_slot to ensure the right approach for past rewards epochs

89fbcbb

CriesofCarrots force-pushed the rpc-partitioned-rewards branch from f367f12 to 89fbcbb Compare January 25, 2024 18:24

CriesofCarrots requested review from t-nelson and HaoranYi January 25, 2024 18:24

HaoranYi approved these changes Jan 25, 2024

View reviewed changes

CriesofCarrots added the v1.18 PRs that should be backported to v1.18 label Jan 25, 2024

CriesofCarrots merged commit 22500c2 into solana-labs:master Jan 25, 2024
36 checks passed

mergify bot mentioned this pull request Jan 25, 2024

v1.18: Add rpc support for partitioned rewards (backport of #34773) #34960

Merged

CriesofCarrots added a commit to CriesofCarrots/solana that referenced this pull request Feb 2, 2024

Revert "Add rpc support for partitioned rewards (solana-labs#34773)"

31aebca

This reverts commit 22500c2.

CriesofCarrots added a commit that referenced this pull request Feb 6, 2024

Revert "Add rpc support for partitioned rewards (#34773)"

f01f361

This reverts commit 22500c2.

mergify bot pushed a commit that referenced this pull request Feb 6, 2024

Revert "Add rpc support for partitioned rewards (#34773)"

a80df7c

This reverts commit 22500c2. (cherry picked from commit f01f361)

CriesofCarrots added a commit that referenced this pull request Feb 7, 2024

Revert "Add rpc support for partitioned rewards (#34773)"

36238fb

This reverts commit 22500c2. (cherry picked from commit f01f361)

willhickey mentioned this pull request Mar 28, 2024

v1.18 commits - please ignore anza-xyz/agave#475

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add rpc support for partitioned rewards #34773

Add rpc support for partitioned rewards #34773

CriesofCarrots commented Jan 12, 2024

codecov bot commented Jan 12, 2024 •

edited

Loading

CriesofCarrots commented Jan 16, 2024

HaoranYi commented Jan 16, 2024

CriesofCarrots commented Jan 16, 2024

HaoranYi commented Jan 16, 2024

t-nelson left a comment

t-nelson Jan 17, 2024

CriesofCarrots Jan 17, 2024

CriesofCarrots Jan 17, 2024

t-nelson Jan 18, 2024

CriesofCarrots Jan 18, 2024

CriesofCarrots commented Jan 17, 2024

CriesofCarrots commented Jan 17, 2024

t-nelson commented Jan 18, 2024

CriesofCarrots commented Jan 23, 2024

CriesofCarrots commented Jan 24, 2024

HaoranYi left a comment

CriesofCarrots commented Jan 25, 2024

mergify bot commented Jan 25, 2024

Add rpc support for partitioned rewards #34773

Add rpc support for partitioned rewards #34773

Conversation

CriesofCarrots commented Jan 12, 2024

Problem

Summary of Changes

codecov bot commented Jan 12, 2024 • edited Loading

Codecov Report

CriesofCarrots commented Jan 16, 2024

HaoranYi commented Jan 16, 2024

CriesofCarrots commented Jan 16, 2024

HaoranYi commented Jan 16, 2024

t-nelson left a comment

Choose a reason for hiding this comment

t-nelson Jan 17, 2024

Choose a reason for hiding this comment

CriesofCarrots Jan 17, 2024

Choose a reason for hiding this comment

CriesofCarrots Jan 17, 2024

Choose a reason for hiding this comment

t-nelson Jan 18, 2024

Choose a reason for hiding this comment

CriesofCarrots Jan 18, 2024

Choose a reason for hiding this comment

CriesofCarrots commented Jan 17, 2024

CriesofCarrots commented Jan 17, 2024

t-nelson commented Jan 18, 2024

CriesofCarrots commented Jan 23, 2024

CriesofCarrots commented Jan 24, 2024

HaoranYi left a comment

Choose a reason for hiding this comment

CriesofCarrots commented Jan 25, 2024

mergify bot commented Jan 25, 2024

codecov bot commented Jan 12, 2024 •

edited

Loading