From 215c70859924ffb2ee882199378e6ad7687e7dff Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Tue, 22 Feb 2022 16:15:10 +0000 Subject: [PATCH] Fix builtin handling on epoch boundaries (backport #23256) (#23273) * Fix builtin handling on epoch boundaries (#23256) (cherry picked from commit bcda74f42f19742331900258933c105b69f424cd) # Conflicts: # runtime/src/bank.rs # runtime/src/genesis_utils.rs * fix conflicts Co-authored-by: Justin Starry --- runtime/src/bank.rs | 35 ++++++++++++++++----------- runtime/src/bank/builtin_programs.rs | 35 +++++++++++++++++++++++++++ runtime/src/builtins.rs | 20 +++++++++------ runtime/src/non_circulating_supply.rs | 2 +- sdk/src/feature_set.rs | 5 ++++ 5 files changed, 75 insertions(+), 22 deletions(-) diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 0fcd7f2cba..7c1ee2dcde 100644 --- a/runtime/src/bank.rs +++ b/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, ) { 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] diff --git a/runtime/src/bank/builtin_programs.rs b/runtime/src/bank/builtin_programs.rs index 79b169970b..018779aa01 100644 --- a/runtime/src/bank/builtin_programs.rs +++ b/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); diff --git a/runtime/src/builtins.rs b/runtime/src/builtins.rs index 1ae06de2ef..716aaac1c3 100644 --- a/runtime/src/builtins.rs +++ b/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 { 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 { diff --git a/runtime/src/non_circulating_supply.rs b/runtime/src/non_circulating_supply.rs index 187f478ebf..2a216c76df 100644 --- a/runtime/src/non_circulating_supply.rs +++ b/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 diff --git a/sdk/src/feature_set.rs b/sdk/src/feature_set.rs index a8d81c5434..5c1c791ae7 100644 --- a/sdk/src/feature_set.rs +++ b/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 = [ + (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"),