From d228b6467ccf93dc1632fad0185ea734371bc2eb Mon Sep 17 00:00:00 2001 From: carllin Date: Tue, 2 Apr 2019 03:55:42 -0700 Subject: [PATCH] Implement finalizer so that all locked accounts are dropped (#3585) * Implement finalizer so that all locked accounts are dropped when finalizer goes out of scope * Add test for tx error with lock conflict * Fix double unlock from destructor running after a call to unlock --- core/src/banking_stage.rs | 12 ++-- core/src/blocktree_processor.rs | 97 ++++++++++++++++++++++++-- runtime/src/bank.rs | 40 ++++++----- runtime/src/lib.rs | 1 + runtime/src/locked_accounts_results.rs | 42 +++++++++++ 5 files changed, 162 insertions(+), 30 deletions(-) create mode 100644 runtime/src/locked_accounts_results.rs diff --git a/core/src/banking_stage.rs b/core/src/banking_stage.rs index ecd847cd1f..1d4a1b0171 100644 --- a/core/src/banking_stage.rs +++ b/core/src/banking_stage.rs @@ -17,6 +17,7 @@ use crate::sigverify_stage::VerifiedPackets; use bincode::deserialize; use solana_metrics::counter::Counter; use solana_runtime::bank::{self, Bank}; +use solana_runtime::locked_accounts_results::LockedAccountsResults; use solana_sdk::pubkey::Pubkey; use solana_sdk::timing::{self, duration_as_us, MAX_RECENT_BLOCKHASHES}; use solana_sdk::transaction::{Transaction, TransactionError}; @@ -253,18 +254,15 @@ impl BankingStage { bank: &Bank, txs: &[Transaction], poh: &Arc>, - lock_results: &[bank::Result<()>], + lock_results: &LockedAccountsResults, ) -> Result<()> { let now = Instant::now(); // Use a shorter maximum age when adding transactions into the pipeline. This will reduce // the likelihood of any single thread getting starved and processing old ids. // TODO: Banking stage threads should be prioritized to complete faster then this queue // expires. - let (loaded_accounts, results) = bank.load_and_execute_transactions( - txs, - lock_results.to_vec(), - MAX_RECENT_BLOCKHASHES / 2, - ); + let (loaded_accounts, results) = + bank.load_and_execute_transactions(txs, lock_results, MAX_RECENT_BLOCKHASHES / 2); let load_execute_time = now.elapsed(); let record_time = { @@ -306,7 +304,7 @@ impl BankingStage { let now = Instant::now(); // Once the accounts are new transactions can enter the pipeline to process them - bank.unlock_accounts(&txs, &lock_results); + drop(lock_results); let unlock_time = now.elapsed(); debug!( diff --git a/core/src/blocktree_processor.rs b/core/src/blocktree_processor.rs index de018ea32e..5d6eec6a7f 100644 --- a/core/src/blocktree_processor.rs +++ b/core/src/blocktree_processor.rs @@ -5,6 +5,7 @@ use crate::leader_schedule_utils; use rayon::prelude::*; use solana_metrics::counter::Counter; use solana_runtime::bank::{Bank, Result}; +use solana_runtime::locked_accounts_results::LockedAccountsResults; use solana_sdk::genesis_block::GenesisBlock; use solana_sdk::timing::duration_as_ms; use solana_sdk::timing::MAX_RECENT_BLOCKHASHES; @@ -19,17 +20,16 @@ fn first_err(results: &[Result<()>]) -> Result<()> { Ok(()) } -fn par_execute_entries(bank: &Bank, entries: &[(&Entry, Vec>)]) -> Result<()> { +fn par_execute_entries(bank: &Bank, entries: &[(&Entry, LockedAccountsResults)]) -> Result<()> { inc_new_counter_info!("bank-par_execute_entries-count", entries.len()); let results: Vec> = entries .into_par_iter() - .map(|(e, lock_results)| { + .map(|(e, locked_accounts)| { let results = bank.load_execute_and_commit_transactions( &e.transactions, - lock_results.to_vec(), + locked_accounts, MAX_RECENT_BLOCKHASHES, ); - bank.unlock_accounts(&e.transactions, &results); first_err(&results) }) .collect(); @@ -57,11 +57,12 @@ pub fn process_entries(bank: &Bank, entries: &[Entry]) -> Result<()> { let lock_results = bank.lock_accounts(&entry.transactions); // if any of the locks error out // execute the current group - if first_err(&lock_results).is_err() { + if first_err(lock_results.locked_accounts_results()).is_err() { par_execute_entries(bank, &mt_group)?; + // Drop all the locks on accounts by clearing the LockedAccountsFinalizer's in the + // mt_group mt_group = vec![]; - //reset the lock and push the entry - bank.unlock_accounts(&entry.transactions, &lock_results); + drop(lock_results); let lock_results = bank.lock_accounts(&entry.transactions); mt_group.push((entry, lock_results)); } else { @@ -616,6 +617,88 @@ mod tests { assert_eq!(bank.get_balance(&keypair3.pubkey()), 2); } + #[test] + fn test_process_entries_2_txes_collision_and_error() { + let (genesis_block, mint_keypair) = GenesisBlock::new(1000); + let bank = Bank::new(&genesis_block); + let keypair1 = Keypair::new(); + let keypair2 = Keypair::new(); + let keypair3 = Keypair::new(); + let keypair4 = Keypair::new(); + + // fund: put 4 in each of 1 and 2 + assert_matches!(bank.transfer(4, &mint_keypair, &keypair1.pubkey()), Ok(_)); + assert_matches!(bank.transfer(4, &mint_keypair, &keypair2.pubkey()), Ok(_)); + assert_matches!(bank.transfer(4, &mint_keypair, &keypair4.pubkey()), Ok(_)); + + // construct an Entry whose 2nd transaction would cause a lock conflict with previous entry + let entry_1_to_mint = next_entry( + &bank.last_blockhash(), + 1, + vec![ + SystemTransaction::new_account( + &keypair1, + &mint_keypair.pubkey(), + 1, + bank.last_blockhash(), + 0, + ), + SystemTransaction::new_move( + &keypair4, + &keypair4.pubkey(), + 1, + Hash::default(), // Should cause a transaction failure with BlockhashNotFound + 0, + ), + ], + ); + + let entry_2_to_3_mint_to_1 = next_entry( + &entry_1_to_mint.hash, + 1, + vec![ + SystemTransaction::new_account( + &keypair2, + &keypair3.pubkey(), + 2, + bank.last_blockhash(), + 0, + ), // should be fine + SystemTransaction::new_account( + &keypair1, + &mint_keypair.pubkey(), + 2, + bank.last_blockhash(), + 0, + ), // will collide + ], + ); + + assert!(process_entries( + &bank, + &[entry_1_to_mint.clone(), entry_2_to_3_mint_to_1.clone()] + ) + .is_err()); + + // First transaction in first entry succeeded, so keypair1 lost 1 lamport + assert_eq!(bank.get_balance(&keypair1.pubkey()), 3); + assert_eq!(bank.get_balance(&keypair2.pubkey()), 4); + + // Check all accounts are unlocked + let txs1 = &entry_1_to_mint.transactions[..]; + let txs2 = &entry_2_to_3_mint_to_1.transactions[..]; + let locked_accounts1 = bank.lock_accounts(txs1); + for result in locked_accounts1.locked_accounts_results() { + assert!(result.is_ok()); + } + // txs1 and txs2 have accounts that conflict, so we must drop txs1 first + drop(locked_accounts1); + let locked_accounts2 = bank.lock_accounts(txs2); + for result in locked_accounts2.locked_accounts_results() { + assert!(result.is_ok()); + } + } + #[test] fn test_process_entries_2_entries_par() { let (genesis_block, mint_keypair) = GenesisBlock::new(1000); diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 92a6e033fc..a53513d743 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -5,6 +5,7 @@ use crate::accounts::{Accounts, ErrorCounters, InstructionAccounts, InstructionLoaders}; use crate::blockhash_queue::BlockhashQueue; +use crate::locked_accounts_results::LockedAccountsResults; use crate::runtime::{ProcessInstruction, Runtime}; use crate::status_cache::StatusCache; use bincode::serialize; @@ -467,18 +468,28 @@ impl Bank { .map_or(Ok(()), |sig| self.get_signature_status(sig).unwrap()) } - pub fn lock_accounts(&self, txs: &[Transaction]) -> Vec> { + pub fn lock_accounts<'a, 'b>( + &'a self, + txs: &'b [Transaction], + ) -> LockedAccountsResults<'a, 'b> { if self.is_frozen() { warn!("=========== FIXME: lock_accounts() working on a frozen bank! ================"); } // TODO: put this assert back in // assert!(!self.is_frozen()); - self.accounts.lock_accounts(self.accounts_id, txs) + let results = self.accounts.lock_accounts(self.accounts_id, txs); + LockedAccountsResults::new(results, &self, txs) } - pub fn unlock_accounts(&self, txs: &[Transaction], results: &[Result<()>]) { - self.accounts - .unlock_accounts(self.accounts_id, txs, results) + pub fn unlock_accounts(&self, locked_accounts_results: &mut LockedAccountsResults) { + if locked_accounts_results.needs_unlock { + locked_accounts_results.needs_unlock = false; + self.accounts.unlock_accounts( + self.accounts_id, + locked_accounts_results.transactions(), + locked_accounts_results.locked_accounts_results(), + ) + } } fn load_accounts( @@ -498,17 +509,17 @@ impl Bank { fn check_refs( &self, txs: &[Transaction], - lock_results: Vec>, + lock_results: &LockedAccountsResults, error_counters: &mut ErrorCounters, ) -> Vec> { txs.iter() - .zip(lock_results.into_iter()) + .zip(lock_results.locked_accounts_results()) .map(|(tx, lock_res)| { if lock_res.is_ok() && !tx.verify_refs() { error_counters.invalid_account_index += 1; Err(TransactionError::InvalidAccountIndex) } else { - lock_res + lock_res.clone() } }) .collect() @@ -575,7 +586,7 @@ impl Bank { pub fn load_and_execute_transactions( &self, txs: &[Transaction], - lock_results: Vec>, + lock_results: &LockedAccountsResults, max_age: usize, ) -> ( Vec>, @@ -741,7 +752,7 @@ impl Bank { pub fn load_execute_and_commit_transactions( &self, txs: &[Transaction], - lock_results: Vec>, + lock_results: &LockedAccountsResults, max_age: usize, ) -> Vec> { let (loaded_accounts, executed) = @@ -753,10 +764,7 @@ impl Bank { #[must_use] pub fn process_transactions(&self, txs: &[Transaction]) -> Vec> { let lock_results = self.lock_accounts(txs); - let results = - self.load_execute_and_commit_transactions(txs, lock_results, MAX_RECENT_BLOCKHASHES); - self.unlock_accounts(txs, &results); - results + self.load_execute_and_commit_transactions(txs, &lock_results, MAX_RECENT_BLOCKHASHES) } /// Create, sign, and process a Transaction from `keypair` to `to` of @@ -1312,7 +1320,7 @@ mod tests { let lock_result = bank.lock_accounts(&pay_alice); let results_alice = bank.load_execute_and_commit_transactions( &pay_alice, - lock_result, + &lock_result, MAX_RECENT_BLOCKHASHES, ); assert_eq!(results_alice[0], Ok(())); @@ -1329,7 +1337,7 @@ mod tests { Err(TransactionError::AccountInUse) ); - bank.unlock_accounts(&pay_alice, &results_alice); + drop(lock_result); assert!(bank.transfer(2, &mint_keypair, &bob.pubkey()).is_ok()); } diff --git a/runtime/src/lib.rs b/runtime/src/lib.rs index 674dfa90d4..0732a9db22 100644 --- a/runtime/src/lib.rs +++ b/runtime/src/lib.rs @@ -5,6 +5,7 @@ pub mod bank_client; mod blockhash_queue; pub mod bloom; pub mod loader_utils; +pub mod locked_accounts_results; mod native_loader; pub mod runtime; mod status_cache; diff --git a/runtime/src/locked_accounts_results.rs b/runtime/src/locked_accounts_results.rs new file mode 100644 index 0000000000..98d8845698 --- /dev/null +++ b/runtime/src/locked_accounts_results.rs @@ -0,0 +1,42 @@ +use crate::bank::{Bank, Result}; +use solana_sdk::transaction::Transaction; + +// Represents the results of trying to lock a set of accounts +pub struct LockedAccountsResults<'a, 'b> { + locked_accounts_results: Vec>, + bank: &'a Bank, + transactions: &'b [Transaction], + pub(crate) needs_unlock: bool, +} + +impl<'a, 'b> LockedAccountsResults<'a, 'b> { + pub fn new( + locked_accounts_results: Vec>, + bank: &'a Bank, + transactions: &'b [Transaction], + ) -> Self { + Self { + locked_accounts_results, + bank, + transactions, + needs_unlock: true, + } + } + + pub fn locked_accounts_results(&self) -> &Vec> { + &self.locked_accounts_results + } + + pub fn transactions(&self) -> &[Transaction] { + self.transactions + } +} + +// Unlock all locked accounts in destructor. +impl<'a, 'b> Drop for LockedAccountsResults<'a, 'b> { + fn drop(&mut self) { + if self.needs_unlock { + self.bank.unlock_accounts(self) + } + } +}