Bytecode difference caused by SSA transform

[nikola-matic]

Fixes #14829

[0xalpharush]

Are there any tests for non-determinism like running in a loop and checking that each compilation of the same source unit results in the same bytecode?

[nikola-matic]

Are there any tests for non-determinism like running in a loop and checking that each compilation of the same source unit results in the same bytecode?

What you're describing is fully deterministic already - i.e. the same sources will always compile to the same bytecode - the issue here is of a different sort; we use a ton of associative containers (e.g. a std::set which typically uses a red black tree underneath) to track values (e.g. generated IR variable names) during the optimization stages, and these can sometimes (extremely rarely) change if you insert an unrelated contract that is unused in the rest of the sources (in this case it's an empty dummy contract) which then causes the order of the elements in such containers to be different than without the said unrelated contract/sources. This is what creates the inconsistency in bytecode, and is what this PR fixes.

These inconsistencies don't actually change the behavior of contracts - the code will semantically remain the same, but the bytecode in such cases should none the less be identical, especially when it comes to source verification.

[cameel]

Given how easy it is to inadvertently run into this problem when using YulString with associative containers, I wonder if we can do something to prevent them in the future. Maybe we could e.g. create wrappers over set<YulString> and map<YulString> that remove any support for iterating them and add a "style" rule preventing the use of unwrapped containers?

[r0qs]

Just as a note, so we can remember to eventually refactor this. We could pass a dictionary here like what was done in https://github.com/ethereum/solidity/blob/bbb7f58be026fdc51b0b4694a6f25c22a1425586/scripts/externalTests/test_helpers.py#L52 or event have a class for compiler settings so we could reuse in other scripts. I believe this will be desirable when we migrate the external tests to python.

[nikola-matic]

@cameel this should be ready now.

[cameel]

Looks good in terms of correctness, just needs two style tweaks.

[ekpyron]

I was just looking at this to consider using it for #15262 and noticed: this swap function won't work, will it? contents is a const reference, you can't swap these things...
what the std::swap below will do is to use the implicit move constructors of UniqueVector...

[nikola-matic]

Yeah, you're right, contents() should ideally just return a reference...

[nikola-matic]

I would have honestly expected a compilation error here, this is really insidious.

[ekpyron]

I mean, it's currently unused - and C++ only type-checks on instantiation :-). And I mean, it's not unused without reason: we don't need it, so we might as well remove the swap - std::swap will just work based on moving regardless