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

bug(config): do not enable solc optimizer by default #2486

Open
Tracked by #8574
hbarcelos opened this issue Jul 26, 2022 · 7 comments
Open
Tracked by #8574

bug(config): do not enable solc optimizer by default #2486

hbarcelos opened this issue Jul 26, 2022 · 7 comments
Labels
A-compatibility Area: compatibility C-forge Command: forge Cmd-forge-build Command: forge build T-bug Type: bug T-likely-breaking Type: requires changes that can be breaking T-to-discuss Type: requires discussion
Milestone

Comments

@hbarcelos
Copy link

hbarcelos commented Jul 26, 2022

Component

Forge

Describe the feature you would like

The solc optimizer has historically been a source of bugs in smart contracts.

After a couple of years of stability, Solidity 0.8.x's optimizer had some outstanding bugs that could lead to catastrophic problems even when code is correct.

While one can appreciate the gains in gas costs that the optimizer brings, this should be an opt-in instead of the default option, since it can introduce bugs for seemingly correct code. Let's not underestimate the power of defaults.

Additional context

Other tools used in the industry also don't enable the optimizer by default:

  • Hardhat

    optimizer: an object with enabled and runs keys. Default value: { enabled: false, runs: 200 }.

  • Truffle

    // optimizer: {
    // enabled: false,
    // runs: 200
    // },

  • Dapp Tools

    DAPP_BUILD_OPTIMIZE | 0 | -- | -- | --

So anyone coming from them who are not willing to use the optimizer can easily forget to disable it.

@hbarcelos hbarcelos added the T-feature Type: feature label Jul 26, 2022
@gakonst gakonst added this to Foundry Jul 26, 2022
@gakonst gakonst moved this to Todo in Foundry Jul 26, 2022
@onbjerg onbjerg added C-forge Command: forge Cmd-forge-build Command: forge build labels Jul 30, 2022
@sambacha
Copy link
Contributor

Even defining no optimizations has lead to incorrect settings, leaving it undefined was not the same as explicitly disabling the optimizer

@hbarcelos
Copy link
Author

Even defining no optimizations has lead to incorrect settings, leaving it undefined was not the same as explicitly disabling the optimizer

I'm not sure I understood what is your point. Could you please clarify?

@onbjerg
Copy link
Member

onbjerg commented Aug 11, 2022

cc @gakonst this was changed when we introduced config later in that PR, @mattsse thinks this was discussed somewhere but can't find the original discussion, do you remember what the rationale was to enable it by default?

@gakonst
Copy link
Member

gakonst commented Aug 11, 2022

That was the dapptools default I think. I'm open minded to either, but I personally like the optimizer for most use-cases that do not have to do with low level asm. Could be wrong.

@hbarcelos
Copy link
Author

That was the dapptools default I think.

Maybe in earlier versions of dapptools, however the latest docs says otherwise (look for "DAPP_BUILD_OPTIMIZE").

As pointed out in the original comment, most of the widely used tools in the ecosystem DO NOT enable optimizations by default.

@Evalir Evalir added this to the v1.0.0 milestone Jul 13, 2023
@zerosnacks zerosnacks added the A-compatibility Area: compatibility label Jun 26, 2024
@zerosnacks zerosnacks added T-bug Type: bug and removed T-feature Type: feature labels Jul 31, 2024
@zerosnacks zerosnacks changed the title Do not enable solc optimizer by default bug(config): do not enable solc optimizer by default Jul 31, 2024
@zerosnacks
Copy link
Member

Marking this as a bug because it behavior can be perceived as unexpected when coming from other tools. I think it makes sense to make this breaking change pre-1.0. I've had previously assumed it wasn't enabled by default.

@zerosnacks zerosnacks added the T-to-discuss Type: requires discussion label Jul 31, 2024
@grandizzy
Copy link
Collaborator

this would also avoid confusion with coverage not being able to compile while test can without any optimization as in #8840 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-compatibility Area: compatibility C-forge Command: forge Cmd-forge-build Command: forge build T-bug Type: bug T-likely-breaking Type: requires changes that can be breaking T-to-discuss Type: requires discussion
Projects
Archived in project
Development

No branches or pull requests

7 participants