diff --git a/ledger/src/builtins.rs b/ledger/src/builtins.rs index 2a99f865cb..f08c167d47 100644 --- a/ledger/src/builtins.rs +++ b/ledger/src/builtins.rs @@ -1,4 +1,7 @@ -use solana_runtime::bank::{Builtin, Builtins}; +use solana_runtime::{ + bank::{Builtin, Builtins}, + builtins::ActivationType, +}; use solana_sdk::{feature_set, genesis_config::ClusterType, pubkey::Pubkey}; /// Builtin programs that are always available @@ -21,15 +24,16 @@ fn genesis_builtins(cluster_type: ClusterType) -> Vec { } /// Builtin programs activated dynamically by feature -fn feature_builtins() -> Vec<(Builtin, Pubkey)> { +fn feature_builtins() -> Vec<(Builtin, Pubkey, ActivationType)> { let builtins = vec![( solana_bpf_loader_program!(), feature_set::bpf_loader2_program::id(), + ActivationType::NewProgram, )]; builtins .into_iter() - .map(|(b, p)| (Builtin::new(&b.0, b.1, b.2), p)) + .map(|(b, p, t)| (Builtin::new(&b.0, b.1, b.2), p, t)) .collect() } diff --git a/programs/failure/tests/failure.rs b/programs/failure/tests/failure.rs index 8194e66e7f..73c616606a 100644 --- a/programs/failure/tests/failure.rs +++ b/programs/failure/tests/failure.rs @@ -12,7 +12,7 @@ fn test_program_native_failure() { let (genesis_config, alice_keypair) = create_genesis_config(50); let program_id = solana_sdk::pubkey::new_rand(); let bank = Bank::new(&genesis_config); - bank.add_native_program("solana_failure_program", &program_id); + bank.add_native_program("solana_failure_program", &program_id, false); // Call user program let instruction = create_invoke_instruction(alice_keypair.pubkey(), program_id, &1u8); diff --git a/runtime/benches/bank.rs b/runtime/benches/bank.rs index 3e84cbbbdf..b5a589af8b 100644 --- a/runtime/benches/bank.rs +++ b/runtime/benches/bank.rs @@ -130,7 +130,7 @@ fn do_bench_transactions( Pubkey::new(&BUILTIN_PROGRAM_ID), process_instruction, ); - bank.add_native_program("solana_noop_program", &Pubkey::new(&NOOP_PROGRAM_ID)); + bank.add_native_program("solana_noop_program", &Pubkey::new(&NOOP_PROGRAM_ID), false); let bank = Arc::new(bank); let bank_client = BankClient::new_shared(&bank); let transactions = create_transactions(&bank_client, &mint_keypair); diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index e197a9712f..9ca911d366 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -10,7 +10,7 @@ use crate::{ accounts_db::{ErrorCounters, SnapshotStorages}, accounts_index::Ancestors, blockhash_queue::BlockhashQueue, - builtins, + builtins::{self, ActivationType}, epoch_stakes::{EpochStakes, NodeVoteAccounts}, instruction_recorder::InstructionRecorder, log_collector::LogCollector, @@ -215,7 +215,7 @@ pub struct Builtins { pub genesis_builtins: Vec, /// Builtin programs activated dynamically by feature - pub feature_builtins: Vec<(Builtin, Pubkey)>, + pub feature_builtins: Vec<(Builtin, Pubkey, ActivationType)>, } const MAX_CACHED_EXECUTORS: usize = 100; // 10 MB assuming programs are around 100k @@ -676,7 +676,7 @@ pub struct Bank { bpf_compute_budget: Option, /// Builtin programs activated dynamically by feature - feature_builtins: Arc>, + feature_builtins: Arc>, /// Last time when the cluster info vote listener has synced with this bank pub last_vote_sync: AtomicU64, @@ -1715,16 +1715,15 @@ impl Bank { // Add additional native programs specified in the genesis config for (name, program_id) in &genesis_config.native_instruction_processors { - self.add_native_program(name, program_id); + self.add_native_program(name, program_id, false); } } - pub fn add_native_program(&self, name: &str, program_id: &Pubkey) { - let mut already_genuine_program_exists = false; - if let Some(mut account) = self.get_account(&program_id) { - already_genuine_program_exists = native_loader::check_id(&account.owner); - - if !already_genuine_program_exists { + // NOTE: must hold idempotent for the same set of arguments + pub fn add_native_program(&self, name: &str, program_id: &Pubkey, must_replace: bool) { + let mut existing_genuine_program = self.get_account(&program_id); + if let Some(account) = &mut existing_genuine_program { + if !native_loader::check_id(&account.owner) { // malicious account is pre-occupying at program_id // forcibly burn and purge it @@ -1737,19 +1736,54 @@ impl Bank { } } - if !already_genuine_program_exists { - assert!( - !self.is_frozen(), - "Can't change frozen bank by adding not-existing new native program ({}, {}). \ - Maybe, inconsistent program activation is detected on snapshot restore?", - name, - program_id - ); + if must_replace { + // updating native program - // Add a bogus executable native account, which will be loaded and ignored. - let account = native_loader::create_loadable_account(name); - self.store_account(&program_id, &account); + match existing_genuine_program { + None => panic!( + "There is no account to replace with native program ({}, {}).", + name, program_id + ), + Some(account) => { + if *name == String::from_utf8_lossy(&account.data) { + // nop; it seems that already AccountsDB is updated. + return; + } + // continue to replace account + } + } + } else { + // introducing native program + + match existing_genuine_program { + None => (), // continue to add account + Some(_account) => { + // nop; it seems that we already have account + + // before returning here to retain idempotent just make sure + // the existing native program name is same with what we're + // supposed to add here (but skipping) But I can't: + // following assertion already catches several different names for same + // program_id + // depending on clusters... + // assert_eq!(name.to_owned(), String::from_utf8_lossy(&account.data)); + return; + } + } } + + assert!( + !self.is_frozen(), + "Can't change frozen bank by adding not-existing new native program ({}, {}). \ + Maybe, inconsistent program activation is detected on snapshot restore?", + name, + program_id + ); + + // Add a bogus executable native account, which will be loaded and ignored. + let account = native_loader::create_loadable_account(name); + self.store_account(&program_id, &account); + debug!("Added native program {} under {:?}", name, program_id); } @@ -3886,8 +3920,21 @@ impl Bank { program_id: Pubkey, process_instruction_with_context: ProcessInstructionWithContext, ) { - debug!("Added program {} under {:?}", name, program_id); - self.add_native_program(name, &program_id); + debug!("Adding program {} under {:?}", name, program_id); + self.add_native_program(name, &program_id, false); + self.message_processor + .add_program(program_id, process_instruction_with_context); + } + + /// Replace a builtin instruction processor if it already exists + pub fn replace_builtin( + &mut self, + name: &str, + program_id: Pubkey, + process_instruction_with_context: ProcessInstructionWithContext, + ) { + debug!("Replacing program {} under {:?}", name, program_id); + self.add_native_program(name, &program_id, true); self.message_processor .add_program(program_id, process_instruction_with_context); } @@ -4021,15 +4068,22 @@ impl Bank { new_feature_activations: &HashSet, ) { let feature_builtins = self.feature_builtins.clone(); - for (builtin, feature) in feature_builtins.iter() { + 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 { - self.add_builtin( - &builtin.name, - builtin.id, - builtin.process_instruction_with_context, - ); + match activation_type { + ActivationType::NewProgram => self.add_builtin( + &builtin.name, + builtin.id, + builtin.process_instruction_with_context, + ), + ActivationType::NewVersion => self.replace_builtin( + &builtin.name, + builtin.id, + builtin.process_instruction_with_context, + ), + } } } } @@ -9230,6 +9284,16 @@ mod tests { .unwrap() .add_builtin("mock_program", program_id, 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] @@ -9283,14 +9347,35 @@ mod tests { Arc::get_mut(&mut bank) .unwrap() - .add_native_program("mock_program", &program_id); + .add_native_program("mock_program", &program_id, false); assert_eq!(bank.get_account_modified_slot(&program_id).unwrap().1, slot); let mut bank = Arc::new(new_from_parent(&bank)); Arc::get_mut(&mut bank) .unwrap() - .add_native_program("mock_program", &program_id); + .add_native_program("mock_program", &program_id, false); assert_eq!(bank.get_account_modified_slot(&program_id).unwrap().1, slot); + + let mut bank = Arc::new(new_from_parent(&bank)); + // When replacing native_program, name must change to disambiguate from repeated + // invocations. + Arc::get_mut(&mut bank) + .unwrap() + .add_native_program("mock_program v2", &program_id, true); + assert_eq!( + bank.get_account_modified_slot(&program_id).unwrap().1, + bank.slot() + ); + + let mut bank = Arc::new(new_from_parent(&bank)); + Arc::get_mut(&mut bank) + .unwrap() + .add_native_program("mock_program v2", &program_id, true); + // replacing with same name shouldn't update account + assert_eq!( + bank.get_account_modified_slot(&program_id).unwrap().1, + bank.parent_slot() + ); } #[test] @@ -9306,16 +9391,35 @@ mod tests { let slot = 123; let program_id = Pubkey::from_str("CiXgo2KHKSDmDnV1F6B69eWFgNAPiSBjjYvfB4cvRNre").unwrap(); - let mut bank = Arc::new(Bank::new_from_parent( + let bank = Bank::new_from_parent( &Arc::new(Bank::new(&genesis_config)), &Pubkey::default(), slot, - )); + ); bank.freeze(); - Arc::get_mut(&mut bank) - .unwrap() - .add_native_program("mock_program", &program_id); + bank.add_native_program("mock_program", &program_id, false); + } + + #[test] + #[should_panic( + expected = "There is no account to replace with native program (mock_program, \ + CiXgo2KHKSDmDnV1F6B69eWFgNAPiSBjjYvfB4cvRNre)." + )] + fn test_add_native_program_replace_none() { + use std::str::FromStr; + let (genesis_config, _mint_keypair) = create_genesis_config(100_000); + + let slot = 123; + let program_id = Pubkey::from_str("CiXgo2KHKSDmDnV1F6B69eWFgNAPiSBjjYvfB4cvRNre").unwrap(); + + let bank = Bank::new_from_parent( + &Arc::new(Bank::new(&genesis_config)), + &Pubkey::default(), + slot, + ); + + bank.add_native_program("mock_program", &program_id, true); } #[test] diff --git a/runtime/src/builtins.rs b/runtime/src/builtins.rs index 8afbe1b88c..bf0d028bba 100644 --- a/runtime/src/builtins.rs +++ b/runtime/src/builtins.rs @@ -30,8 +30,14 @@ fn genesis_builtins() -> Vec { ] } +#[derive(AbiExample, Debug, Clone)] +pub enum ActivationType { + NewProgram, + NewVersion, +} + /// Builtin programs activated dynamically by feature -fn feature_builtins() -> Vec<(Builtin, Pubkey)> { +fn feature_builtins() -> Vec<(Builtin, Pubkey, ActivationType)> { vec![( Builtin::new( "secp256k1_program", @@ -39,6 +45,7 @@ fn feature_builtins() -> Vec<(Builtin, Pubkey)> { solana_secp256k1_program::process_instruction, ), feature_set::secp256k1_program_enabled::id(), + ActivationType::NewProgram, )] } diff --git a/runtime/tests/noop.rs b/runtime/tests/noop.rs index f7233830b5..47026cc9fa 100644 --- a/runtime/tests/noop.rs +++ b/runtime/tests/noop.rs @@ -10,7 +10,7 @@ fn test_program_native_noop() { let (genesis_config, alice_keypair) = create_genesis_config(50); let program_id = solana_sdk::pubkey::new_rand(); let bank = Bank::new(&genesis_config); - bank.add_native_program("solana_noop_program", &program_id); + bank.add_native_program("solana_noop_program", &program_id, false); // Call user program let instruction = create_invoke_instruction(alice_keypair.pubkey(), program_id, &1u8);