Include failed transaction in a block

[bzawisto]

Failed transaction are now included in blocks. Additionally, balance checks shouldn't be done during gas estimation.

[JamesHinshelwood]

This feels like we are re-implementing things that should really be handled inside revm. I'm quite worried about doing this because when we did it before (when we depended on sputnik) we got lots of the edge cases wrong.

Just to summarise my understanding of the root issue here - The problem is that transactions get dropped from the transaction pool if we try to execute them and after executing, realise that there is not enough space left in the block. Could we not instead solve that issue by being more strict on the condition by which we consider executing a transaction - Instead of (optimistically) executing the next transaction and checking it fits, what if we checked the maximum gas spend of a transaction before executing it and stop building the block if it doesn't fit. That way, we only execute a transaction once we are certain it can be included in the block. reth does this and I don't think its a problem from the user's perspective if their transaction with an overly large gas limit gets included one block later than it theoretically could have.

Am I making sense? Happy to have a chat to discuss this instead if you prefer.

cc @DrZoltanFazekas

[bzawisto]

The problem here is actually different (matching block gas limit has already been implemented and merged).

There's a user that submits x transactions and at the time the block is proposed one of the transaction fails to execute because it turned out the user has no longer enough funds (all transactions entered mempool because each individually met the criteria).
Currently we just drop this transaction - it's neither included in a block nor queryable by get_transaction_receipt. User code keeps polling for receipt, but it never receives it so it just times out after several attempts.

In addition, revm handles such cases as Err(EVMError) - with InvalidTransaction. The same way it signals database errors. In such case you won't get ResultAndState.

There are a few ways of handling it:
1). Detect all but Database Errors and treat them as if they were regular failures (e.g. out of gas). In case of lack of funds, this will be just unsuccessful transaction that should be marked as failed. This approach requires doing what is done now or was done previously. Plus the nonce is increased and user pays a fee (which I believe is a behaviour similar to zq1). This is why revm::primitives::Account is used - we can then apply delta in apply_delta_evm.

2. Do the same thing as 1) but don't enforce fees and nonce mutation - then we just construct empty ResultAndState and no deltas are applied. User will have to resubmit txn with the same nonce - this could become

3. Handle failures and re-insert txn to mempool - this doesn't solve the problem of hung clients waiting for receipts but there's a hope that some other txn might be a transfer to this user and affected transaction will be included in next proposal (since lack of funds will no longer apply).

CC: @DrZoltanFazekas @JamesHinshelwood

[DrZoltanFazekas]

I preproduced the case on the Holesky testnet, which we can consider as a "reference" for correct behaviour. First I reduced the balance of my account 0xd819ffce7a58b1e835c25617db7b46a00888b013 to 0.000008323450116360 ether. Then I sent 10 simple transfer transactions knowing that after 5 of them my account will run out of gas. The first 5 transactions were mined in the same block, you can see them as the 5 topmost transactions in the list: https://holesky.etherscan.io/address/0xd819ffce7a58b1e835c25617db7b46a00888b013. I also got the receipts for those 5 transactions. Then my script got stuck waiting for further receipts and timed out minutes later with:

AggregateError: 
    at internalConnectMultiple (node:net:1118:18)
    at internalConnectMultiple (node:net:1186:5)
    at Timeout.internalConnectMultipleTimeout (node:net:1712:5)
    at listOnTimeout (node:internal/timers:583:11)
    at processTimers (node:internal/timers:519:7)

In the Holesky RPC node's mempool there was no pending or queued transaction from my account, so the other 5 transactions for which there wasn't enough balance left must have been dropped.

Note the difference between Holesky and our prototestnet: on the prototestnet the validators would drop the other 5 transactions, but they would stay in the API nodes' mempool as pending (forever). On Holesky, the 5 transactions were dropped on the API node too.