From 5287d86bceb7d3550ce7398d20c37762da146b75 Mon Sep 17 00:00:00 2001 From: Tao Zhu Date: Thu, 29 Feb 2024 00:58:19 +0000 Subject: [PATCH 1/7] 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 e2ab858660361f..1ff6b965ae7866 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -124,7 +124,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, @@ -256,6 +256,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 @@ -551,6 +569,7 @@ impl PartialEq for Bank { epoch_reward_status: _, transaction_processor: _, check_program_modification_slot: _, + 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. @@ -584,6 +603,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() } } @@ -813,6 +834,9 @@ pub struct Bank { transaction_processor: TransactionBatchProcessor, check_program_modification_slot: bool, + + /// Collected fee details + collector_fee_details: RwLock, } struct VoteWithStakeDelegations { @@ -1000,6 +1024,7 @@ impl Bank { epoch_reward_status: EpochRewardStatus::default(), transaction_processor: TransactionBatchProcessor::default(), check_program_modification_slot: false, + collector_fee_details: RwLock::new(CollectorFeeDetails::default()), }; bank.transaction_processor = TransactionBatchProcessor::new( @@ -1319,6 +1344,7 @@ impl Bank { epoch_reward_status: parent.epoch_reward_status.clone(), transaction_processor: TransactionBatchProcessor::default(), check_program_modification_slot: false, + collector_fee_details: RwLock::new(CollectorFeeDetails::default()), }; new.transaction_processor = TransactionBatchProcessor::new( @@ -1866,6 +1892,8 @@ impl Bank { epoch_reward_status: fields.epoch_reward_status, transaction_processor: TransactionBatchProcessor::default(), check_program_modification_slot: false, + // collector_fee_details is not serialized to snapshot + collector_fee_details: RwLock::new(CollectorFeeDetails::default()), }; bank.transaction_processor = TransactionBatchProcessor::new( @@ -4903,6 +4931,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 27ba852ca201dd..a493bb383ed602 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 45643153864b82d0933712dcd4bb402a7438c8b6 Mon Sep 17 00:00:00 2001 From: Tao Zhu Date: Mon, 11 Mar 2024 18:15:48 +0000 Subject: [PATCH 2/7] 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 1ff6b965ae7866..ff401fe74f8f2f 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -256,8 +256,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 23b5ff0db780da7aff1333ca3f710f679a8d148c Mon Sep 17 00:00:00 2001 From: Tao Zhu Date: Tue, 12 Mar 2024 03:00:28 +0000 Subject: [PATCH 3/7] 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 ff401fe74f8f2f..2f5d24d45ee233 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -263,7 +263,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()); @@ -568,7 +568,7 @@ impl PartialEq for Bank { epoch_reward_status: _, transaction_processor: _, check_program_modification_slot: _, - 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. @@ -602,8 +602,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() } } @@ -4986,7 +4984,7 @@ impl Bank { self.collector_fee_details .write() .unwrap() - .add(&accumulated_fee_details); + .accumulate(&accumulated_fee_details); results } From cdc35540c899a1bb7101e9cb3c7a3158bb2641dd Mon Sep 17 00:00:00 2001 From: Tao Zhu Date: Tue, 12 Mar 2024 03:11:48 +0000 Subject: [PATCH 4/7] 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 2f5d24d45ee233..b8450ae470f30e 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -256,7 +256,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 27cd942f3cf48ab89708ce37f59accf059f0e1fb Mon Sep 17 00:00:00 2001 From: Tao Zhu Date: Tue, 12 Mar 2024 21:00:25 +0000 Subject: [PATCH 5/7] 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 f104c8ee2b963d..8525c9a01b27d2 100644 --- a/runtime/src/bank/tests.rs +++ b/runtime/src/bank/tests.rs @@ -13792,3 +13792,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 7bde1953d023f819b5ee8de8eca3a56fb7f54eab Mon Sep 17 00:00:00 2001 From: Tao Zhu Date: Fri, 15 Mar 2024 19:09:16 +0000 Subject: [PATCH 6/7] share function to withdraw errored transaction --- runtime/src/bank.rs | 62 ++++++++++++++++++++++++++------------------- 1 file changed, 36 insertions(+), 26 deletions(-) diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index b8450ae470f30e..a3e81eb26086f1 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -4908,16 +4908,12 @@ impl Bank { lamports_per_signature, ); - // 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)?; - } + self.check_execution_status_and_charge_fee( + tx.message(), + execution_status, + is_nonce, + fee, + )?; fees += fee; Ok(()) @@ -4959,22 +4955,15 @@ impl Bank { .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()), - ), - )?; - } + self.check_execution_status_and_charge_fee( + message, + execution_status, + is_nonce, + fee_details.total_fee( + self.feature_set + .is_active(&remove_rounding_in_fee_calculation::id()), + ), + )?; accumulated_fee_details.accumulate(&fee_details); Ok(()) @@ -4988,6 +4977,27 @@ impl Bank { results } + fn check_execution_status_and_charge_fee( + &self, + message: &SanitizedMessage, + execution_status: &transaction::Result<()>, + is_nonce: bool, + fee: u64, + ) -> Result<()> { + // 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(message.fee_payer(), fee)?; + } + + Ok(()) + } + /// `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 From f59c93da6e0586a0a3e7896822b08fa68f53b712 Mon Sep 17 00:00:00 2001 From: Tao Zhu Date: Mon, 18 Mar 2024 16:42:14 +0000 Subject: [PATCH 7/7] more tests --- runtime/src/bank/tests.rs | 95 +++++++++++++++++++++++++++++++-------- 1 file changed, 77 insertions(+), 18 deletions(-) diff --git a/runtime/src/bank/tests.rs b/runtime/src/bank/tests.rs index 8525c9a01b27d2..ea2354ef8e3586 100644 --- a/runtime/src/bank/tests.rs +++ b/runtime/src/bank/tests.rs @@ -13795,20 +13795,29 @@ fn test_failed_simulation_compute_units() { #[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 + // TX | EXECUTION RESULT | is nonce | COLLECT | ADDITIONAL | COLLECT + // | | | (TX_FEE, PRIO_FEE) | WITHDRAW FROM PAYER | RESULT + // ------------------------------------------------------------------------------------------------------ + // tx1 | not executed | n/a | (0 , 0) | 0 | Original Err + // tx2 | executed and no error | n/a | (5_000, 1_000) | 0 | Ok + // tx3 | executed has error | true | (5_000, 1_000) | 0 | Ok + // tx4 | executed has error | false | (5_000, 1_000) | 6_000 | Ok + // tx5 | executed error, + // payer insufficient fund | false | (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, + transaction_fee: 15_000, + priority_fee: 3_000, }; + let expected_collect_results = vec![ + Err(TransactionError::AccountNotFound), + Ok(()), + Ok(()), + Ok(()), + Err(TransactionError::InsufficientFundsForFee), + ]; let GenesisConfigInfo { mut genesis_config, @@ -13828,10 +13837,18 @@ fn test_filter_program_errors_and_collect_fee_details() { &[&mint_keypair], genesis_config.hash(), )); - let txs = vec![tx.clone(), tx.clone(), tx.clone(), tx]; + let txs = vec![tx.clone(), tx.clone(), tx.clone(), tx.clone(), tx]; let results = vec![ + TransactionExecutionResult::NotExecuted(TransactionError::AccountNotFound), new_execution_result(Ok(()), None), + new_execution_result( + Err(TransactionError::InstructionError( + 1, + SystemError::ResultWithNegativeLamports.into(), + )), + Some(&NonceFull::default()), + ), new_execution_result( Err(TransactionError::InstructionError( 1, @@ -13839,17 +13856,9 @@ fn test_filter_program_errors_and_collect_fee_details() { )), 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!( @@ -13862,3 +13871,53 @@ fn test_filter_program_errors_and_collect_fee_details() { ); assert_eq!(expected_collect_results, results); } + +#[test] +fn test_check_execution_status_and_charge_fee() { + let fee = 5000; + let initial_balance = fee - 1000; + let tx_error = + TransactionError::InstructionError(0, InstructionError::MissingRequiredSignature); + let GenesisConfigInfo { + mut genesis_config, + mint_keypair, + .. + } = create_genesis_config_with_leader(initial_balance, &Pubkey::new_unique(), 3); + genesis_config.fee_rate_governor = FeeRateGovernor::new(5000, 0); + let bank = Bank::new_for_tests(&genesis_config); + let message = new_sanitized_message(Message::new( + &[system_instruction::transfer( + &mint_keypair.pubkey(), + &Pubkey::new_unique(), + 1, + )], + Some(&mint_keypair.pubkey()), + )); + + [Ok(()), Err(tx_error)] + .iter() + .flat_map(|result| [true, false].iter().map(move |is_nonce| (result, is_nonce))) + .for_each(|(result, is_nonce)| { + if result.is_err() && !is_nonce { + assert_eq!( + Err(TransactionError::InsufficientFundsForFee), + bank.check_execution_status_and_charge_fee(&message, result, *is_nonce, fee) + ); + assert_eq!(initial_balance, bank.get_balance(&mint_keypair.pubkey())); + + let small_fee = 1; + assert!(bank + .check_execution_status_and_charge_fee(&message, result, *is_nonce, small_fee) + .is_ok()); + assert_eq!( + initial_balance - small_fee, + bank.get_balance(&mint_keypair.pubkey()) + ); + } else { + assert!(bank + .check_execution_status_and_charge_fee(&message, result, *is_nonce, fee) + .is_ok()); + assert_eq!(initial_balance, bank.get_balance(&mint_keypair.pubkey())); + } + }); +}