diff --git a/svm/src/account_loader.rs b/svm/src/account_loader.rs index f743e6bcba5860..0799ac8ba3cec7 100644 --- a/svm/src/account_loader.rs +++ b/svm/src/account_loader.rs @@ -608,12 +608,15 @@ mod tests { transaction::{Result, SanitizedTransaction, Transaction, TransactionError}, transaction_context::{TransactionAccount, TransactionContext}, }, - std::{borrow::Cow, collections::HashMap, sync::Arc}, + std::{borrow::Cow, cell::RefCell, collections::HashMap, sync::Arc}, }; #[derive(Default)] struct TestCallbacks { accounts_map: HashMap, + #[allow(clippy::type_complexity)] + inspected_accounts: + RefCell, /* is_writable */ bool)>>>, } impl TransactionProcessingCallback for TestCallbacks { @@ -624,6 +627,23 @@ mod tests { fn get_account_shared_data(&self, pubkey: &Pubkey) -> Option { self.accounts_map.get(pubkey).cloned() } + + fn inspect_account( + &self, + address: &Pubkey, + account_state: AccountState, + is_writable: bool, + ) { + let account = match account_state { + AccountState::Dead => None, + AccountState::Alive(account) => Some(account.clone()), + }; + self.inspected_accounts + .borrow_mut() + .entry(*address) + .or_default() + .push((account, is_writable)); + } } fn load_accounts_with_features_and_rent( @@ -640,7 +660,10 @@ mod tests { for (pubkey, account) in accounts { accounts_map.insert(*pubkey, account.clone()); } - let callbacks = TestCallbacks { accounts_map }; + let callbacks = TestCallbacks { + accounts_map, + ..Default::default() + }; load_accounts( &callbacks, &[sanitized_tx], @@ -929,7 +952,10 @@ mod tests { for (pubkey, account) in accounts { accounts_map.insert(*pubkey, account.clone()); } - let callbacks = TestCallbacks { accounts_map }; + let callbacks = TestCallbacks { + accounts_map, + ..Default::default() + }; load_accounts( &callbacks, &[tx], @@ -2108,4 +2134,97 @@ mod tests { assert_eq!(account.rent_epoch(), 0); assert_eq!(account.lamports(), 0); } + + // Ensure `TransactionProcessingCallback::inspect_account()` is called when + // loading accounts for transaction processing. + #[test] + fn test_inspect_account_non_fee_payer() { + let mut mock_bank = TestCallbacks::default(); + + let address0 = Pubkey::new_unique(); // <-- fee payer + let address1 = Pubkey::new_unique(); // <-- initially alive + let address2 = Pubkey::new_unique(); // <-- initially dead + let address3 = Pubkey::new_unique(); // <-- program + + let mut account0 = AccountSharedData::default(); + account0.set_lamports(1_000_000_000); + mock_bank.accounts_map.insert(address0, account0.clone()); + + let mut account1 = AccountSharedData::default(); + account1.set_lamports(2_000_000_000); + mock_bank.accounts_map.insert(address1, account1.clone()); + + // account2 *not* added to the bank's accounts_map + + let mut account3 = AccountSharedData::default(); + account3.set_lamports(4_000_000_000); + account3.set_executable(true); + account3.set_owner(native_loader::id()); + mock_bank.accounts_map.insert(address3, account3.clone()); + + let message = Message { + account_keys: vec![address0, address1, address2, address3], + header: MessageHeader::default(), + instructions: vec![ + CompiledInstruction { + program_id_index: 3, + accounts: vec![0], + data: vec![], + }, + CompiledInstruction { + program_id_index: 3, + accounts: vec![1, 2], + data: vec![], + }, + CompiledInstruction { + program_id_index: 3, + accounts: vec![1], + data: vec![], + }, + ], + recent_blockhash: Hash::new_unique(), + }; + let sanitized_message = new_unchecked_sanitized_message(message); + let sanitized_transaction = SanitizedTransaction::new_for_tests( + sanitized_message, + vec![Signature::new_unique()], + false, + ); + let validation_result = Ok(ValidatedTransactionDetails { + loaded_fee_payer_account: LoadedTransactionAccount { + account: account0.clone(), + ..LoadedTransactionAccount::default() + }, + ..ValidatedTransactionDetails::default() + }); + let _load_results = load_accounts( + &mock_bank, + &[sanitized_transaction], + vec![validation_result], + &mut TransactionErrorMetrics::default(), + None, + &FeatureSet::default(), + &RentCollector::default(), + &ProgramCacheForTxBatch::default(), + ); + + // ensure the loaded accounts are inspected + let mut actual_inspected_accounts: Vec<_> = mock_bank + .inspected_accounts + .borrow() + .iter() + .map(|(k, v)| (*k, v.clone())) + .collect(); + actual_inspected_accounts.sort_unstable_by(|a, b| a.0.cmp(&b.0)); + + let mut expected_inspected_accounts = vec![ + // *not* key0, since it is loaded during fee payer validation + (address1, vec![(Some(account1), true)]), + (address2, vec![(None, true)]), + (address3, vec![(Some(account3), false)]), + ]; + expected_inspected_accounts.sort_unstable_by(|a, b| a.0.cmp(&b.0)); + + assert_eq!(actual_inspected_accounts, expected_inspected_accounts,); + } } diff --git a/svm/src/transaction_processor.rs b/svm/src/transaction_processor.rs index 58ca47269d02f5..c6a5e43149949e 100644 --- a/svm/src/transaction_processor.rs +++ b/svm/src/transaction_processor.rs @@ -15,7 +15,7 @@ use { transaction_account_state_info::TransactionAccountStateInfo, transaction_error_metrics::TransactionErrorMetrics, transaction_execution_result::{ExecutedTransaction, TransactionExecutionDetails}, - transaction_processing_callback::TransactionProcessingCallback, + transaction_processing_callback::{AccountState, TransactionProcessingCallback}, transaction_processing_result::{ProcessedTransaction, TransactionProcessingResult}, }, log::debug, @@ -434,6 +434,12 @@ impl TransactionBatchProcessor { return Err(TransactionError::AccountNotFound); }; + callbacks.inspect_account( + fee_payer_address, + AccountState::Alive(&fee_payer_account), + true, // <-- is_writable + ); + let fee_payer_loaded_rent_epoch = fee_payer_account.rent_epoch(); let fee_payer_rent_debit = collect_rent_from_account( feature_set, @@ -1034,6 +1040,9 @@ mod tests { #[derive(Default, Clone)] pub struct MockBankCallback { pub account_shared_data: Arc>>, + #[allow(clippy::type_complexity)] + pub inspected_accounts: + Arc, /* is_writable */ bool)>>>>, } impl TransactionProcessingCallback for MockBankCallback { @@ -1065,6 +1074,24 @@ mod tests { .unwrap() .insert(*program_id, account_data); } + + fn inspect_account( + &self, + address: &Pubkey, + account_state: AccountState, + is_writable: bool, + ) { + let account = match account_state { + AccountState::Dead => None, + AccountState::Alive(account) => Some(account.clone()), + }; + self.inspected_accounts + .write() + .unwrap() + .entry(*address) + .or_default() + .push((account, is_writable)); + } } #[test] @@ -1853,6 +1880,7 @@ mod tests { mock_accounts.insert(*fee_payer_address, fee_payer_account.clone()); let mock_bank = MockBankCallback { account_shared_data: Arc::new(RwLock::new(mock_accounts)), + ..Default::default() }; let mut error_counters = TransactionErrorMetrics::default(); @@ -1930,6 +1958,7 @@ mod tests { mock_accounts.insert(*fee_payer_address, fee_payer_account.clone()); let mock_bank = MockBankCallback { account_shared_data: Arc::new(RwLock::new(mock_accounts)), + ..Default::default() }; let mut error_counters = TransactionErrorMetrics::default(); @@ -2014,6 +2043,7 @@ mod tests { mock_accounts.insert(*fee_payer_address, fee_payer_account.clone()); let mock_bank = MockBankCallback { account_shared_data: Arc::new(RwLock::new(mock_accounts)), + ..Default::default() }; let mut error_counters = TransactionErrorMetrics::default(); @@ -2051,6 +2081,7 @@ mod tests { mock_accounts.insert(*fee_payer_address, fee_payer_account.clone()); let mock_bank = MockBankCallback { account_shared_data: Arc::new(RwLock::new(mock_accounts)), + ..Default::default() }; let mut error_counters = TransactionErrorMetrics::default(); @@ -2086,6 +2117,7 @@ mod tests { mock_accounts.insert(*fee_payer_address, fee_payer_account.clone()); let mock_bank = MockBankCallback { account_shared_data: Arc::new(RwLock::new(mock_accounts)), + ..Default::default() }; let mut error_counters = TransactionErrorMetrics::default(); @@ -2177,6 +2209,7 @@ mod tests { mock_accounts.insert(*fee_payer_address, fee_payer_account.clone()); let mock_bank = MockBankCallback { account_shared_data: Arc::new(RwLock::new(mock_accounts)), + ..Default::default() }; let mut error_counters = TransactionErrorMetrics::default(); @@ -2242,6 +2275,7 @@ mod tests { mock_accounts.insert(*fee_payer_address, fee_payer_account.clone()); let mock_bank = MockBankCallback { account_shared_data: Arc::new(RwLock::new(mock_accounts)), + ..Default::default() }; let mut error_counters = TransactionErrorMetrics::default(); @@ -2294,6 +2328,7 @@ mod tests { let mock_bank = MockBankCallback { account_shared_data: Arc::new(RwLock::new(mock_accounts)), + ..Default::default() }; let mut error_counters = TransactionErrorMetrics::default(); @@ -2318,4 +2353,61 @@ mod tests { result.err() ); } + + // Ensure `TransactionProcessingCallback::inspect_account()` is called when + // validating the fee payer, since that's when the fee payer account is loaded. + #[test] + fn test_inspect_account_fee_payer() { + let fee_payer_address = Pubkey::new_unique(); + let fee_payer_account = AccountSharedData::new_rent_epoch( + 123_000_000_000, + 0, + &Pubkey::default(), + RENT_EXEMPT_RENT_EPOCH, + ); + let mock_bank = MockBankCallback::default(); + mock_bank + .account_shared_data + .write() + .unwrap() + .insert(fee_payer_address, fee_payer_account.clone()); + + let message = new_unchecked_sanitized_message(Message::new_with_blockhash( + &[ + ComputeBudgetInstruction::set_compute_unit_limit(2000u32), + ComputeBudgetInstruction::set_compute_unit_price(1_000_000_000), + ], + Some(&fee_payer_address), + &Hash::new_unique(), + )); + let batch_processor = TransactionBatchProcessor::::default(); + batch_processor + .validate_transaction_fee_payer( + &mock_bank, + None, + &message, + CheckedTransactionDetails { + nonce: None, + lamports_per_signature: 5000, + }, + &FeatureSet::default(), + &FeeStructure::default(), + &RentCollector::default(), + &mut TransactionErrorMetrics::default(), + ) + .unwrap(); + + // ensure the fee payer is an inspected account + let actual_inspected_accounts: Vec<_> = mock_bank + .inspected_accounts + .read() + .unwrap() + .iter() + .map(|(k, v)| (*k, v.clone())) + .collect(); + assert_eq!( + actual_inspected_accounts.as_slice(), + &[(fee_payer_address, vec![(Some(fee_payer_account), true)])], + ); + } } diff --git a/svm/tests/integration_test.rs b/svm/tests/integration_test.rs index 37a4870812c2d8..08f6db09101e04 100644 --- a/svm/tests/integration_test.rs +++ b/svm/tests/integration_test.rs @@ -655,3 +655,233 @@ fn execute_test_entry(test_entry: SvmTestEntry) { } } } + +#[test] +fn svm_inspect_account() { + let mock_bank = MockBankCallback::default(); + let mut expected_inspected_accounts: HashMap<_, Vec<_>> = HashMap::new(); + + let transfer_program = + deploy_program("simple-transfer".to_string(), DEPLOYMENT_SLOT, &mock_bank); + + let fee_payer_keypair = Keypair::new(); + let sender_keypair = Keypair::new(); + + let fee_payer = fee_payer_keypair.pubkey(); + let sender = sender_keypair.pubkey(); + let recipient = Pubkey::new_unique(); + let system = system_program::id(); + + // Setting up the accounts for the transfer + + // fee payer + let mut fee_payer_account = AccountSharedData::default(); + fee_payer_account.set_lamports(80_020); + mock_bank + .account_shared_data + .write() + .unwrap() + .insert(fee_payer, fee_payer_account.clone()); + expected_inspected_accounts + .entry(fee_payer) + .or_default() + .push((Some(fee_payer_account.clone()), true)); + + // sender + let mut sender_account = AccountSharedData::default(); + sender_account.set_lamports(11_000_000); + mock_bank + .account_shared_data + .write() + .unwrap() + .insert(sender, sender_account.clone()); + expected_inspected_accounts + .entry(sender) + .or_default() + .push((Some(sender_account.clone()), true)); + + // recipient -- initially dead + expected_inspected_accounts + .entry(recipient) + .or_default() + .push((None, true)); + + let instruction = Instruction::new_with_bytes( + transfer_program, + &u64::to_be_bytes(1_000_000), + vec![ + AccountMeta::new(sender, true), + AccountMeta::new(recipient, false), + AccountMeta::new_readonly(system, false), + ], + ); + let transaction = Transaction::new_signed_with_payer( + &[instruction], + Some(&fee_payer), + &[&fee_payer_keypair, &sender_keypair], + Hash::default(), + ); + let sanitized_transaction = SanitizedTransaction::from_transaction_for_tests(transaction); + let transaction_check = Ok(CheckedTransactionDetails { + nonce: None, + lamports_per_signature: 20, + }); + + // Load and execute the transaction + + let batch_processor = TransactionBatchProcessor::::new( + EXECUTION_SLOT, + EXECUTION_EPOCH, + HashSet::new(), + ); + + let fork_graph = Arc::new(RwLock::new(MockForkGraph {})); + + create_executable_environment( + fork_graph.clone(), + &mock_bank, + &mut batch_processor.program_cache.write().unwrap(), + ); + + // The sysvars must be put in the cache + batch_processor.fill_missing_sysvar_cache_entries(&mock_bank); + register_builtins(&mock_bank, &batch_processor); + + let _result = batch_processor.load_and_execute_sanitized_transactions( + &mock_bank, + &[sanitized_transaction], + vec![transaction_check], + &TransactionProcessingEnvironment::default(), + &TransactionProcessingConfig::default(), + ); + + // the system account is modified during transaction processing, + // so set the expected inspected account afterwards. + let system_account = mock_bank + .account_shared_data + .read() + .unwrap() + .get(&system) + .cloned(); + expected_inspected_accounts + .entry(system) + .or_default() + .push((system_account, false)); + + // do another transfer; recipient should be alive now + + // fee payer + let mut fee_payer_account = AccountSharedData::default(); + fee_payer_account.set_lamports(80_000); + mock_bank + .account_shared_data + .write() + .unwrap() + .insert(fee_payer, fee_payer_account.clone()); + expected_inspected_accounts + .entry(fee_payer) + .or_default() + .push((Some(fee_payer_account.clone()), true)); + + // sender + let mut sender_account = AccountSharedData::default(); + sender_account.set_lamports(10_000_000); + mock_bank + .account_shared_data + .write() + .unwrap() + .insert(sender, sender_account.clone()); + expected_inspected_accounts + .entry(sender) + .or_default() + .push((Some(sender_account.clone()), true)); + + // recipient -- now alive + let mut recipient_account = AccountSharedData::default(); + recipient_account.set_lamports(1_000_000); + mock_bank + .account_shared_data + .write() + .unwrap() + .insert(recipient, recipient_account.clone()); + expected_inspected_accounts + .entry(recipient) + .or_default() + .push((Some(recipient_account.clone()), true)); + + let instruction = Instruction::new_with_bytes( + transfer_program, + &u64::to_be_bytes(456), + vec![ + AccountMeta::new(sender, true), + AccountMeta::new(recipient, false), + AccountMeta::new_readonly(system, false), + ], + ); + let transaction = Transaction::new_signed_with_payer( + &[instruction], + Some(&fee_payer), + &[&fee_payer_keypair, &sender_keypair], + Hash::default(), + ); + let sanitized_transaction = SanitizedTransaction::from_transaction_for_tests(transaction); + let transaction_check = Ok(CheckedTransactionDetails { + nonce: None, + lamports_per_signature: 20, + }); + + // Load and execute the second transaction + let _result = batch_processor.load_and_execute_sanitized_transactions( + &mock_bank, + &[sanitized_transaction], + vec![transaction_check], + &TransactionProcessingEnvironment::default(), + &TransactionProcessingConfig::default(), + ); + + // the system account is modified during transaction processing, + // so set the expected inspected account afterwards. + let system_account = mock_bank + .account_shared_data + .read() + .unwrap() + .get(&system) + .cloned(); + expected_inspected_accounts + .entry(system) + .or_default() + .push((system_account, false)); + + // Ensure all the expected inspected accounts were inspected + let actual_inspected_accounts = mock_bank.inspected_accounts.read().unwrap().clone(); + for (expected_pubkey, expected_account) in &expected_inspected_accounts { + let actual_account = actual_inspected_accounts.get(expected_pubkey).unwrap(); + assert_eq!( + expected_account, actual_account, + "pubkey: {expected_pubkey}", + ); + } + + // The transfer program account is also loaded during transaction processing, however the + // account state passed to `inspect_account()` is *not* the same as what is held by + // MockBankCallback::account_shared_data. So we check the transfer program differently. + // + // First ensure we have the correct number of inspected accounts, correctly counting the + // transfer program. + let num_expected_inspected_accounts: usize = + expected_inspected_accounts.values().map(Vec::len).sum(); + let num_actual_inspected_accounts: usize = + actual_inspected_accounts.values().map(Vec::len).sum(); + assert_eq!( + num_expected_inspected_accounts + 2, + num_actual_inspected_accounts, + ); + + // And second, ensure the inspected transfer program accounts are alive and not writable. + let actual_transfer_program_accounts = + actual_inspected_accounts.get(&transfer_program).unwrap(); + for actual_transfer_program_account in actual_transfer_program_accounts { + assert!(actual_transfer_program_account.0.is_some()); + assert!(!actual_transfer_program_account.1); + } +} diff --git a/svm/tests/mock_bank.rs b/svm/tests/mock_bank.rs index ee6a6692a7b0fe..6ece85bfcc1385 100644 --- a/svm/tests/mock_bank.rs +++ b/svm/tests/mock_bank.rs @@ -25,7 +25,7 @@ use { sysvar::SysvarId, }, solana_svm::{ - transaction_processing_callback::TransactionProcessingCallback, + transaction_processing_callback::{AccountState, TransactionProcessingCallback}, transaction_processor::TransactionBatchProcessor, }, solana_type_overrides::sync::{Arc, RwLock}, @@ -56,6 +56,9 @@ impl ForkGraph for MockForkGraph { pub struct MockBankCallback { pub feature_set: Arc, pub account_shared_data: Arc>>, + #[allow(clippy::type_complexity)] + pub inspected_accounts: + Arc, /* is_writable */ bool)>>>>, } impl TransactionProcessingCallback for MockBankCallback { @@ -87,6 +90,19 @@ impl TransactionProcessingCallback for MockBankCallback { .unwrap() .insert(*program_id, account_data); } + + fn inspect_account(&self, address: &Pubkey, account_state: AccountState, is_writable: bool) { + let account = match account_state { + AccountState::Dead => None, + AccountState::Alive(account) => Some(account.clone()), + }; + self.inspected_accounts + .write() + .unwrap() + .entry(*address) + .or_default() + .push((account, is_writable)); + } } impl MockBankCallback {