Skip to content

chore(zkvm-circuit): upgrade to 0.1.0-rc.6 #1610

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

colinlyguo
Copy link
Member

@colinlyguo colinlyguo commented Mar 7, 2025

Purpose or design rationale of this PR

Related commit in scroll-proving-sdk: scroll-tech/scroll-proving-sdk@9524a42

Summary by CodeRabbit

  • New Features

    • Introduced a command-line interface for the zero-knowledge prover with enhanced configuration options including version reporting.
    • Added new configuration parameters for batch processing in Layer 2 relayer operations.
    • Enabled functionality to dump verification keys for improved diagnostics.
  • Improvements

    • Upgraded toolchains and updated dependencies across all projects to boost compatibility and performance.
    • Enhanced error handling and logging across verification and task-processing modules.
    • Bumped the release version from v4.4.89 to v4.4.90.

Copy link

coderabbitai bot commented Mar 7, 2025

Walkthrough

This pull request comprises extensive updates across multiple modules. The changes include version bumps in Go and Rust toolchains, dependency upgrades, new Dockerfiles, and significant restructuring in proof handling logic. Notably, the darwin verifiers were removed in favor of a new EuclidVerifier, and several interfaces and data types were updated from pointer to value semantics. Additionally, a new Rust-based zkvm-prover project was introduced alongside configuration and build script enhancements.

Changes

