Actionable comments posted: 3

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_bytes to witness.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 rust

The change from to_rkyv_bytes::<RancorError> to rkyv_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.

zkvm-prover/.work/chunk/openvm.toml (1)

40-58: New struct_name attributes per curve

All 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 existing tracing::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/upgrade branch, 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.toml

File: Cargo.toml
Lines: 63–64

ruint = { 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 ruint crate, 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-primitives crate.

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 diff between each forked branch and its upstream release to catalog exact API or implementation differences.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Reviewing files that changed from the base of the PR and between ea38ae7 and 59b0f11.

• Cargo.lock is excluded by !**/*.lock

• 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)

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.

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.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)

• GitHub Check: tests
• GitHub Check: tests
• GitHub Check: check

zkvm-prover/.work/batch/openvm.toml (2)

27-29: Verify that the consumer expects an “array-of-tuples” for fp2.supported_moduli

You switched from an array of strings to an array whose single element is a two-item tuple.
If the loader previously iterated over Vec<String> it will now panic/deserialise to None.
Confirm that the corresponding struct has changed to Vec<(String,String)> or similar.

---

30-32: struct_name introduced – ensure it’s wired through deserialisation

struct_name = "Bls12_381G1Affine" is new. Double-check that:

1. The config schema contains this field (otherwise it will be silently ignored or trigger an unknown-field error depending on serde flags).
2. Down-stream code actually uses it instead of hard-coding curve names.

zkvm-prover/.work/chunk/openvm.toml (2)

20-27: Renamed key supported_modulus → supported_moduli – audit codebase

The 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 for fp2.supported_moduli mirrors batch config

Same 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 compatibility

You’ve bumped c-kzg from 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_client to connect_client aligns 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 API get_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", err enhances 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 async get_vk

Confirmed that no synchronous fn get_vk remains in any impl 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_data method.

crates/libzkp/src/proofs.rs (2)

217-217: LGTM! Import cleanup removes unused dependencies.

The simplified imports removing unused BundleInfoV1 and PublicInputs while adding the needed ForkName improve 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::EuclidV1 parameter to pi_hash method
• 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 Envelope to 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 from to from_slice provides more explicit and type-safe construction of EnvelopeV6 and EnvelopeV7 instances 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 argument 0 follows the updated library interface.

---

69-71: LGTM: Consistent KZG API update for proof computation.

The change to use instance method compute_kzg_proof on 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_name to asset_path more 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 exists

Found pub fn dump_verifier_assets(&self, hard_fork_name: &str, out_path: &Path) -> Result<()> in crates/prover-bin/src/prover.rs at 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 .await correctly implements the updated CircuitsHandler trait where get_vk became an async method.

---

199-228: LGTM: Well-structured verifier asset dumping implementation.

The new dump_verifier_assets method 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_name field to BundleWitness with 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_bytes to the instance method rkyv_serialize(None) appears to be part of API modernization. The None parameter 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_prover method 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 asynchronous lock().await properly implements the async trait method and eliminates the potential panic from try_lock().unwrap(). This provides more robust error handling.

Cargo.toml (3)

20-22: Confirm zkvm-prover revision exists; verify revert impact

The git revision 09e998f is valid and points to a revert commit (revert ToArchievedWitness using rkyv_serialize (#129)) on branch wip/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 compatibility

I’ve confirmed that the feat/reth-v74 branch exists in the revm repo (commit 7746160…) but didn’t find any direct use revm or revm:: 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 bump

I ran the following checks against the PR branch and everything compiles cleanly:

• Searched all use alloy and alloy:: occurrences (found only in crates/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-primitives and alloy-serde does not break existing imports or code paths

Since 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 verify ChunkWitness::new behavior

I wasn’t able to locate the ChunkWitness type or its new implementation in this crate— it may live in another module or external crate. Before approving, please confirm that:

• The new constructor simply assigns the three fields without any extra validation or side-effects.
• Its behavior remains fully equivalent to the previous direct struct initialization.

Actionable comments posted: 0

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 void return 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:

1. No error handling: The void return type prevents detecting initialization failures
2. Automatic execution: Tracing initializes during package import, which may not be ideal in all contexts
3. 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
 }

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Reviewing files that changed from the base of the PR and between 59b0f11 and 1515cc5.

• Cargo.lock is excluded by !**/*.lock

• 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)

• crates/libzkp_c/Cargo.toml

• crates/libzkp_c/src/lib.rs

• GitHub Check: check
• GitHub Check: tests
• GitHub Check: tests

Actionable comments posted: 0

coordinator/cmd/tool/verify.go (1)

97-97: Bundle proof handling: Always use expected verification key

The verification key check has been removed for bundle proofs, and the proof's Vk is 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.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Reviewing files that changed from the base of the PR and between 1515cc5 and adac040.

• 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)

• crates/prover-bin/src/prover.rs
• crates/prover-bin/src/zk_circuits_handler/euclidV2.rs

• GitHub Check: tests
• GitHub Check: tests
• GitHub Check: check

coordinator/cmd/tool/verify.go (3)

31-31: LGTM: Logging format adjustment

The logging statement formatting has been improved for better readability.

---

56-56: Critical bug fix: Correct verification key comparison logic

This 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 logic

Same 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.