Manage durable nonce stored value in runtime (#7684)

* Bank: Return nonce pubkey/account from `check_tx_durable_nonce`

* Forward account with HashAgeKind::DurableNonce

* Add durable nonce helper for HashAgeKind

* Add nonce util for advancing stored nonce in runtime

* Advance nonce in runtime

* Store rolled back nonce account on TX InstructionError

* nonce: Add test for replayed InstErr fee theft

Author: t-nelson

core/src/transaction_status_service.rs

@@ -64,7 +64,7 @@ impl TransactionStatusService {
             .zip(balances.post_balances)
         {
             if Bank::can_commit(&status) && !transaction.signatures.is_empty() {
-                let fee_hash = if let Some(HashAgeKind::DurableNonce) = hash_age_kind {
+                let fee_hash = if let Some(HashAgeKind::DurableNonce(_, _)) = hash_age_kind {
                     bank.last_blockhash()
                 } else {
                     transaction.message().recent_blockhash

runtime/src/accounts.rs

@@ -4,6 +4,7 @@ use crate::append_vec::StoredAccount;
 use crate::bank::{HashAgeKind, TransactionProcessResult};
 use crate::blockhash_queue::BlockhashQueue;
 use crate::message_processor::has_duplicates;
+use crate::nonce_utils::prepare_if_nonce_account;
 use crate::rent_collector::RentCollector;
 use crate::system_instruction_processor::{get_system_account_kind, SystemAccountKind};
 use log::*;
@@ -12,6 +13,7 @@ use solana_metrics::inc_new_counter_error;
 use solana_sdk::account::Account;
 use solana_sdk::bank_hash::BankHash;
 use solana_sdk::clock::Slot;
+use solana_sdk::hash::Hash;
 use solana_sdk::message::Message;
 use solana_sdk::native_loader;
 use solana_sdk::nonce_state::NonceState;
@@ -248,7 +250,7 @@ impl Accounts {
             .zip(lock_results.into_iter())
             .map(|etx| match etx {
                 (tx, (Ok(()), hash_age_kind)) => {
-                    let fee_hash = if let Some(HashAgeKind::DurableNonce) = hash_age_kind {
+                    let fee_hash = if let Some(HashAgeKind::DurableNonce(_, _)) = hash_age_kind {
                         hash_queue.last_hash()
                     } else {
                         tx.message().recent_blockhash
@@ -551,9 +553,16 @@ impl Accounts {
         res: &[TransactionProcessResult],
         loaded: &mut [(Result<TransactionLoadResult>, Option<HashAgeKind>)],
         rent_collector: &RentCollector,
+        last_blockhash: &Hash,
     ) {
-        let accounts_to_store =
-            self.collect_accounts_to_store(txs, txs_iteration_order, res, loaded, rent_collector);
+        let accounts_to_store = self.collect_accounts_to_store(
+            txs,
+            txs_iteration_order,
+            res,
+            loaded,
+            rent_collector,
+            last_blockhash,
+        );
         self.accounts_db.store(slot, &accounts_to_store);
     }
 
@@ -574,17 +583,27 @@ impl Accounts {
         res: &'a [TransactionProcessResult],
         loaded: &'a mut [(Result<TransactionLoadResult>, Option<HashAgeKind>)],
         rent_collector: &RentCollector,
+        last_blockhash: &Hash,
     ) -> Vec<(&'a Pubkey, &'a Account)> {
         let mut accounts = Vec::with_capacity(loaded.len());
         for (i, ((raccs, _hash_age_kind), tx)) in loaded
             .iter_mut()
             .zip(OrderedIterator::new(txs, txs_iteration_order))
             .enumerate()
         {
-            let (res, _hash_age_kind) = &res[i];
-            if res.is_err() || raccs.is_err() {
+            if raccs.is_err() {
                 continue;
             }
+            let (res, hash_age_kind) = &res[i];
+            let maybe_nonce = match (res, hash_age_kind) {
+                (Ok(_), Some(HashAgeKind::DurableNonce(pubkey, acc))) => Some((pubkey, acc)),
+                (
+                    Err(TransactionError::InstructionError(_, _)),
+                    Some(HashAgeKind::DurableNonce(pubkey, acc)),
+                ) => Some((pubkey, acc)),
+                (Ok(_), _hash_age_kind) => None,
+                (Err(_), _hash_age_kind) => continue,
+            };
 
             let message = &tx.message();
             let acc = raccs.as_mut().unwrap();
@@ -594,6 +613,7 @@ impl Accounts {
                 .enumerate()
                 .zip(acc.0.iter_mut())
             {
+                prepare_if_nonce_account(account, key, res, maybe_nonce, last_blockhash);
                 if message.is_writable(i) {
                     if account.rent_epoch == 0 {
                         account.rent_epoch = rent_collector.epoch;
@@ -1593,8 +1613,14 @@ mod tests {
                 },
             );
         }
-        let collected_accounts =
-            accounts.collect_accounts_to_store(&txs, None, &loaders, &mut loaded, &rent_collector);
+        let collected_accounts = accounts.collect_accounts_to_store(
+            &txs,
+            None,
+            &loaders,
+            &mut loaded,
+            &rent_collector,
+            &Hash::default(),
+        );
         assert_eq!(collected_accounts.len(), 2);
         assert!(collected_accounts
             .iter()

runtime/src/bank.rs

@@ -183,7 +183,16 @@ pub type TransactionBalances = Vec<Vec<u64>>;
 #[derive(Clone, Debug, Eq, PartialEq)]
 pub enum HashAgeKind {
     Extant,
-    DurableNonce,
+    DurableNonce(Pubkey, Account),
+}
+
+impl HashAgeKind {
+    pub fn is_durable_nonce(&self) -> bool {
+        match self {
+            HashAgeKind::DurableNonce(_, _) => true,
+            _ => false,
+        }
+    }
 }
 
 /// Manager for the state of all accounts and programs after processing its entries.
@@ -969,8 +978,8 @@ impl Bank {
                     let message = tx.message();
                     if hash_queue.check_hash_age(&message.recent_blockhash, max_age) {
                         (Ok(()), Some(HashAgeKind::Extant))
-                    } else if self.check_tx_durable_nonce(&tx) {
-                        (Ok(()), Some(HashAgeKind::DurableNonce))
+                    } else if let Some((pubkey, acc)) = self.check_tx_durable_nonce(&tx) {
+                        (Ok(()), Some(HashAgeKind::DurableNonce(pubkey, acc)))
                     } else {
                         error_counters.reserve_blockhash += 1;
                         (Err(TransactionError::BlockhashNotFound), None)
@@ -1024,16 +1033,16 @@ impl Bank {
             .check_hash_age(hash, max_age)
     }
 
-    pub fn check_tx_durable_nonce(&self, tx: &Transaction) -> bool {
+    pub fn check_tx_durable_nonce(&self, tx: &Transaction) -> Option<(Pubkey, Account)> {
         nonce_utils::transaction_uses_durable_nonce(&tx)
             .and_then(|nonce_ix| nonce_utils::get_nonce_pubkey_from_instruction(&nonce_ix, &tx))
-            .and_then(|nonce_pubkey| self.get_account(&nonce_pubkey))
-            .map_or_else(
-                || false,
-                |nonce_account| {
-                    nonce_utils::verify_nonce(&nonce_account, &tx.message().recent_blockhash)
-                },
-            )
+            .and_then(|nonce_pubkey| {
+                self.get_account(&nonce_pubkey)
+                    .map(|acc| (*nonce_pubkey, acc))
+            })
+            .filter(|(_pubkey, nonce_account)| {
+                nonce_utils::verify_nonce(nonce_account, &tx.message().recent_blockhash)
+            })
     }
 
     pub fn check_transactions(
@@ -1223,7 +1232,11 @@ impl Bank {
         let results = OrderedIterator::new(txs, iteration_order)
             .zip(executed.iter())
             .map(|(tx, (res, hash_age_kind))| {
-                let fee_hash = if let Some(HashAgeKind::DurableNonce) = hash_age_kind {
+                let is_durable_nonce = hash_age_kind
+                    .as_ref()
+                    .map(|hash_age_kind| hash_age_kind.is_durable_nonce())
+                    .unwrap_or(false);
+                let fee_hash = if is_durable_nonce {
                     self.last_blockhash()
                 } else {
                     tx.message().recent_blockhash
@@ -1239,7 +1252,12 @@ impl Bank {
                         // credit the transaction fee even in case of InstructionError
                         // necessary to withdraw from account[0] here because previous
                         // work of doing so (in accounts.load()) is ignored by store_account()
-                        self.withdraw(&message.account_keys[0], fee)?;
+                        //
+                        // ...except nonce accounts, which will have their post-load,
+                        // pre-execute account state stored
+                        if !is_durable_nonce {
+                            self.withdraw(&message.account_keys[0], fee)?;
+                        }
                         fees += fee;
                         Ok(())
                     }
@@ -1291,6 +1309,7 @@ impl Bank {
             executed,
             loaded_accounts,
             &self.rent_collector,
+            &self.last_blockhash(),
         );
         self.collect_rent(executed, loaded_accounts);
 
@@ -1905,6 +1924,14 @@ mod tests {
     use std::{io::Cursor, result, time::Duration};
     use tempfile::TempDir;
 
+    #[test]
+    fn test_hash_age_kind_is_durable_nonce() {
+        assert!(
+            HashAgeKind::DurableNonce(Pubkey::default(), Account::default()).is_durable_nonce()
+        );
+        assert!(!HashAgeKind::Extant.is_durable_nonce());
+    }
+
     #[test]
     fn test_bank_unix_timestamp() {
         let (genesis_config, _mint_keypair) = create_genesis_config(1);
@@ -4713,7 +4740,11 @@ mod tests {
             &[&custodian_keypair, &nonce_keypair],
             nonce_hash,
         );
-        assert!(bank.check_tx_durable_nonce(&tx));
+        let nonce_account = bank.get_account(&nonce_pubkey).unwrap();
+        assert_eq!(
+            bank.check_tx_durable_nonce(&tx),
+            Some((nonce_pubkey, nonce_account))
+        );
     }
 
     #[test]
@@ -4733,7 +4764,7 @@ mod tests {
             &[&custodian_keypair, &nonce_keypair],
             nonce_hash,
         );
-        assert!(!bank.check_tx_durable_nonce(&tx));
+        assert!(bank.check_tx_durable_nonce(&tx).is_none());
     }
 
     #[test]
@@ -4754,7 +4785,7 @@ mod tests {
             nonce_hash,
         );
         tx.message.instructions[0].accounts.clear();
-        assert!(!bank.check_tx_durable_nonce(&tx));
+        assert!(bank.check_tx_durable_nonce(&tx).is_none());
     }
 
     #[test]
@@ -4776,7 +4807,7 @@ mod tests {
             &[&custodian_keypair, &nonce_keypair],
             nonce_hash,
         );
-        assert!(!bank.check_tx_durable_nonce(&tx));
+        assert!(bank.check_tx_durable_nonce(&tx).is_none());
     }
 
     #[test]
@@ -4795,7 +4826,7 @@ mod tests {
             &[&custodian_keypair, &nonce_keypair],
             Hash::default(),
         );
-        assert!(!bank.check_tx_durable_nonce(&tx));
+        assert!(bank.check_tx_durable_nonce(&tx).is_none());
     }
 
     #[test]
@@ -4908,10 +4939,11 @@ mod tests {
             bank.process_transaction(&durable_tx),
             Err(TransactionError::BlockhashNotFound)
         );
-        /* Check fee not charged */
+        /* Check fee not charged and nonce not advanced */
         assert_eq!(bank.get_balance(&custodian_pubkey), 4_640_000);
+        assert_eq!(new_nonce, get_nonce(&bank, &nonce_pubkey).unwrap());
 
-        let nonce_hash = get_nonce(&bank, &nonce_pubkey).unwrap();
+        let nonce_hash = new_nonce;
 
         /* Kick nonce hash off the blockhash_queue */
         for _ in 0..MAX_RECENT_BLOCKHASHES + 1 {
@@ -4935,8 +4967,16 @@ mod tests {
                 system_instruction::SystemError::ResultWithNegativeLamports.into()
             ))
         );
-        /* Check fee charged */
+        /* Check fee charged and nonce has advanced */
         assert_eq!(bank.get_balance(&custodian_pubkey), 4_630_000);
+        assert_ne!(nonce_hash, get_nonce(&bank, &nonce_pubkey).unwrap());
+        /* Confirm replaying a TX that failed with InstructionError::* now
+         * fails with TransactionError::BlockhashNotFound
+         */
+        assert_eq!(
+            bank.process_transaction(&durable_tx),
+            Err(TransactionError::BlockhashNotFound),
+        );
     }
 
     #[test]