feat: gas reports and snapshots by just-mitch · Pull Request #12724 · AztecProtocol/aztec-packages

just-mitch · 2025-03-13T17:35:13Z

Gas Reports

You can run ./bootstrap.sh gas_report to generate a detailed gas report for the current state and update the gas_report.md file.

When running CI or tests with ./bootstrap.sh test, the script will automatically check if gas usage has changed by running ./bootstrap.sh gas_report check. If gas usage has changed, the test will fail and show a diff of the changes.

If the changes in gas usage are expected and desired:

Review the diff shown in the output
Run ./bootstrap.sh gas_report to update the gas report file
Commit the updated gas_report.md file

NOTE: Our gas reporting excludes certain tests due to Forge limitations:

FeeRollupTest and MinimalFeeModelTest test suites are excluded
testInvalidBlobHash and testInvalidBlobProof test cases are excluded

This is related to this Foundry issue.

This means that we don't report gas for blob validation (currently 50k gas per blob, and we use 3 blobs per propose in production).

If you want to run gas reports directly with forge, you must use the environment variable FORGE_GAS_REPORT=true instead of the --gas-report flag. The ./bootstrap.sh gas_report command does this for you automatically.

allow tolerance in snapshot check.

LHerskind · 2025-03-13T18:09:57Z

l1-contracts/test/MultiProof.t.sol

        )
      )
    );
+    // skip blob check if forge gas report is true


This and the rollup.t.sol are both pulling from the rollup base. We could instead have a value that is written in the constructor or so to set it up neatly across all of them and not having multiple places needing to go look for env vars? Could also mean that this value is just set in that one place and not even needed in the others which seems nice

An extra reason for this is that it would be inherited 👀 so we would avoid an issue as just encountered hehe where the ignition ones are using the blobs so it fails because not updated.

LHerskind · 2025-03-13T18:11:57Z

l1-contracts/README.md

+
+When your PR is ready for review, run `./bootstrap.sh snapshot` to update the snapshot, then `./bootstrap.sh test` to make sure you're good.
+
+You can also run `./bootstrap.sh gas_report` to get a gas report for the current state.


Think it would be fine to mention the FORGE_GAS_REPORT here. Also we should just have a general note that a bunch of the blob logic is skipped in the gas reports so they are not exact

LHerskind · 2025-03-13T18:15:25Z

l1-contracts/README.md


+## Gas snapshots and CI
+
+CI will run `forge snapshot --check`. This means that as you develop, you should run `./bootstrap.sh snapshot --diff` to make sure you understand the gas cost of your changes.


I'm not really sure how much I would care about the gas spent on tests, but more the gas spent on the functions in the contracts. e.g., a test spending 15M gas is not super important for me, but if it is a function that we use that is spending 15M gas that is something that is important

LHerskind · 2025-03-13T18:29:08Z

I think storing something like the gas report for core contracts would be more useful than the current snapshots as it is much more clear that it was not just a test changing that caused the diff.

LHerskind · 2025-03-13T21:49:24Z

l1-contracts/bootstrap.sh

-  echo "$hash cd l1-contracts && forge test --no-match-contract UniswapPortalTest"
+  # Test the things that fail with --gas-report (due to forge issues) separately
+  # need to chain these together, otherwise we get a "bus error (core dumped)" occasionally.
+  echo "$hash cd l1-contracts && forge test --match-test \"(testInvalidBlobHash)|(testInvalidBlobProof)\" && forge test --match-contract \"(FeeRollupTest)|(MinimalFeeModelTest)\" && ./bootstrap.sh gas_report check"


I think this way of doing it is too convoluted and easy to mess up such that we are not running a bunch of tests.
Also we know that gas-report and test don't behave the same so i would not tempt fate.

Can you run test and then afterwards doing the gas report instead?

I think the gas report should also run with isolate and force a rebuild to make sure that you got the matching things, otherwise I would expect the diff to mess you up sometimes (it does in this pr actually)

yes, but we then need to pull it out of here and not run through the parallelize
edit: nevermind- I misunderstood your rebuild comment.

LHerskind · 2025-03-13T21:59:32Z

l1-contracts/foundry.toml

+mainnet_fork="https://mainnet.infura.io/v3/9928b52099854248b3a096be07a6b23c"
+
+[fuzz]
+seed = "0000000000000000000000000000000000000000000000000000000000000042"


I'm not sure we would generally do that, was thinking only for the gas reporting to not have the checks and such be messed up.

Generally think it is nice for tests and such that seed is not static, sometimes new things come up because of it.

