search

foundry-rs / foundry

Foundry is a blazing fast, portable and modular toolkit for Ethereum application development written in Rust.
https://getfoundry.sh
Apache License 2.0
8.35k stars 1.77k forks source link

Bug: Foundry does not respect optimizerSteps setting for Yul #5449

Closed sambacha closed 9 months ago

sambacha commented 1 year ago

Component

Forge

Have you ensured that all of these are up to date?

What version of Foundry are you on?

No response

What command(s) is the bug in?

No response

Operating System

macOS (Apple Silicon)

Describe the bug

Foundry.toml configuration 

[profile.default.optimizer_details]
# constantOptimizer = true
yul = true
# this sets the `yulDetails` of the `optimizer_details` for the `default` profile
[profile.default.optimizer_details.yulDetails]
stackAllocation = false
optimizerSteps = 'u'

Build output yields 

optimizerSteps: u:fDnTOc
sambacha commented 1 year ago

What is more if we find the default settings we see:

# this sets the `yulDetails` of the `optimizer_details` for the `default` profile
[profile.default.optimizer_details.yulDetails]
stackAllocation = true
optimizerSteps = 'dhfoDgvulfnTUtnIf'

https://github.com/foundry-rs/foundry/blob/master/config/src/lib.rs#L4198C44-L4198C61

pcaversaccio commented 1 year ago

FWIW, fDnTOc is the default cleanup steps which you configure via the cleanup sequence delimiter :. If not set, the default of fDnTOc is applied (as in your example). You should try to set optimizerSteps = 'u:', i.e. use an empty entry for the default cleanup sequence.

mattsse commented 1 year ago

friendly ping @Evalir

sambacha commented 1 year ago 
dhfoDgvulfnTUtnIf

my man.

@mattsse is there any possibility of exposing this argument on the CLI? It would make setting up a build matrix for comparison of compiled output using different optimizer steps easier to do and review.

sambacha commented 1 year ago

294.36s vs 116.00s is a non-trivial gain in speedup for CI testing purposes

Run with no optimizerSteps

[⠊] Compiling 129 files with 0.8.19
[⠰] Solc 0.8.19 finished in 294.36s
Compiler run successful!		 Contract                           Size (kB) Margin (kB)
Actor                             2.638     21.938      
Auth                               0.982     23.594      
AuthManager                       2.151     22.425      
Buffer                             0.004     24.572      
BytesLib                           0.004     24.572      
CompatibilityFallbackHandler       2.84       21.736      
CryticERC4626FunctionalAccounting 8.45       16.126      
CryticERC4626Harness               24.834     -0.258      
CryticERC4626MustNotRevert         5.645     18.931      
CryticERC4626PropertyBase         0.004     24.572      
CryticERC4626PropertyTests         24.834     -0.258      
CryticERC4626RedeemUsingApproval   8.157     16.419      
CryticERC4626Rounding             6.536     18.04      
CryticERC4626SecurityProps         2.904     21.672      
CryticERC4626SenderIndependent     3.782     20.794      
CryticERC4626VaultProxy           2.454     22.122      
MevETHRateProvider                 0.379     24.197      
MevEth                             22.352     2.224      

 

Run with optimizerSteps = 'dhfoDgvulfnTUtnIf'

$ forge build --sizes --force
[⠊] Compiling...
[⠘] Compiling 129 files with 0.8.19
[⠢] Solc 0.8.19 finished in 116.00s
Compiler run successful!		 Contract                           Size (kB) Margin (kB)
Actor                             3.113     21.463      
Auth                               1.283     23.293      
AuthManager                       3.039     21.537      
Buffer                             0.009     24.567      
BytesLib                           0.009     24.567      
CompatibilityFallbackHandler       3.984     20.592      
CryticERC4626FunctionalAccounting 9.032     15.544      
CryticERC4626Harness               25.193     -0.617      
CryticERC4626MustNotRevert         6.395     18.181      
CryticERC4626PropertyBase         0.009     24.567      
CryticERC4626PropertyTests         25.193     -0.617      
CryticERC4626RedeemUsingApproval   8.429     16.147      
CryticERC4626Rounding             7.067     17.509      
CryticERC4626SecurityProps         3.951     20.625      
CryticERC4626SenderIndependent     4.044     20.532      
CryticERC4626VaultProxy           2.846     21.73      
MevETHRateProvider                 0.594     23.982      
MevEth                             26.2       -1.624      

