Split compute budget instructions process from struct (#33852)

* Split compute budget instruction processing from ComputeBudget struct itself, so CB instructions can be processed elsewhere without involving ComputeBudget

* updated tests

* avoid built ComputeBudget from dated ComputeBudgetLimits in this refactoring PR

* Clean-up program-runtime/src/compute_budget_processor.rs

* Add test for a corner case that deprecated instruction is used to request units greater than max limit;
* Update code to handle the corner case.

Author: tao-stones

accounts-db/src/accounts.rs

@@ -24,7 +24,7 @@ use {
     itertools::Itertools,
     log::*,
     solana_program_runtime::{
-        compute_budget::{self, ComputeBudget},
+        compute_budget_processor::process_compute_budget_instructions,
         loaded_programs::LoadedProgramsForTxBatch,
     },
     solana_sdk::{
@@ -34,9 +34,8 @@ use {
         bpf_loader_upgradeable::{self, UpgradeableLoaderState},
         clock::{BankId, Slot},
         feature_set::{
-            self, add_set_tx_loaded_accounts_data_size_instruction,
-            include_loaded_accounts_data_size_in_fee_calculation,
-            remove_congestion_multiplier_from_fee_calculation, remove_deprecated_request_unit_ix,
+            self, include_loaded_accounts_data_size_in_fee_calculation,
+            remove_congestion_multiplier_from_fee_calculation,
             simplify_writable_program_account_check, FeatureSet,
         },
         fee::FeeStructure,
@@ -246,15 +245,16 @@ impl Accounts {
         feature_set: &FeatureSet,
     ) -> Result<Option<NonZeroUsize>> {
         if feature_set.is_active(&feature_set::cap_transaction_accounts_data_size::id()) {
-            let mut compute_budget =
-                ComputeBudget::new(compute_budget::MAX_COMPUTE_UNIT_LIMIT as u64);
-            let _process_transaction_result = compute_budget.process_instructions(
+            let compute_budget_limits = process_compute_budget_instructions(
                 tx.message().program_instructions_iter(),
-                !feature_set.is_active(&remove_deprecated_request_unit_ix::id()),
-                feature_set.is_active(&add_set_tx_loaded_accounts_data_size_instruction::id()),
-            );
+                feature_set,
+            )
+            .unwrap_or_default();
             // sanitize against setting size limit to zero
-            NonZeroUsize::new(compute_budget.loaded_accounts_data_size_limit).map_or(
+            NonZeroUsize::new(
+                usize::try_from(compute_budget_limits.loaded_accounts_bytes).unwrap_or_default(),
+            )
+            .map_or(
                 Err(TransactionError::InvalidLoadedAccountsDataSizeLimit),
                 |v| Ok(Some(v)),
             )
@@ -721,7 +721,7 @@ impl Accounts {
                         fee_structure.calculate_fee(
                             tx.message(),
                             lamports_per_signature,
-                            &ComputeBudget::fee_budget_limits(tx.message().program_instructions_iter(), feature_set),
+                            &process_compute_budget_instructions(tx.message().program_instructions_iter(), feature_set).unwrap_or_default().into(),
                             feature_set.is_active(&remove_congestion_multiplier_from_fee_calculation::id()),
                             feature_set.is_active(&include_loaded_accounts_data_size_in_fee_calculation::id()),
                         )
@@ -1470,8 +1470,9 @@ mod tests {
             transaction_results::{DurableNonceFee, TransactionExecutionDetails},
         },
         assert_matches::assert_matches,
-        solana_program_runtime::prioritization_fee::{
-            PrioritizationFeeDetails, PrioritizationFeeType,
+        solana_program_runtime::{
+            compute_budget_processor,
+            prioritization_fee::{PrioritizationFeeDetails, PrioritizationFeeType},
         },
         solana_sdk::{
             account::{AccountSharedData, WritableAccount},
@@ -1747,13 +1748,15 @@ mod tests {
         );
 
         let mut feature_set = FeatureSet::all_enabled();
-        feature_set.deactivate(&remove_deprecated_request_unit_ix::id());
+        feature_set.deactivate(&solana_sdk::feature_set::remove_deprecated_request_unit_ix::id());
 
         let message = SanitizedMessage::try_from(tx.message().clone()).unwrap();
         let fee = FeeStructure::default().calculate_fee(
             &message,
             lamports_per_signature,
-            &ComputeBudget::fee_budget_limits(message.program_instructions_iter(), &feature_set),
+            &process_compute_budget_instructions(message.program_instructions_iter(), &feature_set)
+                .unwrap_or_default()
+                .into(),
             true,
             false,
         );
@@ -4249,7 +4252,11 @@ mod tests {
 
         let result_no_limit = Ok(None);
         let result_default_limit = Ok(Some(
-            NonZeroUsize::new(compute_budget::MAX_LOADED_ACCOUNTS_DATA_SIZE_BYTES).unwrap(),
+            NonZeroUsize::new(
+                usize::try_from(compute_budget_processor::MAX_LOADED_ACCOUNTS_DATA_SIZE_BYTES)
+                    .unwrap(),
+            )
+            .unwrap(),
         ));
         let result_requested_limit: Result<Option<NonZeroUsize>> =
             Ok(Some(NonZeroUsize::new(99).unwrap()));
@@ -4277,7 +4284,10 @@ mod tests {
         //    if tx doesn't set limit, then default limit (64MiB)
         //    if tx sets limit, then requested limit
         //    if tx sets limit to zero, then TransactionError::InvalidLoadedAccountsDataSizeLimit
-        feature_set.activate(&add_set_tx_loaded_accounts_data_size_instruction::id(), 0);
+        feature_set.activate(
+            &solana_sdk::feature_set::add_set_tx_loaded_accounts_data_size_instruction::id(),
+            0,
+        );
         test(tx_not_set_limit, &feature_set, &result_default_limit);
         test(tx_set_limit_99, &feature_set, &result_requested_limit);
         test(tx_set_limit_0, &feature_set, &result_invalid_limit);
@@ -4312,13 +4322,15 @@ mod tests {
         );
 
         let mut feature_set = FeatureSet::all_enabled();
-        feature_set.deactivate(&remove_deprecated_request_unit_ix::id());
+        feature_set.deactivate(&solana_sdk::feature_set::remove_deprecated_request_unit_ix::id());
 
         let message = SanitizedMessage::try_from(tx.message().clone()).unwrap();
         let fee = FeeStructure::default().calculate_fee(
             &message,
             lamports_per_signature,
-            &ComputeBudget::fee_budget_limits(message.program_instructions_iter(), &feature_set),
+            &process_compute_budget_instructions(message.program_instructions_iter(), &feature_set)
+                .unwrap_or_default()
+                .into(),
             true,
             false,
         );

cost-model/src/cost_model.rs

@@ -8,17 +8,17 @@
 use {
     crate::{block_cost_limits::*, transaction_cost::*},
     log::*,
-    solana_program_runtime::compute_budget::{
-        ComputeBudget, DEFAULT_INSTRUCTION_COMPUTE_UNIT_LIMIT, MAX_COMPUTE_UNIT_LIMIT,
+    solana_program_runtime::{
+        compute_budget::DEFAULT_HEAP_COST,
+        compute_budget_processor::{
+            process_compute_budget_instructions, ComputeBudgetLimits,
+            DEFAULT_INSTRUCTION_COMPUTE_UNIT_LIMIT, MAX_COMPUTE_UNIT_LIMIT,
+        },
     },
     solana_sdk::{
         borsh0_10::try_from_slice_unchecked,
         compute_budget::{self, ComputeBudgetInstruction},
-        feature_set::{
-            add_set_tx_loaded_accounts_data_size_instruction,
-            include_loaded_accounts_data_size_in_fee_calculation,
-            remove_deprecated_request_unit_ix, FeatureSet,
-        },
+        feature_set::{include_loaded_accounts_data_size_in_fee_calculation, FeatureSet},
         fee::FeeStructure,
         instruction::CompiledInstruction,
         program_utils::limited_deserialize,
@@ -62,10 +62,12 @@ impl CostModel {
     // to set limit, `compute_budget.loaded_accounts_data_size_limit` is set to default
     // limit of 64MB; which will convert to (64M/32K)*8CU = 16_000 CUs
     //
-    pub fn calculate_loaded_accounts_data_size_cost(compute_budget: &ComputeBudget) -> u64 {
+    pub fn calculate_loaded_accounts_data_size_cost(
+        compute_budget_limits: &ComputeBudgetLimits,
+    ) -> u64 {
         FeeStructure::calculate_memory_usage_cost(
-            compute_budget.loaded_accounts_data_size_limit,
-            compute_budget.heap_cost,
+            usize::try_from(compute_budget_limits.loaded_accounts_bytes).unwrap(),
+            DEFAULT_HEAP_COST,
         )
     }
 
@@ -128,32 +130,28 @@ impl CostModel {
         }
 
         // calculate bpf cost based on compute budget instructions
-        let mut compute_budget = ComputeBudget::default();
-
-        let result = compute_budget.process_instructions(
-            transaction.message().program_instructions_iter(),
-            !feature_set.is_active(&remove_deprecated_request_unit_ix::id()),
-            feature_set.is_active(&add_set_tx_loaded_accounts_data_size_instruction::id()),
-        );
 
         // if failed to process compute_budget instructions, the transaction will not be executed
         // by `bank`, therefore it should be considered as no execution cost by cost model.
-        match result {
-            Ok(_) => {
+        match process_compute_budget_instructions(
+            transaction.message().program_instructions_iter(),
+            feature_set,
+        ) {
+            Ok(compute_budget_limits) => {
                 // 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 && compute_unit_limit_is_set {
-                    bpf_costs = compute_budget.compute_unit_limit
+                    bpf_costs = u64::from(compute_budget_limits.compute_unit_limit);
                 }
 
                 if feature_set
                     .is_active(&include_loaded_accounts_data_size_in_fee_calculation::id())
                 {
                     loaded_accounts_data_size_cost =
-                        Self::calculate_loaded_accounts_data_size_cost(&compute_budget);
+                        Self::calculate_loaded_accounts_data_size_cost(&compute_budget_limits);
                 }
             }
             Err(_) => {
@@ -545,7 +543,8 @@ mod tests {
         // default loaded_accounts_data_size_limit
         const DEFAULT_PAGE_COST: u64 = 8;
         let expected_loaded_accounts_data_size_cost =
-            solana_program_runtime::compute_budget::MAX_LOADED_ACCOUNTS_DATA_SIZE_BYTES as u64
+            solana_program_runtime::compute_budget_processor::MAX_LOADED_ACCOUNTS_DATA_SIZE_BYTES
+                as u64
                 / ACCOUNT_DATA_COST_PAGE_SIZE
                 * DEFAULT_PAGE_COST;
 
@@ -663,36 +662,36 @@ mod tests {
     #[allow(clippy::field_reassign_with_default)]
     #[test]
     fn test_calculate_loaded_accounts_data_size_cost() {
-        let mut compute_budget = ComputeBudget::default();
+        let mut compute_budget_limits = ComputeBudgetLimits::default();
 
         // accounts data size are priced in block of 32K, ...
 
         // ... requesting less than 32K should still be charged as one block
-        compute_budget.loaded_accounts_data_size_limit = 31_usize * 1024;
+        compute_budget_limits.loaded_accounts_bytes = 31 * 1024;
         assert_eq!(
-            compute_budget.heap_cost,
-            CostModel::calculate_loaded_accounts_data_size_cost(&compute_budget)
+            DEFAULT_HEAP_COST,
+            CostModel::calculate_loaded_accounts_data_size_cost(&compute_budget_limits)
         );
 
         // ... requesting exact 32K should be charged as one block
-        compute_budget.loaded_accounts_data_size_limit = 32_usize * 1024;
+        compute_budget_limits.loaded_accounts_bytes = 32 * 1024;
         assert_eq!(
-            compute_budget.heap_cost,
-            CostModel::calculate_loaded_accounts_data_size_cost(&compute_budget)
+            DEFAULT_HEAP_COST,
+            CostModel::calculate_loaded_accounts_data_size_cost(&compute_budget_limits)
         );
 
         // ... requesting slightly above 32K should be charged as 2 block
-        compute_budget.loaded_accounts_data_size_limit = 33_usize * 1024;
+        compute_budget_limits.loaded_accounts_bytes = 33 * 1024;
         assert_eq!(
-            compute_budget.heap_cost * 2,
-            CostModel::calculate_loaded_accounts_data_size_cost(&compute_budget)
+            DEFAULT_HEAP_COST * 2,
+            CostModel::calculate_loaded_accounts_data_size_cost(&compute_budget_limits)
         );
 
         // ... requesting exact 64K should be charged as 2 block
-        compute_budget.loaded_accounts_data_size_limit = 64_usize * 1024;
+        compute_budget_limits.loaded_accounts_bytes = 64 * 1024;
         assert_eq!(
-            compute_budget.heap_cost * 2,
-            CostModel::calculate_loaded_accounts_data_size_cost(&compute_budget)
+            DEFAULT_HEAP_COST * 2,
+            CostModel::calculate_loaded_accounts_data_size_cost(&compute_budget_limits)
         );
     }