Remove special casing of ProgramError in blocktree processor

- Also refactor bank.rs and add unit tests

runtime/src/bank.rs

@@ -485,25 +485,11 @@ impl Bank {
         (loaded_accounts, executed)
     }
 
-    pub fn commit_transactions(
+    fn filter_program_errors_and_collect_fee(
         &self,
         txs: &[Transaction],
-        loaded_accounts: &[Result<(InstructionAccounts, InstructionLoaders)>],
         executed: &[Result<()>],
     ) -> Vec<Result<()>> {
-        let now = Instant::now();
-        self.accounts
-            .store_accounts(self.is_root(), txs, executed, loaded_accounts);
-
-        // once committed there is no way to unroll
-        let write_elapsed = now.elapsed();
-        debug!(
-            "store: {}us txs_len={}",
-            duration_as_us(&write_elapsed),
-            txs.len(),
-        );
-        self.update_transaction_statuses(txs, &executed);
-
         let mut fees = 0;
         let results = txs
             .iter()
@@ -526,6 +512,27 @@ impl Bank {
         results
     }
 
+    pub fn commit_transactions(
+        &self,
+        txs: &[Transaction],
+        loaded_accounts: &[Result<(InstructionAccounts, InstructionLoaders)>],
+        executed: &[Result<()>],
+    ) -> Vec<Result<()>> {
+        let now = Instant::now();
+        self.accounts
+            .store_accounts(self.is_root(), txs, executed, loaded_accounts);
+
+        // once committed there is no way to unroll
+        let write_elapsed = now.elapsed();
+        debug!(
+            "store: {}us txs_len={}",
+            duration_as_us(&write_elapsed),
+            txs.len(),
+        );
+        self.update_transaction_statuses(txs, &executed);
+        self.filter_program_errors_and_collect_fee(txs, executed)
+    }
+
     /// Process a batch of transactions.
     #[must_use]
     pub fn load_execute_and_commit_transactions(
@@ -882,8 +889,7 @@ mod tests {
         assert_eq!(bank.get_signature_status(&t1.signatures[0]), Some(Ok(())));
     }
 
-    // TODO: This test demonstrates that fees are not paid when a program fails.
-    // See github issue 1157 (https://github.com/solana-labs/solana/issues/1157)
+    // This test demonstrates that fees are paid even when a program fails.
     #[test]
     fn test_detect_failed_duplicate_transactions_issue_1157() {
         let (genesis_block, mint_keypair) = GenesisBlock::new(2);
@@ -900,7 +906,9 @@ mod tests {
         );
         let signature = tx.signatures[0];
         assert!(!bank.has_signature(&signature));
-        let _res = bank.process_transaction(&tx);
+
+        // Assert that process_transaction has filtered out Program Errors
+        assert_eq!(bank.process_transaction(&tx), Ok(()));
 
         assert!(bank.has_signature(&signature));
         assert_eq!(
@@ -1038,6 +1046,33 @@ mod tests {
         assert_eq!(bank.get_balance(&mint_keypair.pubkey()), 100 - 2 - 3);
     }
 
+    #[test]
+    fn test_filter_program_errors_and_collect_fee() {
+        let (genesis_block, mint_keypair) = GenesisBlock::new(100);
+        let mut bank = Bank::new(&genesis_block);
+        bank.leader = Pubkey::default();
+
+        let key = Keypair::new();
+        let tx1 =
+            SystemTransaction::new_move(&mint_keypair, key.pubkey(), 2, genesis_block.last_id(), 3);
+        let tx2 =
+            SystemTransaction::new_move(&mint_keypair, key.pubkey(), 5, genesis_block.last_id(), 1);
+
+        let results = vec![
+            Ok(()),
+            Err(BankError::ProgramError(
+                1,
+                ProgramError::ResultWithNegativeTokens,
+            )),
+        ];
+
+        let initial_balance = bank.get_balance(&bank.leader);
+        let results = bank.filter_program_errors_and_collect_fee(&vec![tx1, tx2], &results);
+        assert_eq!(bank.get_balance(&bank.leader), initial_balance + 3 + 1);
+        assert_eq!(results[0], Ok(()));
+        assert_eq!(results[1], Ok(()));
+    }
+
     #[test]
     fn test_debits_before_credits() {
         let (genesis_block, mint_keypair) = GenesisBlock::new(2);

src/blocktree_processor.rs

@@ -15,14 +15,7 @@ use std::time::Instant;
 
 pub fn process_entry(bank: &Bank, entry: &Entry) -> Result<()> {
     if !entry.is_tick() {
-        for result in bank.process_transactions(&entry.transactions) {
-            match result {
-                // Entries that result in a ProgramError are still valid and are written in the
-                // ledger so map them to an ok return value
-                Err(BankError::ProgramError(_, _)) => Ok(()),
-                _ => result,
-            }?;
-        }
+        first_err(&bank.process_transactions(&entry.transactions))?;
     } else {
         bank.register_tick(&entry.id);
     }
@@ -36,33 +29,16 @@ fn first_err(results: &[Result<()>]) -> Result<()> {
     Ok(())
 }
 
-fn ignore_program_errors(results: Vec<Result<()>>) -> Vec<Result<()>> {
-    results
-        .into_iter()
-        .map(|result| match result {
-            // Entries that result in a ProgramError are still valid and are written in the
-            // ledger so map them to an ok return value
-            Err(BankError::ProgramError(index, err)) => {
-                info!("program error {:?}, {:?}", index, err);
-                inc_new_counter_info!("bank-ignore_program_err", 1);
-                Ok(())
-            }
-            _ => result,
-        })
-        .collect()
-}
-
 fn par_execute_entries(bank: &Bank, entries: &[(&Entry, Vec<Result<()>>)]) -> Result<()> {
     inc_new_counter_info!("bank-par_execute_entries-count", entries.len());
     let results: Vec<Result<()>> = entries
         .into_par_iter()
         .map(|(e, lock_results)| {
-            let old_results = bank.load_execute_and_commit_transactions(
+            let results = bank.load_execute_and_commit_transactions(
                 &e.transactions,
                 lock_results.to_vec(),
                 MAX_ENTRY_IDS,
             );
-            let results = ignore_program_errors(old_results);
             bank.unlock_accounts(&e.transactions, &results);
             first_err(&results)
         })
@@ -409,29 +385,6 @@ mod tests {
         );
     }
 
-    #[test]
-    fn test_bank_ignore_program_errors() {
-        let expected_results = vec![Ok(()), Ok(())];
-        let results = vec![Ok(()), Ok(())];
-        let updated_results = ignore_program_errors(results);
-        assert_eq!(updated_results, expected_results);
-
-        let results = vec![
-            Err(BankError::ProgramError(
-                1,
-                ProgramError::ResultWithNegativeTokens,
-            )),
-            Ok(()),
-        ];
-        let updated_results = ignore_program_errors(results);
-        assert_eq!(updated_results, expected_results);
-
-        // Other BankErrors should not be ignored
-        let results = vec![Err(BankError::AccountNotFound), Ok(())];
-        let updated_results = ignore_program_errors(results);
-        assert_ne!(updated_results, expected_results);
-    }
-
     fn par_process_entries(bank: &Bank, entries: &[Entry]) -> Result<()> {
         let leader_scheduler = Arc::new(RwLock::new(LeaderScheduler::default()));
         par_process_entries_with_scheduler(bank, entries, &leader_scheduler)