Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

svm: skip appending loaders to loaded accounts #3631

Merged
merged 5 commits into from
Nov 27, 2024
Merged

svm: skip appending loaders to loaded accounts #3631

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
46493f8
svm: skip appending loaders to loaded accounts
2501babe Nov 14, 2024
017abac
fix tests
2501babe Nov 14, 2024
88c5c63
rebase on simd83 loader
2501babe Nov 20, 2024
d369ad5
stop scanning the accounts vec entirely
2501babe Nov 20, 2024
67b4250
revert account keys length logic
2501babe Nov 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
130 changes: 72 additions & 58 deletions svm/src/account_loader.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,12 @@ use {
transaction_execution_result::ExecutedTransaction,
transaction_processing_callback::{AccountState, TransactionProcessingCallback},
},
ahash::AHashMap,
ahash::{AHashMap, AHashSet},
solana_compute_budget::compute_budget_limits::ComputeBudgetLimits,
solana_feature_set::{self as feature_set, FeatureSet},
solana_program_runtime::loaded_programs::ProgramCacheForTxBatch,
solana_sdk::{
account::{Account, AccountSharedData, ReadableAccount, WritableAccount},
account::{Account, AccountSharedData, ReadableAccount, WritableAccount, PROGRAM_OWNERS},
fee::FeeDetails,
native_loader,
nonce::State as NonceState,
Expand Down Expand Up @@ -431,11 +431,11 @@ fn load_transaction_accounts<CB: TransactionProcessingCallback>(
let mut tx_rent: TransactionRent = 0;
let account_keys = message.account_keys();
let mut accounts = Vec::with_capacity(account_keys.len());
let mut accounts_found = Vec::with_capacity(account_keys.len());
let mut validated_loaders = AHashSet::with_capacity(PROGRAM_OWNERS.len());
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

optimization; PROGRAM_OWNERS is a very small set. I imagine we'd actually spend more time hashing here than we would comparing pubkeys?

We can measure but, we can just directly search PROGRAM_OWNERS, and store the some small stack-space to see if the programs have been seen yet: [false; PROGRAM_OWNERS.len()]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to keep the hashset. the capacity of 4 isnt "there can only be 4 loaders" but "we will typically see 2 or maybe 3 loaders if no one is being annoying on purpose, and if they force us to see 5 loaders they waste like a millisecond of compute so have fun i guess"

the loader validation code is extremely delicate--not because of this pr, but before and after this pr--and cannot safely be changed. this is because:

  • the consensus definition of a loader is any executable account owned by NativeLoader, including stake/vote/system. in practice, with current features, this shouldnt actually matter, because these pseudo-loaders never mark accounts they own as executable, which makes the program fail validation before we validate the loader. however if we want to uphold the "svm is 100% simd83-compliant now, and simd83 means batching doesnt affect results" ideal then technically you could exploit the program cache executable bypass to get a pseudo-loader past validation to execution (although it is only a violation of our ideals, since you cant actually do it without simd83 lock relaxation)
  • when disable_account_loader_special_case activates this quirk goes away and we can assume "loader" means "contained in PROGRAM_OWNERS
  • ...and then when remove_accounts_executable_flag_checks activates this assumption is ruined because system/vote/stake are valid loaders and accounts they own are valid programs (according to account loading but not according to program cache) and these kinds of transactions will successfully load and move to execution
  • finally when enable_transaction_loading_failure_fees activates we dont have to care about any of this anymore and can assume again that "loader" means "contained in PROGRAM_OWNERS" as an implementation detail, rather than a consensus constraint, because it doesnt matter how a pseudo-loader transaction fails anymore

and if we changed the technical definition of a loader we would have to introduce a fourth feature gate or backport and cut a new 2.1 declaring previous 2.1 versions invalid

thankfully this is all getting deleted by simd186 anyway

let mut rent_debits = RentDebits::default();
let mut accumulated_accounts_data_size: u32 = 0;

let mut collect_loaded_account = |key, (loaded_account, found)| -> Result<()> {
let mut collect_loaded_account = |key, loaded_account| -> Result<()> {
let LoadedTransactionAccount {
account,
loaded_size,
Expand All @@ -453,47 +453,44 @@ fn load_transaction_accounts<CB: TransactionProcessingCallback>(
rent_debits.insert(key, rent_collected, account.lamports());

accounts.push((*key, account));
accounts_found.push(found);
Ok(())
};

// Since the fee payer is always the first account, collect it first. Note
// that account overrides are already applied during fee payer validation so
// it's fine to use the fee payer directly here rather than checking account
// overrides again.
collect_loaded_account(message.fee_payer(), (loaded_fee_payer_account, true))?;
// Since the fee payer is always the first account, collect it first.
// We can use it directly because it was already loaded during validation.
collect_loaded_account(message.fee_payer(), loaded_fee_payer_account)?;

// Attempt to load and collect remaining non-fee payer accounts
for (account_index, account_key) in account_keys.iter().enumerate().skip(1) {
let (loaded_account, account_found) = load_transaction_account(
let loaded_account = load_transaction_account(
account_loader,
message,
account_key,
account_index,
rent_collector,
)?;
collect_loaded_account(account_key, (loaded_account, account_found))?;
);
collect_loaded_account(account_key, loaded_account)?;
}

let builtins_start_index = accounts.len();
let program_indices = message
.instructions_iter()
.map(|instruction| {
.program_instructions_iter()
.map(|(program_id, instruction)| {
let mut account_indices = Vec::with_capacity(2);
let program_index = instruction.program_id_index as usize;
// This command may never return error, because the transaction is sanitized
let (program_id, program_account) = accounts
.get(program_index)
.ok_or(TransactionError::ProgramAccountNotFound)?;
if native_loader::check_id(program_id) {
return Ok(account_indices);
}

let account_found = accounts_found.get(program_index).unwrap_or(&true);
if !account_found {
let program_index = instruction.program_id_index as usize;
let program_usage_pattern = AccountUsagePattern::new(message, program_index);

let Some(LoadedTransactionAccount {
account: program_account,
..
}) = account_loader.load_account(program_id, program_usage_pattern)
else {
error_metrics.account_not_found += 1;
return Err(TransactionError::ProgramAccountNotFound);
}
};

if !account_loader
.feature_set
Expand All @@ -504,21 +501,40 @@ fn load_transaction_accounts<CB: TransactionProcessingCallback>(
return Err(TransactionError::InvalidProgramForExecution);
}
account_indices.insert(0, program_index as IndexOfAccount);

let owner_id = program_account.owner();
if native_loader::check_id(owner_id) {
return Ok(account_indices);
}
if !accounts
.get(builtins_start_index..)
.ok_or(TransactionError::ProgramAccountNotFound)?
.iter()
.any(|(key, _)| key == owner_id)
{
// NOTE this will be changed to a `load_account()` call in a PR immediately following this one
// we want to stop pushing loaders on the accounts vec, but tests need to change if we do that
// and this PR is complex enough that we want to code review any new behaviors separately
if let Some(owner_account) =
account_loader.callbacks.get_account_shared_data(owner_id)

if !validated_loaders.contains(owner_id) {
// NOTE we load the program owner as `ReadOnlyInstruction` to bypass the program cache
// since the program cache would incorrectly mark a user-created native-owned account as executable
// this preserves consensus until `disable_account_loader_special_case` is active, after which it doesnt matter
//
// there are a panopoly of fetaure gate activations that affect this code, in generally benign ways:
// * `remove_accounts_executable_flag_checks`: incorrect executable flag from program cache no longer matters
// we should still avoid program cache because it changes transaction size
// albeit in a consensus-safe manner because it would result from feature activation
// * `disable_account_loader_special_case`: program cache codepath goes away entirely
// at this point the instruction vs invisible distinction ceases to affect loading
// keeping the distinction may be useful for SIMD-163 (cpi caller restriction), but maybe not
// * `enable_transaction_loading_failure_fees`: loading failures behave the same as execution failures
// at this point we can restrict valid loaders to those contained in `PROGRAM_OWNERS`
// since any other pseudo-loader owner is destined to fail at execution
// * SIMD-186: explicitly defines a sensible transaction data size algorithm
// at this point we stop counting loaders toward transaction data size entirely
//
// when _all three_ of `remove_accounts_executable_flag_checks`, `enable_transaction_loading_failure_fees`,
// and SIMD-186 are active, we do not need to load loaders at all to comply with consensus rules
// we may verify program ids are owned by `PROGRAM_OWNERS` purely as an optimization
// this could even be done before loading the rest of the accounts for a transaction
if let Some(LoadedTransactionAccount {
account: owner_account,
loaded_size: owner_size,
..
}) =
account_loader.load_account(owner_id, AccountUsagePattern::ReadOnlyInstruction)
{
if !native_loader::check_id(owner_account.owner())
|| (!account_loader
Expand All @@ -531,11 +547,11 @@ fn load_transaction_accounts<CB: TransactionProcessingCallback>(
}
accumulate_and_check_loaded_account_data_size(
&mut accumulated_accounts_data_size,
owner_account.data().len(),
owner_size,
compute_budget_limits.loaded_accounts_bytes,
error_metrics,
)?;
accounts.push((*owner_id, owner_account));
validated_loaders.insert(*owner_id);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

github won't let me comment further down - curious if we should add a (debug) assert to make sure that accounts length matches the message's account_keys.len() - we have a few things that now rely on that assumption, which was not true in the past. Slightly concerned maybe someone changes it in future and breaks stuff and we commit some extra appended accounts.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm we could just keep the old way that clamps it to the length of account keys if thats a concern, it doesnt really cost us anything

} else {
error_metrics.account_not_found += 1;
return Err(TransactionError::ProgramAccountNotFound);
Expand All @@ -560,8 +576,7 @@ fn load_transaction_account<CB: TransactionProcessingCallback>(
account_key: &Pubkey,
account_index: usize,
rent_collector: &dyn SVMRentCollector,
) -> Result<(LoadedTransactionAccount, bool)> {
let mut account_found = true;
) -> LoadedTransactionAccount {
let usage_pattern = AccountUsagePattern::new(message, account_index);

let loaded_account = if solana_sdk::sysvar::instructions::check_id(account_key) {
Expand All @@ -588,7 +603,6 @@ fn load_transaction_account<CB: TransactionProcessingCallback>(

loaded_account
} else {
account_found = false;
let mut default_account = AccountSharedData::default();
// All new accounts must be rent-exempt (enforced in Bank::execute_loaded_transaction).
// Currently, rent collection sets rent_epoch to u64::MAX, but initializing the account
Expand All @@ -601,7 +615,7 @@ fn load_transaction_account<CB: TransactionProcessingCallback>(
}
};

Ok((loaded_account, account_found))
loaded_account
}

fn account_shared_data_from_program(
Expand Down Expand Up @@ -1072,7 +1086,7 @@ mod tests {
assert_eq!(error_metrics.account_not_found, 0);
match &loaded_accounts {
TransactionLoadResult::Loaded(loaded_transaction) => {
assert_eq!(loaded_transaction.accounts.len(), 4);
assert_eq!(loaded_transaction.accounts.len(), 3);
assert_eq!(loaded_transaction.accounts[0].1, accounts[0].1);
assert_eq!(loaded_transaction.program_indices.len(), 2);
assert_eq!(loaded_transaction.program_indices[0], &[1]);
Expand Down Expand Up @@ -1580,7 +1594,6 @@ mod tests {
accounts: vec![
(account_keypair.pubkey(), account_data.clone()),
(program_keypair.pubkey(), cached_program),
(loader_v2, loader_data),
],
program_indices: vec![vec![1]],
rent: 0,
Expand Down Expand Up @@ -1750,6 +1763,7 @@ mod tests {
let mut mock_bank = TestCallbacks::default();
let mut account_data = AccountSharedData::default();
account_data.set_owner(native_loader::id());
account_data.set_lamports(1);
account_data.set_executable(true);
mock_bank.accounts_map.insert(key1.pubkey(), account_data);

Expand Down Expand Up @@ -1863,6 +1877,7 @@ mod tests {
let sanitized_message = new_unchecked_sanitized_message(message);
let mut mock_bank = TestCallbacks::default();
let mut account_data = AccountSharedData::default();
account_data.set_lamports(1);
account_data.set_executable(true);
account_data.set_owner(key3.pubkey());
mock_bank.accounts_map.insert(key1.pubkey(), account_data);
Expand Down Expand Up @@ -1917,6 +1932,7 @@ mod tests {
let sanitized_message = new_unchecked_sanitized_message(message);
let mut mock_bank = TestCallbacks::default();
let mut account_data = AccountSharedData::default();
account_data.set_lamports(1);
account_data.set_executable(true);
account_data.set_owner(key3.pubkey());
mock_bank.accounts_map.insert(key1.pubkey(), account_data);
Expand All @@ -1928,6 +1944,7 @@ mod tests {
.insert(key2.pubkey(), fee_payer_account.clone());

let mut account_data = AccountSharedData::default();
account_data.set_lamports(1);
account_data.set_executable(true);
account_data.set_owner(native_loader::id());
mock_bank.accounts_map.insert(key3.pubkey(), account_data);
Expand Down Expand Up @@ -1961,10 +1978,6 @@ mod tests {
key1.pubkey(),
mock_bank.accounts_map[&key1.pubkey()].clone()
),
(
key3.pubkey(),
mock_bank.accounts_map[&key3.pubkey()].clone()
),
],
program_indices: vec![vec![1]],
rent: 0,
Expand Down Expand Up @@ -2002,6 +2015,7 @@ mod tests {
let sanitized_message = new_unchecked_sanitized_message(message);
let mut mock_bank = TestCallbacks::default();
let mut account_data = AccountSharedData::default();
account_data.set_lamports(1);
account_data.set_executable(true);
account_data.set_owner(key3.pubkey());
mock_bank.accounts_map.insert(key1.pubkey(), account_data);
Expand All @@ -2013,6 +2027,7 @@ mod tests {
.insert(key2.pubkey(), fee_payer_account.clone());

let mut account_data = AccountSharedData::default();
account_data.set_lamports(1);
account_data.set_executable(true);
account_data.set_owner(native_loader::id());
mock_bank.accounts_map.insert(key3.pubkey(), account_data);
Expand Down Expand Up @@ -2049,10 +2064,6 @@ mod tests {
mock_bank.accounts_map[&key1.pubkey()].clone()
),
(key4.pubkey(), account_data),
(
key3.pubkey(),
mock_bank.accounts_map[&key3.pubkey()].clone()
),
],
program_indices: vec![vec![1], vec![1]],
rent: 0,
Expand All @@ -2070,6 +2081,7 @@ mod tests {
let last_block_hash = Hash::new_unique();

let mut system_data = AccountSharedData::default();
system_data.set_lamports(1);
system_data.set_executable(true);
system_data.set_owner(native_loader::id());
bank.accounts_map
Expand Down Expand Up @@ -2153,6 +2165,7 @@ mod tests {
let sanitized_message = new_unchecked_sanitized_message(message);
let mut mock_bank = TestCallbacks::default();
let mut account_data = AccountSharedData::default();
account_data.set_lamports(1);
account_data.set_executable(true);
account_data.set_owner(key3.pubkey());
mock_bank.accounts_map.insert(key1.pubkey(), account_data);
Expand All @@ -2164,6 +2177,7 @@ mod tests {
.insert(key2.pubkey(), fee_payer_account.clone());

let mut account_data = AccountSharedData::default();
account_data.set_lamports(1);
account_data.set_executable(true);
account_data.set_owner(native_loader::id());
mock_bank.accounts_map.insert(key3.pubkey(), account_data);
Expand Down Expand Up @@ -2211,10 +2225,6 @@ mod tests {
mock_bank.accounts_map[&key1.pubkey()].clone()
),
(key4.pubkey(), account_data),
(
key3.pubkey(),
mock_bank.accounts_map[&key3.pubkey()].clone()
),
],
program_indices: vec![vec![1], vec![1]],
fee_details: FeeDetails::default(),
Expand Down Expand Up @@ -2263,7 +2273,7 @@ mod tests {
assert!(matches!(
load_result,
TransactionLoadResult::FeesOnly(FeesOnlyTransaction {
load_error: TransactionError::InvalidProgramForExecution,
load_error: TransactionError::ProgramAccountNotFound,
..
}),
));
Expand Down Expand Up @@ -2379,7 +2389,7 @@ mod tests {
let mut account3 = AccountSharedData::default();
account3.set_lamports(4_000_000_000);
account3.set_executable(true);
account3.set_owner(native_loader::id());
account3.set_owner(bpf_loader::id());
mock_bank.accounts_map.insert(address3, account3.clone());
let mut account_loader = (&mock_bank).into();

Expand Down Expand Up @@ -2439,7 +2449,11 @@ mod tests {
// *not* key0, since it is loaded during fee payer validation
(address1, vec![(Some(account1), true)]),
(address2, vec![(None, true)]),
(address3, vec![(Some(account3), false)]),
(
address3,
vec![(Some(account3.clone()), false), (Some(account3), false)],
),
(bpf_loader::id(), vec![(None, false)]),
];
expected_inspected_accounts.sort_unstable_by(|a, b| a.0.cmp(&b.0));

Expand Down
Loading