From db94d1d53759687aedcad1cd8f940783963b45b0 Mon Sep 17 00:00:00 2001 From: Tao Zhu Date: Fri, 26 Jan 2024 21:43:17 +0000 Subject: [PATCH 1/2] step 1 - changed collector_fee type to struct --- runtime/src/bank.rs | 36 +++++++++++++++++++++------- runtime/src/bank/fee_distribution.rs | 16 ++++++------- runtime/src/serde_snapshot/newer.rs | 6 ++--- 3 files changed, 38 insertions(+), 20 deletions(-) diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 03971724438dc9..0083eb2ffa8e08 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -270,6 +270,24 @@ impl AddAssign for SquashTiming { } } +#[derive(Serialize, Deserialize, PartialEq, Clone, Debug, Default, Copy, AbiExample)] +#[serde(rename_all = "camelCase")] +pub struct CollectorFeeDetails { + pub base_fee: u64, + pub priority_fee: u64, + // more fees such as write_lock_fee go here +} + +impl CollectorFeeDetails { + // TODO - just a placeholder + pub fn add(&mut self, fees: u64) { + let _ = self.base_fee.saturating_add(fees); + } + pub fn dummy(&self) -> u64 { + self.base_fee + } +} + #[derive(Debug)] pub struct BankRc { /// where all the Accounts are stored @@ -456,7 +474,7 @@ pub struct BankFieldsToDeserialize { pub(crate) epoch: Epoch, pub(crate) block_height: u64, pub(crate) collector_id: Pubkey, - pub(crate) collector_fees: u64, + pub(crate) collector_fees: CollectorFeeDetails, pub(crate) fee_calculator: FeeCalculator, pub(crate) fee_rate_governor: FeeRateGovernor, pub(crate) collected_rent: u64, @@ -502,7 +520,7 @@ pub(crate) struct BankFieldsToSerialize<'a> { pub(crate) epoch: Epoch, pub(crate) block_height: u64, pub(crate) collector_id: Pubkey, - pub(crate) collector_fees: u64, + pub(crate) collector_fees: CollectorFeeDetails, pub(crate) fee_calculator: FeeCalculator, pub(crate) fee_rate_governor: FeeRateGovernor, pub(crate) collected_rent: u64, @@ -607,7 +625,7 @@ impl PartialEq for Bank { && epoch == &other.epoch && block_height == &other.block_height && collector_id == &other.collector_id - && collector_fees.load(Relaxed) == other.collector_fees.load(Relaxed) + && *collector_fees.read().unwrap() == *other.collector_fees.read().unwrap() && fee_rate_governor == &other.fee_rate_governor && collected_rent.load(Relaxed) == other.collected_rent.load(Relaxed) && rent_collector == &other.rent_collector @@ -759,7 +777,7 @@ pub struct Bank { collector_id: Pubkey, /// Fees that have been collected - collector_fees: AtomicU64, + collector_fees: RwLock, /// Track cluster signature throughput and adjust fee rate pub(crate) fee_rate_governor: FeeRateGovernor, @@ -1000,7 +1018,7 @@ impl Bank { epoch: Epoch::default(), block_height: u64::default(), collector_id: Pubkey::default(), - collector_fees: AtomicU64::default(), + collector_fees: RwLock::new(CollectorFeeDetails::default()), fee_rate_governor: FeeRateGovernor::default(), collected_rent: AtomicU64::default(), rent_collector: RentCollector::default(), @@ -1309,7 +1327,7 @@ impl Bank { parent_hash: parent.hash(), parent_slot: parent.slot(), collector_id: *collector_id, - collector_fees: AtomicU64::new(0), + collector_fees: RwLock::new(CollectorFeeDetails::default()), ancestors: Ancestors::default(), hash: RwLock::new(Hash::default()), is_delta: AtomicBool::new(false), @@ -1813,7 +1831,7 @@ impl Bank { epoch: fields.epoch, block_height: fields.block_height, collector_id: fields.collector_id, - collector_fees: AtomicU64::new(fields.collector_fees), + collector_fees: RwLock::new(fields.collector_fees), fee_rate_governor: fields.fee_rate_governor, collected_rent: AtomicU64::new(fields.collected_rent), // clone()-ing is needed to consider a gated behavior in rent_collector @@ -1932,7 +1950,7 @@ impl Bank { epoch: self.epoch, block_height: self.block_height, collector_id: self.collector_id, - collector_fees: self.collector_fees.load(Relaxed), + collector_fees: *self.collector_fees.read().unwrap(), fee_calculator: FeeCalculator::default(), fee_rate_governor: self.fee_rate_governor.clone(), collected_rent: self.collected_rent.load(Relaxed), @@ -5563,7 +5581,7 @@ impl Bank { }) .collect(); - self.collector_fees.fetch_add(fees, Relaxed); + self.collector_fees.write().unwrap().add(fees); results } diff --git a/runtime/src/bank/fee_distribution.rs b/runtime/src/bank/fee_distribution.rs index 85d68c07fd7448..16c6d46d3e12af 100644 --- a/runtime/src/bank/fee_distribution.rs +++ b/runtime/src/bank/fee_distribution.rs @@ -46,7 +46,7 @@ impl Bank { // still being stake-weighted. // Ref: distribute_rent_to_validators pub(super) fn distribute_transaction_fees(&self) { - let collector_fees = self.collector_fees.load(Relaxed); + let collector_fees = self.collector_fees.read().unwrap().dummy(); if collector_fees != 0 { let (deposit, mut burn) = self.fee_rate_governor.burn(collector_fees); if deposit > 0 { @@ -344,8 +344,8 @@ pub mod tests { genesis.genesis_config.rent = rent; // Ensure rent is non-zero, as genesis_utils sets Rent::free by default let bank = Bank::new_for_tests(&genesis.genesis_config); let transaction_fees = 100; - bank.collector_fees.fetch_add(transaction_fees, Relaxed); - assert_eq!(transaction_fees, bank.collector_fees.load(Relaxed)); + bank.collector_fees.write().unwrap().add(transaction_fees); + assert_eq!(transaction_fees, bank.collector_fees.read().unwrap().dummy()); let (expected_collected_fees, burn_amount) = bank.fee_rate_governor.burn(transaction_fees); assert!(burn_amount > 0); @@ -416,7 +416,7 @@ pub mod tests { fn test_distribute_transaction_fees_zero() { let genesis = create_genesis_config(0); let bank = Bank::new_for_tests(&genesis.genesis_config); - assert_eq!(bank.collector_fees.load(Relaxed), 0); + assert_eq!(bank.collector_fees.read().unwrap().dummy(), 0); let initial_capitalization = bank.capitalization(); let initial_collector_id_balance = bank.get_balance(bank.collector_id()); @@ -438,8 +438,8 @@ pub mod tests { genesis.genesis_config.fee_rate_governor.burn_percent = 100; let bank = Bank::new_for_tests(&genesis.genesis_config); let transaction_fees = 100; - bank.collector_fees.fetch_add(transaction_fees, Relaxed); - assert_eq!(transaction_fees, bank.collector_fees.load(Relaxed)); + bank.collector_fees.write().unwrap().add(transaction_fees); + assert_eq!(transaction_fees, bank.collector_fees.read().unwrap().dummy()); let initial_capitalization = bank.capitalization(); let initial_collector_id_balance = bank.get_balance(bank.collector_id()); @@ -463,8 +463,8 @@ pub mod tests { let genesis = create_genesis_config(0); let bank = Bank::new_for_tests(&genesis.genesis_config); let transaction_fees = 100; - bank.collector_fees.fetch_add(transaction_fees, Relaxed); - assert_eq!(transaction_fees, bank.collector_fees.load(Relaxed)); + bank.collector_fees.write().unwrap().add(transaction_fees); + assert_eq!(transaction_fees, bank.collector_fees.read().unwrap().dummy()); // ensure that account balance will overflow and fee distribution will fail let account = AccountSharedData::new(u64::MAX, 0, &system_program::id()); diff --git a/runtime/src/serde_snapshot/newer.rs b/runtime/src/serde_snapshot/newer.rs index 004e6e61d54868..bdac4a71379a5c 100644 --- a/runtime/src/serde_snapshot/newer.rs +++ b/runtime/src/serde_snapshot/newer.rs @@ -5,7 +5,7 @@ use { *, }, crate::{ - bank::EpochRewardStatus, + bank::{CollectorFeeDetails, EpochRewardStatus}, stakes::{serde_stakes_enum_compat, StakesEnum}, }, solana_accounts_db::{accounts_hash::AccountsHash, ancestors::AncestorsForSerialization}, @@ -49,7 +49,7 @@ struct DeserializableVersionedBank { epoch: Epoch, block_height: u64, collector_id: Pubkey, - collector_fees: u64, + collector_fees: CollectorFeeDetails, fee_calculator: FeeCalculator, fee_rate_governor: FeeRateGovernor, collected_rent: u64, @@ -129,7 +129,7 @@ struct SerializableVersionedBank<'a> { epoch: Epoch, block_height: u64, collector_id: Pubkey, - collector_fees: u64, + collector_fees: CollectorFeeDetails, fee_calculator: FeeCalculator, fee_rate_governor: FeeRateGovernor, collected_rent: u64, From 705497dae3ffb65bd7a4bd27a0a6fa5340ec2aaa Mon Sep 17 00:00:00 2001 From: Tao Zhu Date: Fri, 26 Jan 2024 23:06:17 +0000 Subject: [PATCH 2/2] step 2 - end to end function --- runtime/src/bank.rs | 56 ++++++----- runtime/src/bank/fee_distribution.rs | 134 ++++++++++++++++----------- sdk/src/fee.rs | 13 ++- 3 files changed, 121 insertions(+), 82 deletions(-) diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 0083eb2ffa8e08..a167c6b7420be4 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -148,7 +148,7 @@ use { epoch_schedule::EpochSchedule, feature, feature_set::{self, include_loaded_accounts_data_size_in_fee_calculation, FeatureSet}, - fee::FeeStructure, + fee::{FeeDetails, FeeStructure}, fee_calculator::{FeeCalculator, FeeRateGovernor}, genesis_config::{ClusterType, GenesisConfig}, hard_forks::HardForks, @@ -273,18 +273,26 @@ impl AddAssign for SquashTiming { #[derive(Serialize, Deserialize, PartialEq, Clone, Debug, Default, Copy, AbiExample)] #[serde(rename_all = "camelCase")] pub struct CollectorFeeDetails { - pub base_fee: u64, + pub transaction_fee: u64, pub priority_fee: u64, // more fees such as write_lock_fee go here } impl CollectorFeeDetails { // TODO - just a placeholder - pub fn add(&mut self, fees: u64) { - let _ = self.base_fee.saturating_add(fees); + pub 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); + } + pub fn get(&self) -> Self { + *self } pub fn dummy(&self) -> u64 { - self.base_fee + self.transaction_fee } } @@ -5534,8 +5542,7 @@ impl Bank { txs: &[SanitizedTransaction], execution_results: &[TransactionExecutionResult], ) -> Vec> { - let hash_queue = self.blockhash_queue.read().unwrap(); - let mut fees = 0; + let mut accumulated_fee_details = FeeDetails::default(); let results = txs .iter() @@ -5548,21 +5555,17 @@ impl Bank { TransactionExecutionResult::NotExecuted(err) => Err(err.clone()), }?; - let (lamports_per_signature, is_nonce) = durable_nonce_fee - .map(|durable_nonce_fee| durable_nonce_fee.lamports_per_signature()) - .map(|maybe_lamports_per_signature| (maybe_lamports_per_signature, true)) - .unwrap_or_else(|| { - ( - hash_queue.get_lamports_per_signature(tx.message().recent_blockhash()), - false, - ) - }); - - let lamports_per_signature = - lamports_per_signature.ok_or(TransactionError::BlockhashNotFound)?; - let fee = self.get_fee_for_message_with_lamports_per_signature( - tx.message(), - lamports_per_signature, + let is_nonce = durable_nonce_fee.is_some(); + + // TODO can refactor self.get_fee_for_message_with_lamports_per_signature() out + 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 @@ -5573,15 +5576,18 @@ impl Bank { // post-load, fee deducted, pre-execute account state // stored if execution_status.is_err() && !is_nonce { - self.withdraw(tx.message().fee_payer(), fee)?; + self.withdraw(tx.message().fee_payer(), fee_details.total_fee())?; } - fees += fee; + accumulated_fee_details.accumulate(&fee_details); Ok(()) }) .collect(); - self.collector_fees.write().unwrap().add(fees); + self.collector_fees + .write() + .unwrap() + .add(&accumulated_fee_details); results } diff --git a/runtime/src/bank/fee_distribution.rs b/runtime/src/bank/fee_distribution.rs index 16c6d46d3e12af..34127cc47101df 100644 --- a/runtime/src/bank/fee_distribution.rs +++ b/runtime/src/bank/fee_distribution.rs @@ -1,6 +1,6 @@ use { super::Bank, - crate::svm::account_rent_state::RentState, + crate::{bank::CollectorFeeDetails, svm::account_rent_state::RentState}, log::{debug, warn}, solana_accounts_db::stake_rewards::RewardInfo, solana_sdk::{ @@ -46,47 +46,53 @@ impl Bank { // still being stake-weighted. // Ref: distribute_rent_to_validators pub(super) fn distribute_transaction_fees(&self) { - let collector_fees = self.collector_fees.read().unwrap().dummy(); - if collector_fees != 0 { - let (deposit, mut burn) = self.fee_rate_governor.burn(collector_fees); - if deposit > 0 { - let validate_fee_collector = self.validate_fee_collector_account(); - match self.deposit_fees( - &self.collector_id, - deposit, - DepositFeeOptions { - check_account_owner: validate_fee_collector, - check_rent_paying: validate_fee_collector, - }, - ) { - Ok(post_balance) => { - self.rewards.write().unwrap().push(( - self.collector_id, - RewardInfo { - reward_type: RewardType::Fee, - lamports: deposit as i64, - post_balance, - commission: None, - }, - )); - } - Err(err) => { - debug!( - "Burned {} lamport tx fee instead of sending to {} due to {}", - deposit, self.collector_id, err - ); - datapoint_warn!( - "bank-burned_fee", - ("slot", self.slot(), i64), - ("num_lamports", deposit, i64), - ("error", err.to_string(), String), - ); - burn += deposit; - } + let CollectorFeeDetails { + transaction_fee, + priority_fee, + } = self.collector_fees.read().unwrap().get(); + let (mut deposit, mut burn) = if transaction_fee != 0 { + self.fee_rate_governor.burn(transaction_fee) + } else { + (0, 0) + }; + deposit = deposit.saturating_add(priority_fee); + if deposit > 0 { + let validate_fee_collector = self.validate_fee_collector_account(); + match self.deposit_fees( + &self.collector_id, + deposit, + DepositFeeOptions { + check_account_owner: validate_fee_collector, + check_rent_paying: validate_fee_collector, + }, + ) { + Ok(post_balance) => { + self.rewards.write().unwrap().push(( + self.collector_id, + RewardInfo { + reward_type: RewardType::Fee, + lamports: deposit as i64, + post_balance, + commission: None, + }, + )); + } + Err(err) => { + debug!( + "Burned {} lamport tx fee instead of sending to {} due to {}", + deposit, self.collector_id, err + ); + datapoint_warn!( + "bank-burned_fee", + ("slot", self.slot(), i64), + ("num_lamports", deposit, i64), + ("error", err.to_string(), String), + ); + burn = burn.saturating_add(deposit); } } - self.capitalization.fetch_sub(burn, Relaxed); } + self.capitalization.fetch_sub(burn, Relaxed); } // Deposits fees into a specified account and if successful, returns the new balance of that account @@ -295,8 +301,8 @@ pub mod tests { create_genesis_config_with_vote_accounts, ValidatorVoteKeypairs, }, solana_sdk::{ - account::AccountSharedData, feature_set, native_token::sol_to_lamports, pubkey, - rent::Rent, signature::Signer, + account::AccountSharedData, feature_set, fee::FeeDetails, + native_token::sol_to_lamports, pubkey, rent::Rent, signature::Signer, }, }; @@ -343,11 +349,17 @@ pub mod tests { let min_rent_exempt_balance = rent.minimum_balance(0); genesis.genesis_config.rent = rent; // Ensure rent is non-zero, as genesis_utils sets Rent::free by default let bank = Bank::new_for_tests(&genesis.genesis_config); - let transaction_fees = 100; - bank.collector_fees.write().unwrap().add(transaction_fees); - assert_eq!(transaction_fees, bank.collector_fees.read().unwrap().dummy()); + let fee_details = FeeDetails { + transaction_fee: 100, + prioritization_fee: 0, + }; + bank.collector_fees.write().unwrap().add(&fee_details); + assert_eq!( + fee_details.transaction_fee, + bank.collector_fees.read().unwrap().dummy() + ); let (expected_collected_fees, burn_amount) = - bank.fee_rate_governor.burn(transaction_fees); + bank.fee_rate_governor.burn(fee_details.transaction_fee); assert!(burn_amount > 0); if test_case.scenario == Scenario::RentPaying { @@ -355,7 +367,7 @@ pub mod tests { let initial_balance = 100; let account = AccountSharedData::new(initial_balance, 0, &system_program::id()); bank.store_account(bank.collector_id(), &account); - assert!(initial_balance + transaction_fees < min_rent_exempt_balance); + assert!(initial_balance + fee_details.transaction_fee < min_rent_exempt_balance); } else if test_case.scenario == Scenario::InvalidOwner { // ensure that account owner is invalid and fee distribution will fail let account = @@ -375,7 +387,7 @@ pub mod tests { if test_case.scenario != Scenario::Normal && !test_case.disable_checks { assert_eq!(initial_collector_id_balance, new_collector_id_balance); assert_eq!( - initial_capitalization - transaction_fees, + initial_capitalization - fee_details.transaction_fee, bank.capitalization() ); let locked_rewards = bank.rewards.read().unwrap(); @@ -437,9 +449,15 @@ pub mod tests { let mut genesis = create_genesis_config(0); genesis.genesis_config.fee_rate_governor.burn_percent = 100; let bank = Bank::new_for_tests(&genesis.genesis_config); - let transaction_fees = 100; - bank.collector_fees.write().unwrap().add(transaction_fees); - assert_eq!(transaction_fees, bank.collector_fees.read().unwrap().dummy()); + let fee_details = FeeDetails { + transaction_fee: 100, + prioritization_fee: 0, + }; + bank.collector_fees.write().unwrap().add(&fee_details); + assert_eq!( + fee_details.transaction_fee, + bank.collector_fees.read().unwrap().dummy() + ); let initial_capitalization = bank.capitalization(); let initial_collector_id_balance = bank.get_balance(bank.collector_id()); @@ -448,7 +466,7 @@ pub mod tests { assert_eq!(initial_collector_id_balance, new_collector_id_balance); assert_eq!( - initial_capitalization - transaction_fees, + initial_capitalization - fee_details.transaction_fee, bank.capitalization() ); let locked_rewards = bank.rewards.read().unwrap(); @@ -462,9 +480,15 @@ pub mod tests { fn test_distribute_transaction_fees_overflow_failure() { let genesis = create_genesis_config(0); let bank = Bank::new_for_tests(&genesis.genesis_config); - let transaction_fees = 100; - bank.collector_fees.write().unwrap().add(transaction_fees); - assert_eq!(transaction_fees, bank.collector_fees.read().unwrap().dummy()); + let fee_details = FeeDetails { + transaction_fee: 100, + prioritization_fee: 0, + }; + bank.collector_fees.write().unwrap().add(&fee_details); + assert_eq!( + fee_details.transaction_fee, + bank.collector_fees.read().unwrap().dummy() + ); // ensure that account balance will overflow and fee distribution will fail let account = AccountSharedData::new(u64::MAX, 0, &system_program::id()); @@ -477,7 +501,7 @@ pub mod tests { assert_eq!(initial_collector_id_balance, new_collector_id_balance); assert_eq!( - initial_capitalization - transaction_fees, + initial_capitalization - fee_details.transaction_fee, bank.capitalization() ); let locked_rewards = bank.rewards.read().unwrap(); diff --git a/sdk/src/fee.rs b/sdk/src/fee.rs index de77ac11436595..972328534cfadb 100644 --- a/sdk/src/fee.rs +++ b/sdk/src/fee.rs @@ -34,14 +34,23 @@ pub struct FeeStructure { /// Return type of calculate_fee(...) #[derive(Debug, Default, Clone, Eq, PartialEq)] pub struct FeeDetails { - transaction_fee: u64, - prioritization_fee: u64, + pub transaction_fee: u64, + pub prioritization_fee: u64, } impl FeeDetails { pub fn total_fee(&self) -> u64 { self.transaction_fee.saturating_add(self.prioritization_fee) } + + 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 const ACCOUNT_DATA_COST_PAGE_SIZE: u64 = 32_u64.saturating_mul(1024);