diff --git a/runtime/src/bank/tests.rs b/runtime/src/bank/tests.rs index 2e7ea11b745060..13fb573bc8afad 100644 --- a/runtime/src/bank/tests.rs +++ b/runtime/src/bank/tests.rs @@ -7132,7 +7132,10 @@ fn test_bpf_loader_upgradeable_deploy_with_max_len() { ); assert_eq!( bank.process_transaction(&transaction), - Err(TransactionError::InvalidProgramForExecution), + Err(TransactionError::InstructionError( + 0, + InstructionError::InvalidAccountData + )), ); { let program_cache = bank.transaction_processor.program_cache.read().unwrap(); @@ -7153,7 +7156,10 @@ fn test_bpf_loader_upgradeable_deploy_with_max_len() { let transaction = Transaction::new(&[&binding], message, bank.last_blockhash()); assert_eq!( bank.process_transaction(&transaction), - Err(TransactionError::InvalidProgramForExecution), + Err(TransactionError::InstructionError( + 0, + InstructionError::InvalidAccountData, + )), ); { let program_cache = bank.transaction_processor.program_cache.read().unwrap(); diff --git a/svm/src/account_loader.rs b/svm/src/account_loader.rs index e7a712cac23747..f1c790972c1d57 100644 --- a/svm/src/account_loader.rs +++ b/svm/src/account_loader.rs @@ -421,12 +421,6 @@ pub(crate) fn load_accounts( .and_then(|_| program_cache.find(account_key)) .filter(|program| !program.is_tombstone()) { - // XXX i might be able to skip this inspect, asking brooks - // XXX also i might need to check effective slot and not just tombstone... asking pankaj - if let Some(account) = callbacks.get_account_shared_data(account_key) { - callbacks.inspect_account(account_key, AccountState::Alive(&account), false); - } - loaded_accounts_map.insert_cached_program(*account_key, program); } else if let Some(account) = callbacks.get_account_shared_data(account_key) { callbacks.inspect_account(account_key, AccountState::Alive(&account), is_writable); @@ -445,18 +439,15 @@ pub(crate) fn load_accounts( continue; }; - // likewise, if the program is not executable, transaction loading will fail - if !loaded_program.executable_in_batch { - continue; - } - // if this is a loader, nothing further needs to be done if loaded_program.valid_loader { continue; } - // now we have an executable program that isnt a loader - // we verify its owner is a loader, and add that loader to the accounts map if we dont already have it + // NOTE pending a feature gate, we should `continue` if the program is not executable in batch + // however, for our non-executable escape hatch in transaction loading, we must get loaders for invalid programs + + // verify the program owner is a loader and add that loader to the accounts map if we dont already have it let owner_id = loaded_program.account.owner(); let owner_is_loader = if let Some(loaded_owner) = loaded_accounts_map.get_loaded_account(owner_id) { @@ -572,7 +563,7 @@ fn load_transaction_accounts( required_programs.insert(program_id, program_index); account_indices.insert(0, program_index as IndexOfAccount); - // to preserve existing behavior, we must count loader size, except NativeLoader, once per transaction + // NOTE to preserve existing behavior, we must count loader size, except NativeLoader, once per transaction // this is in addition to counting it if used as an instruction account, pending removal by feature gate // we re-validate here the program is owned by a loader due to interactions with the program cache // which may cause us to execute programs that are not executable_in_batch @@ -628,7 +619,7 @@ fn load_transaction_accounts( } if !executable_in_batch { - // in the old loader, executable was not checked for cached, read-only, non-instruction programs + // NOTE in the old loader, executable was not checked for cached, read-only, non-instruction programs // we preserve this behavior here and must *substitute* the cached program // pending a feature gate to remove this behavior // we need to check cache here, even if we allow non-executable programs in `load_accounts()` diff --git a/svm/tests/integration_test.rs b/svm/tests/integration_test.rs index 2c8c2cef0bb204..bf1c01f6a85547 100644 --- a/svm/tests/integration_test.rs +++ b/svm/tests/integration_test.rs @@ -2216,26 +2216,18 @@ fn svm_inspect_account() { ); } - // The transfer program account is also loaded during transaction processing, however the - // account state passed to `inspect_account()` is *not* the same as what is held by - // MockBankCallback::account_shared_data. So we check the transfer program differently. - // - // First ensure we have the correct number of inspected accounts, correctly counting the - // transfer program. + // The transfer program account is retreived from the program cache, which does not + // inspect accounts, because they are necessarily read-only. Verify it has not made + // its way into the inspected accounts list. let num_expected_inspected_accounts: usize = expected_inspected_accounts.values().map(Vec::len).sum(); let num_actual_inspected_accounts: usize = actual_inspected_accounts.values().map(Vec::len).sum(); + assert_eq!( - num_expected_inspected_accounts + 2, + num_expected_inspected_accounts, num_actual_inspected_accounts, ); - // And second, ensure the inspected transfer program accounts are alive and not writable. - let actual_transfer_program_accounts = - actual_inspected_accounts.get(&transfer_program).unwrap(); - for actual_transfer_program_account in actual_transfer_program_accounts { - assert!(actual_transfer_program_account.0.is_some()); - assert!(!actual_transfer_program_account.1); - } + assert!(actual_inspected_accounts.contains_key(&transfer_program)); }