Fix mismatch between leader/validator bank votability (#3942)

* Fix mismatch between leader/validator bank votability

core/src/banking_stage.rs

@@ -20,7 +20,7 @@ use solana_runtime::bank::Bank;
 use solana_runtime::locked_accounts_results::LockedAccountsResults;
 use solana_sdk::pubkey::Pubkey;
 use solana_sdk::timing::{self, duration_as_us, DEFAULT_TICKS_PER_SLOT, MAX_RECENT_BLOCKHASHES};
-use solana_sdk::transaction::{self, Transaction, TransactionError};
+use solana_sdk::transaction::{self, Transaction};
 use std::cmp;
 use std::net::UdpSocket;
 use std::sync::atomic::{AtomicBool, Ordering};
@@ -367,14 +367,10 @@ impl BankingStage {
         let processed_transactions: Vec<_> = results
             .iter()
             .zip(txs.iter())
-            .filter_map(|(r, x)| match r {
-                Ok(_) => Some(x.clone()),
-                Err(TransactionError::InstructionError(index, err)) => {
-                    debug!("instruction error {:?}, {:?}", index, err);
+            .filter_map(|(r, x)| {
+                if Bank::can_commit(r) {
                     Some(x.clone())
-                }
-                Err(ref e) => {
-                    debug!("process transaction failed {:?}", e);
+                } else {
                     None
                 }
             })
@@ -667,6 +663,7 @@ mod tests {
     use solana_sdk::instruction::InstructionError;
     use solana_sdk::signature::{Keypair, KeypairUtil};
     use solana_sdk::system_transaction;
+    use solana_sdk::transaction::TransactionError;
     use std::sync::mpsc::channel;
     use std::thread::sleep;
 

runtime/src/bank.rs

@@ -423,33 +423,26 @@ impl Bank {
         self.status_cache.write().unwrap().clear_signatures();
     }
 
+    pub fn can_commit(result: &Result<()>) -> bool {
+        match result {
+            Ok(_) => true,
+            Err(TransactionError::InstructionError(_, _)) => true,
+            Err(_) => false,
+        }
+    }
+
     fn update_transaction_statuses(&self, txs: &[Transaction], res: &[Result<()>]) {
         let mut status_cache = self.status_cache.write().unwrap();
         for (i, tx) in txs.iter().enumerate() {
-            match &res[i] {
-                Ok(_) => {
-                    if !tx.signatures.is_empty() {
+            if Self::can_commit(&res[i]) && !tx.signatures.is_empty() {
                 status_cache.insert(
                     &tx.message().recent_blockhash,
                     &tx.signatures[0],
                     self.slot(),
-                            Ok(()),
+                    res[i].clone(),
                 );
             }
         }
-                Err(TransactionError::InstructionError(b, e)) => {
-                    if !tx.signatures.is_empty() {
-                        status_cache.insert(
-                            &tx.message().recent_blockhash,
-                            &tx.signatures[0],
-                            self.slot(),
-                            Err(TransactionError::InstructionError(*b, e.clone())),
-                        );
-                    }
-                }
-                Err(_) => (),
-            }
-        }
     }
 
     /// Looks through a list of tick heights and stakes, and finds the latest
@@ -769,7 +762,9 @@ impl Bank {
             warn!("=========== FIXME: commit_transactions() working on a frozen bank! ================");
         }
 
+        if executed.iter().any(|res| Self::can_commit(res)) {
             self.is_delta.store(true, Ordering::Relaxed);
+        }
 
         // TODO: put this assert back in
         // assert!(!self.is_frozen());
@@ -1952,4 +1947,44 @@ mod tests {
         bank.tick_height.store(max_tick_height, Ordering::Relaxed);
         assert!(bank.is_votable());
     }
+
+    #[test]
+    fn test_is_delta_with_no_committables() {
+        let (genesis_block, mint_keypair) = GenesisBlock::new(8000);
+        let bank = Bank::new(&genesis_block);
+        bank.is_delta.store(false, Ordering::Relaxed);
+
+        let keypair1 = Keypair::new();
+        let keypair2 = Keypair::new();
+        let fail_tx = system_transaction::create_user_account(
+            &keypair1,
+            &keypair2.pubkey(),
+            1,
+            bank.last_blockhash(),
+            0,
+        );
+
+        // Should fail with TransactionError::AccountNotFound, which means
+        // the account which this tx operated on will not be committed. Thus
+        // the bank is_delta should still be false
+        assert_eq!(
+            bank.process_transaction(&fail_tx),
+            Err(TransactionError::AccountNotFound)
+        );
+
+        // Check the bank is_delta is still false
+        assert!(!bank.is_delta.load(Ordering::Relaxed));
+
+        // Should fail with InstructionError, but InstructionErrors are committable,
+        // so is_delta should be true
+        assert_eq!(
+            bank.transfer(10_001, &mint_keypair, &Pubkey::new_rand()),
+            Err(TransactionError::InstructionError(
+                0,
+                InstructionError::new_result_with_negative_lamports(),
+            ))
+        );
+
+        assert!(bank.is_delta.load(Ordering::Relaxed));
+    }
 }