Included failed transaction in a block

[bzawisto]

Currently failed transaction are not included in a block nor returned to the mempool making the client hung waiting for a receipt. This transaction should be included in a block with failed status (and should become queryable).

[JamesHinshelwood]

IMO apply_transaction shouldn't return Result<Option<TransactionApplyResult>> and this is what is causing the confusion. TransactionApplyResult can capture both successful and failed transactions and the outer Result captures unexpected system-level failures. So I don't know what the Option is for. Would something like this work? @bzawisto

diff --git a/zilliqa/src/consensus.rs b/zilliqa/src/consensus.rs
index 5db1426a..ba73cb2e 100644
--- a/zilliqa/src/consensus.rs
+++ b/zilliqa/src/consensus.rs
@@ -795,7 +795,7 @@ impl Consensus {
         txn: VerifiedTransaction,
         current_block: BlockHeader,
         inspector: I,
-    ) -> Result<Option<TransactionApplyResult>> {
+    ) -> Result<TransactionApplyResult> {
         let db = self.db.clone();
         let state = &mut self.state;
         Self::apply_transaction_at(state, db, txn, current_block, inspector)
@@ -807,27 +807,20 @@ impl Consensus {
         txn: VerifiedTransaction,
         current_block: BlockHeader,
         inspector: I,
-    ) -> Result<Option<TransactionApplyResult>> {
+    ) -> Result<TransactionApplyResult> {
         let hash = txn.hash;

         if !db.contains_transaction(&txn.hash)? {
             db.insert_transaction(&txn.hash, &txn.tx)?;
         }

-        let result = state.apply_transaction(txn.clone(), current_block, inspector);
-        let result = match result {
-            Ok(r) => r,
-            Err(error) => {
-                warn!(?hash, ?error, "transaction failed to execute");
-                return Ok(None);
-            }
-        };
+        let result = state.apply_transaction(txn.clone(), current_block, inspector)?;

         if !result.success() {
             info!("Transaction was a failure...");
         }

-        Ok(Some(result))
+        Ok(result)
     }

     pub fn get_txns_to_execute(&mut self) -> Vec<VerifiedTransaction> {
@@ -1219,11 +1212,6 @@ impl Consensus {
                 inspector::noop(),
             )?;

-            // Skip transactions whose execution resulted in an error and drop them.
-            let Some(result) = result else {
-                continue;
-            };
-
             // Decrement gas price and break loop if limit is exceeded
             gas_left = if let Some(g) = gas_left.checked_sub(result.gas_used()) {
                 g
@@ -2507,8 +2495,7 @@ impl Consensus {
             let tx_hash = txn.hash;
             let mut inspector = TouchedAddressInspector::default();
             let result = self
-                .apply_transaction(txn.clone(), block.header, &mut inspector)?
-                .ok_or_else(|| anyhow!("proposed transaction failed to execute"))?;
+                .apply_transaction(txn.clone(), block.header, &mut inspector)?;
             self.transaction_pool.mark_executed(&txn);
             for address in inspector.touched {
                 self.db.add_touched_address(address, tx_hash)?;

[bzawisto]

@JamesHinshelwood I'm afraid your proposal is not sufficient, the failed run captured by ResultAndState applies only to issues related to gas itself (out of gas) or any other evm-specific problems (e.g. invalid instruction). For funds the failure is reported as Result<EvmError> which should be handled separately.