diff --git a/program-runtime/src/invoke_context.rs b/program-runtime/src/invoke_context.rs index 5fa10b7fc6..b7014233bb 100644 --- a/program-runtime/src/invoke_context.rs +++ b/program-runtime/src/invoke_context.rs @@ -370,31 +370,29 @@ impl<'a> InvokeContext<'a> { } self.pre_accounts = Vec::with_capacity(instruction_accounts.len()); - let mut work = |_index_in_instruction: usize, - instruction_account: &InstructionAccount| { - if instruction_account.index_in_transaction - < self.transaction_context.get_number_of_accounts() - { - let account = self - .transaction_context - .get_account_at_index(instruction_account.index_in_transaction)? - .borrow() - .clone(); - self.pre_accounts.push(PreAccount::new( - self.transaction_context.get_key_of_account_at_index( - instruction_account.index_in_transaction, - )?, - account, - )); - return Ok(()); + + for (index_in_instruction, instruction_account) in + instruction_accounts.iter().enumerate() + { + if index_in_instruction != instruction_account.index_in_callee { + continue; // Skip duplicate account } - Err(InstructionError::MissingAccount) - }; - visit_each_account_once( - instruction_accounts, - &mut work, - InstructionError::NotEnoughAccountKeys, - )?; + if instruction_account.index_in_transaction + >= self.transaction_context.get_number_of_accounts() + { + return Err(InstructionError::MissingAccount); + } + let account = self + .transaction_context + .get_account_at_index(instruction_account.index_in_transaction)? + .borrow() + .clone(); + self.pre_accounts.push(PreAccount::new( + self.transaction_context + .get_key_of_account_at_index(instruction_account.index_in_transaction)?, + account, + )); + } } else { let contains = (0..self .transaction_context @@ -481,6 +479,9 @@ impl<'a> InvokeContext<'a> { } /// Verify the results of an instruction + /// + /// Note: `instruction_accounts` must be the same as passed to `InvokeContext::push()`, + /// so that they match the order of `pre_accounts`. fn verify( &mut self, instruction_accounts: &[InstructionAccount], @@ -507,7 +508,10 @@ impl<'a> InvokeContext<'a> { // Verify the per-account instruction results let (mut pre_sum, mut post_sum) = (0_u128, 0_u128); let mut pre_account_index = 0; - let mut work = |_index_in_instruction: usize, instruction_account: &InstructionAccount| { + for (index_in_instruction, instruction_account) in instruction_accounts.iter().enumerate() { + if index_in_instruction != instruction_account.index_in_callee { + continue; // Skip duplicate account + } { // Verify account has no outstanding references let _ = self @@ -560,14 +564,7 @@ impl<'a> InvokeContext<'a> { self.accounts_data_meter .adjust_delta_unchecked(data_len_delta); } - - Ok(()) - }; - visit_each_account_once( - instruction_accounts, - &mut work, - InstructionError::NotEnoughAccountKeys, - )?; + } // Verify that the total sum of all the lamports did not change if pre_sum != post_sum { @@ -577,6 +574,9 @@ impl<'a> InvokeContext<'a> { } /// Verify and update PreAccount state based on program execution + /// + /// Note: `instruction_accounts` must be the same as passed to `InvokeContext::push()`, + /// so that they match the order of `pre_accounts`. fn verify_and_update( &mut self, instruction_accounts: &[InstructionAccount], @@ -592,7 +592,10 @@ impl<'a> InvokeContext<'a> { // Verify the per-account instruction results let (mut pre_sum, mut post_sum) = (0_u128, 0_u128); - let mut work = |_index_in_instruction: usize, instruction_account: &InstructionAccount| { + for (index_in_instruction, instruction_account) in instruction_accounts.iter().enumerate() { + if index_in_instruction != instruction_account.index_in_callee { + continue; // Skip duplicate account + } if instruction_account.index_in_transaction < transaction_context.get_number_of_accounts() { @@ -659,17 +662,11 @@ impl<'a> InvokeContext<'a> { .adjust_delta_unchecked(data_len_delta); } - return Ok(()); + break; } } } - Err(InstructionError::MissingAccount) - }; - visit_each_account_once( - instruction_accounts, - &mut work, - InstructionError::NotEnoughAccountKeys, - )?; + } // Verify that the total sum of all the lamports did not change if pre_sum != post_sum { @@ -741,7 +738,8 @@ impl<'a> InvokeContext<'a> { ) -> Result<(Vec, Vec), InstructionError> { // Finds the index of each account in the instruction by its pubkey. // Then normalizes / unifies the privileges of duplicate accounts. - // Note: This works like visit_each_account_once() and is an O(n^2) algorithm too. + // Note: This is an O(n^2) algorithm, + // but performed on a very small slice and requires no heap allocations. let instruction_context = self.transaction_context.get_current_instruction_context()?; let mut deduplicated_instruction_accounts: Vec = Vec::new(); let mut duplicate_indicies = Vec::with_capacity(instruction.accounts.len()); @@ -1265,32 +1263,6 @@ pub fn mock_process_instruction( transaction_accounts } -/// Visit each unique instruction account index once -pub fn visit_each_account_once( - instruction_accounts: &[InstructionAccount], - work: &mut dyn FnMut(usize, &InstructionAccount) -> Result<(), E>, - inner_error: E, -) -> Result<(), E> { - // Note: This is an O(n^2) algorithm, - // but performed on a very small slice and requires no heap allocations - 'root: for (index, instruction_account) in instruction_accounts.iter().enumerate() { - match instruction_accounts.get(..index) { - Some(range) => { - for (before_index, before) in range.iter().enumerate() { - if before.index_in_transaction == instruction_account.index_in_transaction { - debug_assert_eq!(before_index, instruction_account.index_in_callee); - continue 'root; // skip dups - } - } - debug_assert_eq!(index, instruction_account.index_in_callee); - work(index, instruction_account)?; - } - None => return Err(inner_error), - } - } - Ok(()) -} - #[cfg(test)] mod tests { use { @@ -1316,59 +1288,6 @@ mod tests { }, } - #[test] - fn test_visit_each_account_once() { - let do_work = |accounts: &[InstructionAccount]| -> (usize, usize, usize) { - let mut unique_entries = 0; - let mut index_sum_a = 0; - let mut index_sum_b = 0; - let mut work = |index_in_instruction: usize, entry: &InstructionAccount| { - unique_entries += 1; - index_sum_a += index_in_instruction; - index_sum_b += entry.index_in_transaction; - Ok(()) - }; - visit_each_account_once(accounts, &mut work, InstructionError::NotEnoughAccountKeys) - .unwrap(); - - (unique_entries, index_sum_a, index_sum_b) - }; - - assert_eq!( - (3, 3, 19), - do_work(&[ - InstructionAccount { - index_in_transaction: 7, - index_in_caller: 0, - index_in_callee: 0, - is_signer: false, - is_writable: false, - }, - InstructionAccount { - index_in_transaction: 3, - index_in_caller: 1, - index_in_callee: 1, - is_signer: false, - is_writable: false, - }, - InstructionAccount { - index_in_transaction: 9, - index_in_caller: 2, - index_in_callee: 2, - is_signer: false, - is_writable: false, - }, - InstructionAccount { - index_in_transaction: 3, - index_in_caller: 1, - index_in_callee: 1, - is_signer: false, - is_writable: false, - }, - ]) - ); - } - #[test] fn test_program_entry_debug() { #[allow(clippy::unnecessary_wraps)] diff --git a/programs/bpf_loader/src/serialization.rs b/programs/bpf_loader/src/serialization.rs index 39c383994d..5a6106e167 100644 --- a/programs/bpf_loader/src/serialization.rs +++ b/programs/bpf_loader/src/serialization.rs @@ -14,24 +14,6 @@ use { std::{io::prelude::*, mem::size_of}, }; -/// Look for a duplicate account and return its position if found -pub fn is_duplicate( - instruction_context: &InstructionContext, - index_in_instruction: usize, -) -> Option { - let index_in_transaction = instruction_context.get_index_in_transaction(index_in_instruction); - let old_approach = (instruction_context.get_number_of_program_accounts()..index_in_instruction) - .position(|index_in_instruction| { - instruction_context.get_index_in_transaction(index_in_instruction) - == index_in_transaction - }); - let result = instruction_context - .is_duplicate(index_in_instruction) - .unwrap_or(None); - debug_assert_eq!(result, old_approach); - old_approach -} - pub fn serialize_parameters( transaction_context: &TransactionContext, instruction_context: &InstructionContext, @@ -97,7 +79,7 @@ pub fn serialize_parameters_unaligned( for index_in_instruction in instruction_context.get_number_of_program_accounts() ..instruction_context.get_number_of_accounts() { - let duplicate = is_duplicate(instruction_context, index_in_instruction); + let duplicate = instruction_context.is_duplicate(index_in_instruction)?; size += 1; // dup if duplicate.is_none() { let data_len = instruction_context @@ -125,7 +107,7 @@ pub fn serialize_parameters_unaligned( for index_in_instruction in instruction_context.get_number_of_program_accounts() ..instruction_context.get_number_of_accounts() { - let duplicate = is_duplicate(instruction_context, index_in_instruction); + let duplicate = instruction_context.is_duplicate(index_in_instruction)?; if let Some(position) = duplicate { v.write_u8(position as u8) .map_err(|_| InstructionError::InvalidArgument)?; @@ -179,7 +161,7 @@ pub fn deserialize_parameters_unaligned( ..instruction_context.get_number_of_accounts()) .zip(account_lengths.iter()) { - let duplicate = is_duplicate(instruction_context, index_in_instruction); + let duplicate = instruction_context.is_duplicate(index_in_instruction)?; start += 1; // is_dup if duplicate.is_none() { let mut borrowed_account = instruction_context @@ -217,7 +199,7 @@ pub fn serialize_parameters_aligned( for index_in_instruction in instruction_context.get_number_of_program_accounts() ..instruction_context.get_number_of_accounts() { - let duplicate = is_duplicate(instruction_context, index_in_instruction); + let duplicate = instruction_context.is_duplicate(index_in_instruction)?; size += 1; // dup if duplicate.is_some() { size += 7; // padding to 64-bit aligned @@ -251,7 +233,7 @@ pub fn serialize_parameters_aligned( for index_in_instruction in instruction_context.get_number_of_program_accounts() ..instruction_context.get_number_of_accounts() { - let duplicate = is_duplicate(instruction_context, index_in_instruction); + let duplicate = instruction_context.is_duplicate(index_in_instruction)?; if let Some(position) = duplicate { v.write_u8(position as u8) .map_err(|_| InstructionError::InvalidArgument)?; @@ -316,7 +298,7 @@ pub fn deserialize_parameters_aligned( ..instruction_context.get_number_of_accounts()) .zip(account_lengths.iter()) { - let duplicate = is_duplicate(instruction_context, index_in_instruction); + let duplicate = instruction_context.is_duplicate(index_in_instruction)?; start += size_of::(); // position if duplicate.is_some() { start += 7; // padding to 64-bit aligned diff --git a/programs/bpf_loader/src/syscalls.rs b/programs/bpf_loader/src/syscalls.rs index e3faf83acc..d6299e8b9d 100644 --- a/programs/bpf_loader/src/syscalls.rs +++ b/programs/bpf_loader/src/syscalls.rs @@ -3,7 +3,7 @@ use { crate::{allocator_bump::BpfAllocator, BpfError}, solana_program_runtime::{ ic_logger_msg, ic_msg, - invoke_context::{visit_each_account_once, ComputeMeter, InvokeContext}, + invoke_context::{ComputeMeter, InvokeContext}, stable_log, timings::ExecuteTimings, }, @@ -2964,80 +2964,75 @@ where ))?; accounts.push((*program_account_index, None)); - visit_each_account_once::>( - instruction_accounts, - &mut |_index: usize, instruction_account: &InstructionAccount| { - let account = invoke_context - .transaction_context - .get_account_at_index(instruction_account.index_in_transaction) - .map_err(SyscallError::InstructionError)?; - let account_key = invoke_context - .transaction_context - .get_key_of_account_at_index(instruction_account.index_in_transaction) - .map_err(SyscallError::InstructionError)?; - if account.borrow().executable() { - // Use the known account - if invoke_context - .feature_set - .is_active(&executables_incur_cpi_data_cost::id()) - { - invoke_context - .get_compute_meter() - .consume((account.borrow().data().len() as u64).saturating_div( - invoke_context.get_compute_budget().cpi_bytes_per_unit, - ))?; - } - accounts.push((instruction_account.index_in_transaction, None)); - } else if let Some(caller_account_index) = - account_info_keys.iter().position(|key| *key == account_key) + for (index_in_instruction, instruction_account) in instruction_accounts.iter().enumerate() { + if index_in_instruction != instruction_account.index_in_callee { + continue; // Skip duplicate account + } + let account = invoke_context + .transaction_context + .get_account_at_index(instruction_account.index_in_transaction) + .map_err(SyscallError::InstructionError)?; + let account_key = invoke_context + .transaction_context + .get_key_of_account_at_index(instruction_account.index_in_transaction) + .map_err(SyscallError::InstructionError)?; + if account.borrow().executable() { + // Use the known account + if invoke_context + .feature_set + .is_active(&executables_incur_cpi_data_cost::id()) { - let mut caller_account = do_translate( - account_infos - .get(caller_account_index) - .ok_or(SyscallError::InvalidLength)?, - invoke_context, + invoke_context.get_compute_meter().consume( + (account.borrow().data().len() as u64) + .saturating_div(invoke_context.get_compute_budget().cpi_bytes_per_unit), )?; - { - let mut account = account.borrow_mut(); - account.copy_into_owner_from_slice(caller_account.owner.as_ref()); - account.set_data_from_slice(caller_account.data); - account.set_lamports(*caller_account.lamports); - account.set_executable(caller_account.executable); - account.set_rent_epoch(caller_account.rent_epoch); - } - let caller_account = if instruction_account.is_writable { - let orig_data_lens = invoke_context - .get_orig_account_lengths() - .map_err(SyscallError::InstructionError)?; - caller_account.original_data_len = *orig_data_lens - .get(instruction_account.index_in_caller) - .ok_or_else(|| { - ic_msg!( - invoke_context, - "Internal error: index mismatch for account {}", - account_key - ); - SyscallError::InstructionError(InstructionError::MissingAccount) - })?; - Some(caller_account) - } else { - None - }; - accounts.push((instruction_account.index_in_transaction, caller_account)); - } else { - ic_msg!( - invoke_context, - "Instruction references an unknown account {}", - account_key - ); - return Err( - SyscallError::InstructionError(InstructionError::MissingAccount).into(), - ); } - Ok(()) - }, - SyscallError::InstructionError(InstructionError::NotEnoughAccountKeys).into(), - )?; + accounts.push((instruction_account.index_in_transaction, None)); + } else if let Some(caller_account_index) = + account_info_keys.iter().position(|key| *key == account_key) + { + let mut caller_account = do_translate( + account_infos + .get(caller_account_index) + .ok_or(SyscallError::InvalidLength)?, + invoke_context, + )?; + { + let mut account = account.borrow_mut(); + account.copy_into_owner_from_slice(caller_account.owner.as_ref()); + account.set_data_from_slice(caller_account.data); + account.set_lamports(*caller_account.lamports); + account.set_executable(caller_account.executable); + account.set_rent_epoch(caller_account.rent_epoch); + } + let caller_account = if instruction_account.is_writable { + let orig_data_lens = invoke_context + .get_orig_account_lengths() + .map_err(SyscallError::InstructionError)?; + caller_account.original_data_len = *orig_data_lens + .get(instruction_account.index_in_caller) + .ok_or_else(|| { + ic_msg!( + invoke_context, + "Internal error: index mismatch for account {}", + account_key + ); + SyscallError::InstructionError(InstructionError::MissingAccount) + })?; + Some(caller_account) + } else { + None + }; + accounts.push((instruction_account.index_in_transaction, caller_account)); + } else { + ic_msg!( + invoke_context, + "Instruction references an unknown account {}", + account_key + ); + return Err(SyscallError::InstructionError(InstructionError::MissingAccount).into()); + } + } Ok(accounts) }