SVM: Refactor program match criteria (#1784)

* SVM: hoist `program_modification_slot` up from bank

* SVM: add `check_program_modification_slot` to processing config

* SVM: hoist `program_match_criteria` up from bank

* SVM: drop `get_program_match_critera` from callbacks

core/src/banking_stage/consumer.rs

@@ -606,6 +606,7 @@ impl Consumer {
                 &mut execute_and_commit_timings.execute_timings,
                 TransactionProcessingConfig {
                     account_overrides: None,
+                    check_program_modification_slot: bank.check_program_modification_slot(),
                     compute_budget: bank.compute_budget(),
                     log_messages_bytes_limit: self.log_messages_bytes_limit,
                     limit_to_load_programs: true,

runtime/src/bank.rs

@@ -98,10 +98,7 @@ use {
     solana_perf::perf_libs,
     solana_program_runtime::{
         invoke_context::BuiltinFunctionWithContext,
-        loaded_programs::{
-            ProgramCacheEntry, ProgramCacheEntryOwner, ProgramCacheEntryType,
-            ProgramCacheMatchCriteria,
-        },
+        loaded_programs::{ProgramCacheEntry, ProgramCacheEntryOwner, ProgramCacheEntryType},
         timings::{ExecuteTimingType, ExecuteTimings},
     },
     solana_sdk::{
@@ -109,8 +106,7 @@ use {
             create_account_shared_data_with_fields as create_account, from_account, Account,
             AccountSharedData, InheritableAccountFields, ReadableAccount, WritableAccount,
         },
-        account_utils::StateMut,
-        bpf_loader_upgradeable::{self, UpgradeableLoaderState},
+        bpf_loader_upgradeable,
         clock::{
             BankId, Epoch, Slot, SlotCount, SlotIndex, UnixTimestamp, DEFAULT_HASHES_PER_TICK,
             DEFAULT_TICKS_PER_SECOND, INITIAL_RENT_EPOCH, MAX_PROCESSING_AGE,
@@ -133,7 +129,6 @@ use {
         incinerator,
         inflation::Inflation,
         inner_instruction::InnerInstructions,
-        loader_v4,
         message::{AccountKeys, SanitizedMessage},
         native_loader,
         native_token::LAMPORTS_PER_SOL,
@@ -3407,6 +3402,7 @@ impl Bank {
             &mut timings,
             TransactionProcessingConfig {
                 account_overrides: Some(&account_overrides),
+                check_program_modification_slot: self.check_program_modification_slot,
                 compute_budget: self.compute_budget(),
                 log_messages_bytes_limit: None,
                 limit_to_load_programs: true,
@@ -4840,6 +4836,7 @@ impl Bank {
             timings,
             TransactionProcessingConfig {
                 account_overrides: None,
+                check_program_modification_slot: self.check_program_modification_slot,
                 compute_budget: self.compute_budget(),
                 log_messages_bytes_limit,
                 limit_to_load_programs: false,
@@ -6751,8 +6748,12 @@ impl Bank {
         false
     }
 
-    pub fn check_program_modification_slot(&mut self) {
-        self.check_program_modification_slot = true;
+    pub fn check_program_modification_slot(&self) -> bool {
+        self.check_program_modification_slot
+    }
+
+    pub fn set_check_program_modification_slot(&mut self, check: bool) {
+        self.check_program_modification_slot = check;
     }
 
     pub fn fee_structure(&self) -> &FeeStructure {
@@ -6767,40 +6768,6 @@ impl Bank {
         self.transaction_processor
             .add_builtin(self, program_id, name, builtin)
     }
-
-    /// Find the slot in which the program was most recently modified.
-    /// Returns slot 0 for programs deployed with v1/v2 loaders, since programs deployed
-    /// with those loaders do not retain deployment slot information.
-    /// Returns an error if the program's account state can not be found or parsed.
-    fn program_modification_slot(&self, pubkey: &Pubkey) -> transaction::Result<Slot> {
-        let program = self
-            .get_account(pubkey)
-            .ok_or(TransactionError::ProgramAccountNotFound)?;
-        if bpf_loader_upgradeable::check_id(program.owner()) {
-            if let Ok(UpgradeableLoaderState::Program {
-                programdata_address,
-            }) = program.state()
-            {
-                let programdata = self
-                    .get_account(&programdata_address)
-                    .ok_or(TransactionError::ProgramAccountNotFound)?;
-                if let Ok(UpgradeableLoaderState::ProgramData {
-                    slot,
-                    upgrade_authority_address: _,
-                }) = programdata.state()
-                {
-                    return Ok(slot);
-                }
-            }
-            Err(TransactionError::ProgramAccountNotFound)
-        } else if loader_v4::check_id(program.owner()) {
-            let state = solana_loader_v4_program::get_state(program.data())
-                .map_err(|_| TransactionError::ProgramAccountNotFound)?;
-            Ok(state.slot)
-        } else {
-            Ok(0)
-        }
-    }
 }
 
 impl TransactionProcessingCallback for Bank {
@@ -6820,17 +6787,6 @@ impl TransactionProcessingCallback for Bank {
             .map(|(acc, _)| acc)
     }
 
-    fn get_program_match_criteria(&self, program: &Pubkey) -> ProgramCacheMatchCriteria {
-        if self.check_program_modification_slot {
-            self.program_modification_slot(program)
-                .map_or(ProgramCacheMatchCriteria::Tombstone, |slot| {
-                    ProgramCacheMatchCriteria::DeployedOnOrAfterSlot(slot)
-                })
-        } else {
-            ProgramCacheMatchCriteria::NoCriteria
-        }
-    }
-
     // 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) {

runtime/src/bank/tests.rs

@@ -73,7 +73,6 @@ use {
         incinerator,
         instruction::{AccountMeta, CompiledInstruction, Instruction, InstructionError},
         loader_upgradeable_instruction::UpgradeableLoaderInstruction,
-        loader_v4::{LoaderV4State, LoaderV4Status},
         message::{Message, MessageHeader, SanitizedMessage},
         native_loader,
         native_token::{sol_to_lamports, LAMPORTS_PER_SOL},
@@ -13057,91 +13056,6 @@ fn test_deploy_last_epoch_slot() {
     assert_eq!(result_with_feature_enabled, Ok(()));
 }
 
-#[test]
-fn test_program_modification_slot_account_not_found() {
-    let genesis_config = GenesisConfig::default();
-    let bank = Bank::new_for_tests(&genesis_config);
-    let key = Pubkey::new_unique();
-
-    let result = bank.program_modification_slot(&key);
-    assert_eq!(result.err(), Some(TransactionError::ProgramAccountNotFound));
-
-    let mut account_data = AccountSharedData::new(100, 100, &bpf_loader_upgradeable::id());
-    bank.store_account(&key, &account_data);
-
-    let result = bank.program_modification_slot(&key);
-    assert_eq!(result.err(), Some(TransactionError::ProgramAccountNotFound));
-
-    let state = UpgradeableLoaderState::Program {
-        programdata_address: Pubkey::new_unique(),
-    };
-    account_data.set_data(bincode::serialize(&state).unwrap());
-    bank.store_account(&key, &account_data);
-
-    let result = bank.program_modification_slot(&key);
-    assert_eq!(result.err(), Some(TransactionError::ProgramAccountNotFound));
-
-    account_data.set_owner(loader_v4::id());
-    bank.store_account(&key, &account_data);
-
-    let result = bank.program_modification_slot(&key);
-    assert_eq!(result.err(), Some(TransactionError::ProgramAccountNotFound));
-}
-
-#[test]
-fn test_program_modification_slot_success() {
-    let genesis_config = GenesisConfig::default();
-    let bank = Bank::new_for_tests(&genesis_config);
-
-    let key1 = Pubkey::new_unique();
-    let key2 = Pubkey::new_unique();
-
-    let account_data = AccountSharedData::new_data(
-        100,
-        &UpgradeableLoaderState::Program {
-            programdata_address: key2,
-        },
-        &bpf_loader_upgradeable::id(),
-    )
-    .unwrap();
-    bank.store_account(&key1, &account_data);
-
-    let account_data = AccountSharedData::new_data(
-        100,
-        &UpgradeableLoaderState::ProgramData {
-            slot: 77,
-            upgrade_authority_address: None,
-        },
-        &bpf_loader_upgradeable::id(),
-    )
-    .unwrap();
-    bank.store_account(&key2, &account_data);
-
-    let result = bank.program_modification_slot(&key1);
-    assert_eq!(result.unwrap(), 77);
-
-    let state = LoaderV4State {
-        slot: 58,
-        authority_address: Pubkey::new_unique(),
-        status: LoaderV4Status::Deployed,
-    };
-    let encoded = unsafe {
-        std::mem::transmute::<&LoaderV4State, &[u8; LoaderV4State::program_data_offset()]>(&state)
-    };
-    let mut account_data = AccountSharedData::new(100, encoded.len(), &loader_v4::id());
-    account_data.set_data(encoded.to_vec());
-    bank.store_account(&key1, &account_data);
-
-    let result = bank.program_modification_slot(&key1);
-    assert_eq!(result.unwrap(), 58);
-
-    account_data.set_owner(Pubkey::new_unique());
-    bank.store_account(&key2, &account_data);
-
-    let result = bank.program_modification_slot(&key2);
-    assert_eq!(result.unwrap(), 0);
-}
-
 #[test]
 fn test_blockhash_last_valid_block_height() {
     let genesis_config = GenesisConfig::default();

runtime/src/bank_forks.rs

@@ -225,7 +225,7 @@ impl BankForks {
 
     pub fn insert(&mut self, mut bank: Bank) -> BankWithScheduler {
         if self.root.load(Ordering::Relaxed) < self.highest_slot_at_startup {
-            bank.check_program_modification_slot();
+            bank.set_check_program_modification_slot(true);
         }
 
         let bank = Arc::new(bank);

svm/src/program_loader.rs

@@ -11,11 +11,12 @@ use {
         account::{AccountSharedData, ReadableAccount},
         account_utils::StateMut,
         bpf_loader, bpf_loader_deprecated,
-        bpf_loader_upgradeable::UpgradeableLoaderState,
+        bpf_loader_upgradeable::{self, UpgradeableLoaderState},
         clock::Slot,
         instruction::InstructionError,
         loader_v4::{self, LoaderV4State, LoaderV4Status},
         pubkey::Pubkey,
+        transaction::{self, TransactionError},
     },
     solana_type_overrides::sync::Arc,
 };
@@ -217,6 +218,43 @@ pub fn load_program_with_pubkey<CB: TransactionProcessingCallback>(
     Some(Arc::new(loaded_program))
 }
 
+/// Find the slot in which the program was most recently modified.
+/// Returns slot 0 for programs deployed with v1/v2 loaders, since programs deployed
+/// with those loaders do not retain deployment slot information.
+/// Returns an error if the program's account state can not be found or parsed.
+pub(crate) fn get_program_modification_slot<CB: TransactionProcessingCallback>(
+    callbacks: &CB,
+    pubkey: &Pubkey,
+) -> transaction::Result<Slot> {
+    let program = callbacks
+        .get_account_shared_data(pubkey)
+        .ok_or(TransactionError::ProgramAccountNotFound)?;
+    if bpf_loader_upgradeable::check_id(program.owner()) {
+        if let Ok(UpgradeableLoaderState::Program {
+            programdata_address,
+        }) = program.state()
+        {
+            let programdata = callbacks
+                .get_account_shared_data(&programdata_address)
+                .ok_or(TransactionError::ProgramAccountNotFound)?;
+            if let Ok(UpgradeableLoaderState::ProgramData {
+                slot,
+                upgrade_authority_address: _,
+            }) = programdata.state()
+            {
+                return Ok(slot);
+            }
+        }
+        Err(TransactionError::ProgramAccountNotFound)
+    } else if loader_v4::check_id(program.owner()) {
+        let state = solana_loader_v4_program::get_state(program.data())
+            .map_err(|_| TransactionError::ProgramAccountNotFound)?;
+        Ok(state.slot)
+    } else {
+        Ok(0)
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use {
@@ -816,4 +854,111 @@ mod tests {
             );
         }
     }
+
+    #[test]
+    fn test_program_modification_slot_account_not_found() {
+        let mock_bank = MockBankCallback::default();
+
+        let key = Pubkey::new_unique();
+
+        let result = get_program_modification_slot(&mock_bank, &key);
+        assert_eq!(result.err(), Some(TransactionError::ProgramAccountNotFound));
+
+        let mut account_data = AccountSharedData::new(100, 100, &bpf_loader_upgradeable::id());
+        mock_bank
+            .account_shared_data
+            .borrow_mut()
+            .insert(key, account_data.clone());
+
+        let result = get_program_modification_slot(&mock_bank, &key);
+        assert_eq!(result.err(), Some(TransactionError::ProgramAccountNotFound));
+
+        let state = UpgradeableLoaderState::Program {
+            programdata_address: Pubkey::new_unique(),
+        };
+        account_data.set_data(bincode::serialize(&state).unwrap());
+        mock_bank
+            .account_shared_data
+            .borrow_mut()
+            .insert(key, account_data.clone());
+
+        let result = get_program_modification_slot(&mock_bank, &key);
+        assert_eq!(result.err(), Some(TransactionError::ProgramAccountNotFound));
+
+        account_data.set_owner(loader_v4::id());
+        mock_bank
+            .account_shared_data
+            .borrow_mut()
+            .insert(key, account_data);
+
+        let result = get_program_modification_slot(&mock_bank, &key);
+        assert_eq!(result.err(), Some(TransactionError::ProgramAccountNotFound));
+    }
+
+    #[test]
+    fn test_program_modification_slot_success() {
+        let mock_bank = MockBankCallback::default();
+
+        let key1 = Pubkey::new_unique();
+        let key2 = Pubkey::new_unique();
+
+        let account_data = AccountSharedData::new_data(
+            100,
+            &UpgradeableLoaderState::Program {
+                programdata_address: key2,
+            },
+            &bpf_loader_upgradeable::id(),
+        )
+        .unwrap();
+        mock_bank
+            .account_shared_data
+            .borrow_mut()
+            .insert(key1, account_data);
+
+        let account_data = AccountSharedData::new_data(
+            100,
+            &UpgradeableLoaderState::ProgramData {
+                slot: 77,
+                upgrade_authority_address: None,
+            },
+            &bpf_loader_upgradeable::id(),
+        )
+        .unwrap();
+        mock_bank
+            .account_shared_data
+            .borrow_mut()
+            .insert(key2, account_data);
+
+        let result = get_program_modification_slot(&mock_bank, &key1);
+        assert_eq!(result.unwrap(), 77);
+
+        let state = LoaderV4State {
+            slot: 58,
+            authority_address: Pubkey::new_unique(),
+            status: LoaderV4Status::Deployed,
+        };
+        let encoded = unsafe {
+            std::mem::transmute::<&LoaderV4State, &[u8; LoaderV4State::program_data_offset()]>(
+                &state,
+            )
+        };
+        let mut account_data = AccountSharedData::new(100, encoded.len(), &loader_v4::id());
+        account_data.set_data(encoded.to_vec());
+        mock_bank
+            .account_shared_data
+            .borrow_mut()
+            .insert(key1, account_data.clone());
+
+        let result = get_program_modification_slot(&mock_bank, &key1);
+        assert_eq!(result.unwrap(), 58);
+
+        account_data.set_owner(Pubkey::new_unique());
+        mock_bank
+            .account_shared_data
+            .borrow_mut()
+            .insert(key2, account_data);
+
+        let result = get_program_modification_slot(&mock_bank, &key2);
+        assert_eq!(result.unwrap(), 0);
+    }
 }

svm/src/transaction_processing_callback.rs

@@ -1,7 +1,4 @@
-use {
-    solana_program_runtime::loaded_programs::ProgramCacheMatchCriteria,
-    solana_sdk::{account::AccountSharedData, pubkey::Pubkey},
-};
+use solana_sdk::{account::AccountSharedData, pubkey::Pubkey};
 
 /// Runtime callbacks for transaction processing.
 pub trait TransactionProcessingCallback {
@@ -9,9 +6,5 @@ pub trait TransactionProcessingCallback {
 
     fn get_account_shared_data(&self, pubkey: &Pubkey) -> Option<AccountSharedData>;
 
-    fn get_program_match_criteria(&self, _program: &Pubkey) -> ProgramCacheMatchCriteria {
-        ProgramCacheMatchCriteria::NoCriteria
-    }
-
     fn add_builtin_account(&self, _name: &str, _program_id: &Pubkey) {}
 }

svm/src/transaction_processor.rs

@@ -9,7 +9,7 @@ use {
         },
         account_overrides::AccountOverrides,
         message_processor::MessageProcessor,
-        program_loader::load_program_with_pubkey,
+        program_loader::{get_program_modification_slot, load_program_with_pubkey},
         rollback_accounts::RollbackAccounts,
         transaction_account_state_info::TransactionAccountStateInfo,
         transaction_error_metrics::TransactionErrorMetrics,
@@ -104,6 +104,9 @@ pub struct TransactionProcessingConfig<'a> {
     /// Encapsulates overridden accounts, typically used for transaction
     /// simulation.
     pub account_overrides: Option<&'a AccountOverrides>,
+    /// Whether or not to check a program's modification slot when replenishing
+    /// a program cache instance.
+    pub check_program_modification_slot: bool,
     /// The compute budget to use for transaction execution.
     pub compute_budget: Option<ComputeBudget>,
     /// The maximum number of bytes that log messages can consume.
@@ -258,6 +261,7 @@ impl<FG: ForkGraph> TransactionBatchProcessor<FG> {
         let program_cache_for_tx_batch = Rc::new(RefCell::new(self.replenish_program_cache(
             callbacks,
             &program_accounts_map,
+            config.check_program_modification_slot,
             config.limit_to_load_programs,
         )));
 
@@ -520,16 +524,22 @@ impl<FG: ForkGraph> TransactionBatchProcessor<FG> {
         &self,
         callback: &CB,
         program_accounts_map: &HashMap<Pubkey, u64>,
+        check_program_modification_slot: bool,
         limit_to_load_programs: bool,
     ) -> ProgramCacheForTxBatch {
         let mut missing_programs: Vec<(Pubkey, (ProgramCacheMatchCriteria, u64))> =
             program_accounts_map
                 .iter()
                 .map(|(pubkey, count)| {
-                    (
-                        *pubkey,
-                        (callback.get_program_match_criteria(pubkey), *count),
-                    )
+                    let match_criteria = if check_program_modification_slot {
+                        get_program_modification_slot(callback, pubkey)
+                            .map_or(ProgramCacheMatchCriteria::Tombstone, |slot| {
+                                ProgramCacheMatchCriteria::DeployedOnOrAfterSlot(slot)
+                            })
+                    } else {
+                        ProgramCacheMatchCriteria::NoCriteria
+                    };
+                    (*pubkey, (match_criteria, *count))
                 })
                 .collect();
 
@@ -1301,7 +1311,7 @@ mod tests {
         let mut account_maps: HashMap<Pubkey, u64> = HashMap::new();
         account_maps.insert(key, 4);
 
-        batch_processor.replenish_program_cache(&mock_bank, &account_maps, true);
+        batch_processor.replenish_program_cache(&mock_bank, &account_maps, false, true);
     }
 
     #[test]
@@ -1327,6 +1337,7 @@ mod tests {
             let result = batch_processor.replenish_program_cache(
                 &mock_bank,
                 &account_maps,
+                false,
                 limit_to_load_programs,
             );
             assert!(!result.hit_max_limit);
@@ -1858,7 +1869,8 @@ mod tests {
                     let maps = account_maps.clone();
                     let programs = programs.clone();
                     thread::spawn(move || {
-                        let result = processor.replenish_program_cache(&local_bank, &maps, true);
+                        let result =
+                            processor.replenish_program_cache(&local_bank, &maps, false, true);
                         for key in &programs {
                             let cache_entry = result.find(key);
                             assert!(matches!(

svm/tests/conformance.rs

@@ -279,6 +279,7 @@ fn run_fixture(fixture: InstrFixture, filename: OsString, execute_as_instr: bool
     };
     let processor_config = TransactionProcessingConfig {
         account_overrides: None,
+        check_program_modification_slot: false,
         compute_budget: None,
         log_messages_bytes_limit: None,
         limit_to_load_programs: true,