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

@@ -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);

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_lamports(lamports);
-            borrowed_account.set_data(&data)?;
+            borrowed_account.set_data(&data);
         }
     }
 

programs/bpf_loader/src/syscalls.rs

@@ -3107,17 +3107,18 @@ impl<'a, 'b> SyscallObject<BpfError> 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
+                    (instruction_context.get_number_of_program_accounts()
-                        .get_instruction_accounts_metas()
+                        ..instruction_context.get_number_of_accounts())
-                        .iter()
+                        .map(|index_in_instruction| Ok(AccountMeta {
-                        .map(|meta| Ok(AccountMeta {
+                            pubkey: *invoke_context.get_key_of_account_at_index(
-                            pubkey: *invoke_context
+                                instruction_context
-                                .get_key_of_account_at_index(meta.index_in_transaction)
+                                    .get_index_in_transaction(index_in_instruction)?
-                                .map_err(SyscallError::InstructionError)?,
+                            )?,
-                            is_signer: meta.is_signer,
+                            is_signer: instruction_context.is_signer(index_in_instruction)?,
-                            is_writable: meta.is_writable,
+                            is_writable: instruction_context.is_writable(index_in_instruction)?,
                         }))
-                        .collect::<Result<Vec<_>, EbpfError<BpfError>>>(),
+                        .collect::<Result<Vec<_>, InstructionError>>()
+                        .map_err(SyscallError::InstructionError),
                     result
                 );
                 accounts.clone_from_slice(account_metas.as_slice());

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
+                        (instruction_context.get_number_of_program_accounts()
-                            .get_instruction_accounts_metas()
+                            ..instruction_context.get_number_of_accounts())
-                            .iter()
+                            .map(|index_in_instruction| {
-                            .map(|meta| meta.index_in_transaction as u8)
+                                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();

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)?;

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<S: 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<usize> {
         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<Vec<(usize, InstructionContext)>>;
 
-#[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<AccountMeta> {
-        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<bool, InstructionError> {
+        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<bool, InstructionError> {
+        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<Pubkey> {
         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) {
+    /// Resizes the account data (transaction wide)
-        // TODO: return Err(InstructionError::InvalidRealloc);
+    ///
-        // TODO: return Err(InstructionError::AccountDataSizeChanged);
+    /// 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<T: serde::de::DeserializeOwned>(&self) -> Result<T, InstructionError> {
@@ -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() {
+        self.instruction_context
-            false
+            .is_signer(self.index_in_instruction)
-        } else {
+            .unwrap_or_default()
-            self.instruction_context.instruction_accounts[self
-                .index_in_instruction
-                .saturating_sub(self.instruction_context.program_accounts.len())]
-            .is_signer
-        }
     }
 
     /// 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() {
+        self.instruction_context
-            false
+            .is_writable(self.index_in_instruction)
-        } else {
+            .unwrap_or_default()
-            self.instruction_context.instruction_accounts[self
-                .index_in_instruction
-                .saturating_sub(self.instruction_context.program_accounts.len())]
-            .is_writable
-        }
     }
 }