From 41658f2f589fb09fbabead6888c2ac6c06d39b6a Mon Sep 17 00:00:00 2001 From: Tao Zhu Date: Thu, 29 Feb 2024 00:58:19 +0000 Subject: [PATCH 1/8] Add functions to collect executed transactions fee in details; --- runtime/src/bank.rs | 90 ++++++++++++++++++++++++++++++++++++++++++++- sdk/src/fee.rs | 17 +++++++++ 2 files changed, 106 insertions(+), 1 deletion(-) diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index d1a1805d0d3a20..bb1bf3f5da8a0a 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -122,7 +122,7 @@ use { self, include_loaded_accounts_data_size_in_fee_calculation, remove_rounding_in_fee_calculation, FeatureSet, }, - fee::FeeStructure, + fee::{FeeDetails, FeeStructure}, fee_calculator::{FeeCalculator, FeeRateGovernor}, genesis_config::{ClusterType, GenesisConfig}, hard_forks::HardForks, @@ -253,6 +253,24 @@ impl AddAssign for SquashTiming { } } +#[derive(Serialize, Deserialize, Clone, Debug, Default, PartialEq)] +#[serde(rename_all = "camelCase")] +pub(crate) struct CollectorFeeDetails { + pub transaction_fee: u64, + pub priority_fee: u64, +} + +impl CollectorFeeDetails { + pub(crate) fn add(&mut self, fee_details: &FeeDetails) { + self.transaction_fee = self + .transaction_fee + .saturating_add(fee_details.transaction_fee()); + self.priority_fee = self + .priority_fee + .saturating_add(fee_details.prioritization_fee()); + } +} + #[derive(Debug)] pub struct BankRc { /// where all the Accounts are stored @@ -548,6 +566,7 @@ impl PartialEq for Bank { loaded_programs_cache: _, epoch_reward_status: _, transaction_processor: _, + collector_fee_details, // Ignore new fields explicitly if they do not impact PartialEq. // Adding ".." will remove compile-time checks that if a new field // is added to the struct, this PartialEq is accordingly updated. @@ -581,6 +600,8 @@ impl PartialEq for Bank { && *stakes_cache.stakes() == *other.stakes_cache.stakes() && epoch_stakes == &other.epoch_stakes && is_delta.load(Relaxed) == other.is_delta.load(Relaxed) + && *collector_fee_details.read().unwrap() + == *other.collector_fee_details.read().unwrap() } } @@ -808,6 +829,9 @@ pub struct Bank { epoch_reward_status: EpochRewardStatus, transaction_processor: TransactionBatchProcessor, + + /// Collected fee details + collector_fee_details: RwLock, } struct VoteWithStakeDelegations { @@ -994,6 +1018,7 @@ impl Bank { ))), epoch_reward_status: EpochRewardStatus::default(), transaction_processor: TransactionBatchProcessor::default(), + collector_fee_details: RwLock::new(CollectorFeeDetails::default()), }; bank.transaction_processor = TransactionBatchProcessor::new( @@ -1312,6 +1337,7 @@ impl Bank { loaded_programs_cache: parent.loaded_programs_cache.clone(), epoch_reward_status: parent.epoch_reward_status.clone(), transaction_processor: TransactionBatchProcessor::default(), + collector_fee_details: RwLock::new(CollectorFeeDetails::default()), }; new.transaction_processor = TransactionBatchProcessor::new( @@ -1832,6 +1858,8 @@ impl Bank { ))), epoch_reward_status: fields.epoch_reward_status, transaction_processor: TransactionBatchProcessor::default(), + // collector_fee_details is not serialized to snapshot + collector_fee_details: RwLock::new(CollectorFeeDetails::default()), }; bank.transaction_processor = TransactionBatchProcessor::new( @@ -4871,6 +4899,66 @@ impl Bank { results } + // Note: this function is not yet used; next PR will call it behind a feature gate + #[allow(dead_code)] + fn filter_program_errors_and_collect_fee_details( + &self, + txs: &[SanitizedTransaction], + execution_results: &[TransactionExecutionResult], + ) -> Vec> { + let mut accumulated_fee_details = FeeDetails::default(); + + let results = txs + .iter() + .zip(execution_results) + .map(|(tx, execution_result)| { + let (execution_status, durable_nonce_fee) = match &execution_result { + TransactionExecutionResult::Executed { details, .. } => { + Ok((&details.status, details.durable_nonce_fee.as_ref())) + } + TransactionExecutionResult::NotExecuted(err) => Err(err.clone()), + }?; + let is_nonce = durable_nonce_fee.is_some(); + + let message = tx.message(); + let fee_details = self.fee_structure.calculate_fee_details( + message, + &process_compute_budget_instructions(message.program_instructions_iter()) + .unwrap_or_default() + .into(), + self.feature_set + .is_active(&include_loaded_accounts_data_size_in_fee_calculation::id()), + ); + + // In case of instruction error, even though no accounts + // were stored we still need to charge the payer the + // fee. + // + //...except nonce accounts, which already have their + // post-load, fee deducted, pre-execute account state + // stored + if execution_status.is_err() && !is_nonce { + self.withdraw( + tx.message().fee_payer(), + fee_details.total_fee( + self.feature_set + .is_active(&remove_rounding_in_fee_calculation::id()), + ), + )?; + } + + accumulated_fee_details.accumulate(&fee_details); + Ok(()) + }) + .collect(); + + self.collector_fee_details + .write() + .unwrap() + .add(&accumulated_fee_details); + results + } + /// `committed_transactions_count` is the number of transactions out of `sanitized_txs` /// that was executed. Of those, `committed_transactions_count`, /// `committed_with_failure_result_count` is the number of executed transactions that returned diff --git a/sdk/src/fee.rs b/sdk/src/fee.rs index b325a23ac08d9d..25c4b62acdf61b 100644 --- a/sdk/src/fee.rs +++ b/sdk/src/fee.rs @@ -47,6 +47,23 @@ impl FeeDetails { (total_fee as f64).round() as u64 } } + + pub fn accumulate(&mut self, fee_details: &FeeDetails) { + self.transaction_fee = self + .transaction_fee + .saturating_add(fee_details.transaction_fee); + self.prioritization_fee = self + .prioritization_fee + .saturating_add(fee_details.prioritization_fee) + } + + pub fn transaction_fee(&self) -> u64 { + self.transaction_fee + } + + pub fn prioritization_fee(&self) -> u64 { + self.prioritization_fee + } } pub const ACCOUNT_DATA_COST_PAGE_SIZE: u64 = 32_u64.saturating_mul(1024); From 0b1c908a134ec71975c52b4ea02e0ec830e9049b Mon Sep 17 00:00:00 2001 From: Tao Zhu Date: Mon, 11 Mar 2024 18:15:48 +0000 Subject: [PATCH 2/8] remove unnecessary derive attributes --- runtime/src/bank.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index bb1bf3f5da8a0a..9886f9b2dddd21 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -253,8 +253,7 @@ impl AddAssign for SquashTiming { } } -#[derive(Serialize, Deserialize, Clone, Debug, Default, PartialEq)] -#[serde(rename_all = "camelCase")] +#[derive(Debug, Default, PartialEq)] pub(crate) struct CollectorFeeDetails { pub transaction_fee: u64, pub priority_fee: u64, From 4cf1b49d6dc67f6affd08d6c97c2566e0b8b49f2 Mon Sep 17 00:00:00 2001 From: Tao Zhu Date: Tue, 12 Mar 2024 03:00:28 +0000 Subject: [PATCH 3/8] change function name from add to accumulate; remove collector_fee_details from PartialEq --- runtime/src/bank.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 9886f9b2dddd21..6fff47afce5fb3 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -260,7 +260,7 @@ pub(crate) struct CollectorFeeDetails { } impl CollectorFeeDetails { - pub(crate) fn add(&mut self, fee_details: &FeeDetails) { + pub(crate) fn accumulate(&mut self, fee_details: &FeeDetails) { self.transaction_fee = self .transaction_fee .saturating_add(fee_details.transaction_fee()); @@ -565,7 +565,7 @@ impl PartialEq for Bank { loaded_programs_cache: _, epoch_reward_status: _, transaction_processor: _, - collector_fee_details, + collector_fee_details: _, // Ignore new fields explicitly if they do not impact PartialEq. // Adding ".." will remove compile-time checks that if a new field // is added to the struct, this PartialEq is accordingly updated. @@ -599,8 +599,6 @@ impl PartialEq for Bank { && *stakes_cache.stakes() == *other.stakes_cache.stakes() && epoch_stakes == &other.epoch_stakes && is_delta.load(Relaxed) == other.is_delta.load(Relaxed) - && *collector_fee_details.read().unwrap() - == *other.collector_fee_details.read().unwrap() } } @@ -4954,7 +4952,7 @@ impl Bank { self.collector_fee_details .write() .unwrap() - .add(&accumulated_fee_details); + .accumulate(&accumulated_fee_details); results } From e032eeb1f7ede3d21471714210ccd10c84caa72b Mon Sep 17 00:00:00 2001 From: Tao Zhu Date: Tue, 12 Mar 2024 03:11:48 +0000 Subject: [PATCH 4/8] add AbiExample --- runtime/src/bank.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 6fff47afce5fb3..0b9bb1ab0b799e 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -253,7 +253,7 @@ impl AddAssign for SquashTiming { } } -#[derive(Debug, Default, PartialEq)] +#[derive(AbiExample, Debug, Default, PartialEq)] pub(crate) struct CollectorFeeDetails { pub transaction_fee: u64, pub priority_fee: u64, From ca0ae44681bd4ba0e6c9f2af8b3756ddaf199cc9 Mon Sep 17 00:00:00 2001 From: Tao Zhu Date: Tue, 12 Mar 2024 21:00:25 +0000 Subject: [PATCH 5/8] add test --- runtime/src/bank/tests.rs | 70 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 70 insertions(+) diff --git a/runtime/src/bank/tests.rs b/runtime/src/bank/tests.rs index 29dbdc2e5aeacd..56ebd51916ebf7 100644 --- a/runtime/src/bank/tests.rs +++ b/runtime/src/bank/tests.rs @@ -13794,3 +13794,73 @@ fn test_failed_simulation_compute_units() { let simulation = bank.simulate_transaction(&sanitized, false); assert_eq!(expected_consumed_units, simulation.units_consumed); } + +#[test] +fn test_filter_program_errors_and_collect_fee_details() { + // TX | EXECUTION RESULT | COLLECT | ADDITIONAL | COLLECT + // | (TX_FEE, PRIO_FEE) | WITHDRAW FROM PAYER | RESULT + // -------------------------------------------------------------------------------------------------- + // tx1 | executed and no error | (5_000, 1_000) | 0 | Ok + // tx2 | executed has error | (5_000, 1_000) | 6_000 | Ok + // tx3 | not executed | (0 , 0) | 0 | Original Err + // tx4 | executed error, payer insufficient fund | (0 , 0) | 0 | InsufficientFundsForFee + // + let initial_payer_balance = 7_000; + let additional_payer_withdraw = 6_000; + let expected_collected_fee_details = CollectorFeeDetails { + transaction_fee: 10_000, + priority_fee: 2_000, + }; + + let GenesisConfigInfo { + mut genesis_config, + mint_keypair, + .. + } = create_genesis_config_with_leader(initial_payer_balance, &Pubkey::new_unique(), 3); + genesis_config.fee_rate_governor = FeeRateGovernor::new(5000, 0); + let bank = Bank::new_for_tests(&genesis_config); + + let tx = SanitizedTransaction::from_transaction_for_tests(Transaction::new_signed_with_payer( + &[ + system_instruction::transfer(&mint_keypair.pubkey(), &Pubkey::new_unique(), 2), + ComputeBudgetInstruction::set_compute_unit_limit(1_000), + ComputeBudgetInstruction::set_compute_unit_price(1_000_000), + ], + Some(&mint_keypair.pubkey()), + &[&mint_keypair], + genesis_config.hash(), + )); + let txs = vec![tx.clone(), tx.clone(), tx.clone(), tx]; + + let results = vec![ + new_execution_result(Ok(()), None), + new_execution_result( + Err(TransactionError::InstructionError( + 1, + SystemError::ResultWithNegativeLamports.into(), + )), + None, + ), + TransactionExecutionResult::NotExecuted(TransactionError::AccountNotFound), + new_execution_result(Err(TransactionError::AccountNotFound), None), + ]; + + let expected_collect_results = vec![ + Ok(()), + Ok(()), + Err(TransactionError::AccountNotFound), + Err(TransactionError::InsufficientFundsForFee), + ]; + + let results = bank.filter_program_errors_and_collect_fee_details(&txs, &results); + + assert_eq!( + expected_collected_fee_details, + *bank.collector_fee_details.read().unwrap() + ); + assert_eq!( + initial_payer_balance - additional_payer_withdraw, + bank.get_balance(&mint_keypair.pubkey()) + ); + assert_eq!(expected_collect_results, results); +} From 1af263316cda8276079b10b39b010b3b14245400 Mon Sep 17 00:00:00 2001 From: Tao Zhu Date: Wed, 13 Mar 2024 20:49:10 +0000 Subject: [PATCH 6/8] use target function for benching --- runtime/src/bank.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 0b9bb1ab0b799e..69b153fadf7c6a 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -3710,6 +3710,14 @@ impl Bank { self.update_slot_history(); self.run_incinerator(); + // to bench target function + { + let CollectorFeeDetails { + transaction_fee: _, + priority_fee: _, + } = *self.collector_fee_details.read().unwrap(); + } + // freeze is a one-way trip, idempotent self.freeze_started.store(true, Relaxed); *hash = self.hash_internal_state(); @@ -5069,6 +5077,8 @@ impl Bank { self.update_transaction_statuses(sanitized_txs, &execution_results); let fee_collection_results = self.filter_program_errors_and_collect_fee(sanitized_txs, &execution_results); + // to bench target function + let _ = self.filter_program_errors_and_collect_fee_details(sanitized_txs, &execution_results); update_transaction_statuses_time.stop(); timings.saturating_add_in_place( ExecuteTimingType::UpdateTransactionStatuses, From 91ece909f37b7ec4fe2c2b1b7f4539638f010260 Mon Sep 17 00:00:00 2001 From: Tao Zhu Date: Wed, 13 Mar 2024 21:17:07 +0000 Subject: [PATCH 7/8] Use Mutex --- runtime/src/bank.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 69b153fadf7c6a..2a24bb18e5bc7c 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -828,7 +828,7 @@ pub struct Bank { transaction_processor: TransactionBatchProcessor, /// Collected fee details - collector_fee_details: RwLock, + collector_fee_details: Mutex, } struct VoteWithStakeDelegations { @@ -1015,7 +1015,7 @@ impl Bank { ))), epoch_reward_status: EpochRewardStatus::default(), transaction_processor: TransactionBatchProcessor::default(), - collector_fee_details: RwLock::new(CollectorFeeDetails::default()), + collector_fee_details: Mutex::new(CollectorFeeDetails::default()), }; bank.transaction_processor = TransactionBatchProcessor::new( @@ -1334,7 +1334,7 @@ impl Bank { loaded_programs_cache: parent.loaded_programs_cache.clone(), epoch_reward_status: parent.epoch_reward_status.clone(), transaction_processor: TransactionBatchProcessor::default(), - collector_fee_details: RwLock::new(CollectorFeeDetails::default()), + collector_fee_details: Mutex::new(CollectorFeeDetails::default()), }; new.transaction_processor = TransactionBatchProcessor::new( @@ -1856,7 +1856,7 @@ impl Bank { epoch_reward_status: fields.epoch_reward_status, transaction_processor: TransactionBatchProcessor::default(), // collector_fee_details is not serialized to snapshot - collector_fee_details: RwLock::new(CollectorFeeDetails::default()), + collector_fee_details: Mutex::new(CollectorFeeDetails::default()), }; bank.transaction_processor = TransactionBatchProcessor::new( @@ -3715,7 +3715,7 @@ impl Bank { let CollectorFeeDetails { transaction_fee: _, priority_fee: _, - } = *self.collector_fee_details.read().unwrap(); + } = *self.collector_fee_details.lock().unwrap(); } // freeze is a one-way trip, idempotent @@ -4958,7 +4958,7 @@ impl Bank { .collect(); self.collector_fee_details - .write() + .lock() .unwrap() .accumulate(&accumulated_fee_details); results From 0b77e2ba88ca42dd0e33e0ab910d9743d268889a Mon Sep 17 00:00:00 2001 From: Tao Zhu Date: Wed, 13 Mar 2024 21:30:32 +0000 Subject: [PATCH 8/8] use atomic --- runtime/src/bank.rs | 37 +++++++++++-------------------------- 1 file changed, 11 insertions(+), 26 deletions(-) diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 2a24bb18e5bc7c..15816f746cab45 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -253,21 +253,10 @@ impl AddAssign for SquashTiming { } } -#[derive(AbiExample, Debug, Default, PartialEq)] +#[derive(AbiExample, Debug, Default)] pub(crate) struct CollectorFeeDetails { - pub transaction_fee: u64, - pub priority_fee: u64, -} - -impl CollectorFeeDetails { - pub(crate) fn accumulate(&mut self, fee_details: &FeeDetails) { - self.transaction_fee = self - .transaction_fee - .saturating_add(fee_details.transaction_fee()); - self.priority_fee = self - .priority_fee - .saturating_add(fee_details.prioritization_fee()); - } + pub transaction_fee: AtomicU64, + pub priority_fee: AtomicU64, } #[derive(Debug)] @@ -828,7 +817,7 @@ pub struct Bank { transaction_processor: TransactionBatchProcessor, /// Collected fee details - collector_fee_details: Mutex, + collector_fee_details: Arc, } struct VoteWithStakeDelegations { @@ -1015,7 +1004,7 @@ impl Bank { ))), epoch_reward_status: EpochRewardStatus::default(), transaction_processor: TransactionBatchProcessor::default(), - collector_fee_details: Mutex::new(CollectorFeeDetails::default()), + collector_fee_details: Arc::new(CollectorFeeDetails::default()), }; bank.transaction_processor = TransactionBatchProcessor::new( @@ -1334,7 +1323,7 @@ impl Bank { loaded_programs_cache: parent.loaded_programs_cache.clone(), epoch_reward_status: parent.epoch_reward_status.clone(), transaction_processor: TransactionBatchProcessor::default(), - collector_fee_details: Mutex::new(CollectorFeeDetails::default()), + collector_fee_details: Arc::new(CollectorFeeDetails::default()), }; new.transaction_processor = TransactionBatchProcessor::new( @@ -1856,7 +1845,7 @@ impl Bank { epoch_reward_status: fields.epoch_reward_status, transaction_processor: TransactionBatchProcessor::default(), // collector_fee_details is not serialized to snapshot - collector_fee_details: Mutex::new(CollectorFeeDetails::default()), + collector_fee_details: Arc::new(CollectorFeeDetails::default()), }; bank.transaction_processor = TransactionBatchProcessor::new( @@ -3712,10 +3701,8 @@ impl Bank { // to bench target function { - let CollectorFeeDetails { - transaction_fee: _, - priority_fee: _, - } = *self.collector_fee_details.lock().unwrap(); + let _transaction_fee = self.collector_fee_details.transaction_fee.load(Relaxed); + let _priority_fee = self.collector_fee_details.priority_fee.load(Relaxed); } // freeze is a one-way trip, idempotent @@ -4957,10 +4944,8 @@ impl Bank { }) .collect(); - self.collector_fee_details - .lock() - .unwrap() - .accumulate(&accumulated_fee_details); + self.collector_fee_details.transaction_fee.fetch_add(accumulated_fee_details.transaction_fee(), Relaxed); + self.collector_fee_details.priority_fee.fetch_add(accumulated_fee_details.prioritization_fee(), Relaxed); results }