Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| BOOST_OUTCOME_TRY( | ||
| apply_state_overrides(block_state, incarnation, state_overrides)); |
There was a problem hiding this comment.
[P2] This changes the observable eth_call behavior, not just eth_simulateV1: previously a can_merge failure threw MonadException, which the eth_call submit lambda's catch (MonadException const &) handler mapped to EVMC_INTERNAL_ERROR; now the error flows through the res.has_error() branch (monad_executor.cpp:1541), which unconditionally reports EVMC_REJECTED. If that status change for state-override failures on eth_call isn't intended, the eth_call error branch should map the status via simulation_error_info the way the eth_simulate path does (monad_executor.cpp:2027).
| catch (...) { | ||
| return Result<nlohmann::json>::error_type{ | ||
| SimulationError::InternalError}; | ||
| } |
There was a problem hiding this comment.
[P3] catch (...) collapses every MonadException thrown inside eth_simulate_impl (e.g. MONAD_ASSERT_THROW(sender.has_value(), RECOVER_SENDER_ERR_MSG) in recover_senders_and_authorities, reached via the parent/grandparent context loads at lines 809/824) into the generic "internal error" message. The outer handler in submit_eth_simulate_to_pool previously surfaced e.message() to the client with the same EVMC_INTERNAL_ERROR status, so this is a diagnosability regression for RPC users; consider a preceding catch (MonadException const &e) that logs or preserves the specific message.
There was a problem hiding this comment.
Review Summary
This PR converts the MONAD_ASSERT_THROW in apply_state_overrides into a Result<void> returning SimulationError::StateOverrideFailure, and wraps eth_simulate_impl in a function-level try/catch (...) that reifies escaped exceptions as SimulationError::InternalError. The new enum values are appended (no renumbering), both are handled in the simulation_status switch and the quick_status_code_from_enum mappings, and simulation_error_test.cpp covers the new messages. The enum is C++-only and does not cross the bindgen FFI boundary; monad_executor.h is untouched. Replacing a throw with an error value in RPC-facing code is a good direction, and the single-commit PR is well scoped.
Two findings, neither blocking:
- [P2] The
apply_state_overridessignature change also alterseth_callbehavior: a state-override merge failure now surfaces asEVMC_REJECTED(via the genericres.has_error()branch atmonad_executor.cpp:1541) instead of the previousEVMC_INTERNAL_ERRORfrom theMonadExceptionhandler. The PR description scopes the change toeth_simulateV1, so this looks unintended. - [P3] The blanket
catch (...)ineth_simulate_impldiscardsMonadExceptionmessages (e.g. "failed to recover sender") that the outer handler previously returned to the client, degrading them to a generic "internal error".
Verdict: CORRECT
🤖 Generated with Claude Code
There was a problem hiding this comment.
🟡 Changes recommended
Resolve the changed eth_call error/status behavior before approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR converts eth_simulateV1 execution exceptions and state-override failures into structured error values.
Changes:
- Adds
StateOverrideFailureandInternalError. - Maps errors to statuses and messages.
- Adds error-mapping tests.
- Catches deep execution exceptions during simulation.
File summaries
| File | Description |
|---|---|
category/rpc/simulation_error.hpp |
Adds simulation error types. |
category/rpc/simulation_error.cpp |
Maps errors to statuses and messages. |
category/rpc/simulation_error_test.cpp |
Tests new error mappings. |
category/rpc/monad_executor.cpp |
Reifies state and execution failures. |
Review details
Suppressed comments (1)
category/rpc/monad_executor.cpp:196
- [P2] This helper is also used by
eth_call_impl, whose caller treats everyResulterror asEVMC_REJECTED(seemonad_executor.cpp:1541-1545). Before this change a failedcan_mergethrewMonadException, so the outer handler returnedEVMC_INTERNAL_ERROR; now the same state-override failure is propagated asSimulationErrorand reported as rejected. Preserve the previous exception/status behavior foreth_callor map this specific error to an internal status there.
if (MONAD_UNLIKELY(!block_state.can_merge(state))) {
return SimulationError::StateOverrideFailure;
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| catch (...) { | ||
| return Result<nlohmann::json>::error_type{ | ||
| SimulationError::InternalError}; | ||
| } |
This patch installs an exception handler in the body of `eth_simulateV1` to catch exceptions thrown from deep within the execution stack, turning them into appropriate error values. This change enchances the modularity and reusability of `eth_simulateV1` in terms of external code interfacing with it.
6521601 to
d8fb5c7
Compare
This patch installs an exception handler in the body of
eth_simulateV1to catch exceptions thrown from deep within the execution stack, turning them into appropriate error values. This change enchances the modularity and reusability ofeth_simulateV1in terms of external code interfacing with it.