Optimize Compiler Settings #448

nlordell · 2024-06-21T14:57:49Z

I noticed that the IR optimizer is better for all contracts except the FCL library. This PR optimizes the compiler configuration to take advantage of the IR optimizer, while adding an exception for the FCL P-256 verifier contract so that it does not suffer any regressions.

=== BEFORE ===
  Gas Benchmarking [@bench]
    SafeWebAuthnSignerProxy
      ⛽ deployment: 94366
      ✔ Benchmark signer deployment cost (476ms)
      ⛽ verification (FreshCryptoLib): 216907
      ✔ Benchmark signer verification cost with FreshCryptoLib verifier (116ms)
      ⛽ verification (Daimo): 346809
      ✔ Benchmark signer verification cost with Daimo verifier (103ms)
      ⛽ verification (Dummy): 14309
      ✔ Benchmark signer verification cost with Dummy verifier (100ms)
      ⛽ verification (Precompile): 15098
      ✔ Benchmark signer verification cost with Precompile verifier (104ms)

=== AFTER  ===
  Gas Benchmarking [@bench]
    SafeWebAuthnSignerProxy
      ⛽ deployment: 91042
      ✔ Benchmark signer deployment cost (457ms)
      ⛽ verification (FreshCryptoLib): 213919
      ✔ Benchmark signer verification cost with FreshCryptoLib verifier (121ms)
      ⛽ verification (Daimo): 342642
      ✔ Benchmark signer verification cost with Daimo verifier (110ms)
      ⛽ verification (Dummy): 12564
      ✔ Benchmark signer verification cost with Dummy verifier (104ms)
      ⛽ verification (Precompile): 13379
      ✔ Benchmark signer verification cost with Precompile verifier (98ms)

Furthermore, we bump the compiler version to v0.8.26 as it is the lastest stable release and brings some small gas improvements.

@bench

I noticed that the IR optimizer is better for all contracts **except** the FCL library. This PR optimizes the compiler configuration to take advantage of the IR optimizer, while adding an exception for the FCL P-256 verifier contract so that it does not suffer any regressions. ``` === BEFORE === Gas Benchmarking [@bench] SafeWebAuthnSignerProxy ⛽ deployment: 94366 ✔ Benchmark signer deployment cost (476ms) ⛽ verification (FreshCryptoLib): 216907 ✔ Benchmark signer verification cost with FreshCryptoLib verifier (116ms) ⛽ verification (Daimo): 346809 ✔ Benchmark signer verification cost with Daimo verifier (103ms) ⛽ verification (Dummy): 14309 ✔ Benchmark signer verification cost with Dummy verifier (100ms) ⛽ verification (Precompile): 15098 ✔ Benchmark signer verification cost with Precompile verifier (104ms) === AFTER === Gas Benchmarking [@bench] SafeWebAuthnSignerProxy ⛽ deployment: 90399 ✔ Benchmark signer deployment cost (494ms) ⛽ verification (FreshCryptoLib): 215532 ✔ Benchmark signer verification cost with FreshCryptoLib verifier (123ms) ⛽ verification (Daimo): 345458 ✔ Benchmark signer verification cost with Daimo verifier (103ms) ⛽ verification (Dummy): 12911 ✔ Benchmark signer verification cost with Dummy verifier (96ms) ⛽ verification (Precompile): 13726 ✔ Benchmark signer verification cost with Precompile verifier (106ms) ```

coveralls · 2024-06-21T15:00:29Z

Pull Request Test Coverage Report for Build 9615641386

Details

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 100.0%

Totals
Change from base Build 9578863041:	0.0%
Covered Lines:	90
Relevant Lines:	90

💛 - Coveralls

mmv08 · 2024-06-21T15:06:24Z

modules/passkey/hardhat.config.ts

@@ -31,6 +31,18 @@ const customNetwork = CUSTOM_NODE_URL
    }
  : {}

