diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 66a6092127..14135b7250 100644 --- a/runtime/src/bank.rs +++ b/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> { - 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> { + 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); diff --git a/src/blocktree_processor.rs b/src/blocktree_processor.rs index 7b5a10d85d..0a3b9cdf47 100644 --- a/src/blocktree_processor.rs +++ b/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>) -> Vec> { - 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<()> { inc_new_counter_info!("bank-par_execute_entries-count", entries.len()); let results: Vec> = 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)