Repository navigation
ci(l1,l2): make the cargo cache restorable, and cut duplicate builds in the L2 dev test - #7029
Merged
Merged
Conversation
|
Greptile SummaryThis PR streamlines Rust caching and L2 integration-test builds.
Confidence Score: 5/5The PR appears safe to merge with no actionable defects identified in the changed build and workflow paths. The cache policy, feature forwarding, build profiles, binary paths, and test invocations are internally aligned across the changed workflows.
|
| Filename | Overview |
|---|---|
| .github/actions/setup-rust/action.yml | Restricts Rust cache persistence to the main branch to reduce cache eviction. |
| .github/workflows/main_prover.yaml | Builds the prover integration test with the newly forwarded L2 feature set. |
| .github/workflows/pr-main_l1_l2_dev.yaml | Prebuilds release-fast binaries, starts the prebuilt node, and uses shorter readiness polling. |
| .github/workflows/pr-main_l2.yaml | Adds the L2 feature to prebuilds in the standard and shared-bridge integration jobs. |
| test/Cargo.toml | Forwards the ethrex-test L2 feature to ethrex's l2 and l2-sql features. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
Main[main workflow] -->|save and restore| Cache[Cargo cache]
PR[PR workflow] -->|restore only| Cache
BuildNode[Build ethrex release-fast] --> StartNode[Start L2 node]
BuildTest[Build L2 test release-fast] --> RunTest[Run integration test]
StartNode --> Ready[Wait for proof coordinator]
Ready --> RunTest
Reviews (1): Last reviewed commit: "ci(l1,l2): fix cargo cache eviction and ..." | Re-trigger Greptile
🤖 Kimi Code ReviewThe PR improves CI efficiency and correctness, but one change introduces potential flakiness. Critical: Timeout Reduction Risk
Correctness & Best Practices
Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
ilitteri
approved these changes
Jul 27, 2026
🤖 Codex Code ReviewNo findings. The patch is limited to CI/workflow behavior and Residual risk: this was a static review only. I could not run local Cargo verification in this sandbox because Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
ElFantasma
suggested changes
Jul 29, 2026
| - name: Wait for ethrex L2 | ||
| run: | | ||
| for i in {1..100}; do | ||
| for i in {1..120}; do |
Contributor
There was a problem hiding this comment.
Non-blocking, and flagging it because the opposite change was suggested elsewhere on this PR: this loop is not the flakiness risk it looks like. In the latest run the Wait for ethrex L2 step completed in 5 seconds (15:24:17 → 15:24:22), so 120 × 5 s is roughly 120× the observed startup need. Raising the iteration count would only lengthen the failure path for a genuinely dead node; the 25 min budget it replaces was sized when this step was also absorbing the node's compile time, which now happens in its own step above.
(Relevant to the deposit failure too: if that turns out to be startup-timing related, a longer port poll won't help — port 3900 opens in 5 s either way. The gate would need to be on the actual precondition.)
Per-job keys put 32 cache writers on main across the six workflows that trigger there, at 1.2-2.75GB each. Group them into l1/l2/prover pools with one writer per pool so the budget holds the entries it is given.
These never run on main, so under save-if they never write a key and, with per-job keys, never read one either. Naming a pool is what makes main's entry restorable for them.
edg-l
changed the title
rust-cache's default workspaces covers only the repo root, so tooling/target was never cached, including the ef_tests build inside the l1 pool writer's own job. reorg-tests is the only main job building the root workspace at release-fast, so the dev-profile l1 entry cannot serve it.
Benchmark Results ComparisonBenchmark Results: MstoreBench
Detailed ResultsBenchmark Results: BubbleSort
Benchmark Results: ERC20Approval
Benchmark Results: ERC20Mint
Benchmark Results: ERC20Transfer
Benchmark Results: Factorial
Benchmark Results: FactorialRecursive
Benchmark Results: Fibonacci
Benchmark Results: FibonacciRecursive
Benchmark Results: ManyHashes
Benchmark Results: MstoreBench
Benchmark Results: Push
Benchmark Results: SstoreBench_no_opt
|
ElFantasma
reviewed
Aug 3, 2026
ElFantasma
left a comment
ElFantasma
left a comment
Contributor
There was a problem hiding this comment.
Follow-up on the pooling commits: three main-running workflows are missing save-cache: 'false', so they claim the l1 pool slot the one-writer design reserves.
A job that omitted save-cache became a pool writer, and workflow_dispatch and schedule runs carry refs/heads/main, so daily_loc_report, daily_hive_report, deep_reorg, pr_lint_readme, pr_perf_levm and pr_perf_build_block_bench all claimed the l1 slot with a target dir far narrower than the pool's owner needs. Cache keys are immutable, so the first of those to save wins and the owner's later save is skipped. save-cache now defaults to 'false' and the four pool owners opt in.
A node started with --dev funds the rich accounts by depositing half of each L1 balance through the bridge, and the deployer awaits only the L1 receipts. The credits reach L2 through the L1 watcher a batch later, so the suite read a zero initial balance and test_deposit failed by exactly the 5e26 wei pre-funding. The port-3900 readiness gate opens ~20s before those credits land, which the test binary's own compile time used to cover.
ElFantasma
approved these changes
Aug 3, 2026
MegaRedHand
approved these changes
Aug 3, 2026
edg-l
enabled auto-merge
August 3, 2026 13:36
Merged
1 task done
ilitteri
added a commit
that referenced
this pull request
Aug 20, 2026
…shared-bridge test (#7151) **Motivation** `Integration Test Shared Bridge - Normal` fails intermittently with `Receiver balance did not increase correctly` at `test/tests/l2/shared_bridge.rs:409`. It hit `main` on Aug 3 (`0cc4572c5`) and Aug 13 (`f2998963d`), and `release/v24.0.0` on Aug 12 (`8f3d2ca05`), always with the same message hash `0x66867250…5be951` — so the test is not wrong about what it expects, the message genuinely never arrives. In the failing runs L2B never commits batch 1, its L2→L2 message never becomes pending on the router, and L2A's watcher logs `L2 message not found in pending messages` 153 times with 0 verified. Three separate defects combine to produce that: **1. Both sequencers commit from the same L1 account.** `crates/l2/docker-compose.yaml` gives `ethrex_l2` `--committer.l1-private-key 0x385c…` and the shared-bridge override gave `ethrex_l2_b` the same key (likewise the proof-coordinator key). They post to one L1 on the same 15 s timer, so their commit loops race for the same nonce and the loser is rejected — first as an underpriced replacement, then as a too-low nonce. **2. The loser cannot recover, which is a bug beyond CI.** `bump_gas_generic_tx` computed the blob fee as `1 + (bump_percentage / 100) * 10` and then divided by ten. For the only bump either call site passes, `30`, that evaluates to `fee * 1 / 10`: **every retry lowered `max_fee_per_blob_gas` by 90%**, so a batch commitment could never re-price itself — on CI or on a real L1 during a fee spike. Blob replacements also require the fee to *double* (`DEFAULT_BLOB_PRICE_BUMP_PERCENT`), and that threshold applies to the fee cap and the tip as well, so bumping those by 30% could not have replaced a blob transaction either. Separately, the retry loop never noticed when the nonce had become unreachable, so one `NonceTooLow` burned all ten attempts in ~3.5 s and reported "max retries reached", hiding the cause. **3. The test slept a fixed 180 s and then asserted**, which made the result depend on whatever else the job was doing. The first failure in this job's history is the run of #7029, which aligned the `cargo test --no-run` prebuild's features with the run step so the test binary is no longer rebuilt inside it. Job step timestamps show the window between "Start Sequencer L2B" and the assertion collapsing from **2 m 35 s to 3 s**, moving the assertion onto the sequencers' cold start — where batch 1 carries all 192 privileged deposit mints and the two co-nonced committers collide from their first commit. Worth noting the direction: the passing Jul 22 run had *more* nonce collisions (72 underpriced + 76 too-low) than any failing run, because they happened before the test looked. **Description** - **L2B gets its own committer and proof-coordinator accounts, and its deployment authorizes them.** Changing the keys alone is not enough: `Timelock.commitBatch` is `onlyRole(SEQUENCER)`, and the role is granted from the deployer's `--committer.l1-address` / `--proof-sender.l1-address`, which both default to L2A's accounts. Without the matching addresses every L2B `commitBatch` reverts at gas estimation as an unauthorized caller — the same end symptom, reached a different way. Neither option is forwarded to the deployer container by `docker-compose.yaml`, so they are set on the override's `contract_deployer`, leaving L2A's deployment on the defaults. - **`bump_gas_generic_tx` raises every fee dimension** by a bump that clears the mempool's replacement threshold for the transaction's category — `BLOB_FEE_BUMP_PERCENT` (100) for blob-carrying transactions, `FEE_BUMP_PERCENT` (30) otherwise — using saturating arithmetic so an absurd starting fee cannot overflow. - **The retry loop stops spending attempts that cannot succeed:** it abandons the transaction with `UnreachableNonce` once the chain's nonce has passed it (checked against the chain, not against the rejection text, which differs between clients), and reports the reason when the `maximum_allowed_max_fee_per_gas` / `_blob_gas` clamps leave a bid the mempool already rejected. - **Every shared-bridge assertion now waits for its own condition** instead of sharing one fixed sleep, with a timeout more generous than the 180 s it replaces. The L1 deposits accounting is waited on separately rather than assumed settled once the L2B credit lands: the two are updated by different transactions on different chains and nothing orders them. The forced-inclusion test's expiry and verification waits are deliberately left as sleeps — they are waiting for something *not* to happen. - **Hardening:** a test pins L2B's private keys to the addresses its deployment authorizes, so drift between the two blocks of the override file fails there instead of surfacing at runtime as an ABI-undecodable `AccessControlUnauthorizedAccount` revert and a chain that silently stops committing. Unit tests cover both replacement thresholds, monotonicity across ten retries, saturation, and — so they cannot silently stop discriminating — that the previous blob formula fails them. This is deliberately **not** a revert of #7029. That PR removed an accidental delay which had been masking defects 1 and 2 since the shared-bridge test landed; restoring the delay would hide them again. **Also worth knowing, not changed here:** this job is gated `github.event_name != 'merge_group'`, so the merge queue never runs it and a PR can land without it ever executing against the merged result. Combined with the paths filter, it effectively only runs on pushes to main — which is why a ~3% flake sat unnoticed for weeks. **Checklist** - [x] Updated `STORE_SCHEMA_VERSION` (crates/storage/lib.rs) if the PR includes breaking changes to the `Store` requiring a re-sync. — n/a, no `Store` changes
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Nothing in this repo restores a cargo cache. Every job logs
No cache found, including on main, so every CI leg builds from scratch. TheTest - ubuntu-22.04leg of the last completed L1 run spent 29 minutes on that, and the L2 Dev integration test 44 minutes, of which ~36 was rustc (21 min building ethrex, 15.5 min building the test binary, 6.5 min actually testing).Description
The cache is never readable. An entry saved on a
refs/pull/N/mergeref can only be restored by that same PR;?ref=refs/heads/mainreturnscount=0, so there is nothing on a ref that other branches may read.setup-rustnow saves only on main.Nothing fits in the budget either. Counting matrix legs, 32
setup-rustcalls run on main across six workflows, and rust-cache keys per job, so that is 32 writers at 1.2 to 2.75 GB each. The default budget is 10 GB with LRU eviction (admin-configurable, billed above that);actions/cache/usagecurrently reports 17 entries totalling 31.37 GiB. Either way 32 writers do not coexist, so main would thrash exactly as PR refs do now.setup-rustgainscache-pool, forwarded to rust-cache'sshared-key, which replaces the per-job segment of the key. Four pools, each with exactly one writer, picked as the widest dependency set in it:l1pr-main_l1::testtoolingl1-reorgpr-main_l1::reorg-testsrelease-fastl2pr-main_l2::integration-test-F l2,l2-sqlat releaseprovermain_prover::testEverything else restores only, so a lean job cannot overwrite an entry with a subset and clippy-only target dirs never claim a slot.
reorg-testsgets its own pool because it is the only job on main building the root workspace atrelease-fast, which the dev-profilel1entry cannot serve. That is 6 entries (thel1writer is a 3-platform matrix) instead of 32.The second workspace was never cached. rust-cache's
workspacesdefaults to the repo root, andtoolingis a separate workspace with its own target dir, so the ef_tests, reorgs, loc and hive_report builds were all cold regardless of anything above. That includes a step inside thel1writer's own job, sincepr-main_l1::testrunsmake -C tooling/ef_tests/blockchain test.setup-rustnow lists both workspaces. Entries get bigger for it; the count does not change.Writing is opt-in:
save-cachedefaults to'false'and only the four jobs above pass'true'. It has to be opt-in rather than opt-out, becausescheduleandworkflow_dispatchhave no head ref of their own and fire on the default branch, sogithub.refisrefs/heads/mainandsave-ifevaluates true.daily_loc_reportanddaily_hive_reportwere writing thel1entry nightly on that path, anddeep_reorg,pr_lint_readme,pr_perf_levmandpr_perf_build_block_benchcould. Cache keys are immutable, so the first of those to save wins and the pool owner's richer save is skipped, not merged.The remaining PR-only workflows (
pr_loc,pr_check_l2_genesis,pr_perf_blocks_exec) get a pool name too. They restore only; naming a pool is what makes main's entry readable for them, at no storage cost.The L2 test binary was a near total duplicate of the node build. cargo fingerprints include the resolved feature set, and the two invocations diverged on 18 shared packages, among them sha3, tokio, hyper and idna, which sit low enough in the graph that the second build restarted from libc. Most of it came from
ethrex-test'sl2feature being an empty marker while the crate dev-depends onethrexwith default features, so cmd/ethrex and the whole L2 stack compiled twice in different configurations. Forwardingl2toethrex/l2andethrex/l2-sqlbrings the divergence down to 6 packages and makes cmd/ethrex resolve identically in both builds. What still rebuilds is the three cratesethrex-testturnstesting/test-utilson for, plus their dependents.Three other jobs (
main_prover, and two inpr-main_l2) prebuilt the test binary without--features l2and then ran with it. Harmless while the feature was empty, but now it flips the whole L2 stack config, so they get the flag too.The L2 build was hidden inside the backgrounded
cargo run. A compile error did not fail the job, it let the readiness loop poll for 25 min first, and the test binary was compiled while the node and prover were competing for the same 4 cores. Both binaries are now built in their own steps before the node starts, and the job usesrelease-fastinstead ofrelease. The test waits on 15s batch commits and L1 confirmations rather than on EVM throughput, so a slightly slower binary costs very little here.The deposit test raced the dev pre-funding. Removing the implicit sleep exposed a pre-existing bug. A node started with
--devfunds the rich accounts by depositing half of each L1 balance through the bridge, and the deployer awaits only the L1 receipts; the credits reach L2 through the L1 watcher a batch later. The port-3900 readiness gate opens ~20s before that, a gap the test binary's own compile time used to cover. So the suite read a zero initial L2 balance andtest_depositfailed by exactly the 5e26 wei pre-funding.l2_integration_testnow polls every test account's balance until it is non-zero before asserting any delta, which fixes localmake initruns too.Expect no improvement on this PR itself, since main has to run once to populate the pools.
Not in this PR
Jobs building the root workspace at plain
release(thepr_perf_*benchmarks,deep_reorg) have no writer on main, so they restore~/.cargobut no matching target dir. Same forrevm_comparison, which is excluded from the workspace and has a third target dir. Giving them writers costs entries, and the budget is not measurable from outside the org.aligned-sdkis a non-optional dependency ofethrex-l2and pulls the whole SP1 host stack (~4 min of the critical path) into every-F l2build even with no zkVM backend enabled. Gating it behind a feature needs config and release-plumbing changes.Checklist
STORE_SCHEMA_VERSION(crates/storage/lib.rs) if the PR includes breaking changes to theStorerequiring a re-sync. N/A: CI and feature wiring only.