From db16a7935f1a1d065fe730441f1d98f78bcafa42 Mon Sep 17 00:00:00 2001 From: Enrique Ortiz Date: Fri, 5 May 2023 16:45:26 -0400 Subject: [PATCH 1/8] fix(executor): throw proper errors and ignore prevrandao error --- anvil/src/eth/backend/executor.rs | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/anvil/src/eth/backend/executor.rs b/anvil/src/eth/backend/executor.rs index 0ae5f5eb6ead9..a7282245b16e5 100644 --- a/anvil/src/eth/backend/executor.rs +++ b/anvil/src/eth/backend/executor.rs @@ -18,7 +18,7 @@ use ethers::{ utils::rlp, }; use forge::{ - revm::primitives::ExecutionResult, + revm::primitives::{EVMError, ExecutionResult}, utils::{ b160_to_h160, eval_to_instruction_result, h160_to_b160, halt_to_instruction_result, ru256_to_u256, @@ -271,10 +271,19 @@ impl<'a, 'b, DB: Db + ?Sized, Validator: TransactionValidator> Iterator Ok(exec_result) => exec_result, Err(err) => { warn!(target: "backend", "[{:?}] failed to execute: {:?}", transaction.hash(), err); - return Some(TransactionExecutionOutcome::DatabaseError( - transaction, - DatabaseError::TransactionNotFound(H256::from_str("0x").unwrap()), - )) + match err { + EVMError::Database(err) => { + return Some(TransactionExecutionOutcome::DatabaseError(transaction, err)) + } + EVMError::Transaction(err) => { + return Some(TransactionExecutionOutcome::Invalid(transaction, err.into())) + } + // This will correspond to prevrandao not set, and it should never happen. + // If it does, it's a bug. + e => { + panic!("Failed to execute transaction. This is a bug.\n {:?}", e) + } + } } }; inspector.print_logs(); From 1b4df121c3a9a438779465c5e41c77d59ac98918 Mon Sep 17 00:00:00 2001 From: Enrique Ortiz Date: Fri, 5 May 2023 16:45:53 -0400 Subject: [PATCH 2/8] fix(anvil): port reth gas estimation --- anvil/src/eth/api.rs | 172 +++++++++++++++++++++++++------------------ 1 file changed, 99 insertions(+), 73 deletions(-) diff --git a/anvil/src/eth/api.rs b/anvil/src/eth/api.rs index 40da2aedf14e0..089ee9f697b95 100644 --- a/anvil/src/eth/api.rs +++ b/anvil/src/eth/api.rs @@ -2022,12 +2022,22 @@ impl EthApi { call_to_estimate.gas = Some(gas_limit); // execute the call without writing to db - let (exit, out, gas, _) = self.backend.call_with_state( - &state, - call_to_estimate, - fees.clone(), - block_env.clone(), - )?; + let ethres = + self.backend.call_with_state(&state, call_to_estimate, fees.clone(), block_env.clone()); + + // Exceptional case: init used too much gas, we need to increase the gas limit and try + // again + if let Err(BlockchainError::InvalidTransaction(InvalidTransactionError::GasTooHigh)) = + ethres + { + // if price or limit was included in the request then we can execute the request + // again with the block's gas limit to check if revert is gas related or not + if request.gas.is_some() || request.gas_price.is_some() { + return Err(map_out_of_gas_err(request, state, self.backend.clone(), block_env, fees, gas_limit)) + } + } + + let (exit, out, gas, _) = ethres?; match exit { return_ok!() => { // succeeded @@ -2040,23 +2050,7 @@ impl EthApi { // if price or limit was included in the request then we can execute the request // again with the max gas limit to check if revert is gas related or not return if request.gas.is_some() || request.gas_price.is_some() { - request.gas = Some(self.backend.gas_limit()); - let (exit, out, _, _) = - self.backend.call_with_state(&state, request, fees, block_env)?; - match exit { - return_ok!() => { - // transaction succeeded by manually increasing the gas limit to highest - Err(InvalidTransactionError::BasicOutOfGas(gas_limit).into()) - } - return_revert!() => { - Err(InvalidTransactionError::Revert(Some(convert_transact_out(&out))) - .into()) - } - reason => { - warn!(target: "node", "estimation failed due to {:?}", reason); - Err(BlockchainError::EvmError(reason)) - } - } + Err(map_out_of_gas_err(request, state, self.backend.clone(), block_env, fees, gas_limit)) } else { // the transaction did fail due to lack of gas from the user Err(InvalidTransactionError::Revert(Some(convert_transact_out(&out))).into()) @@ -2068,72 +2062,67 @@ impl EthApi { } } - // at this point we know the call succeeded but want to find the best gas, so we do a binary - // search over the possible range - + // at this point we know the call succeeded but want to find the _best_ (lowest) gas the + // transaction succeeds with. we find this by doing a binary search over the + // possible range NOTE: this is the gas the transaction used, which is less than the + // transaction requires to succeed let gas: U256 = gas.into(); let mut lowest_gas_limit = MIN_TRANSACTION_GAS; // pick a point that's close to the estimated gas let mut mid_gas_limit = std::cmp::min(gas * 3, (highest_gas_limit + lowest_gas_limit) / 2); - let mut last_highest_gas_limit = highest_gas_limit; - // Binary search for the ideal gas limit while (highest_gas_limit - lowest_gas_limit) > U256::one() { request.gas = Some(mid_gas_limit); - match self.backend.call_with_state( + let ethres = self.backend.call_with_state( &state, request.clone(), fees.clone(), block_env.clone(), - ) { - Ok((exit, _, _gas, _)) => { - match exit { - return_ok!() => { - highest_gas_limit = mid_gas_limit; - // if last two successful estimations only vary by 10%, we consider this - // to sufficiently accurate - const ACCURACY: u64 = 10; - if (last_highest_gas_limit - highest_gas_limit) * ACCURACY / - last_highest_gas_limit < - U256::one() - { - return Ok(highest_gas_limit) - } - last_highest_gas_limit = highest_gas_limit; - } - InstructionResult::Revert | - InstructionResult::OutOfGas | - InstructionResult::OutOfFund => { - lowest_gas_limit = mid_gas_limit; - } - reason => { - warn!(target: "node", "estimation failed due to {:?}", reason); - return Err(BlockchainError::EvmError(reason)) - } + ); + + // Exceptional case: init used too much gas, we need to increase the gas limit and try + // again + if let Err(BlockchainError::InvalidTransaction(InvalidTransactionError::GasTooHigh)) = + ethres + { + // increase the lowest gas limit + lowest_gas_limit = mid_gas_limit; + + // new midpoint + mid_gas_limit = (highest_gas_limit + lowest_gas_limit) / 2; + continue + } + + match ethres { + Ok((exit, _, _gas, _)) => match exit { + // If the transaction succeeded, we can set a ceiling for the highest gas limit at the current + // midpoint, as spending any more gas would make no sense (as the TX would still succeed). + return_ok!() => { + highest_gas_limit = mid_gas_limit; } - // new midpoint - mid_gas_limit = (highest_gas_limit + lowest_gas_limit) / 2; - } + // If the transaction failed due to lack of gas, we can set a floor for the lowest gas limit at the current + // midpoint, as spending any less gas would make no sense (as the TX would still revert due to lack of gas). + InstructionResult::Revert | + InstructionResult::OutOfGas | + InstructionResult::OutOfFund => { + lowest_gas_limit = mid_gas_limit; + } + // The tx failed for some other reason. + reason => { + warn!(target: "node", "estimation failed due to {:?}", reason); + return Err(BlockchainError::EvmError(reason)) + } + }, + // We've already checked for the exceptional GasTooHigh case above, so this is a real error. Err(reason) => { - match reason { - // We need to treat REVM reverting due to gas too high just like - // revert/OOG/OOF (see above) - BlockchainError::InvalidTransaction( - InvalidTransactionError::GasTooHigh, - ) => { - lowest_gas_limit = mid_gas_limit; - } - _ => { - warn!(target: "node", "estimation failed due to {:?}", reason); - return Err(reason) - } - }; - // new midpoint - mid_gas_limit = (highest_gas_limit + lowest_gas_limit) / 2; + warn!(target: "node", "estimation failed due to {:?}", reason); + return Err(reason) } - }; + } + // new midpoint + mid_gas_limit = (highest_gas_limit + lowest_gas_limit) / 2; } trace!(target : "node", "Estimated Gas for call {:?}", highest_gas_limit); @@ -2379,3 +2368,40 @@ fn ensure_return_ok(exit: InstructionResult, out: &Option) -> Result Err(BlockchainError::EvmError(reason)), } } + +/// Executes the requests again after an out of gas error to check if the error is gas related or +/// not +#[inline] +fn map_out_of_gas_err( + mut request: EthTransactionRequest, + state: D, + backend: Arc, + block_env: BlockEnv, + fees: FeeDetails, + gas_limit: U256, +) -> BlockchainError where +D: DatabaseRef + { + request.gas = Some(backend.gas_limit()); + let (exit, out, _, _) = + match backend.call_with_state(&state, request, fees, block_env) { + Ok(res) => res, + Err(err) => return err, + }; + match exit { + return_ok!() => { + // transaction succeeded by manually increasing the gas limit to + // highest, which means the caller lacks funds to pay for the tx + return InvalidTransactionError::BasicOutOfGas(gas_limit).into() + } + return_revert!() => { + // reverted again after bumping the limit + return InvalidTransactionError::Revert(Some(convert_transact_out(&out))) + .into() + } + reason => { + warn!(target: "node", "estimation failed due to {:?}", reason); + return BlockchainError::EvmError(reason) + } + } +} From 2460a0523ca12b8ab61ff8a88b7ad77bb0d4a87e Mon Sep 17 00:00:00 2001 From: Enrique Ortiz Date: Fri, 5 May 2023 16:50:51 -0400 Subject: [PATCH 3/8] chore: clippy/fmt --- anvil/src/eth/api.rs | 57 ++++++++++++++++++++----------- anvil/src/eth/backend/executor.rs | 2 +- 2 files changed, 38 insertions(+), 21 deletions(-) diff --git a/anvil/src/eth/api.rs b/anvil/src/eth/api.rs index 089ee9f697b95..f364436f57e9f 100644 --- a/anvil/src/eth/api.rs +++ b/anvil/src/eth/api.rs @@ -2033,7 +2033,14 @@ impl EthApi { // if price or limit was included in the request then we can execute the request // again with the block's gas limit to check if revert is gas related or not if request.gas.is_some() || request.gas_price.is_some() { - return Err(map_out_of_gas_err(request, state, self.backend.clone(), block_env, fees, gas_limit)) + return Err(map_out_of_gas_err( + request, + state, + self.backend.clone(), + block_env, + fees, + gas_limit, + )) } } @@ -2050,7 +2057,14 @@ impl EthApi { // if price or limit was included in the request then we can execute the request // again with the max gas limit to check if revert is gas related or not return if request.gas.is_some() || request.gas_price.is_some() { - Err(map_out_of_gas_err(request, state, self.backend.clone(), block_env, fees, gas_limit)) + Err(map_out_of_gas_err( + request, + state, + self.backend.clone(), + block_env, + fees, + gas_limit, + )) } else { // the transaction did fail due to lack of gas from the user Err(InvalidTransactionError::Revert(Some(convert_transact_out(&out))).into()) @@ -2097,13 +2111,16 @@ impl EthApi { match ethres { Ok((exit, _, _gas, _)) => match exit { - // If the transaction succeeded, we can set a ceiling for the highest gas limit at the current - // midpoint, as spending any more gas would make no sense (as the TX would still succeed). + // If the transaction succeeded, we can set a ceiling for the highest gas limit + // at the current midpoint, as spending any more gas would + // make no sense (as the TX would still succeed). return_ok!() => { highest_gas_limit = mid_gas_limit; } - // If the transaction failed due to lack of gas, we can set a floor for the lowest gas limit at the current - // midpoint, as spending any less gas would make no sense (as the TX would still revert due to lack of gas). + // If the transaction failed due to lack of gas, we can set a floor for the + // lowest gas limit at the current midpoint, as spending any + // less gas would make no sense (as the TX would still revert due to lack of + // gas). InstructionResult::Revert | InstructionResult::OutOfGas | InstructionResult::OutOfFund => { @@ -2115,7 +2132,8 @@ impl EthApi { return Err(BlockchainError::EvmError(reason)) } }, - // We've already checked for the exceptional GasTooHigh case above, so this is a real error. + // We've already checked for the exceptional GasTooHigh case above, so this is a + // real error. Err(reason) => { warn!(target: "node", "estimation failed due to {:?}", reason); return Err(reason) @@ -2377,31 +2395,30 @@ fn map_out_of_gas_err( state: D, backend: Arc, block_env: BlockEnv, - fees: FeeDetails, + fees: FeeDetails, gas_limit: U256, -) -> BlockchainError where -D: DatabaseRef - { +) -> BlockchainError +where + D: DatabaseRef, +{ request.gas = Some(backend.gas_limit()); - let (exit, out, _, _) = - match backend.call_with_state(&state, request, fees, block_env) { - Ok(res) => res, - Err(err) => return err, - }; + let (exit, out, _, _) = match backend.call_with_state(&state, request, fees, block_env) { + Ok(res) => res, + Err(err) => return err, + }; match exit { return_ok!() => { // transaction succeeded by manually increasing the gas limit to // highest, which means the caller lacks funds to pay for the tx - return InvalidTransactionError::BasicOutOfGas(gas_limit).into() + InvalidTransactionError::BasicOutOfGas(gas_limit).into() } return_revert!() => { // reverted again after bumping the limit - return InvalidTransactionError::Revert(Some(convert_transact_out(&out))) - .into() + InvalidTransactionError::Revert(Some(convert_transact_out(&out))).into() } reason => { warn!(target: "node", "estimation failed due to {:?}", reason); - return BlockchainError::EvmError(reason) + BlockchainError::EvmError(reason) } } } diff --git a/anvil/src/eth/backend/executor.rs b/anvil/src/eth/backend/executor.rs index a7282245b16e5..8b3e8e2987688 100644 --- a/anvil/src/eth/backend/executor.rs +++ b/anvil/src/eth/backend/executor.rs @@ -33,7 +33,7 @@ use foundry_evm::{ }, trace::{node::CallTraceNode, CallTraceArena}, }; -use std::{str::FromStr, sync::Arc}; +use std::sync::Arc; use tracing::{trace, warn}; /// Represents an executed transaction (transacted on the DB) From 98874ba81287aaa3cbcad5b233adaeae6c57997e Mon Sep 17 00:00:00 2001 From: Enrique Ortiz Date: Fri, 5 May 2023 17:13:29 -0400 Subject: [PATCH 4/8] chore: fix test --- cli/tests/fixtures/can_create_using_unlocked.stdout | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/tests/fixtures/can_create_using_unlocked.stdout b/cli/tests/fixtures/can_create_using_unlocked.stdout index 80cb0492d2f3e..078403e22b51d 100644 --- a/cli/tests/fixtures/can_create_using_unlocked.stdout +++ b/cli/tests/fixtures/can_create_using_unlocked.stdout @@ -3,4 +3,4 @@ Solc 0.8.15 finished in 1.95s Compiler run successful Deployer: 0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266 Deployed to: 0x5FbDB2315678afecb367f032d93F642f64180aa3 -Transaction hash: 0xd6e14926926a3bf1f51ccd13ccb190887a82d0ba7d5f3f0af7928faa22c4d3ec +Transaction hash: 0x4c3d9f7c4cc26876b43a11ba7ff218374471786a8ae8bf5574deb1d97fc1e851 From 771c6886192d0935c9072811c3f98f25d494b62c Mon Sep 17 00:00:00 2001 From: Matthias Seitz Date: Fri, 5 May 2023 23:46:24 +0200 Subject: [PATCH 5/8] update fixture --- cli/tests/fixtures/can_create_template_contract.stdout | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/tests/fixtures/can_create_template_contract.stdout b/cli/tests/fixtures/can_create_template_contract.stdout index 5013abaffd4be..cf6dd34e14866 100644 --- a/cli/tests/fixtures/can_create_template_contract.stdout +++ b/cli/tests/fixtures/can_create_template_contract.stdout @@ -3,4 +3,4 @@ Solc 0.8.15 finished in 2.27s Compiler run successful Deployer: 0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266 Deployed to: 0x5FbDB2315678afecb367f032d93F642f64180aa3 -Transaction hash: 0xd6e14926926a3bf1f51ccd13ccb190887a82d0ba7d5f3f0af7928faa22c4d3ec +Transaction hash: 0x4c3d9f7c4cc26876b43a11ba7ff218374471786a8ae8bf5574deb1d97fc1e851 From 6cacb0b98398505071012e56c2137010dfb8c789 Mon Sep 17 00:00:00 2001 From: Enrique Ortiz Date: Fri, 5 May 2023 17:54:21 -0400 Subject: [PATCH 6/8] chore: add minimum create transaction gas from reth --- anvil/src/eth/backend/mem/mod.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/anvil/src/eth/backend/mem/mod.rs b/anvil/src/eth/backend/mem/mod.rs index 16a0347317550..9a83b7059703d 100644 --- a/anvil/src/eth/backend/mem/mod.rs +++ b/anvil/src/eth/backend/mem/mod.rs @@ -94,6 +94,8 @@ pub mod storage; // Gas per transaction not creating a contract. pub const MIN_TRANSACTION_GAS: U256 = U256([21_000, 0, 0, 0]); +// Gas per transaction creating a contract. +pub const MIN_CREATE_GAS: U256 = U256([53_000, 0, 0, 0]); pub type State = foundry_evm::HashMap; From edd74d18bf6db9f4f2020caa42bfcb0e965cd70e Mon Sep 17 00:00:00 2001 From: Enrique Ortiz Date: Fri, 5 May 2023 17:55:47 -0400 Subject: [PATCH 7/8] chore: use appropiate minimum gas depending on tx kind --- anvil/src/eth/api.rs | 35 +++++++++++++++++++++++++++++++---- 1 file changed, 31 insertions(+), 4 deletions(-) diff --git a/anvil/src/eth/api.rs b/anvil/src/eth/api.rs index f364436f57e9f..780d670a6dd86 100644 --- a/anvil/src/eth/api.rs +++ b/anvil/src/eth/api.rs @@ -2,7 +2,9 @@ use crate::{ eth::{ backend, backend::{ - db::SerializableState, mem::MIN_TRANSACTION_GAS, notifications::NewBlockNotifications, + db::SerializableState, + mem::{MIN_CREATE_GAS, MIN_TRANSACTION_GAS}, + notifications::NewBlockNotifications, validate::TransactionValidator, }, error::{ @@ -32,8 +34,8 @@ use anvil_core::{ proof::AccountProof, state::StateOverride, transaction::{ - EthTransactionRequest, LegacyTransaction, PendingTransaction, TypedTransaction, - TypedTransactionRequest, + EthTransactionRequest, LegacyTransaction, PendingTransaction, TransactionKind, + TypedTransaction, TypedTransactionRequest, }, EthRequest, }, @@ -2081,7 +2083,8 @@ impl EthApi { // possible range NOTE: this is the gas the transaction used, which is less than the // transaction requires to succeed let gas: U256 = gas.into(); - let mut lowest_gas_limit = MIN_TRANSACTION_GAS; + // Get the starting lowest gas needed depending on the transaction kind. + let mut lowest_gas_limit = determine_base_gas_by_kind(request.clone()); // pick a point that's close to the estimated gas let mut mid_gas_limit = std::cmp::min(gas * 3, (highest_gas_limit + lowest_gas_limit) / 2); @@ -2422,3 +2425,27 @@ where } } } + +/// Determines the minimum gas needed for a transaction depending on the transaction kind. +#[inline] +fn determine_base_gas_by_kind(request: EthTransactionRequest) -> U256 { + match request.into_typed_request() { + Some(request) => match request { + TypedTransactionRequest::Legacy(req) => match req.kind { + TransactionKind::Call(_) => MIN_TRANSACTION_GAS, + TransactionKind::Create => MIN_CREATE_GAS, + }, + TypedTransactionRequest::EIP1559(req) => match req.kind { + TransactionKind::Call(_) => MIN_TRANSACTION_GAS, + TransactionKind::Create => MIN_CREATE_GAS, + }, + TypedTransactionRequest::EIP2930(req) => match req.kind { + TransactionKind::Call(_) => MIN_TRANSACTION_GAS, + TransactionKind::Create => MIN_CREATE_GAS, + }, + }, + // Tighten the gas limit upwards if we don't know the transaction type to avoid deployments + // failing. + _ => MIN_CREATE_GAS, + } +} From 590f43e890dae33b17d46dbaa36ad98bed466303 Mon Sep 17 00:00:00 2001 From: Matthias Seitz Date: Sat, 6 May 2023 00:10:21 +0200 Subject: [PATCH 8/8] update fixture --- cli/tests/fixtures/can_create_template_contract-2nd.stdout | 2 +- cli/tests/fixtures/can_create_using_unlocked-2nd.stdout | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/cli/tests/fixtures/can_create_template_contract-2nd.stdout b/cli/tests/fixtures/can_create_template_contract-2nd.stdout index dfef3326dbd9b..c04ae5bd577d2 100644 --- a/cli/tests/fixtures/can_create_template_contract-2nd.stdout +++ b/cli/tests/fixtures/can_create_template_contract-2nd.stdout @@ -1,4 +1,4 @@ No files changed, compilation skipped Deployer: 0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266 Deployed to: 0xe7f1725E7734CE288F8367e1Bb143E90bb3F0512 -Transaction hash: 0xe4fffb8145d57d12e70bcb9586e2c714592dfd66811a87af92e15638fa027322 +Transaction hash: 0x3d78b08c411f05d5e79adc92a4c814e0f818d1a09c111b0ab688270f35a07ae7 diff --git a/cli/tests/fixtures/can_create_using_unlocked-2nd.stdout b/cli/tests/fixtures/can_create_using_unlocked-2nd.stdout index dfef3326dbd9b..c04ae5bd577d2 100644 --- a/cli/tests/fixtures/can_create_using_unlocked-2nd.stdout +++ b/cli/tests/fixtures/can_create_using_unlocked-2nd.stdout @@ -1,4 +1,4 @@ No files changed, compilation skipped Deployer: 0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266 Deployed to: 0xe7f1725E7734CE288F8367e1Bb143E90bb3F0512 -Transaction hash: 0xe4fffb8145d57d12e70bcb9586e2c714592dfd66811a87af92e15638fa027322 +Transaction hash: 0x3d78b08c411f05d5e79adc92a4c814e0f818d1a09c111b0ab688270f35a07ae7