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.

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

Simplify program loading error handling #2604

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions programs/sbf/tests/programs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1656,7 +1656,7 @@ fn test_program_sbf_invoke_stable_genesis_and_bank() {
let result = bank_client.send_and_confirm_instruction(&mint_keypair, instruction.clone());
assert_eq!(
result.unwrap_err().unwrap(),
TransactionError::ProgramAccountNotFound
TransactionError::InvalidProgramForExecution
);

load_upgradeable_program(
Expand Down Expand Up @@ -1882,7 +1882,7 @@ fn test_program_sbf_invoke_in_same_tx_as_deployment() {
let results = execute_transactions(&bank, vec![tx]);
assert_eq!(
results[0].as_ref().unwrap_err(),
&TransactionError::ProgramAccountNotFound,
&TransactionError::InvalidProgramForExecution,
);
} else {
let (result, _, _) = process_transaction_and_record_inner(&bank, tx);
Expand Down
2 changes: 1 addition & 1 deletion runtime/src/bank/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6921,7 +6921,7 @@ fn test_bpf_loader_upgradeable_deploy_with_max_len() {
);
assert_eq!(
bank.process_transaction(&transaction),
Err(TransactionError::ProgramAccountNotFound),
Err(TransactionError::InvalidProgramForExecution),
);
{
// Make sure it is not in the cache because the account owner is not a loader
Expand Down
33 changes: 13 additions & 20 deletions svm/src/account_loader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,6 @@ 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 rent_debits = RentDebits::default();
let mut accumulated_accounts_data_size: u32 = 0;

Expand All @@ -287,7 +286,7 @@ fn load_transaction_accounts<CB: TransactionProcessingCallback>(
.unique()
.collect::<Vec<&u8>>();

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 @@ -305,19 +304,18 @@ 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))?;
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(
callbacks,
message,
account_key,
Expand All @@ -328,7 +326,7 @@ fn load_transaction_accounts<CB: TransactionProcessingCallback>(
rent_collector,
loaded_programs,
)?;
collect_loaded_account(account_key, (loaded_account, account_found))?;
collect_loaded_account(account_key, loaded_account)?;
}

let builtins_start_index = accounts.len();
Expand All @@ -345,12 +343,6 @@ fn load_transaction_accounts<CB: TransactionProcessingCallback>(
return Ok(account_indices);
}

let account_found = accounts_found.get(program_index).unwrap_or(&true);
if !account_found {
error_metrics.account_not_found += 1;
return Err(TransactionError::ProgramAccountNotFound);
}

if !program_account.executable() {
error_metrics.invalid_program_for_execution += 1;
return Err(TransactionError::InvalidProgramForExecution);
Expand Down Expand Up @@ -408,8 +400,7 @@ fn load_transaction_account<CB: TransactionProcessingCallback>(
feature_set: &FeatureSet,
rent_collector: &RentCollector,
loaded_programs: &ProgramCacheForTxBatch,
) -> Result<(LoadedTransactionAccount, bool)> {
let mut account_found = true;
) -> Result<LoadedTransactionAccount> {
let is_instruction_account = u8::try_from(account_index)
.map(|i| instruction_accounts.contains(&&i))
.unwrap_or(false);
Expand Down Expand Up @@ -466,7 +457,6 @@ fn load_transaction_account<CB: TransactionProcessingCallback>(
}
})
.unwrap_or_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 @@ -480,7 +470,7 @@ fn load_transaction_account<CB: TransactionProcessingCallback>(
})
};

Ok((loaded_account, account_found))
Ok(loaded_account)
}

fn account_shared_data_from_program(loaded_program: &ProgramCacheEntry) -> AccountSharedData {
Expand Down Expand Up @@ -705,12 +695,12 @@ mod tests {

let load_results = load_accounts_aux_test(tx, &accounts, &mut error_metrics);

assert_eq!(error_metrics.account_not_found, 1);
assert_eq!(error_metrics.invalid_program_for_execution, 1);
assert_eq!(load_results.len(), 1);
assert!(matches!(
load_results[0],
TransactionLoadResult::FeesOnly(FeesOnlyTransaction {
load_error: TransactionError::ProgramAccountNotFound,
load_error: TransactionError::InvalidProgramForExecution,
..
}),
));
Expand Down Expand Up @@ -935,7 +925,7 @@ mod tests {
assert!(matches!(
load_results[0],
TransactionLoadResult::FeesOnly(FeesOnlyTransaction {
load_error: TransactionError::ProgramAccountNotFound,
load_error: TransactionError::InvalidProgramForExecution,
..
}),
));
Expand Down Expand Up @@ -1381,7 +1371,10 @@ mod tests {
&loaded_programs,
);

assert_eq!(result.err(), Some(TransactionError::ProgramAccountNotFound));
assert_eq!(
result.err(),
Some(TransactionError::InvalidProgramForExecution)
);
}

#[test]
Expand Down
2 changes: 1 addition & 1 deletion svm/src/transaction_processor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1383,7 +1383,7 @@ mod tests {
let validation_results = vec![
Ok(ValidatedTransactionDetails::default()),
Ok(ValidatedTransactionDetails::default()),
Err(TransactionError::ProgramAccountNotFound),
Err(TransactionError::InvalidProgramForExecution),
];
let owners = vec![owner1, owner2];

Expand Down