Skip to content
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

Merged
merged 2 commits into from
Jan 29, 2025

Conversation

cameel
Copy link
Member

@cameel cameel commented Jan 24, 2025

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.

@cameel cameel added the EOF label Jan 24, 2025
@cameel cameel self-assigned this Jan 24, 2025
Comment on lines -12 to +14
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"}();
Copy link
Member Author

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.

Comment on lines -38 to +45
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));
Copy link
Member Author

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.

Copy link
Member Author

@cameel cameel Jan 24, 2025

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.

Copy link
Member Author

@cameel cameel Jan 28, 2025

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.

Copy link
Member Author

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
Copy link
Member Author

@cameel cameel Jan 24, 2025

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) {
Copy link
Member Author

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}();
Copy link
Member Author

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.

@cameel cameel force-pushed the adjust-semantic-tests-to-work-on-eof-too branch from 43d9396 to f5f54f5 Compare January 24, 2025 02:21
@cameel cameel force-pushed the adjust-semantic-tests-to-work-on-eof-too branch from f5f54f5 to ff98993 Compare January 27, 2025 13:08
@@ -20,12 +24,12 @@ contract C {
}
}
// ----
// x() -> 0
// isSet() -> 0
Copy link
Member

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)

Copy link
Member Author

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.

Copy link
Member Author

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.

test/libsolidity/semanticTests/tryCatch/assert.sol Outdated Show resolved Hide resolved
Copy link
Member

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?

Copy link
Member

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 :)

Copy link
Member Author

@cameel cameel Jan 28, 2025

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.

@cameel cameel force-pushed the adjust-semantic-tests-to-work-on-eof-too branch 3 times, most recently from 12ea3af to 5f0e61e Compare January 28, 2025 22:04
@cameel cameel requested a review from clonker January 28, 2025 22:06
@cameel cameel force-pushed the adjust-semantic-tests-to-work-on-eof-too branch from 5f0e61e to c98f1c2 Compare January 29, 2025 01:40
- 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
@cameel cameel force-pushed the adjust-semantic-tests-to-work-on-eof-too branch from c98f1c2 to 3b1d78d Compare January 29, 2025 13:59
@cameel cameel enabled auto-merge January 29, 2025 13:59
@cameel cameel merged commit ea1b799 into develop Jan 29, 2025
73 of 74 checks passed
@cameel cameel deleted the adjust-semantic-tests-to-work-on-eof-too branch January 29, 2025 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants