svm: better testing for transaction batches

[2501babe]

Problem

transaction loading is a part of the code with poor test coverage. this has recently become (more) problematic for two reasons:

• for SIMD83 (removal of entry-level constraints) we need to change how transaction loading works, and the new behavior will need appreciably more test coverage
• transaction loading is currently unspecified and has several "surprising edge cases" especially when it comes to unintended behaviors of the program cache. we want to fix and specify the exact conditions under which a transaction should or should not be executed. this is of particular concern for when we need to maintain consensus with firedancer

Summary of Changes

using the code in integration_test.rs as a basis, i abstracted out a basic framework for doing arbitrary integration tests starting at the transaction processor load_and_execute_sanitized_transactions entrypoint. program_medley() is largely identical to the previous tests (some numbers changed for convenience) including checking logs and return data. simple_transfer() is a simple example of how to add more tests that go through this

one thing im not sure about (other than names of some things, in particulat TestData is bad) is if this can easily be further abstracted to also support testing from the bank.rs load_execute_and_commit_transactions or load_and_execute_transactions level. however maybe we dont want to abstract this further and just want two different systems. basically we want to also be able to test locks with batches of transactions

[2501babe]

cc @apfitzge if you have thoughts on design, im planning on doing my svm tests on top of this and wanted to not have it be in the main prs

[LucasSte]

however maybe we dont want to abstract this further and just want two different systems. basically we want to also be able to test locks with batches of transactions

Do you mean account locks? They are not calculated in the SVM.

im planning on doing my svm tests on top of this and wanted to not have it be in the main prs

What kind of tests are you planning on adding? I have some fuzzy tests in the SVM and perhaps we could think of integrating everything.

[LucasSte]

Very nice work!

[2501babe]

Do you mean account locks? They are not calculated in the SVM.

yea what i mean is, after the svm changes for simd83 (making it so transactions with read/write and write/write duplicate accounts can execute), we are going to make changes to accountsdb (making it so the locks dont conflict so transactions like that can be scheduled) and we probably want some kind of similar runner system for that. thinking whether we just want one shared one or not, tho i guess can decide once accountsdb work is up

What kind of tests are you planning on adding?

the main things im planning on testing are:

• program cache interactions. this part of the code is already somewhat problematic, and it will get a lot trickier once we have duplicate accounts in transaction batches
• proper behavior and various edge cases relating to batching, eg realloc in batch, fee payer depleted or topped up in batch, and do accounts/fee payers/nonces come out of batches looking right

[2501babe]

ok, i addressed the resolved comments. also i made the top-level test take a Vec<SvmTestEntry> instead of SvmTestEntry (fka TestData) so its possible to generate several variations on the same theme in one setup function. i set up a new execution environment every time since the program cache is dirtied, but once i understand it better ill probably add an option (in a separate pr) to carry it between entries

[LucasSte]

I think you can simply do assert_eq!(self.return_data, execution_details.return_data), provided that you derive PartialEq for ReturnDataAssert.

[2501babe]

cant because theyre different types, but there is a natural From impl!

[2501babe]

it wasnt possible to rebase without writing new code, this is the incremental diff in case github restarts the diff history once i force push this. master now distinguishes two types of "processed" transaction: an "executed" transaction (which may succeed or fail) and a "fees-only" transaction (which is not executed but fee-payer and nonce account changes are committed) in addition to the regular non-executed transaction (no state changes)

hana@laptop:svm$ git diff remotes/origin/20240816_txbatchtest:svm/tests/integration_test.rs HEAD:svm/tests/integration_test.rs
diff --git a/svm/tests/integration_test.rs b/svm/tests/integration_test.rs
index 1c22f8266e..eef355425a 100644
--- a/svm/tests/integration_test.rs
+++ b/svm/tests/integration_test.rs
@@ -21,6 +21,7 @@ use {
     solana_svm::{
         account_loader::{CheckedTransactionDetails, TransactionCheckResult},
         transaction_execution_result::TransactionExecutionDetails,
+        transaction_processing_result::ProcessedTransaction,
         transaction_processor::{
             ExecutionRecordingConfig, TransactionBatchProcessor, TransactionProcessingConfig,
             TransactionProcessingEnvironment,
@@ -573,13 +574,25 @@ fn execute_test_entry(test_entry: SvmTestEntry) {
     // build a hashmap of final account states incrementally
     // NOTE with SIMD-83 an account may appear multiple times, thus we need the final state
     let mut final_accounts_actual = AccountMap::default();
-    for (pubkey, account_data) in batch_output
+    for processed_transaction in batch_output
         .processing_results
         .iter()
-        .filter(|r| r.is_ok())
-        .flat_map(|r| r.as_ref().unwrap().loaded_transaction.accounts.clone())
+        .filter_map(|r| r.as_ref().ok())
     {
-        final_accounts_actual.insert(pubkey, account_data);
+        match processed_transaction {
+            ProcessedTransaction::Executed(executed_transaction) => {
+                for (pubkey, account_data) in
+                    executed_transaction.loaded_transaction.accounts.clone()
+                {
+                    final_accounts_actual.insert(pubkey, account_data);
+                }
+            }
+            // NOTE this is a possible state with `feature_set::enable_transaction_loading_failure_fees` enabled
+            // by using `TransactionProcessingEnvironment::default()` we have all features disabled
+            // in other words, this will be unreachable until we are ready to test fee-only transactions
+            // (or the feature is activated on mainnet and removed... but we should do it before then!)
+            ProcessedTransaction::FeesOnly(_) => unreachable!(),
+        }
     }
 
     // check that all the account states we care about are present and correct
@@ -595,7 +608,9 @@ fn execute_test_entry(test_entry: SvmTestEntry) {
         .zip(test_entry.asserts())
     {
         match processing_result {
-            Ok(tx) => test_item_asserts.check_executed_transaction(&tx.execution_details),
+            Ok(ProcessedTransaction::Executed(executed_transaction)) => test_item_asserts
+                .check_executed_transaction(&executed_transaction.execution_details),
+            Ok(ProcessedTransaction::FeesOnly(_)) => unreachable!(),
             Err(_) => assert!(!test_item_asserts.executed),
         }
     }

will address the rest of comments in next commit!

[2501babe]

oki should be all set now!