From 69827fa90e9999f352673361c794b424bafd4da8 Mon Sep 17 00:00:00 2001 From: Justin Starry Date: Wed, 25 Oct 2023 10:37:40 +0800 Subject: [PATCH 1/8] refactor: create bank::fee_distribution module --- runtime/src/bank.rs | 241 +------------ runtime/src/bank/fee_distribution.rs | 489 +++++++++++++++++++++++++++ runtime/src/bank/tests.rs | 226 ------------- 3 files changed, 493 insertions(+), 463 deletions(-) create mode 100644 runtime/src/bank/fee_distribution.rs diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 1ae2d6606563b4..2aa0baaff8210b 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -69,7 +69,6 @@ use { }, solana_accounts_db::{ account_overrides::AccountOverrides, - account_rent_state::RentState, accounts::{ AccountAddressFilter, Accounts, LoadedTransaction, PubkeyAccountSlot, RewardInterval, TransactionLoadResult, @@ -185,7 +184,7 @@ use { borrow::Cow, cell::RefCell, collections::{HashMap, HashSet}, - convert::{TryFrom, TryInto}, + convert::TryFrom, fmt, mem, ops::{AddAssign, RangeInclusive}, path::PathBuf, @@ -216,6 +215,7 @@ mod address_lookup_table; pub mod bank_hash_details; mod builtin_programs; pub mod epoch_accounts_hash_utils; +mod fee_distribution; mod metrics; mod replace_account; mod serde_snapshot; @@ -3671,62 +3671,6 @@ impl Bank { stake_weighted_timestamp } - // Distribute collected transaction fees for this slot to collector_id (= current leader). - // - // Each validator is incentivized to process more transactions to earn more transaction fees. - // Transaction fees are rewarded for the computing resource utilization cost, directly - // proportional to their actual processing power. - // - // collector_id is rotated according to stake-weighted leader schedule. So the opportunity of - // earning transaction fees are fairly distributed by stake. And missing the opportunity - // (not producing a block as a leader) earns nothing. So, being online is incentivized as a - // form of transaction fees as well. - // - // On the other hand, rent fees are distributed under slightly different philosophy, while - // still being stake-weighted. - // Ref: distribute_rent_to_validators - fn collect_fees(&self) { - let collector_fees = self.collector_fees.load(Relaxed); - - if collector_fees != 0 { - let (deposit, mut burn) = self.fee_rate_governor.burn(collector_fees); - // burn a portion of fees - debug!( - "distributed fee: {} (rounded from: {}, burned: {})", - deposit, collector_fees, burn - ); - - match self.deposit(&self.collector_id, deposit) { - Ok(post_balance) => { - if deposit != 0 { - self.rewards.write().unwrap().push(( - self.collector_id, - RewardInfo { - reward_type: RewardType::Fee, - lamports: deposit as i64, - post_balance, - commission: None, - }, - )); - } - } - Err(_) => { - error!( - "Burning {} fee instead of crediting {}", - deposit, self.collector_id - ); - datapoint_error!( - "bank-burned_fee", - ("slot", self.slot(), i64), - ("num_lamports", deposit, i64) - ); - burn += deposit; - } - } - self.capitalization.fetch_sub(burn, Relaxed); - } - } - pub fn rehash(&self) { let mut hash = self.hash.write().unwrap(); let new = self.hash_internal_state(); @@ -3752,8 +3696,8 @@ impl Bank { if *hash == Hash::default() { // finish up any deferred changes to account state self.collect_rent_eagerly(); - self.collect_fees(); - self.distribute_rent(); + self.distribute_transaction_fees(); + self.distribute_rent_fees(); self.update_slot_history(); self.run_incinerator(); @@ -5652,183 +5596,6 @@ impl Bank { } } - // Distribute collected rent fees for this slot to staked validators (excluding stakers) - // according to stake. - // - // The nature of rent fee is the cost of doing business, every validator has to hold (or have - // access to) the same list of accounts, so we pay according to stake, which is a rough proxy for - // value to the network. - // - // Currently, rent distribution doesn't consider given validator's uptime at all (this might - // change). That's because rent should be rewarded for the storage resource utilization cost. - // It's treated differently from transaction fees, which is for the computing resource - // utilization cost. - // - // We can't use collector_id (which is rotated according to stake-weighted leader schedule) - // as an approximation to the ideal rent distribution to simplify and avoid this per-slot - // computation for the distribution (time: N log N, space: N acct. stores; N = # of - // validators). - // The reason is that rent fee doesn't need to be incentivized for throughput unlike transaction - // fees - // - // Ref: collect_fees - #[allow(clippy::needless_collect)] - fn distribute_rent_to_validators( - &self, - vote_accounts: &VoteAccountsHashMap, - rent_to_be_distributed: u64, - ) { - let mut total_staked = 0; - - // Collect the stake associated with each validator. - // Note that a validator may be present in this vector multiple times if it happens to have - // more than one staked vote account somehow - let mut validator_stakes = vote_accounts - .iter() - .filter_map(|(_vote_pubkey, (staked, account))| { - if *staked == 0 { - None - } else { - total_staked += *staked; - Some((account.node_pubkey()?, *staked)) - } - }) - .collect::>(); - - #[cfg(test)] - if validator_stakes.is_empty() { - // some tests bank.freezes() with bad staking state - self.capitalization - .fetch_sub(rent_to_be_distributed, Relaxed); - return; - } - #[cfg(not(test))] - assert!(!validator_stakes.is_empty()); - - // Sort first by stake and then by validator identity pubkey for determinism. - // If two items are still equal, their relative order does not matter since - // both refer to the same validator. - validator_stakes.sort_unstable_by(|(pubkey1, staked1), (pubkey2, staked2)| { - (staked1, pubkey1).cmp(&(staked2, pubkey2)).reverse() - }); - - let enforce_fix = self.no_overflow_rent_distribution_enabled(); - - let mut rent_distributed_in_initial_round = 0; - let validator_rent_shares = validator_stakes - .into_iter() - .map(|(pubkey, staked)| { - let rent_share = if !enforce_fix { - (((staked * rent_to_be_distributed) as f64) / (total_staked as f64)) as u64 - } else { - (((staked as u128) * (rent_to_be_distributed as u128)) / (total_staked as u128)) - .try_into() - .unwrap() - }; - rent_distributed_in_initial_round += rent_share; - (pubkey, rent_share) - }) - .collect::>(); - - // Leftover lamports after fraction calculation, will be paid to validators starting from highest stake - // holder - let mut leftover_lamports = rent_to_be_distributed - rent_distributed_in_initial_round; - - let mut rewards = vec![]; - validator_rent_shares - .into_iter() - .for_each(|(pubkey, rent_share)| { - let rent_to_be_paid = if leftover_lamports > 0 { - leftover_lamports -= 1; - rent_share + 1 - } else { - rent_share - }; - if !enforce_fix || rent_to_be_paid > 0 { - let mut account = self - .get_account_with_fixed_root(&pubkey) - .unwrap_or_default(); - let rent = self.rent_collector().rent; - let recipient_pre_rent_state = RentState::from_account(&account, &rent); - let distribution = account.checked_add_lamports(rent_to_be_paid); - let recipient_post_rent_state = RentState::from_account(&account, &rent); - let rent_state_transition_allowed = recipient_post_rent_state - .transition_allowed_from(&recipient_pre_rent_state); - if !rent_state_transition_allowed { - warn!( - "Rent distribution of {rent_to_be_paid} to {pubkey} results in \ - invalid RentState: {recipient_post_rent_state:?}" - ); - datapoint_warn!( - "bank-rent_distribution_invalid_state", - ("slot", self.slot(), i64), - ("pubkey", pubkey.to_string(), String), - ("rent_to_be_paid", rent_to_be_paid, i64) - ); - } - if distribution.is_err() - || (self.prevent_rent_paying_rent_recipients() - && !rent_state_transition_allowed) - { - // overflow adding lamports or resulting account is not rent-exempt - self.capitalization.fetch_sub(rent_to_be_paid, Relaxed); - error!( - "Burned {} rent lamports instead of sending to {}", - rent_to_be_paid, pubkey - ); - datapoint_error!( - "bank-burned_rent", - ("slot", self.slot(), i64), - ("num_lamports", rent_to_be_paid, i64) - ); - } else { - self.store_account(&pubkey, &account); - rewards.push(( - pubkey, - RewardInfo { - reward_type: RewardType::Rent, - lamports: rent_to_be_paid as i64, - post_balance: account.lamports(), - commission: None, - }, - )); - } - } - }); - self.rewards.write().unwrap().append(&mut rewards); - - if enforce_fix { - assert_eq!(leftover_lamports, 0); - } else if leftover_lamports != 0 { - warn!( - "There was leftover from rent distribution: {}", - leftover_lamports - ); - self.capitalization.fetch_sub(leftover_lamports, Relaxed); - } - } - - fn distribute_rent(&self) { - let total_rent_collected = self.collected_rent.load(Relaxed); - - let (burned_portion, rent_to_be_distributed) = self - .rent_collector - .rent - .calculate_burn(total_rent_collected); - - debug!( - "distributed rent: {} (rounded from: {}, burned: {})", - rent_to_be_distributed, total_rent_collected, burned_portion - ); - self.capitalization.fetch_sub(burned_portion, Relaxed); - - if rent_to_be_distributed == 0 { - return; - } - - self.distribute_rent_to_validators(&self.vote_accounts(), rent_to_be_distributed); - } - fn collect_rent( &self, execution_results: &[TransactionExecutionResult], diff --git a/runtime/src/bank/fee_distribution.rs b/runtime/src/bank/fee_distribution.rs new file mode 100644 index 00000000000000..41f83c41cce929 --- /dev/null +++ b/runtime/src/bank/fee_distribution.rs @@ -0,0 +1,489 @@ +use { + super::Bank, + log::{debug, error, warn}, + solana_accounts_db::{account_rent_state::RentState, stake_rewards::RewardInfo}, + solana_sdk::{ + account::{ReadableAccount, WritableAccount}, + pubkey::Pubkey, + reward_type::RewardType, + }, + solana_vote::vote_account::VoteAccountsHashMap, + std::sync::atomic::Ordering::Relaxed, +}; + +impl Bank { + // Distribute collected transaction fees for this slot to collector_id (= current leader). + // + // Each validator is incentivized to process more transactions to earn more transaction fees. + // Transaction fees are rewarded for the computing resource utilization cost, directly + // proportional to their actual processing power. + // + // collector_id is rotated according to stake-weighted leader schedule. So the opportunity of + // earning transaction fees are fairly distributed by stake. And missing the opportunity + // (not producing a block as a leader) earns nothing. So, being online is incentivized as a + // form of transaction fees as well. + // + // On the other hand, rent fees are distributed under slightly different philosophy, while + // still being stake-weighted. + // Ref: distribute_rent_to_validators + pub(super) fn distribute_transaction_fees(&self) { + let collector_fees = self.collector_fees.load(Relaxed); + + if collector_fees != 0 { + let (deposit, mut burn) = self.fee_rate_governor.burn(collector_fees); + // burn a portion of fees + debug!( + "distributed fee: {} (rounded from: {}, burned: {})", + deposit, collector_fees, burn + ); + + match self.deposit(&self.collector_id, deposit) { + Ok(post_balance) => { + if deposit != 0 { + self.rewards.write().unwrap().push(( + self.collector_id, + RewardInfo { + reward_type: RewardType::Fee, + lamports: deposit as i64, + post_balance, + commission: None, + }, + )); + } + } + Err(_) => { + error!( + "Burning {} fee instead of crediting {}", + deposit, self.collector_id + ); + datapoint_error!( + "bank-burned_fee", + ("slot", self.slot(), i64), + ("num_lamports", deposit, i64) + ); + burn += deposit; + } + } + self.capitalization.fetch_sub(burn, Relaxed); + } + } + + // Distribute collected rent fees for this slot to staked validators (excluding stakers) + // according to stake. + // + // The nature of rent fee is the cost of doing business, every validator has to hold (or have + // access to) the same list of accounts, so we pay according to stake, which is a rough proxy for + // value to the network. + // + // Currently, rent distribution doesn't consider given validator's uptime at all (this might + // change). That's because rent should be rewarded for the storage resource utilization cost. + // It's treated differently from transaction fees, which is for the computing resource + // utilization cost. + // + // We can't use collector_id (which is rotated according to stake-weighted leader schedule) + // as an approximation to the ideal rent distribution to simplify and avoid this per-slot + // computation for the distribution (time: N log N, space: N acct. stores; N = # of + // validators). + // The reason is that rent fee doesn't need to be incentivized for throughput unlike transaction + // fees + // + // Ref: collect_transaction_fees + #[allow(clippy::needless_collect)] + fn distribute_rent_to_validators( + &self, + vote_accounts: &VoteAccountsHashMap, + rent_to_be_distributed: u64, + ) { + let mut total_staked = 0; + + // Collect the stake associated with each validator. + // Note that a validator may be present in this vector multiple times if it happens to have + // more than one staked vote account somehow + let mut validator_stakes = vote_accounts + .iter() + .filter_map(|(_vote_pubkey, (staked, account))| { + if *staked == 0 { + None + } else { + total_staked += *staked; + Some((account.node_pubkey()?, *staked)) + } + }) + .collect::>(); + + #[cfg(test)] + if validator_stakes.is_empty() { + // some tests bank.freezes() with bad staking state + self.capitalization + .fetch_sub(rent_to_be_distributed, Relaxed); + return; + } + #[cfg(not(test))] + assert!(!validator_stakes.is_empty()); + + // Sort first by stake and then by validator identity pubkey for determinism. + // If two items are still equal, their relative order does not matter since + // both refer to the same validator. + validator_stakes.sort_unstable_by(|(pubkey1, staked1), (pubkey2, staked2)| { + (staked1, pubkey1).cmp(&(staked2, pubkey2)).reverse() + }); + + let enforce_fix = self.no_overflow_rent_distribution_enabled(); + + let mut rent_distributed_in_initial_round = 0; + let validator_rent_shares = validator_stakes + .into_iter() + .map(|(pubkey, staked)| { + let rent_share = if !enforce_fix { + (((staked * rent_to_be_distributed) as f64) / (total_staked as f64)) as u64 + } else { + (((staked as u128) * (rent_to_be_distributed as u128)) / (total_staked as u128)) + .try_into() + .unwrap() + }; + rent_distributed_in_initial_round += rent_share; + (pubkey, rent_share) + }) + .collect::>(); + + // Leftover lamports after fraction calculation, will be paid to validators starting from highest stake + // holder + let mut leftover_lamports = rent_to_be_distributed - rent_distributed_in_initial_round; + + let mut rewards = vec![]; + validator_rent_shares + .into_iter() + .for_each(|(pubkey, rent_share)| { + let rent_to_be_paid = if leftover_lamports > 0 { + leftover_lamports -= 1; + rent_share + 1 + } else { + rent_share + }; + if !enforce_fix || rent_to_be_paid > 0 { + let mut account = self + .get_account_with_fixed_root(&pubkey) + .unwrap_or_default(); + let rent = self.rent_collector().rent; + let recipient_pre_rent_state = RentState::from_account(&account, &rent); + let distribution = account.checked_add_lamports(rent_to_be_paid); + let recipient_post_rent_state = RentState::from_account(&account, &rent); + let rent_state_transition_allowed = recipient_post_rent_state + .transition_allowed_from(&recipient_pre_rent_state); + if !rent_state_transition_allowed { + warn!( + "Rent distribution of {rent_to_be_paid} to {pubkey} results in \ + invalid RentState: {recipient_post_rent_state:?}" + ); + datapoint_warn!( + "bank-rent_distribution_invalid_state", + ("slot", self.slot(), i64), + ("pubkey", pubkey.to_string(), String), + ("rent_to_be_paid", rent_to_be_paid, i64) + ); + } + if distribution.is_err() + || (self.prevent_rent_paying_rent_recipients() + && !rent_state_transition_allowed) + { + // overflow adding lamports or resulting account is not rent-exempt + self.capitalization.fetch_sub(rent_to_be_paid, Relaxed); + error!( + "Burned {} rent lamports instead of sending to {}", + rent_to_be_paid, pubkey + ); + datapoint_error!( + "bank-burned_rent", + ("slot", self.slot(), i64), + ("num_lamports", rent_to_be_paid, i64) + ); + } else { + self.store_account(&pubkey, &account); + rewards.push(( + pubkey, + RewardInfo { + reward_type: RewardType::Rent, + lamports: rent_to_be_paid as i64, + post_balance: account.lamports(), + commission: None, + }, + )); + } + } + }); + self.rewards.write().unwrap().append(&mut rewards); + + if enforce_fix { + assert_eq!(leftover_lamports, 0); + } else if leftover_lamports != 0 { + warn!( + "There was leftover from rent distribution: {}", + leftover_lamports + ); + self.capitalization.fetch_sub(leftover_lamports, Relaxed); + } + } + + pub(super) fn distribute_rent_fees(&self) { + let total_rent_collected = self.collected_rent.load(Relaxed); + + let (burned_portion, rent_to_be_distributed) = self + .rent_collector + .rent + .calculate_burn(total_rent_collected); + + debug!( + "distributed rent: {} (rounded from: {}, burned: {})", + rent_to_be_distributed, total_rent_collected, burned_portion + ); + self.capitalization.fetch_sub(burned_portion, Relaxed); + + if rent_to_be_distributed == 0 { + return; + } + + self.distribute_rent_to_validators(&self.vote_accounts(), rent_to_be_distributed); + } +} + +#[cfg(test)] +pub mod tests { + use { + super::*, + crate::genesis_utils::{ + create_genesis_config_with_leader, create_genesis_config_with_vote_accounts, + ValidatorVoteKeypairs, + }, + log::info, + solana_sdk::{feature_set, native_token::sol_to_lamports, rent::Rent, signature::Signer}, + }; + + #[test] + fn test_distribute_rent_to_validators_overflow() { + solana_logger::setup(); + + // These values are taken from the real cluster (testnet) + const RENT_TO_BE_DISTRIBUTED: u64 = 120_525; + const VALIDATOR_STAKE: u64 = 374_999_998_287_840; + + let validator_pubkey = solana_sdk::pubkey::new_rand(); + let mut genesis_config = + create_genesis_config_with_leader(10, &validator_pubkey, VALIDATOR_STAKE) + .genesis_config; + + let bank = Bank::new_for_tests(&genesis_config); + let old_validator_lamports = bank.get_balance(&validator_pubkey); + bank.distribute_rent_to_validators(&bank.vote_accounts(), RENT_TO_BE_DISTRIBUTED); + let new_validator_lamports = bank.get_balance(&validator_pubkey); + assert_eq!( + new_validator_lamports, + old_validator_lamports + RENT_TO_BE_DISTRIBUTED + ); + + genesis_config + .accounts + .remove(&feature_set::no_overflow_rent_distribution::id()) + .unwrap(); + let bank = std::panic::AssertUnwindSafe(Bank::new_for_tests(&genesis_config)); + let old_validator_lamports = bank.get_balance(&validator_pubkey); + let new_validator_lamports = std::panic::catch_unwind(|| { + bank.distribute_rent_to_validators(&bank.vote_accounts(), RENT_TO_BE_DISTRIBUTED); + bank.get_balance(&validator_pubkey) + }); + + if let Ok(new_validator_lamports) = new_validator_lamports { + info!("asserting overflowing incorrect rent distribution"); + assert_ne!( + new_validator_lamports, + old_validator_lamports + RENT_TO_BE_DISTRIBUTED + ); + } else { + info!("NOT-asserting overflowing incorrect rent distribution"); + } + } + + #[test] + fn test_distribute_rent_to_validators_rent_paying() { + solana_logger::setup(); + + const RENT_PER_VALIDATOR: u64 = 55; + const TOTAL_RENT: u64 = RENT_PER_VALIDATOR * 4; + + let empty_validator = ValidatorVoteKeypairs::new_rand(); + let rent_paying_validator = ValidatorVoteKeypairs::new_rand(); + let becomes_rent_exempt_validator = ValidatorVoteKeypairs::new_rand(); + let rent_exempt_validator = ValidatorVoteKeypairs::new_rand(); + let keypairs = vec![ + &empty_validator, + &rent_paying_validator, + &becomes_rent_exempt_validator, + &rent_exempt_validator, + ]; + let genesis_config_info = create_genesis_config_with_vote_accounts( + sol_to_lamports(1000.), + &keypairs, + vec![sol_to_lamports(1000.); 4], + ); + let mut genesis_config = genesis_config_info.genesis_config; + genesis_config.rent = Rent::default(); // Ensure rent is non-zero, as genesis_utils sets Rent::free by default + + for deactivate_feature in [false, true] { + if deactivate_feature { + genesis_config + .accounts + .remove(&feature_set::prevent_rent_paying_rent_recipients::id()) + .unwrap(); + } + let bank = Bank::new_for_tests(&genesis_config); + let rent = bank.rent_collector().rent; + let rent_exempt_minimum = rent.minimum_balance(0); + + // Make one validator have an empty identity account + let mut empty_validator_account = bank + .get_account_with_fixed_root(&empty_validator.node_keypair.pubkey()) + .unwrap(); + empty_validator_account.set_lamports(0); + bank.store_account( + &empty_validator.node_keypair.pubkey(), + &empty_validator_account, + ); + + // Make one validator almost rent-exempt, less RENT_PER_VALIDATOR + let mut becomes_rent_exempt_validator_account = bank + .get_account_with_fixed_root(&becomes_rent_exempt_validator.node_keypair.pubkey()) + .unwrap(); + becomes_rent_exempt_validator_account + .set_lamports(rent_exempt_minimum - RENT_PER_VALIDATOR); + bank.store_account( + &becomes_rent_exempt_validator.node_keypair.pubkey(), + &becomes_rent_exempt_validator_account, + ); + + // Make one validator rent-exempt + let mut rent_exempt_validator_account = bank + .get_account_with_fixed_root(&rent_exempt_validator.node_keypair.pubkey()) + .unwrap(); + rent_exempt_validator_account.set_lamports(rent_exempt_minimum); + bank.store_account( + &rent_exempt_validator.node_keypair.pubkey(), + &rent_exempt_validator_account, + ); + + let get_rent_state = |bank: &Bank, address: &Pubkey| -> RentState { + let account = bank + .get_account_with_fixed_root(address) + .unwrap_or_default(); + RentState::from_account(&account, &rent) + }; + + // Assert starting RentStates + assert_eq!( + get_rent_state(&bank, &empty_validator.node_keypair.pubkey()), + RentState::Uninitialized + ); + assert_eq!( + get_rent_state(&bank, &rent_paying_validator.node_keypair.pubkey()), + RentState::RentPaying { + lamports: 42, + data_size: 0, + } + ); + assert_eq!( + get_rent_state(&bank, &becomes_rent_exempt_validator.node_keypair.pubkey()), + RentState::RentPaying { + lamports: rent_exempt_minimum - RENT_PER_VALIDATOR, + data_size: 0, + } + ); + assert_eq!( + get_rent_state(&bank, &rent_exempt_validator.node_keypair.pubkey()), + RentState::RentExempt + ); + + let old_empty_validator_lamports = + bank.get_balance(&empty_validator.node_keypair.pubkey()); + let old_rent_paying_validator_lamports = + bank.get_balance(&rent_paying_validator.node_keypair.pubkey()); + let old_becomes_rent_exempt_validator_lamports = + bank.get_balance(&becomes_rent_exempt_validator.node_keypair.pubkey()); + let old_rent_exempt_validator_lamports = + bank.get_balance(&rent_exempt_validator.node_keypair.pubkey()); + + bank.distribute_rent_to_validators(&bank.vote_accounts(), TOTAL_RENT); + + let new_empty_validator_lamports = + bank.get_balance(&empty_validator.node_keypair.pubkey()); + let new_rent_paying_validator_lamports = + bank.get_balance(&rent_paying_validator.node_keypair.pubkey()); + let new_becomes_rent_exempt_validator_lamports = + bank.get_balance(&becomes_rent_exempt_validator.node_keypair.pubkey()); + let new_rent_exempt_validator_lamports = + bank.get_balance(&rent_exempt_validator.node_keypair.pubkey()); + + // Assert ending balances; rent should be withheld if test is active and ending RentState + // is RentPaying, ie. empty_validator and rent_paying_validator + assert_eq!( + if deactivate_feature { + old_empty_validator_lamports + RENT_PER_VALIDATOR + } else { + old_empty_validator_lamports + }, + new_empty_validator_lamports + ); + + assert_eq!( + if deactivate_feature { + old_rent_paying_validator_lamports + RENT_PER_VALIDATOR + } else { + old_rent_paying_validator_lamports + }, + new_rent_paying_validator_lamports + ); + + assert_eq!( + old_becomes_rent_exempt_validator_lamports + RENT_PER_VALIDATOR, + new_becomes_rent_exempt_validator_lamports + ); + + assert_eq!( + old_rent_exempt_validator_lamports + RENT_PER_VALIDATOR, + new_rent_exempt_validator_lamports + ); + + // Assert ending RentStates + assert_eq!( + if deactivate_feature { + RentState::RentPaying { + lamports: RENT_PER_VALIDATOR, + data_size: 0, + } + } else { + RentState::Uninitialized + }, + get_rent_state(&bank, &empty_validator.node_keypair.pubkey()), + ); + assert_eq!( + if deactivate_feature { + RentState::RentPaying { + lamports: old_rent_paying_validator_lamports + RENT_PER_VALIDATOR, + data_size: 0, + } + } else { + RentState::RentPaying { + lamports: old_rent_paying_validator_lamports, + data_size: 0, + } + }, + get_rent_state(&bank, &rent_paying_validator.node_keypair.pubkey()), + ); + assert_eq!( + RentState::RentExempt, + get_rent_state(&bank, &becomes_rent_exempt_validator.node_keypair.pubkey()), + ); + assert_eq!( + RentState::RentExempt, + get_rent_state(&bank, &rent_exempt_validator.node_keypair.pubkey()), + ); + } + } +} diff --git a/runtime/src/bank/tests.rs b/runtime/src/bank/tests.rs index 343e87975b57a5..6993f65d4e1860 100644 --- a/runtime/src/bank/tests.rs +++ b/runtime/src/bank/tests.rs @@ -979,232 +979,6 @@ fn test_rent_distribution() { ); } -#[test] -fn test_distribute_rent_to_validators_overflow() { - solana_logger::setup(); - - // These values are taken from the real cluster (testnet) - const RENT_TO_BE_DISTRIBUTED: u64 = 120_525; - const VALIDATOR_STAKE: u64 = 374_999_998_287_840; - - let validator_pubkey = solana_sdk::pubkey::new_rand(); - let mut genesis_config = - create_genesis_config_with_leader(10, &validator_pubkey, VALIDATOR_STAKE).genesis_config; - - let bank = Bank::new_for_tests(&genesis_config); - let old_validator_lamports = bank.get_balance(&validator_pubkey); - bank.distribute_rent_to_validators(&bank.vote_accounts(), RENT_TO_BE_DISTRIBUTED); - let new_validator_lamports = bank.get_balance(&validator_pubkey); - assert_eq!( - new_validator_lamports, - old_validator_lamports + RENT_TO_BE_DISTRIBUTED - ); - - genesis_config - .accounts - .remove(&feature_set::no_overflow_rent_distribution::id()) - .unwrap(); - let bank = std::panic::AssertUnwindSafe(Bank::new_for_tests(&genesis_config)); - let old_validator_lamports = bank.get_balance(&validator_pubkey); - let new_validator_lamports = std::panic::catch_unwind(|| { - bank.distribute_rent_to_validators(&bank.vote_accounts(), RENT_TO_BE_DISTRIBUTED); - bank.get_balance(&validator_pubkey) - }); - - if let Ok(new_validator_lamports) = new_validator_lamports { - info!("asserting overflowing incorrect rent distribution"); - assert_ne!( - new_validator_lamports, - old_validator_lamports + RENT_TO_BE_DISTRIBUTED - ); - } else { - info!("NOT-asserting overflowing incorrect rent distribution"); - } -} - -#[test] -fn test_distribute_rent_to_validators_rent_paying() { - solana_logger::setup(); - - const RENT_PER_VALIDATOR: u64 = 55; - const TOTAL_RENT: u64 = RENT_PER_VALIDATOR * 4; - - let empty_validator = ValidatorVoteKeypairs::new_rand(); - let rent_paying_validator = ValidatorVoteKeypairs::new_rand(); - let becomes_rent_exempt_validator = ValidatorVoteKeypairs::new_rand(); - let rent_exempt_validator = ValidatorVoteKeypairs::new_rand(); - let keypairs = vec![ - &empty_validator, - &rent_paying_validator, - &becomes_rent_exempt_validator, - &rent_exempt_validator, - ]; - let genesis_config_info = create_genesis_config_with_vote_accounts( - sol_to_lamports(1000.), - &keypairs, - vec![sol_to_lamports(1000.); 4], - ); - let mut genesis_config = genesis_config_info.genesis_config; - genesis_config.rent = Rent::default(); // Ensure rent is non-zero, as genesis_utils sets Rent::free by default - - for deactivate_feature in [false, true] { - if deactivate_feature { - genesis_config - .accounts - .remove(&feature_set::prevent_rent_paying_rent_recipients::id()) - .unwrap(); - } - let bank = Bank::new_for_tests(&genesis_config); - let rent = bank.rent_collector().rent; - let rent_exempt_minimum = rent.minimum_balance(0); - - // Make one validator have an empty identity account - let mut empty_validator_account = bank - .get_account_with_fixed_root(&empty_validator.node_keypair.pubkey()) - .unwrap(); - empty_validator_account.set_lamports(0); - bank.store_account( - &empty_validator.node_keypair.pubkey(), - &empty_validator_account, - ); - - // Make one validator almost rent-exempt, less RENT_PER_VALIDATOR - let mut becomes_rent_exempt_validator_account = bank - .get_account_with_fixed_root(&becomes_rent_exempt_validator.node_keypair.pubkey()) - .unwrap(); - becomes_rent_exempt_validator_account - .set_lamports(rent_exempt_minimum - RENT_PER_VALIDATOR); - bank.store_account( - &becomes_rent_exempt_validator.node_keypair.pubkey(), - &becomes_rent_exempt_validator_account, - ); - - // Make one validator rent-exempt - let mut rent_exempt_validator_account = bank - .get_account_with_fixed_root(&rent_exempt_validator.node_keypair.pubkey()) - .unwrap(); - rent_exempt_validator_account.set_lamports(rent_exempt_minimum); - bank.store_account( - &rent_exempt_validator.node_keypair.pubkey(), - &rent_exempt_validator_account, - ); - - let get_rent_state = |bank: &Bank, address: &Pubkey| -> RentState { - let account = bank - .get_account_with_fixed_root(address) - .unwrap_or_default(); - RentState::from_account(&account, &rent) - }; - - // Assert starting RentStates - assert_eq!( - get_rent_state(&bank, &empty_validator.node_keypair.pubkey()), - RentState::Uninitialized - ); - assert_eq!( - get_rent_state(&bank, &rent_paying_validator.node_keypair.pubkey()), - RentState::RentPaying { - lamports: 42, - data_size: 0, - } - ); - assert_eq!( - get_rent_state(&bank, &becomes_rent_exempt_validator.node_keypair.pubkey()), - RentState::RentPaying { - lamports: rent_exempt_minimum - RENT_PER_VALIDATOR, - data_size: 0, - } - ); - assert_eq!( - get_rent_state(&bank, &rent_exempt_validator.node_keypair.pubkey()), - RentState::RentExempt - ); - - let old_empty_validator_lamports = bank.get_balance(&empty_validator.node_keypair.pubkey()); - let old_rent_paying_validator_lamports = - bank.get_balance(&rent_paying_validator.node_keypair.pubkey()); - let old_becomes_rent_exempt_validator_lamports = - bank.get_balance(&becomes_rent_exempt_validator.node_keypair.pubkey()); - let old_rent_exempt_validator_lamports = - bank.get_balance(&rent_exempt_validator.node_keypair.pubkey()); - - bank.distribute_rent_to_validators(&bank.vote_accounts(), TOTAL_RENT); - - let new_empty_validator_lamports = bank.get_balance(&empty_validator.node_keypair.pubkey()); - let new_rent_paying_validator_lamports = - bank.get_balance(&rent_paying_validator.node_keypair.pubkey()); - let new_becomes_rent_exempt_validator_lamports = - bank.get_balance(&becomes_rent_exempt_validator.node_keypair.pubkey()); - let new_rent_exempt_validator_lamports = - bank.get_balance(&rent_exempt_validator.node_keypair.pubkey()); - - // Assert ending balances; rent should be withheld if test is active and ending RentState - // is RentPaying, ie. empty_validator and rent_paying_validator - assert_eq!( - if deactivate_feature { - old_empty_validator_lamports + RENT_PER_VALIDATOR - } else { - old_empty_validator_lamports - }, - new_empty_validator_lamports - ); - - assert_eq!( - if deactivate_feature { - old_rent_paying_validator_lamports + RENT_PER_VALIDATOR - } else { - old_rent_paying_validator_lamports - }, - new_rent_paying_validator_lamports - ); - - assert_eq!( - old_becomes_rent_exempt_validator_lamports + RENT_PER_VALIDATOR, - new_becomes_rent_exempt_validator_lamports - ); - - assert_eq!( - old_rent_exempt_validator_lamports + RENT_PER_VALIDATOR, - new_rent_exempt_validator_lamports - ); - - // Assert ending RentStates - assert_eq!( - if deactivate_feature { - RentState::RentPaying { - lamports: RENT_PER_VALIDATOR, - data_size: 0, - } - } else { - RentState::Uninitialized - }, - get_rent_state(&bank, &empty_validator.node_keypair.pubkey()), - ); - assert_eq!( - if deactivate_feature { - RentState::RentPaying { - lamports: old_rent_paying_validator_lamports + RENT_PER_VALIDATOR, - data_size: 0, - } - } else { - RentState::RentPaying { - lamports: old_rent_paying_validator_lamports, - data_size: 0, - } - }, - get_rent_state(&bank, &rent_paying_validator.node_keypair.pubkey()), - ); - assert_eq!( - RentState::RentExempt, - get_rent_state(&bank, &becomes_rent_exempt_validator.node_keypair.pubkey()), - ); - assert_eq!( - RentState::RentExempt, - get_rent_state(&bank, &rent_exempt_validator.node_keypair.pubkey()), - ); - } -} - #[test] fn test_rent_exempt_executable_account() { let (mut genesis_config, mint_keypair) = create_genesis_config(100_000); From ea1f7f3c6d6fb12c498e1c6a50bdedd5b381ac0a Mon Sep 17 00:00:00 2001 From: Justin Starry Date: Wed, 25 Oct 2023 11:16:39 +0800 Subject: [PATCH 2/8] feature: add checks to fee distribution --- runtime/src/bank.rs | 8 +- runtime/src/bank/fee_distribution.rs | 610 +++++++++++++++++++++++---- sdk/src/feature_set.rs | 5 + 3 files changed, 548 insertions(+), 75 deletions(-) diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 2aa0baaff8210b..502e12bc1b5132 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -148,7 +148,6 @@ use { incinerator, inflation::Inflation, instruction::InstructionError, - lamports::LamportsError, loader_v4::{self, LoaderV4State, LoaderV4Status}, message::{AccountKeys, SanitizedMessage}, native_loader, @@ -3805,7 +3804,7 @@ impl Bank { .stakes_cache .stakes() .highest_staked_node() - .unwrap_or_default(); + .unwrap_or_else(Pubkey::new_unique); self.blockhash_queue.write().unwrap().genesis_hash( &genesis_config.hash(), @@ -7699,6 +7698,11 @@ impl Bank { .is_active(&feature_set::prevent_rent_paying_rent_recipients::id()) } + pub fn validate_fee_collector_account(&self) -> bool { + self.feature_set + .is_active(&feature_set::validate_fee_collector_account::id()) + } + pub fn read_cost_tracker(&self) -> LockResult> { self.cost_tracker.read() } diff --git a/runtime/src/bank/fee_distribution.rs b/runtime/src/bank/fee_distribution.rs index 41f83c41cce929..71ba1a6583dace 100644 --- a/runtime/src/bank/fee_distribution.rs +++ b/runtime/src/bank/fee_distribution.rs @@ -6,11 +6,29 @@ use { account::{ReadableAccount, WritableAccount}, pubkey::Pubkey, reward_type::RewardType, + system_program, }, solana_vote::vote_account::VoteAccountsHashMap, - std::sync::atomic::Ordering::Relaxed, + std::{result::Result, sync::atomic::Ordering::Relaxed}, + thiserror::Error, }; +#[derive(Debug)] +struct DepositFeeOptions { + check_account_owner: bool, + check_rent_paying: bool, +} + +#[derive(Error, Debug)] +enum DepositFeeError { + #[error("fee account became rent paying")] + InvalidRentPayingAccount, + #[error("lamport overflow")] + LamportOverflow, + #[error("invalid fee account owner")] + InvalidAccountOwner, +} + impl Bank { // Distribute collected transaction fees for this slot to collector_id (= current leader). // @@ -28,18 +46,19 @@ impl Bank { // Ref: distribute_rent_to_validators pub(super) fn distribute_transaction_fees(&self) { let collector_fees = self.collector_fees.load(Relaxed); - if collector_fees != 0 { let (deposit, mut burn) = self.fee_rate_governor.burn(collector_fees); - // burn a portion of fees - debug!( - "distributed fee: {} (rounded from: {}, burned: {})", - deposit, collector_fees, burn - ); - - match self.deposit(&self.collector_id, deposit) { - Ok(post_balance) => { - if deposit != 0 { + 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 { @@ -50,24 +69,56 @@ impl Bank { }, )); } - } - Err(_) => { - error!( - "Burning {} fee instead of crediting {}", - deposit, self.collector_id - ); - datapoint_error!( - "bank-burned_fee", - ("slot", self.slot(), i64), - ("num_lamports", deposit, i64) - ); - burn += deposit; + Err(err) => { + error!( + "Burning {} fee instead of crediting {} due to {err}", + deposit, self.collector_id + ); + datapoint_error!( + "bank-burned_fee", + ("slot", self.slot(), i64), + ("num_lamports", deposit, i64) + ); + burn += deposit; + } } } self.capitalization.fetch_sub(burn, Relaxed); } } + // Deposits fees into a specified account and if successful, returns the new balance of that account + fn deposit_fees( + &self, + pubkey: &Pubkey, + fees: u64, + options: DepositFeeOptions, + ) -> Result { + let mut account = self.get_account_with_fixed_root(pubkey).unwrap_or_default(); + + if options.check_account_owner && !system_program::check_id(account.owner()) { + return Err(DepositFeeError::InvalidAccountOwner); + } + + let rent = self.rent_collector().rent; + let recipient_pre_rent_state = RentState::from_account(&account, &rent); + let distribution = account.checked_add_lamports(fees); + if distribution.is_err() { + return Err(DepositFeeError::LamportOverflow); + } + if options.check_rent_paying { + let recipient_post_rent_state = RentState::from_account(&account, &rent); + let rent_state_transition_allowed = + recipient_post_rent_state.transition_allowed_from(&recipient_pre_rent_state); + if !rent_state_transition_allowed { + return Err(DepositFeeError::InvalidRentPayingAccount); + } + } + + self.store_account(pubkey, &account); + Ok(account.lamports()) + } + // Distribute collected rent fees for this slot to staked validators (excluding stakers) // according to stake. // @@ -161,53 +212,40 @@ impl Bank { rent_share }; if !enforce_fix || rent_to_be_paid > 0 { - let mut account = self - .get_account_with_fixed_root(&pubkey) - .unwrap_or_default(); - let rent = self.rent_collector().rent; - let recipient_pre_rent_state = RentState::from_account(&account, &rent); - let distribution = account.checked_add_lamports(rent_to_be_paid); - let recipient_post_rent_state = RentState::from_account(&account, &rent); - let rent_state_transition_allowed = recipient_post_rent_state - .transition_allowed_from(&recipient_pre_rent_state); - if !rent_state_transition_allowed { - warn!( - "Rent distribution of {rent_to_be_paid} to {pubkey} results in \ - invalid RentState: {recipient_post_rent_state:?}" - ); - datapoint_warn!( - "bank-rent_distribution_invalid_state", - ("slot", self.slot(), i64), - ("pubkey", pubkey.to_string(), String), - ("rent_to_be_paid", rent_to_be_paid, i64) - ); - } - if distribution.is_err() - || (self.prevent_rent_paying_rent_recipients() - && !rent_state_transition_allowed) - { - // overflow adding lamports or resulting account is not rent-exempt - self.capitalization.fetch_sub(rent_to_be_paid, Relaxed); - error!( - "Burned {} rent lamports instead of sending to {}", - rent_to_be_paid, pubkey - ); - datapoint_error!( - "bank-burned_rent", - ("slot", self.slot(), i64), - ("num_lamports", rent_to_be_paid, i64) - ); - } else { - self.store_account(&pubkey, &account); - rewards.push(( - pubkey, - RewardInfo { - reward_type: RewardType::Rent, - lamports: rent_to_be_paid as i64, - post_balance: account.lamports(), - commission: None, - }, - )); + let check_account_owner = self.validate_fee_collector_account(); + let check_rent_paying = self.prevent_rent_paying_rent_recipients(); + match self.deposit_fees( + &pubkey, + rent_to_be_paid, + DepositFeeOptions { + check_account_owner, + check_rent_paying, + }, + ) { + Ok(post_balance) => { + rewards.push(( + pubkey, + RewardInfo { + reward_type: RewardType::Rent, + lamports: rent_to_be_paid as i64, + post_balance, + commission: None, + }, + )); + } + Err(_) => { + // overflow adding lamports or resulting account is invalid + self.capitalization.fetch_sub(rent_to_be_paid, Relaxed); + error!( + "Burned {} rent lamports instead of sending to {}", + rent_to_be_paid, pubkey + ); + datapoint_error!( + "bank-burned_rent", + ("slot", self.slot(), i64), + ("num_lamports", rent_to_be_paid, i64) + ); + } } } }); @@ -251,13 +289,377 @@ pub mod tests { use { super::*, crate::genesis_utils::{ - create_genesis_config_with_leader, create_genesis_config_with_vote_accounts, - ValidatorVoteKeypairs, + create_genesis_config, create_genesis_config_with_leader, + create_genesis_config_with_vote_accounts, ValidatorVoteKeypairs, }, log::info, - solana_sdk::{feature_set, native_token::sol_to_lamports, rent::Rent, signature::Signer}, + solana_sdk::{ + account::AccountSharedData, feature_set, native_token::sol_to_lamports, pubkey, + rent::Rent, signature::Signer, + }, }; + #[test] + fn test_distribute_transaction_fees() { + #[derive(PartialEq)] + enum Scenario { + Normal, + InvalidOwner, + RentPaying, + } + + struct TestCase { + scenario: Scenario, + disable_checks: bool, + } + + impl TestCase { + fn new(scenario: Scenario, disable_checks: bool) -> Self { + Self { + scenario, + disable_checks, + } + } + } + + for test_case in [ + TestCase::new(Scenario::Normal, false), + TestCase::new(Scenario::Normal, true), + TestCase::new(Scenario::InvalidOwner, false), + TestCase::new(Scenario::InvalidOwner, true), + TestCase::new(Scenario::RentPaying, false), + TestCase::new(Scenario::RentPaying, true), + ] { + let mut genesis = create_genesis_config(0); + if test_case.disable_checks { + genesis + .genesis_config + .accounts + .remove(&feature_set::validate_fee_collector_account::id()) + .unwrap(); + } + let rent = Rent::default(); + 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.fetch_add(transaction_fees, Relaxed); + assert_eq!(transaction_fees, bank.collector_fees.load(Relaxed)); + let (expected_collected_fees, burn_amount) = + bank.fee_rate_governor.burn(transaction_fees); + assert!(burn_amount > 0); + + if test_case.scenario == Scenario::RentPaying { + // ensure that account balance + collected fees will make it rent-paying + 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); + } else if test_case.scenario == Scenario::InvalidOwner { + // ensure that account owner is invalid and fee distribution will fail + let account = + AccountSharedData::new(min_rent_exempt_balance, 0, &Pubkey::new_unique()); + bank.store_account(bank.collector_id(), &account); + } else { + let account = + AccountSharedData::new(min_rent_exempt_balance, 0, &system_program::id()); + bank.store_account(bank.collector_id(), &account); + } + + let initial_capitalization = bank.capitalization(); + let initial_collector_id_balance = bank.get_balance(bank.collector_id()); + bank.distribute_transaction_fees(); + let new_collector_id_balance = bank.get_balance(bank.collector_id()); + + 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, + bank.capitalization() + ); + let locked_rewards = bank.rewards.read().unwrap(); + assert!( + locked_rewards.is_empty(), + "There should be no rewards distributed" + ); + } else { + assert_eq!( + initial_collector_id_balance + expected_collected_fees, + new_collector_id_balance + ); + + assert_eq!(initial_capitalization - burn_amount, bank.capitalization()); + + let locked_rewards = bank.rewards.read().unwrap(); + assert_eq!( + locked_rewards.len(), + 1, + "There should be one reward distributed" + ); + + let reward_info = &locked_rewards[0]; + assert_eq!( + reward_info.1.lamports, expected_collected_fees as i64, + "The reward amount should match the expected deposit" + ); + assert_eq!( + reward_info.1.reward_type, + RewardType::Fee, + "The reward type should be Fee" + ); + } + } + } + + #[test] + 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); + + let initial_capitalization = bank.capitalization(); + let initial_collector_id_balance = bank.get_balance(bank.collector_id()); + bank.distribute_transaction_fees(); + let new_collector_id_balance = bank.get_balance(bank.collector_id()); + + assert_eq!(initial_collector_id_balance, new_collector_id_balance); + assert_eq!(initial_capitalization, bank.capitalization()); + let locked_rewards = bank.rewards.read().unwrap(); + assert!( + locked_rewards.is_empty(), + "There should be no rewards distributed" + ); + } + + #[test] + fn test_distribute_transaction_fees_burn_all() { + 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.fetch_add(transaction_fees, Relaxed); + assert_eq!(transaction_fees, bank.collector_fees.load(Relaxed)); + + let initial_capitalization = bank.capitalization(); + let initial_collector_id_balance = bank.get_balance(bank.collector_id()); + bank.distribute_transaction_fees(); + let new_collector_id_balance = bank.get_balance(bank.collector_id()); + + assert_eq!(initial_collector_id_balance, new_collector_id_balance); + assert_eq!( + initial_capitalization - transaction_fees, + bank.capitalization() + ); + let locked_rewards = bank.rewards.read().unwrap(); + assert!( + locked_rewards.is_empty(), + "There should be no rewards distributed" + ); + } + + #[test] + 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.fetch_add(transaction_fees, Relaxed); + assert_eq!(transaction_fees, bank.collector_fees.load(Relaxed)); + + // ensure that account balance will overflow and fee distribution will fail + let account = AccountSharedData::new(u64::MAX, 0, &system_program::id()); + bank.store_account(bank.collector_id(), &account); + + let initial_capitalization = bank.capitalization(); + let initial_collector_id_balance = bank.get_balance(bank.collector_id()); + bank.distribute_transaction_fees(); + let new_collector_id_balance = bank.get_balance(bank.collector_id()); + + assert_eq!(initial_collector_id_balance, new_collector_id_balance); + assert_eq!( + initial_capitalization - transaction_fees, + bank.capitalization() + ); + let locked_rewards = bank.rewards.read().unwrap(); + assert!( + locked_rewards.is_empty(), + "There should be no rewards distributed" + ); + } + + #[test] + fn test_deposit_fees() { + let initial_balance = 1_000_000_000; + let genesis = create_genesis_config(initial_balance); + let bank = Bank::new_for_tests(&genesis.genesis_config); + let pubkey = genesis.mint_keypair.pubkey(); + + let deposit_amount = 500; + let options = DepositFeeOptions { + check_account_owner: true, + check_rent_paying: true, + }; + + // Call the deposit_fees function + let result = bank.deposit_fees(&pubkey, deposit_amount, options); + + // Check the result + match result { + Ok(new_balance) => { + assert_eq!( + new_balance, + initial_balance + deposit_amount, + "New balance should be the sum of the initial balance and deposit amount" + ); + } + Err(e) => panic!("Unexpected error: {:?}", e), + } + } + + #[test] + fn test_deposit_fees_with_overflow() { + let initial_balance = u64::MAX; + let genesis = create_genesis_config(initial_balance); + let bank = Bank::new_for_tests(&genesis.genesis_config); + let pubkey = genesis.mint_keypair.pubkey(); + + let deposit_amount = 500; + let options = DepositFeeOptions { + check_account_owner: false, + check_rent_paying: false, + }; + + // Call the deposit_fees function + let result = bank.deposit_fees(&pubkey, deposit_amount, options); + + // Check the result + match result { + Ok(_) => panic!("Expected an error due to lamport overflow, but received Ok"), + Err(e) => { + match e { + DepositFeeError::LamportOverflow => {} // This is the expected error + _ => panic!("Unexpected error: {:?}", e), + } + } + } + } + + #[test] + fn test_deposit_fees_invalid_account_owner() { + let initial_balance = 1000; + let genesis = create_genesis_config_with_leader(0, &pubkey::new_rand(), initial_balance); + let bank = Bank::new_for_tests(&genesis.genesis_config); + let pubkey = genesis.voting_keypair.pubkey(); + + let deposit_amount = 500; + + // enable check_account_owner + { + let options = DepositFeeOptions { + check_account_owner: true, // Intentionally checking for account owner + check_rent_paying: false, + }; + + // Call the deposit_fees function + let result = bank.deposit_fees(&pubkey, deposit_amount, options); + + // Check the result + match result { + Ok(_) => panic!("Expected an error due to invalid account owner, but received Ok"), + Err(e) => { + match e { + DepositFeeError::InvalidAccountOwner => {} // This is the expected error + _ => panic!("Unexpected error: {:?}", e), + } + } + } + } + + // disable check_account_owner + { + let options = DepositFeeOptions { + check_account_owner: false, + check_rent_paying: false, + }; + + // Call the deposit_fees function + let result = bank.deposit_fees(&pubkey, deposit_amount, options); + + // Check the result + match result { + Ok(new_balance) => { + assert_eq!( + new_balance, + initial_balance + deposit_amount, + "New balance should be the sum of the initial balance and deposit amount" + ); + } + Err(e) => panic!("Unexpected error: {:?}", e), + } + } + } + + #[test] + fn test_deposit_fees_invalid_rent_paying() { + let initial_balance = 0; + let genesis = create_genesis_config(initial_balance); + let pubkey = genesis.mint_keypair.pubkey(); + let mut genesis_config = genesis.genesis_config; + let rent = Rent::default(); + genesis_config.rent = rent; // Ensure rent is non-zero, as genesis_utils sets Rent::free by default + let bank = Bank::new_for_tests(&genesis_config); + let min_rent_exempt_balance = rent.minimum_balance(0); + + let deposit_amount = 500; + assert!(initial_balance + deposit_amount < min_rent_exempt_balance); + + // enable check_rent_paying + { + let options = DepositFeeOptions { + check_account_owner: false, + check_rent_paying: true, + }; + + // Call the deposit_fees function + let result = bank.deposit_fees(&pubkey, deposit_amount, options); + + // Check the result + match result { + Ok(_) => { + panic!("Expected an error due to invalid rent paying account, but received Ok") + } + Err(e) => { + match e { + DepositFeeError::InvalidRentPayingAccount => {} // This is the expected error + _ => panic!("Unexpected error: {:?}", e), + } + } + } + } + + // disable check_rent_paying + { + let options = DepositFeeOptions { + check_account_owner: false, + check_rent_paying: false, + }; + + // Call the deposit_fees function + let result = bank.deposit_fees(&pubkey, deposit_amount, options); + + // Check the result + match result { + Ok(new_balance) => { + assert_eq!( + new_balance, + initial_balance + deposit_amount, + "New balance should be the sum of the initial balance and deposit amount" + ); + } + Err(e) => panic!("Unexpected error: {:?}", e), + } + } + } + #[test] fn test_distribute_rent_to_validators_overflow() { solana_logger::setup(); @@ -486,4 +888,66 @@ pub mod tests { ); } } + + #[test] + fn test_distribute_rent_to_validators_invalid_owner() { + struct TestCase { + disable_owner_check: bool, + use_invalid_owner: bool, + } + + impl TestCase { + fn new(disable_owner_check: bool, use_invalid_owner: bool) -> Self { + Self { + disable_owner_check, + use_invalid_owner, + } + } + } + + for test_case in [ + TestCase::new(false, false), + TestCase::new(false, true), + TestCase::new(true, false), + TestCase::new(true, true), + ] { + let genesis_config_info = + create_genesis_config_with_leader(0, &Pubkey::new_unique(), 100); + let mut genesis_config = genesis_config_info.genesis_config; + genesis_config.rent = Rent::default(); // Ensure rent is non-zero, as genesis_utils sets Rent::free by default + + if test_case.disable_owner_check { + genesis_config + .accounts + .remove(&feature_set::validate_fee_collector_account::id()) + .unwrap(); + } + let bank = Bank::new_for_tests(&genesis_config); + + let initial_balance = 1_000_000; + let account_owner = if test_case.use_invalid_owner { + Pubkey::new_unique() + } else { + system_program::id() + }; + let account = AccountSharedData::new(initial_balance, 0, &account_owner); + bank.store_account(bank.collector_id(), &account); + + let initial_capitalization = bank.capitalization(); + let rent_fees = 100; + bank.distribute_rent_to_validators(&bank.vote_accounts(), rent_fees); + let new_capitalization = bank.capitalization(); + let new_balance = bank.get_balance(bank.collector_id()); + + if test_case.use_invalid_owner && !test_case.disable_owner_check { + assert_eq!(initial_balance, new_balance); + assert_eq!(initial_capitalization - rent_fees, new_capitalization); + assert_eq!(bank.rewards.read().unwrap().len(), 0); + } else { + assert_eq!(initial_balance + rent_fees, new_balance); + assert_eq!(initial_capitalization, new_capitalization); + assert_eq!(bank.rewards.read().unwrap().len(), 1); + } + } + } } diff --git a/sdk/src/feature_set.rs b/sdk/src/feature_set.rs index 8682836c2ba247..aabe184d4408db 100644 --- a/sdk/src/feature_set.rs +++ b/sdk/src/feature_set.rs @@ -724,6 +724,10 @@ pub mod update_hashes_per_tick6 { solana_sdk::declare_id!("FKu1qYwLQSiehz644H6Si65U5ZQ2cp9GxsyFUfYcuADv"); } +pub mod validate_fee_collector_account { + solana_sdk::declare_id!("prpFrMtgNmzaNzkPJg9o753fVvbHKqNrNTm76foJ2wm"); +} + lazy_static! { /// Map of feature identifiers to user-visible description pub static ref FEATURE_NAMES: HashMap = [ @@ -900,6 +904,7 @@ lazy_static! { (update_hashes_per_tick4::id(), "Update desired hashes per tick to 7.6M"), (update_hashes_per_tick5::id(), "Update desired hashes per tick to 9.2M"), (update_hashes_per_tick6::id(), "Update desired hashes per tick to 10M"), + (validate_fee_collector_account::id(), "validate fee collector account #??"), /*************** ADD NEW FEATURES HERE ***************/ ] .iter() From 7d204a2636993280edc4470aa29e36535595f5bf Mon Sep 17 00:00:00 2001 From: Justin Starry Date: Wed, 25 Oct 2023 11:26:17 +0800 Subject: [PATCH 3/8] refactor: move Bank::deposit fn into test_utils --- runtime/benches/accounts.rs | 4 +-- runtime/src/bank.rs | 33 +++++++++++++----------- runtime/src/bank/serde_snapshot.rs | 10 ++++---- runtime/src/bank/tests.rs | 40 +++++++++--------------------- 4 files changed, 38 insertions(+), 49 deletions(-) diff --git a/runtime/benches/accounts.rs b/runtime/benches/accounts.rs index 43361669244578..993c22d2a04e18 100644 --- a/runtime/benches/accounts.rs +++ b/runtime/benches/accounts.rs @@ -42,7 +42,7 @@ fn deposit_many(bank: &Bank, pubkeys: &mut Vec, num: usize) -> Result<() AccountSharedData::new((t + 1) as u64, 0, AccountSharedData::default().owner()); pubkeys.push(pubkey); assert!(bank.get_account(&pubkey).is_none()); - bank.deposit(&pubkey, (t + 1) as u64)?; + test_utils::deposit(bank, &pubkey, (t + 1) as u64)?; assert_eq!(bank.get_account(&pubkey).unwrap(), account); } Ok(()) @@ -80,7 +80,7 @@ fn test_accounts_squash(bencher: &mut Bencher) { &Pubkey::default(), slot, )); - next_bank.deposit(&pubkeys[0], 1).unwrap(); + test_utils::deposit(&next_bank, &pubkeys[0], 1).unwrap(); next_bank.squash(); slot += 1; prev_bank = next_bank; diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 502e12bc1b5132..724af576c5f5fc 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -6483,19 +6483,6 @@ impl Bank { } } - pub fn deposit( - &self, - pubkey: &Pubkey, - lamports: u64, - ) -> std::result::Result { - // This doesn't collect rents intentionally. - // Rents should only be applied to actual TXes - let mut account = self.get_account_with_fixed_root(pubkey).unwrap_or_default(); - account.checked_add_lamports(lamports)?; - self.store_account(pubkey, &account); - Ok(account.lamports()) - } - pub fn accounts(&self) -> Arc { self.rc.accounts.clone() } @@ -8243,7 +8230,12 @@ pub mod test_utils { use { super::Bank, crate::installed_scheduler_pool::BankWithScheduler, - solana_sdk::{hash::hashv, pubkey::Pubkey}, + solana_sdk::{ + account::{ReadableAccount, WritableAccount}, + hash::hashv, + lamports::LamportsError, + pubkey::Pubkey, + }, solana_vote_program::vote_state::{self, BlockTimestamp, VoteStateVersions}, std::sync::Arc, }; @@ -8275,4 +8267,17 @@ pub mod test_utils { vote_state::to(&versioned, &mut vote_account).unwrap(); bank.store_account(vote_pubkey, &vote_account); } + + pub fn deposit( + bank: &Bank, + pubkey: &Pubkey, + lamports: u64, + ) -> std::result::Result { + // This doesn't collect rents intentionally. + // Rents should only be applied to actual TXes + let mut account = bank.get_account_with_fixed_root(pubkey).unwrap_or_default(); + account.checked_add_lamports(lamports)?; + bank.store_account(pubkey, &account); + Ok(account.lamports()) + } } diff --git a/runtime/src/bank/serde_snapshot.rs b/runtime/src/bank/serde_snapshot.rs index 17bba5638f2d47..e1746c52b79f75 100644 --- a/runtime/src/bank/serde_snapshot.rs +++ b/runtime/src/bank/serde_snapshot.rs @@ -3,8 +3,8 @@ mod tests { use { crate::{ bank::{ - epoch_accounts_hash_utils, Bank, BankTestConfig, EpochRewardStatus, - StartBlockHeightAndRewards, + epoch_accounts_hash_utils, test_utils as bank_test_utils, Bank, BankTestConfig, + EpochRewardStatus, StartBlockHeightAndRewards, }, genesis_utils::{activate_all_features, activate_feature}, runtime_config::RuntimeConfig, @@ -109,7 +109,7 @@ mod tests { // Create an account on a non-root fork let key1 = Keypair::new(); - bank1.deposit(&key1.pubkey(), 5).unwrap(); + bank_test_utils::deposit(&bank1, &key1.pubkey(), 5).unwrap(); // If setting an initial EAH, then the bank being snapshotted must be in the EAH calculation // window. Otherwise `bank_to_stream()` below will *not* include the EAH in the bank snapshot, @@ -123,11 +123,11 @@ mod tests { // Test new account let key2 = Keypair::new(); - bank2.deposit(&key2.pubkey(), 10).unwrap(); + bank_test_utils::deposit(&bank2, &key2.pubkey(), 10).unwrap(); assert_eq!(bank2.get_balance(&key2.pubkey()), 10); let key3 = Keypair::new(); - bank2.deposit(&key3.pubkey(), 0).unwrap(); + bank_test_utils::deposit(&bank2, &key3.pubkey(), 0).unwrap(); bank2.freeze(); bank2.squash(); diff --git a/runtime/src/bank/tests.rs b/runtime/src/bank/tests.rs index 6993f65d4e1860..c72936476a5823 100644 --- a/runtime/src/bank/tests.rs +++ b/runtime/src/bank/tests.rs @@ -2407,22 +2407,6 @@ fn test_transfer_to_sysvar() { assert_eq!(bank.get_balance(&sysvar_pubkey), 1_169_280); } -#[test] -fn test_bank_deposit() { - let bank = create_simple_test_bank(100); - - // Test new account - let key = solana_sdk::pubkey::new_rand(); - let new_balance = bank.deposit(&key, 10).unwrap(); - assert_eq!(new_balance, 10); - assert_eq!(bank.get_balance(&key), 10); - - // Existing account - let new_balance = bank.deposit(&key, 3).unwrap(); - assert_eq!(new_balance, 13); - assert_eq!(bank.get_balance(&key), 13); -} - #[test] fn test_bank_withdraw() { let bank = create_simple_test_bank(100); @@ -2434,7 +2418,7 @@ fn test_bank_withdraw() { Err(TransactionError::AccountNotFound) ); - bank.deposit(&key, 3).unwrap(); + test_utils::deposit(&bank, &key, 3).unwrap(); assert_eq!(bank.get_balance(&key), 3); // Low balance @@ -6453,7 +6437,7 @@ fn test_clean_nonrooted() { // Store some lamports in bank 1 let some_lamports = 123; let bank1 = Arc::new(Bank::new_from_parent(bank0.clone(), &Pubkey::default(), 1)); - bank1.deposit(&pubkey0, some_lamports).unwrap(); + test_utils::deposit(&bank1, &pubkey0, some_lamports).unwrap(); goto_end_of_slot(bank1.clone()); bank1.freeze(); bank1.flush_accounts_cache_slot_for_tests(); @@ -6463,7 +6447,7 @@ fn test_clean_nonrooted() { // Store some lamports for pubkey1 in bank 2, root bank 2 // bank2's parent is bank0 let bank2 = Arc::new(Bank::new_from_parent(bank0, &Pubkey::default(), 2)); - bank2.deposit(&pubkey1, some_lamports).unwrap(); + test_utils::deposit(&bank2, &pubkey1, some_lamports).unwrap(); bank2.store_account(&pubkey0, &account_zero); goto_end_of_slot(bank2.clone()); bank2.freeze(); @@ -6478,7 +6462,7 @@ fn test_clean_nonrooted() { bank2.clean_accounts_for_tests(); let bank3 = Arc::new(Bank::new_from_parent(bank2, &Pubkey::default(), 3)); - bank3.deposit(&pubkey1, some_lamports + 1).unwrap(); + test_utils::deposit(&bank3, &pubkey1, some_lamports + 1).unwrap(); goto_end_of_slot(bank3.clone()); bank3.freeze(); bank3.squash(); @@ -6532,8 +6516,8 @@ fn test_shrink_candidate_slots_cached() { // Store some lamports in bank 1 let some_lamports = 123; let bank1 = Arc::new(new_from_parent(bank0)); - bank1.deposit(&pubkey1, some_lamports).unwrap(); - bank1.deposit(&pubkey2, some_lamports).unwrap(); + test_utils::deposit(&bank1, &pubkey1, some_lamports).unwrap(); + test_utils::deposit(&bank1, &pubkey2, some_lamports).unwrap(); goto_end_of_slot(bank1.clone()); bank1.freeze(); bank1.squash(); @@ -6543,7 +6527,7 @@ fn test_shrink_candidate_slots_cached() { // Store some lamports for pubkey1 in bank 2, root bank 2 let bank2 = Arc::new(new_from_parent(bank1)); - bank2.deposit(&pubkey1, some_lamports).unwrap(); + test_utils::deposit(&bank2, &pubkey1, some_lamports).unwrap(); bank2.store_account(&pubkey0, &account0); goto_end_of_slot(bank2.clone()); bank2.freeze(); @@ -6740,7 +6724,7 @@ fn test_add_builtin_account_inherited_cap_while_replacing() { assert_ne!(bank.capitalization(), bank.calculate_capitalization(true)); continue; } - bank.deposit(&program_id, 10).unwrap(); + test_utils::deposit(&bank, &program_id, 10).unwrap(); if pass == 2 { add_root_and_flush_write_cache(&bank); assert_eq!(bank.capitalization(), bank.calculate_capitalization(true)); @@ -6767,7 +6751,7 @@ fn test_add_builtin_account_squatted_while_not_replacing() { assert_ne!(bank.capitalization(), bank.calculate_capitalization(true)); continue; } - bank.deposit(&program_id, 10).unwrap(); + test_utils::deposit(&bank, &program_id, 10).unwrap(); if pass == 1 { add_root_and_flush_write_cache(&bank); assert_eq!(bank.capitalization(), bank.calculate_capitalization(true)); @@ -6890,7 +6874,7 @@ fn test_add_precompiled_account_inherited_cap_while_replacing() { assert_ne!(bank.capitalization(), bank.calculate_capitalization(true)); continue; } - bank.deposit(&program_id, 10).unwrap(); + test_utils::deposit(&bank, &program_id, 10).unwrap(); if pass == 2 { add_root_and_flush_write_cache(&bank); assert_eq!(bank.capitalization(), bank.calculate_capitalization(true)); @@ -6918,7 +6902,7 @@ fn test_add_precompiled_account_squatted_while_not_replacing() { assert_ne!(bank.capitalization(), bank.calculate_capitalization(true)); continue; } - bank.deposit(&program_id, 10).unwrap(); + test_utils::deposit(&bank, &program_id, 10).unwrap(); if pass == 1 { add_root_and_flush_write_cache(&bank); assert_eq!(bank.capitalization(), bank.calculate_capitalization(true)); @@ -7754,7 +7738,7 @@ fn test_compute_active_feature_set() { assert!(!feature_set.is_active(&test_feature)); // Depositing into the `test_feature` account should do nothing - bank.deposit(&test_feature, 42).unwrap(); + test_utils::deposit(&bank, &test_feature, 42).unwrap(); let (feature_set, new_activations) = bank.compute_active_feature_set(true); assert!(new_activations.is_empty()); assert!(!feature_set.is_active(&test_feature)); From 4cb3bca5117f5965e5b3b1465e467307c657d27e Mon Sep 17 00:00:00 2001 From: Justin Starry Date: Mon, 30 Oct 2023 13:14:33 +0100 Subject: [PATCH 4/8] feedback --- runtime/src/bank.rs | 18 +++- runtime/src/bank/fee_distribution.rs | 142 ++++++++------------------- sdk/src/feature_set.rs | 2 +- 3 files changed, 55 insertions(+), 107 deletions(-) diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 724af576c5f5fc..5c559493d1bde3 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -3800,11 +3800,19 @@ impl Bank { } // highest staked node is the first collector - self.collector_id = self - .stakes_cache - .stakes() - .highest_staked_node() - .unwrap_or_else(Pubkey::new_unique); + let highest_staked_node = self.stakes_cache.stakes().highest_staked_node(); + + #[cfg(test)] + { + // in tests, use a new unique pubkey as the collector id if no staked + // nodes are defined in the genesis config + self.collector_id = highest_staked_node.unwrap_or_else(Pubkey::new_unique); + } + + #[cfg(not(test))] + { + self.collector_id = highest_staked_node.expect("no staked nodes available"); + } self.blockhash_queue.write().unwrap().genesis_hash( &genesis_config.hash(), diff --git a/runtime/src/bank/fee_distribution.rs b/runtime/src/bank/fee_distribution.rs index 71ba1a6583dace..ed24b59edd8b80 100644 --- a/runtime/src/bank/fee_distribution.rs +++ b/runtime/src/bank/fee_distribution.rs @@ -19,7 +19,7 @@ struct DepositFeeOptions { check_rent_paying: bool, } -#[derive(Error, Debug)] +#[derive(Error, Debug, PartialEq)] enum DepositFeeError { #[error("fee account became rent paying")] InvalidRentPayingAccount, @@ -70,14 +70,10 @@ impl Bank { )); } Err(err) => { + // notify validator that their fees were burned error!( - "Burning {} fee instead of crediting {} due to {err}", - deposit, self.collector_id - ); - datapoint_error!( - "bank-burned_fee", - ("slot", self.slot(), i64), - ("num_lamports", deposit, i64) + "Burned {deposit} lamport transaction fee instead of sending to {} due to {err}", + self.collector_id ); burn += deposit; } @@ -233,18 +229,15 @@ impl Bank { }, )); } - Err(_) => { + Err(err) => { // overflow adding lamports or resulting account is invalid self.capitalization.fetch_sub(rent_to_be_paid, Relaxed); - error!( - "Burned {} rent lamports instead of sending to {}", - rent_to_be_paid, pubkey - ); - datapoint_error!( - "bank-burned_rent", - ("slot", self.slot(), i64), - ("num_lamports", rent_to_be_paid, i64) - ); + // notify validator that their fees were burned + if self.collector_id == pubkey { + error!( + "Burned {rent_to_be_paid} lamport rent fee instead of sending to {pubkey} due to {err}", + ); + } } } } @@ -499,20 +492,11 @@ pub mod tests { check_rent_paying: true, }; - // Call the deposit_fees function - let result = bank.deposit_fees(&pubkey, deposit_amount, options); - - // Check the result - match result { - Ok(new_balance) => { - assert_eq!( - new_balance, - initial_balance + deposit_amount, - "New balance should be the sum of the initial balance and deposit amount" - ); - } - Err(e) => panic!("Unexpected error: {:?}", e), - } + assert_eq!( + bank.deposit_fees(&pubkey, deposit_amount, options), + Ok(initial_balance + deposit_amount), + "New balance should be the sum of the initial balance and deposit amount" + ); } #[test] @@ -528,19 +512,11 @@ pub mod tests { check_rent_paying: false, }; - // Call the deposit_fees function - let result = bank.deposit_fees(&pubkey, deposit_amount, options); - - // Check the result - match result { - Ok(_) => panic!("Expected an error due to lamport overflow, but received Ok"), - Err(e) => { - match e { - DepositFeeError::LamportOverflow => {} // This is the expected error - _ => panic!("Unexpected error: {:?}", e), - } - } - } + assert_eq!( + bank.deposit_fees(&pubkey, deposit_amount, options), + Err(DepositFeeError::LamportOverflow), + "Expected an error due to lamport overflow" + ); } #[test] @@ -559,19 +535,11 @@ pub mod tests { check_rent_paying: false, }; - // Call the deposit_fees function - let result = bank.deposit_fees(&pubkey, deposit_amount, options); - - // Check the result - match result { - Ok(_) => panic!("Expected an error due to invalid account owner, but received Ok"), - Err(e) => { - match e { - DepositFeeError::InvalidAccountOwner => {} // This is the expected error - _ => panic!("Unexpected error: {:?}", e), - } - } - } + assert_eq!( + bank.deposit_fees(&pubkey, deposit_amount, options), + Err(DepositFeeError::InvalidAccountOwner), + "Expected an error due to invalid account owner" + ); } // disable check_account_owner @@ -581,20 +549,11 @@ pub mod tests { check_rent_paying: false, }; - // Call the deposit_fees function - let result = bank.deposit_fees(&pubkey, deposit_amount, options); - - // Check the result - match result { - Ok(new_balance) => { - assert_eq!( - new_balance, - initial_balance + deposit_amount, - "New balance should be the sum of the initial balance and deposit amount" - ); - } - Err(e) => panic!("Unexpected error: {:?}", e), - } + assert_eq!( + bank.deposit_fees(&pubkey, deposit_amount, options), + Ok(initial_balance + deposit_amount), + "New balance should be the sum of the initial balance and deposit amount" + ); } } @@ -619,21 +578,11 @@ pub mod tests { check_rent_paying: true, }; - // Call the deposit_fees function - let result = bank.deposit_fees(&pubkey, deposit_amount, options); - - // Check the result - match result { - Ok(_) => { - panic!("Expected an error due to invalid rent paying account, but received Ok") - } - Err(e) => { - match e { - DepositFeeError::InvalidRentPayingAccount => {} // This is the expected error - _ => panic!("Unexpected error: {:?}", e), - } - } - } + assert_eq!( + bank.deposit_fees(&pubkey, deposit_amount, options), + Err(DepositFeeError::InvalidRentPayingAccount), + "Expected an error due to invalid rent paying account" + ); } // disable check_rent_paying @@ -643,20 +592,11 @@ pub mod tests { check_rent_paying: false, }; - // Call the deposit_fees function - let result = bank.deposit_fees(&pubkey, deposit_amount, options); - - // Check the result - match result { - Ok(new_balance) => { - assert_eq!( - new_balance, - initial_balance + deposit_amount, - "New balance should be the sum of the initial balance and deposit amount" - ); - } - Err(e) => panic!("Unexpected error: {:?}", e), - } + assert_eq!( + bank.deposit_fees(&pubkey, deposit_amount, options), + Ok(initial_balance + deposit_amount), + "New balance should be the sum of the initial balance and deposit amount" + ); } } diff --git a/sdk/src/feature_set.rs b/sdk/src/feature_set.rs index aabe184d4408db..888528df56aa43 100644 --- a/sdk/src/feature_set.rs +++ b/sdk/src/feature_set.rs @@ -904,7 +904,7 @@ lazy_static! { (update_hashes_per_tick4::id(), "Update desired hashes per tick to 7.6M"), (update_hashes_per_tick5::id(), "Update desired hashes per tick to 9.2M"), (update_hashes_per_tick6::id(), "Update desired hashes per tick to 10M"), - (validate_fee_collector_account::id(), "validate fee collector account #??"), + (validate_fee_collector_account::id(), "validate fee collector account #33888"), /*************** ADD NEW FEATURES HERE ***************/ ] .iter() From a5c86608d87fa864e04f2e0703dda42358b0e413 Mon Sep 17 00:00:00 2001 From: Justin Starry Date: Wed, 1 Nov 2023 19:05:43 +0100 Subject: [PATCH 5/8] feedback 2 --- runtime/src/bank.rs | 22 ++++++++-------------- runtime/src/bank/fee_distribution.rs | 2 +- 2 files changed, 9 insertions(+), 15 deletions(-) diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 5c559493d1bde3..f70ef57c7d71ad 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -3799,20 +3799,14 @@ impl Bank { self.accounts_data_size_initial += account.data().len() as u64; } - // highest staked node is the first collector - let highest_staked_node = self.stakes_cache.stakes().highest_staked_node(); - - #[cfg(test)] - { - // in tests, use a new unique pubkey as the collector id if no staked - // nodes are defined in the genesis config - self.collector_id = highest_staked_node.unwrap_or_else(Pubkey::new_unique); - } - - #[cfg(not(test))] - { - self.collector_id = highest_staked_node.expect("no staked nodes available"); - } + // Highest staked node is the first collector but if a genesis config + // doesn't define and staked nodes, we assume this genesis config is for + // testing and set the collector id to a unique pubkey. + self.collector_id = self + .stakes_cache + .stakes() + .highest_staked_node() + .unwrap_or_else(Pubkey::new_unique); self.blockhash_queue.write().unwrap().genesis_hash( &genesis_config.hash(), diff --git a/runtime/src/bank/fee_distribution.rs b/runtime/src/bank/fee_distribution.rs index ed24b59edd8b80..63686f4c6b53ba 100644 --- a/runtime/src/bank/fee_distribution.rs +++ b/runtime/src/bank/fee_distribution.rs @@ -134,7 +134,7 @@ impl Bank { // The reason is that rent fee doesn't need to be incentivized for throughput unlike transaction // fees // - // Ref: collect_transaction_fees + // Ref: distribute_transaction_fees #[allow(clippy::needless_collect)] fn distribute_rent_to_validators( &self, From 4d192ce275f359da3d9a1105fc2c81c79905ac34 Mon Sep 17 00:00:00 2001 From: Justin Starry Date: Fri, 3 Nov 2023 11:09:38 +0100 Subject: [PATCH 6/8] add datapoints --- runtime/src/bank/fee_distribution.rs | 39 +++++++++++++++++++--------- 1 file changed, 27 insertions(+), 12 deletions(-) diff --git a/runtime/src/bank/fee_distribution.rs b/runtime/src/bank/fee_distribution.rs index 63686f4c6b53ba..ed43c30ccf5c72 100644 --- a/runtime/src/bank/fee_distribution.rs +++ b/runtime/src/bank/fee_distribution.rs @@ -1,6 +1,6 @@ use { super::Bank, - log::{debug, error, warn}, + log::{debug, warn}, solana_accounts_db::{account_rent_state::RentState, stake_rewards::RewardInfo}, solana_sdk::{ account::{ReadableAccount, WritableAccount}, @@ -70,10 +70,15 @@ impl Bank { )); } Err(err) => { - // notify validator that their fees were burned - error!( - "Burned {deposit} lamport transaction fee instead of sending to {} due to {err}", - self.collector_id + debug!( + "Burned {} lamport tx fee instead of sending to {} due to {}", + deposit, self.collector_id, err + ); + datapoint_info!( + "bank-burned_fee", + ("slot", self.slot(), i64), + ("num_lamports", deposit, i64), + ("error", err.to_string(), String), ); burn += deposit; } @@ -197,6 +202,7 @@ impl Bank { // holder let mut leftover_lamports = rent_to_be_distributed - rent_distributed_in_initial_round; + let mut rent_to_burn: u64 = 0; let mut rewards = vec![]; validator_rent_shares .into_iter() @@ -230,20 +236,29 @@ impl Bank { )); } Err(err) => { + debug!( + "Burned {} lamport rent fee instead of sending to {} due to {}", + rent_to_be_paid, pubkey, err + ); + // overflow adding lamports or resulting account is invalid - self.capitalization.fetch_sub(rent_to_be_paid, Relaxed); - // notify validator that their fees were burned - if self.collector_id == pubkey { - error!( - "Burned {rent_to_be_paid} lamport rent fee instead of sending to {pubkey} due to {err}", - ); - } + // so burn lamports and track lamports burned per slot + rent_to_burn = rent_to_burn.saturating_add(rent_to_be_paid); } } } }); self.rewards.write().unwrap().append(&mut rewards); + if rent_to_burn > 0 { + self.capitalization.fetch_sub(rent_to_burn, Relaxed); + datapoint_info!( + "bank-burned_rent", + ("slot", self.slot(), i64), + ("num_lamports", rent_to_burn, i64) + ); + } + if enforce_fix { assert_eq!(leftover_lamports, 0); } else if leftover_lamports != 0 { From cf93883f452518571264b804d101492bee607d55 Mon Sep 17 00:00:00 2001 From: Justin Starry Date: Fri, 3 Nov 2023 11:12:36 +0100 Subject: [PATCH 7/8] change to datapoint_warn --- runtime/src/bank/fee_distribution.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/runtime/src/bank/fee_distribution.rs b/runtime/src/bank/fee_distribution.rs index ed43c30ccf5c72..e1d251c0bf478c 100644 --- a/runtime/src/bank/fee_distribution.rs +++ b/runtime/src/bank/fee_distribution.rs @@ -74,7 +74,7 @@ impl Bank { "Burned {} lamport tx fee instead of sending to {} due to {}", deposit, self.collector_id, err ); - datapoint_info!( + datapoint_warn!( "bank-burned_fee", ("slot", self.slot(), i64), ("num_lamports", deposit, i64), @@ -252,7 +252,7 @@ impl Bank { if rent_to_burn > 0 { self.capitalization.fetch_sub(rent_to_burn, Relaxed); - datapoint_info!( + datapoint_warn!( "bank-burned_rent", ("slot", self.slot(), i64), ("num_lamports", rent_to_burn, i64) From 9978d3278d09857f04aded521b7cf719c05ea477 Mon Sep 17 00:00:00 2001 From: Justin Starry Date: Sun, 5 Nov 2023 15:13:11 +0800 Subject: [PATCH 8/8] typo --- 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 f70ef57c7d71ad..f8170097d94d21 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -3800,7 +3800,7 @@ impl Bank { } // Highest staked node is the first collector but if a genesis config - // doesn't define and staked nodes, we assume this genesis config is for + // doesn't define any staked nodes, we assume this genesis config is for // testing and set the collector id to a unique pubkey. self.collector_id = self .stakes_cache