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) }