fix(evm): fix multiple interpreter gas and call handling bugs#372
Open
starwarfan wants to merge 3 commits intoDTVMStack:mainfrom
Open
fix(evm): fix multiple interpreter gas and call handling bugs#372starwarfan wants to merge 3 commits intoDTVMStack:mainfrom
starwarfan wants to merge 3 commits intoDTVMStack:mainfrom
Conversation
… in interpreter fallback path The SPP (Safe Prepaid Points) optimization shifted gas costs between basic blocks to reduce metering points. When a predecessor block's gas chunk could not be paid for (insufficient gas), the interpreter fell back to per-instruction execution for that block. However, successor blocks whose costs had been shifted to the predecessor retained a metered cost of 0, allowing them to execute their instructions without any gas charge. Additionally, the individual instruction dispatch path (used when the gas chunk condition fails) did not check opcode validity against the current EVM revision. Since undefined opcodes have gas_cost=0 in the EVMC metrics table, they passed the gas check and executed, producing incorrect results for revision-sensitive opcodes like PUSH0 (Shanghai+), SELFBALANCE (Istanbul+), BASEFEE (London+), and RETURNDATASIZE (Byzantium+). Fix 1: Use original basic-block gas costs instead of SPP-shifted costs for the gas chunk cache. This preserves the per-block gas deduction optimization while eliminating the inter-block cost transfer that caused the correctness issue. Fix 2: Add a NamesTable validity check in the individual instruction path, matching the check already present in the gas chunk fast path. Undefined opcodes now correctly trigger EVMC_UNDEFINED_INSTRUCTION. Fixes ~12,800 fuzz crashes in interpreter mode (status mismatch: success vs failure) found by evmone-fuzzer differential testing. Made-with: Cursor
Fix seven bugs found by evmone-fuzzer differential testing: 1. Remove static from calculateGas macros - static variables cached the metrics table from the first call, returning stale gas costs when the EVM revision changed between fuzzer test cases. 2. Fix uint256-to-uint64 truncation in CALL gas argument - use uint256ToUint64 (clamps to UINT64_MAX) instead of static_cast which silently truncated high bits, producing incorrect sub-call gas values. 3. Clear return data on SELFDESTRUCT - SELFDESTRUCT must produce empty output, but DTVM was leaving stale return data from prior operations. 4. Short-circuit CREATE balance check when value is zero - the unconditional get_balance call recorded an account access in the mock host, incorrectly warming msg.recipient and causing EIP-2929 cold access charges to be skipped for later instructions. 5. Add SSTORE as gas chunk terminator - the gas chunk mechanism pre-charges the entire block cost before executing instructions. SSTORE's EIP-2200 minimum gas check (gas_left > 2300) saw artificially low gas because future instructions' costs (e.g. SELFDESTRUCT's 5000) were already deducted. 6. Always set return data from CREATE call result - DTVM only preserved return data on REVERT, but evmone (and the EVM spec) always sets RETURNDATASIZE from the sub-call result regardless of status. This caused RETURNDATASIZE mismatches after failed CREATE calls, leading to wrong stack values and selfdestruct record differences. Fixes remaining ~440 fuzz crashes in interpreter mode found by evmone-fuzzer differential testing (gas_left, status, output, and selfdestruct mismatches). Made-with: Cursor
⚡ Performance Regression Check Results✅ Performance Check Passed (interpreter)Performance Benchmark Results (threshold: 20%)
Summary: 194 benchmarks, 1 regressions ✅ Performance Check Passed (multipass)Performance Benchmark Results (threshold: 20%)
Summary: 194 benchmarks, 0 regressions |
…k test Two fixes: 1. CREATE return data: On successful CREATE, the host returns the deployed contract code as output_data. The previous unconditional setReturnData from the result incorrectly exposed the contract code via RETURNDATASIZE, breaking 12 create_returndata state tests. Per the EVM spec (and geth), return data should be empty after successful CREATE. On REVERT or other failures, return data is correctly set from the sub-call output, matching evmone behavior for non-REVERT CREATE failure fuzz cases. 2. Fallback test: Update MultipleFallbackTriggers to expect EVMC_UNDEFINED_INSTRUCTION for opcode 0xEE, matching the opcode validity check added in the predecessor commit. Made-with: Cursor
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes multiple interpreter-mode EVM correctness issues uncovered by differential fuzzing (gas accounting, opcode validity, call/create semantics, and return-data handling), aligning behavior more closely with EVMC/evmone expectations across EVM revisions and fallback execution paths.
Changes:
- Remove stale
staticcaching of EVMC instruction metrics in opcode gas calculation. - Fix CALL/CREATE/SELFDESTRUCT behavioral mismatches (CALL gas truncation, CREATE balance access warming, CREATE/SELFDESTRUCT return-data handling).
- Adjust interpreter fallback opcode validity checks and gas-chunk boundaries (terminate chunks at SSTORE; use non-SPP block costs).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/tests/evm_fallback_execution_tests.cpp | Updates expectation for invalid opcode handling to EVMC_UNDEFINED_INSTRUCTION. |
| src/evm/opcode_handlers.cpp | Fixes gas metric table staleness and several CALL/CREATE/SELFDESTRUCT semantics. |
| src/evm/interpreter.cpp | Adds opcode validity guard in the per-instruction fallback dispatch path. |
| src/evm/evm_cache.cpp | Terminates gas chunks at SSTORE and uses correct per-block gas costs for SPP chunks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1202
to
+1203
| Context->setReturnData(std::vector<uint8_t>( | ||
| Result.output_data, Result.output_data + Result.output_size)); |
There was a problem hiding this comment.
Result.output_data can be null when Result.output_size == 0 (several other call/create handling sites in the repo explicitly guard for this). Constructing a std::vector from a null pointer range is undefined behavior even if the range is empty. Please guard on Result.output_data != nullptr && Result.output_size > 0 (otherwise clear return data) before calling setReturnData() here.
Suggested change
| Context->setReturnData(std::vector<uint8_t>( | |
| Result.output_data, Result.output_data + Result.output_size)); | |
| if (Result.output_data != nullptr && Result.output_size > 0) { | |
| Context->setReturnData(std::vector<uint8_t>( | |
| Result.output_data, Result.output_data + Result.output_size)); | |
| } else { | |
| Context->setReturnData(std::vector<uint8_t>()); | |
| } |
Contributor
|
conflict |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fixes six additional bugs found by evmone-fuzzer differential testing in interpreter mode, resolving the remaining ~440 crash files after PR #371.
Bug 1 — Stale
staticgas cost tables: ThecalculateGas()macros inopcode_handlers.cppusedstaticvariables to cache the instruction metrics table pointer and gas cost. These were initialized once on the first call and never updated, returning stale gas costs when the EVM revision changed between fuzzer test cases in the individual instruction fallback path.Bug 2 — uint256-to-uint64 truncation in CALL gas:
CallHandler::doExecute()usedstatic_cast<uint64_t>(Gas)to convert theintx::uint256gas argument from the stack, silently truncating bits above bit 63. Large gas values (e.g.,0xFFFF...) became small values instead of being clamped toUINT64_MAX. Changed to use the existinguint256ToUint64()helper.Bug 3 — SELFDESTRUCT not clearing return data:
SelfDestructHandler::doExecute()did not clearReturnData, leaving stale data from prior operations (e.g., STATICCALL) as the final output. The EVM spec requires SELFDESTRUCT to produce empty output.Bug 4 — CREATE unconditionally calling
get_balance:CreateHandler::doExecute()always calledFrame->Host->get_balance(Frame->Msg.recipient)even whenValue == 0. In the MockedHost,get_balance()callsrecord_account_access(), which implicitly warms the address for futureaccess_account()calls. Evmone short-circuits withendowment != 0 &&, skipping the call when value is zero. This caused DTVM to skip 2600-gas EIP-2929 cold access charges that evmone correctly applied (32 gas_left mismatches, all differing by exactly 2600).Bug 5 — SSTORE minimum gas check affected by gas chunk pre-charging: The gas chunk mechanism pre-charges the entire block's gas cost before executing any instructions. When a block contained SSTORE followed by expensive instructions (e.g., SELFDESTRUCT with 5000 gas), the SSTORE EIP-2200 minimum gas check (
gas_left <= 2300) saw artificially low gas because future instructions' costs were already deducted. AddedOP_SSTOREas a gas chunk terminator so blocks always end at SSTORE boundaries, matching evmone's per-instruction gas semantics (7 status mismatches).Bug 6 — CREATE only setting return data on REVERT:
CreateHandler::doExecute()only preserved the sub-call's output asReturnDatawhen the result status wasEVMC_REVERT, clearing it for all other statuses (SUCCESS, FAILURE, etc.). Evmone always setsreturn_datafrom the call result regardless of status (matching EIP-211). This causedRETURNDATASIZEto return 0 instead of the actual output size after non-REVERT CREATE failures, corrupting subsequent stack values and producing wrong selfdestruct records (2 selfdestruct mismatches, 2 gas_left mismatches).1. Does this PR affect any open issues?(Y/N) and add issue references (e.g. "fix #123", "re #123".):
2. What is the scope of this PR (e.g. component or file name):
src/evm/opcode_handlers.cpp, src/evm/evm_cache.cpp
3. Provide a description of the PR(e.g. more details, effects, motivations or doc link):
This PR depends on and includes the commit from PR #371 (SPP gas cost shifting fix and opcode validity check). Together they resolve all ~13,150 interpreter-mode fuzz crashes found by evmone-fuzzer differential testing.
4. Are there any breaking changes?(Y/N) and describe the breaking changes(e.g. more details, motivations or doc link):
5. Provide a description of the tests:
Verified with evmone-fuzzer differential testing in interpreter mode:
Made with Cursor