From 6842f7ec4379724609ba6d87dcc5a1dd8a918cd1 Mon Sep 17 00:00:00 2001 From: frits-double-eye <31902851+frits-double-eye@users.noreply.github.com> Date: Wed, 24 Nov 2021 14:39:20 +0200 Subject: [PATCH] Revert "Refactor: Merge message processor into invoke context (#20308)" This reverts commit 0bd8330ff736cc5321fdab7ed446b6995b9151af. --- program-runtime/src/instruction_processor.rs | 2 +- runtime/src/bank.rs | 23 +- runtime/src/message_processor.rs | 734 +++++++++++-------- sdk/src/process_instruction.rs | 22 +- 4 files changed, 431 insertions(+), 350 deletions(-) diff --git a/program-runtime/src/instruction_processor.rs b/program-runtime/src/instruction_processor.rs index fcbf4c4837b1f9..97a086fa18acef 100644 --- a/program-runtime/src/instruction_processor.rs +++ b/program-runtime/src/instruction_processor.rs @@ -610,7 +610,7 @@ impl InstructionProcessor { message, instruction, program_indices, - Some(account_indices), + account_indices, )?; let mut instruction_processor = InstructionProcessor::default(); diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index c4316841dfb710..53e252d415489f 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -8,7 +8,7 @@ //! `Bank::process_transactions` //! //! It does this by loading the accounts using the reference it holds on the account store, -//! and then passing those to an InvokeContext which handles loading the programs specified +//! and then passing those to the message_processor which handles loading the programs specified //! by the Transaction and executing it. //! //! The bank then stores the results to the accounts store. @@ -65,7 +65,7 @@ use log::*; use rayon::ThreadPool; use solana_measure::measure::Measure; use solana_metrics::{datapoint_debug, inc_new_counter_debug, inc_new_counter_info}; -use solana_program_runtime::{ExecuteDetailsTimings, Executors, InstructionProcessor}; +use solana_program_runtime::{ExecuteDetailsTimings, Executors}; #[allow(deprecated)] use solana_sdk::recent_blockhashes_account; use solana_sdk::{ @@ -946,8 +946,8 @@ pub struct Bank { /// stream for the slot == self.slot is_delta: AtomicBool, - /// The InstructionProcessor - instruction_processor: InstructionProcessor, + /// The Message processor + message_processor: MessageProcessor, compute_budget: Option, @@ -1096,7 +1096,7 @@ impl Bank { stakes: RwLock::::default(), epoch_stakes: HashMap::::default(), is_delta: AtomicBool::default(), - instruction_processor: InstructionProcessor::default(), + message_processor: MessageProcessor::default(), compute_budget: Option::::default(), feature_builtins: Arc::>::default(), last_vote_sync: AtomicU64::default(), @@ -1328,7 +1328,7 @@ impl Bank { is_delta: AtomicBool::new(false), tick_height: AtomicU64::new(parent.tick_height.load(Relaxed)), signature_count: AtomicU64::new(0), - instruction_processor: parent.instruction_processor.clone(), + message_processor: parent.message_processor.clone(), compute_budget: parent.compute_budget, feature_builtins: parent.feature_builtins.clone(), hard_forks: parent.hard_forks.clone(), @@ -1483,7 +1483,7 @@ impl Bank { stakes: RwLock::new(fields.stakes), epoch_stakes: fields.epoch_stakes, is_delta: AtomicBool::new(fields.is_delta), - instruction_processor: new(), + message_processor: new(), compute_budget: None, feature_builtins: new(), last_vote_sync: new(), @@ -3406,8 +3406,7 @@ impl Bank { }; if let Some(legacy_message) = tx.message().legacy_message() { - process_result = MessageProcessor::process_message( - &self.instruction_processor, + process_result = self.message_processor.process_message( legacy_message, &loaded_transaction.program_indices, &account_refcells, @@ -5348,7 +5347,7 @@ impl Bank { ) { debug!("Adding program {} under {:?}", name, program_id); self.add_builtin_account(name, program_id, false); - self.instruction_processor + self.message_processor .add_program(program_id, process_instruction_with_context); debug!("Added program {} under {:?}", name, program_id); } @@ -5362,7 +5361,7 @@ impl Bank { ) { debug!("Replacing program {} under {:?}", name, program_id); self.add_builtin_account(name, program_id, true); - self.instruction_processor + self.message_processor .add_program(program_id, process_instruction_with_context); debug!("Replaced program {} under {:?}", name, program_id); } @@ -5372,7 +5371,7 @@ impl Bank { debug!("Removing program {} under {:?}", name, program_id); // Don't remove the account since the bank expects the account state to // be idempotent - self.instruction_processor.remove_program(program_id); + self.message_processor.remove_program(program_id); debug!("Removed program {} under {:?}", name, program_id); } diff --git a/runtime/src/message_processor.rs b/runtime/src/message_processor.rs index e48cda806f9eea..ca97b372c7f266 100644 --- a/runtime/src/message_processor.rs +++ b/runtime/src/message_processor.rs @@ -48,7 +48,6 @@ impl ComputeMeter for ThisComputeMeter { } } pub struct ThisInvokeContext<'a> { - instruction_index: usize, invoke_stack: Vec>, rent: Rent, pre_accounts: Vec, @@ -60,7 +59,7 @@ pub struct ThisInvokeContext<'a> { bpf_compute_budget: solana_sdk::process_instruction::BpfComputeBudget, compute_meter: Rc>, executors: Rc>, - instruction_recorders: Option<&'a [InstructionRecorder]>, + instruction_recorder: Option, feature_set: Arc, pub timings: ExecuteDetailsTimings, account_db: Arc, @@ -75,25 +74,36 @@ pub struct ThisInvokeContext<'a> { impl<'a> ThisInvokeContext<'a> { #[allow(clippy::too_many_arguments)] pub fn new( + program_id: &Pubkey, rent: Rent, + message: &'a Message, + instruction: &'a CompiledInstruction, + program_indices: &[usize], accounts: &'a [(Pubkey, Rc>)], programs: &'a [(Pubkey, ProcessInstructionWithContext)], log_collector: Option>, compute_budget: ComputeBudget, compute_meter: Rc>, executors: Rc>, - instruction_recorders: Option<&'a [InstructionRecorder]>, + instruction_recorder: Option, feature_set: Arc, account_db: Arc, ancestors: &'a Ancestors, blockhash: &'a Hash, fee_calculator: &'a FeeCalculator, - ) -> Self { - Self { - instruction_index: 0, + ) -> Result { + let pre_accounts = MessageProcessor::create_pre_accounts(message, instruction, accounts); + let compute_meter = if feature_set.is_active(&tx_wide_compute_cap::id()) { + compute_meter + } else { + Rc::new(RefCell::new(ThisComputeMeter { + remaining: compute_budget.max_units, + })) + }; + let mut invoke_context = Self { invoke_stack: Vec::with_capacity(compute_budget.max_invoke_depth), rent, - pre_accounts: Vec::new(), + pre_accounts, accounts, programs, logger: Rc::new(RefCell::new(ThisLogger { log_collector })), @@ -101,7 +111,7 @@ impl<'a> ThisInvokeContext<'a> { bpf_compute_budget: compute_budget.into(), compute_meter, executors, - instruction_recorders, + instruction_recorder, feature_set, timings: ExecuteDetailsTimings::default(), account_db, @@ -110,7 +120,16 @@ impl<'a> ThisInvokeContext<'a> { blockhash, fee_calculator, return_data: None, - } + }; + let account_indices = (0..accounts.len()).collect::>(); + invoke_context.push( + program_id, + message, + instruction, + program_indices, + &account_indices, + )?; + Ok(invoke_context) } } impl<'a> InvokeContext for ThisInvokeContext<'a> { @@ -120,27 +139,12 @@ impl<'a> InvokeContext for ThisInvokeContext<'a> { message: &Message, instruction: &CompiledInstruction, program_indices: &[usize], - account_indices: Option<&[usize]>, + account_indices: &[usize], ) -> Result<(), InstructionError> { if self.invoke_stack.len() > self.compute_budget.max_invoke_depth { return Err(InstructionError::CallDepth); } - if self.invoke_stack.is_empty() { - self.pre_accounts = Vec::with_capacity(instruction.accounts.len()); - let mut work = |_unique_index: usize, account_index: usize| { - if account_index < message.account_keys.len() && account_index < self.accounts.len() - { - let account = self.accounts[account_index].1.borrow(); - self.pre_accounts - .push(PreAccount::new(&self.accounts[account_index].0, &account)); - return Ok(()); - } - Err(InstructionError::MissingAccount) - }; - instruction.visit_each_account(&mut work)?; - } - let contains = self.invoke_stack.iter().any(|frame| frame.key == *key); let is_last = if let Some(last_frame) = self.invoke_stack.last() { last_frame.key == *key @@ -168,11 +172,7 @@ impl<'a> InvokeContext for ThisInvokeContext<'a> { }) .chain(instruction.accounts.iter().map(|index_in_instruction| { let index_in_instruction = *index_in_instruction as usize; - let account_index = if let Some(account_indices) = account_indices { - account_indices[index_in_instruction] - } else { - index_in_instruction - }; + let account_index = account_indices[index_in_instruction]; ( message.is_signer(index_in_instruction), message.is_writable(index_in_instruction, demote_program_write_locks), @@ -193,66 +193,6 @@ impl<'a> InvokeContext for ThisInvokeContext<'a> { fn invoke_depth(&self) -> usize { self.invoke_stack.len() } - fn verify( - &mut self, - message: &Message, - instruction: &CompiledInstruction, - program_indices: &[usize], - ) -> Result<(), InstructionError> { - let program_id = instruction.program_id(&message.account_keys); - let demote_program_write_locks = self.is_feature_active(&demote_program_write_locks::id()); - let do_support_realloc = self.is_feature_active(&do_support_realloc::id()); - - // Verify all executable accounts have zero outstanding refs - for account_index in program_indices.iter() { - self.accounts[*account_index] - .1 - .try_borrow_mut() - .map_err(|_| InstructionError::AccountBorrowOutstanding)?; - } - - // Verify the per-account instruction results - let (mut pre_sum, mut post_sum) = (0_u128, 0_u128); - let mut work = |unique_index: usize, account_index: usize| { - { - // Verify account has no outstanding references - let _ = self.accounts[account_index] - .1 - .try_borrow_mut() - .map_err(|_| InstructionError::AccountBorrowOutstanding)?; - } - let account = self.accounts[account_index].1.borrow(); - self.pre_accounts[unique_index] - .verify( - program_id, - message.is_writable(account_index, demote_program_write_locks), - &self.rent, - &account, - &mut self.timings, - true, - do_support_realloc, - ) - .map_err(|err| { - ic_logger_msg!( - self.logger, - "failed to verify account {}: {}", - self.pre_accounts[unique_index].key(), - err - ); - err - })?; - pre_sum += u128::from(self.pre_accounts[unique_index].lamports()); - post_sum += u128::from(account.lamports()); - Ok(()) - }; - instruction.visit_each_account(&mut work)?; - - // Verify that the total sum of all the lamports did not change - if pre_sum != post_sum { - return Err(InstructionError::UnbalancedInstruction); - } - Ok(()) - } fn verify_and_update( &mut self, instruction: &CompiledInstruction, @@ -266,7 +206,7 @@ impl<'a> InvokeContext for ThisInvokeContext<'a> { .ok_or(InstructionError::CallDepth)?; let program_id = &stack_frame.key; let rent = &self.rent; - let logger = &self.logger; + let logger = self.get_logger(); let accounts = &self.accounts; let pre_accounts = &mut self.pre_accounts; let timings = &mut self.timings; @@ -363,17 +303,9 @@ impl<'a> InvokeContext for ThisInvokeContext<'a> { fn get_executor(&self, pubkey: &Pubkey) -> Option> { self.executors.borrow().get(pubkey) } - fn set_instruction_index(&mut self, instruction_index: usize) { - if !self.feature_set.is_active(&tx_wide_compute_cap::id()) { - self.compute_meter = Rc::new(RefCell::new(ThisComputeMeter { - remaining: self.compute_budget.max_units, - })); - } - self.instruction_index = instruction_index; - } fn record_instruction(&self, instruction: &Instruction) { - if let Some(instruction_recorders) = &self.instruction_recorders { - instruction_recorders[self.instruction_index].record_instruction(instruction.clone()); + if let Some(recorder) = &self.instruction_recorder { + recorder.record_instruction(instruction.clone()); } } fn is_feature_active(&self, feature_id: &Pubkey) -> bool { @@ -451,7 +383,10 @@ impl Logger for ThisLogger { } #[derive(Debug, Default, Clone, Deserialize, Serialize)] -pub struct MessageProcessor {} +pub struct MessageProcessor { + #[serde(skip)] + instruction_processor: InstructionProcessor, +} #[cfg(RUSTC_WITH_SPECIALIZATION)] impl ::solana_frozen_abi::abi_example::AbiExample for MessageProcessor { @@ -463,111 +398,281 @@ impl ::solana_frozen_abi::abi_example::AbiExample for MessageProcessor { } impl MessageProcessor { - /// Process a message. - /// This method calls each instruction in the message over the set of loaded accounts. - /// For each instruction it calls the program entrypoint method and verifies that the result of + /// Add a static entrypoint to intercept instructions before the dynamic loader. + pub fn add_program( + &mut self, + program_id: &Pubkey, + process_instruction: ProcessInstructionWithContext, + ) { + self.instruction_processor + .add_program(program_id, process_instruction); + } + + /// Remove a program. + pub fn remove_program(&mut self, program_id: &Pubkey) { + self.instruction_processor.remove_program(program_id); + } + + /// Record the initial state of the accounts so that they can be compared + /// after the instruction is processed + pub fn create_pre_accounts( + message: &Message, + instruction: &CompiledInstruction, + accounts: &[(Pubkey, Rc>)], + ) -> Vec { + let mut pre_accounts = Vec::with_capacity(instruction.accounts.len()); + { + let mut work = |_unique_index: usize, account_index: usize| { + if account_index < message.account_keys.len() && account_index < accounts.len() { + let account = accounts[account_index].1.borrow(); + pre_accounts.push(PreAccount::new(&accounts[account_index].0, &account)); + return Ok(()); + } + Err(InstructionError::MissingAccount) + }; + let _ = instruction.visit_each_account(&mut work); + } + pre_accounts + } + + /// Verify there are no outstanding borrows + pub fn verify_account_references( + accounts: &[(Pubkey, Rc>)], + program_indices: &[usize], + ) -> Result<(), InstructionError> { + for account_index in program_indices.iter() { + accounts[*account_index] + .1 + .try_borrow_mut() + .map_err(|_| InstructionError::AccountBorrowOutstanding)?; + } + Ok(()) + } + + /// Verify the results of an instruction + #[allow(clippy::too_many_arguments)] + pub fn verify( + message: &Message, + instruction: &CompiledInstruction, + pre_accounts: &[PreAccount], + program_indices: &[usize], + accounts: &[(Pubkey, Rc>)], + rent: &Rent, + timings: &mut ExecuteDetailsTimings, + logger: Rc>, + demote_program_write_locks: bool, + do_support_realloc: bool, + ) -> Result<(), InstructionError> { + // Verify all executable accounts have zero outstanding refs + Self::verify_account_references(accounts, program_indices)?; + + // Verify the per-account instruction results + let (mut pre_sum, mut post_sum) = (0_u128, 0_u128); + { + let program_id = instruction.program_id(&message.account_keys); + let mut work = |unique_index: usize, account_index: usize| { + { + // Verify account has no outstanding references + let _ = accounts[account_index] + .1 + .try_borrow_mut() + .map_err(|_| InstructionError::AccountBorrowOutstanding)?; + } + let account = accounts[account_index].1.borrow(); + pre_accounts[unique_index] + .verify( + program_id, + message.is_writable(account_index, demote_program_write_locks), + rent, + &account, + timings, + true, + do_support_realloc, + ) + .map_err(|err| { + ic_logger_msg!( + logger, + "failed to verify account {}: {}", + pre_accounts[unique_index].key(), + err + ); + err + })?; + pre_sum += u128::from(pre_accounts[unique_index].lamports()); + post_sum += u128::from(account.lamports()); + Ok(()) + }; + instruction.visit_each_account(&mut work)?; + } + + // Verify that the total sum of all the lamports did not change + if pre_sum != post_sum { + return Err(InstructionError::UnbalancedInstruction); + } + Ok(()) + } + + /// Execute an instruction + /// This method calls the instruction's program entrypoint method and verifies that the result of /// the call does not violate the bank's accounting rules. - /// The accounts are committed back to the bank only if every instruction succeeds. + /// The accounts are committed back to the bank only if this function returns Ok(_). #[allow(clippy::too_many_arguments)] - #[allow(clippy::type_complexity)] - pub fn process_message( - instruction_processor: &InstructionProcessor, + fn execute_instruction( + &self, message: &Message, - program_indices: &[Vec], + instruction: &CompiledInstruction, + program_indices: &[usize], accounts: &[(Pubkey, Rc>)], rent_collector: &RentCollector, log_collector: Option>, executors: Rc>, - instruction_recorders: Option<&[InstructionRecorder]>, + instruction_recorder: Option, + instruction_index: usize, feature_set: Arc, compute_budget: ComputeBudget, compute_meter: Rc>, timings: &mut ExecuteDetailsTimings, account_db: Arc, ancestors: &Ancestors, - blockhash: Hash, - fee_calculator: FeeCalculator, - ) -> Result<(), TransactionError> { + blockhash: &Hash, + fee_calculator: &FeeCalculator, + ) -> Result<(), InstructionError> { + let program_id = instruction.program_id(&message.account_keys); + if feature_set.is_active(&prevent_calling_precompiles_as_programs::id()) + && is_precompile(program_id, |id| feature_set.is_active(id)) + { + // Precompiled programs don't have an instruction processor + return Ok(()); + } + + // Fixup the special instructions key if present + // before the account pre-values are taken care of + for (pubkey, accont) in accounts.iter().take(message.account_keys.len()) { + if instructions::check_id(pubkey) { + let mut mut_account_ref = accont.borrow_mut(); + instructions::store_current_index( + mut_account_ref.data_as_mut_slice(), + instruction_index as u16, + ); + break; + } + } + + let program_id = instruction.program_id(&message.account_keys); + + let mut compute_budget = compute_budget; + if feature_set.is_active(&neon_evm_compute_budget::id()) + && *program_id == crate::neon_evm_program::id() + { + // Bump the compute budget for neon_evm + compute_budget.max_units = compute_budget.max_units.max(500_000); + compute_budget.heap_size = Some(256 * 1024); + } + + let programs = self.instruction_processor.programs(); let mut invoke_context = ThisInvokeContext::new( + program_id, rent_collector.rent, + message, + instruction, + program_indices, accounts, - instruction_processor.programs(), + programs, log_collector, compute_budget, compute_meter, executors, - instruction_recorders, + instruction_recorder, feature_set, account_db, ancestors, - &blockhash, - &fee_calculator, - ); - let compute_meter = invoke_context.get_compute_meter(); - for (instruction_index, (instruction, program_indices)) in message - .instructions - .iter() - .zip(program_indices.iter()) - .enumerate() - { - let program_id = instruction.program_id(&message.account_keys); - if invoke_context.is_feature_active(&prevent_calling_precompiles_as_programs::id()) - && is_precompile(program_id, |id| invoke_context.is_feature_active(id)) - { - // Precompiled programs don't have an instruction processor - continue; - } - - let mut time = Measure::start("execute_instruction"); - let pre_remaining_units = compute_meter.borrow().get_remaining(); + blockhash, + fee_calculator, + )?; + + self.instruction_processor.process_instruction( + program_id, + &instruction.data, + &mut invoke_context, + )?; + Self::verify( + message, + instruction, + &invoke_context.pre_accounts, + program_indices, + accounts, + &rent_collector.rent, + timings, + invoke_context.get_logger(), + invoke_context.is_feature_active(&demote_program_write_locks::id()), + invoke_context.is_feature_active(&do_support_realloc::id()), + )?; - // Fixup the special instructions key if present - // before the account pre-values are taken care of - for (pubkey, accont) in accounts.iter().take(message.account_keys.len()) { - if instructions::check_id(pubkey) { - let mut mut_account_ref = accont.borrow_mut(); - instructions::store_current_index( - mut_account_ref.data_as_mut_slice(), - instruction_index as u16, - ); - break; - } - } + timings.accumulate(&invoke_context.timings); - let mut compute_budget = compute_budget; - if invoke_context.is_feature_active(&neon_evm_compute_budget::id()) - && *program_id == crate::neon_evm_program::id() - { - // Bump the compute budget for neon_evm - compute_budget.max_units = compute_budget.max_units.max(500_000); - compute_budget.heap_size = Some(256 * 1024); - } + Ok(()) + } - invoke_context.set_instruction_index(instruction_index); - let result = invoke_context - .push(program_id, message, instruction, program_indices, None) - .and_then(|_| { - instruction_processor.process_instruction( - program_id, - &instruction.data, - &mut invoke_context, - )?; - invoke_context.verify(message, instruction, program_indices)?; - timings.accumulate(&invoke_context.timings); - Ok(()) - }) + /// Process a message. + /// This method calls each instruction in the message over the set of loaded Accounts + /// The accounts are committed back to the bank only if every instruction succeeds + #[allow(clippy::too_many_arguments)] + #[allow(clippy::type_complexity)] + pub fn process_message( + &self, + message: &Message, + program_indices: &[Vec], + accounts: &[(Pubkey, Rc>)], + rent_collector: &RentCollector, + log_collector: Option>, + executors: Rc>, + instruction_recorders: Option<&[InstructionRecorder]>, + feature_set: Arc, + compute_budget: ComputeBudget, + compute_meter: Rc>, + timings: &mut ExecuteDetailsTimings, + account_db: Arc, + ancestors: &Ancestors, + blockhash: Hash, + fee_calculator: FeeCalculator, + ) -> Result<(), TransactionError> { + for (instruction_index, instruction) in message.instructions.iter().enumerate() { + let mut time = Measure::start("execute_instruction"); + let pre_remaining_units = compute_meter.borrow().get_remaining(); + let instruction_recorder = instruction_recorders + .as_ref() + .map(|recorders| recorders[instruction_index].clone()); + let err = self + .execute_instruction( + message, + instruction, + &program_indices[instruction_index], + accounts, + rent_collector, + log_collector.clone(), + executors.clone(), + instruction_recorder, + instruction_index, + feature_set.clone(), + compute_budget, + compute_meter.clone(), + timings, + account_db.clone(), + ancestors, + &blockhash, + &fee_calculator, + ) .map_err(|err| TransactionError::InstructionError(instruction_index as u8, err)); - invoke_context.pop(); - time.stop(); let post_remaining_units = compute_meter.borrow().get_remaining(); + timings.accumulate_program( instruction.program_id(&message.account_keys), time.as_us(), pre_remaining_units - post_remaining_units, ); - result?; + err?; } Ok(()) } @@ -585,47 +690,6 @@ mod tests { secp256k1_program, }; - #[derive(Debug, Serialize, Deserialize)] - enum MockInstruction { - NoopSuccess, - NoopFail, - ModifyOwned, - ModifyNotOwned, - ModifyReadonly, - } - - fn mock_process_instruction( - program_id: &Pubkey, - data: &[u8], - invoke_context: &mut dyn InvokeContext, - ) -> Result<(), InstructionError> { - let keyed_accounts = invoke_context.get_keyed_accounts()?; - assert_eq!(*program_id, keyed_accounts[0].owner()?); - assert_ne!( - keyed_accounts[1].owner()?, - *keyed_accounts[0].unsigned_key() - ); - - if let Ok(instruction) = bincode::deserialize(data) { - match instruction { - MockInstruction::NoopSuccess => (), - MockInstruction::NoopFail => return Err(InstructionError::GenericError), - MockInstruction::ModifyOwned => { - keyed_accounts[0].try_account_ref_mut()?.data_as_mut_slice()[0] = 1 - } - MockInstruction::ModifyNotOwned => { - keyed_accounts[1].try_account_ref_mut()?.data_as_mut_slice()[0] = 1 - } - MockInstruction::ModifyReadonly => { - keyed_accounts[2].try_account_ref_mut()?.data_as_mut_slice()[0] = 1 - } - } - } else { - return Err(InstructionError::InvalidInstructionData); - } - Ok(()) - } - #[test] fn test_invoke_context() { const MAX_DEPTH: usize = 10; @@ -665,7 +729,11 @@ mod tests { let blockhash = Hash::default(); let fee_calculator = FeeCalculator::default(); let mut invoke_context = ThisInvokeContext::new( + &invoke_stack[0], Rent::default(), + &message, + &message.instructions[0], + &[], &accounts, &[], None, @@ -678,13 +746,20 @@ mod tests { &ancestors, &blockhash, &fee_calculator, - ); + ) + .unwrap(); // Check call depth increases and has a limit - let mut depth_reached = 0; - for program_id in invoke_stack.iter() { + let mut depth_reached = 1; + for program_id in invoke_stack.iter().skip(1) { if Err(InstructionError::CallDepth) - == invoke_context.push(program_id, &message, &message.instructions[0], &[], None) + == invoke_context.push( + program_id, + &message, + &message.instructions[0], + &[], + &account_indices, + ) { break; } @@ -747,53 +822,17 @@ mod tests { } #[test] - fn test_invoke_context_verify() { + fn test_verify_account_references() { let accounts = vec![( solana_sdk::pubkey::new_rand(), Rc::new(RefCell::new(AccountSharedData::default())), )]; - let message = Message::new( - &[Instruction::new_with_bincode( - accounts[0].0, - &MockInstruction::NoopSuccess, - vec![AccountMeta::new_readonly(accounts[0].0, false)], - )], - None, - ); - let ancestors = Ancestors::default(); - let blockhash = Hash::default(); - let fee_calculator = FeeCalculator::default(); - let mut invoke_context = ThisInvokeContext::new( - Rent::default(), - &accounts, - &[], - None, - ComputeBudget::default(), - Rc::new(RefCell::new(MockComputeMeter::default())), - Rc::new(RefCell::new(Executors::default())), - None, - Arc::new(FeatureSet::all_enabled()), - Arc::new(Accounts::default_for_tests()), - &ancestors, - &blockhash, - &fee_calculator, - ); - invoke_context - .push( - &accounts[0].0, - &message, - &message.instructions[0], - &[0], - None, - ) - .unwrap(); - assert!(invoke_context - .verify(&message, &message.instructions[0], &[0]) - .is_ok()); + + assert!(MessageProcessor::verify_account_references(&accounts, &[0]).is_ok()); let mut _borrowed = accounts[0].1.borrow(); assert_eq!( - invoke_context.verify(&message, &message.instructions[0], &[0]), + MessageProcessor::verify_account_references(&accounts, &[0]), Err(InstructionError::AccountBorrowOutstanding) ); } @@ -840,8 +879,8 @@ mod tests { let mock_system_program_id = Pubkey::new(&[2u8; 32]); let rent_collector = RentCollector::default(); - let mut instruction_processor = InstructionProcessor::default(); - instruction_processor.add_program(&mock_system_program_id, mock_system_process_instruction); + let mut message_processor = MessageProcessor::default(); + message_processor.add_program(&mock_system_program_id, mock_system_process_instruction); let program_account = Rc::new(RefCell::new(create_loadable_account_for_test( "mock_system_program", @@ -875,8 +914,7 @@ mod tests { Some(&accounts[0].0), ); - let result = MessageProcessor::process_message( - &instruction_processor, + let result = message_processor.process_message( &message, &program_indices, &accounts, @@ -906,8 +944,7 @@ mod tests { Some(&accounts[0].0), ); - let result = MessageProcessor::process_message( - &instruction_processor, + let result = message_processor.process_message( &message, &program_indices, &accounts, @@ -941,8 +978,7 @@ mod tests { Some(&accounts[0].0), ); - let result = MessageProcessor::process_message( - &instruction_processor, + let result = message_processor.process_message( &message, &program_indices, &accounts, @@ -1031,8 +1067,8 @@ mod tests { let mock_program_id = Pubkey::new(&[2u8; 32]); let rent_collector = RentCollector::default(); - let mut instruction_processor = InstructionProcessor::default(); - instruction_processor.add_program(&mock_program_id, mock_system_process_instruction); + let mut message_processor = MessageProcessor::default(); + message_processor.add_program(&mock_program_id, mock_system_process_instruction); let program_account = Rc::new(RefCell::new(create_loadable_account_for_test( "mock_system_program", @@ -1068,8 +1104,7 @@ mod tests { )], Some(&accounts[0].0), ); - let result = MessageProcessor::process_message( - &instruction_processor, + let result = message_processor.process_message( &message, &program_indices, &accounts, @@ -1103,8 +1138,7 @@ mod tests { )], Some(&accounts[0].0), ); - let result = MessageProcessor::process_message( - &instruction_processor, + let result = message_processor.process_message( &message, &program_indices, &accounts, @@ -1136,8 +1170,7 @@ mod tests { Some(&accounts[0].0), ); let ancestors = Ancestors::default(); - let result = MessageProcessor::process_message( - &instruction_processor, + let result = message_processor.process_message( &message, &program_indices, &accounts, @@ -1162,6 +1195,47 @@ mod tests { #[test] fn test_process_cross_program() { + #[derive(Debug, Serialize, Deserialize)] + enum MockInstruction { + NoopSuccess, + NoopFail, + ModifyOwned, + ModifyNotOwned, + ModifyReadonly, + } + + fn mock_process_instruction( + program_id: &Pubkey, + data: &[u8], + invoke_context: &mut dyn InvokeContext, + ) -> Result<(), InstructionError> { + let keyed_accounts = invoke_context.get_keyed_accounts()?; + assert_eq!(*program_id, keyed_accounts[0].owner()?); + assert_ne!( + keyed_accounts[1].owner()?, + *keyed_accounts[0].unsigned_key() + ); + + if let Ok(instruction) = bincode::deserialize(data) { + match instruction { + MockInstruction::NoopSuccess => (), + MockInstruction::NoopFail => return Err(InstructionError::GenericError), + MockInstruction::ModifyOwned => { + keyed_accounts[0].try_account_ref_mut()?.data_as_mut_slice()[0] = 1 + } + MockInstruction::ModifyNotOwned => { + keyed_accounts[1].try_account_ref_mut()?.data_as_mut_slice()[0] = 1 + } + MockInstruction::ModifyReadonly => { + keyed_accounts[2].try_account_ref_mut()?.data_as_mut_slice()[0] = 1 + } + } + } else { + return Err(InstructionError::InvalidInstructionData); + } + Ok(()) + } + let caller_program_id = solana_sdk::pubkey::new_rand(); let callee_program_id = solana_sdk::pubkey::new_rand(); @@ -1171,6 +1245,7 @@ mod tests { let mut program_account = AccountSharedData::new(1, 0, &native_loader::id()); program_account.set_executable(true); + #[allow(unused_mut)] let accounts = vec![ ( solana_sdk::pubkey::new_rand(), @@ -1212,7 +1287,11 @@ mod tests { let blockhash = Hash::default(); let fee_calculator = FeeCalculator::default(); let mut invoke_context = ThisInvokeContext::new( + &caller_program_id, Rent::default(), + &message, + &caller_instruction, + &program_indices, &accounts, programs.as_slice(), None, @@ -1225,16 +1304,8 @@ mod tests { &ancestors, &blockhash, &fee_calculator, - ); - invoke_context - .push( - &caller_program_id, - &message, - &caller_instruction, - &program_indices, - None, - ) - .unwrap(); + ) + .unwrap(); // not owned account modified by the caller (before the invoke) let caller_write_privileges = message @@ -1292,7 +1363,11 @@ mod tests { let blockhash = Hash::default(); let fee_calculator = FeeCalculator::default(); let mut invoke_context = ThisInvokeContext::new( + &caller_program_id, Rent::default(), + &message, + &caller_instruction, + &program_indices, &accounts, programs.as_slice(), None, @@ -1305,16 +1380,8 @@ mod tests { &ancestors, &blockhash, &fee_calculator, - ); - invoke_context - .push( - &caller_program_id, - &message, - &caller_instruction, - &program_indices, - None, - ) - .unwrap(); + ) + .unwrap(); let caller_write_privileges = message .account_keys @@ -1337,6 +1404,47 @@ mod tests { #[test] fn test_native_invoke() { + #[derive(Debug, Serialize, Deserialize)] + enum MockInstruction { + NoopSuccess, + NoopFail, + ModifyOwned, + ModifyNotOwned, + ModifyReadonly, + } + + fn mock_process_instruction( + program_id: &Pubkey, + data: &[u8], + invoke_context: &mut dyn InvokeContext, + ) -> Result<(), InstructionError> { + let keyed_accounts = invoke_context.get_keyed_accounts()?; + assert_eq!(*program_id, keyed_accounts[0].owner()?); + assert_ne!( + keyed_accounts[1].owner()?, + *keyed_accounts[0].unsigned_key() + ); + + if let Ok(instruction) = bincode::deserialize(data) { + match instruction { + MockInstruction::NoopSuccess => (), + MockInstruction::NoopFail => return Err(InstructionError::GenericError), + MockInstruction::ModifyOwned => { + keyed_accounts[0].try_account_ref_mut()?.data_as_mut_slice()[0] = 1 + } + MockInstruction::ModifyNotOwned => { + keyed_accounts[1].try_account_ref_mut()?.data_as_mut_slice()[0] = 1 + } + MockInstruction::ModifyReadonly => { + keyed_accounts[2].try_account_ref_mut()?.data_as_mut_slice()[0] = 1 + } + } + } else { + return Err(InstructionError::InvalidInstructionData); + } + Ok(()) + } + let caller_program_id = solana_sdk::pubkey::new_rand(); let callee_program_id = solana_sdk::pubkey::new_rand(); @@ -1346,6 +1454,7 @@ mod tests { let mut program_account = AccountSharedData::new(1, 0, &native_loader::id()); program_account.set_executable(true); + #[allow(unused_mut)] let accounts = vec![ ( solana_sdk::pubkey::new_rand(), @@ -1383,7 +1492,11 @@ mod tests { let blockhash = Hash::default(); let fee_calculator = FeeCalculator::default(); let mut invoke_context = ThisInvokeContext::new( + &caller_program_id, Rent::default(), + &message, + &caller_instruction, + &program_indices, &accounts, programs.as_slice(), None, @@ -1396,16 +1509,8 @@ mod tests { &ancestors, &blockhash, &fee_calculator, - ); - invoke_context - .push( - &caller_program_id, - &message, - &caller_instruction, - &program_indices, - None, - ) - .unwrap(); + ) + .unwrap(); // not owned account modified by the invoker accounts[0].1.borrow_mut().data_as_mut_slice()[0] = 1; @@ -1459,7 +1564,11 @@ mod tests { let blockhash = Hash::default(); let fee_calculator = FeeCalculator::default(); let mut invoke_context = ThisInvokeContext::new( + &caller_program_id, Rent::default(), + &message, + &caller_instruction, + &program_indices, &accounts, programs.as_slice(), None, @@ -1472,16 +1581,8 @@ mod tests { &ancestors, &blockhash, &fee_calculator, - ); - invoke_context - .push( - &caller_program_id, - &message, - &caller_instruction, - &program_indices, - None, - ) - .unwrap(); + ) + .unwrap(); assert_eq!( InstructionProcessor::native_invoke( @@ -1497,6 +1598,7 @@ mod tests { #[test] fn test_precompile() { + let mut message_processor = MessageProcessor::default(); let mock_program_id = Pubkey::new_unique(); fn mock_process_instruction( _program_id: &Pubkey, @@ -1505,8 +1607,7 @@ mod tests { ) -> Result<(), InstructionError> { Err(InstructionError::Custom(0xbabb1e)) } - let mut instruction_processor = InstructionProcessor::default(); - instruction_processor.add_program(&mock_program_id, mock_process_instruction); + message_processor.add_program(&mock_program_id, mock_process_instruction); let secp256k1_account = AccountSharedData::new_ref(1, 0, &native_loader::id()); secp256k1_account.borrow_mut().set_executable(true); @@ -1528,8 +1629,7 @@ mod tests { None, ); - let result = MessageProcessor::process_message( - &instruction_processor, + let result = message_processor.process_message( &message, &[vec![0], vec![1]], &accounts, diff --git a/sdk/src/process_instruction.rs b/sdk/src/process_instruction.rs index c711a65962fc14..afc665efac1b12 100644 --- a/sdk/src/process_instruction.rs +++ b/sdk/src/process_instruction.rs @@ -58,19 +58,12 @@ pub trait InvokeContext { message: &Message, instruction: &CompiledInstruction, program_indices: &[usize], - account_indices: Option<&[usize]>, + account_indices: &[usize], ) -> Result<(), InstructionError>; /// Pop a stack frame from the invocation stack fn pop(&mut self); /// Current depth of the invocation stake fn invoke_depth(&self) -> usize; - /// Verify the results of an instruction - fn verify( - &mut self, - message: &Message, - instruction: &CompiledInstruction, - program_indices: &[usize], - ) -> Result<(), InstructionError>; /// Verify and update PreAccount state based on program execution fn verify_and_update( &mut self, @@ -99,8 +92,6 @@ pub trait InvokeContext { fn add_executor(&self, pubkey: &Pubkey, executor: Arc); /// Get the completed loader work that can be re-used across executions fn get_executor(&self, pubkey: &Pubkey) -> Option>; - /// Set which instruction in the message is currently being recorded - fn set_instruction_index(&mut self, instruction_index: usize); /// Record invoked instruction fn record_instruction(&self, instruction: &Instruction); /// Get the bank's active feature set @@ -501,7 +492,7 @@ impl<'a> InvokeContext for MockInvokeContext<'a> { _message: &Message, _instruction: &CompiledInstruction, _program_indices: &[usize], - _account_indices: Option<&[usize]>, + _account_indices: &[usize], ) -> Result<(), InstructionError> { self.invoke_stack.push(InvokeContextStackFrame::new( *_key, @@ -515,14 +506,6 @@ impl<'a> InvokeContext for MockInvokeContext<'a> { fn invoke_depth(&self) -> usize { self.invoke_stack.len() } - fn verify( - &mut self, - _message: &Message, - _instruction: &CompiledInstruction, - _program_indices: &[usize], - ) -> Result<(), InstructionError> { - Ok(()) - } fn verify_and_update( &mut self, _instruction: &CompiledInstruction, @@ -570,7 +553,6 @@ impl<'a> InvokeContext for MockInvokeContext<'a> { fn get_executor(&self, _pubkey: &Pubkey) -> Option> { None } - fn set_instruction_index(&mut self, _instruction_index: usize) {} fn record_instruction(&self, _instruction: &Instruction) {} fn is_feature_active(&self, feature_id: &Pubkey) -> bool { !self.disabled_features.contains(feature_id)