feat: coordinator and prover support v0.4.2#1660
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the """ WalkthroughThis update transitions the build and workflow processes to support GPU-accelerated dependencies and multi-key SSH access. Dockerfiles now use CUDA-enabled images, and new scripts manage cloning and fetching GPU-related repositories. Dependency configurations are patched to use GPU forks with added features, and workflow scripts are refactored for handling multiple SSH keys and coordinated checkouts. Additionally, configuration structs were simplified by removing low version circuit parameters, and the version string was incremented. Changes
Sequence Diagram(s)sequenceDiagram
participant Workflow
participant SSHAgent
participant RepoScripts
participant GitHub
Workflow->>SSHAgent: Create .ssh directory, set permissions
SSHAgent->>SSHAgent: Start ssh-agent and add multiple keys
SSHAgent->>GitHub: Add GitHub RSA host key to known_hosts
Workflow->>RepoScripts: Run clone scripts for each GPU repo
RepoScripts->>GitHub: Clone or fetch repositories via SSH
Workflow->>RepoScripts: Run checkout_all.sh to checkout fixed commits
Poem
""" 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. 🪧 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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1660 +/- ##
===========================================
- Coverage 40.76% 40.70% -0.06%
===========================================
Files 225 225
Lines 18448 18448
===========================================
- Hits 7520 7509 -11
- Misses 10194 10209 +15
+ Partials 734 730 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
build/dockerfiles/coordinator-api/checkout_all.sh (1)
8-18: 🛠️ Refactor suggestionEnsure safe directory navigation and quoting
Wrap$DIRexpansions in quotes and validate each path beforecdto give clearer errors. For example:- cd $DIR/plonky3-gpu && git checkout ${PLONKY3_GPU_COMMIT} + if [[ -d "$DIR/plonky3-gpu" ]]; then + cd "$DIR/plonky3-gpu" + git fetch --all --prune + git checkout "$PLONKY3_GPU_COMMIT" + else + echo "ERROR: Directory $DIR/plonky3-gpu not found" + exit 1 + fiThis pattern can be repeated for the other two repositories.
♻️ Duplicate comments (1)
.github/workflows/docker.yml (1)
323-331: Remove redundantssh-agentinvocations
After consolidating agent setup, simplyssh-addthe additional keys without re-runningeval.
🧹 Nitpick comments (11)
build/dockerfiles/coordinator-api/clone_openvm_gpu.sh (1)
6-10: Quote path variables and fetch tags explicitly
For robustness and to ensurecheckout_all.shcan use tags, wrap$DIRin quotes and include--tagswhen fetching:-if [ ! -d $DIR/openvm-gpu ]; then - git clone git@github.com:scroll-tech/openvm-gpu.git $DIR/openvm-gpu -fi -cd $DIR/openvm-gpu && git fetch --all --force +if [ ! -d "$DIR/openvm-gpu" ]; then + git clone git@github.com:scroll-tech/openvm-gpu.git "$DIR/openvm-gpu" +fi +cd "$DIR/openvm-gpu" && git fetch --all --force --tagsbuild/dockerfiles/coordinator-api/clone_plonky3_gpu.sh (1)
1-11: Apply the same robustness enhancements as other clone scripts
- Quote
$DIRexpansions.- Fetch tags to support downstream checkouts.
-if [ ! -d $DIR/plonky3-gpu ]; then - git clone git@github.com:scroll-tech/plonky3-gpu.git $DIR/plonky3-gpu -fi -cd $DIR/plonky3-gpu && git fetch --all --force +if [ ! -d "$DIR/plonky3-gpu" ]; then + git clone git@github.com:scroll-tech/plonky3-gpu.git "$DIR/plonky3-gpu" +fi +cd "$DIR/plonky3-gpu" && git fetch --all --force --tagsbuild/dockerfiles/coordinator-api/clone_openvm_stark_gpu.sh (1)
1-11: Mirror fetch/tag and quoting best practices
Consistently wrap$DIRvariables and fetch tags so that the subsequentcheckout_all.shcan reference the correct tag commits:-if [ ! -d $DIR/openvm-stark-gpu ]; then - git clone git@github.com:scroll-tech/openvm-stark-gpu.git $DIR/openvm-stark-gpu -fi -cd $DIR/openvm-stark-gpu && git fetch --all --force +if [ ! -d "$DIR/openvm-stark-gpu" ]; then + git clone git@github.com:scroll-tech/openvm-stark-gpu.git "$DIR/openvm-stark-gpu" +fi +cd "$DIR/openvm-stark-gpu" && git fetch --all --force --tags.github/workflows/common.yml (1)
45-62: Securely handle SSH private keys
Writing all keys to a single disk file leaves sensitive material lingering on the runner. After adding the keys tossh-agent, remove or shred the file immediately:- ssh-add ~/.ssh/all_keys 2>/dev/null + ssh-add ~/.ssh/all_keys 2>/dev/null + shred --remove ~/.ssh/all_keysAlternatively, add each key directly from stdin without persisting to disk:
for key in \ "${{ secrets.OPENVM_GPU_SSH_PRIVATE_KEY }}" \ "${{ secrets.OPENVM_STARK_GPU_SSH_PRIVATE_KEY }}" \ "${{ secrets.PLONKY3_GPU_SSH_PRIVATE_KEY }}"; do ssh-add <(printf "%s\n" "$key") donebuild/dockerfiles/coordinator-api/checkout_all.sh (1)
4-7: Pin GPU repository commits
Defining explicit commit hashes ensures reproducible builds. Consider adding a comment or link to upstream release notes or PRs for context.build/dockerfiles/coordinator-api.Dockerfile (4)
2-2: Update builder base image with CUDA support
Switching toscrolltech/cuda-go-rust-builder:cuda-11.7.1-go-1.21-rust-nightly-2023-12-03aligns with GPU requirements. For stronger immutability, consider pinning the image by digest.
12-15: Include GPU-enabled repositories in build context
Copyingplonky3-gpuandopenvm-stark-gpuensures the patched source is available. To reduce image layers, you might combine these COPY commands:COPY ./build/dockerfiles/coordinator-api/{plonky3-gpu,openvm-stark-gpu,openvm-gpu} /Ensure these dirs are populated by your clone scripts before this stage.
25-25: Consistent CUDA builder for Go modules
Mirroring the CUDA-enabled builder for the Go dependency download stage is correct. As with the chef stage, pin to a digest if possible.
45-45: Use CUDA runtime base image for deployment
Switching the final stage tonvidia/cuda:11.7.1-runtime-ubuntu22.04is appropriate. To minimize image size, clean up apt caches:RUN apt update \ && apt install -y --no-install-recommends vim netcat-openbsd net-tools curl jq \ && rm -rf /var/lib/apt/lists/*build/dockerfiles/coordinator-api/config.toml (1)
27-34: Consolidate OpenVM Stark patches
You have two[patch]sections targeting the same crates. Merge them into one:[patch."ssh://git@github.com/scroll-tech/openvm-stark-gpu.git"] openvm-stark-backend = { path = "/openvm-stark-gpu/crates/stark-backend", features = ["gpu"] } openvm-stark-sdk = { path = "/openvm-stark-gpu/crates/stark-sdk", features = ["gpu"] }This reduces redundancy and potential confusion.
common/libzkp/impl/Cargo.toml (1)
40-67: Patch Plonky3 crates to GPU-enabled fork
Using tagv0.2.0is clear; for stronger reproducibility, consider pinning to a specific commit hash. Also, confirm if any additional features ordefault-features = falsesettings are required for your use case.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (2)
common/libzkp/impl/Cargo.lockis excluded by!**/*.lockzkvm-prover/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
.github/workflows/common.yml(1 hunks).github/workflows/docker.yml(1 hunks)build/dockerfiles/coordinator-api.Dockerfile(4 hunks)build/dockerfiles/coordinator-api/checkout_all.sh(1 hunks)build/dockerfiles/coordinator-api/clone_openvm_gpu.sh(1 hunks)build/dockerfiles/coordinator-api/clone_openvm_stark_gpu.sh(1 hunks)build/dockerfiles/coordinator-api/clone_plonky3_gpu.sh(1 hunks)build/dockerfiles/coordinator-api/config.toml(1 hunks)common/libzkp/impl/Cargo.toml(2 hunks)common/version/version.go(1 hunks)zkvm-prover/Cargo.toml(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: tests
🔇 Additional comments (9)
common/version/version.go (1)
8-8: Version bump aligns with feature rollout.The
taghas been correctly updated fromv4.5.9tov4.5.10to reflect the new coordinator and prover support changes. No additional changes are needed in this file.zkvm-prover/Cargo.toml (2)
21-21: Ensure version bump consistency across modules
Thescroll-zkvm-prover-eucliddependency has been updated to v0.5.0—please verify that any centralized versioning files (e.g.,common/version.go) or other Cargo manifests also reflect this bump to prevent mix-and-match versions.
54-61: Confirm new Plonky3 GPU patches are used
A large set ofp3-crates (including the newp3-koala-bear) is now patched to the GPU-enabled fork at tag v0.2.0. Ensure each of these crates is actively consumed in the codebase; remove any that remain unused to avoid clutter and unnecessary fetches.build/dockerfiles/coordinator-api/checkout_all.sh (1)
1-2: Enable strict error handling and debugging
Good use ofset -uexto catch unset variables and exit on errors, while printing commands for visibility..github/workflows/docker.yml (2)
347-348: Verify loaded SSH keys count
With the refactored approach, this will now reflect the total keys.
351-351: Checkout pinned commits in one step
Switching tocheckout_all.shcentralizes commit checkout. Ensure the script hasset -e(or equivalent) so failures bubble up to fail the workflow.build/dockerfiles/coordinator-api/config.toml (1)
22-22: Enableevm-provefeature foropenvm-sdk
Adding"evm-prove"aligns with GPU-based proof generation requirements.common/libzkp/impl/Cargo.toml (2)
17-19: Bumpeuclid_proverandeuclid_verifierto v0.5.0
Updating to v0.5.0 matches the coordinator/prover support requirement.
36-39: Patch OpenVM Stark crates to GPU fork
Redirecting toscroll-tech/openvm-stark-gpuwith thegpufeature aligns with GPU acceleration goals.
…multiple SSH clients
colinlyguo
force-pushed
the
feat-coordinator-and-prover-support-v0.4.0
branch
from
2c94b60 to
14e4abb
Compare
colinlyguo
changed the title
6 participants
Purpose or design rationale of this PR
PR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
Deployment tag versioning
Has
tagincommon/version.gobeen updated or have you addedbump-versionlabel to this PR?Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit