build(l1): modernize Dockerfile with BuildKit + shrink context#6725
build(l1): modernize Dockerfile with BuildKit + shrink context#6725edg-l wants to merge 1 commit into
Conversation
|
1af5db8 to
c3de121
Compare
🤖 Claude Code ReviewPR Review:
|
🤖 Kimi Code ReviewThis PR optimizes Docker builds significantly, but has a few correctness and security issues to address. Critical Issues1. Missing
2. Unquoted shell variables (Dockerfile)
Security Concerns3. No checksum verification for downloaded binaries
Correctness & Robustness4. Incomplete tooling directory copy
5. Profile path assumption
Minor Improvements6. Python error handling
7. Docker buildx cache scope
VerdictThe optimizations are solid—particularly the Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
🤖 Codex Code Review
No EVM / consensus / trie / RLP logic changed in this diff, so I did not find Ethereum-protocol-specific issues here. I did not run a full build or tests. Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
Greptile SummaryThis PR modernises the L1
Confidence Score: 3/5The build-context and caching changes are correct, but the binary's embedded git SHA will be wrong on every build until VERGEN_GIT_SHA is wired up in the builder stage — a gap directly contradicting the PR's stated goal of consistent metadata. The VERGEN_GIT_SHA env var is never exported in the builder stage, so every image produced by this PR will embed a vergen placeholder SHA in the compiled binary while the OCI label carries the correct hash. This inconsistency affects observability and version tracing on every deployment. Additionally, cargo-binstall is pulled from an unpinned releases/latest URL, making builds non-reproducible and silently sensitive to upstream releases. The builder stage of Dockerfile (lines 48-93) needs the most attention — specifically the missing VERGEN_GIT_SHA env var and the unpinned cargo-binstall download. Makefile has a minor stamp-file dependency gap for tooling/repl and tooling/monitor.
|
| Filename | Overview |
|---|---|
| Dockerfile | Multi-stage BuildKit Dockerfile with cache mounts and slimmed context; VERGEN_GIT_SHA not forwarded to the builder stage so the SHA embedded in the binary by vergen will be incorrect, and cargo-binstall is fetched from an unpinned releases/latest URL. |
| .dockerignore | Adds exclusions for .git/, heavy fixture directories, and the previously missing vectors_zkevm/ (2.3 GB); cleanly addresses the build-context bloat described in the PR. |
| .github/actions/build-docker/action.yml | Injects GIT_SHA, GIT_BRANCH, and VERSION build args from GitHub context; logic is straightforward and correct. |
| .github/actions/snapsync-run/action.yml | Computes git metadata from local checkout and passes them as build args; consistent with the Makefile approach. |
| Makefile | Adds TAG/IMAGE variables and git-metadata args to the build rule; stamp-file dependencies don't include the newly-copied tooling/repl and tooling/monitor sources. |
| tooling/sync/docker_monitor.py | Adds a _git helper to derive SHA/branch/version from the local repo before calling docker build; approach mirrors the shell scripts in the GitHub Actions. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Build Context\n~9.5 MB after .dockerignore] --> B
subgraph B[rust:1.91-slim-bookworm — chef base]
B1[apt-get: build-essential, libclang-dev,\nlibssl-dev, pkg-config, curl]
B2[curl releases/latest → cargo-binstall\ncargo binstall cargo-chef]
B1 --> B2
end
B --> C & D
subgraph C[planner stage]
C1[COPY benches crates metrics cmd\ntest tooling/repl tooling/monitor\nCargo.toml Cargo.lock .cargo]
C2[cargo chef prepare → recipe.json]
C1 --> C2
end
subgraph D[builder stage]
D1[ARGs: PROFILE BUILD_FLAGS TARGETARCH\nGIT_BRANCH → ENV VERGEN_GIT_BRANCH\nVERGEN_IDEMPOTENT=1]
D2[COPY recipe.json from planner]
D3[cargo chef cook --profile PROFILE\ncache: cargo registry + git + target/]
D4[case TARGETARCH\narm64 → solc-linux-arm\namd64 → solc-linux]
D5[COPY source files + fixtures/genesis]
D6[cargo build --profile PROFILE\ncp target/PROFILE/ethrex → /ethrex/bin/]
D1 --> D2 --> D3 --> D4 --> D5 --> D6
end
C2 -->|recipe.json| D2
subgraph E[ubuntu:24.04 — runtime]
E1[ARGs: GIT_SHA VERSION\nLABEL OCI image metadata]
E2[apt-get: libssl3 ca-certificates]
E3[COPY ethrex binary from builder]
E4[ENTRYPOINT ethrex\nEXPOSE 8545 8551 30303 9090 1729 3900]
E1 --> E2 --> E3 --> E4
end
D6 -->|/ethrex/bin/ethrex| E3
style D1 fill:#ffe0e0,stroke:#cc0000
style B2 fill:#fff3cd,stroke:#856404
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
Dockerfile:54-56
`VERGEN_GIT_SHA` is never set as an env var in the builder stage, so with `.git` excluded from the build context, vergen-git2 falls back to its idempotent placeholder for the SHA field. Only the branch is correctly forwarded. The OCI label gets the right SHA, but the SHA embedded in the binary by vergen (and shown by `--version`) will be a placeholder string rather than the actual commit hash.
```suggestion
ARG GIT_BRANCH=unknown
ARG GIT_SHA=unknown
ENV VERGEN_GIT_BRANCH=$GIT_BRANCH \
VERGEN_GIT_SHA=$GIT_SHA \
VERGEN_IDEMPOTENT=1
```
### Issue 2 of 3
Dockerfile:20-22
The `cargo-binstall` binary is fetched from `releases/latest`, so every fresh CI build will silently upgrade to whatever the newest release is. This breaks build reproducibility and means a newly published (or compromised) version is picked up without any review. Pin to an explicit release tag so that the version can be bumped deliberately via a reviewed commit.
```suggestion
curl -fsSL https://github.com/cargo-bins/cargo-binstall/releases/download/v1.14.1/cargo-binstall-$(uname -m)-unknown-linux-musl.tgz \
| tar -xz -C /usr/local/cargo/bin \
&& cargo binstall --no-confirm cargo-chef
```
### Issue 3 of 3
Makefile:56
The stamp-file rule now triggers a rebuild only when `crates/` or `cmd/` Rust files change, but the new Dockerfile also copies `tooling/repl` and `tooling/monitor`. Changes to those directories won't invalidate the stamp, so `make build-image` will silently skip a needed rebuild.
```suggestion
$(STAMP_FILE): $(shell find crates cmd tooling/repl tooling/monitor -type f -name '*.rs') Cargo.toml Dockerfile
```
Reviews (1): Last reviewed commit: "build: modernize Dockerfile with BuildKi..." | Re-trigger Greptile
b84c334 to
626c2f2
Compare
- Add `# syntax=docker/dockerfile:1.10` and cache mounts for cargo
registry, git index, and target/ (survive across builds).
- Switch builder to rust:1.91-slim-bookworm; install cargo-chef via
cargo-binstall instead of source compile.
- Replace COPY .git with VERGEN_GIT_BRANCH from build ARG (drops 1.1 GB
from context). Add VERGEN_IDEMPOTENT=1 fallback.
- Use ARG TARGETARCH for solc download (multi-platform-ready).
- Only COPY fixtures/genesis (drops ~340 MB of unused test data).
- Use COPY --link throughout.
- Drop COPY cmd/ethrex/networks from runtime image; those JSONs are
include_str!-embedded at compile time.
- Add OCI image labels and GIT_SHA / GIT_BRANCH / VERSION build args.
- Expand .dockerignore: .git, fixtures/{blockchain,blobs,cache,rsp,hive},
tooling/ef_tests/blockchain/vectors_zkevm (2.3 GB).
- Makefile: add TAG variable (default `local`), pass GIT_SHA / GIT_BRANCH
/ VERSION; align run-image and localnet to use $(IMAGE).
- Wire the same build args through build-docker GH action, snapsync-run
action, and tooling/sync/docker_monitor.py.
626c2f2 to
3d3e9e7
Compare
|
Addressed bot reviews in
Skipped: |
Summary
Bring the Dockerfile up to current best practices and stop leaking gigabytes into the build context.
Build context size (vs
main).git/vergen-git2reads branch/SHA fromVERGEN_GIT_BRANCH/VERGEN_GIT_SHAenv (passed via build ARG)tooling/ef_tests/blockchain/vectors_zkevm/vectors/was excluded, this one was missed)tooling/*(non-repl/monitor)tooling/replandtooling/monitorare referenced as path deps by the root workspace'sCargo.toml. The rest oftooling/is a separate cargo workspace never used by theethrexbuild.fixtures/blockchain/fixtures/{blobs,cache,rsp,hive}/Image size
ethrex:main(current)ethrex:test(this PR)Savings come from the slim builder base, removing the redundant runtime copy of
cmd/ethrex/networks(those JSONs areinclude_str!-embedded in the binary), and--no-install-recommends+ apt cache mounts not leaking into the layer.Build speed
# syntax=docker/dockerfile:1.10+ BuildKit cache mounts on~/.cargo/registry,~/.cargo/git, andtarget/(survive across builds even whenrecipe.jsonchanges)./var/cache/aptand/var/lib/apt.cargo binstall cargo-chef(prebuilt binary) replacescargo installsource build, saves ~2 min cold.rust:1.91-slim-bookwormbuilder base (~700 MB smaller thanrust:1.91).COPY --linkthroughout: independent layers can be copied in parallel and their cache survives base-image / earlier-step changes.Reliability
CARGO_NET_GIT_FETCH_WITH_CLI=true+gitin the builder image. The slim base doesn't shipgit, so libgit2 was used by default and could hang inside the container on cold git-dep fetches. Forcing the CLI sidesteps the hang and uses the user's normal git behavior.Image / runtime
title,description,source,licenses,revision,version).ARG TARGETARCHdrives solc download (multi-platform-ready) instead of shelluname -m.ubuntu:24.04+libssl3running as root, to keep snapsync compose named-volume mounts working.Metadata wiring
GIT_SHA/GIT_BRANCH/VERSIONnow flow consistently through:Makefile(newTAGvariable; defaultlocal, override withmake build-image TAG=foo).github/actions/build-docker/action.yml(fromgithub.sha/github.head_ref || github.ref_name).github/actions/snapsync-run/action.yml(from local git)tooling/sync/docker_monitor.py(from local git)Result (verified locally):
Previously the image baked in whatever
vergen-git2read from.git; the metadata was correct but cost 1.1 GB of context per build.Test plan
make build-image TAG=testsucceeds locally; build context transferred ~9.5 MBdocker inspect ethrex:testshows realrevision+versionlabelsdocker run --rm ethrex:test --versionreports the correct branch + full SHA/dataas root (no perm regressions)