From 660f6981c6f19b461a0bb3e7797592ccbd03196f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Mei=C3=9Fner?= Date: Thu, 3 Feb 2022 17:19:42 +0100 Subject: [PATCH] Cleanup: TransactionContext (#22910) * Adds BorrowedAccount::check_sysvar(). * Adds BorrowedAccount::get_data_mut(). * Implements account resizing in BorrowedAccount. * Exposes is_signer() and is_writable() in InstructionContext. * Removes AccountMeta and get_instruction_accounts_metas(). * Makes throwing errors in BorrowedAccount optional. * Removes result return values from BorrowedAccount. --- program-runtime/src/invoke_context.rs | 8 +- program-test/src/lib.rs | 4 +- programs/bpf_loader/src/syscalls.rs | 21 ++-- runtime/src/bank.rs | 14 ++- runtime/src/message_processor.rs | 4 +- sdk/src/transaction_context.rs | 154 ++++++++++---------------- 6 files changed, 89 insertions(+), 116 deletions(-) diff --git a/program-runtime/src/invoke_context.rs b/program-runtime/src/invoke_context.rs index c3638feb2..2da0ad49b 100644 --- a/program-runtime/src/invoke_context.rs +++ b/program-runtime/src/invoke_context.rs @@ -1283,13 +1283,13 @@ mod tests { MockInstruction::NoopFail => return Err(InstructionError::GenericError), MockInstruction::ModifyOwned => instruction_context .try_borrow_instruction_account(transaction_context, 0)? - .set_data(&[1])?, + .set_data(&[1]), MockInstruction::ModifyNotOwned => instruction_context .try_borrow_instruction_account(transaction_context, 1)? - .set_data(&[1])?, + .set_data(&[1]), MockInstruction::ModifyReadonly => instruction_context .try_borrow_instruction_account(transaction_context, 2)? - .set_data(&[1])?, + .set_data(&[1]), MockInstruction::ConsumeComputeUnits { compute_units_to_consume, desired_result, @@ -1303,7 +1303,7 @@ mod tests { } MockInstruction::Resize { new_len } => instruction_context .try_borrow_instruction_account(transaction_context, 0)? - .set_data(&vec![0; new_len])?, + .set_data(&vec![0; new_len]), } } else { return Err(InstructionError::InvalidInstructionData); diff --git a/program-test/src/lib.rs b/program-test/src/lib.rs index 70fb99c28..b497f2bf8 100644 --- a/program-test/src/lib.rs +++ b/program-test/src/lib.rs @@ -178,8 +178,8 @@ pub fn builtin_process_instruction( let mut borrowed_account = instruction_context.try_borrow_account(transaction_context, index_in_instruction)?; if borrowed_account.is_writable() { - borrowed_account.set_lamports(lamports)?; - borrowed_account.set_data(&data)?; + borrowed_account.set_lamports(lamports); + borrowed_account.set_data(&data); } } diff --git a/programs/bpf_loader/src/syscalls.rs b/programs/bpf_loader/src/syscalls.rs index d99667082..b67c35a96 100644 --- a/programs/bpf_loader/src/syscalls.rs +++ b/programs/bpf_loader/src/syscalls.rs @@ -3107,17 +3107,18 @@ impl<'a, 'b> SyscallObject for SyscallGetProcessedSiblingInstruction<' instruction_context.get_program_id(invoke_context.transaction_context); data.clone_from_slice(instruction_context.get_instruction_data()); let account_metas = question_mark!( - instruction_context - .get_instruction_accounts_metas() - .iter() - .map(|meta| Ok(AccountMeta { - pubkey: *invoke_context - .get_key_of_account_at_index(meta.index_in_transaction) - .map_err(SyscallError::InstructionError)?, - is_signer: meta.is_signer, - is_writable: meta.is_writable, + (instruction_context.get_number_of_program_accounts() + ..instruction_context.get_number_of_accounts()) + .map(|index_in_instruction| Ok(AccountMeta { + pubkey: *invoke_context.get_key_of_account_at_index( + instruction_context + .get_index_in_transaction(index_in_instruction)? + )?, + is_signer: instruction_context.is_signer(index_in_instruction)?, + is_writable: instruction_context.is_writable(index_in_instruction)?, })) - .collect::, EbpfError>>(), + .collect::, InstructionError>>() + .map_err(SyscallError::InstructionError), result ); accounts.clone_from_slice(account_metas.as_slice()); diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 2e2c6ec0e..07d1892b7 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -706,10 +706,13 @@ pub fn inner_instructions_list_from_instruction_trace( CompiledInstruction::new_from_raw_parts( instruction_context.get_program_id_index() as u8, instruction_context.get_instruction_data().to_vec(), - instruction_context - .get_instruction_accounts_metas() - .iter() - .map(|meta| meta.index_in_transaction as u8) + (instruction_context.get_number_of_program_accounts() + ..instruction_context.get_number_of_accounts()) + .map(|index_in_instruction| { + instruction_context + .get_index_in_transaction(index_in_instruction) + .unwrap_or_default() as u8 + }) .collect(), ) }) @@ -14918,7 +14921,8 @@ pub(crate) mod tests { let instruction_context = transaction_context.get_current_instruction_context()?; instruction_context .try_borrow_instruction_account(transaction_context, 1)? - .set_data(&[0; 40]) + .set_data(&[0; 40]); + Ok(()) } let program_id = solana_sdk::pubkey::new_rand(); diff --git a/runtime/src/message_processor.rs b/runtime/src/message_processor.rs index f99a63988..3aa7b8164 100644 --- a/runtime/src/message_processor.rs +++ b/runtime/src/message_processor.rs @@ -210,7 +210,7 @@ mod tests { MockSystemInstruction::ChangeData { data } => { instruction_context .try_borrow_instruction_account(transaction_context, 1)? - .set_data(&[data])?; + .set_data(&[data]); Ok(()) } } @@ -409,7 +409,7 @@ mod tests { .try_borrow_instruction_account(transaction_context, 2)?; dup_account.checked_sub_lamports(lamports)?; to_account.checked_add_lamports(lamports)?; - dup_account.set_data(&[data])?; + dup_account.set_data(&[data]); drop(dup_account); let mut from_account = instruction_context .try_borrow_instruction_account(transaction_context, 0)?; diff --git a/sdk/src/transaction_context.rs b/sdk/src/transaction_context.rs index 3eb4bbd56..3afe80646 100644 --- a/sdk/src/transaction_context.rs +++ b/sdk/src/transaction_context.rs @@ -5,6 +5,7 @@ use crate::{ instruction::{InstructionError, TRANSACTION_LEVEL_STACK_HEIGHT}, lamports::LamportsError, pubkey::Pubkey, + sysvar::Sysvar, }; use std::{ cell::{RefCell, RefMut}, @@ -115,6 +116,17 @@ impl TransactionContext { .ok_or(InstructionError::NotEnoughAccountKeys) } + /// Checks if the account key at the given index is the belongs to the given sysvar + pub fn check_sysvar( + &self, + index_in_transaction: usize, + ) -> Result<(), InstructionError> { + if !S::check_id(&self.account_keys[index_in_transaction]) { + return Err(InstructionError::InvalidArgument); + } + Ok(()) + } + /// Searches for an account by its key pub fn find_index_of_account(&self, pubkey: &Pubkey) -> Option { self.account_keys.iter().position(|key| key == pubkey) @@ -241,13 +253,6 @@ impl TransactionContext { /// List of (stack height, instruction) for each top-level instruction pub type InstructionTrace = Vec>; -#[derive(Clone, Debug)] -pub struct AccountMeta { - pub index_in_transaction: usize, - pub is_signer: bool, - pub is_writable: bool, -} - /// Loaded instruction shared between runtime and programs. /// /// This context is valid for the entire duration of a (possibly cross program) instruction being processed. @@ -292,17 +297,6 @@ impl InstructionContext { self.instruction_accounts.len() } - pub fn get_instruction_accounts_metas(&self) -> Vec { - self.instruction_accounts - .iter() - .map(|instruction_account| AccountMeta { - index_in_transaction: instruction_account.index_in_transaction, - is_signer: instruction_account.is_signer, - is_writable: instruction_account.is_writable, - }) - .collect() - } - /// Number of accounts in this Instruction pub fn get_number_of_accounts(&self) -> usize { self.program_accounts @@ -406,6 +400,30 @@ impl InstructionContext { ) } + /// Returns whether an account is a signer + pub fn is_signer(&self, index_in_instruction: usize) -> Result { + Ok(if index_in_instruction < self.program_accounts.len() { + false + } else { + self.instruction_accounts + .get(index_in_instruction.saturating_sub(self.program_accounts.len())) + .ok_or(InstructionError::MissingAccount)? + .is_signer + }) + } + + /// Returns whether an account is writable + pub fn is_writable(&self, index_in_instruction: usize) -> Result { + Ok(if index_in_instruction < self.program_accounts.len() { + false + } else { + self.instruction_accounts + .get(index_in_instruction.saturating_sub(self.program_accounts.len())) + .ok_or(InstructionError::MissingAccount)? + .is_writable + }) + } + /// Calculates the set of all keys of signer accounts in this Instruction pub fn get_signers(&self, transaction_context: &TransactionContext) -> HashSet { let mut result = HashSet::new(); @@ -418,28 +436,6 @@ impl InstructionContext { } result } - - /// Returns whether an account is a signer - pub fn is_signer(&self, index_in_instruction: usize) -> bool { - if index_in_instruction < self.program_accounts.len() { - false - } else { - self.instruction_accounts - [index_in_instruction.saturating_sub(self.program_accounts.len())] - .is_signer - } - } - - /// Returns whether an account is writable - pub fn is_writable(&self, index_in_instruction: usize) -> bool { - if index_in_instruction < self.program_accounts.len() { - false - } else { - self.instruction_accounts - [index_in_instruction.saturating_sub(self.program_accounts.len())] - .is_writable - } - } } /// Shared account borrowed from the TransactionContext and an InstructionContext. @@ -474,10 +470,8 @@ impl<'a> BorrowedAccount<'a> { } /// Assignes the owner of this account (transaction wide) - pub fn set_owner(&mut self, pubkey: &[u8]) -> Result<(), InstructionError> { + pub fn set_owner(&mut self, pubkey: &[u8]) { self.account.copy_into_owner_from_slice(pubkey); - self.verify_writability() - // TODO: return Err(InstructionError::ModifiedProgramId); } /// Returns the number of lamports of this account (transaction wide) @@ -486,16 +480,8 @@ impl<'a> BorrowedAccount<'a> { } /// Overwrites the number of lamports of this account (transaction wide) - pub fn set_lamports(&mut self, lamports: u64) -> Result<(), InstructionError> { + pub fn set_lamports(&mut self, lamports: u64) { self.account.set_lamports(lamports); - if self.index_in_instruction < self.instruction_context.program_accounts.len() { - return Err(InstructionError::ExecutableLamportChange); - } - if !self.is_writable() { - return Err(InstructionError::ReadonlyLamportChange); - } - // TODO: return Err(InstructionError::ExternalAccountLamportSpend); - Ok(()) } /// Adds lamports to this account (transaction wide) @@ -504,7 +490,8 @@ impl<'a> BorrowedAccount<'a> { self.get_lamports() .checked_add(lamports) .ok_or(LamportsError::ArithmeticOverflow)?, - ) + ); + Ok(()) } /// Subtracts lamports from this account (transaction wide) @@ -513,18 +500,7 @@ impl<'a> BorrowedAccount<'a> { self.get_lamports() .checked_sub(lamports) .ok_or(LamportsError::ArithmeticUnderflow)?, - ) - } - - /// Verifies that this account is writable (instruction wide) - fn verify_writability(&self) -> Result<(), InstructionError> { - if self.index_in_instruction < self.instruction_context.program_accounts.len() { - return Err(InstructionError::ExecutableDataModified); - } - if !self.is_writable() { - return Err(InstructionError::ReadonlyDataModified); - } - // TODO: return Err(InstructionError::ExternalAccountDataModified); + ); Ok(()) } @@ -533,21 +509,26 @@ impl<'a> BorrowedAccount<'a> { self.account.data() } + /// Returns a writable slice of the account data (transaction wide) + pub fn get_data_mut(&mut self) -> &mut [u8] { + self.account.data_as_mut_slice() + } + /// Overwrites the account data and size (transaction wide) - pub fn set_data(&mut self, data: &[u8]) -> Result<(), InstructionError> { + pub fn set_data(&mut self, data: &[u8]) { if data.len() == self.account.data().len() { self.account.data_as_mut_slice().copy_from_slice(data); } else { self.account.set_data_from_slice(data); - // TODO: return Err(InstructionError::AccountDataSizeChanged); } - self.verify_writability() } - /*pub fn realloc(&self, new_len: usize, zero_init: bool) { - // TODO: return Err(InstructionError::InvalidRealloc); - // TODO: return Err(InstructionError::AccountDataSizeChanged); - }*/ + /// Resizes the account data (transaction wide) + /// + /// Fills it with zeros at the end if is extended or truncates at the end otherwise. + pub fn set_data_length(&mut self, new_len: usize) { + self.account.data_mut().resize(new_len, 0); + } /// Deserializes the account data into a state pub fn get_state(&self) -> Result { @@ -565,7 +546,7 @@ impl<'a> BorrowedAccount<'a> { return Err(InstructionError::AccountDataTooSmall); } bincode::serialize_into(&mut *data, state).map_err(|_| InstructionError::GenericError)?; - self.verify_writability() + Ok(()) } /// Returns whether this account is executable (transaction wide) @@ -574,11 +555,8 @@ impl<'a> BorrowedAccount<'a> { } /// Configures whether this account is executable (transaction wide) - pub fn set_executable(&mut self, is_executable: bool) -> Result<(), InstructionError> { + pub fn set_executable(&mut self, is_executable: bool) { self.account.set_executable(is_executable); - self.verify_writability() - // TODO: return Err(InstructionError::ExecutableAccountNotRentExempt); - // TODO: return Err(InstructionError::ExecutableModified); } /// Returns the rent epoch of this account (transaction wide) @@ -588,25 +566,15 @@ 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() { - false - } else { - self.instruction_context.instruction_accounts[self - .index_in_instruction - .saturating_sub(self.instruction_context.program_accounts.len())] - .is_signer - } + self.instruction_context + .is_signer(self.index_in_instruction) + .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() { - false - } else { - self.instruction_context.instruction_accounts[self - .index_in_instruction - .saturating_sub(self.instruction_context.program_accounts.len())] - .is_writable - } + self.instruction_context + .is_writable(self.index_in_instruction) + .unwrap_or_default() } }