fix: estimation min set to gas_used - 1 rather than to estimate - 1 (#112)

* fix: estimation min set to gas_used - 1 rather than to estimate - 1

* chore: version bump

* fix: rename to limit

Author: prestwich

Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "trevm"
-version = "0.23.6"
+version = "0.23.7"
 rust-version = "1.83.0"
 edition = "2021"
 authors = ["init4"]

src/est.rs

@@ -95,8 +95,8 @@ impl SearchRange {
 pub enum EstimationResult {
     /// The estimation was successful, the result is the gas estimation.
     Success {
-        /// The gas estimation.
-        estimation: u64,
+        /// The input estimation limit.
+        limit: u64,
         /// The amount of gas that was refunded to the caller as unused.
         refund: u64,
         /// The amount of gas used in the execution.
@@ -106,13 +106,17 @@ pub enum EstimationResult {
     },
     /// Estimation failed due to contract revert.
     Revert {
+        /// The input estimation limit.
+        limit: u64,
         /// The revert reason.
         reason: Bytes,
         /// The amount of gas used in the execution.
         gas_used: u64,
     },
     /// The estimation failed due to EVM halt.
     Halt {
+        /// The input estimation limit.
+        limit: u64,
         /// The halt reason.
         reason: HaltReason,
         /// The amount of gas used in the execution
@@ -123,18 +127,17 @@ pub enum EstimationResult {
 impl core::fmt::Display for EstimationResult {
     fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
         match self {
-            Self::Success { estimation, refund, gas_used, .. } => {
+            Self::Success { limit, refund, gas_used, .. } => {
                 write!(
                     f,
-                    "Success {{ estimation: {}, refund: {}, gas_used: {}, .. }}",
-                    estimation, refund, gas_used
+                    "Success {{ gas_limit: {limit}, refund: {refund}, gas_used: {gas_used}, .. }}",
                 )
             }
-            Self::Revert { gas_used, .. } => {
-                write!(f, "Revert {{ gas_used: {}, .. }}", gas_used)
+            Self::Revert { limit, gas_used, .. } => {
+                write!(f, "Revert {{ gas_limit: {limit}, gas_used: {gas_used}, .. }}")
             }
-            Self::Halt { reason, gas_used } => {
-                write!(f, "Halt {{ reason: {:?}, gas_used: {} }}", reason, gas_used)
+            Self::Halt { limit, reason, gas_used } => {
+                write!(f, "Halt {{ gas_limit: {limit}, reason: {reason:?}, gas_used: {gas_used} }}")
             }
         }
     }
@@ -146,24 +149,24 @@ impl EstimationResult {
     pub fn from_limit_and_execution_result(limit: u64, value: &ExecutionResult) -> Self {
         match value {
             ExecutionResult::Success { gas_used, output, gas_refunded, .. } => Self::Success {
-                estimation: limit,
+                limit,
                 refund: *gas_refunded,
                 gas_used: *gas_used,
                 output: output.clone(),
             },
             ExecutionResult::Revert { output, gas_used } => {
-                Self::Revert { reason: output.clone(), gas_used: *gas_used }
+                Self::Revert { limit, reason: output.clone(), gas_used: *gas_used }
             }
             ExecutionResult::Halt { reason, gas_used } => {
-                Self::Halt { reason: *reason, gas_used: *gas_used }
+                Self::Halt { limit, reason: *reason, gas_used: *gas_used }
             }
         }
     }
 
     /// Create a successful estimation result with a gas estimation of 21000.
     pub const fn basic_transfer_success(estimation: u64) -> Self {
         Self::Success {
-            estimation,
+            limit: estimation,
             refund: 0,
             gas_used: estimation,
             output: Output::Call(Bytes::new()),
@@ -180,11 +183,13 @@ impl EstimationResult {
         !self.is_success()
     }
 
-    /// Get the gas estimation, if the execution was successful.
-    pub const fn gas_estimation(&self) -> Option<u64> {
+    /// Get the gas limit that was set in the EVM when the estimation was
+    /// produced.
+    pub const fn limit(&self) -> u64 {
         match self {
-            Self::Success { estimation, .. } => Some(*estimation),
-            _ => None,
+            Self::Success { limit, .. } => *limit,
+            Self::Revert { limit, .. } => *limit,
+            Self::Halt { limit, .. } => *limit,
         }
     }
 
@@ -242,13 +247,12 @@ impl EstimationResult {
     /// Adjust the binary search range based on the estimation outcome.
     pub(crate) const fn adjust_binary_search_range(
         &self,
-        limit: u64,
         range: &mut SearchRange,
     ) -> Result<(), Self> {
         match self {
-            Self::Success { .. } => range.set_max(limit),
-            Self::Revert { .. } => range.set_min(limit),
-            Self::Halt { reason, gas_used } => {
+            Self::Success { limit, .. } => range.set_max(*limit),
+            Self::Revert { limit, .. } => range.set_min(*limit),
+            Self::Halt { limit, reason, gas_used } => {
                 // Both `OutOfGas` and `InvalidEFOpcode` can occur dynamically
                 // if the gas left is too low. Treat this as an out of gas
                 // condition, knowing that the call succeeds with a
@@ -257,9 +261,9 @@ impl EstimationResult {
                 // Common usage of invalid opcode in OpenZeppelin:
                 // <https://github.com/OpenZeppelin/openzeppelin-contracts/blob/94697be8a3f0dfcd95dfb13ffbd39b5973f5c65d/contracts/metatx/ERC2771Forwarder.sol#L360-L367>
                 if matches!(reason, HaltReason::OutOfGas(_) | HaltReason::InvalidFEOpcode) {
-                    range.set_min(limit);
+                    range.set_min(*limit);
                 } else {
-                    return Err(Self::Halt { reason: *reason, gas_used: *gas_used });
+                    return Err(Self::Halt { limit: *limit, reason: *reason, gas_used: *gas_used });
                 }
             }
         }

src/evm.rs

@@ -1799,7 +1799,7 @@ where
     /// [`MIN_TRANSACTION_GAS`]: crate::MIN_TRANSACTION_GAS
     #[cfg(feature = "estimate_gas")]
     pub fn estimate_gas(mut self) -> Result<(crate::EstimationResult, Self), EvmErrored<Db, Insp>> {
-        use tracing::{debug, enabled};
+        use tracing::{debug, enabled, trace};
 
         if let Some(est) = trevm_try!(self.estimate_gas_simple_transfer(), self) {
             return Ok((crate::EstimationResult::basic_transfer_success(est), self));
@@ -1814,13 +1814,15 @@ where
             crate::est::SearchRange::new(crate::MIN_TRANSACTION_GAS, initial_limit);
 
         let span = tracing::debug_span!(
-            "estimate_gas",
+            "Trevm::estimate_gas",
             start_min = search_range.min(),
             start_max = search_range.max(),
-            tx = "omitted",
         );
         if enabled!(tracing::Level::TRACE) {
             span.record("tx", format!("{:?}", &self.tx()));
+            span.record("block", format!("{:?}", &self.block()));
+        } else {
+            span.record("tx", "omitted. Use TRACE for details");
         }
         let _e = span.enter();
 
@@ -1834,7 +1836,7 @@ where
         // Run an estimate with the max gas limit.
         // NB: we declare these mut as we re-use the binding throughout the
         // function.
-        debug!(gas_limit = search_range.max(), "running optimistic estimate");
+        debug!(gas_limit = self.gas_limit(), "running optimistic estimate");
         let (mut estimate, mut trevm) = self.run_estimate(&search_range.max().into())?;
 
         // If it failed, no amount of gas is likely to work, so we shortcut
@@ -1843,10 +1845,11 @@ where
             debug!(%estimate, "optimistic estimate failed");
             return Ok((estimate, trevm));
         }
+        trace!(%estimate, "optimistic estimate succeeded");
 
         // Now we know that it succeeds at _some_ gas limit. We can now binary
         // search.
-        let mut gas_used = estimate.gas_estimation().expect("checked is_failure");
+        let mut gas_used = estimate.gas_used();
         let gas_refunded = estimate.gas_refunded().expect("checked is_failure");
 
         // NB: if we've made it this far it's very unlikely that `gas_used` is
@@ -1862,6 +1865,7 @@ where
         // frame can forward only 63/64 of the gas it has when it makes a new
         // frame.
         let mut needle = gas_used + gas_refunded + revm::interpreter::gas::CALL_STIPEND * 64 / 63;
+
         // If the first search is outside the range, we don't need to try it.
         if search_range.contains(needle) {
             estimate_and_adjust!(estimate, trevm, needle, search_range);

src/macros.rs

@@ -45,13 +45,17 @@ macro_rules! estimate_and_adjust {
     ($est:ident, $trevm:ident, $gas_limit:ident, $range:ident) => {
         ::tracing::trace!(
             estimate = %$est,
-            gas_limit = $gas_limit,
-            range = %$range,
+            max = %$range.max(),
+            min = %$range.min(),
             "running gas estimate call"
         );
 
         ($est, $trevm) = $trevm.run_estimate(&$gas_limit.into())?;
-        if let Err(e) = $est.adjust_binary_search_range($gas_limit, &mut $range) {
+        if let Err(e) = $est.adjust_binary_search_range(&mut $range) {
+            ::tracing::trace!(
+                %e,
+                "error adjusting binary search range"
+            );
             return Ok((e, $trevm));
         }
     };