From 6c38369042729d6949e09ad35ead11bda112af08 Mon Sep 17 00:00:00 2001 From: Tyera Eulberg Date: Tue, 21 Jul 2020 14:06:49 -0600 Subject: [PATCH] Use OrderedIterator in TransactionStatusService (#11149) * Split out get-first-err for unit testing * Add failing test * Add missing ordering --- core/src/banking_stage.rs | 1 + core/src/transaction_status_service.rs | 12 +-- ledger/src/blockstore_processor.rs | 108 ++++++++++++++++++++----- runtime/src/transaction_batch.rs | 5 ++ 4 files changed, 99 insertions(+), 27 deletions(-) diff --git a/core/src/banking_stage.rs b/core/src/banking_stage.rs index 1f6670c9a3..e62c0c2fd4 100644 --- a/core/src/banking_stage.rs +++ b/core/src/banking_stage.rs @@ -549,6 +549,7 @@ impl BankingStage { send_transaction_status_batch( bank.clone(), batch.transactions(), + batch.iteration_order_vec(), transaction_statuses, TransactionBalancesSet::new(pre_balances, post_balances), sender, diff --git a/core/src/transaction_status_service.rs b/core/src/transaction_status_service.rs index 92ea1de135..926adad150 100644 --- a/core/src/transaction_status_service.rs +++ b/core/src/transaction_status_service.rs @@ -3,6 +3,7 @@ use solana_ledger::{blockstore::Blockstore, blockstore_processor::TransactionSta use solana_runtime::{ bank::{Bank, HashAgeKind}, nonce_utils, + transaction_utils::OrderedIterator, }; use solana_transaction_status::TransactionStatusMeta; use std::{ @@ -50,16 +51,17 @@ impl TransactionStatusService { let TransactionStatusBatch { bank, transactions, + iteration_order, statuses, balances, } = write_transaction_status_receiver.recv_timeout(Duration::from_secs(1))?; let slot = bank.slot(); - for (((transaction, (status, hash_age_kind)), pre_balances), post_balances) in transactions - .iter() - .zip(statuses) - .zip(balances.pre_balances) - .zip(balances.post_balances) + for (((transaction, (status, hash_age_kind)), pre_balances), post_balances) in + OrderedIterator::new(&transactions, iteration_order.as_deref()) + .zip(statuses) + .zip(balances.pre_balances) + .zip(balances.post_balances) { if Bank::can_commit(&status) && !transaction.signatures.is_empty() { let fee_calculator = match hash_age_kind { diff --git a/ledger/src/blockstore_processor.rs b/ledger/src/blockstore_processor.rs index d1fe54a03e..d8e4b205f0 100644 --- a/ledger/src/blockstore_processor.rs +++ b/ledger/src/blockstore_processor.rs @@ -18,13 +18,14 @@ use solana_runtime::{ bank::{Bank, TransactionBalancesSet, TransactionProcessResult, TransactionResults}, bank_forks::BankForks, transaction_batch::TransactionBatch, + transaction_utils::OrderedIterator, }; use solana_sdk::{ clock::{Slot, MAX_PROCESSING_AGE}, genesis_config::GenesisConfig, hash::Hash, pubkey::Pubkey, - signature::Keypair, + signature::{Keypair, Signature}, timing::duration_as_ms, transaction::{Result, Transaction, TransactionError}, }; @@ -57,6 +58,37 @@ fn first_err(results: &[Result<()>]) -> Result<()> { Ok(()) } +// Includes transaction signature for unit-testing +fn get_first_error( + batch: &TransactionBatch, + fee_collection_results: Vec>, +) -> Option<(Result<()>, Signature)> { + let mut first_err = None; + for (result, transaction) in fee_collection_results.iter().zip(OrderedIterator::new( + batch.transactions(), + batch.iteration_order(), + )) { + if let Err(ref err) = result { + if first_err.is_none() { + first_err = Some((result.clone(), transaction.signatures[0])); + } + warn!( + "Unexpected validator error: {:?}, transaction: {:?}", + err, transaction + ); + datapoint_error!( + "validator_process_entry_error", + ( + "error", + format!("error: {:?}, transaction: {:?}", err, transaction), + String + ) + ); + } + } + first_err +} + fn execute_batch( batch: &TransactionBatch, bank: &Arc, @@ -78,33 +110,15 @@ fn execute_batch( send_transaction_status_batch( bank.clone(), batch.transactions(), + batch.iteration_order_vec(), processing_results, balances, sender, ); } - let mut first_err = None; - for (result, transaction) in fee_collection_results.iter().zip(batch.transactions()) { - if let Err(ref err) = result { - if first_err.is_none() { - first_err = Some(result.clone()); - } - warn!( - "Unexpected validator error: {:?}, transaction: {:?}", - err, transaction - ); - datapoint_error!( - "validator_process_entry_error", - ( - "error", - format!("error: {:?}, transaction: {:?}", err, transaction), - String - ) - ); - } - } - first_err.unwrap_or(Ok(())) + let first_err = get_first_error(batch, fee_collection_results); + first_err.map(|(result, _)| result).unwrap_or(Ok(())) } fn execute_batches( @@ -825,6 +839,7 @@ fn process_single_slot( pub struct TransactionStatusBatch { pub bank: Arc, pub transactions: Vec, + pub iteration_order: Option>, pub statuses: Vec, pub balances: TransactionBalancesSet, } @@ -833,6 +848,7 @@ pub type TransactionStatusSender = Sender; pub fn send_transaction_status_batch( bank: Arc, transactions: &[Transaction], + iteration_order: Option>, statuses: Vec, balances: TransactionBalancesSet, transaction_status_sender: TransactionStatusSender, @@ -841,6 +857,7 @@ pub fn send_transaction_status_batch( if let Err(e) = transaction_status_sender.send(TransactionStatusBatch { bank, transactions: transactions.to_vec(), + iteration_order, statuses, balances, }) { @@ -2635,4 +2652,51 @@ pub mod tests { } } } + + #[test] + fn test_get_first_error() { + let GenesisConfigInfo { + genesis_config, + mint_keypair, + .. + } = create_genesis_config(1_000_000_000); + let bank = Arc::new(Bank::new(&genesis_config)); + + let present_account_key = Keypair::new(); + let present_account = Account::new(1, 10, &Pubkey::default()); + bank.store_account(&present_account_key.pubkey(), &present_account); + + let keypair = Keypair::new(); + + // Create array of two transactions which throw different errors + let account_not_found_tx = + system_transaction::transfer(&keypair, &Pubkey::new_rand(), 42, bank.last_blockhash()); + let account_not_found_sig = account_not_found_tx.signatures[0]; + let mut account_loaded_twice = system_transaction::transfer( + &mint_keypair, + &Pubkey::new_rand(), + 42, + bank.last_blockhash(), + ); + account_loaded_twice.message.account_keys[1] = mint_keypair.pubkey(); + let transactions = [account_loaded_twice, account_not_found_tx]; + + // Use inverted iteration_order + let iteration_order: Vec = vec![1, 0]; + + let batch = bank.prepare_batch(&transactions, Some(iteration_order)); + let ( + TransactionResults { + fee_collection_results, + processing_results: _, + }, + _balances, + ) = batch + .bank() + .load_execute_and_commit_transactions(&batch, MAX_PROCESSING_AGE, false); + let (err, signature) = get_first_error(&batch, fee_collection_results).unwrap(); + // First error found should be for the 2nd transaction, due to iteration_order + assert_eq!(err.unwrap_err(), TransactionError::AccountNotFound); + assert_eq!(signature, account_not_found_sig); + } } diff --git a/runtime/src/transaction_batch.rs b/runtime/src/transaction_batch.rs index 9f03ad33fa..08abab3c39 100644 --- a/runtime/src/transaction_batch.rs +++ b/runtime/src/transaction_batch.rs @@ -41,6 +41,11 @@ impl<'a, 'b> TransactionBatch<'a, 'b> { pub fn iteration_order(&self) -> Option<&[usize]> { self.iteration_order.as_deref() } + + pub fn iteration_order_vec(&self) -> Option> { + self.iteration_order.clone() + } + pub fn bank(&self) -> &Bank { self.bank }