diff --git a/svm/src/account_loader.rs b/svm/src/account_loader.rs index eadef33eafe141..279b91a2c38501 100644 --- a/svm/src/account_loader.rs +++ b/svm/src/account_loader.rs @@ -4,7 +4,7 @@ use { nonce_info::NonceInfo, rollback_accounts::RollbackAccounts, transaction_error_metrics::TransactionErrorMetrics, - transaction_processing_callback::{/* XXX AccountState, */ TransactionProcessingCallback,}, + transaction_processing_callback::{AccountState, TransactionProcessingCallback}, }, itertools::Itertools, solana_compute_budget::compute_budget_limits::ComputeBudgetLimits, @@ -228,24 +228,35 @@ pub(crate) fn load_accounts( .map(|(tx, _)| tx) .collect(); - let account_keys: Vec<_> = checked_messages - .iter() - .flat_map(|m| m.account_keys().iter()) - .unique() - .collect(); + let mut account_key_map = HashMap::new(); + for message in checked_messages.iter() { + for (account_index, account_key) in message.account_keys().iter().enumerate() { + if message.is_writable(account_index) { + account_key_map.insert(account_key, true); + } else { + account_key_map.entry(account_key).or_insert(false); + } + } + } - for key in account_keys { - if solana_sdk::sysvar::instructions::check_id(key) { + for (account_key, is_writable) in account_key_map { + if solana_sdk::sysvar::instructions::check_id(account_key) { continue; } else if let Some(account_override) = - account_overrides.and_then(|overrides| overrides.get(key)) + account_overrides.and_then(|overrides| overrides.get(account_key)) { - accounts_map.insert(*key, account_override.clone()); - } else if let Some(account) = callbacks.get_account_shared_data(key) { - accounts_map.insert(*key, account); + accounts_map.insert(*account_key, account_override.clone()); + } else if let Some(account) = callbacks.get_account_shared_data(account_key) { + callbacks.inspect_account(account_key, AccountState::Alive(&account), is_writable); + + accounts_map.insert(*account_key, account); } // XXX we do not insert the default account here because we need to know if its not found later // it raises the question however of whether we need to track if a program account is created in the batch + // XXX HANA FIXME UPDATE check with brooks on monday........ i dont know why we need to do this + else { + callbacks.inspect_account(account_key, AccountState::Dead, is_writable); + } } // XXX it is possible we want to fully validate programs for messages here @@ -522,18 +533,6 @@ fn load_transaction_account( .cloned() // XXX new clone .map(|mut account| { let rent_collected = if is_writable { - // Inspect the account prior to collecting rent, since - // rent collection can modify the account. - /* XXX HANA i messaged brooks asking what this is... newly added yesterday 8/22 - debug_assert!(!was_inspected); - callbacks.inspect_account( - account_key, - AccountState::Alive(&account), - is_writable, - ); - was_inspected = true; - */ - collect_rent_from_account( feature_set, rent_collector, @@ -566,17 +565,6 @@ fn load_transaction_account( }) }; - /* XXX as noted above - if !was_inspected { - let account_state = if account_found { - AccountState::Alive(&loaded_account.account) - } else { - AccountState::Dead - }; - callbacks.inspect_account(account_key, account_state, is_writable); - } - */ - Ok((loaded_account, account_found)) } @@ -650,7 +638,7 @@ mod tests { super::*, crate::{ transaction_account_state_info::TransactionAccountStateInfo, - transaction_processing_callback::{AccountState, TransactionProcessingCallback}, + transaction_processing_callback::TransactionProcessingCallback, }, nonce::state::Versions as NonceVersions, solana_compute_budget::{compute_budget::ComputeBudget, compute_budget_limits}, @@ -2276,7 +2264,7 @@ mod tests { 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 + (address0, vec![(Some(account0), true)]), (address1, vec![(Some(account1), true)]), (address2, vec![(None, true)]), (address3, vec![(Some(account3), false)]), diff --git a/svm/src/transaction_processor.rs b/svm/src/transaction_processor.rs index 92dab15cd7793c..f82845d0fe18ca 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::{AccountState, TransactionProcessingCallback}, + transaction_processing_callback::TransactionProcessingCallback, transaction_processing_result::{ProcessedTransaction, TransactionProcessingResult}, }, log::debug, @@ -281,7 +281,7 @@ impl TransactionBatchProcessor { .feature_set .is_active(&feature_set::enable_transaction_loading_failure_fees::id()); - for (tx, check_result) in sanitized_txs.into_iter().zip(check_results) { + for (tx, check_result) in sanitized_txs.iter().zip(check_results) { let validate_result = check_result.and_then(|tx_details| { // XXX this shouldnt take callback or override self.validate_transaction_fee_payer( @@ -448,12 +448,6 @@ 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, @@ -1014,7 +1008,7 @@ mod tests { super::*, crate::{ account_loader::ValidatedTransactionDetails, nonce_info::NonceInfo, - rollback_accounts::RollbackAccounts, + rollback_accounts::RollbackAccounts, transaction_processing_callback::AccountState, }, solana_compute_budget::compute_budget_limits::ComputeBudgetLimits, solana_program_runtime::loaded_programs::{BlockRelation, ProgramCacheEntryType}, @@ -2374,61 +2368,4 @@ 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)])], - ); - } }