diff --git a/programs/stake/src/stake_instruction.rs b/programs/stake/src/stake_instruction.rs index 7b9e85079b..1eb822b5c4 100644 --- a/programs/stake/src/stake_instruction.rs +++ b/programs/stake/src/stake_instruction.rs @@ -55,14 +55,18 @@ pub fn process_instruction( trace!("process_instruction: {:?}", data); - let mut me = instruction_context.try_borrow_instruction_account(transaction_context, 0)?; - if *me.get_owner() != id() { - return Err(InstructionError::InvalidAccountOwner); - } + let get_stake_account = || { + let me = instruction_context.try_borrow_instruction_account(transaction_context, 0)?; + if *me.get_owner() != id() { + return Err(InstructionError::InvalidAccountOwner); + } + Ok(me) + }; let signers = instruction_context.get_signers(transaction_context); - match limited_deserialize(data)? { - StakeInstruction::Initialize(authorized, lockup) => { + match limited_deserialize(data) { + Ok(StakeInstruction::Initialize(authorized, lockup)) => { + let mut me = get_stake_account()?; let rent = get_sysvar_with_account_check::rent(invoke_context, instruction_context, 1)?; initialize( &mut me, @@ -72,7 +76,8 @@ pub fn process_instruction( &invoke_context.feature_set, ) } - StakeInstruction::Authorize(authorized_pubkey, stake_authorize) => { + Ok(StakeInstruction::Authorize(authorized_pubkey, stake_authorize)) => { + let mut me = get_stake_account()?; let require_custodian_for_locked_stake_authorize = invoke_context .feature_set .is_active(&feature_set::require_custodian_for_locked_stake_authorize::id()); @@ -109,7 +114,8 @@ pub fn process_instruction( ) } } - StakeInstruction::AuthorizeWithSeed(args) => { + Ok(StakeInstruction::AuthorizeWithSeed(args)) => { + let mut me = get_stake_account()?; instruction_context.check_number_of_instruction_accounts(2)?; let require_custodian_for_locked_stake_authorize = invoke_context .feature_set @@ -153,7 +159,8 @@ pub fn process_instruction( ) } } - StakeInstruction::DelegateStake => { + Ok(StakeInstruction::DelegateStake) => { + let me = get_stake_account()?; instruction_context.check_number_of_instruction_accounts(2)?; let clock = get_sysvar_with_account_check::clock(invoke_context, instruction_context, 2)?; @@ -182,7 +189,8 @@ pub fn process_instruction( &signers, ) } - StakeInstruction::Split(lamports) => { + Ok(StakeInstruction::Split(lamports)) => { + let me = get_stake_account()?; instruction_context.check_number_of_instruction_accounts(2)?; drop(me); split( @@ -195,7 +203,8 @@ pub fn process_instruction( &signers, ) } - StakeInstruction::Merge => { + Ok(StakeInstruction::Merge) => { + let me = get_stake_account()?; instruction_context.check_number_of_instruction_accounts(2)?; let clock = get_sysvar_with_account_check::clock(invoke_context, instruction_context, 2)?; @@ -216,7 +225,8 @@ pub fn process_instruction( &signers, ) } - StakeInstruction::Withdraw(lamports) => { + Ok(StakeInstruction::Withdraw(lamports)) => { + let me = get_stake_account()?; instruction_context.check_number_of_instruction_accounts(2)?; let clock = get_sysvar_with_account_check::clock(invoke_context, instruction_context, 2)?; @@ -244,16 +254,19 @@ pub fn process_instruction( &invoke_context.feature_set, ) } - StakeInstruction::Deactivate => { + Ok(StakeInstruction::Deactivate) => { + let mut me = get_stake_account()?; let clock = get_sysvar_with_account_check::clock(invoke_context, instruction_context, 1)?; deactivate(&mut me, &clock, &signers) } - StakeInstruction::SetLockup(lockup) => { + Ok(StakeInstruction::SetLockup(lockup)) => { + let mut me = get_stake_account()?; let clock = invoke_context.get_sysvar_cache().get_clock()?; set_lockup(&mut me, &lockup, &signers, &clock) } - StakeInstruction::InitializeChecked => { + Ok(StakeInstruction::InitializeChecked) => { + let mut me = get_stake_account()?; if invoke_context .feature_set .is_active(&feature_set::vote_stake_checked_instructions::id()) @@ -287,7 +300,8 @@ pub fn process_instruction( Err(InstructionError::InvalidInstructionData) } } - StakeInstruction::AuthorizeChecked(stake_authorize) => { + Ok(StakeInstruction::AuthorizeChecked(stake_authorize)) => { + let mut me = get_stake_account()?; if invoke_context .feature_set .is_active(&feature_set::vote_stake_checked_instructions::id()) @@ -321,7 +335,8 @@ pub fn process_instruction( Err(InstructionError::InvalidInstructionData) } } - StakeInstruction::AuthorizeCheckedWithSeed(args) => { + Ok(StakeInstruction::AuthorizeCheckedWithSeed(args)) => { + let mut me = get_stake_account()?; if invoke_context .feature_set .is_active(&feature_set::vote_stake_checked_instructions::id()) @@ -360,7 +375,8 @@ pub fn process_instruction( Err(InstructionError::InvalidInstructionData) } } - StakeInstruction::SetLockupChecked(lockup_checked) => { + Ok(StakeInstruction::SetLockupChecked(lockup_checked)) => { + let mut me = get_stake_account()?; if invoke_context .feature_set .is_active(&feature_set::vote_stake_checked_instructions::id()) @@ -383,21 +399,31 @@ pub fn process_instruction( Err(InstructionError::InvalidInstructionData) } } - StakeInstruction::GetMinimumDelegation => { + Ok(StakeInstruction::GetMinimumDelegation) => { let feature_set = invoke_context.feature_set.as_ref(); if !feature_set.is_active( &feature_set::add_get_minimum_delegation_instruction_to_stake_program::id(), ) { + // Retain previous behavior of always checking that the first account + // is a stake account until the feature is activated + let _ = get_stake_account()?; return Err(InstructionError::InvalidInstructionData); } let minimum_delegation = crate::get_minimum_delegation(feature_set); let minimum_delegation = Vec::from(minimum_delegation.to_le_bytes()); - drop(me); invoke_context .transaction_context .set_return_data(id(), minimum_delegation) } + Err(err) => { + if !invoke_context.feature_set.is_active( + &feature_set::add_get_minimum_delegation_instruction_to_stake_program::id(), + ) { + let _ = get_stake_account()?; + } + Err(err) + } } } @@ -434,7 +460,7 @@ mod tests { system_program, sysvar, }, solana_vote_program::vote_state::{self, VoteState, VoteStateVersions}, - std::{collections::HashSet, str::FromStr}, + std::{collections::HashSet, str::FromStr, sync::Arc}, }; fn create_default_account() -> AccountSharedData { @@ -5969,4 +5995,137 @@ mod tests { }, ); } + + // Ensure that the correct errors are returned when processing instructions + // + // The GetMinimumDelegation instruction does not take any accounts; so when it was added, + // `process_instruction()` needed to be updated to *not* need a stake account passed in, which + // changes the error *ordering* conditions. These changes shall only occur when the + // `add_get_minimum_delegation_instruction_to_stake_program` feature is enabled, and this test + // ensures it. + // + // For the following combinations of the feature enabled/disabled, if the instruction is + // valid/invalid, and if a stake account is passed in or not, assert the result: + // + // feature | instruction | account || result + // ---------+-------------+---------++-------- + // enabled | good | some || Ok + // enabled | bad | some || Err InvalidInstructionData + // enabled | good | none || Err NotEnoughAccountKeys + // enabled | bad | none || Err InvalidInstructionData + // disabled | good | some || Ok + // disabled | bad | some || Err InvalidInstructionData + // disabled | good | none || Err NotEnoughAccountKeys + // disabled | bad | none || Err NotEnoughAccountKeys + #[test] + fn test_stake_process_instruction_error_ordering() { + let rent = Rent::default(); + let rent_address = sysvar::rent::id(); + let rent_account = account::create_account_shared_data_for_test(&rent); + + let good_stake_address = Pubkey::new_unique(); + let good_stake_account = AccountSharedData::new( + u64::MAX, + std::mem::size_of::(), + &id(), + ); + let good_instruction = instruction::initialize( + &good_stake_address, + &Authorized::auto(&good_stake_address), + &Lockup::default(), + ); + let good_transaction_accounts = vec![ + (good_stake_address, good_stake_account), + (rent_address, rent_account), + ]; + let good_instruction_accounts = vec![ + AccountMeta { + pubkey: good_stake_address, + is_signer: false, + is_writable: false, + }, + AccountMeta { + pubkey: rent_address, + is_signer: false, + is_writable: false, + }, + ]; + let good_accounts = (good_transaction_accounts, good_instruction_accounts); + + // The instruction data needs to deserialize to a bogus StakeInstruction. We likely never + // will have `usize::MAX`-number of instructions, so this should be a safe constant to + // always map to an invalid stake instruction. + let bad_instruction = Instruction::new_with_bincode(id(), &usize::MAX, Vec::default()); + let bad_transaction_accounts = Vec::default(); + let bad_instruction_accounts = Vec::default(); + let bad_accounts = (bad_transaction_accounts, bad_instruction_accounts); + + for ( + is_feature_enabled, + instruction, + (transaction_accounts, instruction_accounts), + expected_result, + ) in [ + (true, &good_instruction, &good_accounts, Ok(())), + ( + true, + &bad_instruction, + &good_accounts, + Err(InstructionError::InvalidInstructionData), + ), + ( + true, + &good_instruction, + &bad_accounts, + Err(InstructionError::NotEnoughAccountKeys), + ), + ( + true, + &bad_instruction, + &bad_accounts, + Err(InstructionError::InvalidInstructionData), + ), + (false, &good_instruction, &good_accounts, Ok(())), + ( + false, + &bad_instruction, + &good_accounts, + Err(InstructionError::InvalidInstructionData), + ), + ( + false, + &good_instruction, + &bad_accounts, + Err(InstructionError::NotEnoughAccountKeys), + ), + ( + false, + &bad_instruction, + &bad_accounts, + Err(InstructionError::NotEnoughAccountKeys), + ), + ] { + mock_process_instruction( + &id(), + Vec::new(), + &instruction.data, + transaction_accounts.clone(), + instruction_accounts.clone(), + None, + expected_result, + if is_feature_enabled { + |first_instruction_account, invoke_context| { + super::process_instruction(first_instruction_account, invoke_context) + } + } else { + |first_instruction_account, invoke_context| { + let mut feature_set = FeatureSet::all_enabled(); + feature_set.deactivate(&feature_set::add_get_minimum_delegation_instruction_to_stake_program::id()); + invoke_context.feature_set = Arc::new(feature_set); + super::process_instruction(first_instruction_account, invoke_context) + } + }, + ); + } + } }