From cd73f843c319729832afb2cf98382d1b740bb5d3 Mon Sep 17 00:00:00 2001 From: Niven Date: Thu, 1 Feb 2024 16:44:46 +0800 Subject: [PATCH] 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 | 1 + lib/ain-cpp-imports/src/lib.rs | 8 ++ lib/ain-grpc/src/call_request.rs | 40 +++---- lib/ain-grpc/src/rpc/debug.rs | 2 +- lib/ain-grpc/src/rpc/eth.rs | 179 +++++++++++++++++++---------- src/ffi/ffiexports.cpp | 4 + src/ffi/ffiexports.h | 6 +- src/init.cpp | 1 + test/functional/feature_evm_gas.py | 1 + 9 files changed, 156 insertions(+), 86 deletions(-) diff --git a/lib/ain-cpp-imports/src/bridge.rs b/lib/ain-cpp-imports/src/bridge.rs index 6ce220aadf..7fabc8fac7 100644 --- a/lib/ain-cpp-imports/src/bridge.rs +++ b/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; diff --git a/lib/ain-cpp-imports/src/lib.rs b/lib/ain-cpp-imports/src/lib.rs index eb0e4e7a97..1e4963bf43 100644 --- a/lib/ain-cpp-imports/src/lib.rs +++ b/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() diff --git a/lib/ain-grpc/src/call_request.rs b/lib/ain-grpc/src/call_request.rs index 98c7b7efba..e8650083a0 100644 --- a/lib/ain-grpc/src/call_request.rs +++ b/lib/ain-grpc/src/call_request.rs @@ -64,11 +64,15 @@ fn guess_tx_type(req: &CallRequest) -> Result { 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 { } impl CallRequest { - pub fn get_effective_gas_price(&self, block_base_fee: U256) -> Result { - 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, 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) + } } } diff --git a/lib/ain-grpc/src/rpc/debug.rs b/lib/ain-grpc/src/rpc/debug.rs index 50326a3d87..d35444571e 100644 --- a/lib/ain-grpc/src/rpc/debug.rs +++ b/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 diff --git a/lib/ain-grpc/src/rpc/eth.rs b/lib/ain-grpc/src/rpc/eth.rs index c795093500..bac7e1ae80 100644 --- a/lib/ain-grpc/src/rpc/eth.rs +++ b/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,18 +793,16 @@ 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 @@ -807,46 +810,84 @@ impl MetachainRPCServer for MetachainRPCModule { 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)) } diff --git a/src/ffi/ffiexports.cpp b/src/ffi/ffiexports.cpp index 422ca35be6..2f50222942 100644 --- a/src/ffi/ffiexports.cpp +++ b/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 &tokens) { LOCK(cs_main); diff --git a/src/ffi/ffiexports.h b/src/ffi/ffiexports.h index beacb2573c..6e2ec29373 100644 --- a/src/ffi/ffiexports.h +++ b/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 blockHash); uint32_t getEthMaxConnections(); uint32_t getEthMaxResponseByteSize(); int64_t getSuggestedPriorityFeePercentile(); +uint64_t getEstimateGasErrorRatio(); std::array getChainWork(std::array blockHash); rust::vec getPoolTransactions(); uint64_t getNativeTxSize(rust::Vec rawTransaction); diff --git a/src/init.cpp b/src/init.cpp index 013748f039..adf8d716bf 100644 --- a/src/init.cpp +++ b/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=", "Append comment to the user agent string", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST); SetupChainParamsBaseOptions(); diff --git a/test/functional/feature_evm_gas.py b/test/functional/feature_evm_gas.py index 30a200a809..75d23c1bbd 100644 --- a/test/functional/feature_evm_gas.py +++ b/test/functional/feature_evm_gas.py @@ -37,6 +37,7 @@ def set_test_params(self): "-metachainheight=105", "-df23height=105", "-subsidytest=1", + "-evmestimategaserrorratio=0", ], ]