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.

Sign up for GitHub

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

Jump to bottom

cleanup feature code after mainnet-beta activation #34289

Merged
merged 2 commits into from
Dec 13, 2023
Merged

cleanup feature code after mainnet-beta activation #34289

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b7a6668
cleanup feature code after mainnet-beta activation
tao-stones Nov 30, 2023
965dafd
add comment for reserved enum
tao-stones Dec 1, 2023
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
144 changes: 6 additions & 138 deletions program-runtime/src/compute_budget_processor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,7 @@ use {
borsh0_10::try_from_slice_unchecked,
compute_budget::{self, ComputeBudgetInstruction},
entrypoint::HEAP_LENGTH as MIN_HEAP_FRAME_BYTES,
feature_set::{
add_set_tx_loaded_accounts_data_size_instruction, remove_deprecated_request_unit_ix,
FeatureSet,
},
feature_set::{add_set_tx_loaded_accounts_data_size_instruction, FeatureSet},
fee::FeeBudgetLimits,
instruction::{CompiledInstruction, InstructionError},
pubkey::Pubkey,
Expand All @@ -33,7 +30,6 @@ pub struct ComputeBudgetLimits {
pub compute_unit_limit: u32,
pub compute_unit_price: u64,
pub loaded_accounts_bytes: u32,
pub deprecated_additional_fee: Option<u64>,
}

impl Default for ComputeBudgetLimits {
Expand All @@ -43,23 +39,17 @@ impl Default for ComputeBudgetLimits {
compute_unit_limit: MAX_COMPUTE_UNIT_LIMIT,
compute_unit_price: 0,
loaded_accounts_bytes: MAX_LOADED_ACCOUNTS_DATA_SIZE_BYTES,
deprecated_additional_fee: None,
}
}
}

