diff --git a/core/src/qos_service.rs b/core/src/qos_service.rs index b8a20db913..550bb1ea95 100644 --- a/core/src/qos_service.rs +++ b/core/src/qos_service.rs @@ -206,12 +206,8 @@ impl QosService { .for_each(|((tx_cost, qos_inclusion_result), was_committed)| { // Only transactions that the qos service included have to be // checked for update - if qos_inclusion_result.is_ok() { - if *was_committed { - cost_tracker.update_execution_cost(tx_cost, None); - } else { - cost_tracker.remove(tx_cost); - } + if qos_inclusion_result.is_ok() && !*was_committed { + cost_tracker.remove(tx_cost); } }); } diff --git a/runtime/src/cost_tracker.rs b/runtime/src/cost_tracker.rs index a2da78843f..3679f34dd5 100644 --- a/runtime/src/cost_tracker.rs +++ b/runtime/src/cost_tracker.rs @@ -5,7 +5,7 @@ //! use { crate::{block_cost_limits::*, cost_model::TransactionCost}, - solana_sdk::{clock::Slot, num_utils::*, pubkey::Pubkey}, + solana_sdk::{clock::Slot, pubkey::Pubkey}, std::collections::HashMap, }; @@ -95,36 +95,6 @@ impl CostTracker { Ok(self.block_cost) } - pub fn update_execution_cost( - &mut self, - estimated_tx_cost: &TransactionCost, - actual_execution_units: Option, - ) { - if let Some(actual_execution_units) = actual_execution_units { - let estimated_execution_units = estimated_tx_cost.execution_cost; - if actual_execution_units == estimated_execution_units { - return; - } - - if let Some(adjustment) = - Self::checked_diff_unsigned(actual_execution_units, estimated_execution_units) - { - self.adjust_transaction_execution_cost(estimated_tx_cost, adjustment); - } else { - // handle adjustment too big to fit in `i64` error, log event as error, - // set block_cost to limit to prevent more transactions being added into - // curernt block. - log::error!( - "cost_tracker detected erroneous attemopt to adjust execution cost, \ - estimated transactino cost {:?}, actual execution units {}", - estimated_tx_cost, - actual_execution_units - ); - self.block_cost = self.block_cost_limit; - } - } - } - pub fn remove(&mut self, tx_cost: &TransactionCost) { self.remove_transaction_cost(tx_cost); } @@ -264,42 +234,6 @@ impl CostTracker { .saturating_sub(tx_cost.account_data_size); self.transaction_count = self.transaction_count.saturating_sub(1); } - - /// Apply execution units adjustment to cost-tracker. - /// If adjustment causes arithmetic overflow, it bumps costs to their limits to - /// prevent adding further transactions into current block. - fn adjust_transaction_execution_cost(&mut self, tx_cost: &TransactionCost, adjustment: i64) { - for account_key in tx_cost.writable_accounts.iter() { - let account_cost = self - .cost_by_writable_accounts - .entry(*account_key) - .or_insert(0); - match UintUtil::checked_add_signed(*account_cost, adjustment) { - Some(adjusted_cost) => *account_cost = adjusted_cost, - None => *account_cost = self.account_cost_limit, - } - } - match UintUtil::checked_add_signed(self.block_cost, adjustment) { - Some(adjusted_cost) => self.block_cost = adjusted_cost, - None => self.block_cost = self.block_cost_limit, - } - if tx_cost.is_simple_vote { - match UintUtil::checked_add_signed(self.vote_cost, adjustment) { - Some(adjusted_cost) => self.vote_cost = adjusted_cost, - None => self.vote_cost = self.vote_cost_limit, - } - } - } - - /// Checked get deltai as `i64` of two `u64`, computes `lhs - rhs`, - /// returning `None` if delta overflows `i64` - fn checked_diff_unsigned(lhs: u64, rhs: u64) -> Option { - if lhs >= rhs { - IntUtil::checked_add_unsigned(0i64, lhs - rhs) - } else { - IntUtil::checked_sub_unsigned(0i64, rhs - lhs) - } - } } #[cfg(test)] @@ -742,122 +676,4 @@ mod tests { assert_eq!(acct2, costliest_account); } } - - #[test] - fn test_adjust_transaction_execution_cost() { - let acct1 = Pubkey::new_unique(); - let acct2 = Pubkey::new_unique(); - let acct3 = Pubkey::new_unique(); - let cost = 100; - let account_max = cost * 2; - let block_max = account_max * 3; // for three accts - - let mut testee = CostTracker::new(account_max, block_max, block_max, None); - let tx_cost = TransactionCost { - writable_accounts: vec![acct1, acct2, acct3], - execution_cost: cost, - ..TransactionCost::default() - }; - let mut expected_block_cost = tx_cost.sum(); - let expected_tx_count = 1; - assert!(testee.try_add(&tx_cost).is_ok()); - assert_eq!(expected_block_cost, testee.block_cost()); - assert_eq!(expected_tx_count, testee.transaction_count()); - testee - .cost_by_writable_accounts - .iter() - .for_each(|(_key, units)| { - assert_eq!(expected_block_cost, *units); - }); - - // adjust up - { - let adjustment = 50i64; - testee.adjust_transaction_execution_cost(&tx_cost, adjustment); - expected_block_cost += 50; - assert_eq!(expected_block_cost, testee.block_cost()); - assert_eq!(expected_tx_count, testee.transaction_count()); - testee - .cost_by_writable_accounts - .iter() - .for_each(|(_key, units)| { - assert_eq!(expected_block_cost, *units); - }); - } - - // adjust down - { - let adjustment = -50i64; - testee.adjust_transaction_execution_cost(&tx_cost, adjustment); - expected_block_cost -= 50; - assert_eq!(expected_block_cost, testee.block_cost()); - assert_eq!(expected_tx_count, testee.transaction_count()); - testee - .cost_by_writable_accounts - .iter() - .for_each(|(_key, units)| { - assert_eq!(expected_block_cost, *units); - }); - } - - // adjust overflow up - { - testee.adjust_transaction_execution_cost(&tx_cost, i64::MAX); - testee.adjust_transaction_execution_cost(&tx_cost, i64::MAX); - // expect block cost set to limit - assert_eq!(block_max, testee.block_cost()); - assert_eq!(expected_tx_count, testee.transaction_count()); - testee - .cost_by_writable_accounts - .iter() - .for_each(|(_key, units)| { - assert_eq!(account_max, *units); - }); - } - - // adjust overflow down - { - testee.adjust_transaction_execution_cost(&tx_cost, i64::MIN); - testee.adjust_transaction_execution_cost(&tx_cost, i64::MIN); - // expect block cost set to limit - assert_eq!(block_max, testee.block_cost()); - assert_eq!(expected_tx_count, testee.transaction_count()); - testee - .cost_by_writable_accounts - .iter() - .for_each(|(_key, units)| { - assert_eq!(account_max, *units); - }); - } - } - - #[test] - fn test_checked_diff_unsigned() { - solana_logger::setup(); - - // no diff - assert_eq!(0, CostTracker::checked_diff_unsigned(10u64, 10u64).unwrap()); - assert_eq!( - 0, - CostTracker::checked_diff_unsigned(u64::MAX, u64::MAX).unwrap() - ); - - // positive diff - assert_eq!(2, CostTracker::checked_diff_unsigned(10u64, 8u64).unwrap()); - assert_eq!( - 8, - CostTracker::checked_diff_unsigned(u64::MAX, u64::MAX - 8u64).unwrap() - ); - - // negative diff - assert_eq!(-2, CostTracker::checked_diff_unsigned(8u64, 10u64).unwrap()); - assert_eq!( - -10, - CostTracker::checked_diff_unsigned(u64::MIN, 10u64).unwrap() - ); - - // diff too big for i64 (overflow) - assert_eq!(None, CostTracker::checked_diff_unsigned(u64::MAX, 0u64)); - assert_eq!(None, CostTracker::checked_diff_unsigned(0u64, u64::MAX)); - } }