From c5be3f6aab6dfcd2be19983ee89b733c3e1b8f26 Mon Sep 17 00:00:00 2001 From: Tao Zhu Date: Wed, 31 Jan 2024 00:25:25 +0000 Subject: [PATCH 1/3] add fee_details to fee calculation --- runtime/src/bank/tests.rs | 1 - sdk/src/fee.rs | 45 +++++++++++++++++++++++++++++++++------ 2 files changed, 38 insertions(+), 8 deletions(-) diff --git a/runtime/src/bank/tests.rs b/runtime/src/bank/tests.rs index a01e9c19de6a39..3a966688783402 100644 --- a/runtime/src/bank/tests.rs +++ b/runtime/src/bank/tests.rs @@ -3336,7 +3336,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()); diff --git a/sdk/src/fee.rs b/sdk/src/fee.rs index f3377b5254f0a6..7a755ac823469e 100644 --- a/sdk/src/fee.rs +++ b/sdk/src/fee.rs @@ -31,6 +31,18 @@ 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) -> u64 { + self.transaction_fee.saturating_add(self.prioritization_fee) + } +} + pub const ACCOUNT_DATA_COST_PAGE_SIZE: u64 = 32_u64.saturating_mul(1024); impl FeeStructure { @@ -91,6 +103,25 @@ 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() as f64 + * congestion_multiplier) + .round() 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 +153,13 @@ 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) as f64) + .round() as u64, + prioritization_fee: budget_limits.prioritization_fee, + } } } From 7c296d152e08f06aca9612ff4517a39c670bc8a6 Mon Sep 17 00:00:00 2001 From: Tao Zhu Date: Tue, 6 Feb 2024 00:50:35 +0000 Subject: [PATCH 2/3] fix - no need to round after summing u64 --- sdk/src/fee.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/sdk/src/fee.rs b/sdk/src/fee.rs index 7a755ac823469e..c7e9c12754b96d 100644 --- a/sdk/src/fee.rs +++ b/sdk/src/fee.rs @@ -154,10 +154,9 @@ impl FeeStructure { }); FeeDetails { - transaction_fee: (signature_fee + transaction_fee: signature_fee .saturating_add(write_lock_fee) - .saturating_add(compute_fee) as f64) - .round() as u64, + .saturating_add(compute_fee), prioritization_fee: budget_limits.prioritization_fee, } } From fbd3e8231aa93ce5f697868855cb95b63e50eb8a Mon Sep 17 00:00:00 2001 From: Tao Zhu Date: Thu, 22 Feb 2024 18:13:53 +0000 Subject: [PATCH 3/3] 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 | 2 +- sdk/src/feature_set.rs | 5 +++ sdk/src/fee.rs | 42 ++++++++++++++----- svm/src/account_loader.rs | 8 +++- 8 files changed, 63 insertions(+), 16 deletions(-) diff --git a/core/src/banking_stage/consumer.rs b/core/src/banking_stage/consumer.rs index 660dc2ac977b0d..81de74022432d9 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 7d9a70931b4410..b0c5e0f6ab3265 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 943713d7ad9bbd..1635850bb2a9c5 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 7e051019c99871..29dde36ac20116 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 3a966688783402..29740b8dbebe92 100644 --- a/runtime/src/bank/tests.rs +++ b/runtime/src/bank/tests.rs @@ -10031,7 +10031,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 82687673246293..abecf4fafb6b1d 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 c7e9c12754b96d..b325a23ac08d9d 100644 --- a/sdk/src/fee.rs +++ b/sdk/src/fee.rs @@ -38,8 +38,14 @@ pub struct FeeDetails { } impl FeeDetails { - pub fn total_fee(&self) -> u64 { - self.transaction_fee.saturating_add(self.prioritization_fee) + 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 + } } } @@ -95,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 { @@ -103,15 +110,13 @@ 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() as f64 - * congestion_multiplier) - .round() as u64 + 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` @@ -210,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 854d59bac095cb..334ad7679561ee 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);