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

@@ -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: {}",

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
+        // else loop on processing the entry
-        let lock_results = bank.lock_accounts(&entry.transactions);
+        loop {
-        // if any of the locks error out
+            // try to lock the accounts
-        // execute the current group
+            let lock_results = bank.lock_accounts(&entry.transactions);
-        let first_lock_err = first_err(lock_results.locked_accounts_results());
+
-        if first_lock_err.is_err() {
+            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
+                // An entry has account lock conflicts with *itself*, which should not happen
-                // if generated by properly functioning leaders
+                // 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 {

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<Result<()>>;
@@ -191,6 +191,10 @@ impl Bank {
         self.slot
     }
 
+    pub fn freeze_lock(&self) -> RwLockReadGuard<Hash> {
+        self.hash.read().unwrap()
+    }
+
     pub fn hash(&self) -> Hash {
         *self.hash.read().unwrap()
     }