From 28113aaa56aa47514c8c6f3805d607d4d0b41256 Mon Sep 17 00:00:00 2001 From: Tao Zhu Date: Mon, 9 Dec 2024 21:42:17 -0600 Subject: [PATCH] use enum to separately define migrating and not-migrating builtins --- builtins-default-costs/Cargo.toml | 1 + builtins-default-costs/src/lib.rs | 210 +++++++++++++------------- compute-budget-instruction/Cargo.toml | 1 + 3 files changed, 104 insertions(+), 108 deletions(-) diff --git a/builtins-default-costs/Cargo.toml b/builtins-default-costs/Cargo.toml index 282a3d9dec25ea..986ef59c9fcc7d 100644 --- a/builtins-default-costs/Cargo.toml +++ b/builtins-default-costs/Cargo.toml @@ -45,6 +45,7 @@ frozen-abi = [ "dep:solana-frozen-abi", "solana-vote-program/frozen-abi", ] +dev-context-only-utils = [] [lints] workspace = true diff --git a/builtins-default-costs/src/lib.rs b/builtins-default-costs/src/lib.rs index ef313cdd5358e0..a1c4000a75499d 100644 --- a/builtins-default-costs/src/lib.rs +++ b/builtins-default-costs/src/lib.rs @@ -1,5 +1,4 @@ #![cfg_attr(feature = "frozen-abi", feature(min_specialization))] -#![feature(const_option)] #![allow(clippy::arithmetic_side_effects)] use { ahash::AHashMap, @@ -14,8 +13,9 @@ use { }; #[derive(Clone)] -struct CoreBpfMigrationFeature { - feature_id: Pubkey, +pub struct MigratingBuiltinCost { + native_cost: u64, + core_bpf_migration_feature: Pubkey, // encoding positional information explicitly for migration feature item, // its value must be correctly corresponding to this object's position // in MIGRATING_BUILTINS_COSTS, otherwise a const validation @@ -23,6 +23,11 @@ struct CoreBpfMigrationFeature { position: usize, } +#[derive(Clone)] +pub struct NotMigratingBuiltinCost { + native_cost: u64, +} + /// DEVELOPER: when a builtin is migrated to sbpf, please add its corresponding /// migration feature ID to BUILTIN_INSTRUCTION_COSTS, and move it from /// NON_MIGRATING_BUILTINS_COSTS to MIGRATING_BUILTINS_COSTS, so the builtin's @@ -30,30 +35,46 @@ struct CoreBpfMigrationFeature { /// When migration completed, eg the feature gate is enabled everywhere, please /// remove that builtin entry from MIGRATING_BUILTINS_COSTS. #[derive(Clone)] -pub struct BuiltinCost { - native_cost: u64, - core_bpf_migration_feature: Option, +pub enum BuiltinCost { + Migrating(MigratingBuiltinCost), + NotMigrating(NotMigratingBuiltinCost), } -/// const function validates `position` correctness at compile time. -#[allow(dead_code)] -const fn validate_position(migrating_builtins: &[(Pubkey, BuiltinCost)]) { - let mut index = 0; - while index < migrating_builtins.len() { - assert!( - migrating_builtins[index] - .1 - .core_bpf_migration_feature - .as_ref() - .unwrap() - .position - == index, - "migration feture must exist and at correct position" - ); - index += 1; +impl BuiltinCost { + pub fn native_cost(&self) -> u64 { + match self { + BuiltinCost::Migrating(MigratingBuiltinCost { native_cost, .. }) => *native_cost, + BuiltinCost::NotMigrating(NotMigratingBuiltinCost { native_cost }) => *native_cost, + } + } + + pub fn core_bpf_migration_feature(&self) -> Option<&Pubkey> { + match self { + BuiltinCost::Migrating(MigratingBuiltinCost { + core_bpf_migration_feature, + .. + }) => Some(core_bpf_migration_feature), + BuiltinCost::NotMigrating(_) => None, + } + } + + pub fn position(&self) -> Option { + match self { + BuiltinCost::Migrating(MigratingBuiltinCost { position, .. }) => Some(*position), + BuiltinCost::NotMigrating(_) => None, + } + } + + fn has_migrated(&self, feature_set: &FeatureSet) -> bool { + match self { + BuiltinCost::Migrating(MigratingBuiltinCost { + core_bpf_migration_feature, + .. + }) => feature_set.is_active(core_bpf_migration_feature), + BuiltinCost::NotMigrating(_) => false, + } } } -const _: () = validate_position(MIGRATING_BUILTINS_COSTS); lazy_static! { /// Number of compute units for each built-in programs @@ -91,100 +112,82 @@ static_assertions::const_assert_eq!( pub const MIGRATING_BUILTINS_COSTS: &[(Pubkey, BuiltinCost)] = &[ ( stake::id(), - BuiltinCost { + BuiltinCost::Migrating(MigratingBuiltinCost { native_cost: solana_stake_program::stake_instruction::DEFAULT_COMPUTE_UNITS, - core_bpf_migration_feature: Some(CoreBpfMigrationFeature { - feature_id: feature_set::migrate_stake_program_to_core_bpf::id(), - position: 0, - }), - }, + core_bpf_migration_feature: feature_set::migrate_stake_program_to_core_bpf::id(), + position: 0, + }), ), ( config::id(), - BuiltinCost { + BuiltinCost::Migrating(MigratingBuiltinCost { native_cost: solana_config_program::config_processor::DEFAULT_COMPUTE_UNITS, - core_bpf_migration_feature: Some(CoreBpfMigrationFeature { - feature_id: feature_set::migrate_config_program_to_core_bpf::id(), - position: 1, - }), - }, + core_bpf_migration_feature: feature_set::migrate_config_program_to_core_bpf::id(), + position: 1, + }), ), ( address_lookup_table::id(), - BuiltinCost { + BuiltinCost::Migrating(MigratingBuiltinCost { native_cost: solana_address_lookup_table_program::processor::DEFAULT_COMPUTE_UNITS, - core_bpf_migration_feature: Some(CoreBpfMigrationFeature { - feature_id: feature_set::migrate_address_lookup_table_program_to_core_bpf::id(), - position: 2, - }), - }, + core_bpf_migration_feature: + feature_set::migrate_address_lookup_table_program_to_core_bpf::id(), + position: 2, + }), ), ]; pub const NON_MIGRATING_BUILTINS_COSTS: &[(Pubkey, BuiltinCost)] = &[ ( vote::id(), - BuiltinCost { + BuiltinCost::NotMigrating(NotMigratingBuiltinCost { native_cost: solana_vote_program::vote_processor::DEFAULT_COMPUTE_UNITS, - core_bpf_migration_feature: None, - }, + }), ), ( system_program::id(), - BuiltinCost { + BuiltinCost::NotMigrating(NotMigratingBuiltinCost { native_cost: solana_system_program::system_processor::DEFAULT_COMPUTE_UNITS, - core_bpf_migration_feature: None, - }, + }), ), ( compute_budget::id(), - BuiltinCost { + BuiltinCost::NotMigrating(NotMigratingBuiltinCost { native_cost: solana_compute_budget_program::DEFAULT_COMPUTE_UNITS, - core_bpf_migration_feature: None, - }, + }), ), ( bpf_loader_upgradeable::id(), - BuiltinCost { + BuiltinCost::NotMigrating(NotMigratingBuiltinCost { native_cost: solana_bpf_loader_program::UPGRADEABLE_LOADER_COMPUTE_UNITS, - core_bpf_migration_feature: None, - }, + }), ), ( bpf_loader_deprecated::id(), - BuiltinCost { + BuiltinCost::NotMigrating(NotMigratingBuiltinCost { native_cost: solana_bpf_loader_program::DEPRECATED_LOADER_COMPUTE_UNITS, - core_bpf_migration_feature: None, - }, + }), ), ( bpf_loader::id(), - BuiltinCost { + BuiltinCost::NotMigrating(NotMigratingBuiltinCost { native_cost: solana_bpf_loader_program::DEFAULT_LOADER_COMPUTE_UNITS, - core_bpf_migration_feature: None, - }, + }), ), ( loader_v4::id(), - BuiltinCost { + BuiltinCost::NotMigrating(NotMigratingBuiltinCost { native_cost: solana_loader_v4_program::DEFAULT_COMPUTE_UNITS, - core_bpf_migration_feature: None, - }, + }), ), // Note: These are precompile, run directly in bank during sanitizing; ( secp256k1_program::id(), - BuiltinCost { - native_cost: 0, - core_bpf_migration_feature: None, - }, + BuiltinCost::NotMigrating(NotMigratingBuiltinCost { native_cost: 0 }), ), ( ed25519_program::id(), - BuiltinCost { - native_cost: 0, - core_bpf_migration_feature: None, - }, + BuiltinCost::NotMigrating(NotMigratingBuiltinCost { native_cost: 0 }), ), ]; @@ -208,18 +211,8 @@ pub fn get_builtin_instruction_cost<'a>( ) -> Option { BUILTIN_INSTRUCTION_COSTS .get(program_id) - .filter( - // Returns true if builtin program id has no core_bpf_migration_feature or feature is not activated; - // otherwise returns false because it's not considered as builtin - |builtin_cost| -> bool { - builtin_cost - .core_bpf_migration_feature - .as_ref() - .map(|migration_feature| !feature_set.is_active(&migration_feature.feature_id)) - .unwrap_or(true) - }, - ) - .map(|builtin_cost| builtin_cost.native_cost) + .filter(|builtin_cost| !builtin_cost.has_migrated(feature_set)) + .map(|builtin_cost| builtin_cost.native_cost()) } pub enum BuiltinMigrationFeatureIndex { @@ -228,50 +221,53 @@ pub enum BuiltinMigrationFeatureIndex { BuiltinWithMigrationFeature(usize), } -/// Given a program pubkey, returns: -/// - None, if it is not in BUILTIN_INSTRUCTION_COSTS dictionary; -/// - Some, is builtin, but no associated migration feature ID; -/// - Some, is builtin, and its associated migration feature ID -/// index in MIGRATION_FEATURES_ID. pub fn get_builtin_migration_feature_index(program_id: &Pubkey) -> BuiltinMigrationFeatureIndex { BUILTIN_INSTRUCTION_COSTS.get(program_id).map_or( BuiltinMigrationFeatureIndex::NotBuiltin, |builtin_cost| { - builtin_cost.core_bpf_migration_feature.as_ref().map_or( + builtin_cost.position().map_or( BuiltinMigrationFeatureIndex::BuiltinNoMigrationFeature, - |migration_feature| { - BuiltinMigrationFeatureIndex::BuiltinWithMigrationFeature( - migration_feature.position, - ) - }, + BuiltinMigrationFeatureIndex::BuiltinWithMigrationFeature, ) }, ) } +/// const function validates `position` correctness at compile time. +#[allow(dead_code)] +const fn validate_position(migrating_builtins: &[(Pubkey, BuiltinCost)]) { + let mut index = 0; + while index < migrating_builtins.len() { + match migrating_builtins[index].1 { + BuiltinCost::Migrating(MigratingBuiltinCost { position, .. }) => assert!( + position == index, + "migration feture must exist and at correct position" + ), + BuiltinCost::NotMigrating(_) => { + panic!("migration feture must exist and at correct position") + } + } + index += 1; + } +} +const _: () = validate_position(MIGRATING_BUILTINS_COSTS); + /// Helper function to return ref of migration feature Pubkey at position `index` /// from MIGRATING_BUILTINS_COSTS pub fn get_migration_feature_id(index: usize) -> &'static Pubkey { - &MIGRATING_BUILTINS_COSTS + MIGRATING_BUILTINS_COSTS .get(index) .expect("valid index of MIGRATING_BUILTINS_COSTS") .1 - .core_bpf_migration_feature - .as_ref() + .core_bpf_migration_feature() .expect("migrating builtin") - .feature_id } +#[cfg(feature = "dev-context-only-utils")] pub fn get_migration_feature_position(feature_id: &Pubkey) -> usize { MIGRATING_BUILTINS_COSTS .iter() - .position(|(_, c)| { - c.core_bpf_migration_feature - .as_ref() - .map(|f| f.feature_id) - .unwrap() - == *feature_id - }) + .position(|(_, c)| c.core_bpf_migration_feature().expect("migrating builtin") == feature_id) .unwrap() } @@ -286,13 +282,11 @@ mod test { .iter() .enumerate() .all(|(index, (_, c))| { - let migration_feature = &c.core_bpf_migration_feature; - migration_feature.is_some() - && migration_feature.as_ref().map(|f| f.position) == Some(index) + c.core_bpf_migration_feature().is_some() && c.position() == Some(index) })); assert!(NON_MIGRATING_BUILTINS_COSTS .iter() - .all(|(_, c)| c.core_bpf_migration_feature.is_none())); + .all(|(_, c)| c.core_bpf_migration_feature().is_none())); } #[test] diff --git a/compute-budget-instruction/Cargo.toml b/compute-budget-instruction/Cargo.toml index b8daccad80d7c5..8e5bdd0b1c9bf7 100644 --- a/compute-budget-instruction/Cargo.toml +++ b/compute-budget-instruction/Cargo.toml @@ -26,6 +26,7 @@ name = "solana_compute_budget_instruction" bincode = { workspace = true } criterion = { workspace = true } rand = { workspace = true } +solana-builtins-default-costs = { workspace = true, features = ["dev-context-only-utils"] } solana-program = { workspace = true } [package.metadata.docs.rs]