EVM instruction interpreter: Add loadimmutable and setimmutable mocks #16387
Conversation
…utable Theoretically, loadimmutable inserts a placeholder into the contract to be deployed which is then replaced by setimmutable once the runtime code is loaded into memory. The interpreter is mainly used for differential fuzzing so we should satisfy that correct code never fails and we can still detect bugs otherwise, most of the time. Therefore loadimmutable is decoupled from setimmutable and just returns a random value (hash of the identifier), setimmutable is treated as a no-op.
clonker
force-pushed
the
evm_instruction_interpreter_kiss_immutables
branch
from
7f71643 to
f8cae91
Compare
There was a problem hiding this comment.
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.
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.
Yep I agree, would be good.
| 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; | ||
| } |
There was a problem hiding this comment.
IMO it would be better store immutable values in a map inside setimmutable() and retrieve them from there in loadimmutable(). You could put that in IntepreterState::immutables.
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?
There was a problem hiding this comment.
It is semantically very different from what's actually happening though. This is used for fuzzing so I imagine a bunch of setimmutable and loadimmutable instructions being mashed together. If I treat setimmutable as a map store and loadimmutable as a map load then that is modeling a runtime relationship between the two - which doesn't exist.
I'm actually surprised that we never ran into it before - is fuzzer somehow less likely to generate code with immutables?
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.
There was a problem hiding this comment.
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.
Theoretically,
loadimmutableinserts a placeholder into the contract to be deployed which is then replaced bysetimmutableonce the runtime code is loaded into memory.The interpreter is mainly used for differential fuzzing so we should satisfy that correct code never fails and we can still - most of the time - detect bugs.
Therefore,
loadimmutableis decoupled fromsetimmutableand just returns a random value (hash of the identifier),setimmutableis treated as a no-op.