+const compilerSettings = {
+  version: '0.8.24',


if we're changing the compiler settings, what about using the latest version?

My rationale for not doing it is as such:

from the changelog there are no significant features or fixes that were added since 0.8.24: source

Currently, the FCL256Verifier is byte-for-byte the same (and won't change with any of the planned changes 🎉, none of the audit competition changes affect it). Updating the compiler version would change that This appears to be false 😞

The audit was done for compiler v0.8.24, I know we are changing the compiler settings, but it feels less extreme than a new compiler version.

I don't feel strongly either way, I did consider it when making this PR but opted to err on the side of caution.

I ran the benchmarks with 0.8.26 and it seems it slightly improves runtime gas costs:

Gas Benchmarking [@bench] SafeWebAuthnSignerProxy ⛽ deployment: 91042 ✔ Benchmark signer deployment cost (568ms) ⛽ verification (FreshCryptoLib): 215607 ✔ Benchmark signer verification cost with FreshCryptoLib verifier (107ms) ⛽ verification (Daimo): 345542 ✔ Benchmark signer verification cost with Daimo verifier (100ms) ⛽ verification (Dummy): 12564 ✔ Benchmark signer verification cost with Dummy verifier (107ms) ⛽ verification (Precompile): 13379 ✔ Benchmark signer verification cost with Precompile verifier (117ms)

With 0.8.25 they're almost identical except the Daimo verifier:

Gas Benchmarking [@bench] SafeWebAuthnSignerProxy ⛽ deployment: 90399 ✔ Benchmark signer deployment cost (508ms) ⛽ verification (FreshCryptoLib): 213844 ✔ Benchmark signer verification cost with FreshCryptoLib verifier (145ms) ⛽ verification (Daimo): 342496 ✔ Benchmark signer verification cost with Daimo verifier (121ms) ⛽ verification (Dummy): 12911 ✔ Benchmark signer verification cost with Dummy verifier (110ms) ⛽ verification (Precompile): 13726 ✔ Benchmark signer verification cost with Precompile verifier (111ms)

I guess those small improvements (from the 0.8.26 changelog) made a slight difference, not a compiler engineer, though:

Compiler Features: Yul IR Code Generation: Cheaper code for reverting with errors of a small static encoding size. Yul Optimizer: New, faster default optimizer step sequence.

Note that the code got slightly bigger from v0.8.25 -> v0.8.26

=== v0.8.25 === Gas Benchmarking [@bench] SafeWebAuthnSignerProxy ⛽ deployment: 90399 ✔ Benchmark signer deployment cost (508ms) === v0.8.26 === Gas Benchmarking [@bench] SafeWebAuthnSignerProxy ⛽ deployment: 91042 ✔ Benchmark signer deployment cost (568ms)

Still, overall, I think you are right, we can bump the compiler version here. My main argument was to reuse the FCLP256Verifier, but that is moot apparently 😞

except the Daimo verifier:

This is slightly random because the verifiers have different gas characteristics depending on what public key/signature is used (which is random in the tests).

I would look specifically at deployment, dummy and precompile gas metrics only (unless you collect data from many runs and compute averages for FCL and Daimo).

Also, Daimo verifier is not compiled (we vendor bytecode directly), so compiler settings should not affect it at all.

IMplemented suggestion in a6cd15a and updated PR description.

mmv08 · 2024-06-21T15:06:56Z

modules/passkey/hardhat.config.ts

+      runs: 10_000_000,
+    },
+    viaIR: true,
+    evmVersion: 'paris',


paris is pre-push0, right? maybe we don't need this either?

We do, so we are BNB compatible.

Also, note that hardhat currently defaults to paris anyway (at least it did when I experimented with switching this setting off), so this is more to explicitly document the EVM version that is being targetted.

it seems like BNB already has push0? https://forum.bnbchain.org/t/bnb-chain-roadmap-mainnet/936

This is super recent! It wasn't the case 4 days ago: hats-finance#19 (comment)

Then s/BNB/Linea/g: I think the point remains the same that PUSH0 isn't generally well supported across L2s:

$ curl -s https://rpc.linea.build -H 'content-type: application/json' --data '{"jsonrpc":"2.0","id":0,"method":"eth_call","params":[{"data":"0x5f"},"latest"]}' {"jsonrpc":"2.0","id":0,"error":{"code":-32000,"message":"invalid opcode: PUSH0"}}

interesting, the forum post says it was implemented in Kepler: 2024/01/23 hardfork

remedcu · 2024-06-21T15:16:19Z

modules/passkey/hardhat.config.ts

+      // in the rest of the project without causing significant regressions to the FCL verifier, we
+      // add a compiler setting override for that specific contract.
+      'contracts/verifiers/FCLP256Verifier.sol': {
+        ...compilerSettings,


Suggested change

...compilerSettings,

version: compilerSettings.version,

I did this on purpose to future proof in case new fields were added. Agree it isn't strictly necessary here, but wanted to future proof just in case. Also it feels to convey intent a bit more IMO: we want exactly compilerSettings except for this single value.

coveralls · 2024-06-21T16:59:39Z

Pull Request Test Coverage Report for Build 9617116293

Details

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 100.0%

Totals
Change from base Build 9578863041:	0.0%
Covered Lines:	90
Relevant Lines:	90

💛 - Coveralls

nlordell requested a review from a team as a code owner June 21, 2024 14:57

nlordell requested review from akshay-ap, mmv08 and remedcu and removed request for a team June 21, 2024 14:57

mmv08 approved these changes Jun 21, 2024

View reviewed changes

mmv08 reviewed Jun 21, 2024

View reviewed changes

remedcu reviewed Jun 21, 2024

View reviewed changes

Bump compiler version to v0.8.26

a6cd15a

akshay-ap approved these changes Jun 25, 2024

View reviewed changes

nlordell requested a review from remedcu June 25, 2024 15:57

remedcu approved these changes Jun 25, 2024

View reviewed changes

nlordell merged commit 1e57772 into main Jun 25, 2024
10 checks passed

nlordell deleted the chore/optimal-solidity-configuration branch June 25, 2024 20:07

github-actions bot locked and limited conversation to collaborators Jun 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize Compiler Settings #448

Optimize Compiler Settings #448

nlordell commented Jun 21, 2024 •

edited

Loading

coveralls commented Jun 21, 2024 •

edited

Loading

mmv08 Jun 21, 2024

nlordell Jun 21, 2024 •

edited

Loading

mmv08 Jun 21, 2024

mmv08 Jun 21, 2024 •

edited

Loading

nlordell Jun 21, 2024 •

edited

Loading

nlordell Jun 21, 2024

nlordell Jun 21, 2024 •

edited

Loading

mmv08 Jun 21, 2024

nlordell Jun 21, 2024

mmv08 Jun 24, 2024

nlordell Jun 24, 2024 •

edited

Loading

mmv08 Jun 24, 2024

remedcu Jun 21, 2024

nlordell Jun 21, 2024

coveralls commented Jun 21, 2024 •

edited

Loading

Optimize Compiler Settings #448

Optimize Compiler Settings #448

Conversation

nlordell commented Jun 21, 2024 • edited Loading

coveralls commented Jun 21, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9615641386

Details

💛 - Coveralls

Choose a reason for hiding this comment

nlordell Jun 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmv08 Jun 21, 2024 • edited Loading

Choose a reason for hiding this comment

nlordell Jun 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nlordell Jun 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nlordell Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jun 21, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9617116293

Details

💛 - Coveralls

nlordell commented Jun 21, 2024 •

edited

Loading

coveralls commented Jun 21, 2024 •

edited

Loading

nlordell Jun 21, 2024 •

edited

Loading

mmv08 Jun 21, 2024 •

edited

Loading

nlordell Jun 21, 2024 •

edited

Loading

nlordell Jun 21, 2024 •

edited

Loading

nlordell Jun 24, 2024 •

edited

Loading

coveralls commented Jun 21, 2024 •

edited

Loading