From a753d286df5beac3b7668d168dca829a143ef450 Mon Sep 17 00:00:00 2001 From: Justin Starry Date: Fri, 18 Feb 2022 20:36:06 +0800 Subject: [PATCH 1/6] Add failing test for precompile transition --- runtime/src/bank.rs | 1 + runtime/src/bank/builtin_programs.rs | 27 +++++++++++++++++++++++++++ 2 files changed, 28 insertions(+) create mode 100644 runtime/src/bank/builtin_programs.rs diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 22b6aeffc22460..298285ad622fd8 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -157,6 +157,7 @@ use { }; mod address_lookup_table; +mod builtin_programs; mod sysvar_cache; mod transaction_account_state_info; diff --git a/runtime/src/bank/builtin_programs.rs b/runtime/src/bank/builtin_programs.rs new file mode 100644 index 00000000000000..79b169970b75b4 --- /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); + } +} From 9efe5c17940decbd6f55f2c5c373cdb08fefe808 Mon Sep 17 00:00:00 2001 From: Justin Starry Date: Fri, 18 Feb 2022 17:50:15 +0800 Subject: [PATCH 2/6] Skip adding builtins if they will be removed --- ledger/src/builtins.rs | 11 ++-- runtime/src/bank.rs | 95 +++++++++++------------------ runtime/src/builtins.rs | 132 ++++++++++++++++++++++++---------------- 3 files changed, 119 insertions(+), 119 deletions(-) diff --git a/ledger/src/builtins.rs b/ledger/src/builtins.rs index c9ef1cc9cee4a1..64a2e4648320b7 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 298285ad622fd8..034b6171ebe926 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, @@ -1197,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>, @@ -1368,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(), @@ -1704,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, @@ -1990,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(), @@ -5515,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 { @@ -5532,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); } @@ -6239,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 @@ -6272,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) { @@ -6452,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_old_features = init_finish_or_warp; + self.apply_builtin_program_feature_transitions( + apply_transitions_for_old_features, + &new_feature_activations, + ); self.reconfigure_token2_native_mint(); } self.ensure_no_storage_rewards_pool(); @@ -6509,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_old_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( - &builtin.name, - &builtin.id, - builtin.process_instruction_with_context, - ), - ActivationType::NewVersion => self.replace_builtin( + let feature_set = self.feature_set.clone(); + let should_apply_action_for_feature = |feature_id: &Pubkey| -> bool { + if apply_transitions_for_old_features { + feature_set.is_active(feature_id) + } else { + new_feature_activations.contains(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::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| { @@ -13351,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/builtins.rs b/runtime/src/builtins.rs index b02163413bc8b6..226f8bf055da31 100644 --- a/runtime/src/builtins.rs +++ b/runtime/src/builtins.rs @@ -51,11 +51,61 @@ macro_rules! with_program_logging { }; } -#[derive(AbiExample, Debug, Clone)] -pub enum ActivationType { - NewProgram, - NewVersion, - RemoveProgram, +/// State transition enum used for adding and removing builtin programs through +/// feature activations. +#[derive(Debug, Clone)] +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, + }, +} + +/// Actions taken by a bank when managing the list of active builtin programs. +#[derive(Debug, Clone)] +pub enum BuiltinAction { + Add(Builtin), + Remove { program_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 { + program_id: previous_builtin.id, + }) + } else { + // Retaining is no different from adding a new builtin. + Some(BuiltinAction::Add(previous_builtin.clone())) + } + } + } + } } #[derive(Clone)] @@ -101,8 +151,8 @@ 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, } /// Builtin programs that are always available @@ -128,16 +178,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 +190,61 @@ 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, - ), + feature_id: feature_set::add_compute_budget_program::id(), + }, // TODO when feature `prevent_calling_precompiles_as_programs` is // cleaned up also remove "secp256k1_program" from the main builtins // list - ( - Builtin::new( + 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, - ), + removal_feature_id: feature_set::prevent_calling_precompiles_as_programs::id(), + }, // TODO when feature `prevent_calling_precompiles_as_programs` is // cleaned up also remove "ed25519_program" from the main builtins // list - ( - Builtin::new( + 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(), } } From fcc54fa18804c3cb0c403ef685f162dbf430a957 Mon Sep 17 00:00:00 2001 From: Justin Starry Date: Fri, 18 Feb 2022 21:54:18 +0800 Subject: [PATCH 3/6] cargo clean --- ci/test-checks.sh | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ci/test-checks.sh b/ci/test-checks.sh index 2d02ead32ca073..c177619d7d028d 100755 --- a/ci/test-checks.sh +++ b/ci/test-checks.sh @@ -44,6 +44,8 @@ echo --- build environment export RUST_BACKTRACE=1 export RUSTFLAGS="-D warnings -A incomplete_features" +_ "$cargo" stable clean + # Only force up-to-date lock files on edge if [[ $CI_BASE_BRANCH = "$EDGE_CHANNEL" ]]; then # Exclude --benches as it's not available in rust stable yet From a5964db73736c534ecbcdbffbececbd9aaf1abc8 Mon Sep 17 00:00:00 2001 From: Jack May Date: Fri, 18 Feb 2022 09:29:06 -0800 Subject: [PATCH 4/6] nits --- runtime/src/bank.rs | 14 ++--- runtime/src/builtins.rs | 118 +++++++++++++++++++--------------------- 2 files changed, 62 insertions(+), 70 deletions(-) diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 034b6171ebe926..269217f0cbe62b 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -6432,9 +6432,9 @@ impl Bank { } if !debug_do_not_add_builtins { - let apply_transitions_for_old_features = init_finish_or_warp; + let apply_transitions_for_new_features = !init_finish_or_warp; self.apply_builtin_program_feature_transitions( - apply_transitions_for_old_features, + apply_transitions_for_new_features, &new_feature_activations, ); self.reconfigure_token2_native_mint(); @@ -6495,15 +6495,15 @@ impl Bank { fn apply_builtin_program_feature_transitions( &mut self, - apply_transitions_for_old_features: bool, + 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_old_features { - feature_set.is_active(feature_id) - } else { + if apply_transitions_for_new_features { new_feature_activations.contains(feature_id) + } else { + feature_set.is_active(feature_id) } }; @@ -6516,7 +6516,7 @@ impl Bank { &builtin.id, builtin.process_instruction_with_context, ), - BuiltinAction::Remove { program_id } => self.remove_builtin(&program_id), + BuiltinAction::Remove(program_id) => self.remove_builtin(&program_id), } } } diff --git a/runtime/src/builtins.rs b/runtime/src/builtins.rs index 226f8bf055da31..f19db3fff3c324 100644 --- a/runtime/src/builtins.rs +++ b/runtime/src/builtins.rs @@ -51,63 +51,6 @@ macro_rules! with_program_logging { }; } -/// State transition enum used for adding and removing builtin programs through -/// feature activations. -#[derive(Debug, Clone)] -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, - }, -} - -/// Actions taken by a bank when managing the list of active builtin programs. -#[derive(Debug, Clone)] -pub enum BuiltinAction { - Add(Builtin), - Remove { program_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 { - program_id: previous_builtin.id, - }) - } else { - // Retaining is no different from adding a new builtin. - Some(BuiltinAction::Add(previous_builtin.clone())) - } - } - } - } -} - #[derive(Clone)] pub struct Builtin { pub name: String, @@ -155,6 +98,61 @@ pub struct Builtins { 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)] +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 fn genesis_builtins() -> Vec { vec![ @@ -201,9 +199,6 @@ fn builtin_feature_transitions() -> Vec { ), feature_id: feature_set::add_compute_budget_program::id(), }, - // TODO when feature `prevent_calling_precompiles_as_programs` is - // cleaned up also remove "secp256k1_program" from the main builtins - // list BuiltinFeatureTransition::RemoveOrRetain { previous_builtin: Builtin::new( "secp256k1_program", @@ -212,9 +207,6 @@ fn builtin_feature_transitions() -> Vec { ), removal_feature_id: feature_set::prevent_calling_precompiles_as_programs::id(), }, - // TODO when feature `prevent_calling_precompiles_as_programs` is - // cleaned up also remove "ed25519_program" from the main builtins - // list BuiltinFeatureTransition::RemoveOrRetain { previous_builtin: Builtin::new( "ed25519_program", From bd40fa4e27806f824114343456c041056416fcae Mon Sep 17 00:00:00 2001 From: Jack May Date: Fri, 18 Feb 2022 13:09:08 -0800 Subject: [PATCH 5/6] fix abi check --- runtime/src/builtins.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/runtime/src/builtins.rs b/runtime/src/builtins.rs index f19db3fff3c324..97d7b180858d09 100644 --- a/runtime/src/builtins.rs +++ b/runtime/src/builtins.rs @@ -107,7 +107,7 @@ pub enum BuiltinAction { /// State transition enum used for adding and removing builtin programs through /// feature activations. -#[derive(Debug, Clone)] +#[derive(Debug, Clone, AbiExample)] pub enum BuiltinFeatureTransition { /// Add a builtin program if a feature is activated. Add { From 1a91f9e79ed09bb9ada7926316e5cc1f02d77b65 Mon Sep 17 00:00:00 2001 From: Jack May Date: Fri, 18 Feb 2022 13:25:10 -0800 Subject: [PATCH 6/6] remove workaround --- ci/test-checks.sh | 2 -- 1 file changed, 2 deletions(-) diff --git a/ci/test-checks.sh b/ci/test-checks.sh index c177619d7d028d..2d02ead32ca073 100755 --- a/ci/test-checks.sh +++ b/ci/test-checks.sh @@ -44,8 +44,6 @@ echo --- build environment export RUST_BACKTRACE=1 export RUSTFLAGS="-D warnings -A incomplete_features" -_ "$cargo" stable clean - # Only force up-to-date lock files on edge if [[ $CI_BASE_BRANCH = "$EDGE_CHANNEL" ]]; then # Exclude --benches as it's not available in rust stable yet