Skip to content

Commit

Permalink
v2.1: Revert - #879 and #768 (backport of #3521) (#3544)
Browse files Browse the repository at this point in the history 
Revert - #879 and #768 (#3521)

* 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.

(cherry picked from commit 57bdb8e)

Co-authored-by: Alexander Meißner <AlexanderMeissner@gmx.net>
  • Loading branch information
@mergify @Lichtso
mergify[bot] and Lichtso authored Nov 8, 2024
1 parent ee92535 commit b6fc167
Show file tree
Hide file tree
Showing 3 changed files with 96 additions and 48 deletions.
77 changes: 58 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 @@ -268,6 +272,7 @@ struct LoadedTransactionAccounts {
pub loaded_accounts_data_size: u32,
}

#[allow(clippy::too_many_arguments)]
fn load_transaction_accounts<CB: TransactionProcessingCallback>(
callbacks: &CB,
message: &impl SVMMessage,
Expand All @@ -277,6 +282,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 +337,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 @@ -403,6 +410,7 @@ fn load_transaction_accounts<CB: TransactionProcessingCallback>(
})
}

#[allow(clippy::too_many_arguments)]
fn load_transaction_account<CB: TransactionProcessingCallback>(
callbacks: &CB,
message: &impl SVMMessage,
Expand All @@ -412,6 +420,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 @@ -439,14 +448,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 @@ -496,14 +502,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 @@ -670,6 +682,7 @@ mod tests {
None,
feature_set,
rent_collector,
&HashMap::new(),
&ProgramCacheForTxBatch::default(),
)
}
Expand Down Expand Up @@ -978,6 +991,7 @@ mod tests {
account_overrides,
&FeatureSet::all_enabled(),
&RentCollector::default(),
&HashMap::new(),
&ProgramCacheForTxBatch::default(),
)
}
Expand Down Expand Up @@ -1273,6 +1287,7 @@ mod tests {
None,
&FeatureSet::default(),
&RentCollector::default(),
&HashMap::new(),
&loaded_programs,
);

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Expand Down Expand Up @@ -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();
Expand All @@ -2363,7 +2399,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 @@ -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 {
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -2459,6 +2497,7 @@ mod tests {
None,
&FeatureSet::default(),
&RentCollector::default(),
&program_accounts,
&program_cache,
)
.unwrap();
Expand Down
Loading

0 comments on commit b6fc167

Please sign in to comment.