From 6f2e556b1653a5cd34418a705d9aa4f50eb295e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Mei=C3=9Fner?= Date: Mon, 5 Sep 2022 16:29:02 +0200 Subject: [PATCH] Cleanup: `TransactionContext` (#27595) * Lets instruction_accounts_lamport_sum() have the &InstructionContext as parameter directly. * Updates docu comments. * Uses accessors methods instead of accessing private properties of other structs. * Adds #![deny(clippy::indexing_slicing)]. * Has get_signers() return a Result instead of using unwrap(). * Removes InvokeContext::get_key_of_account_at_index(). --- program-runtime/src/invoke_context.rs | 9 -- programs/bpf_loader/src/syscalls/mod.rs | 14 ++-- programs/stake/src/stake_instruction.rs | 2 +- programs/vote/src/vote_processor.rs | 2 +- runtime/src/system_instruction_processor.rs | 2 +- sdk/src/transaction_context.rs | 91 +++++++++++---------- 6 files changed, 60 insertions(+), 60 deletions(-) diff --git a/program-runtime/src/invoke_context.rs b/program-runtime/src/invoke_context.rs index 128039d6e2..49c9c2eea0 100644 --- a/program-runtime/src/invoke_context.rs +++ b/program-runtime/src/invoke_context.rs @@ -895,15 +895,6 @@ impl<'a> InvokeContext<'a> { &self.sysvar_cache } - // Get pubkey of account at index - pub fn get_key_of_account_at_index( - &self, - index_in_transaction: usize, - ) -> Result<&Pubkey, InstructionError> { - self.transaction_context - .get_key_of_account_at_index(index_in_transaction) - } - // Set this instruction syscall context pub fn set_syscall_context( &mut self, diff --git a/programs/bpf_loader/src/syscalls/mod.rs b/programs/bpf_loader/src/syscalls/mod.rs index 3902731b8b..ffc3f541cd 100644 --- a/programs/bpf_loader/src/syscalls/mod.rs +++ b/programs/bpf_loader/src/syscalls/mod.rs @@ -1812,12 +1812,14 @@ declare_syscall!( let account_metas = question_mark!( (0..instruction_context.get_number_of_instruction_accounts()) .map(|instruction_account_index| Ok(AccountMeta { - pubkey: *invoke_context.get_key_of_account_at_index( - instruction_context - .get_index_of_instruction_account_in_transaction( - instruction_account_index - )? - )?, + pubkey: *invoke_context + .transaction_context + .get_key_of_account_at_index( + instruction_context + .get_index_of_instruction_account_in_transaction( + instruction_account_index + )? + )?, is_signer: instruction_context .is_instruction_account_signer(instruction_account_index)?, is_writable: instruction_context diff --git a/programs/stake/src/stake_instruction.rs b/programs/stake/src/stake_instruction.rs index cb47d19e6b..795cf9e8b1 100644 --- a/programs/stake/src/stake_instruction.rs +++ b/programs/stake/src/stake_instruction.rs @@ -69,7 +69,7 @@ pub fn process_instruction( Ok(me) }; - let signers = instruction_context.get_signers(transaction_context); + let signers = instruction_context.get_signers(transaction_context)?; match limited_deserialize(data) { Ok(StakeInstruction::Initialize(authorized, lockup)) => { let mut me = get_stake_account()?; diff --git a/programs/vote/src/vote_processor.rs b/programs/vote/src/vote_processor.rs index 99e07e2fac..ab8ff3d16e 100644 --- a/programs/vote/src/vote_processor.rs +++ b/programs/vote/src/vote_processor.rs @@ -70,7 +70,7 @@ pub fn process_instruction( return Err(InstructionError::InvalidAccountOwner); } - let signers = instruction_context.get_signers(transaction_context); + let signers = instruction_context.get_signers(transaction_context)?; match limited_deserialize(data)? { VoteInstruction::InitializeAccount(vote_init) => { let rent = get_sysvar_with_account_check::rent(invoke_context, instruction_context, 1)?; diff --git a/runtime/src/system_instruction_processor.rs b/runtime/src/system_instruction_processor.rs index 82589ea65f..db04256e61 100644 --- a/runtime/src/system_instruction_processor.rs +++ b/runtime/src/system_instruction_processor.rs @@ -324,7 +324,7 @@ pub fn process_instruction( trace!("process_instruction: {:?}", instruction); - let signers = instruction_context.get_signers(transaction_context); + let signers = instruction_context.get_signers(transaction_context)?; match instruction { SystemInstruction::CreateAccount { lamports, diff --git a/sdk/src/transaction_context.rs b/sdk/src/transaction_context.rs index 1691c8f3c6..7d6ef6e56b 100644 --- a/sdk/src/transaction_context.rs +++ b/sdk/src/transaction_context.rs @@ -1,4 +1,5 @@ //! Data shared between program runtime and built-in programs as well as SBF programs +#![deny(clippy::indexing_slicing)] #[cfg(target_os = "solana")] use crate::instruction::AccountPropertyUpdate; @@ -23,8 +24,6 @@ use { }, }; -pub type TransactionAccount = (Pubkey, AccountSharedData); - /// For addressing (nested) properties of the TransactionContext #[repr(u16)] pub enum TransactionContextAttribute { @@ -99,6 +98,9 @@ pub struct InstructionAccount { pub is_writable: bool, } +/// An account key and the matching account +pub type TransactionAccount = (Pubkey, AccountSharedData); + /// Loaded transaction shared between runtime and programs. /// /// This context is valid for the entire duration of a transaction being processed. @@ -273,19 +275,16 @@ impl TransactionContext { .instruction_trace .last() .ok_or(InstructionError::CallDepth)?; - let callee_instruction_accounts_lamport_sum = self.instruction_accounts_lamport_sum( - caller_instruction_context.instruction_accounts.iter(), - )?; + let callee_instruction_accounts_lamport_sum = + self.instruction_accounts_lamport_sum(caller_instruction_context)?; if !self.instruction_stack.is_empty() && self.is_early_verification_of_account_modifications_enabled() { let caller_instruction_context = self.get_current_instruction_context()?; let original_caller_instruction_accounts_lamport_sum = caller_instruction_context.instruction_accounts_lamport_sum; - let current_caller_instruction_accounts_lamport_sum = self - .instruction_accounts_lamport_sum( - caller_instruction_context.instruction_accounts.iter(), - )?; + let current_caller_instruction_accounts_lamport_sum = + self.instruction_accounts_lamport_sum(caller_instruction_context)?; if original_caller_instruction_accounts_lamport_sum != current_caller_instruction_accounts_lamport_sum { @@ -298,7 +297,7 @@ impl TransactionContext { instruction_context.instruction_accounts_lamport_sum = callee_instruction_accounts_lamport_sum; } - let index_in_trace = self.instruction_trace.len().saturating_sub(1); + let index_in_trace = self.get_instruction_trace_length(); self.instruction_trace.push(InstructionContext::default()); if nesting_level >= self.instruction_context_capacity { return Err(InstructionError::CallDepth); @@ -324,13 +323,11 @@ impl TransactionContext { .try_borrow_mut() .map_err(|_| InstructionError::AccountBorrowOutstanding)?; } - self.instruction_accounts_lamport_sum( - instruction_context.instruction_accounts.iter(), - ) - .map(|instruction_accounts_lamport_sum| { - instruction_context.instruction_accounts_lamport_sum - != instruction_accounts_lamport_sum - }) + self.instruction_accounts_lamport_sum(instruction_context) + .map(|instruction_accounts_lamport_sum| { + instruction_context.instruction_accounts_lamport_sum + != instruction_accounts_lamport_sum + }) }) } else { Ok(false) @@ -361,23 +358,26 @@ impl TransactionContext { /// Calculates the sum of all lamports within an instruction #[cfg(not(target_os = "solana"))] - fn instruction_accounts_lamport_sum<'a, I>( - &'a self, - instruction_accounts: I, - ) -> Result - where - I: Iterator, - { + fn instruction_accounts_lamport_sum( + &self, + instruction_context: &InstructionContext, + ) -> Result { if !self.is_early_verification_of_account_modifications_enabled() { return Ok(0); } let mut instruction_accounts_lamport_sum: u128 = 0; - for (instruction_account_index, instruction_account) in instruction_accounts.enumerate() { - if instruction_account_index != instruction_account.index_in_callee { + for instruction_account_index in 0..instruction_context.get_number_of_instruction_accounts() + { + if instruction_context + .is_instruction_account_duplicate(instruction_account_index)? + .is_some() + { continue; // Skip duplicate account } + let index_in_transaction = instruction_context + .get_index_of_instruction_account_in_transaction(instruction_account_index)?; instruction_accounts_lamport_sum = (self - .get_account_at_index(instruction_account.index_in_transaction)? + .get_account_at_index(index_in_transaction)? .try_borrow() .map_err(|_| InstructionError::AccountBorrowOutstanding)? .lamports() as u128) @@ -483,7 +483,7 @@ impl InstructionContext { self.program_accounts .iter() .position(|index_in_transaction| { - &transaction_context.account_keys[*index_in_transaction] == pubkey + transaction_context.account_keys.get(*index_in_transaction) == Some(pubkey) }) } @@ -496,8 +496,10 @@ impl InstructionContext { self.instruction_accounts .iter() .position(|instruction_account| { - &transaction_context.account_keys[instruction_account.index_in_transaction] - == pubkey + transaction_context + .account_keys + .get(instruction_account.index_in_transaction) + == Some(pubkey) }) } @@ -548,7 +550,7 @@ impl InstructionContext { transaction_context: &'b TransactionContext, ) -> Result<&'b Pubkey, InstructionError> { self.get_index_of_program_account_in_transaction( - self.program_accounts.len().saturating_sub(1), + self.get_number_of_program_accounts().saturating_sub(1), ) .and_then(|index_in_transaction| { transaction_context.get_key_of_account_at_index(index_in_transaction) @@ -583,7 +585,7 @@ impl InstructionContext { ) -> Result, InstructionError> { let result = self.try_borrow_program_account( transaction_context, - self.program_accounts.len().saturating_sub(1), + self.get_number_of_program_accounts().saturating_sub(1), ); debug_assert!(result.is_ok()); result @@ -615,8 +617,7 @@ impl InstructionContext { self.try_borrow_account( transaction_context, index_in_transaction, - self.program_accounts - .len() + self.get_number_of_program_accounts() .saturating_add(instruction_account_index), ) } @@ -646,16 +647,20 @@ impl InstructionContext { } /// Calculates the set of all keys of signer instruction accounts in this Instruction - pub fn get_signers(&self, transaction_context: &TransactionContext) -> HashSet { + pub fn get_signers( + &self, + transaction_context: &TransactionContext, + ) -> Result, InstructionError> { let mut result = HashSet::new(); for instruction_account in self.instruction_accounts.iter() { if instruction_account.is_signer { result.insert( - transaction_context.account_keys[instruction_account.index_in_transaction], + *transaction_context + .get_key_of_account_at_index(instruction_account.index_in_transaction)?, ); } } - result + Ok(result) } } @@ -677,7 +682,9 @@ impl<'a> BorrowedAccount<'a> { /// Returns the public key of this account (transaction wide) pub fn get_key(&self) -> &Pubkey { - &self.transaction_context.account_keys[self.index_in_transaction] + self.transaction_context + .get_key_of_account_at_index(self.index_in_transaction) + .unwrap() } /// Returns the owner of this account (transaction wide) @@ -933,26 +940,26 @@ impl<'a> BorrowedAccount<'a> { /// Returns whether this account is a signer (instruction wide) pub fn is_signer(&self) -> bool { - if self.index_in_instruction < self.instruction_context.program_accounts.len() { + if self.index_in_instruction < self.instruction_context.get_number_of_program_accounts() { return false; } self.instruction_context .is_instruction_account_signer( self.index_in_instruction - .saturating_sub(self.instruction_context.program_accounts.len()), + .saturating_sub(self.instruction_context.get_number_of_program_accounts()), ) .unwrap_or_default() } /// Returns whether this account is writable (instruction wide) pub fn is_writable(&self) -> bool { - if self.index_in_instruction < self.instruction_context.program_accounts.len() { + if self.index_in_instruction < self.instruction_context.get_number_of_program_accounts() { return false; } self.instruction_context .is_instruction_account_writable( self.index_in_instruction - .saturating_sub(self.instruction_context.program_accounts.len()), + .saturating_sub(self.instruction_context.get_number_of_program_accounts()), ) .unwrap_or_default() }