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

Two simplifications that will be needed to deal with @zoep's issues regarding initcode equivalence #655

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

msooseth
Copy link
Collaborator

@msooseth msooseth commented Feb 12, 2025

Description

One more simplification of readByte. This one works ALSO when CopySlice is not called with a Lit as a srcOffset. So it's more general, but cannot simplify some of the things that the more specific one can. It definitely helps in some cases, as per Zoe's issue

Note that the SMT overflow vs how the EVM actually works is slightly different on CopySlice. We need this rewrite rule, but it means that in some cases, the SMT will differ from the simplification system, when writing to extremely large offsets -- which always leads to over-gas-limit, so it's not a relevant case. However, the rewrite rule actually triggers, in real-world scenarios, and is useful.

NOTE: this is a cherry-pick from Zoe's changeset so it will apply perfectly when this is merged, appearing as less of a changeset for Zoe's PR.

Checklist

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

@msooseth msooseth marked this pull request as ready for review February 12, 2025 10:39
@msooseth msooseth requested a review from zoep February 12, 2025 10:39
@msooseth msooseth changed the title One more simplification that will be needed to deal with @zoe's issues regarding initcode equivalence One more simplification that will be needed to deal with @zoep's issues regarding initcode equivalence Feb 12, 2025
@msooseth msooseth requested a review from blishko February 12, 2025 11:22
Copy link
Collaborator

@blishko blishko left a comment

Choose a reason for hiding this comment

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

The simplification is sound, but only if writing to a buffer does not overflow.
In that case we have a conflict between the SMT semantics and EVM semantics.
I guess we have agreed to ignore this weird edge case, right?

test/test.hs Outdated Show resolved Hide resolved
test/test.hs Outdated Show resolved Hide resolved
test/test.hs Show resolved Hide resolved
test/test.hs Outdated Show resolved Hide resolved
test/test.hs Outdated Show resolved Hide resolved
test/test.hs Outdated Show resolved Hide resolved
@blishko
Copy link
Collaborator

blishko commented Feb 12, 2025

Also, there seem to be some test failures on windows:

FAIL
      Exception: Maybe.fromJust: Nothing
      CallStack (from HasCallStack):
        error, called at libraries\base\Data\Maybe.hs:150:21 in base:Data.Maybe
        fromJust, called at test\EVM\Test\Tracing.hs:880:30 in hevm-0.54.2-inplace-test:EVM.Test.Tracing
      Use -p '/calldata-wraparound-1/' to rerun this test only.
    calldata-wraparound-2:                                        Error converting trace! exit code:ExitFailure 127

@msooseth msooseth force-pushed the more-simpl-as-needed branch from b035e6e to bcab59e Compare February 12, 2025 15:17
@msooseth
Copy link
Collaborator Author

msooseth commented Feb 12, 2025

The windows build issue is unrelated, unfortunately. It's occurring here too:

https://github.com/ethereum/hevm/actions/runs/13283178489/job/37085893025

which is also unrelated. I'll merge this even if Windows is broken, and I have already asked the Echidna folks to look into the Windows issue. I think it's something small, or at least I'm hoping :) Probably some shell script thing.

But I do wanna make sure the other things are OK :)

@msooseth
Copy link
Collaborator Author

The simplification is sound, but only if writing to a buffer does not overflow. In that case we have a conflict between the SMT semantics and EVM semantics. I guess we have agreed to ignore this weird edge case, right?

Yep, we did :)

@msooseth
Copy link
Collaborator Author

@blishko beware -- I added the same simplification to readWord as well, which I now noticed is missing.

Copy link
Collaborator

@blishko blishko left a comment

Choose a reason for hiding this comment

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

Maybe you should also add a test for the ReadWord simplification?

test/test.hs Outdated Show resolved Hide resolved
test/test.hs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@msooseth msooseth changed the title One more simplification that will be needed to deal with @zoep's issues regarding initcode equivalence Two simplifications that will be needed to deal with @zoep's issues regarding initcode equivalence Feb 14, 2025
@msooseth
Copy link
Collaborator Author

Maybe you should also add a test for the ReadWord simplification?

Yes.. sorry. I just added two :)

Copy link
Collaborator

@blishko blishko left a comment

Choose a reason for hiding this comment

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

LGTM!

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.

2 participants