From 139b9c8c2583a12f84f4c49a9f4bfe57a430bcaa Mon Sep 17 00:00:00 2001 From: Tao Zhu <82401714+tao-stones@users.noreply.github.com> Date: Fri, 23 Feb 2024 08:58:48 -0600 Subject: [PATCH] Add fee_details to fee calculation (#35021) * add fee_details to fee calculation * fix - no need to round after summing u64 * feature gate on removing unwanted rounding --- core/src/banking_stage/consumer.rs | 2 + .../scheduler_controller.rs | 11 +++- programs/sbf/tests/programs.rs | 2 + runtime/src/bank.rs | 7 +- runtime/src/bank/tests.rs | 3 +- sdk/src/feature_set.rs | 5 ++ sdk/src/fee.rs | 64 +++++++++++++++++-- svm/src/account_loader.rs | 8 ++- 8 files changed, 89 insertions(+), 13 deletions(-) diff --git a/core/src/banking_stage/consumer.rs b/core/src/banking_stage/consumer.rs index 660dc2ac9..81de74022 100644 --- a/core/src/banking_stage/consumer.rs +++ b/core/src/banking_stage/consumer.rs @@ -747,6 +747,8 @@ impl Consumer { bank.feature_set.is_active( &feature_set::include_loaded_accounts_data_size_in_fee_calculation::id(), ), + bank.feature_set + .is_active(&feature_set::remove_rounding_in_fee_calculation::id()), ); let (mut fee_payer_account, _slot) = bank .rc diff --git a/core/src/banking_stage/transaction_scheduler/scheduler_controller.rs b/core/src/banking_stage/transaction_scheduler/scheduler_controller.rs index 7d9a70931..b0c5e0f6a 100644 --- a/core/src/banking_stage/transaction_scheduler/scheduler_controller.rs +++ b/core/src/banking_stage/transaction_scheduler/scheduler_controller.rs @@ -25,8 +25,13 @@ use { solana_runtime::{bank::Bank, bank_forks::BankForks}, solana_sdk::{ clock::MAX_PROCESSING_AGE, - feature_set::include_loaded_accounts_data_size_in_fee_calculation, fee::FeeBudgetLimits, - saturating_add_assign, transaction::SanitizedTransaction, + feature_set::{ + include_loaded_accounts_data_size_in_fee_calculation, + remove_rounding_in_fee_calculation, + }, + fee::FeeBudgetLimits, + saturating_add_assign, + transaction::SanitizedTransaction, }, solana_svm::transaction_error_metrics::TransactionErrorMetrics, std::{ @@ -422,6 +427,8 @@ impl SchedulerController { fee_budget_limits, bank.feature_set .is_active(&include_loaded_accounts_data_size_in_fee_calculation::id()), + bank.feature_set + .is_active(&remove_rounding_in_fee_calculation::id()), ); // We need a multiplier here to avoid rounding down too aggressively. diff --git a/programs/sbf/tests/programs.rs b/programs/sbf/tests/programs.rs index 943713d7a..1635850bb 100644 --- a/programs/sbf/tests/programs.rs +++ b/programs/sbf/tests/programs.rs @@ -3713,6 +3713,7 @@ fn test_program_fees() { .unwrap_or_default() .into(), false, + true, ); bank_client .send_and_confirm_message(&[&mint_keypair], message) @@ -3736,6 +3737,7 @@ fn test_program_fees() { .unwrap_or_default() .into(), false, + true, ); assert!(expected_normal_fee < expected_prioritized_fee); diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 7e051019c..29dde36ac 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -117,7 +117,10 @@ use { epoch_info::EpochInfo, epoch_schedule::EpochSchedule, feature, - feature_set::{self, include_loaded_accounts_data_size_in_fee_calculation, FeatureSet}, + feature_set::{ + self, include_loaded_accounts_data_size_in_fee_calculation, + remove_rounding_in_fee_calculation, FeatureSet, + }, fee::FeeStructure, fee_calculator::{FeeCalculator, FeeRateGovernor}, genesis_config::{ClusterType, GenesisConfig}, @@ -4016,6 +4019,8 @@ impl Bank { .into(), self.feature_set .is_active(&include_loaded_accounts_data_size_in_fee_calculation::id()), + self.feature_set + .is_active(&remove_rounding_in_fee_calculation::id()), ) } diff --git a/runtime/src/bank/tests.rs b/runtime/src/bank/tests.rs index 15df308ae..5f5d0884a 100644 --- a/runtime/src/bank/tests.rs +++ b/runtime/src/bank/tests.rs @@ -3333,7 +3333,6 @@ fn test_bank_parent_account_spend() { let key2 = Keypair::new(); let (parent, bank_forks) = Bank::new_with_bank_forks_for_tests(&genesis_config); let amount = genesis_config.rent.minimum_balance(0); - println!("==== amount {}", amount); let tx = system_transaction::transfer(&mint_keypair, &key1.pubkey(), amount, genesis_config.hash()); @@ -10029,7 +10028,7 @@ fn calculate_test_fee( .unwrap_or_default() .into(); - fee_structure.calculate_fee(message, lamports_per_signature, &budget_limits, false) + fee_structure.calculate_fee(message, lamports_per_signature, &budget_limits, false, true) } #[test] diff --git a/sdk/src/feature_set.rs b/sdk/src/feature_set.rs index 826876732..abecf4faf 100644 --- a/sdk/src/feature_set.rs +++ b/sdk/src/feature_set.rs @@ -780,6 +780,10 @@ pub mod enable_chained_merkle_shreds { solana_sdk::declare_id!("7uZBkJXJ1HkuP6R3MJfZs7mLwymBcDbKdqbF51ZWLier"); } +pub mod remove_rounding_in_fee_calculation { + solana_sdk::declare_id!("BtVN7YjDzNE6Dk7kTT7YTDgMNUZTNgiSJgsdzAeTg2jF"); +} + lazy_static! { /// Map of feature identifiers to user-visible description pub static ref FEATURE_NAMES: HashMap = [ @@ -970,6 +974,7 @@ lazy_static! { (cost_model_requested_write_lock_cost::id(), "cost model uses number of requested write locks #34819"), (enable_gossip_duplicate_proof_ingestion::id(), "enable gossip duplicate proof ingestion #32963"), (enable_chained_merkle_shreds::id(), "Enable chained Merkle shreds #34916"), + (remove_rounding_in_fee_calculation::id(), "Removing unwanted rounding in fee calculation #34982"), /*************** ADD NEW FEATURES HERE ***************/ ] .iter() diff --git a/sdk/src/fee.rs b/sdk/src/fee.rs index f3377b525..b325a23ac 100644 --- a/sdk/src/fee.rs +++ b/sdk/src/fee.rs @@ -31,6 +31,24 @@ pub struct FeeStructure { pub compute_fee_bins: Vec, } +#[derive(Debug, Default, Clone, Eq, PartialEq)] +pub struct FeeDetails { + transaction_fee: u64, + prioritization_fee: u64, +} + +impl FeeDetails { + pub fn total_fee(&self, remove_rounding_in_fee_calculation: bool) -> u64 { + let total_fee = self.transaction_fee.saturating_add(self.prioritization_fee); + if remove_rounding_in_fee_calculation { + total_fee + } else { + // backward compatible behavior + (total_fee as f64).round() as u64 + } + } +} + pub const ACCOUNT_DATA_COST_PAGE_SIZE: u64 = 32_u64.saturating_mul(1024); impl FeeStructure { @@ -83,6 +101,7 @@ impl FeeStructure { lamports_per_signature: u64, budget_limits: &FeeBudgetLimits, include_loaded_account_data_size_in_fee: bool, + remove_rounding_in_fee_calculation: bool, ) -> u64 { // Fee based on compute units and signatures let congestion_multiplier = if lamports_per_signature == 0 { @@ -91,6 +110,23 @@ impl FeeStructure { 1.0 // multiplier that has no effect }; + self.calculate_fee_details( + message, + budget_limits, + include_loaded_account_data_size_in_fee, + ) + .total_fee(remove_rounding_in_fee_calculation) + .saturating_mul(congestion_multiplier as u64) + } + + /// Calculate fee details for `SanitizedMessage` + #[cfg(not(target_os = "solana"))] + pub fn calculate_fee_details( + &self, + message: &SanitizedMessage, + budget_limits: &FeeBudgetLimits, + include_loaded_account_data_size_in_fee: bool, + ) -> FeeDetails { let signature_fee = message .num_signatures() .saturating_mul(self.lamports_per_signature); @@ -122,13 +158,12 @@ impl FeeStructure { .unwrap_or_default() }); - ((budget_limits - .prioritization_fee - .saturating_add(signature_fee) - .saturating_add(write_lock_fee) - .saturating_add(compute_fee) as f64) - * congestion_multiplier) - .round() as u64 + FeeDetails { + transaction_fee: signature_fee + .saturating_add(write_lock_fee) + .saturating_add(compute_fee), + prioritization_fee: budget_limits.prioritization_fee, + } } } @@ -180,4 +215,19 @@ mod tests { FeeStructure::calculate_memory_usage_cost(64 * K, heap_cost) ); } + + #[test] + fn test_total_fee_rounding() { + // round large `f64` can lost precision, see feature gate: + // "Removing unwanted rounding in fee calculation #34982" + + let large_fee_details = FeeDetails { + transaction_fee: u64::MAX - 11, + prioritization_fee: 1, + }; + let expected_large_fee = u64::MAX - 10; + + assert_eq!(large_fee_details.total_fee(true), expected_large_fee); + assert_ne!(large_fee_details.total_fee(false), expected_large_fee); + } } diff --git a/svm/src/account_loader.rs b/svm/src/account_loader.rs index 854d59bac..334ad7679 100644 --- a/svm/src/account_loader.rs +++ b/svm/src/account_loader.rs @@ -15,7 +15,10 @@ use { create_executable_meta, is_builtin, is_executable, Account, AccountSharedData, ReadableAccount, WritableAccount, }, - feature_set::{self, include_loaded_accounts_data_size_in_fee_calculation}, + feature_set::{ + self, include_loaded_accounts_data_size_in_fee_calculation, + remove_rounding_in_fee_calculation, + }, fee::FeeStructure, message::SanitizedMessage, native_loader, @@ -74,6 +77,7 @@ pub fn load_accounts( .into(), feature_set .is_active(&include_loaded_accounts_data_size_in_fee_calculation::id()), + feature_set.is_active(&remove_rounding_in_fee_calculation::id()), ) } else { return (Err(TransactionError::BlockhashNotFound), None); @@ -682,6 +686,7 @@ mod tests { .unwrap_or_default() .into(), false, + true, ); assert_eq!(fee, lamports_per_signature); @@ -1210,6 +1215,7 @@ mod tests { .unwrap_or_default() .into(), false, + true, ); assert_eq!(fee, lamports_per_signature + prioritization_fee);