Skip tx simulation on endpoints that don't mine reverts #4143
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a performance optimization by skipping transaction simulation for mempools that do not mine reverting transactions. The implementation correctly adds a conditional check before calling estimate_gas. My review identifies a high-severity issue in the error handling logic for estimate_gas. Currently, if the simulation fails for a non-revert reason such as a network error, the system only logs a warning and continues, which can lead to subsequent failures and mask the root problem. I have suggested a change to fail fast by returning an error in this scenario, a strategy supported by existing repository guidelines for handling unrecoverable errors in critical tasks.
| } else { | ||
| tracing::warn!( | ||
| ?err, | ||
| "couldn't simulate tx before submitting to the mempool" | ||
| ); | ||
| } |
There was a problem hiding this comment.
When estimate_gas fails with a non-revert error (e.g., a network issue), the current logic only logs a warning and proceeds. This can lead to further failed RPC calls for nonce fetching and transaction submission. It is better to fail fast by returning an error immediately.
} else {
return Err(Error::Other(anyhow::Error::from(err)
.context("failed to simulate tx before submitting to the mempool")));
}References
- This recommendation aligns with the principle of fail-fast behavior for unrecoverable errors in critical operations, as supported by the rule on panicking for unrecoverable errors in critical background tasks.
| } | ||
| } | ||
| } else { | ||
| tracing::trace!("skipping tx simulation because mempool does not mine reverting txs"); |
There was a problem hiding this comment.
The mempool (name, id, whatever) too, just in case?
There was a problem hiding this comment.
The name of the mempool is already injected into the logs via an info span here.
4 participants
Description
Based on our logs it seems like simulating a tx before submission can take up to 500ms. Since our submission logic know which mempool mines reverting txs and which don't we can simply skip the simulation step when the current mempool does not mine reverting txs.
Changes
skip tx simulation (estimate_gas()) in mempools that don't mine reverting txs (e.g. mevblocker).