From abdc65b403afb4781884fbd07b727e280d6bfc21 Mon Sep 17 00:00:00 2001 From: Tao Zhu Date: Tue, 23 Jan 2024 18:19:24 +0000 Subject: [PATCH 1/4] Add test that combines account locking and blockspace reserving --- core/src/banking_stage/consumer.rs | 42 ++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/core/src/banking_stage/consumer.rs b/core/src/banking_stage/consumer.rs index 64b68889747633..84aff3692825c4 100644 --- a/core/src/banking_stage/consumer.rs +++ b/core/src/banking_stage/consumer.rs @@ -2510,4 +2510,46 @@ mod tests { [0, 3, 4, 5] ); } + + #[test] + fn test_process_and_record_transactions_with_pre_results() { + solana_logger::setup(); + let lamports = 100_000; + let GenesisConfigInfo { + genesis_config, + mint_keypair, + .. + } = create_slow_genesis_config(lamports); + + let bank = Bank::new_no_wallclock_throttle_for_tests(&genesis_config).0; + let test_txs = vec![system_transaction::transfer( + &mint_keypair, &Pubkey::new_unique(), 1, genesis_config.hash())]; + // a tx would "block" test txs due to shared mint_keypair + let blocker_txs = sanitize_transactions(vec![system_transaction::transfer( + &mint_keypair, &Pubkey::new_unique(), 1, genesis_config.hash())]); + + // set cost tracker limits to simulate "would fit" condition + bank.write_cost_tracker() + .unwrap() + .set_limits(10, std::u64::MAX, std::u64::MAX); + /* configure bank.accounts-lock + let blocker = bank.prepare_sanitized_batch(&blocker_txs); + info!("==== blocker: {:?} {}", blocker.lock_results(), blocker.needs_unlock()); + // */ + + let ProcessTransactionsSummary { + cost_model_throttled_transactions_count, + transactions_attempted_execution_count, + committed_transactions_count, + retryable_transaction_indexes, + .. + } = execute_transactions_with_dummy_poh_service( + bank.clone(), test_txs); + info!("==== result: {} {} {} {:?}", + cost_model_throttled_transactions_count, + transactions_attempted_execution_count, + committed_transactions_count, + retryable_transaction_indexes, + ); + } } From 0a221c19e17c5a63acc2968074652def1f20d10a Mon Sep 17 00:00:00 2001 From: Tao Zhu Date: Tue, 23 Jan 2024 18:41:23 +0000 Subject: [PATCH 2/4] Add test that combines account locking and blockspace reserving --- core/src/banking_stage/consumer.rs | 113 ++++++++++++++++++++++++----- 1 file changed, 95 insertions(+), 18 deletions(-) diff --git a/core/src/banking_stage/consumer.rs b/core/src/banking_stage/consumer.rs index 84aff3692825c4..e6acbba85eb324 100644 --- a/core/src/banking_stage/consumer.rs +++ b/core/src/banking_stage/consumer.rs @@ -2513,8 +2513,62 @@ mod tests { #[test] fn test_process_and_record_transactions_with_pre_results() { + let test_would_fit_and_lock = TestLockingAndReserving { + would_fit: true, + could_lock: true, + expected_cost_model_throttled_transactions_count: 0, + expected_transactions_attempted_execution_count: 1, + expected_committed_transactions_count: 1, + expected_retryable_transaction_count: 0, + }; + assert_process_transactions_with_locking_and_reserving(test_would_fit_and_lock); + + let test_would_fit_could_not_lock = TestLockingAndReserving { + would_fit: true, + could_lock: false, + expected_cost_model_throttled_transactions_count: 0, + expected_transactions_attempted_execution_count: 1, + expected_committed_transactions_count: 0, + expected_retryable_transaction_count: 1, + }; + assert_process_transactions_with_locking_and_reserving(test_would_fit_could_not_lock); + + let test_would_not_fit_could_lock = TestLockingAndReserving { + would_fit: false, + could_lock: true, + expected_cost_model_throttled_transactions_count: 1, + expected_transactions_attempted_execution_count: 1, + expected_committed_transactions_count: 0, + expected_retryable_transaction_count: 1, + }; + assert_process_transactions_with_locking_and_reserving(test_would_not_fit_could_lock); + + let test_would_not_fit_could_not_lock = TestLockingAndReserving { + would_fit: false, + could_lock: false, + expected_cost_model_throttled_transactions_count: 1, + expected_transactions_attempted_execution_count: 1, + expected_committed_transactions_count: 0, + expected_retryable_transaction_count: 1, + }; + assert_process_transactions_with_locking_and_reserving(test_would_not_fit_could_not_lock); + } + + // test setup for combined scenarios of accounts locking and block space reserving + struct TestLockingAndReserving { + // input: + would_fit: bool, + could_lock: bool, + // expectations + expected_cost_model_throttled_transactions_count: usize, + expected_transactions_attempted_execution_count: usize, + expected_committed_transactions_count: usize, + expected_retryable_transaction_count: usize, + } + + fn assert_process_transactions_with_locking_and_reserving(test: TestLockingAndReserving) { solana_logger::setup(); - let lamports = 100_000; + let lamports = 100_000; let GenesisConfigInfo { genesis_config, mint_keypair, @@ -2523,33 +2577,56 @@ mod tests { let bank = Bank::new_no_wallclock_throttle_for_tests(&genesis_config).0; let test_txs = vec![system_transaction::transfer( - &mint_keypair, &Pubkey::new_unique(), 1, genesis_config.hash())]; - // a tx would "block" test txs due to shared mint_keypair - let blocker_txs = sanitize_transactions(vec![system_transaction::transfer( - &mint_keypair, &Pubkey::new_unique(), 1, genesis_config.hash())]); + &mint_keypair, + &Pubkey::new_unique(), + 1, + genesis_config.hash(), + )]; - // set cost tracker limits to simulate "would fit" condition + // setup: if transcation should fail reserving space, set block_limit small. + let block_limit = if test.would_fit { std::u64::MAX } else { 1 }; bank.write_cost_tracker() .unwrap() - .set_limits(10, std::u64::MAX, std::u64::MAX); - /* configure bank.accounts-lock - let blocker = bank.prepare_sanitized_batch(&blocker_txs); - info!("==== blocker: {:?} {}", blocker.lock_results(), blocker.needs_unlock()); - // */ + .set_limits(block_limit, std::u64::MAX, std::u64::MAX); + // setup: if transaction should fail locking, pre-lock mint_keypair with blocker_txs + let blocker_txs = sanitize_transactions(vec![system_transaction::transfer( + &mint_keypair, + &Pubkey::new_unique(), + 1, + genesis_config.hash(), + )]); + let _blocker = if test.could_lock { + None + } else { + Some(bank.prepare_sanitized_batch(&blocker_txs)) + }; + + // execute: let ProcessTransactionsSummary { - cost_model_throttled_transactions_count, + cost_model_throttled_transactions_count, transactions_attempted_execution_count, committed_transactions_count, retryable_transaction_indexes, .. - } = execute_transactions_with_dummy_poh_service( - bank.clone(), test_txs); - info!("==== result: {} {} {} {:?}", - cost_model_throttled_transactions_count, + } = execute_transactions_with_dummy_poh_service(bank.clone(), test_txs); + + // check results: + assert_eq!( + cost_model_throttled_transactions_count, + test.expected_cost_model_throttled_transactions_count + ); + assert_eq!( transactions_attempted_execution_count, + test.expected_transactions_attempted_execution_count + ); + assert_eq!( committed_transactions_count, - retryable_transaction_indexes, - ); + test.expected_committed_transactions_count + ); + assert_eq!( + retryable_transaction_indexes.len(), + test.expected_retryable_transaction_count + ); } } From 7d7efcc668b427c81ef806f362dfec7479446674 Mon Sep 17 00:00:00 2001 From: Tao Zhu Date: Wed, 24 Jan 2024 02:00:04 +0000 Subject: [PATCH 3/4] [WIP] immediately un-reserve CU of txs that failed to get lock --- core/src/banking_stage/consumer.rs | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/core/src/banking_stage/consumer.rs b/core/src/banking_stage/consumer.rs index e6acbba85eb324..9482ed1266a1bb 100644 --- a/core/src/banking_stage/consumer.rs +++ b/core/src/banking_stage/consumer.rs @@ -460,7 +460,7 @@ impl Consumer { pre_results: impl Iterator>, ) -> ProcessTransactionBatchOutput { let ( - (transaction_qos_cost_results, cost_model_throttled_transactions_count), + (mut transaction_qos_cost_results, cost_model_throttled_transactions_count), cost_model_us, ) = measure_us!(self.qos_service.select_and_accumulate_transaction_costs( bank, @@ -479,6 +479,31 @@ impl Consumer { }) )); + // Remove reserved block space for transaction failed to acquire lock + let unlocked_cost_results: Vec<_> = batch + .lock_results() + .iter() + .zip(transaction_qos_cost_results.iter_mut()) + .map(|(lock_result, cost)| { + if let Err(_lock_err) = lock_result { + if let Ok(tx_cost) = cost { + // reset cost to lock_err, so it won't be accidentally removed more than once + // TODO *cost = Err(lock_err.clone()); + return Some(tx_cost); + } + } + None + }) + .collect(); + { + let mut cost_tracker = bank.write_cost_tracker().unwrap(); + unlocked_cost_results.iter().for_each(|tx_cost| { + if let Some(tx_cost) = tx_cost { + cost_tracker.remove(tx_cost); + } + }); + } + // retryable_txs includes AccountInUse, WouldExceedMaxBlockCostLimit // WouldExceedMaxAccountCostLimit, WouldExceedMaxVoteCostLimit // and WouldExceedMaxAccountDataCostLimit From 7c3941c568c55448cb0032e5609bad1d040da818 Mon Sep 17 00:00:00 2001 From: Tao Zhu Date: Wed, 24 Jan 2024 05:33:45 +0000 Subject: [PATCH 4/4] set entry in transaction_qos_cost_results to Err() if its tx failed to get lock --- core/src/banking_stage/consumer.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/core/src/banking_stage/consumer.rs b/core/src/banking_stage/consumer.rs index 9482ed1266a1bb..5ef9fe39771d26 100644 --- a/core/src/banking_stage/consumer.rs +++ b/core/src/banking_stage/consumer.rs @@ -485,11 +485,12 @@ impl Consumer { .iter() .zip(transaction_qos_cost_results.iter_mut()) .map(|(lock_result, cost)| { - if let Err(_lock_err) = lock_result { - if let Ok(tx_cost) = cost { - // reset cost to lock_err, so it won't be accidentally removed more than once - // TODO *cost = Err(lock_err.clone()); - return Some(tx_cost); + if let Err(lock_err) = lock_result { + if cost.is_ok() { + // set cost to lock_err, so it won't be accidentally removed more than once + let mut c = Err(lock_err.clone()); + std::mem::swap(cost, &mut c); + return Some(c.unwrap()); } } None