From 35e8f966e36b81cc37d30afa6c915b7ed724b843 Mon Sep 17 00:00:00 2001 From: Rob Walker Date: Thu, 23 May 2019 17:35:15 -0700 Subject: [PATCH] add freeze_lock() and fix par_process_entries() failure to detect self conflict (#4415) * add freeze_lock and fix par_process_entries failure to detect self conflict * fixup * fixup --- core/src/banking_stage.rs | 3 + core/src/blocktree_processor.rs | 144 +++++++++++++++++++++++++++----- runtime/src/bank.rs | 6 +- 3 files changed, 131 insertions(+), 22 deletions(-) diff --git a/core/src/banking_stage.rs b/core/src/banking_stage.rs index 96c89e4d5..823659d32 100644 --- a/core/src/banking_stage.rs +++ b/core/src/banking_stage.rs @@ -427,6 +427,8 @@ impl BankingStage { bank.load_and_execute_transactions(txs, lock_results, MAX_RECENT_BLOCKHASHES / 2); let load_execute_time = now.elapsed(); + let freeze_lock = bank.freeze_lock(); + let mut recordable_txs = vec![]; let (record_time, record_locks) = { let now = Instant::now(); @@ -442,6 +444,7 @@ impl BankingStage { }; drop(record_locks); + drop(freeze_lock); debug!( "bank: {} load_execute: {}us record: {}us commit: {}us txs_len: {}", diff --git a/core/src/blocktree_processor.rs b/core/src/blocktree_processor.rs index dc9afbf81..3bae7ba22 100644 --- a/core/src/blocktree_processor.rs +++ b/core/src/blocktree_processor.rs @@ -17,7 +17,9 @@ use std::time::{Duration, Instant}; fn first_err(results: &[Result<()>]) -> Result<()> { for r in results { - r.clone()?; + if r.is_err() { + return r.clone(); + } } Ok(()) } @@ -69,19 +71,28 @@ pub fn process_entries(bank: &Bank, entries: &[Entry]) -> Result<()> { if entry.is_tick() { // if its a tick, execute the group and register the tick par_execute_entries(bank, &mt_group)?; - bank.register_tick(&entry.hash); mt_group = vec![]; + bank.register_tick(&entry.hash); continue; } - // try to lock the accounts - let lock_results = bank.lock_accounts(&entry.transactions); - // if any of the locks error out - // execute the current group - let first_lock_err = first_err(lock_results.locked_accounts_results()); - if first_lock_err.is_err() { + // else loop on processing the entry + loop { + // try to lock the accounts + let lock_results = bank.lock_accounts(&entry.transactions); + + let first_lock_err = first_err(lock_results.locked_accounts_results()); + + // if locking worked + if first_lock_err.is_ok() { + // push the entry to the mt_group + mt_group.push((entry, lock_results)); + // done with this entry + break; + } + // else we failed to lock, 2 possible reasons if mt_group.is_empty() { - // An entry has account lock conflicts with itself, which should not happen - // if generated by properly functioning leaders + // An entry has account lock conflicts with *itself*, which should not happen + // if generated by a properly functioning leader datapoint!( "validator_process_entry_error", ( @@ -93,19 +104,14 @@ pub fn process_entries(bank: &Bank, entries: &[Entry]) -> Result<()> { String ) ); - + // bail first_lock_err?; + } else { + // else we have an entry that conflicts with a prior entry + // execute the current queue and try to process this entry again + par_execute_entries(bank, &mt_group)?; + mt_group = vec![]; } - par_execute_entries(bank, &mt_group)?; - // Drop all the locks on accounts by clearing the LockedAccountsFinalizer's in the - // mt_group - mt_group = vec![]; - drop(lock_results); - let lock_results = bank.lock_accounts(&entry.transactions); - mt_group.push((entry, lock_results)); - } else { - // push the entry to the mt_group - mt_group.push((entry, lock_results)); } } par_execute_entries(bank, &mt_group)?; @@ -938,6 +944,102 @@ pub mod tests { } } + #[test] + fn test_process_entries_2nd_entry_collision_with_self_and_error() { + solana_logger::setup(); + + let GenesisBlockInfo { + genesis_block, + mint_keypair, + .. + } = create_genesis_block(1000); + let bank = Bank::new(&genesis_block); + let keypair1 = Keypair::new(); + let keypair2 = Keypair::new(); + let keypair3 = 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(4, &mint_keypair, &keypair2.pubkey()), Ok(_)); + + // 3 entries: first has a transfer, 2nd has a conflict with 1st, 3rd has a conflict with itself + let entry_1_to_mint = next_entry( + &bank.last_blockhash(), + 1, + vec![system_transaction::transfer( + &keypair1, + &mint_keypair.pubkey(), + 1, + bank.last_blockhash(), + )], + ); + // should now be: + // keypair1=4 + // keypair2=4 + // keypair3=0 + + let entry_2_to_3_and_1_to_mint = next_entry( + &entry_1_to_mint.hash, + 1, + vec![ + system_transaction::create_user_account( + &keypair2, + &keypair3.pubkey(), + 2, + bank.last_blockhash(), + ), // should be fine + system_transaction::transfer( + &keypair1, + &mint_keypair.pubkey(), + 2, + bank.last_blockhash(), + ), // will collide with predecessor + ], + ); + // should now be: + // keypair1=2 + // keypair2=2 + // keypair3=2 + + let entry_conflict_itself = next_entry( + &entry_2_to_3_and_1_to_mint.hash, + 1, + vec![ + system_transaction::transfer( + &keypair1, + &keypair3.pubkey(), + 1, + bank.last_blockhash(), + ), + system_transaction::transfer( + &keypair1, + &keypair2.pubkey(), + 1, + bank.last_blockhash(), + ), // should be fine + ], + ); + // would now be: + // keypair1=0 + // keypair2=3 + // keypair3=3 + + assert!(process_entries( + &bank, + &[ + entry_1_to_mint.clone(), + entry_2_to_3_and_1_to_mint.clone(), + entry_conflict_itself.clone() + ] + ) + .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); + } + #[test] fn test_process_entries_2_entries_par() { let GenesisBlockInfo { diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 359e5262f..29ee49e66 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -31,7 +31,7 @@ use solana_sdk::transaction::{Result, Transaction, TransactionError}; use std::borrow::Borrow; use std::cmp; use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; -use std::sync::{Arc, RwLock}; +use std::sync::{Arc, RwLock, RwLockReadGuard}; use std::time::Instant; type BankStatusCache = StatusCache>; @@ -191,6 +191,10 @@ impl Bank { self.slot } + pub fn freeze_lock(&self) -> RwLockReadGuard { + self.hash.read().unwrap() + } + pub fn hash(&self) -> Hash { *self.hash.read().unwrap() }