Skip to content

Feature cleanup - Reward full priority fee to validators #22 #4959

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
tao-stones merged 1 commit into from
Feb 14, 2025
Merged

Feature cleanup - Reward full priority fee to validators #22 #4959

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
29 changes: 3 additions & 26 deletions runtime/src/bank.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ use {
solana_compute_budget::compute_budget::ComputeBudget,
solana_compute_budget_instruction::instructions_processor::process_compute_budget_instructions,
solana_cost_model::{block_cost_limits::simd_0207_block_limits, cost_tracker::CostTracker},
solana_feature_set::{self as feature_set, reward_full_priority_fee, FeatureSet},
solana_feature_set::{self as feature_set, FeatureSet},
solana_fee::FeeFeatures,
solana_lattice_hash::lt_hash::LtHash,
solana_measure::{meas_dur, measure::Measure, measure_time, measure_us},
Expand Down Expand Up @@ -2593,11 +2593,7 @@ impl Bank {
if *hash == Hash::default() {
// finish up any deferred changes to account state
self.collect_rent_eagerly();
if self.feature_set.is_active(&reward_full_priority_fee::id()) {
self.distribute_transaction_fee_details();
} else {
self.distribute_transaction_fees();
}
self.distribute_transaction_fee_details();
self.distribute_rent_fees();
self.update_slot_history();
self.run_incinerator();
Expand Down Expand Up @@ -3615,21 +3611,6 @@ impl Bank {
self.update_accounts_data_size_delta_off_chain(data_size_delta);
}

fn filter_program_errors_and_collect_fee(
&self,
processing_results: &[TransactionProcessingResult],
) {
let mut fees = 0;

processing_results.iter().for_each(|processing_result| {
if let Ok(processed_tx) = processing_result {
fees += processed_tx.fee_details().total_fee();
}
});

self.collector_fees.fetch_add(fees, Relaxed);
}

// Note: this function is not yet used; next PR will call it behind a feature gate
Copy link
Copy Markdown

@apfitzge apfitzge Feb 13, 2025

Choose a reason for hiding this comment

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

this comment could be removed

fn filter_program_errors_and_collect_fee_details(
&self,
Expand Down Expand Up @@ -3765,11 +3746,7 @@ impl Bank {
let ((), update_transaction_statuses_us) =
measure_us!(self.update_transaction_statuses(sanitized_txs, &processing_results));

if self.feature_set.is_active(&reward_full_priority_fee::id()) {
self.filter_program_errors_and_collect_fee_details(&processing_results)
} else {
self.filter_program_errors_and_collect_fee(&processing_results)
};
self.filter_program_errors_and_collect_fee_details(&processing_results);

timings.saturating_add_in_place(ExecuteTimingType::StoreUs, store_accounts_us);
timings.saturating_add_in_place(
Expand Down
141 changes: 3 additions & 138 deletions runtime/src/bank/fee_distribution.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ use {
super::Bank,
crate::bank::CollectorFeeDetails,
log::{debug, warn},
solana_feature_set::reward_full_priority_fee,
solana_fee::FeeFeatures,
solana_runtime_transaction::transaction_with_meta::TransactionWithMeta,
solana_sdk::{
Expand Down Expand Up @@ -44,19 +43,6 @@ impl Bank {
// 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.calculate_reward_and_burn_fees(collector_fees);
if deposit > 0 {
self.deposit_or_burn_fee(deposit, &mut burn);
}
self.capitalization.fetch_sub(burn, Relaxed);
}
}

// Replace `distribute_transaction_fees()` after Feature Gate: Reward full priority fee to
// validators #34731;
pub(super) fn distribute_transaction_fee_details(&self) {
let fee_details = self.collector_fee_details.read().unwrap();
if fee_details.total() == 0 {
Expand Down Expand Up @@ -84,19 +70,11 @@ impl Bank {
fee_budget_limits.prioritization_fee,
FeeFeatures::from(self.feature_set.as_ref()),
);
let (reward, _burn) = if self.feature_set.is_active(&reward_full_priority_fee::id()) {
self.calculate_reward_and_burn_fee_details(&CollectorFeeDetails::from(fee_details))
} else {
let fee = fee_details.total_fee();
self.calculate_reward_and_burn_fees(fee)
};
let (reward, _burn) =
self.calculate_reward_and_burn_fee_details(&CollectorFeeDetails::from(fee_details));
reward
}

fn calculate_reward_and_burn_fees(&self, fee: u64) -> (u64, u64) {
self.fee_rate_governor.burn(fee)
}

fn calculate_reward_and_burn_fee_details(
&self,
fee_details: &CollectorFeeDetails,
Expand Down Expand Up @@ -185,7 +163,7 @@ impl Bank {
// The reason is that rent fee doesn't need to be incentivized for throughput unlike transaction
// fees
//
// Ref: distribute_transaction_fees
// Ref: distribute_transaction_fee_details
fn distribute_rent_to_validators(
&self,
vote_accounts: &VoteAccountsHashMap,
Expand Down Expand Up @@ -430,119 +408,6 @@ pub mod tests {
}
}

#[test]
Copy link
Copy Markdown
Author

@tao-stones tao-stones Feb 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

For removed tests, there are corresponding tests for test_distribute_transaction_fee_details_* version

fn test_distribute_transaction_fees_normal() {
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));
let (expected_collected_fees, burn_amount) = bank.fee_rate_governor.burn(transaction_fees);

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 + 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;
Expand Down
78 changes: 0 additions & 78 deletions runtime/src/bank/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3034,84 +3034,6 @@ fn test_bank_blockhash_compute_unit_fee_structure() {
);
}

#[test]
Copy link
Copy Markdown
Author

@tao-stones tao-stones Feb 12, 2025

Choose a reason for hiding this comment

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

test_filter_program_errors_and_collect_fee_details covers these two removed tests.

fn test_filter_program_errors_and_collect_fee() {
let leader = solana_pubkey::new_rand();
let GenesisConfigInfo { genesis_config, .. } =
create_genesis_config_with_leader(100_000, &leader, 3);
let mut bank = Bank::new_for_tests(&genesis_config);
// this test is only for when `feature_set::reward_full_priority_fee` inactivated
bank.deactivate_feature(&feature_set::reward_full_priority_fee::id());

let tx_fee = 42;
let fee_details = FeeDetails::new(tx_fee, 0);
let processing_results = vec![
Err(TransactionError::AccountNotFound),
new_executed_processing_result(Ok(()), fee_details),
new_executed_processing_result(
Err(TransactionError::InstructionError(
1,
SystemError::ResultWithNegativeLamports.into(),
)),
fee_details,
),
Ok(ProcessedTransaction::FeesOnly(Box::new(
FeesOnlyTransaction {
load_error: TransactionError::InvalidProgramForExecution,
rollback_accounts: RollbackAccounts::default(),
fee_details,
},
))),
];
let initial_balance = bank.get_balance(&leader);

bank.filter_program_errors_and_collect_fee(&processing_results);
bank.freeze();
assert_eq!(
bank.get_balance(&leader),
initial_balance + bank.fee_rate_governor.burn(tx_fee * 3).0
);
}

#[test]
fn test_filter_program_errors_and_collect_priority_fee() {
let leader = solana_pubkey::new_rand();
let GenesisConfigInfo { genesis_config, .. } =
create_genesis_config_with_leader(1000000, &leader, 3);
let mut bank = Bank::new_for_tests(&genesis_config);
// this test is only for when `feature_set::reward_full_priority_fee` inactivated
bank.deactivate_feature(&feature_set::reward_full_priority_fee::id());

let priority_fee = 42;
let fee_details: FeeDetails = FeeDetails::new(0, priority_fee);
let processing_results = vec![
Err(TransactionError::AccountNotFound),
new_executed_processing_result(Ok(()), fee_details),
new_executed_processing_result(
Err(TransactionError::InstructionError(
1,
SystemError::ResultWithNegativeLamports.into(),
)),
fee_details,
),
Ok(ProcessedTransaction::FeesOnly(Box::new(
FeesOnlyTransaction {
load_error: TransactionError::InvalidProgramForExecution,
rollback_accounts: RollbackAccounts::default(),
fee_details,
},
))),
];
let initial_balance = bank.get_balance(&leader);

bank.filter_program_errors_and_collect_fee(&processing_results);
bank.freeze();
assert_eq!(
bank.get_balance(&leader),
initial_balance + bank.fee_rate_governor.burn(priority_fee * 3).0
);
}

#[test]
fn test_debits_before_credits() {
let (genesis_config, mint_keypair) =
Expand Down