Revert "v2.0: Refactor and additional metrics for cost tracking (back…

…port of #1888) (#1900) (#1937)

Revert "v2.0: Refactor and additional metrics for cost tracking (backport of #1888) (#1900)"

This reverts commit 0aef62e.

core/benches/consumer.rs

@@ -22,6 +22,7 @@ use {
     solana_runtime::bank::Bank,
     solana_sdk::{
         account::{Account, ReadableAccount},
+        feature_set::apply_cost_tracker_during_replay,
         signature::Keypair,
         signer::Signer,
         stake_history::Epoch,
@@ -96,7 +97,7 @@ struct BenchFrame {
     signal_receiver: Receiver<(Arc<Bank>, (Entry, u64))>,
 }
 
-fn setup() -> BenchFrame {
+fn setup(apply_cost_tracker_during_replay: bool) -> BenchFrame {
     let mint_total = u64::MAX;
     let GenesisConfigInfo {
         mut genesis_config, ..
@@ -108,6 +109,10 @@ fn setup() -> BenchFrame {
 
     let mut bank = Bank::new_for_benches(&genesis_config);
 
+    if !apply_cost_tracker_during_replay {
+        bank.deactivate_feature(&apply_cost_tracker_during_replay::id());
+    }
+
     // Allow arbitrary transaction processing time for the purposes of this bench
     bank.ns_per_slot = u128::MAX;
 
@@ -134,7 +139,11 @@ fn setup() -> BenchFrame {
     }
 }
 
-fn bench_process_and_record_transactions(bencher: &mut Bencher, batch_size: usize) {
+fn bench_process_and_record_transactions(
+    bencher: &mut Bencher,
+    batch_size: usize,
+    apply_cost_tracker_during_replay: bool,
+) {
     const TRANSACTIONS_PER_ITERATION: usize = 64;
     assert_eq!(
         TRANSACTIONS_PER_ITERATION % batch_size,
@@ -152,7 +161,7 @@ fn bench_process_and_record_transactions(bencher: &mut Bencher, batch_size: usiz
         poh_recorder,
         poh_service,
         signal_receiver: _signal_receiver,
-    } = setup();
+    } = setup(apply_cost_tracker_during_replay);
     let consumer = create_consumer(&poh_recorder);
     let transactions = create_transactions(&bank, 2_usize.pow(20));
     let mut transaction_iter = transactions.chunks(batch_size);
@@ -177,15 +186,30 @@ fn bench_process_and_record_transactions(bencher: &mut Bencher, batch_size: usiz
 
 #[bench]
 fn bench_process_and_record_transactions_unbatched(bencher: &mut Bencher) {
-    bench_process_and_record_transactions(bencher, 1);
+    bench_process_and_record_transactions(bencher, 1, true);
 }
 
 #[bench]
 fn bench_process_and_record_transactions_half_batch(bencher: &mut Bencher) {
-    bench_process_and_record_transactions(bencher, 32);
+    bench_process_and_record_transactions(bencher, 32, true);
 }
 
 #[bench]
 fn bench_process_and_record_transactions_full_batch(bencher: &mut Bencher) {
-    bench_process_and_record_transactions(bencher, 64);
+    bench_process_and_record_transactions(bencher, 64, true);
+}
+
+#[bench]
+fn bench_process_and_record_transactions_unbatched_disable_tx_cost_update(bencher: &mut Bencher) {
+    bench_process_and_record_transactions(bencher, 1, false);
+}
+
+#[bench]
+fn bench_process_and_record_transactions_half_batch_disable_tx_cost_update(bencher: &mut Bencher) {
+    bench_process_and_record_transactions(bencher, 32, false);
+}
+
+#[bench]
+fn bench_process_and_record_transactions_full_batch_disable_tx_cost_update(bencher: &mut Bencher) {
+    bench_process_and_record_transactions(bencher, 64, false);
 }

core/src/banking_stage/consumer.rs

@@ -518,14 +518,27 @@ impl Consumer {
 
         // Costs of all transactions are added to the cost_tracker before processing.
         // To ensure accurate tracking of compute units, transactions that ultimately
-        // were not included in the block should have their cost removed, the rest
-        // should update with their actually consumed units.
-        QosService::remove_or_update_costs(
+        // were not included in the block should have their cost removed.
+        QosService::remove_costs(
             transaction_qos_cost_results.iter(),
             commit_transactions_result.as_ref().ok(),
             bank,
         );
 
+        // once feature `apply_cost_tracker_during_replay` is activated, leader shall no longer
+        // adjust block with executed cost (a behavior more inline with bankless leader), it
+        // should use requested, or default `compute_unit_limit` as transaction's execution cost.
+        if !bank
+            .feature_set
+            .is_active(&feature_set::apply_cost_tracker_during_replay::id())
+        {
+            QosService::update_costs(
+                transaction_qos_cost_results.iter(),
+                commit_transactions_result.as_ref().ok(),
+                bank,
+            );
+        }
+
         retryable_transaction_indexes
             .iter_mut()
             .for_each(|x| *x += chunk_offset);
@@ -1419,6 +1432,16 @@ mod tests {
 
     #[test]
     fn test_bank_process_and_record_transactions_cost_tracker() {
+        for apply_cost_tracker_during_replay_enabled in [true, false] {
+            bank_process_and_record_transactions_cost_tracker(
+                apply_cost_tracker_during_replay_enabled,
+            );
+        }
+    }
+
+    fn bank_process_and_record_transactions_cost_tracker(
+        apply_cost_tracker_during_replay_enabled: bool,
+    ) {
         solana_logger::setup();
         let GenesisConfigInfo {
             genesis_config,
@@ -1427,6 +1450,9 @@ mod tests {
         } = create_slow_genesis_config(10_000);
         let mut bank = Bank::new_for_tests(&genesis_config);
         bank.ns_per_slot = u128::MAX;
+        if !apply_cost_tracker_during_replay_enabled {
+            bank.deactivate_feature(&feature_set::apply_cost_tracker_during_replay::id());
+        }
         let bank = bank.wrap_with_bank_forks_for_tests().0;
         let pubkey = solana_sdk::pubkey::new_rand();
 
@@ -1495,7 +1521,8 @@ mod tests {
 
             // TEST: it's expected that the allocation will execute but the transfer will not
             // because of a shared write-lock between mint_keypair. Ensure only the first transaction
-            // takes compute units in the block
+            // takes compute units in the block AND the apply_cost_tracker_during_replay_enabled feature
+            // is applied correctly
             let allocate_keypair = Keypair::new();
             let transactions = sanitize_transactions(vec![
                 system_transaction::allocate(
@@ -1534,7 +1561,7 @@ mod tests {
             );
             assert_eq!(retryable_transaction_indexes, vec![1]);
 
-            let expected_block_cost = {
+            let expected_block_cost = if !apply_cost_tracker_during_replay_enabled {
                 let (actual_programs_execution_cost, actual_loaded_accounts_data_size_cost) =
                     match commit_transactions_result.first().unwrap() {
                         CommitTransactionDetails::Committed {
@@ -1560,6 +1587,8 @@ mod tests {
                 }
 
                 block_cost + cost.sum()
+            } else {
+                block_cost + CostModel::calculate_cost(&transactions[0], &bank.feature_set).sum()
             };
 
             assert_eq!(get_block_cost(), expected_block_cost);

core/src/banking_stage/qos_service.rs

@@ -132,28 +132,39 @@ impl QosService {
         (select_results, num_included)
     }
 
-    /// Removes transaction costs from the cost tracker if not committed or recorded, or
-    /// updates the transaction costs for committed transactions.
-    pub fn remove_or_update_costs<'a>(
+    /// Updates the transaction costs for committed transactions. Does not handle removing costs
+    /// for transactions that didn't get recorded or committed
+    pub fn update_costs<'a>(
+        transaction_cost_results: impl Iterator<Item = &'a transaction::Result<TransactionCost>>,
+        transaction_committed_status: Option<&Vec<CommitTransactionDetails>>,
+        bank: &Bank,
+    ) {
+        if let Some(transaction_committed_status) = transaction_committed_status {
+            Self::update_committed_transaction_costs(
+                transaction_cost_results,
+                transaction_committed_status,
+                bank,
+            )
+        }
+    }
+
+    /// Removes transaction costs from the cost tracker if not committed or recorded
+    pub fn remove_costs<'a>(
         transaction_cost_results: impl Iterator<Item = &'a transaction::Result<TransactionCost>>,
         transaction_committed_status: Option<&Vec<CommitTransactionDetails>>,
         bank: &Bank,
     ) {
         match transaction_committed_status {
-            Some(transaction_committed_status) => {
-                Self::remove_or_update_recorded_transaction_costs(
-                    transaction_cost_results,
-                    transaction_committed_status,
-                    bank,
-                )
-            }
-            None => Self::remove_unrecorded_transaction_costs(transaction_cost_results, bank),
+            Some(transaction_committed_status) => Self::remove_uncommitted_transaction_costs(
+                transaction_cost_results,
+                transaction_committed_status,
+                bank,
+            ),
+            None => Self::remove_transaction_costs(transaction_cost_results, bank),
         }
     }
 
-    /// For recorded transactions, remove units reserved by uncommitted transaction, or update
-    /// units for committed transactions.
-    fn remove_or_update_recorded_transaction_costs<'a>(
+    fn remove_uncommitted_transaction_costs<'a>(
         transaction_cost_results: impl Iterator<Item = &'a transaction::Result<TransactionCost>>,
         transaction_committed_status: &Vec<CommitTransactionDetails>,
         bank: &Bank,
@@ -167,31 +178,45 @@ impl QosService {
                 // checked for update
                 if let Ok(tx_cost) = tx_cost {
                     num_included += 1;
-                    match transaction_committed_details {
-                        CommitTransactionDetails::Committed {
-                            compute_units,
-                            loaded_accounts_data_size,
-                        } => {
-                            cost_tracker.update_execution_cost(
-                                tx_cost,
-                                *compute_units,
-                                CostModel::calculate_loaded_accounts_data_size_cost(
-                                    *loaded_accounts_data_size,
-                                    &bank.feature_set,
-                                ),
-                            );
-                        }
-                        CommitTransactionDetails::NotCommitted => {
-                            cost_tracker.remove(tx_cost);
-                        }
+                    if *transaction_committed_details == CommitTransactionDetails::NotCommitted {
+                        cost_tracker.remove(tx_cost)
                     }
                 }
             });
         cost_tracker.sub_transactions_in_flight(num_included);
     }
 
-    /// Remove reserved units for transaction batch that unsuccessfully recorded.
-    fn remove_unrecorded_transaction_costs<'a>(
+    fn update_committed_transaction_costs<'a>(
+        transaction_cost_results: impl Iterator<Item = &'a transaction::Result<TransactionCost>>,
+        transaction_committed_status: &Vec<CommitTransactionDetails>,
+        bank: &Bank,
+    ) {
+        let mut cost_tracker = bank.write_cost_tracker().unwrap();
+        transaction_cost_results
+            .zip(transaction_committed_status)
+            .for_each(|(estimated_tx_cost, transaction_committed_details)| {
+                // Only transactions that the qos service included have to be
+                // checked for update
+                if let Ok(estimated_tx_cost) = estimated_tx_cost {
+                    if let CommitTransactionDetails::Committed {
+                        compute_units,
+                        loaded_accounts_data_size,
+                    } = transaction_committed_details
+                    {
+                        cost_tracker.update_execution_cost(
+                            estimated_tx_cost,
+                            *compute_units,
+                            CostModel::calculate_loaded_accounts_data_size_cost(
+                                *loaded_accounts_data_size,
+                                &bank.feature_set,
+                            ),
+                        )
+                    }
+                }
+            });
+    }
+
+    fn remove_transaction_costs<'a>(
         transaction_cost_results: impl Iterator<Item = &'a transaction::Result<TransactionCost>>,
         bank: &Bank,
     ) {
@@ -759,11 +784,18 @@ mod tests {
                 + (execute_units_adjustment + loaded_accounts_data_size_cost_adjustment)
                     * transaction_count;
 
-            QosService::remove_or_update_costs(
-                qos_cost_results.iter(),
-                Some(&committed_status),
-                &bank,
+            // All transactions are committed, no costs should be removed
+            QosService::remove_costs(qos_cost_results.iter(), Some(&committed_status), &bank);
+            assert_eq!(
+                total_txs_cost,
+                bank.read_cost_tracker().unwrap().block_cost()
+            );
+            assert_eq!(
+                transaction_count,
+                bank.read_cost_tracker().unwrap().transaction_count()
             );
+
+            QosService::update_costs(qos_cost_results.iter(), Some(&committed_status), &bank);
             assert_eq!(
                 final_txs_cost,
                 bank.read_cost_tracker().unwrap().block_cost()
@@ -811,7 +843,18 @@ mod tests {
                 bank.read_cost_tracker().unwrap().block_cost()
             );
 
-            QosService::remove_or_update_costs(qos_cost_results.iter(), None, &bank);
+            // update costs doesn't impact non-committed
+            QosService::update_costs(qos_cost_results.iter(), None, &bank);
+            assert_eq!(
+                total_txs_cost,
+                bank.read_cost_tracker().unwrap().block_cost()
+            );
+            assert_eq!(
+                transaction_count,
+                bank.read_cost_tracker().unwrap().transaction_count()
+            );
+
+            QosService::remove_costs(qos_cost_results.iter(), None, &bank);
             assert_eq!(0, bank.read_cost_tracker().unwrap().block_cost());
             assert_eq!(0, bank.read_cost_tracker().unwrap().transaction_count());
         }
@@ -883,11 +926,8 @@ mod tests {
                 })
                 .collect();
 
-            QosService::remove_or_update_costs(
-                qos_cost_results.iter(),
-                Some(&committed_status),
-                &bank,
-            );
+            QosService::remove_costs(qos_cost_results.iter(), Some(&committed_status), &bank);
+            QosService::update_costs(qos_cost_results.iter(), Some(&committed_status), &bank);
 
             // assert the final block cost
             let mut expected_final_txs_count = 0u64;