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

Why force turning off solc optimizer in 0.7.0 beta? #417

Closed
boolafish opened this issue Oct 2, 2019 · 18 comments
Closed

Why force turning off solc optimizer in 0.7.0 beta? #417

boolafish opened this issue Oct 2, 2019 · 18 comments
Labels

Comments

@boolafish
Copy link

boolafish commented Oct 2, 2019

Hi, recently somehow I get a code that can only compile with optimizer on 😓😓😓

But anyway, just realized that during the coverage, it actually force flagging the using optimizer flag to false: https://github.com/sc-forks/solidity-coverage/blob/beta/dist/truffle.plugin.js#L92

Is there a reason to do so? Also, shall we also just keep in mind that the coverage would always be running without optimizer on?

boolafish added a commit to omgnetwork/plasma-contracts that referenced this issue Oct 2, 2019
Somehow if we adding 'require' inside 'buildParams' in BondSize and use that in our Routers during their constructor, the compile would
fail with the exception of "Assembly exception for bytecode". However, this would be fine if the optimizer is turned on.

After a few tries, by making it being called in a non-constructor solves the compile issue. Thus, this commit changes the constructor for Routers
to become a 'init' function instead.

This was found because solidity coverage would force turning off the optimizer, see: sc-forks/solidity-coverage#417
boolafish added a commit to omgnetwork/plasma-contracts that referenced this issue Oct 2, 2019
Somehow if we adding 'require' inside 'buildParams' in BondSize and use that in our Routers during their constructor, the compile would
fail with the exception of "Assembly exception for bytecode". However, this would be fine if the optimizer is turned on.

After a few tries, by making it being called in a non-constructor solves the compile issue. Thus, this commit changes the constructor for Routers
to become a 'init' function instead.

This was found because solidity coverage would force turning off the optimizer, see: sc-forks/solidity-coverage#417
@cgewecke
Copy link
Member

cgewecke commented Oct 2, 2019

@boolafish Yes, the optimizer is turned off because the tool instruments Solidity by injecting small no-op statements which optimization might remove.

Can you link to the code which only compiles w/ optimization on?
Why does it not work when optimization is off?

@cgewecke
Copy link
Member

cgewecke commented Oct 2, 2019

@boolafish
Copy link
Author

boolafish commented Oct 2, 2019

Yeah, the above link is the fix on my side.
If you're interested, I put a problem-ish branch on my fork:

branch: fail_with_optimizer_off
latest commit with some observation of mine: here

@cgewecke
Copy link
Member

cgewecke commented Oct 2, 2019

@boolafish Ah that's really interesting thank you.

I think the coverage tool should probably try to be clearer about what the compilation settings are in the docs and terminal output. There are probably more cases like this....

boolafish added a commit to omgnetwork/plasma-contracts that referenced this issue Oct 2, 2019
Somehow if we adding 'require' inside 'buildParams' in BondSize and use that in our Routers during their constructor, the compile would
fail with the exception of "Assembly exception for bytecode". However, this would be fine if the optimizer is turned on.

After a few tries, by making it being called in a non-constructor solves the compile issue. Thus, this commit changes the constructor for Routers
to become a 'init' function instead.

This was found because solidity coverage would force turning off the optimizer, see: sc-forks/solidity-coverage#417
boolafish added a commit to omgnetwork/plasma-contracts that referenced this issue Oct 2, 2019
Somehow if we adding 'require' inside 'buildParams' in BondSize and use that in our Routers during their constructor, the compile would
fail with the exception of "Assembly exception for bytecode". However, this would be fine if the optimizer is turned on.

After a few tries, by making it being called in a non-constructor solves the compile issue. Thus, this commit changes the constructor for Routers
to become a 'init' function instead.

This was found because solidity coverage would force turning off the optimizer, see: sc-forks/solidity-coverage#417
@superduck35
Copy link

fwiw, I also had this issue and was lucky to search through existing issues on here for an answer. It's a hard one to communicate to the user, but yeah I think just being clearer about what the compilation settings are is a step in the right direction. Not sure why my method execution fails when optimisation is disabled though :)

@cgewecke
Copy link
Member

cgewecke commented Oct 2, 2019

@alsoc77 Out of curiosity - what is your compilation error message? Is the code where youre seeing this public?

