[Fix] Compatible with prover from 4.5.33 #1733
Conversation
WalkthroughAdds a universal-task compatibility fix: new Rust helper and C-ABI export, a C header and Go wrapper, version-gated invocation in universal Assign flows (Batch/Bundle), workspace dependency rev bumps, and minor test/version constant updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Assign as ProverTask.Assign (Universal)
participant Version as version.CheckScrollRepoVersion
participant GoFFI as UnivTaskCompatibilityFix (Go)
participant C as univ_task_compatibility_fix (C)
participant Rust as libzkp::univ_task_compatibility_fix
Assign->>Version: isCompatibilityFixingVersion(proverVersion)
alt requires compatibility fix
Assign->>GoFFI: UnivTaskCompatibilityFix(taskMsg.TaskData)
GoFFI->>C: pass C string
C->>Rust: call univ_task_compatibility_fix(&str)
Rust-->>C: Ok(JSON) / Err
alt Success
C-->>GoFFI: char* (JSON)
GoFFI-->>Assign: string (JSON)
Assign->>Assign: update taskMsg.TaskData
Assign-->>Caller: continue flow
else Error
C-->>GoFFI: null
GoFFI-->>Assign: error
Assign-->>Caller: ErrCoordinatorInternalFailure
end
else no fix needed
Assign-->>Caller: continue flow
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (5)
crates/libzkp/src/lib.rs (2)
42-56: Fix doc typos; optionally encode vk as base64 if legacy expects it.
- Typos in comments: “seralized” → “serialized”; “expcted” → “expected”; “identify” → “identical”.
- If the legacy prover expects vk as base64 in JSON, add serde attr (vk is bytes):
Apply this diff to fix typos (and optionally add base64 encoding for vk):
#[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` pub aggregated_proofs: Vec<VmInternalStarkProof>, /// Fork name specify pub fork_name: String, - /// The vk of app which is expcted to prove this task - pub vk: Vec<u8>, - /// An identifier assigned by coordinator, it should be kept identify for the + /// The vk of app which is expected to prove this task + /// NOTE: if legacy expects base64, keep the following line; otherwise drop it. + #[serde(with = "vec_as_base64")] + pub vk: Vec<u8>, + /// An identifier assigned by coordinator, it should be kept identical for the /// same task (for example, using chunk, batch and bundle hashes) pub identifier: String, }Please confirm the expected JSON shape (arrays vs base64 strings) for both vk and serialized_witness for prover 4.5.33.
58-67: Return value handling is fine. Consider unit test coverage.Recommend a small test: input a minimal ProvingTask JSON, assert output contains expected fields/types.
I can draft a Rust unit test exercising this transformation if helpful.
coordinator/internal/logic/libzkp/libzkp.h (1)
57-59: Document ownership and use const-correct param for FFI.Return is an allocated C string; callers must release it. Make input const.
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 on success, or NULL on error. +// The caller must call release_string on the returned pointer when done. +char* univ_task_compatibility_fix(const char* task_json);coordinator/internal/logic/libzkp/lib.go (1)
144-159: FFI wrapper looks correct; memory ownership handled properly.The CGO string conversions and release_string usage are consistent with the rest of the file. No issues spotted.
Optional: pre-validate JSON to fail fast before FFI and add more context to the error.
import ( "fmt" "os" "strings" "unsafe" + "encoding/json" @@ func UnivTaskCompatibilityFix(taskJSON string) (string, error) { + if !json.Valid([]byte(taskJSON)) { + return "", fmt.Errorf("invalid universal task JSON") + } cTaskJSON := goToCString(taskJSON) defer freeCString(cTaskJSON) resultPtr := C.univ_task_compatibility_fix(cTaskJSON) if resultPtr == nil { - return "", fmt.Errorf("univ_task_compatibility_fix failed") + return "", fmt.Errorf("univ_task_compatibility_fix failed (see native logs)") }coordinator/internal/logic/provertask/prover_task.go (1)
211-220: Add observability around applied compatibility fix.A lightweight info/debug log or metric when the fix is applied will aid incident triage without reading native logs.
func fixCompatibility(schema *coordinatorType.GetTaskSchema) error { - - fixedTask, err := libzkp.UnivTaskCompatibilityFix(schema.TaskData) + fixedTask, err := libzkp.UnivTaskCompatibilityFix(schema.TaskData) if err != nil { return err } schema.TaskData = fixedTask - + // log at debug to avoid noise; promotes traceability + // log.Debug("applied universal task compatibility fix") return nil }Optional: default behavior on unparsable prover versions currently applies the fix (negation returns true on parse error). Consider failing closed with a warning, or explicitly handling unknown versions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockcrates/gpu_override/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
Cargo.toml(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)crates/libzkp/src/lib.rs(1 hunks)crates/libzkp_c/src/lib.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
crates/libzkp/src/lib.rs (1)
crates/libzkp_c/src/lib.rs (1)
univ_task_compatibility_fix(253-262)
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(145-159)
coordinator/internal/logic/provertask/batch_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/bundle_prover_task.go (2)
coordinator/internal/types/auth.go (1)
ProverVersion(20-20)coordinator/internal/logic/provertask/prover_task.go (1)
ErrCoordinatorInternalFailure(27-27)
crates/libzkp_c/src/lib.rs (2)
crates/libzkp/src/lib.rs (1)
univ_task_compatibility_fix(29-67)crates/libzkp_c/src/utils.rs (1)
c_char_to_str(3-6)
coordinator/internal/logic/libzkp/libzkp.h (2)
crates/libzkp/src/lib.rs (1)
univ_task_compatibility_fix(29-67)crates/libzkp_c/src/lib.rs (1)
univ_task_compatibility_fix(253-262)
⏰ 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 (4)
Cargo.toml (1)
20-22: ZKVM deps bump looks consistent (all pinned to the same commit).No issues from this change alone.
crates/libzkp/src/lib.rs (1)
28-41: Universal-task transformation logic LGTM.Moves instead of clones; field mapping for aggregated_proofs looks correct.
coordinator/internal/logic/provertask/bundle_prover_task.go (1)
215-221: I've requested scripts to verify the version gate constant, locaterecoverActiveAttempts, and inspect the error-handling snippet. Once the outputs are in, I can confirm if the suggested changes and version threshold are accurate.coordinator/internal/logic/provertask/prover_task.go (1)
17-17: Importing version helper is appropriate.This centralizes semver checks; no concerns.
| const CompatibilityVersion = "4.5.43" | ||
|
|
||
| func isCompatibilityFixingVersion(ver string) bool { | ||
| return !version.CheckScrollRepoVersion(ver, CompatibilityVersion) | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Version gate likely inverted and threshold appears inconsistent with PR intent.
- Current code applies the fix when proverVersion < CompatibilityVersion due to the negation.
- PR title/description says “Compatible with prover from 4.5.33,” but the constant is "4.5.43". If the fix is required for 4.5.33 and newer, the predicate should not be negated and the constant should be 4.5.33. If instead the fix is for older provers, set the constant accordingly and keep the negation. Please confirm the intended direction.
Proposed if the fix targets >= 4.5.33:
-const CompatibilityVersion = "4.5.43"
+const CompatibilityVersion = "4.5.33"
@@
-func isCompatibilityFixingVersion(ver string) bool {
- return !version.CheckScrollRepoVersion(ver, CompatibilityVersion)
-}
+func isCompatibilityFixingVersion(ver string) bool {
+ // apply fix for provers >= 4.5.33
+ return version.CheckScrollRepoVersion(ver, CompatibilityVersion)
+}To verify usages and avoid regressions, run:
If the fix is intended for “older than X,” replace X with the true minimum and keep the negation, but please update the comment to state that explicitly.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -nP 'CompatibilityVersion|isCompatibilityFixingVersion\s*\(' -C3
rg -nP 'CheckScrollRepoVersion\(' common/versionLength of output: 2956
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Dump implementation of CheckScrollRepoVersion
sed -n '1,200p' common/version/prover_version.go
# Dump tests for CheckScrollRepoVersion
sed -n '1,200p' common/version/prover_version_test.goLength of output: 3503
Align CompatibilityVersion and predicate with intended target version
- CompatibilityVersion is set to
"4.5.43"but the PR description specifies compatibility from4.5.33; update the constant to the actual minimum version. - The
!CheckScrollRepoVersionnegation means fixes apply only to provers < CompatibilityVersion; if the fix should target provers ≥ the minimum version, remove the negation (i.e. usereturn CheckScrollRepoVersion(ver, CompatibilityVersion)).
🤖 Prompt for AI Agents
In coordinator/internal/logic/provertask/prover_task.go around lines 205 to 209,
the CompatibilityVersion constant and predicate are wrong for the intended
target; change the constant value from "4.5.43" to "4.5.33" and remove the
negation so the function returns version.CheckScrollRepoVersion(ver,
CompatibilityVersion) (i.e., fixes apply to provers >= the minimum version).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
coordinator/test/api_test.go (1)
135-135: Avoid hardcoding the test prover version; derive it from the compat threshold to prevent driftBumping to v4.5.45 is fine and should exercise the new compatibility path. To keep tests resilient when the threshold changes, compute it from the source of truth.
Apply within this line:
- version.Version = "v4.5.45" + version.Version = fmt.Sprintf("v%s", provertask.CompatibilityVersion)Add the import:
import ( // ... provertask "scroll-tech/coordinator/internal/logic/provertask" )Also verify that
CompatibilityVersionmatches the PR intent (“from 4.5.33”) and thatisCompatibilityFixingVersion("v4.5.45")evaluates true. If the minimum allowed version in setup (v4.4.89) changes later, consider building the expected error message in TestOutdatedProverVersion fromconf.ProverManager.Verifier.MinProverVersionto avoid string skew.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
coordinator/test/api_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
coordinator/test/api_test.go (1)
common/version/version.go (1)
Version(31-31)
⏰ 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: tests
- GitHub Check: check
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #1733 +/- ##
===========================================
+ Coverage 37.47% 37.49% +0.01%
===========================================
Files 243 243
Lines 20534 20571 +37
===========================================
+ Hits 7696 7713 +17
- Misses 12027 12039 +12
- Partials 811 819 +8
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:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
common/version/version.go (1)
8-8: Optional: inject tag via -ldflags instead of hardcodingAvoid PR churn by defaulting to a dev tag and setting the real tag at build time.
Apply:
-var tag = "v4.5.46" +// Overridable via: go build -ldflags "-X scroll-tech/common/version.tag=v4.5.46" +var tag = "v0.0.0-dev"Example (Makefile/CI):
go build -ldflags "-X scroll-tech/common/version.tag=$(GIT_TAG)"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
common/version/version.go(1 hunks)
⏰ 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: check
- GitHub Check: tests
- GitHub Check: tests
🔇 Additional comments (1)
common/version/version.go (1)
8-8: Version gates normalize the ‘v’ prefix via Masterminds/semver
CheckScrollRepoVersion (and by extension isCompatibilityFixingVersion) uses github.com/Masterminds/semver’s NewConstraint/NewVersion, which coerces and strips a leading “v” when parsing version strings (github.com). Bumping the tag to “v4.5.46” will not misfire any compatibility or upgrade checks.
Unexpected serde behavior in OpenVm proof has broken the backward compatibility of coordinator. This PR detect the prover version and apply fixing to the encoding of universal task to resume the compatibility
Summary by CodeRabbit
New Features
Bug Fixes
Chores