Fix - Reloading of unload entries after finishing the recompilation p…

…hase (#1199)

* Adds reproducer to test_feature_activation_loaded_programs_epoch_transition.

* Differentiate entries by environment instead of adjusting the effective slot.

* Fixes tests of assign_program().

* Fixes env order in test_feature_activation_loaded_programs_recompilation_phase().

* Turns test_load_program_effective_slot() into test_load_program_environment().

* Adds comments inside ProgramCache::assign_program().

(cherry picked from commit 979f619)

# Conflicts:
#	program-runtime/src/loaded_programs.rs
#	runtime/src/bank/tests.rs
#	svm/src/program_loader.rs

program-runtime/src/loaded_programs.rs

@@ -718,12 +718,43 @@ impl<FG: ForkGraph> LoadedPrograms<FG> {
 
     /// Insert a single entry. It's typically called during transaction loading,
     /// when the cache doesn't contain the entry corresponding to program `key`.
+<<<<<<< HEAD
     pub fn assign_program(&mut self, key: Pubkey, entry: Arc<LoadedProgram>) -> bool {
         let slot_versions = &mut self.entries.entry(key).or_default().slot_versions;
+=======
+    pub fn assign_program(&mut self, key: Pubkey, entry: Arc<ProgramCacheEntry>) -> bool {
+        debug_assert!(!matches!(
+            &entry.program,
+            ProgramCacheEntryType::DelayVisibility
+        ));
+        // 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();
+>>>>>>> 979f619077 (Fix - Reloading of unload entries after finishing the recompilation phase (#1199))
         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();
@@ -1762,8 +1793,47 @@ mod tests {
         }
     }
 
+<<<<<<< HEAD
     #[test]
     fn test_assign_program_tombstones() {
+=======
+    #[test_matrix(
+        (
+            ProgramCacheEntryType::Closed,
+            ProgramCacheEntryType::FailedVerification(get_mock_env()),
+            new_loaded_entry(get_mock_env()),
+        ),
+        (
+            ProgramCacheEntryType::FailedVerification(get_mock_env()),
+            ProgramCacheEntryType::Closed,
+            ProgramCacheEntryType::Unloaded(get_mock_env()),
+            new_loaded_entry(get_mock_env()),
+            ProgramCacheEntryType::Builtin(BuiltinProgram::new_mock()),
+        )
+    )]
+    #[test_matrix(
+        (
+            ProgramCacheEntryType::Unloaded(get_mock_env()),
+        ),
+        (
+            ProgramCacheEntryType::FailedVerification(get_mock_env()),
+            ProgramCacheEntryType::Closed,
+            ProgramCacheEntryType::Unloaded(get_mock_env()),
+            ProgramCacheEntryType::Builtin(BuiltinProgram::new_mock()),
+        )
+    )]
+    #[test_matrix(
+        (ProgramCacheEntryType::Builtin(BuiltinProgram::new_mock()),),
+        (
+            ProgramCacheEntryType::FailedVerification(get_mock_env()),
+            ProgramCacheEntryType::Closed,
+            ProgramCacheEntryType::Unloaded(get_mock_env()),
+            new_loaded_entry(get_mock_env()),
+        )
+    )]
+    #[should_panic(expected = "Unexpected replacement of an entry")]
+    fn test_assign_program_failure(old: ProgramCacheEntryType, new: ProgramCacheEntryType) {
+>>>>>>> 979f619077 (Fix - Reloading of unload entries after finishing the recompilation phase (#1199))
         let mut cache = new_mock_cache::<TestForkGraph>();
         let program1 = Pubkey::new_unique();
         let env = cache.environments.program_runtime_v1.clone();

runtime/src/bank/tests.rs

@@ -12117,8 +12117,44 @@ fn test_feature_activation_loaded_programs_recompilation_phase() {
     // Advance to next epoch, which starts the recompilation phase
     let bank = new_from_parent_next_epoch(bank, bank_forks.as_ref(), 1);
 
+<<<<<<< HEAD
     // Execute after feature is enabled to check it was filtered out and reverified.
     let result_with_feature_enabled = bank.process_transaction(&transaction2);
+=======
+    // Advance the bank to recompile the program.
+    {
+        let program_cache = bank.transaction_processor.program_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(),
+            &current_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.transaction_processor.program_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(),
+            &current_env
+        ));
+    }
+
+    // Advance the bank to cross the epoch boundary and activate the feature.
+    goto_end_of_slot(bank.clone());
+    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, bank.last_blockhash());
+    let result_with_feature_enabled = bank.process_transaction(&transaction);
+>>>>>>> 979f619077 (Fix - Reloading of unload entries after finishing the recompilation phase (#1199))
     assert_eq!(
         result_with_feature_enabled,
         Err(TransactionError::InstructionError(
@@ -12178,6 +12214,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.transaction_processor.program_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());
 }