From 8b8a21a52f51d2a4904075b6ada356281c44c0ca Mon Sep 17 00:00:00 2001 From: Tao Zhu <82401714+taozhu-chicago@users.noreply.github.com> Date: Mon, 18 Sep 2023 16:06:24 -0500 Subject: [PATCH] cleanup feature: enable request heap frame instruction #30076 (#33243) * cleanup feature: enable request heap frame instruction #30076 * update sbf tests * removed out dated comments and test --- accounts-db/src/accounts.rs | 15 +-- cost-model/src/cost_model.rs | 8 -- program-runtime/src/compute_budget.rs | 132 +------------------- programs/sbf/tests/programs.rs | 2 - runtime/src/bank.rs | 2 - runtime/src/bank/tests.rs | 51 +------- runtime/src/transaction_priority_details.rs | 1 - 7 files changed, 12 insertions(+), 199 deletions(-) diff --git a/accounts-db/src/accounts.rs b/accounts-db/src/accounts.rs index f570fbdd2ad42a..47b372d981843a 100644 --- a/accounts-db/src/accounts.rs +++ b/accounts-db/src/accounts.rs @@ -252,7 +252,6 @@ impl Accounts { let _process_transaction_result = compute_budget.process_instructions( tx.message().program_instructions_iter(), !feature_set.is_active(&remove_deprecated_request_unit_ix::id()), - true, // don't reject txs that use request heap size ix feature_set.is_active(&add_set_tx_loaded_accounts_data_size_instruction::id()), ); // sanitize against setting size limit to zero @@ -723,7 +722,7 @@ impl Accounts { fee_structure.calculate_fee( tx.message(), lamports_per_signature, - &ComputeBudget::fee_budget_limits(tx.message().program_instructions_iter(), feature_set, Some(self.accounts_db.expected_cluster_type())), + &ComputeBudget::fee_budget_limits(tx.message().program_instructions_iter(), feature_set), feature_set.is_active(&remove_congestion_multiplier_from_fee_calculation::id()), feature_set.is_active(&include_loaded_accounts_data_size_in_fee_calculation::id()), ) @@ -1758,11 +1757,7 @@ mod tests { let fee = FeeStructure::default().calculate_fee( &message, lamports_per_signature, - &ComputeBudget::fee_budget_limits( - message.program_instructions_iter(), - &feature_set, - None, - ), + &ComputeBudget::fee_budget_limits(message.program_instructions_iter(), &feature_set), true, false, ); @@ -4327,11 +4322,7 @@ mod tests { let fee = FeeStructure::default().calculate_fee( &message, lamports_per_signature, - &ComputeBudget::fee_budget_limits( - message.program_instructions_iter(), - &feature_set, - None, - ), + &ComputeBudget::fee_budget_limits(message.program_instructions_iter(), &feature_set), true, false, ); diff --git a/cost-model/src/cost_model.rs b/cost-model/src/cost_model.rs index e321719e993030..b3ffdad3e6a2a6 100644 --- a/cost-model/src/cost_model.rs +++ b/cost-model/src/cost_model.rs @@ -119,17 +119,9 @@ impl CostModel { // calculate bpf cost based on compute budget instructions let mut compute_budget = ComputeBudget::default(); - // Starting from v1.15, cost model uses compute_budget.set_compute_unit_limit to - // measure bpf_costs (code below), vs earlier versions that use estimated - // bpf instruction costs. The calculated transaction costs are used by leaders - // during block packing, different costs for same transaction due to different versions - // will not impact consensus. So for v1.15+, should call compute budget with - // the feature gate `enable_request_heap_frame_ix` enabled. - let enable_request_heap_frame_ix = true; let result = compute_budget.process_instructions( transaction.message().program_instructions_iter(), !feature_set.is_active(&remove_deprecated_request_unit_ix::id()), - enable_request_heap_frame_ix, feature_set.is_active(&add_set_tx_loaded_accounts_data_size_instruction::id()), ); diff --git a/program-runtime/src/compute_budget.rs b/program-runtime/src/compute_budget.rs index 44fb070b3786ae..a1272cf1707c14 100644 --- a/program-runtime/src/compute_budget.rs +++ b/program-runtime/src/compute_budget.rs @@ -5,11 +5,10 @@ use { compute_budget::{self, ComputeBudgetInstruction}, entrypoint::HEAP_LENGTH as MIN_HEAP_FRAME_BYTES, feature_set::{ - add_set_tx_loaded_accounts_data_size_instruction, enable_request_heap_frame_ix, - remove_deprecated_request_unit_ix, FeatureSet, + add_set_tx_loaded_accounts_data_size_instruction, remove_deprecated_request_unit_ix, + FeatureSet, }, fee::FeeBudgetLimits, - genesis_config::ClusterType, instruction::{CompiledInstruction, InstructionError}, pubkey::Pubkey, transaction::TransactionError, @@ -191,7 +190,6 @@ impl ComputeBudget { &mut self, instructions: impl Iterator, support_request_units_deprecated: bool, - enable_request_heap_frame_ix: bool, support_set_loaded_accounts_data_size_limit_ix: bool, ) -> Result { let mut num_non_compute_budget_instructions: u32 = 0; @@ -260,8 +258,7 @@ impl ComputeBudget { } if let Some((bytes, i)) = requested_heap_size { - if !enable_request_heap_frame_ix - || bytes > MAX_HEAP_FRAME_BYTES + if bytes > MAX_HEAP_FRAME_BYTES || bytes < MIN_HEAP_FRAME_BYTES as u32 || bytes % 1024 != 0 { @@ -293,23 +290,13 @@ impl ComputeBudget { pub fn fee_budget_limits<'a>( instructions: impl Iterator, feature_set: &FeatureSet, - maybe_cluster_type: Option, ) -> FeeBudgetLimits { let mut compute_budget = Self::default(); - // A cluster specific feature gate, when not activated it keeps v1.13 behavior in mainnet-beta; - // once activated for v1.14+, it allows compute_budget::request_heap_frame and - // compute_budget::set_compute_unit_price co-exist in same transaction. - let enable_request_heap_frame_ix = feature_set - .is_active(&enable_request_heap_frame_ix::id()) - || maybe_cluster_type - .and_then(|cluster_type| (cluster_type != ClusterType::MainnetBeta).then_some(0)) - .is_some(); let prioritization_fee_details = compute_budget .process_instructions( instructions, !feature_set.is_active(&remove_deprecated_request_unit_ix::id()), - enable_request_heap_frame_ix, feature_set.is_active(&add_set_tx_loaded_accounts_data_size_instruction::id()), ) .unwrap_or_default(); @@ -369,7 +356,7 @@ mod tests { }; macro_rules! test { - ( $instructions: expr, $expected_result: expr, $expected_budget: expr, $enable_request_heap_frame_ix: expr, $support_set_loaded_accounts_data_size_limit_ix: expr ) => { + ( $instructions: expr, $expected_result: expr, $expected_budget: expr, $support_set_loaded_accounts_data_size_limit_ix: expr ) => { let payer_keypair = Keypair::new(); let tx = SanitizedTransaction::from_transaction_for_tests(Transaction::new( &[&payer_keypair], @@ -380,20 +367,13 @@ mod tests { let result = compute_budget.process_instructions( tx.message().program_instructions_iter(), false, /*not support request_units_deprecated*/ - $enable_request_heap_frame_ix, $support_set_loaded_accounts_data_size_limit_ix, ); assert_eq!($expected_result, result); assert_eq!(compute_budget, $expected_budget); }; ( $instructions: expr, $expected_result: expr, $expected_budget: expr) => { - test!( - $instructions, - $expected_result, - $expected_budget, - true, - false - ); + test!($instructions, $expected_result, $expected_budget, false); }; } @@ -654,104 +634,8 @@ mod tests { ); } - #[test] - fn test_process_instructions_disable_request_heap_frame() { - // assert empty message results default compute budget and fee - test!( - &[], - Ok(PrioritizationFeeDetails::default()), - ComputeBudget { - compute_unit_limit: 0, - ..ComputeBudget::default() - }, - false, - false - ); - - // assert requesting heap frame when feature is disable will result instruction error - test!( - &[ - ComputeBudgetInstruction::request_heap_frame(40 * 1024), - Instruction::new_with_bincode(Pubkey::new_unique(), &0_u8, vec![]), - ], - Err(TransactionError::InstructionError( - 0, - InstructionError::InvalidInstructionData - )), - ComputeBudget::default(), - false, - false - ); - test!( - &[ - Instruction::new_with_bincode(Pubkey::new_unique(), &0_u8, vec![]), - ComputeBudgetInstruction::request_heap_frame(MAX_HEAP_FRAME_BYTES), - ], - Err(TransactionError::InstructionError( - 1, - InstructionError::InvalidInstructionData, - )), - ComputeBudget::default(), - false, - false - ); - test!( - &[ - Instruction::new_with_bincode(Pubkey::new_unique(), &0_u8, vec![]), - ComputeBudgetInstruction::request_heap_frame(MAX_HEAP_FRAME_BYTES), - ComputeBudgetInstruction::set_compute_unit_limit(MAX_COMPUTE_UNIT_LIMIT), - ComputeBudgetInstruction::set_compute_unit_price(u64::MAX), - ], - Err(TransactionError::InstructionError( - 1, - InstructionError::InvalidInstructionData, - )), - ComputeBudget::default(), - false, - false - ); - test!( - &[ - Instruction::new_with_bincode(Pubkey::new_unique(), &0_u8, vec![]), - ComputeBudgetInstruction::set_compute_unit_limit(1), - ComputeBudgetInstruction::request_heap_frame(MAX_HEAP_FRAME_BYTES), - ComputeBudgetInstruction::set_compute_unit_price(u64::MAX), - ], - Err(TransactionError::InstructionError( - 2, - InstructionError::InvalidInstructionData, - )), - ComputeBudget::default(), - false, - false - ); - - // assert normal results when not requesting heap frame when the feature is disabled - test!( - &[ - Instruction::new_with_bincode(Pubkey::new_unique(), &0_u8, vec![]), - Instruction::new_with_bincode(Pubkey::new_unique(), &0_u8, vec![]), - Instruction::new_with_bincode(Pubkey::new_unique(), &0_u8, vec![]), - Instruction::new_with_bincode(Pubkey::new_unique(), &0_u8, vec![]), - Instruction::new_with_bincode(Pubkey::new_unique(), &0_u8, vec![]), - Instruction::new_with_bincode(Pubkey::new_unique(), &0_u8, vec![]), - Instruction::new_with_bincode(Pubkey::new_unique(), &0_u8, vec![]), - Instruction::new_with_bincode(Pubkey::new_unique(), &0_u8, vec![]), - ], - Ok(PrioritizationFeeDetails::default()), - ComputeBudget { - compute_unit_limit: DEFAULT_INSTRUCTION_COMPUTE_UNIT_LIMIT as u64 * 7, - ..ComputeBudget::default() - }, - false, - false - ); - } - #[test] fn test_process_loaded_accounts_data_size_limit_instruction() { - let enable_request_heap_frame_ix: bool = true; - // Assert for empty instructions, change value of support_set_loaded_accounts_data_size_limit_ix // will not change results, which should all be default for support_set_loaded_accounts_data_size_limit_ix in [true, false] { @@ -762,7 +646,6 @@ mod tests { compute_unit_limit: 0, ..ComputeBudget::default() }, - enable_request_heap_frame_ix, support_set_loaded_accounts_data_size_limit_ix ); } @@ -801,7 +684,6 @@ mod tests { ], expected_result, expected_budget, - enable_request_heap_frame_ix, support_set_loaded_accounts_data_size_limit_ix ); } @@ -840,7 +722,6 @@ mod tests { ], expected_result, expected_budget, - enable_request_heap_frame_ix, support_set_loaded_accounts_data_size_limit_ix ); } @@ -868,7 +749,6 @@ mod tests { ),], expected_result, expected_budget, - enable_request_heap_frame_ix, support_set_loaded_accounts_data_size_limit_ix ); } @@ -904,7 +784,6 @@ mod tests { ], expected_result, expected_budget, - enable_request_heap_frame_ix, support_set_loaded_accounts_data_size_limit_ix ); } @@ -929,7 +808,6 @@ mod tests { let result = compute_budget.process_instructions( transaction.message().program_instructions_iter(), false, //not support request_units_deprecated - true, //enable_request_heap_frame_ix, true, //support_set_loaded_accounts_data_size_limit_ix, ); diff --git a/programs/sbf/tests/programs.rs b/programs/sbf/tests/programs.rs index b690ea2ffef434..9bdec77a897f59 100644 --- a/programs/sbf/tests/programs.rs +++ b/programs/sbf/tests/programs.rs @@ -3836,7 +3836,6 @@ fn test_program_fees() { &ComputeBudget::fee_budget_limits( sanitized_message.program_instructions_iter(), &feature_set, - None, ), true, false, @@ -3864,7 +3863,6 @@ fn test_program_fees() { &ComputeBudget::fee_budget_limits( sanitized_message.program_instructions_iter(), &feature_set, - None, ), true, false, diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index def01b9d5bcbf1..543c90350697f1 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -4092,7 +4092,6 @@ impl Bank { &ComputeBudget::fee_budget_limits( message.program_instructions_iter(), &self.feature_set, - Some(self.cluster_type()), ), self.feature_set .is_active(&remove_congestion_multiplier_from_fee_calculation::id()), @@ -5179,7 +5178,6 @@ impl Bank { !self .feature_set .is_active(&remove_deprecated_request_unit_ix::id()), - true, // don't reject txs that use request heap size ix self.feature_set .is_active(&add_set_tx_loaded_accounts_data_size_instruction::id()), ); diff --git a/runtime/src/bank/tests.rs b/runtime/src/bank/tests.rs index 001494e5594e7a..299ece5fc8998c 100644 --- a/runtime/src/bank/tests.rs +++ b/runtime/src/bank/tests.rs @@ -63,7 +63,7 @@ use { entrypoint::MAX_PERMITTED_DATA_INCREASE, epoch_schedule::{EpochSchedule, MINIMUM_SLOTS_PER_EPOCH}, feature::{self, Feature}, - feature_set::{self, enable_request_heap_frame_ix, FeatureSet}, + feature_set::{self, FeatureSet}, fee::FeeStructure, fee_calculator::FeeRateGovernor, genesis_config::{create_genesis_config, ClusterType, GenesisConfig}, @@ -2829,7 +2829,6 @@ fn test_bank_tx_compute_unit_fee() { &FeeStructure::default(), false, true, - true, ); let (expected_fee_collected, expected_fee_burned) = @@ -3011,7 +3010,6 @@ fn test_bank_blockhash_compute_unit_fee_structure() { &FeeStructure::default(), false, true, - true, ); assert_eq!( bank.get_balance(&mint_keypair.pubkey()), @@ -3030,7 +3028,6 @@ fn test_bank_blockhash_compute_unit_fee_structure() { &FeeStructure::default(), false, true, - true, ); assert_eq!( bank.get_balance(&mint_keypair.pubkey()), @@ -3144,7 +3141,6 @@ fn test_filter_program_errors_and_collect_compute_unit_fee() { &FeeStructure::default(), false, true, - true, ) * 2 ) .0 @@ -10074,7 +10070,6 @@ fn calculate_test_fee( lamports_per_signature: u64, fee_structure: &FeeStructure, support_set_accounts_data_size_limit_ix: bool, - enable_request_heap_frame_ix: bool, remove_congestion_multiplier: bool, ) -> u64 { let mut feature_set = FeatureSet::all_enabled(); @@ -10084,12 +10079,8 @@ fn calculate_test_fee( feature_set.deactivate(&include_loaded_accounts_data_size_in_fee_calculation::id()); } - if !enable_request_heap_frame_ix { - feature_set.deactivate(&enable_request_heap_frame_ix::id()); - } - let budget_limits = - ComputeBudget::fee_budget_limits(message.program_instructions_iter(), &feature_set, None); + ComputeBudget::fee_budget_limits(message.program_instructions_iter(), &feature_set); fee_structure.calculate_fee( message, lamports_per_signature, @@ -10115,7 +10106,6 @@ fn test_calculate_fee() { }, support_set_accounts_data_size_limit_ix, true, - true, ), 0 ); @@ -10133,7 +10123,6 @@ fn test_calculate_fee() { }, support_set_accounts_data_size_limit_ix, true, - true, ), 1 ); @@ -10156,7 +10145,6 @@ fn test_calculate_fee() { }, support_set_accounts_data_size_limit_ix, true, - true, ), 4 ); @@ -10184,7 +10172,6 @@ fn test_calculate_fee_compute_units() { &fee_structure, support_set_accounts_data_size_limit_ix, true, - true, ), max_fee + lamports_per_signature ); @@ -10204,7 +10191,6 @@ fn test_calculate_fee_compute_units() { &fee_structure, support_set_accounts_data_size_limit_ix, true, - true, ), max_fee + 3 * lamports_per_signature ); @@ -10246,7 +10232,6 @@ fn test_calculate_fee_compute_units() { &fee_structure, support_set_accounts_data_size_limit_ix, true, - true, ); assert_eq!( fee, @@ -10286,7 +10271,6 @@ fn test_calculate_prioritization_fee() { &fee_structure, true, true, - true, ); assert_eq!( fee, @@ -10332,7 +10316,6 @@ fn test_calculate_fee_secp256k1() { &fee_structure, support_set_accounts_data_size_limit_ix, true, - true, ), 2 ); @@ -10353,7 +10336,6 @@ fn test_calculate_fee_secp256k1() { &fee_structure, support_set_accounts_data_size_limit_ix, true, - true, ), 11 ); @@ -11994,7 +11976,6 @@ fn test_calculate_fee_with_congestion_multiplier() { cheap_lamports_per_signature, &fee_structure, true, - true, remove_congestion_multiplier, ), signature_fee * signature_count @@ -12016,7 +11997,6 @@ fn test_calculate_fee_with_congestion_multiplier() { expensive_lamports_per_signature, &fee_structure, true, - true, remove_congestion_multiplier, ), signature_fee * signature_count / denominator @@ -12047,35 +12027,12 @@ fn test_calculate_fee_with_request_heap_frame_flag() { )) .unwrap(); - // assert when enable_request_heap_frame_ix is enabled, prioritization fee will be counted + // assert when request_heap_frame is presented in tx, prioritization fee will be counted // into transaction fee - let mut enable_request_heap_frame_ix = true; assert_eq!( - calculate_test_fee( - &message, - lamports_per_signature, - &fee_structure, - true, - enable_request_heap_frame_ix, - true, - ), + calculate_test_fee(&message, lamports_per_signature, &fee_structure, true, true,), signature_fee + request_cu * lamports_per_cu ); - - // assert when enable_request_heap_frame_ix is disabled (an v1.13 behavior), prioritization fee will not be counted - // into transaction fee - enable_request_heap_frame_ix = false; - assert_eq!( - calculate_test_fee( - &message, - lamports_per_signature, - &fee_structure, - true, - enable_request_heap_frame_ix, - true, - ), - signature_fee - ); } #[test] diff --git a/runtime/src/transaction_priority_details.rs b/runtime/src/transaction_priority_details.rs index 08cbefd3280b93..0d0a94df4ed393 100644 --- a/runtime/src/transaction_priority_details.rs +++ b/runtime/src/transaction_priority_details.rs @@ -28,7 +28,6 @@ pub trait GetTransactionPriorityDetails { .process_instructions( instructions, true, // supports prioritization by request_units_deprecated instruction - true, // enable request heap frame instruction true, // enable support set accounts data size instruction // TODO: round_compute_unit_price_enabled: bool )