diff --git a/core/src/banking_stage.rs b/core/src/banking_stage.rs index 08540dc04d..14aabf33da 100644 --- a/core/src/banking_stage.rs +++ b/core/src/banking_stage.rs @@ -1351,34 +1351,22 @@ impl BankingStage { gossip_vote_sender: &ReplayVoteSender, qos_service: &QosService, ) -> ProcessTransactionBatchOutput { - let ( - (transactions_qos_results, cost_model_throttled_transactions_count, transaction_costs), - cost_model_time, - ) = Measure::this( - |_| { - let tx_costs = qos_service.compute_transaction_costs(txs.iter()); + let mut cost_model_time = Measure::start("cost_model"); - let (transactions_qos_results, num_included) = - qos_service.select_transactions_per_cost(txs.iter(), tx_costs.iter(), bank); + let transaction_costs = qos_service.compute_transaction_costs(txs.iter()); - let cost_model_throttled_transactions_count = - txs.len().saturating_sub(num_included); + let (transactions_qos_results, num_included) = + qos_service.select_transactions_per_cost(txs.iter(), transaction_costs.iter(), bank); - qos_service.accumulate_estimated_transaction_costs( - &Self::accumulate_batched_transaction_costs( - tx_costs.iter(), - transactions_qos_results.iter(), - ), - ); - ( - transactions_qos_results, - cost_model_throttled_transactions_count, - tx_costs, - ) - }, - (), - "cost_model", + let cost_model_throttled_transactions_count = txs.len().saturating_sub(num_included); + + qos_service.accumulate_estimated_transaction_costs( + &Self::accumulate_batched_transaction_costs( + transaction_costs.iter(), + transactions_qos_results.iter(), + ), ); + cost_model_time.stop(); // Only lock accounts for those transactions are selected for the block; // Once accounts are locked, other threads cannot encode transactions that will modify the @@ -1410,6 +1398,9 @@ impl BankingStage { .. } = execute_and_commit_transactions_output; + // TODO: This does not revert the cost tracker changes from all unexecuted transactions + // yet: For example tx that are too old will not be included in the block, but are not + // retryable. QosService::update_or_remove_transaction_costs( transaction_costs.iter(), transactions_qos_results.iter(), diff --git a/core/src/qos_service.rs b/core/src/qos_service.rs index f83862284e..2927e841a8 100644 --- a/core/src/qos_service.rs +++ b/core/src/qos_service.rs @@ -133,7 +133,7 @@ impl QosService { let mut num_included = 0; let select_results = transactions .zip(transactions_costs) - .map(|(tx, cost)| match cost_tracker.try_add(tx, cost) { + .map(|(tx, cost)| match cost_tracker.try_add(cost) { Ok(current_block_cost) => { debug!("slot {:?}, transaction {:?}, cost {:?}, fit into current block, current block cost {}", bank.slot(), tx, cost, current_block_cost); self.metrics.stats.selected_txs_count.fetch_add(1, Ordering::Relaxed); @@ -184,8 +184,10 @@ impl QosService { transaction_costs .zip(transaction_qos_results) .enumerate() - .for_each(|(index, (tx_cost, qos_result))| { - if qos_result.is_ok() && retryable_transaction_indexes.contains(&index) { + .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. diff --git a/ledger-tool/src/main.rs b/ledger-tool/src/main.rs index 7e612513fe..9ce708bda9 100644 --- a/ledger-tool/src/main.rs +++ b/ledger-tool/src/main.rs @@ -822,7 +822,7 @@ fn compute_slot_cost(blockstore: &Blockstore, slot: Slot) -> Result<(), String> num_programs += transaction.message().instructions().len(); let tx_cost = cost_model.calculate_cost(&transaction); - let result = cost_tracker.try_add(&transaction, &tx_cost); + let result = cost_tracker.try_add(&tx_cost); if result.is_err() { println!( "Slot: {}, CostModel rejected transaction {:?}, reason {:?}", diff --git a/runtime/src/cost_model.rs b/runtime/src/cost_model.rs index ce5d3ff32a..35e6ab2179 100644 --- a/runtime/src/cost_model.rs +++ b/runtime/src/cost_model.rs @@ -16,7 +16,7 @@ use { const MAX_WRITABLE_ACCOUNTS: usize = 256; // costs are stored in number of 'compute unit's -#[derive(Debug, Clone)] +#[derive(Debug)] pub struct TransactionCost { pub writable_accounts: Vec, pub signature_cost: u64, diff --git a/runtime/src/cost_tracker.rs b/runtime/src/cost_tracker.rs index 9c418a9ab0..ffc36e08bc 100644 --- a/runtime/src/cost_tracker.rs +++ b/runtime/src/cost_tracker.rs @@ -1,11 +1,11 @@ //! `cost_tracker` keeps tracking transaction cost per chained accounts as well as for entire block //! The main functions are: //! - would_fit(&tx_cost), immutable function to test if tx with tx_cost would fit into current block -//! - add_transaction(&tx_cost), mutable function to accumulate tx_cost to tracker. +//! - add_transaction_cost(&tx_cost), mutable function to accumulate tx_cost to tracker. //! use { crate::{block_cost_limits::*, cost_model::TransactionCost}, - solana_sdk::{clock::Slot, pubkey::Pubkey, transaction::SanitizedTransaction}, + solana_sdk::{clock::Slot, pubkey::Pubkey}, std::collections::HashMap, }; @@ -89,11 +89,7 @@ impl CostTracker { self.vote_cost_limit = vote_cost_limit; } - pub fn try_add( - &mut self, - _transaction: &SanitizedTransaction, - tx_cost: &TransactionCost, - ) -> Result { + pub fn try_add(&mut self, tx_cost: &TransactionCost) -> Result { self.would_fit(tx_cost)?; self.add_transaction_cost(tx_cost); Ok(self.block_cost) @@ -104,7 +100,7 @@ impl CostTracker { _estimated_tx_cost: &TransactionCost, _actual_execution_cost: u64, ) { - // adjust block_cost / vote_cost / account_cost by (actual_execution_cost - execution_cost) + // TODO: adjust block_cost / vote_cost / account_cost by (actual_execution_cost - execution_cost) } pub fn remove(&mut self, tx_cost: &TransactionCost) { @@ -260,7 +256,9 @@ mod tests { hash::Hash, signature::{Keypair, Signer}, system_transaction, - transaction::{MessageHash, SimpleAddressLoader, VersionedTransaction}, + transaction::{ + MessageHash, SanitizedTransaction, SimpleAddressLoader, VersionedTransaction, + }, }, solana_vote_program::vote_transaction, std::{cmp, sync::Arc}, @@ -360,7 +358,7 @@ mod tests { // build testee to have capacity for one simple transaction let mut testee = CostTracker::new(cost, cost, cost, None); assert!(testee.would_fit(&tx_cost).is_ok()); - testee.add_transaction(&tx_cost); + testee.add_transaction_cost(&tx_cost); assert_eq!(cost, testee.block_cost); assert_eq!(0, testee.vote_cost); let (_costliest_account, costliest_account_cost) = testee.find_costliest_account(); @@ -376,7 +374,7 @@ mod tests { // build testee to have capacity for one simple transaction let mut testee = CostTracker::new(cost, cost, cost, None); assert!(testee.would_fit(&tx_cost).is_ok()); - testee.add_transaction(&tx_cost); + testee.add_transaction_cost(&tx_cost); assert_eq!(cost, testee.block_cost); assert_eq!(cost, testee.vote_cost); let (_costliest_account, costliest_account_cost) = testee.find_costliest_account(); @@ -394,7 +392,7 @@ mod tests { let mut testee = CostTracker::new(cost, cost, cost, None); assert!(testee.would_fit(&tx_cost).is_ok()); let old = testee.account_data_size; - testee.add_transaction(&tx_cost); + testee.add_transaction_cost(&tx_cost); assert_eq!(old + 1, testee.account_data_size); } @@ -411,11 +409,11 @@ mod tests { let mut testee = CostTracker::new(cost1 + cost2, cost1 + cost2, cost1 + cost2, None); { assert!(testee.would_fit(&tx_cost1).is_ok()); - testee.add_transaction(&tx_cost1); + testee.add_transaction_cost(&tx_cost1); } { assert!(testee.would_fit(&tx_cost2).is_ok()); - testee.add_transaction(&tx_cost2); + testee.add_transaction_cost(&tx_cost2); } assert_eq!(cost1 + cost2, testee.block_cost); assert_eq!(1, testee.cost_by_writable_accounts.len()); @@ -438,11 +436,11 @@ mod tests { CostTracker::new(cmp::max(cost1, cost2), cost1 + cost2, cost1 + cost2, None); { assert!(testee.would_fit(&tx_cost1).is_ok()); - testee.add_transaction(&tx_cost1); + testee.add_transaction_cost(&tx_cost1); } { assert!(testee.would_fit(&tx_cost2).is_ok()); - testee.add_transaction(&tx_cost2); + testee.add_transaction_cost(&tx_cost2); } assert_eq!(cost1 + cost2, testee.block_cost); assert_eq!(2, testee.cost_by_writable_accounts.len()); @@ -465,7 +463,7 @@ mod tests { // should have room for first transaction { assert!(testee.would_fit(&tx_cost1).is_ok()); - testee.add_transaction(&tx_cost1); + testee.add_transaction_cost(&tx_cost1); } // but no more sapce on the same chain (same signer account) { @@ -493,7 +491,7 @@ mod tests { // should have room for first transaction { assert!(testee.would_fit(&tx_cost1).is_ok()); - testee.add_transaction(&tx_cost1); + testee.add_transaction_cost(&tx_cost1); } // but no more room for package as whole { @@ -521,7 +519,7 @@ mod tests { // should have room for first vote { assert!(testee.would_fit(&tx_cost1).is_ok()); - testee.add_transaction(&tx_cost1); + testee.add_transaction_cost(&tx_cost1); } // but no more room for package as whole { @@ -591,45 +589,38 @@ mod tests { } #[test] - fn test_cost_tracker_commit_and_cancel() { + fn test_cost_tracker_remove() { let (mint_keypair, start_hash) = test_setup(); // build two transactions with diff accounts let second_account = Keypair::new(); - let (tx1, tx_cost1) = build_simple_transaction(&mint_keypair, &start_hash); - let (tx2, tx_cost2) = build_simple_transaction(&second_account, &start_hash); + let (_tx1, tx_cost1) = build_simple_transaction(&mint_keypair, &start_hash); + let (_tx2, tx_cost2) = build_simple_transaction(&second_account, &start_hash); let cost1 = tx_cost1.sum(); let cost2 = tx_cost2.sum(); // build testee let mut testee = CostTracker::new(cost1 + cost2, cost1 + cost2, cost1 + cost2, None); - assert!(testee.try_add(&tx1, &tx_cost1).is_ok()); - assert!(testee.try_add(&tx2, &tx_cost2).is_ok()); - - // assert the block cost is still zero - assert_eq!(0, testee.block_cost); - - // assert tx1 cost applied to tracker if committed - testee.commit_transaction(&tx1, None); - assert_eq!(cost1, testee.block_cost); - - // assert tx2 cost will not be applied to tracker if cancelled - testee.cancel_transaction(&tx2); - assert_eq!(cost1, testee.block_cost); - - // still can add tx2 - assert!(testee.try_add(&tx2, &tx_cost2).is_ok()); - // cannot add tx1 while tx2 is pending - assert!(testee.try_add(&tx1, &tx_cost1).is_err()); - // after commit tx2, the block will have both tx1 and tx2 - testee.commit_transaction(&tx2, None); + assert!(testee.try_add(&tx_cost1).is_ok()); + assert!(testee.try_add(&tx_cost2).is_ok()); assert_eq!(cost1 + cost2, testee.block_cost); + + // removing a tx_cost affects block_cost + testee.remove(&tx_cost1); + assert_eq!(cost2, testee.block_cost); + + // add back tx1 + assert!(testee.try_add(&tx_cost1).is_ok()); + assert_eq!(cost1 + cost2, testee.block_cost); + + // cannot add tx1 again, cost limit would be exceeded + assert!(testee.try_add(&tx_cost1).is_err()); } #[test] fn test_cost_tracker_try_add_is_atomic() { let (mint_keypair, start_hash) = test_setup(); // build two mocking vote transactions with diff accounts - let (tx1, _tx_cost1) = build_simple_vote_transaction(&mint_keypair, &start_hash); + let (_tx1, _tx_cost1) = build_simple_vote_transaction(&mint_keypair, &start_hash); let acct1 = Pubkey::new_unique(); let acct2 = Pubkey::new_unique(); @@ -651,8 +642,7 @@ mod tests { execution_cost: cost, ..TransactionCost::default() }; - assert!(testee.try_add(&tx1, &tx_cost).is_ok()); - testee.commit_transaction(&tx1, None); + assert!(testee.try_add(&tx_cost).is_ok()); let (_costliest_account, costliest_account_cost) = testee.find_costliest_account(); assert_eq!(cost, testee.block_cost); assert_eq!(3, testee.cost_by_writable_accounts.len()); @@ -670,8 +660,7 @@ mod tests { execution_cost: cost, ..TransactionCost::default() }; - assert!(testee.try_add(&tx1, &tx_cost).is_ok()); - testee.commit_transaction(&tx1, None); + assert!(testee.try_add(&tx_cost).is_ok()); let (costliest_account, costliest_account_cost) = testee.find_costliest_account(); assert_eq!(cost * 2, testee.block_cost); assert_eq!(3, testee.cost_by_writable_accounts.len()); @@ -691,8 +680,7 @@ mod tests { execution_cost: cost, ..TransactionCost::default() }; - assert!(testee.try_add(&tx1, &tx_cost).is_err()); - testee.commit_transaction(&tx1, None); + assert!(testee.try_add(&tx_cost).is_err()); let (costliest_account, costliest_account_cost) = testee.find_costliest_account(); assert_eq!(cost * 2, testee.block_cost); assert_eq!(3, testee.cost_by_writable_accounts.len());