Skip adding builtins if they will be removed (#23233)

* Add failing test for precompile transition

* Skip adding builtins if they will be removed

* cargo clean

* nits

* fix abi check

* remove workaround

Co-authored-by: Jack May <jack@solana.com>

ledger/src/builtins.rs

@@ -1,7 +1,4 @@
-use {
+use solana_runtime::builtins::{Builtin, BuiltinFeatureTransition, Builtins};
-    solana_runtime::builtins::{ActivationType, Builtin, Builtins},
-    solana_sdk::pubkey::Pubkey,
-};
 
 macro_rules! to_builtin {
     ($b:expr) => {
@@ -37,14 +34,14 @@ fn genesis_builtins(bpf_jit: bool) -> Vec<Builtin> {
     ]
 }
 
-/// Builtin programs activated dynamically by feature
+/// Dynamic feature transitions for builtin programs
-fn feature_builtins() -> Vec<(Builtin, Pubkey, ActivationType)> {
+fn builtin_feature_transitions() -> Vec<BuiltinFeatureTransition> {
     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(),
     }
 }

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<ComputeBudget>,
 
-    /// Builtin programs activated dynamically by feature
+    /// Dynamic feature transitions for builtin programs
     #[allow(clippy::rc_buffer)]
-    feature_builtins: Arc<Vec<(Builtin, Pubkey, ActivationType)>>,
+    builtin_feature_transitions: Arc<Vec<BuiltinFeatureTransition>>,
 
     /// Protocol-level rewards that were distributed by this bank
     pub rewards: RwLock<Vec<(Pubkey, RewardInfo)>>,
@@ -1367,7 +1368,7 @@ impl Bank {
             is_delta: AtomicBool::default(),
             builtin_programs: BuiltinPrograms::default(),
             compute_budget: Option::<ComputeBudget>::default(),
-            feature_builtins: Arc::<Vec<(Builtin, Pubkey, ActivationType)>>::default(),
+            builtin_feature_transitions: Arc::<Vec<BuiltinFeatureTransition>>::default(),
             rewards: RwLock::<Vec<(Pubkey, RewardInfo)>>::default(),
             cluster_type: Option::<ClusterType>::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
+                .feature_transitions
-                .extend_from_slice(&additional_builtins.feature_builtins);
+                .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) {
+    pub fn remove_builtin(&mut self, program_id: &Pubkey) {
-        debug!("Removing program {} under {:?}", name, program_id);
+        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<Pubkey>,
     ) {
-        let feature_builtins = self.feature_builtins.clone();
+        let feature_set = self.feature_set.clone();
-        for (builtin, feature, activation_type) in feature_builtins.iter() {
+        let should_apply_action_for_feature = |feature_id: &Pubkey| -> bool {
-            let should_populate = init_or_warp && self.feature_set.is_active(feature)
+            if apply_transitions_for_new_features {
-                || !init_or_warp && new_feature_activations.contains(feature);
+                new_feature_activations.contains(feature_id)
-            if should_populate {
+            } else {
-                match activation_type {
+                feature_set.is_active(feature_id)
-                    ActivationType::NewProgram => self.add_builtin(
+            }
+        };
+
+        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(
+                    BuiltinAction::Remove(program_id) => self.remove_builtin(&program_id),
-                        &builtin.name,
-                        &builtin.id,
-                        builtin.process_instruction_with_context,
-                    ),
-                    ActivationType::RemoveProgram => {
-                        self.remove_builtin(&builtin.name, &builtin.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]

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);
+    }
+}

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>,
 
-    /// Builtin programs activated or deactivated dynamically by feature
+    /// Dynamic feature transitions for builtin programs
-    pub feature_builtins: Vec<(Builtin, Pubkey, ActivationType)>,
+    pub feature_transitions: Vec<BuiltinFeatureTransition>,
+}
+
+/// 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<BuiltinAction> {
+        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<Builtin> {
             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
+/// Dynamic feature transitions for builtin programs
-///
+fn builtin_feature_transitions() -> Vec<BuiltinFeatureTransition> {
-/// 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)> {
     vec![
-        (
+        BuiltinFeatureTransition::Add {
-            Builtin::new(
+            builtin: Builtin::new(
                 "compute_budget_program",
                 solana_sdk::compute_budget::id(),
                 solana_compute_budget_program::process_instruction,
             ),
-            feature_set::add_compute_budget_program::id(),
+            feature_id: feature_set::add_compute_budget_program::id(),
-            ActivationType::NewProgram,
+        },
-        ),
+        BuiltinFeatureTransition::RemoveOrRetain {
-        // TODO when feature `prevent_calling_precompiles_as_programs` is
+            previous_builtin: Builtin::new(
-        // cleaned up also remove "secp256k1_program" from the main builtins
-        // list
-        (
-            Builtin::new(
                 "secp256k1_program",
                 solana_sdk::secp256k1_program::id(),
                 dummy_process_instruction,
             ),
-            feature_set::prevent_calling_precompiles_as_programs::id(),
+            removal_feature_id: feature_set::prevent_calling_precompiles_as_programs::id(),
-            ActivationType::RemoveProgram,
+        },
-        ),
+        BuiltinFeatureTransition::RemoveOrRetain {
-        // TODO when feature `prevent_calling_precompiles_as_programs` is
+            previous_builtin: Builtin::new(
-        // cleaned up also remove "ed25519_program" from the main builtins
-        // list
-        (
-            Builtin::new(
                 "ed25519_program",
                 solana_sdk::ed25519_program::id(),
                 dummy_process_instruction,
             ),
-            feature_set::prevent_calling_precompiles_as_programs::id(),
+            removal_feature_id: feature_set::prevent_calling_precompiles_as_programs::id(),
-            ActivationType::RemoveProgram,
+        },
-        ),
+        BuiltinFeatureTransition::Add {
-        (
+            builtin: Builtin::new(
-            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(),
+            feature_id: feature_set::versioned_tx_message_enabled::id(),
-            ActivationType::NewProgram,
+        },
-        ),
+        BuiltinFeatureTransition::Add {
-        (
+            builtin: Builtin::new(
-            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(),
+            feature_id: feature_set::zk_token_sdk_enabled::id(),
-            ActivationType::NewProgram,
+        },
-        ),
     ]
 }
 
 pub(crate) fn get() -> Builtins {
     Builtins {
         genesis_builtins: genesis_builtins(),
-        feature_builtins: feature_builtins(),
+        feature_transitions: builtin_feature_transitions(),
     }
 }