From a0bc125771e4d6b18b18801ad733d4b6cda36e71 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Mei=C3=9Fner?= Date: Thu, 7 Nov 2024 02:34:38 +0000 Subject: [PATCH] Revert "Refactor - Remove `program_accounts_map` from account_loader (#768)" This reverts commit e7617a1b1fa53a3908a79270e0f4616a36d489c0. --- svm/src/account_loader.rs | 75 ++++++++++++++++++++++++-------- svm/src/transaction_processor.rs | 10 ++--- 2 files changed, 61 insertions(+), 24 deletions(-) diff --git a/svm/src/account_loader.rs b/svm/src/account_loader.rs index b81969379db712..d162f55e1ded34 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, ); @@ -277,6 +281,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 +336,7 @@ fn load_transaction_accounts( account_overrides, feature_set, rent_collector, + program_accounts, loaded_programs, )?; collect_loaded_account(account_key, (loaded_account, account_found))?; @@ -416,6 +422,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; @@ -443,14 +450,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 { @@ -500,14 +504,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`. @@ -674,6 +684,7 @@ mod tests { None, feature_set, rent_collector, + &HashMap::new(), &ProgramCacheForTxBatch::default(), ) } @@ -990,6 +1001,7 @@ mod tests { account_overrides, &FeatureSet::all_enabled(), &RentCollector::default(), + &HashMap::new(), &ProgramCacheForTxBatch::default(), ) } @@ -1285,6 +1297,7 @@ mod tests { None, &FeatureSet::default(), &RentCollector::default(), + &HashMap::new(), &loaded_programs, ); @@ -1350,6 +1363,7 @@ mod tests { None, &FeatureSet::default(), &RentCollector::default(), + &HashMap::new(), &loaded_programs, ); @@ -1383,6 +1397,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); @@ -1392,7 +1407,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); @@ -1403,7 +1418,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()); @@ -1420,6 +1435,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(), @@ -1443,12 +1460,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!( @@ -1457,7 +1475,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, @@ -1490,6 +1508,7 @@ mod tests { None, &FeatureSet::default(), &RentCollector::default(), + &program_accounts, &loaded_programs, ); @@ -1516,6 +1535,7 @@ mod tests { None, &FeatureSet::default(), &RentCollector::default(), + &program_accounts, &loaded_programs, ); @@ -1565,6 +1585,7 @@ mod tests { None, &FeatureSet::default(), &RentCollector::default(), + &HashMap::new(), &loaded_programs, ); @@ -1610,6 +1631,7 @@ mod tests { None, &FeatureSet::default(), &RentCollector::default(), + &HashMap::new(), &loaded_programs, ); @@ -1667,6 +1689,7 @@ mod tests { None, &FeatureSet::default(), &RentCollector::default(), + &HashMap::new(), &loaded_programs, ); @@ -1730,6 +1753,7 @@ mod tests { None, &FeatureSet::default(), &RentCollector::default(), + &HashMap::new(), &loaded_programs, ); @@ -1784,6 +1808,7 @@ mod tests { None, &FeatureSet::default(), &RentCollector::default(), + &HashMap::new(), &loaded_programs, ); @@ -1848,6 +1873,7 @@ mod tests { None, &FeatureSet::default(), &RentCollector::default(), + &HashMap::new(), &loaded_programs, ); @@ -1936,6 +1962,7 @@ mod tests { None, &FeatureSet::default(), &RentCollector::default(), + &HashMap::new(), &loaded_programs, ); @@ -2001,6 +2028,7 @@ mod tests { None, &FeatureSet::default(), &RentCollector::default(), + &HashMap::new(), &ProgramCacheForTxBatch::default(), ); @@ -2097,6 +2125,7 @@ mod tests { None, &FeatureSet::default(), &RentCollector::default(), + &HashMap::new(), &loaded_programs, ); @@ -2169,6 +2198,7 @@ mod tests { None, &feature_set, &rent_collector, + &HashMap::new(), &ProgramCacheForTxBatch::default(), ); @@ -2190,6 +2220,7 @@ mod tests { None, &feature_set, &rent_collector, + &HashMap::new(), &ProgramCacheForTxBatch::default(), ); @@ -2340,6 +2371,7 @@ mod tests { None, &FeatureSet::default(), &RentCollector::default(), + &HashMap::new(), &ProgramCacheForTxBatch::default(), ); @@ -2367,6 +2399,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(); @@ -2375,7 +2409,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 @@ -2385,7 +2419,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 { @@ -2435,12 +2469,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, @@ -2471,6 +2507,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 eb28d087ba618e..051399391a632f 100644 --- a/svm/src/transaction_processor.rs +++ b/svm/src/transaction_processor.rs @@ -287,13 +287,12 @@ 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, (&native_loader, 0)); + } let (mut program_cache_for_tx_batch, program_cache_us) = measure_us!({ - let native_loader = native_loader::id(); - for builtin_program in self.builtin_program_ids.read().unwrap().iter() { - program_accounts_map.insert(*builtin_program, (&native_loader, 0)); - } - let program_cache_for_tx_batch = self.replenish_program_cache( callbacks, &program_accounts_map, @@ -324,6 +323,7 @@ impl TransactionBatchProcessor { environment .rent_collector .unwrap_or(&RentCollector::default()), + &program_accounts_map, &program_cache_for_tx_batch, ));