From 204d282e4417498b47ef7c4ebd91cfc4c43b89a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Mei=C3=9Fner?= Date: Thu, 7 Nov 2024 11:32:44 +0100 Subject: [PATCH] Revert - #879 and #768 (#3511) * Revert "Cleanup - Removes the owner form the result of `filter_executable_program_accounts()` (#879)" This reverts commit 5fe30cb788232da8e853c7c70a3070395df6af0a. * Revert "Refactor - Remove `program_accounts_map` from account_loader (#768)" This reverts commit e7617a1b1fa53a3908a79270e0f4616a36d489c0. --- svm/src/account_loader.rs | 42 +++++++++++++++++++------ svm/src/transaction_processor.rs | 53 ++++++++++++++++++-------------- 2 files changed, 62 insertions(+), 33 deletions(-) diff --git a/svm/src/account_loader.rs b/svm/src/account_loader.rs index c62963c09e0934..080433a977941a 100644 --- a/svm/src/account_loader.rs +++ b/svm/src/account_loader.rs @@ -9,7 +9,7 @@ use { solana_compute_budget::compute_budget_processor::{ process_compute_budget_instructions, ComputeBudgetLimits, }, - solana_program_runtime::loaded_programs::{ProgramCacheEntry, ProgramCacheForTxBatch}, + solana_program_runtime::loaded_programs::ProgramCacheForTxBatch, solana_sdk::{ account::{Account, AccountSharedData, ReadableAccount, WritableAccount}, feature_set::{self, FeatureSet}, @@ -27,7 +27,7 @@ use { transaction_context::{IndexOfAccount, TransactionAccount}, }, solana_system_program::{get_system_account_kind, SystemAccountKind}, - std::num::NonZeroUsize, + std::{collections::HashMap, num::NonZeroUsize}, }; // for the load instructions @@ -162,6 +162,7 @@ pub(crate) fn load_accounts( account_overrides: Option<&AccountOverrides>, feature_set: &FeatureSet, rent_collector: &RentCollector, + program_accounts: &HashMap, loaded_programs: &ProgramCacheForTxBatch, ) -> Vec { txs.iter() @@ -179,6 +180,7 @@ pub(crate) fn load_accounts( account_overrides, feature_set, rent_collector, + program_accounts, loaded_programs, ) } @@ -195,6 +197,7 @@ fn load_transaction_accounts( account_overrides: Option<&AccountOverrides>, feature_set: &FeatureSet, rent_collector: &RentCollector, + program_accounts: &HashMap, loaded_programs: &ProgramCacheForTxBatch, ) -> Result { let mut tx_rent: TransactionRent = 0; @@ -240,13 +243,10 @@ fn load_transaction_accounts( .then_some(()) .and_then(|_| loaded_programs.find(key)) { - callbacks - .get_account_shared_data(key) - .ok_or(TransactionError::AccountNotFound)?; // Optimization to skip loading of accounts which are only used as // programs in top-level instructions and not passed as instruction accounts. - let program_account = account_shared_data_from_program(&program); - (program.account_size, program_account, 0) + account_shared_data_from_program(key, program_accounts) + .map(|program_account| (program.account_size, program_account, 0))? } else { callbacks .get_account_shared_data(key) @@ -391,14 +391,20 @@ fn get_requested_loaded_accounts_data_size_limit( ) } -fn account_shared_data_from_program(loaded_program: &ProgramCacheEntry) -> AccountSharedData { +fn account_shared_data_from_program( + key: &Pubkey, + program_accounts: &HashMap, +) -> Result { // It's an executable program account. The program is already loaded in the cache. // So the account data is not needed. Return a dummy AccountSharedData with meta // information. let mut program_account = AccountSharedData::default(); - program_account.set_owner(loaded_program.account_owner()); + let (program_owner, _count) = program_accounts + .get(key) + .ok_or(TransactionError::AccountNotFound)?; + program_account.set_owner(**program_owner); program_account.set_executable(true); - program_account + Ok(program_account) } /// Accumulate loaded account data size into `accumulated_accounts_data_size`. @@ -511,6 +517,7 @@ mod tests { None, feature_set, rent_collector, + &HashMap::new(), &ProgramCacheForTxBatch::default(), ) } @@ -796,6 +803,7 @@ mod tests { account_overrides, &FeatureSet::all_enabled(), &RentCollector::default(), + &HashMap::new(), &ProgramCacheForTxBatch::default(), ) } @@ -1153,6 +1161,7 @@ mod tests { None, &FeatureSet::default(), &RentCollector::default(), + &HashMap::new(), &loaded_programs, ); @@ -1220,6 +1229,7 @@ mod tests { None, &FeatureSet::default(), &RentCollector::default(), + &HashMap::new(), &loaded_programs, ); @@ -1283,6 +1293,7 @@ mod tests { None, &FeatureSet::default(), &RentCollector::default(), + &HashMap::new(), &loaded_programs, ); @@ -1327,6 +1338,7 @@ mod tests { None, &FeatureSet::default(), &RentCollector::default(), + &HashMap::new(), &loaded_programs, ); @@ -1371,6 +1383,7 @@ mod tests { None, &FeatureSet::default(), &RentCollector::default(), + &HashMap::new(), &loaded_programs, ); @@ -1427,6 +1440,7 @@ mod tests { None, &FeatureSet::default(), &RentCollector::default(), + &HashMap::new(), &loaded_programs, ); @@ -1492,6 +1506,7 @@ mod tests { None, &FeatureSet::default(), &RentCollector::default(), + &HashMap::new(), &loaded_programs, ); @@ -1545,6 +1560,7 @@ mod tests { None, &FeatureSet::default(), &RentCollector::default(), + &HashMap::new(), &loaded_programs, ); @@ -1608,6 +1624,7 @@ mod tests { None, &FeatureSet::default(), &RentCollector::default(), + &HashMap::new(), &loaded_programs, ); @@ -1698,6 +1715,7 @@ mod tests { None, &FeatureSet::default(), &RentCollector::default(), + &HashMap::new(), &loaded_programs, ); @@ -1766,6 +1784,7 @@ mod tests { None, &FeatureSet::default(), &RentCollector::default(), + &HashMap::new(), &ProgramCacheForTxBatch::default(), ); @@ -1854,6 +1873,7 @@ mod tests { None, &FeatureSet::default(), &RentCollector::default(), + &HashMap::new(), &loaded_programs, ); @@ -1924,6 +1944,7 @@ mod tests { None, &feature_set, &rent_collector, + &HashMap::new(), &ProgramCacheForTxBatch::default(), ); @@ -1942,6 +1963,7 @@ mod tests { None, &feature_set, &rent_collector, + &HashMap::new(), &ProgramCacheForTxBatch::default(), ); diff --git a/svm/src/transaction_processor.rs b/svm/src/transaction_processor.rs index edcba870c6e89f..309b7927b437ac 100644 --- a/svm/src/transaction_processor.rs +++ b/svm/src/transaction_processor.rs @@ -47,6 +47,7 @@ use { inner_instruction::{InnerInstruction, InnerInstructionsList}, instruction::{CompiledInstruction, TRANSACTION_LEVEL_STACK_HEIGHT}, message::SanitizedMessage, + native_loader, pubkey::Pubkey, rent_collector::RentCollector, saturating_add_assign, @@ -255,8 +256,9 @@ impl TransactionBatchProcessor { &validation_results, PROGRAM_OWNERS, ); + let native_loader = native_loader::id(); for builtin_program in self.builtin_program_ids.read().unwrap().iter() { - program_accounts_map.insert(*builtin_program, 0); + program_accounts_map.insert(*builtin_program, (&native_loader, 0)); } let program_cache_for_tx_batch = Rc::new(RefCell::new(self.replenish_program_cache( @@ -291,6 +293,7 @@ impl TransactionBatchProcessor { environment .rent_collector .unwrap_or(&RentCollector::default()), + &program_accounts_map, &program_cache_for_tx_batch.borrow(), ); load_time.stop(); @@ -497,13 +500,13 @@ impl TransactionBatchProcessor { /// Returns a map from executable program accounts (all accounts owned by any loader) /// to their usage counters, for the transactions with a valid blockhash or nonce. - fn filter_executable_program_accounts( + fn filter_executable_program_accounts<'a, CB: TransactionProcessingCallback>( callbacks: &CB, txs: &[SanitizedTransaction], validation_results: &[TransactionValidationResult], - program_owners: &[Pubkey], - ) -> HashMap { - let mut result: HashMap = HashMap::new(); + program_owners: &'a [Pubkey], + ) -> HashMap { + let mut result: HashMap = HashMap::new(); validation_results.iter().zip(txs).for_each(|etx| { if let (Ok(_), tx) = etx { tx.message() @@ -511,15 +514,16 @@ impl TransactionBatchProcessor { .iter() .for_each(|key| match result.entry(*key) { Entry::Occupied(mut entry) => { - let count = entry.get_mut(); + let (_, count) = entry.get_mut(); saturating_add_assign!(*count, 1); } Entry::Vacant(entry) => { - if callbacks - .account_matches_owners(key, program_owners) - .is_some() + if let Some(index) = + callbacks.account_matches_owners(key, program_owners) { - entry.insert(1); + if let Some(owner) = program_owners.get(index) { + entry.insert((owner, 1)); + } } } }); @@ -531,14 +535,14 @@ impl TransactionBatchProcessor { fn replenish_program_cache( &self, callback: &CB, - program_accounts_map: &HashMap, + program_accounts_map: &HashMap, check_program_modification_slot: bool, limit_to_load_programs: bool, ) -> ProgramCacheForTxBatch { let mut missing_programs: Vec<(Pubkey, (ProgramCacheMatchCriteria, u64))> = program_accounts_map .iter() - .map(|(pubkey, count)| { + .map(|(pubkey, (_, count))| { let match_criteria = if check_program_modification_slot { get_program_modification_slot(callback, pubkey) .map_or(ProgramCacheMatchCriteria::Tombstone, |slot| { @@ -1317,9 +1321,10 @@ mod tests { batch_processor.program_cache.write().unwrap().fork_graph = Some(Arc::downgrade(&fork_graph)); let key = Pubkey::new_unique(); + let owner = Pubkey::new_unique(); - let mut account_maps: HashMap = HashMap::new(); - account_maps.insert(key, 4); + let mut account_maps: HashMap = HashMap::new(); + account_maps.insert(key, (&owner, 4)); batch_processor.replenish_program_cache(&mock_bank, &account_maps, false, true); } @@ -1332,6 +1337,7 @@ mod tests { batch_processor.program_cache.write().unwrap().fork_graph = Some(Arc::downgrade(&fork_graph)); let key = Pubkey::new_unique(); + let owner = Pubkey::new_unique(); let mut account_data = AccountSharedData::default(); account_data.set_owner(bpf_loader::id()); @@ -1341,8 +1347,8 @@ mod tests { .unwrap() .insert(key, account_data); - let mut account_maps: HashMap = HashMap::new(); - account_maps.insert(key, 4); + let mut account_maps: HashMap = HashMap::new(); + account_maps.insert(key, (&owner, 4)); let mut loaded_missing = 0; for limit_to_load_programs in [false, true] { @@ -1451,8 +1457,8 @@ mod tests { ); assert_eq!(result.len(), 2); - assert_eq!(result[&key1], 2); - assert_eq!(result[&key2], 1); + assert_eq!(result[&key1], (&owner1, 2)); + assert_eq!(result[&key2], (&owner2, 1)); } #[test] @@ -1541,13 +1547,13 @@ mod tests { programs .get(&account3_pubkey) .expect("failed to find the program account"), - &2 + &(&program1_pubkey, 2) ); assert_eq!( programs .get(&account4_pubkey) .expect("failed to find the program account"), - &1 + &(&program2_pubkey, 1) ); } @@ -1639,7 +1645,7 @@ mod tests { programs .get(&account3_pubkey) .expect("failed to find the program account"), - &1 + &(&program1_pubkey, 1) ); } @@ -1864,16 +1870,17 @@ mod tests { batch_processor.program_cache.write().unwrap().fork_graph = Some(Arc::downgrade(&fork_graph)); + const LOADER: Pubkey = bpf_loader_upgradeable::id(); let programs = vec![ deploy_program("hello-solana".to_string(), &mut mock_bank), deploy_program("simple-transfer".to_string(), &mut mock_bank), deploy_program("clock-sysvar".to_string(), &mut mock_bank), ]; - let account_maps: HashMap = programs + let account_maps: HashMap = programs .iter() .enumerate() - .map(|(idx, key)| (*key, idx as u64)) + .map(|(idx, key)| (*key, (&LOADER, idx as u64))) .collect(); for _ in 0..10 {