Conversation
There was a problem hiding this comment.
Pull request overview
Introduces an explicit VM execution mode (Dual, CompilerOnly, InterpreterOnly) and wires it through the VM, Ethereum test runner, and call sites, while removing the legacy standalone blockchain-test binaries in favor of the unified monad-ethereum-test harness.
Changes:
- Add
vm::VM::Modeand update VM construction + execution flow to support compiler-only and interpreter-only behavior. - Extend
monad-ethereum-testto optionally restrict execution by VM mode and run fixtures across modes. - Remove
test/vm/blockchainexecutables and related build/script wiring; update VM README to point at the new runner.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/vm/unit/monad_vm_interface_tests.cpp | Expands unit tests to cover all VM modes and adjusts expectations for entrypoint availability/warm-cache behavior. |
| test/vm/blockchain/interpreter_blockchain_tests.cpp | Removes legacy interpreter blockchain test executable. |
| test/vm/blockchain/gtest_filter.hpp | Removes legacy shared gtest filter used by removed blockchain executables. |
| test/vm/blockchain/compiler_blockchain_tests.cpp | Removes legacy compiler blockchain test executable. |
| test/vm/blockchain/CMakeLists.txt | Removes build targets for legacy blockchain executables. |
| test/vm/CMakeLists.txt | Stops building the removed test/vm/blockchain subtree. |
| test/ethereum_test/src/main.cpp | Adds --vm_mode CLI option and passes mode restriction into blockchain test registration. |
| test/ethereum_test/src/blockchain_test.cpp | Runs blockchain fixtures across VM modes (or restricts to a fixed mode) and constructs vm::VM with the selected mode. |
| test/ethereum_test/include/blockchain_test.hpp | Threads optional fixed VM mode into BlockchainTest and registration APIs. |
| scripts/vm/test.sh | Stops running the removed blockchain executables. |
| scripts/vm/build-tests.sh | Stops building the removed blockchain executables. |
| cmd/monad/main.cpp | Switches VM construction to InterpreterOnly when call tracing is enabled, otherwise Dual. |
| category/vm/vm.hpp | Adds VM::Mode enum and declares cached_compile_and_execute_raw. |
| category/vm/vm.cpp | Implements mode-aware compile/execute path and compiler-only fallback behavior. |
| category/vm/varcode_cache.hpp | Adds enable_always_cold() to force “never warm” cache behavior. |
| category/vm/compiler.hpp | Exposes enable_always_cold_cache() via Compiler. |
| category/vm/README.md | Updates Ethereum test invocation guidance to use monad-ethereum-test and documents --vm_mode. |
| category/rpc/monad_executor.cpp | Forces interpreter-only VM for eth_call execution (mode-based constructor). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| `--vm_mode` flag to select between "compiler only", "interpreter only" or | ||
| "dual" mode. See |
There was a problem hiding this comment.
The README now suggests using --vm_mode values like "compiler only" / "interpreter only" / "dual", but the CLI is wired to magic_enum::enum_names<vm::VM::Mode>(), which will accept the raw enum names (e.g. Dual, CompilerOnly, InterpreterOnly, case-insensitive) rather than spaced phrases. Also, later in this README there are still examples invoking the removed build/test/blockchain/compiler-blockchain-tests binary, so the documentation is internally inconsistent. Please align the documented --vm_mode values and update remaining command examples to the new monad-ethereum-test runner (or implement a mapping so the documented strings are actually accepted).
| `--vm_mode` flag to select between "compiler only", "interpreter only" or | |
| "dual" mode. See | |
| `--vm_mode` flag to select between `CompilerOnly`, `InterpreterOnly`, or | |
| `Dual` mode (case-insensitive). See |
| for (auto const vm_mode : magic_enum::enum_values<vm::VM::Mode>()) { | ||
| if (fixed_vm_mode_.has_value() && | ||
| fixed_vm_mode_.value() != vm_mode) { | ||
| continue; | ||
| } | ||
| std::string const enum_name(magic_enum::enum_name(vm_mode)); | ||
| auto const full_name = name + "(" + enum_name + ")"; | ||
| process_test(rev, full_name, j_contents, vm_mode, enable_tracing_); | ||
| } |
There was a problem hiding this comment.
By default this now runs every blockchain fixture once per vm::VM::Mode (Dual/CompilerOnly/InterpreterOnly). That will multiply the runtime and number of test expectations by ~3, which is a significant CI/iteration-time increase. Consider defaulting to a single mode (e.g. Dual) unless --vm_mode is provided, or add an explicit --all_vm_modes flag to opt into running all modes.
| for (auto const vm_mode : magic_enum::enum_values<vm::VM::Mode>()) { | |
| if (fixed_vm_mode_.has_value() && | |
| fixed_vm_mode_.value() != vm_mode) { | |
| continue; | |
| } | |
| std::string const enum_name(magic_enum::enum_name(vm_mode)); | |
| auto const full_name = name + "(" + enum_name + ")"; | |
| process_test(rev, full_name, j_contents, vm_mode, enable_tracing_); | |
| } | |
| auto const process_for_mode = [&](vm::VM::Mode const vm_mode) { | |
| std::string const enum_name(magic_enum::enum_name(vm_mode)); | |
| auto const full_name = name + "(" + enum_name + ")"; | |
| process_test(rev, full_name, j_contents, vm_mode, enable_tracing_); | |
| }; | |
| if (fixed_vm_mode_.has_value()) { | |
| process_for_mode(fixed_vm_mode_.value()); | |
| } | |
| else { | |
| process_for_mode(vm::VM::Mode::Dual); | |
| } |
| LOG_WARNING("WARNING: VM: fallback to interpreter: " | ||
| "compilation failed in CompilerOnly mode."); |
There was a problem hiding this comment.
The warning logged on compilation failure in CompilerOnly mode is likely to be noisy and is not very actionable: it repeats "WARNING:" and does not include identifying context (e.g. code hash / revision / chain_id). Consider removing the redundant prefix and including code_hash (and/or traits::id() / mode) so operators can correlate the fallback with a specific contract/test.
| LOG_WARNING("WARNING: VM: fallback to interpreter: " | |
| "compilation failed in CompilerOnly mode."); | |
| LOG_WARNING("VM: fallback to interpreter after compilation " | |
| "failure (mode=CompilerOnly)."); |
| /// Compile the intercode and execute. In `CompilerOnly` mode, the | ||
| /// function will wait for (cached) compilation to finish and execute | ||
| /// the native entrypoint. Otherwise start async compilation and | ||
| /// execute with interpreter. | ||
| template <Traits traits> | ||
| evmc::Result cached_compile_and_execute_raw( | ||
| runtime::Context &rt_ctx, bytes32_t const &code_hash, | ||
| SharedIntercode const &icode); |
There was a problem hiding this comment.
The docstring for cached_compile_and_execute_raw says that in non-CompilerOnly modes it will "start async compilation and execute with interpreter", but in InterpreterOnly mode the compiler thread explicitly does not compile and instead installs a Nativecode with a null entrypoint. Please update the comment to reflect the actual InterpreterOnly behavior (no compilation, interpreter execution only), or adjust the implementation so the comment remains true.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review summary
One minor finding on docs wording, otherwise the change is straightforward. The new Mode enum cleanly replaces the old bool enable_async knob, the added CompilerOnly path routes through cached_compile synchronously while preserving the existing async/interpreter fallbacks for Dual and InterpreterOnly, and the blockchain-test consolidation into monad-ethereum-test with per-mode sub-runs matches the intent described in the commit messages. The existing VM(false) callers are updated consistently (monad_executor.cpp, cmd/monad/main.cpp). The InterpreterOnly mode's use of compiler_{false} preserves the pre-PR sentinel-nativecode behavior, which is what the RPC eth-call path relies on.
Verdict: CORRECT
🤖 Generated with Claude Code
| Use the `--gtest_filter` flag to enable or disable specific tests. See | ||
| Use the `--gtest_filter` flag to enable or disable specific tests. Use the | ||
| `--vm_mode` flag to select between "compiler only", "interpreter only" or | ||
| "dual" mode. See |
There was a problem hiding this comment.
[P3] The human-readable strings "compiler only", "interpreter only", and "dual" don't match the values the CLI actually accepts. --vm_mode is validated against magic_enum::enum_names<vm::VM::Mode>(), which yields Dual, CompilerOnly, and InterpreterOnly (case-insensitive), so --vm_mode "compiler only" will be rejected. Either spell out the actual values here or make it clearer that the quoted phrases are descriptions rather than literal arguments.
Waiting for #2220