run with optimizerSteps = 'fDnTOc'

$ forge build --sizes --force [⠊] Compiling... [⠒] Compiling 129 files with 0.8.19 [⠒] Solc 0.8.19 finished in 123.23s Compiler run successful! Contract Size (kB) Margin (kB)
Actor 3.178 21.398
Auth 1.308 23.268
AuthManager 3.039 21.537
Buffer 0.043 24.533
BytesLib 0.043 24.533
CompatibilityFallbackHandler 3.982 20.594
CryticERC4626FunctionalAccounting 9.16 15.416
CryticERC4626Harness 25.453 -0.877
CryticERC4626MustNotRevert 6.496 18.08
CryticERC4626PropertyBase 0.009 24.567
CryticERC4626PropertyTests 25.453 -0.877
CryticERC4626RedeemUsingApproval 8.588 15.988
CryticERC4626Rounding 7.226 17.35
CryticERC4626SecurityProps 3.979 20.597
CryticERC4626SenderIndependent 4.069 20.507
CryticERC4626VaultProxy 2.903 21.673
MevETHRateProvider 0.594 23.982
MevEth 26.658 -2.082

run using optimizerSteps = 'u:fDnTOc'

[⠆] Compiling 129 files with 0.8.19 [⠘] Solc 0.8.19 finished in 119.62s Compiler run successful! Contract Size (kB) Margin (kB)
Actor 3.113 21.463
Auth 1.283 23.293
AuthManager 3.041 21.535
Buffer 0.009 24.567
BytesLib 0.009 24.567
CompatibilityFallbackHandler 3.985 20.591
CryticERC4626FunctionalAccounting 9.042 15.534
CryticERC4626Harness 25.236 -0.66
CryticERC4626MustNotRevert 6.395 18.181
CryticERC4626PropertyBase 0.009 24.567
CryticERC4626PropertyTests 25.236 -0.66
CryticERC4626RedeemUsingApproval 8.456 16.12
CryticERC4626Rounding 7.077 17.499
CryticERC4626SecurityProps 3.936 20.64
CryticERC4626SenderIndependent 4.044 20.532
CryticERC4626VaultProxy 2.846 21.73
MevETHRateProvider 0.594 23.982
MevEth 26.504 -1.928
pcaversaccio commented 1 year ago 
dhfoDgvulfnTUtnIf

my man.

@mattsse is there any possibility of exposing this argument on the CLI? It would make setting up a build matrix for comparison of compiled output using different optimizer steps easier to do and review.

Haven't checked whether this works, but what about using:

forge config --json | jq -r ".optimizer_details.yulDetails.optimizerSteps"
sambacha commented 1 year ago

On second thought, it may in fact be an actual feature and not bug not to respect optimizer steps

https://soliditylang.org/blog/2023/07/19/full-inliner-non-expression-split-argument-evaluation-order-bug/

The use of custom optimizer sequences in the first place is uncommon. The feature is intentionally not widely advertised. Using it in a way that provides actual benefits is not easy. Anyone with enough knowledge to use it would also be likely to avoid inefficient sequences necessary to trigger this bug.

I now remember why I don't even bother: "We assigned the bug an overall score of "low". The bug has "high" severity in affected cases, but we deem the likelihood of it actually affecting deployed contracts as "very low"."

sambacha commented 1 year ago

We can close this issue, my rage has subsided.

Though is it possible to have custom alerts for users using this setting in afflicted compiler versions? @mattsse

mattsse commented 1 year ago

supportive for both

sambacha commented 1 year ago

supportive for both

  • adding aliases for certain configs
  • checking versions

I only worry about the ongoing maintenance of providing such checks etc. It may create a false sense of security. The compiler team does provide a json file of all reported security issues that could be leveraged at the very least.