Nonce accounts must be writeable (#21260)

* Nonce accounts must be writeable

* feedback

* feedback

rpc/src/rpc.rs

@@ -56,7 +56,7 @@ use {
         epoch_info::EpochInfo,
         epoch_schedule::EpochSchedule,
         exit::Exit,
-        feature_set,
+        feature_set::{self, nonce_must_be_writable},
         fee_calculator::FeeCalculator,
         hash::Hash,
         message::{Message, SanitizedMessage},
@@ -3396,7 +3396,11 @@ pub mod rpc_full {
                 .unwrap_or(0);
 
             let durable_nonce_info = transaction
-                .get_durable_nonce()
+                .get_durable_nonce(
+                    preflight_bank
+                        .feature_set
+                        .is_active(&nonce_must_be_writable::id()),
+                )
                 .map(|&pubkey| (pubkey, *transaction.message().recent_blockhash()));
             if durable_nonce_info.is_some() {
                 // While it uses a defined constant, this last_valid_block_height value is chosen arbitrarily.

runtime/src/bank.rs

@@ -92,7 +92,9 @@ use solana_sdk::{
     epoch_info::EpochInfo,
     epoch_schedule::EpochSchedule,
     feature,
-    feature_set::{self, disable_fee_calculator, tx_wide_compute_cap, FeatureSet},
+    feature_set::{
+        self, disable_fee_calculator, nonce_must_be_writable, tx_wide_compute_cap, FeatureSet,
+    },
     fee_calculator::{FeeCalculator, FeeRateGovernor},
     genesis_config::{ClusterType, GenesisConfig},
     hard_forks::HardForks,
@@ -3541,7 +3543,7 @@ impl Bank {
         &self,
         tx: &SanitizedTransaction,
     ) -> Option<(Pubkey, AccountSharedData)> {
-        tx.get_durable_nonce()
+        tx.get_durable_nonce(self.feature_set.is_active(&nonce_must_be_writable::id()))
             .and_then(|nonce_pubkey| {
                 self.get_account_with_fixed_root(nonce_pubkey)
                     .map(|acc| (*nonce_pubkey, acc))
@@ -11433,6 +11435,58 @@ pub(crate) mod tests {
         assert_ne!(stored_fee_calculator, fee_calculator);
     }
 
+    #[test]
+    fn test_check_ro_durable_nonce_fails() {
+        let (mut bank, _mint_keypair, custodian_keypair, nonce_keypair) =
+            setup_nonce_with_bank(10_000_000, |_| {}, 5_000_000, 250_000, None).unwrap();
+        Arc::get_mut(&mut bank)
+            .unwrap()
+            .activate_feature(&feature_set::nonce_must_be_writable::id());
+        let custodian_pubkey = custodian_keypair.pubkey();
+        let nonce_pubkey = nonce_keypair.pubkey();
+
+        let nonce_hash = get_nonce_account(&bank, &nonce_pubkey).unwrap();
+        let account_metas = vec![
+            AccountMeta::new_readonly(nonce_pubkey, false),
+            #[allow(deprecated)]
+            AccountMeta::new_readonly(sysvar::recent_blockhashes::id(), false),
+            AccountMeta::new_readonly(nonce_pubkey, true),
+        ];
+        let nonce_instruction = Instruction::new_with_bincode(
+            system_program::id(),
+            &system_instruction::SystemInstruction::AdvanceNonceAccount,
+            account_metas,
+        );
+        let tx = Transaction::new_signed_with_payer(
+            &[nonce_instruction],
+            Some(&custodian_pubkey),
+            &[&custodian_keypair, &nonce_keypair],
+            nonce_hash,
+        );
+        // Caught by the system program because the tx hash is valid
+        assert_eq!(
+            bank.process_transaction(&tx),
+            Err(TransactionError::InstructionError(
+                0,
+                InstructionError::InvalidArgument
+            ))
+        );
+        // Kick nonce hash off the blockhash_queue
+        for _ in 0..MAX_RECENT_BLOCKHASHES + 1 {
+            goto_end_of_slot(Arc::get_mut(&mut bank).unwrap());
+            bank = Arc::new(new_from_parent(&bank));
+        }
+        // Caught by the runtime because it is a nonce transaction
+        assert_eq!(
+            bank.process_transaction(&tx),
+            Err(TransactionError::BlockhashNotFound)
+        );
+        assert_eq!(
+            bank.check_tx_durable_nonce(&SanitizedTransaction::from_transaction_for_tests(tx)),
+            None
+        );
+    }
+
     #[test]
     fn test_collect_balances() {
         let (genesis_config, _mint_keypair) = create_genesis_config(500);

runtime/src/nonce_keyed_account.rs

@@ -1,7 +1,8 @@
 use solana_sdk::{
     account::{ReadableAccount, WritableAccount},
     account_utils::State as AccountUtilsState,
-    feature_set, ic_msg,
+    feature_set::{self, nonce_must_be_writable},
+    ic_msg,
     instruction::{checked_add, InstructionError},
     keyed_account::KeyedAccount,
     nonce::{self, state::Versions, State},
@@ -48,6 +49,16 @@ impl<'a> NonceKeyedAccount for KeyedAccount<'a> {
     ) -> Result<(), InstructionError> {
         let merge_nonce_error_into_system_error = invoke_context
             .is_feature_active(&feature_set::merge_nonce_error_into_system_error::id());
+
+        if invoke_context.is_feature_active(&nonce_must_be_writable::id()) && !self.is_writable() {
+            ic_msg!(
+                invoke_context,
+                "Advance nonce account: Account {} must be writeable",
+                self.unsigned_key()
+            );
+            return Err(InstructionError::InvalidArgument);
+        }
+
         let state = AccountUtilsState::<Versions>::state(self)?.convert_to_current();
         match state {
             State::Initialized(data) => {
@@ -102,6 +113,16 @@ impl<'a> NonceKeyedAccount for KeyedAccount<'a> {
     ) -> Result<(), InstructionError> {
         let merge_nonce_error_into_system_error = invoke_context
             .is_feature_active(&feature_set::merge_nonce_error_into_system_error::id());
+
+        if invoke_context.is_feature_active(&nonce_must_be_writable::id()) && !self.is_writable() {
+            ic_msg!(
+                invoke_context,
+                "Withdraw nonce account: Account {} must be writeable",
+                self.unsigned_key()
+            );
+            return Err(InstructionError::InvalidArgument);
+        }
+
         let signer = match AccountUtilsState::<Versions>::state(self)?.convert_to_current() {
             State::Uninitialized => {
                 if lamports > self.lamports()? {
@@ -178,6 +199,16 @@ impl<'a> NonceKeyedAccount for KeyedAccount<'a> {
     ) -> Result<(), InstructionError> {
         let merge_nonce_error_into_system_error = invoke_context
             .is_feature_active(&feature_set::merge_nonce_error_into_system_error::id());
+
+        if invoke_context.is_feature_active(&nonce_must_be_writable::id()) && !self.is_writable() {
+            ic_msg!(
+                invoke_context,
+                "Initialize nonce account: Account {} must be writeable",
+                self.unsigned_key()
+            );
+            return Err(InstructionError::InvalidArgument);
+        }
+
         match AccountUtilsState::<Versions>::state(self)?.convert_to_current() {
             State::Uninitialized => {
                 let min_balance = rent.minimum_balance(self.data_len()?);
@@ -219,6 +250,16 @@ impl<'a> NonceKeyedAccount for KeyedAccount<'a> {
     ) -> Result<(), InstructionError> {
         let merge_nonce_error_into_system_error = invoke_context
             .is_feature_active(&feature_set::merge_nonce_error_into_system_error::id());
+
+        if invoke_context.is_feature_active(&nonce_must_be_writable::id()) && !self.is_writable() {
+            ic_msg!(
+                invoke_context,
+                "Authorize nonce account: Account {} must be writeable",
+                self.unsigned_key()
+            );
+            return Err(InstructionError::InvalidArgument);
+        }
+
         match AccountUtilsState::<Versions>::state(self)?.convert_to_current() {
             State::Initialized(data) => {
                 if !signers.contains(&data.authority) {

runtime/src/system_instruction_processor.rs

@@ -1484,10 +1484,11 @@ mod tests {
 
     #[test]
     fn test_process_nonce_ix_no_acc_data_fail() {
+        let none_address = Pubkey::new_unique();
         assert_eq!(
             process_nonce_instruction(&system_instruction::advance_nonce_account(
-                &Pubkey::default(),
+                &none_address,
-                &Pubkey::default()
+                &none_address
             )),
             Err(InstructionError::InvalidAccountData),
         );
@@ -1509,7 +1510,7 @@ mod tests {
         assert_eq!(
             process_instruction(
                 &serialize(&SystemInstruction::AdvanceNonceAccount).unwrap(),
-                &[(true, false, Pubkey::default(), create_default_account())],
+                &[(true, true, Pubkey::new_unique(), create_default_account())],
             ),
             Err(InstructionError::NotEnoughAccountKeys),
         );
@@ -1521,7 +1522,7 @@ mod tests {
             process_instruction(
                 &serialize(&SystemInstruction::AdvanceNonceAccount).unwrap(),
                 &[
-                    (true, false, Pubkey::default(), create_default_account()),
+                    (true, true, Pubkey::new_unique(), create_default_account()),
                     (
                         false,
                         false,
@@ -1537,11 +1538,12 @@ mod tests {
 
     #[test]
     fn test_process_nonce_ix_ok() {
+        let nonce_address = Pubkey::new_unique();
         let nonce_account = Rc::new(nonce_account::create_account(1_000_000));
         process_instruction(
-            &serialize(&SystemInstruction::InitializeNonceAccount(Pubkey::default())).unwrap(),
+            &serialize(&SystemInstruction::InitializeNonceAccount(nonce_address)).unwrap(),
             &[
-                (true, false, Pubkey::default(), nonce_account.clone()),
+                (true, true, nonce_address, nonce_account.clone()),
                 (
                     false,
                     false,
@@ -1569,7 +1571,7 @@ mod tests {
         #[allow(deprecated)]
         let blockhash_id = sysvar::recent_blockhashes::id();
         let keyed_accounts = [
-            (true, false, Pubkey::default(), nonce_account),
+            (true, true, nonce_address, nonce_account),
             (false, false, blockhash_id, new_recent_blockhashes_account),
         ];
         assert_eq!(
@@ -1595,11 +1597,12 @@ mod tests {
 
     #[test]
     fn test_process_withdraw_ix_no_acc_data_fail() {
+        let nonce_address = Pubkey::new_unique();
         assert_eq!(
             process_nonce_instruction(&system_instruction::withdraw_nonce_account(
-                &Pubkey::default(),
+                &nonce_address,
-                &Pubkey::default(),
+                &Pubkey::new_unique(),
-                &Pubkey::default(),
+                &nonce_address,
                 1,
             )),
             Err(InstructionError::InvalidAccountData),
@@ -1684,8 +1687,8 @@ mod tests {
                 &[
                     (
                         true,
-                        false,
+                        true,
-                        Pubkey::default(),
+                        Pubkey::new_unique(),
                         Rc::new(nonce_account::create_account(1_000_000)),
                     ),
                     (true, false, Pubkey::default(), create_default_account()),
@@ -1788,14 +1791,15 @@ mod tests {
 
     #[test]
     fn test_process_initialize_ix_ok() {
+        let nonce_address = Pubkey::new_unique();
         assert_eq!(
             process_instruction(
-                &serialize(&SystemInstruction::InitializeNonceAccount(Pubkey::default())).unwrap(),
+                &serialize(&SystemInstruction::InitializeNonceAccount(nonce_address)).unwrap(),
                 &[
                     (
                         true,
-                        false,
+                        true,
-                        Pubkey::default(),
+                        nonce_address,
                         Rc::new(nonce_account::create_account(1_000_000)),
                     ),
                     (
@@ -1819,11 +1823,12 @@ mod tests {
 
     #[test]
     fn test_process_authorize_ix_ok() {
+        let nonce_address = Pubkey::new_unique();
         let nonce_account = Rc::new(nonce_account::create_account(1_000_000));
         process_instruction(
-            &serialize(&SystemInstruction::InitializeNonceAccount(Pubkey::default())).unwrap(),
+            &serialize(&SystemInstruction::InitializeNonceAccount(nonce_address)).unwrap(),
             &[
-                (true, false, Pubkey::default(), nonce_account.clone()),
+                (true, true, nonce_address, nonce_account.clone()),
                 (
                     false,
                     false,
@@ -1842,8 +1847,8 @@ mod tests {
         .unwrap();
         assert_eq!(
             process_instruction(
-                &serialize(&SystemInstruction::AuthorizeNonceAccount(Pubkey::default())).unwrap(),
+                &serialize(&SystemInstruction::AuthorizeNonceAccount(nonce_address)).unwrap(),
-                &[(true, false, Pubkey::default(), nonce_account)],
+                &[(true, true, nonce_address, nonce_account)],
             ),
             Ok(()),
         );
@@ -1851,11 +1856,12 @@ mod tests {
 
     #[test]
     fn test_process_authorize_bad_account_data_fail() {
+        let nonce_address = Pubkey::new_unique();
         assert_eq!(
             process_nonce_instruction(&system_instruction::authorize_nonce_account(
-                &Pubkey::default(),
+                &nonce_address,
-                &Pubkey::default(),
+                &Pubkey::new_unique(),
-                &Pubkey::default(),
+                &nonce_address,
             )),
             Err(InstructionError::InvalidAccountData),
         );
@@ -1916,6 +1922,7 @@ mod tests {
 
     #[test]
     fn test_nonce_initialize_with_empty_recent_blockhashes_fail() {
+        let nonce_address = Pubkey::new_unique();
         let nonce_account = Rc::new(nonce_account::create_account(1_000_000));
         #[allow(deprecated)]
         let new_recent_blockhashes_account = Rc::new(RefCell::new(
@@ -1925,9 +1932,9 @@ mod tests {
         ));
         assert_eq!(
             process_instruction(
-                &serialize(&SystemInstruction::InitializeNonceAccount(Pubkey::default())).unwrap(),
+                &serialize(&SystemInstruction::InitializeNonceAccount(nonce_address)).unwrap(),
                 &[
-                    (true, false, Pubkey::default(), nonce_account),
+                    (true, true, nonce_address, nonce_account),
                     (
                         false,
                         false,
@@ -1949,11 +1956,12 @@ mod tests {
 
     #[test]
     fn test_nonce_advance_with_empty_recent_blockhashes_fail() {
+        let nonce_address = Pubkey::new_unique();
         let nonce_account = Rc::new(nonce_account::create_account(1_000_000));
         process_instruction(
-            &serialize(&SystemInstruction::InitializeNonceAccount(Pubkey::default())).unwrap(),
+            &serialize(&SystemInstruction::InitializeNonceAccount(nonce_address)).unwrap(),
             &[
-                (true, false, Pubkey::default(), nonce_account.clone()),
+                (true, true, nonce_address, nonce_account.clone()),
                 (
                     false,
                     false,
@@ -1979,7 +1987,7 @@ mod tests {
         #[allow(deprecated)]
         let blockhash_id = sysvar::recent_blockhashes::id();
         let keyed_accounts = [
-            (true, false, Pubkey::default(), nonce_account),
+            (true, false, nonce_address, nonce_account),
             (false, false, blockhash_id, new_recent_blockhashes_account),
         ];
         assert_eq!(

sdk/src/feature_set.rs

@@ -237,6 +237,10 @@ pub mod reject_deployment_of_unresolved_syscalls {
     solana_sdk::declare_id!("DqniU3MfvdpU3yhmNF1RKeaM5TZQELZuyFGosASRVUoy");
 }
 
+pub mod nonce_must_be_writable {
+    solana_sdk::declare_id!("BiCU7M5w8ZCMykVSyhZ7Q3m2SWoR2qrEQ86ERcDX77ME");
+}
+
 lazy_static! {
     /// Map of feature identifiers to user-visible description
     pub static ref FEATURE_NAMES: HashMap<Pubkey, &'static str> = [
@@ -291,6 +295,7 @@ lazy_static! {
         (disable_fee_calculator::id(), "deprecate fee calculator"),
         (add_compute_budget_program::id(), "Add compute_budget_program"),
         (reject_deployment_of_unresolved_syscalls::id(), "Reject deployment of programs with unresolved syscall symbols"),
+        (nonce_must_be_writable::id(), "nonce must be writable"),
         /*************** ADD NEW FEATURES HERE ***************/
     ]
     .iter()

sdk/src/transaction/mod.rs

@@ -502,14 +502,23 @@ pub fn uses_durable_nonce(tx: &Transaction) -> Option<&CompiledInstruction> {
     message
         .instructions
         .get(NONCED_TX_MARKER_IX_INDEX as usize)
-        .filter(|maybe_ix| {
+        .filter(|instruction| {
-            let prog_id_idx = maybe_ix.program_id_index as usize;
+            // Is system program
-            match message.account_keys.get(prog_id_idx) {
+            matches!(
-                Some(program_id) => system_program::check_id(program_id),
+                message.account_keys.get(instruction.program_id_index as usize),
-                _ => false,
+                Some(program_id) if system_program::check_id(program_id)
-            }
+            )
-        } && matches!(limited_deserialize(&maybe_ix.data), Ok(SystemInstruction::AdvanceNonceAccount))
+            // Is a nonce advance instruction
-        )
+            && matches!(
+                limited_deserialize(&instruction.data),
+                Ok(SystemInstruction::AdvanceNonceAccount)
+            )
+            // Nonce account is writable
+            && matches!(
+                instruction.accounts.get(0),
+                Some(index) if message.is_writable(*index as usize, true)
+            )
+        })
 }
 
 #[deprecated]
@@ -532,7 +541,7 @@ mod tests {
         hash::hash,
         instruction::AccountMeta,
         signature::{Keypair, Presigner, Signer},
-        system_instruction,
+        system_instruction, sysvar,
     };
     use bincode::{deserialize, serialize, serialized_size};
     use std::mem::size_of;
@@ -993,6 +1002,32 @@ mod tests {
         assert!(uses_durable_nonce(&tx).is_none());
     }
 
+    #[test]
+    fn tx_uses_ro_nonce_account() {
+        let from_keypair = Keypair::new();
+        let from_pubkey = from_keypair.pubkey();
+        let nonce_keypair = Keypair::new();
+        let nonce_pubkey = nonce_keypair.pubkey();
+        let account_metas = vec![
+            AccountMeta::new_readonly(nonce_pubkey, false),
+            #[allow(deprecated)]
+            AccountMeta::new_readonly(sysvar::recent_blockhashes::id(), false),
+            AccountMeta::new_readonly(nonce_pubkey, true),
+        ];
+        let nonce_instruction = Instruction::new_with_bincode(
+            system_program::id(),
+            &system_instruction::SystemInstruction::AdvanceNonceAccount,
+            account_metas,
+        );
+        let tx = Transaction::new_signed_with_payer(
+            &[nonce_instruction],
+            Some(&from_pubkey),
+            &[&from_keypair, &nonce_keypair],
+            Hash::default(),
+        );
+        assert!(uses_durable_nonce(&tx).is_none());
+    }
+
     #[test]
     fn tx_uses_nonce_wrong_first_nonce_ix_fail() {
         let from_keypair = Keypair::new();

sdk/src/transaction/sanitized.rs

@@ -161,7 +161,7 @@ impl SanitizedTransaction {
     }
 
     /// If the transaction uses a durable nonce, return the pubkey of the nonce account
-    pub fn get_durable_nonce(&self) -> Option<&Pubkey> {
+    pub fn get_durable_nonce(&self, nonce_must_be_writable: bool) -> Option<&Pubkey> {
         self.message
             .instructions()
             .get(NONCED_TX_MARKER_IX_INDEX as usize)
@@ -180,7 +180,11 @@ impl SanitizedTransaction {
             .and_then(|ix| {
                 ix.accounts.get(0).and_then(|idx| {
                     let idx = *idx as usize;
-                    self.message.get_account_key(idx)
+                    if nonce_must_be_writable && !self.message.is_writable(idx, true) {
+                        None
+                    } else {
+                        self.message.get_account_key(idx)
+                    }
                 })
             })
     }