Fix - LoadedProgramType::Closed (#31922)

* Makes Bank::load_program() return correct tombstones.

* Removes early TX failure caused by closed and invalid programs.

* Adjusts the feature gate of simplify_writable_program_account_check.

ledger-tool/src/program.rs

@@ -15,9 +15,7 @@ use {
     },
     solana_program_runtime::{
         invoke_context::InvokeContext,
-        loaded_programs::{
+        loaded_programs::{LoadProgramMetrics, LoadedProgramType, DELAY_VISIBILITY_SLOT_OFFSET},
-            LoadProgramMetrics, LoadedProgram, LoadedProgramType, DELAY_VISIBILITY_SLOT_OFFSET,
-        },
         with_mock_invoke_context,
     },
     solana_rbpf::{
@@ -540,22 +538,8 @@ pub fn program(ledger_path: &Path, matches: &ArgMatches<'_>) {
     let mut loaded_programs =
         LoadedProgramsForTxBatch::new(bank.slot() + DELAY_VISIBILITY_SLOT_OFFSET);
     for key in cached_account_keys {
-        let program = bank.load_program(&key).unwrap_or_else(|err| {
+        loaded_programs.replenish(key, bank.load_program(&key));
-            // Create a tombstone for the program in the cache
-            debug!("Failed to load program {}, error {:?}", key, err);
-            Arc::new(LoadedProgram::new_tombstone(
-                0,
-                LoadedProgramType::FailedVerification(
-                    bank.loaded_programs_cache
-                        .read()
-                        .unwrap()
-                        .program_runtime_environment_v1
-                        .clone(),
-                ),
-            ))
-        });
         debug!("Loaded program {}", key);
-        loaded_programs.replenish(key, program);
     }
     invoke_context.programs_loaded_for_tx_batch = &loaded_programs;
 

runtime/src/accounts.rs

@@ -26,16 +26,15 @@ use {
     solana_address_lookup_table_program::{error::AddressLookupError, state::AddressLookupTable},
     solana_program_runtime::{
         compute_budget::{self, ComputeBudget},
-        loaded_programs::{LoadedProgram, LoadedProgramType, LoadedProgramsForTxBatch},
+        loaded_programs::LoadedProgramsForTxBatch,
     },
     solana_sdk::{
         account::{Account, AccountSharedData, ReadableAccount, WritableAccount},
         account_utils::StateMut,
-        bpf_loader_upgradeable,
+        bpf_loader_upgradeable::{self, UpgradeableLoaderState},
         clock::{BankId, Slot},
         feature_set::{
-            self, add_set_tx_loaded_accounts_data_size_instruction,
+            self, add_set_tx_loaded_accounts_data_size_instruction, enable_request_heap_frame_ix,
-            delay_visibility_of_program_deployment, enable_request_heap_frame_ix,
             include_loaded_accounts_data_size_in_fee_calculation,
             remove_congestion_multiplier_from_fee_calculation, remove_deprecated_request_unit_ix,
             simplify_writable_program_account_check, use_default_units_in_fee_calculation,
@@ -295,27 +294,8 @@ impl Accounts {
 
     fn account_shared_data_from_program(
         key: &Pubkey,
-        feature_set: &FeatureSet,
-        program: &LoadedProgram,
         program_accounts: &HashMap<Pubkey, (&Pubkey, u64)>,
     ) -> Result<AccountSharedData> {
-        // Check for tombstone
-        let result = match &program.program {
-            LoadedProgramType::FailedVerification(_) | LoadedProgramType::Closed => {
-                Err(TransactionError::InvalidProgramForExecution)
-            }
-            LoadedProgramType::DelayVisibility => {
-                debug_assert!(feature_set.is_active(&delay_visibility_of_program_deployment::id()));
-                Err(TransactionError::InvalidProgramForExecution)
-            }
-            _ => Ok(()),
-        };
-        if feature_set.is_active(&simplify_writable_program_account_check::id()) {
-            // Currently CPI only fails if an execution is actually attempted. With this check it
-            // would also fail if a transaction just references an invalid program. So the checking
-            // of the result is being feature gated.
-            result?;
-        }
         // It's an executable program account. The program is already loaded in the cache.
         // So the account data is not needed. Return a dummy AccountSharedData with meta
         // information.
@@ -389,9 +369,12 @@ impl Accounts {
                         account_overrides.and_then(|overrides| overrides.get(key))
                     {
                         (account_override.data().len(), account_override.clone(), 0)
-                    } else if let Some(program) = (!instruction_account && !message.is_writable(i))
+                    } else if let Some(program) = (feature_set
-                        .then_some(())
+                        .is_active(&simplify_writable_program_account_check::id())
-                        .and_then(|_| loaded_programs.find(key))
+                        && !instruction_account
+                        && !message.is_writable(i))
+                    .then_some(())
+                    .and_then(|_| loaded_programs.find(key))
                     {
                         // This condition block does special handling for accounts that are passed
                         // as instruction account to any of the instructions in the transaction.
@@ -399,13 +382,8 @@ impl Accounts {
                         // (that are passed to the program as instruction accounts). So such accounts
                         // are needed to be loaded even though corresponding compiled program may
                         // already be present in the cache.
-                        Self::account_shared_data_from_program(
+                        Self::account_shared_data_from_program(key, program_accounts)
-                            key,
+                            .map(|program_account| (program.account_size, program_account, 0))?
-                            feature_set,
-                            program.as_ref(),
-                            program_accounts,
-                        )
-                        .map(|program_account| (program.account_size, program_account, 0))?
                     } else {
                         self.accounts_db
                             .load_with_fixed_root(ancestors, key)
@@ -461,18 +439,39 @@ impl Accounts {
                         validated_fee_payer = true;
                     }
 
-                    if bpf_loader_upgradeable::check_id(account.owner()) {
+                    if !feature_set.is_active(&simplify_writable_program_account_check::id()) {
-                        if !feature_set.is_active(&simplify_writable_program_account_check::id())
+                        if bpf_loader_upgradeable::check_id(account.owner()) {
-                            && message.is_writable(i)
+                            if message.is_writable(i) && !message.is_upgradeable_loader_present() {
-                            && !message.is_upgradeable_loader_present()
+                                error_counters.invalid_writable_account += 1;
-                        {
+                                return Err(TransactionError::InvalidWritableAccount);
+                            }
+
+                            if account.executable() {
+                                // The upgradeable loader requires the derived ProgramData account
+                                if let Ok(UpgradeableLoaderState::Program {
+                                    programdata_address,
+                                }) = account.state()
+                                {
+                                    if self
+                                        .accounts_db
+                                        .load_with_fixed_root(ancestors, &programdata_address)
+                                        .is_none()
+                                    {
+                                        error_counters.account_not_found += 1;
+                                        return Err(TransactionError::ProgramAccountNotFound);
+                                    }
+                                } else {
+                                    error_counters.invalid_program_for_execution += 1;
+                                    return Err(TransactionError::InvalidProgramForExecution);
+                                }
+                            }
+                        } else if account.executable() && message.is_writable(i) {
                             error_counters.invalid_writable_account += 1;
                             return Err(TransactionError::InvalidWritableAccount);
                         }
-                    } else if account.executable() && message.is_writable(i) {
+                    }
-                        error_counters.invalid_writable_account += 1;
+
-                        return Err(TransactionError::InvalidWritableAccount);
+                    if in_reward_interval
-                    } else if in_reward_interval
                         && message.is_writable(i)
                         && solana_stake_program::check_id(account.owner())
                     {
@@ -1491,7 +1490,6 @@ mod tests {
         },
         solana_sdk::{
             account::{AccountSharedData, WritableAccount},
-            bpf_loader_upgradeable::UpgradeableLoaderState,
             compute_budget::ComputeBudgetInstruction,
             epoch_schedule::EpochSchedule,
             genesis_config::ClusterType,
@@ -2490,8 +2488,12 @@ mod tests {
             instructions,
         );
         let tx = Transaction::new(&[&keypair], message.clone(), Hash::default());
-        let loaded_accounts =
+        let loaded_accounts = load_accounts_with_excluded_features(
-            load_accounts_with_excluded_features(tx, &accounts, &mut error_counters, None);
+            tx,
+            &accounts,
+            &mut error_counters,
+            Some(&[simplify_writable_program_account_check::id()]),
+        );
 
         assert_eq!(error_counters.invalid_writable_account, 1);
         assert_eq!(loaded_accounts.len(), 1);
@@ -2504,8 +2506,12 @@ mod tests {
         message.account_keys = vec![key0, key1, key2]; // revert key change
         message.header.num_readonly_unsigned_accounts = 2; // mark both executables as readonly
         let tx = Transaction::new(&[&keypair], message, Hash::default());
-        let loaded_accounts =
+        let loaded_accounts = load_accounts_with_excluded_features(
-            load_accounts_with_excluded_features(tx, &accounts, &mut error_counters, None);
+            tx,
+            &accounts,
+            &mut error_counters,
+            Some(&[simplify_writable_program_account_check::id()]),
+        );
 
         assert_eq!(error_counters.invalid_writable_account, 1);
         assert_eq!(loaded_accounts.len(), 1);

runtime/src/bank.rs

@@ -4106,11 +4106,14 @@ impl Bank {
         }
     }
 
-    pub fn load_program(&self, pubkey: &Pubkey) -> Result<Arc<LoadedProgram>> {
+    pub fn load_program(&self, pubkey: &Pubkey) -> Arc<LoadedProgram> {
         let program = if let Some(program) = self.get_account_with_fixed_root(pubkey) {
             program
         } else {
-            return Err(TransactionError::ProgramAccountNotFound);
+            return Arc::new(LoadedProgram::new_tombstone(
+                self.slot,
+                LoadedProgramType::Closed,
+            ));
         };
         let mut transaction_accounts = vec![(*pubkey, program)];
         let is_upgradeable_loader =
@@ -4125,10 +4128,16 @@ impl Bank {
                 {
                     transaction_accounts.push((programdata_address, programdata_account));
                 } else {
-                    return Err(TransactionError::ProgramAccountNotFound);
+                    return Arc::new(LoadedProgram::new_tombstone(
+                        self.slot,
+                        LoadedProgramType::Closed,
+                    ));
                 }
             } else {
-                return Err(TransactionError::ProgramAccountNotFound);
+                return Arc::new(LoadedProgram::new_tombstone(
+                    self.slot,
+                    LoadedProgramType::Closed,
+                ));
             }
         }
         let mut transaction_context = TransactionContext::new(
@@ -4137,24 +4146,20 @@ impl Bank {
             1,
             1,
         );
-        let instruction_context = transaction_context
+        let instruction_context = transaction_context.get_next_instruction_context().unwrap();
-            .get_next_instruction_context()
-            .map_err(|err| TransactionError::InstructionError(0, err))?;
         instruction_context.configure(if is_upgradeable_loader { &[0, 1] } else { &[0] }, &[], &[]);
-        transaction_context
+        transaction_context.push().unwrap();
-            .push()
-            .map_err(|err| TransactionError::InstructionError(0, err))?;
         let instruction_context = transaction_context
             .get_current_instruction_context()
-            .map_err(|err| TransactionError::InstructionError(0, err))?;
+            .unwrap();
         let program = instruction_context
             .try_borrow_program_account(&transaction_context, 0)
-            .map_err(|err| TransactionError::InstructionError(0, err))?;
+            .unwrap();
         let programdata = if is_upgradeable_loader {
             Some(
                 instruction_context
                     .try_borrow_program_account(&transaction_context, 1)
-                    .map_err(|err| TransactionError::InstructionError(0, err))?,
+                    .unwrap(),
             )
         } else {
             None
@@ -4170,14 +4175,19 @@ impl Bank {
             None, // log_collector
             &program,
             programdata.as_ref().unwrap_or(&program),
-            program_runtime_environment_v1,
+            program_runtime_environment_v1.clone(),
         )
         .map(|(loaded_program, metrics)| {
             let mut timings = ExecuteDetailsTimings::default();
             metrics.submit_datapoint(&mut timings);
             loaded_program
         })
-        .map_err(|err| TransactionError::InstructionError(0, err))
+        .unwrap_or_else(|_| {
+            Arc::new(LoadedProgram::new_tombstone(
+                self.slot,
+                LoadedProgramType::FailedVerification(program_runtime_environment_v1),
+            ))
+        })
     }
 
     pub fn clear_program_cache(&self) {
@@ -4423,20 +4433,7 @@ impl Bank {
         let missing_programs: Vec<(Pubkey, Arc<LoadedProgram>)> = missing_programs
             .iter()
             .map(|(key, count)| {
-                let program = self.load_program(key).unwrap_or_else(|err| {
+                let program = self.load_program(key);
-                    // Create a tombstone for the program in the cache
-                    debug!("Failed to load program {}, error {:?}", key, err);
-                    Arc::new(LoadedProgram::new_tombstone(
-                        self.slot,
-                        LoadedProgramType::FailedVerification(
-                            self.loaded_programs_cache
-                                .read()
-                                .unwrap()
-                                .program_runtime_environment_v1
-                                .clone(),
-                        ),
-                    ))
-                });
                 program.tx_usage_counter.store(*count, Ordering::Relaxed);
                 (*key, program)
             })

runtime/src/bank/tests.rs

@@ -7353,8 +7353,6 @@ fn test_bank_load_program() {
     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);
-    assert!(program.is_ok());
-    let program = program.unwrap();
     assert!(matches!(program.program, LoadedProgramType::LegacyV1(_)));
     assert_eq!(
         program.account_size,
@@ -7368,7 +7366,7 @@ fn test_bpf_loader_upgradeable_deploy_with_max_len() {
     let mut bank = Bank::new_for_tests(&genesis_config);
     bank.feature_set = Arc::new(FeatureSet::all_enabled());
     let bank = Arc::new(bank);
-    let bank_client = BankClient::new_shared(&bank);
+    let mut bank_client = BankClient::new_shared(&bank);
 
     // Setup keypairs and addresses
     let payer_keypair = Keypair::new();
@@ -7509,9 +7507,7 @@ fn test_bpf_loader_upgradeable_deploy_with_max_len() {
         assert_eq!(*elf.get(i).unwrap(), *byte);
     }
 
-    let loaded_program = bank
+    let loaded_program = bank.load_program(&program_keypair.pubkey());
-        .load_program(&program_keypair.pubkey())
-        .expect("Failed to load the program");
 
     // Invoke deployed program
     mock_process_instruction(
@@ -7539,6 +7535,7 @@ fn test_bpf_loader_upgradeable_deploy_with_max_len() {
     // Test initialized program account
     bank.clear_signatures();
     bank.store_account(&buffer_address, &buffer_account);
+    let bank = bank_client.advance_slot(1, &mint_keypair.pubkey()).unwrap();
     let message = Message::new(
         &[Instruction::new_with_bincode(
             bpf_loader_upgradeable::id(),