diff --git a/Cargo.lock b/Cargo.lock index 90d37815cebba9..629d897225c250 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7682,6 +7682,7 @@ dependencies = [ name = "solana-svm" version = "2.1.0" dependencies = [ + "assert_matches", "bincode", "itertools 0.12.1", "lazy_static", diff --git a/ledger/src/blockstore_processor.rs b/ledger/src/blockstore_processor.rs index d34543db73993c..304cc549e57f5b 100644 --- a/ledger/src/blockstore_processor.rs +++ b/ledger/src/blockstore_processor.rs @@ -3801,13 +3801,14 @@ pub mod tests { #[test] fn test_update_transaction_statuses() { - // Make sure instruction errors still update the signature cache let GenesisConfigInfo { genesis_config, mint_keypair, .. } = create_genesis_config(11_000); let (bank, _bank_forks) = Bank::new_with_bank_forks_for_tests(&genesis_config); + + // Make sure instruction errors still update the signature cache let pubkey = solana_sdk::pubkey::new_rand(); bank.transfer(1_000, &mint_keypair, &pubkey).unwrap(); assert_eq!(bank.transaction_count(), 1); @@ -3824,6 +3825,29 @@ pub mod tests { Err(TransactionError::AlreadyProcessed) ); + // Make sure fees-only transactions still update the signature cache + let missing_program_id = Pubkey::new_unique(); + let tx = Transaction::new_signed_with_payer( + &[Instruction::new_with_bincode( + missing_program_id, + &10, + Vec::new(), + )], + Some(&mint_keypair.pubkey()), + &[&mint_keypair], + bank.last_blockhash(), + ); + // First process attempt will fail but still update status cache + assert_eq!( + bank.process_transaction(&tx), + Err(TransactionError::ProgramAccountNotFound) + ); + // Second attempt will be rejected since tx was already in status cache + assert_eq!( + bank.process_transaction(&tx), + Err(TransactionError::AlreadyProcessed) + ); + // Make sure other errors don't update the signature cache let tx = system_transaction::transfer(&mint_keypair, &pubkey, 1000, Hash::default()); let signature = tx.signatures[0]; diff --git a/programs/sbf/tests/programs.rs b/programs/sbf/tests/programs.rs index b36f4d88bda09b..1b44bacfb55ffa 100644 --- a/programs/sbf/tests/programs.rs +++ b/programs/sbf/tests/programs.rs @@ -62,7 +62,7 @@ use { transaction::{SanitizedTransaction, Transaction, TransactionError, VersionedTransaction}, }, solana_svm::{ - transaction_commit_result::CommittedTransaction, + transaction_commit_result::{CommittedTransaction, TransactionCommitResult}, transaction_execution_result::InnerInstruction, transaction_processor::ExecutionRecordingConfig, }, @@ -92,6 +92,20 @@ fn process_transaction_and_record_inner( Vec>, Vec, ) { + let commit_result = load_execute_and_commit_transaction(bank, tx); + let CommittedTransaction { + inner_instructions, + log_messages, + status, + .. + } = commit_result.unwrap(); + let inner_instructions = inner_instructions.expect("cpi recording should be enabled"); + let log_messages = log_messages.expect("log recording should be enabled"); + (status, inner_instructions, log_messages) +} + +#[cfg(feature = "sbf_rust")] +fn load_execute_and_commit_transaction(bank: &Bank, tx: Transaction) -> TransactionCommitResult { let txs = vec![tx]; let tx_batch = bank.prepare_batch_for_tests(txs); let mut commit_results = bank @@ -108,15 +122,7 @@ fn process_transaction_and_record_inner( None, ) .0; - let CommittedTransaction { - inner_instructions, - log_messages, - status, - .. - } = commit_results.swap_remove(0).unwrap(); - let inner_instructions = inner_instructions.expect("cpi recording should be enabled"); - let log_messages = log_messages.expect("log recording should be enabled"); - (status, inner_instructions, log_messages) + commit_results.pop().unwrap() } #[cfg(feature = "sbf_rust")] @@ -1880,10 +1886,10 @@ fn test_program_sbf_invoke_in_same_tx_as_deployment() { bank.last_blockhash(), ); if index == 0 { - let results = execute_transactions(&bank, vec![tx]); + let result = load_execute_and_commit_transaction(&bank, tx); assert_eq!( - results[0].as_ref().unwrap_err(), - &TransactionError::ProgramAccountNotFound, + result.unwrap().status, + Err(TransactionError::ProgramAccountNotFound), ); } else { let (result, _, _) = process_transaction_and_record_inner(&bank, tx); diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index f54e5f46793c64..34a1cd9d70b89c 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -160,7 +160,8 @@ use { }, transaction_processing_callback::TransactionProcessingCallback, transaction_processing_result::{ - TransactionProcessingResult, TransactionProcessingResultExtensions, + ProcessedTransaction, TransactionProcessingResult, + TransactionProcessingResultExtensions, }, transaction_processor::{ ExecutionRecordingConfig, TransactionBatchProcessor, TransactionLogMessages, @@ -328,6 +329,7 @@ pub struct LoadAndExecuteTransactionsOutput { pub processed_counts: ProcessedTransactionCounts, } +#[derive(Debug, PartialEq)] pub struct TransactionSimulationResult { pub result: Result<()>, pub logs: TransactionLogMessages, @@ -3098,14 +3100,13 @@ impl Bank { assert_eq!(sanitized_txs.len(), processing_results.len()); for (tx, processing_result) in sanitized_txs.iter().zip(processing_results) { if let Ok(processed_tx) = &processing_result { - let details = &processed_tx.execution_details; // Add the message hash to the status cache to ensure that this message // won't be processed again with a different signature. status_cache.insert( tx.message().recent_blockhash(), tx.message_hash(), self.slot(), - details.status.clone(), + processed_tx.status(), ); // Add the transaction signature to the status cache so that transaction status // can be queried by transaction signature over RPC. In the future, this should @@ -3114,7 +3115,7 @@ impl Bank { tx.message().recent_blockhash(), tx.signature(), self.slot(), - details.status.clone(), + processed_tx.status(), ); } } @@ -3364,30 +3365,35 @@ impl Bank { let processing_result = processing_results .pop() .unwrap_or(Err(TransactionError::InvalidProgramForExecution)); - let flattened_result = processing_result.flattened_result(); - let (post_simulation_accounts, logs, return_data, inner_instructions) = + let (post_simulation_accounts, result, logs, return_data, inner_instructions) = match processing_result { - Ok(processed_tx) => { - let details = processed_tx.execution_details; - let post_simulation_accounts = processed_tx - .loaded_transaction - .accounts - .into_iter() - .take(number_of_accounts) - .collect::>(); - ( - post_simulation_accounts, - details.log_messages, - details.return_data, - details.inner_instructions, - ) - } - Err(_) => (vec![], None, None, None), + Ok(processed_tx) => match processed_tx { + ProcessedTransaction::Executed(executed_tx) => { + let details = executed_tx.execution_details; + let post_simulation_accounts = executed_tx + .loaded_transaction + .accounts + .into_iter() + .take(number_of_accounts) + .collect::>(); + ( + post_simulation_accounts, + details.status, + details.log_messages, + details.return_data, + details.inner_instructions, + ) + } + ProcessedTransaction::FeesOnly(fees_only_tx) => { + (vec![], Err(fees_only_tx.load_error), None, None, None) + } + }, + Err(error) => (vec![], Err(error), None, None, None), }; let logs = logs.unwrap_or_default(); TransactionSimulationResult { - result: flattened_result, + result, logs, post_simulation_accounts, units_consumed, @@ -3567,7 +3573,8 @@ impl Bank { .filter_map(|(processing_result, transaction)| { // Skip log collection for unprocessed transactions let processed_tx = processing_result.processed_transaction()?; - let execution_details = &processed_tx.execution_details; + // Skip log collection for unexecuted transactions + let execution_details = processed_tx.execution_details()?; Self::collect_transaction_logs( &transaction_log_collector_config, transaction, @@ -3717,7 +3724,7 @@ impl Bank { .iter() .for_each(|processing_result| match processing_result { Ok(processed_tx) => { - fees += processed_tx.loaded_transaction.fee_details.total_fee(); + fees += processed_tx.fee_details().total_fee(); } Err(_) => {} }); @@ -3736,8 +3743,7 @@ impl Bank { .iter() .for_each(|processing_result| match processing_result { Ok(processed_tx) => { - accumulated_fee_details - .accumulate(&processed_tx.loaded_transaction.fee_details); + accumulated_fee_details.accumulate(&processed_tx.fee_details()); } Err(_) => {} }); @@ -3810,7 +3816,9 @@ impl Bank { let ((), update_executors_us) = measure_us!({ let mut cache = None; for processing_result in &processing_results { - if let Some(executed_tx) = processing_result.processed_transaction() { + if let Some(ProcessedTransaction::Executed(executed_tx)) = + processing_result.processed_transaction() + { let programs_modified_by_tx = &executed_tx.programs_modified_by_tx; if executed_tx.was_successful() && !programs_modified_by_tx.is_empty() { cache @@ -3826,7 +3834,7 @@ impl Bank { let accounts_data_len_delta = processing_results .iter() .filter_map(|processing_result| processing_result.processed_transaction()) - .map(|processed_tx| &processed_tx.execution_details) + .filter_map(|processed_tx| processed_tx.execution_details()) .filter_map(|details| { details .status @@ -3864,37 +3872,52 @@ impl Bank { ) -> Vec { processing_results .into_iter() - .map(|processing_result| { - let processed_tx = processing_result?; - let execution_details = processed_tx.execution_details; - let LoadedTransaction { - rent_debits, - accounts: loaded_accounts, - loaded_accounts_data_size, - fee_details, - .. - } = processed_tx.loaded_transaction; - - // Rent is only collected for successfully executed transactions - let rent_debits = if execution_details.was_successful() { - rent_debits - } else { - RentDebits::default() - }; + .map(|processing_result| match processing_result? { + ProcessedTransaction::Executed(executed_tx) => { + let execution_details = executed_tx.execution_details; + let LoadedTransaction { + rent_debits, + accounts: loaded_accounts, + loaded_accounts_data_size, + fee_details, + .. + } = executed_tx.loaded_transaction; + + // Rent is only collected for successfully executed transactions + let rent_debits = if execution_details.was_successful() { + rent_debits + } else { + RentDebits::default() + }; - Ok(CommittedTransaction { - status: execution_details.status, - log_messages: execution_details.log_messages, - inner_instructions: execution_details.inner_instructions, - return_data: execution_details.return_data, - executed_units: execution_details.executed_units, - fee_details, - rent_debits, + Ok(CommittedTransaction { + status: execution_details.status, + log_messages: execution_details.log_messages, + inner_instructions: execution_details.inner_instructions, + return_data: execution_details.return_data, + executed_units: execution_details.executed_units, + fee_details, + rent_debits, + loaded_account_stats: TransactionLoadedAccountsStats { + loaded_accounts_count: loaded_accounts.len(), + loaded_accounts_data_size, + }, + }) + } + ProcessedTransaction::FeesOnly(fees_only_tx) => Ok(CommittedTransaction { + status: Err(fees_only_tx.load_error), + log_messages: None, + inner_instructions: None, + return_data: None, + executed_units: 0, + rent_debits: RentDebits::default(), + fee_details: fees_only_tx.fee_details, loaded_account_stats: TransactionLoadedAccountsStats { - loaded_accounts_count: loaded_accounts.len(), - loaded_accounts_data_size, + loaded_accounts_count: fees_only_tx.rollback_accounts.count(), + loaded_accounts_data_size: fees_only_tx.rollback_accounts.data_size() + as u32, }, - }) + }), }) .collect() } @@ -3903,6 +3926,7 @@ impl Bank { let collected_rent = processing_results .iter() .filter_map(|processing_result| processing_result.processed_transaction()) + .filter_map(|processed_tx| processed_tx.executed_transaction()) .filter(|executed_tx| executed_tx.was_successful()) .map(|executed_tx| executed_tx.loaded_transaction.rent) .sum(); @@ -5853,6 +5877,11 @@ impl Bank { .processed_transaction() .map(|processed_tx| (tx, processed_tx)) }) + .filter_map(|(tx, processed_tx)| { + processed_tx + .executed_transaction() + .map(|executed_tx| (tx, executed_tx)) + }) .filter(|(_, executed_tx)| executed_tx.was_successful()) .flat_map(|(tx, executed_tx)| { let num_account_keys = tx.message().account_keys().len(); diff --git a/runtime/src/bank/tests.rs b/runtime/src/bank/tests.rs index 36d3ca8d1c7f8b..dfbd449731179a 100644 --- a/runtime/src/bank/tests.rs +++ b/runtime/src/bank/tests.rs @@ -101,7 +101,8 @@ use { }, solana_stake_program::stake_state::{self, StakeStateV2}, solana_svm::{ - account_loader::LoadedTransaction, + account_loader::{FeesOnlyTransaction, LoadedTransaction}, + rollback_accounts::RollbackAccounts, transaction_commit_result::TransactionCommitResultExtensions, transaction_execution_result::ExecutedTransaction, }, @@ -234,25 +235,27 @@ fn test_race_register_tick_freeze() { } } -fn new_processing_result( +fn new_executed_processing_result( status: Result<()>, fee_details: FeeDetails, ) -> TransactionProcessingResult { - Ok(ExecutedTransaction { - loaded_transaction: LoadedTransaction { - fee_details, - ..LoadedTransaction::default() - }, - execution_details: TransactionExecutionDetails { - status, - log_messages: None, - inner_instructions: None, - return_data: None, - executed_units: 0, - accounts_data_len_delta: 0, + Ok(ProcessedTransaction::Executed(Box::new( + ExecutedTransaction { + loaded_transaction: LoadedTransaction { + fee_details, + ..LoadedTransaction::default() + }, + execution_details: TransactionExecutionDetails { + status, + log_messages: None, + inner_instructions: None, + return_data: None, + executed_units: 0, + accounts_data_len_delta: 0, + }, + programs_modified_by_tx: HashMap::new(), }, - programs_modified_by_tx: HashMap::new(), - }) + ))) } impl Bank { @@ -2880,14 +2883,22 @@ fn test_filter_program_errors_and_collect_fee() { let tx_fee = 42; let fee_details = FeeDetails::new(tx_fee, 0, false); let processing_results = vec![ - new_processing_result(Ok(()), fee_details), - new_processing_result( + Err(TransactionError::AccountNotFound), + new_executed_processing_result(Ok(()), fee_details), + new_executed_processing_result( Err(TransactionError::InstructionError( 1, SystemError::ResultWithNegativeLamports.into(), )), fee_details, ), + Ok(ProcessedTransaction::FeesOnly(Box::new( + FeesOnlyTransaction { + load_error: TransactionError::InvalidProgramForExecution, + rollback_accounts: RollbackAccounts::default(), + fee_details, + }, + ))), ]; let initial_balance = bank.get_balance(&leader); @@ -2895,7 +2906,7 @@ fn test_filter_program_errors_and_collect_fee() { bank.freeze(); assert_eq!( bank.get_balance(&leader), - initial_balance + bank.fee_rate_governor.burn(tx_fee * 2).0 + initial_balance + bank.fee_rate_governor.burn(tx_fee * 3).0 ); } @@ -2911,14 +2922,22 @@ fn test_filter_program_errors_and_collect_priority_fee() { let priority_fee = 42; let fee_details: FeeDetails = FeeDetails::new(0, priority_fee, false); let processing_results = vec![ - new_processing_result(Ok(()), fee_details), - new_processing_result( + Err(TransactionError::AccountNotFound), + new_executed_processing_result(Ok(()), fee_details), + new_executed_processing_result( Err(TransactionError::InstructionError( 1, SystemError::ResultWithNegativeLamports.into(), )), fee_details, ), + Ok(ProcessedTransaction::FeesOnly(Box::new( + FeesOnlyTransaction { + load_error: TransactionError::InvalidProgramForExecution, + rollback_accounts: RollbackAccounts::default(), + fee_details, + }, + ))), ]; let initial_balance = bank.get_balance(&leader); @@ -2926,7 +2945,7 @@ fn test_filter_program_errors_and_collect_priority_fee() { bank.freeze(); assert_eq!( bank.get_balance(&leader), - initial_balance + bank.fee_rate_governor.burn(priority_fee * 2).0 + initial_balance + bank.fee_rate_governor.burn(priority_fee * 3).0 ); } @@ -3081,6 +3100,103 @@ fn test_interleaving_locks() { .is_ok()); } +#[test_case(false; "disable fees-only transactions")] +#[test_case(true; "enable fees-only transactions")] +fn test_load_and_execute_commit_transactions_fees_only(enable_fees_only_txs: bool) { + let GenesisConfigInfo { + mut genesis_config, .. + } = genesis_utils::create_genesis_config(100 * LAMPORTS_PER_SOL); + if !enable_fees_only_txs { + genesis_config + .accounts + .remove(&solana_sdk::feature_set::enable_transaction_loading_failure_fees::id()); + } + genesis_config.rent = Rent::default(); + genesis_config.fee_rate_governor = FeeRateGovernor::new(5000, 0); + let (bank, _bank_forks) = Bank::new_with_bank_forks_for_tests(&genesis_config); + let bank = Bank::new_from_parent( + bank, + &Pubkey::new_unique(), + genesis_config.epoch_schedule.get_first_slot_in_epoch(1), + ); + + // Use rent-paying fee payer to show that rent is not collected for fees + // only transactions even when they use a rent-paying account. + let rent_paying_fee_payer = Pubkey::new_unique(); + bank.store_account( + &rent_paying_fee_payer, + &AccountSharedData::new( + genesis_config.rent.minimum_balance(0) - 1, + 0, + &system_program::id(), + ), + ); + + // Use nonce to show that loaded account stats also included loaded + // nonce account size + let nonce_size = nonce::State::size(); + let nonce_balance = genesis_config.rent.minimum_balance(nonce_size); + let nonce_pubkey = Pubkey::new_unique(); + let nonce_authority = rent_paying_fee_payer; + let nonce_initial_hash = DurableNonce::from_blockhash(&Hash::new_unique()); + let nonce_data = nonce::state::Data::new(nonce_authority, nonce_initial_hash, 5000); + let nonce_account = AccountSharedData::new_data( + nonce_balance, + &nonce::state::Versions::new(nonce::State::Initialized(nonce_data.clone())), + &system_program::id(), + ) + .unwrap(); + bank.store_account(&nonce_pubkey, &nonce_account); + + // Invoke missing program to trigger load error in order to commit a + // fees-only transaction + let missing_program_id = Pubkey::new_unique(); + let transaction = Transaction::new_unsigned(Message::new_with_blockhash( + &[ + system_instruction::advance_nonce_account(&nonce_pubkey, &rent_paying_fee_payer), + Instruction::new_with_bincode(missing_program_id, &0, vec![]), + ], + Some(&rent_paying_fee_payer), + &nonce_data.blockhash(), + )); + + let batch = bank.prepare_batch_for_tests(vec![transaction]); + let commit_results = bank + .load_execute_and_commit_transactions( + &batch, + MAX_PROCESSING_AGE, + true, + ExecutionRecordingConfig::new_single_setting(true), + &mut ExecuteTimings::default(), + None, + ) + .0; + + if enable_fees_only_txs { + assert_eq!( + commit_results, + vec![Ok(CommittedTransaction { + status: Err(TransactionError::ProgramAccountNotFound), + log_messages: None, + inner_instructions: None, + return_data: None, + executed_units: 0, + fee_details: FeeDetails::new(5000, 0, true), + rent_debits: RentDebits::default(), + loaded_account_stats: TransactionLoadedAccountsStats { + loaded_accounts_count: 2, + loaded_accounts_data_size: nonce_size as u32, + }, + })] + ); + } else { + assert_eq!( + commit_results, + vec![Err(TransactionError::ProgramAccountNotFound)] + ); + } +} + #[test] fn test_readonly_relaxed_locks() { let (genesis_config, _) = create_genesis_config(3); @@ -6932,6 +7048,15 @@ fn test_bpf_loader_upgradeable_deploy_with_max_len() { assert!(slot_versions.is_empty()); } + // Advance bank to get a new last blockhash so that when we retry invocation + // after creating the program, the new transaction created below with the + // same `invocation_message` as above doesn't return `AlreadyProcessed` when + // processed. + goto_end_of_slot(bank.clone()); + let bank = bank_client + .advance_slot(1, bank_forks.as_ref(), &mint_keypair.pubkey()) + .unwrap(); + // Load program file let mut file = File::open("../programs/bpf_loader/test_elfs/out/noop_aligned.so") .expect("file open failed"); @@ -6997,8 +7122,8 @@ fn test_bpf_loader_upgradeable_deploy_with_max_len() { let program_cache = bank.transaction_processor.program_cache.read().unwrap(); let slot_versions = program_cache.get_slot_versions_for_tests(&program_keypair.pubkey()); assert_eq!(slot_versions.len(), 1); - assert_eq!(slot_versions[0].deployment_slot, 0); - assert_eq!(slot_versions[0].effective_slot, 0); + assert_eq!(slot_versions[0].deployment_slot, bank.slot()); + assert_eq!(slot_versions[0].effective_slot, bank.slot()); assert!(matches!( slot_versions[0].program, ProgramCacheEntryType::Closed, @@ -7021,8 +7146,8 @@ fn test_bpf_loader_upgradeable_deploy_with_max_len() { let program_cache = bank.transaction_processor.program_cache.read().unwrap(); let slot_versions = program_cache.get_slot_versions_for_tests(&buffer_address); assert_eq!(slot_versions.len(), 1); - assert_eq!(slot_versions[0].deployment_slot, 0); - assert_eq!(slot_versions[0].effective_slot, 0); + assert_eq!(slot_versions[0].deployment_slot, bank.slot()); + assert_eq!(slot_versions[0].effective_slot, bank.slot()); assert!(matches!( slot_versions[0].program, ProgramCacheEntryType::Closed, @@ -7123,14 +7248,14 @@ fn test_bpf_loader_upgradeable_deploy_with_max_len() { let program_cache = bank.transaction_processor.program_cache.read().unwrap(); let slot_versions = program_cache.get_slot_versions_for_tests(&program_keypair.pubkey()); assert_eq!(slot_versions.len(), 2); - assert_eq!(slot_versions[0].deployment_slot, 0); - assert_eq!(slot_versions[0].effective_slot, 0); + assert_eq!(slot_versions[0].deployment_slot, bank.slot() - 1); + assert_eq!(slot_versions[0].effective_slot, bank.slot() - 1); assert!(matches!( slot_versions[0].program, ProgramCacheEntryType::Closed, )); - assert_eq!(slot_versions[1].deployment_slot, 0); - assert_eq!(slot_versions[1].effective_slot, 1); + assert_eq!(slot_versions[1].deployment_slot, bank.slot() - 1); + assert_eq!(slot_versions[1].effective_slot, bank.slot()); assert!(matches!( slot_versions[1].program, ProgramCacheEntryType::Loaded(_), @@ -12772,22 +12897,56 @@ fn test_failed_simulation_compute_units() { assert_eq!(expected_consumed_units, simulation.units_consumed); } +/// Test that simulations report the load error of fees-only transactions +#[test] +fn test_failed_simulation_load_error() { + let (genesis_config, mint_keypair) = create_genesis_config(LAMPORTS_PER_SOL); + let bank = Bank::new_for_tests(&genesis_config); + let (bank, _bank_forks) = bank.wrap_with_bank_forks_for_tests(); + let missing_program_id = Pubkey::new_unique(); + let message = Message::new( + &[Instruction::new_with_bincode( + missing_program_id, + &0, + vec![], + )], + Some(&mint_keypair.pubkey()), + ); + let transaction = Transaction::new(&[&mint_keypair], message, bank.last_blockhash()); + + bank.freeze(); + let sanitized = SanitizedTransaction::from_transaction_for_tests(transaction); + let simulation = bank.simulate_transaction(&sanitized, false); + assert_eq!( + simulation, + TransactionSimulationResult { + result: Err(TransactionError::ProgramAccountNotFound), + logs: vec![], + post_simulation_accounts: vec![], + units_consumed: 0, + return_data: None, + inner_instructions: None, + } + ); +} + #[test] fn test_filter_program_errors_and_collect_fee_details() { - // TX | EXECUTION RESULT | COLLECT | COLLECT + // TX | PROCESSING RESULT | COLLECT | COLLECT // | | (TX_FEE, PRIO_FEE) | RESULT // --------------------------------------------------------------------------------- - // tx1 | not executed | (0 , 0) | Original Err - // tx2 | executed and no error | (5_000, 1_000) | Ok + // tx1 | not processed | (0 , 0) | Original Err + // tx2 | processed but not executed | (5_000, 1_000) | Ok // tx3 | executed has error | (5_000, 1_000) | Ok + // tx4 | executed and no error | (5_000, 1_000) | Ok // let initial_payer_balance = 7_000; let tx_fee = 5000; let priority_fee = 1000; - let tx_fee_details = FeeDetails::new(tx_fee, priority_fee, false); + let fee_details = FeeDetails::new(tx_fee, priority_fee, false); let expected_collected_fee_details = CollectorFeeDetails { - transaction_fee: 2 * tx_fee, - priority_fee: 2 * priority_fee, + transaction_fee: 3 * tx_fee, + priority_fee: 3 * priority_fee, }; let GenesisConfigInfo { @@ -12799,14 +12958,21 @@ fn test_filter_program_errors_and_collect_fee_details() { let results = vec![ Err(TransactionError::AccountNotFound), - new_processing_result(Ok(()), tx_fee_details), - new_processing_result( + Ok(ProcessedTransaction::FeesOnly(Box::new( + FeesOnlyTransaction { + load_error: TransactionError::InvalidProgramForExecution, + rollback_accounts: RollbackAccounts::default(), + fee_details, + }, + ))), + new_executed_processing_result( Err(TransactionError::InstructionError( 0, SystemError::ResultWithNegativeLamports.into(), )), - tx_fee_details, + fee_details, ), + new_executed_processing_result(Ok(()), fee_details), ]; bank.filter_program_errors_and_collect_fee_details(&results); diff --git a/sdk/src/feature_set.rs b/sdk/src/feature_set.rs index 7322fdbfde900c..0cb89f631eabef 100644 --- a/sdk/src/feature_set.rs +++ b/sdk/src/feature_set.rs @@ -841,6 +841,10 @@ pub mod vote_only_retransmitter_signed_fec_sets { solana_sdk::declare_id!("RfEcA95xnhuwooVAhUUksEJLZBF7xKCLuqrJoqk4Zph"); } +pub mod enable_transaction_loading_failure_fees { + solana_sdk::declare_id!("PaymEPK2oqwT9TXAVfadjztH2H6KfLEB9Hhd5Q5frvP"); +} + pub mod enable_turbine_extended_fanout_experiments { solana_sdk::declare_id!("BZn14Liea52wtBwrXUxTv6vojuTTmfc7XGEDTXrvMD7b"); } @@ -1054,6 +1058,7 @@ lazy_static! { (move_stake_and_move_lamports_ixs::id(), "Enable MoveStake and MoveLamports stake program instructions #1610"), (ed25519_precompile_verify_strict::id(), "Use strict verification in ed25519 precompile SIMD-0152"), (vote_only_retransmitter_signed_fec_sets::id(), "vote only on retransmitter signed fec sets"), + (enable_transaction_loading_failure_fees::id(), "Enable fees for some additional transaction failures SIMD-0082"), (enable_turbine_extended_fanout_experiments::id(), "enable turbine extended fanout experiments #"), (deprecate_legacy_vote_ixs::id(), "Deprecate legacy vote instructions"), /*************** ADD NEW FEATURES HERE ***************/ diff --git a/svm/Cargo.toml b/svm/Cargo.toml index 603f0c8ae8a1d5..2627f4fbd8bf5a 100644 --- a/svm/Cargo.toml +++ b/svm/Cargo.toml @@ -40,6 +40,7 @@ crate-type = ["lib"] name = "solana_svm" [dev-dependencies] +assert_matches = { workspace = true } bincode = { workspace = true } lazy_static = { workspace = true } libsecp256k1 = { workspace = true } diff --git a/svm/src/account_saver.rs b/svm/src/account_saver.rs index f0881050dea4bc..0ecbd181e6698f 100644 --- a/svm/src/account_saver.rs +++ b/svm/src/account_saver.rs @@ -2,7 +2,8 @@ use { crate::{ rollback_accounts::RollbackAccounts, transaction_processing_result::{ - TransactionProcessingResult, TransactionProcessingResultExtensions, + ProcessedTransaction, TransactionProcessingResult, + TransactionProcessingResultExtensions, }, }, solana_sdk::{ @@ -27,12 +28,15 @@ fn max_number_of_accounts_to_collect( .processed_transaction() .map(|processed_tx| (processed_tx, tx)) }) - .map( - |(processed_tx, tx)| match processed_tx.execution_details.status { - Ok(_) => tx.num_write_locks() as usize, - Err(_) => processed_tx.loaded_transaction.rollback_accounts.count(), - }, - ) + .map(|(processed_tx, tx)| match processed_tx { + ProcessedTransaction::Executed(executed_tx) => { + match executed_tx.execution_details.status { + Ok(_) => tx.num_write_locks() as usize, + Err(_) => executed_tx.loaded_transaction.rollback_accounts.count(), + } + } + ProcessedTransaction::FeesOnly(fees_only_tx) => fees_only_tx.rollback_accounts.count(), + }) .sum() } @@ -51,22 +55,36 @@ pub fn collect_accounts_to_store<'a, T: SVMMessage>( continue; }; - if processed_tx.execution_details.status.is_ok() { - collect_accounts_for_successful_tx( - &mut accounts, - &mut transactions, - transaction, - &processed_tx.loaded_transaction.accounts, - ); - } else { - collect_accounts_for_failed_tx( - &mut accounts, - &mut transactions, - transaction, - &mut processed_tx.loaded_transaction.rollback_accounts, - durable_nonce, - lamports_per_signature, - ); + match processed_tx { + ProcessedTransaction::Executed(executed_tx) => { + if executed_tx.execution_details.status.is_ok() { + collect_accounts_for_successful_tx( + &mut accounts, + &mut transactions, + transaction, + &executed_tx.loaded_transaction.accounts, + ); + } else { + collect_accounts_for_failed_tx( + &mut accounts, + &mut transactions, + transaction, + &mut executed_tx.loaded_transaction.rollback_accounts, + durable_nonce, + lamports_per_signature, + ); + } + } + ProcessedTransaction::FeesOnly(fees_only_tx) => { + collect_accounts_for_failed_tx( + &mut accounts, + &mut transactions, + transaction, + &mut fees_only_tx.rollback_accounts, + durable_nonce, + lamports_per_signature, + ); + } } } (accounts, transactions) @@ -141,7 +159,7 @@ mod tests { use { super::*, crate::{ - account_loader::LoadedTransaction, + account_loader::{FeesOnlyTransaction, LoadedTransaction}, nonce_info::NonceInfo, transaction_execution_result::{ExecutedTransaction, TransactionExecutionDetails}, }, @@ -178,22 +196,24 @@ mod tests { )) } - fn new_processing_result( + fn new_executed_processing_result( status: Result<()>, loaded_transaction: LoadedTransaction, ) -> TransactionProcessingResult { - Ok(ExecutedTransaction { - execution_details: TransactionExecutionDetails { - status, - log_messages: None, - inner_instructions: None, - return_data: None, - executed_units: 0, - accounts_data_len_delta: 0, + Ok(ProcessedTransaction::Executed(Box::new( + ExecutedTransaction { + execution_details: TransactionExecutionDetails { + status, + log_messages: None, + inner_instructions: None, + return_data: None, + executed_units: 0, + accounts_data_len_delta: 0, + }, + loaded_transaction, + programs_modified_by_tx: HashMap::new(), }, - loaded_transaction, - programs_modified_by_tx: HashMap::new(), - }) + ))) } #[test] @@ -259,8 +279,8 @@ mod tests { let txs = vec![tx0.clone(), tx1.clone()]; let mut processing_results = vec![ - new_processing_result(Ok(()), loaded0), - new_processing_result(Ok(()), loaded1), + new_executed_processing_result(Ok(()), loaded0), + new_executed_processing_result(Ok(()), loaded1), ]; let max_collected_accounts = max_number_of_accounts_to_collect(&txs, &processing_results); assert_eq!(max_collected_accounts, 2); @@ -280,7 +300,7 @@ mod tests { } #[test] - fn test_nonced_failure_accounts_rollback_fee_payer_only() { + fn test_collect_accounts_for_failed_tx_rollback_fee_payer_only() { let from = keypair_from_seed(&[1; 32]).unwrap(); let from_address = from.pubkey(); let to_address = Pubkey::new_unique(); @@ -312,7 +332,7 @@ mod tests { }; let txs = vec![tx]; - let mut processing_results = vec![new_processing_result( + let mut processing_results = vec![new_executed_processing_result( Err(TransactionError::InstructionError( 1, InstructionError::InvalidArgument, @@ -337,7 +357,7 @@ mod tests { } #[test] - fn test_nonced_failure_accounts_rollback_separate_nonce_and_fee_payer() { + fn test_collect_accounts_for_failed_tx_rollback_separate_nonce_and_fee_payer() { let nonce_address = Pubkey::new_unique(); let nonce_authority = keypair_from_seed(&[0; 32]).unwrap(); let from = keypair_from_seed(&[1; 32]).unwrap(); @@ -398,7 +418,7 @@ mod tests { let durable_nonce = DurableNonce::from_blockhash(&Hash::new_unique()); let txs = vec![tx]; - let mut processing_results = vec![new_processing_result( + let mut processing_results = vec![new_executed_processing_result( Err(TransactionError::InstructionError( 1, InstructionError::InvalidArgument, @@ -437,7 +457,7 @@ mod tests { } #[test] - fn test_nonced_failure_accounts_rollback_same_nonce_and_fee_payer() { + fn test_collect_accounts_for_failed_tx_rollback_same_nonce_and_fee_payer() { let nonce_authority = keypair_from_seed(&[0; 32]).unwrap(); let nonce_address = nonce_authority.pubkey(); let from = keypair_from_seed(&[1; 32]).unwrap(); @@ -496,7 +516,7 @@ mod tests { let durable_nonce = DurableNonce::from_blockhash(&Hash::new_unique()); let txs = vec![tx]; - let mut processing_results = vec![new_processing_result( + let mut processing_results = vec![new_executed_processing_result( Err(TransactionError::InstructionError( 1, InstructionError::InvalidArgument, @@ -524,4 +544,44 @@ mod tests { ) .is_some()); } + + #[test] + fn test_collect_accounts_for_failed_fees_only_tx() { + let from = keypair_from_seed(&[1; 32]).unwrap(); + let from_address = from.pubkey(); + let to_address = Pubkey::new_unique(); + + let instructions = vec![system_instruction::transfer(&from_address, &to_address, 42)]; + let message = Message::new(&instructions, Some(&from_address)); + let blockhash = Hash::new_unique(); + let tx = new_sanitized_tx(&[&from], message, blockhash); + + let from_account_pre = AccountSharedData::new(4242, 0, &Pubkey::default()); + + let txs = vec![tx]; + let mut processing_results = vec![Ok(ProcessedTransaction::FeesOnly(Box::new( + FeesOnlyTransaction { + load_error: TransactionError::InvalidProgramForExecution, + fee_details: FeeDetails::default(), + rollback_accounts: RollbackAccounts::FeePayerOnly { + fee_payer_account: from_account_pre.clone(), + }, + }, + )))]; + let max_collected_accounts = max_number_of_accounts_to_collect(&txs, &processing_results); + assert_eq!(max_collected_accounts, 1); + let durable_nonce = DurableNonce::from_blockhash(&Hash::new_unique()); + let (collected_accounts, _) = + collect_accounts_to_store(&txs, &mut processing_results, &durable_nonce, 0); + assert_eq!(collected_accounts.len(), 1); + assert_eq!( + collected_accounts + .iter() + .find(|(pubkey, _account)| *pubkey == &from_address) + .map(|(_pubkey, account)| *account) + .cloned() + .unwrap(), + from_account_pre, + ); + } } diff --git a/svm/src/rollback_accounts.rs b/svm/src/rollback_accounts.rs index 71b670d37c4f85..c2c02f2f80bd43 100644 --- a/svm/src/rollback_accounts.rs +++ b/svm/src/rollback_accounts.rs @@ -79,6 +79,22 @@ impl RollbackAccounts { Self::SeparateNonceAndFeePayer { .. } => 2, } } + + /// Size of accounts tracked for rollback, used when calculating the actual + /// cost of transaction processing in the cost model. + pub fn data_size(&self) -> usize { + match self { + Self::FeePayerOnly { fee_payer_account } => fee_payer_account.data().len(), + Self::SameNonceAndFeePayer { nonce } => nonce.account().data().len(), + Self::SeparateNonceAndFeePayer { + nonce, + fee_payer_account, + } => fee_payer_account + .data() + .len() + .saturating_add(nonce.account().data().len()), + } + } } #[cfg(test)] diff --git a/svm/src/transaction_commit_result.rs b/svm/src/transaction_commit_result.rs index 8bbada73634ad9..6d838ea0786d53 100644 --- a/svm/src/transaction_commit_result.rs +++ b/svm/src/transaction_commit_result.rs @@ -9,6 +9,7 @@ use { pub type TransactionCommitResult = TransactionResult; #[derive(Clone, Debug)] +#[cfg_attr(feature = "dev-context-only-utils", derive(PartialEq))] pub struct CommittedTransaction { pub status: TransactionResult<()>, pub log_messages: Option>, diff --git a/svm/src/transaction_execution_result.rs b/svm/src/transaction_execution_result.rs index 6a41294ddb975e..c226ae262a82ad 100644 --- a/svm/src/transaction_execution_result.rs +++ b/svm/src/transaction_execution_result.rs @@ -7,36 +7,16 @@ pub use solana_sdk::inner_instruction::{InnerInstruction, InnerInstructionsList} use { crate::account_loader::LoadedTransaction, solana_program_runtime::loaded_programs::ProgramCacheEntry, - solana_sdk::{ - pubkey::Pubkey, - transaction::{self, TransactionError}, - transaction_context::TransactionReturnData, - }, + solana_sdk::{pubkey::Pubkey, transaction, transaction_context::TransactionReturnData}, std::{collections::HashMap, sync::Arc}, }; -#[derive(Debug, Default, Clone)] +#[derive(Debug, Default, Clone, PartialEq)] pub struct TransactionLoadedAccountsStats { pub loaded_accounts_data_size: u32, pub loaded_accounts_count: usize, } -/// Type safe representation of a transaction execution attempt which -/// differentiates between a transaction that was executed (will be -/// committed to the ledger) and a transaction which wasn't executed -/// and will be dropped. -/// -/// Note: `Result` is not -/// used because it's easy to forget that the inner `details.status` field -/// is what should be checked to detect a successful transaction. This -/// enum provides a convenience method `Self::was_executed_successfully` to -/// make such checks hard to do incorrectly. -#[derive(Debug, Clone)] -pub enum TransactionExecutionResult { - Executed(Box), - NotExecuted(TransactionError), -} - #[derive(Debug, Clone)] pub struct ExecutedTransaction { pub loaded_transaction: LoadedTransaction, @@ -46,7 +26,7 @@ pub struct ExecutedTransaction { impl ExecutedTransaction { pub fn was_successful(&self) -> bool { - self.execution_details.status.is_ok() + self.execution_details.was_successful() } } diff --git a/svm/src/transaction_processing_result.rs b/svm/src/transaction_processing_result.rs index 0ad68e0d18a803..7802b9ac213808 100644 --- a/svm/src/transaction_processing_result.rs +++ b/svm/src/transaction_processing_result.rs @@ -1,10 +1,15 @@ use { - crate::transaction_execution_result::ExecutedTransaction, - solana_sdk::transaction::Result as TransactionResult, + crate::{ + account_loader::FeesOnlyTransaction, + transaction_execution_result::{ExecutedTransaction, TransactionExecutionDetails}, + }, + solana_sdk::{ + fee::FeeDetails, + transaction::{Result as TransactionResult, TransactionError}, + }, }; pub type TransactionProcessingResult = TransactionResult; -pub type ProcessedTransaction = ExecutedTransaction; pub trait TransactionProcessingResultExtensions { fn was_processed(&self) -> bool; @@ -14,6 +19,16 @@ pub trait TransactionProcessingResultExtensions { fn flattened_result(&self) -> TransactionResult<()>; } +#[derive(Debug)] +pub enum ProcessedTransaction { + /// Transaction was executed, but if execution failed, all account state changes + /// will be rolled back except deducted fees and any advanced nonces + Executed(Box), + /// Transaction was not able to be executed but fees are able to be + /// collected and any nonces are advanceable + FeesOnly(Box), +} + impl TransactionProcessingResultExtensions for TransactionProcessingResult { fn was_processed(&self) -> bool { self.is_ok() @@ -21,7 +36,7 @@ impl TransactionProcessingResultExtensions for TransactionProcessingResult { fn was_processed_with_successful_result(&self) -> bool { match self { - Ok(processed_tx) => processed_tx.was_successful(), + Ok(processed_tx) => processed_tx.was_processed_with_successful_result(), Err(_) => false, } } @@ -43,6 +58,43 @@ impl TransactionProcessingResultExtensions for TransactionProcessingResult { fn flattened_result(&self) -> TransactionResult<()> { self.as_ref() .map_err(|err| err.clone()) - .and_then(|processed_tx| processed_tx.execution_details.status.clone()) + .and_then(|processed_tx| processed_tx.status()) + } +} + +impl ProcessedTransaction { + fn was_processed_with_successful_result(&self) -> bool { + match self { + Self::Executed(executed_tx) => executed_tx.execution_details.status.is_ok(), + Self::FeesOnly(_) => false, + } + } + + pub fn status(&self) -> TransactionResult<()> { + match self { + Self::Executed(executed_tx) => executed_tx.execution_details.status.clone(), + Self::FeesOnly(details) => Err(TransactionError::clone(&details.load_error)), + } + } + + pub fn fee_details(&self) -> FeeDetails { + match self { + Self::Executed(executed_tx) => executed_tx.loaded_transaction.fee_details, + Self::FeesOnly(details) => details.fee_details, + } + } + + pub fn executed_transaction(&self) -> Option<&ExecutedTransaction> { + match self { + Self::Executed(context) => Some(context), + Self::FeesOnly { .. } => None, + } + } + + pub fn execution_details(&self) -> Option<&TransactionExecutionDetails> { + match self { + Self::Executed(context) => Some(&context.execution_details), + Self::FeesOnly { .. } => None, + } } } diff --git a/svm/src/transaction_processor.rs b/svm/src/transaction_processor.rs index 8b1a30f1ad913b..75ddbca4ca1f73 100644 --- a/svm/src/transaction_processor.rs +++ b/svm/src/transaction_processor.rs @@ -16,7 +16,7 @@ use { transaction_error_metrics::TransactionErrorMetrics, transaction_execution_result::{ExecutedTransaction, TransactionExecutionDetails}, transaction_processing_callback::TransactionProcessingCallback, - transaction_processing_result::TransactionProcessingResult, + transaction_processing_result::{ProcessedTransaction, TransactionProcessingResult}, }, log::debug, percentage::Percentage, @@ -37,7 +37,7 @@ use { solana_sdk::{ account::{AccountSharedData, ReadableAccount, PROGRAM_OWNERS}, clock::{Epoch, Slot}, - feature_set::{remove_rounding_in_fee_calculation, FeatureSet}, + feature_set::{self, remove_rounding_in_fee_calculation, FeatureSet}, fee::{FeeBudgetLimits, FeeStructure}, hash::Hash, inner_instruction::{InnerInstruction, InnerInstructionsList}, @@ -269,12 +269,12 @@ impl TransactionBatchProcessor { ); if program_cache_for_tx_batch.hit_max_limit { - const ERROR: TransactionError = TransactionError::ProgramCacheHitMaxLimit; - let processing_results = vec![Err(ERROR); sanitized_txs.len()]; return LoadAndExecuteSanitizedTransactionsOutput { error_metrics, execute_timings, - processing_results, + processing_results: (0..sanitized_txs.len()) + .map(|_| Err(TransactionError::ProgramCacheHitMaxLimit)) + .collect(), }; } @@ -294,13 +294,22 @@ impl TransactionBatchProcessor { &program_cache_for_tx_batch, )); + let enable_transaction_loading_failure_fees = environment + .feature_set + .is_active(&feature_set::enable_transaction_loading_failure_fees::id()); let (processing_results, execution_us): (Vec, u64) = measure_us!(loaded_transactions .into_iter() .zip(sanitized_txs.iter()) .map(|(load_result, tx)| match load_result { TransactionLoadResult::NotLoaded(err) => Err(err), - TransactionLoadResult::FeesOnly(fees_only_tx) => Err(fees_only_tx.load_error), + TransactionLoadResult::FeesOnly(fees_only_tx) => { + if enable_transaction_loading_failure_fees { + Ok(ProcessedTransaction::FeesOnly(Box::new(fees_only_tx))) + } else { + Err(fees_only_tx.load_error) + } + } TransactionLoadResult::Loaded(loaded_transaction) => { let executed_tx = self.execute_loaded_transaction( tx, @@ -318,7 +327,7 @@ impl TransactionBatchProcessor { program_cache_for_tx_batch.merge(&executed_tx.programs_modified_by_tx); } - Ok(executed_tx) + Ok(ProcessedTransaction::Executed(Box::new(executed_tx))) } }) .collect()); diff --git a/svm/tests/concurrent_tests.rs b/svm/tests/concurrent_tests.rs index e11019be16a346..2e84fbba243663 100644 --- a/svm/tests/concurrent_tests.rs +++ b/svm/tests/concurrent_tests.rs @@ -7,6 +7,7 @@ use { }, transaction_builder::SanitizedTransactionBuilder, }, + assert_matches::assert_matches, mock_bank::MockBankCallback, shuttle::{ sync::{Arc, RwLock}, @@ -22,7 +23,9 @@ use { }, solana_svm::{ account_loader::{CheckedTransactionDetails, TransactionCheckResult}, - transaction_processing_result::TransactionProcessingResultExtensions, + transaction_processing_result::{ + ProcessedTransaction, TransactionProcessingResultExtensions, + }, transaction_processor::{ ExecutionRecordingConfig, TransactionBatchProcessor, TransactionProcessingConfig, TransactionProcessingEnvironment, @@ -251,10 +254,13 @@ fn svm_concurrent() { &processing_config, ); - for (idx, item) in result.processing_results.iter().enumerate() { - assert!(item.was_processed()); + for (idx, processing_result) in result.processing_results.iter().enumerate() { + assert!(processing_result.was_processed()); + let processed_tx = processing_result.processed_transaction().unwrap(); + assert_matches!(processed_tx, &ProcessedTransaction::Executed(_)); + let executed_tx = processed_tx.executed_transaction().unwrap(); let inserted_accounts = &check_tx_data[idx]; - for (key, account_data) in &item.as_ref().unwrap().loaded_transaction.accounts { + for (key, account_data) in &executed_tx.loaded_transaction.accounts { if *key == inserted_accounts.fee_payer { assert_eq!(account_data.lamports(), BALANCE - 10000); } else if *key == inserted_accounts.sender { diff --git a/svm/tests/conformance.rs b/svm/tests/conformance.rs index 5b32c2e164c2d2..5015e1fadd9d0c 100644 --- a/svm/tests/conformance.rs +++ b/svm/tests/conformance.rs @@ -336,9 +336,10 @@ fn run_fixture(fixture: InstrFixture, filename: OsString, execute_as_instr: bool return; } - let executed_tx = result.processing_results[0] + let processed_tx = result.processing_results[0] .processed_transaction() .unwrap(); + let executed_tx = processed_tx.executed_transaction().unwrap(); let execution_details = &executed_tx.execution_details; let loaded_accounts = &executed_tx.loaded_transaction.accounts; verify_accounts_and_data( diff --git a/svm/tests/integration_test.rs b/svm/tests/integration_test.rs index f35f023e54ae2a..317535f52cd361 100644 --- a/svm/tests/integration_test.rs +++ b/svm/tests/integration_test.rs @@ -275,6 +275,8 @@ fn svm_integration() { let executed_tx_0 = result.processing_results[0] .processed_transaction() + .unwrap() + .executed_transaction() .unwrap(); assert!(executed_tx_0.was_successful()); let logs = executed_tx_0 @@ -286,6 +288,8 @@ fn svm_integration() { let executed_tx_1 = result.processing_results[1] .processed_transaction() + .unwrap() + .executed_transaction() .unwrap(); assert!(executed_tx_1.was_successful()); @@ -301,6 +305,8 @@ fn svm_integration() { let executed_tx_2 = result.processing_results[2] .processed_transaction() + .unwrap() + .executed_transaction() .unwrap(); let return_data = executed_tx_2 .execution_details @@ -314,6 +320,8 @@ fn svm_integration() { let executed_tx_3 = result.processing_results[3] .processed_transaction() + .unwrap() + .executed_transaction() .unwrap(); assert!(executed_tx_3.execution_details.status.is_err()); assert!(executed_tx_3