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

Bytecode difference caused by SSA transform #14968

Merged
merged 1 commit into from Apr 17, 2024

Conversation

nikola-matic
Copy link
Collaborator

Fixes #14829

@nikola-matic nikola-matic added this to the 0.8.26 milestone Mar 28, 2024
@nikola-matic nikola-matic marked this pull request as ready for review March 28, 2024 15:45
@0xalpharush
Copy link
Contributor

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

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.

@nikola-matic nikola-matic force-pushed the bytecode-different-with-dummy-file branch from 11b27e3 to b423c2a Compare April 2, 2024 14:15
@cameel
Copy link
Member

cameel commented Apr 2, 2024

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?

libsolutil/CommonData.h Outdated Show resolved Hide resolved
libyul/optimiser/SSATransform.cpp Outdated Show resolved Hide resolved
libyul/optimiser/SSATransform.cpp Outdated Show resolved Hide resolved
@nikola-matic nikola-matic force-pushed the bytecode-different-with-dummy-file branch from b423c2a to a031490 Compare April 3, 2024 12:59
libyul/optimiser/SSATransform.cpp Outdated Show resolved Hide resolved
libyul/optimiser/SSATransform.cpp Show resolved Hide resolved
libyul/optimiser/SSATransform.cpp Outdated Show resolved Hide resolved
libsolutil/CommonData.h Outdated Show resolved Hide resolved
test/libyul/yulOptimizerTests/ssaTransform/function.yul Outdated Show resolved Hide resolved
@nikola-matic nikola-matic force-pushed the bytecode-different-with-dummy-file branch from a031490 to 972c464 Compare April 5, 2024 09:05
libsolutil/CommonData.h Outdated Show resolved Hide resolved
Changelog.md Outdated Show resolved Hide resolved
@nikola-matic nikola-matic force-pushed the bytecode-different-with-dummy-file branch from 5148ff3 to 7b86576 Compare April 15, 2024 13:44
libsolutil/CommonData.h Outdated Show resolved Hide resolved
@nikola-matic nikola-matic force-pushed the bytecode-different-with-dummy-file branch 4 times, most recently from 843b813 to 6822901 Compare April 16, 2024 09:15
Comment on lines 42 to 44
via_ir: bool,
optimize: bool = False,
yul_optimizations: Optional[str] = None) -> FileReport:
Copy link
Member

@r0qs r0qs Apr 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just as a note, so we can remember to eventually refactor this. We could pass a dictionary here like what was done in

def compiler_settings(evm_version: str, via_ir: bool = False, optimizer: bool = False, yul: bool = False) -> dict:
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 nikola-matic force-pushed the bytecode-different-with-dummy-file branch 4 times, most recently from b4a8906 to df5229e Compare April 16, 2024 20:05
@nikola-matic
Copy link
Collaborator Author

@cameel this should be ready now.

cameel
cameel previously approved these changes Apr 17, 2024
Copy link
Member

@cameel cameel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

scripts/common/cmdline_helpers.py Outdated Show resolved Hide resolved
Changelog

Review comments

Unique vector

Repro test
@nikola-matic nikola-matic merged commit 39af449 into develop Apr 17, 2024
73 checks passed
@nikola-matic nikola-matic deleted the bytecode-different-with-dummy-file branch April 17, 2024 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants