[Feat] For prover 4.6.1 #1742
[Feat] For prover 4.6.1 #1742
Conversation
WalkthroughUpdates workspace dependencies and version tag; changes coordinator build/config paths and DB DSN; adds a universal task compatibility fix implemented in Rust/C/Go and invoked during prover task assignment; adjusts e2e defaults and adds a Goose migration SQL wrapper. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Prover Client
participant Coord as Coordinator
participant PT as ProverTask Logic
participant GoFFI as Go libzkp wrapper
participant CFFI as C ABI
participant Rust as libzkp (Rust)
Client->>Coord: GetTask()
Coord->>PT: Assign(taskCtx, taskMsg)
PT->>PT: isCompatibilityFixingVersion(proverVersion)?
alt Needs fix
PT->>GoFFI: UnivTaskCompatibilityFix(taskJSON)
GoFFI->>CFFI: univ_task_compatibility_fix(char*)
CFFI->>Rust: univ_task_compatibility_fix(&str)
Rust-->>CFFI: Ok(fixed_json) / Err
CFFI-->>GoFFI: fixed_json or NULL
GoFFI-->>PT: fixed_json or error
alt Error
PT-->>Coord: return internal error
Coord-->>Client: 5xx (task assignment failed)
else Success
PT->>PT: replace schema.task with fixed_json
PT-->>Coord: continue assignment flow
Coord-->>Client: Task response
end
else No fix
PT-->>Coord: continue assignment flow
Coord-->>Client: Task response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
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. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
![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
🧹 Nitpick comments (10)
coordinator/test/api_test.go (1)
135-135: Ensure version bump intent and keep test expectations in syncSetting version.Version = "v4.5.45" is fine for simulating a newer prover; just confirm it aligns with the configured min_prover_version in these tests (currently "v4.4.89" in setupCoordinator) and with runtime defaults to avoid drift.
Optionally, define a single test constant for the minimum version and reuse it in both the conf and expected error strings to avoid future mismatches.
coordinator/conf/config.json (1)
25-25: Avoid committing literal DB credentials in templatesHardcoding dev:dev triggers secret scanners and can leak into ephemeral environments.
Apply this diff to use a neutral placeholder (compatible with local envsubst or manual edits):
- "dsn": "postgres://dev:dev@localhost/scroll?sslmode=disable", + "dsn": "postgres://${DB_USER}:${DB_PASS}@${DB_HOST:-localhost}/scroll?sslmode=disable",If you don’t plan to expand env vars, at least switch to obvious placeholders to silence scanners (e.g., :).
Also confirm min_prover_version here matches your rollout plan (PR text mentions compatibility with 4.5.33; current value is "v4.4.45").
coordinator/internal/logic/libzkp/libzkp.h (1)
59-60: Clarify ownership and use const for the input parameterMatch the Rust C-ABI (takes *const c_char) and document that the caller must free the returned string via release_string to avoid leaks.
Apply this diff:
-// Universal task compatibility fix function -char* univ_task_compatibility_fix(char* task_json); +// Universal task compatibility fix function +// Returns a newly-allocated JSON string; caller must call release_string() on the result. +char* univ_task_compatibility_fix(const char* task_json);crates/libzkp_c/src/lib.rs (1)
241-252: Fix typo in error log and add a null check for robustness.
- Typo: "compability" → "compatibility".
- Add a null check to avoid UB if a non-Go caller ever passes NULL.
pub unsafe extern "C" fn univ_task_compatibility_fix(task_json: *const c_char) -> *mut c_char { - let task_json_str = c_char_to_str(task_json); + if task_json.is_null() { + tracing::error!("univ_task_compatibility_fix received null task_json"); + return std::ptr::null_mut(); + } + let task_json_str = c_char_to_str(task_json); match libzkp::univ_task_compatibility_fix(task_json_str) { Ok(result) => CString::new(result).unwrap().into_raw(), Err(e) => { - tracing::error!("univ_task_compability_fix failed, error: {:#}", e); + tracing::error!("univ_task_compatibility_fix failed, error: {:#}", e); std::ptr::null_mut() } } }coordinator/build/setup_releases.sh (1)
10-10: Harden the script: strict mode, tool checks, and download failure handling.Add strict flags and pre-flight checks; consider retrying downloads to avoid flaky CI.
Additional changes (outside the selected lines) to add after the shebang:
set -Eeuo pipefail command -v jq >/dev/null || { echo "jq is required"; exit 1; } command -v wget >/dev/null || { echo "wget is required"; exit 1; }Optionally wrap wget with retries:
wget --tries=3 --timeout=30 https://circuit-release.s3.us-west-2.amazonaws.com/scroll-zkvm/releases/$SCROLL_ZKVM_VERSION/verifier/verifier.bin -O "${ASSET_DIR}/verifier.bin" wget --tries=3 --timeout=30 https://circuit-release.s3.us-west-2.amazonaws.com/scroll-zkvm/releases/$SCROLL_ZKVM_VERSION/verifier/root_verifier_vk -O "${ASSET_DIR}/root_verifier_vk" wget --tries=3 --timeout=30 https://circuit-release.s3.us-west-2.amazonaws.com/scroll-zkvm/releases/$SCROLL_ZKVM_VERSION/verifier/openVmVk.json -O "${ASSET_DIR}/openVmVk.json"Also applies to: 56-56
tests/prover-e2e/Makefile (1)
4-5: Validate numeric inputs for BEGIN_BLOCK/END_BLOCK.Prevent accidental non-numeric values that could break the tool invocation.
check_vars: - @if [ -z "$(BEGIN_BLOCK)" ] || [ -z "$(END_BLOCK)" ]; then \ + @if [ -z "$(BEGIN_BLOCK)" ] || [ -z "$(END_BLOCK)" ]; then \ echo "Error: BEGIN_BLOCK and END_BLOCK must be defined"; \ echo "Usage: make import_data BEGIN_BLOCK=<start_block> END_BLOCK=<end_block>"; \ exit 1; \ fi + @if ! echo "$(BEGIN_BLOCK)" | grep -Eq '^[0-9]+$$'; then \ + echo "Error: BEGIN_BLOCK must be numeric"; exit 1; \ + fi + @if ! echo "$(END_BLOCK)" | grep -Eq '^[0-9]+$$'; then \ + echo "Error: END_BLOCK must be numeric"; exit 1; \ + fiCargo.toml (1)
20-23: Nit: fix comment typo and consider aligning workspace version with release.
- Typo: "compatiblity" → "compatibility".
- Optional: bump
[workspace.package].versionfrom4.5.47to4.6.1for consistency with the PR headline.-# include compatiblity fixing from "fix/coordinator" +# include compatibility fixing from "fix/coordinator"If intended, also update (outside this hunk):
- version = "4.5.47" -> "4.6.1"
crates/libzkp/src/lib.rs (1)
49-88: Polish docs; verify byte encoding expectations for compatibility.
- Fix typos in doc comments.
- Confirm whether vk should serialize as base64 (like elsewhere via vec_as_base64). If the old prover expects raw arrays, current code is fine; otherwise, add serde attribute.
/// Convert the universal task json into compatible form for old prover pub fn univ_task_compatibility_fix(task_json: &str) -> eyre::Result<String> { @@ #[derive(Serialize)] struct CompatibleProvingTask { - /// seralized witness which should be written into stdin first + /// serialized witness which should be written into stdin first pub serialized_witness: Vec<Vec<u8>>, /// aggregated proof carried by babybear fields, should be written into stdin - /// followed `serialized_witness` + /// followed by `serialized_witness` pub aggregated_proofs: Vec<VmInternalStarkProof>, /// Fork name specify pub fork_name: String, - /// The vk of app which is expcted to prove this task + /// The vk of the app which is expected to prove this task pub vk: Vec<u8>, - /// An identifier assigned by coordinator, it should be kept identify for the - /// same task (for example, using chunk, batch and bundle hashes) + /// An identifier assigned by coordinator; it should be kept identical for the + /// same task (e.g., using chunk, batch, and bundle hashes) pub identifier: String, }If base64 encoding is required for
vk, use:
- Add
use scroll_zkvm_types::utils::vec_as_base64;(already available in this module)- Annotate:
#[serde(with = "vec_as_base64")] pub vk: Vec<u8>,coordinator/internal/logic/provertask/prover_task.go (2)
207-210: Function naming and minor robustnessRename to reads-as-boolean and trim input; use the renamed constant.
Apply this diff:
-func isCompatibilityFixingVersion(ver string) bool { - return !version.CheckScrollRepoVersion(ver, CompatibilityVersion) -} +// requiresCompatibilityFix returns true if the prover version is below MinCompatFixVersion. +func requiresCompatibilityFix(ver string) bool { + ver = strings.TrimSpace(ver) + return !version.CheckScrollRepoVersion(ver, MinCompatFixVersion) +}Note: Update call sites accordingly in batch_prover_task.go and bundle_prover_task.go.
211-220: Defensive nil-check and error context in fixCompatibilityAvoid potential panic on nil schema and wrap errors for easier triage. Also confirm that only TaskData needs adjustment (no HardForkName/UseSnark changes for older provers).
Apply this diff:
-func fixCompatibility(schema *coordinatorType.GetTaskSchema) error { - - fixedTask, err := libzkp.UnivTaskCompatibilityFix(schema.TaskData) - if err != nil { - return err - } - schema.TaskData = fixedTask - - return nil -} +// fixCompatibility applies the universal-task compatibility fix in-place. +func fixCompatibility(schema *coordinatorType.GetTaskSchema) error { + if schema == nil { + return errors.New("fixCompatibility: nil schema") + } + fixedTask, err := libzkp.UnivTaskCompatibilityFix(schema.TaskData) + if err != nil { + return fmt.Errorf("fixCompatibility: UnivTaskCompatibilityFix failed: %w", err) + } + schema.TaskData = fixedTask + return nil +}If the legacy task format also differed in fields other than TaskData for < MinCompatFixVersion, adjust here (or document why only TaskData is impacted).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
Cargo.toml(1 hunks)common/version/version.go(1 hunks)coordinator/Makefile(1 hunks)coordinator/build/setup_releases.sh(2 hunks)coordinator/conf/config.json(1 hunks)coordinator/internal/logic/libzkp/lib.go(1 hunks)coordinator/internal/logic/libzkp/libzkp.h(1 hunks)coordinator/internal/logic/provertask/batch_prover_task.go(1 hunks)coordinator/internal/logic/provertask/bundle_prover_task.go(1 hunks)coordinator/internal/logic/provertask/prover_task.go(2 hunks)coordinator/test/api_test.go(1 hunks)crates/libzkp/src/lib.rs(1 hunks)crates/libzkp_c/src/lib.rs(1 hunks)tests/prover-e2e/Makefile(1 hunks)tests/prover-e2e/prepare/dump_block_records.sql(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
coordinator/internal/logic/libzkp/libzkp.h (2)
crates/libzkp/src/lib.rs (1)
univ_task_compatibility_fix(50-88)crates/libzkp_c/src/lib.rs (1)
univ_task_compatibility_fix(243-252)
coordinator/internal/logic/provertask/bundle_prover_task.go (3)
coordinator/internal/types/auth.go (1)
ProverVersion(20-20)coordinator/internal/logic/provertask/prover_task.go (1)
ErrCoordinatorInternalFailure(27-27)coordinator/internal/logic/submitproof/proof_receiver.go (1)
ErrCoordinatorInternalFailure(47-47)
coordinator/internal/logic/provertask/batch_prover_task.go (2)
coordinator/internal/types/auth.go (1)
ProverVersion(20-20)coordinator/internal/logic/provertask/prover_task.go (1)
ErrCoordinatorInternalFailure(27-27)
coordinator/internal/logic/provertask/prover_task.go (3)
common/version/prover_version.go (1)
CheckScrollRepoVersion(37-55)coordinator/internal/types/get_task.go (1)
GetTaskSchema(12-19)coordinator/internal/logic/libzkp/lib.go (1)
UnivTaskCompatibilityFix(152-166)
crates/libzkp_c/src/lib.rs (2)
crates/libzkp/src/lib.rs (1)
univ_task_compatibility_fix(50-88)crates/libzkp_c/src/utils.rs (1)
c_char_to_str(3-6)
crates/libzkp/src/lib.rs (1)
crates/libzkp_c/src/lib.rs (1)
univ_task_compatibility_fix(243-252)
coordinator/test/api_test.go (1)
common/version/version.go (1)
Version(31-31)
🪛 Checkov (3.2.334)
coordinator/conf/config.json
[medium] 25-26: Basic Auth Credentials
(CKV_SECRET_4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
🔇 Additional comments (7)
common/version/version.go (1)
8-8: Version tag bump to v4.6.1 looks goodNo behavioral change beyond the tag value.
Confirm the build embeds ZkVersion via ldflags as intended (Makefile uses -X scroll-tech/common/version.ZkVersion=${ZK_VERSION}) so Version resolves to v4.6.1--.
coordinator/Makefile (1)
38-41: Config template path change: LGTMCreating build/bin/conf and copying a single config.json to config.template.json matches the new setup flow.
Double-check coordinator/build/setup_releases.sh expects bin/conf/config.template.json (new path) and not the old copied conf directory.
tests/prover-e2e/prepare/dump_block_records.sql (1)
2-9: Goose migration header emission looks correctPrinting Goose markers through SELECT with unaligned output is a pragmatic approach for generating the migration file.
Ensure psql settings (\t on, \a) yield no extra spacing in your CI environment so Goose parses the headers exactly.
coordinator/internal/logic/provertask/bundle_prover_task.go (1)
215-221: Compatibility fix hook for universal bundle tasks: LGTMApplied post-applyUniversal with clear logging and safe failure handling.
Please add/confirm a test that requests a universal task with an older prover version to exercise this path and validate the transformed schema.
coordinator/internal/logic/provertask/batch_prover_task.go (1)
217-223: Compatibility fix hook for universal batch tasks: LGTMPlacement and error handling are appropriate.
Ensure the same coverage exists for batch universal tasks with an older prover version to catch regressions in the compatibility transformer.
coordinator/internal/logic/provertask/prover_task.go (2)
17-17: Import of version package: LGTMThe dependency is appropriate for version gating logic.
205-206: Rename CompatibilityVersion → MinCompatFixVersion and document semanticsCompatibilityVersion is ambiguous; it represents the minimum prover version that does NOT need the compatibility fix — rename and add a short doc comment.
-const CompatibilityVersion = "4.5.43" +// MinCompatFixVersion is the minimum prover version that does NOT need the compatibility fix. +const MinCompatFixVersion = "4.5.43"
- Update references that call isCompatibilityFixingVersion in:
- coordinator/internal/logic/provertask/prover_task.go
- coordinator/internal/logic/provertask/bundle_prover_task.go
- coordinator/internal/logic/provertask/batch_prover_task.go
- Confirm the numeric threshold: Cargo.toml uses 4.5.47 and Cargo.lock contains 4.5.39 / 4.5.32; the PR claims compatibility with 4.5.33 — verify whether the constant should remain 4.5.43 or be lowered to 4.5.33 (and adjust tests/docs accordingly).
| // UnivTaskCompatibilityFix calls the universal task compatibility fix function | ||
| func UnivTaskCompatibilityFix(taskJSON string) (string, error) { | ||
| cTaskJSON := goToCString(taskJSON) | ||
| defer freeCString(cTaskJSON) | ||
|
|
||
| resultPtr := C.univ_task_compatibility_fix(cTaskJSON) | ||
| if resultPtr == nil { | ||
| return "", fmt.Errorf("univ_task_compatibility_fix failed") | ||
| } | ||
|
|
||
| // Convert result to Go string and free C memory | ||
| result := C.GoString(resultPtr) | ||
| C.release_string(resultPtr) | ||
|
|
||
| return result, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
FFI wrapper looks good; confirm header const-correctness.
Memory handling is correct (free input, release returned). Confirm libzkp.h declares const char* for the parameter.
🏁 Script executed:
#!/bin/bash
# Show the header declaration for univ_task_compatibility_fix
fd -a libzkp.h
rg -n -C2 'univ_task_compatibility_fix' $(fd -a libzkp.h)Length of output: 197
Change header to use const char for input*
libzkp.h currently declares char* univ_task_compatibility_fix(char* task_json); — update the parameter to const char* task_json (return char* can remain). File: coordinator/internal/logic/libzkp/libzkp.h:60
🤖 Prompt for AI Agents
coordinator/internal/logic/libzkp/lib.go lines 151-166: update the C header
declaration to accept a const char* for the input parameter so it matches the
intended immutable input; open coordinator/internal/logic/libzkp/libzkp.h
(around line 60) and change the function prototype from char*
univ_task_compatibility_fix(char* task_json); to char*
univ_task_compatibility_fix(const char* task_json); then save the header so the
cgo build uses the new signature (no change needed in Go callsite since Go
passes a C string pointer for read-only input).
| /// # Safety | ||
| #[no_mangle] | ||
| pub unsafe extern "C" fn univ_task_compatibility_fix(task_json: *const c_char) -> *mut c_char { | ||
| let task_json_str = c_char_to_str(task_json); | ||
| match libzkp::univ_task_compatibility_fix(task_json_str) { | ||
| Ok(result) => CString::new(result).unwrap().into_raw(), | ||
| Err(e) => { | ||
| tracing::error!("univ_task_compability_fix failed, error: {:#}", e); | ||
| std::ptr::null_mut() | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Const-correctness with C header declaration.
Ensure the C header uses const char* for the parameter to match this implementation.
🏁 Script executed:
#!/bin/bash
# Verify the C declaration is const-correct
rg -n -C2 -P '\buniv_task_compatibility_fix\s*\(' coordinator | sed -n '1,200p'
rg -n -P '\buniv_task_compatibility_fix\s*\(\s*const\s+char\s*\*' || echo "Header may be missing const for parameter"Length of output: 791
Use const in C header parameter for univ_task_compatibility_fix
coordinator/internal/logic/libzkp/libzkp.h:60 currently has
char* univ_task_compatibility_fix(char* task_json);
Change to
char* univ_task_compatibility_fix(const char* task_json);
to match the Rust signature (task_json: *const c_char).
🤖 Prompt for AI Agents
In crates/libzkp_c/src/lib.rs around lines 241 to 252, the Rust FFI function
accepts a const c_char pointer but the C header declares a non-const char*;
update the C header declaration (coordinator/internal/logic/libzkp/libzkp.h line
~60) to use const char* for the task_json parameter so the signatures match
(char* univ_task_compatibility_fix(char* task_json); -> char*
univ_task_compatibility_fix(const char* task_json);) and then rebuild/regenerate
any C bindings or headers used by the project to ensure consistency.
| -- Write footer | ||
| SELECT ''; | ||
| SELECT '-- +goose StatementEnd'; | ||
| SELECT '-- +goose Down'; | ||
| SELECT '-- +goose StatementBegin'; | ||
| SELECT 'DELETE FROM l2_block;'; | ||
| SELECT '-- +goose StatementEnd'; |
There was a problem hiding this comment.
Limit the Down migration to rows inserted by this script
DELETE FROM l2_block; will wipe the entire table, which is risky even in e2e.
Apply this diff to target the same block range used above:
-SELECT 'DELETE FROM l2_block;';
+SELECT 'DELETE FROM l2_block WHERE number BETWEEN 10973700 AND 10973730;';If BEGIN_BLOCK/END_BLOCK are parameterized elsewhere, mirror them here to keep Up/Down symmetrical.
🤖 Prompt for AI Agents
In tests/prover-e2e/prepare/dump_block_records.sql around lines 31 to 37, the
Down migration currently does "DELETE FROM l2_block;" which will remove the
entire table; change it to delete only the rows inserted by this script by
adding a WHERE clause that mirrors the same block range used in the Up (e.g.
"WHERE block_number BETWEEN <BEGIN_BLOCK> AND <END_BLOCK>" or whatever parameter
names are used), and if BEGIN_BLOCK/END_BLOCK are parameterized elsewhere,
reference the same parameter names here so the Up and Down are symmetrical and
only target the inserted rows.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #1742 +/- ##
===========================================
- Coverage 36.96% 36.91% -0.06%
===========================================
Files 245 245
Lines 20893 20930 +37
===========================================
+ Hits 7724 7727 +3
- Misses 12351 12384 +33
- Partials 818 819 +1
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:
|
This PR is prepared for prover 4.6.1, in which zkvm use openvm 1.4. It has updated the zkvm-prover dependency and cherry-pick some commits from
cloakbranchIt is also ready for coordinator to be deployed in mainnet (compatible with the online prover
4.5.33enabling us a smooth upgrading)local e2e test has passed for following configurations:
cloakbranch) + prover (built with current PR) + guest 0.6.0-rc.1Summary by CodeRabbit
New Features
Build/Setup
Configuration
Tests
Chores