Repository navigation
chore: bump Foundry to 1.8.5 - #25602
Conversation
AztecBot
added
ci-draft
|
Publishing the Short version: on the Stacked lint PR: #25605. Created by claudebox · group: |
|
Runbook v2 (login step now reads the Docker Hub password from a silent prompt instead of a command line, so it never lands in shell history or a file): https://gist.github.com/AztecBot/9b1b2331855fc1315420fdd9cfc59e1f — supersedes the link in the comment above. Created by claudebox · group: |
| sir="${parts[1]}" | ||
| iid="${parts[2]}" | ||
| trap 'aws_terminate_instance $iid $sir || true' EXIT | ||
| local state_dir=$(mktemp -d /tmp/aws_request_instance.XXXXXX) |
There was a problem hiding this comment.
The EXIT trap on the next line is single-quoted, so $state_dir is expanded when the script exits. By then build_ec2 has returned and its local is gone. With set -euo pipefail on, the trap aborts with state_dir: unbound variable before aws_terminate_instance runs, and || true can't catch an expansion error. The failure path does the same, because errexit unwinds the function before the trap fires.
So the instance is never terminated. On success the script also exits 1 instead of 0, which fails build_all under parallel, and deploy never reaches update_manifests or update_amis. Dropping local fixes it:
| local state_dir=$(mktemp -d /tmp/aws_request_instance.XXXXXX) | |
| state_dir=$(mktemp -d /tmp/aws_request_instance.XXXXXX) |
Written by Claude
There was a problem hiding this comment.
Written by human: I haven't verified the report above
|
FYI this needs porting to v6 and private |
|
@spalladino the review comment on On the v6/private port: added Created by claudebox · group: |
|
Publishing the #25606 adds a manually dispatched That SHA is the current head of this branch, whose Created by claudebox · group: |
|
Review follow-up, pushed as
Checked and unchanged: CI build order on this head (1383 passed, 0 failed, 3 skipped); Not verifiable here: the Created by claudebox · group: |
|
CI diagnosis for the 16:02 UTC run on The isolated test pulls That run pulled Created by claudebox · group: |
… compiles incrementally
… terminate the instance
…l builds Foundry 1.8.5's forge install syncs every submodule of the repository to foundry.lock and exits non-zero when the lock names a revision that a shallow clone does not have. The lock lists lib/circuits, labs and noir/noir-repo at revisions behind the ones the repository records, so the build failed on a cache miss. git submodule update already checks the libraries out at the recorded revisions. Also correct the isolate comment: gas reports isolate every call regardless of the setting, so it has no effect on the committed reports.
The 16:02 UTC run pulled a devbox:3.1 published before the daemon config disabled the containerd image store, so the inner Docker extracted build:3.1 outside the cached /var/lib/docker volume and ran out of disk. The image republished at 16:15 UTC carries the setting.
forge script holds an exclusive lock on the script's recovery state for the whole broadcast and exits immediately when another forge already holds it, so two deployments launched from the same project directory cannot overlap. forge_broadcast.js now serializes them on a lock file in cache/, reclaiming a lock whose holder has exited.
Carried as labs-patches/0011 until aztec-node takes it. anvil 1.8 returns a transaction's hash before mining it, so clients built without an explicit polling interval paid viem's 4 s default on every receipt wait; the l1_publisher integration test went from 102 s to past its 600 s budget. The patch defaults the interval to the 1 s the node's own clients use and has the jest runners and the e2e compose file set 100 ms.
…tion Carried as labs-patches/0012. epoch-cache, sequencer-client and cli tests share the host and all bound anvil to 8545; CI hit the collision in epoch_cache.integration.test.ts.
Under the osaka EVM target the 32-validator flush in one transaction falls a few thousand gas short of the per-deposit flush floor at a 16M limit; 2^24 is the largest a single transaction may use.
… after next's 0011
93971ae to
ba65660
Compare
The build runs forge fmt in barretenberg/sol, which under 1.8.5 rewrote Relations.sol and CommitmentScheme.sol and left the working tree dirty and out of sync with honk_contract.hpp and honk_zk_contract.hpp. Whitespace-only; both templates regenerated with copy_to_cpp.sh.
…ed project Foundry 1.8 records its remappings as absolute paths in cache/solidity-files-cache.json and drops the whole cache when they differ from the project's. The l1-artifacts bundle is copied to a temp directory before every deploy, so each forge script there recompiled from scratch. Since the entry queue flush became a second forge script run, that compile happens after the rollup is deployed. On the interval-mining anvil the tests use, the chain advances one 12 s block per wall-clock second while solc runs, so the flush landed ~100 s after genesis instead of ~36 s: past the epoch 3 validator-set sample time (and, on slow runners, past epoch 3 itself), which broke the epoch cache integration tests. forge_broadcast.js now rebases the cache's recorded paths onto the directory it runs in before broadcasting, so the prebuilt artifacts are reused and no compilation happens.
Flakey Tests🤖 says: This CI run detected 1 tests that failed, but were tolerated due to a .test_patterns.yml entry. |
charlielye
left a comment
charlielye
left a comment
There was a problem hiding this comment.
ive managed to build this and have pushed the new amis.
if it goes green i guess its good to merge!
|
@spalladino has reservations about merging it for v6, just in case it causes problems |
## Summary Stacked on #25602 (base branch `cb/foundry-1.8.5`). Brings `forge lint` under Foundry 1.8.5 from 245 warnings to zero in `l1-contracts`, without changing any deployed bytecode. | Where | Count before | What this PR does | | --- | --- | --- | | `test/` + `script/` (`environment-read-across-mutation`) | 85 | Real fix: the flagged `block.timestamp` / `block.number` reads in functions that also `vm.warp` / `vm.roll` now use `vm.getBlockTimestamp()` / `vm.getBlockNumber()`, which is what the lint asks for (the optimizer may reuse an environment read across the cheatcode). 84 lines in 33 files. | | `src/` policy lints (`block-timestamp`, `require-revert-in-loop`, `calls-loop`, `reentrancy-events`, `missing-zero-check`) | 109 | Excluded in `foundry.toml` `[lint]`, each with a one-line reason: slot timing is timestamp-based by design; committee loops check and call per member on purpose; events are emitted after the calls they describe with state already settled; the zero-address sites are constructor/governance wiring. | | `src/` correctness lints (`unsafe-typecast` 25, `unused-return` 10, `uninitialized-local` 11, `empty-block` 3, `erc20-unchecked-transfer` 2, `reentrancy-no-eth`, `encode-packed-collision`, `missing-events-*` 2) | 55 | Reviewed one by one. Every site gets an inline `// forge-lint: disable-next-item(...)` preceded by a comment stating why it is safe (bounds, storage width, guard above, intentional truncation). Seven `uninitialized-local` sites got an explicit `= 0` initialiser where that is bytecode-neutral; the four in `SlashingProposer` are suppressed instead because an explicit initialiser there changes the emitted code by one byte. | `lint_on_build` stays on, so `forge build` is now quiet. ## Verification (Foundry 1.8.5 binaries) - `forge lint` / `forge build`: 0 warnings (was 245). - `forge fmt --check`: clean. All 41 directives are on their own line (fmt's comment wrapping merges a directive into a preceding long comment, which silently disables it; two such cases were caught and fixed). - **Bytecode:** built the base branch and this branch side by side and compared the creation and runtime bytecode of all 125 `src/` contracts byte-by-byte. Outside CBOR metadata hashes (which change with any source text), there are **no differences**. The lint PR does not alter what gets deployed. - `forge test`: 1383 passed, 0 failed, 3 skipped, in six separate full runs on this tree, including one with the Osaka EVM target the base branch now uses. - `solhint`: unchanged. ## Judgement calls for review - `unused-return` on `approve(...)` (StakingLib ×2, GSE ×2, MultiAdder): suppressed on the stated assumption that the staking asset is the protocol's OZ ERC20 (returns true or reverts). If a non-OZ asset is ever plausible, `forceApprove` is the real fix, at a bytecode cost. - `encode-packed-collision` in `SlashingProposer._preparePayloadDataAndAddress`: suppressed because `validators` and `amounts` are both sized by `actionCount` behind a fixed 32-byte round prefix, so the packed layout cannot be ambiguous. Worth a second pair of eyes since it is a hashing site. - `missing-events-*` (GSE.addRollup, Inbox.markProvenConsumed): suppressed rather than adding events, to keep bytecode identical. Adding events is a product decision. --- *Created by [claudebox](https://claudebox.work/v2/sessions/f38155cc1541e786/jobs/8) · group: `slackbot` · requested by Mike (@iAmMichaelConnor) · [Slack thread](https://aztecfoundation.slack.com/archives/D0B2N7W1WJD/p1791282577517869?thread_ts=1791282577.517869&cid=D0B2N7W1WJD)* --------- Co-authored-by: Santiago Palladino <santiago@aztec-labs.com>
## Summary Stacked on #25605 (base branch `cb/foundry-1.8.5-lint`). Bumps the Solidity compiler to 0.8.37 (latest, released 2026-09-10) in every place the repo pins it: - `l1-contracts/foundry.toml`: 0.8.30 → 0.8.37 (`solc = "./solc-0.8.37"`; `bootstrap.sh download_solc` derives the version from this line and fetches the binary) - `barretenberg/sol/foundry.toml`, which points at that same binary - `barretenberg/acir_tests/sol-test` (the solc-js harness that deploys and runs generated verifiers end to end): `package.json` now pins `solc` at exactly `0.8.37`, and the workspace `yarn.lock` moves its resolution from 0.8.28 to 0.8.37 **Unlike the two PRs below it in the stack, this one changes what gets deployed, slightly.** It should be merged on its own decision by whoever signs off L1 bytecode, not as tooling hygiene. It deliberately carries no port labels for that reason. ## Why - solc 0.8.30 has seven entries in the official known-bugs list; 0.8.37 has none. None of the seven is reachable in this code as written: five require the IR pipeline (`via_ir` is not set in either project), the memory `bytes` element `delete` pattern does not occur in `l1-contracts/src`, and no contract uses a custom storage layout. So this is not a security fix; it removes the need to re-argue that for every future change. - 0.8.31 made Osaka the default EVM target. Under 0.8.30 the `evm_version = 'osaka'` set by #25602 is labelled experimental. - 0.8.36 adds the Amsterdam EVM target. - One compiler version across forge and the solc-js harness means the verifier tested end to end is compiled by the same compiler as the one built by forge. ## Effect on compiled output (measured) `l1-contracts/src`, built with Foundry 1.8.5, Osaka target, optimizer 100 runs, 0.8.30 vs 0.8.37, 86 contracts with runtime code: | | Result | | --- | --- | | Runtime code identical (metadata aside) | 84 of 86 | | Runtime code changed | `SlasherDeploymentExtLib` (+15 bytes), `StakingAssetHandler` (mock, +15 bytes) | | Constructor code changed, runtime identical | `Rollup` (+7), `RollupCore` (+7), `SlashingProposer` (+15), `GovernanceProposer` (+15), `TestERC20` (+15), `MockFeeJuicePortal` (+15) | | `Rollup` runtime size | 24,222 bytes, unchanged (354 below the EIP-170 limit) | | `ValidatorOperationsExtLib` runtime size | 23,767 bytes, unchanged | The changed code is the helper that clears storage slots when a `string`/`bytes` value is written to storage: 0.8.37 emits an extra bounds check there, consistent with the 0.8.32 fix for array clearing near the end of storage. `SlasherDeploymentExtLib` changes only because it embeds `SlashingProposer`'s constructor code. Every contract's metadata trailer also changes, since it records the compiler version. `barretenberg/sol`: `BlakeHonkVerifier` and `BlakeHonkZKVerifier` (compiled against a placeholder verification key, since real keys need a barretenberg build) are byte-identical apart from the single compiler-version byte in the trailer: 15,161 and 16,175 bytes of runtime code, unchanged. `sol-test` harness: compiling its `HonkTest.sol` together with a real bb-generated `HonkVerifier` under solc-js 0.8.28 and 0.8.37, with the harness's own settings (optimizer runs 1, no CBOR metadata, default EVM target), gives byte-identical creation and runtime code for both contracts (verifier runtime 16,624 bytes). The harness sets no `evmVersion`, so its default target moves from Cancun to Osaka with this compiler; the output does not change. ## Verification (Foundry 1.8.5 + solc 0.8.37) - `forge test` in `l1-contracts`: 1383 passed, 0 failed, 3 skipped (1386). - `forge fmt --check` clean; `forge lint` 0 warnings. - `scripts/check_contract_sizes.sh`: within EIP-170 on both profiles. - `scripts/test_rollup_upgrade.sh` (anvil + forge script broadcast): completed successfully. - `yarn install --mode=update-lockfile` in `barretenberg/acir_tests`: the lock diff is the `solc` entry and its `tmp` dependency (0.0.33 → 0.2.6, dropping `os-tmpdir`), nothing else. Not run locally, left to CI: the `barretenberg/sol` test suite and the `acir_tests` Solidity flows (both need a barretenberg build for keys and proofs), the real generated `HonkVerifier` in `l1-contracts`, and the arm64 svm download of 0.8.37. ## Things to know - 0.8.37 adds 14 non-fatal compiler warnings in the `src` build. Four are in this repo: two comparisons of contract-typed variables in `EmpireBase.sol` (deprecated; fix is an explicit `address(...)` cast) and two library functions named `at` (`AddressSnapshotLib`, `StakingQueue`), which is slated to become a keyword. The other ten are in OpenZeppelin (`at`, `error` identifiers). Left untouched here to keep this PR to the compiler pin. - `barretenberg/acir_tests/sol-test/package-lock.json` is removed. `sol-test` is a yarn workspace of `barretenberg/acir_tests` and resolves through that directory's `yarn.lock`; nothing in the repo runs `npm install` or `npm ci` there (searched the acir_tests scripts, the barretenberg bootstrap, `.github` and `ci3`), and the file had not been updated since December 2024, so it recorded a solc version (0.8.27) that no install used. - The build image does not embed solc, so no image rebuild is needed for this PR. --- *Created by [claudebox](https://claudebox.work/v2/sessions/f38155cc1541e786/jobs/25) · group: `slackbot` · requested by Mike (@iAmMichaelConnor) · [Slack thread](https://aztecfoundation.slack.com/archives/D0B2N7W1WJD/p1791282577517869?thread_ts=1791282577.517869&cid=D0B2N7W1WJD)* --------- Co-authored-by: Santiago Palladino <santiago@aztec-labs.com>
5 participants
Summary
Moves the pinned Foundry toolchain from 1.4.1 to 1.8.5 (latest release, published 2026-10-05) everywhere the repo pins it, adapts
l1-contractsto the behaviour changes shipped in Foundry 1.5 through 1.8, targets the Osaka EVM that mainnet runs, and introduces build image tag3.1so this toolchain can be published without disturbingnext.Pins updated:
bootstrap.sh(expected_abs_foundry_version, which the toolchain check greps fromforge --version/anvil --version)build-images/src/Dockerfile(FOUNDRY_VERSION, the build/devbox image)scripts/setup-container.shAdaptations required by the new toolchain
forge fmtoutput changed (Foundry 1.5 formatter). 48 Solidity files underl1-contractsreformatted withforge fmt1.8.5 soforge fmt --checkpasses. Every change is whitespace-only: stripping all whitespace from each file before and after yields identical token streams (checked for all 48 files).foundry.toml:isolate = false. Foundry 1.8 runs forge tests with--isolateby default, which executes each top-level call as its own transaction. That charges cold-access and intrinsic gas per call, which pushed the large validator-set tests (RollupGetters,tmnt333,flushEntryQueuefuzz) past the gas limit, and failed thegetCurrentEpochCommitteegas-growth assertion. Pinning it off keeps the semantics the suite was written against. It does not affect the committed gas reports: a gas report (FORGE_GAS_REPORT=true) isolates every call whatever this is set to, and the same report is byte-identical with it on or off. Regeneratinggas_report.jsonunder 1.8.5 reproduces every committedmin/max; only fuzz-driven call counts and averages drift (the 1.5 fuzzer samples inputs differently even with--fuzz-seed 42).gas_benchmark.mdregenerated under 1.8.5 is identical to whatnextproduces under 1.4.1, andpartial_epoch_proof_gas_report.jsonreproduces exactly.foundry.toml:evm_version = 'osaka'. Mainnet runs the Osaka EVM (Fusaka activated; the current fork reported byeth_configis the second BPO fork of 2026-01-07, with no next fork scheduled), and Foundry 1.7+ defaults forge and anvil to Osaka. Building with the Osaka target produces byte-identical creation and runtime code for all 125src/contracts (solc 0.8.30 emits no Osaka-only opcodes here), the full suite passes, the Rollup gas report matches on everymin/max, and nothing insrc/uses the modexp precompile that EIP-7883 reprices. solc 0.8.30 still labelsosakaexperimental in its help text; it is the chain we deploy to, so the tests should simulate it.foundry.toml: droppedvariable_override_spacing. Not a recognised[fmt]key in 1.8.5 (every forge invocation warned about it);override_spacing = falseon the next line is the key that is honoured.test/governance/governance/tmnt331.t.sol.Governance__CallerCannotBeSelf()declares no parameters, but the test encoded anaddressargument into itsexpectRevertdata. Older forge matched that leniently; 1.8.5 compares revert data exactly, so the test now encodes the selector alone.scripts/forge_broadcast.js.forge script1.8.5 rejects--batch-size(error: unexpected argument '--batch-size' found), which brokescripts/test_rollup_upgrade.sh. The wrapper now passes--slow(send the next tx only after the previous one is confirmed) in the automining-anvil case where it previously used--batch-size 1, and leaves forge's default batching elsewhere. Real-chain deploys therefore no longer send in batches of 8; there is no flag left that reproduces that. If strictly sequential live deploys are preferred, pass--slowunconditionally.l1-contracts/bootstrap.sh build_srcandbarretenberg/sol/bootstrap.sh build_solno longer runforge install. Under 1.8.5forge installsyncs every submodule of the repository tofoundry.lockand exits 1 when the lock names a revision a shallow clone does not have. Both lock files listl1-contracts/lib/circuits,labsandnoir/noir-repobehind the revisions the repository records, so the cache-miss build failed before compiling anything (1.4.1 exits 0 and touches nothing). Thegit submodule update --init --recursive ./libthat follows already checks the libraries out at the recorded revisions.l1-contracts/bootstrap.sh build_verifierends with a fullforge build. Under 1.8.5, runningforge testafter an explicit-pathforge buildso that forge incrementally compiles only the handful of leftover files (shouting.t.sol,test/script/*, scripts) produces inconsistent artifacts: 3 of 3 such runs failed 177 tests withEvmError: Revertat the same ~2.93M gas point in every Rollup-deploying suite. A full build before the artifacts are uploaded meansforge testhas nothing left to compile; that sequence passed every time.Build image
3.1build-images/bootstrap.shversion,build-images/run.sh,.devcontainer/dev/devcontainer.json,ci3/bootstrap_ec2,ci3/docker_isolate,ci3/aws/ami_update.shandci3/tests/signal_testnow reference3.1. CI runners boot from an AMI with3.0already cached andbootstrap_ec2runsdocker run aztecprotocol/devbox:<tag>without pulling first, so a re-pushed3.0would never reach them and would break every other PR onnext(whose checkout pins 1.4.1). A fresh tag is pulled on first use and leavesnextuntouched.build-images/bootstrap.sh build_ec2andci3/aws/ami_update.share ported to the currentaws_request_instancecontract (state directory,KEY_NAME,aws_terminate_instance <state_dir>); they still called the pre-March-2026 three-argument form and failed withstate_dir: unbound variablebefore doing anything. The port was exercised end to end against stubbedaws,sshanddockercommands: it exits 0, requests and terminates four instances, pushes the manifests and writes both AMI id files, where the scripts as they are onnextfail with$4: unbound variablebefore requesting anything. It has not been run against real AWS.To go green, the
3.1images (build,devbox,sysbox, amd64 + arm64 + manifest) must be published to Docker Hub. #25606 adds a workflow that runs this branch's own deploy script; a laptop runbook is in the PR thread as a fallback. Refreshing the AMIs (ci3/aws/ami_update.sh, commit the newci3/aws/ami_id_*) only speeds up cold starts and can follow later from the CI AWS account. A mainframe admin also needs to point/usr/local/bin/launch_sysboxatsysbox:3.1.Verified locally with Foundry 1.8.5 binaries
forge fmt --checkforge build(bootstrap's file set)forge test(full suite, Osaka target)scripts/test_rollup_upgrade.sh(anvil 1.8.5 + forge script broadcast)scripts/check_contract_sizes.sh(both profiles, Osaka target)solhintsrc/contracts (metadata aside)Things to know
forge buildnow emits ~245 non-fatalforge-lintwarnings because 1.8 added many detectors. A stacked PR on this branch brings that to zero without changing bytecode.forge test --match-contract MerkleCheck --ffimatches no contract onnext(noMerkleCheckcontract exists undertest/); this predates the bump and still exits 0.uniswap_trade_on_l1_from_l2.test.tsin.test_patterns.ymlis untouched.Created by claudebox · group:
slackbot· requested by Mike (@iAmMichaelConnor) · Slack thread