From 8ed7ebe801c25d83c1be73b04155fa18f91d9dc8 Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Wed, 19 Jul 2023 16:03:45 -0500 Subject: [PATCH] v1.16: cost model could double count builtin instruction cost (backport of #32422) (#32516) * cost model could double count builtin instruction cost (#32422) 1. add tests to demo builtin cost could be double counted; 2. quick fix for now (cherry picked from commit c69bc00f69e2111afb1f6a727c1c6f3b7b1a02bb) # Conflicts: # runtime/src/cost_model.rs * fix backport merge conflict * fix a logic error --------- Co-authored-by: Tao Zhu <82401714+taozhu-chicago@users.noreply.github.com> Co-authored-by: Tao Zhu --- program-runtime/src/compute_budget.rs | 38 +++++++++++++++++++ runtime/src/cost_model.rs | 54 ++++++++++++++++++++++++--- 2 files changed, 87 insertions(+), 5 deletions(-) diff --git a/program-runtime/src/compute_budget.rs b/program-runtime/src/compute_budget.rs index 7f00f4386dcb04..77a37c9f86a05b 100644 --- a/program-runtime/src/compute_budget.rs +++ b/program-runtime/src/compute_budget.rs @@ -289,6 +289,7 @@ mod tests { pubkey::Pubkey, signature::Keypair, signer::Signer, + system_instruction::{self}, transaction::{SanitizedTransaction, Transaction}, }, }; @@ -835,4 +836,41 @@ mod tests { ); } } + + #[test] + fn test_process_mixed_instructions_without_compute_budget() { + let payer_keypair = Keypair::new(); + + let transaction = + SanitizedTransaction::from_transaction_for_tests(Transaction::new_signed_with_payer( + &[ + Instruction::new_with_bincode(Pubkey::new_unique(), &0_u8, vec![]), + system_instruction::transfer(&payer_keypair.pubkey(), &Pubkey::new_unique(), 2), + ], + Some(&payer_keypair.pubkey()), + &[&payer_keypair], + Hash::default(), + )); + + let mut compute_budget = ComputeBudget::default(); + let result = compute_budget.process_instructions( + transaction.message().program_instructions_iter(), + true, + false, //not support request_units_deprecated + true, //enable_request_heap_frame_ix, + true, //support_set_loaded_accounts_data_size_limit_ix, + ); + + // assert process_instructions will be successful with default, + assert_eq!(Ok(PrioritizationFeeDetails::default()), result); + // assert the default compute_unit_limit is 2 times default: one for bpf ix, one for + // builtin ix. + assert_eq!( + compute_budget, + ComputeBudget { + compute_unit_limit: 2 * DEFAULT_INSTRUCTION_COMPUTE_UNIT_LIMIT as u64, + ..ComputeBudget::default() + } + ); + } } diff --git a/runtime/src/cost_model.rs b/runtime/src/cost_model.rs index 36cbbd5f407805..745bb89228995d 100644 --- a/runtime/src/cost_model.rs +++ b/runtime/src/cost_model.rs @@ -9,9 +9,11 @@ use { crate::block_cost_limits::*, log::*, solana_program_runtime::compute_budget::{ - ComputeBudget, DEFAULT_INSTRUCTION_COMPUTE_UNIT_LIMIT, + ComputeBudget, DEFAULT_INSTRUCTION_COMPUTE_UNIT_LIMIT, MAX_COMPUTE_UNIT_LIMIT, }, solana_sdk::{ + borsh::try_from_slice_unchecked, + compute_budget::{self, ComputeBudgetInstruction}, feature_set::{ add_set_tx_loaded_accounts_data_size_instruction, remove_deprecated_request_unit_ix, use_default_units_in_fee_calculation, FeatureSet, @@ -150,16 +152,27 @@ impl CostModel { let mut builtin_costs = 0u64; let mut bpf_costs = 0u64; let mut data_bytes_len_total = 0u64; + let mut compute_unit_limit_is_set = false; for (program_id, instruction) in transaction.message().program_instructions_iter() { // to keep the same behavior, look for builtin first if let Some(builtin_cost) = BUILT_IN_INSTRUCTION_COSTS.get(program_id) { builtin_costs = builtin_costs.saturating_add(*builtin_cost); } else { - bpf_costs = bpf_costs.saturating_add(DEFAULT_INSTRUCTION_COMPUTE_UNIT_LIMIT.into()); + bpf_costs = bpf_costs + .saturating_add(u64::from(DEFAULT_INSTRUCTION_COMPUTE_UNIT_LIMIT)) + .min(u64::from(MAX_COMPUTE_UNIT_LIMIT)); } data_bytes_len_total = data_bytes_len_total.saturating_add(instruction.data.len() as u64); + + if compute_budget::check_id(program_id) { + if let Ok(ComputeBudgetInstruction::SetComputeUnitLimit(_)) = + try_from_slice_unchecked(&instruction.data) + { + compute_unit_limit_is_set = true; + } + } } // calculate bpf cost based on compute budget instructions @@ -180,8 +193,12 @@ impl CostModel { feature_set.is_active(&add_set_tx_loaded_accounts_data_size_instruction::id()), ); - // if tx contained user-space instructions and a more accurate estimate available correct it - if bpf_costs > 0 && result.is_ok() { + // if tx contained user-space instructions and a more accurate estimate available correct it, + // where "user-space instructions" must be specifically checked by + // 'compute_unit_limit_is_set' flag, because compute_budget does not distinguish + // builtin and bpf instructions when calculating default compute-unit-limit. (see + // compute_budget.rs test `test_process_mixed_instructions_without_compute_budget`) + if bpf_costs > 0 && result.is_ok() && compute_unit_limit_is_set { bpf_costs = budget.compute_unit_limit } @@ -256,7 +273,7 @@ mod tests { solana_sdk::{ compute_budget::{self, ComputeBudgetInstruction}, hash::Hash, - instruction::CompiledInstruction, + instruction::{CompiledInstruction, Instruction}, message::Message, signature::{Keypair, Signer}, system_instruction::{self}, @@ -537,4 +554,31 @@ mod tests { assert_eq!(*expected_execution_cost, tx_cost.builtins_execution_cost); assert_eq!(2, tx_cost.writable_accounts.len()); } + + #[test] + fn test_transaction_cost_with_mix_instruction_without_compute_budget() { + let (mint_keypair, start_hash) = test_setup(); + + let transaction = + SanitizedTransaction::from_transaction_for_tests(Transaction::new_signed_with_payer( + &[ + Instruction::new_with_bincode(Pubkey::new_unique(), &0_u8, vec![]), + system_instruction::transfer(&mint_keypair.pubkey(), &Pubkey::new_unique(), 2), + ], + Some(&mint_keypair.pubkey()), + &[&mint_keypair], + start_hash, + )); + // transaction has one builtin instruction, and one bpf instruction, no ComputeBudget::compute_unit_limit + let expected_builtin_cost = *BUILT_IN_INSTRUCTION_COSTS + .get(&solana_system_program::id()) + .unwrap(); + let expected_bpf_cost = DEFAULT_INSTRUCTION_COMPUTE_UNIT_LIMIT; + + let mut tx_cost = TransactionCost::default(); + CostModel::get_transaction_cost(&mut tx_cost, &transaction, &FeatureSet::all_enabled()); + + assert_eq!(expected_builtin_cost, tx_cost.builtins_execution_cost); + assert_eq!(expected_bpf_cost as u64, tx_cost.bpf_execution_cost); + } }