feat(extractors/solana): add block rewards

[sistemd]

Closes #1629

[claude]

Code Review Summary

This PR adds support for extracting Solana block rewards, which is a valuable addition to the data extraction pipeline. The implementation follows the existing patterns in the codebase well.

Key Changes Reviewed

• New block_rewards table module with proper Arrow schema definition
• Integration of block rewards into both Old Faithful (OF1) and RPC code paths
• Refactoring of Reward and RewardType types to be shared between block rewards and transaction rewards
• Improved error handling with anyhow::Result return types where parsing can fail

Areas for Consideration

1. Error context accuracy: Some .context() messages mention "missing" rewards when the actual failure modes are about malformed data
2. Builder capacity hints: The BlockRewardsRowsBuilder uses capacity of 1, which may cause unnecessary reallocations for blocks with many rewards
3. Node lookup ordering: The change from .pop() to .last() in of1_client.rs assumes consistent iteration order from the node map
4. Error variant semantics: Using MissingNode error when the node exists but is the wrong type could be confusing

What Looks Good

• Clean separation of block rewards into its own module
• Proper handling of both Proto and Bincode formats
• Good slot mismatch validation for data integrity
• Consistent use of the ok_or_bail! macro for stream error handling
• Test manifest updated appropriately

[claude]

The From<solana_storage_proto::StoredExtendedReward> implementation here uses value.reward_type.map(Into::into), but value.reward_type is already Option<solana_transaction_status::RewardType>.

Looking at the impl below for From<rpc_client::RewardType>, it handles the standard Solana reward types (Fee, Rent, Staking, Voting), but there's no From<solana_transaction_status::RewardType> implementation defined.

This will cause a compile error unless there's an implicit conversion or blanket impl I'm missing. Could you verify this compiles correctly, or add the missing From impl for solana_transaction_status::RewardType?

[claude]

Minor concern: The builder's initial capacity of 1 for each field seems too small for the typical use case where a block has multiple rewards. Consider accepting a capacity hint parameter or using a larger default:

pub(crate) fn with_capacity(num_rewards: usize) -> Self {
    Self {
        special_block_num: UInt64Builder::with_capacity(num_rewards),
        slot: UInt64Builder::with_capacity(num_rewards),
        // ...
    }
}

This would help avoid reallocations when appending rewards, similar to how TransactionRowsBuilder::with_capacity works.

[claude]

The error context here says "missing of1 block rewards", but from_of1_rewards handles Option<DecodedBlockRewards> and returns Ok(Self { slot, rewards: vec![] }) when rewards are None.

So this function doesn't actually fail on missing rewards - it gracefully handles them. The .context() call at the call site in extractor.rs:369 is misleading. Consider either:

1. Removing the context from the call site since the error wouldn't be about "missing" rewards
2. Or documenting that errors here are about malformed rewards data, not missing rewards

[claude]

There's a potential edge case here: nodes.nodes.last() returns a reference to the last element in iteration order of the HashMap (or whatever map type Nodes uses internally). This assumes the block node is always the last one added during read_until_block.

Previously with .pop(), the code was explicitly removing and consuming the last element from a mutable nodes. Now with .last(), you're relying on iteration order being consistent.

If Nodes uses a HashMap, the iteration order is not guaranteed and could vary. Could you confirm that Nodes::nodes preserves insertion order (e.g., uses IndexMap or similar)?

[claude]

The slot mismatch check is a good data integrity validation. However, the error message could be more actionable by including the CID or other context about which block/rewards node caused the mismatch:

#[error("reward slot mismatch for cid {cid}: expected {expected}, found {found}")]
RewardSlotMismatch { expected: u64, found: u64, cid: String },

This would help with debugging if this error occurs in production.

[claude]

The error variant for when the node exists but isn't a Rewards node reuses MissingNode, which is semantically incorrect - the node isn't missing, it's the wrong type. This could be confusing for debugging.

Consider using UnexpectedNode instead (similar to line 570-573), or adding a dedicated WrongNodeType error variant:

let Some(car_parser::node::Node::Rewards(rewards)) = nodes.nodes.get(&block.rewards) else {
    return Err(Of1StreamError::UnexpectedNode {
        kind: "non-rewards",
        cid: block.rewards.to_string(),
    });
};

[claude]

In from_stored_meta, the rewards conversion manually constructs tables::block_rewards::Reward with reward_type: reward.reward_type.map(From::from).

However, rpc_client::Reward already has a From impl into tables::block_rewards::Reward (line 106-115 in block_rewards.rs). Consider using that for consistency:

let rewards = rpc_tx_meta.rewards.map(|rewards| {
    rewards.into_iter().map(Into::into).collect()
});

This matches the pattern used in from_rpc_meta (lines 421-423) and reduces code duplication.

[claude]

The ok_or_bail! macro is a useful pattern for stream error handling. However, defining it inside the async_stream::stream! block means it's recreated for every stream instantiation.

Consider moving this to a module-level macro or extracting the error-and-return pattern into a helper. That said, macro hygiene within async_stream::stream! can be tricky, so the current approach is pragmatic if moving it causes issues.

[claude]

The error context "missing of1 block rewards" is misleading. Looking at BlockRewards::from_of1_rewards, it accepts Option<DecodedBlockRewards> and returns an empty rewards list when None is passed.

The actual errors that could occur here are related to malformed reward data (e.g., invalid reward type values, unparseable commission strings). Consider a more accurate context:

let block_rewards = tables::block_rewards::BlockRewards::from_of1_rewards(slot, block_rewards)
    .context("parsing of1 block rewards")?;

[claude]

For blocks with many rewards, you could pass a capacity hint to the builder:

let block_rewards_row = {
    let mut builder = block_rewards::BlockRewardsRowsBuilder::with_capacity(block_rewards.rewards.len());
    // ...
};

This would require adding a with_capacity method to BlockRewardsRowsBuilder as mentioned in the other comment.