-
Notifications
You must be signed in to change notification settings - Fork 8
fix: gas estimation for simple transfers #78
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
mod builder; | ||
|
||
use alloy::rpc::types::BlockOverrides; | ||
pub use builder::ConcurrentStateBuilder; | ||
|
||
mod cache_state; | ||
|
@@ -8,7 +9,7 @@ pub use cache_state::ConcurrentCacheState; | |
mod sync_state; | ||
pub use sync_state::{ConcurrentState, ConcurrentStateInfo}; | ||
|
||
use crate::{EvmNeedsBlock, Trevm}; | ||
use crate::{Block, EvmNeedsBlock, EvmNeedsTx, Trevm}; | ||
use revm::{ | ||
db::{states::bundle_state::BundleRetention, BundleState}, | ||
DatabaseRef, | ||
|
@@ -41,3 +42,37 @@ impl<Ext, Db: DatabaseRef + Sync> EvmNeedsBlock<'_, Ext, ConcurrentState<Db>> { | |
bundle | ||
} | ||
} | ||
|
||
impl<Ext, Db: DatabaseRef + Sync> EvmNeedsTx<'_, Ext, ConcurrentState<Db>> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. drive-by to add feature parity with |
||
/// Apply block overrides to the current block. | ||
/// | ||
/// Note that this is NOT reversible. The overrides are applied directly to | ||
/// the underlying state and these changes cannot be removed. If it is | ||
/// important that you have access to the pre-change state, you should wrap | ||
/// the existing DB in a new [`ConcurrentState`] and apply the overrides to | ||
/// that. | ||
pub fn apply_block_overrides(mut self, overrides: &BlockOverrides) -> Self { | ||
overrides.fill_block(&mut self.inner); | ||
|
||
if let Some(hashes) = &overrides.block_hash { | ||
self.inner.db_mut().info.block_hashes.write().unwrap().extend(hashes) | ||
} | ||
|
||
self | ||
} | ||
|
||
/// Apply block overrides to the current block, if they are provided. | ||
/// | ||
/// Note that this is NOT reversible. The overrides are applied directly to | ||
/// the underlying state and these changes cannot be removed. If it is | ||
/// important that you have access to the pre-change state, you should wrap | ||
/// the existing DB in a new [`ConcurrentState`] and apply the overrides to | ||
/// that. | ||
pub fn maybe_apply_block_overrides(self, overrides: Option<&BlockOverrides>) -> Self { | ||
if let Some(overrides) = overrides { | ||
self.apply_block_overrides(overrides) | ||
} else { | ||
self | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,10 +14,10 @@ use alloy::{ | |
use core::convert::Infallible; | ||
use revm::{ | ||
db::{states::bundle_state::BundleRetention, BundleState, State}, | ||
interpreter::gas::CALL_STIPEND, | ||
interpreter::gas::{calculate_initial_tx_gas, CALL_STIPEND}, | ||
primitives::{ | ||
AccountInfo, BlockEnv, Bytecode, EVMError, EvmState, ExecutionResult, InvalidTransaction, | ||
ResultAndState, SpecId, TxEnv, TxKind, KECCAK_EMPTY, | ||
AccountInfo, AuthorizationList, BlockEnv, Bytecode, EVMError, Env, EvmState, | ||
ExecutionResult, InvalidTransaction, ResultAndState, SpecId, TxEnv, TxKind, KECCAK_EMPTY, | ||
}, | ||
Database, DatabaseCommit, DatabaseRef, Evm, | ||
}; | ||
|
@@ -71,6 +71,30 @@ impl<'a, Ext, Db: Database + DatabaseCommit, TrevmState> Trevm<'a, Ext, Db, Trev | |
self.inner | ||
} | ||
|
||
/// Get a reference to the inner env. This contains the current | ||
/// [`BlockEnv`], [`TxEnv`], and [`CfgEnv`]. | ||
/// | ||
/// These values may be meaningless, stale, or otherwise incorrect. Reading | ||
/// them should be done with caution, as it may lead to logic bugs. | ||
/// | ||
/// [`CfgEnv`]: revm::primitives::CfgEnv | ||
pub fn env_unchecked(&self) -> &Env { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. drive-by util functions |
||
&self.inner().context.evm.inner.env | ||
} | ||
|
||
/// Get a mutable reference to the inner env. This contains the current | ||
/// [`BlockEnv`], [`TxEnv`], and [`CfgEnv`]. | ||
/// | ||
/// These values may be meaningless, stale, or otherwise incorrect. Reading | ||
/// them should be done with caution, as it may lead to logic bugs. | ||
/// Modifying these values may lead to inconsistent state or invalid | ||
/// execution. | ||
/// | ||
/// [`CfgEnv`]: revm::primitives::CfgEnv | ||
pub fn env_mut_unchecked(&mut self) -> &mut Env { | ||
&mut self.inner_mut_unchecked().context.evm.inner.env | ||
} | ||
|
||
/// Get the id of the currently running hardfork spec. Convenience function | ||
/// calling [`Evm::spec_id`]. | ||
pub fn spec_id(&self) -> SpecId { | ||
|
@@ -1176,7 +1200,7 @@ impl<'a, Ext, Db: Database + DatabaseCommit, TrevmState: HasTx> Trevm<'a, Ext, D | |
} | ||
} | ||
|
||
// -- HAS TX with State<Db> | ||
// -- NEEDS TX with State<Db> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fix doc |
||
|
||
impl<Ext, Db: Database> EvmNeedsTx<'_, Ext, State<Db>> { | ||
/// Apply block overrides to the current block. | ||
|
@@ -1236,12 +1260,36 @@ impl<'a, Ext, Db: Database + DatabaseCommit> EvmReady<'a, Ext, Db> { | |
} | ||
} | ||
|
||
/// Calculate the minimum gas required to start EVM execution. | ||
/// | ||
/// This uses [`calculate_initial_tx_gas`] to calculate the initial gas. | ||
/// Its output is dependent on | ||
/// - the EVM spec | ||
/// - the input data | ||
/// - whether the transaction is a contract creation or a call | ||
/// - the EIP-2930 access list | ||
/// - the number of [EIP-7702] authorizations | ||
/// | ||
/// [EIP-2930]: https://eips.ethereum.org/EIPS/eip-2930 | ||
/// [EIP-7702]: https://eips.ethereum.org/EIPS/eip-7702 | ||
fn calculate_initial_gas(&self) -> u64 { | ||
calculate_initial_tx_gas( | ||
self.spec_id(), | ||
&[], | ||
false, | ||
&self.tx().access_list, | ||
self.tx().authorization_list.as_ref().map(AuthorizationList::len).unwrap_or_default() | ||
as u64, | ||
) | ||
.initial_gas | ||
} | ||
|
||
/// Estimate gas for a simple transfer. This will | ||
/// - Check that the transaction has no input data. | ||
/// - Check that the target is not a `create`. | ||
/// - Check that the target is not a contract. | ||
/// - Return the minimum gas required for the transfer. | ||
fn estimate_gas_simple_transfer(&mut self) -> Result<Option<()>, EVMError<Db::Error>> { | ||
fn estimate_gas_simple_transfer(&mut self) -> Result<Option<u64>, EVMError<Db::Error>> { | ||
if !self.is_transfer() { | ||
return Ok(None); | ||
} | ||
|
@@ -1254,8 +1302,9 @@ impl<'a, Ext, Db: Database + DatabaseCommit> EvmReady<'a, Ext, Db> { | |
return Ok(None); | ||
} | ||
|
||
// If the target is not a contract, then the gas is the minimum gas. | ||
Ok(Some(())) | ||
// delegate calculation to revm. This ensures that things like bogus | ||
// 2930 access lists don't mess up our estimates | ||
Ok(Some(self.calculate_initial_gas())) | ||
} | ||
|
||
/// Convenience function to simplify nesting of [`Self::estimate_gas`]. | ||
|
@@ -1330,22 +1379,28 @@ impl<'a, Ext, Db: Database + DatabaseCommit> EvmReady<'a, Ext, Db> { | |
/// | ||
/// [here]: https://github.com/paradigmxyz/reth/blob/ad503a08fa242b28ad3c1fea9caa83df2dfcf72d/crates/rpc/rpc-eth-api/src/helpers/estimate.rs#L35-L42 | ||
pub fn estimate_gas(mut self) -> Result<(EstimationResult, Self), EvmErrored<'a, Ext, Db>> { | ||
if unwrap_or_trevm_err!(self.estimate_gas_simple_transfer(), self).is_some() { | ||
return Ok((EstimationResult::basic_transfer_success(), self)); | ||
if let Some(est) = unwrap_or_trevm_err!(self.estimate_gas_simple_transfer(), self) { | ||
return Ok((EstimationResult::basic_transfer_success(est), self)); | ||
} | ||
|
||
// We shrink the gas limit to 64 bits, as using more than 18 quintillion | ||
// gas in a block is not likely. | ||
// gas in a block is unlikely. | ||
let initial_limit = self.gas_limit(); | ||
|
||
// Start the search range at 21_000 gas. | ||
let mut search_range = SearchRange::new(MIN_TRANSACTION_GAS, initial_limit); | ||
|
||
// Block it to the gas cap. | ||
search_range.maybe_lower_max(self.block_gas_limit().saturating_to::<u64>()); | ||
|
||
// Check that the account has enough ETH to cover the gas, and lower if | ||
// necessary. | ||
let allowance = unwrap_or_trevm_err!(self.gas_allowance(), self); | ||
search_range.maybe_lower_max(allowance); | ||
|
||
// Raise the floor to the amount of gas required to initialize the EVM. | ||
search_range.maybe_raise_min(self.calculate_initial_gas()); | ||
|
||
// Run an estimate with the max gas limit. | ||
// NB: we declare these mut as we re-use the binding throughout the | ||
// function. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are we skipping a version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to match revm