Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Included failed transaction in a block #1443

Open
bzawisto opened this issue Sep 12, 2024 · 2 comments · May be fixed by #1451
Open

Included failed transaction in a block #1443

bzawisto opened this issue Sep 12, 2024 · 2 comments · May be fixed by #1451
Assignees
Labels
bug Something isn't working phase1 proto-testnet

Comments

@bzawisto
Copy link
Contributor

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).

@bzawisto bzawisto added the phase1 proto-testnet label Sep 12, 2024
@bzawisto bzawisto self-assigned this Sep 12, 2024
@JamesHinshelwood
Copy link
Contributor

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)?;

@DrZoltanFazekas DrZoltanFazekas added the bug Something isn't working label Sep 12, 2024
@bzawisto
Copy link
Contributor Author

@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.

@bzawisto bzawisto linked a pull request Sep 13, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working phase1 proto-testnet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants