Skip to content

fix(eth): retry gas estimate without fee fields on base-fee rejection (Gnosis/Nethermind) #2728

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.

Sign up for GitHub

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

Closed
CharlVS wants to merge 1 commit into from
Closed

fix(eth): retry gas estimate without fee fields on base-fee rejection (Gnosis/Nethermind) #2728

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 61 additions & 17 deletions mm2src/coins/eth.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5188,6 +5188,38 @@ impl EthCoin {
Box::new(fut.boxed().compat())
}

/// Estimates gas for the contract call described by `base_req` (which must NOT yet carry any
/// gas-price/fee fields), applying `pay_for_gas_option` to populate them.
///
/// The fee fields are supplied because some contracts branch on gas price (e.g. TUSD, see
/// https://github.com/KomodoPlatform/atomicDEX-API/issues/643). However, gas *usage* itself does
/// not depend on the fee values, and some nodes (e.g. Nethermind, the client behind Gnosis)
/// validate the EIP-1559 fee fields during `eth_estimateGas` and reject the call when
/// `max_fee_per_gas` is below the current base fee (e.g. "miner premium is negative"). When that
/// happens we retry the estimate without the fee fields so gas estimation (and thus order posting
/// and fee preimages) keeps working even when the base fee spikes above our (high-policy) estimate.
async fn estimate_gas_with_pay_for_gas_option(
&self,
base_req: CallRequest,
pay_for_gas_option: PayForGasOption,
) -> Web3RpcResult<U256> {
let req_with_fees = call_request_with_pay_for_gas_option(base_req.clone(), pay_for_gas_option);
let res = self.estimate_gas_wrapper(req_with_fees).compat().await;
match res {
Err(ref e) if is_max_fee_per_gas_below_base_fee_error(&e.to_string()) => {
warn!(
"eth_estimateGas rejected the EIP-1559 fee fields ({}); retrying without them as gas usage is fee-independent",
e
);
self.estimate_gas_wrapper(base_req)
.compat()
.await
.map_to_mm(Web3RpcError::from)
},
other => other.map_to_mm(Web3RpcError::from),
}
}

/// Estimates how much gas is necessary to allow the contract call to complete.
/// `contract_addr` can be a ERC20 token address or any other contract address.
///
Expand Down Expand Up @@ -5219,13 +5251,8 @@ impl EthCoin {
to: Some(contract_addr),
..CallRequest::default()
};
// gas price must be supplied because some smart contracts base their
// logic on gas price, e.g. TUSD: https://github.com/KomodoPlatform/atomicDEX-API/issues/643
let estimate_gas_req = call_request_with_pay_for_gas_option(estimate_gas_req, pay_for_gas_option);
coin.estimate_gas_wrapper(estimate_gas_req)
.compat()
coin.estimate_gas_with_pay_for_gas_option(estimate_gas_req, pay_for_gas_option)
.await
.map_to_mm(Web3RpcError::from)
}

/// Calls estimate_gas_for_contract_call if the `estimate_gas_mult` conf param is set or `default_gas` is None.
Expand Down Expand Up @@ -6586,12 +6613,12 @@ impl MmCoin for EthCoin {
gas: None,
..CallRequest::default()
};
// gas price must be supplied because some smart contracts base their
// logic on gas price, e.g. TUSD: https://github.com/KomodoPlatform/atomicDEX-API/issues/643
let estimate_gas_req = call_request_with_pay_for_gas_option(estimate_gas_req, pay_for_gas_option.clone());
// Please note if the wallet's balance is insufficient to withdraw, then `estimate_gas` may fail with the `Exception` error.
// Ideally we should determine the case when we have the insufficient balance and return `TradePreimageError::NotSufficientBalance` error.
let gas_limit = self.estimate_gas_wrapper(estimate_gas_req).compat().await?;
let gas_limit = self
.estimate_gas_with_pay_for_gas_option(estimate_gas_req, pay_for_gas_option.clone())
.await
.map_mm_err()?;
let total_fee = calc_total_fee(gas_limit, &pay_for_gas_option).map_mm_err()?;
let amount = u256_to_big_decimal(total_fee, ETH_DECIMALS).map_mm_err()?;
Ok(TradeFee {
Expand Down Expand Up @@ -7456,12 +7483,31 @@ impl From<Web3RpcError> for EthGasDetailsErr {
}
}

/// Returns `true` when an RPC error reports that the provided EIP-1559 `max_fee_per_gas` is below the
/// current block base fee (so the call/tx would be underpriced). Different nodes word this differently:
/// - geth: `"max fee per gas less than block base fee"`
/// - older Nethermind: `"fee cap less than block base fee"`
/// - newer Nethermind (e.g. the client behind Gnosis): `"miner premium is negative"`
fn is_max_fee_per_gas_below_base_fee_error(error_str: &str) -> bool {
let error_str = error_str.to_lowercase();
error_str.contains("fee cap less than block base fee")
|| error_str.contains("max fee per gas less than block base fee")
|| error_str.contains("miner premium is negative")
}

/// Extracts the `(provided_fee_cap, required_base_fee)` pair (in wei) from a "max fee per gas below base
/// fee" RPC error, when the node embeds the numbers. Handles both wordings seen in the wild:
/// - geth (older): `"... gasFeeCap: 140 baseFee: 146 ..."`
/// - geth / Nethermind >= v1.38.0: `"... maxFeePerGas: 140000000000, baseFee: 146283608928 ..."`
///
/// Returns `None` when no numbers are present (e.g. the bare `"miner premium is negative"`), in which
/// case the caller falls back to the generic [`EthGasDetailsErr::GasFeeCapBelowBaseFee`].
fn parse_fee_cap_error(message: &str) -> Option<(U256, U256)> {
let re = Regex::new(r"gasfeecap: (\d+)\s+basefee: (\d+)").ok()?;
let caps = re.captures(message)?;
let fee_cap_re = Regex::new(r"(?i)(?:gasfeecap|maxfeepergas):\s*(\d+)").ok()?;
let base_fee_re = Regex::new(r"(?i)basefee:\s*(\d+)").ok()?;

let user_cap_str = caps.get(1)?.as_str();
let required_base_str = caps.get(2)?.as_str();
let user_cap_str = fee_cap_re.captures(message)?.get(1)?.as_str();
let required_base_str = base_fee_re.captures(message)?.get(1)?.as_str();

let user_cap = U256::from_dec_str(user_cap_str).ok()?;
let required_base = U256::from_dec_str(required_base_str).ok()?;
Expand Down Expand Up @@ -7560,9 +7606,7 @@ async fn get_eth_gas_details_from_withdraw_fee(
let amount = u256_to_big_decimal(eth_value, eth_coin.decimals).map_mm_err()?;

return MmError::err(EthGasDetailsErr::AmountTooLow { amount, threshold });
} else if error_str.contains("fee cap less than block base fee")
|| error_str.contains("max fee per gas less than block base fee")
{
} else if is_max_fee_per_gas_below_base_fee_error(&error_str) {
if let Some((user_cap, required_base)) = parse_fee_cap_error(&error_str) {
// The RPC error gives fee values in wei. Convert to Gwei (9 decimals) for the user.
let provided_fee_cap = u256_to_big_decimal(user_cap, ETH_GWEI_DECIMALS).map_mm_err()?;
Expand Down
127 changes: 127 additions & 0 deletions mm2src/coins/eth/eth_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -521,6 +521,133 @@ fn test_get_fee_to_send_taker_fee() {
assert_eq!(actual, expected_fee);
}

/// Regression test for Gnosis/Nethermind: their newer release validates the EIP-1559 fee fields during
/// `eth_estimateGas` and rejects the call with `"miner premium is negative"` when `max_fee_per_gas` is
/// below the current base fee. Since gas *usage* is independent of the fee values, the estimate must
/// transparently retry without the fee fields so fee preimages / order posting keep working.
/// See [`EthCoin::estimate_gas_with_pay_for_gas_option`].
#[cfg(not(target_arch = "wasm32"))]
#[test]
fn test_estimate_gas_retries_without_fee_fields_when_max_fee_below_base_fee() {
const ESTIMATED_GAS: u64 = 60_000;
static FEE_BEARING_ATTEMPTS: AtomicU64 = AtomicU64::new(0);
static FEE_LESS_ATTEMPTS: AtomicU64 = AtomicU64::new(0);

// Force EIP-1559 fee fields into the estimate request regardless of the configured policy.
EthCoin::get_swap_pay_for_gas_option.mock_safe(|_, _| {
MockResult::Return(Box::pin(futures::future::ok(PayForGasOption::Eip1559 {
max_fee_per_gas: 1_000_000_000u64.into(),
max_priority_fee_per_gas: 100_000_000u64.into(),
})))
});

// Emulate a Nethermind/Gnosis node: reject while the fee fields are present, succeed once omitted.
EthCoin::estimate_gas_wrapper.mock_safe(|_, req: CallRequest| {
if req.max_fee_per_gas.is_some() || req.max_priority_fee_per_gas.is_some() || req.gas_price.is_some() {
FEE_BEARING_ATTEMPTS.fetch_add(1, AtomicOrdering::Relaxed);
MockResult::Return(Box::new(futures01::future::err(web3::Error::InvalidResponse(
"RPC error: Error { code: ServerError(-32000), message: \"miner premium is negative\", data: None }"
.to_string(),
))))
} else {
FEE_LESS_ATTEMPTS.fetch_add(1, AtomicOrdering::Relaxed);
MockResult::Return(Box::new(futures01::future::ok(ESTIMATED_GAS.into())))
}
});

let (_ctx, coin) = eth_coin_for_test(
EthCoinType::Erc20 {
platform: ETH.to_string(),
token_addr: Address::default(),
},
&["http://dummy.dummy"],
None,
ETH_SEPOLIA_CHAIN_ID,
);

let gas = block_on(coin.estimate_gas_for_contract_call(Address::default(), vec![0u8; 4].into(), 0.into()))
.expect("estimate_gas_for_contract_call should succeed via the fee-less retry");

assert_eq!(gas, ESTIMATED_GAS.into());
assert_eq!(
FEE_BEARING_ATTEMPTS.load(AtomicOrdering::Relaxed),
1,
"the first attempt must include the EIP-1559 fee fields"
);
assert_eq!(
FEE_LESS_ATTEMPTS.load(AtomicOrdering::Relaxed),
1,
"the rejected fee fields must trigger exactly one fee-less retry"
);
}

/// Estimate errors unrelated to the base fee must propagate as-is (no fee-less retry), so we never mask
/// a genuine failure behind a second request.
#[cfg(not(target_arch = "wasm32"))]
#[test]
fn test_estimate_gas_does_not_retry_on_unrelated_error() {
static ATTEMPTS: AtomicU64 = AtomicU64::new(0);

EthCoin::get_swap_pay_for_gas_option.mock_safe(|_, _| {
MockResult::Return(Box::pin(futures::future::ok(PayForGasOption::Eip1559 {
max_fee_per_gas: 1_000_000_000u64.into(),
max_priority_fee_per_gas: 100_000_000u64.into(),
})))
});
EthCoin::estimate_gas_wrapper.mock_safe(|_, _| {
ATTEMPTS.fetch_add(1, AtomicOrdering::Relaxed);
MockResult::Return(Box::new(futures01::future::err(web3::Error::InvalidResponse(
"execution reverted".to_string(),
))))
});

let (_ctx, coin) = eth_coin_for_test(
EthCoinType::Erc20 {
platform: ETH.to_string(),
token_addr: Address::default(),
},
&["http://dummy.dummy"],
None,
ETH_SEPOLIA_CHAIN_ID,
);

let err = block_on(coin.estimate_gas_for_contract_call(Address::default(), vec![0u8; 4].into(), 0.into()))
.expect_err("unrelated estimate errors must not be retried");
assert!(
err.to_string().to_lowercase().contains("execution reverted"),
"unexpected error: {}",
err
);
assert_eq!(
ATTEMPTS.load(AtomicOrdering::Relaxed),
1,
"an unrelated error must not trigger a retry"
);
}

/// `parse_fee_cap_error` must extract the fee cap and base fee from both the geth wording and the
/// Nethermind >= v1.38.0 wording (and yield `None` for the numberless "miner premium is negative").
#[cfg(not(target_arch = "wasm32"))]
#[test]
fn test_parse_fee_cap_error() {
// geth (older) wording: space-separated values.
let geth = "max fee per gas less than block base fee: address 0xabc, gasfeecap: 140 basefee: 146";
assert_eq!(
parse_fee_cap_error(geth),
Some((U256::from(140u64), U256::from(146u64)))
);

// geth / Nethermind >= v1.38.0 wording: comma-separated, mixed case.
let nethermind = "max fee per gas less than block base fee: address 0xABC, maxFeePerGas: 140000000000, baseFee: 146283608928 (supplied gas 56786)";
assert_eq!(
parse_fee_cap_error(nethermind),
Some((U256::from(140000000000u64), U256::from(146283608928u64)))
);

// The bare Nethermind condition carries no numbers, so the caller falls back to the generic error.
assert_eq!(parse_fee_cap_error("miner premium is negative"), None);
}

/// Some ERC20 tokens return the `error: -32016, message: \"The execution failed due to an exception.\"` error
/// if the balance is insufficient.
/// So [`EthCoin::get_fee_to_send_taker_fee`] must return [`TradePreimageError::NotSufficientBalance`].
Expand Down
Loading