From 29c3bc5523b8a365f27ee85beee77d651822c5c1 Mon Sep 17 00:00:00 2001 From: hana <81144685+2501babe@users.noreply.github.com> Date: Mon, 6 Jan 2025 02:39:33 -0800 Subject: [PATCH 01/10] accounts-db: relax intrabatch account locks --- Cargo.lock | 2 + accounts-db/Cargo.toml | 2 + accounts-db/benches/bench_lock_accounts.rs | 20 +- accounts-db/src/account_locks.rs | 100 +++++--- accounts-db/src/accounts.rs | 260 ++++++++++++++++----- core/src/banking_stage/consume_worker.rs | 39 +++- core/src/banking_stage/consumer.rs | 65 +++++- feature-set/src/lib.rs | 2 +- ledger/src/blockstore_processor.rs | 75 ++++-- runtime/src/bank.rs | 88 ++++--- runtime/src/bank/tests.rs | 53 +++-- runtime/src/transaction_batch.rs | 51 ++-- 12 files changed, 564 insertions(+), 193 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 0d6f5731373553..ac5400930dc2ba 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6624,6 +6624,7 @@ dependencies = [ name = "solana-accounts-db" version = "2.3.0" dependencies = [ + "agave-reserved-account-keys", "ahash 0.8.11", "assert_matches", "bincode", @@ -6682,6 +6683,7 @@ dependencies = [ "solana-reward-info", "solana-sdk-ids", "solana-sha256-hasher", + "solana-signature", "solana-signer", "solana-slot-hashes", "solana-slot-history", diff --git a/accounts-db/Cargo.toml b/accounts-db/Cargo.toml index 546bdf3e6a009e..e76bf6f41ff8e5 100644 --- a/accounts-db/Cargo.toml +++ b/accounts-db/Cargo.toml @@ -86,6 +86,7 @@ crate-type = ["lib"] name = "solana_accounts_db" [dev-dependencies] +agave-reserved-account-keys = { workspace = true } assert_matches = { workspace = true } criterion = { workspace = true } libsecp256k1 = { workspace = true } @@ -98,6 +99,7 @@ solana-compute-budget = { workspace = true } solana-instruction = { workspace = true } solana-logger = { workspace = true } solana-sdk-ids = { workspace = true } +solana-signature = { workspace = true, features = ["rand"] } solana-slot-history = { workspace = true } static_assertions = { workspace = true } strum = { workspace = true, features = ["derive"] } diff --git a/accounts-db/benches/bench_lock_accounts.rs b/accounts-db/benches/bench_lock_accounts.rs index 00d4805f54de13..89f5b418767b98 100644 --- a/accounts-db/benches/bench_lock_accounts.rs +++ b/accounts-db/benches/bench_lock_accounts.rs @@ -62,16 +62,21 @@ fn create_test_transactions(lock_count: usize, read_conflicts: bool) -> Vec, + write_locks: AHashMap, readonly_locks: AHashMap, } @@ -20,10 +20,61 @@ impl AccountLocks { /// The bool in the tuple indicates if the account is writable. /// Returns an error if any of the accounts are already locked in a way /// that conflicts with the requested lock. + /// NOTE this is the pre-SIMD83 logic and can be removed once SIMD83 is active. pub fn try_lock_accounts<'a>( &mut self, keys: impl Iterator + Clone, - ) -> Result<(), TransactionError> { + ) -> TransactionResult<()> { + self.can_lock_accounts(keys.clone())?; + self.lock_accounts(keys); + + Ok(()) + } + + /// Lock accounts for all transactions in a batch which don't conflict + /// with existing locks. Returns a vector of `TransactionResult` indicating + /// success or failure for each transaction in the batch. + /// NOTE this is the SIMD83 logic; after the feature is active, it becomes + /// the only logic, and this note can be removed with the feature gate. + pub fn try_lock_transaction_batch<'a>( + &mut self, + validated_batch_keys: impl Iterator< + Item = TransactionResult + Clone>, + >, + ) -> Vec> { + let available_batch_keys: Vec<_> = validated_batch_keys + .map(|validated_keys| { + validated_keys + .clone() + .and_then(|keys| self.can_lock_accounts(keys)) + .and(validated_keys) + }) + .collect(); + + available_batch_keys + .into_iter() + .map(|available_keys| available_keys.map(|keys| self.lock_accounts(keys))) + .collect() + } + + /// Unlock the account keys in `keys` after a transaction. + /// The bool in the tuple indicates if the account is writable. + /// In debug-mode this function will panic if an attempt is made to unlock + /// an account that wasn't locked in the way requested. + pub fn unlock_accounts<'a>(&mut self, keys: impl Iterator) { + for (k, writable) in keys { + if writable { + self.unlock_write(k); + } else { + self.unlock_readonly(k); + } + } + } + + fn can_lock_accounts<'a>( + &self, + keys: impl Iterator + Clone, + ) -> TransactionResult<()> { for (key, writable) in keys.clone() { if writable { if !self.can_write_lock(key) { @@ -34,27 +85,15 @@ impl AccountLocks { } } - for (key, writable) in keys { - if writable { - self.lock_write(key); - } else { - self.lock_readonly(key); - } - } - Ok(()) } - /// Unlock the account keys in `keys` after a transaction. - /// The bool in the tuple indicates if the account is writable. - /// In debug-mode this function will panic if an attempt is made to unlock - /// an account that wasn't locked in the way requested. - pub fn unlock_accounts<'a>(&mut self, keys: impl Iterator) { - for (k, writable) in keys { + fn lock_accounts<'a>(&mut self, keys: impl Iterator + Clone) { + for (key, writable) in keys { if writable { - self.unlock_write(k); + self.lock_write(key); } else { - self.unlock_readonly(k); + self.lock_readonly(key); } } } @@ -66,7 +105,7 @@ impl AccountLocks { #[cfg_attr(feature = "dev-context-only-utils", qualifiers(pub))] fn is_locked_write(&self, key: &Pubkey) -> bool { - self.write_locks.contains(key) + self.write_locks.get(key).is_some_and(|count| *count > 0) } fn can_read_lock(&self, key: &Pubkey) -> bool { @@ -84,7 +123,7 @@ impl AccountLocks { } fn lock_write(&mut self, key: &Pubkey) { - self.write_locks.insert(*key); + *self.write_locks.entry(*key).or_default() += 1; } fn unlock_readonly(&mut self, key: &Pubkey) { @@ -103,11 +142,18 @@ impl AccountLocks { } fn unlock_write(&mut self, key: &Pubkey) { - let removed = self.write_locks.remove(key); - debug_assert!( - removed, - "Attempted to remove a write-lock for a key that wasn't write-locked" - ); + if let hash_map::Entry::Occupied(mut occupied_entry) = self.write_locks.entry(*key) { + let count = occupied_entry.get_mut(); + *count -= 1; + if *count == 0 { + occupied_entry.remove_entry(); + } + } else { + debug_assert!( + false, + "Attempted to remove a write-lock for a key that wasn't write-locked" + ); + } } } @@ -115,7 +161,7 @@ impl AccountLocks { pub fn validate_account_locks( account_keys: AccountKeys, tx_account_lock_limit: usize, -) -> Result<(), TransactionError> { +) -> TransactionResult<()> { if account_keys.len() > tx_account_lock_limit { Err(TransactionError::TooManyAccountLocks) } else if has_duplicates(account_keys) { diff --git a/accounts-db/src/accounts.rs b/accounts-db/src/accounts.rs index 01c9d75468e10a..76feb7921314ef 100644 --- a/accounts-db/src/accounts.rs +++ b/accounts-db/src/accounts.rs @@ -572,51 +572,60 @@ impl Accounts { &self, txs: impl Iterator, tx_account_lock_limit: usize, + relax_intrabatch_account_locks: bool, ) -> Vec> { // Validate the account locks, then get iterator if successful validation. - let tx_account_locks_results: Vec> = txs - .map(|tx| { - validate_account_locks(tx.account_keys(), tx_account_lock_limit) - .map(|_| TransactionAccountLocksIterator::new(tx)) - }) - .collect(); - self.lock_accounts_inner(tx_account_locks_results) + let tx_account_locks_results = txs.map(|tx| { + validate_account_locks(tx.account_keys(), tx_account_lock_limit) + .map(|_| TransactionAccountLocksIterator::new(tx)) + }); + self.lock_accounts_inner(tx_account_locks_results, relax_intrabatch_account_locks) } + /// This function will prevent multiple threads from modifying the same account state at the + /// same time, possibly excluding transactions based on prior results #[must_use] pub fn lock_accounts_with_results<'a>( &self, txs: impl Iterator, results: impl Iterator>, tx_account_lock_limit: usize, + relax_intrabatch_account_locks: bool, ) -> Vec> { // Validate the account locks, then get iterator if successful validation. - let tx_account_locks_results: Vec> = txs - .zip(results) - .map(|(tx, result)| match result { - Ok(()) => validate_account_locks(tx.account_keys(), tx_account_lock_limit) - .map(|_| TransactionAccountLocksIterator::new(tx)), - Err(err) => Err(err), - }) - .collect(); - self.lock_accounts_inner(tx_account_locks_results) + let tx_account_locks_results = txs.zip(results).map(|(tx, result)| match result { + Ok(()) => validate_account_locks(tx.account_keys(), tx_account_lock_limit) + .map(|_| TransactionAccountLocksIterator::new(tx)), + Err(err) => Err(err), + }); + self.lock_accounts_inner(tx_account_locks_results, relax_intrabatch_account_locks) } #[must_use] - fn lock_accounts_inner( + fn lock_accounts_inner<'a>( &self, - tx_account_locks_results: Vec>>, + tx_account_locks_results: impl Iterator< + Item = Result>, + >, + relax_intrabatch_account_locks: bool, ) -> Vec> { let account_locks = &mut self.account_locks.lock().unwrap(); - tx_account_locks_results - .into_iter() - .map(|tx_account_locks_result| match tx_account_locks_result { - Ok(tx_account_locks) => { - account_locks.try_lock_accounts(tx_account_locks.accounts_with_is_writable()) - } - Err(err) => Err(err), - }) - .collect() + if relax_intrabatch_account_locks { + let validated_batch_keys = tx_account_locks_results.map(|tx_account_locks_result| { + tx_account_locks_result + .map(|tx_account_locks| tx_account_locks.accounts_with_is_writable()) + }); + + account_locks.try_lock_transaction_batch(validated_batch_keys) + } else { + tx_account_locks_results + .map(|tx_account_locks_result| match tx_account_locks_result { + Ok(tx_account_locks) => account_locks + .try_lock_accounts(tx_account_locks.accounts_with_is_writable()), + Err(err) => Err(err), + }) + .collect() + } } /// Once accounts are unlocked, new transactions that modify that state can enter the pipeline @@ -662,15 +671,18 @@ impl Accounts { mod tests { use { super::*, + agave_reserved_account_keys::ReservedAccountKeys, solana_account::{AccountSharedData, WritableAccount}, solana_address_lookup_table_interface::state::LookupTableMeta, solana_hash::Hash, + solana_instruction::{AccountMeta, Instruction}, solana_keypair::Keypair, solana_message::{ - compiled_instruction::CompiledInstruction, v0::MessageAddressTableLookup, Message, - MessageHeader, + compiled_instruction::CompiledInstruction, v0::MessageAddressTableLookup, + LegacyMessage, Message, MessageHeader, SanitizedMessage, }, solana_sdk_ids::native_loader, + solana_signature::Signature, solana_signer::{signers::Signers, Signer}, solana_transaction::{sanitized::MAX_TX_ACCOUNT_LOCKS, Transaction}, solana_transaction_error::TransactionError, @@ -680,6 +692,7 @@ mod tests { sync::atomic::{AtomicBool, AtomicU64, Ordering}, thread, time, }, + test_case::test_case, }; fn new_sanitized_tx( @@ -694,6 +707,23 @@ mod tests { )) } + fn sanitized_tx_from_metas(accounts: Vec) -> SanitizedTransaction { + let instruction = Instruction { + accounts, + program_id: Pubkey::default(), + data: vec![], + }; + + let message = Message::new(&[instruction], None); + + let sanitized_message = SanitizedMessage::Legacy(LegacyMessage::new( + message, + &ReservedAccountKeys::empty_key_set(), + )); + + SanitizedTransaction::new_for_tests(sanitized_message, vec![Signature::new_unique()], false) + } + #[test] fn test_hold_range_in_memory() { let accounts_db = AccountsDb::default_for_tests(); @@ -895,8 +925,9 @@ mod tests { assert_eq!(loaded, vec![]); } - #[test] - fn test_lock_accounts_with_duplicates() { + #[test_case(false; "old")] + #[test_case(true; "simd83")] + fn test_lock_accounts_with_duplicates(relax_intrabatch_account_locks: bool) { let accounts_db = AccountsDb::new_single_for_tests(); let accounts = Accounts::new(Arc::new(accounts_db)); @@ -911,12 +942,17 @@ mod tests { }; let tx = new_sanitized_tx(&[&keypair], message, Hash::default()); - let results = accounts.lock_accounts([tx].iter(), MAX_TX_ACCOUNT_LOCKS); + let results = accounts.lock_accounts( + [tx].iter(), + MAX_TX_ACCOUNT_LOCKS, + relax_intrabatch_account_locks, + ); assert_eq!(results[0], Err(TransactionError::AccountLoadedTwice)); } - #[test] - fn test_lock_accounts_with_too_many_accounts() { + #[test_case(false; "old")] + #[test_case(true; "simd83")] + fn test_lock_accounts_with_too_many_accounts(relax_intrabatch_account_locks: bool) { let accounts_db = AccountsDb::new_single_for_tests(); let accounts = Accounts::new(Arc::new(accounts_db)); @@ -939,7 +975,11 @@ mod tests { }; let txs = vec![new_sanitized_tx(&[&keypair], message, Hash::default())]; - let results = accounts.lock_accounts(txs.iter(), MAX_TX_ACCOUNT_LOCKS); + let results = accounts.lock_accounts( + txs.iter(), + MAX_TX_ACCOUNT_LOCKS, + relax_intrabatch_account_locks, + ); assert_eq!(results, vec![Ok(())]); accounts.unlock_accounts(txs.iter().zip(&results)); } @@ -961,13 +1001,18 @@ mod tests { }; let txs = vec![new_sanitized_tx(&[&keypair], message, Hash::default())]; - let results = accounts.lock_accounts(txs.iter(), MAX_TX_ACCOUNT_LOCKS); + let results = accounts.lock_accounts( + txs.iter(), + MAX_TX_ACCOUNT_LOCKS, + relax_intrabatch_account_locks, + ); assert_eq!(results[0], Err(TransactionError::TooManyAccountLocks)); } } - #[test] - fn test_accounts_locks() { + #[test_case(false; "old")] + #[test_case(true; "simd83")] + fn test_accounts_locks(relax_intrabatch_account_locks: bool) { let keypair0 = Keypair::new(); let keypair1 = Keypair::new(); let keypair2 = Keypair::new(); @@ -995,7 +1040,11 @@ mod tests { instructions, ); let tx = new_sanitized_tx(&[&keypair0], message, Hash::default()); - let results0 = accounts.lock_accounts([tx.clone()].iter(), MAX_TX_ACCOUNT_LOCKS); + let results0 = accounts.lock_accounts( + [tx.clone()].iter(), + MAX_TX_ACCOUNT_LOCKS, + relax_intrabatch_account_locks, + ); assert_eq!(results0, vec![Ok(())]); assert!(accounts @@ -1025,7 +1074,11 @@ mod tests { ); let tx1 = new_sanitized_tx(&[&keypair1], message, Hash::default()); let txs = vec![tx0, tx1]; - let results1 = accounts.lock_accounts(txs.iter(), MAX_TX_ACCOUNT_LOCKS); + let results1 = accounts.lock_accounts( + txs.iter(), + MAX_TX_ACCOUNT_LOCKS, + relax_intrabatch_account_locks, + ); assert_eq!( results1, vec![ @@ -1051,7 +1104,11 @@ mod tests { instructions, ); let tx = new_sanitized_tx(&[&keypair1], message, Hash::default()); - let results2 = accounts.lock_accounts([tx].iter(), MAX_TX_ACCOUNT_LOCKS); + let results2 = accounts.lock_accounts( + [tx].iter(), + MAX_TX_ACCOUNT_LOCKS, + relax_intrabatch_account_locks, + ); assert_eq!( results2, vec![Ok(())] // Now keypair1 account can be locked as writable @@ -1065,8 +1122,9 @@ mod tests { .is_locked_readonly(&keypair1.pubkey())); } - #[test] - fn test_accounts_locks_multithreaded() { + #[test_case(false; "old")] + #[test_case(true; "simd83")] + fn test_accounts_locks_multithreaded(relax_intrabatch_account_locks: bool) { let counter = Arc::new(AtomicU64::new(0)); let exit = Arc::new(AtomicBool::new(false)); @@ -1113,9 +1171,11 @@ mod tests { let exit_clone = exit.clone(); thread::spawn(move || loop { let txs = vec![writable_tx.clone()]; - let results = accounts_clone - .clone() - .lock_accounts(txs.iter(), MAX_TX_ACCOUNT_LOCKS); + let results = accounts_clone.clone().lock_accounts( + txs.iter(), + MAX_TX_ACCOUNT_LOCKS, + relax_intrabatch_account_locks, + ); for result in results.iter() { if result.is_ok() { counter_clone.clone().fetch_add(1, Ordering::Release); @@ -1129,9 +1189,11 @@ mod tests { let counter_clone = counter; for _ in 0..5 { let txs = vec![readonly_tx.clone()]; - let results = accounts_arc - .clone() - .lock_accounts(txs.iter(), MAX_TX_ACCOUNT_LOCKS); + let results = accounts_arc.clone().lock_accounts( + txs.iter(), + MAX_TX_ACCOUNT_LOCKS, + relax_intrabatch_account_locks, + ); if results[0].is_ok() { let counter_value = counter_clone.clone().load(Ordering::Acquire); thread::sleep(time::Duration::from_millis(50)); @@ -1143,8 +1205,9 @@ mod tests { exit.store(true, Ordering::Relaxed); } - #[test] - fn test_demote_program_write_locks() { + #[test_case(false; "old")] + #[test_case(true; "simd83")] + fn test_demote_program_write_locks(relax_intrabatch_account_locks: bool) { let keypair0 = Keypair::new(); let keypair1 = Keypair::new(); let keypair2 = Keypair::new(); @@ -1172,7 +1235,11 @@ mod tests { instructions, ); let tx = new_sanitized_tx(&[&keypair0], message, Hash::default()); - let results0 = accounts.lock_accounts([tx].iter(), MAX_TX_ACCOUNT_LOCKS); + let results0 = accounts.lock_accounts( + [tx].iter(), + MAX_TX_ACCOUNT_LOCKS, + relax_intrabatch_account_locks, + ); assert!(results0[0].is_ok()); // Instruction program-id account demoted to readonly @@ -1208,8 +1275,9 @@ mod tests { } } - #[test] - fn test_accounts_locks_with_results() { + #[test_case(false; "old")] + #[test_case(true; "simd83")] + fn test_accounts_locks_with_results(relax_intrabatch_account_locks: bool) { let keypair0 = Keypair::new(); let keypair1 = Keypair::new(); let keypair2 = Keypair::new(); @@ -1269,6 +1337,7 @@ mod tests { txs.iter(), qos_results.into_iter(), MAX_TX_ACCOUNT_LOCKS, + relax_intrabatch_account_locks, ); assert_eq!( @@ -1294,6 +1363,89 @@ mod tests { .is_locked_write(&keypair2.pubkey())); } + #[test_case(false; "old")] + #[test_case(true; "simd83")] + fn test_accounts_locks_intrabatch_conflicts(relax_intrabatch_account_locks: bool) { + let pubkey = Pubkey::new_unique(); + let account_data = AccountSharedData::new(1, 0, &Pubkey::default()); + let accounts_db = Arc::new(AccountsDb::new_single_for_tests()); + accounts_db.store_for_tests( + 0, + &[ + (&Pubkey::default(), &account_data), + (&pubkey, &account_data), + ], + ); + + let r_tx = sanitized_tx_from_metas(vec![AccountMeta { + pubkey, + is_writable: false, + is_signer: false, + }]); + + let w_tx = sanitized_tx_from_metas(vec![AccountMeta { + pubkey, + is_writable: true, + is_signer: false, + }]); + + // one w tx alone always works + let accounts = Accounts::new(accounts_db.clone()); + let results = accounts.lock_accounts( + [w_tx.clone()].iter(), + MAX_TX_ACCOUNT_LOCKS, + relax_intrabatch_account_locks, + ); + + assert_eq!(results, vec![Ok(())]); + + // wr conflict cross-batch always fails + let results = accounts.lock_accounts( + [r_tx.clone()].iter(), + MAX_TX_ACCOUNT_LOCKS, + relax_intrabatch_account_locks, + ); + + assert_eq!(results, vec![Err(TransactionError::AccountInUse)]); + + // ww conflict cross-batch always fails + let results = accounts.lock_accounts( + [w_tx.clone()].iter(), + MAX_TX_ACCOUNT_LOCKS, + relax_intrabatch_account_locks, + ); + + assert_eq!(results, vec![Err(TransactionError::AccountInUse)]); + + // wr conflict in-batch succeeds or fails based on feature + let accounts = Accounts::new(accounts_db.clone()); + let results = accounts.lock_accounts( + [w_tx.clone(), r_tx.clone()].iter(), + MAX_TX_ACCOUNT_LOCKS, + relax_intrabatch_account_locks, + ); + + if relax_intrabatch_account_locks { + assert_eq!(results, vec![Ok(()), Ok(())]); + } else { + assert_eq!(results, vec![Ok(()), Err(TransactionError::AccountInUse)]); + } + + // ww conflict in-batch succeeds or fails based on feature + let accounts = Accounts::new(accounts_db.clone()); + let results = accounts.lock_accounts( + [w_tx.clone(), r_tx.clone()].iter(), + MAX_TX_ACCOUNT_LOCKS, + relax_intrabatch_account_locks, + ); + + if relax_intrabatch_account_locks { + assert_eq!(results, vec![Ok(()), Ok(())]); + } else { + assert_eq!(results, vec![Ok(()), Err(TransactionError::AccountInUse)]); + } + } + #[test] fn huge_clean() { solana_logger::setup(); diff --git a/core/src/banking_stage/consume_worker.rs b/core/src/banking_stage/consume_worker.rs index 461f644e6edd1d..575d0ccd9b30b5 100644 --- a/core/src/banking_stage/consume_worker.rs +++ b/core/src/banking_stage/consume_worker.rs @@ -768,6 +768,7 @@ mod tests { thread::JoinHandle, }, tempfile::TempDir, + test_case::test_case, }; // Helper struct to create tests that hold channels, files, etc. @@ -787,7 +788,9 @@ mod tests { consumed_receiver: Receiver>>, } - fn setup_test_frame() -> ( + fn setup_test_frame( + relax_intrabatch_account_locks: bool, + ) -> ( TestFrame, ConsumeWorker>, ) { @@ -798,11 +801,15 @@ mod tests { } = create_slow_genesis_config(10_000); let (bank, bank_forks) = Bank::new_no_wallclock_throttle_for_tests(&genesis_config); // Warp to next epoch for MaxAge tests. - let bank = Arc::new(Bank::new_from_parent( + let mut bank = Bank::new_from_parent( bank.clone(), &Pubkey::new_unique(), bank.get_epoch_info().slots_in_epoch, - )); + ); + if !relax_intrabatch_account_locks { + bank.deactivate_feature(&agave_feature_set::relax_intrabatch_account_locks::id()); + } + let bank = Arc::new(bank); let ledger_path = get_tmp_ledger_path_auto_delete!(); let blockstore = Blockstore::open(ledger_path.path()) @@ -861,7 +868,7 @@ mod tests { #[test] fn test_worker_consume_no_bank() { - let (test_frame, worker) = setup_test_frame(); + let (test_frame, worker) = setup_test_frame(true); let TestFrame { mint_keypair, genesis_config, @@ -905,7 +912,7 @@ mod tests { #[test] fn test_worker_consume_simple() { - let (test_frame, worker) = setup_test_frame(); + let (test_frame, worker) = setup_test_frame(true); let TestFrame { mint_keypair, genesis_config, @@ -952,9 +959,10 @@ mod tests { let _ = worker_thread.join().unwrap(); } - #[test] - fn test_worker_consume_self_conflicting() { - let (test_frame, worker) = setup_test_frame(); + #[test_case(false; "old")] + #[test_case(true; "simd83")] + fn test_worker_consume_self_conflicting(relax_intrabatch_account_locks: bool) { + let (test_frame, worker) = setup_test_frame(relax_intrabatch_account_locks); let TestFrame { mint_keypair, genesis_config, @@ -998,7 +1006,16 @@ mod tests { assert_eq!(consumed.work.batch_id, bid); assert_eq!(consumed.work.ids, vec![id1, id2]); assert_eq!(consumed.work.max_ages, vec![max_age, max_age]); - assert_eq!(consumed.retryable_indexes, vec![1]); // id2 is retryable since lock conflict + + // id2 succeeds with simd83, or is retryable due to lock conflict without simd83 + assert_eq!( + consumed.retryable_indexes, + if relax_intrabatch_account_locks { + vec![] + } else { + vec![1] + } + ); drop(test_frame); let _ = worker_thread.join().unwrap(); @@ -1006,7 +1023,7 @@ mod tests { #[test] fn test_worker_consume_multiple_messages() { - let (test_frame, worker) = setup_test_frame(); + let (test_frame, worker) = setup_test_frame(true); let TestFrame { mint_keypair, genesis_config, @@ -1081,7 +1098,7 @@ mod tests { #[test] fn test_worker_ttl() { - let (test_frame, worker) = setup_test_frame(); + let (test_frame, worker) = setup_test_frame(true); let TestFrame { mint_keypair, genesis_config, diff --git a/core/src/banking_stage/consumer.rs b/core/src/banking_stage/consumer.rs index d466ac47b7e09a..c5b106726233ad 100644 --- a/core/src/banking_stage/consumer.rs +++ b/core/src/banking_stage/consumer.rs @@ -531,6 +531,7 @@ mod tests { thread::{Builder, JoinHandle}, time::Duration, }, + test_case::test_case, }; fn execute_transactions_with_dummy_poh_service( @@ -1007,8 +1008,11 @@ mod tests { let _ = poh_simulator.join(); } - #[test] - fn test_bank_process_and_record_transactions_cost_tracker() { + #[test_case(false; "old")] + #[test_case(true; "simd83")] + fn test_bank_process_and_record_transactions_cost_tracker( + relax_intrabatch_account_locks: bool, + ) { solana_logger::setup(); let GenesisConfigInfo { genesis_config, @@ -1016,6 +1020,9 @@ mod tests { .. } = create_slow_genesis_config(10_000); let mut bank = Bank::new_for_tests(&genesis_config); + if !relax_intrabatch_account_locks { + bank.deactivate_feature(&agave_feature_set::relax_intrabatch_account_locks::id()); + } bank.ns_per_slot = u128::MAX; let (bank, _bank_forks) = bank.wrap_with_bank_forks_for_tests(); let pubkey = solana_pubkey::new_rand(); @@ -1098,6 +1105,14 @@ mod tests { system_transaction::transfer(&mint_keypair, &pubkey, 2, genesis_config.hash()), ]); + let conflicting_transaction = sanitize_transactions(vec![system_transaction::transfer( + &Keypair::new(), + &pubkey, + 1, + genesis_config.hash(), + )]); + bank.try_lock_accounts(&conflicting_transaction); + let process_transactions_batch_output = consumer.process_and_record_transactions(&bank, &transactions); @@ -1162,22 +1177,46 @@ mod tests { let _ = poh_simulator.join(); } - #[test] - fn test_bank_process_and_record_transactions_account_in_use() { + #[test_case(false, false; "old::locked")] + #[test_case(false, true; "old::duplicate")] + #[test_case(true, false; "simd83::locked")] + #[test_case(true, true; "simd83::duplicate")] + fn test_bank_process_and_record_transactions_account_in_use( + relax_intrabatch_account_locks: bool, + use_duplicate_transaction: bool, + ) { solana_logger::setup(); let GenesisConfigInfo { genesis_config, mint_keypair, .. } = create_slow_genesis_config(10_000); - let (bank, _bank_forks) = Bank::new_no_wallclock_throttle_for_tests(&genesis_config); + let mut bank = Bank::new_for_tests(&genesis_config); + if !relax_intrabatch_account_locks { + bank.deactivate_feature(&agave_feature_set::relax_intrabatch_account_locks::id()); + } + bank.ns_per_slot = u128::MAX; + let (bank, _bank_forks) = bank.wrap_with_bank_forks_for_tests(); let pubkey = solana_pubkey::new_rand(); let pubkey1 = solana_pubkey::new_rand(); let transactions = sanitize_transactions(vec![ system_transaction::transfer(&mint_keypair, &pubkey, 1, genesis_config.hash()), - system_transaction::transfer(&mint_keypair, &pubkey1, 1, genesis_config.hash()), + system_transaction::transfer( + &mint_keypair, + if use_duplicate_transaction { + &pubkey + } else { + &pubkey1 + }, + 1, + genesis_config.hash(), + ), ]); + assert_eq!( + transactions[0].signature() == transactions[1].signature(), + use_duplicate_transaction + ); let ledger_path = get_tmp_ledger_path_auto_delete!(); let blockstore = Blockstore::open(ledger_path.path()) @@ -1212,6 +1251,20 @@ mod tests { ); let consumer = Consumer::new(committer, recorder, QosService::new(1), None); + // with simd83 and no duplicate, we take a cross-batch lock on an account to create a conflict + // with a duplicate transaction and simd83 it comes from signature equality in the batch + // without simd83 the conflict comes from locks in batch + if relax_intrabatch_account_locks && !use_duplicate_transaction { + let conflicting_transaction = + sanitize_transactions(vec![system_transaction::transfer( + &Keypair::new(), + &pubkey1, + 1, + genesis_config.hash(), + )]); + bank.try_lock_accounts(&conflicting_transaction); + } + let process_transactions_batch_output = consumer.process_and_record_transactions(&bank, &transactions); diff --git a/feature-set/src/lib.rs b/feature-set/src/lib.rs index 02d92c361c234e..78f1dcb3fa07b2 100644 --- a/feature-set/src/lib.rs +++ b/feature-set/src/lib.rs @@ -1059,7 +1059,7 @@ pub mod drop_unchained_merkle_shreds { } pub mod relax_intrabatch_account_locks { - solana_pubkey::declare_id!("EbAhnReKK8Sf88CvAfAXbgKji8DV48rsp4q2sgHqgWef"); + solana_pubkey::declare_id!("B31HjiHtU5KCvzeGW2pa9DLDvrYazWdF6qFeKE5UQoqG"); } pub mod create_slashing_program { diff --git a/ledger/src/blockstore_processor.rs b/ledger/src/blockstore_processor.rs index b4d04523134fcf..dba86e414f8eae 100644 --- a/ledger/src/blockstore_processor.rs +++ b/ledger/src/blockstore_processor.rs @@ -3638,8 +3638,11 @@ pub mod tests { }); } - #[test] - fn test_process_entries_2nd_entry_collision_with_self_and_error() { + #[test_case(false; "old")] + #[test_case(true; "simd83")] + fn test_process_entries_2nd_entry_collision_with_self_and_error( + relax_intrabatch_account_locks: bool, + ) { solana_logger::setup(); let GenesisConfigInfo { @@ -3647,7 +3650,11 @@ pub mod tests { mint_keypair, .. } = create_genesis_config(1000); - let (bank, _bank_forks) = Bank::new_with_bank_forks_for_tests(&genesis_config); + let mut bank = Bank::new_for_tests(&genesis_config); + if !relax_intrabatch_account_locks { + bank.deactivate_feature(&agave_feature_set::relax_intrabatch_account_locks::id()); + } + let (bank, _bank_forks) = bank.wrap_with_bank_forks_for_tests(); let keypair1 = Keypair::new(); let keypair2 = Keypair::new(); let keypair3 = Keypair::new(); @@ -3687,7 +3694,7 @@ pub mod tests { &mint_keypair.pubkey(), 2, bank.last_blockhash(), - ), // will collide with predecessor + ), // will collide with preceding entry ], ); // should now be: @@ -3710,28 +3717,37 @@ pub mod tests { &keypair2.pubkey(), 1, bank.last_blockhash(), - ), // should be fine + ), // will collide with preceding transaction ], ); - // would now be: + // if successful, becomes: // keypair1=0 // keypair2=3 // keypair3=3 - assert!(process_entries_for_tests_without_scheduler( + // succeeds following simd83 locking, fails otherwise + let result = process_entries_for_tests_without_scheduler( &bank, vec![ entry_1_to_mint, entry_2_to_3_and_1_to_mint, entry_conflict_itself, ], - ) - .is_err()); + ); - // last entry should have been aborted before par_execute_entries - assert_eq!(bank.get_balance(&keypair1.pubkey()), 2); - assert_eq!(bank.get_balance(&keypair2.pubkey()), 2); - assert_eq!(bank.get_balance(&keypair3.pubkey()), 2); + let balances = [ + bank.get_balance(&keypair1.pubkey()), + bank.get_balance(&keypair2.pubkey()), + bank.get_balance(&keypair3.pubkey()), + ]; + + if relax_intrabatch_account_locks { + assert!(result.is_ok()); + assert_eq!(balances, [0, 3, 3]); + } else { + assert!(result.is_err()); + assert_eq!(balances, [2, 2, 2]); + } } #[test] @@ -4041,14 +4057,19 @@ pub mod tests { ); } - #[test] - fn test_update_transaction_statuses_fail() { + #[test_case(false; "old")] + #[test_case(true; "simd83")] + fn test_update_transaction_statuses_fail(relax_intrabatch_account_locks: bool) { let GenesisConfigInfo { genesis_config, mint_keypair, .. } = create_genesis_config(11_000); - let (bank, _bank_forks) = Bank::new_with_bank_forks_for_tests(&genesis_config); + let mut bank = Bank::new_for_tests(&genesis_config); + if !relax_intrabatch_account_locks { + bank.deactivate_feature(&agave_feature_set::relax_intrabatch_account_locks::id()); + } + let (bank, _bank_forks) = bank.wrap_with_bank_forks_for_tests(); let keypair1 = Keypair::new(); let keypair2 = Keypair::new(); let success_tx = system_transaction::transfer( @@ -4057,7 +4078,7 @@ pub mod tests { 1, bank.last_blockhash(), ); - let fail_tx = system_transaction::transfer( + let test_tx = system_transaction::transfer( &mint_keypair, &keypair2.pubkey(), 2, @@ -4069,17 +4090,29 @@ pub mod tests { 1, vec![ success_tx, - fail_tx.clone(), // will collide + test_tx.clone(), // will collide ], ); + // succeeds with simd83, fails because of account locking conflict otherwise assert_eq!( process_entries_for_tests_without_scheduler(&bank, vec![entry_1_to_mint]), - Err(TransactionError::AccountInUse) + if relax_intrabatch_account_locks { + Ok(()) + } else { + Err(TransactionError::AccountInUse) + } ); - // Should not see duplicate signature error - assert_eq!(bank.process_transaction(&fail_tx), Ok(())); + // fails with simd83 as already processed, succeeds otherwise + assert_eq!( + bank.process_transaction(&test_tx), + if relax_intrabatch_account_locks { + Err(TransactionError::AlreadyProcessed) + } else { + Ok(()) + } + ); } #[test] diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 6297e4b1c41174..beb414e754b9da 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -3175,13 +3175,8 @@ impl Bank { ) }) .collect::>>()?; - let tx_account_lock_limit = self.get_transaction_account_lock_limit(); - let lock_results = self - .rc - .accounts - .lock_accounts(sanitized_txs.iter(), tx_account_lock_limit); Ok(TransactionBatch::new( - lock_results, + self.try_lock_accounts(&sanitized_txs), self, OwnedOrBorrowed::Owned(sanitized_txs), )) @@ -3189,39 +3184,73 @@ impl Bank { /// Attempt to take locks on the accounts in a transaction batch pub fn try_lock_accounts(&self, txs: &[impl SVMMessage]) -> Vec> { + self.try_lock_accounts_with_results(txs, txs.iter().map(|_| Ok(()))) + } + + /// Attempt to take locks on the accounts in a transaction batch, and their cost + /// limited packing status and duplicate transaction conflict status + pub fn try_lock_accounts_with_results( + &self, + txs: &[impl SVMMessage], + tx_results: impl Iterator>, + ) -> Vec> { let tx_account_lock_limit = self.get_transaction_account_lock_limit(); - self.rc - .accounts - .lock_accounts(txs.iter(), tx_account_lock_limit) + let relax_intrabatch_account_locks = self + .feature_set + .is_active(&feature_set::relax_intrabatch_account_locks::id()); + self.rc.accounts.lock_accounts_with_results( + txs.iter(), + tx_results, + tx_account_lock_limit, + relax_intrabatch_account_locks, + ) } /// Prepare a locked transaction batch from a list of sanitized transactions. - pub fn prepare_sanitized_batch<'a, 'b, Tx: SVMMessage>( + pub fn prepare_sanitized_batch<'a, 'b, Tx: TransactionWithMeta>( &'a self, txs: &'b [Tx], ) -> TransactionBatch<'a, 'b, Tx> { - TransactionBatch::new( - self.try_lock_accounts(txs), - self, - OwnedOrBorrowed::Borrowed(txs), - ) + self.prepare_sanitized_batch_with_results(txs, txs.iter().map(|_| Ok(()))) } /// Prepare a locked transaction batch from a list of sanitized transactions, and their cost /// limited packing status - pub fn prepare_sanitized_batch_with_results<'a, 'b, Tx: SVMMessage>( + pub fn prepare_sanitized_batch_with_results<'a, 'b, Tx: TransactionWithMeta>( &'a self, transactions: &'b [Tx], transaction_results: impl Iterator>, ) -> TransactionBatch<'a, 'b, Tx> { // this lock_results could be: Ok, AccountInUse, WouldExceedBlockMaxLimit or WouldExceedAccountMaxLimit - let tx_account_lock_limit = self.get_transaction_account_lock_limit(); - let lock_results = self.rc.accounts.lock_accounts_with_results( - transactions.iter(), - transaction_results, - tx_account_lock_limit, - ); - TransactionBatch::new(lock_results, self, OwnedOrBorrowed::Borrowed(transactions)) + + let relax_intrabatch_account_locks = self + .feature_set + .is_active(&feature_set::relax_intrabatch_account_locks::id()); + + // with simd83 enabled, we must deduplicate transactions by message hash + // previously, conflicting account locks would dedupe transactions as a side effect + let mut batch_message_hashes = AHashSet::with_capacity(transactions.len()); + let transaction_results = + transaction_results + .enumerate() + .map(|(i, tx_result)| match tx_result { + Ok(()) if relax_intrabatch_account_locks => { + // `HashSet::insert()` returns `true` when the value does *not* already exist + if batch_message_hashes.insert(transactions[i].message_hash()) { + Ok(()) + } else { + Err(TransactionError::AccountInUse) + } + } + Ok(()) => Ok(()), + Err(e) => Err(e), + }); + + TransactionBatch::new( + self.try_lock_accounts_with_results(transactions, transaction_results), + self, + OwnedOrBorrowed::Borrowed(transactions), + ) } /// Prepare a transaction batch from a single transaction without locking accounts @@ -3241,7 +3270,7 @@ impl Bank { } /// Prepare a transaction batch from a single transaction after locking accounts - pub fn prepare_locked_batch_from_single_tx<'a, Tx: SVMMessage>( + pub fn prepare_locked_batch_from_single_tx<'a, Tx: TransactionWithMeta>( &'a self, transaction: &'a Tx, ) -> TransactionBatch<'a, 'a, Tx> { @@ -7113,16 +7142,15 @@ impl Bank { &self, txs: Vec, ) -> TransactionBatch> { - let transaction_account_lock_limit = self.get_transaction_account_lock_limit(); let sanitized_txs = txs .into_iter() .map(RuntimeTransaction::from_transaction_for_tests) .collect::>(); - let lock_results = self - .rc - .accounts - .lock_accounts(sanitized_txs.iter(), transaction_account_lock_limit); - TransactionBatch::new(lock_results, self, OwnedOrBorrowed::Owned(sanitized_txs)) + TransactionBatch::new( + self.try_lock_accounts(&sanitized_txs), + self, + OwnedOrBorrowed::Owned(sanitized_txs), + ) } /// Set the initial accounts data size diff --git a/runtime/src/bank/tests.rs b/runtime/src/bank/tests.rs index fb9c2641a3092d..2334c4ef9dccea 100644 --- a/runtime/src/bank/tests.rs +++ b/runtime/src/bank/tests.rs @@ -3052,34 +3052,39 @@ fn test_debits_before_credits() { let (bank, _bank_forks) = Bank::new_with_bank_forks_for_tests(&genesis_config); let keypair = Keypair::new(); let tx0 = system_transaction::transfer( - &mint_keypair, - &keypair.pubkey(), - sol_to_lamports(2.), - genesis_config.hash(), - ); - let tx1 = system_transaction::transfer( &keypair, &mint_keypair.pubkey(), sol_to_lamports(1.), genesis_config.hash(), ); + let tx1 = system_transaction::transfer( + &mint_keypair, + &keypair.pubkey(), + sol_to_lamports(2.), + genesis_config.hash(), + ); let txs = vec![tx0, tx1]; let results = bank.process_transactions(txs.iter()); - assert!(results[1].is_err()); + assert!(results[0].is_err()); // Assert bad transactions aren't counted. assert_eq!(bank.transaction_count(), 1); assert_eq!(bank.non_vote_transaction_count_since_restart(), 1); } -#[test] -fn test_readonly_accounts() { +#[test_case(false; "old")] +#[test_case(true; "simd83")] +fn test_readonly_accounts(relax_intrabatch_account_locks: bool) { let GenesisConfigInfo { genesis_config, mint_keypair, .. } = create_genesis_config_with_leader(500, &solana_pubkey::new_rand(), 0); - let (bank, _bank_forks) = Bank::new_with_bank_forks_for_tests(&genesis_config); + let mut bank = Bank::new_for_tests(&genesis_config); + if !relax_intrabatch_account_locks { + bank.deactivate_feature(&feature_set::relax_intrabatch_account_locks::id()); + } + let (bank, _bank_forks) = bank.wrap_with_bank_forks_for_tests(); let vote_pubkey0 = solana_pubkey::new_rand(); let vote_pubkey1 = solana_pubkey::new_rand(); @@ -3143,9 +3148,16 @@ fn test_readonly_accounts() { ); let txs = vec![tx0, tx1]; let results = bank.process_transactions(txs.iter()); - // However, an account may not be locked as read-only and writable at the same time. + // Whether an account can be locked as read-only and writable at the same time depends on features. assert_eq!(results[0], Ok(())); - assert_eq!(results[1], Err(TransactionError::AccountInUse)); + assert_eq!( + results[1], + if relax_intrabatch_account_locks { + Ok(()) + } else { + Err(TransactionError::AccountInUse) + } + ); } #[test] @@ -9428,8 +9440,9 @@ fn test_vote_epoch_panic() { ); } -#[test] -fn test_tx_log_order() { +#[test_case(false; "old")] +#[test_case(true; "simd83")] +fn test_tx_log_order(relax_intrabatch_account_locks: bool) { let GenesisConfigInfo { genesis_config, mint_keypair, @@ -9439,7 +9452,11 @@ fn test_tx_log_order() { &Pubkey::new_unique(), bootstrap_validator_stake_lamports(), ); - let (bank, _bank_forks) = Bank::new_with_bank_forks_for_tests(&genesis_config); + let mut bank = Bank::new_for_tests(&genesis_config); + if !relax_intrabatch_account_locks { + bank.deactivate_feature(&feature_set::relax_intrabatch_account_locks::id()); + } + let (bank, _bank_forks) = bank.wrap_with_bank_forks_for_tests(); *bank.transaction_log_collector_config.write().unwrap() = TransactionLogCollectorConfig { mentioned_addresses: HashSet::new(), filter: TransactionLogCollectorFilter::All, @@ -9496,7 +9513,11 @@ fn test_tx_log_order() { .as_ref() .unwrap()[2] .contains(&"failed".to_string())); - assert!(commit_results[2].is_err()); + if relax_intrabatch_account_locks { + assert!(commit_results[2].is_ok()); + } else { + assert!(commit_results[2].is_err()); + } let stored_logs = &bank.transaction_log_collector.read().unwrap().logs; let success_log_info = stored_logs diff --git a/runtime/src/transaction_batch.rs b/runtime/src/transaction_batch.rs index 58eda791a01c24..8198381943a036 100644 --- a/runtime/src/transaction_batch.rs +++ b/runtime/src/transaction_batch.rs @@ -120,11 +120,13 @@ mod tests { solana_system_transaction as system_transaction, solana_transaction::sanitized::SanitizedTransaction, solana_transaction_error::TransactionError, + test_case::test_case, }; - #[test] - fn test_transaction_batch() { - let (bank, txs) = setup(false); + #[test_case(false; "old")] + #[test_case(true; "simd83")] + fn test_transaction_batch(relax_intrabatch_account_locks: bool) { + let (bank, txs) = setup(false, relax_intrabatch_account_locks); // Test getting locked accounts let batch = bank.prepare_sanitized_batch(&txs); @@ -144,9 +146,10 @@ mod tests { assert!(batch2.lock_results().iter().all(|x| x.is_ok())); } - #[test] - fn test_simulation_batch() { - let (bank, txs) = setup(false); + #[test_case(false; "old")] + #[test_case(true; "simd83")] + fn test_simulation_batch(relax_intrabatch_account_locks: bool) { + let (bank, txs) = setup(false, relax_intrabatch_account_locks); // Prepare batch without locks let batch = bank.prepare_unlocked_batch_from_single_tx(&txs[0]); @@ -161,20 +164,23 @@ mod tests { assert!(batch3.lock_results().iter().all(|x| x.is_ok())); } - #[test] - fn test_unlock_failures() { - let (bank, txs) = setup(true); + #[test_case(false; "old")] + #[test_case(true; "simd83")] + fn test_unlock_failures(relax_intrabatch_account_locks: bool) { + let (bank, txs) = setup(true, relax_intrabatch_account_locks); + let expected_lock_results = if relax_intrabatch_account_locks { + vec![Ok(()), Ok(()), Ok(())] + } else { + vec![Ok(()), Err(TransactionError::AccountInUse), Ok(())] + }; // Test getting locked accounts let mut batch = bank.prepare_sanitized_batch(&txs); - assert_eq!( - batch.lock_results, - vec![Ok(()), Err(TransactionError::AccountInUse), Ok(())] - ); + assert_eq!(batch.lock_results, expected_lock_results,); let qos_results = vec![ Ok(()), - Err(TransactionError::AccountInUse), + Err(TransactionError::WouldExceedMaxBlockCostLimit), Err(TransactionError::WouldExceedMaxBlockCostLimit), ]; batch.unlock_failures(qos_results.clone()); @@ -183,22 +189,25 @@ mod tests { // Dropping the batch should unlock remaining locked transactions drop(batch); - // The next batch should be able to lock all but the conflicting tx + // The next batch should be able to take all the same locks as before let batch2 = bank.prepare_sanitized_batch(&txs); - assert_eq!( - batch2.lock_results, - vec![Ok(()), Err(TransactionError::AccountInUse), Ok(())] - ); + assert_eq!(batch2.lock_results, expected_lock_results,); } - fn setup(insert_conflicting_tx: bool) -> (Bank, Vec>) { + fn setup( + insert_conflicting_tx: bool, + relax_intrabatch_account_locks: bool, + ) -> (Bank, Vec>) { let dummy_leader_pubkey = solana_pubkey::new_rand(); let GenesisConfigInfo { genesis_config, mint_keypair, .. } = create_genesis_config_with_leader(500, &dummy_leader_pubkey, 100); - let bank = Bank::new_for_tests(&genesis_config); + let mut bank = Bank::new_for_tests(&genesis_config); + if !relax_intrabatch_account_locks { + bank.deactivate_feature(&agave_feature_set::relax_intrabatch_account_locks::id()); + } let pubkey = solana_pubkey::new_rand(); let keypair2 = Keypair::new(); From d0cabccf67423cf606fd2f72822120d7556b2472 Mon Sep 17 00:00:00 2001 From: hana <81144685+2501babe@users.noreply.github.com> Date: Mon, 12 May 2025 23:50:42 -0700 Subject: [PATCH 02/10] collect inner iter --- accounts-db/src/account_locks.rs | 47 ++++++++++++++------------------ accounts-db/src/accounts.rs | 22 ++++++++++----- 2 files changed, 35 insertions(+), 34 deletions(-) diff --git a/accounts-db/src/account_locks.rs b/accounts-db/src/account_locks.rs index 4b80082a792c0a..92d839018c1e43 100644 --- a/accounts-db/src/account_locks.rs +++ b/accounts-db/src/account_locks.rs @@ -21,11 +21,8 @@ impl AccountLocks { /// Returns an error if any of the accounts are already locked in a way /// that conflicts with the requested lock. /// NOTE this is the pre-SIMD83 logic and can be removed once SIMD83 is active. - pub fn try_lock_accounts<'a>( - &mut self, - keys: impl Iterator + Clone, - ) -> TransactionResult<()> { - self.can_lock_accounts(keys.clone())?; + pub fn try_lock_accounts(&mut self, keys: &[(&Pubkey, bool)]) -> TransactionResult<()> { + self.can_lock_accounts(keys)?; self.lock_accounts(keys); Ok(()) @@ -38,22 +35,21 @@ impl AccountLocks { /// the only logic, and this note can be removed with the feature gate. pub fn try_lock_transaction_batch<'a>( &mut self, - validated_batch_keys: impl Iterator< - Item = TransactionResult + Clone>, - >, + validated_batch_keys: impl Iterator>>, ) -> Vec> { let available_batch_keys: Vec<_> = validated_batch_keys - .map(|validated_keys| { - validated_keys - .clone() - .and_then(|keys| self.can_lock_accounts(keys)) - .and(validated_keys) + .map(|validated_keys| match validated_keys { + Ok(ref keys) => match self.can_lock_accounts(keys) { + Ok(_) => validated_keys, + Err(e) => Err(e), + }, + Err(e) => Err(e), }) .collect(); available_batch_keys .into_iter() - .map(|available_keys| available_keys.map(|keys| self.lock_accounts(keys))) + .map(|available_keys| available_keys.map(|keys| self.lock_accounts(&keys))) .collect() } @@ -71,12 +67,9 @@ impl AccountLocks { } } - fn can_lock_accounts<'a>( - &self, - keys: impl Iterator + Clone, - ) -> TransactionResult<()> { - for (key, writable) in keys.clone() { - if writable { + fn can_lock_accounts(&self, keys: &[(&Pubkey, bool)]) -> TransactionResult<()> { + for (key, writable) in keys { + if *writable { if !self.can_write_lock(key) { return Err(TransactionError::AccountInUse); } @@ -88,9 +81,9 @@ impl AccountLocks { Ok(()) } - fn lock_accounts<'a>(&mut self, keys: impl Iterator + Clone) { + fn lock_accounts(&mut self, keys: &[(&Pubkey, bool)]) { for (key, writable) in keys { - if writable { + if *writable { self.lock_write(key); } else { self.lock_readonly(key); @@ -211,23 +204,23 @@ mod tests { let key2 = Pubkey::new_unique(); // Add write and read-lock. - let result = account_locks.try_lock_accounts([(&key1, true), (&key2, false)].into_iter()); + let result = account_locks.try_lock_accounts(&[(&key1, true), (&key2, false)]); assert!(result.is_ok()); // Try to add duplicate write-lock. - let result = account_locks.try_lock_accounts([(&key1, true)].into_iter()); + let result = account_locks.try_lock_accounts(&[(&key1, true)]); assert_eq!(result, Err(TransactionError::AccountInUse)); // Try to add write lock on read-locked account. - let result = account_locks.try_lock_accounts([(&key2, true)].into_iter()); + let result = account_locks.try_lock_accounts(&[(&key2, true)]); assert_eq!(result, Err(TransactionError::AccountInUse)); // Try to add read lock on write-locked account. - let result = account_locks.try_lock_accounts([(&key1, false)].into_iter()); + let result = account_locks.try_lock_accounts(&[(&key1, false)]); assert_eq!(result, Err(TransactionError::AccountInUse)); // Add read lock on read-locked account. - let result = account_locks.try_lock_accounts([(&key2, false)].into_iter()); + let result = account_locks.try_lock_accounts(&[(&key2, false)]); assert!(result.is_ok()); // Unlock write and read locks. diff --git a/accounts-db/src/accounts.rs b/accounts-db/src/accounts.rs index 76feb7921314ef..06d1c805f792d0 100644 --- a/accounts-db/src/accounts.rs +++ b/accounts-db/src/accounts.rs @@ -611,17 +611,25 @@ impl Accounts { ) -> Vec> { let account_locks = &mut self.account_locks.lock().unwrap(); if relax_intrabatch_account_locks { - let validated_batch_keys = tx_account_locks_results.map(|tx_account_locks_result| { - tx_account_locks_result - .map(|tx_account_locks| tx_account_locks.accounts_with_is_writable()) - }); + let validated_batch_keys = tx_account_locks_results + .map(|tx_account_locks_result| { + tx_account_locks_result.map(|tx_account_locks| { + tx_account_locks + .accounts_with_is_writable() + .collect::>() + }) + }) + .collect::>(); - account_locks.try_lock_transaction_batch(validated_batch_keys) + account_locks.try_lock_transaction_batch(validated_batch_keys.into_iter()) } else { tx_account_locks_results .map(|tx_account_locks_result| match tx_account_locks_result { - Ok(tx_account_locks) => account_locks - .try_lock_accounts(tx_account_locks.accounts_with_is_writable()), + Ok(tx_account_locks) => account_locks.try_lock_accounts( + &tx_account_locks + .accounts_with_is_writable() + .collect::>(), + ), Err(err) => Err(err), }) .collect() From 9e73e22cc4a9ef132d20f309180c623d859831c2 Mon Sep 17 00:00:00 2001 From: hana <81144685+2501babe@users.noreply.github.com> Date: Fri, 23 May 2025 22:58:22 -0700 Subject: [PATCH 03/10] better refactor of account lock code --- accounts-db/benches/bench_lock_accounts.rs | 2 + accounts-db/src/accounts.rs | 89 +++++++++------------- runtime/src/bank.rs | 2 +- 3 files changed, 39 insertions(+), 54 deletions(-) diff --git a/accounts-db/benches/bench_lock_accounts.rs b/accounts-db/benches/bench_lock_accounts.rs index 89f5b418767b98..c6ecea9fd26f60 100644 --- a/accounts-db/benches/bench_lock_accounts.rs +++ b/accounts-db/benches/bench_lock_accounts.rs @@ -85,12 +85,14 @@ fn bench_entry_lock_accounts(c: &mut Criterion) { let transactions = create_test_transactions(lock_count, read_conflicts); group.throughput(Throughput::Elements(transactions.len() as u64)); let transaction_batches: Vec<_> = transactions.chunks(batch_size).collect(); + let batch_results = vec![Ok(()); batch_size].into_iter(); group.bench_function(name.as_str(), move |b| { b.iter(|| { for batch in &transaction_batches { let results = accounts.lock_accounts( black_box(batch.iter()), + batch_results.clone(), MAX_TX_ACCOUNT_LOCKS, relax_intrabatch_account_locks, ); diff --git a/accounts-db/src/accounts.rs b/accounts-db/src/accounts.rs index 06d1c805f792d0..7667fc53926d9a 100644 --- a/accounts-db/src/accounts.rs +++ b/accounts-db/src/accounts.rs @@ -565,72 +565,41 @@ impl Accounts { self.accounts_db.store_uncached(slot, &[(pubkey, account)]); } - /// This function will prevent multiple threads from modifying the same account state at the - /// same time - #[must_use] - pub fn lock_accounts<'a, Tx: SVMMessage + 'a>( - &self, - txs: impl Iterator, - tx_account_lock_limit: usize, - relax_intrabatch_account_locks: bool, - ) -> Vec> { - // Validate the account locks, then get iterator if successful validation. - let tx_account_locks_results = txs.map(|tx| { - validate_account_locks(tx.account_keys(), tx_account_lock_limit) - .map(|_| TransactionAccountLocksIterator::new(tx)) - }); - self.lock_accounts_inner(tx_account_locks_results, relax_intrabatch_account_locks) - } - /// This function will prevent multiple threads from modifying the same account state at the /// same time, possibly excluding transactions based on prior results #[must_use] - pub fn lock_accounts_with_results<'a>( + pub fn lock_accounts<'a>( &self, txs: impl Iterator, results: impl Iterator>, tx_account_lock_limit: usize, relax_intrabatch_account_locks: bool, ) -> Vec> { - // Validate the account locks, then get iterator if successful validation. - let tx_account_locks_results = txs.zip(results).map(|(tx, result)| match result { - Ok(()) => validate_account_locks(tx.account_keys(), tx_account_lock_limit) - .map(|_| TransactionAccountLocksIterator::new(tx)), - Err(err) => Err(err), - }); - self.lock_accounts_inner(tx_account_locks_results, relax_intrabatch_account_locks) - } - - #[must_use] - fn lock_accounts_inner<'a>( - &self, - tx_account_locks_results: impl Iterator< - Item = Result>, - >, - relax_intrabatch_account_locks: bool, - ) -> Vec> { - let account_locks = &mut self.account_locks.lock().unwrap(); - if relax_intrabatch_account_locks { - let validated_batch_keys = tx_account_locks_results - .map(|tx_account_locks_result| { - tx_account_locks_result.map(|tx_account_locks| { - tx_account_locks + // Validate the account locks, then get keys and is_writable if successful validation. + // We collect to fully evaluate before taking the account_locks mutex. + let validated_batch_keys = txs + .zip(results) + .map(|(tx, result)| { + result + .and_then(|_| validate_account_locks(tx.account_keys(), tx_account_lock_limit)) + .map(|_| { + TransactionAccountLocksIterator::new(tx) .accounts_with_is_writable() .collect::>() }) - }) - .collect::>(); + }) + .collect::>() + .into_iter(); + + let account_locks = &mut self.account_locks.lock().unwrap(); - account_locks.try_lock_transaction_batch(validated_batch_keys.into_iter()) + if relax_intrabatch_account_locks { + account_locks.try_lock_transaction_batch(validated_batch_keys) } else { - tx_account_locks_results - .map(|tx_account_locks_result| match tx_account_locks_result { - Ok(tx_account_locks) => account_locks.try_lock_accounts( - &tx_account_locks - .accounts_with_is_writable() - .collect::>(), - ), - Err(err) => Err(err), + validated_batch_keys + .map(|result_validated_tx_keys| match result_validated_tx_keys { + Ok(validated_tx_keys) => account_locks.try_lock_accounts(&validated_tx_keys), + Err(e) => Err(e), }) .collect() } @@ -952,6 +921,7 @@ mod tests { let tx = new_sanitized_tx(&[&keypair], message, Hash::default()); let results = accounts.lock_accounts( [tx].iter(), + [Ok(())].into_iter(), MAX_TX_ACCOUNT_LOCKS, relax_intrabatch_account_locks, ); @@ -985,6 +955,7 @@ mod tests { let txs = vec![new_sanitized_tx(&[&keypair], message, Hash::default())]; let results = accounts.lock_accounts( txs.iter(), + vec![Ok(()); txs.len()].into_iter(), MAX_TX_ACCOUNT_LOCKS, relax_intrabatch_account_locks, ); @@ -1011,6 +982,7 @@ mod tests { let txs = vec![new_sanitized_tx(&[&keypair], message, Hash::default())]; let results = accounts.lock_accounts( txs.iter(), + vec![Ok(()); txs.len()].into_iter(), MAX_TX_ACCOUNT_LOCKS, relax_intrabatch_account_locks, ); @@ -1050,6 +1022,7 @@ mod tests { let tx = new_sanitized_tx(&[&keypair0], message, Hash::default()); let results0 = accounts.lock_accounts( [tx.clone()].iter(), + [Ok(())].into_iter(), MAX_TX_ACCOUNT_LOCKS, relax_intrabatch_account_locks, ); @@ -1084,6 +1057,7 @@ mod tests { let txs = vec![tx0, tx1]; let results1 = accounts.lock_accounts( txs.iter(), + vec![Ok(()); txs.len()].into_iter(), MAX_TX_ACCOUNT_LOCKS, relax_intrabatch_account_locks, ); @@ -1114,6 +1088,7 @@ mod tests { let tx = new_sanitized_tx(&[&keypair1], message, Hash::default()); let results2 = accounts.lock_accounts( [tx].iter(), + [Ok(())].into_iter(), MAX_TX_ACCOUNT_LOCKS, relax_intrabatch_account_locks, ); @@ -1181,6 +1156,7 @@ mod tests { let txs = vec![writable_tx.clone()]; let results = accounts_clone.clone().lock_accounts( txs.iter(), + vec![Ok(()); txs.len()].into_iter(), MAX_TX_ACCOUNT_LOCKS, relax_intrabatch_account_locks, ); @@ -1199,6 +1175,7 @@ mod tests { let txs = vec![readonly_tx.clone()]; let results = accounts_arc.clone().lock_accounts( txs.iter(), + vec![Ok(()); txs.len()].into_iter(), MAX_TX_ACCOUNT_LOCKS, relax_intrabatch_account_locks, ); @@ -1245,6 +1222,7 @@ mod tests { let tx = new_sanitized_tx(&[&keypair0], message, Hash::default()); let results0 = accounts.lock_accounts( [tx].iter(), + [Ok(())].into_iter(), MAX_TX_ACCOUNT_LOCKS, relax_intrabatch_account_locks, ); @@ -1341,7 +1319,7 @@ mod tests { Ok(()), ]; - let results = accounts.lock_accounts_with_results( + let results = accounts.lock_accounts( txs.iter(), qos_results.into_iter(), MAX_TX_ACCOUNT_LOCKS, @@ -1401,6 +1379,7 @@ mod tests { let accounts = Accounts::new(accounts_db.clone()); let results = accounts.lock_accounts( [w_tx.clone()].iter(), + [Ok(())].into_iter(), MAX_TX_ACCOUNT_LOCKS, relax_intrabatch_account_locks, ); @@ -1410,6 +1389,7 @@ mod tests { // wr conflict cross-batch always fails let results = accounts.lock_accounts( [r_tx.clone()].iter(), + [Ok(())].into_iter(), MAX_TX_ACCOUNT_LOCKS, relax_intrabatch_account_locks, ); @@ -1419,6 +1399,7 @@ mod tests { // ww conflict cross-batch always fails let results = accounts.lock_accounts( [w_tx.clone()].iter(), + [Ok(())].into_iter(), MAX_TX_ACCOUNT_LOCKS, relax_intrabatch_account_locks, ); @@ -1429,6 +1410,7 @@ mod tests { let accounts = Accounts::new(accounts_db.clone()); let results = accounts.lock_accounts( [w_tx.clone(), r_tx.clone()].iter(), + [Ok(()), Ok(())].into_iter(), MAX_TX_ACCOUNT_LOCKS, relax_intrabatch_account_locks, ); @@ -1443,6 +1425,7 @@ mod tests { let accounts = Accounts::new(accounts_db.clone()); let results = accounts.lock_accounts( [w_tx.clone(), r_tx.clone()].iter(), + [Ok(()), Ok(())].into_iter(), MAX_TX_ACCOUNT_LOCKS, relax_intrabatch_account_locks, ); diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index beb414e754b9da..d8e28a1e031499 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -3198,7 +3198,7 @@ impl Bank { let relax_intrabatch_account_locks = self .feature_set .is_active(&feature_set::relax_intrabatch_account_locks::id()); - self.rc.accounts.lock_accounts_with_results( + self.rc.accounts.lock_accounts( txs.iter(), tx_results, tx_account_lock_limit, From 169f1ea34640d761ed255883bae64383d6b5ad60 Mon Sep 17 00:00:00 2001 From: hana <81144685+2501babe@users.noreply.github.com> Date: Wed, 28 May 2025 00:03:18 -0700 Subject: [PATCH 04/10] undo slice refactor --- accounts-db/src/account_locks.rs | 36 +++++++++++++++++++------------- accounts-db/src/accounts.rs | 8 ++----- 2 files changed, 24 insertions(+), 20 deletions(-) diff --git a/accounts-db/src/account_locks.rs b/accounts-db/src/account_locks.rs index 92d839018c1e43..20958cf3c12066 100644 --- a/accounts-db/src/account_locks.rs +++ b/accounts-db/src/account_locks.rs @@ -21,8 +21,11 @@ impl AccountLocks { /// Returns an error if any of the accounts are already locked in a way /// that conflicts with the requested lock. /// NOTE this is the pre-SIMD83 logic and can be removed once SIMD83 is active. - pub fn try_lock_accounts(&mut self, keys: &[(&Pubkey, bool)]) -> TransactionResult<()> { - self.can_lock_accounts(keys)?; + pub fn try_lock_accounts<'a>( + &mut self, + keys: impl Iterator + Clone, + ) -> TransactionResult<()> { + self.can_lock_accounts(keys.clone())?; self.lock_accounts(keys); Ok(()) @@ -35,11 +38,13 @@ impl AccountLocks { /// the only logic, and this note can be removed with the feature gate. pub fn try_lock_transaction_batch<'a>( &mut self, - validated_batch_keys: impl Iterator>>, + validated_batch_keys: impl Iterator< + Item = TransactionResult + Clone>, + >, ) -> Vec> { let available_batch_keys: Vec<_> = validated_batch_keys .map(|validated_keys| match validated_keys { - Ok(ref keys) => match self.can_lock_accounts(keys) { + Ok(ref keys) => match self.can_lock_accounts(keys.clone()) { Ok(_) => validated_keys, Err(e) => Err(e), }, @@ -49,7 +54,7 @@ impl AccountLocks { available_batch_keys .into_iter() - .map(|available_keys| available_keys.map(|keys| self.lock_accounts(&keys))) + .map(|available_keys| available_keys.map(|keys| self.lock_accounts(keys))) .collect() } @@ -67,9 +72,12 @@ impl AccountLocks { } } - fn can_lock_accounts(&self, keys: &[(&Pubkey, bool)]) -> TransactionResult<()> { + fn can_lock_accounts<'a>( + &self, + keys: impl Iterator, + ) -> TransactionResult<()> { for (key, writable) in keys { - if *writable { + if writable { if !self.can_write_lock(key) { return Err(TransactionError::AccountInUse); } @@ -81,9 +89,9 @@ impl AccountLocks { Ok(()) } - fn lock_accounts(&mut self, keys: &[(&Pubkey, bool)]) { + fn lock_accounts<'a>(&mut self, keys: impl Iterator) { for (key, writable) in keys { - if *writable { + if writable { self.lock_write(key); } else { self.lock_readonly(key); @@ -204,23 +212,23 @@ mod tests { let key2 = Pubkey::new_unique(); // Add write and read-lock. - let result = account_locks.try_lock_accounts(&[(&key1, true), (&key2, false)]); + let result = account_locks.try_lock_accounts([(&key1, true), (&key2, false)].into_iter()); assert!(result.is_ok()); // Try to add duplicate write-lock. - let result = account_locks.try_lock_accounts(&[(&key1, true)]); + let result = account_locks.try_lock_accounts([(&key1, true)].into_iter()); assert_eq!(result, Err(TransactionError::AccountInUse)); // Try to add write lock on read-locked account. - let result = account_locks.try_lock_accounts(&[(&key2, true)]); + let result = account_locks.try_lock_accounts([(&key2, true)].into_iter()); assert_eq!(result, Err(TransactionError::AccountInUse)); // Try to add read lock on write-locked account. - let result = account_locks.try_lock_accounts(&[(&key1, false)]); + let result = account_locks.try_lock_accounts([(&key1, false)].into_iter()); assert_eq!(result, Err(TransactionError::AccountInUse)); // Add read lock on read-locked account. - let result = account_locks.try_lock_accounts(&[(&key2, false)]); + let result = account_locks.try_lock_accounts([(&key2, false)].into_iter()); assert!(result.is_ok()); // Unlock write and read locks. diff --git a/accounts-db/src/accounts.rs b/accounts-db/src/accounts.rs index 7667fc53926d9a..6c9b39f4a13068 100644 --- a/accounts-db/src/accounts.rs +++ b/accounts-db/src/accounts.rs @@ -582,11 +582,7 @@ impl Accounts { .map(|(tx, result)| { result .and_then(|_| validate_account_locks(tx.account_keys(), tx_account_lock_limit)) - .map(|_| { - TransactionAccountLocksIterator::new(tx) - .accounts_with_is_writable() - .collect::>() - }) + .map(|_| TransactionAccountLocksIterator::new(tx).accounts_with_is_writable()) }) .collect::>() .into_iter(); @@ -598,7 +594,7 @@ impl Accounts { } else { validated_batch_keys .map(|result_validated_tx_keys| match result_validated_tx_keys { - Ok(validated_tx_keys) => account_locks.try_lock_accounts(&validated_tx_keys), + Ok(validated_tx_keys) => account_locks.try_lock_accounts(validated_tx_keys), Err(e) => Err(e), }) .collect() From 37f509553bfeaa9d113702b2abe25caf86061bae Mon Sep 17 00:00:00 2001 From: hana <81144685+2501babe@users.noreply.github.com> Date: Wed, 28 May 2025 05:06:11 -0700 Subject: [PATCH 05/10] in-place vec edit --- accounts-db/src/account_locks.rs | 17 +++++++++-------- accounts-db/src/accounts.rs | 4 ++-- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/accounts-db/src/account_locks.rs b/accounts-db/src/account_locks.rs index 20958cf3c12066..152bcbe596837d 100644 --- a/accounts-db/src/account_locks.rs +++ b/accounts-db/src/account_locks.rs @@ -38,21 +38,22 @@ impl AccountLocks { /// the only logic, and this note can be removed with the feature gate. pub fn try_lock_transaction_batch<'a>( &mut self, - validated_batch_keys: impl Iterator< - Item = TransactionResult + Clone>, + mut validated_batch_keys: Vec< + TransactionResult + Clone>, >, ) -> Vec> { - let available_batch_keys: Vec<_> = validated_batch_keys - .map(|validated_keys| match validated_keys { + validated_batch_keys.iter_mut().for_each(|validated_keys| { + let result = std::mem::replace(validated_keys, Err(TransactionError::AccountInUse)); + *validated_keys = match result { Ok(ref keys) => match self.can_lock_accounts(keys.clone()) { - Ok(_) => validated_keys, + Ok(_) => result, Err(e) => Err(e), }, Err(e) => Err(e), - }) - .collect(); + }; + }); - available_batch_keys + validated_batch_keys .into_iter() .map(|available_keys| available_keys.map(|keys| self.lock_accounts(keys))) .collect() diff --git a/accounts-db/src/accounts.rs b/accounts-db/src/accounts.rs index 6c9b39f4a13068..9c06ee08305e9a 100644 --- a/accounts-db/src/accounts.rs +++ b/accounts-db/src/accounts.rs @@ -584,8 +584,7 @@ impl Accounts { .and_then(|_| validate_account_locks(tx.account_keys(), tx_account_lock_limit)) .map(|_| TransactionAccountLocksIterator::new(tx).accounts_with_is_writable()) }) - .collect::>() - .into_iter(); + .collect::>(); let account_locks = &mut self.account_locks.lock().unwrap(); @@ -593,6 +592,7 @@ impl Accounts { account_locks.try_lock_transaction_batch(validated_batch_keys) } else { validated_batch_keys + .into_iter() .map(|result_validated_tx_keys| match result_validated_tx_keys { Ok(validated_tx_keys) => account_locks.try_lock_accounts(validated_tx_keys), Err(e) => Err(e), From 83bf9009adb4c9799362a93e05eba7821ce8aa28 Mon Sep 17 00:00:00 2001 From: hana <81144685+2501babe@users.noreply.github.com> Date: Fri, 30 May 2025 03:55:08 -0700 Subject: [PATCH 06/10] enforce transaction dedupe in replay --- ledger/src/blockstore_processor.rs | 59 ++++++++++++++++++++++++++++++ runtime/src/bank.rs | 47 +++++++++++------------- 2 files changed, 80 insertions(+), 26 deletions(-) diff --git a/ledger/src/blockstore_processor.rs b/ledger/src/blockstore_processor.rs index dba86e414f8eae..1af6118157bd93 100644 --- a/ledger/src/blockstore_processor.rs +++ b/ledger/src/blockstore_processor.rs @@ -3750,6 +3750,65 @@ pub mod tests { } } + #[test_case(false; "old")] + #[test_case(true; "simd83")] + fn test_process_entry_duplicate_transaction(relax_intrabatch_account_locks: bool) { + solana_logger::setup(); + + let GenesisConfigInfo { + genesis_config, + mint_keypair, + .. + } = create_genesis_config(1000); + let mut bank = Bank::new_for_tests(&genesis_config); + if !relax_intrabatch_account_locks { + bank.deactivate_feature(&agave_feature_set::relax_intrabatch_account_locks::id()); + } + let (bank, _bank_forks) = bank.wrap_with_bank_forks_for_tests(); + let keypair1 = Keypair::new(); + let keypair2 = Keypair::new(); + + // fund: put some money in each of 1 and 2 + assert_matches!(bank.transfer(5, &mint_keypair, &keypair1.pubkey()), Ok(_)); + assert_matches!(bank.transfer(5, &mint_keypair, &keypair2.pubkey()), Ok(_)); + + // one entry, two instances of the same transaction. this entry is invalid + // without simd83: due to lock conflicts + // with simd83: due to message hash duplication + let entry_1_to_2_twice = next_entry( + &bank.last_blockhash(), + 1, + vec![ + system_transaction::transfer( + &keypair1, + &keypair2.pubkey(), + 1, + bank.last_blockhash(), + ), + system_transaction::transfer( + &keypair1, + &keypair2.pubkey(), + 1, + bank.last_blockhash(), + ), + ], + ); + // should now be: + // keypair1=5 + // keypair2=5 + + // succeeds following simd83 locking, fails otherwise + let result = process_entries_for_tests_without_scheduler(&bank, vec![entry_1_to_2_twice]); + + let balances = [ + bank.get_balance(&keypair1.pubkey()), + bank.get_balance(&keypair2.pubkey()), + ]; + + assert!(result.is_err()); + assert_eq!(balances, [5, 5]); + } + #[test] fn test_process_entries_2_entries_par() { let GenesisConfigInfo { diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index d8e28a1e031499..2921573089427b 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -3183,7 +3183,7 @@ impl Bank { } /// Attempt to take locks on the accounts in a transaction batch - pub fn try_lock_accounts(&self, txs: &[impl SVMMessage]) -> Vec> { + pub fn try_lock_accounts(&self, txs: &[impl TransactionWithMeta]) -> Vec> { self.try_lock_accounts_with_results(txs, txs.iter().map(|_| Ok(()))) } @@ -3191,13 +3191,32 @@ impl Bank { /// limited packing status and duplicate transaction conflict status pub fn try_lock_accounts_with_results( &self, - txs: &[impl SVMMessage], + txs: &[impl TransactionWithMeta], tx_results: impl Iterator>, ) -> Vec> { let tx_account_lock_limit = self.get_transaction_account_lock_limit(); let relax_intrabatch_account_locks = self .feature_set .is_active(&feature_set::relax_intrabatch_account_locks::id()); + + // with simd83 enabled, we must fail transactions that duplicate a prior message hash + // previously, conflicting account locks would fail such transactions as a side effect + let mut batch_message_hashes = AHashSet::with_capacity(txs.len()); + let tx_results = tx_results + .enumerate() + .map(|(i, tx_result)| match tx_result { + Ok(()) if relax_intrabatch_account_locks => { + // `HashSet::insert()` returns `true` when the value does *not* already exist + if batch_message_hashes.insert(txs[i].message_hash()) { + Ok(()) + } else { + Err(TransactionError::AlreadyProcessed) + } + } + Ok(()) => Ok(()), + Err(e) => Err(e), + }); + self.rc.accounts.lock_accounts( txs.iter(), tx_results, @@ -3222,30 +3241,6 @@ impl Bank { transaction_results: impl Iterator>, ) -> TransactionBatch<'a, 'b, Tx> { // this lock_results could be: Ok, AccountInUse, WouldExceedBlockMaxLimit or WouldExceedAccountMaxLimit - - let relax_intrabatch_account_locks = self - .feature_set - .is_active(&feature_set::relax_intrabatch_account_locks::id()); - - // with simd83 enabled, we must deduplicate transactions by message hash - // previously, conflicting account locks would dedupe transactions as a side effect - let mut batch_message_hashes = AHashSet::with_capacity(transactions.len()); - let transaction_results = - transaction_results - .enumerate() - .map(|(i, tx_result)| match tx_result { - Ok(()) if relax_intrabatch_account_locks => { - // `HashSet::insert()` returns `true` when the value does *not* already exist - if batch_message_hashes.insert(transactions[i].message_hash()) { - Ok(()) - } else { - Err(TransactionError::AccountInUse) - } - } - Ok(()) => Ok(()), - Err(e) => Err(e), - }); - TransactionBatch::new( self.try_lock_accounts_with_results(transactions, transaction_results), self, From cafa4089b66bceefd3ec183df7bba129626e6364 Mon Sep 17 00:00:00 2001 From: hana <81144685+2501babe@users.noreply.github.com> Date: Fri, 30 May 2025 04:09:37 -0700 Subject: [PATCH 07/10] simplify lock accounts error propagation --- accounts-db/src/account_locks.rs | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/accounts-db/src/account_locks.rs b/accounts-db/src/account_locks.rs index 152bcbe596837d..113b41231dd737 100644 --- a/accounts-db/src/account_locks.rs +++ b/accounts-db/src/account_locks.rs @@ -43,14 +43,11 @@ impl AccountLocks { >, ) -> Vec> { validated_batch_keys.iter_mut().for_each(|validated_keys| { - let result = std::mem::replace(validated_keys, Err(TransactionError::AccountInUse)); - *validated_keys = match result { - Ok(ref keys) => match self.can_lock_accounts(keys.clone()) { - Ok(_) => result, - Err(e) => Err(e), - }, - Err(e) => Err(e), - }; + if let Ok(ref keys) = validated_keys { + if let Err(e) = self.can_lock_accounts(keys.clone()) { + *validated_keys = Err(e); + } + } }); validated_batch_keys From e2a0190e15509b2efc3d0e0fb136018c1f3bafdd Mon Sep 17 00:00:00 2001 From: hana <81144685+2501babe@users.noreply.github.com> Date: Fri, 30 May 2025 08:22:00 -0700 Subject: [PATCH 08/10] fix dup tx tests for our brave new world --- core/src/banking_stage/consumer.rs | 82 ++++++++++++++++++++++-------- 1 file changed, 61 insertions(+), 21 deletions(-) diff --git a/core/src/banking_stage/consumer.rs b/core/src/banking_stage/consumer.rs index c5b106726233ad..d38ac0b69f132e 100644 --- a/core/src/banking_stage/consumer.rs +++ b/core/src/banking_stage/consumer.rs @@ -1290,8 +1290,14 @@ mod tests { processed_with_successful_result_count: 1, } ); - assert_eq!(retryable_transaction_indexes, vec![1]); assert!(commit_transactions_result.is_ok()); + + // with simd3, duplicate transactions are not retryable + if relax_intrabatch_account_locks && use_duplicate_transaction { + assert_eq!(retryable_transaction_indexes, Vec::::new()); + } else { + assert_eq!(retryable_transaction_indexes, vec![1]); + } } #[test] @@ -1350,30 +1356,48 @@ mod tests { ); } - #[test] - fn test_process_transactions_account_in_use() { + #[test_case(false, false; "old::locked")] + #[test_case(false, true; "old::duplicate")] + #[test_case(true, false; "simd83::locked")] + #[test_case(true, true; "simd83::duplicate")] + fn test_process_transactions_account_in_use( + relax_intrabatch_account_locks: bool, + use_duplicate_transaction: bool, + ) { solana_logger::setup(); let GenesisConfigInfo { genesis_config, mint_keypair, .. } = create_slow_genesis_config(10_000); - let (bank, _bank_forks) = Bank::new_no_wallclock_throttle_for_tests(&genesis_config); + let mut bank = Bank::new_for_tests(&genesis_config); + if !relax_intrabatch_account_locks { + bank.deactivate_feature(&agave_feature_set::relax_intrabatch_account_locks::id()); + } + bank.ns_per_slot = u128::MAX; + let (bank, _bank_forks) = bank.wrap_with_bank_forks_for_tests(); // set cost tracker limits to MAX so it will not filter out TXs bank.write_cost_tracker() .unwrap() .set_limits(u64::MAX, u64::MAX, u64::MAX); - // Make all repetitive transactions that conflict on the `mint_keypair`, so only 1 should be executed - let transactions = vec![ - system_transaction::transfer( + let mut transactions = vec![]; + let destination = Pubkey::new_unique(); + let mut amount = 1; + + // Make distinct, or identical, transactions that conflict on the `mint_keypair` + for _ in 0..TARGET_NUM_TRANSACTIONS_PER_BATCH { + transactions.push(system_transaction::transfer( &mint_keypair, - &Pubkey::new_unique(), - 1, - genesis_config.hash() - ); - TARGET_NUM_TRANSACTIONS_PER_BATCH - ]; + &destination, + amount, + genesis_config.hash(), + )); + + if !use_duplicate_transaction { + amount += 1; + } + } let transactions_len = transactions.len(); let ProcessTransactionBatchOutput { @@ -1381,7 +1405,14 @@ mod tests { .. } = execute_transactions_with_dummy_poh_service(bank, transactions); - // All the transactions should have been replayed, but only 2 committed (first and last) + // If SIMD-83 is enabled *and* the transactions are distinct, all are executed. + // In the three other cases, only one is executed. In all four cases, all are attempted. + let execution_count = if relax_intrabatch_account_locks && !use_duplicate_transaction { + transactions_len + } else { + 1 + } as u64; + assert_eq!( execute_and_commit_transactions_output .transaction_counts @@ -1392,20 +1423,29 @@ mod tests { execute_and_commit_transactions_output .transaction_counts .processed_count, - 1 + execution_count ); assert_eq!( execute_and_commit_transactions_output .transaction_counts .processed_with_successful_result_count, - 1 + execution_count ); - // Everything except first of the transactions failed and are retryable - assert_eq!( - execute_and_commit_transactions_output.retryable_transaction_indexes, - (1..transactions_len).collect::>() - ); + // If SIMD-83 is enabled and the transactions are distinct, there are zero retryable (all executed). + // If SIMD-83 is enabled and the transactions are identical, there are zero retryable (marked AlreadyProcessed). + // If SIMD-83 is not enabled, all but the first are retryable (marked AccountInUse). + if relax_intrabatch_account_locks { + assert_eq!( + execute_and_commit_transactions_output.retryable_transaction_indexes, + Vec::::new() + ); + } else { + assert_eq!( + execute_and_commit_transactions_output.retryable_transaction_indexes, + (1..transactions_len).collect::>() + ); + } } #[test] From 014e74315aecb9cdf4647bdf0de2acb861a396ed Mon Sep 17 00:00:00 2001 From: hana <81144685+2501babe@users.noreply.github.com> Date: Fri, 30 May 2025 08:23:38 -0700 Subject: [PATCH 09/10] use andrews key --- feature-set/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/feature-set/src/lib.rs b/feature-set/src/lib.rs index 78f1dcb3fa07b2..9b2d03e3266cdc 100644 --- a/feature-set/src/lib.rs +++ b/feature-set/src/lib.rs @@ -1059,7 +1059,7 @@ pub mod drop_unchained_merkle_shreds { } pub mod relax_intrabatch_account_locks { - solana_pubkey::declare_id!("B31HjiHtU5KCvzeGW2pa9DLDvrYazWdF6qFeKE5UQoqG"); + solana_pubkey::declare_id!("ENTRYnPAoT5Swwx73YDGzMp3XnNH1kxacyvLosRHza1i"); } pub mod create_slashing_program { From d957541e2efad87c0fce6f1d2f0f26c33c750919 Mon Sep 17 00:00:00 2001 From: hana <81144685+2501babe@users.noreply.github.com> Date: Fri, 30 May 2025 22:41:29 -0700 Subject: [PATCH 10/10] test cleanups --- core/src/banking_stage/consumer.rs | 4 ++-- ledger/src/blockstore_processor.rs | 6 +++++- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/core/src/banking_stage/consumer.rs b/core/src/banking_stage/consumer.rs index d38ac0b69f132e..42bbf748bc3fee 100644 --- a/core/src/banking_stage/consumer.rs +++ b/core/src/banking_stage/consumer.rs @@ -1214,7 +1214,7 @@ mod tests { ), ]); assert_eq!( - transactions[0].signature() == transactions[1].signature(), + transactions[0].message_hash() == transactions[1].message_hash(), use_duplicate_transaction ); @@ -1252,7 +1252,7 @@ mod tests { let consumer = Consumer::new(committer, recorder, QosService::new(1), None); // with simd83 and no duplicate, we take a cross-batch lock on an account to create a conflict - // with a duplicate transaction and simd83 it comes from signature equality in the batch + // with a duplicate transaction and simd83 it comes from message hash equality in the batch // without simd83 the conflict comes from locks in batch if relax_intrabatch_account_locks && !use_duplicate_transaction { let conflicting_transaction = diff --git a/ledger/src/blockstore_processor.rs b/ledger/src/blockstore_processor.rs index 1af6118157bd93..08d610ce6ab115 100644 --- a/ledger/src/blockstore_processor.rs +++ b/ledger/src/blockstore_processor.rs @@ -3805,8 +3805,12 @@ pub mod tests { bank.get_balance(&keypair2.pubkey()), ]; - assert!(result.is_err()); assert_eq!(balances, [5, 5]); + if relax_intrabatch_account_locks { + assert_eq!(result, Err(TransactionError::AlreadyProcessed)); + } else { + assert_eq!(result, Err(TransactionError::AccountInUse)); + } } #[test]