From bc90d5fb7dd987bd70be6ddadd64fe563a0b519a Mon Sep 17 00:00:00 2001 From: hana <81144685+2501babe@users.noreply.github.com> Date: Wed, 11 Sep 2024 02:23:09 -0700 Subject: [PATCH] svm: advance nonce for fee-only transactions sooner (#2741) * write integration tests for nonce transactions these pass against master as-written * advance nonce when creating rollback accounts previously rollback accounts carried the fee payer that was to be committed, but an untouched nonce now, the account saver can use the nonce and fee payer from rollback accounts identically, possibly merging them for a fee-paying nonce * fix nonce-related unit tests * remove nonce hack from integration test * support four tx states in svm integration * advance nonce in check transactions * fix tests for new interfaces * remove last blockhash timing and tx result mutator * change copy mut to proper accountshareddata function * adddress feedback * please clippy * fix merge issues * get lamports_per_signature from blockhash * revert niche lps change * fix merge issues again --- core/src/banking_stage/committer.rs | 7 +- core/src/banking_stage/consume_worker.rs | 10 - core/src/banking_stage/consumer.rs | 13 - .../leader_slot_timing_metrics.rs | 3 - runtime/src/account_saver.rs | 100 +--- runtime/src/bank.rs | 22 +- runtime/src/bank/check_transactions.rs | 117 ++++- runtime/src/bank/tests.rs | 4 +- svm/src/rollback_accounts.rs | 8 +- svm/src/transaction_processing_result.rs | 8 - svm/src/transaction_processor.rs | 9 +- svm/tests/integration_test.rs | 474 +++++++++++++++--- svm/tests/mock_bank.rs | 38 +- 13 files changed, 584 insertions(+), 229 deletions(-) diff --git a/core/src/banking_stage/committer.rs b/core/src/banking_stage/committer.rs index 4c5bf8bbf5382b..ff27104251759e 100644 --- a/core/src/banking_stage/committer.rs +++ b/core/src/banking_stage/committer.rs @@ -12,7 +12,7 @@ use { transaction_batch::TransactionBatch, vote_sender_types::ReplayVoteSender, }, - solana_sdk::{hash::Hash, pubkey::Pubkey, saturating_add_assign}, + solana_sdk::{pubkey::Pubkey, saturating_add_assign}, solana_svm::{ transaction_commit_result::{TransactionCommitResult, TransactionCommitResultExtensions}, transaction_processing_result::{ @@ -65,13 +65,10 @@ impl Committer { self.transaction_status_sender.is_some() } - #[allow(clippy::too_many_arguments)] pub(super) fn commit_transactions( &self, batch: &TransactionBatch, processing_results: Vec, - last_blockhash: Hash, - lamports_per_signature: u64, starting_transaction_index: Option, bank: &Arc, pre_balance_info: &mut PreBalanceInfo, @@ -87,8 +84,6 @@ impl Committer { let (commit_results, commit_time_us) = measure_us!(bank.commit_transactions( batch.sanitized_transactions(), processing_results, - last_blockhash, - lamports_per_signature, processed_counts, &mut execute_and_commit_timings.execute_timings, )); diff --git a/core/src/banking_stage/consume_worker.rs b/core/src/banking_stage/consume_worker.rs index 3902ce8829f163..b676168bb04d4d 100644 --- a/core/src/banking_stage/consume_worker.rs +++ b/core/src/banking_stage/consume_worker.rs @@ -275,7 +275,6 @@ impl ConsumeWorkerMetrics { collect_balances_us, load_execute_us, freeze_lock_us, - last_blockhash_us, record_us, commit_us, find_and_send_votes_us, @@ -291,9 +290,6 @@ impl ConsumeWorkerMetrics { self.timing_metrics .freeze_lock_us .fetch_add(*freeze_lock_us, Ordering::Relaxed); - self.timing_metrics - .last_blockhash_us - .fetch_add(*last_blockhash_us, Ordering::Relaxed); self.timing_metrics .record_us .fetch_add(*record_us, Ordering::Relaxed); @@ -494,7 +490,6 @@ struct ConsumeWorkerTimingMetrics { collect_balances_us: AtomicU64, load_execute_us: AtomicU64, freeze_lock_us: AtomicU64, - last_blockhash_us: AtomicU64, record_us: AtomicU64, commit_us: AtomicU64, find_and_send_votes_us: AtomicU64, @@ -527,11 +522,6 @@ impl ConsumeWorkerTimingMetrics { self.freeze_lock_us.swap(0, Ordering::Relaxed), i64 ), - ( - "last_blockhash_us", - self.last_blockhash_us.swap(0, Ordering::Relaxed), - i64 - ), ("record_us", self.record_us.swap(0, Ordering::Relaxed), i64), ("commit_us", self.commit_us.swap(0, Ordering::Relaxed), i64), ( diff --git a/core/src/banking_stage/consumer.rs b/core/src/banking_stage/consumer.rs index 25942d11c1f914..b8cd383a896634 100644 --- a/core/src/banking_stage/consumer.rs +++ b/core/src/banking_stage/consumer.rs @@ -666,17 +666,6 @@ impl Consumer { let (freeze_lock, freeze_lock_us) = measure_us!(bank.freeze_lock()); execute_and_commit_timings.freeze_lock_us = freeze_lock_us; - // In order to avoid a race condition, leaders must get the last - // blockhash *before* recording transactions because recording - // transactions will only succeed if the block max tick height hasn't - // been reached yet. If they get the last blockhash *after* recording - // transactions, the block max tick height could have already been - // reached and the blockhash queue could have already been updated with - // a new blockhash. - let ((last_blockhash, lamports_per_signature), last_blockhash_us) = - measure_us!(bank.last_blockhash_and_lamports_per_signature()); - execute_and_commit_timings.last_blockhash_us = last_blockhash_us; - let (record_transactions_summary, record_us) = measure_us!(self .transaction_recorder .record_transactions(bank.slot(), processed_transactions)); @@ -713,8 +702,6 @@ impl Consumer { self.committer.commit_transactions( batch, processing_results, - last_blockhash, - lamports_per_signature, starting_transaction_index, bank, &mut pre_balance_info, diff --git a/core/src/banking_stage/leader_slot_timing_metrics.rs b/core/src/banking_stage/leader_slot_timing_metrics.rs index 0de9296ce91aac..31b3dc0a24e7ca 100644 --- a/core/src/banking_stage/leader_slot_timing_metrics.rs +++ b/core/src/banking_stage/leader_slot_timing_metrics.rs @@ -10,7 +10,6 @@ pub struct LeaderExecuteAndCommitTimings { pub collect_balances_us: u64, pub load_execute_us: u64, pub freeze_lock_us: u64, - pub last_blockhash_us: u64, pub record_us: u64, pub commit_us: u64, pub find_and_send_votes_us: u64, @@ -23,7 +22,6 @@ impl LeaderExecuteAndCommitTimings { saturating_add_assign!(self.collect_balances_us, other.collect_balances_us); saturating_add_assign!(self.load_execute_us, other.load_execute_us); saturating_add_assign!(self.freeze_lock_us, other.freeze_lock_us); - saturating_add_assign!(self.last_blockhash_us, other.last_blockhash_us); saturating_add_assign!(self.record_us, other.record_us); saturating_add_assign!(self.commit_us, other.commit_us); saturating_add_assign!(self.find_and_send_votes_us, other.find_and_send_votes_us); @@ -40,7 +38,6 @@ impl LeaderExecuteAndCommitTimings { ("collect_balances_us", self.collect_balances_us as i64, i64), ("load_execute_us", self.load_execute_us as i64, i64), ("freeze_lock_us", self.freeze_lock_us as i64, i64), - ("last_blockhash_us", self.last_blockhash_us as i64, i64), ("record_us", self.record_us as i64, i64), ("commit_us", self.commit_us as i64, i64), ( diff --git a/runtime/src/account_saver.rs b/runtime/src/account_saver.rs index da4188b87f441c..941e4934175af3 100644 --- a/runtime/src/account_saver.rs +++ b/runtime/src/account_saver.rs @@ -1,8 +1,8 @@ use { core::borrow::Borrow, solana_sdk::{ - account::AccountSharedData, nonce::state::DurableNonce, pubkey::Pubkey, - transaction::SanitizedTransaction, transaction_context::TransactionAccount, + account::AccountSharedData, pubkey::Pubkey, transaction::SanitizedTransaction, + transaction_context::TransactionAccount, }, solana_svm::{ rollback_accounts::RollbackAccounts, @@ -51,9 +51,7 @@ fn max_number_of_accounts_to_collect( pub fn collect_accounts_to_store<'a, T: SVMMessage>( txs: &'a [T], txs_refs: &'a Option>>, - processing_results: &'a mut [TransactionProcessingResult], - durable_nonce: &DurableNonce, - lamports_per_signature: u64, + processing_results: &'a [TransactionProcessingResult], ) -> ( Vec<(&'a Pubkey, &'a AccountSharedData)>, Option>, @@ -63,10 +61,9 @@ pub fn collect_accounts_to_store<'a, T: SVMMessage>( let mut transactions = txs_refs .is_some() .then(|| Vec::with_capacity(collect_capacity)); - for (index, (processing_result, transaction)) in - processing_results.iter_mut().zip(txs).enumerate() + for (index, (processing_result, transaction)) in processing_results.iter().zip(txs).enumerate() { - let Some(processed_tx) = processing_result.processed_transaction_mut() else { + let Some(processed_tx) = processing_result.processed_transaction() else { // Don't store any accounts if tx wasn't executed continue; }; @@ -88,9 +85,7 @@ pub fn collect_accounts_to_store<'a, T: SVMMessage>( &mut transactions, transaction, transaction_ref, - &mut executed_tx.loaded_transaction.rollback_accounts, - durable_nonce, - lamports_per_signature, + &executed_tx.loaded_transaction.rollback_accounts, ); } } @@ -100,9 +95,7 @@ pub fn collect_accounts_to_store<'a, T: SVMMessage>( &mut transactions, transaction, transaction_ref, - &mut fees_only_tx.rollback_accounts, - durable_nonce, - lamports_per_signature, + &fees_only_tx.rollback_accounts, ); } } @@ -142,25 +135,18 @@ fn collect_accounts_for_failed_tx<'a, T: SVMMessage>( collected_account_transactions: &mut Option>, transaction: &'a T, transaction_ref: Option<&'a SanitizedTransaction>, - rollback_accounts: &'a mut RollbackAccounts, - durable_nonce: &DurableNonce, - lamports_per_signature: u64, + rollback_accounts: &'a RollbackAccounts, ) { let fee_payer_address = transaction.fee_payer(); match rollback_accounts { RollbackAccounts::FeePayerOnly { fee_payer_account } => { - collected_accounts.push((fee_payer_address, &*fee_payer_account)); + collected_accounts.push((fee_payer_address, fee_payer_account)); if let Some(collected_account_transactions) = collected_account_transactions { collected_account_transactions .push(transaction_ref.expect("transaction ref must exist if collecting")); } } RollbackAccounts::SameNonceAndFeePayer { nonce } => { - // Since we know we are dealing with a valid nonce account, - // unwrap is safe here - nonce - .try_advance_nonce(*durable_nonce, lamports_per_signature) - .unwrap(); collected_accounts.push((nonce.address(), nonce.account())); if let Some(collected_account_transactions) = collected_account_transactions { collected_account_transactions @@ -171,17 +157,12 @@ fn collect_accounts_for_failed_tx<'a, T: SVMMessage>( nonce, fee_payer_account, } => { - collected_accounts.push((fee_payer_address, &*fee_payer_account)); + collected_accounts.push((fee_payer_address, fee_payer_account)); if let Some(collected_account_transactions) = collected_account_transactions { collected_account_transactions .push(transaction_ref.expect("transaction ref must exist if collecting")); } - // Since we know we are dealing with a valid nonce account, - // unwrap is safe here - nonce - .try_advance_nonce(*durable_nonce, lamports_per_signature) - .unwrap(); collected_accounts.push((nonce.address(), nonce.account())); if let Some(collected_account_transactions) = collected_account_transactions { collected_account_transactions @@ -204,7 +185,7 @@ mod tests { message::Message, native_loader, nonce::{ - state::{Data as NonceData, Versions as NonceVersions}, + state::{Data as NonceData, DurableNonce, Versions as NonceVersions}, State as NonceState, }, nonce_account, @@ -315,7 +296,7 @@ mod tests { }; let txs = vec![tx0.clone(), tx1.clone()]; - let mut processing_results = vec![ + let processing_results = vec![ new_executed_processing_result(Ok(()), loaded0), new_executed_processing_result(Ok(()), loaded1), ]; @@ -324,13 +305,8 @@ mod tests { for collect_transactions in [false, true] { let transaction_refs = collect_transactions.then(|| txs.iter().collect::>()); - let (collected_accounts, transactions) = collect_accounts_to_store( - &txs, - &transaction_refs, - &mut processing_results, - &DurableNonce::default(), - 0, - ); + let (collected_accounts, transactions) = + collect_accounts_to_store(&txs, &transaction_refs, &processing_results); assert_eq!(collected_accounts.len(), 2); assert!(collected_accounts .iter() @@ -383,7 +359,7 @@ mod tests { }; let txs = vec![tx]; - let mut processing_results = vec![new_executed_processing_result( + let processing_results = vec![new_executed_processing_result( Err(TransactionError::InstructionError( 1, InstructionError::InvalidArgument, @@ -392,17 +368,11 @@ mod tests { )]; let max_collected_accounts = max_number_of_accounts_to_collect(&txs, &processing_results); assert_eq!(max_collected_accounts, 1); - let durable_nonce = DurableNonce::from_blockhash(&Hash::new_unique()); for collect_transactions in [false, true] { let transaction_refs = collect_transactions.then(|| txs.iter().collect::>()); - let (collected_accounts, transactions) = collect_accounts_to_store( - &txs, - &transaction_refs, - &mut processing_results, - &durable_nonce, - 0, - ); + let (collected_accounts, transactions) = + collect_accounts_to_store(&txs, &transaction_refs, &processing_results); assert_eq!(collected_accounts.len(), 1); assert_eq!( collected_accounts @@ -483,9 +453,8 @@ mod tests { loaded_accounts_data_size: 0, }; - let durable_nonce = DurableNonce::from_blockhash(&Hash::new_unique()); let txs = vec![tx]; - let mut processing_results = vec![new_executed_processing_result( + let processing_results = vec![new_executed_processing_result( Err(TransactionError::InstructionError( 1, InstructionError::InvalidArgument, @@ -497,13 +466,8 @@ mod tests { for collect_transactions in [false, true] { let transaction_refs = collect_transactions.then(|| txs.iter().collect::>()); - let (collected_accounts, transactions) = collect_accounts_to_store( - &txs, - &transaction_refs, - &mut processing_results, - &durable_nonce, - 0, - ); + let (collected_accounts, transactions) = + collect_accounts_to_store(&txs, &transaction_refs, &processing_results); assert_eq!(collected_accounts.len(), 2); assert_eq!( collected_accounts @@ -597,9 +561,8 @@ mod tests { loaded_accounts_data_size: 0, }; - let durable_nonce = DurableNonce::from_blockhash(&Hash::new_unique()); let txs = vec![tx]; - let mut processing_results = vec![new_executed_processing_result( + let processing_results = vec![new_executed_processing_result( Err(TransactionError::InstructionError( 1, InstructionError::InvalidArgument, @@ -611,13 +574,8 @@ mod tests { for collect_transactions in [false, true] { let transaction_refs = collect_transactions.then(|| txs.iter().collect::>()); - let (collected_accounts, transactions) = collect_accounts_to_store( - &txs, - &transaction_refs, - &mut processing_results, - &durable_nonce, - 0, - ); + let (collected_accounts, transactions) = + collect_accounts_to_store(&txs, &transaction_refs, &processing_results); assert_eq!(collected_accounts.len(), 1); let collected_nonce_account = collected_accounts .iter() @@ -658,7 +616,7 @@ mod tests { let from_account_pre = AccountSharedData::new(4242, 0, &Pubkey::default()); let txs = vec![tx]; - let mut processing_results = vec![Ok(ProcessedTransaction::FeesOnly(Box::new( + let processing_results = vec![Ok(ProcessedTransaction::FeesOnly(Box::new( FeesOnlyTransaction { load_error: TransactionError::InvalidProgramForExecution, fee_details: FeeDetails::default(), @@ -669,17 +627,11 @@ mod tests { )))]; let max_collected_accounts = max_number_of_accounts_to_collect(&txs, &processing_results); assert_eq!(max_collected_accounts, 1); - let durable_nonce = DurableNonce::from_blockhash(&Hash::new_unique()); for collect_transactions in [false, true] { let transaction_refs = collect_transactions.then(|| txs.iter().collect::>()); - let (collected_accounts, transactions) = collect_accounts_to_store( - &txs, - &transaction_refs, - &mut processing_results, - &durable_nonce, - 0, - ); + let (collected_accounts, transactions) = + collect_accounts_to_store(&txs, &transaction_refs, &processing_results); assert_eq!(collected_accounts.len(), 1); assert_eq!( collected_accounts diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 44d8195c993c5a..e0072749a4c078 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -128,7 +128,6 @@ use { message::{AccountKeys, SanitizedMessage}, native_loader, native_token::LAMPORTS_PER_SOL, - nonce::state::DurableNonce, packet::PACKET_DATA_SIZE, precompiles::get_precompiles, pubkey::Pubkey, @@ -3017,8 +3016,11 @@ impl Bank { blockhash_queue.get_lamports_per_signature(message.recent_blockhash()) } .or_else(|| { - self.load_message_nonce_account(message) - .map(|(_nonce, nonce_data)| nonce_data.get_lamports_per_signature()) + self.load_message_nonce_account(message).map( + |(_nonce_address, _nonce_account, nonce_data)| { + nonce_data.get_lamports_per_signature() + }, + ) })?; Some(self.get_fee_for_message_with_lamports_per_signature(message, lamports_per_signature)) } @@ -3762,9 +3764,7 @@ impl Bank { pub fn commit_transactions( &self, sanitized_txs: &[SanitizedTransaction], - mut processing_results: Vec, - last_blockhash: Hash, - lamports_per_signature: u64, + processing_results: Vec, processed_counts: &ProcessedTransactionCounts, timings: &mut ExecuteTimings, ) -> Vec { @@ -3799,8 +3799,6 @@ impl Bank { } let ((), store_accounts_us) = measure_us!({ - let durable_nonce = DurableNonce::from_blockhash(&last_blockhash); - // If geyser is present, we must collect `SanitizedTransaction` // references in order to comply with that interface - until it // is changed. @@ -3813,9 +3811,7 @@ impl Bank { let (accounts_to_store, transactions) = collect_accounts_to_store( sanitized_txs, &maybe_transaction_refs, - &mut processing_results, - &durable_nonce, - lamports_per_signature, + &processing_results, ); self.rc.accounts.store_cached( (self.slot(), accounts_to_store.as_slice()), @@ -4634,13 +4630,9 @@ impl Bank { }, ); - let (last_blockhash, lamports_per_signature) = - self.last_blockhash_and_lamports_per_signature(); let commit_results = self.commit_transactions( batch.sanitized_transactions(), processing_results, - last_blockhash, - lamports_per_signature, &processed_counts, timings, ); diff --git a/runtime/src/bank/check_transactions.rs b/runtime/src/bank/check_transactions.rs index d966d986fb8305..0f0d70f15b07ab 100644 --- a/runtime/src/bank/check_transactions.rs +++ b/runtime/src/bank/check_transactions.rs @@ -3,13 +3,21 @@ use { solana_accounts_db::blockhash_queue::BlockhashQueue, solana_perf::perf_libs, solana_sdk::{ + account::AccountSharedData, + account_utils::StateMut, clock::{ MAX_PROCESSING_AGE, MAX_TRANSACTION_FORWARDING_DELAY, MAX_TRANSACTION_FORWARDING_DELAY_GPU, }, message::SanitizedMessage, - nonce::{self, state::DurableNonce, NONCED_TX_MARKER_IX_INDEX}, + nonce::{ + state::{ + Data as NonceData, DurableNonce, State as NonceState, Versions as NonceVersions, + }, + NONCED_TX_MARKER_IX_INDEX, + }, nonce_account, + pubkey::Pubkey, transaction::{Result as TransactionResult, SanitizedTransaction, TransactionError}, }, solana_svm::{ @@ -71,6 +79,10 @@ impl Bank { let hash_queue = self.blockhash_queue.read().unwrap(); let last_blockhash = hash_queue.last_hash(); let next_durable_nonce = DurableNonce::from_blockhash(&last_blockhash); + // safe so long as the BlockhashQueue is consistent + let next_lamports_per_signature = hash_queue + .get_lamports_per_signature(&last_blockhash) + .unwrap(); sanitized_txs .iter() @@ -81,6 +93,7 @@ impl Bank { max_age, &next_durable_nonce, &hash_queue, + next_lamports_per_signature, error_counters, ), Err(e) => Err(e.clone()), @@ -94,6 +107,7 @@ impl Bank { max_age: usize, next_durable_nonce: &DurableNonce, hash_queue: &BlockhashQueue, + next_lamports_per_signature: u64, error_counters: &mut TransactionErrorMetrics, ) -> TransactionCheckResult { let recent_blockhash = tx.message().recent_blockhash(); @@ -102,12 +116,16 @@ impl Bank { nonce: None, lamports_per_signature: hash_info.lamports_per_signature(), }) - } else if let Some((nonce, nonce_data)) = - self.check_and_load_message_nonce_account(tx.message(), next_durable_nonce) + } else if let Some((nonce, previous_lamports_per_signature)) = self + .check_load_and_advance_message_nonce_account( + tx.message(), + next_durable_nonce, + next_lamports_per_signature, + ) { Ok(CheckedTransactionDetails { nonce: Some(nonce), - lamports_per_signature: nonce_data.get_lamports_per_signature(), + lamports_per_signature: previous_lamports_per_signature, }) } else { error_counters.blockhash_not_found += 1; @@ -115,23 +133,40 @@ impl Bank { } } - pub(super) fn check_and_load_message_nonce_account( + pub(super) fn check_load_and_advance_message_nonce_account( &self, message: &SanitizedMessage, next_durable_nonce: &DurableNonce, - ) -> Option<(NonceInfo, nonce::state::Data)> { + next_lamports_per_signature: u64, + ) -> Option<(NonceInfo, u64)> { let nonce_is_advanceable = message.recent_blockhash() != next_durable_nonce.as_hash(); - if nonce_is_advanceable { - self.load_message_nonce_account(message) - } else { - None + if !nonce_is_advanceable { + return None; } + + let (nonce_address, mut nonce_account, nonce_data) = + self.load_message_nonce_account(message)?; + + let previous_lamports_per_signature = nonce_data.get_lamports_per_signature(); + let next_nonce_state = NonceState::new_initialized( + &nonce_data.authority, + *next_durable_nonce, + next_lamports_per_signature, + ); + nonce_account + .set_state(&NonceVersions::new(next_nonce_state)) + .ok()?; + + Some(( + NonceInfo::new(nonce_address, nonce_account), + previous_lamports_per_signature, + )) } pub(super) fn load_message_nonce_account( &self, message: &SanitizedMessage, - ) -> Option<(NonceInfo, nonce::state::Data)> { + ) -> Option<(Pubkey, AccountSharedData, NonceData)> { let nonce_address = message.get_durable_nonce()?; let nonce_account = self.get_account_with_fixed_root(nonce_address)?; let nonce_data = @@ -144,7 +179,7 @@ impl Bank { return None; } - Some((NonceInfo::new(*nonce_address, nonce_account), nonce_data)) + Some((*nonce_address, nonce_account, nonce_data)) } fn check_status_cache( @@ -200,6 +235,7 @@ mod tests { #[test] fn test_check_and_load_message_nonce_account_ok() { + const STALE_LAMPORTS_PER_SIGNATURE: u64 = 42; let (bank, _mint_keypair, custodian_keypair, nonce_keypair, _) = setup_nonce_with_bank( 10_000_000, |_| {}, @@ -221,11 +257,37 @@ mod tests { Some(&custodian_pubkey), &nonce_hash, )); - let nonce_account = bank.get_account(&nonce_pubkey).unwrap(); + + // set a spurious lamports_per_signature value + let mut nonce_account = bank.get_account(&nonce_pubkey).unwrap(); let nonce_data = get_nonce_data_from_account(&nonce_account).unwrap(); + nonce_account + .set_state(&NonceVersions::new(NonceState::new_initialized( + &nonce_data.authority, + nonce_data.durable_nonce, + STALE_LAMPORTS_PER_SIGNATURE, + ))) + .unwrap(); + bank.store_account(&nonce_pubkey, &nonce_account); + + let nonce_account = bank.get_account(&nonce_pubkey).unwrap(); + let (_, next_lamports_per_signature) = bank.last_blockhash_and_lamports_per_signature(); + let mut expected_nonce_info = NonceInfo::new(nonce_pubkey, nonce_account); + expected_nonce_info + .try_advance_nonce(bank.next_durable_nonce(), next_lamports_per_signature) + .unwrap(); + + // we now expect to: + // * advance the nonce account to the current durable nonce value + // * set the blockhash queue's last blockhash's lamports_per_signature value in the nonce data + // * retrieve the previous lamports_per_signature value set on the nonce data for transaction fee checks assert_eq!( - bank.check_and_load_message_nonce_account(&message, &bank.next_durable_nonce()), - Some((NonceInfo::new(nonce_pubkey, nonce_account), nonce_data)) + bank.check_load_and_advance_message_nonce_account( + &message, + &bank.next_durable_nonce(), + next_lamports_per_signature + ), + Some((expected_nonce_info, STALE_LAMPORTS_PER_SIGNATURE)), ); } @@ -252,8 +314,13 @@ mod tests { Some(&custodian_pubkey), &nonce_hash, )); + let (_, lamports_per_signature) = bank.last_blockhash_and_lamports_per_signature(); assert!(bank - .check_and_load_message_nonce_account(&message, &bank.next_durable_nonce()) + .check_load_and_advance_message_nonce_account( + &message, + &bank.next_durable_nonce(), + lamports_per_signature + ) .is_none()); } @@ -281,10 +348,12 @@ mod tests { &nonce_hash, ); message.instructions[0].accounts.clear(); + let (_, lamports_per_signature) = bank.last_blockhash_and_lamports_per_signature(); assert!(bank - .check_and_load_message_nonce_account( + .check_load_and_advance_message_nonce_account( &new_sanitized_message(message), &bank.next_durable_nonce(), + lamports_per_signature, ) .is_none()); } @@ -314,8 +383,13 @@ mod tests { Some(&custodian_pubkey), &nonce_hash, )); + let (_, lamports_per_signature) = bank.last_blockhash_and_lamports_per_signature(); assert!(bank - .check_and_load_message_nonce_account(&message, &bank.next_durable_nonce()) + .check_load_and_advance_message_nonce_account( + &message, + &bank.next_durable_nonce(), + lamports_per_signature + ) .is_none()); } @@ -341,8 +415,13 @@ mod tests { Some(&custodian_pubkey), &Hash::default(), )); + let (_, lamports_per_signature) = bank.last_blockhash_and_lamports_per_signature(); assert!(bank - .check_and_load_message_nonce_account(&message, &bank.next_durable_nonce()) + .check_load_and_advance_message_nonce_account( + &message, + &bank.next_durable_nonce(), + lamports_per_signature + ) .is_none()); } } diff --git a/runtime/src/bank/tests.rs b/runtime/src/bank/tests.rs index ec62293b28f105..4df464cdfea6ff 100644 --- a/runtime/src/bank/tests.rs +++ b/runtime/src/bank/tests.rs @@ -5732,10 +5732,12 @@ fn test_check_ro_durable_nonce_fails() { bank.process_transaction(&tx), Err(TransactionError::BlockhashNotFound) ); + let (_, lamports_per_signature) = bank.last_blockhash_and_lamports_per_signature(); assert_eq!( - bank.check_and_load_message_nonce_account( + bank.check_load_and_advance_message_nonce_account( &new_sanitized_message(tx.message().clone()), &bank.next_durable_nonce(), + lamports_per_signature, ), None ); diff --git a/svm/src/rollback_accounts.rs b/svm/src/rollback_accounts.rs index c2c02f2f80bd43..1a9a764e131186 100644 --- a/svm/src/rollback_accounts.rs +++ b/svm/src/rollback_accounts.rs @@ -51,6 +51,12 @@ impl RollbackAccounts { if let Some(nonce) = nonce { if &fee_payer_address == nonce.address() { + // `nonce` contains an AccountSharedData which has already been advanced to the current DurableNonce + // `fee_payer_account` is an AccountSharedData as it currently exists on-chain + // thus if the nonce account is being used as the fee payer, we need to update that data here + // so we capture both the data change for the nonce and the lamports/rent epoch change for the fee payer + fee_payer_account.set_data_from_slice(nonce.account().data()); + RollbackAccounts::SameNonceAndFeePayer { nonce: NonceInfo::new(fee_payer_address, fee_payer_account), } @@ -63,7 +69,7 @@ impl RollbackAccounts { } else { // When rolling back failed transactions which don't use nonces, the // runtime should not update the fee payer's rent epoch so reset the - // rollback fee payer acocunt's rent epoch to its originally loaded + // rollback fee payer account's rent epoch to its originally loaded // rent epoch value. In the future, a feature gate could be used to // alter this behavior such that rent epoch updates are handled the // same for both nonce and non-nonce failed transactions. diff --git a/svm/src/transaction_processing_result.rs b/svm/src/transaction_processing_result.rs index 7802b9ac213808..0658b5035fda0f 100644 --- a/svm/src/transaction_processing_result.rs +++ b/svm/src/transaction_processing_result.rs @@ -15,7 +15,6 @@ pub trait TransactionProcessingResultExtensions { fn was_processed(&self) -> bool; fn was_processed_with_successful_result(&self) -> bool; fn processed_transaction(&self) -> Option<&ProcessedTransaction>; - fn processed_transaction_mut(&mut self) -> Option<&mut ProcessedTransaction>; fn flattened_result(&self) -> TransactionResult<()>; } @@ -48,13 +47,6 @@ impl TransactionProcessingResultExtensions for TransactionProcessingResult { } } - fn processed_transaction_mut(&mut self) -> Option<&mut ProcessedTransaction> { - match self { - Ok(processed_tx) => Some(processed_tx), - Err(_) => None, - } - } - fn flattened_result(&self) -> TransactionResult<()> { self.as_ref() .map_err(|err| err.clone()) diff --git a/svm/src/transaction_processor.rs b/svm/src/transaction_processor.rs index 76ececd6ec5cd0..f0c9f681a74955 100644 --- a/svm/src/transaction_processor.rs +++ b/svm/src/transaction_processor.rs @@ -475,8 +475,8 @@ impl TransactionBatchProcessor { fee_details.total_fee(), )?; - // Capture fee-subtracted fee payer account and original nonce account state - // to rollback to if transaction execution fails. + // Capture fee-subtracted fee payer account and next nonce account state + // to commit if transaction execution fails. let rollback_accounts = RollbackAccounts::new( nonce, *fee_payer_address, @@ -2180,13 +2180,14 @@ mod tests { let lamports_per_signature = 5000; let rent_collector = RentCollector::default(); let compute_unit_limit = 2 * solana_compute_budget_program::DEFAULT_COMPUTE_UNITS; + let last_blockhash = Hash::new_unique(); let message = new_unchecked_sanitized_message(Message::new_with_blockhash( &[ ComputeBudgetInstruction::set_compute_unit_limit(compute_unit_limit as u32), ComputeBudgetInstruction::set_compute_unit_price(1_000_000), ], Some(&Pubkey::new_unique()), - &Hash::new_unique(), + &last_blockhash, )); let compute_budget_limits = process_compute_budget_instructions(SVMMessage::program_instructions_iter(&message)) @@ -2216,10 +2217,12 @@ mod tests { let mut error_counters = TransactionErrorMetrics::default(); let batch_processor = TransactionBatchProcessor::::default(); + let nonce = Some(NonceInfo::new( *fee_payer_address, fee_payer_account.clone(), )); + let result = batch_processor.validate_transaction_fee_payer( &mock_bank, None, diff --git a/svm/tests/integration_test.rs b/svm/tests/integration_test.rs index 08f6db09101e04..53d9d04f445183 100644 --- a/svm/tests/integration_test.rs +++ b/svm/tests/integration_test.rs @@ -6,20 +6,25 @@ use { MockBankCallback, MockForkGraph, WALLCLOCK_TIME, }, solana_sdk::{ - account::{AccountSharedData, WritableAccount}, + account::{AccountSharedData, ReadableAccount, WritableAccount}, clock::Slot, + feature_set::{self, FeatureSet}, hash::Hash, instruction::{AccountMeta, Instruction}, native_token::LAMPORTS_PER_SOL, + nonce::{self, state::DurableNonce}, pubkey::Pubkey, signature::Signer, signer::keypair::Keypair, system_instruction, system_program, system_transaction, + sysvar::rent::Rent, transaction::{SanitizedTransaction, Transaction, TransactionError}, transaction_context::TransactionReturnData, }, solana_svm::{ account_loader::{CheckedTransactionDetails, TransactionCheckResult}, + nonce_info::NonceInfo, + rollback_accounts::RollbackAccounts, transaction_execution_result::TransactionExecutionDetails, transaction_processing_result::ProcessedTransaction, transaction_processor::{ @@ -27,6 +32,7 @@ use { TransactionProcessingEnvironment, }, }, + solana_svm_transaction::svm_message::SVMMessage, solana_type_overrides::sync::{Arc, RwLock}, std::collections::{HashMap, HashSet}, test_case::test_case, @@ -39,23 +45,27 @@ const DEPLOYMENT_SLOT: u64 = 0; const EXECUTION_SLOT: u64 = 5; // The execution slot must be greater than the deployment slot const EXECUTION_EPOCH: u64 = 2; // The execution epoch must be greater than the deployment epoch const LAMPORTS_PER_SIGNATURE: u64 = 5000; +const LAST_BLOCKHASH: Hash = Hash::new_from_array([7; 32]); // Arbitrary constant hash for advancing nonces -pub type AccountMap = HashMap; +pub type AccountsMap = HashMap; // container for a transaction batch and all data needed to run and verify it against svm #[derive(Debug, Default)] pub struct SvmTestEntry { + // features are disabled by default; these will be enabled + pub enabled_features: Vec, + // programs to deploy to the new svm before transaction execution pub initial_programs: Vec<(String, Slot)>, // accounts to deploy to the new svm before transaction execution - pub initial_accounts: AccountMap, + pub initial_accounts: AccountsMap, // transactions to execute and transaction-specific checks to perform on the results from svm pub transaction_batch: Vec, // expected final account states, checked after transaction execution - pub final_accounts: AccountMap, + pub final_accounts: AccountsMap, } impl SvmTestEntry { @@ -108,18 +118,53 @@ impl SvmTestEntry { // convenience function that adds a transaction that is expected to succeed pub fn push_transaction(&mut self, transaction: Transaction) { + self.push_transaction_with_status(transaction, ExecutionStatus::Succeeded) + } + + // convenience function that adds a transaction with an expected execution status + pub fn push_transaction_with_status( + &mut self, + transaction: Transaction, + status: ExecutionStatus, + ) { self.transaction_batch.push(TransactionBatchItem { transaction, + asserts: TransactionBatchItemAsserts { + status, + ..TransactionBatchItemAsserts::default() + }, ..TransactionBatchItem::default() }); } - // convenience function that adds a transaction that is expected to execute but fail - pub fn push_failed_transaction(&mut self, transaction: Transaction) { + // convenience function that adds a nonce transaction that is expected to succeed + // we accept the prior nonce state and advance it for the check status, since this happens before svm + pub fn push_nonce_transaction(&mut self, transaction: Transaction, nonce_info: NonceInfo) { + self.push_nonce_transaction_with_status(transaction, nonce_info, ExecutionStatus::Succeeded) + } + + // convenience function that adds a nonce transaction with an expected execution status + // we accept the prior nonce state and advance it for the check status, since this happens before svm + pub fn push_nonce_transaction_with_status( + &mut self, + transaction: Transaction, + mut nonce_info: NonceInfo, + status: ExecutionStatus, + ) { + nonce_info + .try_advance_nonce( + DurableNonce::from_blockhash(&LAST_BLOCKHASH), + LAMPORTS_PER_SIGNATURE, + ) + .unwrap(); + self.transaction_batch.push(TransactionBatchItem { transaction, - asserts: TransactionBatchItemAsserts::failed(), - ..TransactionBatchItem::default() + asserts: TransactionBatchItemAsserts { + status, + ..TransactionBatchItemAsserts::default() + }, + ..TransactionBatchItem::with_nonce(nonce_info) }); } @@ -155,6 +200,18 @@ pub struct TransactionBatchItem { pub asserts: TransactionBatchItemAsserts, } +impl TransactionBatchItem { + fn with_nonce(nonce_info: NonceInfo) -> Self { + Self { + check_result: Ok(CheckedTransactionDetails { + nonce: Some(nonce_info), + lamports_per_signature: LAMPORTS_PER_SIGNATURE, + }), + ..Self::default() + } + } +} + impl Default for TransactionBatchItem { fn default() -> Self { Self { @@ -163,7 +220,7 @@ impl Default for TransactionBatchItem { nonce: None, lamports_per_signature: LAMPORTS_PER_SIGNATURE, }), - asserts: TransactionBatchItemAsserts::succeeded(), + asserts: TransactionBatchItemAsserts::default(), } } } @@ -171,45 +228,33 @@ impl Default for TransactionBatchItem { // asserts for a given transaction in a batch // we can automatically check whether it executed, whether it succeeded // log items we expect to see (exect match only), and rodata -#[derive(Clone, Debug)] +#[derive(Clone, Debug, Default)] pub struct TransactionBatchItemAsserts { - pub executed: bool, - pub succeeded: bool, + pub status: ExecutionStatus, pub logs: Vec, pub return_data: ReturnDataAssert, } impl TransactionBatchItemAsserts { - pub fn succeeded() -> Self { - Self { - executed: true, - succeeded: true, - logs: vec![], - return_data: ReturnDataAssert::Skip, - } + pub fn succeeded(&self) -> bool { + self.status.succeeded() } - pub fn failed() -> Self { - Self { - executed: true, - succeeded: false, - logs: vec![], - return_data: ReturnDataAssert::Skip, - } + pub fn executed(&self) -> bool { + self.status.executed() } - pub fn not_executed() -> Self { - Self { - executed: false, - succeeded: false, - logs: vec![], - return_data: ReturnDataAssert::Skip, - } + pub fn processed(&self) -> bool { + self.status.processed() + } + + pub fn discarded(&self) -> bool { + self.status.discarded() } pub fn check_executed_transaction(&self, execution_details: &TransactionExecutionDetails) { - assert!(self.executed); - assert_eq!(self.succeeded, execution_details.status.is_ok()); + assert!(self.executed()); + assert_eq!(self.succeeded(), execution_details.status.is_ok()); if !self.logs.is_empty() { let actual_logs = execution_details.log_messages.as_ref().unwrap(); @@ -227,7 +272,51 @@ impl TransactionBatchItemAsserts { } } -#[derive(Clone, Debug, Default, PartialEq)] +impl From for TransactionBatchItemAsserts { + fn from(status: ExecutionStatus) -> Self { + Self { + status, + ..Self::default() + } + } +} + +// states a transaction can end in after a trip through the batch processor: +// * discarded: no-op. not even processed. a flawed transaction excluded from the entry +// * processed-failed: aka fee (and nonce) only. charged and added to an entry but not executed, would have failed invariably +// * executed-failed: failed during execution. as above, fees charged and nonce advanced +// * succeeded: what we all aspire to be in our transaction processing lifecycles +#[derive(Copy, Clone, Debug, Default, PartialEq, Eq, PartialOrd, Ord)] +pub enum ExecutionStatus { + Discarded, + ProcessedFailed, + ExecutedFailed, + #[default] + Succeeded, +} + +// note we avoid the word "failed" because it is confusing +// the batch processor uses it to mean "executed and not succeeded" +// but intuitively (and from the point of a user) it could just as likely mean "any state other than succeeded" +impl ExecutionStatus { + pub fn succeeded(self) -> bool { + self == Self::Succeeded + } + + pub fn executed(self) -> bool { + self > Self::ProcessedFailed + } + + pub fn processed(self) -> bool { + self != Self::Discarded + } + + pub fn discarded(self) -> bool { + self == Self::Discarded + } +} + +#[derive(Clone, Debug, Default, PartialEq, Eq)] pub enum ReturnDataAssert { Some(TransactionReturnData), None, @@ -397,12 +486,15 @@ fn program_medley() -> Vec { ], ); - test_entry.push_failed_transaction(Transaction::new_signed_with_payer( - &[instruction], - Some(&fee_payer), - &[&fee_payer_keypair, &sender_keypair], - Hash::default(), - )); + test_entry.push_transaction_with_status( + Transaction::new_signed_with_payer( + &[instruction], + Some(&fee_payer), + &[&fee_payer_keypair, &sender_keypair], + Hash::default(), + ), + ExecutionStatus::ExecutedFailed, + ); test_entry.transaction_batch[3] .asserts @@ -425,7 +517,7 @@ fn program_medley() -> Vec { Hash::default(), ), check_result: Err(TransactionError::BlockhashNotFound), - asserts: TransactionBatchItemAsserts::not_executed(), + asserts: ExecutionStatus::Discarded.into(), }); } @@ -473,12 +565,15 @@ fn simple_transfer() -> Vec { source_data.set_lamports(transfer_amount - 1); test_entry.add_initial_account(source, &source_data); - test_entry.push_failed_transaction(system_transaction::transfer( - &source_keypair, - &Pubkey::new_unique(), - transfer_amount, - Hash::default(), - )); + test_entry.push_transaction_with_status( + system_transaction::transfer( + &source_keypair, + &Pubkey::new_unique(), + transfer_amount, + Hash::default(), + ), + ExecutionStatus::ExecutedFailed, + ); test_entry.decrease_expected_lamports(&source, LAMPORTS_PER_SIGNATURE); } @@ -493,23 +588,22 @@ fn simple_transfer() -> Vec { Hash::default(), ), check_result: Err(TransactionError::BlockhashNotFound), - asserts: TransactionBatchItemAsserts::not_executed(), + asserts: ExecutionStatus::Discarded.into(), }); } // 3: a non-executable transfer that fails loading the fee-payer // NOTE when we support the processed/executed distinction, this is NOT processed { - test_entry.transaction_batch.push(TransactionBatchItem { - transaction: system_transaction::transfer( + test_entry.push_transaction_with_status( + system_transaction::transfer( &Keypair::new(), &Pubkey::new_unique(), transfer_amount, Hash::default(), ), - asserts: TransactionBatchItemAsserts::not_executed(), - ..TransactionBatchItem::default() - }); + ExecutionStatus::Discarded, + ); } // 4: a non-executable transfer that fails loading the program @@ -531,16 +625,217 @@ fn simple_transfer() -> Vec { system_instruction::transfer(&source, &Pubkey::new_unique(), transfer_amount); instruction.program_id = Pubkey::new_unique(); - test_entry.transaction_batch.push(TransactionBatchItem { - transaction: Transaction::new_signed_with_payer( + test_entry.push_transaction_with_status( + Transaction::new_signed_with_payer( &[instruction], Some(&source), &[&source_keypair], Hash::default(), ), - asserts: TransactionBatchItemAsserts::not_executed(), - ..TransactionBatchItem::default() - }); + ExecutionStatus::Discarded, + ); + } + + vec![test_entry] +} + +fn simple_nonce_fee_only( + enable_fee_only_transactions: bool, + fee_paying_nonce: bool, +) -> Vec { + let mut test_entry = SvmTestEntry::default(); + if enable_fee_only_transactions { + test_entry + .enabled_features + .push(feature_set::enable_transaction_loading_failure_fees::id()); + } + + let program_name = "hello-solana".to_string(); + let real_program_id = program_address(&program_name); + test_entry + .initial_programs + .push((program_name, DEPLOYMENT_SLOT)); + + // create and return a transaction, fee payer, and nonce info + // sets up initial account states but not final ones + // there are four cases of fee_paying_nonce and fake_fee_payer: + // * false/false: normal nonce account with rent minimum, normal fee payer account with 1sol + // * true/false: normal nonce account used to pay fees with rent minimum plus 1sol + // * false/true: normal nonce account with rent minimum, fee payer doesnt exist + // * true/true: same account for both which does not exist + let mk_nonce_transaction = |test_entry: &mut SvmTestEntry, program_id, fake_fee_payer: bool| { + let fee_payer_keypair = Keypair::new(); + let fee_payer = fee_payer_keypair.pubkey(); + let nonce_pubkey = if fee_paying_nonce { + fee_payer + } else { + Pubkey::new_unique() + }; + + let nonce_size = nonce::State::size(); + let mut nonce_balance = Rent::default().minimum_balance(nonce_size); + + if !fake_fee_payer && !fee_paying_nonce { + let mut fee_payer_data = AccountSharedData::default(); + fee_payer_data.set_lamports(LAMPORTS_PER_SOL); + test_entry.add_initial_account(fee_payer, &fee_payer_data); + } else if fee_paying_nonce { + nonce_balance = nonce_balance.saturating_add(LAMPORTS_PER_SOL); + } + + let nonce_initial_hash = DurableNonce::from_blockhash(&Hash::new_unique()); + let nonce_data = + nonce::state::Data::new(fee_payer, nonce_initial_hash, LAMPORTS_PER_SIGNATURE); + let nonce_account = AccountSharedData::new_data( + nonce_balance, + &nonce::state::Versions::new(nonce::State::Initialized(nonce_data.clone())), + &system_program::id(), + ) + .unwrap(); + let nonce_info = NonceInfo::new(nonce_pubkey, nonce_account.clone()); + + if !(fake_fee_payer && fee_paying_nonce) { + test_entry.add_initial_account(nonce_pubkey, &nonce_account); + } + + let instructions = vec![ + system_instruction::advance_nonce_account(&nonce_pubkey, &fee_payer), + Instruction::new_with_bytes(program_id, &[], vec![]), + ]; + + let transaction = Transaction::new_signed_with_payer( + &instructions, + Some(&fee_payer), + &[&fee_payer_keypair], + nonce_data.blockhash(), + ); + + (transaction, fee_payer, nonce_info) + }; + + // successful nonce transaction, regardless of features + { + let (transaction, fee_payer, mut nonce_info) = + mk_nonce_transaction(&mut test_entry, real_program_id, false); + test_entry.push_nonce_transaction(transaction, nonce_info.clone()); + + test_entry.decrease_expected_lamports(&fee_payer, LAMPORTS_PER_SIGNATURE); + + nonce_info + .try_advance_nonce( + DurableNonce::from_blockhash(&LAST_BLOCKHASH), + LAMPORTS_PER_SIGNATURE, + ) + .unwrap(); + + test_entry + .final_accounts + .get_mut(nonce_info.address()) + .unwrap() + .data_as_mut_slice() + .copy_from_slice(nonce_info.account().data()); + } + + // non-executing nonce transaction (fee payer doesnt exist) regardless of features + { + let (transaction, _fee_payer, nonce_info) = + mk_nonce_transaction(&mut test_entry, real_program_id, true); + + test_entry + .final_accounts + .entry(*nonce_info.address()) + .and_modify(|account| account.set_rent_epoch(0)); + + test_entry.push_nonce_transaction_with_status( + transaction, + nonce_info, + ExecutionStatus::Discarded, + ); + } + + // failing nonce transaction (bad system instruction) regardless of features + { + let (transaction, fee_payer, mut nonce_info) = + mk_nonce_transaction(&mut test_entry, system_program::id(), false); + test_entry.push_nonce_transaction_with_status( + transaction, + nonce_info.clone(), + ExecutionStatus::ExecutedFailed, + ); + + test_entry.decrease_expected_lamports(&fee_payer, LAMPORTS_PER_SIGNATURE); + + nonce_info + .try_advance_nonce( + DurableNonce::from_blockhash(&LAST_BLOCKHASH), + LAMPORTS_PER_SIGNATURE, + ) + .unwrap(); + + test_entry + .final_accounts + .get_mut(nonce_info.address()) + .unwrap() + .data_as_mut_slice() + .copy_from_slice(nonce_info.account().data()); + } + + // and this (program doesnt exist) will be a non-executing transaction without the feature + // or a fee-only transaction with it. which is identical to failed *except* rent is not updated + { + let (transaction, fee_payer, mut nonce_info) = + mk_nonce_transaction(&mut test_entry, Pubkey::new_unique(), false); + + if enable_fee_only_transactions { + test_entry.push_nonce_transaction_with_status( + transaction, + nonce_info.clone(), + ExecutionStatus::ProcessedFailed, + ); + + test_entry.decrease_expected_lamports(&fee_payer, LAMPORTS_PER_SIGNATURE); + + nonce_info + .try_advance_nonce( + DurableNonce::from_blockhash(&LAST_BLOCKHASH), + LAMPORTS_PER_SIGNATURE, + ) + .unwrap(); + + test_entry + .final_accounts + .get_mut(nonce_info.address()) + .unwrap() + .data_as_mut_slice() + .copy_from_slice(nonce_info.account().data()); + + // if the nonce account pays fees, it keeps its new rent epoch, otherwise it resets + if !fee_paying_nonce { + test_entry + .final_accounts + .get_mut(nonce_info.address()) + .unwrap() + .set_rent_epoch(0); + } + } else { + test_entry + .final_accounts + .get_mut(&fee_payer) + .unwrap() + .set_rent_epoch(0); + + test_entry + .final_accounts + .get_mut(nonce_info.address()) + .unwrap() + .set_rent_epoch(0); + + test_entry.push_nonce_transaction_with_status( + transaction, + nonce_info, + ExecutionStatus::Discarded, + ); + } } vec![test_entry] @@ -548,6 +843,10 @@ fn simple_transfer() -> Vec { #[test_case(program_medley())] #[test_case(simple_transfer())] +#[test_case(simple_nonce_fee_only(false, false))] +#[test_case(simple_nonce_fee_only(true, false))] +#[test_case(simple_nonce_fee_only(false, true))] +#[test_case(simple_nonce_fee_only(true, true))] fn svm_integration(test_entries: Vec) { for test_entry in test_entries { execute_test_entry(test_entry); @@ -596,37 +895,61 @@ fn execute_test_entry(test_entry: SvmTestEntry) { ..Default::default() }; + let mut feature_set = FeatureSet::default(); + for feature_id in &test_entry.enabled_features { + feature_set.activate(feature_id, 0); + } + + let processing_environment = TransactionProcessingEnvironment { + blockhash: LAST_BLOCKHASH, + feature_set: feature_set.into(), + lamports_per_signature: LAMPORTS_PER_SIGNATURE, + ..TransactionProcessingEnvironment::default() + }; + // execute transaction batch let (transactions, check_results) = test_entry.prepare_transactions(); let batch_output = batch_processor.load_and_execute_sanitized_transactions( &mock_bank, &transactions, check_results, - &TransactionProcessingEnvironment::default(), + &processing_environment, &processing_config, ); // build a hashmap of final account states incrementally, starting with all initial states, updating to all final states // NOTE with SIMD-83 an account may appear multiple times in the same batch let mut final_accounts_actual = test_entry.initial_accounts.clone(); - for processed_transaction in batch_output - .processing_results - .iter() - .filter_map(|r| r.as_ref().ok()) - { + + for (index, processed_transaction) in batch_output.processing_results.iter().enumerate() { match processed_transaction { - ProcessedTransaction::Executed(executed_transaction) => { + Ok(ProcessedTransaction::Executed(executed_transaction)) => { for (pubkey, account_data) in executed_transaction.loaded_transaction.accounts.clone() { final_accounts_actual.insert(pubkey, account_data); } } - // NOTE this is a possible state with `feature_set::enable_transaction_loading_failure_fees` enabled - // by using `TransactionProcessingEnvironment::default()` we have all features disabled - // in other words, this will be unreachable until we are ready to test fee-only transactions - // (or the feature is activated on mainnet and removed... but we should do it before then!) - ProcessedTransaction::FeesOnly(_) => unreachable!(), + Ok(ProcessedTransaction::FeesOnly(fees_only_transaction)) => { + let fee_payer = transactions[index].fee_payer(); + + match fees_only_transaction.rollback_accounts.clone() { + RollbackAccounts::FeePayerOnly { fee_payer_account } => { + final_accounts_actual.insert(*fee_payer, fee_payer_account); + } + RollbackAccounts::SameNonceAndFeePayer { nonce } => { + final_accounts_actual.insert(*nonce.address(), nonce.account().clone()); + } + RollbackAccounts::SeparateNonceAndFeePayer { + nonce, + fee_payer_account, + } => { + final_accounts_actual.insert(*fee_payer, fee_payer_account); + final_accounts_actual.insert(*nonce.address(), nonce.account().clone()); + } + } + } + Err(_) => {} } } @@ -650,8 +973,11 @@ fn execute_test_entry(test_entry: SvmTestEntry) { match processing_result { Ok(ProcessedTransaction::Executed(executed_transaction)) => test_item_asserts .check_executed_transaction(&executed_transaction.execution_details), - Ok(ProcessedTransaction::FeesOnly(_)) => unreachable!(), - Err(_) => assert!(!test_item_asserts.executed), + Ok(ProcessedTransaction::FeesOnly(_)) => { + assert!(test_item_asserts.processed()); + assert!(!test_item_asserts.executed()); + } + Err(_) => assert!(test_item_asserts.discarded()), } } } diff --git a/svm/tests/mock_bank.rs b/svm/tests/mock_bank.rs index 57f00360e91bfb..5797d514888201 100644 --- a/svm/tests/mock_bank.rs +++ b/svm/tests/mock_bank.rs @@ -1,7 +1,9 @@ +#[allow(deprecated)] +use solana_sdk::sysvar::recent_blockhashes::{Entry as BlockhashesEntry, RecentBlockhashes}; use { solana_bpf_loader_program::syscalls::{ - SyscallAbort, SyscallGetClockSysvar, SyscallInvokeSignedRust, SyscallLog, SyscallMemcpy, - SyscallMemset, SyscallSetReturnData, + SyscallAbort, SyscallGetClockSysvar, SyscallGetRentSysvar, SyscallInvokeSignedRust, + SyscallLog, SyscallMemcpy, SyscallMemset, SyscallSetReturnData, }, solana_compute_budget::compute_budget::ComputeBudget, solana_feature_set::FeatureSet, @@ -21,6 +23,7 @@ use { clock::{Clock, UnixTimestamp}, native_loader, pubkey::Pubkey, + rent::Rent, slot_hashes::Slot, sysvar::SysvarId, }, @@ -201,6 +204,8 @@ pub fn create_executable_environment( program_cache.fork_graph = Some(Arc::downgrade(&fork_graph)); // We must fill in the sysvar cache entries + + // clock contents are important because we use them for a sysvar loading test let clock = Clock { slot: DEPLOYMENT_SLOT, epoch_start_timestamp: WALLCLOCK_TIME.saturating_sub(10) as UnixTimestamp, @@ -216,6 +221,31 @@ pub fn create_executable_environment( .write() .unwrap() .insert(Clock::id(), account_data); + + // default rent is fine + let rent = Rent::default(); + + let mut account_data = AccountSharedData::default(); + account_data.set_data(bincode::serialize(&rent).unwrap()); + mock_bank + .account_shared_data + .write() + .unwrap() + .insert(Rent::id(), account_data); + + // SystemInstruction::AdvanceNonceAccount asserts RecentBlockhashes is non-empty + // but then just gets the blockhash from InvokeContext. so the sysvar doesnt need real entries + #[allow(deprecated)] + let recent_blockhashes = vec![BlockhashesEntry::default()]; + + let mut account_data = AccountSharedData::default(); + account_data.set_data(bincode::serialize(&recent_blockhashes).unwrap()); + #[allow(deprecated)] + mock_bank + .account_shared_data + .write() + .unwrap() + .insert(RecentBlockhashes::id(), account_data); } #[allow(unused)] @@ -304,5 +334,9 @@ fn create_custom_environment<'a>() -> BuiltinProgram> { .register_function_hashed(*b"sol_get_clock_sysvar", SyscallGetClockSysvar::vm) .expect("Registration failed"); + function_registry + .register_function_hashed(*b"sol_get_rent_sysvar", SyscallGetRentSysvar::vm) + .expect("Registration failed"); + BuiltinProgram::new_loader(vm_config, function_registry) }