@cgewecke
Copy link
Member

cgewecke commented Oct 2, 2019

For context @boolafish's error is

"Assembly exception for bytecode"

which looks like a internal solidity crash... I wonder if these incidents should be collected and reported to solc.

@superduck35
Copy link

My scenario is a bit tricky to disect. I did end up figuring out a solution though.
I have a ContractFactory that has a createContract func in which it deploys a new , and there was an issue in the bytecode of the constructor of the child that was causing a revert. One of the libraries that was linked to this child was using an external contract that hadn't been deployed, rather than referencing an interface of this contract.
So the solution was to create a local interface for this external contract.
I suspect the optimizer was figuring out what part of the external contract it needed, then just slicing that bit off and putting it in, whereas an unoptimized compilation was attempting to do something else.
Anyway, thanks for this. Love the plugin -- really appreciate the Istanbul reporting, thanks @cgewecke

@Ferparishuertas
Copy link

Ferparishuertas commented Nov 19, 2020

CompileError: CompilerError: Stack too deep when compiling inline assembly: Variable headStart is 1 slot(s) too deep inside the stack.

With optimizer it works.

It's a bunch of smart contracts with AbiEncoderv2 using structs as params and return values

Any idea how to make coverage work?

Solc 7.1

@cgewecke
Copy link
Member

@Ferparishuertas

No, sorry. Thanks for reporting this.

I think these cases should be raised at ethereum/solidity if it's possible to generate minimized reproductions for them. It seems strange that optimization would affect whether or not compilation succeeds.

@Ferparishuertas
Copy link

Will definetly do it. Any contract over 24k, with Abiv2encoder raises this problem with optimizer=false

@AndonMitev
Copy link

So guys do you came up with any solution about the
CompilerError: Stack too deep when compiling inline assembly: Variable value0 is 1 slot(s) too deep inside the stack. Error.
Yes I'm using optimizer ON on hardhat config settings, but not able to run my coverage, is there anyway to turn it on in the plugin as well?

@cgewecke
Copy link
Member

cgewecke commented Apr 12, 2021

@AndonMitev No, haven't found a solution. This is an open issue about this at solidity 10354. If I understand the maintainer's comments there correctly, this is a known limitation of the solidity compiler and AbiEncoderV2.

Out of curiosity, which compiler version are you using?

One resource you might look at it is the argentlabs Filter for Paraswap V4 cross link just above in this thread. There's an explanation of how they restructured their contracts to get around this problem in that PR.

@AndonMitev
Copy link

@AndonMitev No, haven't found a solution. This is an open issue about this at solidity 10354. If I understand the maintainer's comments there correctly, this is a known limitation of the solidity compiler and AbiEncoderV2.

Out of curiosity, which compiler version are you using?

One resource you might look at it is the argentlabs Filter for Paraswap V4 cross link just above in this thread. There's an explanation of how they restructured their contracts to get around this problem in that PR.

version: '0.7.3',

@abdulsamijay
Copy link

Hey all,
Have we found a solution or a work-around to this ? I cant get coverage to run as it does not compile with optimizer on. I am facing the same issue as @AndonMitev
CompilerError: Stack too deep when compiling inline assembly: Variable value0 is 1 slot(s) too deep inside the stack. Error.

@JasoonS
Copy link
Contributor

JasoonS commented Jul 3, 2021

BTW, I got it fixed and working here: https://npmjs.com/package/@float-capital/solidity-coverage

Hopefully will be up-streamed soon #642 👍 You can test it in the interim.

Simply install the package and change the following in your hardhat.config.js:

-require("solidity-coverage");
+require("@float-capital/solidity-coverage");

@TheBlockchainAuditor
Copy link

BTW, I got it fixed and working here: https://npmjs.com/package/@float-capital/solidity-coverage

Hopefully will be up-streamed soon #642 +1 You can test it in the interim.

Simply install the package and change the following in your hardhat.config.js:

-require("solidity-coverage");
+require("@float-capital/solidity-coverage");

This worked for me but I had to set my hardfork to berlin!

Thanks JasoonS!!

@cgewecke
Copy link
Member

Closing this for housekeeping. v0.8.7 now supports viaIR natively. While the optimizer is usually. turned off, if viaIR is enabled it allows optimization.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

8 participants