diff --git a/rpc/src/rpc.rs b/rpc/src/rpc.rs index 391a4ca5c3..ce78c4ea04 100644 --- a/rpc/src/rpc.rs +++ b/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. diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 5109de86f5..c3a7a91561 100644 --- a/runtime/src/bank.rs +++ b/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); diff --git a/runtime/src/nonce_keyed_account.rs b/runtime/src/nonce_keyed_account.rs index fa29ade3fe..7f03fc0eb2 100644 --- a/runtime/src/nonce_keyed_account.rs +++ b/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::::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::::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::::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::::state(self)?.convert_to_current() { State::Initialized(data) => { if !signers.contains(&data.authority) { diff --git a/runtime/src/system_instruction_processor.rs b/runtime/src/system_instruction_processor.rs index 75e4697228..a0cc9dc594 100644 --- a/runtime/src/system_instruction_processor.rs +++ b/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(), - &Pubkey::default() + &none_address, + &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(), - &Pubkey::default(), - &Pubkey::default(), + &nonce_address, + &Pubkey::new_unique(), + &nonce_address, 1, )), Err(InstructionError::InvalidAccountData), @@ -1684,8 +1687,8 @@ mod tests { &[ ( true, - false, - Pubkey::default(), + true, + 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, - Pubkey::default(), + true, + 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(), - &[(true, false, Pubkey::default(), nonce_account)], + &serialize(&SystemInstruction::AuthorizeNonceAccount(nonce_address)).unwrap(), + &[(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(), - &Pubkey::default(), - &Pubkey::default(), + &nonce_address, + &Pubkey::new_unique(), + &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!( diff --git a/sdk/src/feature_set.rs b/sdk/src/feature_set.rs index 4edceec4d9..ed4ecdeb4a 100644 --- a/sdk/src/feature_set.rs +++ b/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 = [ @@ -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() diff --git a/sdk/src/transaction/mod.rs b/sdk/src/transaction/mod.rs index 117179f351..5d910569e2 100644 --- a/sdk/src/transaction/mod.rs +++ b/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| { - let prog_id_idx = maybe_ix.program_id_index as usize; - match message.account_keys.get(prog_id_idx) { - Some(program_id) => system_program::check_id(program_id), - _ => false, - } - } && matches!(limited_deserialize(&maybe_ix.data), Ok(SystemInstruction::AdvanceNonceAccount)) - ) + .filter(|instruction| { + // Is system program + matches!( + message.account_keys.get(instruction.program_id_index as usize), + Some(program_id) if system_program::check_id(program_id) + ) + // 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(); diff --git a/sdk/src/transaction/sanitized.rs b/sdk/src/transaction/sanitized.rs index bbc4e61e47..2768e665da 100644 --- a/sdk/src/transaction/sanitized.rs +++ b/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) + } }) }) }