From 21bc43ed58c63c827ba4db30426965ef3e807180 Mon Sep 17 00:00:00 2001 From: Trent Nelson Date: Tue, 1 Jun 2021 17:25:53 -0600 Subject: [PATCH] nonce: Unify `NonceError` with `SystemError` --- cli/src/cli.rs | 58 ++++--- cli/src/feature.rs | 32 +++- cli/src/nonce.rs | 91 ++++++++++- client/src/client_error.rs | 24 ++- runtime/src/accounts.rs | 62 ++++++-- runtime/src/bank.rs | 60 ++++++++ sdk/program/src/nonce/mod.rs | 2 + sdk/program/src/system_instruction.rs | 209 +++++++++++++++++++++++++- sdk/src/feature_set.rs | 5 + sdk/src/nonce_keyed_account.rs | 49 ++++-- sdk/src/transaction.rs | 3 +- 11 files changed, 521 insertions(+), 74 deletions(-) diff --git a/cli/src/cli.rs b/cli/src/cli.rs index e081ea4d9..8a0b58928 100644 --- a/cli/src/cli.rs +++ b/cli/src/cli.rs @@ -12,12 +12,10 @@ use solana_cli_output::{ }; use solana_client::{ blockhash_query::BlockhashQuery, - client_error::{ClientError, ClientErrorKind, Result as ClientResult}, + client_error::{ClientError, Result as ClientResult}, nonce_utils, rpc_client::RpcClient, rpc_config::{RpcLargestAccountsFilter, RpcSendTransactionConfig, RpcTransactionLogsFilter}, - rpc_request::{RpcError, RpcResponseErrorData}, - rpc_response::RpcSimulateTransactionResult, }; use solana_remote_wallet::remote_wallet::RemoteWalletManager; use solana_sdk::{ @@ -1536,43 +1534,41 @@ pub fn request_and_confirm_airdrop( Ok(signature) } +fn common_error_adapter(ix_error: &InstructionError) -> Option +where + E: 'static + std::error::Error + DecodeError + FromPrimitive, +{ + if let InstructionError::Custom(code) = ix_error { + E::decode_custom_error_to_enum(*code) + } else { + None + } +} + pub fn log_instruction_custom_error( result: ClientResult, config: &CliConfig, ) -> ProcessResult where E: 'static + std::error::Error + DecodeError + FromPrimitive, +{ + log_instruction_custom_error_ex::(result, config, common_error_adapter) +} + +pub fn log_instruction_custom_error_ex( + result: ClientResult, + config: &CliConfig, + error_adapter: F, +) -> ProcessResult +where + E: 'static + std::error::Error + DecodeError + FromPrimitive, + F: Fn(&InstructionError) -> Option, { match result { Err(err) => { - // If transaction simulation returns a known Custom InstructionError, decode it - if let ClientErrorKind::RpcError(RpcError::RpcResponseError { - data: - RpcResponseErrorData::SendTransactionPreflightFailure( - RpcSimulateTransactionResult { - err: - Some(TransactionError::InstructionError( - _, - InstructionError::Custom(code), - )), - .. - }, - ), - .. - }) = err.kind() - { - if let Some(specific_error) = E::decode_custom_error_to_enum(*code) { - return Err(specific_error.into()); - } - } - // If the transaction was instead submitted and returned a known Custom - // InstructionError, decode it - if let ClientErrorKind::TransactionError(TransactionError::InstructionError( - _, - InstructionError::Custom(code), - )) = err.kind() - { - if let Some(specific_error) = E::decode_custom_error_to_enum(*code) { + let maybe_tx_err = err.get_transaction_error(); + if let Some(TransactionError::InstructionError(_, ix_error)) = maybe_tx_err { + if let Some(specific_error) = error_adapter(&ix_error) { return Err(specific_error.into()); } } diff --git a/cli/src/feature.rs b/cli/src/feature.rs index 86d6a24dc..5e64b338d 100644 --- a/cli/src/feature.rs +++ b/cli/src/feature.rs @@ -10,6 +10,7 @@ use solana_cli_output::{QuietDisplay, VerboseDisplay}; use solana_client::{client_error::ClientError, rpc_client::RpcClient}; use solana_remote_wallet::remote_wallet::RemoteWalletManager; use solana_sdk::{ + account::Account, clock::Slot, feature::{self, Feature}, feature_set::FEATURE_NAMES, @@ -312,6 +313,31 @@ fn feature_activation_allowed(rpc_client: &RpcClient, quiet: bool) -> Result Option { + feature::from_account(&account).map(|feature| match feature.activated_at { + None => CliFeatureStatus::Pending, + Some(activation_slot) => CliFeatureStatus::Active(activation_slot), + }) +} + +fn get_feature_status( + rpc_client: &RpcClient, + feature_id: &Pubkey, +) -> Result, Box> { + rpc_client + .get_account(feature_id) + .map(status_from_account) + .map_err(|e| e.into()) +} + +pub fn get_feature_is_active( + rpc_client: &RpcClient, + feature_id: &Pubkey, +) -> Result> { + get_feature_status(rpc_client, feature_id) + .map(|status| matches!(status, Some(CliFeatureStatus::Active(_)))) +} + fn process_status( rpc_client: &RpcClient, config: &CliConfig, @@ -327,11 +353,7 @@ fn process_status( let feature_id = &feature_ids[i]; let feature_name = FEATURE_NAMES.get(feature_id).unwrap(); if let Some(account) = account { - if let Some(feature) = feature::from_account(&account) { - let feature_status = match feature.activated_at { - None => CliFeatureStatus::Pending, - Some(activation_slot) => CliFeatureStatus::Active(activation_slot), - }; + if let Some(feature_status) = status_from_account(account) { features.push(CliFeature { id: feature_id.to_string(), description: feature_name.to_string(), diff --git a/cli/src/nonce.rs b/cli/src/nonce.rs index 7c581260b..90e53be79 100644 --- a/cli/src/nonce.rs +++ b/cli/src/nonce.rs @@ -1,9 +1,10 @@ use crate::{ checks::{check_account_for_fee_with_commitment, check_unique_pubkeys}, cli::{ - log_instruction_custom_error, CliCommand, CliCommandInfo, CliConfig, CliError, - ProcessResult, + log_instruction_custom_error, log_instruction_custom_error_ex, CliCommand, CliCommandInfo, + CliConfig, CliError, ProcessResult, }, + feature::get_feature_is_active, memo::WithMemo, spend_utils::{resolve_spend_tx_and_check_account_balance, SpendAmount}, }; @@ -20,16 +21,19 @@ use solana_client::{nonce_utils::*, rpc_client::RpcClient}; use solana_remote_wallet::remote_wallet::RemoteWalletManager; use solana_sdk::{ account::Account, + feature_set::merge_nonce_error_into_system_error, hash::Hash, + instruction::InstructionError, message::Message, nonce::{self, State}, pubkey::Pubkey, system_instruction::{ advance_nonce_account, authorize_nonce_account, create_nonce_account, - create_nonce_account_with_seed, withdraw_nonce_account, NonceError, SystemError, + create_nonce_account_with_seed, instruction_to_nonce_error, withdraw_nonce_account, + NonceError, SystemError, }, system_program, - transaction::Transaction, + transaction::{Transaction, TransactionError}, }; use std::sync::Arc; @@ -367,8 +371,21 @@ pub fn process_authorize_nonce_account( &tx.message, config.commitment, )?; + let merge_errors = + get_feature_is_active(rpc_client, &merge_nonce_error_into_system_error::id())?; let result = rpc_client.send_and_confirm_transaction_with_spinner(&tx); - log_instruction_custom_error::(result, config) + + if merge_errors { + log_instruction_custom_error::(result, config) + } else { + log_instruction_custom_error_ex::(result, config, |ix_error| { + if let InstructionError::Custom(_) = ix_error { + instruction_to_nonce_error(ix_error, merge_errors) + } else { + None + } + }) + } } pub fn process_create_nonce_account( @@ -452,8 +469,40 @@ pub fn process_create_nonce_account( let mut tx = Transaction::new_unsigned(message); tx.try_sign(&config.signers, recent_blockhash)?; + let merge_errors = + get_feature_is_active(rpc_client, &merge_nonce_error_into_system_error::id())?; let result = rpc_client.send_and_confirm_transaction_with_spinner(&tx); - log_instruction_custom_error::(result, config) + + let err_ix_index = if let Err(err) = &result { + err.get_transaction_error().and_then(|tx_err| { + if let TransactionError::InstructionError(ix_index, _) = tx_err { + Some(ix_index) + } else { + None + } + }) + } else { + None + }; + + match err_ix_index { + // SystemInstruction::InitializeNonceAccount failed + Some(1) => { + if merge_errors { + log_instruction_custom_error::(result, config) + } else { + log_instruction_custom_error_ex::(result, config, |ix_error| { + if let InstructionError::Custom(_) = ix_error { + instruction_to_nonce_error(ix_error, merge_errors) + } else { + None + } + }) + } + } + // SystemInstruction::CreateAccount{,WithSeed} failed + _ => log_instruction_custom_error::(result, config), + } } pub fn process_get_nonce( @@ -506,8 +555,21 @@ pub fn process_new_nonce( &tx.message, config.commitment, )?; + let merge_errors = + get_feature_is_active(rpc_client, &merge_nonce_error_into_system_error::id())?; let result = rpc_client.send_and_confirm_transaction_with_spinner(&tx); - log_instruction_custom_error::(result, config) + + if merge_errors { + log_instruction_custom_error::(result, config) + } else { + log_instruction_custom_error_ex::(result, config, |ix_error| { + if let InstructionError::Custom(_) = ix_error { + instruction_to_nonce_error(ix_error, merge_errors) + } else { + None + } + }) + } } pub fn process_show_nonce_account( @@ -569,8 +631,21 @@ pub fn process_withdraw_from_nonce_account( &tx.message, config.commitment, )?; + let merge_errors = + get_feature_is_active(rpc_client, &merge_nonce_error_into_system_error::id())?; let result = rpc_client.send_and_confirm_transaction_with_spinner(&tx); - log_instruction_custom_error::(result, config) + + if merge_errors { + log_instruction_custom_error::(result, config) + } else { + log_instruction_custom_error_ex::(result, config, |ix_error| { + if let InstructionError::Custom(_) = ix_error { + instruction_to_nonce_error(ix_error, merge_errors) + } else { + None + } + }) + } } #[cfg(test)] diff --git a/client/src/client_error.rs b/client/src/client_error.rs index fec5d047c..365d70941 100644 --- a/client/src/client_error.rs +++ b/client/src/client_error.rs @@ -1,5 +1,5 @@ use { - crate::rpc_request, + crate::{rpc_request, rpc_response}, solana_faucet::faucet::FaucetError, solana_sdk::{ signature::SignerError, transaction::TransactionError, transport::TransportError, @@ -30,6 +30,24 @@ pub enum ClientErrorKind { Custom(String), } +impl ClientErrorKind { + pub fn get_transaction_error(&self) -> Option { + match self { + Self::RpcError(rpc_request::RpcError::RpcResponseError { + data: + rpc_request::RpcResponseErrorData::SendTransactionPreflightFailure( + rpc_response::RpcSimulateTransactionResult { + err: Some(tx_err), .. + }, + ), + .. + }) => Some(tx_err.clone()), + Self::TransactionError(tx_err) => Some(tx_err.clone()), + _ => None, + } + } +} + impl From for ClientErrorKind { fn from(err: TransportError) -> Self { match err { @@ -86,6 +104,10 @@ impl ClientError { pub fn kind(&self) -> &ClientErrorKind { &self.kind } + + pub fn get_transaction_error(&self) -> Option { + self.kind.get_transaction_error() + } } impl From for ClientError { diff --git a/runtime/src/accounts.rs b/runtime/src/accounts.rs index eb3b473a1..75b0dcdf4 100644 --- a/runtime/src/accounts.rs +++ b/runtime/src/accounts.rs @@ -30,6 +30,7 @@ use solana_sdk::{ hash::Hash, message::Message, native_loader, nonce, + nonce::NONCED_TX_MARKER_IX_INDEX, pubkey::Pubkey, transaction::Result, transaction::{Transaction, TransactionError}, @@ -891,6 +892,7 @@ impl Accounts { rent_collector: &RentCollector, last_blockhash_with_fee_calculator: &(Hash, FeeCalculator), rent_for_sysvars: bool, + merge_nonce_error_into_system_error: bool, ) { let accounts_to_store = self.collect_accounts_to_store( txs, @@ -899,6 +901,7 @@ impl Accounts { rent_collector, last_blockhash_with_fee_calculator, rent_for_sysvars, + merge_nonce_error_into_system_error, ); self.accounts_db.store_cached(slot, &accounts_to_store); } @@ -923,6 +926,7 @@ impl Accounts { rent_collector: &RentCollector, last_blockhash_with_fee_calculator: &(Hash, FeeCalculator), rent_for_sysvars: bool, + merge_nonce_error_into_system_error: bool, ) -> Vec<(&'a Pubkey, &'a AccountSharedData)> { let mut accounts = Vec::with_capacity(loaded.len()); for (i, ((raccs, _nonce_rollback), tx)) in loaded.iter_mut().zip(txs).enumerate() { @@ -935,13 +939,19 @@ impl Accounts { let pubkey = nonce_rollback.nonce_address(); let acc = nonce_rollback.nonce_account(); let maybe_fee_account = nonce_rollback.fee_account(); - Some((pubkey, acc, maybe_fee_account)) + Some((pubkey, acc, maybe_fee_account, true)) } - (Err(TransactionError::InstructionError(_, _)), Some(nonce_rollback)) => { + (Err(TransactionError::InstructionError(index, _)), Some(nonce_rollback)) => { + 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 + }; let pubkey = nonce_rollback.nonce_address(); let acc = nonce_rollback.nonce_account(); let maybe_fee_account = nonce_rollback.fee_account(); - Some((pubkey, acc, maybe_fee_account)) + Some((pubkey, acc, maybe_fee_account, !nonce_marker_ix_failed)) } (Ok(_), _nonce_rollback) => None, (Err(_), _nonce_rollback) => continue, @@ -972,11 +982,11 @@ impl Accounts { if res.is_err() { match (is_nonce_account, is_fee_payer, maybe_nonce_rollback) { // nonce is fee-payer, state updated in `prepare_if_nonce_account()` - (true, true, Some((_, _, None))) => (), + (true, true, Some((_, _, None, _))) => (), // nonce not fee-payer, state updated in `prepare_if_nonce_account()` - (true, false, Some((_, _, Some(_)))) => (), + (true, false, Some((_, _, Some(_), _))) => (), // not nonce, but fee-payer. rollback to cached state - (false, true, Some((_, _, Some(fee_payer_account)))) => { + (false, true, Some((_, _, Some(fee_payer_account), _))) => { *account = fee_payer_account.clone(); } _ => panic!("unexpected nonce_rollback condition"), @@ -1005,15 +1015,28 @@ pub fn prepare_if_nonce_account( account: &mut AccountSharedData, account_pubkey: &Pubkey, tx_result: &Result<()>, - maybe_nonce_rollback: Option<(&Pubkey, &AccountSharedData, Option<&AccountSharedData>)>, + maybe_nonce_rollback: Option<( + &Pubkey, + &AccountSharedData, + Option<&AccountSharedData>, + bool, + )>, last_blockhash_with_fee_calculator: &(Hash, FeeCalculator), ) -> bool { - if let Some((nonce_key, nonce_acc, _maybe_fee_account)) = maybe_nonce_rollback { + if let Some((nonce_key, nonce_acc, _maybe_fee_account, advance_blockhash)) = + maybe_nonce_rollback + { if account_pubkey == nonce_key { if tx_result.is_err() { // Nonce TX failed with an InstructionError. Roll back // its account state *account = nonce_acc.clone(); + } + + if advance_blockhash { + // Advance the stored blockhash to prevent fee theft by replaying + // transactions that have failed with an `InstructionError` + // Since hash_age_kind is DurableNonce, unwrap is safe here let state = StateMut::::state(nonce_acc) .unwrap() @@ -1975,6 +1998,7 @@ mod tests { &rent_collector, &(Hash::default(), FeeCalculator::default()), true, + true, // merge_nonce_error_into_system_error ); assert_eq!(collected_accounts.len(), 2); assert!(collected_accounts @@ -2102,18 +2126,23 @@ mod tests { account: &mut AccountSharedData, account_pubkey: &Pubkey, tx_result: &Result<()>, - maybe_nonce_rollback: Option<(&Pubkey, &AccountSharedData, Option<&AccountSharedData>)>, + maybe_nonce_rollback: Option<( + &Pubkey, + &AccountSharedData, + Option<&AccountSharedData>, + bool, + )>, last_blockhash_with_fee_calculator: &(Hash, FeeCalculator), expect_account: &AccountSharedData, ) -> bool { // Verify expect_account's relationship match maybe_nonce_rollback { - Some((nonce_pubkey, _nonce_account, _maybe_fee_account)) + Some((nonce_pubkey, _nonce_account, _maybe_fee_account, _)) if nonce_pubkey == account_pubkey && tx_result.is_ok() => { assert_eq!(expect_account, account) // Account update occurs in system_instruction_processor } - Some((nonce_pubkey, nonce_account, _maybe_fee_account)) + Some((nonce_pubkey, nonce_account, _maybe_fee_account, _)) if nonce_pubkey == account_pubkey => { assert_ne!(expect_account, nonce_account) @@ -2156,7 +2185,8 @@ mod tests { Some(( &pre_account_pubkey, &pre_account, - maybe_fee_account.as_ref() + maybe_fee_account.as_ref(), + false, )), &(last_blockhash, last_fee_calculator), &expect_account, @@ -2207,7 +2237,8 @@ mod tests { Some(( &pre_account_pubkey, &pre_account, - maybe_fee_account.as_ref() + maybe_fee_account.as_ref(), + true, )), &(last_blockhash, last_fee_calculator), &expect_account, @@ -2247,7 +2278,8 @@ mod tests { Some(( &pre_account_pubkey, &pre_account, - maybe_fee_account.as_ref() + maybe_fee_account.as_ref(), + true, )), &(last_blockhash, last_fee_calculator), &expect_account, @@ -2344,6 +2376,7 @@ mod tests { &rent_collector, &(next_blockhash, FeeCalculator::default()), true, + true, // merge_nonce_error_into_system_error ); assert_eq!(collected_accounts.len(), 2); assert_eq!( @@ -2460,6 +2493,7 @@ mod tests { &rent_collector, &(next_blockhash, FeeCalculator::default()), true, + true, // merge_nonce_error_into_system_error ); assert_eq!(collected_accounts.len(), 1); let collected_nonce_account = collected_accounts diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index f5012911e..8405afce5 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -3534,6 +3534,7 @@ impl Bank { &self.rent_collector, &self.last_blockhash_with_fee_calculator(), self.rent_for_sysvars(), + self.merge_nonce_error_into_system_error(), ); let rent_debits = self.collect_rent(executed, loaded_txs); @@ -5222,6 +5223,11 @@ impl Bank { .is_active(&feature_set::libsecp256k1_0_5_upgrade_enabled::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()) + } + // Check if the wallclock time from bank creation to now has exceeded the allotted // time for transaction processing pub fn should_bank_still_be_processing_txs( @@ -10194,6 +10200,60 @@ pub(crate) mod tests { ); } + #[test] + fn test_nonce_authority() { + solana_logger::setup(); + let (mut bank, _mint_keypair, custodian_keypair, nonce_keypair) = + setup_nonce_with_bank(10_000_000, |_| {}, 5_000_000, 250_000, None).unwrap(); + let alice_keypair = Keypair::new(); + let alice_pubkey = alice_keypair.pubkey(); + let custodian_pubkey = custodian_keypair.pubkey(); + let nonce_pubkey = nonce_keypair.pubkey(); + let bad_nonce_authority_keypair = Keypair::new(); + let bad_nonce_authority = bad_nonce_authority_keypair.pubkey(); + let custodian_account = bank.get_account(&custodian_pubkey).unwrap(); + + debug!("alice: {}", alice_pubkey); + debug!("custodian: {}", custodian_pubkey); + debug!("nonce: {}", nonce_pubkey); + debug!("nonce account: {:?}", bank.get_account(&nonce_pubkey)); + debug!("cust: {:?}", custodian_account); + let nonce_hash = get_nonce_account(&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)); + } + + let durable_tx = Transaction::new_signed_with_payer( + &[ + system_instruction::advance_nonce_account(&nonce_pubkey, &bad_nonce_authority), + system_instruction::transfer(&custodian_pubkey, &alice_pubkey, 42), + ], + Some(&custodian_pubkey), + &[&custodian_keypair, &bad_nonce_authority_keypair], + nonce_hash, + ); + debug!("{:?}", durable_tx); + let initial_custodian_balance = custodian_account.lamports(); + assert_eq!( + bank.process_transaction(&durable_tx), + Err(TransactionError::InstructionError( + 0, + InstructionError::MissingRequiredSignature, + )) + ); + /* Check fee charged and nonce has *not* advanced */ + assert_eq!( + bank.get_balance(&custodian_pubkey), + initial_custodian_balance - 10_000 + ); + assert_eq!(nonce_hash, get_nonce_account(&bank, &nonce_pubkey).unwrap()); + } + #[test] fn test_nonce_payer() { solana_logger::setup(); diff --git a/sdk/program/src/nonce/mod.rs b/sdk/program/src/nonce/mod.rs index 5aa54121a..3247fbe4e 100644 --- a/sdk/program/src/nonce/mod.rs +++ b/sdk/program/src/nonce/mod.rs @@ -1,2 +1,4 @@ pub mod state; pub use state::State; + +pub const NONCED_TX_MARKER_IX_INDEX: u8 = 0; diff --git a/sdk/program/src/system_instruction.rs b/sdk/program/src/system_instruction.rs index 7d16cef6c..8b4021f78 100644 --- a/sdk/program/src/system_instruction.rs +++ b/sdk/program/src/system_instruction.rs @@ -2,7 +2,7 @@ use crate::sysvar::recent_blockhashes; use crate::{ decode_error::DecodeError, - instruction::{AccountMeta, Instruction}, + instruction::{AccountMeta, Instruction, InstructionError}, nonce, pubkey::Pubkey, system_program, @@ -25,6 +25,12 @@ pub enum SystemError { MaxSeedLengthExceeded, #[error("provided address does not match addressed derived from seed")] AddressWithSeedMismatch, + #[error("advancing stored nonce requires a populated RecentBlockhashes sysvar")] + NonceNoRecentBlockhashes, + #[error("stored nonce is still in recent_blockhashes")] + NonceBlockhashNotExpired, + #[error("specified nonce does not match stored nonce")] + NonceUnexpectedBlockhashValue, } impl DecodeError for SystemError { @@ -51,6 +57,83 @@ impl DecodeError for NonceError { } } +#[derive(Error, Debug, Clone, PartialEq, FromPrimitive, ToPrimitive)] +enum NonceErrorAdapter { + #[error("recent blockhash list is empty")] + NoRecentBlockhashes, + #[error("stored nonce is still in recent_blockhashes")] + NotExpired, + #[error("specified nonce does not match stored nonce")] + UnexpectedValue, + #[error("cannot handle request in current account state")] + BadAccountState, +} + +impl DecodeError for NonceErrorAdapter { + fn type_of() -> &'static str { + "NonceErrorAdapter" + } +} + +impl From for NonceError { + fn from(e: NonceErrorAdapter) -> Self { + match e { + NonceErrorAdapter::NoRecentBlockhashes => NonceError::NoRecentBlockhashes, + NonceErrorAdapter::NotExpired => NonceError::NotExpired, + NonceErrorAdapter::UnexpectedValue => NonceError::UnexpectedValue, + NonceErrorAdapter::BadAccountState => NonceError::BadAccountState, + } + } +} + +pub fn nonce_to_instruction_error(error: NonceError, use_system_variant: bool) -> InstructionError { + if use_system_variant { + match error { + NonceError::NoRecentBlockhashes => SystemError::NonceNoRecentBlockhashes.into(), + NonceError::NotExpired => SystemError::NonceBlockhashNotExpired.into(), + NonceError::UnexpectedValue => SystemError::NonceUnexpectedBlockhashValue.into(), + NonceError::BadAccountState => InstructionError::InvalidAccountData, + } + } else { + match error { + NonceError::NoRecentBlockhashes => NonceErrorAdapter::NoRecentBlockhashes.into(), + NonceError::NotExpired => NonceErrorAdapter::NotExpired.into(), + NonceError::UnexpectedValue => NonceErrorAdapter::UnexpectedValue.into(), + NonceError::BadAccountState => NonceErrorAdapter::BadAccountState.into(), + } + } +} + +pub fn instruction_to_nonce_error( + error: &InstructionError, + use_system_variant: bool, +) -> Option { + if use_system_variant { + match error { + InstructionError::Custom(discriminant) => { + match SystemError::decode_custom_error_to_enum(*discriminant) { + Some(SystemError::NonceNoRecentBlockhashes) => { + Some(NonceError::NoRecentBlockhashes) + } + Some(SystemError::NonceBlockhashNotExpired) => Some(NonceError::NotExpired), + Some(SystemError::NonceUnexpectedBlockhashValue) => { + Some(NonceError::UnexpectedValue) + } + _ => None, + } + } + InstructionError::InvalidAccountData => Some(NonceError::BadAccountState), + _ => None, + } + } else if let InstructionError::Custom(discriminant) = error { + let maybe: Option = + NonceErrorAdapter::decode_custom_error_to_enum(*discriminant); + maybe.map(NonceError::from) + } else { + None + } +} + /// maximum permitted size of data: 10 MB pub const MAX_PERMITTED_DATA_LENGTH: u64 = 10 * 1024 * 1024; @@ -492,6 +575,7 @@ pub fn authorize_nonce_account( mod tests { use super::*; use crate::instruction::{Instruction, InstructionError}; + use num_traits::ToPrimitive; fn get_keys(instruction: &Instruction) -> Vec { instruction.accounts.iter().map(|x| x.pubkey).collect() @@ -561,4 +645,127 @@ mod tests { pretty_err::(NonceError::BadAccountState.into()) ); } + + #[test] + fn test_nonce_to_instruction_error() { + assert_eq!( + nonce_to_instruction_error(NonceError::NoRecentBlockhashes, false), + NonceError::NoRecentBlockhashes.into(), + ); + assert_eq!( + nonce_to_instruction_error(NonceError::NotExpired, false), + NonceError::NotExpired.into(), + ); + assert_eq!( + nonce_to_instruction_error(NonceError::UnexpectedValue, false), + NonceError::UnexpectedValue.into(), + ); + assert_eq!( + nonce_to_instruction_error(NonceError::BadAccountState, false), + NonceError::BadAccountState.into(), + ); + assert_eq!( + nonce_to_instruction_error(NonceError::NoRecentBlockhashes, true), + SystemError::NonceNoRecentBlockhashes.into(), + ); + assert_eq!( + nonce_to_instruction_error(NonceError::NotExpired, true), + SystemError::NonceBlockhashNotExpired.into(), + ); + assert_eq!( + nonce_to_instruction_error(NonceError::UnexpectedValue, true), + SystemError::NonceUnexpectedBlockhashValue.into(), + ); + assert_eq!( + nonce_to_instruction_error(NonceError::BadAccountState, true), + InstructionError::InvalidAccountData, + ); + } + + #[test] + fn test_instruction_to_nonce_error() { + assert_eq!( + instruction_to_nonce_error( + &InstructionError::Custom(NonceErrorAdapter::NoRecentBlockhashes.to_u32().unwrap(),), + false, + ), + Some(NonceError::NoRecentBlockhashes), + ); + assert_eq!( + instruction_to_nonce_error( + &InstructionError::Custom(NonceErrorAdapter::NotExpired.to_u32().unwrap(),), + false, + ), + Some(NonceError::NotExpired), + ); + assert_eq!( + instruction_to_nonce_error( + &InstructionError::Custom(NonceErrorAdapter::UnexpectedValue.to_u32().unwrap(),), + false, + ), + Some(NonceError::UnexpectedValue), + ); + assert_eq!( + instruction_to_nonce_error( + &InstructionError::Custom(NonceErrorAdapter::BadAccountState.to_u32().unwrap(),), + false, + ), + Some(NonceError::BadAccountState), + ); + assert_eq!( + instruction_to_nonce_error(&InstructionError::Custom(u32::MAX), false), + None, + ); + assert_eq!( + instruction_to_nonce_error( + &InstructionError::Custom(SystemError::NonceNoRecentBlockhashes.to_u32().unwrap(),), + true, + ), + Some(NonceError::NoRecentBlockhashes), + ); + assert_eq!( + instruction_to_nonce_error( + &InstructionError::Custom(SystemError::NonceBlockhashNotExpired.to_u32().unwrap(),), + true, + ), + Some(NonceError::NotExpired), + ); + assert_eq!( + instruction_to_nonce_error( + &InstructionError::Custom( + SystemError::NonceUnexpectedBlockhashValue.to_u32().unwrap(), + ), + true, + ), + Some(NonceError::UnexpectedValue), + ); + assert_eq!( + instruction_to_nonce_error(&InstructionError::InvalidAccountData, true), + Some(NonceError::BadAccountState), + ); + assert_eq!( + instruction_to_nonce_error(&InstructionError::Custom(u32::MAX), true), + None, + ); + } + + #[test] + fn test_nonce_error_adapter_compat() { + assert_eq!( + NonceError::NoRecentBlockhashes.to_u32(), + NonceErrorAdapter::NoRecentBlockhashes.to_u32(), + ); + assert_eq!( + NonceError::NotExpired.to_u32(), + NonceErrorAdapter::NotExpired.to_u32(), + ); + assert_eq!( + NonceError::UnexpectedValue.to_u32(), + NonceErrorAdapter::UnexpectedValue.to_u32(), + ); + assert_eq!( + NonceError::BadAccountState.to_u32(), + NonceErrorAdapter::BadAccountState.to_u32(), + ); + } } diff --git a/sdk/src/feature_set.rs b/sdk/src/feature_set.rs index f1597e026..7717bb230 100644 --- a/sdk/src/feature_set.rs +++ b/sdk/src/feature_set.rs @@ -175,6 +175,10 @@ pub mod spl_token_v2_set_authority_fix { solana_sdk::declare_id!("FToKNBYyiF4ky9s8WsmLBXHCht17Ek7RXaLZGHzzQhJ1"); } +pub mod merge_nonce_error_into_system_error { + solana_sdk::declare_id!("21AWDosvp3pBamFW91KB35pNoaoZVTM7ess8nr2nt53B"); +} + lazy_static! { /// Map of feature identifiers to user-visible description pub static ref FEATURE_NAMES: HashMap = [ @@ -213,6 +217,7 @@ lazy_static! { (libsecp256k1_0_5_upgrade_enabled::id(), "upgrade libsecp256k1 to v0.5.0"), (tx_wide_compute_cap::id(), "Transaction wide compute cap"), (spl_token_v2_set_authority_fix::id(), "spl-token set_authority fix"), + (merge_nonce_error_into_system_error::id(), "merge NonceError into SystemError"), /*************** ADD NEW FEATURES HERE ***************/ ] .iter() diff --git a/sdk/src/nonce_keyed_account.rs b/sdk/src/nonce_keyed_account.rs index f2ddc1d78..a998e0f12 100644 --- a/sdk/src/nonce_keyed_account.rs +++ b/sdk/src/nonce_keyed_account.rs @@ -3,7 +3,7 @@ use crate::{ account::{ReadableAccount, WritableAccount}, account_utils::State as AccountUtilsState, - ic_msg, + feature_set, ic_msg, keyed_account::KeyedAccount, nonce_account::create_account, process_instruction::InvokeContext, @@ -12,7 +12,7 @@ use solana_program::{ instruction::{checked_add, InstructionError}, nonce::{self, state::Versions, State}, pubkey::Pubkey, - system_instruction::NonceError, + system_instruction::{nonce_to_instruction_error, NonceError}, sysvar::rent::Rent, }; use std::collections::HashSet; @@ -51,6 +51,8 @@ impl<'a> NonceKeyedAccount for KeyedAccount<'a> { signers: &HashSet, invoke_context: &dyn InvokeContext, ) -> Result<(), InstructionError> { + let merge_nonce_error_into_system_error = invoke_context + .is_feature_active(&feature_set::merge_nonce_error_into_system_error::id()); let state = AccountUtilsState::::state(self)?.convert_to_current(); match state { State::Initialized(data) => { @@ -68,7 +70,10 @@ impl<'a> NonceKeyedAccount for KeyedAccount<'a> { invoke_context, "Advance nonce account: nonce can only advance once per slot" ); - return Err(NonceError::NotExpired.into()); + return Err(nonce_to_instruction_error( + NonceError::NotExpired, + merge_nonce_error_into_system_error, + )); } let new_data = nonce::state::Data { @@ -84,7 +89,10 @@ impl<'a> NonceKeyedAccount for KeyedAccount<'a> { "Advance nonce account: Account {} state is invalid", self.unsigned_key() ); - Err(NonceError::BadAccountState.into()) + Err(nonce_to_instruction_error( + NonceError::BadAccountState, + merge_nonce_error_into_system_error, + )) } } } @@ -97,6 +105,8 @@ impl<'a> NonceKeyedAccount for KeyedAccount<'a> { signers: &HashSet, invoke_context: &dyn InvokeContext, ) -> Result<(), InstructionError> { + let merge_nonce_error_into_system_error = invoke_context + .is_feature_active(&feature_set::merge_nonce_error_into_system_error::id()); let signer = match AccountUtilsState::::state(self)?.convert_to_current() { State::Uninitialized => { if lamports > self.lamports()? { @@ -117,7 +127,10 @@ impl<'a> NonceKeyedAccount for KeyedAccount<'a> { invoke_context, "Withdraw nonce account: nonce can only advance once per slot" ); - return Err(NonceError::NotExpired.into()); + return Err(nonce_to_instruction_error( + NonceError::NotExpired, + merge_nonce_error_into_system_error, + )); } self.set_state(&Versions::new_current(State::Uninitialized))?; } else { @@ -168,6 +181,8 @@ impl<'a> NonceKeyedAccount for KeyedAccount<'a> { rent: &Rent, invoke_context: &dyn InvokeContext, ) -> Result<(), InstructionError> { + let merge_nonce_error_into_system_error = invoke_context + .is_feature_active(&feature_set::merge_nonce_error_into_system_error::id()); match AccountUtilsState::::state(self)?.convert_to_current() { State::Uninitialized => { let min_balance = rent.minimum_balance(self.data_len()?); @@ -193,7 +208,10 @@ impl<'a> NonceKeyedAccount for KeyedAccount<'a> { "Initialize nonce account: Account {} state is invalid", self.unsigned_key() ); - Err(NonceError::BadAccountState.into()) + Err(nonce_to_instruction_error( + NonceError::BadAccountState, + merge_nonce_error_into_system_error, + )) } } } @@ -204,6 +222,8 @@ impl<'a> NonceKeyedAccount for KeyedAccount<'a> { signers: &HashSet, invoke_context: &dyn InvokeContext, ) -> Result<(), InstructionError> { + let merge_nonce_error_into_system_error = invoke_context + .is_feature_active(&feature_set::merge_nonce_error_into_system_error::id()); match AccountUtilsState::::state(self)?.convert_to_current() { State::Initialized(data) => { if !signers.contains(&data.authority) { @@ -226,7 +246,10 @@ impl<'a> NonceKeyedAccount for KeyedAccount<'a> { "Authorize nonce account: Account {} state is invalid", self.unsigned_key() ); - Err(NonceError::BadAccountState.into()) + Err(nonce_to_instruction_error( + NonceError::BadAccountState, + merge_nonce_error_into_system_error, + )) } } } @@ -254,7 +277,7 @@ mod test { nonce::{self, State}, nonce_account::verify_nonce_account, process_instruction::MockInvokeContext, - system_instruction::NonceError, + system_instruction::SystemError, }; use solana_program::hash::{hash, Hash}; @@ -419,7 +442,7 @@ mod test { .initialize_nonce_account(&authorized, &rent, &invoke_context) .unwrap(); let result = keyed_account.advance_nonce_account(&signers, &invoke_context); - assert_eq!(result, Err(NonceError::NotExpired.into())); + assert_eq!(result, Err(SystemError::NonceBlockhashNotExpired.into())); }) } @@ -435,7 +458,7 @@ mod test { signers.insert(*keyed_account.signer_key().unwrap()); let invoke_context = create_invoke_context_with_blockhash(63); let result = keyed_account.advance_nonce_account(&signers, &invoke_context); - assert_eq!(result, Err(NonceError::BadAccountState.into())); + assert_eq!(result, Err(InstructionError::InvalidAccountData)); }) } @@ -752,7 +775,7 @@ mod test { &signers, &invoke_context, ); - assert_eq!(result, Err(NonceError::NotExpired.into())); + assert_eq!(result, Err(SystemError::NonceBlockhashNotExpired.into())); }) }) } @@ -893,7 +916,7 @@ mod test { let invoke_context = create_invoke_context_with_blockhash(0); let result = keyed_account.initialize_nonce_account(&authorized, &rent, &invoke_context); - assert_eq!(result, Err(NonceError::BadAccountState.into())); + assert_eq!(result, Err(InstructionError::InvalidAccountData)); }) } @@ -963,7 +986,7 @@ mod test { &signers, &invoke_context, ); - assert_eq!(result, Err(NonceError::BadAccountState.into())); + assert_eq!(result, Err(InstructionError::InvalidAccountData)); }) } diff --git a/sdk/src/transaction.rs b/sdk/src/transaction.rs index 48ba248f8..7a2ad1ee5 100644 --- a/sdk/src/transaction.rs +++ b/sdk/src/transaction.rs @@ -8,6 +8,7 @@ use crate::{ hash::Hash, instruction::{CompiledInstruction, Instruction, InstructionError}, message::Message, + nonce::NONCED_TX_MARKER_IX_INDEX, program_utils::limited_deserialize, pubkey::Pubkey, short_vec, @@ -464,7 +465,7 @@ pub fn uses_durable_nonce(tx: &Transaction) -> Option<&CompiledInstruction> { let message = tx.message(); message .instructions - .get(0) + .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) {