diff --git a/core/src/banking_stage.rs b/core/src/banking_stage.rs index 05f2711749..a3edd35a1b 100644 --- a/core/src/banking_stage.rs +++ b/core/src/banking_stage.rs @@ -117,6 +117,8 @@ pub struct ExecuteAndCommitTransactionsOutput { // how many such transactions were committed commit_transactions_result: Result<(), PohRecorderError>, execute_and_commit_timings: LeaderExecuteAndCommitTimings, + // Transaction execution detail results + execution_results: Vec, } #[derive(Debug, Default)] @@ -1247,6 +1249,7 @@ impl BankingStage { retryable_transaction_indexes, commit_transactions_result: Err(e), execute_and_commit_timings, + execution_results, }; } @@ -1261,7 +1264,7 @@ impl BankingStage { bank.commit_transactions( sanitized_txs, &mut loaded_transactions, - execution_results, + execution_results.clone(), executed_transactions_count as u64, executed_transactions_count .saturating_sub(executed_with_successful_result_count) @@ -1333,6 +1336,7 @@ impl BankingStage { retryable_transaction_indexes, commit_transactions_result: Ok(()), execute_and_commit_timings, + execution_results, } } @@ -1389,6 +1393,7 @@ impl BankingStage { let ExecuteAndCommitTransactionsOutput { ref mut retryable_transaction_indexes, ref execute_and_commit_timings, + ref execution_results, .. } = execute_and_commit_transactions_output; @@ -1398,7 +1403,7 @@ impl BankingStage { QosService::update_or_remove_transaction_costs( transaction_costs.iter(), transactions_qos_results.iter(), - retryable_transaction_indexes, + execution_results.iter(), bank, ); diff --git a/core/src/qos_service.rs b/core/src/qos_service.rs index 2927e841a8..038283e2f2 100644 --- a/core/src/qos_service.rs +++ b/core/src/qos_service.rs @@ -7,7 +7,7 @@ use { crossbeam_channel::{unbounded, Receiver, Sender}, solana_measure::measure::Measure, solana_runtime::{ - bank::Bank, + bank::{Bank, TransactionExecutionResult}, cost_model::{CostModel, TransactionCost}, cost_tracker::CostTrackerError, }, @@ -177,24 +177,22 @@ impl QosService { pub fn update_or_remove_transaction_costs<'a>( transaction_costs: impl Iterator, transaction_qos_results: impl Iterator>, - retryable_transaction_indexes: &[usize], + execution_results: impl Iterator, bank: &Arc, ) { let mut cost_tracker = bank.write_cost_tracker().unwrap(); transaction_costs .zip(transaction_qos_results) - .enumerate() - .for_each(|(index, (tx_cost, qos_inclusion_result))| { - // Only transactions that the qos service incuded have been added to the - // cost tracker. - if qos_inclusion_result.is_ok() && retryable_transaction_indexes.contains(&index) { - cost_tracker.remove(tx_cost); - } else { - // TODO: Update the cost tracker with the actual execution compute units. - // Will have to plumb it in next; For now, keep estimated costs. - // - // let actual_execution_cost = 0; - // cost_tracker.update_execution_cost(tx_cost, actual_execution_cost); + .zip(execution_results) + .for_each(|((tx_cost, qos_inclusion_result), execution_result)| { + // Only transactions that the qos service included have to be + // checked for remove or update/commit + if qos_inclusion_result.is_ok() { + if execution_result.was_executed_successfully() { + cost_tracker.update_execution_cost(tx_cost, None); + } else { + cost_tracker.remove(tx_cost); + } } }); } @@ -497,7 +495,7 @@ mod tests { super::*, itertools::Itertools, solana_runtime::{ - bank::Bank, + bank::TransactionExecutionDetails, genesis_utils::{create_genesis_config, GenesisConfigInfo}, }, solana_sdk::{ @@ -599,4 +597,157 @@ mod tests { assert!(results[2].is_err()); assert!(results[3].is_err()); } + + #[test] + fn test_update_or_remove_transaction_costs_executed() { + solana_logger::setup(); + let GenesisConfigInfo { genesis_config, .. } = create_genesis_config(10); + let bank = Arc::new(Bank::new_for_tests(&genesis_config)); + + // make some transfer transactions + // calculate their costs, apply to cost_tracker + let transaction_count = 5; + let keypair = Keypair::new(); + let transfer_tx = SanitizedTransaction::from_transaction_for_tests( + system_transaction::transfer(&keypair, &keypair.pubkey(), 1, Hash::default()), + ); + let txs: Vec = (0..transaction_count) + .map(|_| transfer_tx.clone()) + .collect(); + + // assert all tx_costs should be applied to cost_tracker if all execution_results are all Executed + { + let qos_service = QosService::new(Arc::new(RwLock::new(CostModel::default())), 1); + let txs_costs = qos_service.compute_transaction_costs(txs.iter()); + let total_txs_costs: u64 = txs_costs.iter().map(|cost| cost.sum()).sum(); + let (qos_results, _num_included) = + qos_service.select_transactions_per_cost(txs.iter(), txs_costs.iter(), &bank); + assert_eq!( + total_txs_costs, + bank.read_cost_tracker().unwrap().block_cost() + ); + let execution_results: Vec = (0..transaction_count) + .map(|_| { + TransactionExecutionResult::Executed(TransactionExecutionDetails { + status: Ok(()), + log_messages: None, + inner_instructions: None, + durable_nonce_fee: None, + return_data: None, + }) + }) + .collect(); + QosService::update_or_remove_transaction_costs( + txs_costs.iter(), + qos_results.iter(), + execution_results.iter(), + &bank, + ); + assert_eq!( + total_txs_costs, + bank.read_cost_tracker().unwrap().block_cost() + ); + assert_eq!( + transaction_count, + bank.read_cost_tracker().unwrap().transaction_count() + ); + } + } + + #[test] + fn test_update_or_remove_transaction_costs_not_executed() { + solana_logger::setup(); + let GenesisConfigInfo { genesis_config, .. } = create_genesis_config(10); + let bank = Arc::new(Bank::new_for_tests(&genesis_config)); + + // make some transfer transactions + // calculate their costs, apply to cost_tracker + let transaction_count = 5; + let keypair = Keypair::new(); + let transfer_tx = SanitizedTransaction::from_transaction_for_tests( + system_transaction::transfer(&keypair, &keypair.pubkey(), 1, Hash::default()), + ); + let txs: Vec = (0..transaction_count) + .map(|_| transfer_tx.clone()) + .collect(); + + // assert all tx_costs should be removed from cost_tracker if all execution_results are all NotExecuted + { + let qos_service = QosService::new(Arc::new(RwLock::new(CostModel::default())), 1); + let txs_costs = qos_service.compute_transaction_costs(txs.iter()); + let (qos_results, _num_included) = + qos_service.select_transactions_per_cost(txs.iter(), txs_costs.iter(), &bank); + let execution_results: Vec = (0..transaction_count) + .map(|_| TransactionExecutionResult::NotExecuted(TransactionError::AccountNotFound)) + .collect(); + QosService::update_or_remove_transaction_costs( + txs_costs.iter(), + qos_results.iter(), + execution_results.iter(), + &bank, + ); + assert_eq!(0, bank.read_cost_tracker().unwrap().block_cost()); + assert_eq!(0, bank.read_cost_tracker().unwrap().transaction_count()); + } + } + + #[test] + fn test_update_or_remove_transaction_costs_mixed_execution() { + solana_logger::setup(); + let GenesisConfigInfo { genesis_config, .. } = create_genesis_config(10); + let bank = Arc::new(Bank::new_for_tests(&genesis_config)); + + // make some transfer transactions + // calculate their costs, apply to cost_tracker + let transaction_count = 5; + let keypair = Keypair::new(); + let transfer_tx = SanitizedTransaction::from_transaction_for_tests( + system_transaction::transfer(&keypair, &keypair.pubkey(), 1, Hash::default()), + ); + let txs: Vec = (0..transaction_count) + .map(|_| transfer_tx.clone()) + .collect(); + + // assert only successfully executed tx_costs are applied cost_tracker + { + let qos_service = QosService::new(Arc::new(RwLock::new(CostModel::default())), 1); + let txs_costs = qos_service.compute_transaction_costs(txs.iter()); + let (qos_results, _num_included) = + qos_service.select_transactions_per_cost(txs.iter(), txs_costs.iter(), &bank); + let execution_results: Vec = (0..transaction_count) + .map(|n| { + if n < 1 { + TransactionExecutionResult::NotExecuted(TransactionError::AccountNotFound) + } else { + TransactionExecutionResult::Executed(TransactionExecutionDetails { + status: Ok(()), + log_messages: None, + inner_instructions: None, + durable_nonce_fee: None, + return_data: None, + }) + } + }) + .collect(); + QosService::update_or_remove_transaction_costs( + txs_costs.iter(), + qos_results.iter(), + execution_results.iter(), + &bank, + ); + let expected_committed_units: u64 = txs_costs + .iter() + .enumerate() + .map(|(n, cost)| if n < 1 { 0 } else { cost.sum() }) + .sum(); + assert_eq!( + expected_committed_units, + bank.read_cost_tracker().unwrap().block_cost() + ); + assert_eq!( + transaction_count - 1, + bank.read_cost_tracker().unwrap().transaction_count() + ); + } + } } diff --git a/runtime/src/cost_tracker.rs b/runtime/src/cost_tracker.rs index ffc36e08bc..5307579808 100644 --- a/runtime/src/cost_tracker.rs +++ b/runtime/src/cost_tracker.rs @@ -97,10 +97,21 @@ impl CostTracker { pub fn update_execution_cost( &mut self, - _estimated_tx_cost: &TransactionCost, - _actual_execution_cost: u64, + estimated_tx_cost: &TransactionCost, + actual_execution_units: Option, ) { - // TODO: adjust block_cost / vote_cost / account_cost by (actual_execution_cost - execution_cost) + if let Some(actual_execution_units) = actual_execution_units { + let mut adjustment = + TransactionCost::new_with_capacity(estimated_tx_cost.writable_accounts.len()); + adjustment + .writable_accounts + .extend(&estimated_tx_cost.writable_accounts); + adjustment.execution_cost = estimated_tx_cost + .execution_cost + .saturating_sub(actual_execution_units); + adjustment.is_simple_vote = estimated_tx_cost.is_simple_vote; + self.sub_transaction_cost(&adjustment); + } } pub fn remove(&mut self, tx_cost: &TransactionCost) { @@ -224,7 +235,7 @@ impl CostTracker { self.transaction_count = self.transaction_count.saturating_add(1); } - fn remove_transaction_cost(&mut self, tx_cost: &TransactionCost) { + fn sub_transaction_cost(&mut self, tx_cost: &TransactionCost) { let cost = tx_cost.sum(); for account_key in tx_cost.writable_accounts.iter() { let account_cost = self @@ -240,6 +251,10 @@ impl CostTracker { self.account_data_size = self .account_data_size .saturating_sub(tx_cost.account_data_size); + } + + fn remove_transaction_cost(&mut self, tx_cost: &TransactionCost) { + self.sub_transaction_cost(tx_cost); self.transaction_count = self.transaction_count.saturating_sub(1); } }