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

Include failed transaction in a block #1451

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

Unreleased changes, in reverse chronological order. New entries are added at the top of this list.

- [#1443](https://github.com/Zilliqa/zq2/pull/1451): Include failed transaction in blocks
- [#1472](https://github.com/Zilliqa/zq2/pull/1472): Add `GetTransactionsForTxBlockEx` API.
- [#1476](https://github.com/Zilliqa/zq2/pull/1476): Fix topic in EVM-encoded Scilla events.
- [#1474](https://github.com/Zilliqa/zq2/pull/1474): Fix reading `ByStr20` maps in Scilla via EVM.
Expand Down
74 changes: 65 additions & 9 deletions zilliqa/src/exec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,17 +9,16 @@ use std::{
sync::{Arc, MutexGuard},
};

use alloy::primitives::{hex, Address, U256};
use anyhow::{anyhow, Result};
use bytes::Bytes;
use alloy::primitives::{hex, Address, Bytes, U256};
use anyhow::{anyhow, bail, Result};
use eth_trie::{EthTrie, Trie};
use ethabi::Token;
use libp2p::PeerId;
use revm::{
inspector_handle_register,
primitives::{
AccountInfo, BlockEnv, Bytecode, Env, ExecutionResult, HaltReason, HandlerCfg, Output,
ResultAndState, SpecId, TxEnv, B256, KECCAK_EMPTY,
AccountInfo, AccountStatus, BlockEnv, Bytecode, EVMError, Env, ExecutionResult, HaltReason,
HandlerCfg, Output, ResultAndState, SpecId, TxEnv, B256, KECCAK_EMPTY,
},
Database, DatabaseRef, Evm, Inspector,
};
Expand All @@ -37,7 +36,7 @@ use crate::{
message::{Block, BlockHeader},
precompiles::{get_custom_precompiles, scilla_call_handle_register},
scilla::{self, split_storage_key, storage_key, Scilla},
state::{contract_addr, Account, Code, State},
state::{contract_addr, Account, Code, Code::Evm as EvmCode, State},
time::SystemTime,
transaction::{
total_scilla_gas_price, EvmGas, EvmLog, Log, ScillaGas, ScillaLog, ScillaParam,
Expand Down Expand Up @@ -497,9 +496,66 @@ impl State {
})
.build();

let e = evm.transact()?;
let transact_result = evm.transact();

let (mut state, cfg) = evm.into_db_and_env_with_handler_cfg();
Ok((e, state.finalize(), cfg.env))

let result_and_state = match transact_result {
Err(
EVMError::Transaction(_)
| EVMError::Header(_)
| EVMError::Custom(_)
| EVMError::Precompile(_),
) => {
let account = state.load_account(from_addr)?;
let Account {
code,
nonce,
balance,
..
} = &mut account.account;

let code_hash = match &code {
EvmCode(vec) if vec.is_empty() => KECCAK_EMPTY,
// Just signal that the code exists by returning non-KECCAK_EMPTY hash (we don't have to be precise here)
EvmCode(_) => B256::ZERO,
_ => KECCAK_EMPTY,
};
let code = match code {
EvmCode(code) => mem::take(code),
_ => bail!("Evm account contains non-evm code type!"),
};

// The only updated fields are nonce and balance, rest can be set to default (means: no update)
let affected_acc = revm::primitives::Account {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

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

Copy link
Contributor

@DrZoltanFazekas DrZoltanFazekas Oct 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

status: AccountStatus::default(),
storage: HashMap::new(),
info: AccountInfo {
balance: balance
.saturating_sub(gas_price * u128::from(gas_limit.0) + amount)
.try_into()?,
nonce: *nonce + 1,
code_hash,
code: Some(Bytecode::new_raw(code.into())),
},
};
let mut state = HashMap::new();
state.insert(from_addr, affected_acc);
ResultAndState {
result: ExecutionResult::Revert {
gas_used: 0u64,
output: Bytes::default(),
},
state,
}
}
Err(err) => {
bail!(err);
}
Ok(result) => result,
};

Ok((result_and_state, state.finalize(), cfg.env))
}

fn apply_transaction_scilla(
Expand Down Expand Up @@ -958,7 +1014,7 @@ impl State {
None,
current_block,
inspector::noop(),
BaseFeeCheck::Validate,
BaseFeeCheck::Ignore,
)?;

match result {
Expand Down
69 changes: 69 additions & 0 deletions zilliqa/tests/it/eth.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1306,3 +1306,72 @@ async fn deploy_deterministic_deployment_proxy(mut network: Network) {
.unwrap()
);
}

#[zilliqa_macros::test]
async fn include_failed_tx_in_block(mut network: Network) {
let wallet_1 = network.genesis_wallet().await;
let wallet_2 = network.random_wallet().await;

let provider = wallet_1.provider();

let funds = wallet_1
.get_balance(wallet_1.address(), None)
.await
.unwrap();
let block_gas_limit = network.nodes[0]
.inner
.lock()
.unwrap()
.config
.consensus
.eth_block_gas_limit
.0
- 1;

let gas_price = funds / block_gas_limit;
//let gas_price = (gas_price / 2) + 10;

// Send a transaction from wallet 1 to wallet 2.
let _hash_1 = wallet_1
.send_transaction(
TransactionRequest::pay(wallet_2.address(), 10)
.nonce(0)
.gas_price(gas_price)
.gas(block_gas_limit),
None,
)
.await
.unwrap()
.tx_hash();

// Send second transaction from wallet 1 to wallet 2.
let hash_2 = wallet_1
.send_transaction(
TransactionRequest::pay(wallet_2.address(), 10)
.nonce(1)
.gas_price(gas_price)
.gas(block_gas_limit),
None,
)
.await
.unwrap()
.tx_hash();

// Process pending transaction
network
.run_until_async(
|| async {
provider
.get_transaction_receipt(hash_2)
.await
.unwrap()
.is_some()
},
50,
)
.await
.unwrap();

let receipt_2 = provider.get_transaction_receipt(hash_2).await.unwrap();
assert_eq!(receipt_2.unwrap().status.unwrap(), U64::zero());
}