feat: upgrade zkvm-prover to feynman fork #1686
Conversation
WalkthroughThis update introduces dependency upgrades, branch and revision changes in Rust workspace manifests, and configuration adjustments for supported cryptographic parameters. It also enhances error logging in Go coordinator modules, revises proof verification logic, updates serialization and witness construction in Rust prover code, and adds tracing initialization for C and Go FFI integration. Changes
Sequence Diagram(s)sequenceDiagram
participant GoApp as Go Coordinator
participant LibZKP_C as C FFI Layer
participant RustZKP as Rust Prover
GoApp->>LibZKP_C: Package init (calls init_tracing)
LibZKP_C->>LibZKP_C: Initialize tracing subscriber (once)
LibZKP_C-->>GoApp: Tracing ready
GoApp->>LibZKP_C: Assign prover task (calls into Rust)
LibZKP_C->>RustZKP: Generate prover task
RustZKP-->>LibZKP_C: Return result (with improved error logging)
LibZKP_C-->>GoApp: Task result (error details included in logs)
sequenceDiagram
participant ProverBin as Prover CLI
participant LocalProver as LocalProver
participant Handler as CircuitsHandler
ProverBin->>LocalProver: dump_verifier_assets(fork, path)
LocalProver->>Handler: get_vk(Chunk), get_vk(Batch), get_vk(Bundle) (async)
Handler-->>LocalProver: VK strings
LocalProver->>LocalProver: Write VKDump JSON, copy verifier.bin
LocalProver-->>ProverBin: Done
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
⏰ Context from checks skipped due to timeout of 90000ms (3)
✨ 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 (
|
+ fix compatibility of universal task for old fork
![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
♻️ Duplicate comments (1)
crates/libzkp/src/tasks/chunk.rs (1)
74-74: Verify serialization method compatibility and error handling.The serialization method has been changed from
to_rkyv_bytestowitness.rkyv_serialize(None)?. This is a significant change that could affect data compatibility.Please verify that the new serialization method produces compatible output:
#!/bin/bash # Description: Search for any remaining usage of the old serialization method # Check for any remaining usage of to_rkyv_bytes rg "to_rkyv_bytes" --type rust # Check for similar serialization pattern changes rg "rkyv_serialize" --type rust -A 2 -B 2 # Look for any error handling related to RancorError rg "RancorError" --type rustThe change from
to_rkyv_bytes::<RancorError>torkyv_serialize(None)?suggests a different error handling approach. Ensure this doesn't introduce silent failures.This appears related to the "EuclidV2" upgrade mentioned in the past review comment.
🧹 Nitpick comments (5)
zkvm-prover/.work/chunk/openvm.toml (1)
40-58: Newstruct_nameattributes per curveAll three curve entries now expose
struct_name.
If this replaces implicit naming conventions, ensure the verifier/prover selects curves via this field rather than by array index or hard-coded constants.crates/libzkp/Cargo.toml (1)
11-11: Clarify the "native-keccak" suppression comment.The comment mentions depressing the effect of "native-keccak" but doesn't explain why this is necessary or what specific issue it addresses. Consider adding more context to help future maintainers understand the reasoning.
crates/libzkp_c/src/lib.rs (1)
107-107: Consider removing debug console logging.The addition of
println!alongside existingtracing::error!creates redundant logging. Consider keeping only the structured logging approach for consistency.-println!("gen_universal_task failed at pre interpret step, error: {e}");Cargo.toml (2)
24-25: Consider pinning to a specific commit instead of a branch.The stateless-block-verifier dependencies are pointing to the
chore/upgradebranch, which is a moving target and can lead to non-reproducible builds.Consider pinning to a specific commit hash for better reproducibility:
-sbv-primitives = { git = "https://github.com/scroll-tech/stateless-block-verifier", branch = "chore/upgrade", features = ["scroll"] } -sbv-utils = { git = "https://github.com/scroll-tech/stateless-block-verifier", branch = "chore/upgrade" } +sbv-primitives = { git = "https://github.com/scroll-tech/stateless-block-verifier", rev = "<specific-commit-hash>", features = ["scroll"] } +sbv-utils = { git = "https://github.com/scroll-tech/stateless-block-verifier", rev = "<specific-commit-hash>" }
63-64: Review Forked Dependencies in Cargo.tomlFile: Cargo.toml
Lines: 63–64ruint = { git = "https://github.com/scroll-tech/uint.git", branch = "v1.15.0" } alloy-primitives = { git = "https://github.com/scroll-tech/alloy-core", branch = "v1.2.0" }Implications of these forks
- scroll-tech/uint.git v1.15.0 is tailored for Scroll’s zkEVM (custom APIs, performance tweaks) and may lag behind or diverge from the upstream
ruintcrate, which targets general Rust usage and enjoys broader maintenance.- scroll-tech/alloy-core v1.2.0 likely follows a similar pattern versus the upstream
alloy-primitivescrate.Recommendations
- Contribute Scroll-specific fixes/features back upstream where feasible.
- Document the rationale for each fork in your README or inline comments.
- Set up a regular sync process to pull in security patches and bugfixes from upstream.
- Plan a migration path to the upstream crates once required changes are merged.
For a definitive comparison, run a side-by-side
git diffbetween each forked branch and its upstream release to catalog exact API or implementation differences.
📜 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 (19)
Cargo.toml(2 hunks)coordinator/conf/config.json(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/chunk_prover_task.go(1 hunks)crates/l2geth/src/rpc_client.rs(2 hunks)crates/libzkp/Cargo.toml(2 hunks)crates/libzkp/src/proofs.rs(3 hunks)crates/libzkp/src/tasks/batch.rs(3 hunks)crates/libzkp/src/tasks/batch/utils.rs(2 hunks)crates/libzkp/src/tasks/bundle.rs(2 hunks)crates/libzkp/src/tasks/chunk.rs(2 hunks)crates/libzkp_c/src/lib.rs(2 hunks)crates/prover-bin/src/main.rs(3 hunks)crates/prover-bin/src/prover.rs(3 hunks)crates/prover-bin/src/zk_circuits_handler.rs(1 hunks)crates/prover-bin/src/zk_circuits_handler/euclidV2.rs(2 hunks)zkvm-prover/.work/batch/openvm.toml(2 hunks)zkvm-prover/.work/chunk/openvm.toml(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
crates/libzkp/src/proofs.rs (1)
Learnt from: colinlyguo
PR: scroll-tech/scroll#1530
File: tests/integration-test/go.mod:21-21
Timestamp: 2024-10-20T15:30:18.084Z
Learning: In the `tests/integration-test` folder, updates to import statements or dependencies are not necessary since it's only used for testing purposes.
🧬 Code Graph Analysis (2)
crates/prover-bin/src/zk_circuits_handler.rs (3)
crates/prover-bin/src/zk_circuits_handler/euclidV2.rs (1)
get_vk(82-84)crates/prover-bin/src/zk_circuits_handler/euclid.rs (1)
get_vk(108-115)common/types/message/message.go (1)
ProofType(20-20)
crates/prover-bin/src/zk_circuits_handler/euclidV2.rs (3)
crates/prover-bin/src/zk_circuits_handler.rs (1)
get_vk(14-14)crates/prover-bin/src/zk_circuits_handler/euclid.rs (1)
get_vk(108-115)common/types/message/message.go (1)
ProofType(20-20)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
🔇 Additional comments (29)
zkvm-prover/.work/batch/openvm.toml (2)
27-29: Verify that the consumer expects an “array-of-tuples” forfp2.supported_moduliYou switched from an array of strings to an array whose single element is a two-item tuple.
If the loader previously iterated overVec<String>it will now panic/deserialise toNone.
Confirm that the corresponding struct has changed toVec<(String,String)>or similar.
30-32:struct_nameintroduced – ensure it’s wired through deserialisation
struct_name = "Bls12_381G1Affine"is new. Double-check that:
- The config schema contains this field (otherwise it will be silently ignored or trigger an unknown-field error depending on
serdeflags).- Down-stream code actually uses it instead of hard-coding curve names.
zkvm-prover/.work/chunk/openvm.toml (2)
20-27: Renamed keysupported_modulus→supported_moduli– audit codebaseThe pluralised field propagates here as well. Search for
"supported_modulus"usages in the prover/coordinator crates; stale references will break deserialisation at runtime.
30-32: Type change forfp2.supported_modulimirrors batch configSame concern as in the batch file: make sure the consumer expects a list of tuples, not strings.
crates/libzkp/Cargo.toml (1)
23-23: Verify c-kzg v2.0 API compatibilityYou’ve bumped
c-kzgfrom 1.0 to 2.0, which may introduce breaking changes. Before merging, please cross-check all uses of the crate against the v2.0 changelog or release notes and update any altered signatures or removed APIs.Key usage sites to review:
- crates/libzkp/src/tasks/batch.rs
•c_kzg::Bytes48
•point_eval::to_blob→c_kzg::Blob
•point_eval::blob_to_kzg_commitment→c_kzg::KzgCommitment
•point_eval::get_kzg_proof→c_kzg::KzgProof- crates/libzkp/src/tasks/batch/utils.rs
•c_kzg::BYTES_PER_BLOB,c_kzg::FIELD_ELEMENTS_PER_BLOB
•c_kzg::Blob::new(...)
•c_kzg::ethereum_kzg_settings(0).blob_to_kzg_commitment(...)
•c_kzg::ethereum_kzg_settings(0).compute_kzg_proof(...)If any of these types or functions were renamed, had their parameters changed, or were removed in v2.0, update your calls accordingly. It’s also a good idea to run the full test suite after syncing with the v2.0 API.
crates/l2geth/src/rpc_client.rs (2)
79-79: API migration looks correct.The change from
on_clienttoconnect_clientaligns with alloy provider API updates. The functionality remains the same.
108-110: Block fetching API migration is well-implemented.The change from
get_block_by_hash(block_hash, BlockTransactionsKind::Full)to the fluent APIget_block_by_hash(block_hash).full()is more readable and follows modern Rust API design patterns.coordinator/internal/logic/provertask/batch_prover_task.go (1)
200-200: Excellent error logging improvement.Adding the actual error (
"err", err) to the structured log provides valuable debugging information when universal prover task generation fails. This enhancement improves observability.coordinator/internal/logic/provertask/chunk_prover_task.go (1)
194-194: Consistent error logging improvement.This change mirrors the improvement made in
batch_prover_task.go, ensuring consistent error reporting across all prover task types. The addition of"err", errenhances debugging capabilities.coordinator/internal/logic/provertask/bundle_prover_task.go (1)
196-196: LGTM! Enhanced error logging for better diagnostics.This change improves error visibility by including the actual error details in the log when universal prover task generation fails. This aligns with similar logging enhancements across the prover task modules.
crates/prover-bin/src/zk_circuits_handler.rs (1)
14-14: All CircuitsHandler implementations updated to asyncget_vkConfirmed that no synchronous
fn get_vkremains in anyimpl CircuitsHandler.
Checked files:
- crates/prover-bin/src/zk_circuits_handler/euclid.rs
- crates/prover-bin/src/zk_circuits_handler/euclidV2.rs
Interface is now consistent with the async
get_proof_datamethod.crates/libzkp/src/proofs.rs (2)
217-217: LGTM! Import cleanup removes unused dependencies.The simplified imports removing unused
BundleInfoV1andPublicInputswhile adding the neededForkNameimprove code clarity.
244-256: LGTM! Test modernization and type simplification.The changes simplify the bundle info construction by:
- Removing unnecessary type annotations and
.into()conversions- Adding explicit
ForkName::EuclidV1parameter topi_hashmethod- Fixing the assignment to use the variable directly
These improvements enhance code readability and align with modernized APIs.
crates/libzkp/src/tasks/batch.rs (2)
7-8: LGTM! Import addition for envelope types.The addition of
Envelopeto the imports supports the updated envelope construction patterns used in the batch proving task.
120-121: LGTM! Updated envelope construction method calls.The change from
fromtofrom_sliceprovides more explicit and type-safe construction ofEnvelopeV6andEnvelopeV7instances from byte slices. This aligns with the broader serialization pattern updates across the proving task modules.Also applies to: 135-136
crates/libzkp/src/tasks/batch/utils.rs (2)
45-47: LGTM: Correct implementation of c-kzg 2.0 API changes.The change from static method calls to instance method calls on
ethereum_kzg_settings(0)correctly implements the new c-kzg 2.0 API. The explicit argument0follows the updated library interface.
69-71: LGTM: Consistent KZG API update for proof computation.The change to use instance method
compute_kzg_proofon the KZG settings object is consistent with the blob commitment changes above and correctly implements the c-kzg 2.0 API.crates/prover-bin/src/main.rs (2)
38-39: LGTM: Parameter name better reflects expanded functionality.The rename from
file_nametoasset_pathmore accurately describes the parameter's purpose, as it now handles dumping various verifier assets rather than just a single VK file.
60-63: Confirm dump_verifier_assets implementation existsFound
pub fn dump_verifier_assets(&self, hard_fork_name: &str, out_path: &Path) -> Result<()>incrates/prover-bin/src/prover.rsat line 199. No further changes needed.crates/prover-bin/src/prover.rs (2)
75-75: LGTM: Correct async/await implementation for updated trait.The change from synchronous to asynchronous call with
.awaitcorrectly implements the updatedCircuitsHandlertrait whereget_vkbecame an async method.
199-228: LGTM: Well-structured verifier asset dumping implementation.The new
dump_verifier_assetsmethod provides comprehensive functionality:
- Proper error handling for missing fork configuration
- Uses EuclidV2Handler for universal proving
- Dumps both universal verifier assets and specific VK files
- Creates structured JSON output with chunk, batch, and bundle VKs
The implementation correctly delegates to the appropriate handler and provides clear error messages.
crates/libzkp/src/tasks/bundle.rs (2)
49-49: LGTM: Fork name support added to witness.Adding the
fork_namefield toBundleWitnesswith proper case normalization supports the multi-fork configuration mentioned in the PR objectives. The conversion to lowercase ensures consistency.
86-86: LGTM: Updated serialization method call.The change from the utility function
to_rkyv_bytesto the instance methodrkyv_serialize(None)appears to be part of API modernization. TheNoneparameter likely represents default serialization options.crates/prover-bin/src/zk_circuits_handler/euclidV2.rs (2)
60-64: LGTM: Provides access to underlying prover with clear documentation.The
get_provermethod correctly exposes the bundle prover as a representative until universal prover implementation. The documentation clearly explains this is a temporary solution.
82-84: LGTM: Correct async implementation with proper mutex handling.The change from synchronous
try_lock()to asynchronouslock().awaitproperly implements the async trait method and eliminates the potential panic fromtry_lock().unwrap(). This provides more robust error handling.Cargo.toml (3)
20-22: Confirm zkvm-prover revision exists; verify revert impactThe git revision
09e998fis valid and points to a revert commit (revert ToArchievedWitness using rkyv_serialize (#129)) on branchwip/building-reth. Before merging, please double-check:
- That this revert aligns with the intended “feynman fork” changes and hasn’t removed functionality your code depends on.
- All zkvm-prover–related tests still pass with the new revision.
- There are no new security concerns introduced by this commit (e.g., review serialization changes).
File to inspect:
• Cargo.toml (lines 20–22)
50-62: Verify revm patch usage and compatibilityI’ve confirmed that the
feat/reth-v74branch exists in the revm repo (commit 7746160…) but didn’t find any directuse revmorrevm::imports in your Rust code—meaning revm is likely only a transitive dependency (e.g. via reth). Please verify locally that each patched revm crate is still required and that pulling in that branch doesn’t introduce breakage:
- Run a dependency tree check to surface all revm crates:
cargo tree -e no-dev | grep revm- Ensure that all of the patched crates (revm, revm-bytecode, revm-context, …, revm-state) appear and are compatible.
- If any patched crate is no longer used or causes conflicts, consider removing its patch or updating its version/branch.
32-35: No breaking changes found after alloy version bumpI ran the following checks against the PR branch and everything compiles cleanly:
- Searched all
use alloyandalloy::occurrences (found only incrates/l2geth/src/rpc_client.rs)- Ran
cargo check—no errors related to alloy, only dependency downloads for various alloy crates- Confirmed that disabling default-features on
alloy,alloy-primitivesandalloy-serdedoes not break existing imports or code pathsSince there are no compilation errors or missing APIs, the update to alloy v1 / primitives v1.2 / serde v1 is safe to merge.
crates/libzkp/src/tasks/chunk.rs (1)
121-125: Please manually verifyChunkWitness::newbehaviorI wasn’t able to locate the
ChunkWitnesstype or itsnewimplementation in this crate— it may live in another module or external crate. Before approving, please confirm that:
- The
newconstructor simply assigns the three fields without any extra validation or side-effects.- Its behavior remains fully equivalent to the previous direct struct initialization.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1686 +/- ##
===========================================
+ Coverage 39.84% 39.95% +0.10%
===========================================
Files 236 236
Lines 18814 18808 -6
===========================================
+ Hits 7496 7514 +18
+ Misses 10583 10555 -28
- Partials 735 739 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
coordinator/internal/logic/libzkp/libzkp.h (1)
11-12: Consider adding error handling capability.The function declaration looks good and follows C conventions. However, the
voidreturn type prevents error reporting if tracing initialization fails.Consider whether this function should return an error code to enable proper error handling in the caller.
-// Init log tracing -void init_tracing(); +// Init log tracing - returns 0 on success, non-zero on error +int init_tracing();coordinator/internal/logic/libzkp/lib.go (1)
20-22: Consider error handling and initialization timing.The automatic tracing initialization follows Go conventions but has some limitations:
- No error handling: The void return type prevents detecting initialization failures
- Automatic execution: Tracing initializes during package import, which may not be ideal in all contexts
- No safeguards: No protection against multiple calls or checking if already initialized
Consider adding explicit initialization with error handling:
-func init() { - C.init_tracing() -} +// InitTracing initializes the tracing subsystem +// Should be called once during application startup +func InitTracing() error { + // Note: This would require updating the C function to return an error code + C.init_tracing() + return nil +}Alternatively, if automatic initialization is preferred, consider adding error logging:
+import "log" + func init() { - C.init_tracing() + // Initialize tracing - errors cannot be reported due to void return type + C.init_tracing() + // Consider adding a separate C function to check initialization status }
📜 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 (4)
coordinator/internal/logic/libzkp/lib.go(1 hunks)coordinator/internal/logic/libzkp/libzkp.h(1 hunks)crates/libzkp_c/Cargo.toml(1 hunks)crates/libzkp_c/src/lib.rs(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- crates/libzkp_c/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/libzkp_c/src/lib.rs
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: tests
|
With the last commit, we have successfully verified proofs picked up from mainnet and sepolia testnet, (the zkvm_version of stuffs is |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
coordinator/cmd/tool/verify.go (1)
97-97: Bundle proof handling: Always use expected verification keyThe verification key check has been removed for bundle proofs, and the proof's
Vkis always overwritten with the expected key from the verifier. This suggests that bundle proofs have different handling requirements compared to chunk and batch proofs, where the verification key validation is intentionally bypassed.This design decision should be documented to clarify why bundle proofs don't require verification key validation like the other proof types.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
coordinator/cmd/tool/verify.go(4 hunks)crates/prover-bin/src/prover.rs(3 hunks)crates/prover-bin/src/zk_circuits_handler/euclidV2.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/prover-bin/src/prover.rs
- crates/prover-bin/src/zk_circuits_handler/euclidV2.rs
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
🔇 Additional comments (3)
coordinator/cmd/tool/verify.go (3)
31-31: LGTM: Logging format adjustmentThe logging statement formatting has been improved for better readability.
56-56: Critical bug fix: Correct verification key comparison logicThis change fixes a critical logic error in the original code. The condition has been correctly inverted from
bytes.Equal(proof.Vk, vk)to!bytes.Equal(proof.Vk, vk).The original logic was backwards - it would return an error when the verification keys MATCHED, which would cause valid proofs to be rejected. The corrected logic now properly returns an error when the keys DON'T match, as intended.
77-77: Critical bug fix: Correct verification key comparison logicSame critical logic fix as line 56. The condition has been correctly inverted to return an error when verification keys don't match, rather than when they do match. This ensures batch proofs with valid verification keys are not incorrectly rejected.
colinlyguo
changed the title
For updates in deployment please refer this doc:
https://www.notion.so/scrollzkp/Deployment-of-coordinator-prover-for-feynman-upgrade-2237792d22af807583c6cd3920bda3d2?showMoveTo=true&saveParent=true
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation
Chores