feat: upgrading for feynman - #1690
Conversation
Sorry, something went wrong.
WalkthroughThis update revises dependency versions for several zkVM prover workspace crates, restructures GPU and CPU build flows in the Makefile, introduces a new GPU-specific Cargo patch configuration, removes multiple configuration files and legacy scripts related to GPU repo cloning and patching, and updates the release download script to handle new versioning and download logic for required files. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Makefile
participant crates/gpu_override
participant crates/prover-bin
User->>Makefile: make prover
Makefile->>Makefile: Set ZK_VERSION (CPU version)
Makefile->>Makefile: Run prover_gpu target
Makefile->>crates/gpu_override: Build prover with GPU patches
Note right of Makefile: CPU build uses prover_cpu target (optional)
sequenceDiagram
participant User
participant download-release.sh
participant GitHub Releases
User->>download-release.sh: ./download-release.sh [version]
download-release.sh->>download-release.sh: Map version name to release version
download-release.sh->>download-release.sh: Create .work/chunk, .work/batch, .work/bundle
download-release.sh->>GitHub Releases: Download app.vmexe and openvm.toml for each circuit type
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review details Configuration used: CodeRabbit UI Reviewing files that changed from the base of the PR and between c8712d1 and 6fa1625. 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. ❤️ Share 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (.coderabbit.yaml)
Documentation and Community
|
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)crates/gpu_override/.cargo/config.toml (2)📜 Review detailszkvm-prover/Makefile (1)2-14: Commented-out patch block clutters the config.
If these overrides are no longer needed, delete them; if they are needed, ship them active and pin to a commit for reproducibility.
20-45: Many crates point to the same commit – consider workspace-level patch & a constant.
Duplicating the hash 18 times is error-prone. Move the override to the workspace root or use a Cargo workspace.dependencies table to reference a single variable.
zkvm-prover/download-release.sh (1)35-42: Dead/disabled GPU-selection logic – remove or revive.
The old ifeq (${PLONKY3_GPU_VERSION},) block is now commented. Keeping stale code hampers readability.
Cargo.toml (2)23-35: Three almost-identical wget blocks – loop it.
for part in chunk batch bundle; do dir=.work/$part mkdir -p "$dir" for file in app.vmexe openvm.toml; do wget -q "https://circuit-release.s3.us-west-2.amazonaws.com/scroll-zkvm/releases/$SCROLL_ZKVM_VERSION/$part/$file" \ -O "$dir/$file" done doneCuts duplication and makes future circuit additions trivial.
6-7: Workspace still lists crates/prover-bin but Makefile switched to gpu_override.
If prover-bin is obsolete, drop it and add crates/gpu_override to avoid unused-crate bloat and ensure cargo check covers the new code.
19-23: Revision bump LGTM, but pin by tag if possible.
Using raw commit hashes is fine short-term; long-term, tagged releases convey intent and survive force-pushes.
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Reviewing files that changed from the base of the PR and between a0ee508 and 0e1d3e4.
⛔ Files ignored due to path filters (1)- Cargo.lock is excluded by !**/*.lock
- Cargo.toml (1 hunks)
- crates/gpu_override/.cargo/config.toml (1 hunks)
- crates/prover-bin/.cargo/config.toml (0 hunks)
- zkvm-prover/.work/.gitignore (1 hunks)
- zkvm-prover/.work/batch/openvm.toml (0 hunks)
- zkvm-prover/.work/bundle/openvm.toml (0 hunks)
- zkvm-prover/.work/chunk/openvm.toml (0 hunks)
- zkvm-prover/Makefile (2 hunks)
- zkvm-prover/download-release.sh (1 hunks)
- zkvm-prover/.work/bundle/openvm.toml
- zkvm-prover/.work/chunk/openvm.toml
- zkvm-prover/.work/batch/openvm.toml
- crates/prover-bin/.cargo/config.toml
- GitHub Check: tests
zkvm-prover/.work/.gitignore (1)2-2: Good call on ignoring openvm.toml.
Keeps transient config out of source control.
Sorry, something went wrong.
|
bump version? |
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)build/dockerfiles/coordinator-api/config.toml (2)📜 Review details4-15: Absolute paths tie the build to a specific container layout
All patched crates reference /openvm-gpu/... (leading slash).
This:
- Breaks cargo check outside the Docker image;
- Makes future container re-organisation painful.
Prefer relative paths (e.g. path = "../../openvm-gpu/…" from the config file location) or anchor them via an env var ({ path = "${OPENVM_GPU_SRC}/crates/…" }) set in the Dockerfile.
58-58: Aliasing p3-monty-31 without features may disable SIMD/GPU paths
The upstream crate exposes optional feature flags (gpu, simd, etc.).
By patching it with no default-features stanza you inherit whatever the crate’s defaults are—which could differ between upstream tags and break determinism.
Explicitly include the expected feature set, e.g.:-p3-monty-31 = { path = "/plonky3-gpu/monty-31" } +p3-monty-31 = { path = "/plonky3-gpu/monty-31", default-features = false, features = ["gpu"] }Also applies to: 87-87
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Reviewing files that changed from the base of the PR and between 0e1d3e4 and 654cd42.
📒 Files selected for processing (3)- build/dockerfiles/coordinator-api/checkout_all.sh (1 hunks)
- build/dockerfiles/coordinator-api/config.toml (5 hunks)
- crates/gpu_override/.cargo/config.toml (1 hunks)
- build/dockerfiles/coordinator-api/checkout_all.sh
- crates/gpu_override/.cargo/config.toml
- GitHub Check: tests
build/dockerfiles/coordinator-api/config.toml (1)44-46: nightly-features enabled – ensure the toolchain is pinned
The p3-baby-bear patch now opts into "nightly-features".
Compilation will fail on stable Rust. Double-check that:
- A rust-toolchain.toml (or rust-toolchain) at workspace root pins an appropriate nightly;
- CI images/install scripts install the same nightly.
If this is accidental, drop the feature flag.
Also applies to: 73-75
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)build/dockerfiles/coordinator-api.Dockerfile (1)📜 Review details42-47: Slim-down the final image & keep security hygiene
Installing vim and leaving APT caches bloats the runtime image by hundreds of MB. Use --no-install-recommends and clean the cache.-RUN apt update && apt install vim netcat-openbsd net-tools curl jq -y +RUN apt-get update \ + && DEBIAN_FRONTEND=noninteractive apt-get install --no-install-recommends -y \ + netcat-openbsd net-tools curl jq \ + && rm -rf /var/lib/apt/lists/*
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Reviewing files that changed from the base of the PR and between 5667d0b and 8b62b67.
📒 Files selected for processing (8)- .github/workflows/docker.yml (0 hunks)
- build/dockerfiles/coordinator-api.Dockerfile (2 hunks)
- build/dockerfiles/coordinator-api/checkout_all.sh (0 hunks)
- build/dockerfiles/coordinator-api/clone_openvm_gpu.sh (0 hunks)
- build/dockerfiles/coordinator-api/clone_openvm_stark_gpu.sh (0 hunks)
- build/dockerfiles/coordinator-api/clone_plonky3_gpu.sh (0 hunks)
- build/dockerfiles/coordinator-api/config.toml (0 hunks)
- build/dockerfiles/coordinator-api/gitconfig (0 hunks)
- build/dockerfiles/coordinator-api/gitconfig
- build/dockerfiles/coordinator-api/clone_openvm_stark_gpu.sh
- build/dockerfiles/coordinator-api/checkout_all.sh
- build/dockerfiles/coordinator-api/clone_plonky3_gpu.sh
- build/dockerfiles/coordinator-api/clone_openvm_gpu.sh
- .github/workflows/docker.yml
- build/dockerfiles/coordinator-api/config.toml
[error] 3-3: Use absolute WORKDIR
(DL3000)
⏰ Context from checks skipped due to timeout of 90000ms (10)- GitHub Check: bridgehistoryapi-db-cli
- GitHub Check: coordinator-cron
- GitHub Check: coordinator-api
- GitHub Check: bridgehistoryapi-api
- GitHub Check: gas_oracle
- GitHub Check: bridgehistoryapi-fetcher
- GitHub Check: rollup_relayer
- GitHub Check: rollup-db-cli
- GitHub Check: blob_uploader
- GitHub Check: tests
build/dockerfiles/coordinator-api.Dockerfile (1)42-50: Double-check glibc compatibility after switching to Ubuntu 20.04
libzkp.so is compiled in the builder (Ubuntu 22.04 base image). Running it on older glibc (20.04 ships 2.31) can cause symbol-version errors. Please verify at runtime or align both stages to the same distro level.
Sorry, something went wrong.
This PR would trace the upgrades in the progress of feynman integration
Summary by CodeRabbit