Fix builtin handling on epoch boundaries (backport #23256)

runtime/src/bank.rs

@@ -3181,6 +3181,11 @@ impl Bank {
 
     /// Add a precompiled program account
     pub fn add_precompiled_account(&self, program_id: &Pubkey) {
+        self.add_precompiled_account_with_owner(program_id, native_loader::id())
+    }
+
+    // Used by tests to simulate clusters with precompiles that aren't owned by the native loader
+    fn add_precompiled_account_with_owner(&self, program_id: &Pubkey, owner: Pubkey) {
         if let Some(account) = self.get_account_with_fixed_root(program_id) {
             if account.executable() {
                 // The account is already executable, that's all we need
@@ -3202,7 +3207,7 @@ impl Bank {
         let (lamports, rent_epoch) = self.inherit_specially_retained_account_fields(&None);
         let account = AccountSharedData::from(Account {
             lamports,
-            owner: native_loader::id(),
+            owner,
             data: vec![],
             executable: true,
             rent_epoch,
@@ -6492,12 +6497,12 @@ impl Bank {
 
     fn apply_builtin_program_feature_transitions(
         &mut self,
-        apply_transitions_for_new_features: bool,
+        only_apply_transitions_for_new_features: bool,
         new_feature_activations: &HashSet<Pubkey>,
     ) {
         let feature_set = self.feature_set.clone();
-        let should_apply_action_for_feature = |feature_id: &Pubkey| -> bool {
-            if apply_transitions_for_new_features {
+        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)
@@ -6506,7 +6511,9 @@ impl Bank {
 
         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) {
+            if let Some(builtin_action) =
+                transition.to_action(&should_apply_action_for_feature_transition)
+            {
                 match builtin_action {
                     BuiltinAction::Add(builtin) => self.add_builtin(
                         &builtin.name,
@@ -7088,7 +7095,7 @@ pub(crate) mod tests {
             cluster_type: ClusterType::MainnetBeta,
             ..GenesisConfig::default()
         }));
-        let sysvar_and_builtin_program_delta0 = 12;
+        let sysvar_and_builtin_program_delta0 = 10;
         assert_eq!(
             bank0.capitalization(),
             42 * 42 + sysvar_and_builtin_program_delta0
@@ -8843,7 +8850,7 @@ pub(crate) mod tests {
         // not being eagerly-collected for exact rewards calculation
         bank0.restore_old_behavior_for_fragile_tests();
 
-        let sysvar_and_builtin_program_delta0 = 12;
+        let sysvar_and_builtin_program_delta0 = 10;
         assert_eq!(
             bank0.capitalization(),
             42 * 1_000_000_000 + sysvar_and_builtin_program_delta0
@@ -8978,7 +8985,7 @@ pub(crate) mod tests {
         // not being eagerly-collected for exact rewards calculation
         bank.restore_old_behavior_for_fragile_tests();
 
-        let sysvar_and_builtin_program_delta = 12;
+        let sysvar_and_builtin_program_delta = 10;
         assert_eq!(
             bank.capitalization(),
             42 * 1_000_000_000 + sysvar_and_builtin_program_delta
@@ -13028,25 +13035,25 @@ pub(crate) mod tests {
             if bank.slot == 0 {
                 assert_eq!(
                     bank.hash().to_string(),
-                    "CMCWTWsU67zjmayMhSMGBTzHbW1WMCtkM5m7xk9qSnY5"
+                    "7jRygKXT3WYoJWigqwHvnEHY2MGtCpV9DicY51kGjzKW"
                 );
             }
             if bank.slot == 32 {
                 assert_eq!(
                     bank.hash().to_string(),
-                    "4kbXeShX8vMnRuuADCkxSEir1oc2PrBNbx6vPkWcDtJU"
+                    "B8qZwsyx9h9NeUcVTG7uWFLCgxVQBca5fVmziWjNwfec"
                 );
             }
             if bank.slot == 64 {
                 assert_eq!(
                     bank.hash().to_string(),
-                    "CSZ8QCDF8qhqKDxafPzjNJpHcRAXmQzAb8eUi1Emt35E"
+                    "A893DSezmoRuEAWPQdHNEzShx4AtaY2VFFRKEeh9sx42"
                 );
             }
             if bank.slot == 128 {
                 assert_eq!(
                     bank.hash().to_string(),
-                    "Ewh1SYKy8eiSE77sEvjav33SznfWYSwa5TwqbiYWseG2"
+                    "9qHRKxR56VhJ49rHiCpXcw6AyvYh7r2JT5jDxJyeqDnT"
                 );
                 break;
             }
@@ -13274,7 +13281,7 @@ pub(crate) mod tests {
         // No more slots should be shrunk
         assert_eq!(bank2.shrink_candidate_slots(), 0);
         // alive_counts represents the count of alive accounts in the three slots 0,1,2
-        assert_eq!(alive_counts, vec![11, 1, 7]);
+        assert_eq!(alive_counts, vec![9, 1, 7]);
     }
 
     #[test]
@@ -13322,7 +13329,7 @@ pub(crate) mod tests {
             .map(|_| bank.process_stale_slot_with_budget(0, force_to_return_alive_account))
             .sum();
         // consumed_budgets represents the count of alive accounts in the three slots 0,1,2
-        assert_eq!(consumed_budgets, 12);
+        assert_eq!(consumed_budgets, 10);
     }
 
     #[test]

runtime/src/bank/builtin_programs.rs

@@ -5,6 +5,41 @@ mod tests {
         solana_sdk::{feature_set::FeatureSet, genesis_config::create_genesis_config},
     };
 
+    #[test]
+    fn test_apply_builtin_program_feature_transitions_for_new_epoch() {
+        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());
+            // Simulate cluster which added ed25519 precompile with a system program owner
+            if precompile.program_id == ed25519_program::id() {
+                bank.add_precompiled_account_with_owner(
+                    &precompile.program_id,
+                    solana_sdk::system_program::id(),
+                );
+            } else {
+                bank.add_precompiled_account(&precompile.program_id);
+            }
+        }
+
+        // Normally feature transitions are applied to a bank that hasn't been
+        // frozen yet.  Freeze the bank early to ensure that no account changes
+        // are made.
+        bank.freeze();
+
+        // Simulate crossing an epoch boundary for a new bank
+        let only_apply_transitions_for_new_features = true;
+        bank.apply_builtin_program_feature_transitions(
+            only_apply_transitions_for_new_features,
+            &HashSet::new(),
+        );
+    }
+
     #[test]
     fn test_startup_from_snapshot_after_precompile_transition() {
         let (genesis_config, _mint_keypair) = create_genesis_config(100_000);

runtime/src/builtins.rs

@@ -113,7 +113,8 @@ pub enum BuiltinFeatureTransition {
     /// Remove a builtin program if a feature is activated or
     /// retain a previously added builtin.
     RemoveOrRetain {
-        previous_builtin: Builtin,
+        previously_added_builtin: Builtin,
+        addition_feature_id: Pubkey,
         removal_feature_id: Pubkey,
     },
 }
@@ -135,14 +136,17 @@ impl BuiltinFeatureTransition {
                 }
             }
             Self::RemoveOrRetain {
-                previous_builtin,
+                previously_added_builtin,
+                addition_feature_id,
                 removal_feature_id,
             } => {
                 if should_apply_action_for_feature(removal_feature_id) {
-                    Some(BuiltinAction::Remove(previous_builtin.id))
-                } else {
+                    Some(BuiltinAction::Remove(previously_added_builtin.id))
+                } else if should_apply_action_for_feature(addition_feature_id) {
                     // Retaining is no different from adding a new builtin.
-                    Some(BuiltinAction::Add(previous_builtin.clone()))
+                    Some(BuiltinAction::Add(previously_added_builtin.clone()))
+                } else {
+                    None
                 }
             }
         }
@@ -196,19 +200,21 @@ fn builtin_feature_transitions() -> Vec<BuiltinFeatureTransition> {
             feature_id: feature_set::add_compute_budget_program::id(),
         },
         BuiltinFeatureTransition::RemoveOrRetain {
-            previous_builtin: Builtin::new(
+            previously_added_builtin: Builtin::new(
                 "secp256k1_program",
                 solana_sdk::secp256k1_program::id(),
                 dummy_process_instruction,
             ),
+            addition_feature_id: feature_set::secp256k1_program_enabled::id(),
             removal_feature_id: feature_set::prevent_calling_precompiles_as_programs::id(),
         },
         BuiltinFeatureTransition::RemoveOrRetain {
-            previous_builtin: Builtin::new(
+            previously_added_builtin: Builtin::new(
                 "ed25519_program",
                 solana_sdk::ed25519_program::id(),
                 dummy_process_instruction,
             ),
+            addition_feature_id: feature_set::ed25519_program_enabled::id(),
             removal_feature_id: feature_set::prevent_calling_precompiles_as_programs::id(),
         },
         BuiltinFeatureTransition::Add {

runtime/src/non_circulating_supply.rs

@@ -278,7 +278,7 @@ mod tests {
             ..GenesisConfig::default()
         };
         let mut bank = Arc::new(Bank::new_for_tests(&genesis_config));
-        let sysvar_and_native_program_delta = 12;
+        let sysvar_and_native_program_delta = 10;
         assert_eq!(
             bank.capitalization(),
             (num_genesis_accounts + num_non_circulating_accounts + num_stake_accounts) * balance

sdk/src/feature_set.rs

@@ -53,6 +53,10 @@ pub mod full_inflation {
     }
 }
 
+pub mod secp256k1_program_enabled {
+    solana_sdk::declare_id!("E3PHP7w8kB7np3CTQ1qQ2tW3KCtjRSXBQgW9vM2mWv2Y");
+}
+
 pub mod spl_token_v2_multisig_fix {
     solana_sdk::declare_id!("E5JiFDQCwyC6QfT9REFyMpfK2mHcmv1GUDySU1Ue7TYv");
 }
@@ -302,6 +306,7 @@ pub mod add_get_processed_sibling_instruction_syscall {
 lazy_static! {
     /// Map of feature identifiers to user-visible description
     pub static ref FEATURE_NAMES: HashMap<Pubkey, &'static str> = [
+        (secp256k1_program_enabled::id(), "secp256k1 program"),
         (deprecate_rewards_sysvar::id(), "deprecate unused rewards sysvar"),
         (pico_inflation::id(), "pico inflation"),
         (full_inflation::devnet_and_testnet::id(), "full inflation on devnet and testnet"),