Skip to content

Commit

Permalink
Split compute budget instructions process from struct (solana-labs#33852
Browse files Browse the repository at this point in the history 
)

* 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.
  • Loading branch information
@tao-stones
tao-stones authored Oct 27, 2023
1 parent ba112a0 commit 510b6b9
Show file tree
Hide file tree
Showing 10 changed files with 881 additions and 762 deletions.
52 changes: 32 additions & 20 deletions accounts-db/src/accounts.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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::{
Expand All @@ -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,
Expand Down Expand Up @@ -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)),
)
Expand Down Expand Up @@ -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()),
)
Expand Down Expand Up @@ -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},
Expand Down Expand Up @@ -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,
);
Expand Down Expand Up @@ -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()));
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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,
);
Expand Down
69 changes: 34 additions & 35 deletions cost-model/src/cost_model.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
)
}

Expand Down Expand Up @@ -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(_) => {
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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)
);
}

Expand Down
Loading

0 comments on commit 510b6b9

Please sign in to comment.