diff --git a/runtime/src/accounts.rs b/runtime/src/accounts.rs index 62297e016c..6705ad6cec 100644 --- a/runtime/src/accounts.rs +++ b/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(); - let account = nonce.account(); - 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 + if leave_nonce_on_success { + None } else { - false - }; - 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, - )) + Some((nonce, false /* rollback */)) + } } - (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,49 +1105,39 @@ 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) - && (execution_result.is_ok() - || (maybe_nonce.is_some() && (is_nonce_account || is_fee_payer))) - { - if execution_result.is_err() { - match (is_nonce_account, is_fee_payer, maybe_nonce) { - // nonce is fee-payer, state updated in `prepare_if_nonce_account()` - (true, true, Some((_, _, None, _))) => (), - // nonce not fee-payer, state updated in `prepare_if_nonce_account()` - (true, false, Some((_, _, Some(_), _))) => (), - // not nonce, but fee-payer. rollback to cached state - (false, true, Some((_, _, Some(fee_payer_account), _))) => { - *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, - account, - rent_for_sysvars, - ); - loaded_transaction.rent += rent; - loaded_transaction - .rent_debits - .insert(address, rent, account.lamports()); - } + if message.is_writable(i, demote_program_write_locks) { + let is_nonce_account = prepare_if_nonce_account( + address, + account, + execution_result, + is_fee_payer, + maybe_nonce, + blockhash, + lamports_per_signature, + ); - // Add to the accounts to store - accounts.push((&*address, &*account)); + if execution_result.is_ok() || is_nonce_account || is_fee_payer { + if account.rent_epoch() == INITIAL_RENT_EPOCH { + let rent = rent_collector.collect_from_created_account( + address, + account, + rent_for_sysvars, + ); + loaded_transaction.rent += rent; + loaded_transaction.rent_debits.insert( + address, + rent, + account.lamports(), + ); + } + + // Add to the accounts to store + accounts.push((&*address, &*account)); + } } } } @@ -1168,52 +1145,59 @@ impl 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<( - &Pubkey, - &AccountSharedData, - Option<&AccountSharedData>, - bool, - )>, + is_fee_payer: bool, + maybe_nonce: Option<(&'a NonceFull, bool)>, blockhash: &Hash, lamports_per_signature: u64, ) -> bool { - if let Some((nonce_key, nonce_account, _, advance_blockhash)) = maybe_nonce { - if address == nonce_key { - if execution_result.is_err() { + if let Some((nonce, rollback)) = maybe_nonce { + if address == nonce.address() { + if rollback { // The transaction failed which would normally drop the account - // processing changes, since this account is now being included in - // the accounts written back to the db, roll it back to + // processing changes, since this account is now being included + // 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 - let state = StateMut::::state(nonce_account) - .unwrap() - .convert_to_current(); - if let NonceState::Initialized(ref data) = state { - account - .set_state(&NonceVersions::new_current(NonceState::new_initialized( - &data.authority, - blockhash, - lamports_per_signature, - ))) - .unwrap(); + // 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 + let state = StateMut::::state(nonce.account()) + .unwrap() + .convert_to_current(); + if let NonceState::Initialized(ref data) = state { + account + .set_state(&NonceVersions::new_current(NonceState::new_initialized( + &data.authority, + blockhash, + lamports_per_signature, + ))) + .unwrap(); + } + 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(); } } - return true; + + false } + } else { + false } - false } pub fn create_test_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<( - &Pubkey, - &AccountSharedData, - Option<&AccountSharedData>, - bool, - )>, + is_fee_payer: bool, + maybe_nonce: Option<(&NonceFull, bool)>, blockhash: &Hash, lamports_per_signature: u64, expect_account: &AccountSharedData, ) -> bool { // Verify expect_account's relationship - match maybe_nonce { - Some((nonce_address, _nonce_account, _maybe_fee_payer_account, _)) - if nonce_address == account_address && tx_result.is_ok() => - { - assert_eq!(expect_account, account) // Account update occurs in system_instruction_processor + if !is_fee_payer { + match maybe_nonce { + Some((nonce, _)) if nonce.address() == account_address => { + assert_ne!(expect_account, nonce.account()) + } + _ => assert_eq!(expect_account, account), } - 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 data = - NonceVersions::new_current(NonceState::Initialized(nonce::state::Data::default())); - expect_account.set_state(&data).unwrap(); + let mut expect_account = pre_account; + expect_account + .set_state(&NonceVersions::new_current(NonceState::Initialized( + 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, - )), + 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(( - &pre_account_address, - &pre_account, - maybe_fee_payer_account.as_ref(), - true, - )), + false, + Some((&nonce, 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(( - &pre_account_address, - &pre_account, - maybe_fee_payer_account.as_ref(), - true, - )), + false, + Some((&nonce, 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 diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index ebb3e3e8c0..e0d19f3267 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -669,17 +669,13 @@ impl NonceFull { let nonce_address = *partial.address(); if *fee_payer_address == nonce_address { - Ok(Self { - address: nonce_address, - account: fee_payer_account, - fee_payer_account: None, - }) + Ok(Self::new(nonce_address, fee_payer_account, None)) } else { - Ok(Self { - address: nonce_address, - account: partial.account().clone(), - fee_payer_account: Some(fee_payer_account), - }) + Ok(Self::new( + nonce_address, + partial.account().clone(), + Some(fee_payer_account), + )) } } else { Err(TransactionError::AccountNotFound) @@ -3935,15 +3931,14 @@ impl Bank { inner_instructions.push(None); } - let nonce = - if let Err(TransactionError::InstructionError(_, _)) = &process_result { + let nonce = match &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() - } else if process_result.is_err() { - None - } else { - nonce.clone() - }; + nonce.clone() // May need to advance the nonce + } + _ => None, + }; (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() ); diff --git a/sdk/src/feature_set.rs b/sdk/src/feature_set.rs index ab0cefd3c6..a228f4c229 100644 --- a/sdk/src/feature_set.rs +++ b/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 = [ @@ -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()