Skip to content

Commit

Permalink
Revert "Split compute budget instructions process from struct itself … (
Browse files Browse the repository at this point in the history 
#33784)

Revert "Split compute budget instructions process from struct itself (#33513)"

This reverts commit c73bebe. This
was found to be a consensus breaking change.
  • Loading branch information
@steviez
steviez authored Oct 20, 2023
1 parent e1a9f8e commit c98c24b
Show file tree
Hide file tree
Showing 10 changed files with 762 additions and 796 deletions.
52 changes: 20 additions & 32 deletions accounts-db/src/accounts.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ use {
itertools::Itertools,
log::*,
solana_program_runtime::{
compute_budget_processor::process_compute_budget_instructions,
compute_budget::{self, ComputeBudget},
loaded_programs::LoadedProgramsForTxBatch,
},
solana_sdk::{
Expand All @@ -35,8 +35,9 @@ use {
bpf_loader_upgradeable::{self, UpgradeableLoaderState},
clock::{BankId, Slot},
feature_set::{
self, include_loaded_accounts_data_size_in_fee_calculation,
remove_congestion_multiplier_from_fee_calculation,
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,
simplify_writable_program_account_check, FeatureSet,
},
fee::FeeStructure,
Expand Down Expand Up @@ -246,16 +247,15 @@ impl Accounts {
feature_set: &FeatureSet,
) -> Result<Option<NonZeroUsize>> {
if feature_set.is_active(&feature_set::cap_transaction_accounts_data_size::id()) {
let compute_budget_limits = process_compute_budget_instructions(
let mut compute_budget =
ComputeBudget::new(compute_budget::MAX_COMPUTE_UNIT_LIMIT as u64);
let _process_transaction_result = compute_budget.process_instructions(
tx.message().program_instructions_iter(),
feature_set,
)
.unwrap_or_default();
!feature_set.is_active(&remove_deprecated_request_unit_ix::id()),
feature_set.is_active(&add_set_tx_loaded_accounts_data_size_instruction::id()),
);
// sanitize against setting size limit to zero
NonZeroUsize::new(
usize::try_from(compute_budget_limits.loaded_accounts_bytes).unwrap_or_default(),
)
.map_or(
NonZeroUsize::new(compute_budget.loaded_accounts_data_size_limit).map_or(
Err(TransactionError::InvalidLoadedAccountsDataSizeLimit),
|v| Ok(Some(v)),
)
Expand Down Expand Up @@ -722,7 +722,7 @@ impl Accounts {
fee_structure.calculate_fee(
tx.message(),
lamports_per_signature,
&process_compute_budget_instructions(tx.message().program_instructions_iter(), feature_set).unwrap_or_default().into(),
&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()),
)
Expand Down Expand Up @@ -1474,9 +1474,8 @@ mod tests {
transaction_results::{DurableNonceFee, TransactionExecutionDetails},
},
assert_matches::assert_matches,
solana_program_runtime::{
compute_budget_processor,
prioritization_fee::{PrioritizationFeeDetails, PrioritizationFeeType},
solana_program_runtime::prioritization_fee::{
PrioritizationFeeDetails, PrioritizationFeeType,
},
solana_sdk::{
account::{AccountSharedData, WritableAccount},
Expand Down Expand Up @@ -1752,15 +1751,13 @@ mod tests {
);

let mut feature_set = FeatureSet::all_enabled();
feature_set.deactivate(&solana_sdk::feature_set::remove_deprecated_request_unit_ix::id());
feature_set.deactivate(&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,
&process_compute_budget_instructions(message.program_instructions_iter(), &feature_set)
.unwrap_or_default()
.into(),
&ComputeBudget::fee_budget_limits(message.program_instructions_iter(), &feature_set),
true,
false,
);
Expand Down Expand Up @@ -4256,11 +4253,7 @@ mod tests {

let result_no_limit = Ok(None);
let result_default_limit = Ok(Some(
NonZeroUsize::new(
usize::try_from(compute_budget_processor::MAX_LOADED_ACCOUNTS_DATA_SIZE_BYTES)
.unwrap(),
)
.unwrap(),
NonZeroUsize::new(compute_budget::MAX_LOADED_ACCOUNTS_DATA_SIZE_BYTES).unwrap(),
));
let result_requested_limit: Result<Option<NonZeroUsize>> =
Ok(Some(NonZeroUsize::new(99).unwrap()));
Expand Down Expand Up @@ -4288,10 +4281,7 @@ 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(
&solana_sdk::feature_set::add_set_tx_loaded_accounts_data_size_instruction::id(),
0,
);
feature_set.activate(&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 @@ -4326,15 +4316,13 @@ mod tests {
);

let mut feature_set = FeatureSet::all_enabled();
feature_set.deactivate(&solana_sdk::feature_set::remove_deprecated_request_unit_ix::id());
feature_set.deactivate(&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,
&process_compute_budget_instructions(message.program_instructions_iter(), &feature_set)
.unwrap_or_default()
.into(),
&ComputeBudget::fee_budget_limits(message.program_instructions_iter(), &feature_set),
true,
false,
);
Expand Down
69 changes: 35 additions & 34 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::DEFAULT_HEAP_COST,
compute_budget_processor::{
process_compute_budget_instructions, ComputeBudgetLimits,
DEFAULT_INSTRUCTION_COMPUTE_UNIT_LIMIT, MAX_COMPUTE_UNIT_LIMIT,
},
solana_program_runtime::compute_budget::{
ComputeBudget, DEFAULT_INSTRUCTION_COMPUTE_UNIT_LIMIT, MAX_COMPUTE_UNIT_LIMIT,
},
solana_sdk::{
borsh0_10::try_from_slice_unchecked,
compute_budget::{self, ComputeBudgetInstruction},
feature_set::{include_loaded_accounts_data_size_in_fee_calculation, FeatureSet},
feature_set::{
add_set_tx_loaded_accounts_data_size_instruction,
include_loaded_accounts_data_size_in_fee_calculation,
remove_deprecated_request_unit_ix, FeatureSet,
},
fee::FeeStructure,
instruction::CompiledInstruction,
program_utils::limited_deserialize,
Expand Down Expand Up @@ -62,12 +62,10 @@ 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_limits: &ComputeBudgetLimits,
) -> u64 {
pub fn calculate_loaded_accounts_data_size_cost(compute_budget: &ComputeBudget) -> u64 {
FeeStructure::calculate_memory_usage_cost(
usize::try_from(compute_budget_limits.loaded_accounts_bytes).unwrap(),
DEFAULT_HEAP_COST,
compute_budget.loaded_accounts_data_size_limit,
compute_budget.heap_cost,
)
}

Expand Down Expand Up @@ -130,28 +128,32 @@ 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 process_compute_budget_instructions(
transaction.message().program_instructions_iter(),
feature_set,
) {
Ok(compute_budget_limits) => {
match result {
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 && compute_unit_limit_is_set {
bpf_costs = u64::from(compute_budget_limits.compute_unit_limit);
bpf_costs = compute_budget.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_limits);
Self::calculate_loaded_accounts_data_size_cost(&compute_budget);
}
}
Err(_) => {
Expand Down Expand Up @@ -543,8 +545,7 @@ 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_processor::MAX_LOADED_ACCOUNTS_DATA_SIZE_BYTES
as u64
solana_program_runtime::compute_budget::MAX_LOADED_ACCOUNTS_DATA_SIZE_BYTES as u64
/ ACCOUNT_DATA_COST_PAGE_SIZE
* DEFAULT_PAGE_COST;

Expand Down Expand Up @@ -662,36 +663,36 @@ mod tests {
#[allow(clippy::field_reassign_with_default)]
#[test]
fn test_calculate_loaded_accounts_data_size_cost() {
let mut compute_budget_limits = ComputeBudgetLimits::default();
let mut compute_budget = ComputeBudget::default();

// accounts data size are priced in block of 32K, ...

// ... requesting less than 32K should still be charged as one block
compute_budget_limits.loaded_accounts_bytes = 31 * 1024;
compute_budget.loaded_accounts_data_size_limit = 31_usize * 1024;
assert_eq!(
DEFAULT_HEAP_COST,
CostModel::calculate_loaded_accounts_data_size_cost(&compute_budget_limits)
compute_budget.heap_cost,
CostModel::calculate_loaded_accounts_data_size_cost(&compute_budget)
);

// ... requesting exact 32K should be charged as one block
compute_budget_limits.loaded_accounts_bytes = 32 * 1024;
compute_budget.loaded_accounts_data_size_limit = 32_usize * 1024;
assert_eq!(
DEFAULT_HEAP_COST,
CostModel::calculate_loaded_accounts_data_size_cost(&compute_budget_limits)
compute_budget.heap_cost,
CostModel::calculate_loaded_accounts_data_size_cost(&compute_budget)
);

// ... requesting slightly above 32K should be charged as 2 block
compute_budget_limits.loaded_accounts_bytes = 33 * 1024;
compute_budget.loaded_accounts_data_size_limit = 33_usize * 1024;
assert_eq!(
DEFAULT_HEAP_COST * 2,
CostModel::calculate_loaded_accounts_data_size_cost(&compute_budget_limits)
compute_budget.heap_cost * 2,
CostModel::calculate_loaded_accounts_data_size_cost(&compute_budget)
);

// ... requesting exact 64K should be charged as 2 block
compute_budget_limits.loaded_accounts_bytes = 64 * 1024;
compute_budget.loaded_accounts_data_size_limit = 64_usize * 1024;
assert_eq!(
DEFAULT_HEAP_COST * 2,
CostModel::calculate_loaded_accounts_data_size_cost(&compute_budget_limits)
compute_budget.heap_cost * 2,
CostModel::calculate_loaded_accounts_data_size_cost(&compute_budget)
);
}

Expand Down
Loading

0 comments on commit c98c24b

Please sign in to comment.