File(s) Change Summary
.github/workflows/common.yml Updated Rust toolchain version from nightly-2023-12-03 to nightly-2024-12-06.
Makefile, bridge-history-api/go.mod, common/go.mod, coordinator/go.mod, database/go.mod, tests/integration-test/go.mod, rollup/go.mod Bumped Go version from 1.21 to 1.22, set toolchain to go1.22.2, revised multiple dependency versions, updated L2GETH_TAG, and removed the -u flag in dependency fetch commands.
build/dockerfiles/prover.Dockerfile, build/dockerfiles/prover.dockerignore Introduced a new Dockerfile and Dockerignore to build and run the Rust-based “prover” application.
common/libzkp/impl/*, common/libzkp/interface/libzkp.h Reworked Cargo.toml dependencies, updated rust-toolchain, added new dump_vk functions, removed darwin verifiers, and introduced the EuclidVerifier with added verification and key dumping methods.
common/types/message/message.go, common/version/version.go Restructured proof-related types and interfaces (e.g., changing slice types from pointers to values) and bumped the version from “v4.4.89” to “v4.4.90”.
coordinator/* (cmd/tool, internal/logic/, internal/orm/, tests, verifier/*) Streamlined proof handling by switching from pointer to value semantics, updated logging, added OpenVM support, and enhanced error handling and filtering in ORM queries.
rollup/* (conf, internal/config, controller, tests) Enhanced relayer and proposer logic with new configuration fields (e.g., max_chunks_per_batch, test_env_bypass_only_until_fork_boundary), improved error handling and codec support, and updated tests accordingly.
scroll-contracts Updated the subproject commit reference.
zkvm-prover/* (Cargo.toml, Makefile, config.json, shell scripts, rust-toolchain, rustfmt.toml, src/*) Introduced a new Rust-based zkvm-prover project with comprehensive configuration, build scripts, CLI, and an asynchronous proving service with dedicated circuit handlers.

Sequence Diagram(s)

sequenceDiagram
    participant C as Client
    participant L as libzkp Interface
    participant EU as EuclidVerifier
    participant FS as File System
    C->>L: Call dump_vk(fork_name, file)
    L->>EU: _dump_vk(fork_name, file)
    EU->>FS: Write verification keys to file
    FS-->>EU: File written
    EU-->>L: Return verification keys
    L-->>C: Complete dump_vk call
Loading
sequenceDiagram
    participant CLI as Prover CLI
    participant MP as Main (zkvm-prover)
    participant CP as Config Parser
    participant LP as LocalProver Service
    CLI->>MP: Parse command-line arguments
    MP->>CP: Load configuration file
    CP-->>MP: Return configuration data
    MP->>LP: Initialize LocalProver with config
    LP->>LP: Set active circuit handler based on hard fork
    LP->>LP: Process prove request asynchronously
    LP-->>MP: Return prove response
    MP-->>CLI: Output proof result
Loading

Suggested labels

bump-version

Suggested reviewers

  • georgehao
  • 0xmountaintop
  • Thegaram
  • zimpha
  • jonastheis

Poem

Oh, what a hop, what a thrill to see,
Bumps and upgrades in code, as lively as can be.
From Rusty toolchains to proving keys,
I nibble on changes with joyful glee.
Leaping through files with a twitch of my nose,
Carrot-fueled code blossoms where innovation grows!
🐇💻✨

Warning

There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure.

🔧 golangci-lint (1.62.2)

level=warning msg="[runner] Can't run linter goanalysis_metalinter: buildir: failed to load package zstd: could not load export data: no export data for "github.com/scroll-tech/da-codec/encoding/zstd""
level=error msg="Running error: can't run linter goanalysis_metalinter\nbuildir: failed to load package zstd: could not load export data: no export data for "github.com/scroll-tech/da-codec/encoding/zstd""

✨ Finishing Touches
  • 📝 Generate Docstrings

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
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@colinlyguo colinlyguo changed the base branch from develop to omerfirmak/euclid-prover March 7, 2025 04:40
@codecov-commenter
Copy link

codecov-commenter commented Mar 7, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 42.12%. Comparing base (4333d51) to head (b2e1a6c).

Additional details and impacted files
@@                     Coverage Diff                      @@
##           omerfirmak/euclid-prover    #1610      +/-   ##
============================================================
- Coverage                     42.12%   42.12%   -0.01%     
============================================================
  Files                           222      222              
  Lines                         17739    17739              
============================================================
- Hits                           7473     7472       -1     
- Misses                         9557     9558       +1     
  Partials                        709      709              
Flag Coverage Δ
bridge-history-api 8.06% <ø> (ø)
common 28.77% <ø> (ø)
coordinator 34.19% <ø> (-0.04%) ⬇️
database 42.05% <ø> (ø)
rollup 54.47% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.

Actionable comments posted: 9

🔭 Outside diff range comments (1)
common/libzkp/impl/src/verifier.rs (1)

47-59: 💡 Verification agent

🧩 Analysis chain

Simplified verifier initialization.

The initialization logic has been significantly simplified, directly creating an EuclidVerifier from the assets path rather than the previous complex configuration process. This makes the code more maintainable.

Let's check if VERIFIER_HIGH is populated correctly by examining the usage:


🏁 Script executed:

#!/bin/bash
# Check if VERIFIER_HIGH is used properly in the codebase
rg "VERIFIER_HIGH" common/libzkp/impl/src/

Length of output: 234


Action Required: Verify Global Verifier Initialization

The simplified initialization appears intended to create an instance of EuclidVerifier using the provided assets path and associate it with the high-version verifier. However, while the static variable declared is VERIFIER_HIGH, the init function is setting VERIFIER_LOW. The rg search confirmed that VERIFIER_HIGH is indeed referenced in a conditional check (if let Some(verifier) = VERIFIER_HIGH.get() {...}), but it isn’t being initialized in the shown snippet.

Please review and correct the inconsistency by ensuring that either:

  • The initialization sets VERIFIER_HIGH if that’s the intended global, or
  • Update the conditional check to reference VERIFIER_LOW if that is correct.
🧹 Nitpick comments (32)
coordinator/internal/orm/batch.go (1)

86-87: Add a code comment explaining the codec_version filter.

You've added a filter to exclude batches with codec_version = 5 in the SQL query. This is likely related to the zkvm-circuit upgrade to 0.1.0-rc.6, but the purpose of this filter isn't immediately clear from the code.

Consider adding a comment explaining why this specific codec version is being excluded:

 sql := fmt.Sprintf("SELECT * FROM batch WHERE proving_status = %d AND total_attempts < %d AND active_attempts < %d AND chunk_proofs_status = %d AND codec_version != 5 AND batch.deleted_at IS NULL ORDER BY batch.index LIMIT 1;",
+    // Excluding codec_version 5 as it's not compatible with zkvm-circuit 0.1.0-rc.6
     int(types.ProvingTaskUnassigned), maxTotalAttempts, maxActiveAttempts, int(types.ChunkProofsStatusReady))
coordinator/internal/orm/chunk.go (1)

77-78: Add a code comment explaining the codec_version filter.

Similar to the batch.go file, you've added a filter to exclude chunks with codec_version = 5 in the SQL query. The purpose of this filter isn't immediately clear without additional context.

Consider adding a comment explaining why this specific codec version is being excluded:

 sql := fmt.Sprintf("SELECT * FROM chunk WHERE proving_status = %d AND total_attempts < %d AND active_attempts < %d AND end_block_number <= %d AND codec_version != 5 AND chunk.deleted_at IS NULL ORDER BY chunk.index LIMIT 1;",
+    // Excluding codec_version 5 as it's not compatible with zkvm-circuit 0.1.0-rc.6
     int(types.ProvingTaskUnassigned), maxTotalAttempts, maxActiveAttempts, height)
common/libzkp/interface/libzkp.h (1)

11-12: Ensure adequate documentation for new API function

The newly added dump_vk function lacks documentation comments explaining its purpose, parameters, and expected usage. Consider adding documentation to maintain API clarity.

 char verify_chunk_proof(char* proof, char* fork_name);

+// dump_vk writes the verification key for the specified fork to the given file path
+// Parameters:
+//   - fork_name: Name of the fork to dump the verification key for
+//   - file: File path to write the verification key to
 void dump_vk(char* fork_name, char* file);
rollup/internal/config/l2.go (1)

50-50: Good addition of batch chunk limiting configuration

The addition of MaxChunksPerBatch provides important control over batch processing in the rollup system, which aligns well with the zkvm-circuit upgrade goals.

Consider adding a brief comment explaining the purpose of this parameter and any recommended value ranges.

 MaxUncompressedBatchBytesSize   uint64  `json:"max_uncompressed_batch_bytes_size"`
+// MaxChunksPerBatch limits the number of chunks that can be included in a single batch
 MaxChunksPerBatch               int     `json:"max_chunks_per_batch"`
zkvm-prover/print_halo2gpu_version.sh (1)

13-13: Improve regex pattern robustness

The grep pattern for extracting the halo2gpu path might be fragile if the config file format changes.

Consider using a more robust approach, such as:

-halo2gpu_path=$(grep -Po '(?<=paths = \[")([^"]*)' $config_file)
+halo2gpu_path=$(awk -F'"' '/paths = \[/{print $2}' $config_file)

Or better yet, use a package like toml or cargo metadata to parse the configuration properly.

zkvm-prover/src/zk_circuits_handler.rs (1)

1-12: New CircuitsHandler trait looks good, but missing documentation

The trait definition is well-structured and follows Rust best practices using async_trait, but it lacks documentation comments.

Consider adding rustdoc comments to document the trait and its methods:

+/// Handler for ZK circuit operations
+///
+/// This trait defines the interface for handling verification keys and proof generation
+/// operations for different proof types.
 #[async_trait]
 pub trait CircuitsHandler: Sync + Send {
+    /// Retrieves the verification key for the specified proof type
+    ///
+    /// # Arguments
+    /// * `task_type` - The type of proof to get the verification key for
+    ///
+    /// # Returns
+    /// The verification key as a byte vector, or None if not available
     async fn get_vk(&self, task_type: ProofType) -> Option<Vec<u8>>;

+    /// Generates proof data for the given prove request
+    ///
+    /// # Arguments
+    /// * `prove_request` - The request containing all necessary information for proof generation
+    ///
+    /// # Returns
+    /// The proof data as a string, or an error if proof generation fails
     async fn get_proof_data(&self, prove_request: ProveRequest) -> Result<String>;
 }
build/dockerfiles/prover.Dockerfile (3)

15-15: Specify target directory for source code copy

The COPY command doesn't specify a destination directory, which means it will copy to the current working directory (likely the root). This could lead to unexpected behavior.

-COPY ./zkvm-prover .
+COPY ./zkvm-prover /app
+WORKDIR /app

17-17: Consider capturing build errors

The cargo build command doesn't capture or handle potential build failures explicitly.

-RUN cargo build --release
+RUN cargo build --release && echo "Build completed successfully"

19-23: Consider security best practices for runtime container

The runtime container runs as root by default and lacks security constraints.

Consider adding:

 FROM ubuntu:24.04 AS runtime
 
 COPY --from=builder /app/target/release/prover /usr/local/bin/
 
+# Create non-root user
+RUN useradd -m prover
+USER prover
+
+# Set health check
+HEALTHCHECK --interval=30s --timeout=10s --start-period=5s --retries=3 CMD [ "prover", "--version" ]
+
 ENTRYPOINT ["prover"]
zkvm-prover/Cargo.toml (1)

30-47: Consider organizing dependencies by purpose

The dependencies section would benefit from organization by purpose (e.g., networking, crypto, serialization) with comments explaining each group's purpose. This would improve maintainability as the project grows.

zkvm-prover/src/main.rs (2)

28-47: Improve error handling and shutdown process

The main function runs the prover but doesn't handle the result from prover.run(). Consider these improvements:

  1. Properly handle the result from run()
  2. Implement graceful shutdown on signals
  3. Add more detailed error logging
 #[tokio::main]
 async fn main() -> anyhow::Result<()> {
     init_tracing();
+    
+    // Set up signal handlers for graceful shutdown
+    let shutdown = tokio::signal::ctrl_c();
 
     let args = Args::parse();
 
     if args.version {
         println!("version is {}", get_version());
         std::process::exit(0);
     }
 
     let cfg = LocalProverConfig::from_file(args.config_file)?;
     let sdk_config = cfg.sdk_config.clone();
     let local_prover = LocalProver::new(cfg);
     let prover = ProverBuilder::new(sdk_config, local_prover).build().await?;
 
-    prover.run().await;
+    // Run the prover and handle any errors
+    match prover.run().await {
+        Ok(_) => log::info!("Prover completed successfully"),
+        Err(e) => {
+            log::error!("Prover failed: {}", e);
+            return Err(e.into());
+        }
+    }
 
     Ok(())
 }

12-26: Consider enhancing CLI argument handling

The CLI arguments are minimal but functional. Consider adding:

  1. Environment variable fallbacks for configuration options
  2. Validation for configuration file existence
  3. More descriptive help messages
 #[derive(Parser, Debug)]
 #[clap(disable_version_flag = true)]
 struct Args {
     /// Path of config file
-    #[arg(long = "config", default_value = "conf/config.json")]
+    #[arg(
+        long = "config",
+        default_value = "conf/config.json",
+        env = "PROVER_CONFIG",
+        help = "Path to the prover configuration JSON file"
+    )]
     config_file: String,
 
     /// Version of this prover
-    #[arg(short, long, action = ArgAction::SetTrue)]
+    #[arg(
+        short, 
+        long, 
+        action = ArgAction::SetTrue,
+        help = "Display the prover version and exit"
+    )]
     version: bool,
 
     /// Path of log file
-    #[arg(long = "log.file")]
+    #[arg(
+        long = "log.file",
+        env = "PROVER_LOG_FILE",
+        help = "Path to store logs (if not specified, logs to stdout)"
+    )]
     log_file: Option<String>,
 }
coordinator/internal/logic/provertask/bundle_prover_task.go (1)

224-231: Changed from pointer semantics to value semantics for batch proofs.

The code now uses value semantics ([]message.BatchProof) instead of pointer semantics ([]*message.BatchProof) for batch proofs. This change reduces indirection and potentially simplifies memory management.

Ensure this change is consistent across all related code to avoid mixing pointer and value semantics. Run tests to verify the behavior remains consistent.

common/libzkp/impl/src/verifier/euclid.rs (4)

1-14: Introduce documentation comments for EuclidVerifier.
Adding doc comments for the struct fields (e.g., chunk, batch, and bundle verifiers) can help others understand their roles faster.


16-31: Review setup paths for EuclidVerifier.
The constructor sets up three verifiers from the provided paths. If these paths can be user-defined, consider adding validation or error handling for missing or invalid files in lieu of panicking.


33-53: Handle potential JSON errors more gracefully.
When deserializing proofs, capturing serde errors distinctly (rather than unwrapping) can deliver clearer feedback to callers.


55-64: Consider alternative VK encoding or encryption.
Base64 encoding is convenient, but if there's concern about sensitive data, evaluating a different format or encrypting the file might be beneficial.

zkvm-prover/src/zk_circuits_handler/euclid.rs (2)

12-16: Consider adding documentation for each prover field.

Providing a brief comment for chunk_prover, batch_prover, and bundle_prover fields can help new contributors quickly understand their responsibilities and relationships with the rest of the codebase.


60-92: Avoid blocking concurrency with a single mutex.

Multiple proof requests for “Chunk,” “Batch,” or “Bundle” tasks will contend on the same lock. If performance or parallelism is critical, consider per-prover locks or a lock-free design to reduce blocking.

coordinator/internal/logic/verifier/mock.go (2)

19-24: Use consistent logging or comments for mock behavior.

When returning a mock verification result, logging or clarifying comments can help other maintainers quickly grasp that these verifications are placeholders. Consistent messaging avoids confusion across different mock methods.


27-32: Check the performance impact of repeated string conversion.

Casting the proof to string (string(proof.Proof())) is fine for a mock, but if proofs grow large, it might have overhead. It's minor in this mock code but worth noting if the logic expands in the future.

coordinator/internal/logic/verifier/verifier.go (2)

60-64: Document how rustVkDump values are produced.

Since dump_vk presumably writes a JSON file containing these fields, a comment clarifying the origin and structure of chunk_vk, batch_vk, and bundle_vk supports maintainability.


102-109: Ensure error handling coverage for loadLowVersionVKs and loadOpenVMVks.

Breaking changes in these methods (e.g., invalid file paths) might propagate. Verify you handle or log errors thoroughly, especially given reliance on external file I/O and FFI calls.

coordinator/internal/logic/provertask/batch_prover_task.go (1)

218-239: Updated proof handling to use value semantics

The code now uses value semantics instead of pointer semantics for chunk proofs, consistent with the changes in other files. The type check for ChunkInfo access has been improved to use proper type assertion.

Consider adding an error check for the type assertion:

-        if haloProot, ok := proof.(*message.Halo2ChunkProof); ok {
-            if haloProot.ChunkInfo != nil {
-                chunkInfo.TxBytes = haloProot.ChunkInfo.TxBytes
-            }
+        if haloProot, ok := proof.(*message.Halo2ChunkProof); ok {
+            if haloProot.ChunkInfo != nil {
+                chunkInfo.TxBytes = haloProot.ChunkInfo.TxBytes
+            }
+        } else {
+            log.Debug("chunk proof is not a Halo2ChunkProof", "chunk_hash", chunk.Hash)
         }
rollup/internal/controller/relayer/l2_relayer.go (2)

208-211: Improve error diagnostic with contextual details
Currently, the returned error does not include contextual information (e.g., chunk vs. block). Including it in the error message or logs would greatly aid debugging.

208     if err = r.l2BlockOrm.InsertL2Blocks(r.ctx, chunk.Blocks); err != nil {
-       return fmt.Errorf("failed to insert genesis block: %v", err)
+       return fmt.Errorf("failed to insert genesis block for chunk %d: %w",
+           0,  // or chunk index if available
+           err,
+       )
}

530-556: Fix typographical error
A minor typo is present in the log statement at line 544 ("firsr"). This can be distracting and should be corrected in all affected logs.

- log.Error("failed to get firsr unfinalized chunk", "chunk index", firstUnfinalizedBatch.StartChunkIndex)
+ log.Error("failed to get first unfinalized chunk", "chunk index", firstUnfinalizedBatch.StartChunkIndex)
zkvm-prover/src/prover.rs (4)

23-41: Consider adding config validations
For production environments, consider validating required fields in LocalProverConfig (e.g., missing circuit entries).


49-55: Single active proving task
Storing current_task in LocalProver suggests only one proving job can run at a time. If multiple concurrent tasks are desired later, plan for additional concurrency or scheduling logic.


57-131: Implementation of the ProvingService trait
The flow of provedo_prove and query_task is straightforward. However, be mindful that a second .prove() call while a first is in progress would overwrite current_task.


133-190: Concurrency approach
Using tokio::task::spawn_blocking is a well-established way to offload CPU-bound proof logic. Just confirm that blocking tasks don’t hamper performance if you plan to scale or handle multiple simultaneous proofs.

common/types/message/message.go (2)

49-52: Consider replacing interface{} with a more specific type.
Using interface{} for BatchHeader can lead to extra type assertions or reflection overhead. If there's a known structure or set of possible structures, a more explicit type might improve code safety and readability.


246-253: Consider a non-panic error handling strategy.
Currently, marshaling errors trigger a panic. Replace it with an error return or a logged error to prevent unexpected process termination.

 func (p *OpenVMChunkProof) Proof() []byte {
-   proofJson, err := json.Marshal(p.VmProof)
-   if err != nil {
-       panic(fmt.Sprint("marshaling error", err))
-   }
+   proofJson, err := json.Marshal(p.VmProof)
+   if err != nil {
+       // handle error gracefully, e.g. return nil or log the error
+       return nil
+   }
    return proofJson
 }
🛑 Comments failed to post (9)
zkvm-prover/print_halo2gpu_version.sh (3)

20-20: 🛠️ Refactor suggestion

Add error handling for directory operations

Similar to the previous comment, the popd command should include error handling.

-popd
+popd || { echo "Error: Unable to return to previous directory"; exit 1; }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

popd || { echo "Error: Unable to return to previous directory"; exit 1; }
🧰 Tools
🪛 Shellcheck (0.10.0)

[warning] 20-20: Use 'popd ... || exit' or 'popd ... || return' in case popd fails.

(SC2164)


15-15: 🛠️ Refactor suggestion

Add error handling for directory operations

The pushd command lacks error handling which could lead to unexpected behavior if the directory doesn't exist or can't be accessed.

-pushd $halo2gpu_path
+pushd $halo2gpu_path || { echo "Error: Cannot change directory to $halo2gpu_path"; exit 1; }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

pushd $halo2gpu_path || { echo "Error: Cannot change directory to $halo2gpu_path"; exit 1; }
🧰 Tools
🪛 Shellcheck (0.10.0)

[warning] 15-15: Use 'pushd ... || exit' or 'pushd ... || return' in case pushd fails.

(SC2164)


17-18: 🛠️ Refactor suggestion

Add error handling for git operations

The git command lacks error handling if the directory is not a git repository or if the command fails for other reasons.

-commit_hash=$(git log --pretty=format:%h -n 1)
-echo "${commit_hash:0:7}"
+if ! commit_hash=$(git log --pretty=format:%h -n 1 2>/dev/null); then
+  echo "Error: Failed to get git commit hash"
+  exit 1
+fi
+echo "${commit_hash:0:7}"
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

if ! commit_hash=$(git log --pretty=format:%h -n 1 2>/dev/null); then
  echo "Error: Failed to get git commit hash"
  exit 1
fi
echo "${commit_hash:0:7}"
build/dockerfiles/prover.Dockerfile (1)

21-21: ⚠️ Potential issue

Fix source path for binary copy

The path to the built binary seems incorrect. Given the build context, it should be relative to the current directory.

-COPY --from=builder /target/release/prover /usr/local/bin/
+COPY --from=builder /app/target/release/prover /usr/local/bin/
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

COPY --from=builder /app/target/release/prover /usr/local/bin/
common/libzkp/impl/Cargo.toml (1)

17-18: 💡 Verification agent

🧩 Analysis chain

Confirm tag availability for Euclid-based crates.
Make sure the tag v0.1.0-rc.6 exists and is stable in scroll-tech/zkvm-prover.git.


🏁 Script executed:

#!/bin/bash
# Checking if tag v0.1.0-rc.6 is present in the GitHub repository for scroll-zkvm-prover.
gh release view v0.1.0-rc.6 --repo scroll-tech/zkvm-prover || echo "Tag v0.1.0-rc.6 not found."

Length of output: 141


Action Required: Update Euclid-based Crates Tag

The tag v0.1.0-rc.6 specified in common/libzkp/impl/Cargo.toml could not be found in the scroll-tech/zkvm-prover.git repository. Please update the tag reference to a valid and stable release to ensure the crates rely on an existing version.

  • Location: common/libzkp/impl/Cargo.toml, lines 17–18.
  • Action: Verify the correct tag (e.g., by checking the repository's release list) and update both euclid_prover and euclid_verifier dependencies accordingly.
zkvm-prover/src/zk_circuits_handler/euclid.rs (1)

49-57: 🛠️ Refactor suggestion

Handle potential lock poisoning gracefully.

This code calls self.try_lock().unwrap(), which will panic if the lock is poisoned. Consider using lock().await or handling the error more gracefully in production environments to avoid crashing the entire process.

coordinator/internal/logic/verifier/verifier.go (1)

232-264: ⚠️ Potential issue

Verify concurrency implications of writing and removing the temp file.

If this function is invoked concurrently for the same forkName, race conditions could arise when multiple threads read, write, and remove the same file. Consider including unique naming, or synchronizing calls if concurrency is possible.

- tempFile := path.Join(os.TempDir(), "openVmVk.json")
+ tempFile := path.Join(os.TempDir(), fmt.Sprintf("openVmVk_%s.json", forkName))
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

func (v *Verifier) loadOpenVMVks(forkName string) error {
	tempFile := path.Join(os.TempDir(), fmt.Sprintf("openVmVk_%s.json", forkName))
	defer func() {
		if err := os.Remove(tempFile); err != nil {
			log.Error("failed to remove temp file", "err", err)
		}
	}()

	forkNameCStr := C.CString(forkName)
	defer C.free(unsafe.Pointer(forkNameCStr))
	tempFileCStr := C.CString(tempFile)
	defer C.free(unsafe.Pointer(tempFileCStr))

	C.dump_vk(forkNameCStr, tempFileCStr)

	f, err := os.Open(tempFile)
	if err != nil {
		return err
	}
	byt, err := io.ReadAll(f)
	if err != nil {
		return err
	}

	var dump rustVkDump
	if err := json.Unmarshal(byt, &dump); err != nil {
		return err
	}
	v.OpenVMVkMap[dump.Chunk] = struct{}{}
	v.OpenVMVkMap[dump.Batch] = struct{}{}
	v.OpenVMVkMap[dump.Bundle] = struct{}{}
	return nil
}
common/types/message/message.go (2)

356-382: ⚠️ Potential issue

Insufficient length check for Instances.
Though you verify len(pf.Proof)%32 == 0, you do not confirm pf.Instances has ≥384 bytes. This omission can cause a runtime slice error in Proof().


324-354: ⚠️ Potential issue

Potential out-of-bounds slice risk at line 351.
p.EvmProof.Instances[:384] can panic if len(p.EvmProof.Instances) < 384. Add a length check in Proof() or within the sanity check logic before slicing.

 func (p *OpenVMBundleProof) Proof() []byte {
+   if len(p.EvmProof.Instances) < 384 {
+       // handle this error scenario, e.g. log or return a partial result
+       panic("Instances slice too short, must be at least 384 bytes")
+   }
    proofBytes := make([]byte, 0, 384+len(p.EvmProof.Proof))
    proofBytes = append(proofBytes, p.EvmProof.Instances[:384]...)
    return append(proofBytes, p.EvmProof.Proof...)
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

// OpenVMBundleProof includes the proof info that are required for verification of a bundle of batch proofs.
type OpenVMBundleProof struct {
	MetaData struct {
		BundleInfo    *OpenVMBundleInfo `json:"bundle_info"`
		BunndlePIHash common.Hash       `json:"bundle_pi_hash"`
	} `json:"metadata"`

	EvmProof   *OpenVMEvmProof `json:"proof"`
	Vk         []byte          `json:"vk,omitempty"`
	GitVersion string          `json:"git_version,omitempty"`
}

// Proof returns the proof bytes that are eventually passed as calldata for on-chain bundle proof verification.
//
// There are 12 accumulators for a SNARK proof. The accumulators are the first 12 elements of the EvmProof's
// Instances field. The remaining items in Instances are supplied on-chain by the ScrollChain contract.
//
// The structure of these bytes is:
// | byte index start | byte length    | value    | description         |
// |------------------|----------------|----------|---------------------|
// | 0                | 32             | accs[0]  | accumulator 1       |
// | 32               | 32             | accs[1]  | accumulator 2       |
// | 32*i ...         | 32             | accs[i]  | accumulator i ...   |
// | 352              | 32             | accs[11] | accumulator 12      |
// | 384              | dynamic        | proof    | proof bytes         |
func (p *OpenVMBundleProof) Proof() []byte {
	if len(p.EvmProof.Instances) < 384 {
		// handle this error scenario, e.g. log or return a partial result
		panic("Instances slice too short, must be at least 384 bytes")
	}
	proofBytes := make([]byte, 0, 384+len(p.EvmProof.Proof))
	proofBytes = append(proofBytes, p.EvmProof.Instances[:384]...)
	return append(proofBytes, p.EvmProof.Proof...)
}

@colinlyguo colinlyguo merged commit b30f4d0 into omerfirmak/euclid-prover Mar 7, 2025
18 checks passed
@colinlyguo colinlyguo deleted the euclid-prover/update-zkvm-circuit-to-0.1.0-rc.6 branch March 7, 2025 05:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants