From e8a5f54927995ee4ad52fafe284fcb16956a6a92 Mon Sep 17 00:00:00 2001 From: Justin Starry Date: Fri, 9 Aug 2024 12:24:12 +0000 Subject: [PATCH] refactor: committed tx may not have been executed --- core/src/banking_stage/committer.rs | 2 +- ledger-tool/src/main.rs | 6 +-- ledger/src/blockstore_processor.rs | 23 ++++------ programs/sbf/tests/programs.rs | 21 ++++----- rpc/src/transaction_status_service.rs | 34 +++++--------- runtime/src/bank.rs | 59 ++++++++++++++----------- runtime/src/bank/bank_hash_details.rs | 31 ++++++++++++- runtime/src/bank/tests.rs | 11 +---- svm/src/transaction_commit_result.rs | 24 +++++----- svm/src/transaction_execution_result.rs | 9 +++- 10 files changed, 113 insertions(+), 107 deletions(-) diff --git a/core/src/banking_stage/committer.rs b/core/src/banking_stage/committer.rs index c020c92866dec0..ca62ebe8f46389 100644 --- a/core/src/banking_stage/committer.rs +++ b/core/src/banking_stage/committer.rs @@ -101,7 +101,7 @@ impl Committer { // transaction committed to block. qos_service uses these information to adjust // reserved block space. Ok(committed_tx) => CommitTransactionDetails::Committed { - compute_units: committed_tx.execution_details.executed_units, + compute_units: committed_tx.executed_units, loaded_accounts_data_size: committed_tx .loaded_account_stats .loaded_accounts_data_size, diff --git a/ledger-tool/src/main.rs b/ledger-tool/src/main.rs index 1005f30a71cd65..acb5762ef1a15e 100644 --- a/ledger-tool/src/main.rs +++ b/ledger-tool/src/main.rs @@ -708,16 +708,14 @@ fn record_transactions( .collect(); let is_simple_vote_tx = tx.is_simple_vote_transaction(); - let execution_results = commit_result - .ok() - .map(|committed_tx| committed_tx.execution_details); + let commit_details = commit_result.ok().map(|committed_tx| committed_tx.into()); TransactionDetails { signature: tx.signature().to_string(), accounts, instructions, is_simple_vote_tx, - execution_results, + commit_details, index, } }) diff --git a/ledger/src/blockstore_processor.rs b/ledger/src/blockstore_processor.rs index 8d8211e02a07c1..3ff6bb8988c04b 100644 --- a/ledger/src/blockstore_processor.rs +++ b/ledger/src/blockstore_processor.rs @@ -238,7 +238,7 @@ fn check_block_cost_limits( if let Ok(committed_tx) = commit_result { Some(CostModel::calculate_cost_for_executed_transaction( tx, - committed_tx.execution_details.executed_units, + committed_tx.executed_units, committed_tx.loaded_account_stats.loaded_accounts_data_size, &bank.feature_set, )) @@ -2245,9 +2245,7 @@ pub mod tests { }, solana_svm::{ transaction_commit_result::CommittedTransaction, - transaction_execution_result::{ - TransactionExecutionDetails, TransactionLoadedAccountsStats, - }, + transaction_execution_result::TransactionLoadedAccountsStats, transaction_processor::ExecutionRecordingConfig, }, solana_vote::vote_account::VoteAccount, @@ -5079,20 +5077,17 @@ pub mod tests { let txs = vec![tx.clone(), tx]; let commit_results = vec![ Ok(CommittedTransaction { + status: Ok(()), + log_messages: None, + inner_instructions: None, + return_data: None, + executed_units: actual_execution_cu, + fee_details: FeeDetails::default(), + rent_debits: RentDebits::default(), loaded_account_stats: TransactionLoadedAccountsStats { loaded_accounts_data_size: actual_loaded_accounts_data_size, loaded_accounts_count: 2, }, - execution_details: TransactionExecutionDetails { - status: Ok(()), - log_messages: None, - inner_instructions: None, - return_data: None, - executed_units: actual_execution_cu, - accounts_data_len_delta: 0, - }, - fee_details: FeeDetails::default(), - rent_debits: RentDebits::default(), }), Err(TransactionError::AccountNotFound), ]; diff --git a/programs/sbf/tests/programs.rs b/programs/sbf/tests/programs.rs index f9afcf03013566..2290c5e5cc1c19 100644 --- a/programs/sbf/tests/programs.rs +++ b/programs/sbf/tests/programs.rs @@ -63,7 +63,7 @@ use { }, solana_svm::{ transaction_commit_result::CommittedTransaction, - transaction_execution_result::{InnerInstruction, TransactionExecutionDetails}, + transaction_execution_result::InnerInstruction, transaction_processor::ExecutionRecordingConfig, }, solana_timings::ExecuteTimings, @@ -107,12 +107,12 @@ fn process_transaction_and_record_inner( None, ) .0; - let TransactionExecutionDetails { + let CommittedTransaction { inner_instructions, log_messages, status, .. - } = commit_results.swap_remove(0).unwrap().execution_details; + } = 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) @@ -163,16 +163,12 @@ fn execute_transactions( )| { commit_result.map(|committed_tx| { let CommittedTransaction { + status, + log_messages, + inner_instructions, + return_data, + executed_units, fee_details, - execution_details: - TransactionExecutionDetails { - status, - log_messages, - inner_instructions, - return_data, - executed_units, - .. - }, .. } = committed_tx; @@ -5244,7 +5240,6 @@ fn test_function_call_args() { let return_data = &result[0] .as_ref() .unwrap() - .execution_details .return_data .as_ref() .unwrap() diff --git a/rpc/src/transaction_status_service.rs b/rpc/src/transaction_status_service.rs index 49a78f22db7752..fa9e4aa69f4fd1 100644 --- a/rpc/src/transaction_status_service.rs +++ b/rpc/src/transaction_status_service.rs @@ -6,10 +6,7 @@ use { blockstore::Blockstore, blockstore_processor::{TransactionStatusBatch, TransactionStatusMessage}, }, - solana_svm::{ - transaction_commit_result::CommittedTransaction, - transaction_execution_result::TransactionExecutionDetails, - }, + solana_svm::transaction_commit_result::CommittedTransaction, solana_transaction_status::{ extract_and_fmt_memos, map_inner_instructions, Reward, TransactionStatusMeta, }, @@ -96,15 +93,11 @@ impl TransactionStatusService { ) { if let Ok(committed_tx) = commit_result { let CommittedTransaction { - execution_details: - TransactionExecutionDetails { - status, - log_messages, - inner_instructions, - return_data, - executed_units, - .. - }, + status, + log_messages, + inner_instructions, + return_data, + executed_units, fee_details, rent_debits, .. @@ -332,17 +325,14 @@ pub(crate) mod tests { rent_debits.insert(&pubkey, 123, 456); let commit_result = Ok(CommittedTransaction { - loaded_account_stats: TransactionLoadedAccountsStats::default(), - execution_details: TransactionExecutionDetails { - status: Ok(()), - log_messages: None, - inner_instructions: None, - return_data: None, - executed_units: 0, - accounts_data_len_delta: 0, - }, + status: Ok(()), + log_messages: None, + inner_instructions: None, + return_data: None, + executed_units: 0, fee_details: FeeDetails::default(), rent_debits, + loaded_account_stats: TransactionLoadedAccountsStats::default(), }); let balances = TransactionBalancesSet { diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 6b93ec643ee031..e1ff8853d43c1d 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -149,7 +149,7 @@ use { stake_state::StakeStateV2, }, solana_svm::{ - account_loader::collect_rent_from_account, + account_loader::{collect_rent_from_account, LoadedTransaction}, account_overrides::AccountOverrides, account_saver::collect_accounts_to_store, transaction_commit_result::{CommittedTransaction, TransactionCommitResult}, @@ -3859,29 +3859,37 @@ impl Bank { ) -> Vec { processing_results .into_iter() - .map(|processing_result| match processing_result { - Ok(processed_tx) => { - let loaded_tx = &processed_tx.loaded_transaction; - let loaded_account_stats = TransactionLoadedAccountsStats { - loaded_accounts_data_size: loaded_tx.loaded_accounts_data_size, - loaded_accounts_count: loaded_tx.accounts.len(), - }; - - // Rent is only collected for successfully executed transactions - let rent_debits = if processed_tx.was_successful() { - processed_tx.loaded_transaction.rent_debits - } else { - RentDebits::default() - }; + .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() + }; - Ok(CommittedTransaction { - loaded_account_stats, - execution_details: processed_tx.execution_details, - fee_details: processed_tx.loaded_transaction.fee_details, - rent_debits, - }) - } - Err(err) => Err(err), + 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, + }, + }) }) .collect() } @@ -4615,7 +4623,7 @@ impl Bank { pub fn process_transaction_with_metadata( &self, tx: impl Into, - ) -> Result { + ) -> Result { let txs = vec![tx.into()]; let batch = self.prepare_entry_batch(txs)?; @@ -4632,8 +4640,7 @@ impl Bank { Some(1000 * 1000), ); - let committed_tx = commit_results.remove(0)?; - Ok(committed_tx.execution_details) + commit_results.remove(0) } /// Process multiple transaction in a single batch. This is used for benches and unit tests. diff --git a/runtime/src/bank/bank_hash_details.rs b/runtime/src/bank/bank_hash_details.rs index a6c449a50c72b8..5ab13d85c4d89b 100644 --- a/runtime/src/bank/bank_hash_details.rs +++ b/runtime/src/bank/bank_hash_details.rs @@ -15,10 +15,14 @@ use { solana_sdk::{ account::{Account, AccountSharedData, ReadableAccount}, clock::{Epoch, Slot}, + fee::FeeDetails, hash::Hash, + inner_instruction::InnerInstructionsList, pubkey::Pubkey, + transaction::Result as TransactionResult, + transaction_context::TransactionReturnData, }, - solana_svm::transaction_execution_result::TransactionExecutionDetails, + solana_svm::transaction_commit_result::CommittedTransaction, solana_transaction_status::UiInstruction, std::str::FromStr, }; @@ -74,7 +78,30 @@ pub struct TransactionDetails { pub accounts: Vec, pub instructions: Vec, pub is_simple_vote_tx: bool, - pub execution_results: Option, + pub commit_details: Option, +} + +#[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize)] +pub struct TransactionCommitDetails { + pub status: TransactionResult<()>, + pub log_messages: Option>, + pub inner_instructions: Option, + pub return_data: Option, + pub executed_units: u64, + pub fee_details: FeeDetails, +} + +impl From for TransactionCommitDetails { + fn from(committed_tx: CommittedTransaction) -> Self { + Self { + status: committed_tx.status, + log_messages: committed_tx.log_messages, + inner_instructions: committed_tx.inner_instructions, + return_data: committed_tx.return_data, + executed_units: committed_tx.executed_units, + fee_details: committed_tx.fee_details, + } + } } /// The components that go into a bank hash calculation for a single bank/slot. diff --git a/runtime/src/bank/tests.rs b/runtime/src/bank/tests.rs index a46ae1f8578342..85d51a10f8e2e9 100644 --- a/runtime/src/bank/tests.rs +++ b/runtime/src/bank/tests.rs @@ -5732,7 +5732,7 @@ fn test_pre_post_transaction_balances() { // Failed transactions still produce balance sets // This is an InstructionError - fees charged assert_eq!( - commit_results[2].transaction_result(), + commit_results[2].as_ref().unwrap().status, Err(TransactionError::InstructionError( 0, InstructionError::Custom(1), @@ -9090,7 +9090,6 @@ fn test_tx_log_order() { assert!(commit_results[0] .as_ref() .unwrap() - .execution_details .log_messages .as_ref() .unwrap()[1] @@ -9099,7 +9098,6 @@ fn test_tx_log_order() { assert!(commit_results[1] .as_ref() .unwrap() - .execution_details .log_messages .as_ref() .unwrap()[2] @@ -9193,12 +9191,7 @@ fn test_tx_return_data() { None, ) .0; - let return_data = commit_results[0] - .as_ref() - .unwrap() - .execution_details - .return_data - .clone(); + let return_data = commit_results[0].as_ref().unwrap().return_data.clone(); if let Some(index) = index { let return_data = return_data.unwrap(); assert_eq!(return_data.program_id, mock_program_id); diff --git a/svm/src/transaction_commit_result.rs b/svm/src/transaction_commit_result.rs index 53590c0c5d2b50..8bbada73634ad9 100644 --- a/svm/src/transaction_commit_result.rs +++ b/svm/src/transaction_commit_result.rs @@ -1,9 +1,8 @@ use { - crate::transaction_execution_result::{ - TransactionExecutionDetails, TransactionLoadedAccountsStats, - }, + crate::transaction_execution_result::TransactionLoadedAccountsStats, solana_sdk::{ - fee::FeeDetails, rent_debits::RentDebits, transaction::Result as TransactionResult, + fee::FeeDetails, inner_instruction::InnerInstructionsList, rent_debits::RentDebits, + transaction::Result as TransactionResult, transaction_context::TransactionReturnData, }, }; @@ -11,16 +10,19 @@ pub type TransactionCommitResult = TransactionResult; #[derive(Clone, Debug)] pub struct CommittedTransaction { - pub loaded_account_stats: TransactionLoadedAccountsStats, - pub execution_details: TransactionExecutionDetails, + pub status: TransactionResult<()>, + pub log_messages: Option>, + pub inner_instructions: Option, + pub return_data: Option, + pub executed_units: u64, pub fee_details: FeeDetails, pub rent_debits: RentDebits, + pub loaded_account_stats: TransactionLoadedAccountsStats, } pub trait TransactionCommitResultExtensions { fn was_committed(&self) -> bool; fn was_executed_successfully(&self) -> bool; - fn transaction_result(&self) -> TransactionResult<()>; } impl TransactionCommitResultExtensions for TransactionCommitResult { @@ -30,14 +32,8 @@ impl TransactionCommitResultExtensions for TransactionCommitResult { fn was_executed_successfully(&self) -> bool { match self { - Ok(committed_tx) => committed_tx.execution_details.status.is_ok(), + Ok(committed_tx) => committed_tx.status.is_ok(), Err(_) => false, } } - - fn transaction_result(&self) -> TransactionResult<()> { - self.as_ref() - .map_err(|err| err.clone()) - .and_then(|committed_tx| committed_tx.execution_details.status.clone()) - } } diff --git a/svm/src/transaction_execution_result.rs b/svm/src/transaction_execution_result.rs index 2ac684d4cc219c..6a41294ddb975e 100644 --- a/svm/src/transaction_execution_result.rs +++ b/svm/src/transaction_execution_result.rs @@ -6,7 +6,6 @@ pub use solana_sdk::inner_instruction::{InnerInstruction, InnerInstructionsList}; use { crate::account_loader::LoadedTransaction, - serde::{Deserialize, Serialize}, solana_program_runtime::loaded_programs::ProgramCacheEntry, solana_sdk::{ pubkey::Pubkey, @@ -51,7 +50,7 @@ impl ExecutedTransaction { } } -#[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize)] +#[derive(Clone, Debug, Eq, PartialEq)] pub struct TransactionExecutionDetails { pub status: transaction::Result<()>, pub log_messages: Option>, @@ -62,3 +61,9 @@ pub struct TransactionExecutionDetails { /// NOTE: This value is valid IFF `status` is `Ok`. pub accounts_data_len_delta: i64, } + +impl TransactionExecutionDetails { + pub fn was_successful(&self) -> bool { + self.status.is_ok() + } +}