From caf181045978b4c0e7aaf22b8b33bf8b9ed6dd26 Mon Sep 17 00:00:00 2001 From: Andrew Fitzgerald Date: Fri, 9 Feb 2024 08:51:21 -0800 Subject: [PATCH 1/2] v1.18: Scheduler - prioritization fees/cost (#34888) --- .../prio_graph_scheduler.rs | 21 +-- .../scheduler_controller.rs | 83 +++++++++--- .../transaction_state.rs | 125 ++++-------------- .../transaction_state_container.rs | 44 ++---- 4 files changed, 110 insertions(+), 163 deletions(-) diff --git a/core/src/banking_stage/transaction_scheduler/prio_graph_scheduler.rs b/core/src/banking_stage/transaction_scheduler/prio_graph_scheduler.rs index e17f34d3223411..700bbc21a4b440 100644 --- a/core/src/banking_stage/transaction_scheduler/prio_graph_scheduler.rs +++ b/core/src/banking_stage/transaction_scheduler/prio_graph_scheduler.rs @@ -191,7 +191,7 @@ impl PrioGraphScheduler { saturating_add_assign!(num_scheduled, 1); let sanitized_transaction_ttl = transaction_state.transition_to_pending(); - let cost = transaction_state.transaction_cost().sum(); + let cost = transaction_state.cost(); let SanitizedTransactionTTL { transaction, @@ -490,12 +490,9 @@ mod tests { crate::banking_stage::consumer::TARGET_NUM_TRANSACTIONS_PER_BATCH, crossbeam_channel::{unbounded, Receiver}, itertools::Itertools, - solana_cost_model::cost_model::CostModel, - solana_runtime::transaction_priority_details::TransactionPriorityDetails, solana_sdk::{ - compute_budget::ComputeBudgetInstruction, feature_set::FeatureSet, hash::Hash, - message::Message, pubkey::Pubkey, signature::Keypair, signer::Signer, - system_instruction, transaction::Transaction, + compute_budget::ComputeBudgetInstruction, hash::Hash, message::Message, pubkey::Pubkey, + signature::Keypair, signer::Signer, system_instruction, transaction::Transaction, }, std::borrow::Borrow, }; @@ -568,20 +565,12 @@ mod tests { let id = TransactionId::new(index as u64); let transaction = prioritized_tranfers(from_keypair.borrow(), to_pubkeys, lamports, priority); - let transaction_cost = CostModel::calculate_cost(&transaction, &FeatureSet::default()); let transaction_ttl = SanitizedTransactionTTL { transaction, max_age_slot: Slot::MAX, }; - container.insert_new_transaction( - id, - transaction_ttl, - TransactionPriorityDetails { - priority, - compute_unit_limit: 1, - }, - transaction_cost, - ); + const TEST_TRANSACTION_COST: u64 = 5000; + container.insert_new_transaction(id, transaction_ttl, priority, TEST_TRANSACTION_COST); } container diff --git a/core/src/banking_stage/transaction_scheduler/scheduler_controller.rs b/core/src/banking_stage/transaction_scheduler/scheduler_controller.rs index 225ff6a53e18c5..b8e5afd453be19 100644 --- a/core/src/banking_stage/transaction_scheduler/scheduler_controller.rs +++ b/core/src/banking_stage/transaction_scheduler/scheduler_controller.rs @@ -20,10 +20,12 @@ use { solana_accounts_db::transaction_error_metrics::TransactionErrorMetrics, solana_cost_model::cost_model::CostModel, solana_measure::measure_us, + solana_program_runtime::compute_budget_processor::process_compute_budget_instructions, solana_runtime::{bank::Bank, bank_forks::BankForks}, solana_sdk::{ - clock::MAX_PROCESSING_AGE, saturating_add_assign, timing::AtomicInterval, - transaction::SanitizedTransaction, + clock::MAX_PROCESSING_AGE, + feature_set::include_loaded_accounts_data_size_in_fee_calculation, fee::FeeBudgetLimits, + saturating_add_assign, timing::AtomicInterval, transaction::SanitizedTransaction, }, std::{ sync::{Arc, RwLock}, @@ -309,21 +311,24 @@ impl SchedulerController { let mut error_counts = TransactionErrorMetrics::default(); for chunk in packets.chunks(CHUNK_SIZE) { let mut post_sanitization_count: usize = 0; - let (transactions, priority_details): (Vec<_>, Vec<_>) = chunk + let (transactions, fee_budget_limits_vec): (Vec<_>, Vec<_>) = chunk .iter() .filter_map(|packet| { - packet - .build_sanitized_transaction(feature_set, vote_only, bank.as_ref()) - .map(|tx| (tx, packet.priority_details())) + packet.build_sanitized_transaction(feature_set, vote_only, bank.as_ref()) }) .inspect(|_| saturating_add_assign!(post_sanitization_count, 1)) - .filter(|(tx, _)| { + .filter(|tx| { SanitizedTransaction::validate_account_locks( tx.message(), transaction_account_lock_limit, ) .is_ok() }) + .filter_map(|tx| { + process_compute_budget_instructions(tx.message().program_instructions_iter()) + .map(|compute_budget| (tx, compute_budget.into())) + .ok() + }) .unzip(); let check_results = bank.check_transactions( @@ -335,16 +340,17 @@ impl SchedulerController { let post_lock_validation_count = transactions.len(); let mut post_transaction_check_count: usize = 0; - for ((transaction, priority_details), _) in transactions + for ((transaction, fee_budget_limits), _) in transactions .into_iter() - .zip(priority_details) + .zip(fee_budget_limits_vec) .zip(check_results) .filter(|(_, check_result)| check_result.0.is_ok()) { saturating_add_assign!(post_transaction_check_count, 1); let transaction_id = self.transaction_id_generator.next(); - let transaction_cost = CostModel::calculate_cost(&transaction, &bank.feature_set); + let (priority, cost) = + Self::calculate_priority_and_cost(&transaction, &fee_budget_limits, &bank); let transaction_ttl = SanitizedTransactionTTL { transaction, max_age_slot: last_slot_in_epoch, @@ -353,8 +359,8 @@ impl SchedulerController { if self.container.insert_new_transaction( transaction_id, transaction_ttl, - priority_details, - transaction_cost, + priority, + cost, ) { saturating_add_assign!(self.count_metrics.num_dropped_on_capacity, 1); } @@ -382,6 +388,51 @@ impl SchedulerController { ); } } + + /// Calculate priority and cost for a transaction: + /// + /// Cost is calculated through the `CostModel`, + /// and priority is calculated through a formula here that attempts to sell + /// blockspace to the highest bidder. + /// + /// The priority is calculated as: + /// P = R / (1 + C) + /// where P is the priority, R is the reward, + /// and C is the cost towards block-limits. + /// + /// Current minimum costs are on the order of several hundred, + /// so the denominator is effectively C, and the +1 is simply + /// to avoid any division by zero due to a bug - these costs + /// are calculated by the cost-model and are not direct + /// from user input. They should never be zero. + /// Any difference in the prioritization is negligible for + /// the current transaction costs. + fn calculate_priority_and_cost( + transaction: &SanitizedTransaction, + fee_budget_limits: &FeeBudgetLimits, + bank: &Bank, + ) -> (u64, u64) { + let cost = CostModel::calculate_cost(transaction, &bank.feature_set).sum(); + let fee = bank.fee_structure.calculate_fee( + transaction.message(), + 5_000, // this just needs to be non-zero + fee_budget_limits, + bank.feature_set + .is_active(&include_loaded_accounts_data_size_in_fee_calculation::id()), + ); + + // We need a multiplier here to avoid rounding down too aggressively. + // For many transactions, the cost will be greater than the fees in terms of raw lamports. + // For the purposes of calculating prioritization, we multiply the fees by a large number so that + // the cost is a small fraction. + // An offset of 1 is used in the denominator to explicitly avoid division by zero. + const MULTIPLIER: u64 = 1_000_000; + ( + fee.saturating_mul(MULTIPLIER) + .saturating_div(cost.saturating_add(1)), + cost, + ) + } } #[derive(Default)] @@ -468,7 +519,7 @@ impl SchedulerCountMetrics { self.num_dropped_on_age_and_status, i64 ), - ("num_dropped_on_capacity", self.num_dropped_on_capacity, i64) + ("num_dropped_on_capacity", self.num_dropped_on_capacity, i64), ); } @@ -680,7 +731,7 @@ mod tests { from_keypair: &Keypair, to_pubkey: &Pubkey, lamports: u64, - priority: u64, + compute_unit_price: u64, recent_blockhash: Hash, ) -> Transaction { // Fund the sending key, so that the transaction does not get filtered by the fee-payer check. @@ -695,7 +746,7 @@ mod tests { } let transfer = system_instruction::transfer(&from_keypair.pubkey(), to_pubkey, lamports); - let prioritization = ComputeBudgetInstruction::set_compute_unit_price(priority); + let prioritization = ComputeBudgetInstruction::set_compute_unit_price(compute_unit_price); let message = Message::new(&[transfer, prioritization], Some(&from_keypair.pubkey())); Transaction::new(&vec![from_keypair], message, recent_blockhash) } @@ -951,7 +1002,7 @@ mod tests { &Keypair::new(), &Pubkey::new_unique(), 1, - i, + i * 10, bank.last_blockhash(), ) }) diff --git a/core/src/banking_stage/transaction_scheduler/transaction_state.rs b/core/src/banking_stage/transaction_scheduler/transaction_state.rs index 650ffa1cd3ce7e..c53c60b8aee16b 100644 --- a/core/src/banking_stage/transaction_scheduler/transaction_state.rs +++ b/core/src/banking_stage/transaction_scheduler/transaction_state.rs @@ -1,8 +1,4 @@ -use { - solana_cost_model::transaction_cost::TransactionCost, - solana_runtime::transaction_priority_details::TransactionPriorityDetails, - solana_sdk::{slot_history::Slot, transaction::SanitizedTransaction}, -}; +use solana_sdk::{clock::Slot, transaction::SanitizedTransaction}; /// Simple wrapper type to tie a sanitized transaction to max age slot. pub(crate) struct SanitizedTransactionTTL { @@ -34,77 +30,38 @@ pub(crate) enum TransactionState { /// The transaction is available for scheduling. Unprocessed { transaction_ttl: SanitizedTransactionTTL, - transaction_priority_details: TransactionPriorityDetails, - transaction_cost: TransactionCost, - forwarded: bool, + priority: u64, + cost: u64, }, /// The transaction is currently scheduled or being processed. - Pending { - transaction_priority_details: TransactionPriorityDetails, - transaction_cost: TransactionCost, - forwarded: bool, - }, + Pending { priority: u64, cost: u64 }, } impl TransactionState { /// Creates a new `TransactionState` in the `Unprocessed` state. - pub(crate) fn new( - transaction_ttl: SanitizedTransactionTTL, - transaction_priority_details: TransactionPriorityDetails, - transaction_cost: TransactionCost, - ) -> Self { + pub(crate) fn new(transaction_ttl: SanitizedTransactionTTL, priority: u64, cost: u64) -> Self { Self::Unprocessed { transaction_ttl, - transaction_priority_details, - transaction_cost, - forwarded: false, - } - } - - /// Returns a reference to the priority details of the transaction. - pub(crate) fn transaction_priority_details(&self) -> &TransactionPriorityDetails { - match self { - Self::Unprocessed { - transaction_priority_details, - .. - } => transaction_priority_details, - Self::Pending { - transaction_priority_details, - .. - } => transaction_priority_details, + priority, + cost, } } - /// Returns a reference to the transaction cost of the transaction. - pub(crate) fn transaction_cost(&self) -> &TransactionCost { - match self { - Self::Unprocessed { - transaction_cost, .. - } => transaction_cost, - Self::Pending { - transaction_cost, .. - } => transaction_cost, - } - } - - /// Returns the priority of the transaction. + /// Return the priority of the transaction. + /// This is *not* the same as the `compute_unit_price` of the transaction. + /// The priority is used to order transactions for processing. pub(crate) fn priority(&self) -> u64 { - self.transaction_priority_details().priority - } - - /// Returns whether or not the transaction has already been forwarded. - pub(crate) fn forwarded(&self) -> bool { match self { - Self::Unprocessed { forwarded, .. } => *forwarded, - Self::Pending { forwarded, .. } => *forwarded, + Self::Unprocessed { priority, .. } => *priority, + Self::Pending { priority, .. } => *priority, } } - /// Sets the transaction as forwarded. - pub(crate) fn set_forwarded(&mut self) { + /// Return the cost of the transaction. + pub(crate) fn cost(&self) -> u64 { match self { - Self::Unprocessed { forwarded, .. } => *forwarded = true, - Self::Pending { forwarded, .. } => *forwarded = true, + Self::Unprocessed { cost, .. } => *cost, + Self::Pending { cost, .. } => *cost, } } @@ -119,15 +76,10 @@ impl TransactionState { match self.take() { TransactionState::Unprocessed { transaction_ttl, - transaction_priority_details, - transaction_cost, - forwarded, + priority, + cost, } => { - *self = TransactionState::Pending { - transaction_priority_details, - transaction_cost, - forwarded, - }; + *self = TransactionState::Pending { priority, cost }; transaction_ttl } TransactionState::Pending { .. } => { @@ -145,16 +97,11 @@ impl TransactionState { pub(crate) fn transition_to_unprocessed(&mut self, transaction_ttl: SanitizedTransactionTTL) { match self.take() { TransactionState::Unprocessed { .. } => panic!("already unprocessed"), - TransactionState::Pending { - transaction_priority_details, - transaction_cost, - forwarded, - } => { + TransactionState::Pending { priority, cost } => { *self = Self::Unprocessed { transaction_ttl, - transaction_priority_details, - transaction_cost, - forwarded, + priority, + cost, } } } @@ -179,14 +126,8 @@ impl TransactionState { core::mem::replace( self, Self::Pending { - transaction_priority_details: TransactionPriorityDetails { - priority: 0, - compute_unit_limit: 0, - }, - transaction_cost: TransactionCost::SimpleVote { - writable_accounts: vec![], - }, - forwarded: false, + priority: 0, + cost: 0, }, ) } @@ -196,7 +137,6 @@ impl TransactionState { mod tests { use { super::*, - solana_cost_model::transaction_cost::UsageCostDetails, solana_sdk::{ compute_budget::ComputeBudgetInstruction, hash::Hash, message::Message, signature::Keypair, signer::Signer, system_instruction, transaction::Transaction, @@ -215,24 +155,13 @@ mod tests { ]; let message = Message::new(&ixs, Some(&from_keypair.pubkey())); let tx = Transaction::new(&[&from_keypair], message, Hash::default()); - let transaction_cost = TransactionCost::Transaction(UsageCostDetails { - signature_cost: 5000, - ..UsageCostDetails::default() - }); let transaction_ttl = SanitizedTransactionTTL { transaction: SanitizedTransaction::from_transaction_for_tests(tx), max_age_slot: Slot::MAX, }; - - TransactionState::new( - transaction_ttl, - TransactionPriorityDetails { - priority, - compute_unit_limit: 0, - }, - transaction_cost, - ) + const TEST_TRANSACTION_COST: u64 = 5000; + TransactionState::new(transaction_ttl, priority, TEST_TRANSACTION_COST) } #[test] @@ -294,7 +223,7 @@ mod tests { } #[test] - fn test_transaction_priority_details() { + fn test_priority() { let priority = 15; let mut transaction_state = create_transaction_state(priority); assert_eq!(transaction_state.priority(), priority); diff --git a/core/src/banking_stage/transaction_scheduler/transaction_state_container.rs b/core/src/banking_stage/transaction_scheduler/transaction_state_container.rs index d7d79cb21b7c32..caca701455beba 100644 --- a/core/src/banking_stage/transaction_scheduler/transaction_state_container.rs +++ b/core/src/banking_stage/transaction_scheduler/transaction_state_container.rs @@ -5,8 +5,6 @@ use { }, crate::banking_stage::scheduler_messages::TransactionId, min_max_heap::MinMaxHeap, - solana_cost_model::transaction_cost::TransactionCost, - solana_runtime::transaction_priority_details::TransactionPriorityDetails, std::collections::HashMap, }; @@ -98,18 +96,13 @@ impl TransactionStateContainer { &mut self, transaction_id: TransactionId, transaction_ttl: SanitizedTransactionTTL, - transaction_priority_details: TransactionPriorityDetails, - transaction_cost: TransactionCost, + priority: u64, + cost: u64, ) -> bool { - let priority_id = - TransactionPriorityId::new(transaction_priority_details.priority, transaction_id); + let priority_id = TransactionPriorityId::new(priority, transaction_id); self.id_to_transaction_state.insert( transaction_id, - TransactionState::new( - transaction_ttl, - transaction_priority_details, - transaction_cost, - ), + TransactionState::new(transaction_ttl, priority, cost), ); self.push_id_into_queue(priority_id) } @@ -155,10 +148,8 @@ impl TransactionStateContainer { mod tests { use { super::*, - solana_cost_model::cost_model::CostModel, solana_sdk::{ compute_budget::ComputeBudgetInstruction, - feature_set::FeatureSet, hash::Hash, message::Message, signature::Keypair, @@ -169,13 +160,8 @@ mod tests { }, }; - fn test_transaction( - priority: u64, - ) -> ( - SanitizedTransactionTTL, - TransactionPriorityDetails, - TransactionCost, - ) { + /// Returns (transaction_ttl, priority, cost) + fn test_transaction(priority: u64) -> (SanitizedTransactionTTL, u64, u64) { let from_keypair = Keypair::new(); let ixs = vec![ system_instruction::transfer( @@ -191,31 +177,23 @@ mod tests { message, Hash::default(), )); - let transaction_cost = CostModel::calculate_cost(&tx, &FeatureSet::default()); let transaction_ttl = SanitizedTransactionTTL { transaction: tx, max_age_slot: Slot::MAX, }; - ( - transaction_ttl, - TransactionPriorityDetails { - priority, - compute_unit_limit: 0, - }, - transaction_cost, - ) + const TEST_TRANSACTION_COST: u64 = 5000; + (transaction_ttl, priority, TEST_TRANSACTION_COST) } fn push_to_container(container: &mut TransactionStateContainer, num: usize) { for id in 0..num as u64 { let priority = id; - let (transaction_ttl, transaction_priority_details, transaction_cost) = - test_transaction(priority); + let (transaction_ttl, priority, cost) = test_transaction(priority); container.insert_new_transaction( TransactionId::new(id), transaction_ttl, - transaction_priority_details, - transaction_cost, + priority, + cost, ); } } From 03dbd6080f5a4721b017d04e11df2ee4d8299a87 Mon Sep 17 00:00:00 2001 From: Andrew Fitzgerald Date: Fri, 15 Mar 2024 12:11:56 -0700 Subject: [PATCH 2/2] add commented out functions - simplify diff --- .../transaction_state.rs | 49 +++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/core/src/banking_stage/transaction_scheduler/transaction_state.rs b/core/src/banking_stage/transaction_scheduler/transaction_state.rs index c53c60b8aee16b..6cfd483db2b897 100644 --- a/core/src/banking_stage/transaction_scheduler/transaction_state.rs +++ b/core/src/banking_stage/transaction_scheduler/transaction_state.rs @@ -47,6 +47,55 @@ impl TransactionState { } } + /* + /// Returns a reference to the priority details of the transaction. + pub(crate) fn transaction_priority_details(&self) -> &TransactionPriorityDetails { + match self { + Self::Unprocessed { + transaction_priority_details, + .. + } => transaction_priority_details, + Self::Pending { + transaction_priority_details, + .. + } => transaction_priority_details, + } + } + + /// Returns a reference to the transaction cost of the transaction. + pub(crate) fn transaction_cost(&self) -> &TransactionCost { + match self { + Self::Unprocessed { + transaction_cost, .. + } => transaction_cost, + Self::Pending { + transaction_cost, .. + } => transaction_cost, + } + } + + /// Returns the priority of the transaction. + pub(crate) fn priority(&self) -> u64 { + self.transaction_priority_details().priority + } + + /// Returns whether or not the transaction has already been forwarded. + pub(crate) fn forwarded(&self) -> bool { + match self { + Self::Unprocessed { forwarded, .. } => *forwarded, + Self::Pending { forwarded, .. } => *forwarded, + } + } + + /// Sets the transaction as forwarded. + pub(crate) fn set_forwarded(&mut self) { + match self { + Self::Unprocessed { forwarded, .. } => *forwarded = true, + Self::Pending { forwarded, .. } => *forwarded = true, + } + } + */ + /// Return the priority of the transaction. /// This is *not* the same as the `compute_unit_price` of the transaction. /// The priority is used to order transactions for processing.