diff --git a/ledger/src/builtins.rs b/ledger/src/builtins.rs index c9ef1cc9c..64a2e4648 100644 --- a/ledger/src/builtins.rs +++ b/ledger/src/builtins.rs @@ -1,7 +1,4 @@ -use { - solana_runtime::builtins::{ActivationType, Builtin, Builtins}, - solana_sdk::pubkey::Pubkey, -}; +use solana_runtime::builtins::{Builtin, BuiltinFeatureTransition, Builtins}; macro_rules! to_builtin { ($b:expr) => { @@ -37,14 +34,14 @@ fn genesis_builtins(bpf_jit: bool) -> Vec { ] } -/// Builtin programs activated dynamically by feature -fn feature_builtins() -> Vec<(Builtin, Pubkey, ActivationType)> { +/// Dynamic feature transitions for builtin programs +fn builtin_feature_transitions() -> Vec { vec![] } pub(crate) fn get(bpf_jit: bool) -> Builtins { Builtins { genesis_builtins: genesis_builtins(bpf_jit), - feature_builtins: feature_builtins(), + feature_transitions: builtin_feature_transitions(), } } diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 9fb3ec1c1..fedfd6d61 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -46,7 +46,7 @@ use { accounts_update_notifier_interface::AccountsUpdateNotifier, ancestors::{Ancestors, AncestorsForSerialization}, blockhash_queue::BlockhashQueue, - builtins::{self, ActivationType, Builtin, Builtins}, + builtins::{self, BuiltinAction, BuiltinFeatureTransition, Builtins}, cost_tracker::CostTracker, epoch_stakes::{EpochStakes, NodeVoteAccounts}, inline_spl_associated_token_account, inline_spl_token, @@ -157,6 +157,7 @@ use { }; mod address_lookup_table; +mod builtin_programs; mod sysvar_cache; mod transaction_account_state_info; @@ -1196,9 +1197,9 @@ pub struct Bank { compute_budget: Option, - /// Builtin programs activated dynamically by feature + /// Dynamic feature transitions for builtin programs #[allow(clippy::rc_buffer)] - feature_builtins: Arc>, + builtin_feature_transitions: Arc>, /// Protocol-level rewards that were distributed by this bank pub rewards: RwLock>, @@ -1367,7 +1368,7 @@ impl Bank { is_delta: AtomicBool::default(), builtin_programs: BuiltinPrograms::default(), compute_budget: Option::::default(), - feature_builtins: Arc::>::default(), + builtin_feature_transitions: Arc::>::default(), rewards: RwLock::>::default(), cluster_type: Option::::default(), lazy_rent_collection: AtomicBool::default(), @@ -1703,7 +1704,7 @@ impl Bank { signature_count: AtomicU64::new(0), builtin_programs, compute_budget: parent.compute_budget, - feature_builtins: parent.feature_builtins.clone(), + builtin_feature_transitions: parent.builtin_feature_transitions.clone(), hard_forks: parent.hard_forks.clone(), rewards: RwLock::new(vec![]), cluster_type: parent.cluster_type, @@ -1989,7 +1990,7 @@ impl Bank { is_delta: AtomicBool::new(fields.is_delta), builtin_programs: new(), compute_budget: None, - feature_builtins: new(), + builtin_feature_transitions: new(), rewards: new(), cluster_type: Some(genesis_config.cluster_type), lazy_rent_collection: new(), @@ -5514,8 +5515,8 @@ impl Bank { .genesis_builtins .extend_from_slice(&additional_builtins.genesis_builtins); builtins - .feature_builtins - .extend_from_slice(&additional_builtins.feature_builtins); + .feature_transitions + .extend_from_slice(&additional_builtins.feature_transitions); } if !debug_do_not_add_builtins { for builtin in builtins.genesis_builtins { @@ -5531,7 +5532,7 @@ impl Bank { } } } - self.feature_builtins = Arc::new(builtins.feature_builtins); + self.builtin_feature_transitions = Arc::new(builtins.feature_transitions); self.apply_feature_activations(true, debug_do_not_add_builtins); } @@ -6238,29 +6239,9 @@ impl Bank { debug!("Added program {} under {:?}", name, program_id); } - /// Replace a builtin instruction processor if it already exists - pub fn replace_builtin( - &mut self, - name: &str, - program_id: &Pubkey, - process_instruction: ProcessInstructionWithContext, - ) { - debug!("Replacing program {} under {:?}", name, program_id); - self.add_builtin_account(name, program_id, true); - if let Some(entry) = self - .builtin_programs - .vec - .iter_mut() - .find(|entry| entry.program_id == *program_id) - { - entry.process_instruction = process_instruction; - } - debug!("Replaced program {} under {:?}", name, program_id); - } - /// Remove a builtin instruction processor if it already exists - pub fn remove_builtin(&mut self, name: &str, program_id: &Pubkey) { - debug!("Removing program {} under {:?}", name, program_id); + pub fn remove_builtin(&mut self, program_id: &Pubkey) { + debug!("Removing program {}", program_id); // Don't remove the account since the bank expects the account state to // be idempotent if let Some(position) = self @@ -6271,7 +6252,7 @@ impl Bank { { self.builtin_programs.vec.remove(position); } - debug!("Removed program {} under {:?}", name, program_id); + debug!("Removed program {}", program_id); } pub fn add_precompile(&mut self, program_id: &Pubkey) { @@ -6451,7 +6432,11 @@ impl Bank { } if !debug_do_not_add_builtins { - self.ensure_feature_builtins(init_finish_or_warp, &new_feature_activations); + let apply_transitions_for_new_features = !init_finish_or_warp; + self.apply_builtin_program_feature_transitions( + apply_transitions_for_new_features, + &new_feature_activations, + ); self.reconfigure_token2_native_mint(); } self.ensure_no_storage_rewards_pool(); @@ -6508,33 +6493,34 @@ impl Bank { newly_activated } - fn ensure_feature_builtins( + fn apply_builtin_program_feature_transitions( &mut self, - init_or_warp: bool, + apply_transitions_for_new_features: bool, new_feature_activations: &HashSet, ) { - let feature_builtins = self.feature_builtins.clone(); - for (builtin, feature, activation_type) in feature_builtins.iter() { - let should_populate = init_or_warp && self.feature_set.is_active(feature) - || !init_or_warp && new_feature_activations.contains(feature); - if should_populate { - match activation_type { - ActivationType::NewProgram => self.add_builtin( + let feature_set = self.feature_set.clone(); + let should_apply_action_for_feature = |feature_id: &Pubkey| -> bool { + if apply_transitions_for_new_features { + new_feature_activations.contains(feature_id) + } else { + feature_set.is_active(feature_id) + } + }; + + let builtin_feature_transitions = self.builtin_feature_transitions.clone(); + for transition in builtin_feature_transitions.iter() { + if let Some(builtin_action) = transition.to_action(&should_apply_action_for_feature) { + match builtin_action { + BuiltinAction::Add(builtin) => self.add_builtin( &builtin.name, &builtin.id, builtin.process_instruction_with_context, ), - ActivationType::NewVersion => self.replace_builtin( - &builtin.name, - &builtin.id, - builtin.process_instruction_with_context, - ), - ActivationType::RemoveProgram => { - self.remove_builtin(&builtin.name, &builtin.id) - } + BuiltinAction::Remove(program_id) => self.remove_builtin(&program_id), } } } + for precompile in get_precompiles() { #[allow(clippy::blocks_in_if_conditions)] if precompile.feature.map_or(false, |ref feature_id| { @@ -13350,16 +13336,6 @@ pub(crate) mod tests { mock_ix_processor, ); assert_eq!(bank.get_account_modified_slot(&program_id).unwrap().1, slot); - - Arc::get_mut(&mut bank).unwrap().replace_builtin( - "mock_program v2", - &program_id, - mock_ix_processor, - ); - assert_eq!( - bank.get_account_modified_slot(&program_id).unwrap().1, - bank.slot() - ); } #[test] diff --git a/runtime/src/bank/builtin_programs.rs b/runtime/src/bank/builtin_programs.rs new file mode 100644 index 000000000..79b169970 --- /dev/null +++ b/runtime/src/bank/builtin_programs.rs @@ -0,0 +1,27 @@ +#[cfg(test)] +mod tests { + use { + crate::bank::*, + solana_sdk::{feature_set::FeatureSet, genesis_config::create_genesis_config}, + }; + + #[test] + fn test_startup_from_snapshot_after_precompile_transition() { + let (genesis_config, _mint_keypair) = create_genesis_config(100_000); + + let mut bank = Bank::new_for_tests(&genesis_config); + bank.feature_set = Arc::new(FeatureSet::all_enabled()); + bank.finish_init(&genesis_config, None, false); + + // Overwrite precompile accounts to simulate a cluster which already added precompiles. + for precompile in get_precompiles() { + bank.store_account(&precompile.program_id, &AccountSharedData::default()); + bank.add_precompiled_account(&precompile.program_id); + } + + bank.freeze(); + + // Simulate starting up from snapshot finishing the initialization for a frozen bank + bank.finish_init(&genesis_config, None, false); + } +} diff --git a/runtime/src/builtins.rs b/runtime/src/builtins.rs index b02163413..97d7b1808 100644 --- a/runtime/src/builtins.rs +++ b/runtime/src/builtins.rs @@ -51,13 +51,6 @@ macro_rules! with_program_logging { }; } -#[derive(AbiExample, Debug, Clone)] -pub enum ActivationType { - NewProgram, - NewVersion, - RemoveProgram, -} - #[derive(Clone)] pub struct Builtin { pub name: String, @@ -101,8 +94,63 @@ pub struct Builtins { /// Builtin programs that are always available pub genesis_builtins: Vec, - /// Builtin programs activated or deactivated dynamically by feature - pub feature_builtins: Vec<(Builtin, Pubkey, ActivationType)>, + /// Dynamic feature transitions for builtin programs + pub feature_transitions: Vec, +} + +/// Actions taken by a bank when managing the list of active builtin programs. +#[derive(Debug, Clone)] +pub enum BuiltinAction { + Add(Builtin), + Remove(Pubkey), +} + +/// State transition enum used for adding and removing builtin programs through +/// feature activations. +#[derive(Debug, Clone, AbiExample)] +pub enum BuiltinFeatureTransition { + /// Add a builtin program if a feature is activated. + Add { + builtin: Builtin, + feature_id: Pubkey, + }, + /// Remove a builtin program if a feature is activated or + /// retain a previously added builtin. + RemoveOrRetain { + previous_builtin: Builtin, + removal_feature_id: Pubkey, + }, +} + +impl BuiltinFeatureTransition { + pub fn to_action( + &self, + should_apply_action_for_feature: &impl Fn(&Pubkey) -> bool, + ) -> Option { + match self { + Self::Add { + builtin, + feature_id, + } => { + if should_apply_action_for_feature(feature_id) { + Some(BuiltinAction::Add(builtin.clone())) + } else { + None + } + } + Self::RemoveOrRetain { + previous_builtin, + removal_feature_id, + } => { + if should_apply_action_for_feature(removal_feature_id) { + Some(BuiltinAction::Remove(previous_builtin.id)) + } else { + // Retaining is no different from adding a new builtin. + Some(BuiltinAction::Add(previous_builtin.clone())) + } + } + } + } } /// Builtin programs that are always available @@ -128,16 +176,6 @@ fn genesis_builtins() -> Vec { solana_config_program::id(), with_program_logging!(solana_config_program::config_processor::process_instruction), ), - Builtin::new( - "secp256k1_program", - solana_sdk::secp256k1_program::id(), - dummy_process_instruction, - ), - Builtin::new( - "ed25519_program", - solana_sdk::ed25519_program::id(), - dummy_process_instruction, - ), ] } @@ -150,73 +188,55 @@ fn dummy_process_instruction( Ok(()) } -/// Builtin programs activated dynamically by feature -/// -/// Note: If the feature_builtin is intended to replace another builtin program, it must have a new -/// name. -/// This is to enable the runtime to determine categorically whether the builtin update has -/// occurred, and preserve idempotency in Bank::add_native_program across genesis, snapshot, and -/// normal child Bank creation. -/// https://github.com/solana-labs/solana/blob/84b139cc94b5be7c9e0c18c2ad91743231b85a0d/runtime/src/bank.rs#L1723 -fn feature_builtins() -> Vec<(Builtin, Pubkey, ActivationType)> { +/// Dynamic feature transitions for builtin programs +fn builtin_feature_transitions() -> Vec { vec![ - ( - Builtin::new( + BuiltinFeatureTransition::Add { + builtin: Builtin::new( "compute_budget_program", solana_sdk::compute_budget::id(), solana_compute_budget_program::process_instruction, ), - feature_set::add_compute_budget_program::id(), - ActivationType::NewProgram, - ), - // TODO when feature `prevent_calling_precompiles_as_programs` is - // cleaned up also remove "secp256k1_program" from the main builtins - // list - ( - Builtin::new( + feature_id: feature_set::add_compute_budget_program::id(), + }, + BuiltinFeatureTransition::RemoveOrRetain { + previous_builtin: Builtin::new( "secp256k1_program", solana_sdk::secp256k1_program::id(), dummy_process_instruction, ), - feature_set::prevent_calling_precompiles_as_programs::id(), - ActivationType::RemoveProgram, - ), - // TODO when feature `prevent_calling_precompiles_as_programs` is - // cleaned up also remove "ed25519_program" from the main builtins - // list - ( - Builtin::new( + removal_feature_id: feature_set::prevent_calling_precompiles_as_programs::id(), + }, + BuiltinFeatureTransition::RemoveOrRetain { + previous_builtin: Builtin::new( "ed25519_program", solana_sdk::ed25519_program::id(), dummy_process_instruction, ), - feature_set::prevent_calling_precompiles_as_programs::id(), - ActivationType::RemoveProgram, - ), - ( - Builtin::new( + removal_feature_id: feature_set::prevent_calling_precompiles_as_programs::id(), + }, + BuiltinFeatureTransition::Add { + builtin: Builtin::new( "address_lookup_table_program", solana_address_lookup_table_program::id(), solana_address_lookup_table_program::processor::process_instruction, ), - feature_set::versioned_tx_message_enabled::id(), - ActivationType::NewProgram, - ), - ( - Builtin::new( + feature_id: feature_set::versioned_tx_message_enabled::id(), + }, + BuiltinFeatureTransition::Add { + builtin: Builtin::new( "zk_token_proof_program", solana_zk_token_sdk::zk_token_proof_program::id(), with_program_logging!(solana_zk_token_proof_program::process_instruction), ), - feature_set::zk_token_sdk_enabled::id(), - ActivationType::NewProgram, - ), + feature_id: feature_set::zk_token_sdk_enabled::id(), + }, ] } pub(crate) fn get() -> Builtins { Builtins { genesis_builtins: genesis_builtins(), - feature_builtins: feature_builtins(), + feature_transitions: builtin_feature_transitions(), } }