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

Keccak improvements #406

Merged
merged 18 commits into from
Dec 1, 2023
Merged

Keccak improvements #406

merged 18 commits into from
Dec 1, 2023

Conversation

msooseth
Copy link
Collaborator

@msooseth msooseth commented Oct 10, 2023

Description

This change pushes the Keccak concretization to the very END of the steps we do. We don't concretize keccak (i.e. execute SHA3) during interpretation unless necessary (e.g. to do an RPC call). This allows us to take advantage of the fancy pattern matching in readStorage more often, hence making it more effective. In particular, this for example passes the new transfer-dapp test instantly that previously Z3 struggled with.

This improvement required quite a bit of change in the way we handle Keccak. The most hairy parts are:

  • We now have a symbolic value in our VMOpts to know if we are doing concrete or symbolic execution. Concrete exe
  • Keccak.hs now requires a bit more care, as we need to do some concretization as well
  • we now have concKeccakSimp True/False to BOTH concretize and simplify (or not) the expression. This is basically a beefed-up simplify. This is sometimes needed, rather than pure simplify.
  • in EVM.hs the accessStorage function had to be changed slightly so we can check whether we get a Nothing either with or without concretization. If we get a Nothing with either of the two, we do an rpc call. This is necessary, or we would never look up the rpc.

I also added a more thorough Add+Sub simplification. This is more complete than what we had. It was needed for one of the debugging sessions I was doing and so while it's unrelated, I think it should be OK(?) Let me know if you need it separately :)

Checklist

  • tested locally
  • added automated tests
  • updated the docs
  • updated the changelog

@msooseth msooseth force-pushed the storage-rewrite-clean-merged branch 7 times, most recently from ded2a21 to bf76fa8 Compare October 16, 2023 13:14
@msooseth msooseth changed the title [DRAFT] Keccak improvements *on top of config change* [DRAFT] Keccak improvements -- on top of monadic config change Oct 24, 2023
@msooseth msooseth force-pushed the storage-rewrite-clean-merged branch 10 times, most recently from ba2277f to e989bd7 Compare October 27, 2023 09:25
@msooseth msooseth force-pushed the storage-rewrite-clean-merged branch from e989bd7 to 098a93a Compare November 2, 2023 16:46
@msooseth msooseth changed the title [DRAFT] Keccak improvements -- on top of monadic config change Keccak improvements -- on top of monadic config change Nov 2, 2023
@msooseth msooseth requested a review from zoep November 2, 2023 18:07
@msooseth msooseth changed the title Keccak improvements -- on top of monadic config change Keccak improvements Nov 2, 2023
CHANGELOG.md Outdated Show resolved Hide resolved
msooseth and others added 2 commits November 3, 2023 13:09
Co-authored-by: Zoe Paraskevopoulou <[email protected]>
test/EVM/Test/Tracing.hs Outdated Show resolved Hide resolved
src/EVM/SMT.hs Show resolved Hide resolved
src/EVM/SMT.hs Show resolved Hide resolved
@msooseth msooseth requested a review from d-xo November 21, 2023 15:13
src/EVM.hs Outdated Show resolved Hide resolved
@msooseth
Copy link
Collaborator Author

OK, I think I managed to improve this a bit :)

@msooseth
Copy link
Collaborator Author

NOTE: I now know why we can't concretize earlier -- we could miss information that Keccak assumptions need. We COULD concretize earlier, but then the Keccak info would need to be extracted before concretization and passed to assertProps. Maybe it's worth thinking about. Right now it's "fragile" -- if we concretize earlier than assertProps, it can give spurious counterexamples.

src/EVM/Expr.hs Show resolved Hide resolved
Copy link
Collaborator

@d-xo d-xo left a comment

Choose a reason for hiding this comment

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

just one more small nit, but apart from that lgtm :)

@msooseth msooseth merged commit ff9ac5d into main Dec 1, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants