Feature: Early verification of account modifications in BorrowedAccount

cli/src/program.rs

@@ -46,6 +46,7 @@ use {
         signature::{keypair_from_seed, read_keypair_file, Keypair, Signature, Signer},
         system_instruction::{self, SystemError},
         system_program,
+        sysvar::rent::Rent,
         transaction::{Transaction, TransactionError},
         transaction_context::TransactionContext,
     },
@@ -2060,7 +2061,7 @@ fn read_and_verify_elf(program_location: &str) -> Result<Vec<u8>, Box<dyn std::e
     let mut program_data = Vec::new();
     file.read_to_end(&mut program_data)
         .map_err(|err| format!("Unable to read program file: {}", err))?;
-    let mut transaction_context = TransactionContext::new(Vec::new(), 1, 1);
+    let mut transaction_context = TransactionContext::new(Vec::new(), Some(Rent::default()), 1, 1);
     let mut invoke_context = InvokeContext::new_mock(&mut transaction_context, &[]);
 
     // Verify the program

ledger/src/blockstore_processor.rs

@@ -4532,7 +4532,10 @@ pub mod tests {
             mint_keypair,
             ..
         } = create_genesis_config((1_000_000 + NUM_ACCOUNTS + 1) * LAMPORTS_PER_SOL);
