From b6fc1676882fb203d7d005bf13ce579e1d6c0f4f Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Fri, 8 Nov 2024 18:27:12 +0100 Subject: [PATCH] v2.1: Revert - #879 and #768 (backport of #3521) (#3544) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Revert - #879 and #768 (#3521) * 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. (cherry picked from commit 57bdb8e2e218e3ce791564954e48f27b43c845e1) Co-authored-by: Alexander Meißner --- svm/src/account_loader.rs | 77 ++++++++++++++++++++++++-------- svm/src/transaction_processor.rs | 61 ++++++++++++++----------- svm/tests/concurrent_tests.rs | 6 ++- 3 files changed, 96 insertions(+), 48 deletions(-) diff --git a/svm/src/account_loader.rs b/svm/src/account_loader.rs index 9cc7eb6ecf37b6..fbaa222e938a23 100644 --- a/svm/src/account_loader.rs +++ b/svm/src/account_loader.rs @@ -9,7 +9,7 @@ use { itertools::Itertools, solana_compute_budget::compute_budget_limits::ComputeBudgetLimits, solana_feature_set::{self as feature_set, FeatureSet}, - solana_program_runtime::loaded_programs::{ProgramCacheEntry, ProgramCacheForTxBatch}, + solana_program_runtime::loaded_programs::ProgramCacheForTxBatch, solana_sdk::{ account::{Account, AccountSharedData, ReadableAccount, WritableAccount}, fee::FeeDetails, @@ -30,7 +30,7 @@ use { solana_svm_rent_collector::svm_rent_collector::SVMRentCollector, solana_svm_transaction::svm_message::SVMMessage, solana_system_program::{get_system_account_kind, SystemAccountKind}, - std::num::NonZeroU32, + std::{collections::HashMap, num::NonZeroU32}, }; // for the load instructions @@ -194,6 +194,7 @@ pub(crate) fn load_accounts( account_overrides: Option<&AccountOverrides>, feature_set: &FeatureSet, rent_collector: &dyn SVMRentCollector, + program_accounts: &HashMap, loaded_programs: &ProgramCacheForTxBatch, ) -> Vec { txs.iter() @@ -207,6 +208,7 @@ pub(crate) fn load_accounts( account_overrides, feature_set, rent_collector, + program_accounts, loaded_programs, ) }) @@ -221,6 +223,7 @@ fn load_transaction( account_overrides: Option<&AccountOverrides>, feature_set: &FeatureSet, rent_collector: &dyn SVMRentCollector, + program_accounts: &HashMap, loaded_programs: &ProgramCacheForTxBatch, ) -> TransactionLoadResult { match validation_result { @@ -235,6 +238,7 @@ fn load_transaction( account_overrides, feature_set, rent_collector, + program_accounts, loaded_programs, ); @@ -268,6 +272,7 @@ struct LoadedTransactionAccounts { pub loaded_accounts_data_size: u32, } +#[allow(clippy::too_many_arguments)] fn load_transaction_accounts( callbacks: &CB, message: &impl SVMMessage, @@ -277,6 +282,7 @@ fn load_transaction_accounts( account_overrides: Option<&AccountOverrides>, feature_set: &FeatureSet, rent_collector: &dyn SVMRentCollector, + program_accounts: &HashMap, loaded_programs: &ProgramCacheForTxBatch, ) -> Result { let mut tx_rent: TransactionRent = 0; @@ -331,6 +337,7 @@ fn load_transaction_accounts( account_overrides, feature_set, rent_collector, + program_accounts, loaded_programs, )?; collect_loaded_account(account_key, (loaded_account, account_found))?; @@ -403,6 +410,7 @@ fn load_transaction_accounts( }) } +#[allow(clippy::too_many_arguments)] fn load_transaction_account( callbacks: &CB, message: &impl SVMMessage, @@ -412,6 +420,7 @@ fn load_transaction_account( account_overrides: Option<&AccountOverrides>, feature_set: &FeatureSet, rent_collector: &dyn SVMRentCollector, + program_accounts: &HashMap, loaded_programs: &ProgramCacheForTxBatch, ) -> Result<(LoadedTransactionAccount, bool)> { let mut account_found = true; @@ -439,14 +448,11 @@ fn load_transaction_account( .then_some(()) .and_then(|_| loaded_programs.find(account_key)) { - callbacks - .get_account_shared_data(account_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. LoadedTransactionAccount { loaded_size: program.account_size, - account: account_shared_data_from_program(&program), + account: account_shared_data_from_program(account_key, program_accounts)?, rent_collected: 0, } } else { @@ -496,14 +502,20 @@ fn load_transaction_account( Ok((loaded_account, account_found)) } -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`. @@ -670,6 +682,7 @@ mod tests { None, feature_set, rent_collector, + &HashMap::new(), &ProgramCacheForTxBatch::default(), ) } @@ -978,6 +991,7 @@ mod tests { account_overrides, &FeatureSet::all_enabled(), &RentCollector::default(), + &HashMap::new(), &ProgramCacheForTxBatch::default(), ) } @@ -1273,6 +1287,7 @@ mod tests { None, &FeatureSet::default(), &RentCollector::default(), + &HashMap::new(), &loaded_programs, ); @@ -1338,6 +1353,7 @@ mod tests { None, &FeatureSet::default(), &RentCollector::default(), + &HashMap::new(), &loaded_programs, ); @@ -1371,6 +1387,7 @@ mod tests { let mut mock_bank = TestCallbacks::default(); let account_keypair = Keypair::new(); let program_keypair = Keypair::new(); + let loader_v2 = bpf_loader::id(); let mut account_data = AccountSharedData::default(); account_data.set_lamports(200); @@ -1380,7 +1397,7 @@ mod tests { let mut program_data = AccountSharedData::default(); program_data.set_lamports(200); - program_data.set_owner(bpf_loader::id()); + program_data.set_owner(loader_v2); mock_bank .accounts_map .insert(program_keypair.pubkey(), program_data); @@ -1391,7 +1408,7 @@ mod tests { loader_data.set_owner(native_loader::id()); mock_bank .accounts_map - .insert(bpf_loader::id(), loader_data.clone()); + .insert(loader_v2, loader_data.clone()); mock_bank .accounts_map .insert(native_loader::id(), loader_data.clone()); @@ -1408,6 +1425,8 @@ mod tests { Hash::default(), )); + let mut program_accounts = HashMap::new(); + program_accounts.insert(program_keypair.pubkey(), (&loader_v2, 0)); let mut loaded_programs = ProgramCacheForTxBatch::default(); loaded_programs.replenish( program_keypair.pubkey(), @@ -1431,12 +1450,13 @@ mod tests { None, &FeatureSet::default(), &RentCollector::default(), + &program_accounts, &loaded_programs, ); // Executable flag is bypassed let mut cached_program = AccountSharedData::default(); - cached_program.set_owner(bpf_loader::id()); + cached_program.set_owner(loader_v2); cached_program.set_executable(true); assert_eq!( @@ -1445,7 +1465,7 @@ mod tests { accounts: vec![ (account_keypair.pubkey(), account_data.clone()), (program_keypair.pubkey(), cached_program), - (bpf_loader::id(), loader_data), + (loader_v2, loader_data), ], program_indices: vec![vec![1]], rent: 0, @@ -1478,6 +1498,7 @@ mod tests { None, &FeatureSet::default(), &RentCollector::default(), + &program_accounts, &loaded_programs, ); @@ -1504,6 +1525,7 @@ mod tests { None, &FeatureSet::default(), &RentCollector::default(), + &program_accounts, &loaded_programs, ); @@ -1553,6 +1575,7 @@ mod tests { None, &FeatureSet::default(), &RentCollector::default(), + &HashMap::new(), &loaded_programs, ); @@ -1598,6 +1621,7 @@ mod tests { None, &FeatureSet::default(), &RentCollector::default(), + &HashMap::new(), &loaded_programs, ); @@ -1655,6 +1679,7 @@ mod tests { None, &FeatureSet::default(), &RentCollector::default(), + &HashMap::new(), &loaded_programs, ); @@ -1718,6 +1743,7 @@ mod tests { None, &FeatureSet::default(), &RentCollector::default(), + &HashMap::new(), &loaded_programs, ); @@ -1772,6 +1798,7 @@ mod tests { None, &FeatureSet::default(), &RentCollector::default(), + &HashMap::new(), &loaded_programs, ); @@ -1836,6 +1863,7 @@ mod tests { None, &FeatureSet::default(), &RentCollector::default(), + &HashMap::new(), &loaded_programs, ); @@ -1924,6 +1952,7 @@ mod tests { None, &FeatureSet::default(), &RentCollector::default(), + &HashMap::new(), &loaded_programs, ); @@ -1989,6 +2018,7 @@ mod tests { None, &FeatureSet::default(), &RentCollector::default(), + &HashMap::new(), &ProgramCacheForTxBatch::default(), ); @@ -2085,6 +2115,7 @@ mod tests { None, &FeatureSet::default(), &RentCollector::default(), + &HashMap::new(), &loaded_programs, ); @@ -2157,6 +2188,7 @@ mod tests { None, &feature_set, &rent_collector, + &HashMap::new(), &ProgramCacheForTxBatch::default(), ); @@ -2178,6 +2210,7 @@ mod tests { None, &feature_set, &rent_collector, + &HashMap::new(), &ProgramCacheForTxBatch::default(), ); @@ -2328,6 +2361,7 @@ mod tests { None, &FeatureSet::default(), &RentCollector::default(), + &HashMap::new(), &ProgramCacheForTxBatch::default(), ); @@ -2355,6 +2389,8 @@ mod tests { fn test_load_transaction_accounts_data_sizes() { let mut mock_bank = TestCallbacks::default(); + let loader_v2 = bpf_loader::id(); + let loader_v3 = bpf_loader_upgradeable::id(); let program1_keypair = Keypair::new(); let program1 = program1_keypair.pubkey(); let program2 = Pubkey::new_unique(); @@ -2363,7 +2399,7 @@ mod tests { let program2_size = std::mem::size_of::() as u32; let mut program2_account = AccountSharedData::default(); - program2_account.set_owner(bpf_loader_upgradeable::id()); + program2_account.set_owner(loader_v3); program2_account.set_executable(true); program2_account.set_data(vec![0; program2_size as usize]); program2_account @@ -2373,7 +2409,7 @@ mod tests { .unwrap(); mock_bank.accounts_map.insert(program2, program2_account); let mut programdata2_account = AccountSharedData::default(); - programdata2_account.set_owner(bpf_loader_upgradeable::id()); + programdata2_account.set_owner(loader_v3); programdata2_account.set_data(vec![0; program2_size as usize]); programdata2_account .set_state(&UpgradeableLoaderState::ProgramData { @@ -2423,12 +2459,14 @@ mod tests { let (account2_size, _) = make_account(account2, program2, false); let (native_loader_size, _) = make_account(native_loader::id(), native_loader::id(), true); - let (bpf_loader_size, _) = make_account(bpf_loader::id(), native_loader::id(), true); - let (upgradeable_loader_size, _) = - make_account(bpf_loader_upgradeable::id(), native_loader::id(), true); + let (bpf_loader_size, _) = make_account(loader_v2, native_loader::id(), true); + let (upgradeable_loader_size, _) = make_account(loader_v3, native_loader::id(), true); - let (_program1_size, _) = make_account(program1, bpf_loader::id(), true); + let (_program1_size, _) = make_account(program1, loader_v2, true); + let mut program_accounts = HashMap::new(); + program_accounts.insert(program1, (&loader_v2, 0)); + program_accounts.insert(program2, (&loader_v3, 0)); let mut program_cache = ProgramCacheForTxBatch::default(); let program1_entry = ProgramCacheEntry { account_size: 0, @@ -2459,6 +2497,7 @@ mod tests { None, &FeatureSet::default(), &RentCollector::default(), + &program_accounts, &program_cache, ) .unwrap(); diff --git a/svm/src/transaction_processor.rs b/svm/src/transaction_processor.rs index 551ff30076252d..14a67bc21702b8 100644 --- a/svm/src/transaction_processor.rs +++ b/svm/src/transaction_processor.rs @@ -45,6 +45,7 @@ use { hash::Hash, inner_instruction::{InnerInstruction, InnerInstructionsList}, instruction::{CompiledInstruction, TRANSACTION_LEVEL_STACK_HEIGHT}, + native_loader, pubkey::Pubkey, rent_collector::RentCollector, saturating_add_assign, @@ -278,19 +279,21 @@ impl TransactionBatchProcessor { &mut error_metrics )); - let (mut program_accounts_map, filter_executable_us) = - measure_us!(Self::filter_executable_program_accounts( + let native_loader = native_loader::id(); + let (program_accounts_map, filter_executable_us) = measure_us!({ + let mut program_accounts_map = Self::filter_executable_program_accounts( callbacks, sanitized_txs, &validation_results, - PROGRAM_OWNERS - )); - - let (mut program_cache_for_tx_batch, program_cache_us) = measure_us!({ + PROGRAM_OWNERS, + ); 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)); } + program_accounts_map + }); + let (mut program_cache_for_tx_batch, program_cache_us) = measure_us!({ let program_cache_for_tx_batch = self.replenish_program_cache( callbacks, &program_accounts_map, @@ -321,6 +324,7 @@ impl TransactionBatchProcessor { environment .rent_collector .unwrap_or(&RentCollector::default()), + &program_accounts_map, &program_cache_for_tx_batch, )); @@ -529,28 +533,29 @@ 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: &[impl SVMMessage], 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.account_keys() .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)); + } } } }); @@ -563,14 +568,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| { @@ -1377,9 +1382,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); } @@ -1392,6 +1398,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()); @@ -1401,8 +1408,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] { @@ -1511,8 +1518,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] @@ -1601,13 +1608,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) ); } @@ -1699,7 +1706,7 @@ mod tests { programs .get(&account3_pubkey) .expect("failed to find the program account"), - &1 + &(&program1_pubkey, 1) ); } diff --git a/svm/tests/concurrent_tests.rs b/svm/tests/concurrent_tests.rs index 4d0a500b845bc7..021d91915f31e5 100644 --- a/svm/tests/concurrent_tests.rs +++ b/svm/tests/concurrent_tests.rs @@ -16,6 +16,7 @@ use { solana_program_runtime::loaded_programs::ProgramCacheEntryType, solana_sdk::{ account::{AccountSharedData, ReadableAccount, WritableAccount}, + bpf_loader_upgradeable, hash::Hash, instruction::AccountMeta, pubkey::Pubkey, @@ -44,16 +45,17 @@ fn program_cache_execution(threads: usize) { let fork_graph = Arc::new(RwLock::new(MockForkGraph {})); 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(), 0, &mut mock_bank), deploy_program("simple-transfer".to_string(), 0, &mut mock_bank), deploy_program("clock-sysvar".to_string(), 0, &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(); let ths: Vec<_> = (0..threads)