fix: align VM with EVM stack semantics and harden decode/cache#692
Closed
Trustdev-eth wants to merge 1 commit into
Closed
fix: align VM with EVM stack semantics and harden decode/cache#692Trustdev-eth wants to merge 1 commit into
Trustdev-eth wants to merge 1 commit into
Conversation
- JUMPI pops condition then counter; JUMP/JUMPI reject out-of-range targets without slicing past bytecode. - Restore correct stack order for CALL family, RETURN/REVERT, and memory copy opcodes (incl. gas on destination expansion where applicable). - SIGNEXTEND skips work when byte index > 31; EXP gas uses ceil(bit_len / 8) for nonzero exponents. - MCOPY uses EIP-5656 stack order and max(source, dest) expansion for dynamic gas. - Stack peek/peek_no longer fabricates values; insufficient depth surfaces as stack underflow in step. - Symbolic executor uses JUMPI inputs[0]/[1] for condition vs jump target and swaps branch exploration PCs. - Decode rejects calldata shorter than four bytes and replaces panics with errors on raw heuristic failure. - Cache rejects path-like keys; safe hex decode and portable delete via std::fs; docs clarify expiry is absolute unix ts. - Use std::fs for delete_path in heimdall-common on Windows-compatible removal. BREAKING CHANGE: Stack::peek returns Option<StackFrame>; Stack::peek_n returns Option<Vec<_>>. Tests: update jump/JUMPI bytecode and assertions for corrected control flow. Co-authored-by: Cursor <cursoragent@cursor.com>
Jon-Becker
reviewed
May 9, 2026
Owner
There was a problem hiding this comment.
why are these changes necessary? please fill out the PR description.
how have you validated that the evm changes do not break decompilation and or other vm tests? there seems to be a lack of test coverage. generally, if we change the way a function works we should add tests and ensure old tests continue to pass.
please see CI checks as well.
thank you for contributing!
|
|
||
| let binary_vec = decode_hex(&binary_string) | ||
| .map_err(|e| Error::Generic(format!("failed to decode hex: {e:?}")))?; | ||
| let binary_vec = decode_hex(&binary_string)?; |
Owner
There was a problem hiding this comment.
why did we remove the verbose error string?
| /// Decode a hex string into a bytearray | ||
| pub(crate) fn decode_hex(s: &str) -> Result<Vec<u8>, ParseIntError> { | ||
| (0..s.len()).step_by(2).map(|i| u8::from_str_radix(&s[i..i + 2], 16)).collect() | ||
| pub(crate) fn decode_hex(s: &str) -> Result<Vec<u8>, Error> { |
|
|
||
| let potential_type = | ||
| to_type(potential_types.first().expect("potential types is empty")); | ||
| let potential_type = if let Some(t) = potential_types.first() { |
Owner
There was a problem hiding this comment.
we could use more idiomatic rust here and use potential_types.first().unwrap_or("bytes32")
| pub fn signextend(vm: &mut VM, operation: WrappedOpcode) -> Result<()> { | ||
| let x = vm.stack.pop()?.value; | ||
| let b = vm.stack.pop()?.value; | ||
| let byte_idx = vm.stack.pop()?.value; |
Owner
There was a problem hiding this comment.
please update test expectations
| /// CALL - Message-call into an account | ||
| pub fn call(vm: &mut VM, operation: WrappedOpcode) -> Result<()> { | ||
| // Stack (top first): retSize, retOffset, argsSize, argsOffset, value, address, gas | ||
| let _ret_size = vm.stack.pop()?.value; |
| pub fn callcode(vm: &mut VM, operation: WrappedOpcode) -> Result<()> { | ||
| let _ret_size = vm.stack.pop()?.value; | ||
| let _ret_offset = vm.stack.pop()?.value; | ||
| let _args_size = vm.stack.pop()?.value; |
| pub fn delegatecall(vm: &mut VM, operation: WrappedOpcode) -> Result<()> { | ||
| // Stack (top first): retSize, retOffset, argsSize, argsOffset, address, gas | ||
| let _ret_size = vm.stack.pop()?.value; | ||
| let _ret_offset = vm.stack.pop()?.value; |
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
2 participants
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.
JUMPI pops condition then counter; JUMP/JUMPI reject out-of-range targets without slicing past bytecode.
Restore correct stack order for CALL family, RETURN/REVERT, and memory copy opcodes (incl. gas on destination expansion where applicable).
SIGNEXTEND skips work when byte index > 31; EXP gas uses ceil(bit_len / 8) for nonzero exponents.
MCOPY uses EIP-5656 stack order and max(source, dest) expansion for dynamic gas.
Stack peek/peek_no longer fabricates values; insufficient depth surfaces as stack underflow in step.
Symbolic executor uses JUMPI inputs[0]/[1] for condition vs jump target and swaps branch exploration PCs.
Decode rejects calldata shorter than four bytes and replaces panics with errors on raw heuristic failure.
Cache rejects path-like keys; safe hex decode and portable delete via std::fs; docs clarify expiry is absolute unix ts.
Use std::fs for delete_path in heimdall-common on Windows-compatible removal.
BREAKING CHANGE: Stack::peek returns Option; Stack::peek_n returns Option<Vec<_>>.
Tests: update jump/JUMPI bytecode and assertions for corrected control flow.
What changed? Why?
Notes to reviewers
How has it been tested?