From ddaf56de7172708e31ba0eda3ce12e8cfa8d2a94 Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Mon, 13 May 2024 19:14:57 +0200 Subject: [PATCH] v1.18: Fix - Reloading of unload entries after finishing the recompilation phase (backport of #1199) (#1275) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Fix - Reloading of unload entries after finishing the recompilation phase (#1199) * Differentiate entries by environment instead of adjusting the effective slot. * Adds comments inside ProgramCache::assign_program(). * Adds reproducer to test_feature_activation_loaded_programs_epoch_transition. * Fixes env order in test_feature_activation_loaded_programs_recompilation_phase(). * Syncs both tests with master. --------- (cherry picked from commit 979f619077026c354e76010d22a5dae7d16129de) Co-authored-by: Alexander Meißner --- program-runtime/src/loaded_programs.rs | 22 ++++++ runtime/src/bank.rs | 15 ---- runtime/src/bank/tests.rs | 101 ++++++++++++++++++------- 3 files changed, 96 insertions(+), 42 deletions(-) diff --git a/program-runtime/src/loaded_programs.rs b/program-runtime/src/loaded_programs.rs index 055041f7976c92..7527ee45f08d48 100644 --- a/program-runtime/src/loaded_programs.rs +++ b/program-runtime/src/loaded_programs.rs @@ -719,11 +719,33 @@ impl LoadedPrograms { /// Insert a single entry. It's typically called during transaction loading, /// when the cache doesn't contain the entry corresponding to program `key`. pub fn assign_program(&mut self, key: Pubkey, entry: Arc) -> bool { + // This function always returns `true` during normal operation. + // Only during the recompilation phase this can return `false` + // for entries with `upcoming_environments`. + fn is_current_env( + environments: &ProgramRuntimeEnvironments, + env_opt: Option<&ProgramRuntimeEnvironment>, + ) -> bool { + env_opt + .map(|env| { + Arc::ptr_eq(env, &environments.program_runtime_v1) + || Arc::ptr_eq(env, &environments.program_runtime_v2) + }) + .unwrap_or(true) + } let slot_versions = &mut self.entries.entry(key).or_default().slot_versions; match slot_versions.binary_search_by(|at| { at.effective_slot .cmp(&entry.effective_slot) .then(at.deployment_slot.cmp(&entry.deployment_slot)) + .then( + // This `.then()` has no effect during normal operation. + // Only during the recompilation phase this does allow entries + // which only differ in their environment to be interleaved in `slot_versions`. + is_current_env(&self.environments, at.program.get_environment()).cmp( + &is_current_env(&self.environments, entry.program.get_environment()), + ), + ) }) { Ok(index) => { let existing = slot_versions.get_mut(index).unwrap(); diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 541f74e5c500a0..bc018b722b4fcf 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -4728,21 +4728,6 @@ impl Bank { let mut timings = ExecuteDetailsTimings::default(); load_program_metrics.submit_datapoint(&mut timings); - if !Arc::ptr_eq( - &environments.program_runtime_v1, - &loaded_programs_cache.environments.program_runtime_v1, - ) || !Arc::ptr_eq( - &environments.program_runtime_v2, - &loaded_programs_cache.environments.program_runtime_v2, - ) { - // There can be two entries per program when the environment changes. - // One for the old environment before the epoch boundary and one for the new environment after the epoch boundary. - // These two entries have the same deployment slot, so they must differ in their effective slot instead. - // This is done by setting the effective slot of the entry for the new environment to the epoch boundary. - loaded_program.effective_slot = loaded_program - .effective_slot - .max(self.epoch_schedule.get_first_slot_in_epoch(effective_epoch)); - } if let Some(recompile) = recompile { loaded_program.tx_usage_counter = AtomicU64::new(recompile.tx_usage_counter.load(Ordering::Relaxed)); diff --git a/runtime/src/bank/tests.rs b/runtime/src/bank/tests.rs index 9039954fd48eea..18c9727a991d73 100644 --- a/runtime/src/bank/tests.rs +++ b/runtime/src/bank/tests.rs @@ -12058,12 +12058,6 @@ fn test_feature_activation_loaded_programs_recompilation_phase() { .remove(&feature_set::reject_callx_r10::id()); let (root_bank, bank_forks) = Bank::new_with_bank_forks_for_tests(&genesis_config); - // Test a basic transfer - let amount = genesis_config.rent.minimum_balance(0); - let pubkey = solana_sdk::pubkey::new_rand(); - root_bank.transfer(amount, &mint_keypair, &pubkey).unwrap(); - assert_eq!(root_bank.get_balance(&pubkey), amount); - // Program Setup let program_keypair = Keypair::new(); let program_data = @@ -12077,26 +12071,19 @@ fn test_feature_activation_loaded_programs_recompilation_phase() { }); root_bank.store_account(&program_keypair.pubkey(), &program_account); - // Compose instruction using the desired program - let instruction1 = Instruction::new_with_bytes(program_keypair.pubkey(), &[], Vec::new()); - let message1 = Message::new(&[instruction1], Some(&mint_keypair.pubkey())); - let binding1 = mint_keypair.insecure_clone(); - let signers1 = vec![&binding1]; - let transaction1 = Transaction::new(&signers1, message1, root_bank.last_blockhash()); + // Compose message using the desired program. + let instruction = Instruction::new_with_bytes(program_keypair.pubkey(), &[], Vec::new()); + let message = Message::new(&[instruction], Some(&mint_keypair.pubkey())); + let binding = mint_keypair.insecure_clone(); + let signers = vec![&binding]; - // Advance the bank so the next transaction can be submitted. + // Advance the bank so that the program becomes effective. goto_end_of_slot(root_bank.clone()); let bank = new_from_parent_with_fork_next_slot(root_bank, bank_forks.as_ref()); - // Compose second instruction using the same program with a different block hash - let instruction2 = Instruction::new_with_bytes(program_keypair.pubkey(), &[], Vec::new()); - let message2 = Message::new(&[instruction2], Some(&mint_keypair.pubkey())); - let binding2 = mint_keypair.insecure_clone(); - let signers2 = vec![&binding2]; - let transaction2 = Transaction::new(&signers2, message2, bank.last_blockhash()); - - // Execute before feature is enabled to get program into the cache. - let result_without_feature_enabled = bank.process_transaction(&transaction1); + // Load the program with the old environment. + let transaction = Transaction::new(&signers, message.clone(), bank.last_blockhash()); + let result_without_feature_enabled = bank.process_transaction(&transaction); assert_eq!( result_without_feature_enabled, Err(TransactionError::InstructionError( @@ -12105,7 +12092,7 @@ fn test_feature_activation_loaded_programs_recompilation_phase() { )) ); - // Activate feature + // Schedule feature activation to trigger a change of environment at the epoch boundary. let feature_account_balance = std::cmp::max(genesis_config.rent.minimum_balance(Feature::size_of()), 1); bank.store_account( @@ -12113,12 +12100,57 @@ fn test_feature_activation_loaded_programs_recompilation_phase() { &feature::create_account(&Feature { activated_at: None }, feature_account_balance), ); + // Advance the bank to middle of epoch to start the recompilation phase. + goto_end_of_slot(bank.clone()); + let bank = new_bank_from_parent_with_bank_forks(&bank_forks, bank, &Pubkey::default(), 16); + let current_env = bank + .loaded_programs_cache + .read() + .unwrap() + .get_environments_for_epoch(0) + .program_runtime_v1 + .clone(); + let upcoming_env = bank + .loaded_programs_cache + .read() + .unwrap() + .get_environments_for_epoch(1) + .program_runtime_v1 + .clone(); + + // Advance the bank to recompile the program. + { + let program_cache = bank.loaded_programs_cache.read().unwrap(); + let slot_versions = program_cache.get_slot_versions_for_tests(&program_keypair.pubkey()); + assert_eq!(slot_versions.len(), 1); + assert!(Arc::ptr_eq( + slot_versions[0].program.get_environment().unwrap(), + ¤t_env + )); + } + goto_end_of_slot(bank.clone()); + let bank = new_from_parent_with_fork_next_slot(bank, bank_forks.as_ref()); + { + let program_cache = bank.loaded_programs_cache.write().unwrap(); + let slot_versions = program_cache.get_slot_versions_for_tests(&program_keypair.pubkey()); + assert_eq!(slot_versions.len(), 2); + assert!(Arc::ptr_eq( + slot_versions[0].program.get_environment().unwrap(), + &upcoming_env + )); + assert!(Arc::ptr_eq( + slot_versions[1].program.get_environment().unwrap(), + ¤t_env + )); + } + + // Advance the bank to cross the epoch boundary and activate the feature. goto_end_of_slot(bank.clone()); - // Advance to next epoch, which starts the recompilation phase - let bank = new_from_parent_next_epoch(bank, bank_forks.as_ref(), 1); + let bank = new_bank_from_parent_with_bank_forks(&bank_forks, bank, &Pubkey::default(), 33); - // Execute after feature is enabled to check it was filtered out and reverified. - let result_with_feature_enabled = bank.process_transaction(&transaction2); + // Load the program with the new environment. + let transaction = Transaction::new(&signers, message, bank.last_blockhash()); + let result_with_feature_enabled = bank.process_transaction(&transaction); assert_eq!( result_with_feature_enabled, Err(TransactionError::InstructionError( @@ -12178,6 +12210,21 @@ fn test_feature_activation_loaded_programs_epoch_transition() { let bank = new_bank_from_parent_with_bank_forks(&bank_forks, bank, &Pubkey::default(), 33); // Load the program with the new environment. + let transaction = Transaction::new(&signers, message.clone(), bank.last_blockhash()); + assert!(bank.process_transaction(&transaction).is_ok()); + + { + // Prune for rerooting and thus finishing the recompilation phase. + let mut program_cache = bank.loaded_programs_cache.write().unwrap(); + program_cache.prune(bank.slot(), bank.epoch()); + + // Unload all (which is only the entry with the new environment) + program_cache.sort_and_unload(percentage::Percentage::from(0)); + } + + // Reload the unloaded program with the new environment. + goto_end_of_slot(bank.clone()); + let bank = new_from_parent_with_fork_next_slot(bank, bank_forks.as_ref()); let transaction = Transaction::new(&signers, message, bank.last_blockhash()); assert!(bank.process_transaction(&transaction).is_ok()); }