svm: advance nonce for fee-only transactions sooner (anza-xyz#2741)

* write integration tests for nonce transactions

these pass against master as-written

* advance nonce when creating rollback accounts

previously rollback accounts carried the fee payer that was to be committed, but an untouched nonce
now, the account saver can use the nonce and fee payer from rollback accounts identically, possibly merging them for a fee-paying nonce

* fix nonce-related unit tests

* remove nonce hack from integration test

* support four tx states in svm integration

* advance nonce in check transactions

* fix tests for new interfaces

* remove last blockhash timing and tx result mutator

* change copy mut to proper accountshareddata function

* adddress feedback

* please clippy

* fix merge issues

* get lamports_per_signature from blockhash

* revert niche lps change

* fix merge issues again

core/src/banking_stage/committer.rs

@@ -12,7 +12,7 @@ use {
         transaction_batch::TransactionBatch,
         vote_sender_types::ReplayVoteSender,
     },
-    solana_sdk::{hash::Hash, pubkey::Pubkey, saturating_add_assign},
+    solana_sdk::{pubkey::Pubkey, saturating_add_assign},
     solana_svm::{
         transaction_commit_result::{TransactionCommitResult, TransactionCommitResultExtensions},
         transaction_processing_result::{
@@ -65,13 +65,10 @@ impl Committer {
         self.transaction_status_sender.is_some()
     }
 
-    #[allow(clippy::too_many_arguments)]
     pub(super) fn commit_transactions(
         &self,
         batch: &TransactionBatch,
         processing_results: Vec<TransactionProcessingResult>,
-        last_blockhash: Hash,
-        lamports_per_signature: u64,
         starting_transaction_index: Option<usize>,
         bank: &Arc<Bank>,
         pre_balance_info: &mut PreBalanceInfo,
@@ -87,8 +84,6 @@ impl Committer {
         let (commit_results, commit_time_us) = measure_us!(bank.commit_transactions(
             batch.sanitized_transactions(),
             processing_results,
-            last_blockhash,
-            lamports_per_signature,
             processed_counts,
             &mut execute_and_commit_timings.execute_timings,
         ));

core/src/banking_stage/consume_worker.rs

@@ -275,7 +275,6 @@ impl ConsumeWorkerMetrics {
             collect_balances_us,
             load_execute_us,
             freeze_lock_us,
-            last_blockhash_us,
             record_us,
             commit_us,
             find_and_send_votes_us,
@@ -291,9 +290,6 @@ impl ConsumeWorkerMetrics {
         self.timing_metrics
             .freeze_lock_us
             .fetch_add(*freeze_lock_us, Ordering::Relaxed);
-        self.timing_metrics
-            .last_blockhash_us
-            .fetch_add(*last_blockhash_us, Ordering::Relaxed);
         self.timing_metrics
             .record_us
             .fetch_add(*record_us, Ordering::Relaxed);
@@ -494,7 +490,6 @@ struct ConsumeWorkerTimingMetrics {
     collect_balances_us: AtomicU64,
     load_execute_us: AtomicU64,
     freeze_lock_us: AtomicU64,
-    last_blockhash_us: AtomicU64,
     record_us: AtomicU64,
     commit_us: AtomicU64,
     find_and_send_votes_us: AtomicU64,
@@ -527,11 +522,6 @@ impl ConsumeWorkerTimingMetrics {
                 self.freeze_lock_us.swap(0, Ordering::Relaxed),
                 i64
             ),
-            (
-                "last_blockhash_us",
-                self.last_blockhash_us.swap(0, Ordering::Relaxed),
-                i64
-            ),
             ("record_us", self.record_us.swap(0, Ordering::Relaxed), i64),
             ("commit_us", self.commit_us.swap(0, Ordering::Relaxed), i64),
             (

core/src/banking_stage/consumer.rs

@@ -666,17 +666,6 @@ impl Consumer {
         let (freeze_lock, freeze_lock_us) = measure_us!(bank.freeze_lock());
         execute_and_commit_timings.freeze_lock_us = freeze_lock_us;
 
-        // In order to avoid a race condition, leaders must get the last
-        // blockhash *before* recording transactions because recording
-        // transactions will only succeed if the block max tick height hasn't
-        // been reached yet. If they get the last blockhash *after* recording
-        // transactions, the block max tick height could have already been
-        // reached and the blockhash queue could have already been updated with
-        // a new blockhash.
-        let ((last_blockhash, lamports_per_signature), last_blockhash_us) =
-            measure_us!(bank.last_blockhash_and_lamports_per_signature());
-        execute_and_commit_timings.last_blockhash_us = last_blockhash_us;
-
         let (record_transactions_summary, record_us) = measure_us!(self
             .transaction_recorder
             .record_transactions(bank.slot(), processed_transactions));
@@ -713,8 +702,6 @@ impl Consumer {
                 self.committer.commit_transactions(
                     batch,
                     processing_results,
-                    last_blockhash,
-                    lamports_per_signature,
                     starting_transaction_index,
                     bank,
                     &mut pre_balance_info,

core/src/banking_stage/leader_slot_timing_metrics.rs

@@ -10,7 +10,6 @@ pub struct LeaderExecuteAndCommitTimings {
     pub collect_balances_us: u64,
     pub load_execute_us: u64,
     pub freeze_lock_us: u64,
-    pub last_blockhash_us: u64,
     pub record_us: u64,
     pub commit_us: u64,
     pub find_and_send_votes_us: u64,
@@ -23,7 +22,6 @@ impl LeaderExecuteAndCommitTimings {
         saturating_add_assign!(self.collect_balances_us, other.collect_balances_us);
         saturating_add_assign!(self.load_execute_us, other.load_execute_us);
         saturating_add_assign!(self.freeze_lock_us, other.freeze_lock_us);
-        saturating_add_assign!(self.last_blockhash_us, other.last_blockhash_us);
         saturating_add_assign!(self.record_us, other.record_us);
         saturating_add_assign!(self.commit_us, other.commit_us);
         saturating_add_assign!(self.find_and_send_votes_us, other.find_and_send_votes_us);
@@ -40,7 +38,6 @@ impl LeaderExecuteAndCommitTimings {
             ("collect_balances_us", self.collect_balances_us as i64, i64),
             ("load_execute_us", self.load_execute_us as i64, i64),
             ("freeze_lock_us", self.freeze_lock_us as i64, i64),
-            ("last_blockhash_us", self.last_blockhash_us as i64, i64),
             ("record_us", self.record_us as i64, i64),
             ("commit_us", self.commit_us as i64, i64),
             (

runtime/src/account_saver.rs

@@ -1,8 +1,8 @@
 use {
     core::borrow::Borrow,
     solana_sdk::{
-        account::AccountSharedData, nonce::state::DurableNonce, pubkey::Pubkey,
-        transaction::SanitizedTransaction, transaction_context::TransactionAccount,
+        account::AccountSharedData, pubkey::Pubkey, transaction::SanitizedTransaction,
+        transaction_context::TransactionAccount,
     },
     solana_svm::{
         rollback_accounts::RollbackAccounts,
@@ -51,9 +51,7 @@ fn max_number_of_accounts_to_collect(
 pub fn collect_accounts_to_store<'a, T: SVMMessage>(
     txs: &'a [T],
     txs_refs: &'a Option<Vec<impl Borrow<SanitizedTransaction>>>,
-    processing_results: &'a mut [TransactionProcessingResult],
-    durable_nonce: &DurableNonce,
-    lamports_per_signature: u64,
+    processing_results: &'a [TransactionProcessingResult],
 ) -> (
     Vec<(&'a Pubkey, &'a AccountSharedData)>,
     Option<Vec<&'a SanitizedTransaction>>,
@@ -63,10 +61,9 @@ pub fn collect_accounts_to_store<'a, T: SVMMessage>(
     let mut transactions = txs_refs
         .is_some()
         .then(|| Vec::with_capacity(collect_capacity));
-    for (index, (processing_result, transaction)) in
-        processing_results.iter_mut().zip(txs).enumerate()
+    for (index, (processing_result, transaction)) in processing_results.iter().zip(txs).enumerate()
     {
-        let Some(processed_tx) = processing_result.processed_transaction_mut() else {
+        let Some(processed_tx) = processing_result.processed_transaction() else {
             // Don't store any accounts if tx wasn't executed
             continue;
         };
@@ -88,9 +85,7 @@ pub fn collect_accounts_to_store<'a, T: SVMMessage>(
                         &mut transactions,
                         transaction,
                         transaction_ref,
-                        &mut executed_tx.loaded_transaction.rollback_accounts,
-                        durable_nonce,
-                        lamports_per_signature,
+                        &executed_tx.loaded_transaction.rollback_accounts,
                     );
                 }
             }
@@ -100,9 +95,7 @@ pub fn collect_accounts_to_store<'a, T: SVMMessage>(
                     &mut transactions,
                     transaction,
                     transaction_ref,
-                    &mut fees_only_tx.rollback_accounts,
-                    durable_nonce,
-                    lamports_per_signature,
+                    &fees_only_tx.rollback_accounts,
                 );
             }
         }
@@ -142,25 +135,18 @@ fn collect_accounts_for_failed_tx<'a, T: SVMMessage>(
     collected_account_transactions: &mut Option<Vec<&'a SanitizedTransaction>>,
     transaction: &'a T,
     transaction_ref: Option<&'a SanitizedTransaction>,
-    rollback_accounts: &'a mut RollbackAccounts,
-    durable_nonce: &DurableNonce,
-    lamports_per_signature: u64,
+    rollback_accounts: &'a RollbackAccounts,
 ) {
     let fee_payer_address = transaction.fee_payer();
     match rollback_accounts {
         RollbackAccounts::FeePayerOnly { fee_payer_account } => {
-            collected_accounts.push((fee_payer_address, &*fee_payer_account));
+            collected_accounts.push((fee_payer_address, fee_payer_account));
             if let Some(collected_account_transactions) = collected_account_transactions {
                 collected_account_transactions
                     .push(transaction_ref.expect("transaction ref must exist if collecting"));
             }
         }
         RollbackAccounts::SameNonceAndFeePayer { nonce } => {
-            // Since we know we are dealing with a valid nonce account,
-            // unwrap is safe here
-            nonce
-                .try_advance_nonce(*durable_nonce, lamports_per_signature)
-                .unwrap();
             collected_accounts.push((nonce.address(), nonce.account()));
             if let Some(collected_account_transactions) = collected_account_transactions {
                 collected_account_transactions
@@ -171,17 +157,12 @@ fn collect_accounts_for_failed_tx<'a, T: SVMMessage>(
             nonce,
             fee_payer_account,
         } => {
-            collected_accounts.push((fee_payer_address, &*fee_payer_account));
+            collected_accounts.push((fee_payer_address, fee_payer_account));
             if let Some(collected_account_transactions) = collected_account_transactions {
                 collected_account_transactions
                     .push(transaction_ref.expect("transaction ref must exist if collecting"));
             }
 
-            // Since we know we are dealing with a valid nonce account,
-            // unwrap is safe here
-            nonce
-                .try_advance_nonce(*durable_nonce, lamports_per_signature)
-                .unwrap();
             collected_accounts.push((nonce.address(), nonce.account()));
             if let Some(collected_account_transactions) = collected_account_transactions {
                 collected_account_transactions
@@ -204,7 +185,7 @@ mod tests {
             message::Message,
             native_loader,
             nonce::{
-                state::{Data as NonceData, Versions as NonceVersions},
+                state::{Data as NonceData, DurableNonce, Versions as NonceVersions},
                 State as NonceState,
             },
             nonce_account,
@@ -315,7 +296,7 @@ mod tests {
         };
 
         let txs = vec![tx0.clone(), tx1.clone()];
-        let mut processing_results = vec![
+        let processing_results = vec![
             new_executed_processing_result(Ok(()), loaded0),
             new_executed_processing_result(Ok(()), loaded1),
         ];
@@ -324,13 +305,8 @@ mod tests {
 
         for collect_transactions in [false, true] {
             let transaction_refs = collect_transactions.then(|| txs.iter().collect::<Vec<_>>());
-            let (collected_accounts, transactions) = collect_accounts_to_store(
-                &txs,
-                &transaction_refs,
-                &mut processing_results,
-                &DurableNonce::default(),
-                0,
-            );
+            let (collected_accounts, transactions) =
+                collect_accounts_to_store(&txs, &transaction_refs, &processing_results);
             assert_eq!(collected_accounts.len(), 2);
             assert!(collected_accounts
                 .iter()
@@ -383,7 +359,7 @@ mod tests {
         };
 
         let txs = vec![tx];
-        let mut processing_results = vec![new_executed_processing_result(
+        let processing_results = vec![new_executed_processing_result(
             Err(TransactionError::InstructionError(
                 1,
                 InstructionError::InvalidArgument,
@@ -392,17 +368,11 @@ mod tests {
         )];
         let max_collected_accounts = max_number_of_accounts_to_collect(&txs, &processing_results);
         assert_eq!(max_collected_accounts, 1);
-        let durable_nonce = DurableNonce::from_blockhash(&Hash::new_unique());
 
         for collect_transactions in [false, true] {
             let transaction_refs = collect_transactions.then(|| txs.iter().collect::<Vec<_>>());
-            let (collected_accounts, transactions) = collect_accounts_to_store(
-                &txs,
-                &transaction_refs,
-                &mut processing_results,
-                &durable_nonce,
-                0,
-            );
+            let (collected_accounts, transactions) =
+                collect_accounts_to_store(&txs, &transaction_refs, &processing_results);
             assert_eq!(collected_accounts.len(), 1);
             assert_eq!(
                 collected_accounts
@@ -483,9 +453,8 @@ mod tests {
             loaded_accounts_data_size: 0,
         };
 
-        let durable_nonce = DurableNonce::from_blockhash(&Hash::new_unique());
         let txs = vec![tx];
-        let mut processing_results = vec![new_executed_processing_result(
+        let processing_results = vec![new_executed_processing_result(
             Err(TransactionError::InstructionError(
                 1,
                 InstructionError::InvalidArgument,
@@ -497,13 +466,8 @@ mod tests {
 
         for collect_transactions in [false, true] {
             let transaction_refs = collect_transactions.then(|| txs.iter().collect::<Vec<_>>());
-            let (collected_accounts, transactions) = collect_accounts_to_store(
-                &txs,
-                &transaction_refs,
-                &mut processing_results,
-                &durable_nonce,
-                0,
-            );
+            let (collected_accounts, transactions) =
+                collect_accounts_to_store(&txs, &transaction_refs, &processing_results);
             assert_eq!(collected_accounts.len(), 2);
             assert_eq!(
                 collected_accounts
@@ -597,9 +561,8 @@ mod tests {
             loaded_accounts_data_size: 0,
         };
 
-        let durable_nonce = DurableNonce::from_blockhash(&Hash::new_unique());
         let txs = vec![tx];
-        let mut processing_results = vec![new_executed_processing_result(
+        let processing_results = vec![new_executed_processing_result(
             Err(TransactionError::InstructionError(
                 1,
                 InstructionError::InvalidArgument,
@@ -611,13 +574,8 @@ mod tests {
 
         for collect_transactions in [false, true] {
             let transaction_refs = collect_transactions.then(|| txs.iter().collect::<Vec<_>>());
-            let (collected_accounts, transactions) = collect_accounts_to_store(
-                &txs,
-                &transaction_refs,
-                &mut processing_results,
-                &durable_nonce,
-                0,
-            );
+            let (collected_accounts, transactions) =
+                collect_accounts_to_store(&txs, &transaction_refs, &processing_results);
             assert_eq!(collected_accounts.len(), 1);
             let collected_nonce_account = collected_accounts
                 .iter()
@@ -658,7 +616,7 @@ mod tests {
         let from_account_pre = AccountSharedData::new(4242, 0, &Pubkey::default());
 
         let txs = vec![tx];
-        let mut processing_results = vec![Ok(ProcessedTransaction::FeesOnly(Box::new(
+        let processing_results = vec![Ok(ProcessedTransaction::FeesOnly(Box::new(
             FeesOnlyTransaction {
                 load_error: TransactionError::InvalidProgramForExecution,
                 fee_details: FeeDetails::default(),
@@ -669,17 +627,11 @@ mod tests {
         )))];
         let max_collected_accounts = max_number_of_accounts_to_collect(&txs, &processing_results);
         assert_eq!(max_collected_accounts, 1);
-        let durable_nonce = DurableNonce::from_blockhash(&Hash::new_unique());
 
         for collect_transactions in [false, true] {
             let transaction_refs = collect_transactions.then(|| txs.iter().collect::<Vec<_>>());
-            let (collected_accounts, transactions) = collect_accounts_to_store(
-                &txs,
-                &transaction_refs,
-                &mut processing_results,
-                &durable_nonce,
-                0,
-            );
+            let (collected_accounts, transactions) =
+                collect_accounts_to_store(&txs, &transaction_refs, &processing_results);
             assert_eq!(collected_accounts.len(), 1);
             assert_eq!(
                 collected_accounts