Cleanup: invoke_context.rs (#22107)

* Removes message::Message from invoke_context.rs completely.

* Simplifies sol_invoke_signed() of program-test

* Start search for non program accounts at front (instead of the back).
Program and programdata accounts use rposition(), everything else uses position().

program-runtime/src/invoke_context.rs

@@ -16,7 +16,6 @@ use {
         hash::Hash,
         instruction::{AccountMeta, Instruction, InstructionError},
         keyed_account::{create_keyed_accounts_unified, keyed_account_at_index, KeyedAccount},
-        message::Message,
         native_loader,
         pubkey::Pubkey,
         rent::Rent,
@@ -28,7 +27,7 @@ use {
 pub type TransactionAccountRefCell = (Pubkey, RefCell<AccountSharedData>);
 pub type TransactionAccountRefCells = Vec<TransactionAccountRefCell>;
 
-#[derive(Debug)]
+#[derive(Clone, Debug)]
 pub struct InstructionAccount {
     pub index: usize,
     pub is_signer: bool,
@@ -531,32 +530,45 @@ impl<'a> InvokeContext<'a> {
         instruction: &Instruction,
         signers: &[Pubkey],
     ) -> Result<(Vec<InstructionAccount>, Vec<bool>, Vec<usize>), InstructionError> {
-        // Normalize / unify the privileges of duplicate accounts by
+        // Finds the index of each account in the instruction by its pubkey.
-        // constructing a message and compiling an instruction
+        // Then normalizes / unifies the privileges of duplicate accounts.
-        // only to throw them away immediately after.
+        // Note: This works like visit_each_account_once() and is an O(n^2) algorithm too.
-        let message = Message::new(&[instruction.clone()], None);
+        let mut deduplicated_instruction_accounts: Vec<InstructionAccount> = Vec::new();
-        let instruction_accounts = message.instructions[0]
+        let mut duplicate_indicies = Vec::with_capacity(instruction.accounts.len());
-            .accounts
+        for account_meta in instruction.accounts.iter() {
-            .iter()
+            let account_index = self
-            .map(|index_in_instruction| {
+                .accounts
-                let index_in_instruction = *index_in_instruction as usize;
+                .iter()
-                let account_index = self
+                .position(|(key, _account)| key == &account_meta.pubkey)
-                    .find_index_of_account(&message.account_keys[index_in_instruction])
+                .ok_or_else(|| {
-                    .ok_or_else(|| {
+                    ic_msg!(
-                        ic_msg!(
+                        self,
-                            self,
+                        "Instruction references an unknown account {}",
-                            "Instruction references an unknown account {}",
+                        account_meta.pubkey,
-                            message.account_keys[index_in_instruction],
+                    );
-                        );
+                    InstructionError::MissingAccount
-                        InstructionError::MissingAccount
+                })?;
-                    })?;
+            if let Some(duplicate_index) = deduplicated_instruction_accounts
-                Ok(InstructionAccount {
+                .iter()
+                .position(|instruction_account| instruction_account.index == account_index)
+            {
+                duplicate_indicies.push(duplicate_index);
+                let instruction_account = &mut deduplicated_instruction_accounts[duplicate_index];
+                instruction_account.is_signer |= account_meta.is_signer;
+                instruction_account.is_writable |= account_meta.is_writable;
+            } else {
+                duplicate_indicies.push(deduplicated_instruction_accounts.len());
+                deduplicated_instruction_accounts.push(InstructionAccount {
                     index: account_index,
-                    is_signer: message.is_signer(index_in_instruction),
+                    is_signer: account_meta.is_signer,
-                    is_writable: message.is_writable(index_in_instruction),
+                    is_writable: account_meta.is_writable,
-                })
+                });
-            })
+            }
-            .collect::<Result<Vec<_>, InstructionError>>()?;
+        }
+        let instruction_accounts = duplicate_indicies
+            .into_iter()
+            .map(|duplicate_index| deduplicated_instruction_accounts[duplicate_index].clone())
+            .collect();
 
         // Check for privilege escalation
         let caller_keyed_accounts = self.get_instruction_keyed_accounts()?;
@@ -605,7 +617,11 @@ impl<'a> InvokeContext<'a> {
         let program_account_index = caller_keyed_accounts
             .iter()
             .find(|keyed_account| &callee_program_id == keyed_account.unsigned_key())
-            .and_then(|_keyed_account| self.find_index_of_account(&callee_program_id))
+            .and_then(|_keyed_account| {
+                self.accounts
+                    .iter()
+                    .rposition(|(key, _account)| key == &callee_program_id)
+            })
             .ok_or_else(|| {
                 ic_msg!(self, "Unknown program {}", callee_program_id);
                 InstructionError::MissingAccount
@@ -621,8 +637,10 @@ impl<'a> InvokeContext<'a> {
                 programdata_address,
             } = program_account.state()?
             {
-                if let Some(programdata_account_index) =
+                if let Some(programdata_account_index) = self
-                    self.find_index_of_account(&programdata_address)
+                    .accounts
+                    .iter()
+                    .rposition(|(key, _account)| key == &programdata_address)
                 {
                     program_indices.push(programdata_account_index);
                 } else {
@@ -659,16 +677,17 @@ impl<'a> InvokeContext<'a> {
         caller_write_privileges: Option<&[bool]>,
         program_indices: &[usize],
     ) -> Result<u64, InstructionError> {
+        let program_id = program_indices
+            .last()
+            .map(|index| self.accounts[*index].0)
+            .unwrap_or_else(native_loader::id);
+
         let is_lowest_invocation_level = self.invoke_stack.is_empty();
         if !is_lowest_invocation_level {
             // Verify the calling program hasn't misbehaved
             self.verify_and_update(instruction_accounts, caller_write_privileges)?;
         }
 
-        let program_id = program_indices
-            .last()
-            .map(|index| self.accounts[*index].0)
-            .unwrap_or_else(native_loader::id);
         let result = self
             .push(instruction_accounts, program_indices)
             .and_then(|_| {
@@ -811,16 +830,6 @@ impl<'a> InvokeContext<'a> {
         self.executors.borrow().get(pubkey)
     }
 
-    /// Finds an account_index by its key
-    pub fn find_index_of_account(&self, pubkey: &Pubkey) -> Option<usize> {
-        for (index, (key, _account)) in self.accounts.iter().enumerate().rev() {
-            if key == pubkey {
-                return Some(index);
-            }
-        }
-        None
-    }
-
     /// Returns an account by its account_index
     pub fn get_account_key_at_index(&self, account_index: usize) -> &Pubkey {
         &self.accounts[account_index].0
@@ -995,11 +1004,7 @@ mod tests {
     use {
         super::*,
         serde::{Deserialize, Serialize},
-        solana_sdk::{
+        solana_sdk::account::{ReadableAccount, WritableAccount},
-            account::{ReadableAccount, WritableAccount},
-            instruction::AccountMeta,
-            native_loader,
-        },
     };
 
     #[derive(Debug, Serialize, Deserialize)]
@@ -1278,23 +1283,20 @@ mod tests {
             AccountMeta::new(accounts[1].0, false),
             AccountMeta::new_readonly(accounts[2].0, false),
         ];
-
+        let instruction_accounts = metas
+            .iter()
+            .enumerate()
+            .map(|(account_index, account_meta)| InstructionAccount {
+                index: account_index,
+                is_signer: account_meta.is_signer,
+                is_writable: account_meta.is_writable,
+            })
+            .collect::<Vec<_>>();
         let instruction = Instruction::new_with_bincode(
             callee_program_id,
             &MockInstruction::NoopSuccess,
             metas.clone(),
         );
-        let message = Message::new(&[instruction], None);
-        let instruction_accounts = message
-            .account_keys
-            .iter()
-            .enumerate()
-            .map(|(index, _)| InstructionAccount {
-                index,
-                is_signer: message.is_signer(index),
-                is_writable: message.is_writable(index),
-            })
-            .collect::<Vec<_>>();
         let builtin_programs = &[BuiltinProgram {
             program_id: callee_program_id,
             process_instruction: mock_process_instruction,
@@ -1308,7 +1310,7 @@ mod tests {
         accounts[0].1.borrow_mut().data_as_mut_slice()[0] = 1;
         assert_eq!(
             invoke_context.process_instruction(
-                &message.instructions[0].data,
+                &instruction.data,
                 &instruction_accounts,
                 None,
                 &program_indices[1..],
@@ -1321,7 +1323,7 @@ mod tests {
         accounts[2].1.borrow_mut().data_as_mut_slice()[0] = 1;
         assert_eq!(
             invoke_context.process_instruction(
-                &message.instructions[0].data,
+                &instruction.data,
                 &instruction_accounts,
                 None,
                 &program_indices[1..],
@@ -1396,24 +1398,20 @@ mod tests {
             AccountMeta::new_readonly(accounts[2].0, false),
             AccountMeta::new_readonly(accounts[3].0, false),
         ];
+        let instruction_accounts = metas
+            .iter()
+            .enumerate()
+            .map(|(account_index, account_meta)| InstructionAccount {
+                index: account_index,
+                is_signer: account_meta.is_signer,
+                is_writable: account_meta.is_writable,
+            })
+            .collect::<Vec<_>>();
         let callee_instruction = Instruction::new_with_bincode(
             callee_program_id,
             &MockInstruction::NoopSuccess,
             metas.clone(),
         );
-        let message = Message::new(&[callee_instruction.clone()], None);
-        let instruction_accounts = message.instructions[0]
-            .accounts
-            .iter()
-            .map(|account_index| {
-                let account_index = *account_index as usize;
-                InstructionAccount {
-                    index: account_index,
-                    is_signer: message.is_signer(account_index),
-                    is_writable: message.is_writable(account_index),
-                }
-            })
-            .collect::<Vec<_>>();
 
         let mut invoke_context = InvokeContext::new_mock(&accounts, builtin_programs);
         invoke_context

program-test/src/lib.rs

@@ -30,7 +30,6 @@ use {
         genesis_config::{ClusterType, GenesisConfig},
         hash::Hash,
         instruction::{Instruction, InstructionError},
-        message::Message,
         native_token::sol_to_lamports,
         poh_config::PohConfig,
         program_error::{ProgramError, ACCOUNT_BORROW_FAILED, UNSUPPORTED_SYSVAR},
@@ -235,7 +234,6 @@ impl solana_sdk::program_stubs::SyscallStubs for SyscallStubs {
         let log_collector = invoke_context.get_log_collector();
 
         let caller = *invoke_context.get_caller().expect("get_caller");
-        let message = Message::new(&[instruction.clone()], None);
 
         stable_log::program_invoke(
             &log_collector,
@@ -243,36 +241,6 @@ impl solana_sdk::program_stubs::SyscallStubs for SyscallStubs {
             invoke_context.invoke_depth(),
         );
 
-        // Convert AccountInfos into Accounts
-        let mut accounts = Vec::with_capacity(message.account_keys.len());
-        for (i, account_key) in message.account_keys.iter().enumerate() {
-            let (account_index, account_info) = invoke_context
-                .find_index_of_account(account_key)
-                .zip(
-                    account_infos
-                        .iter()
-                        .find(|account_info| account_info.unsigned_key() == account_key),
-                )
-                .ok_or(InstructionError::MissingAccount)
-                .unwrap();
-            {
-                let mut account = invoke_context
-                    .get_account_at_index(account_index)
-                    .borrow_mut();
-                account.copy_into_owner_from_slice(account_info.owner.as_ref());
-                account.set_data_from_slice(&account_info.try_borrow_data().unwrap());
-                account.set_lamports(account_info.lamports());
-                account.set_executable(account_info.executable);
-                account.set_rent_epoch(account_info.rent_epoch);
-            }
-            let account_info = if message.is_writable(i) {
-                Some(account_info)
-            } else {
-                None
-            };
-            accounts.push((account_index, account_info));
-        }
-
         let signers = signers_seeds
             .iter()
             .map(|seeds| Pubkey::create_program_address(seeds, &caller).unwrap())
@@ -281,6 +249,30 @@ impl solana_sdk::program_stubs::SyscallStubs for SyscallStubs {
             .prepare_instruction(instruction, &signers)
             .unwrap();
 
+        // Convert AccountInfos into Accounts
+        let mut accounts = Vec::with_capacity(instruction_accounts.len());
+        for instruction_account in instruction_accounts.iter() {
+            let account_key = invoke_context.get_account_key_at_index(instruction_account.index);
+            let account_info = account_infos
+                .iter()
+                .find(|account_info| account_info.unsigned_key() == account_key)
+                .ok_or(InstructionError::MissingAccount)
+                .unwrap();
+            {
+                let mut account = invoke_context
+                    .get_account_at_index(instruction_account.index)
+                    .borrow_mut();
+                account.copy_into_owner_from_slice(account_info.owner.as_ref());
+                account.set_data_from_slice(&account_info.try_borrow_data().unwrap());
+                account.set_lamports(account_info.lamports());
+                account.set_executable(account_info.executable);
+                account.set_rent_epoch(account_info.rent_epoch);
+            }
+            if instruction_account.is_writable {
+                accounts.push((instruction_account.index, account_info));
+            }
+        }
+
         if let Some(instruction_recorder) = &invoke_context.instruction_recorder {
             instruction_recorder.record_instruction(instruction.clone());
         }
@@ -295,30 +287,28 @@ impl solana_sdk::program_stubs::SyscallStubs for SyscallStubs {
 
         // Copy writeable account modifications back into the caller's AccountInfos
         for (account_index, account_info) in accounts.into_iter() {
-            if let Some(account_info) = account_info {
+            let account = invoke_context.get_account_at_index(account_index);
-                let account = invoke_context.get_account_at_index(account_index);
+            let account_borrow = account.borrow();
-                let account_borrow = account.borrow();
+            **account_info.try_borrow_mut_lamports().unwrap() = account_borrow.lamports();
-                **account_info.try_borrow_mut_lamports().unwrap() = account_borrow.lamports();
+            let mut data = account_info.try_borrow_mut_data()?;
-                let mut data = account_info.try_borrow_mut_data()?;
+            let new_data = account_borrow.data();
-                let new_data = account_borrow.data();
+            if account_info.owner != account_borrow.owner() {
-                if account_info.owner != account_borrow.owner() {
+                // TODO Figure out a better way to allow the System Program to set the account owner
-                    // TODO Figure out a better way to allow the System Program to set the account owner
+                #[allow(clippy::transmute_ptr_to_ptr)]
-                    #[allow(clippy::transmute_ptr_to_ptr)]
+                #[allow(mutable_transmutes)]
-                    #[allow(mutable_transmutes)]
+                let account_info_mut =
-                    let account_info_mut =
+                    unsafe { transmute::<&Pubkey, &mut Pubkey>(account_info.owner) };
-                        unsafe { transmute::<&Pubkey, &mut Pubkey>(account_info.owner) };
+                *account_info_mut = *account_borrow.owner();
-                    *account_info_mut = *account_borrow.owner();
-                }
-                // TODO: Figure out how to allow the System Program to resize the account data
-                assert!(
-                    data.len() == new_data.len(),
-                    "Account data resizing not supported yet: {} -> {}. \
-                        Consider making this test conditional on `#[cfg(feature = \"test-bpf\")]`",
-                    data.len(),
-                    new_data.len()
-                );
-                data.clone_from_slice(new_data);
             }
+            // TODO: Figure out how to allow the System Program to resize the account data
+            assert!(
+                data.len() == new_data.len(),
+                "Account data resizing not supported yet: {} -> {}. \
+                    Consider making this test conditional on `#[cfg(feature = \"test-bpf\")]`",
+                data.len(),
+                new_data.len()
+            );
+            data.clone_from_slice(new_data);
         }
 
         stable_log::program_success(&log_collector, &instruction.program_id);