impl From<ComputeBudgetLimits> for FeeBudgetLimits {
fn from(val: ComputeBudgetLimits) -> Self {
let prioritization_fee =
if let Some(deprecated_additional_fee) = val.deprecated_additional_fee {
deprecated_additional_fee
} else {
let prioritization_fee_details = PrioritizationFeeDetails::new(
PrioritizationFeeType::ComputeUnitPrice(val.compute_unit_price),
u64::from(val.compute_unit_limit),
);
prioritization_fee_details.get_fee()
};
let prioritization_fee_details = PrioritizationFeeDetails::new(
PrioritizationFeeType::ComputeUnitPrice(val.compute_unit_price),
u64::from(val.compute_unit_limit),
);
let prioritization_fee = prioritization_fee_details.get_fee();

FeeBudgetLimits {
// NOTE - usize::from(u32).unwrap() may fail if target is 16-bit and
Expand All @@ -81,8 +71,6 @@ pub fn process_compute_budget_instructions<'a>(
instructions: impl Iterator<Item = (&'a Pubkey, &'a CompiledInstruction)>,
feature_set: &FeatureSet,
) -> Result<ComputeBudgetLimits, TransactionError> {
let support_request_units_deprecated =
!feature_set.is_active(&remove_deprecated_request_unit_ix::id());
let support_set_loaded_accounts_data_size_limit_ix =
feature_set.is_active(&add_set_tx_loaded_accounts_data_size_instruction::id());

Expand All @@ -91,7 +79,6 @@ pub fn process_compute_budget_instructions<'a>(
let mut updated_compute_unit_price = None;
let mut requested_heap_size = None;
let mut updated_loaded_accounts_data_size_limit = None;
let mut deprecated_additional_fee = None;

for (i, (program_id, instruction)) in instructions.enumerate() {
if compute_budget::check_id(program_id) {
Expand All @@ -102,21 +89,6 @@ pub fn process_compute_budget_instructions<'a>(
let duplicate_instruction_error = TransactionError::DuplicateInstruction(i as u8);

match try_from_slice_unchecked(&instruction.data) {
Ok(ComputeBudgetInstruction::RequestUnitsDeprecated {
units: compute_unit_limit,
additional_fee,
}) if support_request_units_deprecated => {
if updated_compute_unit_limit.is_some() {
return Err(duplicate_instruction_error);
}
if updated_compute_unit_price.is_some() {
return Err(duplicate_instruction_error);
}
updated_compute_unit_limit = Some(compute_unit_limit);
updated_compute_unit_price =
support_deprecated_requested_units(additional_fee, compute_unit_limit);
deprecated_additional_fee = Some(u64::from(additional_fee));
}
Ok(ComputeBudgetInstruction::RequestHeapFrame(bytes)) => {
if requested_heap_size.is_some() {
return Err(duplicate_instruction_error);
Expand Down Expand Up @@ -179,7 +151,6 @@ pub fn process_compute_budget_instructions<'a>(
compute_unit_limit,
compute_unit_price,
loaded_accounts_bytes,
deprecated_additional_fee,
})
}

Expand All @@ -188,17 +159,6 @@ fn sanitize_requested_heap_size(bytes: u32) -> bool {
&& bytes % 1024 == 0
}

// Supports request_units_deprecated ix, returns compute_unit_price from deprecated requested
// units.
fn support_deprecated_requested_units(additional_fee: u32, compute_unit_limit: u32) -> Option<u64> {
// TODO: remove support of 'Deprecated' after feature remove_deprecated_request_unit_ix::id() is activated
let prioritization_fee_details = PrioritizationFeeDetails::new(
PrioritizationFeeType::Deprecated(u64::from(additional_fee)),
u64::from(compute_unit_limit),
);
Some(prioritization_fee_details.get_priority())
}

#[cfg(test)]
mod tests {
use {
Expand All @@ -224,7 +184,6 @@ mod tests {
Hash::default(),
));
let mut feature_set = FeatureSet::default();
feature_set.activate(&remove_deprecated_request_unit_ix::id(), 0);
if $support_set_loaded_accounts_data_size_limit_ix {
feature_set.activate(&add_set_tx_loaded_accounts_data_size_instruction::id(), 0);
}
Expand Down Expand Up @@ -448,22 +407,6 @@ mod tests {
],
Err(TransactionError::DuplicateInstruction(2))
);

// deprecated
test!(
&[Instruction::new_with_borsh(
compute_budget::id(),
&compute_budget::ComputeBudgetInstruction::RequestUnitsDeprecated {
units: 1_000,
additional_fee: 10
},
vec![]
)],
Err(TransactionError::InstructionError(
0,
InstructionError::InvalidInstructionData,
))
);
}

#[test]
Expand Down Expand Up @@ -608,7 +551,6 @@ mod tests {
));

let mut feature_set = FeatureSet::default();
feature_set.activate(&remove_deprecated_request_unit_ix::id(), 0);
feature_set.activate(&add_set_tx_loaded_accounts_data_size_instruction::id(), 0);

let result = process_compute_budget_instructions(
Expand All @@ -627,78 +569,4 @@ mod tests {
})
);
}

fn try_prioritization_fee_from_deprecated_requested_units(
additional_fee: u32,
compute_unit_limit: u32,
) {
let payer_keypair = Keypair::new();
let tx = SanitizedTransaction::from_transaction_for_tests(Transaction::new(
&[&payer_keypair],
Message::new(
&[Instruction::new_with_borsh(
compute_budget::id(),
&compute_budget::ComputeBudgetInstruction::RequestUnitsDeprecated {
units: compute_unit_limit,
additional_fee,
},
vec![],
)],
Some(&payer_keypair.pubkey()),
),
Hash::default(),
));

// sucessfully process deprecated instruction
let compute_budget_limits = process_compute_budget_instructions(
tx.message().program_instructions_iter(),
&FeatureSet::default(),
)
.unwrap();

// assert compute_budget_limit
let expected_compute_unit_price = (additional_fee as u128)
.saturating_mul(1_000_000)
.checked_div(compute_unit_limit as u128)
.map(|cu_price| u64::try_from(cu_price).unwrap_or(u64::MAX))
.unwrap();
let expected_compute_unit_limit = compute_unit_limit.min(MAX_COMPUTE_UNIT_LIMIT);
assert_eq!(
compute_budget_limits.compute_unit_price,
expected_compute_unit_price
);
assert_eq!(
compute_budget_limits.compute_unit_limit,
expected_compute_unit_limit
);

// assert fee_budget_limits
let fee_budget_limits = FeeBudgetLimits::from(compute_budget_limits);
assert_eq!(
fee_budget_limits.prioritization_fee,
u64::from(additional_fee)
);
assert_eq!(
fee_budget_limits.compute_unit_limit,
u64::from(expected_compute_unit_limit)
);
}

#[test]
fn test_support_deprecated_requested_units() {
// a normal case
try_prioritization_fee_from_deprecated_requested_units(647, 6002);

// requesting cu limit more than MAX, div result will be round down
try_prioritization_fee_from_deprecated_requested_units(
640,
MAX_COMPUTE_UNIT_LIMIT + 606_002,
);

// requesting cu limit more than MAX, div result will round up
try_prioritization_fee_from_deprecated_requested_units(
764,
MAX_COMPUTE_UNIT_LIMIT + 606_004,
);
}
}
89 changes: 0 additions & 89 deletions program-runtime/src/prioritization_fee.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@ type MicroLamports = u128;

