diff --git a/banks-server/src/banks_server.rs b/banks-server/src/banks_server.rs index c08a41c5d91a6b..643f5272c386b6 100644 --- a/banks-server/src/banks_server.rs +++ b/banks-server/src/banks_server.rs @@ -354,7 +354,8 @@ impl Banks for BanksServer { result: Err(error), metadata: None, }, - TransactionExecutionResult::Executed { details, .. } => { + TransactionExecutionResult::Executed(executed_tx) => { + let details = executed_tx.execution_details; BanksTransactionResultWithMetadata { result: details.status, metadata: Some(TransactionMetadata { diff --git a/core/src/banking_stage/committer.rs b/core/src/banking_stage/committer.rs index 1e20a05fba3b2b..33e00a1cf57a4c 100644 --- a/core/src/banking_stage/committer.rs +++ b/core/src/banking_stage/committer.rs @@ -13,10 +13,7 @@ use { vote_sender_types::ReplayVoteSender, }, solana_sdk::{hash::Hash, pubkey::Pubkey, saturating_add_assign}, - solana_svm::{ - account_loader::TransactionLoadResult, - transaction_results::{TransactionExecutionResult, TransactionResults}, - }, + solana_svm::transaction_results::{TransactionExecutionResult, TransactionResults}, solana_transaction_status::{ token_balances::TransactionTokenBalancesSet, TransactionTokenBalance, }, @@ -67,7 +64,6 @@ impl Committer { pub(super) fn commit_transactions( &self, batch: &TransactionBatch, - loaded_transactions: &mut [TransactionLoadResult], execution_results: Vec, last_blockhash: Hash, lamports_per_signature: u64, @@ -88,7 +84,6 @@ impl Committer { let (tx_results, commit_time_us) = measure_us!(bank.commit_transactions( batch.sanitized_transactions(), - loaded_transactions, execution_results, last_blockhash, lamports_per_signature, diff --git a/core/src/banking_stage/consumer.rs b/core/src/banking_stage/consumer.rs index 9a0108a3851770..bed73d36d0cdbf 100644 --- a/core/src/banking_stage/consumer.rs +++ b/core/src/banking_stage/consumer.rs @@ -606,7 +606,6 @@ impl Consumer { execute_and_commit_timings.load_execute_us = load_execute_us; let LoadAndExecuteTransactionsOutput { - mut loaded_transactions, execution_results, mut retryable_transaction_indexes, executed_transactions_count, @@ -681,7 +680,6 @@ impl Consumer { let (commit_time_us, commit_transaction_statuses) = if executed_transactions_count != 0 { self.committer.commit_transactions( batch, - &mut loaded_transactions, execution_results, last_blockhash, lamports_per_signature, diff --git a/ledger-tool/src/main.rs b/ledger-tool/src/main.rs index af1d8b37597e37..c83b94641f0d89 100644 --- a/ledger-tool/src/main.rs +++ b/ledger-tool/src/main.rs @@ -2905,6 +2905,7 @@ fn record_transactions( .collect(); let is_simple_vote_tx = tx.is_simple_vote_transaction(); + let execution_results = execution_results.map(|(details, _)| details); TransactionDetails { accounts, diff --git a/ledger/src/blockstore_processor.rs b/ledger/src/blockstore_processor.rs index bdcf82febb59de..70cdf37b567ff5 100644 --- a/ledger/src/blockstore_processor.rs +++ b/ledger/src/blockstore_processor.rs @@ -45,6 +45,7 @@ use { solana_sdk::{ clock::{Slot, MAX_PROCESSING_AGE}, feature_set, + fee::FeeDetails, genesis_config::GenesisConfig, hash::Hash, pubkey::Pubkey, @@ -2132,7 +2133,7 @@ pub enum TransactionStatusMessage { pub struct TransactionStatusBatch { pub bank: Arc, pub transactions: Vec, - pub execution_results: Vec>, + pub execution_results: Vec>, pub balances: TransactionBalancesSet, pub token_balances: TransactionTokenBalancesSet, pub rent_debits: Vec, @@ -2165,7 +2166,10 @@ impl TransactionStatusSender { execution_results: execution_results .into_iter() .map(|result| match result { - TransactionExecutionResult::Executed { details, .. } => Some(details), + TransactionExecutionResult::Executed(executed_tx) => Some(( + executed_tx.execution_details, + executed_tx.loaded_transaction.fee_details, + )), TransactionExecutionResult::NotExecuted(_) => None, }) .collect(), @@ -2272,7 +2276,10 @@ pub mod tests { system_transaction, transaction::{Transaction, TransactionError}, }, - solana_svm::transaction_processor::ExecutionRecordingConfig, + solana_svm::{ + account_loader::LoadedTransaction, transaction_processor::ExecutionRecordingConfig, + transaction_results::ExecutedTransaction, + }, solana_vote::vote_account::VoteAccount, solana_vote_program::{ self, @@ -5107,18 +5114,18 @@ pub mod tests { .set_limits(u64::MAX, block_limit, u64::MAX); let txs = vec![tx.clone(), tx]; let results = vec![ - TransactionExecutionResult::Executed { - details: TransactionExecutionDetails { + TransactionExecutionResult::Executed(Box::new(ExecutedTransaction { + loaded_transaction: LoadedTransaction::default(), + execution_details: TransactionExecutionDetails { status: Ok(()), log_messages: None, inner_instructions: None, - fee_details: solana_sdk::fee::FeeDetails::default(), return_data: None, executed_units: actual_execution_cu, accounts_data_len_delta: 0, }, programs_modified_by_tx: HashMap::new(), - }, + })), TransactionExecutionResult::NotExecuted(TransactionError::AccountNotFound), ]; let loaded_accounts_stats = vec![ diff --git a/programs/sbf/tests/programs.rs b/programs/sbf/tests/programs.rs index 99cdba5666b793..ef8743092b2be8 100644 --- a/programs/sbf/tests/programs.rs +++ b/programs/sbf/tests/programs.rs @@ -175,16 +175,16 @@ fn execute_transactions( post_token_balances, )| { match execution_result { - TransactionExecutionResult::Executed { details, .. } => { + TransactionExecutionResult::Executed(executed_tx) => { + let fee_details = executed_tx.loaded_transaction.fee_details; let TransactionExecutionDetails { status, log_messages, inner_instructions, - fee_details, return_data, executed_units, .. - } = details; + } = executed_tx.execution_details; let inner_instructions = inner_instructions.map(|inner_instructions| { map_inner_instructions(inner_instructions).collect() diff --git a/rpc/src/transaction_status_service.rs b/rpc/src/transaction_status_service.rs index 34e9b7fdc6f8b2..5ce1c595417be6 100644 --- a/rpc/src/transaction_status_service.rs +++ b/rpc/src/transaction_status_service.rs @@ -94,14 +94,13 @@ impl TransactionStatusService { rent_debits, transaction_indexes, ) { - if let Some(details) = execution_result { + if let Some((details, fee_details)) = execution_result { let TransactionExecutionDetails { status, log_messages, inner_instructions, return_data, executed_units, - fee_details, .. } = details; let tx_account_locks = transaction.get_account_locks_unchecked(); @@ -325,15 +324,17 @@ pub(crate) mod tests { let mut rent_debits = RentDebits::default(); rent_debits.insert(&pubkey, 123, 456); - let transaction_result = Some(TransactionExecutionDetails { - status: Ok(()), - log_messages: None, - inner_instructions: None, - fee_details: FeeDetails::default(), - return_data: None, - executed_units: 0, - accounts_data_len_delta: 0, - }); + let transaction_result = Some(( + TransactionExecutionDetails { + status: Ok(()), + log_messages: None, + inner_instructions: None, + return_data: None, + executed_units: 0, + accounts_data_len_delta: 0, + }, + FeeDetails::default(), + )); let balances = TransactionBalancesSet { pre_balances: vec![vec![123456]], diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 7898831773ad18..5d35ec182bd4ce 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -58,7 +58,6 @@ use { }, byteorder::{ByteOrder, LittleEndian}, dashmap::{DashMap, DashSet}, - itertools::izip, log::*, rayon::{ iter::{IntoParallelIterator, IntoParallelRefIterator, ParallelIterator}, @@ -158,7 +157,6 @@ use { solana_svm::{ account_loader::{ collect_rent_from_account, CheckedTransactionDetails, TransactionCheckResult, - TransactionLoadResult, }, account_overrides::AccountOverrides, account_saver::collect_accounts_to_store, @@ -324,7 +322,6 @@ impl BankRc { } pub struct LoadAndExecuteTransactionsOutput { - pub loaded_transactions: Vec, // Vector of results indicating whether a transaction was executed or could not // be executed. Note executed transactions can still have failed! pub execution_results: Vec, @@ -3313,7 +3310,6 @@ impl Bank { let mut timings = ExecuteTimings::default(); let LoadAndExecuteTransactionsOutput { - loaded_transactions, mut execution_results, .. } = self.load_and_execute_transactions( @@ -3338,19 +3334,6 @@ impl Bank { }, ); - let post_simulation_accounts = loaded_transactions - .into_iter() - .next() - .and_then(|loaded_transactions_res| loaded_transactions_res.ok()) - .map(|loaded_transaction| { - loaded_transaction - .accounts - .into_iter() - .take(number_of_accounts) - .collect::>() - }) - .unwrap_or_default(); - let units_consumed = timings .details @@ -3370,14 +3353,25 @@ impl Bank { TransactionError::InvalidProgramForExecution, )); let flattened_result = execution_result.flattened_result(); - let (logs, return_data, inner_instructions) = match execution_result { - TransactionExecutionResult::Executed { details, .. } => ( - details.log_messages, - details.return_data, - details.inner_instructions, - ), - TransactionExecutionResult::NotExecuted(_) => (None, None, None), - }; + let (post_simulation_accounts, logs, return_data, inner_instructions) = + match execution_result { + TransactionExecutionResult::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.log_messages, + details.return_data, + details.inner_instructions, + ) + } + TransactionExecutionResult::NotExecuted(_) => (vec![], None, None, None), + }; let logs = logs.unwrap_or_default(); TransactionSimulationResult { @@ -3780,7 +3774,6 @@ impl Bank { } LoadAndExecuteTransactionsOutput { - loaded_transactions: sanitized_output.loaded_transactions, execution_results: sanitized_output.execution_results, retryable_transaction_indexes, executed_transactions_count, @@ -3865,8 +3858,8 @@ impl Bank { let results = execution_results .iter() .map(|execution_result| match execution_result { - TransactionExecutionResult::Executed { details, .. } => { - fees += details.fee_details.total_fee(); + TransactionExecutionResult::Executed(executed_tx) => { + fees += executed_tx.loaded_transaction.fee_details.total_fee(); Ok(()) } TransactionExecutionResult::NotExecuted(err) => Err(err.clone()), @@ -3887,8 +3880,8 @@ impl Bank { let results = execution_results .iter() .map(|execution_result| match execution_result { - TransactionExecutionResult::Executed { details, .. } => { - accumulated_fee_details.accumulate(&details.fee_details); + TransactionExecutionResult::Executed(executed_tx) => { + accumulated_fee_details.accumulate(&executed_tx.loaded_transaction.fee_details); Ok(()) } TransactionExecutionResult::NotExecuted(err) => Err(err.clone()), @@ -3905,8 +3898,7 @@ impl Bank { pub fn commit_transactions( &self, sanitized_txs: &[SanitizedTransaction], - loaded_txs: &mut [TransactionLoadResult], - execution_results: Vec, + mut execution_results: Vec, last_blockhash: Hash, lamports_per_signature: u64, counts: ExecutedTransactionCounts, @@ -3948,8 +3940,7 @@ impl Bank { let durable_nonce = DurableNonce::from_blockhash(&last_blockhash); let (accounts_to_store, transactions) = collect_accounts_to_store( sanitized_txs, - &execution_results, - loaded_txs, + &mut execution_results, &durable_nonce, lamports_per_signature, ); @@ -3957,12 +3948,12 @@ impl Bank { .accounts .store_cached((self.slot(), accounts_to_store.as_slice()), &transactions); } - let rent_debits = self.collect_rent(&execution_results, loaded_txs); + let rent_debits = self.collect_rent(&mut execution_results); // Cached vote and stake accounts are synchronized with accounts-db // after each transaction. let mut update_stakes_cache_time = Measure::start("update_stakes_cache_time"); - self.update_stakes_cache(sanitized_txs, &execution_results, loaded_txs); + self.update_stakes_cache(sanitized_txs, &execution_results); update_stakes_cache_time.stop(); // once committed there is no way to unroll @@ -3977,12 +3968,9 @@ impl Bank { Measure::start("store_executors_which_were_deployed_time"); let mut cache = None; for execution_result in &execution_results { - if let TransactionExecutionResult::Executed { - details, - programs_modified_by_tx, - } = execution_result - { - if details.status.is_ok() && !programs_modified_by_tx.is_empty() { + if let TransactionExecutionResult::Executed(executed_tx) = execution_result { + let programs_modified_by_tx = &executed_tx.programs_modified_by_tx; + if executed_tx.was_successful() && !programs_modified_by_tx.is_empty() { cache .get_or_insert_with(|| { self.transaction_processor.program_cache.write().unwrap() @@ -4030,7 +4018,7 @@ impl Bank { update_transaction_statuses_time.as_us(), ); - let loaded_accounts_stats = Self::collect_loaded_accounts_stats(loaded_txs); + let loaded_accounts_stats = Self::collect_loaded_accounts_stats(&execution_results); assert_eq!( loaded_accounts_stats.len(), execution_results.len(), @@ -4046,38 +4034,40 @@ impl Bank { } fn collect_loaded_accounts_stats( - loaded_txs: &[TransactionLoadResult], + execution_results: &[TransactionExecutionResult], ) -> Vec> { - loaded_txs + execution_results .iter() - .map(|load_result| match load_result { - Ok(loaded_tx) => Ok(TransactionLoadedAccountsStats { - loaded_accounts_data_size: loaded_tx.loaded_accounts_data_size, - loaded_accounts_count: loaded_tx.accounts.len(), - }), - Err(err) => Err(err.clone()), + .map(|execution_result| match execution_result { + TransactionExecutionResult::Executed(executed_tx) => { + let loaded_tx = &executed_tx.loaded_transaction; + Ok(TransactionLoadedAccountsStats { + loaded_accounts_data_size: loaded_tx.loaded_accounts_data_size, + loaded_accounts_count: loaded_tx.accounts.len(), + }) + } + TransactionExecutionResult::NotExecuted(err) => Err(err.clone()), }) .collect() } fn collect_rent( &self, - execution_results: &[TransactionExecutionResult], - loaded_txs: &mut [TransactionLoadResult], + execution_results: &mut [TransactionExecutionResult], ) -> Vec { let mut collected_rent: u64 = 0; - let rent_debits: Vec<_> = loaded_txs + + let rent_debits: Vec<_> = execution_results .iter_mut() - .zip(execution_results) - .map(|(load_result, execution_result)| { - if let (Ok(loaded_transaction), true) = - (load_result, execution_result.was_executed_successfully()) + .map(|execution_result| match execution_result { + TransactionExecutionResult::Executed(executed_tx) + if executed_tx.was_successful() => { + let loaded_transaction = &mut executed_tx.loaded_transaction; collected_rent += loaded_transaction.rent; mem::take(&mut loaded_transaction.rent_debits) - } else { - RentDebits::default() } + _ => RentDebits::default(), }) .collect(); self.collected_rent.fetch_add(collected_rent, Relaxed); @@ -4750,7 +4740,6 @@ impl Bank { }; let LoadAndExecuteTransactionsOutput { - mut loaded_transactions, execution_results, executed_transactions_count, executed_non_vote_transactions_count, @@ -4776,7 +4765,6 @@ impl Bank { self.last_blockhash_and_lamports_per_signature(); let results = self.commit_transactions( batch.sanitized_transactions(), - &mut loaded_transactions, execution_results, last_blockhash, lamports_per_signature, @@ -6035,18 +6023,21 @@ impl Bank { &self, txs: &[SanitizedTransaction], execution_results: &[TransactionExecutionResult], - loaded_txs: &[TransactionLoadResult], ) { debug_assert_eq!(txs.len(), execution_results.len()); - debug_assert_eq!(txs.len(), loaded_txs.len()); let new_warmup_cooldown_rate_epoch = self.new_warmup_cooldown_rate_epoch(); - izip!(txs, execution_results, loaded_txs) - .filter(|(_, execution_result, _)| execution_result.was_executed_successfully()) - .flat_map(|(tx, _, load_result)| { - load_result.iter().flat_map(|loaded_transaction| { - let num_account_keys = tx.message().account_keys().len(); - loaded_transaction.accounts.iter().take(num_account_keys) - }) + txs.iter() + .zip(execution_results) + .filter_map(|(tx, execution_result)| { + execution_result + .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(); + let loaded_tx = &executed_tx.loaded_transaction; + loaded_tx.accounts.iter().take(num_account_keys) }) .for_each(|(pubkey, account)| { // note that this could get timed to: self.rc.accounts.accounts_db.stats.stakes_cache_check_and_store_us, diff --git a/runtime/src/bank/tests.rs b/runtime/src/bank/tests.rs index 925c126c9afefe..07914d8702a2d5 100644 --- a/runtime/src/bank/tests.rs +++ b/runtime/src/bank/tests.rs @@ -100,7 +100,10 @@ use { transaction_context::TransactionAccount, }, solana_stake_program::stake_state::{self, StakeStateV2}, - solana_svm::nonce_info::NoncePartial, + solana_svm::{ + account_loader::LoadedTransaction, nonce_info::NoncePartial, + transaction_results::ExecutedTransaction, + }, solana_timings::ExecuteTimings, solana_vote_program::{ vote_instruction, @@ -229,18 +232,21 @@ fn test_race_register_tick_freeze() { } fn new_execution_result(status: Result<()>, fee_details: FeeDetails) -> TransactionExecutionResult { - TransactionExecutionResult::Executed { - details: TransactionExecutionDetails { + TransactionExecutionResult::Executed(Box::new(ExecutedTransaction { + loaded_transaction: LoadedTransaction { + fee_details, + ..LoadedTransaction::default() + }, + execution_details: TransactionExecutionDetails { status, log_messages: None, inner_instructions: None, - fee_details, return_data: None, executed_units: 0, accounts_data_len_delta: 0, }, programs_modified_by_tx: HashMap::new(), - } + })) } impl Bank { @@ -5876,18 +5882,15 @@ fn test_pre_post_transaction_balances() { // Failed transactions still produce balance sets // This is an InstructionError - fees charged - assert_matches!( - transaction_results.execution_results[2], - TransactionExecutionResult::Executed { - details: TransactionExecutionDetails { - status: Err(TransactionError::InstructionError( - 0, - InstructionError::Custom(1), - )), - .. - }, - .. - } + let executed_tx = transaction_results.execution_results[2] + .executed_transaction() + .unwrap(); + assert_eq!( + executed_tx.execution_details.status, + Err(TransactionError::InstructionError( + 0, + InstructionError::Custom(1), + )), ); assert_eq!( transaction_balances_set.pre_balances[2], diff --git a/svm/doc/spec.md b/svm/doc/spec.md index 8f148b557debf0..a851b051bb909a 100644 --- a/svm/doc/spec.md +++ b/svm/doc/spec.md @@ -209,8 +209,6 @@ The output of the transaction batch processor's - `execution_results`: Vector of results indicating whether a transaction was executed or could not be executed. Note executed transactions can still have failed! -- `loaded_transactions`: Vector of loaded transactions from transactions that - were processed. # Functional Model @@ -232,12 +230,6 @@ In bank context `load_and_execute_sanitized_transactions` is called from from `load_execute_and_commit_transactions` which receives a batch of transactions from its caller. -Multiple results of `load_and_execute_sanitized_transactions` are aggregated in -the struct `LoadAndExecuteSanitizedTransactionsOutput` - - `LoadAndExecuteSanitizedTransactionsOutput` contains - - vector of `TransactionLoadResult` - - vector of `TransactionExecutionResult` - Steps of `load_and_execute_sanitized_transactions` 1. Steps of preparation for execution diff --git a/svm/src/account_loader.rs b/svm/src/account_loader.rs index 8c2fb73d8925af..20c7dab99f0552 100644 --- a/svm/src/account_loader.rs +++ b/svm/src/account_loader.rs @@ -52,6 +52,7 @@ pub struct ValidatedTransactionDetails { } #[derive(PartialEq, Eq, Debug, Clone)] +#[cfg_attr(feature = "dev-context-only-utils", derive(Default))] pub struct LoadedTransaction { pub accounts: Vec, pub program_indices: TransactionProgramIndices, diff --git a/svm/src/account_saver.rs b/svm/src/account_saver.rs index 2fcfe2a3883009..3520af5e1771d5 100644 --- a/svm/src/account_saver.rs +++ b/svm/src/account_saver.rs @@ -1,7 +1,7 @@ use { crate::{ - account_loader::TransactionLoadResult, nonce_info::NonceInfo, - rollback_accounts::RollbackAccounts, transaction_results::TransactionExecutionResult, + nonce_info::NonceInfo, rollback_accounts::RollbackAccounts, + transaction_results::TransactionExecutionResult, }, solana_sdk::{ account::AccountSharedData, @@ -18,27 +18,25 @@ use { pub fn collect_accounts_to_store<'a>( txs: &'a [SanitizedTransaction], - execution_results: &'a [TransactionExecutionResult], - load_results: &'a mut [TransactionLoadResult], + execution_results: &'a mut [TransactionExecutionResult], durable_nonce: &DurableNonce, lamports_per_signature: u64, ) -> ( Vec<(&'a Pubkey, &'a AccountSharedData)>, Vec>, ) { - let mut accounts = Vec::with_capacity(load_results.len()); - let mut transactions = Vec::with_capacity(load_results.len()); - for (i, (tx_load_result, tx)) in load_results.iter_mut().zip(txs).enumerate() { - let Ok(loaded_transaction) = tx_load_result else { - // Don't store any accounts if tx failed to load + // TODO: calculate a better initial capacity for these vectors. The current + // usage of the length of execution results is far from accurate. + let mut accounts = Vec::with_capacity(execution_results.len()); + let mut transactions = Vec::with_capacity(execution_results.len()); + for (execution_result, tx) in execution_results.iter_mut().zip(txs) { + let Some(executed_tx) = execution_result.executed_transaction_mut() else { + // Don't store any accounts if tx wasn't executed continue; }; - let execution_status = match &execution_results[i] { - TransactionExecutionResult::Executed { details, .. } => &details.status, - // Don't store any accounts if tx wasn't executed - TransactionExecutionResult::NotExecuted(_) => continue, - }; + let loaded_transaction = &mut executed_tx.loaded_transaction; + let execution_status = &executed_tx.execution_details.status; // Accounts that are invoked and also not passed as an instruction // account to a program don't need to be stored because it's assumed @@ -139,8 +137,9 @@ mod tests { use { super::*, crate::{ - account_loader::LoadedTransaction, nonce_info::NoncePartial, - transaction_results::TransactionExecutionDetails, + account_loader::LoadedTransaction, + nonce_info::NoncePartial, + transaction_results::{ExecutedTransaction, TransactionExecutionDetails}, }, solana_compute_budget::compute_budget_processor::ComputeBudgetLimits, solana_sdk::{ @@ -172,19 +171,22 @@ mod tests { )) } - fn new_execution_result(status: Result<()>) -> TransactionExecutionResult { - TransactionExecutionResult::Executed { - details: TransactionExecutionDetails { + fn new_execution_result( + status: Result<()>, + loaded_transaction: LoadedTransaction, + ) -> TransactionExecutionResult { + TransactionExecutionResult::Executed(Box::new(ExecutedTransaction { + execution_details: TransactionExecutionDetails { status, log_messages: None, inner_instructions: None, - fee_details: FeeDetails::default(), return_data: None, executed_units: 0, accounts_data_len_delta: 0, }, + loaded_transaction, programs_modified_by_tx: HashMap::new(), - } + })) } #[test] @@ -226,7 +228,7 @@ mod tests { ]; let tx1 = new_sanitized_tx(&[&keypair1], message, Hash::default()); - let loaded0 = Ok(LoadedTransaction { + let loaded0 = LoadedTransaction { accounts: transaction_accounts0, program_indices: vec![], fee_details: FeeDetails::default(), @@ -235,9 +237,9 @@ mod tests { rent: 0, rent_debits: RentDebits::default(), loaded_accounts_data_size: 0, - }); + }; - let loaded1 = Ok(LoadedTransaction { + let loaded1 = LoadedTransaction { accounts: transaction_accounts1, program_indices: vec![], fee_details: FeeDetails::default(), @@ -246,19 +248,15 @@ mod tests { rent: 0, rent_debits: RentDebits::default(), loaded_accounts_data_size: 0, - }); - - let mut loaded = vec![loaded0, loaded1]; + }; let txs = vec![tx0.clone(), tx1.clone()]; - let execution_results = vec![new_execution_result(Ok(())); 2]; - let (collected_accounts, transactions) = collect_accounts_to_store( - &txs, - &execution_results, - loaded.as_mut_slice(), - &DurableNonce::default(), - 0, - ); + let mut execution_results = vec![ + new_execution_result(Ok(()), loaded0), + new_execution_result(Ok(()), loaded1), + ]; + let (collected_accounts, transactions) = + collect_accounts_to_store(&txs, &mut execution_results, &DurableNonce::default(), 0); assert_eq!(collected_accounts.len(), 2); assert!(collected_accounts .iter() @@ -443,7 +441,7 @@ mod tests { let from_account_pre = AccountSharedData::new(4242, 0, &Pubkey::default()); let nonce = NoncePartial::new(nonce_address, nonce_account_pre.clone()); - let loaded = Ok(LoadedTransaction { + let loaded = LoadedTransaction { accounts: transaction_accounts, program_indices: vec![], fee_details: FeeDetails::default(), @@ -455,22 +453,19 @@ mod tests { rent: 0, rent_debits: RentDebits::default(), loaded_accounts_data_size: 0, - }); - - let mut loaded = vec![loaded]; + }; let durable_nonce = DurableNonce::from_blockhash(&Hash::new_unique()); let txs = vec![tx]; - let execution_results = vec![new_execution_result(Err( - TransactionError::InstructionError(1, InstructionError::InvalidArgument), - ))]; - let (collected_accounts, _) = collect_accounts_to_store( - &txs, - &execution_results, - loaded.as_mut_slice(), - &durable_nonce, - 0, - ); + let mut execution_results = vec![new_execution_result( + Err(TransactionError::InstructionError( + 1, + InstructionError::InvalidArgument, + )), + loaded, + )]; + let (collected_accounts, _) = + collect_accounts_to_store(&txs, &mut execution_results, &durable_nonce, 0); assert_eq!(collected_accounts.len(), 2); assert_eq!( collected_accounts @@ -543,7 +538,7 @@ mod tests { AccountSharedData::new_data(42, &nonce_state, &system_program::id()).unwrap(); let nonce = NoncePartial::new(nonce_address, nonce_account_pre.clone()); - let loaded = Ok(LoadedTransaction { + let loaded = LoadedTransaction { accounts: transaction_accounts, program_indices: vec![], fee_details: FeeDetails::default(), @@ -554,22 +549,19 @@ mod tests { rent: 0, rent_debits: RentDebits::default(), loaded_accounts_data_size: 0, - }); - - let mut loaded = vec![loaded]; + }; let durable_nonce = DurableNonce::from_blockhash(&Hash::new_unique()); let txs = vec![tx]; - let execution_results = vec![new_execution_result(Err( - TransactionError::InstructionError(1, InstructionError::InvalidArgument), - ))]; - let (collected_accounts, _) = collect_accounts_to_store( - &txs, - &execution_results, - loaded.as_mut_slice(), - &durable_nonce, - 0, - ); + let mut execution_results = vec![new_execution_result( + Err(TransactionError::InstructionError( + 1, + InstructionError::InvalidArgument, + )), + loaded, + )]; + let (collected_accounts, _) = + collect_accounts_to_store(&txs, &mut execution_results, &durable_nonce, 0); assert_eq!(collected_accounts.len(), 1); let collected_nonce_account = collected_accounts .iter() diff --git a/svm/src/transaction_processor.rs b/svm/src/transaction_processor.rs index c3600772b80091..a936f94710c21c 100644 --- a/svm/src/transaction_processor.rs +++ b/svm/src/transaction_processor.rs @@ -5,7 +5,7 @@ use { account_loader::{ collect_rent_from_account, load_accounts, validate_fee_payer, CheckedTransactionDetails, LoadedTransaction, TransactionCheckResult, - TransactionLoadResult, TransactionValidationResult, ValidatedTransactionDetails, + TransactionValidationResult, ValidatedTransactionDetails, }, account_overrides::AccountOverrides, message_processor::MessageProcessor, @@ -14,7 +14,9 @@ use { transaction_account_state_info::TransactionAccountStateInfo, transaction_error_metrics::TransactionErrorMetrics, transaction_processing_callback::TransactionProcessingCallback, - transaction_results::{TransactionExecutionDetails, TransactionExecutionResult}, + transaction_results::{ + ExecutedTransaction, TransactionExecutionDetails, TransactionExecutionResult, + }, }, log::debug, percentage::Percentage, @@ -76,8 +78,6 @@ pub struct LoadAndExecuteSanitizedTransactionsOutput { // Vector of results indicating whether a transaction was executed or could not // be executed. Note executed transactions can still have failed! pub execution_results: Vec, - // Vector of loaded transactions from transactions that were processed. - pub loaded_transactions: Vec, } /// Configuration of the recording capabilities for transaction execution @@ -274,21 +274,19 @@ impl TransactionBatchProcessor { if program_cache_for_tx_batch.borrow().hit_max_limit { const ERROR: TransactionError = TransactionError::ProgramCacheHitMaxLimit; - let loaded_transactions = vec![Err(ERROR); sanitized_txs.len()]; let execution_results = vec![TransactionExecutionResult::NotExecuted(ERROR); sanitized_txs.len()]; return LoadAndExecuteSanitizedTransactionsOutput { error_metrics, execute_timings, execution_results, - loaded_transactions, }; } program_cache_for_tx_batch }); - let (mut loaded_transactions, load_accounts_us) = measure_us!(load_accounts( + let (loaded_transactions, load_accounts_us) = measure_us!(load_accounts( callbacks, sanitized_txs, validation_results, @@ -303,12 +301,12 @@ impl TransactionBatchProcessor { let (execution_results, execution_us): (Vec, u64) = measure_us!(loaded_transactions - .iter_mut() + .into_iter() .zip(sanitized_txs.iter()) .map(|(load_result, tx)| match load_result { Err(e) => TransactionExecutionResult::NotExecuted(e.clone()), Ok(loaded_transaction) => { - let result = self.execute_loaded_transaction( + let executed_tx = self.execute_loaded_transaction( tx, loaded_transaction, &mut execute_timings, @@ -318,21 +316,15 @@ impl TransactionBatchProcessor { config, ); - if let TransactionExecutionResult::Executed { - details, - programs_modified_by_tx, - } = &result - { - // Update batch specific cache of the loaded programs with the modifications - // made by the transaction, if it executed successfully. - if details.status.is_ok() { - program_cache_for_tx_batch - .borrow_mut() - .merge(programs_modified_by_tx); - } + // Update batch specific cache of the loaded programs with the modifications + // made by the transaction, if it executed successfully. + if executed_tx.was_successful() { + program_cache_for_tx_batch + .borrow_mut() + .merge(&executed_tx.programs_modified_by_tx); } - result + TransactionExecutionResult::Executed(Box::new(executed_tx)) } }) .collect()); @@ -372,7 +364,6 @@ impl TransactionBatchProcessor { error_metrics, execute_timings, execution_results, - loaded_transactions, } } @@ -704,13 +695,13 @@ impl TransactionBatchProcessor { fn execute_loaded_transaction( &self, tx: &SanitizedTransaction, - loaded_transaction: &mut LoadedTransaction, + mut loaded_transaction: LoadedTransaction, execute_timings: &mut ExecuteTimings, error_metrics: &mut TransactionErrorMetrics, program_cache_for_tx_batch: &mut ProgramCacheForTxBatch, environment: &TransactionProcessingEnvironment, config: &TransactionProcessingConfig, - ) -> TransactionExecutionResult { + ) -> ExecutedTransaction { let transaction_accounts = std::mem::take(&mut loaded_transaction.accounts); fn transaction_accounts_lamports_sum( @@ -874,16 +865,16 @@ impl TransactionBatchProcessor { None }; - TransactionExecutionResult::Executed { - details: TransactionExecutionDetails { + ExecutedTransaction { + execution_details: TransactionExecutionDetails { status, log_messages, inner_instructions, - fee_details: loaded_transaction.fee_details, return_data, executed_units, accounts_data_len_delta, }, + loaded_transaction, programs_modified_by_tx: program_cache_for_tx_batch.drain_modified_entries(), } } @@ -1137,7 +1128,7 @@ mod tests { false, ); - let mut loaded_transaction = LoadedTransaction { + let loaded_transaction = LoadedTransaction { accounts: vec![(Pubkey::new_unique(), AccountSharedData::default())], program_indices: vec![vec![0]], fee_details: FeeDetails::default(), @@ -1153,59 +1144,38 @@ mod tests { let mut processing_config = TransactionProcessingConfig::default(); processing_config.recording_config.enable_log_recording = true; - let result = batch_processor.execute_loaded_transaction( + let executed_tx = batch_processor.execute_loaded_transaction( &sanitized_transaction, - &mut loaded_transaction, + loaded_transaction.clone(), &mut ExecuteTimings::default(), &mut TransactionErrorMetrics::default(), &mut program_cache_for_tx_batch, &processing_environment, &processing_config, ); - - let TransactionExecutionResult::Executed { - details: TransactionExecutionDetails { log_messages, .. }, - .. - } = result - else { - panic!("Unexpected result") - }; - assert!(log_messages.is_some()); + assert!(executed_tx.execution_details.log_messages.is_some()); processing_config.log_messages_bytes_limit = Some(2); - let result = batch_processor.execute_loaded_transaction( + let executed_tx = batch_processor.execute_loaded_transaction( &sanitized_transaction, - &mut loaded_transaction, + loaded_transaction.clone(), &mut ExecuteTimings::default(), &mut TransactionErrorMetrics::default(), &mut program_cache_for_tx_batch, &processing_environment, &processing_config, ); - - let TransactionExecutionResult::Executed { - details: - TransactionExecutionDetails { - log_messages, - inner_instructions, - .. - }, - .. - } = result - else { - panic!("Unexpected result") - }; - assert!(log_messages.is_some()); - assert!(inner_instructions.is_none()); + assert!(executed_tx.execution_details.log_messages.is_some()); + assert!(executed_tx.execution_details.inner_instructions.is_none()); processing_config.recording_config.enable_log_recording = false; processing_config.recording_config.enable_cpi_recording = true; processing_config.log_messages_bytes_limit = None; - let result = batch_processor.execute_loaded_transaction( + let executed_tx = batch_processor.execute_loaded_transaction( &sanitized_transaction, - &mut loaded_transaction, + loaded_transaction, &mut ExecuteTimings::default(), &mut TransactionErrorMetrics::default(), &mut program_cache_for_tx_batch, @@ -1213,20 +1183,8 @@ mod tests { &processing_config, ); - let TransactionExecutionResult::Executed { - details: - TransactionExecutionDetails { - log_messages, - inner_instructions, - .. - }, - .. - } = result - else { - panic!("Unexpected result") - }; - assert!(log_messages.is_none()); - assert!(inner_instructions.is_some()); + assert!(executed_tx.execution_details.log_messages.is_none()); + assert!(executed_tx.execution_details.inner_instructions.is_some()); } #[test] @@ -1259,7 +1217,7 @@ mod tests { let mut account_data = AccountSharedData::default(); account_data.set_owner(bpf_loader::id()); - let mut loaded_transaction = LoadedTransaction { + let loaded_transaction = LoadedTransaction { accounts: vec![ (key1, AccountSharedData::default()), (key2, AccountSharedData::default()), @@ -1281,7 +1239,7 @@ mod tests { let _ = batch_processor.execute_loaded_transaction( &sanitized_transaction, - &mut loaded_transaction, + loaded_transaction, &mut ExecuteTimings::default(), &mut error_metrics, &mut program_cache_for_tx_batch, diff --git a/svm/src/transaction_results.rs b/svm/src/transaction_results.rs index c11896d671b166..8071bf11ac47c9 100644 --- a/svm/src/transaction_results.rs +++ b/svm/src/transaction_results.rs @@ -5,10 +5,10 @@ )] pub use solana_sdk::inner_instruction::{InnerInstruction, InnerInstructionsList}; use { + crate::account_loader::LoadedTransaction, serde::{Deserialize, Serialize}, solana_program_runtime::loaded_programs::ProgramCacheEntry, solana_sdk::{ - fee::FeeDetails, pubkey::Pubkey, rent_debits::RentDebits, transaction::{self, TransactionError}, @@ -42,39 +42,57 @@ pub struct TransactionLoadedAccountsStats { /// make such checks hard to do incorrectly. #[derive(Debug, Clone)] pub enum TransactionExecutionResult { - Executed { - details: TransactionExecutionDetails, - programs_modified_by_tx: HashMap>, - }, + Executed(Box), NotExecuted(TransactionError), } +#[derive(Debug, Clone)] +pub struct ExecutedTransaction { + pub loaded_transaction: LoadedTransaction, + pub execution_details: TransactionExecutionDetails, + pub programs_modified_by_tx: HashMap>, +} + +impl ExecutedTransaction { + pub fn was_successful(&self) -> bool { + self.execution_details.status.is_ok() + } +} + impl TransactionExecutionResult { pub fn was_executed_successfully(&self) -> bool { - match self { - Self::Executed { details, .. } => details.status.is_ok(), - Self::NotExecuted { .. } => false, - } + self.executed_transaction() + .map(|executed_tx| executed_tx.was_successful()) + .unwrap_or(false) } pub fn was_executed(&self) -> bool { + self.executed_transaction().is_some() + } + + pub fn details(&self) -> Option<&TransactionExecutionDetails> { + self.executed_transaction() + .map(|executed_tx| &executed_tx.execution_details) + } + + pub fn flattened_result(&self) -> transaction::Result<()> { match self { - Self::Executed { .. } => true, - Self::NotExecuted(_) => false, + Self::Executed(executed_tx) => executed_tx.execution_details.status.clone(), + Self::NotExecuted(err) => Err(err.clone()), } } - pub fn details(&self) -> Option<&TransactionExecutionDetails> { + pub fn executed_transaction(&self) -> Option<&ExecutedTransaction> { match self { - Self::Executed { details, .. } => Some(details), - Self::NotExecuted(_) => None, + Self::Executed(executed_tx) => Some(executed_tx.as_ref()), + Self::NotExecuted { .. } => None, } } - pub fn flattened_result(&self) -> transaction::Result<()> { + pub fn executed_transaction_mut(&mut self) -> Option<&mut ExecutedTransaction> { match self { - Self::Executed { details, .. } => details.status.clone(), - Self::NotExecuted(err) => Err(err.clone()), + Self::Executed(executed_tx) => Some(executed_tx.as_mut()), + Self::NotExecuted { .. } => None, } } } @@ -84,7 +102,6 @@ pub struct TransactionExecutionDetails { pub status: transaction::Result<()>, pub log_messages: Option>, pub inner_instructions: Option, - pub fee_details: FeeDetails, pub return_data: Option, pub executed_units: u64, /// The change in accounts data len for this transaction. diff --git a/svm/tests/conformance.rs b/svm/tests/conformance.rs index 7e495f98484e92..8e82cf98623837 100644 --- a/svm/tests/conformance.rs +++ b/svm/tests/conformance.rs @@ -344,9 +344,11 @@ fn run_fixture(fixture: InstrFixture, filename: OsString, execute_as_instr: bool return; } - let execution_details = result.execution_results[0].details().unwrap(); + let executed_tx = result.execution_results[0].executed_transaction().unwrap(); + let execution_details = &executed_tx.execution_details; + let loaded_accounts = &executed_tx.loaded_transaction.accounts; verify_accounts_and_data( - &result.loaded_transactions[0].as_ref().unwrap().accounts, + loaded_accounts, output, execution_details.executed_units, input.cu_avail, diff --git a/svm/tests/integration_test.rs b/svm/tests/integration_test.rs index f0981e5dff108a..2a8384bd10f7b0 100644 --- a/svm/tests/integration_test.rs +++ b/svm/tests/integration_test.rs @@ -430,39 +430,32 @@ fn svm_integration() { ); assert_eq!(result.execution_results.len(), 5); - assert!(result.execution_results[0] - .details() - .unwrap() - .status - .is_ok()); - let logs = result.execution_results[0] - .details() - .unwrap() + + let executed_tx_0 = result.execution_results[0].executed_transaction().unwrap(); + assert!(executed_tx_0.was_successful()); + let logs = executed_tx_0 + .execution_details .log_messages .as_ref() .unwrap(); assert!(logs.contains(&"Program log: Hello, Solana!".to_string())); - assert!(result.execution_results[1] - .details() - .unwrap() - .status - .is_ok()); + let executed_tx_1 = result.execution_results[1].executed_transaction().unwrap(); + assert!(executed_tx_1.was_successful()); // The SVM does not commit the account changes in MockBank let recipient_key = transactions[1].message().account_keys()[2]; - let recipient_data = result.loaded_transactions[1] - .as_ref() - .unwrap() + let recipient_data = executed_tx_1 + .loaded_transaction .accounts .iter() .find(|key| key.0 == recipient_key) .unwrap(); assert_eq!(recipient_data.1.lamports(), 900010); - let return_data = result.execution_results[2] - .details() - .unwrap() + let executed_tx_2 = result.execution_results[2].executed_transaction().unwrap(); + let return_data = executed_tx_2 + .execution_details .return_data .as_ref() .unwrap(); @@ -471,14 +464,10 @@ fn svm_integration() { let clock_info: Clock = bincode::deserialize(clock_data.data()).unwrap(); assert_eq!(clock_info.unix_timestamp, time); - assert!(result.execution_results[3] - .details() - .unwrap() - .status - .is_err()); - assert!(result.execution_results[3] - .details() - .unwrap() + let executed_tx_3 = result.execution_results[3].executed_transaction().unwrap(); + assert!(executed_tx_3.execution_details.status.is_err()); + assert!(executed_tx_3 + .execution_details .log_messages .as_ref() .unwrap()