Fix - FailedVerification and Closed tombstones (solana-labs#419)

* Only the verifier can cause FailedVerification, everything else is Closed

* Removes the environments parameter from load_program_accounts().

* cargo fmt

* Simplify invocation of deployed program

* Attempt to invoke a program before it is deployed

* Attempt to invoke a buffer before it is used in a deployment

* Escalates Option return value of load_program_accounts() to load_program_with_pubkey().

* Review feedback

ledger-tool/src/program.rs

@@ -522,7 +522,11 @@ pub fn program(ledger_path: &Path, matches: &ArgMatches<'_>) {
     let mut loaded_programs =
         bank.new_program_cache_for_tx_batch_for_slot(bank.slot() + DELAY_VISIBILITY_SLOT_OFFSET);
     for key in cached_account_keys {
-        loaded_programs.replenish(key, bank.load_program(&key, false, bank.epoch()));
+        loaded_programs.replenish(
+            key,
+            bank.load_program(&key, false, bank.epoch())
+                .expect("Couldn't find program account"),
+        );
         debug!("Loaded program {}", key);
     }
     invoke_context.programs_loaded_for_tx_batch = &loaded_programs;

program-runtime/src/loaded_programs.rs

@@ -63,11 +63,11 @@ pub trait ForkGraph {
 /// Actual payload of [LoadedProgram].
 #[derive(Default)]
 pub enum LoadedProgramType {
-    /// Tombstone for programs which did not pass the verifier.
-    ///
-    /// These can potentially come back alive if the environment changes.
+    /// Tombstone for programs which currently do not pass the verifier but could if the feature set changed.
     FailedVerification(ProgramRuntimeEnvironment),
-    /// Tombstone for programs which were explicitly undeployed / closed.
+    /// Tombstone for programs that were either explicitly closed or never deployed.
+    ///
+    /// It's also used for accounts belonging to program loaders, that don't actually contain program code (e.g. buffer accounts for LoaderV3 programs).
     #[default]
     Closed,
     /// Tombstone for programs which have recently been modified but the new version is not visible yet.
@@ -776,12 +776,17 @@ impl<FG: ForkGraph> ProgramCache<FG> {
             Ok(index) => {
                 let existing = slot_versions.get_mut(index).unwrap();
                 match (&existing.program, &entry.program) {
+                    // Add test for Closed => Loaded transition in same slot
                     (LoadedProgramType::Builtin(_), LoadedProgramType::Builtin(_))
+                    | (LoadedProgramType::Closed, LoadedProgramType::LegacyV0(_))
+                    | (LoadedProgramType::Closed, LoadedProgramType::LegacyV1(_))
+                    | (LoadedProgramType::Closed, LoadedProgramType::Typed(_))
                     | (LoadedProgramType::Unloaded(_), LoadedProgramType::LegacyV0(_))
                     | (LoadedProgramType::Unloaded(_), LoadedProgramType::LegacyV1(_))
                     | (LoadedProgramType::Unloaded(_), LoadedProgramType::Typed(_)) => {}
                     #[cfg(test)]
-                    (LoadedProgramType::Unloaded(_), LoadedProgramType::TestLoaded(_)) => {}
+                    (LoadedProgramType::Closed, LoadedProgramType::TestLoaded(_))
+                    | (LoadedProgramType::Unloaded(_), LoadedProgramType::TestLoaded(_)) => {}
                     _ => {
                         // Something is wrong, I can feel it ...
                         error!("ProgramCache::assign_program() failed key={:?} existing={:?} entry={:?}", key, slot_versions, entry);
@@ -1680,7 +1685,6 @@ mod tests {
     #[test_matrix(
         (
             LoadedProgramType::FailedVerification(Arc::new(BuiltinProgram::new_mock())),
-            LoadedProgramType::Closed,
             LoadedProgramType::TestLoaded(Arc::new(BuiltinProgram::new_mock())),
         ),
         (
@@ -1692,7 +1696,10 @@ mod tests {
         )
     )]
     #[test_matrix(
-        (LoadedProgramType::Unloaded(Arc::new(BuiltinProgram::new_mock())),),
+        (
+            LoadedProgramType::Closed,
+            LoadedProgramType::Unloaded(Arc::new(BuiltinProgram::new_mock())),
+        ),
         (
             LoadedProgramType::FailedVerification(Arc::new(BuiltinProgram::new_mock())),
             LoadedProgramType::Closed,
@@ -1739,6 +1746,10 @@ mod tests {
         );
     }
 
+    #[test_case(
+        LoadedProgramType::Closed,
+        LoadedProgramType::TestLoaded(Arc::new(BuiltinProgram::new_mock()))
+    )]
     #[test_case(
         LoadedProgramType::Unloaded(Arc::new(BuiltinProgram::new_mock())),
         LoadedProgramType::TestLoaded(Arc::new(BuiltinProgram::new_mock()))

runtime/src/bank.rs

@@ -1253,16 +1253,19 @@ impl Bank {
                 {
                     let effective_epoch = program_cache.latest_root_epoch.saturating_add(1);
                     drop(program_cache);
-                    let recompiled = new.load_program(&key, false, effective_epoch);
-                    recompiled
-                        .tx_usage_counter
-                        .fetch_add(program_to_recompile.tx_usage_counter.load(Relaxed), Relaxed);
-                    recompiled
-                        .ix_usage_counter
-                        .fetch_add(program_to_recompile.ix_usage_counter.load(Relaxed), Relaxed);
-                    let mut program_cache =
-                        new.transaction_processor.program_cache.write().unwrap();
-                    program_cache.assign_program(key, recompiled);
+                    if let Some(recompiled) = new.load_program(&key, false, effective_epoch) {
+                        recompiled.tx_usage_counter.fetch_add(
+                            program_to_recompile.tx_usage_counter.load(Relaxed),
+                            Relaxed,
+                        );
+                        recompiled.ix_usage_counter.fetch_add(
+                            program_to_recompile.ix_usage_counter.load(Relaxed),
+                            Relaxed,
+                        );
+                        let mut program_cache =
+                            new.transaction_processor.program_cache.write().unwrap();
+                        program_cache.assign_program(key, recompiled);
+                    }
                 }
             } else if new.epoch() != program_cache.latest_root_epoch
                 || slot_index.saturating_add(slots_in_recompilation_phase) >= slots_in_epoch
@@ -6886,7 +6889,7 @@ impl Bank {
         pubkey: &Pubkey,
         reload: bool,
         effective_epoch: Epoch,
-    ) -> Arc<LoadedProgram> {
+    ) -> Option<Arc<LoadedProgram>> {
         self.transaction_processor
             .load_program_with_pubkey(self, pubkey, reload, effective_epoch)
     }

runtime/src/bank/tests.rs

@@ -39,11 +39,7 @@ use {
         compute_budget::ComputeBudget,
         compute_budget_processor::{self, MAX_COMPUTE_UNIT_LIMIT},
         declare_process_instruction,
-        invoke_context::mock_process_instruction,
-        loaded_programs::{
-            LoadedProgram, LoadedProgramType, LoadedProgramsForTxBatch,
-            DELAY_VISIBILITY_SLOT_OFFSET,
-        },
+        loaded_programs::{LoadedProgram, LoadedProgramType, LoadedProgramsForTxBatch},
         prioritization_fee::{PrioritizationFeeDetails, PrioritizationFeeType},
         timings::ExecuteTimings,
     },
@@ -7141,7 +7137,7 @@ fn test_bank_load_program() {
     programdata_account.set_rent_epoch(1);
     bank.store_account_and_update_capitalization(&key1, &program_account);
     bank.store_account_and_update_capitalization(&programdata_key, &programdata_account);
-    let program = bank.load_program(&key1, false, bank.epoch());
+    let program = bank.load_program(&key1, false, bank.epoch()).unwrap();
     assert_matches!(program.program, LoadedProgramType::LegacyV1(_));
     assert_eq!(
         program.account_size,
@@ -7167,6 +7163,26 @@ fn test_bpf_loader_upgradeable_deploy_with_max_len() {
     );
     let upgrade_authority_keypair = Keypair::new();
 
+    // Invoke not yet deployed program
+    let instruction = Instruction::new_with_bytes(program_keypair.pubkey(), &[], Vec::new());
+    let invocation_message = Message::new(&[instruction], Some(&mint_keypair.pubkey()));
+    let binding = mint_keypair.insecure_clone();
+    let transaction = Transaction::new(
+        &[&binding],
+        invocation_message.clone(),
+        bank.last_blockhash(),
+    );
+    assert_eq!(
+        bank.process_transaction(&transaction),
+        Err(TransactionError::ProgramAccountNotFound),
+    );
+    {
+        // Make sure it is not in the cache because the account owner is not a loader
+        let program_cache = bank.transaction_processor.program_cache.read().unwrap();
+        let slot_versions = program_cache.get_slot_versions_for_tests(&program_keypair.pubkey());
+        assert!(slot_versions.is_empty());
+    }
+
     // Load program file
     let mut file = File::open("../programs/bpf_loader/test_elfs/out/noop_aligned.so")
         .expect("file open failed");
@@ -7214,6 +7230,28 @@ fn test_bpf_loader_upgradeable_deploy_with_max_len() {
         &bpf_loader_upgradeable::id(),
     );
 
+    // Test buffer invocation
+    bank.store_account(&buffer_address, &buffer_account);
+    let instruction = Instruction::new_with_bytes(buffer_address, &[], Vec::new());
+    let message = Message::new(&[instruction], Some(&mint_keypair.pubkey()));
+    let transaction = Transaction::new(&[&binding], message, bank.last_blockhash());
+    assert_eq!(
+        bank.process_transaction(&transaction),
+        Err(TransactionError::InstructionError(
+            0,
+            InstructionError::InvalidAccountData,
+        )),
+    );
+    {
+        let program_cache = bank.transaction_processor.program_cache.read().unwrap();
+        let slot_versions = program_cache.get_slot_versions_for_tests(&buffer_address);
+        assert_eq!(slot_versions.len(), 1);
+        assert!(matches!(
+            slot_versions[0].program,
+            LoadedProgramType::Closed,
+        ));
+    }
+
     // Test successful deploy
     let payer_base_balance = LAMPORTS_PER_SOL;
     let deploy_fees = {
@@ -7231,7 +7269,6 @@ fn test_bpf_loader_upgradeable_deploy_with_max_len() {
             &system_program::id(),
         ),
     );
-    bank.store_account(&buffer_address, &buffer_account);
     bank.store_account(&program_keypair.pubkey(), &AccountSharedData::default());
     bank.store_account(&programdata_address, &AccountSharedData::default());
     let message = Message::new(
@@ -7296,30 +7333,15 @@ fn test_bpf_loader_upgradeable_deploy_with_max_len() {
         assert_eq!(*elf.get(i).unwrap(), *byte);
     }
 
-    let loaded_program = bank.load_program(&program_keypair.pubkey(), false, bank.epoch());
+    // Advance the bank so that the program becomes effective
+    goto_end_of_slot(bank.clone());
+    let bank = bank_client
+        .advance_slot(1, bank_forks.as_ref(), &mint_keypair.pubkey())
+        .unwrap();
 
-    // Invoke deployed program
-    mock_process_instruction(
-        &bpf_loader_upgradeable::id(),
-        vec![0, 1],
-        &[],
-        vec![
-            (programdata_address, post_programdata_account),
-            (program_keypair.pubkey(), post_program_account),
-        ],
-        Vec::new(),
-        Ok(()),
-        solana_bpf_loader_program::Entrypoint::vm,
-        |invoke_context| {
-            invoke_context
-                .programs_modified_by_tx
-                .set_slot_for_tests(bank.slot() + DELAY_VISIBILITY_SLOT_OFFSET);
-            invoke_context
-                .programs_modified_by_tx
-                .replenish(program_keypair.pubkey(), loaded_program.clone());
-        },
-        |_invoke_context| {},
-    );
+    // Invoke the deployed program
+    let transaction = Transaction::new(&[&binding], invocation_message, bank.last_blockhash());
+    assert!(bank.process_transaction(&transaction).is_ok());
 
     // Test initialized program account
     bank.clear_signatures();