Cleanup the bank's use of nonces (#21246)

runtime/src/accounts.rs

@@ -30,7 +30,7 @@ use solana_sdk::{
     hash::Hash,
     message::SanitizedMessage,
     native_loader,
-    nonce::{state::Versions as NonceVersions, State as NonceState, NONCED_TX_MARKER_IX_INDEX},
+    nonce::{state::Versions as NonceVersions, State as NonceState},
     pubkey::Pubkey,
     system_program, sysvar,
     sysvar::instructions::construct_instructions_data,
@@ -1033,8 +1033,8 @@ impl Accounts {
         blockhash: &Hash,
         lamports_per_signature: u64,
         rent_for_sysvars: bool,
-        merge_nonce_error_into_system_error: bool,
         demote_program_write_locks: bool,
+        leave_nonce_on_success: bool,
     ) {
         let accounts_to_store = self.collect_accounts_to_store(
             txs,
@@ -1044,8 +1044,8 @@ impl Accounts {
             blockhash,
             lamports_per_signature,
             rent_for_sysvars,
-            merge_nonce_error_into_system_error,
             demote_program_write_locks,
+            leave_nonce_on_success,
         );
         self.accounts_db.store_cached(slot, &accounts_to_store);
     }
@@ -1072,8 +1072,8 @@ impl Accounts {
         blockhash: &Hash,
         lamports_per_signature: u64,
         rent_for_sysvars: bool,
-        merge_nonce_error_into_system_error: bool,
         demote_program_write_locks: bool,
+        leave_nonce_on_success: bool,
     ) -> Vec<(&'a Pubkey, &'a AccountSharedData)> {
         let mut accounts = Vec::with_capacity(load_results.len());
         for (i, ((tx_load_result, _), tx)) in load_results.iter_mut().zip(txs).enumerate() {
@@ -1085,30 +1085,17 @@ impl Accounts {
             let (execution_result, nonce) = &execution_results[i];
             let maybe_nonce = match (execution_result, nonce) {
                 (Ok(_), Some(nonce)) => {
-                    let address = nonce.address();
+                    if leave_nonce_on_success {
-                    let account = nonce.account();
+                        None
-                    let maybe_fee_payer_account = nonce.fee_payer_account();
-                    Some((address, account, maybe_fee_payer_account, true))
-                }
-                (Err(TransactionError::InstructionError(index, _)), Some(nonce)) => {
-                    let nonce_marker_ix_failed = if merge_nonce_error_into_system_error {
-                        // Don't advance stored blockhash when the nonce marker ix fails
-                        *index == NONCED_TX_MARKER_IX_INDEX
                     } else {
-                        false
+                        Some((nonce, false /* rollback */))
-                    };
-                    let address = nonce.address();
-                    let account = nonce.account();
-                    let maybe_fee_payer_account = nonce.fee_payer_account();
-                    Some((
-                        address,
-                        account,
-                        maybe_fee_payer_account,
-                        !nonce_marker_ix_failed,
-                    ))
                     }
-                (Ok(_), _nonce) => None,
+                }
-                (Err(_), _nonce) => continue,
+                (Err(TransactionError::InstructionError(_, _)), Some(nonce)) => {
+                    Some((nonce, true /* rollback */))
+                }
+                (Ok(_), _) => None, // Success, don't do any additional nonce processing
+                (Err(_), _) => continue, // Not nonce, don't store any accounts
             };
 
             let message = tx.message();
@@ -1118,35 +1105,22 @@ impl Accounts {
                 .zip(loaded_transaction.accounts.iter_mut())
                 .filter(|(i, _)| message.is_non_loader_key(*i))
             {
-                let is_nonce_account = prepare_if_nonce_account(
-                    address,
-                    account,
-                    execution_result,
-                    maybe_nonce,
-                    blockhash,
-                    lamports_per_signature,
-                );
                 if fee_payer_index.is_none() {
                     fee_payer_index = Some(i);
                 }
                 let is_fee_payer = Some(i) == fee_payer_index;
-                if message.is_writable(i, demote_program_write_locks)
+                if message.is_writable(i, demote_program_write_locks) {
-                    && (execution_result.is_ok()
+                    let is_nonce_account = prepare_if_nonce_account(
-                        || (maybe_nonce.is_some() && (is_nonce_account || is_fee_payer)))
+                        address,
-                {
+                        account,
-                    if execution_result.is_err() {
+                        execution_result,
-                        match (is_nonce_account, is_fee_payer, maybe_nonce) {
+                        is_fee_payer,
-                            // nonce is fee-payer, state updated in `prepare_if_nonce_account()`
+                        maybe_nonce,
-                            (true, true, Some((_, _, None, _))) => (),
+                        blockhash,
-                            // nonce not fee-payer, state updated in `prepare_if_nonce_account()`
+                        lamports_per_signature,
-                            (true, false, Some((_, _, Some(_), _))) => (),
+                    );
-                            // not nonce, but fee-payer. rollback to cached state
+
-                            (false, true, Some((_, _, Some(fee_payer_account), _))) => {
+                    if execution_result.is_ok() || is_nonce_account || is_fee_payer {
-                                *account = fee_payer_account.clone();
-                            }
-                            _ => panic!("unexpected nonce condition"),
-                        }
-                    }
                         if account.rent_epoch() == INITIAL_RENT_EPOCH {
                             let rent = rent_collector.collect_from_created_account(
                                 address,
@@ -1154,9 +1128,11 @@ impl Accounts {
                                 rent_for_sysvars,
                             );
                             loaded_transaction.rent += rent;
-                        loaded_transaction
+                            loaded_transaction.rent_debits.insert(
-                            .rent_debits
+                                address,
-                            .insert(address, rent, account.lamports());
+                                rent,
+                                account.lamports(),
+                            );
                         }
 
                         // Add to the accounts to store
@@ -1164,40 +1140,37 @@ impl Accounts {
                     }
                 }
             }
+        }
         accounts
     }
 }
 
-pub fn prepare_if_nonce_account(
+pub fn prepare_if_nonce_account<'a>(
     address: &Pubkey,
     account: &mut AccountSharedData,
     execution_result: &Result<()>,
-    maybe_nonce: Option<(
+    is_fee_payer: bool,
-        &Pubkey,
+    maybe_nonce: Option<(&'a NonceFull, bool)>,
-        &AccountSharedData,
-        Option<&AccountSharedData>,
-        bool,
-    )>,
     blockhash: &Hash,
     lamports_per_signature: u64,
 ) -> bool {
-    if let Some((nonce_key, nonce_account, _, advance_blockhash)) = maybe_nonce {
+    if let Some((nonce, rollback)) = maybe_nonce {
-        if address == nonce_key {
+        if address == nonce.address() {
-            if execution_result.is_err() {
+            if rollback {
                 // The transaction failed which would normally drop the account
-                // processing changes, since this account is now being included in
+                // processing changes, since this account is now being included
-                // the accounts written back to the db, roll it back to
+                // in the accounts written back to the db, roll it back to
                 // pre-processing state.
-                *account = nonce_account.clone();
+                *account = nonce.account().clone();
             }
 
-            if advance_blockhash {
             // Advance the stored blockhash to prevent fee theft by someone
             // replaying nonce transactions that have failed with an
             // `InstructionError`.
             //
-                // Since we know we are dealing with a valid nonce account, unwrap is safe here
+            // Since we know we are dealing with a valid nonce account,
-                let state = StateMut::<NonceVersions>::state(nonce_account)
+            // unwrap is safe here
+            let state = StateMut::<NonceVersions>::state(nonce.account())
                 .unwrap()
                 .convert_to_current();
             if let NonceState::Initialized(ref data) = state {
@@ -1209,12 +1182,23 @@ pub fn prepare_if_nonce_account(
                     )))
                     .unwrap();
             }
-            }
+            true
-            return true;
+        } else {
+            if execution_result.is_err() && is_fee_payer {
+                if let Some(fee_payer_account) = nonce.fee_payer_account() {
+                    // Instruction error and fee-payer for this nonce tx is not
+                    // the nonce account itself, rollback the fee payer to the
+                    // fee-paid original state.
+                    *account = fee_payer_account.clone();
                 }
             }
+
             false
         }
+    } else {
+        false
+    }
+}
 
 pub fn create_test_accounts(
     accounts: &Accounts,
@@ -2596,8 +2580,8 @@ mod tests {
             &Hash::default(),
             0,
             true,
-            true, // merge_nonce_error_into_system_error
             true, // demote_program_write_locks
+            true, // leave_nonce_on_success
         );
         assert_eq!(collected_accounts.len(), 2);
         assert!(collected_accounts
@@ -2722,39 +2706,32 @@ mod tests {
         account_address: &Pubkey,
         account: &mut AccountSharedData,
         tx_result: &Result<()>,
-        maybe_nonce: Option<(
+        is_fee_payer: bool,
-            &Pubkey,
+        maybe_nonce: Option<(&NonceFull, bool)>,
-            &AccountSharedData,
-            Option<&AccountSharedData>,
-            bool,
-        )>,
         blockhash: &Hash,
         lamports_per_signature: u64,
         expect_account: &AccountSharedData,
     ) -> bool {
         // Verify expect_account's relationship
+        if !is_fee_payer {
             match maybe_nonce {
-            Some((nonce_address, _nonce_account, _maybe_fee_payer_account, _))
+                Some((nonce, _)) if nonce.address() == account_address => {
-                if nonce_address == account_address && tx_result.is_ok() =>
+                    assert_ne!(expect_account, nonce.account())
-            {
-                assert_eq!(expect_account, account) // Account update occurs in system_instruction_processor
-            }
-            Some((nonce_address, nonce_account, _maybe_fee_payer_account, _))
-                if nonce_address == account_address =>
-            {
-                assert_ne!(expect_account, nonce_account)
                 }
                 _ => assert_eq!(expect_account, account),
             }
+        }
 
         prepare_if_nonce_account(
             account_address,
             account,
             tx_result,
+            is_fee_payer,
             maybe_nonce,
             blockhash,
             lamports_per_signature,
         );
+        assert_eq!(expect_account, account);
         expect_account == account
     }
 
@@ -2769,22 +2746,25 @@ mod tests {
             maybe_fee_payer_account,
         ) = create_accounts_prepare_if_nonce_account();
         let post_account_address = pre_account_address;
+        let nonce = NonceFull::new(
+            pre_account_address,
+            pre_account.clone(),
+            maybe_fee_payer_account,
+        );
 
-        let mut expect_account = post_account.clone();
+        let mut expect_account = pre_account;
-        let data =
+        expect_account
-            NonceVersions::new_current(NonceState::Initialized(nonce::state::Data::default()));
+            .set_state(&NonceVersions::new_current(NonceState::Initialized(
-        expect_account.set_state(&data).unwrap();
+                nonce::state::Data::new(Pubkey::default(), blockhash, lamports_per_signature),
+            )))
+            .unwrap();
 
         assert!(run_prepare_if_nonce_account_test(
             &post_account_address,
             &mut post_account,
             &Ok(()),
-            Some((
-                &pre_account_address,
-                &pre_account,
-                maybe_fee_payer_account.as_ref(),
             false,
-            )),
+            Some((&nonce, true)),
             &blockhash,
             lamports_per_signature,
             &expect_account,
@@ -2809,6 +2789,7 @@ mod tests {
             &post_account_address,
             &mut post_account,
             &Ok(()),
+            false,
             None,
             &blockhash,
             lamports_per_signature,
@@ -2827,18 +2808,16 @@ mod tests {
             maybe_fee_payer_account,
         ) = create_accounts_prepare_if_nonce_account();
 
+        let nonce = NonceFull::new(pre_account_address, pre_account, maybe_fee_payer_account);
+
         let expect_account = post_account.clone();
         // Wrong key
         assert!(run_prepare_if_nonce_account_test(
             &Pubkey::new(&[1u8; 32]),
             &mut post_account,
             &Ok(()),
-            Some((
+            false,
-                &pre_account_address,
+            Some((&nonce, true)),
-                &pre_account,
-                maybe_fee_payer_account.as_ref(),
-                true,
-            )),
             &blockhash,
             lamports_per_signature,
             &expect_account,
@@ -2856,8 +2835,10 @@ mod tests {
             maybe_fee_payer_account,
         ) = create_accounts_prepare_if_nonce_account();
         let post_account_address = pre_account_address;
-
         let mut expect_account = pre_account.clone();
+
+        let nonce = NonceFull::new(pre_account_address, pre_account, maybe_fee_payer_account);
+
         expect_account
             .set_state(&NonceVersions::new_current(NonceState::Initialized(
                 nonce::state::Data::new(Pubkey::default(), blockhash, lamports_per_signature),
@@ -2871,18 +2852,81 @@ mod tests {
                 0,
                 InstructionError::InvalidArgument,
             )),
-            Some((
+            false,
-                &pre_account_address,
+            Some((&nonce, true)),
-                &pre_account,
-                maybe_fee_payer_account.as_ref(),
-                true,
-            )),
             &blockhash,
             lamports_per_signature,
             &expect_account,
         ));
     }
 
+    #[test]
+    fn test_rollback_nonce_fee_payer() {
+        let nonce_account = AccountSharedData::new_data(1, &(), &system_program::id()).unwrap();
+        let pre_fee_payer_account =
+            AccountSharedData::new_data(42, &(), &system_program::id()).unwrap();
+        let mut post_fee_payer_account =
+            AccountSharedData::new_data(84, &[1, 2, 3, 4], &system_program::id()).unwrap();
+        let nonce = NonceFull::new(
+            Pubkey::new_unique(),
+            nonce_account,
+            Some(pre_fee_payer_account.clone()),
+        );
+
+        assert!(run_prepare_if_nonce_account_test(
+            &Pubkey::new_unique(),
+            &mut post_fee_payer_account.clone(),
+            &Err(TransactionError::InstructionError(
+                0,
+                InstructionError::InvalidArgument,
+            )),
+            false,
+            Some((&nonce, true)),
+            &Hash::default(),
+            1,
+            &post_fee_payer_account.clone(),
+        ));
+
+        assert!(run_prepare_if_nonce_account_test(
+            &Pubkey::new_unique(),
+            &mut post_fee_payer_account.clone(),
+            &Ok(()),
+            true,
+            Some((&nonce, true)),
+            &Hash::default(),
+            1,
+            &post_fee_payer_account.clone(),
+        ));
+
+        assert!(run_prepare_if_nonce_account_test(
+            &Pubkey::new_unique(),
+            &mut post_fee_payer_account.clone(),
+            &Err(TransactionError::InstructionError(
+                0,
+                InstructionError::InvalidArgument,
+            )),
+            true,
+            None,
+            &Hash::default(),
+            1,
+            &post_fee_payer_account.clone(),
+        ));
+
+        assert!(run_prepare_if_nonce_account_test(
+            &Pubkey::new_unique(),
+            &mut post_fee_payer_account,
+            &Err(TransactionError::InstructionError(
+                0,
+                InstructionError::InvalidArgument,
+            )),
+            true,
+            Some((&nonce, true)),
+            &Hash::default(),
+            1,
+            &pre_fee_payer_account,
+        ));
+    }
+
     #[test]
     fn test_nonced_failure_accounts_rollback_from_pays() {
         let rent_collector = RentCollector::default();
@@ -2966,8 +3010,8 @@ mod tests {
             &next_blockhash,
             0,
             true,
-            true, // merge_nonce_error_into_system_error
             true, // demote_program_write_locks
+            true, // leave_nonce_on_success
         );
         assert_eq!(collected_accounts.len(), 2);
         assert_eq!(
@@ -3077,8 +3121,8 @@ mod tests {
             &next_blockhash,
             0,
             true,
-            true, // merge_nonce_error_into_system_error
             true, // demote_program_write_locks
+            true, // leave_nonce_on_success
         );
         assert_eq!(collected_accounts.len(), 1);
         let collected_nonce_account = collected_accounts

runtime/src/bank.rs

@@ -669,17 +669,13 @@ impl NonceFull {
 
             let nonce_address = *partial.address();
             if *fee_payer_address == nonce_address {
-                Ok(Self {
+                Ok(Self::new(nonce_address, fee_payer_account, None))
-                    address: nonce_address,
-                    account: fee_payer_account,
-                    fee_payer_account: None,
-                })
             } else {
-                Ok(Self {
+                Ok(Self::new(
-                    address: nonce_address,
+                    nonce_address,
-                    account: partial.account().clone(),
+                    partial.account().clone(),
-                    fee_payer_account: Some(fee_payer_account),
+                    Some(fee_payer_account),
-                })
+                ))
             }
         } else {
             Err(TransactionError::AccountNotFound)
@@ -3935,14 +3931,13 @@ impl Bank {
                         inner_instructions.push(None);
                     }
 
-                    let nonce =
+                    let nonce = match &process_result {
-                        if let Err(TransactionError::InstructionError(_, _)) = &process_result {
+                        Ok(_) => nonce.clone(), // May need to calculate the fee based on the nonce
+                        Err(TransactionError::InstructionError(_, _)) => {
                             error_counters.instruction_error += 1;
-                            nonce.clone()
+                            nonce.clone() // May need to advance the nonce
-                        } else if process_result.is_err() {
+                        }
-                            None
+                        _ => None,
-                        } else {
-                            nonce.clone()
                     };
 
                     (process_result, nonce)
@@ -4176,8 +4171,8 @@ impl Bank {
             &blockhash,
             lamports_per_signature,
             self.rent_for_sysvars(),
-            self.merge_nonce_error_into_system_error(),
             self.demote_program_write_locks(),
+            self.leave_nonce_on_success(),
         );
         let rent_debits = self.collect_rent(executed_results, loaded_txs);
 
@@ -6038,11 +6033,6 @@ impl Bank {
             .is_active(&feature_set::verify_tx_signatures_len::id())
     }
 
-    pub fn merge_nonce_error_into_system_error(&self) -> bool {
-        self.feature_set
-            .is_active(&feature_set::merge_nonce_error_into_system_error::id())
-    }
-
     pub fn versioned_tx_message_enabled(&self) -> bool {
         self.feature_set
             .is_active(&feature_set::versioned_tx_message_enabled::id())
@@ -6058,6 +6048,11 @@ impl Bank {
             .is_active(&feature_set::demote_program_write_locks::id())
     }
 
+    pub fn leave_nonce_on_success(&self) -> bool {
+        self.feature_set
+            .is_active(&feature_set::leave_nonce_on_success::id())
+    }
+
     pub fn stakes_remove_delegation_if_inactive_enabled(&self) -> bool {
         self.feature_set
             .is_active(&feature_set::stakes_remove_delegation_if_inactive::id())
@@ -11367,9 +11362,6 @@ pub(crate) mod tests {
         debug!("cust: {:?}", custodian_account);
         let nonce_hash = get_nonce_blockhash(&bank, &nonce_pubkey).unwrap();
 
-        Arc::get_mut(&mut bank)
-            .unwrap()
-            .activate_feature(&feature_set::merge_nonce_error_into_system_error::id());
         for _ in 0..MAX_RECENT_BLOCKHASHES + 1 {
             goto_end_of_slot(Arc::get_mut(&mut bank).unwrap());
             bank = Arc::new(new_from_parent(&bank));
@@ -11403,7 +11395,7 @@ pub(crate) mod tests {
                     .get_fee_for_message(&recent_message.try_into().unwrap())
                     .unwrap()
         );
-        assert_eq!(
+        assert_ne!(
             nonce_hash,
             get_nonce_blockhash(&bank, &nonce_pubkey).unwrap()
         );

sdk/src/feature_set.rs

@@ -245,6 +245,10 @@ pub mod spl_token_v3_3_0_release {
     solana_sdk::declare_id!("Ftok2jhqAqxUWEiCVRrfRs9DPppWP8cgTB7NQNKL88mS");
 }
 
+pub mod leave_nonce_on_success {
+    solana_sdk::declare_id!("E8MkiWZNNPGU6n55jkGzyj8ghUmjCHRmDFdYYFYHxWhQ");
+}
+
 lazy_static! {
     /// Map of feature identifiers to user-visible description
     pub static ref FEATURE_NAMES: HashMap<Pubkey, &'static str> = [
@@ -301,6 +305,7 @@ lazy_static! {
         (reject_deployment_of_unresolved_syscalls::id(), "Reject deployment of programs with unresolved syscall symbols"),
         (nonce_must_be_writable::id(), "nonce must be writable"),
         (spl_token_v3_3_0_release::id(), "spl-token v3.3.0 release"),
+        (leave_nonce_on_success::id(), "leave nonce as is on success"),
         /*************** ADD NEW FEATURES HERE ***************/
     ]
     .iter()