Skip to content

Commit

Permalink
Simd 118: replace calculation-blocks config value with constant (anza…
Browse files Browse the repository at this point in the history
…-xyz#1251)

* Add const for num calculation blocks

* Use const in partitioned_epoch_rewards module

* Remove unused config-struct field

* Unpub test-only fn

* Preserve single-slot testing framework

* Fix incorrect type alias

* Use const instead of magic number in tests

* Cleanup get_reward_total_num_blocks
  • Loading branch information
CriesofCarrots authored May 10, 2024
1 parent 90cf5f0 commit fadfa61
Show file tree
Hide file tree
Showing 5 changed files with 64 additions and 86 deletions.
28 changes: 7 additions & 21 deletions accounts-db/src/partitioned_rewards.rs
Original file line number Diff line number Diff line change
@@ -1,20 +1,13 @@
//! Code related to partitioned rewards distribution
//!
use solana_sdk::clock::Slot;
#[derive(Debug)]
/// Configuration options for partitioned epoch rewards.
/// This struct allows various forms of testing, especially prior to feature activation.
pub struct PartitionedEpochRewardsConfig {
/// Number of blocks for reward calculation and storing vote accounts.
/// Distributing rewards to stake accounts begins AFTER this many blocks.
/// Normally, this will be 1.
/// if force_one_slot_partitioned_rewards, this will be 0 (ie. we take 0 blocks just for reward calculation)
pub reward_calculation_num_blocks: Slot,
/// number of stake accounts to store in one block during partitioned reward interval
/// normally, this is a number tuned for reasonable performance, such as 4096 accounts/block
/// if force_one_slot_partitioned_rewards, this will usually be u64::MAX so that all stake accounts are written in the first block
pub stake_account_stores_per_block: Slot,
pub stake_account_stores_per_block: u64,
/// if true, end of epoch bank rewards will force using partitioned rewards distribution.
/// see `set_test_enable_partitioned_rewards`
pub test_enable_partitioned_rewards: bool,
Expand All @@ -26,9 +19,6 @@ pub struct PartitionedEpochRewardsConfig {
impl Default for PartitionedEpochRewardsConfig {
fn default() -> Self {
Self {
// reward calculation happens synchronously during the first block of the epoch boundary.
// So, # blocks for reward calculation is 1.
reward_calculation_num_blocks: 1,
// # stake accounts to store in one block during partitioned reward interval
// Target to store 64 rewards per entry/tick in a block. A block has a minimum of 64
// entries/tick. This gives 4096 total rewards to store in one block.
Expand All @@ -47,7 +37,6 @@ pub enum TestPartitionedEpochRewards {
CompareResults,
ForcePartitionedEpochRewardsInOneBlock,
PartitionedEpochRewardsConfigRewardBlocks {
reward_calculation_num_blocks: u64,
stake_account_stores_per_block: u64,
},
}
Expand All @@ -63,12 +52,12 @@ impl PartitionedEpochRewardsConfig {
Self::set_test_enable_partitioned_rewards()
}
TestPartitionedEpochRewards::PartitionedEpochRewardsConfigRewardBlocks {
reward_calculation_num_blocks, stake_account_stores_per_block } => {
Self::set_test_enable_partitioned_rewards_with_custom_number_of_stake_accounts_per_block(
reward_calculation_num_blocks,
stake_account_stores_per_block)
}

stake_account_stores_per_block,
} => {
Self::set_test_enable_partitioned_rewards_with_custom_number_of_stake_accounts_per_block(
stake_account_stores_per_block
)
}
}
}

Expand All @@ -77,7 +66,6 @@ impl PartitionedEpochRewardsConfig {
/// code.
fn set_test_enable_partitioned_rewards() -> Self {
Self {
reward_calculation_num_blocks: 0,
stake_account_stores_per_block: u64::MAX,
test_enable_partitioned_rewards: true,
// irrelevant if we are not running old code path
Expand All @@ -98,11 +86,9 @@ impl PartitionedEpochRewardsConfig {
/// A method that configures how many reward reward calculation blocks and how many stake
/// accounts to store per reward block.
fn set_test_enable_partitioned_rewards_with_custom_number_of_stake_accounts_per_block(
reward_calculation_num_blocks: u64,
stake_account_stores_per_block: u64,
) -> Self {
Self {
reward_calculation_num_blocks,
stake_account_stores_per_block,
test_enable_partitioned_rewards: true,
// irrelevant if we are not running old code path
Expand Down
9 changes: 7 additions & 2 deletions runtime/src/bank/partitioned_epoch_rewards/calculation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use {
CalculateRewardsAndDistributeVoteRewardsResult, CalculateValidatorRewardsResult,
EpochRewardCalculateParamInfo, PartitionedRewardsCalculation, StakeRewardCalculation,
StakeRewardCalculationPartitioned, StakeRewards, VoteRewardsAccounts,
REWARD_CALCULATION_NUM_BLOCKS,
},
crate::{
bank::{
Expand Down Expand Up @@ -60,7 +61,12 @@ impl Bank {

let slot = self.slot();
let distribution_starting_block_height =
self.block_height() + self.get_reward_calculation_num_blocks();
// For live-cluster testing pre-activation
if self.force_partition_rewards_in_first_block_of_epoch() {
self.block_height()
} else {
self.block_height() + REWARD_CALCULATION_NUM_BLOCKS
};

let num_partitions = stake_rewards_by_partition.len() as u64;

Expand Down Expand Up @@ -663,7 +669,6 @@ mod tests {
let mut accounts_db_config: AccountsDbConfig = ACCOUNTS_DB_CONFIG_FOR_TESTING.clone();
accounts_db_config.test_partitioned_epoch_rewards =
TestPartitionedEpochRewards::PartitionedEpochRewardsConfigRewardBlocks {
reward_calculation_num_blocks: 1,
stake_account_stores_per_block,
};

Expand Down
24 changes: 19 additions & 5 deletions runtime/src/bank/partitioned_epoch_rewards/distribution.rs
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,9 @@ mod tests {
use {
super::*,
crate::bank::{
partitioned_epoch_rewards::epoch_rewards_hasher::hash_rewards_into_partitions,
partitioned_epoch_rewards::{
epoch_rewards_hasher::hash_rewards_into_partitions, REWARD_CALCULATION_NUM_BLOCKS,
},
tests::create_genesis_config,
},
rand::Rng,
Expand Down Expand Up @@ -271,7 +273,10 @@ mod tests {

let stake_rewards = hash_rewards_into_partitions(stake_rewards, &Hash::new(&[1; 32]), 2);

bank.set_epoch_reward_status_active(bank.block_height() + 1, stake_rewards);
bank.set_epoch_reward_status_active(
bank.block_height() + REWARD_CALCULATION_NUM_BLOCKS,
stake_rewards,
);

bank.distribute_partitioned_epoch_rewards();
}
Expand All @@ -294,7 +299,10 @@ mod tests {
bank.epoch_schedule().slots_per_epoch as usize + 1,
);

bank.set_epoch_reward_status_active(bank.block_height() + 1, stake_rewards);
bank.set_epoch_reward_status_active(
bank.block_height() + REWARD_CALCULATION_NUM_BLOCKS,
stake_rewards,
);

bank.distribute_partitioned_epoch_rewards();
}
Expand All @@ -304,7 +312,10 @@ mod tests {
let (genesis_config, _mint_keypair) = create_genesis_config(1_000_000 * LAMPORTS_PER_SOL);
let mut bank = Bank::new_for_tests(&genesis_config);

bank.set_epoch_reward_status_active(bank.block_height() + 1, vec![]);
bank.set_epoch_reward_status_active(
bank.block_height() + REWARD_CALCULATION_NUM_BLOCKS,
vec![],
);

bank.distribute_partitioned_epoch_rewards();
}
Expand Down Expand Up @@ -410,7 +421,10 @@ mod tests {

let stake_rewards_bucket =
hash_rewards_into_partitions(stake_rewards, &Hash::new(&[1; 32]), 100);
bank.set_epoch_reward_status_active(bank.block_height() + 1, stake_rewards_bucket.clone());
bank.set_epoch_reward_status_active(
bank.block_height() + REWARD_CALCULATION_NUM_BLOCKS,
stake_rewards_bucket.clone(),
);

// Test partitioned stores
let mut total_rewards = 0;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,10 @@ pub(in crate::bank::partitioned_epoch_rewards) fn hash_rewards_into_partitions(
mod tests {
use {
super::*,
crate::bank::{tests::create_genesis_config, Bank},
crate::bank::{
partitioned_epoch_rewards::REWARD_CALCULATION_NUM_BLOCKS, tests::create_genesis_config,
Bank,
},
solana_accounts_db::stake_rewards::StakeReward,
solana_sdk::{epoch_schedule::EpochSchedule, native_token::LAMPORTS_PER_SOL},
std::collections::HashMap,
Expand Down Expand Up @@ -109,7 +112,10 @@ mod tests {
let stake_rewards_bucket =
hash_rewards_into_partitions(stake_rewards, &Hash::new(&[1; 32]), 10);

bank.set_epoch_reward_status_active(bank.block_height(), stake_rewards_bucket.clone());
bank.set_epoch_reward_status_active(
bank.block_height() + REWARD_CALCULATION_NUM_BLOCKS,
stake_rewards_bucket.clone(),
);

// This call should panic, i.e. 15 is out of the num_credit_blocks
let _range = &stake_rewards_bucket[15];
Expand Down
79 changes: 23 additions & 56 deletions runtime/src/bank/partitioned_epoch_rewards/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ use {
solana_sdk::{
account::AccountSharedData,
account_utils::StateMut,
clock::Slot,
feature_set,
pubkey::Pubkey,
reward_info::RewardInfo,
Expand All @@ -23,6 +22,10 @@ use {
std::sync::Arc,
};

/// Number of blocks for reward calculation and storing vote accounts.
/// Distributing rewards to stake accounts begins AFTER this many blocks.
const REWARD_CALCULATION_NUM_BLOCKS: u64 = 1;

#[derive(AbiExample, Debug, Serialize, Deserialize, Clone, PartialEq)]
struct PartitionedStakeReward {
/// Stake account address
Expand Down Expand Up @@ -175,13 +178,6 @@ impl Bank {
.stake_account_stores_per_block
}

/// reward calculation happens synchronously during the first block of the epoch boundary.
/// So, # blocks for reward calculation is 1.
pub(super) fn get_reward_calculation_num_blocks(&self) -> Slot {
self.partitioned_epoch_rewards_config()
.reward_calculation_num_blocks
}

/// Calculate the number of blocks required to distribute rewards to all stake accounts.
pub(super) fn get_reward_distribution_num_blocks(&self, rewards: &StakeRewards) -> u64 {
let total_stake_accounts = rewards.len();
Expand Down Expand Up @@ -219,7 +215,6 @@ impl Bank {
pub(super) fn force_partition_rewards_in_first_block_of_epoch(&self) -> bool {
self.partitioned_epoch_rewards_config()
.test_enable_partitioned_rewards
&& self.get_reward_calculation_num_blocks() == 0
&& self.partitioned_rewards_stake_account_stores_per_block() == u64::MAX
}
}
Expand Down Expand Up @@ -274,12 +269,6 @@ mod tests {
RewardInterval::OutsideInterval
}
}

/// Return the total number of blocks in reward interval (including both calculation and crediting).
pub(in crate::bank) fn get_reward_total_num_blocks(&self, rewards: &StakeRewards) -> u64 {
self.get_reward_calculation_num_blocks()
+ self.get_reward_distribution_num_blocks(rewards)
}
}

#[test]
Expand All @@ -293,7 +282,10 @@ mod tests {
.map(|_| StakeReward::new_random())
.collect::<Vec<_>>();

bank.set_epoch_reward_status_active(bank.block_height() + 1, vec![stake_rewards]);
bank.set_epoch_reward_status_active(
bank.block_height() + REWARD_CALCULATION_NUM_BLOCKS,
vec![stake_rewards],
);
assert!(bank.get_reward_interval() == RewardInterval::InsideInterval);

bank.force_reward_interval_end_for_tests();
Expand All @@ -310,7 +302,7 @@ mod tests {
assert!(bank.is_partitioned_rewards_feature_enabled());
}

/// Test get_reward_distribution_num_blocks, get_reward_calculation_num_blocks, get_reward_total_num_blocks during small epoch
/// Test get_reward_distribution_num_blocks during small epoch
/// The num_credit_blocks should be cap to 10% of the total number of blocks in the epoch.
#[test]
fn test_get_reward_distribution_num_blocks_cap() {
Expand All @@ -322,7 +314,6 @@ mod tests {
let mut accounts_db_config: AccountsDbConfig = ACCOUNTS_DB_CONFIG_FOR_TESTING.clone();
accounts_db_config.test_partitioned_epoch_rewards =
TestPartitionedEpochRewards::PartitionedEpochRewardsConfigRewardBlocks {
reward_calculation_num_blocks: 1,
stake_account_stores_per_block: 10,
};

Expand All @@ -346,9 +337,7 @@ mod tests {
assert_eq!(stake_account_stores_per_block, 10);

let check_num_reward_distribution_blocks =
|num_stakes: u64,
expected_num_reward_distribution_blocks: u64,
expected_num_reward_computation_blocks: u64| {
|num_stakes: u64, expected_num_reward_distribution_blocks: u64| {
// Given the short epoch, i.e. 32 slots, we should cap the number of reward distribution blocks to 32/10 = 3.
let stake_rewards = (0..num_stakes)
.map(|_| StakeReward::new_random())
Expand All @@ -358,34 +347,25 @@ mod tests {
bank.get_reward_distribution_num_blocks(&stake_rewards),
expected_num_reward_distribution_blocks
);
assert_eq!(
bank.get_reward_calculation_num_blocks(),
expected_num_reward_computation_blocks
);
assert_eq!(
bank.get_reward_total_num_blocks(&stake_rewards),
bank.get_reward_distribution_num_blocks(&stake_rewards)
+ bank.get_reward_calculation_num_blocks(),
);
};

for test_record in [
// num_stakes, expected_num_reward_distribution_blocks, expected_num_reward_computation_blocks
(0, 1, 1),
(1, 1, 1),
(stake_account_stores_per_block, 1, 1),
(2 * stake_account_stores_per_block - 1, 2, 1),
(2 * stake_account_stores_per_block, 2, 1),
(3 * stake_account_stores_per_block - 1, 3, 1),
(3 * stake_account_stores_per_block, 3, 1),
(4 * stake_account_stores_per_block, 3, 1), // cap at 3
(5 * stake_account_stores_per_block, 3, 1), //cap at 3
// num_stakes, expected_num_reward_distribution_blocks
(0, 1),
(1, 1),
(stake_account_stores_per_block, 1),
(2 * stake_account_stores_per_block - 1, 2),
(2 * stake_account_stores_per_block, 2),
(3 * stake_account_stores_per_block - 1, 3),
(3 * stake_account_stores_per_block, 3),
(4 * stake_account_stores_per_block, 3), // cap at 3
(5 * stake_account_stores_per_block, 3), //cap at 3
] {
check_num_reward_distribution_blocks(test_record.0, test_record.1, test_record.2);
check_num_reward_distribution_blocks(test_record.0, test_record.1);
}
}

/// Test get_reward_distribution_num_blocks, get_reward_calculation_num_blocks, get_reward_total_num_blocks during normal epoch gives the expected result
/// Test get_reward_distribution_num_blocks during normal epoch gives the expected result
#[test]
fn test_get_reward_distribution_num_blocks_normal() {
solana_logger::setup();
Expand All @@ -402,15 +382,9 @@ mod tests {
.collect::<Vec<_>>();

assert_eq!(bank.get_reward_distribution_num_blocks(&stake_rewards), 2);
assert_eq!(bank.get_reward_calculation_num_blocks(), 1);
assert_eq!(
bank.get_reward_total_num_blocks(&stake_rewards),
bank.get_reward_distribution_num_blocks(&stake_rewards)
+ bank.get_reward_calculation_num_blocks(),
);
}

/// Test get_reward_distribution_num_blocks, get_reward_calculation_num_blocks, get_reward_total_num_blocks during warm up epoch gives the expected result.
/// Test get_reward_distribution_num_blocks during warm up epoch gives the expected result.
/// The num_credit_blocks should be 1 during warm up epoch.
#[test]
fn test_get_reward_distribution_num_blocks_warmup() {
Expand All @@ -419,12 +393,6 @@ mod tests {
let bank = Bank::new_for_tests(&genesis_config);
let rewards = vec![];
assert_eq!(bank.get_reward_distribution_num_blocks(&rewards), 1);
assert_eq!(bank.get_reward_calculation_num_blocks(), 1);
assert_eq!(
bank.get_reward_total_num_blocks(&rewards),
bank.get_reward_distribution_num_blocks(&rewards)
+ bank.get_reward_calculation_num_blocks(),
);
}

#[test]
Expand Down Expand Up @@ -570,7 +538,6 @@ mod tests {
let mut accounts_db_config: AccountsDbConfig = ACCOUNTS_DB_CONFIG_FOR_TESTING.clone();
accounts_db_config.test_partitioned_epoch_rewards =
TestPartitionedEpochRewards::PartitionedEpochRewardsConfigRewardBlocks {
reward_calculation_num_blocks: 1,
stake_account_stores_per_block: 50,
};

Expand Down

0 comments on commit fadfa61

Please sign in to comment.