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
(v0.8.21) Contract code changes with additional files in compilation #14494
Comments
Ok, I can so far confirm that this will result in different libevmasm assembly.
|
The assembly differs in some details in the stack ordering (I e.g. see different swaps). Haven't been able to trace down a reason yet. |
Interestingly, the difference in swapping is not equivalent, e.g. I'm seeing stuff like
So this may also be non-determinism in the stack layouts (yielding different stack layouts across basic blocks resulting in differing stack shuffling). But I'd rather expect it to be a secondary effect of something else. |
But ok - optimized Yul being identical modulo AST IDs, but libevmasm assembly differing points towards the code transform |
Independently of solving this particular issue, though, since we seem to keep running into, we should look into test coverage for this. So far, the ultimate reason has been offsets in the AST IDs, so we should consider a bytecode comparison run with an artificial offset in the AST IDs - that may catch more of these cases, resp. avoid them in the future. |
Looking more closely at this again, there's actually a small differences in the optimized Yul that may explain the divergence, so it's may be the yul optimizer after all. |
The difference is an additional variable in one yul function: function fun_upperBinaryLookup(var_self_3885_slot, var_key, var_low, var_high) -> var
{
for { }
lt(var_low, var_high)
{ }
{
let expr := and(var_low, var_high)
let _1 := 1
let sum := add(expr, shr(_1, xor(var_low, var_high)))
if gt(expr, sum) { panic_error_0x11() }
let var_mid := sum
mstore( 0, var_self_3885_slot)
let _2 := 0xffffffff
let cleaned := and(sload( add(keccak256( 0, 0x20), sum)), _2)
switch gt( cleaned, and( var_key, _2))
case 0 {
let sum_1 := add(sum, _1)
if gt(sum, sum_1) { panic_error_0x11() }
var_low := sum_1
}
default
{
var_high := var_mid
}
}
var := var_high
} vs function fun_upperBinaryLookup(var_self_slot, var_key, var_low, var_high) -> var
{
for { }
lt(var_low, var_high)
{ }
{
let expr := and(var_low, var_high)
let _1 := 1
let sum := add(expr, shr(_1, xor(var_low, var_high)))
if gt(expr, sum) { panic_error_0x11() }
mstore( 0, var_self_slot)
let _2 := 0xffffffff
let cleaned := and(sload( add(keccak256( 0, 0x20), sum)), _2)
switch gt( cleaned, and( var_key, _2))
case 0 {
let sum_1 := add(sum, _1)
if gt(sum, sum_1) { panic_error_0x11() }
var_low := sum_1
}
default
{
var_high := sum
}
}
var := var_high
}
Eliminating the variable manually yields the same bytecode again (so it's not the naming in yul identifiers or immutables). |
I played with this today with the intention of preparing a bytecode comparison run for #14495. Unfortunately a simple addition of a single file with pragmas or even a simple contract to compilation does not seem to break things - bytecode check passes just fine. Something more complicated is needed to trigger this. So I tried to reduce this issue to a minimal example to figure out the necessary conditions and here's what I was left with: mkdir @
cat <<=== > a.sol
import "@/za.sol";
import "b.sol";
contract A is B {
function a() public pure {
zb();
}
}
===
cat <<=== > b.sol
import "@/zb.sol";
import "c.sol";
abstract contract B is C {}
===
cat <<=== > c.sol
abstract contract C
{
function f() public pure {}
function c() public view returns (uint) {
try this.f() { return 0; }
catch {}
}
}
===
cat <<=== > @/za.sol
import "@/zc.sol";
===
cat <<=== > @/zb.sol
import "@/zc.sol";
function zb() pure {
zc();
}
===
cat <<=== > @/zc.sol
function zc() pure returns (bytes memory returndata) {
if (returndata.length == 0) {
return "";
}
}
===
diff --color --unified \
<(solc a.sol b.sol --bin --via-ir --optimize | grep '======= a.sol:A =======' --after-context=2 | fold --width 100) \
<(solc a.sol --bin --via-ir --optimize | grep '======= a.sol:A =======' --after-context=2 | fold --width 100) I could not go below 6 files or get rid of a subdirectory, though I suspect this is needed only due to the way it affects the import/compilation order and examples without it should be possible as well. The import pattern goes like this:
But the pattern itself is not enough. The Not sure if I should continue with the bytecode check in that case. Seems like it will be hard to come up with something that does not cause issues by itself, but does when combined with some random code examples and is not simply this specific case. |
Sadly, I think what you found there is a completely unrelated issue. In your case I see a different order of Yul functions in optimized IR code, whereas in the orginally posted example, I see a single variable not being eliminated from one Yul function. The underlying cause may still be the same, but the symptoms are different enough that I'd actually expect two causes. |
But yeah, since it then looks like it's non-trivial to guard against these cases by traditional testing, I wonder if fuzzing can help (pinging @bshastry). To summarize the overreaching problem here for @bshastry: We want to guarantee that the bytecode the compiler produces for a contract The target here would not be to find security vulnerabilities, but compiler misbehaviour that's causing issues for tooling. |
Idea from the call: create a |
I just had another quick look at #14494 (comment) - that indeed has a differing order of functions in unoptimized IR already, so it's indeed a different issue that already occurs in via-IR code generation and not only during optimization (the original case of this issue is confirmed in the optimizer, since it vanishes if we ignore the name hints in the name dispenser of the yul optimizer) |
@ekpyron Just so I understand correctly Source A
Source B
where the two source files are generated at random. And then, we compile using optimized via-ir setting Compilation 1: Source A: contract A and the invariant that should hold is bytecodes of the following pairs are identical?
tl;dr In each fuzzing iteration, the fuzzer generates two random source units and performs a total of 4 compilations to check if individual and combined compilation produce the same bytecode. |
I'm also wondering if wrt #14494 (comment), it suffices to compare (1) and (3), thereby limiting it to two compilations per fuzzer iteration since source A and B are generated uniformly at random. Also, in the original issue above, there seem to be more than two source files. #14494 (comment) says at least 6 source files were required to trigger issue, so I'm wondering if two source fuzzing setup would suffice. Maybe, generate N source files (N > 2), pick one contract from one source at random and check if individual and grouped (all N source files) report identical bytecode? |
Yes, changing (1) and (3), respectively in general picking one specific contract, should suffice. The case of #14494 (comment) was a bit different and we already solved it in #14562 - that case relied on the files to actually import each other, which would make a fuzzing setup more complicated, but it's also less likely that there's more hidden issues like this. So I'd focus on cases like originally reported here, which are in the Yul optimizer and won't involve importing, but just additional source files - but additional source files are also just the means for changing AST IDs (our guarantee is that changing AST IDs should not affect compilation result). So in the meantime @cameel had the idea of #14548, which may work for tracing issues like this down more directly - for fuzzing this would just mean:
We wanted to build that as regular CI run on our test suite first to see if that already catches the issue we have on |
I debugged this yesterday and found the problem - the direct cause of the disappearance of the Here's the analysis I posted on the channel:
Today I prepared a proper fix for it (#14574) and verified that it really works. I also created a smaller repro to include as a regression test. At first I tried from Solidity level but it was too hard because the bug requires naming differences to be triggered and these differences are caused by the AST ID differences. It would probably have to be based on that multi-file repro I posted above. In the and I gave up and instead trimmed down the unoptimized Yul output. That reproduces the problem when passed through Combined with @r0qs's fix, this now compiles the Standard JSON input from the description without bytecode differences |
Here I am again with another issue with extra files in compilation causing trouble 🫡
By extra files I mean files that the target contract does not depend on.
Recap
First a recap on previous issues similarly on extra files in compilation:
This was an older issue that seems to affect the AST IDs in
0.6.12
and0.7.0
. We also encountered this and handled in Sourcify's case Handle Solidityv0.6.12
andv0.7.0
extra files bytecode mismatch sourcify#618I've encountered this in a
0.8.19
contract and got fixed in0.8.21
with Deterministically choose memory slots for variables during stack-to-memory. #14311.Current Issue
I've encountered the current issue in ethereum/sourcify#1065.
My initial suspicion was that it's an instance of the 2. case because the contract was a
0.8.19
contract. Unfortunately, I was able to reproduce the case in0.8.21
.Although the 1. case should have been resolved, I suspect it still can be related because the metadata hashes are identical but the bytecodes differ.
To reproduce
I named inputs as "hardhat" and "sourcify". "Sourcify" uses the minimum number of source files, because it is based on the ones listed in the contract metadata. "Hardhat" uses extra because by default Hardhat inputs everything to the compiler. Sourcify has two different inputs
0.8.19
and0.8.21
because it includes theevmVersion
inside the input i.e.paris
vsshanghai
.N2MERC721-hardhat-input.json.txt
N2MERC721-sourcify-input-0.8.21.json.txt
N2MERC721-sourcify-input-0.8.19.json.txt
You can see the two inputs differ only in some extra files + some of the compiler settings
These are the extra files in Hardhat input
Also leaving the JS script I used to check the differences of the keys of `.sources` in the JSON inputs
Usage:
node checkFileKeysDifferences.js solcInput-1.json solcInput-2.json
To extract the runtime bytecode:
My Bytecode outputs
N2MERC721-0.8.21-sourcify-runtime-bytecode.txt
N2MERC721-0.8.21-hardhat-runtime-bytecode.txt
N2MERC721-0.8.19-sourcify-runtime-bytecode.txt
N2MERC721-0.8.19-hardhat-runtime-bytecode.txt
To check the diff:
Will give you a diff like this:
Environment
0.8.19
and0.8.21
(both Emscripten and Darwin.appleclang)paris
andshanghai
Again, one particular thing that got my attention was that the metadata hashes were identical in all cases. This leaves me thinking if this is similar to the 1. case I mentioned.
The text was updated successfully, but these errors were encountered: