v1.16: cost model could double count builtin instruction cost (backpo…

…rt 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 c69bc00) # Conflicts: # runtime/src/cost_model.rs * fix backport merge conflict * fix a logic error --------- Co-authored-by: Tao Zhu <[email protected]> Co-authored-by: Tao Zhu <[email protected]>
solana-labs · Jul 19, 2023 · 8ed7ebe · 8ed7ebe
1 parent e75eac9
commit 8ed7ebe
Show file tree

Hide file tree

Showing 2 changed files with 87 additions and 5 deletions.
diff --git 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
@@ -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);
+    }
 }