reorder load_account and program_cache replenish

Author: HaoranYi

runtime/src/bank.rs

@@ -6885,6 +6885,17 @@ impl TransactionProcessingCallback for Bank {
             .ok()
     }
 
+    fn load_account_with(
+        &self,
+        pubkey: &Pubkey,
+        callback: impl for<'a> Fn(&'a AccountSharedData) -> bool,
+    ) -> Option<(AccountSharedData, Slot)> {
+        self.rc
+            .accounts
+            .accounts_db
+            .load_account_with(&self.ancestors, pubkey, callback)
+    }
+
     fn get_account_shared_data(&self, pubkey: &Pubkey) -> Option<AccountSharedData> {
         self.rc
             .accounts

runtime/src/bank/builtins/core_bpf_migration/mod.rs

@@ -249,7 +249,7 @@ impl Bank {
         let source = SourceBuffer::new_checked(self, &config.source_buffer_address)?;
 
         // Attempt serialization first before modifying the bank.
-        let new_target_program_account = self.new_target_program_account(&target)?;
+        let mut new_target_program_account = self.new_target_program_account(&target)?;
         let new_target_program_data_account =
             self.new_target_program_data_account(&source, config.upgrade_authority_address)?;
 
@@ -275,6 +275,12 @@ impl Bank {
             new_target_program_data_account.data(),
         )?;
 
+        // After successfully deployed the `Builtin` program, we need to mark
+        // the builtin program account as executable. This is required for
+        // transaction account loading, when "fake program account" optimization
+        // is disabled.
+        new_target_program_account.set_executable(true);
+
         // Calculate the lamports to burn.
         // The target program account will be replaced, so burn its lamports.
         // The source buffer account will be cleared, so burn its lamports.

runtime/src/bank/tests.rs

@@ -7212,12 +7212,10 @@ fn test_bpf_loader_upgradeable_deploy_with_max_len() {
         invocation_message.clone(),
         bank.last_blockhash(),
     );
+    // See comments below for Test buffer invocation.
     assert_eq!(
         bank.process_transaction(&transaction),
-        Err(TransactionError::InstructionError(
-            0,
-            InstructionError::InvalidAccountData
-        )),
+        Err(TransactionError::InvalidProgramForExecution),
     );
     {
         let program_cache = bank.transaction_processor.program_cache.read().unwrap();
@@ -7236,12 +7234,14 @@ fn test_bpf_loader_upgradeable_deploy_with_max_len() {
     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());
+    // This is a slightly change of behavior for this PR. In the old code, we
+    // short-circuit buffer account to fake it and mark it executable. So it
+    // passed program execution check but failed with `Err(InstructionError(0, InvalidAccountData))`.
+    // With this PR, we don't do that. The buffer account will NOT be executable. A different error is throw `Err(InvalidProgramForExecution)`.
+    // This will NOT break consensus fortunately! Changing to `Err(InvalidProgramForExecution)` won't affect bank hash!
     assert_eq!(
         bank.process_transaction(&transaction),
-        Err(TransactionError::InstructionError(
-            0,
-            InstructionError::InvalidAccountData,
-        )),
+        Err(TransactionError::InvalidProgramForExecution),
     );
     {
         let program_cache = bank.transaction_processor.program_cache.read().unwrap();