ci: download latest leanSpec fixture release instead of generating fixtures#385
Conversation
Greptile SummaryThis PR replaces in-CI fixture generation (checkout +
Confidence Score: 3/5Not safe to merge as-is: the Makefile SHA check silently passes on mismatch, and the CI cache-save logic can persist bad fixture state under a valid cache key. The Makefile's integrity check — the main new safety mechanism — does nothing on failure because Both
|
| Filename | Overview |
|---|---|
| .github/workflows/ci.yml | Replaces fixture generation with download-and-verify from leanSpec releases; SHA used for cache key is fetched separately from the one used during verification (TOCTOU risk), and the always() cache save can poison the cache if the download step fails. |
| Makefile | SHA integrity check on line 40 is a no-op because ; does not propagate failure — a tampered or corrupted archive is silently extracted; leanSpec target is now dead code with no prerequisite relationship to leanSpec/fixtures. |
| CLAUDE.md | Documentation updated to reflect that fixtures are now downloaded from releases rather than regenerated locally — straightforward and accurate. |
Comments Outside Diff (1)
-
.github/workflows/ci.yml, line 85-90 (link)Cache save with
always()can poison the cache on download failureThe save step runs under
always() && steps.cache-fixtures.outputs.cache-hit != 'true', meaning it fires even when the "Download leanSpec fixtures release" step fails (e.g. network error, SHA mismatch). At that pointleanSpec/fixtureseither doesn't exist or holds stale content, but the save stores it under the SHA-keyed cache entry. On the next run the cache key matches (the SHA file hasn't changed), the restore step reports a hit, the download is skipped, and tests run against empty or stale fixtures without any error about the fixture state. The old workflow usedcache-hit == 'false'specifically to avoid this. Consider gating the save on the download step succeeding.Prompt To Fix With AI
This is a comment left during a code review. Path: .github/workflows/ci.yml Line: 85-90 Comment: **Cache save with `always()` can poison the cache on download failure** The save step runs under `always() && steps.cache-fixtures.outputs.cache-hit != 'true'`, meaning it fires even when the "Download leanSpec fixtures release" step fails (e.g. network error, SHA mismatch). At that point `leanSpec/fixtures` either doesn't exist or holds stale content, but the save stores it under the SHA-keyed cache entry. On the next run the cache key matches (the SHA file hasn't changed), the restore step reports a hit, the download is skipped, and tests run against empty or stale fixtures without any error about the fixture state. The old workflow used `cache-hit == 'false'` specifically to avoid this. Consider gating the save on the download step succeeding. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 4 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 4
Makefile:40-43
**SHA check doesn't abort on mismatch**
The test `[ "$$expected" = "$$actual" ]` on line 40 is followed by `;` rather than `||` or `&&`, so its exit status is thrown away. In Make, the entire recipe runs in a single shell, and `;` does not propagate failure — the subsequent `rm -rf`, `mkdir -p`, and `tar` commands execute unconditionally regardless of whether the check passes. A mismatched or actively tampered archive is silently extracted. The CI workflow correctly uses `if [ ... ]; then exit 1; fi` but this Makefile equivalent does nothing.
### Issue 2 of 4
.github/workflows/ci.yml:85-90
**Cache save with `always()` can poison the cache on download failure**
The save step runs under `always() && steps.cache-fixtures.outputs.cache-hit != 'true'`, meaning it fires even when the "Download leanSpec fixtures release" step fails (e.g. network error, SHA mismatch). At that point `leanSpec/fixtures` either doesn't exist or holds stale content, but the save stores it under the SHA-keyed cache entry. On the next run the cache key matches (the SHA file hasn't changed), the restore step reports a hit, the download is skipped, and tests run against empty or stale fixtures without any error about the fixture state. The old workflow used `cache-hit == 'false'` specifically to avoid this. Consider gating the save on the download step succeeding.
### Issue 3 of 4
.github/workflows/ci.yml:50-61
**TOCTOU between SHA fetch for cache key and actual download**
The SHA is fetched once in "Get leanSpec fixtures SHA" (to build the cache key) and then the `.sha256` file is fetched again inside "Download leanSpec fixtures release" for verification. If a new leanSpec release lands between these two steps, the cache key reflects the old SHA while the downloaded artifacts belong to the new release. The integrity check still passes (both files are self-consistent from the new release), but the fixtures are then saved under the old SHA key, which will never match on the next run, causing an unnecessary re-download every time a release ships during a CI run.
### Issue 4 of 4
Makefile:30-31
**`leanSpec` target is now dead code**
`leanSpec/fixtures` was updated to no longer depend on the `leanSpec` prerequisite, so the `leanSpec` clone target is never invoked by any downstream rule. The `git checkout` line was also dropped in this PR, leaving the target in a half-refactored state (it clones but doesn't pin a revision). If nothing depends on it, consider removing it to avoid confusion.
Reviews (1): Last reviewed commit: "ci: download latest leanSpec fixture rel..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| leanSpec: | ||
| git clone https://github.com/leanEthereum/leanSpec.git --single-branch |
There was a problem hiding this comment.
leanSpec target is now dead code
leanSpec/fixtures was updated to no longer depend on the leanSpec prerequisite, so the leanSpec clone target is never invoked by any downstream rule. The git checkout line was also dropped in this PR, leaving the target in a half-refactored state (it clones but doesn't pin a revision). If nothing depends on it, consider removing it to avoid confusion.
Prompt To Fix With AI
This is a comment left during a code review.
Path: Makefile
Line: 30-31
Comment:
**`leanSpec` target is now dead code**
`leanSpec/fixtures` was updated to no longer depend on the `leanSpec` prerequisite, so the `leanSpec` clone target is never invoked by any downstream rule. The `git checkout` line was also dropped in this PR, leaving the target in a half-refactored state (it clones but doesn't pin a revision). If nothing depends on it, consider removing it to avoid confusion.
How can I resolve this? If you propose a fix, please make it concise.
|
We might want to wait for devnet 5 to be merged to merge this. Otherwise, we'll have devnet 4 code running devnet 5 tests |
LFG |
|
@dicethedev can you please solve the conflicts? |
You can review again @pablodeymo |
|
@dicethedev there are new conflicts... |
|
Also, this job is failing: https://github.com/lambdaclass/ethlambda/actions/runs/26908418008/job/79379802140?pr=385 |
Resolve the Makefile conflict in favor of the PR's release-download approach while keeping main's new shadow-build targets. Also clean up leftovers from the previous merge of main that broke the PR's intent: - Drop the duplicate leanSpec/fixtures fill recipe that overrode the download recipe (GNU Make uses the last definition), along with the now-unused LEAN_SPEC_COMMIT_HASH pin, leanSpec clone target, and the stale leanSpec PR #745 keys workaround comment. - Remove orphaned production-keys hash lines from the CI download step (they referenced a deleted step id and ran uv outside any project), the dead production-keys cache steps, and the old fixture-generation step. Gate the fixtures cache save on the download step's outcome so a failed download never persists a partial fixture set.
actionlint flagged the release-info step for using three individual appends to $GITHUB_OUTPUT; write them in a single block redirect instead.
Thank you Mega! |
|
@dicethedev we're making some quick changes here before merging. CI times were taking too long so this PR is now a priority, sorry. |
…429) ## Motivation #385 switched CI to download the latest leanSpec fixtures release instead of generating them from a pinned commit. The current release carries fixture-schema changes the test runners don't understand yet, so the Test job on `main` fails. Note: leanSpec's release is a single rolling `latest` tag — it was republished mid-review (2026-06-10 17:31 UTC), which is what the second commit addresses. ## Description Runner/harness-only changes — no consensus behavior is touched, and ethlambda already matches the spec in every affected case: - **`tickToSlot` (fork-choice block steps):** new flag, default `true`. The early-arrival tests set it to `false` to deliver a block ahead of the store clock and assert the clock doesn't move. The forkchoice spectest runner and the Hive test driver now only tick to the block's slot when the flag is set. - **`rejectionReason`:** leanSpec renamed `expectException`. The `verify_signatures` and `state_transition` fixture types accept both spellings via a serde alias. - **`ssz_test`:** the SSZ fixture format string was renamed from `ssz`; the runner accepts both. - **`proofSetting` / mocked proofs:** leanSpec now fills most vectors with placeholder aggregation proofs (`MOCKED-AGGREGATION-PROOF` sentinel) instead of paying for recursive SNARK merges, marking the regime in a top-level `proofSetting` field (0 = mocked, must not be verified; 1 = real and valid; 2 = real and invalid). The forkchoice runner routes mocked vectors through a new `on_gossip_aggregated_attestation_without_verification` (mirroring the `on_block`/`on_block_without_verification` split). Production paths always verify. Known follow-ups (out of scope): - The Hive test driver receives fork-choice steps one at a time with no fixture-level context, so it cannot honor `proofSetting` yet; Hive runs against mocked-proof fixtures will need a protocol addition. - Tracking the rolling `latest` release means any upstream republish can break `main`'s CI at any time; pinning the fixtures tarball SHA256 would restore reproducibility. ## How to Test ``` make test # downloads latest release fixtures, all suites green ``` Locally verified against the 2026-06-10 17:31 UTC fixtures drop: full `cargo test --workspace --release` passes (108 forkchoice, 118 SSZ, 67 STF, all remaining suites green).
3 participants
🗒️ Description / Motivation
This PR updates the CI and test fixture workflow so leanSpec fixtures are downloaded from the published leanSpec release assets instead of being generated in CI from a pinned checkout.
The repository now publishes the latest production fixtures as release assets, so CI should consume those directly rather than regenerating them.
This improves:
It also removes the overhead and complexity of generating fixtures during CI runs.
What Changed
ci.ymlRemoved:
Added:
fixtures-prod-scheme.tar.gzleanSpec/fixturesMakefileUpdated the
leanSpec/fixturestarget:uv run fill ...fixture generation flowCLAUDE.mdUpdated development documentation to reflect the new fixture workflow:
Correctness / Behavior Guarantees
Preserved Invariants
leanSpec/fixturesmake testbehavior remains unchangedBehavior Changes
Reviewer Notes
make leanSpec/fixturesworkflowTests Added / Run
No new unit or integration tests were added.
Verification Performed
make -n leanSpec/fixturesci.ymlRelated Issues / PRs
✅ Verification Checklist
make fmt— cleanmake lint(clippy -D warnings) — cleancargo test --workspace --release— all passing