adddress feedback

runtime/src/bank/check_transactions.rs

@@ -129,30 +129,27 @@ impl Bank {
         next_durable_nonce: &DurableNonce,
     ) -> Option<(NonceInfo, u64)> {
         let nonce_is_advanceable = message.recent_blockhash() != next_durable_nonce.as_hash();
-        if nonce_is_advanceable {
-            if let Some((nonce_address, mut nonce_account, nonce_data)) =
-                self.load_message_nonce_account(message)
-            {
-                let lamports_per_signature = nonce_data.get_lamports_per_signature();
-                let next_nonce_state = NonceState::new_initialized(
-                    &nonce_data.authority,
-                    *next_durable_nonce,
-                    lamports_per_signature,
-                );
-                nonce_account
-                    .set_state(&NonceVersions::new(next_nonce_state))
-                    .ok()?;
-
-                Some((
-                    NonceInfo::new(nonce_address, nonce_account),
-                    lamports_per_signature,
-                ))
-            } else {
-                None
-            }
-        } else {
-            None
+        if !nonce_is_advanceable {
+            return None;
         }
+
+        let (nonce_address, mut nonce_account, nonce_data) =
+            self.load_message_nonce_account(message)?;
+
+        let lamports_per_signature = self.get_lamports_per_signature();
+        let next_nonce_state = NonceState::new_initialized(
+            &nonce_data.authority,
+            *next_durable_nonce,
+            lamports_per_signature,
+        );
+        nonce_account
+            .set_state(&NonceVersions::new(next_nonce_state))
+            .ok()?;
+
+        Some((
+            NonceInfo::new(nonce_address, nonce_account),
+            lamports_per_signature,
+        ))
     }
 
     pub(super) fn load_message_nonce_account(
@@ -238,6 +235,7 @@ mod tests {
         .unwrap();
         let custodian_pubkey = custodian_keypair.pubkey();
         let nonce_pubkey = nonce_keypair.pubkey();
+        let stale_lamports_per_signature = 42;
 
         let nonce_hash = get_nonce_blockhash(&bank, &nonce_pubkey).unwrap();
         let message = new_sanitized_message(Message::new_with_blockhash(
@@ -248,18 +246,32 @@ mod tests {
             Some(&custodian_pubkey),
             &nonce_hash,
         ));
-        let nonce_account = bank.get_account(&nonce_pubkey).unwrap();
+
+        // set a spurious lamports_per_signature value
+        let mut nonce_account = bank.get_account(&nonce_pubkey).unwrap();
         let nonce_data = get_nonce_data_from_account(&nonce_account).unwrap();
+        nonce_account
+            .set_state(&NonceVersions::new(NonceState::new_initialized(
+                &nonce_data.authority,
+                nonce_data.durable_nonce,
+                stale_lamports_per_signature,
+            )))
+            .unwrap();
+        bank.store_account(&nonce_pubkey, &nonce_account);
 
-        let lamports_per_signature = nonce_data.get_lamports_per_signature();
+        let nonce_account = bank.get_account(&nonce_pubkey).unwrap();
+        let current_lamports_per_signature = bank.get_lamports_per_signature();
         let mut expected_nonce_info = NonceInfo::new(nonce_pubkey, nonce_account);
         expected_nonce_info
-            .try_advance_nonce(bank.next_durable_nonce(), lamports_per_signature)
+            .try_advance_nonce(bank.next_durable_nonce(), current_lamports_per_signature)
             .unwrap();
 
+        // we now expect to:
+        // * advance the nonce to the current value
+        // * set the bank's lamports_per_signature in the nonce data
         assert_eq!(
             bank.check_load_and_advance_message_nonce_account(&message, &bank.next_durable_nonce()),
-            Some((expected_nonce_info, lamports_per_signature)),
+            Some((expected_nonce_info, current_lamports_per_signature)),
         );
     }
 

svm/src/rollback_accounts.rs

@@ -51,6 +51,10 @@ impl RollbackAccounts {
 
         if let Some(nonce) = nonce {
             if &fee_payer_address == nonce.address() {
+                // `nonce` contains an AccountSharedData which has already been advanced to the current DurableNonce
+                // `fee_payer_account` is an AccountSharedData as it currently exists on-chain
+                // thus if the nonce account is being used as the fee payer, we need to update that data here
+                // so we capture both the data change for the nonce and the lamports/rent epoch change for the fee payer
                 fee_payer_account.set_data_from_slice(nonce.account().data());
 
                 RollbackAccounts::SameNonceAndFeePayer {
@@ -65,7 +69,7 @@ impl RollbackAccounts {
         } else {
             // When rolling back failed transactions which don't use nonces, the
             // runtime should not update the fee payer's rent epoch so reset the
-            // rollback fee payer acocunt's rent epoch to its originally loaded
+            // rollback fee payer account's rent epoch to its originally loaded
             // rent epoch value. In the future, a feature gate could be used to
             // alter this behavior such that rent epoch updates are handled the
             // same for both nonce and non-nonce failed transactions.

svm/src/transaction_processor.rs

@@ -1001,7 +1001,7 @@ mod tests {
             fee_calculator::FeeCalculator,
             hash::Hash,
             message::{LegacyMessage, Message, MessageHeader, SanitizedMessage},
-            nonce::{self, state::DurableNonce},
+            nonce,
             rent_collector::{RentCollector, RENT_EXEMPT_RENT_EPOCH},
             rent_debits::RentDebits,
             reserved_account_keys::ReservedAccountKeys,
@@ -2180,12 +2180,10 @@ mod tests {
             let mut error_counters = TransactionErrorMetrics::default();
             let batch_processor = TransactionBatchProcessor::<TestForkGraph>::default();
 
-            let durable_nonce = DurableNonce::from_blockhash(&last_blockhash);
-            let mut nonce_info = NonceInfo::new(*fee_payer_address, fee_payer_account.clone());
-            nonce_info
-                .try_advance_nonce(durable_nonce, lamports_per_signature)
-                .unwrap();
-            let nonce = Some(nonce_info);
+            let nonce = Some(NonceInfo::new(
+                *fee_payer_address,
+                fee_payer_account.clone(),
+            ));
 
             let result = batch_processor.validate_transaction_fee_payer(
                 &mock_bank,

svm/tests/integration_test.rs

@@ -639,7 +639,10 @@ fn simple_transfer() -> Vec<SvmTestEntry> {
     vec![test_entry]
 }
 
-fn simple_nonce_fee_only(enable_fee_only_transactions: bool) -> Vec<SvmTestEntry> {
+fn simple_nonce_fee_only(
+    enable_fee_only_transactions: bool,
+    fee_paying_nonce: bool,
+) -> Vec<SvmTestEntry> {
     let mut test_entry = SvmTestEntry::default();
     if enable_fee_only_transactions {
         test_entry
@@ -655,19 +658,31 @@ fn simple_nonce_fee_only(enable_fee_only_transactions: bool) -> Vec<SvmTestEntry
 
     // create and return a transaction, fee payer, and nonce info
     // sets up initial account states but not final ones
+    // there are four cases of fee_paying_nonce and fake_fee_payer:
+    // * false/false: normal nonce account with rent minimum, normal fee payer account with 1sol
+    // * true/false: normal nonce account used to pay fees with rent minimum plus 1sol
+    // * false/true: normal nonce account with rent minimum, fee payer doesnt exist
+    // * true/true: same account for both which does not exist
     let mk_nonce_transaction = |test_entry: &mut SvmTestEntry, program_id, fake_fee_payer: bool| {
         let fee_payer_keypair = Keypair::new();
         let fee_payer = fee_payer_keypair.pubkey();
+        let nonce_pubkey = if fee_paying_nonce {
+            fee_payer
+        } else {
+            Pubkey::new_unique()
+        };
+
+        let nonce_size = nonce::State::size();
+        let mut nonce_balance = Rent::default().minimum_balance(nonce_size);
 
-        if !fake_fee_payer {
+        if !fake_fee_payer && !fee_paying_nonce {
             let mut fee_payer_data = AccountSharedData::default();
             fee_payer_data.set_lamports(LAMPORTS_PER_SOL);
             test_entry.add_initial_account(fee_payer, &fee_payer_data);
+        } else if fee_paying_nonce {
+            nonce_balance += LAMPORTS_PER_SOL;
         }
 
-        let nonce_size = nonce::State::size();
-        let nonce_balance = Rent::default().minimum_balance(nonce_size);
-        let nonce_pubkey = Pubkey::new_unique();
         let nonce_initial_hash = DurableNonce::from_blockhash(&Hash::new_unique());
         let nonce_data =
             nonce::state::Data::new(fee_payer, nonce_initial_hash, LAMPORTS_PER_SIGNATURE);
@@ -679,7 +694,9 @@ fn simple_nonce_fee_only(enable_fee_only_transactions: bool) -> Vec<SvmTestEntry
         .unwrap();
         let nonce_info = NonceInfo::new(nonce_pubkey, nonce_account.clone());
 
-        test_entry.add_initial_account(nonce_pubkey, &nonce_account);
+        if !(fake_fee_payer && fee_paying_nonce) {
+            test_entry.add_initial_account(nonce_pubkey, &nonce_account);
+        }
 
         let instructions = vec![
             system_instruction::advance_nonce_account(&nonce_pubkey, &fee_payer),
@@ -727,8 +744,7 @@ fn simple_nonce_fee_only(enable_fee_only_transactions: bool) -> Vec<SvmTestEntry
         test_entry
             .final_accounts
             .get_mut(nonce_info.address())
-            .unwrap()
-            .set_rent_epoch(0);
+            .map(|a| a.set_rent_epoch(0));
 
         test_entry.push_nonce_transaction_with_status(
             transaction,
@@ -792,11 +808,15 @@ fn simple_nonce_fee_only(enable_fee_only_transactions: bool) -> Vec<SvmTestEntry
                 .unwrap()
                 .data_as_mut_slice()
                 .copy_from_slice(nonce_info.account().data());
-            test_entry
-                .final_accounts
-                .get_mut(nonce_info.address())
-                .unwrap()
-                .set_rent_epoch(0);
+
+            // if the nonce account pays fees, it keeps its new rent epoch, otherwise it resets
+            if !fee_paying_nonce {
+                test_entry
+                    .final_accounts
+                    .get_mut(nonce_info.address())
+                    .unwrap()
+                    .set_rent_epoch(0);
+            }
         } else {
             test_entry
                 .final_accounts
@@ -823,8 +843,10 @@ fn simple_nonce_fee_only(enable_fee_only_transactions: bool) -> Vec<SvmTestEntry
 
 #[test_case(program_medley())]
 #[test_case(simple_transfer())]
-#[test_case(simple_nonce_fee_only(false))]
-#[test_case(simple_nonce_fee_only(true))]
+#[test_case(simple_nonce_fee_only(false, false))]
+#[test_case(simple_nonce_fee_only(true, false))]
+#[test_case(simple_nonce_fee_only(false, true))]
+#[test_case(simple_nonce_fee_only(true, true))]
 fn svm_integration(test_entries: Vec<SvmTestEntry>) {
     for test_entry in test_entries {
         execute_test_entry(test_entry);

svm/tests/mock_bank.rs

@@ -217,7 +217,8 @@ pub fn create_executable_environment(
         .unwrap()
         .insert(Rent::id(), account_data);
 
-    // advance nonce asserts recent blockhashes is non-empty but then just gets the blockhash from ic
+    // SystemInstruction::AdvanceNonceAccount asserts RecentBlockhashes is non-empty
+    // but then just gets the blockhash from InvokeContext. so the sysvar doesnt need real entries
     #[allow(deprecated)]
     let recent_blockhashes = vec![BlockhashesEntry::default()];