Support compile-time evaluation of keccak256 - #16989
matheusaaguiar wants to merge 5 commits into
Conversation
msooseth
left a comment
msooseth
left a comment
There was a problem hiding this comment.
Something feels off? Maybe I'm wrong.
| bytes result(_left.size()); | ||
| unsigned bitWidth = static_cast<unsigned>(_left.size()) * 8; | ||
|
|
||
| if (_right.numerator() >= bitWidth) | ||
| return result; |
There was a problem hiding this comment.
Are you sure? On evm.codes we have:
PUSH32 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF
PUSH2 450
SAR
evaluating to 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
There was a problem hiding this comment.
Shifts are always truncated.
The docs don't say anything for fixed-size byte arrays, but it seems they are following the same rule.
SHL always set the new bits to 0, while SAR set to the value of the previous most significant bit.
Your example shows the described behavior of SAR
However, the codegen apparently always cleanup the value.
contract C {
function f() public returns (bytes32, bytes32) {
bytes32 x = 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF;
bytes32 y;
assembly {
y := sar(450, x)
}
return (y, x >> 450);
}
}
// ----
// f() -> 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff, 0x00There was a problem hiding this comment.
Ah, actually codegen uses SHR for fixed-bytes. So it is not about cleanup.
In both pipelines for unsigned values, it is used SHR.
|
Maybe we should have a property test for all comptime evaluations? Should not be tooooo hard to write using EVMOne + a generator for these expressions? |
Yeah, I have already started working on a property test for the shift/not PR (16597). |
Yay! ❤️ |
2 participants
Implements #16421.
Introduces constant evaluation of the
keccak256builtin function.Support for
bytes32constants is also added since the builtin needs it for argument and return values.There are no user-visible changes because there is no comp-time context that uses bytes32 currently.
Also note that the commit containing the refactor of
TypedValueis here for convenience.I expected it to be merged already as part of another PR, but now I am tempted to move to its own PR...