-        let bank = Bank::new_for_tests(&genesis_config);
+        let mut bank = Bank::new_for_tests(&genesis_config);
+        bank.deactivate_feature(
+            &feature_set::enable_early_verification_of_account_modifications::id(),
+        );
         let bank = Arc::new(bank);
         assert!(bank
             .feature_set
@@ -4595,6 +4598,9 @@ pub mod tests {
             mock_realloc::process_instruction,
         );
         bank.set_accounts_data_size_initial_for_tests(INITIAL_ACCOUNTS_DATA_SIZE);
+        bank.deactivate_feature(
+            &feature_set::enable_early_verification_of_account_modifications::id(),
+        );
         let bank = Arc::new(bank);
         let bank = Arc::new(Bank::new_from_parent(&bank, &Pubkey::new_unique(), 1));
         assert!(bank

program-test/src/lib.rs

@@ -183,8 +183,14 @@ pub fn builtin_process_instruction(
         if borrowed_account.get_lamports() != lamports {
             borrowed_account.set_lamports(lamports)?;
         }
-        if borrowed_account.get_data() != data {
-            borrowed_account.set_data(&data)?;
+        // The redundant check helps to avoid the expensive data comparison if we can
+        match borrowed_account
+            .can_data_be_resized(data.len())
+            .and_then(|_| borrowed_account.can_data_be_changed())
+        {
+            Ok(()) => borrowed_account.set_data(&data)?,
+            Err(err) if borrowed_account.get_data() != data => return Err(err),
+            _ => {}
         }
         if borrowed_account.get_owner() != &owner {
             borrowed_account.set_owner(owner.as_ref())?;
@@ -302,14 +308,23 @@ impl solana_sdk::program_stubs::SyscallStubs for SyscallStubs {
                     .unwrap();
             }
             let account_info_data = account_info.try_borrow_data().unwrap();
-            if borrowed_account.get_data() != *account_info_data {
-                borrowed_account.set_data(&account_info_data).unwrap();
+            // The redundant check helps to avoid the expensive data comparison if we can
+            match borrowed_account
+                .can_data_be_resized(account_info_data.len())
+                .and_then(|_| borrowed_account.can_data_be_changed())
+            {
+                Ok(()) => borrowed_account.set_data(&account_info_data).unwrap(),
+                Err(err) if borrowed_account.get_data() != *account_info_data => {
+                    panic!("{:?}", err);
+                }
+                _ => {}
             }
             if borrowed_account.is_executable() != account_info.executable {
                 borrowed_account
                     .set_executable(account_info.executable)
                     .unwrap();
             }
+            // Change the owner at the end so that we are allowed to change the lamports and data before
             if borrowed_account.get_owner() != account_info.owner {
                 borrowed_account
                     .set_owner(account_info.owner.as_ref())

programs/bpf/tests/programs.rs

@@ -305,16 +305,29 @@ fn run_program(name: &str) -> u64 {
                     tracer = Some(vm.get_tracer().clone());
                 }
             }
-            deserialize_parameters(
+            assert!(match deserialize_parameters(
                 invoke_context.transaction_context,
                 invoke_context
                     .transaction_context
                     .get_current_instruction_context()
                     .unwrap(),
                 parameter_bytes.as_slice(),
                 &account_lengths,
-            )
-            .unwrap();
+            ) {
+                Ok(()) => true,
+                Err(InstructionError::ModifiedProgramId) => true,
+                Err(InstructionError::ExternalAccountLamportSpend) => true,
+                Err(InstructionError::ReadonlyLamportChange) => true,
+                Err(InstructionError::ExecutableLamportChange) => true,
+                Err(InstructionError::ExecutableAccountNotRentExempt) => true,
+                Err(InstructionError::ExecutableModified) => true,
+                Err(InstructionError::AccountDataSizeChanged) => true,
+                Err(InstructionError::InvalidRealloc) => true,
+                Err(InstructionError::ExecutableDataModified) => true,
+                Err(InstructionError::ReadonlyDataModified) => true,
+                Err(InstructionError::ExternalAccountDataModified) => true,
+                _ => false,
+            });
         }
         instruction_count
     })

programs/bpf_loader/benches/serialization.rs

@@ -9,6 +9,7 @@ use {
     solana_sdk::{
         account::{Account, AccountSharedData},
         bpf_loader,
+        sysvar::rent::Rent,
         transaction_context::{InstructionAccount, TransactionContext},
     },
     test::Bencher,
@@ -101,7 +102,8 @@ fn create_inputs() -> TransactionContext {
             },
         )
         .collect::<Vec<_>>();
-    let mut transaction_context = TransactionContext::new(transaction_accounts, 1, 1);
+    let mut transaction_context =
+        TransactionContext::new(transaction_accounts, Some(Rent::default()), 1, 1);
     let instruction_data = vec![1u8, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11];
     transaction_context
         .push(&[0], &instruction_accounts, &instruction_data, true)

programs/bpf_loader/src/serialization.rs

@@ -164,18 +164,28 @@ pub fn deserialize_parameters_unaligned(
             start += size_of::<u8>(); // is_signer
             start += size_of::<u8>(); // is_writable
             start += size_of::<Pubkey>(); // key
-            let _ = borrowed_account.set_lamports(LittleEndian::read_u64(
+            let lamports = LittleEndian::read_u64(
                 buffer
                     .get(start..)
                     .ok_or(InstructionError::InvalidArgument)?,
-            ));
+            );
+            if borrowed_account.get_lamports() != lamports {
+                borrowed_account.set_lamports(lamports)?;
+            }
             start += size_of::<u64>() // lamports
                 + size_of::<u64>(); // data length
-            let _ = borrowed_account.set_data(
-                buffer
-                    .get(start..start + pre_len)
-                    .ok_or(InstructionError::InvalidArgument)?,
-            );
+            let data = buffer
+                .get(start..start + pre_len)
+                .ok_or(InstructionError::InvalidArgument)?;
+            // The redundant check helps to avoid the expensive data comparison if we can
+            match borrowed_account
+                .can_data_be_resized(data.len())
+                .and_then(|_| borrowed_account.can_data_be_changed())
+            {
+                Ok(()) => borrowed_account.set_data(data)?,
+                Err(err) if borrowed_account.get_data() != data => return Err(err),
+                _ => {}
+            }
             start += pre_len // data
                 + size_of::<Pubkey>() // owner
                 + size_of::<u8>() // executable
@@ -302,39 +312,50 @@ pub fn deserialize_parameters_aligned(
                 + size_of::<u8>() // executable
                 + size_of::<u32>() // original_data_len
                 + size_of::<Pubkey>(); // key
-            let _ = borrowed_account.set_owner(
-                buffer
-                    .get(start..start + size_of::<Pubkey>())
-                    .ok_or(InstructionError::InvalidArgument)?,
-            );
+            let owner = buffer
+                .get(start..start + size_of::<Pubkey>())
+                .ok_or(InstructionError::InvalidArgument)?;
             start += size_of::<Pubkey>(); // owner
-            let _ = borrowed_account.set_lamports(LittleEndian::read_u64(
+            let lamports = LittleEndian::read_u64(
                 buffer
                     .get(start..)
                     .ok_or(InstructionError::InvalidArgument)?,
-            ));
+            );
+            if borrowed_account.get_lamports() != lamports {
+                borrowed_account.set_lamports(lamports)?;
+            }
             start += size_of::<u64>(); // lamports
             let post_len = LittleEndian::read_u64(
                 buffer
                     .get(start..)
                     .ok_or(InstructionError::InvalidArgument)?,
             ) as usize;
             start += size_of::<u64>(); // data length
-
             if post_len.saturating_sub(*pre_len) > MAX_PERMITTED_DATA_INCREASE
                 || post_len > MAX_PERMITTED_DATA_LENGTH as usize
             {
                 return Err(InstructionError::InvalidRealloc);
             }
             let data_end = start + post_len;
-            let _ = borrowed_account.set_data(
-                buffer
-                    .get(start..data_end)
-                    .ok_or(InstructionError::InvalidArgument)?,
-            );
+            let data = buffer
+                .get(start..data_end)
+                .ok_or(InstructionError::InvalidArgument)?;
+            // The redundant check helps to avoid the expensive data comparison if we can
+            match borrowed_account
+                .can_data_be_resized(data.len())
+                .and_then(|_| borrowed_account.can_data_be_changed())
+            {
+                Ok(()) => borrowed_account.set_data(data)?,
+                Err(err) if borrowed_account.get_data() != data => return Err(err),
+                _ => {}
+            }
             start += *pre_len + MAX_PERMITTED_DATA_INCREASE; // data
             start += (start as *const u8).align_offset(BPF_ALIGN_OF_U128);
             start += size_of::<u64>(); // rent_epoch
+            if borrowed_account.get_owner().to_bytes() != owner {
+                // Change the owner at the end so that we are allowed to change the lamports and data before
+                borrowed_account.set_owner(owner)?;
+            }
         }
     }
     Ok(())
@@ -351,6 +372,7 @@ mod tests {
             bpf_loader,
             entrypoint::deserialize,
             instruction::AccountMeta,
+            sysvar::rent::Rent,
         },
         std::{
             cell::RefCell,
@@ -453,8 +475,12 @@ mod tests {
             instruction_accounts,
             &program_indices,
         );
-        let mut transaction_context =
-            TransactionContext::new(preparation.transaction_accounts, 1, 1);
+        let mut transaction_context = TransactionContext::new(
+            preparation.transaction_accounts,
+            Some(Rent::default()),
+            1,
+            1,
+        );
         let mut invoke_context = InvokeContext::new_mock(&mut transaction_context, &[]);
         invoke_context
             .push(
@@ -512,16 +538,6 @@ mod tests {
             );
         }
 
-        for index_in_transaction in 1..original_accounts.len() {
-            let mut account = invoke_context
-                .transaction_context
-                .get_account_at_index(index_in_transaction)
-                .unwrap()
-                .borrow_mut();
-            account.set_lamports(0);
-            account.set_data(vec![0; 0]);
-            account.set_owner(Pubkey::default());
-        }
         deserialize_parameters(
             invoke_context.transaction_context,
             instruction_context,
@@ -545,13 +561,12 @@ mod tests {
             .unwrap()
             .1
             .set_owner(bpf_loader_deprecated::id());
-        let _ = invoke_context
+        invoke_context
             .transaction_context
-            .get_current_instruction_context()
+            .get_account_at_index(0)
             .unwrap()
-            .try_borrow_program_account(invoke_context.transaction_context, 0)
-            .unwrap()
-            .set_owner(bpf_loader_deprecated::id().as_ref());
+            .borrow_mut()
+            .set_owner(bpf_loader_deprecated::id());
 
         let (mut serialized, account_lengths) =
             serialize_parameters(invoke_context.transaction_context, instruction_context).unwrap();
@@ -578,15 +593,6 @@ mod tests {
             assert_eq!(account.rent_epoch(), account_info.rent_epoch);
         }
 
-        for index_in_transaction in 1..original_accounts.len() {
-            let mut account = invoke_context
-                .transaction_context
-                .get_account_at_index(index_in_transaction)
-                .unwrap()
-                .borrow_mut();
-            account.set_lamports(0);
-            account.set_data(vec![0; 0]);
-        }
         deserialize_parameters(
             invoke_context.transaction_context,
             instruction_context,