Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Persist EpochRewards sysvar #572

Merged
7 changes: 4 additions & 3 deletions program-test/tests/sysvar.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,10 +65,11 @@ fn epoch_reward_sysvar_getter_process_instruction(
// input[0] == 1 indicates the bank is in reward period.
if input[0] == 0 {
// epoch rewards sysvar should not exist for banks that are not in reward period
let epoch_rewards = EpochRewards::get();
assert!(epoch_rewards.is_err());
let epoch_rewards = EpochRewards::get()?;
assert!(!epoch_rewards.active);
} else {
let _epoch_rewards = EpochRewards::get()?;
let epoch_rewards = EpochRewards::get()?;
assert!(epoch_rewards.active);
}

Ok(())
Expand Down
21 changes: 10 additions & 11 deletions runtime/src/bank/partitioned_epoch_rewards/distribution.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ impl Bank {
EpochRewardStatus::Active(_)
));
self.epoch_reward_status = EpochRewardStatus::Inactive;
self.destroy_epoch_rewards_sysvar();
self.set_epoch_rewards_sysvar_to_inactive();
}
}

Expand Down Expand Up @@ -211,7 +211,10 @@ mod tests {
let total_rewards = 1_000_000_000;
bank.create_epoch_rewards_sysvar(total_rewards, 0, 42);
let pre_epoch_rewards_account = bank.get_account(&sysvar::epoch_rewards::id()).unwrap();
assert_eq!(pre_epoch_rewards_account.lamports(), total_rewards);
let expected_balance =
bank.get_minimum_balance_for_rent_exemption(pre_epoch_rewards_account.data().len());
// Expected balance is the sysvar rent-exempt balance
assert_eq!(pre_epoch_rewards_account.lamports(), expected_balance);

// Set up a partition of rewards to distribute
let expected_num = 100;
Expand All @@ -230,22 +233,18 @@ mod tests {
bank.distribute_epoch_rewards_in_partition(&all_rewards, 0);
let post_cap = bank.capitalization();
let post_epoch_rewards_account = bank.get_account(&sysvar::epoch_rewards::id()).unwrap();
let expected_epoch_rewards_sysvar_lamports_remaining =
total_rewards - rewards_to_distribute;

// Assert that epoch rewards sysvar lamports decreases by the distributed rewards
assert_eq!(
post_epoch_rewards_account.lamports(),
expected_epoch_rewards_sysvar_lamports_remaining
);
// Assert that epoch rewards sysvar lamports balance does not change
assert_eq!(post_epoch_rewards_account.lamports(), expected_balance);

let epoch_rewards: sysvar::epoch_rewards::EpochRewards =
from_account(&post_epoch_rewards_account).unwrap();
assert_eq!(epoch_rewards.total_rewards, total_rewards);
assert_eq!(epoch_rewards.distributed_rewards, rewards_to_distribute,);

// Assert that the bank total capital didn't change
assert_eq!(pre_cap, post_cap);
// Assert that the bank total capital changed by the amount of rewards
// distributed
assert_eq!(pre_cap + rewards_to_distribute, post_cap);
}

/// Test partitioned credits and reward history updates of epoch rewards do cover all the rewards
Expand Down
109 changes: 86 additions & 23 deletions runtime/src/bank/partitioned_epoch_rewards/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -379,15 +379,19 @@ mod tests {
.map(|_| ValidatorVoteKeypairs::new_rand())
.collect::<Vec<_>>();

let GenesisConfigInfo { genesis_config, .. } = create_genesis_config_with_vote_accounts(
let GenesisConfigInfo {
mut genesis_config, ..
} = create_genesis_config_with_vote_accounts(
1_000_000_000,
&validator_keypairs,
vec![2_000_000_000; expected_num_delegations],
);
let slots_per_epoch = 32;
genesis_config.epoch_schedule = EpochSchedule::new(slots_per_epoch);

let bank0 = Bank::new_for_tests(&genesis_config);
let num_slots_in_epoch = bank0.get_slots_in_epoch(bank0.epoch());
assert_eq!(num_slots_in_epoch, 32);
assert_eq!(num_slots_in_epoch, slots_per_epoch);

let mut previous_bank = Arc::new(Bank::new_from_parent(
Arc::new(bank0),
Expand All @@ -396,7 +400,7 @@ mod tests {
));

// simulate block progress
for slot in 2..=num_slots_in_epoch + 2 {
for slot in 2..=(2 * slots_per_epoch) + 2 {
let pre_cap = previous_bank.capitalization();
let curr_bank = Bank::new_from_parent(previous_bank, &Pubkey::default(), slot);
let post_cap = curr_bank.capitalization();
Expand Down Expand Up @@ -425,32 +429,57 @@ mod tests {
curr_bank.store_account_and_update_capitalization(&vote_id, &vote_account);
}

if slot == num_slots_in_epoch {
if slot % num_slots_in_epoch == 0 {
// This is the first block of epoch 1. Reward computation should happen in this block.
// assert reward compute status activated at epoch boundary
assert_matches!(
curr_bank.get_reward_interval(),
RewardInterval::InsideInterval
);

// cap should increase because of new epoch rewards
assert!(post_cap > pre_cap);
} else if slot == num_slots_in_epoch + 1 || slot == num_slots_in_epoch + 2 {
// 1. when curr_slot == num_slots_in_epoch + 1, the 2nd block of epoch 1, reward distribution should happen in this block.
// however, all stake rewards are paid at the this block therefore reward_status should have transitioned to inactive. And since
// rewards are transferred from epoch_rewards sysvar to stake accounts. The cap should stay the same.
// 2. when curr_slot == num_slots_in_epoch+2, the 3rd block of epoch 1. reward distribution should have already completed. Therefore,
// reward_status should stay inactive and cap should stay the same.
if slot == num_slots_in_epoch {
// cap should increase because of new epoch rewards
assert!(post_cap > pre_cap);
} else {
assert_eq!(post_cap, pre_cap);
}
Comment on lines +440 to +445

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit confused by this check. Shouldn't the cap increase at both epoch rollovers and not just the first? ie also when slot == 2 * num_slots_in_epoch?

When I ran this locally, I saw that post_cap is only 1 lamport bigger when slot == num_slots_in_epoch, which seems a bit strange. Is it just the rent exemption for the epoch rewards sysvar account?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the cap increase at both epoch rollovers and not just the first?

No. At the first block of epoch 1, the EpochRewards sysvar gets created for the first time, which increases the capitalization the amount of the sysvar's rent-exempt reserve (see below).
At the first block of epoch 2, the capitalization doesn't change: the sysvar has persisted throughout epoch 1, so no new rent-exempt reserve, and distribution starts the following block, so no new rewards.

Is it just the rent exemption for the epoch rewards sysvar account?

Correct. Sysvar lamport balances default to 1 lamport when the GenesisConfig sets Rent::free() (which it does in most of the genesis_utils fns, like create_genesis_config_with_vote_accounts()... that's a whole other issue which we can discuss separately 😅 ).

} else if slot == num_slots_in_epoch + 1 {
// 1. when curr_slot == num_slots_in_epoch + 1, the 2nd block of
// epoch 1, reward distribution should happen in this block.
// however, all stake rewards are paid at the this block
CriesofCarrots marked this conversation as resolved.
Show resolved Hide resolved
// therefore reward_status should have transitioned to inactive.
// The cap should increase accordingly.
assert_matches!(
curr_bank.get_reward_interval(),
RewardInterval::OutsideInterval
);

assert_eq!(post_cap, pre_cap);
let account = curr_bank
.get_account(&solana_sdk::sysvar::epoch_rewards::id())
.unwrap();
let epoch_rewards: solana_sdk::sysvar::epoch_rewards::EpochRewards =
solana_sdk::account::from_account(&account).unwrap();
assert_eq!(post_cap, pre_cap + epoch_rewards.distributed_rewards);
} else {
// 2. when curr_slot == num_slots_in_epoch+2, the 3rd block of
// epoch 1 (or any other slot). reward distribution should have
// already completed. Therefore, reward_status should stay
// inactive and cap should stay the same.
assert_matches!(
curr_bank.get_reward_interval(),
RewardInterval::OutsideInterval
);

// slot is not in rewards, cap should not change
assert_eq!(post_cap, pre_cap);
}
// EpochRewards sysvar is created in the first block of epoch 1.
// Ensure the sysvar persists thereafter.
if slot > num_slots_in_epoch {
let epoch_rewards_lamports =
curr_bank.get_balance(&solana_sdk::sysvar::epoch_rewards::id());
assert!(epoch_rewards_lamports > 0);
}
CriesofCarrots marked this conversation as resolved.
Show resolved Hide resolved
previous_bank = Arc::new(curr_bank);
}
}
Expand Down Expand Up @@ -513,6 +542,14 @@ mod tests {
// simulate block progress
for slot in 2..=num_slots_in_epoch + 3 {
let pre_cap = previous_bank.capitalization();

let pre_sysvar_account = previous_bank
.get_account(&solana_sdk::sysvar::epoch_rewards::id())
.unwrap_or_default();
let pre_epoch_rewards: solana_sdk::sysvar::epoch_rewards::EpochRewards =
solana_sdk::account::from_account(&pre_sysvar_account).unwrap_or_default();
let pre_distributed_rewards = pre_epoch_rewards.distributed_rewards;

let curr_bank = Bank::new_from_parent(previous_bank, &Pubkey::default(), slot);
let post_cap = curr_bank.capitalization();

Expand Down Expand Up @@ -551,27 +588,53 @@ mod tests {
// cap should increase because of new epoch rewards
assert!(post_cap > pre_cap);
} else if slot == num_slots_in_epoch + 1 {
// When curr_slot == num_slots_in_epoch + 1, the 2nd block of epoch 1, reward distribution should happen in this block.
// however, since rewards are transferred from epoch_rewards sysvar to stake accounts. The cap should stay the same.
// When curr_slot == num_slots_in_epoch + 1, the 2nd block of
// epoch 1, reward distribution should happen in this block. The
// cap should increase accordingly.
assert_matches!(
curr_bank.get_reward_interval(),
RewardInterval::InsideInterval
);

assert_eq!(post_cap, pre_cap);
} else if slot == num_slots_in_epoch + 2 || slot == num_slots_in_epoch + 3 {
// 1. when curr_slot == num_slots_in_epoch + 2, the 3nd block of epoch 1, reward distribution should happen in this block.
// however, all stake rewards are paid at the this block therefore reward_status should have transitioned to inactive. And since
// rewards are transferred from epoch_rewards sysvar to stake accounts. The cap should stay the same.
// 2. when curr_slot == num_slots_in_epoch+2, the 3rd block of epoch 1. reward distribution should have already completed. Therefore,
// reward_status should stay inactive and cap should stay the same.
let account = curr_bank
.get_account(&solana_sdk::sysvar::epoch_rewards::id())
.unwrap();
let epoch_rewards: solana_sdk::sysvar::epoch_rewards::EpochRewards =
solana_sdk::account::from_account(&account).unwrap();
assert_eq!(
post_cap,
pre_cap + epoch_rewards.distributed_rewards - pre_distributed_rewards
);
} else if slot == num_slots_in_epoch + 2 {
// When curr_slot == num_slots_in_epoch + 2, the 3nd block of
// epoch 1, reward distribution should happen in this block.
// however, all stake rewards are paid at the this block
// therefore reward_status should have transitioned to inactive.
// The cap should increase accordingly.
assert_matches!(
curr_bank.get_reward_interval(),
RewardInterval::OutsideInterval
);

assert_eq!(post_cap, pre_cap);
let account = curr_bank
.get_account(&solana_sdk::sysvar::epoch_rewards::id())
.unwrap();
let epoch_rewards: solana_sdk::sysvar::epoch_rewards::EpochRewards =
solana_sdk::account::from_account(&account).unwrap();
assert_eq!(
post_cap,
pre_cap + epoch_rewards.distributed_rewards - pre_distributed_rewards
);
} else {
// When curr_slot == num_slots_in_epoch + 3, the 4th block of
// epoch 1 (or any other slot). reward distribution should have
// already completed. Therefore, reward_status should stay
// inactive and cap should stay the same.
assert_matches!(
curr_bank.get_reward_interval(),
RewardInterval::OutsideInterval
);

// slot is not in rewards, cap should not change
assert_eq!(post_cap, pre_cap);
}
Expand Down
70 changes: 37 additions & 33 deletions runtime/src/bank/partitioned_epoch_rewards/sysvar.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ impl Bank {
) {
assert!(self.is_partitioned_rewards_code_enabled());

assert!(total_rewards >= distributed_rewards);

let epoch_rewards = sysvar::epoch_rewards::EpochRewards {
total_rewards,
distributed_rewards,
Expand All @@ -42,13 +44,10 @@ impl Bank {
};

self.update_sysvar_account(&sysvar::epoch_rewards::id(), |account| {
let mut inherited_account_fields =
self.inherit_specially_retained_account_fields(account);

assert!(total_rewards >= distributed_rewards);
// set the account lamports to the undistributed rewards
inherited_account_fields.0 = total_rewards - distributed_rewards;
create_account(&epoch_rewards, inherited_account_fields)
create_account(
&epoch_rewards,
self.inherit_specially_retained_account_fields(account),
)
});

self.log_epoch_rewards_sysvar("create");
Expand All @@ -66,30 +65,37 @@ impl Bank {
epoch_rewards.distribute(distributed);

self.update_sysvar_account(&sysvar::epoch_rewards::id(), |account| {
let mut inherited_account_fields =
self.inherit_specially_retained_account_fields(account);

let lamports = inherited_account_fields.0;
assert!(lamports >= distributed);
inherited_account_fields.0 = lamports - distributed;
create_account(&epoch_rewards, inherited_account_fields)
create_account(
&epoch_rewards,
self.inherit_specially_retained_account_fields(account),
)
});

self.log_epoch_rewards_sysvar("update");
}

pub(in crate::bank::partitioned_epoch_rewards) fn destroy_epoch_rewards_sysvar(&self) {
if let Some(account) = self.get_account(&sysvar::epoch_rewards::id()) {
if account.lamports() > 0 {
info!(
"burning {} extra lamports in EpochRewards sysvar account at slot {}",
account.lamports(),
self.slot()
);
self.log_epoch_rewards_sysvar("burn");
self.burn_and_purge_account(&sysvar::epoch_rewards::id(), account);
}
}
/// Update EpochRewards sysvar with distributed rewards
pub(in crate::bank::partitioned_epoch_rewards) fn set_epoch_rewards_sysvar_to_inactive(&self) {
let mut epoch_rewards: sysvar::epoch_rewards::EpochRewards = from_account(
&self
.get_account(&sysvar::epoch_rewards::id())
.unwrap_or_default(),
)
.unwrap_or_default();
assert_eq!(
epoch_rewards.distributed_rewards,
epoch_rewards.total_rewards
);
epoch_rewards.active = false;

self.update_sysvar_account(&sysvar::epoch_rewards::id(), |account| {
create_account(
&epoch_rewards,
self.inherit_specially_retained_account_fields(account),
)
});

self.log_epoch_rewards_sysvar("set_inactive");
}
}

Expand Down Expand Up @@ -129,14 +135,17 @@ mod tests {

bank.create_epoch_rewards_sysvar(total_rewards, 10, 42);
let account = bank.get_account(&sysvar::epoch_rewards::id()).unwrap();
assert_eq!(account.lamports(), total_rewards - 10);
let expected_balance = bank.get_minimum_balance_for_rent_exemption(account.data().len());
// Expected balance is the sysvar rent-exempt balance
assert_eq!(account.lamports(), expected_balance);
let epoch_rewards: sysvar::epoch_rewards::EpochRewards = from_account(&account).unwrap();
assert_eq!(epoch_rewards, expected_epoch_rewards);

// make a distribution from epoch rewards sysvar
bank.update_epoch_rewards_sysvar(10);
let account = bank.get_account(&sysvar::epoch_rewards::id()).unwrap();
assert_eq!(account.lamports(), total_rewards - 20);
// Balance should not change
assert_eq!(account.lamports(), expected_balance);
let epoch_rewards: sysvar::epoch_rewards::EpochRewards = from_account(&account).unwrap();
let expected_epoch_rewards = sysvar::epoch_rewards::EpochRewards {
distribution_starting_block_height: 42,
Expand All @@ -148,10 +157,5 @@ mod tests {
active: true,
};
assert_eq!(epoch_rewards, expected_epoch_rewards);

// burn epoch rewards sysvar
bank.burn_and_purge_account(&sysvar::epoch_rewards::id(), account);
let account = bank.get_account(&sysvar::epoch_rewards::id());
assert!(account.is_none());
}
}
Loading