Enhance error handling in EoaExecutorWorker for transaction simulation #36
Conversation
- Added a fallback case to handle transaction reverts during gas estimation, ensuring that fundamentally broken transactions fail appropriately.
WalkthroughAdds a revert-detection fallback in gas estimation error handling: if EIP-1559 estimation fails and no revert data is present but the message includes "revert", return TransactionSimulationFailed with a descriptive message. Also consolidates a legacy gas price error format string into a single line. No API changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Worker
participant Provider
participant Engine
rect rgb(245,245,255)
note over Worker: Estimate gas (EIP-1559)
Worker->>Provider: eth_estimateGas (EIP-1559)
alt Estimation succeeds
Provider-->>Worker: Gas estimate
Worker->>Engine: Proceed with transaction
else Estimation fails
Provider-->>Worker: Error payload
alt Has revert data
note over Worker: Existing handling
Worker-->>Engine: TransactionSimulationFailed (with revert data)
else Message contains "revert" (new)
note over Worker: New branch: message-based revert detection
Worker-->>Engine: TransactionSimulationFailed ("Transaction reverted during gas estimation: …", with inner error)
else Other error
Worker->>Provider: Fallback to legacy gas price path
Provider-->>Worker: Legacy gas price or error
Worker->>Engine: Continue or propagate error (formatted single-line)
end
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
executors/src/eoa/worker/transaction.rs (2)
327-337: Broaden revert detection and avoid full Unicode lowercasing
- Use to_ascii_lowercase() (cheaper than to_lowercase()).
- Optionally recognize other revert-like phrases commonly returned by nodes (“invalid opcode”, “bad instruction”, “out of gas”, etc.), which are also deterministic execution failures.
Proposed focused change:
- } else if error_payload.message.to_lowercase().contains("revert") { + } else { + // Fallback: detect deterministic EVM execution failures via message text. + // Use ASCII case-insensitive checks for common phrases. + let msg_lc = error_payload.message.to_ascii_lowercase(); + if msg_lc.contains("revert") + || msg_lc.contains("invalid opcode") + || msg_lc.contains("bad instruction") + || msg_lc.contains("out of gas") + || msg_lc.contains("stack underflow") + || msg_lc.contains("stack overflow") + || msg_lc.contains("invalid jump") + { + tracing::debug!("Classifying gas estimation failure as deterministic revert (no data): {}", error_payload.message); // This is a revert - the transaction is fundamentally broken // This should fail the individual transaction, not the worker // We need this fallback case because some providers don't return revert data - return Err(EoaExecutorWorkerError::TransactionSimulationFailed { - message: format!( - "Transaction reverted during gas estimation: {}", - error_payload.message - ), - inner_error: e.to_engine_error(&self.chain), - }); + return Err(EoaExecutorWorkerError::TransactionSimulationFailed { + message: format!( + "Transaction reverted during gas estimation: {}", + error_payload.message + ), + inner_error: e.to_engine_error(&self.chain), + }); + } }
327-337: Add test coverage for revert-without-data classification in gas estimationPlease add unit tests for the
estimate_gascall inexecutors/src/eoa/worker/transaction.rsto verify all revert‐handling paths:• When the mocked provider returns
RpcError::ErrorResp { data: Some(revert_data), message, … }, assert that the result is
Err(EoaExecutorWorkerError::TransactionSimulationFailed)and that the error message includes the hex-encodedrevert_data.
• When it returnsRpcError::ErrorResp { data: None, message }wheremessage.to_lowercase().contains("revert"), assert the sameTransactionSimulationFailedwithout data in the message.
• When it returns any otherRpcError::ErrorResp(no “revert” marker), assert that the originalRpcErroris returned (so the worker will nack).You can use a test double or
mockallto implement a fakeProviderthat returns these scenarios, and place the tests alongside existing ones inexecutors/src/eoa/worker.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
executors/src/eoa/worker/transaction.rs(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
executors/src/eoa/worker/transaction.rs (1)
executors/src/eoa/error_classifier.rs (1)
message(224-236)
🔇 Additional comments (2)
executors/src/eoa/worker/transaction.rs (2)
213-215: LGTM: streamlined legacy gas price error message formattingSingle-line format keeps the same semantics and improves readability. No functional change.
327-337: Good fallback for revert-without-data during gas estimationTreating “revert” in the error message as a simulation failure closes the gap for providers that omit revert data. This aligns with the intent to fail the transaction rather than the worker.
| ), | ||
| inner_error: e.to_engine_error(&self.chain), | ||
| }); | ||
| } else if error_payload.message.to_lowercase().contains("revert") { |
There was a problem hiding this comment.
What happens for execution reverted with data 0x which can be lack of gas (evm panic), we handle that elsewhere? If not would add an extra condition that data is not 0x potentially
Don't need to block this pr on this, just wanted to add the comment for future reference
There was a problem hiding this comment.
this is simulation not execution, so we don't have the actual gas limit with us yet. In case you're referring to insufficient balance, that's handled here:
engine-core/executors/src/eoa/worker/error.rs
Line 208 in c932232
Summary by CodeRabbit