From 0d117d420c8936577fb7d6d3c8dfb80cc1ce655e Mon Sep 17 00:00:00 2001 From: Pankaj Garg Date: Fri, 26 Jan 2024 13:46:44 -0800 Subject: [PATCH] Remove BlockhashQueue dependency from SVM related code (#34974) --- accounts-db/src/transaction_results.rs | 2 +- core/src/banking_stage/consumer.rs | 30 ++++++++-------- .../scheduler_controller.rs | 4 +-- .../unprocessed_transaction_storage.rs | 2 +- runtime/src/bank.rs | 36 +++++++++---------- runtime/src/bank/tests.rs | 16 ++------- runtime/src/svm/account_loader.rs | 26 ++++---------- 7 files changed, 45 insertions(+), 71 deletions(-) diff --git a/accounts-db/src/transaction_results.rs b/accounts-db/src/transaction_results.rs index bcfe18585..bc0a330f5 100644 --- a/accounts-db/src/transaction_results.rs +++ b/accounts-db/src/transaction_results.rs @@ -17,7 +17,7 @@ use { }, }; -pub type TransactionCheckResult = (transaction::Result<()>, Option); +pub type TransactionCheckResult = (transaction::Result<()>, Option, Option); pub struct TransactionResults { pub fee_collection_results: Vec>, diff --git a/core/src/banking_stage/consumer.rs b/core/src/banking_stage/consumer.rs index d5dccca98..ad42da3ba 100644 --- a/core/src/banking_stage/consumer.rs +++ b/core/src/banking_stage/consumer.rs @@ -403,7 +403,9 @@ impl Consumer { let pre_results = vec![Ok(()); txs.len()]; let check_results = bank.check_transactions(txs, &pre_results, MAX_PROCESSING_AGE, &mut error_counters); - let check_results = check_results.into_iter().map(|(result, _nonce)| result); + let check_results = check_results + .into_iter() + .map(|(result, _nonce, _lamports)| result); let mut output = self.process_and_record_transactions_with_pre_results( bank, txs, @@ -787,7 +789,7 @@ impl Consumer { valid_txs .iter() .enumerate() - .filter_map(|(index, (x, _h))| if x.is_ok() { Some(index) } else { None }) + .filter_map(|(index, (x, _h, _lamports))| if x.is_ok() { Some(index) } else { None }) .collect_vec() } } @@ -2488,24 +2490,24 @@ mod tests { fn test_bank_filter_valid_transaction_indexes() { assert_eq!( Consumer::filter_valid_transaction_indexes(&[ - (Err(TransactionError::BlockhashNotFound), None), - (Err(TransactionError::BlockhashNotFound), None), - (Ok(()), None), - (Err(TransactionError::BlockhashNotFound), None), - (Ok(()), None), - (Ok(()), None), + (Err(TransactionError::BlockhashNotFound), None, None), + (Err(TransactionError::BlockhashNotFound), None, None), + (Ok(()), None, None), + (Err(TransactionError::BlockhashNotFound), None, None), + (Ok(()), None, None), + (Ok(()), None, None), ]), [2, 4, 5] ); assert_eq!( Consumer::filter_valid_transaction_indexes(&[ - (Ok(()), None), - (Err(TransactionError::BlockhashNotFound), None), - (Err(TransactionError::BlockhashNotFound), None), - (Ok(()), None), - (Ok(()), None), - (Ok(()), None), + (Ok(()), None, None), + (Err(TransactionError::BlockhashNotFound), None, None), + (Err(TransactionError::BlockhashNotFound), None, None), + (Ok(()), None, None), + (Ok(()), None, None), + (Ok(()), None, None), ]), [0, 3, 4, 5] ); diff --git a/core/src/banking_stage/transaction_scheduler/scheduler_controller.rs b/core/src/banking_stage/transaction_scheduler/scheduler_controller.rs index 225ff6a53..c336f56f8 100644 --- a/core/src/banking_stage/transaction_scheduler/scheduler_controller.rs +++ b/core/src/banking_stage/transaction_scheduler/scheduler_controller.rs @@ -168,7 +168,7 @@ impl SchedulerController { let fee_check_results: Vec<_> = check_results .into_iter() .zip(transactions) - .map(|((result, _nonce), tx)| { + .map(|((result, _nonce, _lamports), tx)| { result?; // if there's already error do nothing Consumer::check_fee_payer_unlocked(bank, tx.message(), &mut error_counters) }) @@ -226,7 +226,7 @@ impl SchedulerController { &mut error_counters, ); - for ((result, _nonce), id) in check_results.into_iter().zip(chunk.iter()) { + for ((result, _nonce, _lamports), id) in check_results.into_iter().zip(chunk.iter()) { if result.is_err() { saturating_add_assign!(self.count_metrics.num_dropped_on_age_and_status, 1); self.container.remove_by_id(&id.id); diff --git a/core/src/banking_stage/unprocessed_transaction_storage.rs b/core/src/banking_stage/unprocessed_transaction_storage.rs index f8d99c779..257bf1b14 100644 --- a/core/src/banking_stage/unprocessed_transaction_storage.rs +++ b/core/src/banking_stage/unprocessed_transaction_storage.rs @@ -776,7 +776,7 @@ impl ThreadLocalUnprocessedPackets { .iter() .enumerate() .filter_map( - |(tx_index, (result, _))| if result.is_ok() { Some(tx_index) } else { None }, + |(tx_index, (result, _, _))| if result.is_ok() { Some(tx_index) } else { None }, ) .collect_vec() } diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index bfab5a7c2..548e762c9 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -4460,7 +4460,7 @@ impl Bank { &hash_queue, error_counters, ), - Err(e) => (Err(e.clone()), None), + Err(e) => (Err(e.clone()), None, None), }) .collect() } @@ -4475,14 +4475,20 @@ impl Bank { ) -> TransactionCheckResult { let recent_blockhash = tx.message().recent_blockhash(); if hash_queue.is_hash_valid_for_age(recent_blockhash, max_age) { - (Ok(()), None) + ( + Ok(()), + None, + hash_queue.get_lamports_per_signature(tx.message().recent_blockhash()), + ) } else if let Some((address, account)) = self.check_transaction_for_nonce(tx, next_durable_nonce) { - (Ok(()), Some(NoncePartial::new(address, account))) + let nonce = NoncePartial::new(address, account); + let lamports_per_signature = nonce.lamports_per_signature(); + (Ok(()), Some(nonce), lamports_per_signature) } else { error_counters.blockhash_not_found += 1; - (Err(TransactionError::BlockhashNotFound), None) + (Err(TransactionError::BlockhashNotFound), None, None) } } @@ -4508,16 +4514,16 @@ impl Bank { sanitized_txs .iter() .zip(lock_results) - .map(|(sanitized_tx, (lock_result, nonce))| { + .map(|(sanitized_tx, (lock_result, nonce, lamports))| { let sanitized_tx = sanitized_tx.borrow(); if lock_result.is_ok() && self.is_transaction_already_processed(sanitized_tx, &rcache) { error_counters.already_processed += 1; - return (Err(TransactionError::AlreadyProcessed), None); + return (Err(TransactionError::AlreadyProcessed), None, None); } - (lock_result, nonce) + (lock_result, nonce, lamports) }) .collect() } @@ -5075,19 +5081,11 @@ impl Bank { txs: &[SanitizedTransaction], lock_results: &mut [TransactionCheckResult], program_owners: &'a [Pubkey], - hash_queue: &BlockhashQueue, ) -> HashMap { let mut result: HashMap = HashMap::new(); lock_results.iter_mut().zip(txs).for_each(|etx| { - if let ((Ok(()), nonce), tx) = etx { - if nonce - .as_ref() - .map(|nonce| nonce.lamports_per_signature()) - .unwrap_or_else(|| { - hash_queue.get_lamports_per_signature(tx.message().recent_blockhash()) - }) - .is_some() - { + if let ((Ok(()), _nonce, lamports_per_signature), tx) = etx { + if lamports_per_signature.is_some() { tx.message() .account_keys() .iter() @@ -5113,7 +5111,7 @@ impl Bank { // If the transaction's nonce account was not valid, and blockhash is not found, // the transaction will fail to process. Let's not load any programs from the // transaction, and update the status of the transaction. - *etx.0 = (Err(TransactionError::BlockhashNotFound), None); + *etx.0 = (Err(TransactionError::BlockhashNotFound), None, None); } } }); @@ -5340,7 +5338,6 @@ impl Bank { sanitized_txs, check_results, PROGRAM_OWNERS, - &self.blockhash_queue.read().unwrap(), ); let native_loader = native_loader::id(); for builtin_program in self.builtin_programs.iter() { @@ -5357,7 +5354,6 @@ impl Bank { &self.ancestors, sanitized_txs, check_results, - &self.blockhash_queue.read().unwrap(), error_counters, &self.rent_collector, &self.feature_set, diff --git a/runtime/src/bank/tests.rs b/runtime/src/bank/tests.rs index 763b8c7db..337556246 100644 --- a/runtime/src/bank/tests.rs +++ b/runtime/src/bank/tests.rs @@ -10989,8 +10989,7 @@ fn test_rent_state_list_len() { &bank.accounts().accounts_db, &bank.ancestors, &[sanitized_tx.clone()], - &[(Ok(()), None)], - &bank.blockhash_queue.read().unwrap(), + &[(Ok(()), None, Some(0))], &mut error_counters, &bank.rent_collector, &bank.feature_set, @@ -13723,8 +13722,6 @@ fn test_filter_executable_program_accounts() { &AccountSharedData::new(40, 1, &program2_pubkey), ); - let mut hash_queue = BlockhashQueue::new(100); - let tx1 = Transaction::new_with_compiled_instructions( &[&keypair1], &[non_program_pubkey1], @@ -13732,7 +13729,6 @@ fn test_filter_executable_program_accounts() { vec![account1_pubkey, account2_pubkey, account3_pubkey], vec![CompiledInstruction::new(1, &(), vec![0])], ); - hash_queue.register_hash(&tx1.message().recent_blockhash, 0); let sanitized_tx1 = SanitizedTransaction::from_transaction_for_tests(tx1); let tx2 = Transaction::new_with_compiled_instructions( @@ -13742,7 +13738,6 @@ fn test_filter_executable_program_accounts() { vec![account4_pubkey, account3_pubkey, account2_pubkey], vec![CompiledInstruction::new(1, &(), vec![0])], ); - hash_queue.register_hash(&tx2.message().recent_blockhash, 0); let sanitized_tx2 = SanitizedTransaction::from_transaction_for_tests(tx2); let ancestors = vec![(0, 0)].into_iter().collect(); @@ -13750,9 +13745,8 @@ fn test_filter_executable_program_accounts() { let programs = bank.filter_executable_program_accounts( &ancestors, &[sanitized_tx1, sanitized_tx2], - &mut [(Ok(()), None), (Ok(()), None)], + &mut [(Ok(()), None, Some(0)), (Ok(()), None, Some(0))], owners, - &hash_queue, ); // The result should contain only account3_pubkey, and account4_pubkey as the program accounts @@ -13822,8 +13816,6 @@ fn test_filter_executable_program_accounts_invalid_blockhash() { &AccountSharedData::new(40, 1, &program2_pubkey), ); - let mut hash_queue = BlockhashQueue::new(100); - let tx1 = Transaction::new_with_compiled_instructions( &[&keypair1], &[non_program_pubkey1], @@ -13831,7 +13823,6 @@ fn test_filter_executable_program_accounts_invalid_blockhash() { vec![account1_pubkey, account2_pubkey, account3_pubkey], vec![CompiledInstruction::new(1, &(), vec![0])], ); - hash_queue.register_hash(&tx1.message().recent_blockhash, 0); let sanitized_tx1 = SanitizedTransaction::from_transaction_for_tests(tx1); let tx2 = Transaction::new_with_compiled_instructions( @@ -13846,13 +13837,12 @@ fn test_filter_executable_program_accounts_invalid_blockhash() { let ancestors = vec![(0, 0)].into_iter().collect(); let owners = &[program1_pubkey, program2_pubkey]; - let mut lock_results = vec![(Ok(()), None), (Ok(()), None)]; + let mut lock_results = vec![(Ok(()), None, Some(0)), (Ok(()), None, None)]; let programs = bank.filter_executable_program_accounts( &ancestors, &[sanitized_tx1, sanitized_tx2], &mut lock_results, owners, - &hash_queue, ); // The result should contain only account3_pubkey as the program accounts diff --git a/runtime/src/svm/account_loader.rs b/runtime/src/svm/account_loader.rs index 8fa432db1..beedace9e 100644 --- a/runtime/src/svm/account_loader.rs +++ b/runtime/src/svm/account_loader.rs @@ -7,8 +7,7 @@ use { accounts::{LoadedTransaction, TransactionLoadResult, TransactionRent}, accounts_db::AccountsDb, ancestors::Ancestors, - blockhash_queue::BlockhashQueue, - nonce_info::{NonceFull, NonceInfo}, + nonce_info::NonceFull, rent_collector::{RentCollector, RENT_EXEMPT_RENT_EPOCH}, rent_debits::RentDebits, transaction_error_metrics::TransactionErrorMetrics, @@ -45,7 +44,6 @@ pub(crate) fn load_accounts( ancestors: &Ancestors, txs: &[SanitizedTransaction], lock_results: &[TransactionCheckResult], - hash_queue: &BlockhashQueue, error_counters: &mut TransactionErrorMetrics, rent_collector: &RentCollector, feature_set: &FeatureSet, @@ -59,17 +57,11 @@ pub(crate) fn load_accounts( txs.iter() .zip(lock_results) .map(|etx| match etx { - (tx, (Ok(()), nonce)) => { - let lamports_per_signature = nonce - .as_ref() - .map(|nonce| nonce.lamports_per_signature()) - .unwrap_or_else(|| { - hash_queue.get_lamports_per_signature(tx.message().recent_blockhash()) - }); + (tx, (Ok(()), nonce, lamports_per_signature)) => { let fee = if let Some(lamports_per_signature) = lamports_per_signature { fee_structure.calculate_fee( tx.message(), - lamports_per_signature, + *lamports_per_signature, &process_compute_budget_instructions( tx.message().program_instructions_iter(), ) @@ -118,7 +110,7 @@ pub(crate) fn load_accounts( (Ok(loaded_transaction), nonce) } - (_, (Err(e), _nonce)) => (Err(e.clone()), None), + (_, (Err(e), _nonce, _lamports_per_signature)) => (Err(e.clone()), None), }) .collect() } @@ -525,8 +517,6 @@ mod tests { feature_set: &FeatureSet, fee_structure: &FeeStructure, ) -> Vec { - let mut hash_queue = BlockhashQueue::new(100); - hash_queue.register_hash(&tx.message().recent_blockhash, lamports_per_signature); let accounts_db = AccountsDb::new_single_for_tests(); let accounts = Accounts::new(Arc::new(accounts_db)); for ka in ka.iter() { @@ -539,8 +529,7 @@ mod tests { &accounts.accounts_db, &ancestors, &[sanitized_tx], - &[(Ok(()), None)], - &hash_queue, + &[(Ok(()), None, Some(lamports_per_signature))], error_counters, rent_collector, feature_set, @@ -1008,8 +997,6 @@ mod tests { ) -> Vec { let tx = SanitizedTransaction::from_transaction_for_tests(tx); let rent_collector = RentCollector::default(); - let mut hash_queue = BlockhashQueue::new(100); - hash_queue.register_hash(tx.message().recent_blockhash(), 10); let ancestors = vec![(0, 0)].into_iter().collect(); let mut error_counters = TransactionErrorMetrics::default(); @@ -1017,8 +1004,7 @@ mod tests { &accounts.accounts_db, &ancestors, &[tx], - &[(Ok(()), None)], - &hash_queue, + &[(Ok(()), None, Some(10))], &mut error_counters, &rent_collector, &FeatureSet::all_enabled(),