Ah I didn't see that you could specify --fuzz-seed. Yes for sure. This made me nervous too.

LHerskind · 2025-03-13T22:22:30Z

l1-contracts/test/base/RollupBase.sol

      }
      blobHashes[0] = blobHash;
-      vm.blobhashes(blobHashes);
+      // https://github.com/foundry-rs/foundry/issues/10074


just-mitch · 2025-03-13T22:54:57Z

feat: gas reports and snapshots #12724 👈 (View in Graphite)
master

This stack of pull requests is managed by Graphite. Learn more about stacking.

LHerskind

Solid stuff 🫡

* master: fix: filter for empty attestations when pulling from block (#12740) feat: one-way noir sync (#12592) feat(avm): Address derivation gadget (#12721) chore(ci): add workflow dispatch to masternet (#12739) feat: add default accounts (#12734) feat: gas reports and snapshots (#12724) fix(avm): fix vm1 fake proof size (#12733) feat(bb): extend_edges optimization for zero values past end_index (#12703) fix: remove hard coding of constructor for account manager (#12678) git subrepo push --branch=master noir-projects/aztec-nr git_subrepo.sh: Fix parent in .gitrepo file. [skip ci] chore: replace relative paths to noir-protocol-circuits git subrepo push --branch=master barretenberg fix: verify_honk_proof inputs generation in bootstrap (#12457) fix: Patches to cycle_group and cycle_group fuzzer (#12385)

Skips the uniswap test in ci again as we deleted that part by mistake in #12724

just-mitch requested a review from charlielye as a code owner March 13, 2025 17:35

just-mitch linked an issue Mar 13, 2025 that may be closed by this pull request

feat: l1 gas reports and snapshots #12616

Closed

just-mitch requested review from LHerskind and Maddiaa0 March 13, 2025 17:35

just-mitch added 3 commits March 13, 2025 13:50

feat: ./bootstrap gas_report

bd476a9

chore: make snapshot, update bootstrap, and readme

15d01be

chore: better readme

9f0c7b1

Maddiaa0 approved these changes Mar 13, 2025

View reviewed changes

Update snapshot after rebase.

7385dab

allow tolerance in snapshot check.

just-mitch force-pushed the 12616-feat-l1-gas-reports branch from 6c6fe5c to 7385dab Compare March 13, 2025 17:56

LHerskind reviewed Mar 13, 2025

View reviewed changes

just-mitch added 2 commits March 13, 2025 16:20

re-focus effort on gas report

1ecf549

chore: get the gas report snapshots working

fd20f41

just-mitch requested a review from LHerskind March 13, 2025 21:34

LHerskind reviewed Mar 13, 2025

View reviewed changes

just-mitch force-pushed the 12616-feat-l1-gas-reports branch from 88401df to f7cf45b Compare March 13, 2025 22:54

just-mitch force-pushed the 12616-feat-l1-gas-reports branch 3 times, most recently from 6901067 to a3d5df5 Compare March 14, 2025 00:45

chore: remove .gas_snapshot

c3e5e47

just-mitch force-pushed the 12616-feat-l1-gas-reports branch from a3d5df5 to c3e5e47 Compare March 14, 2025 09:36

LHerskind approved these changes Mar 14, 2025

View reviewed changes

LHerskind merged commit 30f307a into master Mar 14, 2025
7 checks passed

LHerskind deleted the 12616-feat-l1-gas-reports branch March 14, 2025 11:20

AztecBot mentioned this pull request Mar 14, 2025

chore(master): release 0.81.0 #12689

Merged

LHerskind mentioned this pull request Mar 18, 2025

chore: once again skip the uniswap tests #12859

Merged

just-mitch pushed a commit that referenced this pull request Mar 18, 2025

chore: once again skip the uniswap tests (#12859)

4f37dec

Skips the uniswap test in ci again as we deleted that part by mistake in #12724


		When your PR is ready for review, run `./bootstrap.sh snapshot` to update the snapshot, then `./bootstrap.sh test` to make sure you're good.

		You can also run `./bootstrap.sh gas_report` to get a gas report for the current state.


		## Gas snapshots and CI

		CI will run `forge snapshot --check`. This means that as you develop, you should run `./bootstrap.sh snapshot --diff` to make sure you understand the gas cost of your changes.

Conversation

just-mitch commented Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Gas Reports

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LHerskind commented Mar 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

just-mitch Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

just-mitch commented Mar 13, 2025

Uh oh!

LHerskind left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

just-mitch commented Mar 13, 2025 •

edited

Loading

just-mitch Mar 13, 2025 •

edited

Loading