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..c6ecea9fd26f60 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 = 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()), MAX_TX_ACCOUNT_LOCKS); + let results = accounts.lock_accounts( + black_box(batch.iter()), + batch_results.clone(), + MAX_TX_ACCOUNT_LOCKS, + relax_intrabatch_account_locks, + ); accounts.unlock_accounts(batch.iter().zip(&results)); } }) diff --git a/accounts-db/src/account_locks.rs b/accounts-db/src/account_locks.rs index 43796937b8cc42..113b41231dd737 100644 --- a/accounts-db/src/account_locks.rs +++ b/accounts-db/src/account_locks.rs @@ -5,13 +5,13 @@ use { solana_message::AccountKeys, solana_pubkey::Pubkey, solana_transaction::sanitized::MAX_TX_ACCOUNT_LOCKS, - solana_transaction_error::TransactionError, + solana_transaction_error::{TransactionError, TransactionResult}, std::{cell::RefCell, collections::hash_map}, }; #[derive(Debug, Default)] pub struct AccountLocks { - write_locks: AHashSet, + write_locks: AHashMap, readonly_locks: AHashMap, } @@ -20,29 +20,40 @@ 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> { - for (key, writable) in keys.clone() { - if writable { - if !self.can_write_lock(key) { - return Err(TransactionError::AccountInUse); - } - } else if !self.can_read_lock(key) { - return Err(TransactionError::AccountInUse); - } - } + ) -> TransactionResult<()> { + self.can_lock_accounts(keys.clone())?; + self.lock_accounts(keys); - for (key, writable) in keys { - if writable { - self.lock_write(key); - } else { - self.lock_readonly(key); + 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, + mut validated_batch_keys: Vec< + TransactionResult + Clone>, + >, + ) -> Vec> { + validated_batch_keys.iter_mut().for_each(|validated_keys| { + if let Ok(ref keys) = validated_keys { + if let Err(e) = self.can_lock_accounts(keys.clone()) { + *validated_keys = Err(e); + } } - } + }); - Ok(()) + validated_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. @@ -59,6 +70,33 @@ impl AccountLocks { } } + fn can_lock_accounts<'a>( + &self, + keys: impl Iterator, + ) -> TransactionResult<()> { + for (key, writable) in keys { + if writable { + if !self.can_write_lock(key) { + return Err(TransactionError::AccountInUse); + } + } else if !self.can_read_lock(key) { + return Err(TransactionError::AccountInUse); + } + } + + Ok(()) + } + + fn lock_accounts<'a>(&mut self, keys: impl Iterator) { + for (key, writable) in keys { + if writable { + self.lock_write(key); + } else { + self.lock_readonly(key); + } + } + } + #[cfg_attr(feature = "dev-context-only-utils", qualifiers(pub))] fn is_locked_readonly(&self, key: &Pubkey) -> bool { self.readonly_locks.get(key).is_some_and(|count| *count > 0) @@ -66,7 +104,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 +122,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 +141,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 +160,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..9c06ee08305e9a 100644 --- a/accounts-db/src/accounts.rs +++ b/accounts-db/src/accounts.rs @@ -566,57 +566,39 @@ impl Accounts { } /// This function will prevent multiple threads from modifying the same account state at the - /// same time + /// same time, possibly excluding transactions based on prior results #[must_use] - pub fn lock_accounts<'a, Tx: SVMMessage + 'a>( - &self, - txs: impl Iterator, - tx_account_lock_limit: usize, - ) -> 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) - } - - #[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: Vec> = txs + // 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)| match result { - Ok(()) => validate_account_locks(tx.account_keys(), tx_account_lock_limit) - .map(|_| TransactionAccountLocksIterator::new(tx)), - Err(err) => Err(err), + .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(); - self.lock_accounts_inner(tx_account_locks_results) - } + .collect::>(); - #[must_use] - fn lock_accounts_inner( - &self, - tx_account_locks_results: Vec>>, - ) -> 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 { + 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), + }) + .collect() + } } /// Once accounts are unlocked, new transactions that modify that state can enter the pipeline @@ -662,15 +644,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 +665,7 @@ mod tests { sync::atomic::{AtomicBool, AtomicU64, Ordering}, thread, time, }, + test_case::test_case, }; fn new_sanitized_tx( @@ -694,6 +680,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 +898,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 +915,18 @@ 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(), + [Ok(())].into_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 +949,12 @@ 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(), + vec![Ok(()); txs.len()].into_iter(), + MAX_TX_ACCOUNT_LOCKS, + relax_intrabatch_account_locks, + ); assert_eq!(results, vec![Ok(())]); accounts.unlock_accounts(txs.iter().zip(&results)); } @@ -961,13 +976,19 @@ 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(), + vec![Ok(()); txs.len()].into_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 +1016,12 @@ 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(), + [Ok(())].into_iter(), + MAX_TX_ACCOUNT_LOCKS, + relax_intrabatch_account_locks, + ); assert_eq!(results0, vec![Ok(())]); assert!(accounts @@ -1025,7 +1051,12 @@ 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(), + vec![Ok(()); txs.len()].into_iter(), + MAX_TX_ACCOUNT_LOCKS, + relax_intrabatch_account_locks, + ); assert_eq!( results1, vec![ @@ -1051,7 +1082,12 @@ 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(), + [Ok(())].into_iter(), + MAX_TX_ACCOUNT_LOCKS, + relax_intrabatch_account_locks, + ); assert_eq!( results2, vec![Ok(())] // Now keypair1 account can be locked as writable @@ -1065,8 +1101,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 +1150,12 @@ 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(), + vec![Ok(()); txs.len()].into_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 +1169,12 @@ 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(), + vec![Ok(()); txs.len()].into_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 +1186,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 +1216,12 @@ 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(), + [Ok(())].into_iter(), + MAX_TX_ACCOUNT_LOCKS, + relax_intrabatch_account_locks, + ); assert!(results0[0].is_ok()); // Instruction program-id account demoted to readonly @@ -1208,8 +1257,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(); @@ -1265,10 +1315,11 @@ 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, + relax_intrabatch_account_locks, ); assert_eq!( @@ -1294,6 +1345,94 @@ 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(), + [Ok(())].into_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(), + [Ok(())].into_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(), + [Ok(())].into_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(), + [Ok(()), Ok(())].into_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(), + [Ok(()), Ok(())].into_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..42bbf748bc3fee 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].message_hash() == transactions[1].message_hash(), + 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 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 = + 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); @@ -1237,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] @@ -1297,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 { @@ -1328,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 @@ -1339,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] diff --git a/feature-set/src/lib.rs b/feature-set/src/lib.rs index 02d92c361c234e..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!("EbAhnReKK8Sf88CvAfAXbgKji8DV48rsp4q2sgHqgWef"); + solana_pubkey::declare_id!("ENTRYnPAoT5Swwx73YDGzMp3XnNH1kxacyvLosRHza1i"); } pub mod create_slashing_program { diff --git a/ledger/src/blockstore_processor.rs b/ledger/src/blockstore_processor.rs index b4d04523134fcf..08d610ce6ab115 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,100 @@ 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_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_eq!(balances, [5, 5]); + if relax_intrabatch_account_locks { + assert_eq!(result, Err(TransactionError::AlreadyProcessed)); + } else { + assert_eq!(result, Err(TransactionError::AccountInUse)); + } } #[test] @@ -4041,14 +4120,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 +4141,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 +4153,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..2921573089427b 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -3175,53 +3175,77 @@ 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), )) } /// 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(()))) + } + + /// 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 TransactionWithMeta], + 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()); + + // 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, + 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)) + 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 +3265,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 +7137,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();