undo transaction cost from cost_tracker if it was not executed successfully

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<TransactionExecutionResult>,
 }
 
 #[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,
         );
 

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<Item = &'a TransactionCost>,
         transaction_qos_results: impl Iterator<Item = &'a transaction::Result<()>>,
-        retryable_transaction_indexes: &[usize],
+        execution_results: impl Iterator<Item = &'a TransactionExecutionResult>,
         bank: &Arc<Bank>,
     ) {
         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<SanitizedTransaction> = (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<TransactionExecutionResult> = (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<SanitizedTransaction> = (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<TransactionExecutionResult> = (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<SanitizedTransaction> = (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<TransactionExecutionResult> = (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()
+            );
+        }
+    }
 }

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<u64>,
     ) {
-        // 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);
     }
 }