Skip to content
This repository has been archived by the owner on Jan 22, 2025. It is now read-only.

Burn fees collected into invalid accounts #33887

Merged
merged 8 commits into from
Nov 6, 2023
Merged

Burn fees collected into invalid accounts #33887

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
69827fa
refactor: create bank::fee_distribution module
jstarry Oct 25, 2023
ea1f7f3
feature: add checks to fee distribution
jstarry Oct 25, 2023
7d204a2
refactor: move Bank::deposit fn into test_utils
jstarry Oct 25, 2023
4cb3bca
feedback
jstarry Oct 30, 2023
a5c8660
feedback 2
jstarry Nov 1, 2023
4d192ce
add datapoints
jstarry Nov 3, 2023
cf93883
change to datapoint_warn
jstarry Nov 3, 2023
9978d32
typo
jstarry Nov 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions runtime/benches/accounts.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ fn deposit_many(bank: &Bank, pubkeys: &mut Vec<Pubkey>, 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(())
Expand Down Expand Up @@ -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;
Expand Down
282 changes: 29 additions & 253 deletions runtime/src/bank.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,6 @@ use {
},
solana_accounts_db::{
account_overrides::AccountOverrides,
account_rent_state::RentState,
accounts::{
AccountAddressFilter, Accounts, LoadedTransaction, PubkeyAccountSlot, RewardInterval,
TransactionLoadResult,
Expand Down Expand Up @@ -149,7 +148,6 @@ use {
incinerator,
inflation::Inflation,
instruction::InstructionError,
lamports::LamportsError,
loader_v4::{self, LoaderV4State, LoaderV4Status},
message::{AccountKeys, SanitizedMessage},
native_loader,
Expand Down Expand Up @@ -185,7 +183,7 @@ use {
borrow::Cow,
cell::RefCell,
collections::{HashMap, HashSet},
convert::{TryFrom, TryInto},
convert::TryFrom,
fmt, mem,
ops::{AddAssign, RangeInclusive},
path::PathBuf,
Expand Down Expand Up @@ -216,6 +214,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;
Expand Down Expand Up @@ -3671,62 +3670,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();
Expand All @@ -3752,8 +3695,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();

Expand Down Expand Up @@ -3861,7 +3804,7 @@ impl Bank {
.stakes_cache
.stakes()
.highest_staked_node()
.unwrap_or_default();
.unwrap_or_else(Pubkey::new_unique);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this method should absolutely not be used outside of tests under any circumstances. can we just fail/panic instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's only used in tests right now... using the system program (default pubkey) as the highest staked node should also not happen outside of tests in any circumstances. I'll clean it up in a follow up change though to be sure

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR fix is being worked on here: #34135


self.blockhash_queue.write().unwrap().genesis_hash(
&genesis_config.hash(),
Expand Down Expand Up @@ -5652,183 +5595,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::<Vec<(Pubkey, u64)>>();

#[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::<Vec<(Pubkey, u64)>>();

// 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],
Expand Down Expand Up @@ -6717,19 +6483,6 @@ impl Bank {
}
}

pub fn deposit(
&self,
pubkey: &Pubkey,
lamports: u64,
) -> std::result::Result<u64, LamportsError> {
// 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<Accounts> {
self.rc.accounts.clone()
}
Expand Down Expand Up @@ -7932,6 +7685,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<RwLockReadGuard<CostTracker>> {
self.cost_tracker.read()
}
Expand Down Expand Up @@ -8472,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,
};
Expand Down Expand Up @@ -8504,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<u64, LamportsError> {
// 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())
}
}
Loading