refactor: adding builtin accounts (#7900)

runtime/src/bank.rs

@@ -4091,8 +4091,7 @@ impl Bank {
                         .unwrap_or(false)
                 };
                 if builtin.enable_feature_id.is_none() && !builtin_is_bpf(&builtin.program_id) {
-                    self.transaction_processor.add_builtin(
-                        self,
+                    self.add_builtin(
                         builtin.program_id,
                         builtin.name,
                         ProgramCacheEntry::new_builtin(0, builtin.name.len(), builtin.entrypoint),
@@ -5085,8 +5084,7 @@ impl Bank {
         program_id: Pubkey,
         builtin_function: BuiltinFunctionWithContext,
     ) {
-        self.transaction_processor.add_builtin(
-            self,
+        self.add_builtin(
             program_id,
             "mockup",
             ProgramCacheEntry::new_builtin(self.slot, 0, builtin_function),
@@ -5383,8 +5381,7 @@ impl Bank {
                     };
 
                 if should_enable_builtin_on_feature_transition {
-                    self.transaction_processor.add_builtin(
-                        self,
+                    self.add_builtin(
                         builtin.program_id,
                         builtin.name,
                         ProgramCacheEntry::new_builtin(
@@ -5521,8 +5518,54 @@ impl Bank {
     }
 
     pub fn add_builtin(&self, program_id: Pubkey, name: &str, builtin: ProgramCacheEntry) {
-        self.transaction_processor
-            .add_builtin(self, program_id, name, builtin)
+        debug!("Adding program {name} under {program_id:?}");
+        self.add_builtin_account(name, &program_id);
+        self.transaction_processor.add_builtin(program_id, builtin);
+        debug!("Added program {name} under {program_id:?}");
+    }
+
+    // NOTE: must hold idempotent for the same set of arguments
+    /// Add a builtin program account
+    fn add_builtin_account(&self, name: &str, program_id: &Pubkey) {
+        let existing_genuine_program =
+            self.get_account_with_fixed_root(program_id)
+                .and_then(|account| {
+                    // it's very unlikely to be squatted at program_id as non-system account because of burden to
+                    // find victim's pubkey/hash. So, when account.owner is indeed native_loader's, it's
+                    // safe to assume it's a genuine program.
+                    if native_loader::check_id(account.owner()) {
+                        Some(account)
+                    } else {
+                        // malicious account is pre-occupying at program_id
+                        self.burn_and_purge_account(program_id, account);
+                        None
+                    }
+                });
+
+        // introducing builtin program
+        if existing_genuine_program.is_some() {
+            // The existing account is sufficient
+            return;
+        }
+
+        assert!(
+            !self.freeze_started(),
+            "Can't change frozen bank by adding not-existing new builtin program ({name}, \
+             {program_id}). Maybe, inconsistent program activation is detected on snapshot \
+             restore?"
+        );
+
+        // Add a bogus executable builtin account, which will be loaded and ignored.
+        let (lamports, rent_epoch) =
+            self.inherit_specially_retained_account_fields(&existing_genuine_program);
+        let account: AccountSharedData = AccountSharedData::from(Account {
+            lamports,
+            data: name.as_bytes().to_vec(),
+            owner: solana_sdk_ids::native_loader::id(),
+            executable: true,
+            rent_epoch,
+        });
+        self.store_account_and_update_capitalization(program_id, &account);
     }
 
     pub fn get_bank_hash_stats(&self) -> BankHashStats {
@@ -5586,50 +5629,6 @@ impl TransactionProcessingCallback for Bank {
             .load_with_fixed_root(&self.ancestors, pubkey)
     }
 
-    // NOTE: must hold idempotent for the same set of arguments
-    /// Add a builtin program account
-    fn add_builtin_account(&self, name: &str, program_id: &Pubkey) {
-        let existing_genuine_program =
-            self.get_account_with_fixed_root(program_id)
-                .and_then(|account| {
-                    // it's very unlikely to be squatted at program_id as non-system account because of burden to
-                    // find victim's pubkey/hash. So, when account.owner is indeed native_loader's, it's
-                    // safe to assume it's a genuine program.
-                    if native_loader::check_id(account.owner()) {
-                        Some(account)
-                    } else {
-                        // malicious account is pre-occupying at program_id
-                        self.burn_and_purge_account(program_id, account);
-                        None
-                    }
-                });
-
-        // introducing builtin program
-        if existing_genuine_program.is_some() {
-            // The existing account is sufficient
-            return;
-        }
-
-        assert!(
-            !self.freeze_started(),
-            "Can't change frozen bank by adding not-existing new builtin program ({name}, \
-             {program_id}). Maybe, inconsistent program activation is detected on snapshot \
-             restore?"
-        );
-
-        // Add a bogus executable builtin account, which will be loaded and ignored.
-        let (lamports, rent_epoch) =
-            self.inherit_specially_retained_account_fields(&existing_genuine_program);
-        let account: AccountSharedData = AccountSharedData::from(Account {
-            lamports,
-            data: name.as_bytes().to_vec(),
-            owner: solana_sdk_ids::native_loader::id(),
-            executable: true,
-            rent_epoch,
-        });
-        self.store_account_and_update_capitalization(program_id, &account);
-    }
-
     fn inspect_account(&self, address: &Pubkey, account_state: AccountState, is_writable: bool) {
         self.inspect_account_for_accounts_lt_hash(address, &account_state, is_writable);
     }

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

@@ -655,8 +655,7 @@ pub(crate) mod tests {
             let account =
                 AccountSharedData::new_data(1, &builtin_name, &native_loader::id()).unwrap();
             bank.store_account_and_update_capitalization(&builtin_id, &account);
-            bank.transaction_processor.add_builtin(
-                &bank,
+            bank.add_builtin(
                 builtin_id,
                 builtin_name.as_str(),
                 ProgramCacheEntry::default(),
@@ -792,8 +791,7 @@ pub(crate) mod tests {
             let account =
                 AccountSharedData::new_data(1, &builtin_name, &native_loader::id()).unwrap();
             bank.store_account_and_update_capitalization(&builtin_id, &account);
-            bank.transaction_processor.add_builtin(
-                &bank,
+            bank.add_builtin(
                 builtin_id,
                 builtin_name.as_str(),
                 ProgramCacheEntry::default(),
@@ -843,8 +841,7 @@ pub(crate) mod tests {
             let account =
                 AccountSharedData::new_data(1, &builtin_name, &native_loader::id()).unwrap();
             bank.store_account_and_update_capitalization(&builtin_id, &account);
-            bank.transaction_processor.add_builtin(
-                &bank,
+            bank.add_builtin(
                 builtin_id,
                 builtin_name.as_str(),
                 ProgramCacheEntry::default(),
@@ -894,8 +891,7 @@ pub(crate) mod tests {
             let account =
                 AccountSharedData::new_data(1, &builtin_name, &native_loader::id()).unwrap();
             bank.store_account_and_update_capitalization(&builtin_id, &account);
-            bank.transaction_processor.add_builtin(
-                &bank,
+            bank.add_builtin(
                 builtin_id,
                 builtin_name.as_str(),
                 ProgramCacheEntry::default(),
@@ -1184,8 +1180,7 @@ pub(crate) mod tests {
         // Set up the CPI mockup to test CPI'ing to the migrated program.
         let cpi_program_id = Pubkey::new_unique();
         let cpi_program_name = "mock_cpi_program";
-        root_bank.transaction_processor.add_builtin(
-            &root_bank,
+        root_bank.add_builtin(
             cpi_program_id,
             cpi_program_name,
             ProgramCacheEntry::new_builtin(0, cpi_program_name.len(), cpi_mockup::Entrypoint::vm),

runtime/src/bank/tests.rs

@@ -3711,14 +3711,12 @@ fn test_add_instruction_processor_for_existing_unrelated_accounts() {
             continue;
         }
 
-        bank.transaction_processor.add_builtin(
-            &bank,
+        bank.add_builtin(
             vote_id,
             "mock_program1",
             ProgramCacheEntry::new_builtin(0, 0, MockBuiltin::vm),
         );
-        bank.transaction_processor.add_builtin(
-            &bank,
+        bank.add_builtin(
             stake_id,
             "mock_program2",
             ProgramCacheEntry::new_builtin(0, 0, MockBuiltin::vm),
@@ -5144,8 +5142,7 @@ fn test_fuzz_instructions() {
         .map(|i| {
             let key = solana_pubkey::new_rand();
             let name = format!("program{i:?}");
-            bank.transaction_processor.add_builtin(
-                &bank,
+            bank.add_builtin(
                 key,
                 name.as_str(),
                 ProgramCacheEntry::new_builtin(0, 0, MockBuiltin::vm),

svm-callback/src/lib.rs

@@ -35,8 +35,6 @@ pub trait InvokeContextCallback {
 pub trait TransactionProcessingCallback: InvokeContextCallback {
     fn get_account_shared_data(&self, pubkey: &Pubkey) -> Option<(AccountSharedData, Slot)>;
 
-    fn add_builtin_account(&self, _name: &str, _program_id: &Pubkey) {}
-
     fn inspect_account(&self, _address: &Pubkey, _account_state: AccountState, _is_writable: bool) {
     }
 }

svm/src/program_loader.rs

@@ -291,14 +291,6 @@ mod tests {
                 .get(pubkey)
                 .map(|account| (account.clone(), 0))
         }
-
-        fn add_builtin_account(&self, name: &str, program_id: &Pubkey) {
-            let mut account_data = AccountSharedData::default();
-            account_data.set_data(name.as_bytes().to_vec());
-            self.account_shared_data
-                .borrow_mut()
-                .insert(*program_id, account_data);
-        }
     }
 
     #[test]

svm/src/transaction_processor.rs

@@ -1055,21 +1055,12 @@ impl<FG: ForkGraph> TransactionBatchProcessor<FG> {
     }
 
     /// Add a built-in program
-    pub fn add_builtin<CB: TransactionProcessingCallback>(
-        &self,
-        callbacks: &CB,
-        program_id: Pubkey,
-        name: &str,
-        builtin: ProgramCacheEntry,
-    ) {
-        debug!("Adding program {name} under {program_id:?}");
-        callbacks.add_builtin_account(name, &program_id);
+    pub fn add_builtin(&self, program_id: Pubkey, builtin: ProgramCacheEntry) {
         self.builtin_program_ids.write().unwrap().insert(program_id);
         self.global_program_cache
             .write()
             .unwrap()
             .assign_program(program_id, Arc::new(builtin));
-        debug!("Added program {name} under {program_id:?}");
     }
 
     #[cfg(feature = "dev-context-only-utils")]
@@ -1163,15 +1154,6 @@ mod tests {
                 .map(|account| (account.clone(), 0))
         }
 
-        fn add_builtin_account(&self, name: &str, program_id: &Pubkey) {
-            let mut account_data = AccountSharedData::default();
-            account_data.set_data(name.as_bytes().to_vec());
-            self.account_shared_data
-                .write()
-                .unwrap()
-                .insert(*program_id, account_data);
-        }
-
         fn inspect_account(
             &self,
             address: &Pubkey,
@@ -1884,7 +1866,6 @@ mod tests {
 
     #[test]
     fn test_add_builtin() {
-        let mock_bank = MockBankCallback::default();
         let fork_graph = Arc::new(RwLock::new(TestForkGraph {}));
         let batch_processor =
             TransactionBatchProcessor::new(0, 0, Arc::downgrade(&fork_graph), None, None);
@@ -1897,12 +1878,7 @@ mod tests {
             |_invoke_context, _param0, _param1, _param2, _param3, _param4| {},
         );
 
-        batch_processor.add_builtin(&mock_bank, key, name, program);
-
-        assert_eq!(
-            mock_bank.account_shared_data.read().unwrap()[&key].data(),
-            name.as_bytes()
-        );
+        batch_processor.add_builtin(key, program);
 
         let mut loaded_programs_for_tx_batch = ProgramCacheForTxBatch::new_from_cache(
             0,

svm/tests/mock_bank.rs

@@ -76,21 +76,6 @@ impl TransactionProcessingCallback for MockBankCallback {
             .map(|account| (account.clone(), 0))
     }
 
-    fn add_builtin_account(&self, name: &str, program_id: &Pubkey) {
-        let account_data = AccountSharedData::from(Account {
-            lamports: 5000,
-            data: name.as_bytes().to_vec(),
-            owner: solana_sdk_ids::native_loader::id(),
-            executable: true,
-            rent_epoch: 0,
-        });
-
-        self.account_shared_data
-            .write()
-            .unwrap()
-            .insert(*program_id, account_data);
-    }
-
     fn inspect_account(&self, address: &Pubkey, account_state: AccountState, is_writable: bool) {
         let account = match account_state {
             AccountState::Dead => None,
@@ -119,6 +104,29 @@ impl MockBankCallback {
         )
     }
 
+    pub fn add_builtin(
+        &self,
+        batch_processor: &TransactionBatchProcessor<MockForkGraph>,
+        program_id: Pubkey,
+        name: &str,
+        builtin: ProgramCacheEntry,
+    ) {
+        let account_data = AccountSharedData::from(Account {
+            lamports: 5000,
+            data: name.as_bytes().to_vec(),
+            owner: solana_sdk_ids::native_loader::id(),
+            executable: true,
+            rent_epoch: 0,
+        });
+
+        self.account_shared_data
+            .write()
+            .unwrap()
+            .insert(program_id, account_data);
+
+        batch_processor.add_builtin(program_id, builtin);
+    }
+
     #[allow(unused)]
     pub fn override_feature_set(&mut self, new_set: SVMFeatureSet) {
         self.feature_set = new_set
@@ -268,8 +276,8 @@ pub fn register_builtins(
     const DEPLOYMENT_SLOT: u64 = 0;
     // We must register LoaderV3 as a loadable account, otherwise programs won't execute.
     let loader_v3_name = "solana_bpf_loader_upgradeable_program";
-    batch_processor.add_builtin(
-        mock_bank,
+    mock_bank.add_builtin(
+        batch_processor,
         solana_sdk_ids::bpf_loader_upgradeable::id(),
         loader_v3_name,
         ProgramCacheEntry::new_builtin(
@@ -281,8 +289,8 @@ pub fn register_builtins(
 
     // Other loaders are needed for testing program cache behavior.
     let loader_v1_name = "solana_bpf_loader_deprecated_program";
-    batch_processor.add_builtin(
-        mock_bank,
+    mock_bank.add_builtin(
+        batch_processor,
         bpf_loader_deprecated::id(),
         loader_v1_name,
         ProgramCacheEntry::new_builtin(
@@ -293,8 +301,8 @@ pub fn register_builtins(
     );
 
     let loader_v2_name = "solana_bpf_loader_program";
-    batch_processor.add_builtin(
-        mock_bank,
+    mock_bank.add_builtin(
+        batch_processor,
         bpf_loader::id(),
         loader_v2_name,
         ProgramCacheEntry::new_builtin(
@@ -306,8 +314,8 @@ pub fn register_builtins(
 
     if with_loader_v4 {
         let loader_v4_name = "solana_loader_v4_program";
-        batch_processor.add_builtin(
-            mock_bank,
+        mock_bank.add_builtin(
+            batch_processor,
             loader_v4::id(),
             loader_v4_name,
             ProgramCacheEntry::new_builtin(
@@ -321,8 +329,8 @@ pub fn register_builtins(
     // In order to perform a transference of native tokens using the system instruction,
     // the system program builtin must be registered.
     let system_program_name = "system_program";
-    batch_processor.add_builtin(
-        mock_bank,
+    mock_bank.add_builtin(
+        batch_processor,
         solana_system_program::id(),
         system_program_name,
         ProgramCacheEntry::new_builtin(
@@ -334,8 +342,8 @@ pub fn register_builtins(
 
     // For testing realloc, we need the compute budget program
     let compute_budget_program_name = "compute_budget_program";
-    batch_processor.add_builtin(
-        mock_bank,
+    mock_bank.add_builtin(
+        batch_processor,
         compute_budget::id(),
         compute_budget_program_name,
         ProgramCacheEntry::new_builtin(