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

hevm/dapp: solc 0.8.9/0.8.8 support #827

Merged
merged 17 commits into from
Oct 8, 2021
Merged

hevm/dapp: solc 0.8.9/0.8.8 support #827

merged 17 commits into from
Oct 8, 2021

Conversation

transmissions11
Copy link
Contributor

@transmissions11 transmissions11 commented Oct 4, 2021

Description

Checklist

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

@transmissions11
Copy link
Contributor Author

Oops forgot my branch had an old WIP command I was working on. Removed it— should hopefully fix CI

@transmissions11
Copy link
Contributor Author

hmm looks like one of the tests that uses Yul is failing to compile... got any ideas @d-xo?

src/hevm/README.md Outdated Show resolved Hide resolved
@d-xo
Copy link
Contributor

d-xo commented Oct 5, 2021

It's failing in the part of the test suite where we compare the SMTChecker in solc with the symbolic execution engine in hevm. It looks like it's failing because of a solc bug, we can just skip this test for now (add "circularReferencesPruner/nested_same_name.yul" to the list of ignored tests here).

> solc-0.8.9 --strict-assembly hi.yul
Warning: Yul is still experimental. Please use the output with care.

======= hi.yul (EVM) =======

Pretty printed source:
object "object" {
    code {
        calldatacopy(0, 0, 1024)
        {
            function z() -> x
            { x := y() }
            function y() -> x
            { x := z() }
        }
        {
            function z() -> x
            { x := y() }
            function y() -> x
            { x := z() }
        }
    }
}

Exception while assembling: /solidity/libevmasm/Assembly.cpp(720): Throw in function const solidity::evmasm::LinkerObject& solidity::evmasm::Assembly::assemble() const
Dynamic exception type: boost::wrapexcept<solidity::evmasm::AssemblyException>
std::exception::what: Duplicate tag position.
[solidity::util::tag_comment*] = Duplicate tag position.

@d-xo
Copy link
Contributor

d-xo commented Oct 5, 2021

solc bug tracked at: ethereum/solidity#12090

@transmissions11
Copy link
Contributor Author

alright added to the ignored file

@transmissions11
Copy link
Contributor Author

wat CI still failing

@d-xo
Copy link
Contributor

d-xo commented Oct 7, 2021

hmmmm, it's a different error. I think perhaps caused by this. We do a bunch of symlinking / nix store magic when building our test projects in ci (using build-dapp-package.nix), and I think the changes to --allow-paths introduced in solc-0.8.8 mean that these symlinks are now resolved to paths outside of the project directory.

Fixing this probably requires redoing how we run our dapp integration tests in ci.

cc @cameel, am I understanding the changes from the issue linked above correctly?

@transmissions11
Copy link
Contributor Author

Oof ok I might just close this and open a new PR that only adds 0.8.8/9 to nix and doesn't set it as the default

@d-xo
Copy link
Contributor

d-xo commented Oct 7, 2021

yeah lets not let this ci annoyance block getting 0.8.9 into dapptools...

@transmissions11
Copy link
Contributor Author

transmissions11 commented Oct 8, 2021

Alright removed the new default stuff and just added support for 0.8.9/0.8.8— lgtm?

@transmissions11 transmissions11 changed the title hevm/dapp: solc 0.8.9 default hevm/dapp: solc 0.8.9/0.8.8 support Oct 8, 2021
@d-xo d-xo merged commit 009f850 into dapphub:master Oct 8, 2021
@d-xo
Copy link
Contributor

d-xo commented Oct 8, 2021

Thanks!

@cameel
Copy link

cameel commented Oct 8, 2021

@d-xo

hmmmm, it's a different error. I think perhaps caused by this. We do a bunch of symlinking / nix store magic when building our test projects in ci (using build-dapp-package.nix), and I think the changes to --allow-paths introduced in solc-0.8.8 mean that these symlinks are now resolved to paths outside of the project directory.

Yeah, if you have symlinks, you need to whitelist the dir containing the actual file. You probably never noticed it because solc automatically whitelists dirs containing the files specified on the CLI and it had a bug where a file name without a directory reduced to an empty path and this whitelisted the whole filesystem. And there were also a few other bugs that whitelisted stuff that was not supposed to be whitelisted - ethereum/solidity#11688 (comment) has the full list. The whole whitelisting was actually a huge mess with many weird corner cases that were untested and mostly broken due to lack of path normalization. I have rewritten it and covered it with tests so it should hopefully no longer have such issues.

That's probably why it worked for you so far so in the worst case you can just add --allow-paths / and get the equivalent of the old behavior. If you at least broadly know which dirs can contain contracts, you can whitelist them. And if you don't and you accept that links can lead anywhere in the filesystem then you basically assume --allow-paths / anyway.

Fixing this probably requires redoing how we run our dapp integration tests in ci.

On the topic of integration tests, ethereum/solidity#10854 / ethereum/solidity#9237 has been sitting on my todo list for a while and want to finally deal with it in the near future. If you could give us something that we could run in one of the CircleCI docker images to execute your tests on development builds of the compiler, we could detect breakage like this before we release new versions and coordinate.

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.

4 participants