ci: migrate Catalyst workflows to Taiko - #1
Conversation
Two review findings from #1. 1. `BLST_PORTABLE` was inert. `blst` selects portable mode from the Cargo feature `portable` (build.rs:160); the only environment variable its build script reads is `BLST_TEST_NO_STD`. The `ARG`/`--build-arg` pair therefore did nothing except suppress BuildKit's "build args were not consumed" warning, which made a no-op look plumbed through. Builds kept falling through to host CPU detection and baked ADX/BMI2-only assembly into the published amd64 image, which SIGILLs on pre-Broadwell/pre-Zen hosts. `blst` reaches this workspace through `c-kzg`, whose `portable` feature forwards to `blst/portable`. Every existing dependent (alloy-eips, alloy-consensus, revm-precompile) takes c-kzg with default-features = false, so the feature was off. Declare c-kzg directly from `common` with that one feature and let feature unification carry it to the single `blst` node. Verified with `cargo tree -p node -e features -i blst`: common -> c-kzg feature "portable" -> blst feature "portable". Lockfile impact is one line; c-kzg stays at 2.1.7 and blst at 0.3.16. The Dockerfile ARG and both build-args blocks are removed, since the control surface is now the Cargo feature. 2. The retired p2p_node_docker_build.yml left tools/p2p_boot_node/ with no image pipeline, while simple-taiko-node-nethermind's docker-compose.yml pulls nethermind/catalyst-p2p-bootnode:latest and four services depend on it. Nothing goes red -- E2E would silently keep testing a frozen Nethermind build. Matrix the validate/publish/merge jobs over image so the bootnode is built and published alongside the node. Repointing the compose file to GAR is a follow-up in that repo. Also adds tools/p2p_boot_node/Dockerfile to the path filters (the bare "Dockerfile" entry only matches the repo root) and records why `paths` is safe to combine with tag pushes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
kimo-ice
left a comment
There was a problem hiding this comment.
Review
The Rust CI half is solid and proven green. The Docker half is the concern: it has never executed, and the PR body describes safeguards that aren't in the diff.
Blocking
1. The PR-time Docker validation described in the summary does not exist.
The summary says "Validate Docker builds without credentials on pull requests." There is no such job. docker-build.yml triggers only on push to master, v* tags, and workflow_dispatch — no pull_request. And ci.yml has exactly five jobs: fmt, clippy, typos, test, dependency-audit. No Docker job anywhere.
Consequence: the entire publish path — WIF auth, per-arch digest export, artifact round-trip, imagetools create — first executes on a master push, against the shared production registry. Combined with the PR's own note that two local full builds never got past the Optimism git fetch, nothing has compiled this Dockerfile end to end. Suggest adding a pull_request job with push: false, no auth, amd64 only.
2. Three more description ↔ implementation mismatches. Reviewers approve off the summary, so these matter:
| Summary says | Diff does |
|---|---|
| "GitHub-hosted runners" | self-hosted arc-runner-set / arc-runner-set-arm64 |
| "main-node-only Docker workflow" | matrix also builds + publishes catalyst-p2p-bootnode |
"Make BLST_PORTABLE an effective Docker build argument" |
removes the build-arg entirely in favor of a Cargo feature |
High
3. p2p-boot-node scope contradiction. ci.yml excludes it from clippy as "outside the maintained main-node scope" — yet docker-build.yml publishes catalyst-p2p-bootnode to GAR on every master push and tag. Meanwhile cargo test --workspace does not exclude it. So a shipped production image is compiled and tested but deliberately not linted. Either drop it from the publish matrix or drop the clippy exclusion.
4. No build cache on the publish jobs. Four cold full-workspace Rust builds (2 images × 2 arches, 120-min timeouts) per qualifying master push, on self-hosted ARC. The old workflow was gated by if: github.repository == 'NethermindEth/Catalyst', so it never ran in this fork — this PR turns these on for the first time. Worth adding cache-from/cache-to, and considering the old per-image paths scoping so a node-only change doesn't rebuild the bootnode.
Verified correct
The BLST portability fix is real, and better than the description suggests. Checked both halves:
blst0.3.xbuild.rspicks portable mode purely fromcfg!(feature = "portable")— it never reads aBLST_PORTABLEenv var. Master'sDockerfilealso never declaredARG BLST_PORTABLE, so the oldbuild-args: BLST_PORTABLE=1was doubly a no-op.cargo tree -e features -i c-kzgon this branch showsc-kzg feature "portable"enabled by exactly one crate:common. alloy-consensus/alloy-eips pull c-kzg withdefault-features = false, soportablewas genuinely off before. Release images really were baking in host-detected ADX/BMI2 — the SIGILL risk in the comment is not hypothetical.
One trade-off not stated: portable is now global, so BLS/KZG gives up ADX/MULX on modern hosts too. Right call for portable release images, but worth making conscious.
WIF config is sound. Provider and service account are byte-identical to taiko-mono's working taiko-client-rs--docker.yml, and every action major version (checkout@v7, upload-artifact@v7, download-artifact@v8, login@v4, buildx@v4, build-push@v7, metadata@v6, auth@v3) matches what taiko-mono runs today. arc-runner-set-arm64 is in active use there. The open question is only whether the provider's attribute condition admits taikoxyz/Catalyst.
E2E workflows are unaffected. They docker buildx build --load locally and use nethermind/catalyst-node only as a local tag — they never pull from Docker Hub. Retiring the Docker Hub publish doesn't break them.
Medium / nits
- Docker Hub publishing is dropped silently.
nethermind/catalyst-nodewas public; GAR is not. Any downstream consumer pulling from Docker Hub will silently pin to a stale image. Should be an announced break. mergejob fragility: ifdownload-artifactmatches nothing,${{ runner.temp }}/digestsmay not exist and the step dies onworking-directorybefore the script's own empty-digest guard runs.mkdir -pfirst.cancel-in-progress: trueon a publish workflow can kill a run between digest push and manifest create, orphaning untagged digests in GAR. Preferfalsehere.- CI runs on draft PRs —
opened/synchronizefire beforeready_for_review, so drafts burn self-hosted runner time. Add adraft == falseguard if that's a concern. rustup showas installer in.github/actions/setup-rust/action.yml: works today (CI is green, and the p2p Dockerfile does the same), but auto-install onshowis deprecated upstream.rustup toolchain installis future-proof.- Scope creep in
Cargo.lock: ruint 1.18→1.20 pulls in six newark-*0.6.0 crates. Audit-justified, but a dep-graph change riding in a CI PR deserves a line in the description. - CODEOWNERS is still Nethermind's — global owners
@mskrzypkows @mikhailUshakoff, and*docker*.yml→@nmjustinchan @derrix060. Follow-up for a "migrate to Taiko" effort, out of scope here. masterhas no branch protection, so none of these checks are required to merge — deleting the old workflows breaks nothing, but nothing enforces the new ones either.
Two review findings from #1. 1. `BLST_PORTABLE` was inert. `blst` selects portable mode from the Cargo feature `portable` (build.rs:160); the only environment variable its build script reads is `BLST_TEST_NO_STD`. The `ARG`/`--build-arg` pair therefore did nothing except suppress BuildKit's "build args were not consumed" warning, which made a no-op look plumbed through. Builds kept falling through to host CPU detection and baked ADX/BMI2-only assembly into the published amd64 image, which SIGILLs on pre-Broadwell/pre-Zen hosts. `blst` reaches this workspace through `c-kzg`, whose `portable` feature forwards to `blst/portable`. Every existing dependent (alloy-eips, alloy-consensus, revm-precompile) takes c-kzg with default-features = false, so the feature was off. Declare c-kzg directly from `common` with that one feature and let feature unification carry it to the single `blst` node. Verified with `cargo tree -p node -e features -i blst`: common -> c-kzg feature "portable" -> blst feature "portable". Lockfile impact is one line; c-kzg stays at 2.1.7 and blst at 0.3.16. The Dockerfile ARG and both build-args blocks are removed, since the control surface is now the Cargo feature. 2. The retired p2p_node_docker_build.yml left tools/p2p_boot_node/ with no image pipeline, while simple-taiko-node-nethermind's docker-compose.yml pulls nethermind/catalyst-p2p-bootnode:latest and four services depend on it. Nothing goes red -- E2E would silently keep testing a frozen Nethermind build. Matrix the validate/publish/merge jobs over image so the bootnode is built and published alongside the node. Repointing the compose file to GAR is a follow-up in that repo. Also adds tools/p2p_boot_node/Dockerfile to the path filters (the bare "Dockerfile" entry only matches the repo root) and records why `paths` is safe to combine with tag pushes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
docker/metadata-action defaults to `flavor: latest=auto`, under which
`type=ref,event=tag` sets the `latest` flag on every tag push. Tag rules
are processed in priority order (ref=600 before raw=200), and setVersion
only assigns `version.latest` when it is still undefined, so the first
rule wins: a `v*` push produced `v1.2.3`, `sha-xxxxxxx` and `latest`.
The `enable={{is_default_branch}}` guard did not prevent this — it is
evaluated against `refs/heads/`, so it is simply false on a tag ref and
that rule is skipped entirely rather than constraining `latest`.
Consequence: cutting a patch release off an older line after a newer
minor had shipped would roll `latest` backwards for every consumer.
Pin `latest=false` so the tag comes only from the explicit `type=raw`
rule. `master` still publishes `latest` (a literal raw tag value, which
the flavor flag does not gate); `v*` now publishes only the version and
sha tags.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The docker workflow ran a full credential-free image build on every pull request that touched any `.rs` or `Cargo.toml` file, which is nearly all of them. That cost ~18 minutes per PR for catalyst-node and gated nothing that the master build would not catch. Drop the `pull_request` trigger and the PR-only `validate` job with it. Replace the `github.event_name != 'pull_request'` guards on `publish` and `merge` with an explicit ref guard. Those guards were about to become tautologies once the PR trigger was gone, and they did not constrain `workflow_dispatch`: a dispatch from any feature branch would authenticate via WIF and push to the shared registry. Gating on the ref keeps dispatch usable on master and release tags while closing that path, and it holds even if a pull_request trigger is reintroduced later. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Run CI and image builds on the self-hosted ARC pool that taiko-mono already uses (`arc-runner-set`, `arc-runner-set-arm64`) instead of GitHub-hosted runners. A label swap alone is not enough: the ARC images are minimal and ship neither rustup nor a C toolchain, so `rustup show` would fail outright and anything compiling the workspace would fail in bindgen for lack of libclang (reth's MDBX bindings). Add a local composite action that installs both, mirroring the preparation steps taiko-mono repeats inline in its Rust workflows. Every step short-circuits when the tool is already present, so this stays correct if the runner image gains them later. Jobs that never compile the workspace (fmt, dependency-audit) skip the apt install via `native-deps: "false"`. Keep rust-toolchain.toml as the single source of truth for the channel and components: bootstrap rustup with `--default-toolchain none` and let `rustup show` resolve it, rather than duplicating the version in the workflow the way `dtolnay/rust-toolchain` would require. Also disable the buildx binary cache on the docker jobs, matching taiko-mono's configuration for this pool, and declare the custom runner labels so actionlint stops rejecting them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`crate-ci/typos` shells out to wget from its entrypoint, and the ARC images ship curl but not wget, so the job died with exit 127 before checking anything. Install the binary through taiki-e/install-action, matching how this workflow already installs cargo-deny, and invoke `typos` directly. The CLI reads _typos.toml by itself, so nothing the action provided is lost, and this also matches the command CLAUDE.md documents for local runs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
h2 before 0.4.16 queues empty DATA frames without limit, so a peer that does not drain streams can drive unbounded memory growth or a panic on length overflow. Low severity, reached transitively through hyper -> alloy-transport-http -> alloy. This landed in the advisory database between two CI runs of the same tree: dependency-audit passed, then failed on the next run with no source change. That is the expected behaviour for a time-based check, and the reason this job runs on every push rather than only when Cargo.lock moves. `cargo update -p h2 --precise 0.4.16` also reshuffles which duplicate version of windows-sys, socket2, and syn a few dependents point at. That churn is not avoidable from the CLI -- the identical lock results with and without --precise -- and it is confined to reassignment among versions already present in the graph. `cargo metadata --locked` still resolves. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Addresses review findings on the CI migration. - Add a pull_request validation matrix that builds both Dockerfiles with no cloud auth and no registry write. `cargo test` compiles the crates but never exercises either Dockerfile, so base-image, package-install and artifact-copy failures previously reached master and first surfaced in the publish job. - Add `**/src/**/*.json` to both path filters. `sol!` reads the ABI JSON at compile time (pacaya/src/l1/bindings.rs and friends), so an ABI-only change altered the binary without triggering a rebuild, leaving `latest` serving code generated from the previous ABI. - Add a gha BuildKit cache scoped per image and arch. The Dockerfile's cache mounts live in the ephemeral ARC builder, so every qualifying push ran four cold full-workspace builds against a 120-minute timeout. PR builds read the cache but never write it. - Make cancel-in-progress apply to pull_request runs only. Publishing pushes per-arch digests before creating the manifest that references them, so cancelling in between orphaned untagged digests in GAR. - Move the empty-digest guard off `working-directory`. download-artifact does not create the path when its pattern matches nothing, so the runner failed to enter the directory before the guard could report why. - Probe pkg-config and clang in setup-rust, not just cc and llvm-config. A runner with a compiler and LLVM but no pkg-config or libclang short-circuited the install and failed later inside bindgen.
fd11dc9 to
4edaba1
Compare
Summary
us-docker.pkg.dev/evmchain/images/catalyst-node.linux/amd64andlinux/arm64images by digest and merge them into one manifest onmasterandv*tags.BLST_PORTABLEan effective Docker build argument and updateCargo.lockto patchedanyhow,crossbeam-epoch, andruintreleases required by the dependency audit.Validation
cargo fmt --all -- --checkcargo clippy --workspace --all-features --exclude p2p-boot-node --locked -- -D warningscargo test --workspace --locked --verbose(129 unit tests plus doc tests)typos .cargo deny check(passes; retains one existing transitive yankedspin 0.9.8warning)actionlint .github/workflows/*.ymldocker buildx build --call=check --file Dockerfile .Before merge
taikoxyz/Catalystandgar-github-action@evmchain.iam.gserviceaccount.comcan writeus-docker.pkg.dev/evmchain/images/catalyst-node.