Remove overly restrictive check_account_access for partitioned epoch rewards (#631)

* Remove rewards-interval-related check_account_access implementation

* Move RewardsInterval to tests module

* Update test to new StakeProgram functionality
This commit is contained in:
Tyera 2024-04-09 21:55:45 -06:00 committed by GitHub
parent 4535ea60a9
commit dd8e1f4c73
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 88 additions and 60 deletions

View File

@ -42,8 +42,7 @@ use {
builtins::{BuiltinPrototype, BUILTINS}, builtins::{BuiltinPrototype, BUILTINS},
metrics::*, metrics::*,
partitioned_epoch_rewards::{ partitioned_epoch_rewards::{
EpochRewardCalculateParamInfo, EpochRewardStatus, RewardInterval, StakeRewards, EpochRewardCalculateParamInfo, EpochRewardStatus, StakeRewards, VoteRewardsAccounts,
VoteRewardsAccounts,
}, },
}, },
bank_forks::BankForks, bank_forks::BankForks,
@ -6862,26 +6861,6 @@ impl TransactionProcessingCallback for Bank {
self.feature_set.clone() self.feature_set.clone()
} }
fn check_account_access(
&self,
message: &SanitizedMessage,
account_index: usize,
account: &AccountSharedData,
error_counters: &mut TransactionErrorMetrics,
) -> Result<()> {
if self.get_reward_interval() == RewardInterval::InsideInterval
&& message.is_writable(account_index)
&& solana_stake_program::check_id(account.owner())
{
error_counters.program_execution_temporarily_restricted += 1;
Err(TransactionError::ProgramExecutionTemporarilyRestricted {
account_index: account_index as u8,
})
} else {
Ok(())
}
}
fn get_program_match_criteria(&self, program: &Pubkey) -> LoadedProgramMatchCriteria { fn get_program_match_criteria(&self, program: &Pubkey) -> LoadedProgramMatchCriteria {
if self.check_program_modification_slot { if self.check_program_modification_slot {
self.transaction_processor self.transaction_processor

View File

@ -18,14 +18,6 @@ use {
std::sync::Arc, std::sync::Arc,
}; };
#[derive(Debug, PartialEq, Eq, Copy, Clone)]
pub(super) enum RewardInterval {
/// the slot within the epoch is INSIDE the reward distribution interval
InsideInterval,
/// the slot within the epoch is OUTSIDE the reward distribution interval
OutsideInterval,
}
#[derive(AbiExample, Debug, Clone, PartialEq, Serialize, Deserialize)] #[derive(AbiExample, Debug, Clone, PartialEq, Serialize, Deserialize)]
pub(crate) struct StartBlockHeightAndRewards { pub(crate) struct StartBlockHeightAndRewards {
/// the block height of the slot at which rewards distribution began /// the block height of the slot at which rewards distribution began
@ -155,15 +147,6 @@ impl Bank {
} }
} }
/// Return `RewardInterval` enum for current bank
pub(super) fn get_reward_interval(&self) -> RewardInterval {
if matches!(self.epoch_reward_status, EpochRewardStatus::Active(_)) {
RewardInterval::InsideInterval
} else {
RewardInterval::OutsideInterval
}
}
/// true if it is ok to run partitioned rewards code. /// true if it is ok to run partitioned rewards code.
/// This means the feature is activated or certain testing situations. /// This means the feature is activated or certain testing situations.
pub(super) fn is_partitioned_rewards_code_enabled(&self) -> bool { pub(super) fn is_partitioned_rewards_code_enabled(&self) -> bool {
@ -206,16 +189,37 @@ mod tests {
partitioned_rewards::TestPartitionedEpochRewards, partitioned_rewards::TestPartitionedEpochRewards,
}, },
solana_sdk::{ solana_sdk::{
account::Account,
epoch_schedule::EpochSchedule, epoch_schedule::EpochSchedule,
native_token::LAMPORTS_PER_SOL, native_token::LAMPORTS_PER_SOL,
signature::Signer, signature::Signer,
signer::keypair::Keypair,
stake::instruction::StakeError,
system_transaction, system_transaction,
transaction::Transaction,
vote::state::{VoteStateVersions, MAX_LOCKOUT_HISTORY}, vote::state::{VoteStateVersions, MAX_LOCKOUT_HISTORY},
}, },
solana_vote_program::{vote_state, vote_transaction}, solana_vote_program::{vote_state, vote_transaction},
}; };
#[derive(Debug, PartialEq, Eq, Copy, Clone)]
enum RewardInterval {
/// the slot within the epoch is INSIDE the reward distribution interval
InsideInterval,
/// the slot within the epoch is OUTSIDE the reward distribution interval
OutsideInterval,
}
impl Bank { impl Bank {
/// Return `RewardInterval` enum for current bank
fn get_reward_interval(&self) -> RewardInterval {
if matches!(self.epoch_reward_status, EpochRewardStatus::Active(_)) {
RewardInterval::InsideInterval
} else {
RewardInterval::OutsideInterval
}
}
/// Return the total number of blocks in reward interval (including both calculation and crediting). /// 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 { pub(in crate::bank) fn get_reward_total_num_blocks(&self, rewards: &StakeRewards) -> u64 {
self.get_reward_calculation_num_blocks() self.get_reward_calculation_num_blocks()
@ -642,28 +646,54 @@ mod tests {
} }
} }
/// Test that program execution that involves stake accounts should fail during reward period. /// Test that program execution that attempts to mutate a stake account
/// Any programs, which result in stake account changes, will throw `ProgramExecutionTemporarilyRestricted` error when /// incorrectly should fail during reward period. A credit should succeed,
/// in reward period. /// but a withdrawal shoudl fail.
#[test] #[test]
fn test_program_execution_restricted_for_stake_account_in_reward_period() { fn test_program_execution_restricted_for_stake_account_in_reward_period() {
use solana_sdk::transaction::TransactionError::ProgramExecutionTemporarilyRestricted; use solana_sdk::transaction::TransactionError::InstructionError;
let validator_vote_keypairs = ValidatorVoteKeypairs::new_rand(); let validator_vote_keypairs = ValidatorVoteKeypairs::new_rand();
let validator_keypairs = vec![&validator_vote_keypairs]; let validator_keypairs = vec![&validator_vote_keypairs];
let GenesisConfigInfo { genesis_config, .. } = create_genesis_config_with_vote_accounts( let GenesisConfigInfo {
mut genesis_config,
mint_keypair,
..
} = create_genesis_config_with_vote_accounts(
1_000_000_000, 1_000_000_000,
&validator_keypairs, &validator_keypairs,
vec![1_000_000_000; 1], vec![1_000_000_000; 1],
); );
let node_key = &validator_keypairs[0].node_keypair; // Add stake account to try to mutate
let stake_key = &validator_keypairs[0].stake_keypair; let vote_key = validator_keypairs[0].vote_keypair.pubkey();
let vote_account = genesis_config
.accounts
.iter()
.find(|(&address, _)| address == vote_key)
.map(|(_, account)| account)
.unwrap()
.clone();
let new_stake_signer = Keypair::new();
let new_stake_address = new_stake_signer.pubkey();
let new_stake_account = Account::from(solana_stake_program::stake_state::create_account(
&new_stake_address,
&vote_key,
&vote_account.into(),
&genesis_config.rent,
2_000_000_000,
));
genesis_config
.accounts
.extend(vec![(new_stake_address, new_stake_account)]);
let (mut previous_bank, bank_forks) = Bank::new_with_bank_forks_for_tests(&genesis_config); let (mut previous_bank, bank_forks) = Bank::new_with_bank_forks_for_tests(&genesis_config);
let num_slots_in_epoch = previous_bank.get_slots_in_epoch(previous_bank.epoch()); let num_slots_in_epoch = previous_bank.get_slots_in_epoch(previous_bank.epoch());
assert_eq!(num_slots_in_epoch, 32); assert_eq!(num_slots_in_epoch, 32);
let transfer_amount = 5_000;
for slot in 1..=num_slots_in_epoch + 2 { for slot in 1..=num_slots_in_epoch + 2 {
let bank = new_bank_from_parent_with_bank_forks( let bank = new_bank_from_parent_with_bank_forks(
bank_forks.as_ref(), bank_forks.as_ref(),
@ -685,25 +715,46 @@ mod tests {
); );
bank.process_transaction(&vote).unwrap(); bank.process_transaction(&vote).unwrap();
// Insert a transfer transaction from node account to stake account // Insert a transfer transaction from the mint to new stake account
let tx = system_transaction::transfer( let system_tx = system_transaction::transfer(
node_key, &mint_keypair,
&stake_key.pubkey(), &new_stake_address,
1, transfer_amount,
bank.last_blockhash(), bank.last_blockhash(),
); );
let r = bank.process_transaction(&tx); let system_result = bank.process_transaction(&system_tx);
// Credits should always succeed
assert!(system_result.is_ok());
// Attempt to withdraw from new stake account to the mint
let stake_ix = solana_sdk::stake::instruction::withdraw(
&new_stake_address,
&new_stake_address,
&mint_keypair.pubkey(),
transfer_amount,
None,
);
let stake_tx = Transaction::new_signed_with_payer(
&[stake_ix],
Some(&mint_keypair.pubkey()),
&[&mint_keypair, &new_stake_signer],
bank.last_blockhash(),
);
let stake_result = bank.process_transaction(&stake_tx);
if slot == num_slots_in_epoch { if slot == num_slots_in_epoch {
// When the bank is at the beginning of the new epoch, i.e. slot 32, // When the bank is at the beginning of the new epoch, i.e. slot
// ProgramExecutionTemporarilyRestricted should be thrown for the transfer transaction. // 32, StakeError::EpochRewardsActive should be thrown for
// actions like StakeInstruction::Withdraw
assert_eq!( assert_eq!(
r, stake_result,
Err(ProgramExecutionTemporarilyRestricted { account_index: 1 }) Err(InstructionError(0, StakeError::EpochRewardsActive.into()))
); );
} else { } else {
// When the bank is outside of reward interval, the transfer transaction should not be affected and will succeed. // When the bank is outside of reward interval, the withdraw
assert!(r.is_ok()); // transaction should not be affected and will succeed.
assert!(stake_result.is_ok());
} }
// Push a dummy blockhash, so that the latest_blockhash() for the transfer transaction in each // Push a dummy blockhash, so that the latest_blockhash() for the transfer transaction in each

View File

@ -298,8 +298,6 @@ fn load_transaction_accounts<CB: TransactionProcessingCallback>(
validated_fee_payer = true; validated_fee_payer = true;
} }
callbacks.check_account_access(message, i, &account, error_counters)?;
tx_rent += rent; tx_rent += rent;
rent_debits.insert(key, rent, account.lamports()); rent_debits.insert(key, rent, account.lamports());