-
Notifications
You must be signed in to change notification settings - Fork 6.3k
EVM instruction interpreter: Add loadimmutable and setimmutable mocks #16387
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -565,6 +565,26 @@ u256 EVMInstructionInterpreter::evalBuiltin( | |
| return 0; | ||
| } | ||
|
|
||
| if (fun == "loadimmutable") | ||
| { | ||
| yulAssert(_arguments.size() == 1); | ||
| yulAssert(std::holds_alternative<Literal>(_arguments[0])); | ||
| std::string const identifier = formatLiteral(std::get<Literal>(_arguments[0])); | ||
| // Return a deterministic value based on the identifier. | ||
| // This is sufficient for differential fuzzing since the same identifier | ||
| // will always return the same value, maintaining trace equivalence. | ||
| return u256(h256(identifier)); | ||
| } | ||
|
|
||
| if (fun == "setimmutable") | ||
| { | ||
| yulAssert(_arguments.size() == 3); | ||
| // No-op: The real implementation patches placeholder bytes in memory-loaded runtime code. | ||
| // For differential fuzzing, this ensures correct code never fails (no false positives), though some | ||
| // bugs in setimmutable handling may not be detected (potential false negatives). | ||
| return 0; | ||
| } | ||
|
Comment on lines
+568
to
+586
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO it would be better store immutable values in a map inside It would still be extremely simple in terms of implementation. You can ignore all the complications of whether we are in creation or deployed bytecode. Just always allow it and only revert if the value has not been set yet. As a bonus it will also let us print their values along with state in interpreter tests. Immutables are not some obscure corner case, so I'd think being able to correctly evaluate conditions that include them would be quite important. At least on a level comparable to correctly reading/writing to storage. I'm actually surprised that we never ran into it before - is fuzzer somehow less likely to generate code with immutables?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is semantically very different from what's actually happening though. This is used for fuzzing so I imagine a bunch of
I think it just got stuck in different parts of the corpus before and nobody ever bothered to fix the interpreter to a point where it can progress. Now that I have done that, these other things surface.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, right. I was thinking about the case where you set it at creation and read at runtime and can just overapproximate by also allowing to read it back already at creation time. But now that I think of it, we execute only in one of those contexts at a time, so we only have situations where we store but not read or read but not store. In that case this is probably good enough. |
||
|
|
||
| yulAssert(false, "Unknown builtin: " + fun); | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be good to have at least some minimal smoke tests for the interpreter. Working with code that's not covered with any tests and you can't tell immediately if you broke it is never nice.
We could create something like
test/libyul/yulInterpreterTests/isoltestTesting/and put such tests there. We do that with other kinds of tests.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually wanted to comment the same for #16267 earlier, but that was merged before I got to it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep I agree, would be good.