Skip to content

Commit

Permalink
Revert "Refactor - Remove program_accounts_map from account_loader (s…
Browse files Browse the repository at this point in the history 
…olana-labs#768)"

This reverts commit e7617a1.
  • Loading branch information
@Lichtso
Lichtso committed Nov 7, 2024
1 parent d411166 commit a0bc125
Show file tree
Hide file tree
Showing 2 changed files with 61 additions and 24 deletions.
75 changes: 56 additions & 19 deletions svm/src/account_loader.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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
Expand Down Expand Up @@ -194,6 +194,7 @@ pub(crate) fn load_accounts<CB: TransactionProcessingCallback>(
account_overrides: Option<&AccountOverrides>,
feature_set: &FeatureSet,
rent_collector: &dyn SVMRentCollector,
program_accounts: &HashMap<Pubkey, (&Pubkey, u64)>,
loaded_programs: &ProgramCacheForTxBatch,
) -> Vec<TransactionLoadResult> {
txs.iter()
Expand All @@ -207,6 +208,7 @@ pub(crate) fn load_accounts<CB: TransactionProcessingCallback>(
account_overrides,
feature_set,
rent_collector,
program_accounts,
loaded_programs,
)
})
Expand All @@ -221,6 +223,7 @@ fn load_transaction<CB: TransactionProcessingCallback>(
account_overrides: Option<&AccountOverrides>,
feature_set: &FeatureSet,
rent_collector: &dyn SVMRentCollector,
program_accounts: &HashMap<Pubkey, (&Pubkey, u64)>,
loaded_programs: &ProgramCacheForTxBatch,
) -> TransactionLoadResult {
match validation_result {
Expand All @@ -235,6 +238,7 @@ fn load_transaction<CB: TransactionProcessingCallback>(
account_overrides,
feature_set,
rent_collector,
program_accounts,
loaded_programs,
);

Expand Down Expand Up @@ -277,6 +281,7 @@ fn load_transaction_accounts<CB: TransactionProcessingCallback>(
account_overrides: Option<&AccountOverrides>,
feature_set: &FeatureSet,
rent_collector: &dyn SVMRentCollector,
program_accounts: &HashMap<Pubkey, (&Pubkey, u64)>,
loaded_programs: &ProgramCacheForTxBatch,
) -> Result<LoadedTransactionAccounts> {
let mut tx_rent: TransactionRent = 0;
Expand Down Expand Up @@ -331,6 +336,7 @@ fn load_transaction_accounts<CB: TransactionProcessingCallback>(
account_overrides,
feature_set,
rent_collector,
program_accounts,
loaded_programs,
)?;
collect_loaded_account(account_key, (loaded_account, account_found))?;
Expand Down Expand Up @@ -416,6 +422,7 @@ fn load_transaction_account<CB: TransactionProcessingCallback>(
account_overrides: Option<&AccountOverrides>,
feature_set: &FeatureSet,
rent_collector: &dyn SVMRentCollector,
program_accounts: &HashMap<Pubkey, (&Pubkey, u64)>,
loaded_programs: &ProgramCacheForTxBatch,
) -> Result<(LoadedTransactionAccount, bool)> {
let mut account_found = true;
Expand Down Expand Up @@ -443,14 +450,11 @@ fn load_transaction_account<CB: TransactionProcessingCallback>(
.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 {
Expand Down Expand Up @@ -500,14 +504,20 @@ fn load_transaction_account<CB: TransactionProcessingCallback>(
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<Pubkey, (&Pubkey, u64)>,
) -> Result<AccountSharedData> {
// 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`.
Expand Down Expand Up @@ -674,6 +684,7 @@ mod tests {
None,
feature_set,
rent_collector,
&HashMap::new(),
&ProgramCacheForTxBatch::default(),
)
}
Expand Down Expand Up @@ -990,6 +1001,7 @@ mod tests {
account_overrides,
&FeatureSet::all_enabled(),
&RentCollector::default(),
&HashMap::new(),
&ProgramCacheForTxBatch::default(),
)
}
Expand Down Expand Up @@ -1285,6 +1297,7 @@ mod tests {
None,
&FeatureSet::default(),
&RentCollector::default(),
&HashMap::new(),
&loaded_programs,
);

Expand Down Expand Up @@ -1350,6 +1363,7 @@ mod tests {
None,
&FeatureSet::default(),
&RentCollector::default(),
&HashMap::new(),
&loaded_programs,
);

Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand All @@ -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());
Expand All @@ -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(),
Expand All @@ -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!(
Expand All @@ -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,
Expand Down Expand Up @@ -1490,6 +1508,7 @@ mod tests {
None,
&FeatureSet::default(),
&RentCollector::default(),
&program_accounts,
&loaded_programs,
);

Expand All @@ -1516,6 +1535,7 @@ mod tests {
None,
&FeatureSet::default(),
&RentCollector::default(),
&program_accounts,
&loaded_programs,
);

Expand Down Expand Up @@ -1565,6 +1585,7 @@ mod tests {
None,
&FeatureSet::default(),
&RentCollector::default(),
&HashMap::new(),
&loaded_programs,
);

Expand Down Expand Up @@ -1610,6 +1631,7 @@ mod tests {
None,
&FeatureSet::default(),
&RentCollector::default(),
&HashMap::new(),
&loaded_programs,
);

Expand Down Expand Up @@ -1667,6 +1689,7 @@ mod tests {
None,
&FeatureSet::default(),
&RentCollector::default(),
&HashMap::new(),
&loaded_programs,
);

Expand Down Expand Up @@ -1730,6 +1753,7 @@ mod tests {
None,
&FeatureSet::default(),
&RentCollector::default(),
&HashMap::new(),
&loaded_programs,
);

Expand Down Expand Up @@ -1784,6 +1808,7 @@ mod tests {
None,
&FeatureSet::default(),
&RentCollector::default(),
&HashMap::new(),
&loaded_programs,
);

Expand Down Expand Up @@ -1848,6 +1873,7 @@ mod tests {
None,
&FeatureSet::default(),
&RentCollector::default(),
&HashMap::new(),
&loaded_programs,
);

Expand Down Expand Up @@ -1936,6 +1962,7 @@ mod tests {
None,
&FeatureSet::default(),
&RentCollector::default(),
&HashMap::new(),
&loaded_programs,
);

Expand Down Expand Up @@ -2001,6 +2028,7 @@ mod tests {
None,
&FeatureSet::default(),
&RentCollector::default(),
&HashMap::new(),
&ProgramCacheForTxBatch::default(),
);

Expand Down Expand Up @@ -2097,6 +2125,7 @@ mod tests {
None,
&FeatureSet::default(),
&RentCollector::default(),
&HashMap::new(),
&loaded_programs,
);

Expand Down Expand Up @@ -2169,6 +2198,7 @@ mod tests {
None,
&feature_set,
&rent_collector,
&HashMap::new(),
&ProgramCacheForTxBatch::default(),
);

Expand All @@ -2190,6 +2220,7 @@ mod tests {
None,
&feature_set,
&rent_collector,
&HashMap::new(),
&ProgramCacheForTxBatch::default(),
);

Expand Down Expand Up @@ -2340,6 +2371,7 @@ mod tests {
None,
&FeatureSet::default(),
&RentCollector::default(),
&HashMap::new(),
&ProgramCacheForTxBatch::default(),
);

Expand Down Expand Up @@ -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();
Expand All @@ -2375,7 +2409,7 @@ mod tests {

let program2_size = std::mem::size_of::<UpgradeableLoaderState>() 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
Expand All @@ -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 {
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -2471,6 +2507,7 @@ mod tests {
None,
&FeatureSet::default(),
&RentCollector::default(),
&program_accounts,
&program_cache,
)
.unwrap();
Expand Down
10 changes: 5 additions & 5 deletions svm/src/transaction_processor.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -287,13 +287,12 @@ impl<FG: ForkGraph> TransactionBatchProcessor<FG> {
&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,
Expand Down Expand Up @@ -324,6 +323,7 @@ impl<FG: ForkGraph> TransactionBatchProcessor<FG> {
environment
.rent_collector
.unwrap_or(&RentCollector::default()),
&program_accounts_map,
&program_cache_for_tx_batch,
));

Expand Down

0 comments on commit a0bc125

Please sign in to comment.