From 660e41a8e182faadec2861db74b86b8dfe1679f5 Mon Sep 17 00:00:00 2001 From: Andrew Fitzgerald Date: Mon, 2 Oct 2023 09:03:12 -0700 Subject: [PATCH] Remove entry shuffling (#33378) --- core/benches/banking_stage.rs | 17 ++----- ledger/src/blockstore_processor.rs | 72 ++++++++---------------------- rpc/src/rpc.rs | 1 - 3 files changed, 22 insertions(+), 68 deletions(-) diff --git a/core/benches/banking_stage.rs b/core/benches/banking_stage.rs index 6219f4abb..2526c2a63 100644 --- a/core/benches/banking_stage.rs +++ b/core/benches/banking_stage.rs @@ -390,7 +390,6 @@ fn bench_banking_stage_multi_programs_with_voting(bencher: &mut Bencher) { } fn simulate_process_entries( - randomize_txs: bool, mint_keypair: &Keypair, mut tx_vector: Vec, genesis_config: &GenesisConfig, @@ -423,10 +422,11 @@ fn simulate_process_entries( hash: next_hash(&bank.last_blockhash(), 1, &tx_vector), transactions: tx_vector, }; - process_entries_for_tests(&bank, vec![entry], randomize_txs, None, None).unwrap(); + process_entries_for_tests(&bank, vec![entry], None, None).unwrap(); } -fn bench_process_entries(randomize_txs: bool, bencher: &mut Bencher) { +#[bench] +fn bench_process_entries(bencher: &mut Bencher) { // entropy multiplier should be big enough to provide sufficient entropy // but small enough to not take too much time while executing the test. let entropy_multiplier: usize = 25; @@ -446,7 +446,6 @@ fn bench_process_entries(randomize_txs: bool, bencher: &mut Bencher) { bencher.iter(|| { simulate_process_entries( - randomize_txs, &mint_keypair, tx_vector.clone(), &genesis_config, @@ -456,13 +455,3 @@ fn bench_process_entries(randomize_txs: bool, bencher: &mut Bencher) { ); }); } - -#[bench] -fn bench_process_entries_without_order_shuffeling(bencher: &mut Bencher) { - bench_process_entries(false, bencher); -} - -#[bench] -fn bench_process_entries_with_order_shuffeling(bencher: &mut Bencher) { - bench_process_entries(true, bencher); -} diff --git a/ledger/src/blockstore_processor.rs b/ledger/src/blockstore_processor.rs index 488ada146..f5a883608 100644 --- a/ledger/src/blockstore_processor.rs +++ b/ledger/src/blockstore_processor.rs @@ -13,7 +13,6 @@ use { crossbeam_channel::Sender, itertools::Itertools, log::*, - rand::{seq::SliceRandom, thread_rng}, rayon::{prelude::*, ThreadPool}, scopeguard::defer, solana_accounts_db::{ @@ -421,7 +420,6 @@ fn execute_batches( pub fn process_entries_for_tests( bank: &Arc, entries: Vec, - randomize: bool, transaction_status_sender: Option<&TransactionStatusSender>, replay_vote_sender: Option<&ReplayVoteSender>, ) -> Result<()> { @@ -453,7 +451,6 @@ pub fn process_entries_for_tests( let result = process_entries( bank, &mut replay_entries, - randomize, transaction_status_sender, replay_vote_sender, &mut batch_timing, @@ -465,11 +462,9 @@ pub fn process_entries_for_tests( result } -// Note: If randomize is true this will shuffle entries' transactions in-place. fn process_entries( bank: &Arc, entries: &mut [ReplayEntry], - randomize: bool, transaction_status_sender: Option<&TransactionStatusSender>, replay_vote_sender: Option<&ReplayVoteSender>, batch_timing: &mut BatchExecutionTiming, @@ -479,7 +474,6 @@ fn process_entries( // accumulator for entries that can be processed in parallel let mut batches = vec![]; let mut tick_hashes = vec![]; - let mut rng = thread_rng(); for ReplayEntry { entry, @@ -511,18 +505,8 @@ fn process_entries( } EntryType::Transactions(transactions) => { let starting_index = *starting_index; - let transaction_indexes = if randomize { - let mut transactions_and_indexes: Vec<(SanitizedTransaction, usize)> = - transactions.drain(..).zip(starting_index..).collect(); - transactions_and_indexes.shuffle(&mut rng); - let (txs, indexes): (Vec<_>, Vec<_>) = - transactions_and_indexes.into_iter().unzip(); - *transactions = txs; - indexes - } else { - (starting_index..starting_index.saturating_add(transactions.len())).collect() - }; - + let transaction_indexes = + (starting_index..starting_index.saturating_add(transactions.len())).collect(); loop { // try to lock the accounts let batch = bank.prepare_sanitized_batch(transactions); @@ -1249,11 +1233,9 @@ fn confirm_slot_entries( starting_index: tx_starting_index, }) .collect(); - // Note: This will shuffle entries' transactions in-place. let process_result = process_entries( bank, &mut replay_entries, - true, // shuffle transactions. transaction_status_sender, replay_vote_sender, batch_execute_timing, @@ -2614,7 +2596,7 @@ pub mod tests { ); // Now ensure the TX is accepted despite pointing to the ID of an empty entry. - process_entries_for_tests(&bank, slot_entries, true, None, None).unwrap(); + process_entries_for_tests(&bank, slot_entries, None, None).unwrap(); assert_eq!(bank.process_transaction(&tx), Ok(())); } @@ -2749,7 +2731,7 @@ pub mod tests { assert_eq!(bank.tick_height(), 0); let tick = next_entry(&genesis_config.hash(), 1, vec![]); assert_eq!( - process_entries_for_tests(&bank, vec![tick], true, None, None), + process_entries_for_tests(&bank, vec![tick], None, None), Ok(()) ); assert_eq!(bank.tick_height(), 1); @@ -2784,7 +2766,7 @@ pub mod tests { ); let entry_2 = next_entry(&entry_1.hash, 1, vec![tx]); assert_eq!( - process_entries_for_tests(&bank, vec![entry_1, entry_2], true, None, None), + process_entries_for_tests(&bank, vec![entry_1, entry_2], None, None), Ok(()) ); assert_eq!(bank.get_balance(&keypair1.pubkey()), 2); @@ -2843,7 +2825,6 @@ pub mod tests { process_entries_for_tests( &bank, vec![entry_1_to_mint, entry_2_to_3_mint_to_1], - false, None, None, ), @@ -2915,7 +2896,6 @@ pub mod tests { assert!(process_entries_for_tests( &bank, vec![entry_1_to_mint.clone(), entry_2_to_3_mint_to_1.clone()], - false, None, None, ) @@ -3031,7 +3011,7 @@ pub mod tests { let entry = next_entry(&bank.last_blockhash(), 1, vec![tx]); let bank = Arc::new(bank); - let result = process_entries_for_tests(&bank, vec![entry], false, None, None); + let result = process_entries_for_tests(&bank, vec![entry], None, None); bank.freeze(); let blockhash_ok = bank.last_blockhash(); let bankhash_ok = bank.hash(); @@ -3072,7 +3052,7 @@ pub mod tests { let entry = next_entry(&bank.last_blockhash(), 1, vec![tx]); let bank = Arc::new(bank); - let _result = process_entries_for_tests(&bank, vec![entry], false, None, None); + let _result = process_entries_for_tests(&bank, vec![entry], None, None); bank.freeze(); assert_eq!(blockhash_ok, bank.last_blockhash()); @@ -3171,7 +3151,6 @@ pub mod tests { entry_2_to_3_and_1_to_mint, entry_conflict_itself, ], - false, None, None, ) @@ -3221,7 +3200,7 @@ pub mod tests { system_transaction::transfer(&keypair2, &keypair4.pubkey(), 1, bank.last_blockhash()); let entry_2 = next_entry(&entry_1.hash, 1, vec![tx]); assert_eq!( - process_entries_for_tests(&bank, vec![entry_1, entry_2], true, None, None), + process_entries_for_tests(&bank, vec![entry_1, entry_2], None, None), Ok(()) ); assert_eq!(bank.get_balance(&keypair3.pubkey()), 1); @@ -3282,7 +3261,7 @@ pub mod tests { }) .collect(); assert_eq!( - process_entries_for_tests(&bank, entries, true, None, None), + process_entries_for_tests(&bank, entries, None, None), Ok(()) ); } @@ -3345,7 +3324,7 @@ pub mod tests { // Transfer lamports to each other let entry = next_entry(&bank.last_blockhash(), 1, tx_vector); assert_eq!( - process_entries_for_tests(&bank, vec![entry], true, None, None), + process_entries_for_tests(&bank, vec![entry], None, None), Ok(()) ); bank.squash(); @@ -3405,13 +3384,7 @@ pub mod tests { system_transaction::transfer(&keypair1, &keypair4.pubkey(), 1, bank.last_blockhash()); let entry_2 = next_entry(&tick.hash, 1, vec![tx]); assert_eq!( - process_entries_for_tests( - &bank, - vec![entry_1, tick, entry_2.clone()], - true, - None, - None, - ), + process_entries_for_tests(&bank, vec![entry_1, tick, entry_2.clone()], None, None,), Ok(()) ); assert_eq!(bank.get_balance(&keypair3.pubkey()), 1); @@ -3422,7 +3395,7 @@ pub mod tests { system_transaction::transfer(&keypair2, &keypair3.pubkey(), 1, bank.last_blockhash()); let entry_3 = next_entry(&entry_2.hash, 1, vec![tx]); assert_eq!( - process_entries_for_tests(&bank, vec![entry_3], true, None, None), + process_entries_for_tests(&bank, vec![entry_3], None, None), Err(TransactionError::AccountNotFound) ); } @@ -3502,7 +3475,7 @@ pub mod tests { ); assert_eq!( - process_entries_for_tests(&bank, vec![entry_1_to_mint], false, None, None), + process_entries_for_tests(&bank, vec![entry_1_to_mint], None, None), Err(TransactionError::AccountInUse) ); @@ -3705,7 +3678,7 @@ pub mod tests { }) .collect(); info!("paying iteration {}", i); - process_entries_for_tests(&bank, entries, true, None, None).expect("paying failed"); + process_entries_for_tests(&bank, entries, None, None).expect("paying failed"); let entries: Vec<_> = (0..NUM_TRANSFERS) .step_by(NUM_TRANSFERS_PER_ENTRY) @@ -3728,7 +3701,7 @@ pub mod tests { .collect(); info!("refunding iteration {}", i); - process_entries_for_tests(&bank, entries, true, None, None).expect("refunding failed"); + process_entries_for_tests(&bank, entries, None, None).expect("refunding failed"); // advance to next block process_entries_for_tests( @@ -3736,7 +3709,6 @@ pub mod tests { (0..bank.ticks_per_slot()) .map(|_| next_entry_mut(&mut hash, 1, vec![])) .collect::>(), - true, None, None, ) @@ -3778,7 +3750,7 @@ pub mod tests { let entry = next_entry(&new_blockhash, 1, vec![tx]); entries.push(entry); - process_entries_for_tests(&bank0, entries, true, None, None).unwrap(); + process_entries_for_tests(&bank0, entries, None, None).unwrap(); assert_eq!(bank0.get_balance(&keypair.pubkey()), 1) } @@ -3944,8 +3916,7 @@ pub mod tests { .collect(); let entry = next_entry(&bank_1_blockhash, 1, vote_txs); let (replay_vote_sender, replay_vote_receiver) = crossbeam_channel::unbounded(); - let _ = - process_entries_for_tests(&bank1, vec![entry], true, None, Some(&replay_vote_sender)); + let _ = process_entries_for_tests(&bank1, vec![entry], None, Some(&replay_vote_sender)); let successes: BTreeSet = replay_vote_receiver .try_iter() .map(|(vote_pubkey, ..)| vote_pubkey) @@ -4309,9 +4280,7 @@ pub mod tests { if let TransactionStatusMessage::Batch(batch) = batch { assert_eq!(batch.transactions.len(), 2); assert_eq!(batch.transaction_indexes.len(), 2); - // Assert contains instead of the actual vec due to randomize - assert!(batch.transaction_indexes.contains(&0)); - assert!(batch.transaction_indexes.contains(&1)); + assert_eq!(batch.transaction_indexes, [0, 1]); } else { panic!("batch should have been sent"); } @@ -4355,10 +4324,7 @@ pub mod tests { if let TransactionStatusMessage::Batch(batch) = batch { assert_eq!(batch.transactions.len(), 3); assert_eq!(batch.transaction_indexes.len(), 3); - // Assert contains instead of the actual vec due to randomize - assert!(batch.transaction_indexes.contains(&2)); - assert!(batch.transaction_indexes.contains(&3)); - assert!(batch.transaction_indexes.contains(&4)); + assert_eq!(batch.transaction_indexes, [2, 3, 4]); } else { panic!("batch should have been sent"); } diff --git a/rpc/src/rpc.rs b/rpc/src/rpc.rs index d37a6f882..7ff2ffa42 100644 --- a/rpc/src/rpc.rs +++ b/rpc/src/rpc.rs @@ -4599,7 +4599,6 @@ pub fn populate_blockstore_for_tests( solana_ledger::blockstore_processor::process_entries_for_tests( &bank, entries, - true, Some( &solana_ledger::blockstore_processor::TransactionStatusSender { sender: transaction_status_sender,