pub enum PrioritizationFeeType {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we plan to introduce more variants of prioritization in the near future?
If not, imo we should just get rid of this enum entirely.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I agree. Didn't want to do it in "feature cleanup" PR. Would like to do it when this pr is merged. Given it's take some time before landing the cleanup PR, dont know what's the best way to remind myself to refactor tho 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe just an issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#34301

ComputeUnitPrice(u64),
// TODO: remove 'Deprecated' after feature remove_deprecated_request_unit_ix::id() is activated
Deprecated(u64),
}

#[derive(Default, Debug, PartialEq, Eq)]
Expand All @@ -18,17 +16,6 @@ pub struct PrioritizationFeeDetails {
impl PrioritizationFeeDetails {
pub fn new(fee_type: PrioritizationFeeType, compute_unit_limit: u64) -> Self {
match fee_type {
// TODO: remove support of 'Deprecated' after feature remove_deprecated_request_unit_ix::id() is activated
PrioritizationFeeType::Deprecated(fee) => {
let micro_lamport_fee: MicroLamports =
(fee as u128).saturating_mul(MICRO_LAMPORTS_PER_LAMPORT as u128);
let priority = micro_lamport_fee
.checked_div(compute_unit_limit as u128)
.map(|priority| u64::try_from(priority).unwrap_or(u64::MAX))
.unwrap_or(0);

Self { fee, priority }
}
PrioritizationFeeType::ComputeUnitPrice(cu_price) => {
let micro_lamport_fee: MicroLamports =
(cu_price as u128).saturating_mul(compute_unit_limit as u128);
Expand Down Expand Up @@ -66,10 +53,6 @@ mod test {
FeeDetails::new(FeeType::ComputeUnitPrice(0), compute_units),
FeeDetails::default(),
);
assert_eq!(
FeeDetails::new(FeeType::Deprecated(0), compute_units),
FeeDetails::default(),
);
}
}

Expand Down Expand Up @@ -128,76 +111,4 @@ mod test {
},
);
}

#[test]
fn test_new_with_deprecated_fee() {
assert_eq!(
FeeDetails::new(FeeType::Deprecated(1), MICRO_LAMPORTS_PER_LAMPORT / 2 - 1),
FeeDetails {
fee: 1,
priority: 2,
},
"should round down fee rate of (>2.0) to priority value 1"
);

assert_eq!(
FeeDetails::new(FeeType::Deprecated(1), MICRO_LAMPORTS_PER_LAMPORT / 2),
FeeDetails {
fee: 1,
priority: 2,
},
);

assert_eq!(
FeeDetails::new(FeeType::Deprecated(1), MICRO_LAMPORTS_PER_LAMPORT / 2 + 1),
FeeDetails {
fee: 1,
priority: 1,
},
"should round down fee rate of (<2.0) to priority value 1"
);

assert_eq!(
FeeDetails::new(FeeType::Deprecated(1), MICRO_LAMPORTS_PER_LAMPORT),
FeeDetails {
fee: 1,
priority: 1,
},
);

assert_eq!(
FeeDetails::new(FeeType::Deprecated(42), 42 * MICRO_LAMPORTS_PER_LAMPORT),
FeeDetails {
fee: 42,
priority: 1,
},
);

assert_eq!(
FeeDetails::new(FeeType::Deprecated(420), 42 * MICRO_LAMPORTS_PER_LAMPORT),
FeeDetails {
fee: 420,
priority: 10,
},
);

assert_eq!(
FeeDetails::new(
FeeType::Deprecated(u64::MAX),
2 * MICRO_LAMPORTS_PER_LAMPORT
),
FeeDetails {
fee: u64::MAX,
priority: u64::MAX / 2,
},
);

assert_eq!(
FeeDetails::new(FeeType::Deprecated(u64::MAX), u64::MAX),
FeeDetails {
fee: u64::MAX,
priority: MICRO_LAMPORTS_PER_LAMPORT,
},
);
}
}
8 changes: 3 additions & 5 deletions programs/sbf/tests/programs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ use {
clock::MAX_PROCESSING_AGE,
compute_budget::ComputeBudgetInstruction,
entrypoint::MAX_PERMITTED_DATA_INCREASE,
feature_set::{self, remove_deprecated_request_unit_ix, FeatureSet},
feature_set::{self, FeatureSet},
fee::FeeStructure,
loader_instruction,
message::{v0::LoadedAddresses, SanitizedMessage},
Expand Down Expand Up @@ -3831,8 +3831,7 @@ fn test_program_fees() {
Some(&mint_keypair.pubkey()),
);

let mut feature_set = FeatureSet::all_enabled();
feature_set.deactivate(&remove_deprecated_request_unit_ix::id());
let feature_set = FeatureSet::all_enabled();

let sanitized_message = SanitizedMessage::try_from(message.clone()).unwrap();
let expected_normal_fee = fee_structure.calculate_fee(
Expand Down Expand Up @@ -3862,8 +3861,7 @@ fn test_program_fees() {
Some(&mint_keypair.pubkey()),
);
let sanitized_message = SanitizedMessage::try_from(message.clone()).unwrap();
let mut feature_set = FeatureSet::all_enabled();
feature_set.deactivate(&remove_deprecated_request_unit_ix::id());
let feature_set = FeatureSet::all_enabled();
let expected_prioritized_fee = fee_structure.calculate_fee(
&sanitized_message,
congestion_multiplier,
Expand Down
6 changes: 2 additions & 4 deletions runtime/src/accounts/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -750,8 +750,7 @@ mod tests {
instructions,
);

let mut feature_set = FeatureSet::all_enabled();
feature_set.deactivate(&solana_sdk::feature_set::remove_deprecated_request_unit_ix::id());
let feature_set = FeatureSet::all_enabled();

let message = SanitizedMessage::try_from(tx.message().clone()).unwrap();
let fee = FeeStructure::default().calculate_fee(
Expand Down Expand Up @@ -1621,8 +1620,7 @@ mod tests {
Hash::default(),
);

let mut feature_set = FeatureSet::all_enabled();
feature_set.deactivate(&solana_sdk::feature_set::remove_deprecated_request_unit_ix::id());
let feature_set = FeatureSet::all_enabled();

let message = SanitizedMessage::try_from(tx.message().clone()).unwrap();
let fee = FeeStructure::default().calculate_fee(
Expand Down
Loading