RPC: Fix estimate gas RPC (#2808)

* Fix estimate gas RPC

* Add estimate gas error ratio to configurable param

* Fix comments

lib/ain-cpp-imports/src/bridge.rs

@@ -38,6 +38,7 @@ pub mod ffi {
         fn getEthMaxConnections() -> u32;
         fn getEthMaxResponseByteSize() -> u32;
         fn getSuggestedPriorityFeePercentile() -> i64;
+        fn getEstimateGasErrorRatio() -> u64;
         fn getDifficulty(block_hash: [u8; 32]) -> u32;
         fn getChainWork(block_hash: [u8; 32]) -> [u8; 32];
         fn getPoolTransactions() -> Vec<TransactionData>;

lib/ain-cpp-imports/src/lib.rs

@@ -54,6 +54,9 @@ mod ffi {
     pub fn getSuggestedPriorityFeePercentile() -> i64 {
         unimplemented!("{}", UNIMPL_MSG)
     }
+    pub fn getEstimateGasErrorRatio() -> u64 {
+        unimplemented!("{}", UNIMPL_MSG)
+    }
     pub fn getNetwork() -> String {
         unimplemented!("{}", UNIMPL_MSG)
     }
@@ -180,6 +183,11 @@ pub fn get_suggested_priority_fee_percentile() -> i64 {
     ffi::getSuggestedPriorityFeePercentile()
 }
 
+/// Gets the gas estimation error ratio for Ethereum RPC calls.
+pub fn get_estimate_gas_error_ratio() -> u64 {
+    ffi::getEstimateGasErrorRatio()
+}
+
 /// Retrieves the network identifier as a string.
 pub fn get_network() -> String {
     ffi::getNetwork()

lib/ain-grpc/src/call_request.rs

@@ -64,11 +64,15 @@ fn guess_tx_type(req: &CallRequest) -> Result<TxType, Error> {
         return TxType::try_from(tx_type);
     }
 
+    // Validate call request gas fees
     if req.gas_price.is_some()
         && (req.max_fee_per_gas.is_some() || req.max_priority_fee_per_gas.is_some())
     {
         return Err(RPCError::InvalidGasPrice.into());
     }
+    if req.max_fee_per_gas.is_some() && req.max_priority_fee_per_gas.is_none() {
+        return Err(RPCError::InvalidGasPrice.into());
+    }
 
     if req.max_fee_per_gas.is_some() && req.max_priority_fee_per_gas.is_some() {
         Ok(TxType::EIP1559)
@@ -80,34 +84,24 @@ fn guess_tx_type(req: &CallRequest) -> Result<TxType, Error> {
 }
 
 impl CallRequest {
-    pub fn get_effective_gas_price(&self, block_base_fee: U256) -> Result<U256, Error> {
-        if self.gas_price.is_some()
-            && (self.max_fee_per_gas.is_some() || self.max_priority_fee_per_gas.is_some())
-        {
-            return Err(RPCError::InvalidGasPrice.into());
-        }
-
+    pub fn get_effective_gas_price(&self) -> Result<Option<U256>, Error> {
         match guess_tx_type(self)? {
-            TxType::Legacy | TxType::EIP2930 => match self.gas_price {
-                Some(gas_price) => {
-                    if gas_price == U256::zero() {
-                        Ok(block_base_fee)
-                    } else {
-                        Ok(gas_price)
+            TxType::Legacy | TxType::EIP2930 => {
+                if let Some(gas_price) = self.gas_price {
+                    if gas_price.is_zero() {
+                        return Ok(None);
                     }
                 }
-                None => Ok(block_base_fee),
-            },
-            TxType::EIP1559 => match self.max_fee_per_gas {
-                Some(max_fee_per_gas) => {
-                    if max_fee_per_gas == U256::zero() {
-                        Ok(block_base_fee)
-                    } else {
-                        Ok(max_fee_per_gas)
+                Ok(self.gas_price)
+            }
+            TxType::EIP1559 => {
+                if let Some(max_fee_per_gas) = self.max_fee_per_gas {
+                    if max_fee_per_gas.is_zero() {
+                        return Ok(None);
                     }
                 }
-                None => Ok(block_base_fee),
-            },
+                Ok(self.max_fee_per_gas)
+            }
         }
     }
 

lib/ain-grpc/src/rpc/debug.rs

@@ -125,7 +125,7 @@ impl MetachainDebugRPCServer for MetachainDebugRPCModule {
             .block
             .calculate_base_fee(block_hash, block_gas_target_factor)
             .map_err(to_custom_err)?;
-        let gas_price = call.get_effective_gas_price(block_base_fee)?;
+        let gas_price = call.get_effective_gas_price()?.unwrap_or(block_base_fee);
 
         let TxResponse { used_gas, .. } = self
             .handler

lib/ain-grpc/src/rpc/eth.rs

@@ -337,7 +337,7 @@ impl MetachainRPCServer for MetachainRPCModule {
 
         let block = self.get_block(block_number)?;
         let block_base_fee = block.header.base_fee;
-        let gas_price = call.get_effective_gas_price(block_base_fee)?;
+        let gas_price = call.get_effective_gas_price()?.unwrap_or(block_base_fee);
 
         let TxResponse {
             data, exit_reason, ..
@@ -774,9 +774,14 @@ impl MetachainRPCServer for MetachainRPCModule {
         Ok(nonce)
     }
 
-    /// EstimateGas executes the requested code against the current pending block/state and
-    /// returns the used amount of gas.
-    /// Ref: https://github.com/ethereum/go-ethereum/blob/master/accounts/abi/bind/backends/simulated.go#L537-L639
+    /// Estimate returns the lowest possible gas limit that allows the transaction to
+    /// run successfully with the provided context options. It returns an error if the
+    /// transaction would always revert, or if there are unexpected failures.
+    ///
+    /// To configure the gas estimation error ratio, set-evmestimategaserrorratio=n on
+    /// startup. Otherwise, the default parameter is set at 15% error ratio.
+    ///
+    /// Ref: https://github.com/ethereum/go-ethereum/blob/e2778cd59f04f7587c9aa5983282074026ff6684/eth/gasestimator/gasestimator.go
     fn estimate_gas(
         &self,
         call: CallRequest,
@@ -788,65 +793,101 @@ impl MetachainRPCServer for MetachainRPCModule {
         let caller = call.from.unwrap_or_default();
         let byte_data = call.get_data()?;
         let data = byte_data.0.as_slice();
+        let overlay = state_overrides.map(override_to_overlay);
 
         let block_gas_limit = ain_cpp_imports::get_attribute_values(None).block_gas_limit;
-
         let call_gas = u64::try_from(call.gas.unwrap_or(U256::from(block_gas_limit)))
             .map_err(to_custom_err)?;
 
-        let overlay = state_overrides.map(override_to_overlay);
-        // Determine the lowest and highest possible gas limits to binary search in between
-        let mut lo = Self::CONFIG.gas_transaction_call - 1;
-        let mut hi = call_gas;
-        if call_gas < Self::CONFIG.gas_transaction_call {
-            hi = block_gas_limit;
+        // Determine the highest gas limit can be used during the estimation.
+        let mut hi = block_gas_limit;
+        if call_gas >= Self::CONFIG.gas_transaction_call {
+            hi = call_gas;
         }
 
         // Get block base fee
         let block = self.get_block(block_number)?;
         let block_base_fee = block.header.base_fee;
 
         // Normalize the max fee per gas the call is willing to spend.
-        let fee_cap = call.get_effective_gas_price(block_base_fee)?;
-
-        // Recap the highest gas allowance with account's balance
-        if call.from.is_some() {
-            let balance = if let Some(balance) = overlay
-                .as_ref()
-                .and_then(|o| o.get_account(&caller).map(|acc| acc.balance))
-            {
-                balance
-            } else {
-                self.handler
-                    .core
-                    .get_balance(caller, block.header.state_root)
-                    .map_err(to_custom_err)?
-            };
-            let mut available = balance;
-            if let Some(value) = call.value {
-                if balance < value {
-                    return Err(RPCError::InsufficientFunds.into());
+        let fee_cap = call.get_effective_gas_price()?;
+
+        // Recap the highest gas allowance with account's balance if gas price
+        if let Some(cap) = fee_cap {
+            if call.from.is_some() {
+                let balance = if let Some(balance) = overlay
+                    .as_ref()
+                    .and_then(|o| o.get_account(&caller).map(|acc| acc.balance))
+                {
+                    balance
+                } else {
+                    self.handler
+                        .core
+                        .get_balance(caller, block.header.state_root)
+                        .map_err(to_custom_err)?
+                };
+                let mut available = balance;
+                if let Some(value) = call.value {
+                    if balance < value {
+                        return Err(RPCError::InsufficientFunds.into());
+                    }
+                    available = balance.checked_sub(value).ok_or(RPCError::ValueUnderflow)?;
                 }
-                available = balance.checked_sub(value).ok_or(RPCError::ValueUnderflow)?;
-            }
 
-            let allowance = available
-                .checked_div(fee_cap)
-                .ok_or(RPCError::DivideError)?;
-            debug!(target:"rpc",  "[estimate_gas] allowance: {:#?}", allowance);
+                let allowance = available.checked_div(cap).ok_or(RPCError::DivideError)?;
+                debug!(target:"rpc",  "[estimate_gas] allowance: {:#?}", allowance);
 
-            if let Ok(allowance) = u64::try_from(allowance) {
-                if hi > allowance {
-                    debug!("[estimate_gas] gas estimation capped by limited funds. original: {:#?}, balance: {:#?}, feecap: {:#?}, fundable: {:#?}", hi, balance, fee_cap, allowance);
-                    hi = allowance;
+                if let Ok(allowance) = u64::try_from(allowance) {
+                    if hi > allowance {
+                        debug!("[estimate_gas] gas estimation capped by limited funds. original: {:#?}, balance: {:#?}, feecap: {:#?}, fundable: {:#?}", hi, balance, fee_cap, allowance);
+                        hi = allowance;
+                    }
+                }
+            }
+        }
+        let fee_cap = fee_cap.unwrap_or(block_base_fee);
+
+        // If the transaction is plain value transfer, short circuit estimation and directly
+        // try 21_000. Returning 21_000 without any execution is dangerous as some tx field
+        // combos might bump the price up even for plain transfers (e.g. unused access list
+        // items). Ever so slightly wasteful, but safer overall.
+        if data.is_empty() {
+            if let Some(to) = call.to {
+                if overlay.as_ref().and_then(|o| o.get_code(&to)).is_none()
+                    && self
+                        .handler
+                        .core
+                        .get_code(to, block.header.state_root)
+                        .map_err(to_custom_err)?
+                        .is_none()
+                {
+                    let tx_response = self
+                        .handler
+                        .core
+                        .call(
+                            EthCallArgs {
+                                caller,
+                                to: Some(to),
+                                value: call.value.unwrap_or_default(),
+                                data,
+                                gas_limit: call_gas,
+                                gas_price: fee_cap,
+                                access_list: call.access_list.clone().unwrap_or_default(),
+                                block_number: block.header.number,
+                            },
+                            overlay.clone(),
+                        )
+                        .map_err(RPCError::EvmError)?;
+                    if let ExitReason::Succeed(_) = tx_response.exit_reason {
+                        return Ok(U256::from(tx_response.used_gas));
+                    }
                 }
             }
         }
-
-        let cap = hi;
 
         // Create a helper to check if a gas allowance results in an executable transaction
-        let executable = |gas_limit: u64| -> Result<(bool, bool), Error> {
+        // Returns: (tx execution failure flag, out of gas failure flag, used gas)
+        let executable = |gas_limit: u64| -> Result<(bool, bool, u64), Error> {
             // Consensus error, this means the provided message call or transaction will
             // never be accepted no matter how much gas it is assigned. Return the error
             // directly, don't struggle any more
@@ -869,15 +910,45 @@ impl MetachainRPCServer for MetachainRPCModule {
                 .map_err(RPCError::EvmError)?;
 
             match tx_response.exit_reason {
-                ExitReason::Error(ExitError::OutOfGas) => Ok((true, true)),
-                ExitReason::Succeed(_) => Ok((false, false)),
-                _ => Ok((true, false)),
+                ExitReason::Error(ExitError::OutOfGas) => Ok((true, true, tx_response.used_gas)),
+                ExitReason::Succeed(_) => Ok((false, false, tx_response.used_gas)),
+                _ => Ok((true, false, tx_response.used_gas)),
             }
         };
 
+        // We first execute the transaction at the highest allowable gas limit, since
+        // if this fails we can return error immediately.
+        let (failed, out_of_gas, used_gas) = executable(hi)?;
+        if failed {
+            if !out_of_gas {
+                return Err(RPCError::TxExecutionFailed.into());
+            } else {
+                return Err(RPCError::GasCapTooLow(hi).into());
+            }
+        }
+
+        // For almost any transaction, the gas consumed by the unconstrained execution
+        // above lower-bounds the gas limit required for it to succeed. One exception
+        // is those that explicitly check gas remaining in order to execute within a
+        // given limit, but we probably don't want to return the lowest possible gas
+        // limit for these cases anyway.
+        let mut lo = used_gas.saturating_sub(1u64);
         while lo + 1 < hi {
+            // Safe, since highest gas limit possible is set at BLOCK_GAS_LIMIT
+            let diff_percentage = ((hi.saturating_sub(lo) as f64) / (hi as f64) * 100f64) as u64;
+            if diff_percentage < ain_cpp_imports::get_estimate_gas_error_ratio() {
+                break;
+            }
+
             let sum = hi.checked_add(lo).ok_or(RPCError::ValueOverflow)?;
-            let mid = sum.checked_div(2u64).ok_or(RPCError::DivideError)?;
+            let mut mid = sum.checked_div(2u64).ok_or(RPCError::DivideError)?;
+
+            // Most txs don't need much higher gas limit than their gas used, and most txs don't
+            // require near the full block limit of gas, so the selection of where to bisect the
+            // range here is skewed to favor the low side.
+            if mid > lo.saturating_mul(2u64) {
+                mid = lo * 2;
+            }
 
             let (failed, ..) = executable(mid)?;
             if failed {
@@ -886,20 +957,6 @@ impl MetachainRPCServer for MetachainRPCModule {
                 hi = mid;
             }
         }
-
-        // Reject the transaction as invalid if it still fails at the highest allowance
-        if hi == cap {
-            let (failed, out_of_gas) = executable(hi)?;
-            if failed {
-                if !out_of_gas {
-                    return Err(RPCError::TxExecutionFailed.into());
-                } else {
-                    return Err(RPCError::GasCapTooLow(cap).into());
-                }
-            }
-        }
-
-        debug!(target:"rpc",  "[estimate_gas] estimated gas: {:#?} at block {:#x}", hi, block.header.number);
         Ok(U256::from(hi))
     }
 

src/ffi/ffiexports.cpp

@@ -315,6 +315,10 @@ int64_t getSuggestedPriorityFeePercentile() {
     return gArgs.GetArg("-evmtxpriorityfeepercentile", DEFAULT_SUGGESTED_PRIORITY_FEE_PERCENTILE);
 }
 
+uint64_t getEstimateGasErrorRatio() {
+    return gArgs.GetArg("-evmestimategaserrorratio", DEFAULT_ESTIMATE_GAS_ERROR_RATIO);
+}
+
 bool getDST20Tokens(std::size_t mnview_ptr, rust::vec<DST20Token> &tokens) {
     LOCK(cs_main);
 

src/ffi/ffiexports.h

@@ -15,9 +15,12 @@ static constexpr CAmount DEFAULT_EVM_RBF_FEE_INCREMENT = COIN / 10;
 static constexpr uint32_t DEFAULT_ETH_MAX_CONNECTIONS = 100;
 static constexpr uint32_t DEFAULT_ETH_MAX_RESPONSE_SIZE_MB = 25;  // 25 megabytes
 
-// Defaults for attributes relating to gasprice oracle settings
+// Default for attributes relating to gasprice setting
 static constexpr int64_t DEFAULT_SUGGESTED_PRIORITY_FEE_PERCENTILE = 60;
 
+// Default for attributes relating to gasprice setting
+static constexpr uint64_t DEFAULT_ESTIMATE_GAS_ERROR_RATIO = 15;
+
 static constexpr uint32_t DEFAULT_ECC_LRU_CACHE_COUNT = 10000;
 static constexpr uint32_t DEFAULT_EVMV_LRU_CACHE_COUNT = 10000;
 
@@ -74,6 +77,7 @@ uint32_t getDifficulty(std::array<uint8_t, 32> blockHash);
 uint32_t getEthMaxConnections();
 uint32_t getEthMaxResponseByteSize();
 int64_t getSuggestedPriorityFeePercentile();
+uint64_t getEstimateGasErrorRatio();
 std::array<uint8_t, 32> getChainWork(std::array<uint8_t, 32> blockHash);
 rust::vec<TransactionData> getPoolTransactions();
 uint64_t getNativeTxSize(rust::Vec<uint8_t> rawTransaction);

src/init.cpp

@@ -613,6 +613,7 @@ void SetupServerArgs()
     gArgs.AddArg("-shrinkdebugfile", "Shrink debug.log file on client startup (default: 1 when no -debug)", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
     gArgs.AddArg("-tdsinglekeycheck", "Set the single key check flag for transferdomain RPC. If enabled, transfers between domain are only allowed if the addresses specified corresponds to the same key (default: true)", ArgsManager::ALLOW_ANY, OptionsCategory::RPC);
     gArgs.AddArg("-evmtxpriorityfeepercentile", strprintf("Set the suggested priority fee for EVM transactions (default: %u)", DEFAULT_SUGGESTED_PRIORITY_FEE_PERCENTILE), ArgsManager::ALLOW_ANY, OptionsCategory::RPC);
+    gArgs.AddArg("-evmestimategaserrorratio", strprintf("Set the gas estimation error ratio for eth_estimateGas RPC (default: %u)", DEFAULT_ESTIMATE_GAS_ERROR_RATIO), ArgsManager::ALLOW_ANY, OptionsCategory::RPC);
     gArgs.AddArg("-uacomment=<cmt>", "Append comment to the user agent string", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
 
     SetupChainParamsBaseOptions();

test/functional/feature_evm_gas.py

@@ -37,6 +37,7 @@ def set_test_params(self):
                 "-metachainheight=105",
                 "-df23height=105",
                 "-subsidytest=1",
+                "-evmestimategaserrorratio=0",
             ],
         ]