Refactor - Simplify built-in program feature transition (#31565)

* Removes builtins::get_builtins(), instead derive them from Bank::builtin_programs.

* Removes BuiltinAction.
Turns BuiltinFeatureTransition from an enum into a struct.

* Makes Bank::remove_builtin() place tombstones.

runtime/src/bank.rs

@@ -56,7 +56,7 @@ use {
         ancestors::{Ancestors, AncestorsForSerialization},
         bank::metrics::*,
         blockhash_queue::BlockhashQueue,
-        builtins::{self, BuiltinAction, BuiltinFeatureTransition, Builtins},
+        builtins::{self, BuiltinFeatureTransition, Builtins},
         cost_tracker::CostTracker,
         epoch_accounts_hash::{self, EpochAccountsHash},
         epoch_stakes::{EpochStakes, NodeVoteAccounts},
@@ -7260,7 +7260,7 @@ impl Bank {
         !self.is_delta.load(Relaxed)
     }
 
-    /// Add an instruction processor to intercept instructions before the dynamic loader.
+    /// Add a built-in program
     pub fn add_builtin(&mut self, program_id: Pubkey, builtin: Arc<LoadedProgram>) {
         let name = match &builtin.program {
             LoadedProgramType::Builtin(name, _) => name,
@@ -7283,19 +7283,18 @@ impl Bank {
         debug!("Added program {} under {:?}", name, program_id);
     }
 
-    /// Remove a builtin instruction processor if it already exists
-    pub fn remove_builtin(&mut self, program_id: &Pubkey) {
+    /// Remove a built-in instruction processor
+    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
-            .builtin_programs
-            .vec
-            .iter()
-            .position(|entry| entry.0 == *program_id)
-        {
-            self.builtin_programs.vec.remove(position);
-        }
+        self.add_builtin(
+            program_id,
+            Arc::new(LoadedProgram::new_tombstone(
+                self.slot,
+                LoadedProgramType::Closed,
+            )),
+        );
         debug!("Removed program {}", program_id);
     }
 
@@ -7520,25 +7519,17 @@ impl Bank {
         new_feature_activations: &HashSet<Pubkey>,
     ) {
         let feature_set = self.feature_set.clone();
-        let should_apply_action_for_feature_transition = |feature_id: &Pubkey| -> bool {
-            if only_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_transition)
-            {
-                match builtin_action {
-                    BuiltinAction::Add(program_id, builtin) => {
-                        self.add_builtin(program_id, builtin)
-                    }
-                    BuiltinAction::Remove(program_id) => self.remove_builtin(&program_id),
-                }
+            let should_apply_action_for_feature_transition =
+                if only_apply_transitions_for_new_features {
+                    new_feature_activations.contains(&transition.feature_id)
+                } else {
+                    feature_set.is_active(&transition.feature_id)
+                };
+            if should_apply_action_for_feature_transition {
+                self.add_builtin(transition.program_id, transition.builtin.clone());
             }
         }
 

runtime/src/builtins.rs

@@ -15,69 +15,19 @@ pub struct Builtins {
     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(Pubkey, Arc<LoadedProgram>),
-    Remove(Pubkey),
+/// Transitions of built-in programs at epoch bondaries when features are activated.
+#[derive(Debug, Default, Clone)]
+pub struct BuiltinFeatureTransition {
+    pub feature_id: Pubkey,
+    pub program_id: Pubkey,
+    pub builtin: Arc<LoadedProgram>,
 }
 
-/// 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 {
-        program_id: Pubkey,
-        builtin: Arc<LoadedProgram>,
-        feature_id: Pubkey,
-    },
-    /// Remove a builtin program if a feature is activated or
-    /// retain a previously added builtin.
-    RemoveOrRetain {
-        program_id: Pubkey,
-        previously_added_builtin: Arc<LoadedProgram>,
-        addition_feature_id: Pubkey,
-        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 {
-                program_id,
-                builtin,
-                feature_id,
-            } => {
-                if should_apply_action_for_feature(feature_id) {
-                    Some(BuiltinAction::Add(*program_id, builtin.clone()))
-                } else {
-                    None
-                }
-            }
-            Self::RemoveOrRetain {
-                program_id,
-                previously_added_builtin,
-                addition_feature_id,
-                removal_feature_id,
-            } => {
-                if should_apply_action_for_feature(removal_feature_id) {
-                    Some(BuiltinAction::Remove(*program_id))
-                } else if should_apply_action_for_feature(addition_feature_id) {
-                    // Retaining is no different from adding a new builtin.
-                    Some(BuiltinAction::Add(
-                        *program_id,
-                        previously_added_builtin.clone(),
-                    ))
-                } else {
-                    None
-                }
-            }
-        }
+#[cfg(RUSTC_WITH_SPECIALIZATION)]
+impl solana_frozen_abi::abi_example::AbiExample for BuiltinFeatureTransition {
+    fn example() -> Self {
+        // BuiltinFeatureTransition isn't serializable by definition.
+        Self::default()
     }
 }
 
@@ -152,13 +102,13 @@ fn genesis_builtins() -> Vec<(Pubkey, Arc<LoadedProgram>)> {
 
 /// Dynamic feature transitions for builtin programs
 fn builtin_feature_transitions() -> Vec<BuiltinFeatureTransition> {
-    vec![BuiltinFeatureTransition::Add {
+    vec![BuiltinFeatureTransition {
+        feature_id: feature_set::zk_token_sdk_enabled::id(),
         program_id: solana_zk_token_sdk::zk_token_proof_program::id(),
         builtin: create_builtin(
             "zk_token_proof_program".to_string(),
             solana_zk_token_proof_program::process_instruction,
         ),
-        feature_id: feature_set::zk_token_sdk_enabled::id(),
     }]
 }
 
@@ -168,21 +118,3 @@ pub(crate) fn get() -> Builtins {
         feature_transitions: builtin_feature_transitions(),
     }
 }
-
-/// Returns the addresses of all builtin programs.
-pub fn get_pubkeys() -> Vec<Pubkey> {
-    let builtins = get();
-
-    let mut pubkeys = Vec::new();
-    pubkeys.extend(
-        builtins
-            .genesis_builtins
-            .iter()
-            .map(|(program_id, _builtin)| program_id),
-    );
-    pubkeys.extend(builtins.feature_transitions.iter().filter_map(|f| match f {
-        BuiltinFeatureTransition::Add { program_id, .. } => Some(program_id),
-        BuiltinFeatureTransition::RemoveOrRetain { .. } => None,
-    }));
-    pubkeys
-}

runtime/src/snapshot_minimizer.rs

@@ -7,7 +7,7 @@ use {
         },
         accounts_partition,
         bank::Bank,
-        builtins, static_ids,
+        static_ids,
     },
     dashmap::DashSet,
     log::info,
@@ -114,9 +114,13 @@ impl<'a> SnapshotMinimizer<'a> {
 
     /// Used to get builtin accounts in `minimize`
     fn get_builtins(&self) {
-        builtins::get_pubkeys().iter().for_each(|pubkey| {
-            self.minimized_account_set.insert(*pubkey);
-        });
+        self.bank
+            .get_builtin_programs()
+            .vec
+            .iter()
+            .for_each(|(pubkey, _builtin)| {
+                self.minimized_account_set.insert(*pubkey);
+            });
     }
 
     /// Used to get static runtime accounts in `minimize`