Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: rename compute_budget_instruction_details for accuracy #4409

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ impl Default for MigrationBuiltinFeatureCounter {
#[cfg_attr(test, derive(Eq, PartialEq))]
#[cfg_attr(feature = "dev-context-only-utils", derive(Clone))]
#[derive(Default, Debug)]
pub struct ComputeBudgetInstructionDetails {
pub struct InstructionDetails {
// compute-budget instruction details:
// the first field in tuple is instruction index, second field is the unsanitized value set by user
requested_compute_unit_limit: Option<(u8, u32)>,
Expand All @@ -50,12 +50,12 @@ pub struct ComputeBudgetInstructionDetails {
migrating_builtin_feature_counters: MigrationBuiltinFeatureCounter,
}

impl ComputeBudgetInstructionDetails {
impl InstructionDetails {
pub fn try_from<'a>(
instructions: impl Iterator<Item = (&'a Pubkey, SVMInstruction<'a>)> + Clone,
) -> Result<Self> {
let mut filter = ComputeBudgetProgramIdFilter::new();
let mut compute_budget_instruction_details = ComputeBudgetInstructionDetails::default();
let mut compute_budget_instruction_details = InstructionDetails::default();

for (i, (program_id, instruction)) in instructions.clone().enumerate() {
if filter.is_compute_budget_program(instruction.program_id_index as usize, program_id) {
Expand Down Expand Up @@ -253,15 +253,15 @@ mod test {
ComputeBudgetInstruction::request_heap_frame(40 * 1024),
Instruction::new_with_bincode(Pubkey::new_unique(), &(), vec![]),
]);
let expected_details = Ok(ComputeBudgetInstructionDetails {
let expected_details = Ok(InstructionDetails {
requested_heap_size: Some((1, 40 * 1024)),
num_non_compute_budget_instructions: Saturating(2),
num_non_migratable_builtin_instructions: Saturating(1),
num_non_builtin_instructions: Saturating(2),
..ComputeBudgetInstructionDetails::default()
..InstructionDetails::default()
});
assert_eq!(
ComputeBudgetInstructionDetails::try_from(SVMMessage::program_instructions_iter(&tx),),
InstructionDetails::try_from(SVMMessage::program_instructions_iter(&tx),),
expected_details
);

Expand All @@ -271,7 +271,7 @@ mod test {
ComputeBudgetInstruction::request_heap_frame(41 * 1024),
]);
assert_eq!(
ComputeBudgetInstructionDetails::try_from(SVMMessage::program_instructions_iter(&tx),),
InstructionDetails::try_from(SVMMessage::program_instructions_iter(&tx),),
Err(TransactionError::DuplicateInstruction(2))
);
}
Expand All @@ -283,13 +283,13 @@ mod test {
ComputeBudgetInstruction::set_compute_unit_limit(u32::MAX),
Instruction::new_with_bincode(Pubkey::new_unique(), &(), vec![]),
]);
let expected_details = Ok(ComputeBudgetInstructionDetails {
let expected_details = Ok(InstructionDetails {
requested_compute_unit_limit: Some((1, u32::MAX)),
num_non_compute_budget_instructions: Saturating(2),
..ComputeBudgetInstructionDetails::default()
..InstructionDetails::default()
});
assert_eq!(
ComputeBudgetInstructionDetails::try_from(SVMMessage::program_instructions_iter(&tx),),
InstructionDetails::try_from(SVMMessage::program_instructions_iter(&tx),),
expected_details
);

Expand All @@ -299,7 +299,7 @@ mod test {
ComputeBudgetInstruction::set_compute_unit_limit(u32::MAX),
]);
assert_eq!(
ComputeBudgetInstructionDetails::try_from(SVMMessage::program_instructions_iter(&tx),),
InstructionDetails::try_from(SVMMessage::program_instructions_iter(&tx),),
Err(TransactionError::DuplicateInstruction(2))
);
}
Expand All @@ -311,15 +311,15 @@ mod test {
ComputeBudgetInstruction::set_compute_unit_price(u64::MAX),
Instruction::new_with_bincode(Pubkey::new_unique(), &(), vec![]),
]);
let expected_details = Ok(ComputeBudgetInstructionDetails {
let expected_details = Ok(InstructionDetails {
requested_compute_unit_price: Some((1, u64::MAX)),
num_non_compute_budget_instructions: Saturating(2),
num_non_migratable_builtin_instructions: Saturating(1),
num_non_builtin_instructions: Saturating(2),
..ComputeBudgetInstructionDetails::default()
..InstructionDetails::default()
});
assert_eq!(
ComputeBudgetInstructionDetails::try_from(SVMMessage::program_instructions_iter(&tx),),
InstructionDetails::try_from(SVMMessage::program_instructions_iter(&tx),),
expected_details
);

Expand All @@ -329,7 +329,7 @@ mod test {
ComputeBudgetInstruction::set_compute_unit_price(u64::MAX),
]);
assert_eq!(
ComputeBudgetInstructionDetails::try_from(SVMMessage::program_instructions_iter(&tx),),
InstructionDetails::try_from(SVMMessage::program_instructions_iter(&tx),),
Err(TransactionError::DuplicateInstruction(2))
);
}
Expand All @@ -341,15 +341,15 @@ mod test {
ComputeBudgetInstruction::set_loaded_accounts_data_size_limit(u32::MAX),
Instruction::new_with_bincode(Pubkey::new_unique(), &(), vec![]),
]);
let expected_details = Ok(ComputeBudgetInstructionDetails {
let expected_details = Ok(InstructionDetails {
requested_loaded_accounts_data_size_limit: Some((1, u32::MAX)),
num_non_compute_budget_instructions: Saturating(2),
num_non_migratable_builtin_instructions: Saturating(1),
num_non_builtin_instructions: Saturating(2),
..ComputeBudgetInstructionDetails::default()
..InstructionDetails::default()
});
assert_eq!(
ComputeBudgetInstructionDetails::try_from(SVMMessage::program_instructions_iter(&tx),),
InstructionDetails::try_from(SVMMessage::program_instructions_iter(&tx),),
expected_details
);

Expand All @@ -359,17 +359,17 @@ mod test {
ComputeBudgetInstruction::set_loaded_accounts_data_size_limit(u32::MAX),
]);
assert_eq!(
ComputeBudgetInstructionDetails::try_from(SVMMessage::program_instructions_iter(&tx),),
InstructionDetails::try_from(SVMMessage::program_instructions_iter(&tx),),
Err(TransactionError::DuplicateInstruction(2))
);
}

fn prep_feature_minimial_cus_for_builtin_instructions(
is_active: bool,
instruction_details: &ComputeBudgetInstructionDetails,
instruction_details: &InstructionDetails,
) -> (FeatureSet, u32) {
let mut feature_set = FeatureSet::default();
let ComputeBudgetInstructionDetails {
let InstructionDetails {
num_non_compute_budget_instructions,
num_non_migratable_builtin_instructions,
num_non_builtin_instructions,
Expand All @@ -394,7 +394,7 @@ mod test {
#[test]
fn test_sanitize_and_convert_to_compute_budget_limits() {
// empty details, default ComputeBudgetLimits with 0 compute_unit_limits
let instruction_details = ComputeBudgetInstructionDetails::default();
let instruction_details = InstructionDetails::default();
assert_eq!(
instruction_details
.sanitize_and_convert_to_compute_budget_limits(&FeatureSet::default()),
Expand All @@ -405,11 +405,11 @@ mod test {
);

// no compute-budget instructions, all default ComputeBudgetLimits except cu-limit
let instruction_details = ComputeBudgetInstructionDetails {
let instruction_details = InstructionDetails {
num_non_compute_budget_instructions: Saturating(4),
num_non_migratable_builtin_instructions: Saturating(1),
num_non_builtin_instructions: Saturating(3),
..ComputeBudgetInstructionDetails::default()
..InstructionDetails::default()
};
for is_active in [true, false] {
let (feature_set, expected_compute_unit_limit) =
Expand All @@ -428,12 +428,12 @@ mod test {
InstructionError::InvalidInstructionData,
));
// invalid: requested_heap_size can't be zero
let instruction_details = ComputeBudgetInstructionDetails {
let instruction_details = InstructionDetails {
requested_compute_unit_limit: Some((1, 0)),
requested_compute_unit_price: Some((2, 0)),
requested_heap_size: Some((3, 0)),
requested_loaded_accounts_data_size_limit: Some((4, 1024)),
..ComputeBudgetInstructionDetails::default()
..InstructionDetails::default()
};
for is_active in [true, false] {
let (feature_set, _expected_compute_unit_limit) =
Expand All @@ -445,12 +445,12 @@ mod test {
}

// invalid: requested_heap_size can't be less than MIN_HEAP_FRAME_BYTES
let instruction_details = ComputeBudgetInstructionDetails {
let instruction_details = InstructionDetails {
requested_compute_unit_limit: Some((1, 0)),
requested_compute_unit_price: Some((2, 0)),
requested_heap_size: Some((3, MIN_HEAP_FRAME_BYTES - 1)),
requested_loaded_accounts_data_size_limit: Some((4, 1024)),
..ComputeBudgetInstructionDetails::default()
..InstructionDetails::default()
};
for is_active in [true, false] {
let (feature_set, _expected_compute_unit_limit) =
Expand All @@ -462,12 +462,12 @@ mod test {
}

// invalid: requested_heap_size can't be more than MAX_HEAP_FRAME_BYTES
let instruction_details = ComputeBudgetInstructionDetails {
let instruction_details = InstructionDetails {
requested_compute_unit_limit: Some((1, 0)),
requested_compute_unit_price: Some((2, 0)),
requested_heap_size: Some((3, MAX_HEAP_FRAME_BYTES + 1)),
requested_loaded_accounts_data_size_limit: Some((4, 1024)),
..ComputeBudgetInstructionDetails::default()
..InstructionDetails::default()
};
for is_active in [true, false] {
let (feature_set, _expected_compute_unit_limit) =
Expand All @@ -479,12 +479,12 @@ mod test {
}

// invalid: requested_heap_size must be round by 1024
let instruction_details = ComputeBudgetInstructionDetails {
let instruction_details = InstructionDetails {
requested_compute_unit_limit: Some((1, 0)),
requested_compute_unit_price: Some((2, 0)),
requested_heap_size: Some((3, MIN_HEAP_FRAME_BYTES + 1024 + 1)),
requested_loaded_accounts_data_size_limit: Some((4, 1024)),
..ComputeBudgetInstructionDetails::default()
..InstructionDetails::default()
};
for is_active in [true, false] {
let (feature_set, _expected_compute_unit_limit) =
Expand All @@ -496,12 +496,12 @@ mod test {
}

// invalid: loaded_account_data_size can't be zero
let instruction_details = ComputeBudgetInstructionDetails {
let instruction_details = InstructionDetails {
requested_compute_unit_limit: Some((1, 0)),
requested_compute_unit_price: Some((2, 0)),
requested_heap_size: Some((3, 40 * 1024)),
requested_loaded_accounts_data_size_limit: Some((4, 0)),
..ComputeBudgetInstructionDetails::default()
..InstructionDetails::default()
};
for is_active in [true, false] {
let (feature_set, _expected_compute_unit_limit) =
Expand All @@ -513,13 +513,13 @@ mod test {
}

// valid: acceptable MAX
let instruction_details = ComputeBudgetInstructionDetails {
let instruction_details = InstructionDetails {
requested_compute_unit_limit: Some((1, u32::MAX)),
requested_compute_unit_price: Some((2, u64::MAX)),
requested_heap_size: Some((3, MAX_HEAP_FRAME_BYTES)),
requested_loaded_accounts_data_size_limit: Some((4, u32::MAX)),
num_non_compute_budget_instructions: Saturating(4),
..ComputeBudgetInstructionDetails::default()
..InstructionDetails::default()
};
for is_active in [true, false] {
let (feature_set, _expected_compute_unit_limit) =
Expand All @@ -537,12 +537,12 @@ mod test {

// valid
let val: u32 = 1024 * 40;
let instruction_details = ComputeBudgetInstructionDetails {
let instruction_details = InstructionDetails {
requested_compute_unit_limit: Some((1, val)),
requested_compute_unit_price: Some((2, val as u64)),
requested_heap_size: Some((3, val)),
requested_loaded_accounts_data_size_limit: Some((4, val)),
..ComputeBudgetInstructionDetails::default()
..InstructionDetails::default()
};
for is_active in [true, false] {
let (feature_set, _expected_compute_unit_limit) =
Expand Down Expand Up @@ -571,17 +571,16 @@ mod test {
]);
let feature_id_index =
get_migration_feature_position(&feature_set::migrate_stake_program_to_core_bpf::id());
let mut expected_details = ComputeBudgetInstructionDetails {
let mut expected_details = InstructionDetails {
num_non_compute_budget_instructions: Saturating(2),
num_non_builtin_instructions: Saturating(1),
..ComputeBudgetInstructionDetails::default()
..InstructionDetails::default()
};
expected_details
.migrating_builtin_feature_counters
.migrating_builtin[feature_id_index] = Saturating(1);
let expected_details = Ok(expected_details);
let details =
ComputeBudgetInstructionDetails::try_from(SVMMessage::program_instructions_iter(&tx));
let details = InstructionDetails::try_from(SVMMessage::program_instructions_iter(&tx));
assert_eq!(details, expected_details);
let details = details.unwrap();

Expand Down
4 changes: 2 additions & 2 deletions compute-budget-instruction/src/instructions_processor.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use {
crate::compute_budget_instruction_details::*, solana_compute_budget::compute_budget_limits::*,
crate::instruction_details::*, solana_compute_budget::compute_budget_limits::*,
solana_feature_set::FeatureSet, solana_pubkey::Pubkey,
solana_svm_transaction::instruction::SVMInstruction,
solana_transaction_error::TransactionError,
Expand All @@ -14,7 +14,7 @@ pub fn process_compute_budget_instructions<'a>(
instructions: impl Iterator<Item = (&'a Pubkey, SVMInstruction<'a>)> + Clone,
feature_set: &FeatureSet,
) -> Result<ComputeBudgetLimits, TransactionError> {
ComputeBudgetInstructionDetails::try_from(instructions)?
InstructionDetails::try_from(instructions)?
.sanitize_and_convert_to_compute_budget_limits(feature_set)
}

Expand Down
2 changes: 1 addition & 1 deletion compute-budget-instruction/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
#![allow(clippy::arithmetic_side_effects)]

mod builtin_programs_filter;
pub mod compute_budget_instruction_details;
mod compute_budget_program_id_filter;
pub mod instruction_details;
pub mod instructions_processor;
4 changes: 2 additions & 2 deletions core/src/banking_stage/consumer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -574,7 +574,7 @@ impl Consumer {
.iter()
.filter_map(|transaction| {
transaction
.compute_budget_instruction_details()
.instruction_details()
.sanitize_and_convert_to_compute_budget_limits(&bank.feature_set)
.ok()
.map(|limits| limits.compute_unit_price)
Expand Down Expand Up @@ -759,7 +759,7 @@ impl Consumer {
let fee_payer = transaction.fee_payer();
let fee_budget_limits = FeeBudgetLimits::from(
transaction
.compute_budget_instruction_details()
.instruction_details()
.sanitize_and_convert_to_compute_budget_limits(&bank.feature_set)?,
);
let fee = solana_fee::calculate_fee(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ impl SanitizedTransactionReceiveAndBuffer {
.is_ok()
})
.filter_map(|(packet, tx, deactivation_slot)| {
tx.compute_budget_instruction_details()
tx.instruction_details()
.sanitize_and_convert_to_compute_budget_limits(&working_bank.feature_set)
.map(|compute_budget| {
(packet, tx, deactivation_slot, compute_budget.into())
Expand Down
4 changes: 2 additions & 2 deletions cost-model/src/cost_model.rs
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,7 @@ impl CostModel {
// will not be executed by `bank`, therefore it should be considered
// as no execution cost by cost model.
match meta
.compute_budget_instruction_details()
.instruction_details()
.sanitize_and_convert_to_compute_budget_limits(feature_set)
{
Ok(compute_budget_limits) => {
Expand Down Expand Up @@ -293,7 +293,7 @@ impl CostModel {
// 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.
let (programs_execution_costs, loaded_accounts_data_size_cost) = match transaction
.compute_budget_instruction_details()
.instruction_details()
.sanitize_and_convert_to_compute_budget_limits(feature_set)
{
Ok(compute_budget_limits) => (
Expand Down
6 changes: 3 additions & 3 deletions cost-model/src/transaction_cost.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
#[cfg(feature = "dev-context-only-utils")]
use solana_compute_budget_instruction::compute_budget_instruction_details::ComputeBudgetInstructionDetails;
use solana_compute_budget_instruction::instruction_details::InstructionDetails;
use {
crate::block_cost_limits, solana_pubkey::Pubkey,
solana_runtime_transaction::transaction_meta::StaticMeta,
Expand Down Expand Up @@ -265,8 +265,8 @@ impl solana_runtime_transaction::transaction_meta::StaticMeta for WritableKeysTr
&DUMMY
}

fn compute_budget_instruction_details(&self) -> &ComputeBudgetInstructionDetails {
unimplemented!("WritableKeysTransaction::compute_budget_instruction_details")
fn instruction_details(&self) -> &InstructionDetails {
unimplemented!("WritableKeysTransaction::instruction_details")
}
}

Expand Down
Loading
Loading