Skip to content

Add RewardType::PartitionData and record at calculation#1322

Closed
CriesofCarrots wants to merge 7 commits intoanza-xyz:masterfrom
CriesofCarrots:simd-118-partition-reward-type
Closed

Add RewardType::PartitionData and record at calculation#1322
CriesofCarrots wants to merge 7 commits intoanza-xyz:masterfrom
CriesofCarrots:simd-118-partition-reward-type

Conversation

@CriesofCarrots
Copy link
Copy Markdown

Problem

When the partitioned rewards feature is activated, the only way for RPC and clients to find the rewards for a specific address will be to iterate through blocks at the beginning of each epoch. This is because we do not persist information about how the rewards were partitioned beyond the current epoch. In fact, we can't even tell how long such an iteration will take without knowing how many blocks the rewards distribution spans.

On the abandoned SIMD-106, we ultimately concluded that the best way to provide this data long-term would be to record it in the ledger.

Summary of Changes

Add RewardType::PartitionData
Record a RewardType::PartitionData on initial rewards calculation, ie in the first block of the epoch

This will enable RPC and other client services to find stake rewards by:

  1. Pulling the rewards and metadata from the first block of the epoch
  2. Generating an EpochRewardsHasher from the parent_blockhash in the metadata and the num_partitions in the PartitionData reward

@codecov-commenter
Copy link
Copy Markdown

Codecov Report

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

Project coverage is 81.6%. Comparing base (af6930d) to head (5f7067c).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1322   +/-   ##
=======================================
  Coverage    81.6%    81.6%           
=======================================
  Files         867      868    +1     
  Lines      368900   368962   +62     
=======================================
+ Hits       301052   301109   +57     
- Misses      67848    67853    +5     

@CriesofCarrots CriesofCarrots force-pushed the simd-118-partition-reward-type branch from 5f7067c to 7db9ce2 Compare May 31, 2024 05:44
@CriesofCarrots CriesofCarrots marked this pull request as ready for review May 31, 2024 05:45
@CriesofCarrots CriesofCarrots requested a review from jstarry May 31, 2024 05:45
@jstarry
Copy link
Copy Markdown

jstarry commented Jun 3, 2024

I don't think this data should live in the rewards list personally. Main argument is that this new reward type doesn't share any common fields with other reward types. It's not a reward, it's metadata for rewards. I think it makes sense for this to be added as either metadata in the first block or (obviously more involved) as metadata for an epoch data structure.

@CriesofCarrots
Copy link
Copy Markdown
Author

@jstarry , while I understand that PartitionData is kind of a bastardization of the RewardType type, it doesn't seem like that big a stretch to me, and more importantly, the new type benefits from the existing rewards architecture. If we add partition data as separate block or epoch metadata, we'll need to add a new RocksDb column, which requires various handling for column creation/pruning/backward compatibility, and is yet another separate source that has to be kept in sync for returning block data and uploading to bigtable. That seems like an unnecessarily long pole to me.
We could always rename RewardType, or do something else fancy with types and serialization, if it's primarily the semantics that bother you.

@t-nelson , could you weigh in with your thoughts? You were the one who originally spitballed the RewardType idea

@jstarry
Copy link
Copy Markdown

jstarry commented Jun 4, 2024

Could you add it as a new field to the Rewards struct instead of adding a reward entry?

solana.storage.confirmed_block.rs

#[allow(clippy::derive_partial_eq_without_eq)]
#[derive(Clone, PartialEq, ::prost::Message)]
pub struct Rewards {
    #[prost(message, repeated, tag = "1")]
    pub rewards: ::prost::alloc::vec::Vec<Reward>,
    // num_partitions here
}

@CriesofCarrots
Copy link
Copy Markdown
Author

Could you add it as a new field to the Rewards struct instead of adding a reward entry?

Hmm, not a bad idea. That seems possible. I'll work up a poc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants