Skip to content

Commit

Permalink
Simplify program loading error handling
Browse files Browse the repository at this point in the history
  • Loading branch information
jstarry committed Aug 15, 2024
1 parent 92acf94 commit b51ce5e
Show file tree
Hide file tree
Showing 4 changed files with 17 additions and 24 deletions.
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

0 comments on commit b51ce5e

Please sign in to comment.