[Refactor] Universal task #1680
Conversation
WalkthroughThis change migrates the zero-knowledge proof (ZKP) verifier and related infrastructure from the legacy Changes
Sequence Diagram(s)sequenceDiagram
participant GoApp as Go Coordinator
participant CFFI as libzkp-c (C FFI)
participant RustLib as libzkp (Rust)
participant L2Geth as l2geth (RPC Client)
GoApp->>CFFI: Call init_verifier(config)
CFFI->>RustLib: verifier_init(config)
RustLib-->>CFFI: Result
GoApp->>CFFI: Call gen_universal_task(task_type, task_json, fork_name)
alt TaskType == Chunk
CFFI->>L2Geth: get_client()
CFFI->>RustLib: gen_universal_task(..., interpreter=L2Geth)
else other types
CFFI->>RustLib: gen_universal_task(...)
end
RustLib-->>CFFI: (hash, metadata, universal_task)
CFFI-->>GoApp: HandlingResult
GoApp->>CFFI: verify_*_proof(proof, fork_name)
CFFI->>RustLib: verify_proof(proof, fork_name, task_type)
RustLib-->>CFFI: bool
CFFI-->>GoApp: c_char (success/fail)
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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 (
|
common/libzkp/integration_test/testdata/phase1/proofs/README.md
Outdated
Show resolved
Hide resolved
common/libzkp/integration_test/testdata/phase2/witnesses/8.json
Outdated
Show resolved
Hide resolved
|
overall comments:
|
noel2004
force-pushed
the
refactor/zkvm_2
branch
from
8982e33 to
4db49af
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1680 +/- ##
===========================================
+ Coverage 40.05% 40.13% +0.07%
===========================================
Files 232 234 +2
Lines 18502 18614 +112
===========================================
+ Hits 7411 7470 +59
- Misses 10369 10412 +43
- Partials 722 732 +10
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: 0
🧹 Nitpick comments (1)
.github/workflows/common.yml (1)
44-44: Confirm Cargo cache mapping matches new Rust workspace layoutYou’ve updated the
rust-cacheaction to map the root.workspace totarget, which works for a single-root workspace. If your refactor introduced sub-crates with independenttargetdirs (e.g. undercrates/libzkp,crates/prover-bin, etc.), you may want to list them explicitly to ensure all build artifacts are cached:with: workspaces: | . -> target crates/libzkp -> crates/libzkp/target crates/libzkp-c -> crates/libzkp-c/target crates/l2geth -> crates/l2geth/target crates/prover-bin -> crates/prover-bin/target
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/common.yml(1 hunks).github/workflows/integration.yml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/integration.yml
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: check
- GitHub Check: tests
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
coordinator/libzkp/Makefile (1)
1-18: Makefile: invalidcargo cleanflag & missing PHONY targets
cargo cleandoes not accept--release; it silently ignores the flag but newer Cargo versions treat unknown flags as an error.- Targets
buildandcleanare not declared in.PHONY, causingcheckmakewarnings.- Providing an
allalias improves UX (makewithout args builds the library).Proposed patch:
-.PHONY: help fmt clippy test test-ci test-all +.PHONY: all build clean fmt clippy test test-ci test-all + +all: build @@ -clean: - @cargo clean --release -p libzkp -p libzkp-c -p l2geth - @rm -f lib/libzkp.so +clean: + @cargo clean -p libzkp -p libzkp-c -p l2geth + @rm -f lib/libzkp.so
♻️ Duplicate comments (2)
coordinator/internal/logic/submitproof/proof_receiver.go (1)
18-19: Same build-tag problem as aboveThe coordinator now imports
scroll-tech/coordinator/libzkp, but the package still fails type-checking because no build-eligible Go sources are found (seegolangci-lintoutput).Unless this is addressed, any command that touches this file (
go vet,go test ./..., the GitHub workflow) will exit with a non-zero status.Action: ensure at least one build-eligible file exists in
coordinator/libzkp, or wrap this import with the correct//go:buildtag.coordinator/internal/logic/verifier/verifier.go (1)
15-16: Import path compiles only iflibzkpprovides build-eligible sourcesSame root cause as the earlier two occurrences – the new path cannot be resolved in the current build context.
Failing to fix this will break all non-mock_verifierbuilds.
🧹 Nitpick comments (2)
.github/workflows/coordinator.yml (1)
116-116: UpdatedLD_LIBRARY_PATHis still commented outThe path was changed from
../common/libzkp/libto./libzkp/lib, but the entire line remains commented.
If the intention is to run tests against the real shared library (rather than themock_verifiertag), remember to uncomment this line; otherwise the change has no effect.coordinator/Makefile (1)
26-26: Add PHONY declaration for libzkp target.Static analysis correctly identifies that the
libzkptarget should be declared as PHONY since it doesn't create a file with that name.-.PHONY: lint docker clean coordinator coordinator_skip_libzkp mock_coordinator +.PHONY: lint docker clean coordinator coordinator_skip_libzkp mock_coordinator libzkp
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockgo.workis excluded by!**/*.work
📒 Files selected for processing (16)
.github/workflows/coordinator.yml(1 hunks).github/workflows/integration.yml(1 hunks)Cargo.toml(1 hunks)README.md(1 hunks)build/dockerfiles/coordinator-api.Dockerfile(3 hunks)common/libzkp/go.mod(0 hunks)common/version/version.go(1 hunks)coordinator/Makefile(2 hunks)coordinator/internal/logic/provertask/prover_task.go(1 hunks)coordinator/internal/logic/submitproof/proof_receiver.go(1 hunks)coordinator/internal/logic/verifier/verifier.go(2 hunks)coordinator/libzkp/Makefile(1 hunks)coordinator/libzkp/e2e-test.sh(1 hunks)coordinator/libzkp/lib.go(3 hunks)coordinator/libzkp/mock_universal_task.go(1 hunks)coordinator/libzkp/universal_task.go(1 hunks)
💤 Files with no reviewable changes (1)
- common/libzkp/go.mod
✅ Files skipped from review due to trivial changes (3)
- coordinator/libzkp/e2e-test.sh
- README.md
- common/version/version.go
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/integration.yml
- build/dockerfiles/coordinator-api.Dockerfile
- Cargo.toml
🧰 Additional context used
🪛 checkmake (0.2.2)
coordinator/libzkp/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
coordinator/Makefile
[warning] 26-26: Target "libzkp" should be declared PHONY.
(phonydeclared)
🪛 golangci-lint (1.64.8)
coordinator/internal/logic/provertask/prover_task.go
16-16: could not import scroll-tech/coordinator/libzkp (-: build constraints exclude all Go files in libzkp)
(typecheck)
coordinator/internal/logic/submitproof/proof_receiver.go
18-18: could not import scroll-tech/coordinator/libzkp (-: build constraints exclude all Go files in libzkp)
(typecheck)
coordinator/internal/logic/verifier/verifier.go
15-15: could not import scroll-tech/coordinator/libzkp (-: build constraints exclude all Go files in libzkp)
(typecheck)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
🔇 Additional comments (10)
coordinator/internal/logic/provertask/prover_task.go (1)
16-17: ```shell
#!/bin/bash
set -eecho "Module path in coordinator/go.mod:"
grep -m1 '^module ' coordinator/go.mod</details> <details> <summary>coordinator/libzkp/universal_task.go (2)</summary> `11-36`: **Well-implemented C FFI function with proper memory management.** The implementation correctly handles: - Memory management with proper `defer` cleanup of C strings - Error handling by checking the `result.ok` field - Type conversions between C and Go types - Resource cleanup with `defer C.release_task_result(result)` --- `30-33`: Let’s locate all occurrences of `expected_pi_hash` to find its definition and usages: ```shell #!/bin/bash # Search for all references to expected_pi_hash in the codebase rg -n "expected_pi_hash"coordinator/libzkp/mock_universal_task.go (2)
14-42: Well-structured mock implementation for testing.The mock function correctly:
- Matches the real function signature
- Handles different task types with appropriate metadata structures
- Includes proper error handling for JSON marshaling
- Uses build tags to exclude from production builds
- Provides useful debug logging
19-33: Verify task type constants are accessible.The code references
TaskTypeChunk,TaskTypeBatch, andTaskTypeBundleconstants but they're not imported or defined in this file. Ensure these constants are properly accessible from the package scope.#!/bin/bash # Verify task type constants are defined and accessible rg -A 3 "TaskType(Chunk|Batch|Bundle)" coordinator/libzkp/coordinator/Makefile (2)
5-5: Build system refactoring looks good.The changes properly:
- Update the libzkp path to the new local directory structure
- Delegate build operations to the libzkp subdirectory Makefile
- Update target dependencies to use the new path
Also applies to: 20-21, 28-28
8-12: Let’s locate the actualCargo.lockand confirm our grep patterns against it:#!/bin/bash set -e echo "🔍 Searching for Cargo.lock..." find . -type f -name Cargo.lock echo -e "\n📄 Verifying patterns in each Cargo.lock:" find . -type f -name Cargo.lock | while read -r lock; do echo -e "\n--- $lock ---" echo "* Searching for zkvm-prover entries *" grep -n -C1 "zkvm-prover" "$lock" || echo "⚠️ No matches for zkvm-prover" echo -e "\n* Searching for openvm.git entries *" grep -n -C1 "openvm.git" "$lock" || echo "⚠️ No matches for openvm.git" donecoordinator/libzkp/lib.go (3)
111-114: Excellent safety improvement for C pointer handling.The conditional assignment of
cVkDataprevents undefined behavior whenvkDatais an empty slice, as taking the address of the first element of an empty slice is unsafe.
99-101: Clean delegation pattern for universal task generation.The refactoring properly separates the public API from the internal implementation, allowing for different implementations (real vs mock) via build tags.
79-83: ```shell
#!/bin/bashSearch for TaskType enum definition and its variants in Rust source files
rg 'enum TaskType' -n --glob '.rs'
rg 'TaskType' -n --glob '.rs' -A3 -B3</details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
coordinator/internal/logic/submitproof/proof_receiver.go (1)
22-22: Same import resolution issue as in prover_task.go.This file has the same build constraint issue preventing the import of the new libzkp package.
coordinator/internal/logic/verifier/verifier.go (1)
18-18: Same import resolution issue as other files.This file has the same build constraint issue preventing the import of the new libzkp package.
🧹 Nitpick comments (5)
coordinator/internal/logic/libzkp/Makefile (1)
3-6: Consider adding error handling and making the build more robust.The build process could be more robust with better error handling and validation.
build: - @cargo build --release -p libzkp-c - @mkdir -p lib - @cp -f ../../../../target/release/libzkp.so lib/ + @echo "Building libzkp-c crate..." + @cargo build --release -p libzkp-c || { echo "Build failed"; exit 1; } + @mkdir -p lib + @if [ -f ../../../../target/release/libzkp.so ]; then \ + cp -f ../../../../target/release/libzkp.so lib/; \ + echo "Library copied successfully"; \ + else \ + echo "Error: libzkp.so not found after build"; \ + exit 1; \ + ficoordinator/internal/logic/libzkp/universal_task.go (1)
11-36: The C FFI implementation looks correct but could be more robust.The memory management with defer statements is good, and the type conversions are handled properly. However, consider these improvements:
- Add input validation:
func generateUniversalTask(taskType int, taskJSON, forkName string) (bool, string, string, []byte) { + if taskJSON == "" || forkName == "" { + return false, "", "", nil + } + cTask := goToCString(taskJSON)
- Consider exporting the function if it's meant to be used outside the package:
-func generateUniversalTask(taskType int, taskJSON, forkName string) (bool, string, string, []byte) { +func GenerateUniversalTask(taskType int, taskJSON, forkName string) (bool, string, string, []byte) {coordinator/internal/logic/libzkp/mock_universal_task.go (3)
16-16: Consider using structured logging instead of fmt.Printf.For consistency with the rest of the codebase and better log management, consider using a structured logging framework instead of
fmt.Printf.- fmt.Printf("call mocked generate universal task %d, taskJson %s\n", taskType, taskJSON) + log.Info("call mocked generate universal task", "taskType", taskType, "taskJSON", taskJSON)
18-33: Consider adding more realistic mock data for better test coverage.The current mock implementation creates empty metadata structures, which may not adequately test downstream logic that expects populated data. Consider adding minimal but realistic mock data.
For example, populate some basic fields:
case TaskTypeChunk: metadata = struct { ChunkInfo *message.ChunkInfo `json:"chunk_info"` - }{ChunkInfo: &message.ChunkInfo{}} + }{ChunkInfo: &message.ChunkInfo{ + TotalL1MessagesPoppedBefore: 0, + TotalL1MessagesPoppedInBatch: 0, + }}
37-37: Improve error logging consistency.Use the same logging approach as the success case for consistency.
- fmt.Println("mock encoding json fail:", err) + fmt.Printf("mock encoding json fail: %v\n", err)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.work.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
.github/workflows/coordinator.yml(0 hunks).github/workflows/integration.yml(1 hunks)build/dockerfiles/coordinator-api.Dockerfile(3 hunks)coordinator/Makefile(2 hunks)coordinator/internal/logic/libzkp/Makefile(1 hunks)coordinator/internal/logic/libzkp/e2e-test.sh(1 hunks)coordinator/internal/logic/libzkp/lib.go(3 hunks)coordinator/internal/logic/libzkp/mock_universal_task.go(1 hunks)coordinator/internal/logic/libzkp/universal_task.go(1 hunks)coordinator/internal/logic/provertask/prover_task.go(1 hunks)coordinator/internal/logic/submitproof/proof_receiver.go(1 hunks)coordinator/internal/logic/verifier/verifier.go(2 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/coordinator.yml
✅ Files skipped from review due to trivial changes (1)
- coordinator/internal/logic/libzkp/e2e-test.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/integration.yml
- build/dockerfiles/coordinator-api.Dockerfile
🧰 Additional context used
🪛 checkmake (0.2.2)
coordinator/Makefile
[warning] 26-26: Target "libzkp" should be declared PHONY.
(phonydeclared)
coordinator/internal/logic/libzkp/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
🪛 golangci-lint (1.64.8)
coordinator/internal/logic/provertask/prover_task.go
19-19: could not import scroll-tech/coordinator/internal/logic/libzkp (-: build constraints exclude all Go files in internal/logic/libzkp)
(typecheck)
coordinator/internal/logic/submitproof/proof_receiver.go
22-22: could not import scroll-tech/coordinator/internal/logic/libzkp (-: build constraints exclude all Go files in internal/logic/libzkp)
(typecheck)
coordinator/internal/logic/verifier/verifier.go
18-18: could not import scroll-tech/coordinator/internal/logic/libzkp (-: build constraints exclude all Go files in internal/logic/libzkp)
(typecheck)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: tests
🔇 Additional comments (6)
coordinator/internal/logic/provertask/prover_task.go (1)
19-19: ```shell
#!/bin/bashShow definition of GenerateUniversalTask in libzkp
rg -C3 "func GenerateUniversalTask" coordinator/internal/logic/libzkp
</details> <details> <summary>coordinator/internal/logic/verifier/verifier.go (1)</summary> `21-21`: **Verify that the referenced Rust source paths are correct.** The comments reference `libzkp/impl/src/verifier.rs` and `coordinator/internal/logic/libzkp/impl/src/verifier.rs`, but based on the AI summary, the new structure uses crates like `libzkp` and `libzkp-c`. Please ensure these path references are accurate. ```shell #!/bin/bash # Check if the referenced Rust files exist echo "=== Checking for referenced Rust verifier files ===" fd -t f "verifier.rs" . # Check the actual structure of the new libzkp crates echo "=== Checking Rust crate structure ===" fd -t f -e rs . crates/ 2>/dev/null || echo "No crates directory found" fd -t f -e rs . coordinator/internal/logic/libzkp/ 2>/dev/null || echo "No Rust files in libzkp directory"Also applies to: 36-36
coordinator/internal/logic/libzkp/universal_task.go (1)
5-9: Verify that the C header file exists and is properly configured.The code includes
libzkp.hbut there's no verification that this header file exists or that the C library is properly linked.#!/bin/bash # Check if the C header file exists echo "=== Checking for libzkp.h header file ===" fd -t f "libzkp.h" . # Check if there are any CGO configuration files echo "=== Checking for CGO configuration ===" rg -A 5 -B 5 "CGO_|#cgo" coordinator/internal/logic/libzkp/coordinator/Makefile (1)
37-38: ```shell
#!/bin/bashBroad search for any references to the commented-out Makefile target
rg -i "coordinator_api_skip_libzkp" || true
</details> <details> <summary>coordinator/internal/logic/libzkp/lib.go (2)</summary> `111-114`: **Excellent fix for null pointer dereference!** Good improvement to safely handle empty `vkData` slices by conditionally passing a nil pointer instead of attempting to dereference an empty slice. --- `79-83`: **Consider centralizing task type constants.** The task type constants are well-defined, but consider if these should be centralized with other constants or if they might conflict with constants defined elsewhere in the codebase. Check if similar constants are defined elsewhere that might conflict: ```shell #!/bin/bash # Search for other task type constants rg -A 5 -B 5 "TaskType|TASK_TYPE" --type go
5 participants
The refactoring of zkvm prover for "universal tasks", continue with the updates in #1682
The update is focus on the rust part:
cratesdirectorySummary by CodeRabbit
New Features
libzkp-c) for proof verification and task handling, with a Go FFI wrapper for coordinator integration.Refactor
Bug Fixes
Documentation
Chores
End-users benefit from a more robust, modular, and maintainable ZKP system with improved APIs, integration options, and documentation.