From ced8f6a512c61e0dd5308095ae8457add4a39e94 Mon Sep 17 00:00:00 2001 From: Tao Zhu <82401714+taozhu-chicago@users.noreply.github.com> Date: Fri, 9 Sep 2022 17:24:21 -0500 Subject: [PATCH] Add feature gate to remove support for RequestUnitsDeprecated instruction (#27503) * feature gate: remove support for RequestUnitsDeprecated instruction #27500 * review update: stop support deprecated ix for prioritization * Apply suggestions from code review Co-authored-by: Justin Starry Co-authored-by: Justin Starry --- program-runtime/src/compute_budget.rs | 27 ++++++- program-runtime/src/prioritization_fee.rs | 2 + programs/bpf/tests/programs.rs | 2 + runtime/src/accounts.rs | 7 +- runtime/src/bank.rs | 86 ++++++++++++++------- runtime/src/transaction_priority_details.rs | 3 +- sdk/src/compute_budget.rs | 1 + sdk/src/feature_set.rs | 5 ++ 8 files changed, 99 insertions(+), 34 deletions(-) diff --git a/program-runtime/src/compute_budget.rs b/program-runtime/src/compute_budget.rs index 804d47f1f..e2b9ed38c 100644 --- a/program-runtime/src/compute_budget.rs +++ b/program-runtime/src/compute_budget.rs @@ -130,6 +130,7 @@ impl ComputeBudget { &mut self, instructions: impl Iterator, default_units_per_instruction: bool, + support_request_units_deprecated: bool, ) -> Result { let mut num_non_compute_budget_instructions: usize = 0; let mut updated_compute_unit_limit = None; @@ -148,7 +149,7 @@ impl ComputeBudget { Ok(ComputeBudgetInstruction::RequestUnitsDeprecated { units: compute_unit_limit, additional_fee, - }) => { + }) if support_request_units_deprecated => { if updated_compute_unit_limit.is_some() { return Err(duplicate_instruction_error); } @@ -243,8 +244,11 @@ mod tests { Hash::default(), )); let mut compute_budget = ComputeBudget::default(); - let result = - compute_budget.process_instructions(tx.message().program_instructions_iter(), true); + let result = compute_budget.process_instructions( + tx.message().program_instructions_iter(), + true, + false, /*not support request_units_deprecated*/ + ); assert_eq!($expected_result, result); assert_eq!(compute_budget, $expected_budget); }; @@ -491,5 +495,22 @@ mod tests { Err(TransactionError::DuplicateInstruction(2)), ComputeBudget::default() ); + + // deprecated + test!( + &[Instruction::new_with_borsh( + compute_budget::id(), + &compute_budget::ComputeBudgetInstruction::RequestUnitsDeprecated { + units: 1_000, + additional_fee: 10 + }, + vec![] + )], + Err(TransactionError::InstructionError( + 0, + InstructionError::InvalidInstructionData, + )), + ComputeBudget::default() + ); } } diff --git a/program-runtime/src/prioritization_fee.rs b/program-runtime/src/prioritization_fee.rs index 97b14f93b..14fe3cce6 100644 --- a/program-runtime/src/prioritization_fee.rs +++ b/program-runtime/src/prioritization_fee.rs @@ -5,6 +5,7 @@ type MicroLamports = u128; pub enum PrioritizationFeeType { ComputeUnitPrice(u64), + // TODO: remove 'Deprecated' after feature remove_deprecated_request_unit_ix::id() is activated Deprecated(u64), } @@ -17,6 +18,7 @@ pub struct PrioritizationFeeDetails { impl PrioritizationFeeDetails { pub fn new(fee_type: PrioritizationFeeType, compute_unit_limit: u64) -> Self { match fee_type { + // TODO: remove support of 'Deprecated' after feature remove_deprecated_request_unit_ix::id() is activated PrioritizationFeeType::Deprecated(fee) => { let priority = if compute_unit_limit == 0 { 0 diff --git a/programs/bpf/tests/programs.rs b/programs/bpf/tests/programs.rs index 8016050e8..2147af06e 100644 --- a/programs/bpf/tests/programs.rs +++ b/programs/bpf/tests/programs.rs @@ -3798,6 +3798,7 @@ fn test_program_fees() { congestion_multiplier, &fee_structure, true, + false, ); bank_client .send_and_confirm_message(&[&mint_keypair], message) @@ -3819,6 +3820,7 @@ fn test_program_fees() { congestion_multiplier, &fee_structure, true, + false, ); assert!(expected_normal_fee < expected_prioritized_fee); diff --git a/runtime/src/accounts.rs b/runtime/src/accounts.rs index 8a0a5bcd1..b83599fa4 100644 --- a/runtime/src/accounts.rs +++ b/runtime/src/accounts.rs @@ -34,7 +34,10 @@ use { account_utils::StateMut, bpf_loader_upgradeable::{self, UpgradeableLoaderState}, clock::{BankId, Slot, INITIAL_RENT_EPOCH}, - feature_set::{self, use_default_units_in_fee_calculation, FeatureSet}, + feature_set::{ + self, remove_deprecated_request_unit_ix, use_default_units_in_fee_calculation, + FeatureSet, + }, fee::FeeStructure, genesis_config::ClusterType, hash::Hash, @@ -553,6 +556,7 @@ impl Accounts { lamports_per_signature, fee_structure, feature_set.is_active(&use_default_units_in_fee_calculation::id()), + !feature_set.is_active(&remove_deprecated_request_unit_ix::id()), ) } else { return (Err(TransactionError::BlockhashNotFound), None); @@ -1674,6 +1678,7 @@ mod tests { lamports_per_signature, &FeeStructure::default(), true, + false, ); assert_eq!(fee, lamports_per_signature); diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index b5811a163..5cdf3a505 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -112,7 +112,7 @@ use { feature, feature_set::{ self, disable_fee_calculator, enable_early_verification_of_account_modifications, - use_default_units_in_fee_calculation, FeatureSet, + remove_deprecated_request_unit_ix, use_default_units_in_fee_calculation, FeatureSet, }, fee::FeeStructure, fee_calculator::{FeeCalculator, FeeRateGovernor}, @@ -3728,6 +3728,9 @@ impl Bank { &self.fee_structure, self.feature_set .is_active(&use_default_units_in_fee_calculation::id()), + !self + .feature_set + .is_active(&remove_deprecated_request_unit_ix::id()), )) } @@ -3769,6 +3772,9 @@ impl Bank { &self.fee_structure, self.feature_set .is_active(&use_default_units_in_fee_calculation::id()), + !self + .feature_set + .is_active(&remove_deprecated_request_unit_ix::id()), ) } @@ -4570,30 +4576,34 @@ impl Bank { .map(|(accs, tx)| match accs { (Err(e), _nonce) => TransactionExecutionResult::NotExecuted(e.clone()), (Ok(loaded_transaction), nonce) => { - let compute_budget = if let Some(compute_budget) = - self.runtime_config.compute_budget - { - compute_budget - } else { - let mut compute_budget = - ComputeBudget::new(compute_budget::MAX_COMPUTE_UNIT_LIMIT as u64); + let compute_budget = + if let Some(compute_budget) = self.runtime_config.compute_budget { + compute_budget + } else { + let mut compute_budget = + ComputeBudget::new(compute_budget::MAX_COMPUTE_UNIT_LIMIT as u64); - let mut compute_budget_process_transaction_time = - Measure::start("compute_budget_process_transaction_time"); - let process_transaction_result = compute_budget - .process_instructions(tx.message().program_instructions_iter(), true); - compute_budget_process_transaction_time.stop(); - saturating_add_assign!( - timings - .execute_accessories - .compute_budget_process_transaction_us, - compute_budget_process_transaction_time.as_us() - ); - if let Err(err) = process_transaction_result { - return TransactionExecutionResult::NotExecuted(err); - } - compute_budget - }; + let mut compute_budget_process_transaction_time = + Measure::start("compute_budget_process_transaction_time"); + let process_transaction_result = compute_budget.process_instructions( + tx.message().program_instructions_iter(), + true, + !self + .feature_set + .is_active(&remove_deprecated_request_unit_ix::id()), + ); + compute_budget_process_transaction_time.stop(); + saturating_add_assign!( + timings + .execute_accessories + .compute_budget_process_transaction_us, + compute_budget_process_transaction_time.as_us() + ); + if let Err(err) = process_transaction_result { + return TransactionExecutionResult::NotExecuted(err); + } + compute_budget + }; self.execute_loaded_transaction( tx, @@ -4866,6 +4876,7 @@ impl Bank { lamports_per_signature: u64, fee_structure: &FeeStructure, use_default_units_per_instruction: bool, + support_request_units_deprecated: bool, ) -> u64 { // Fee based on compute units and signatures const BASE_CONGESTION: f64 = 5_000.0; @@ -4881,6 +4892,7 @@ impl Bank { .process_instructions( message.program_instructions_iter(), use_default_units_per_instruction, + support_request_units_deprecated, ) .unwrap_or_default(); let prioritization_fee = prioritization_fee_details.get_fee(); @@ -4946,6 +4958,9 @@ impl Bank { &self.fee_structure, self.feature_set .is_active(&use_default_units_in_fee_calculation::id()), + !self + .feature_set + .is_active(&remove_deprecated_request_unit_ix::id()), ); // In case of instruction error, even though no accounts @@ -10937,6 +10952,7 @@ pub(crate) mod tests { .lamports_per_signature, &FeeStructure::default(), true, + false, ); let (expected_fee_collected, expected_fee_burned) = @@ -11118,6 +11134,7 @@ pub(crate) mod tests { cheap_lamports_per_signature, &FeeStructure::default(), true, + false, ); assert_eq!( bank.get_balance(&mint_keypair.pubkey()), @@ -11135,6 +11152,7 @@ pub(crate) mod tests { expensive_lamports_per_signature, &FeeStructure::default(), true, + false, ); assert_eq!( bank.get_balance(&mint_keypair.pubkey()), @@ -11250,6 +11268,7 @@ pub(crate) mod tests { .lamports_per_signature, &FeeStructure::default(), true, + false, ) * 2 ) .0 @@ -17797,6 +17816,7 @@ pub(crate) mod tests { ..FeeStructure::default() }, true, + false, ), 0 ); @@ -17811,6 +17831,7 @@ pub(crate) mod tests { ..FeeStructure::default() }, true, + false, ), 1 ); @@ -17830,6 +17851,7 @@ pub(crate) mod tests { ..FeeStructure::default() }, true, + false, ), 4 ); @@ -17849,7 +17871,7 @@ pub(crate) mod tests { let message = SanitizedMessage::try_from(Message::new(&[], Some(&Pubkey::new_unique()))).unwrap(); assert_eq!( - Bank::calculate_fee(&message, 1, &fee_structure, true), + Bank::calculate_fee(&message, 1, &fee_structure, true, false), max_fee + lamports_per_signature ); @@ -17861,7 +17883,7 @@ pub(crate) mod tests { SanitizedMessage::try_from(Message::new(&[ix0, ix1], Some(&Pubkey::new_unique()))) .unwrap(); assert_eq!( - Bank::calculate_fee(&message, 1, &fee_structure, true), + Bank::calculate_fee(&message, 1, &fee_structure, true, false), max_fee + 3 * lamports_per_signature ); @@ -17894,7 +17916,7 @@ pub(crate) mod tests { Some(&Pubkey::new_unique()), )) .unwrap(); - let fee = Bank::calculate_fee(&message, 1, &fee_structure, true); + let fee = Bank::calculate_fee(&message, 1, &fee_structure, true, false); assert_eq!( fee, lamports_per_signature + prioritization_fee_details.get_fee() @@ -17932,7 +17954,10 @@ pub(crate) mod tests { Some(&key0), )) .unwrap(); - assert_eq!(Bank::calculate_fee(&message, 1, &fee_structure, true), 2); + assert_eq!( + Bank::calculate_fee(&message, 1, &fee_structure, true, false), + 2 + ); secp_instruction1.data = vec![0]; secp_instruction2.data = vec![10]; @@ -17941,7 +17966,10 @@ pub(crate) mod tests { Some(&key0), )) .unwrap(); - assert_eq!(Bank::calculate_fee(&message, 1, &fee_structure, true), 11); + assert_eq!( + Bank::calculate_fee(&message, 1, &fee_structure, true, false), + 11 + ); } #[test] diff --git a/runtime/src/transaction_priority_details.rs b/runtime/src/transaction_priority_details.rs index 6f406d0ed..2337dc24c 100644 --- a/runtime/src/transaction_priority_details.rs +++ b/runtime/src/transaction_priority_details.rs @@ -23,7 +23,8 @@ pub trait GetTransactionPriorityDetails { let prioritization_fee_details = compute_budget .process_instructions( instructions, - true, // use default units per instruction + true, // use default units per instruction + false, // stop supporting prioritization by request_units_deprecated instruction ) .ok()?; Some(TransactionPriorityDetails { diff --git a/sdk/src/compute_budget.rs b/sdk/src/compute_budget.rs index 1509286ba..28ce683ee 100644 --- a/sdk/src/compute_budget.rs +++ b/sdk/src/compute_budget.rs @@ -22,6 +22,7 @@ crate::declare_id!("ComputeBudget111111111111111111111111111111"); )] pub enum ComputeBudgetInstruction { /// Deprecated + // TODO: after feature remove_deprecated_request_unit_ix::id() is activated, replace it with 'unused' RequestUnitsDeprecated { /// Units to request units: u32, diff --git a/sdk/src/feature_set.rs b/sdk/src/feature_set.rs index 0ee39158a..2e6c9154c 100644 --- a/sdk/src/feature_set.rs +++ b/sdk/src/feature_set.rs @@ -518,6 +518,10 @@ pub mod epoch_accounts_hash { solana_sdk::declare_id!("5GpmAKxaGsWWbPp4bNXFLJxZVvG92ctxf7jQnzTQjF3n"); } +pub mod remove_deprecated_request_unit_ix { + solana_sdk::declare_id!("EfhYd3SafzGT472tYQDUc4dPd2xdEfKs5fwkowUgVt4W"); +} + lazy_static! { /// Map of feature identifiers to user-visible description pub static ref FEATURE_NAMES: HashMap = [ @@ -642,6 +646,7 @@ lazy_static! { (vote_state_update_root_fix::id(), "fix root in vote state updates #27361"), (cap_accounts_data_allocations_per_transaction::id(), "cap accounts data allocations per transaction #27375"), (epoch_accounts_hash::id(), "enable epoch accounts hash calculation #27539"), + (remove_deprecated_request_unit_ix::id(), "remove support for RequestUnitsDeprecated instruction #27500"), /*************** ADD NEW FEATURES HERE ***************/ ] .iter()