-
Notifications
You must be signed in to change notification settings - Fork 6k
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
Adjust semantic tests to work on both EOF and legacy #15768
Conversation
new B1{value: 10}(); | ||
new B2{value: 10}(); | ||
new B3{value: 10}(); | ||
new B1{value: 10, salt: hex"00"}(); | ||
new B2{value: 10, salt: hex"01"}(); | ||
new B3{value: 10, salt: hex"02"}(); |
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.
Cases like this fail on EOF, because the default salt is 0
and therefore unsalted new
always generates the same address. And deploying to the same address causes a revert unless the initcode is identical.
uint[4] memory input; | ||
uint[6] memory input; | ||
input[0] = p1.X; | ||
input[1] = p1.Y; | ||
input[2] = p2.X; | ||
input[3] = p2.Y; | ||
bool success; | ||
assembly { | ||
success := call(sub(gas(), 2000), 6, 0, input, 0xc0, r, 0x60) | ||
// Use "invalid" to make gas estimation work | ||
switch success case 0 { invalid() } | ||
} | ||
(bool success, bytes memory encodedResult) = address(6).call(abi.encode(input)); |
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.
This is something I don't really understand. According to https://www.evm.codes/precompiled ecadd
(precompile at address 0x06
) accepts 4 uints. However the original code was passing in 6 of them (0xc0
bytes). It reverts if I try to pass in 4.
Similarly, ecmul
below seems to expect 4 rather than 3 parameters.
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.
BTW, the reason I'm removing the assembly blocks in this file is of course that call()
is only available on legacy and the equivalent extcall()
only on EOF. We currently can't have assembly that works on both, but we can use <address>.call()
in Solidity, which gets automatically translated to the right opcode.
This contract seems to be using assembly only to work around some problem related to gas estimation (and maybe also to make the call cheaper), which is not an issue in our 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.
For the record, @ekpyron got to the bottom of it:
My guess would be that it's a typo in ancient versions of this (there's very old snippets of this piece of code around) (i.e. it should always have been 0x80 as size, but the oldest versions dating back to 2017 or older just had 0xC0, and since that also works as per spec, nobody ever fixed it, and instead the hard-coded cases in our implementation were adjusted to accommodate those - funny actually :-)) (see Zokrates/ZoKrates#1364 and such - and Zokrates/ZoKrates#1364 (comment) in particular :-))
#15788 fixes this.
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.
This one was almost identical to gas_and_value_basic.sol
(modulo some formatting/naming). I think that it was created based on the other one when we introduced the brace syntax. But then when we removed that syntax, we updated the original test instead of removing it.
// gas legacyOptimized: 180756 | ||
// getAddress() -> 0x137aa4dfc0911524504fcd4d98501f179bc13b4a | ||
// balance: 0x137aa4dfc0911524504fcd4d98501f179bc13b4a -> 500 | ||
// balance: 0x0000000000000000000000000000000000001234 -> 500 |
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.
Here the problem was that the balance
helper requires hard-coding the address and, if we get that address by deploying a contract, it will be different on EOF.
To test balance, we don't really need a new contract though. We can just send some value to a randomly chosen address and then it won't change on EOF.
@@ -6,19 +6,19 @@ contract Succeeds { | |||
} | |||
|
|||
contract C { | |||
function f() public returns (Reverts x, uint, string memory txt) { |
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.
Not sure what the uint
was for. It's unused so it does not make any difference other than adding an extra zero in expectations. It does seems too relevant to the test so I removed it.
} | ||
|
||
function sendAmount(uint256 amount) public returns (uint256 bal) { | ||
return h.getBalance{value: amount + 3, gas: 1000}(); |
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.
The name of this test is so vague that it's useless. I couldn't tell what this was supposed to test just from that, but looking at the oldest version, it was for multiple applications of .value()
:
return h.getBalance.value(amount).gas(1000).value(amount + 3)();// overwrite value
This is no longer possible with the {value: ...}
syntax so the test is obsolete and should have been removed when we deprecated the old syntax.
43d9396
to
f5f54f5
Compare
f5f54f5
to
ff98993
Compare
@@ -20,12 +24,12 @@ contract C { | |||
} | |||
} | |||
// ---- | |||
// x() -> 0 | |||
// isSet() -> 0 |
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.
This yields 0
instead of false
(not analogous to the true
below). Not sure why and I don't think it matters much, just a bit inconsistent (also besides the point of the PR)
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.
In soltest expectations we don't generally enforce the type. So you can write either and it will be accepted.
This is just cosmetic, but I'll be updating the test due to #15665 (comment) anyway so I'll change 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.
Test updated. Now it no longer calls the function (which is what caused the difference on EOF due to lack of EXTCODESIZE
check) but we have other tests covering that.
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.
this looks like tryCatch/assert.sol
just that it isn't restricted on any version - how come this one needs a restriction? is one of the two enough or am i missing something?
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.
ah this one has a require
instead of assert
:)
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.
Yeah, one has assert
the other has require
. I renamed trivial.sol
to require.sol
to make that clearer.
But you do have a point here. Somehow I missed the fact that this now just works and neither version nor gas restriction is needed. Not sure why it was needed in the first place - I assumed that was because assert
used to eat all gas (via INVALID
opcode) and was later changed to revert with Panic
. But that actually did not depend on EVM version so that's not it. In any case, I removed the restrictions now.
Ah, no, called it too early. The EVM restriction is actually still needed. It should have been in both assert.sol
and trivial.sol
. It just passes CI here because we run tests for older EVMs only nightly.
12ea3af
to
5f0e61e
Compare
5f0e61e
to
c98f1c2
Compare
- The ones in `functionCall` were testing multiple applications of `.value()`, which is no longer possible with the `{value: ...}` syntax. - The ones in `various` became identical when the `.value()` syntax was deprecated.
…th EOF and legacy
c98f1c2
to
3b1d78d
Compare
Prerequisite for #15665.
We have some semantic tests which don't pass on EOF as is, but can fairly easily be adjusted to pass without losing the original purpose of the test. This PR does that for the cases I noticed while reviewing #15665.
It also removes some tests for the old
.value()
syntax that are now redundant.