Skip to content

Commit

Permalink
Revert - #879 and #768 (#3511)
Browse files Browse the repository at this point in the history 
* Revert "Cleanup - Removes the owner form the result of `filter_executable_program_accounts()` (#879)"

This reverts commit 5fe30cb.

* Revert "Refactor - Remove `program_accounts_map` from account_loader (#768)"

This reverts commit e7617a1.
  • Loading branch information
@Lichtso
Lichtso authored Nov 7, 2024
1 parent 6f2b6f2 commit 204d282
Show file tree
Hide file tree
Showing 2 changed files with 62 additions and 33 deletions.
42 changes: 32 additions & 10 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 {
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},
Expand All @@ -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
Expand Down Expand Up @@ -162,6 +162,7 @@ pub(crate) fn load_accounts<CB: TransactionProcessingCallback>(
account_overrides: Option<&AccountOverrides>,
feature_set: &FeatureSet,
rent_collector: &RentCollector,
program_accounts: &HashMap<Pubkey, (&Pubkey, u64)>,
loaded_programs: &ProgramCacheForTxBatch,
) -> Vec<TransactionLoadResult> {
txs.iter()
Expand All @@ -179,6 +180,7 @@ pub(crate) fn load_accounts<CB: TransactionProcessingCallback>(
account_overrides,
feature_set,
rent_collector,
program_accounts,
loaded_programs,
)
}
Expand All @@ -195,6 +197,7 @@ fn load_transaction_accounts<CB: TransactionProcessingCallback>(
account_overrides: Option<&AccountOverrides>,
feature_set: &FeatureSet,
rent_collector: &RentCollector,
program_accounts: &HashMap<Pubkey, (&Pubkey, u64)>,
loaded_programs: &ProgramCacheForTxBatch,
) -> Result<LoadedTransaction> {
let mut tx_rent: TransactionRent = 0;
Expand Down Expand Up @@ -240,13 +243,10 @@ fn load_transaction_accounts<CB: TransactionProcessingCallback>(
.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)
Expand Down Expand Up @@ -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<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 @@ -511,6 +517,7 @@ mod tests {
None,
feature_set,
rent_collector,
&HashMap::new(),
&ProgramCacheForTxBatch::default(),
)
}
Expand Down Expand Up @@ -796,6 +803,7 @@ mod tests {
account_overrides,
&FeatureSet::all_enabled(),
&RentCollector::default(),
&HashMap::new(),
&ProgramCacheForTxBatch::default(),
)
}
Expand Down Expand Up @@ -1153,6 +1161,7 @@ mod tests {
None,
&FeatureSet::default(),
&RentCollector::default(),
&HashMap::new(),
&loaded_programs,
);

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

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

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

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

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

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

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

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

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

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

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

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

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

Expand Down
53 changes: 30 additions & 23 deletions svm/src/transaction_processor.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -255,8 +256,9 @@ 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, 0);
program_accounts_map.insert(*builtin_program, (&native_loader, 0));
}

let program_cache_for_tx_batch = Rc::new(RefCell::new(self.replenish_program_cache(
Expand Down Expand Up @@ -291,6 +293,7 @@ impl<FG: ForkGraph> TransactionBatchProcessor<FG> {
environment
.rent_collector
.unwrap_or(&RentCollector::default()),
&program_accounts_map,
&program_cache_for_tx_batch.borrow(),
);
load_time.stop();
Expand Down Expand Up @@ -497,29 +500,30 @@ impl<FG: ForkGraph> TransactionBatchProcessor<FG> {

/// 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<CB: TransactionProcessingCallback>(
fn filter_executable_program_accounts<'a, CB: TransactionProcessingCallback>(
callbacks: &CB,
txs: &[SanitizedTransaction],
validation_results: &[TransactionValidationResult],
program_owners: &[Pubkey],
) -> HashMap<Pubkey, u64> {
let mut result: HashMap<Pubkey, u64> = HashMap::new();
program_owners: &'a [Pubkey],
) -> HashMap<Pubkey, (&'a Pubkey, u64)> {
let mut result: HashMap<Pubkey, (&'a Pubkey, u64)> = HashMap::new();
validation_results.iter().zip(txs).for_each(|etx| {
if let (Ok(_), tx) = etx {
tx.message()
.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));
}
}
}
});
Expand All @@ -531,14 +535,14 @@ impl<FG: ForkGraph> TransactionBatchProcessor<FG> {
fn replenish_program_cache<CB: TransactionProcessingCallback>(
&self,
callback: &CB,
program_accounts_map: &HashMap<Pubkey, u64>,
program_accounts_map: &HashMap<Pubkey, (&Pubkey, u64)>,
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| {
Expand Down Expand Up @@ -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<Pubkey, u64> = HashMap::new();
account_maps.insert(key, 4);
let mut account_maps: HashMap<Pubkey, (&Pubkey, u64)> = HashMap::new();
account_maps.insert(key, (&owner, 4));

batch_processor.replenish_program_cache(&mock_bank, &account_maps, false, true);
}
Expand All @@ -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());
Expand All @@ -1341,8 +1347,8 @@ mod tests {
.unwrap()
.insert(key, account_data);

let mut account_maps: HashMap<Pubkey, u64> = HashMap::new();
account_maps.insert(key, 4);
let mut account_maps: HashMap<Pubkey, (&Pubkey, u64)> = HashMap::new();
account_maps.insert(key, (&owner, 4));
let mut loaded_missing = 0;

for limit_to_load_programs in [false, true] {
Expand Down Expand Up @@ -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]
Expand Down Expand Up @@ -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)
);
}

Expand Down Expand Up @@ -1639,7 +1645,7 @@ mod tests {
programs
.get(&account3_pubkey)
.expect("failed to find the program account"),
&1
&(&program1_pubkey, 1)
);
}

Expand Down Expand Up @@ -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<Pubkey, u64> = programs
let account_maps: HashMap<Pubkey, (&Pubkey, u64)> = programs
.iter()
.enumerate()
.map(|(idx, key)| (*key, idx as u64))
.map(|(idx, key)| (*key, (&LOADER, idx as u64)))
.collect();

for _ in 0..10 {
Expand Down

0 comments on commit 204d282

Please sign in to comment.