feat(prover): integrate proving-sdk && support multiple task types #1587
Conversation
…ailing task to same prover
WalkthroughThe update bumps the version tag from "v4.4.86" to "v4.4.87" and adds new dependencies in Cargo.toml. The prover configuration file is restructured with renamed, removed, and newly added fields. Several files responsible for configuration management, coordinator API interactions, key signing, geth client, task caching/processing, and version retrieval have been removed. The main function and prover implementation have been refactored to support asynchronous execution, and type definitions and circuits handler implementations have been updated to work with asynchronous flows and new circuit types. Changes
Sequence Diagram(s)sequenceDiagram
participant Main
participant ProverBuilder
participant LocalProver
participant CircuitsHandler
Main->>ProverBuilder: Initialize with LocalProverConfig
ProverBuilder->>LocalProver: Build async LocalProver
LocalProver->>CircuitsHandler: Async call (get_vk/prove/query_task)
CircuitsHandler-->>LocalProver: Return response
LocalProver-->>Main: Prover run completes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (23)
prover/src/zk_circuits_handler.rs (3)
24-28: Introduction of async trait methods.Defining async trait methods (
get_vk,get_proof_data) fosters concurrency. Make sure to handle possible race conditions if concurrent calls rely on shared state.
47-51: Initialization ofAssetsDirEnvConfig.Handling potential initialization errors with a log and an
exit. Consider returning a typed error instead of forcibly exiting, which might hamper testability or graceful recovery in production.
140-166: Async initialization of verification keys.Looping over each builder and each
ProverTypeis valid, but be aware of performance if multiple fork names and multiple prover types grow in number. Caching or parallelizing might be beneficial if the operation is costly.prover/prover.rs (2)
50-51: Initialization of verification keys.The
init_vkscall here returnsvksbut we don’t see direct usage in this code snippet. Make sure the subsequent flow references or caches these in the coordinator appropriately.
180-183:do_submit()minimal wrapper aroundsubmit_proof().Straightforward approach. Logging extra details on success/failure might aid debugging, but not strictly required.
prover/src/prover.rs (3)
35-53:get_vks()implementation.Extracts
prover_typesfrom the incoming circuit list. Thorough approach to building the response. If circuit definitions grow, consider parallelizing the VK retrieval.
128-139:LocalProver::new(): building the provider and setting initial state.No immediate issues, though error handling is done via
unwrap()inside. Maybe consider returning aResult<Self>to handle creation errors gracefully.
142-179:do_prove()storing a spawned handle incurrent_task.• Only one handle is kept at a time.
• Potentially set tasks to fail if a new request arrives before the existing finishes.
This might be intended but double-check if multiple concurrent tasks are needed.coordinator/internal/logic/provertask/bundle_prover_task.go (1)
90-95: Repetitive logic across multiple tasksThe logic for fetching assigned vs. unassigned tasks in 50-task chunks appears duplicated in related files. Consider extracting this into a shared helper to maintain DRY (Don't Repeat Yourself) principles.
coordinator/internal/logic/provertask/batch_prover_task.go (1)
91-96: Dedicated methods for assigned vs. unassigned tasksThe separation of assigned vs. unassigned queries improves clarity. Consider error logging to confirm the number of tasks retrieved.
prover/src/zk_circuits_handler/darwin.rs (1)
281-285: End-to-end batch proof test coverageGenerating the batch proof with real chunk proofs improves confidence in the aggregator logic. Consider expanding coverage to negative test cases (e.g., invalid chunk proofs).
prover/src/zk_circuits_handler/darwin_v2.rs (1)
279-285: Genuine chunk traces and batch proof linkingReading real chunk traces from disk, generating chunk proofs, and subsequently forming a batch proof validates the new concurrency approach at scale. Expanding negative scenarios could yield more robust coverage.
coordinator/internal/orm/chunk.go (3)
76-86: Consider using placeholders and clarifying the function name
- Constructing the SQL string via
fmt.Sprintfmay be more prone to mistakes and could potentially pose security concerns in other contexts. Consider using GORM's chainable methods or parameterized queries (placeholders) for consistency and safety.- The function name is
GetUnassignedChunkbut returns a slice of chunks. Consider renaming it toGetUnassignedChunksto reflect this behavior accurately.
88-101: Review the comment regarding “limit”
The docstring says “retrieves unassigned chunk count based on the specified limit,” but there is nolimitparameter in the signature or code. This discrepancy may cause confusion. Consider updating the docstring to remove or clarify any mention of a limit.
104-115: DRY up similar functionality
GetAssignedChunksmirrors much of the logic inGetUnassignedChunk. Consider abstracting out shared parts—like building the query or returning the chunks—into a helper function that takes the desired status (assigned vs. unassigned). This would reduce duplicative code and simplify maintenance.coordinator/internal/orm/prover_task.go (1)
151-167: Consider using.First()and handling the no-record scenario.
Currently, the method uses.Find()with.Limit(1), which will not return an error if no record is found. This could silently return an empty struct. Consider using.First(&proverTask)and handlinggorm.ErrRecordNotFoundto improve clarity, ensuring the caller knows when no matching record is returned.coordinator/internal/orm/bundle.go (3)
57-70: Mitigate potential SQL injection by using placeholders.
The raw SQL string usesfmt.Sprintfto embed parameters (e.g.,proving_status,limit). Although these fields are numeric, it’s generally safer to use parameter placeholders (i.e.,?) binding where possible to reduce the risk of SQL injection.
71-83: Efficient counting approach recommendation.
The method correctly counts unassigned bundles. Consider using raw SQL placeholders or GORM chain calls to avoid string formatting. This will also keep queries consistent withGetUnassignedBundles.
86-99: Align method naming and queries with unassigned method.
This method’s logic mirrorsGetUnassignedBundlesbut for assigned bundles. Maintain consistent patterns—particularly using placeholders and error messages—to ensure uniformity and reduce code maintenance overhead.coordinator/internal/orm/batch.go (3)
81-93: Safeguard raw SQL inGetUnassignedBatches.
Similar to other methods, consider utilizing GORM clause builders or parameterized queries instead offmt.Sprintfto mitigate potential SQL injection risks, even if arguments are numeric today.
95-108: Parallel the structure of your retrieval methods.
Use the same style (raw or parameter-based) forGetUnassignedBatchCountto remain consistent across your codebase. This improves maintainability and consistency.
111-122: Maintain consistent naming for debug and user-facing error messages.
To make logs more searchable and consistent, ensure that messages and method references match (i.e., “Batch.GetAssignedBatcheserror” consistently).prover/Cargo.toml (1)
46-46: Consider specifying a minimum version for async-trait.While using
"0.1"allows for compatible updates, consider specifying a minimum version (e.g.,"^0.1.50") to ensure known bug fixes and security patches are included.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
prover/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
common/version/version.go(1 hunks)coordinator/conf/config.json(1 hunks)coordinator/internal/config/config.go(1 hunks)coordinator/internal/logic/provertask/batch_prover_task.go(2 hunks)coordinator/internal/logic/provertask/bundle_prover_task.go(2 hunks)coordinator/internal/logic/provertask/chunk_prover_task.go(2 hunks)coordinator/internal/logic/provertask/prover_task.go(1 hunks)coordinator/internal/orm/batch.go(1 hunks)coordinator/internal/orm/bundle.go(1 hunks)coordinator/internal/orm/chunk.go(1 hunks)coordinator/internal/orm/prover_task.go(1 hunks)prover/Cargo.toml(2 hunks)prover/config.json(1 hunks)prover/prover.rs(1 hunks)prover/src/config.rs(0 hunks)prover/src/coordinator_client.rs(0 hunks)prover/src/coordinator_client/types.rs(3 hunks)prover/src/geth_client.rs(0 hunks)prover/src/key_signer.rs(0 hunks)prover/src/main.rs(3 hunks)prover/src/prover.rs(1 hunks)prover/src/task_cache.rs(0 hunks)prover/src/task_processor.rs(0 hunks)prover/src/types.rs(2 hunks)prover/src/utils.rs(2 hunks)prover/src/zk_circuits_handler.rs(7 hunks)prover/src/zk_circuits_handler/darwin.rs(10 hunks)prover/src/zk_circuits_handler/darwin_v2.rs(10 hunks)
💤 Files with no reviewable changes (6)
- prover/src/task_processor.rs
- prover/src/config.rs
- prover/src/geth_client.rs
- prover/src/task_cache.rs
- prover/src/key_signer.rs
- prover/src/coordinator_client.rs
✅ Files skipped from review due to trivial changes (1)
- common/version/version.go
🔇 Additional comments (93)
prover/src/main.rs (3)
12-13: Transitioning to LocalProver import looks consistent.The new imports from
prover::LocalProverandscroll_proving_sdkalign with the updated architecture for multiple task types.
31-33: Adoption of asynchronous runtime is appropriate.Using
#[tokio::main]and callinginit_tracing()is a solid move for asynchronous operations. Just ensure all downstream async calls handle errors gracefully or propagate them further if critical.
51-51: Consider error handling for theprover.run()call.Right now, any potential error from
prover.run().awaitis ignored. Check if error handling or logging may be necessary to diagnose potential runtime failures.prover/src/types.rs (3)
4-4: Direct usage ofCircuitTypeis aligned with the new approach.Importing
CircuitTypefromscroll_proving_sdklooks consistent with the removal ofTaskType.
51-51: Replacement ofTaskTypewithCircuitTypeinTaskstruct.This correctly aligns with the updated circuit-based logic in the SDK. Ensure all references to
task_typeusage match the new type.
61-61: RefactoringProofDetailto useCircuitType.Consistent usage of
CircuitTypehelps unify proof workflows. Looks good.prover/src/zk_circuits_handler.rs (12)
5-5: Refactored import to retrieveProverTypeand circuit logic fromutils.Combining multiple components in one import statement is reasonable. No issues here.
7-7: Use ofasync_trait::async_trait.This trait is crucial to enable async methods in traits. The approach is correct.
10-13: Importing fromscroll_proving_sdkfor local config and circuit types.The shift to using
LocalProverConfigandCircuitTypeis consistent with the design shift. Looks good.
31-32: Type alias forCircuitsHandlerBuilder.Clear definition. It’s helpful to unify how circuits handlers are created.
34-35:CircuitsHandlerProviderconfig and builder map.This structure neatly centralizes circuit creation logic.
39-40: Tracking the current fork name and circuit.Maintaining these as optional fields is good. Be mindful of concurrency if multiple threads query this provider.
43-44:new(config: LocalProverConfig)returningResult<Self>.Returning a
Resultis correct for error propagation.
54-54: Builder closure referencesconfigfor circuit paths.Looks appropriate. Make sure the config paths exist and are validated.
75-75: Second builder closure forDarwinV2Handler.Consistent pattern with the first builder. Good maintainability.
99-99:current_prover_typeset toNoneinitially.Indicates no default solver type. This is fine, but watch for usage before assignment.
109-110:get_circuits_handlerconcurrency concerns.Caching a single
current_circuitmight cause conflicts if multiple calls to this function occur in parallel for different forks. Ensure external access is well-synchronized or each request runs in a single flow.
126-130: Updating cached fields after building a new handler.This approach is correct for reusing the same circuit. Confirm that switching from one fork name to another is thread-safe if multiple threads are possible.
prover/prover.rs (13)
1-17: Introduction of the newProverstruct for orchestrating tasks.This file manages config, signer, coordinator client, etc. Ensure each dependency is tested or mocked for integration.
18-24:Prover<'a>fields referencingRefCellwrappers.
RefCellallows interior mutability, but can hide borrow panics at runtime if not carefully handled. Continue validating concurrency usage or consider using more explicit concurrency primitives.
26-27:new(config, coordinator_listener): aggregator approach.Overall aggregator approach is fine—just watch for potential partial initialization if any step fails.
31-35: Conditional Geth client creation based onProverType::Chunk.Makes sense for tasks requiring direct chain queries. Good guard approach.
47-50: InstantiatingCircuitsHandlerProviderwithconfig, geth_client.clone().Keep an eye on potential data races if
CircuitsHandlerProvideralso reads/writes the same client object.
54-55: Creation ofCoordinatorClientwithvks.Indicates the new approach is to pass VKs upfront. Ensure the client is equipped to handle an updated key set if keys change at runtime.
65-66: Constructor returningOk(prover).Straightforward, no issues.
68-70:get_public_key()delegates to the KeySigner.Well-structured approach. Good encapsulation.
72-114:fetch_task()logic for building aGetTaskRequestand retrieving tasks.• Aggregates multiple
TaskTypes from config.
• Conditionally retrieves block number if we haveChunkinprover_types.
• Returns an error if the block is zero or not found.
Overall logic is fine. Just ensure any concurrency scenario around coordinator calls is addressed.
116-130:prove_task(): bridging fromTaskto circuit handler.Identifies
ProverTypefromtask_typewithget_prover_type. Could fail gracefully if no type is found. Then retrieves a circuit handler and callsdo_prove(). Good separation of concerns.
132-141:do_prove(): constructingProofDetail.Populates the proof detail once proof data is retrieved from the handler. Straightforward and consistent.
143-159:submit_proof()building aSubmitProofRequest.Clean approach to sending proof data. Just remain consistent with error handling if the coordinator call fails.
161-178:submit_error()building a request with failure details.Neat flow for capturing errors. The partial data is helpful for debugging.
prover/src/prover.rs (11)
2-2: Dependency onutils::get_prover_type.Matches the removal of
TaskTypeby derivingProverTypefromCircuitType.
5-5: Usage ofanyhow::Result.Offers convenient error handling. Ensure distinct error contexts for deeper debugging.
6-6: Adoptingasync_trait::async_traitfor concurrency.Aligns with the new async-based design.
7-16: Integration withscroll_proving_sdk::prover::ProvingService.The trait usage is consistent with the approach to unify local proving tasks. Excellent job leveraging the existing SDK.
23-27:LocalProverstruct: concurrency fields withRwLockandMutex.Using a read-write lock for the circuits provider is reasonable. The
Arc<Mutex<Option<JoinHandle>>>for the current task implies a single concurrent proof at a time. This is okay if you want to strictly serialize tasks.
30-33:is_local()returningtrue.Simple trait method, correct for local usage.
54-65:prove()method with fallback on error building aProveResponse.Neatly handled; the error path uses
build_prove_error_responsewith minimal overhead. Good practice.
66-74:get_circuits_handlerbehind a write lock.Acquiring a write lock while building or fetching the handler is correct if the underlying data might change. Just confirm that high concurrency usage is acceptable if tasks are queued or slowed by the lock.
85-124:query_task()logic for tracking ongoing tasks.• Removes the handle if completed, returns success or error.
• If not finished, returnsTaskStatus::Proving.
• If no current task, returns a failure.
This is a straightforward approach for single-task concurrency, but ensure that multiple tasks or queries won’t conflict.
183-204:build_prove_error_response(): defaulting fields to empty or undefined.Ensure downstream consumers handle these placeholder values properly.
206-225:build_query_task_response(): similar placeholder usage.Same note about placeholders. Good consistent error response structure.
coordinator/internal/logic/provertask/chunk_prover_task.go (4)
7-7: Import "strings" looks appropriateThe newly introduced import for
"strings"appears to be used by the code to detect external prover prefixes. This seems valid and aligns with the new logic forExternalProverNamePrefix.
81-82: Offset initialization is validInitializing
assignedOffsetandunassignedOffsetto zero provides a clear approach to interleaving assigned vs. unassigned tasks. No concerns here.
89-93: Enhanced error handling is helpfulFetching unassigned chunks within a separate method call and handling errors distinctly can improve debugging clarity and reusability.
95-119: Confirm concurrency safety in the assignment loopWhile iterating over assigned and unassigned tasks, ensure no race conditions occur if multiple goroutines invoke
Assignsimultaneously. Consider using proper transaction isolation or locking at the ORM level to avoid conflicting updates to the same tasks.coordinator/internal/logic/provertask/bundle_prover_task.go (3)
7-7: Import "strings" is appropriateThis import is used to detect external prover prefixes and is consistent with the approach seen in
chunk_prover_task.go.
83-84: Good practice: offset variablesSame approach as in the chunk task, storing offsets to separate assigned from unassigned tasks is consistent and maintains clarity.
97-119: Avoid re-assigning failing tasks to the same proverThis updated check prevents the same failing job from cycling to the identical prover. Verify that this logic covers all edge cases, including tasks that failed for different reasons.
coordinator/internal/logic/provertask/batch_prover_task.go (2)
83-84: Offset approach is consistentInitializing two parallel offsets for assigned and unassigned sets is consistent with the approach in
chunk_prover_task.goandbundle_prover_task.go.
97-119: Loop termination conditionsThe loop breaks early if
tmpBatchTaskisnil; this might cause repeatednilreturns if multiple offset increments exhaust both slices quickly. Ensure that this meets your design objectives for distributing tasks evenly across provers.prover/src/zk_circuits_handler/darwin.rs (15)
2-11: Imports complement the move to async & concurrencyThe introduced imports (
async_trait,RwLock,CircuitType, etc.) align with the asynchronous refactor and concurrency approach, making the code thread-safe and future-proof for multi-prover usage.
42-43: Transition to RwLock for concurrencyChanging from a mutable reference or
RefCelltoRwLockis a solid improvement for safe concurrent access tochunk_proverandbatch_prover.
76-81: Initialize RwLock as intendedUsing
RwLock::newto wrap the chunk or batch prover is consistent with the concurrency shift. Verify that the underlying library references are Send + Sync compatible.
90-91: Factory method pattern reiterated
new(prover_type, params_dir, assets_dir)simply delegates tonew_multi, which is good for code reusability.
100-101: Use .write() for proof generationAcquiring a write lock before generating the chunk proof is necessary because the prover modifies internal state. Ensure that read locks are used whenever possible to reduce contention.
109-111: Parsing ProveRequest inputDeserializing
prove_request.inputintoVec<BlockTrace>is clear and ensures the method remains decoupled from external data structures. Good usage ofserde_json.
127-133: Validate chunk protocol before generating batch proofInvoking
check_protocol_of_chunkswith a write lock emphasizes controlled state changes. The check for non-matching chunk protocol helps avoid producing invalid proofs.
150-153: New asynchronous method signatureDeclaring
gen_bundle_proof_rawasasyncensures consistent concurrency across all proof generation methods. Good alignment with the rest of the codebase.
155-155: Synchronized bundle proof generationUsing
write().awaitonbatch_proverprevents concurrent writes on shared BatchProver state. This is an essential concurrency safeguard.
166-168: ProveRequest-based method parametersTransitioning from a raw
TasktoProveRequestis more flexible and decouples the request schema from the internal logic.
179-195: Asynchronous read locks for VK retrievalUsing
.read().awaitensures multiple concurrent read operations can proceed, as verifying the VK does not require exclusive access.
200-204: Unified entry point for proof data retrieval
get_proof_datacleanly switches among multiple circuit types. The usage of pattern matching overprove_request.circuit_typeis succinct and maintainable.
249-250: Asynchronous test annotationUsing
#[tokio::test]for testing is consistent with the newly introduced async logic. This approach ensures the test harness aligns with asynchronous patterns.
258-260: Chunk VK retrieval and validationFetching the chunk VK asynchronously and comparing it with a file-based reference is a solid end-to-end test for verifying the correctness of the loaded verifier key.
Line range hint
363-369: Consistent VK checks across circuit typesEnsuring that the on-file VK matches the runtime VK is critical for guaranteeing the circuit's correctness. Good practice for regression checks.
prover/src/zk_circuits_handler/darwin_v2.rs (15)
2-11: Asynchronous concurrency readinessThe newer imports (
async_trait,RwLock,CircuitType) align with an asynchronous concurrency model, just like indarwin.rs.
42-43: Use of RwLock ensures write safetyUtilizing
RwLock<ChunkProver>andRwLock<BatchProver>enforces concurrency while allowing multiple reads and exclusive writes, preventing data corruption.
76-81: Param-based RwLock initializationsWrapping each prover in
RwLock::new(...)is a natural progression to protect the state. Confirm that each underlying struct is indeed threadsafe and does not rely on thread-local references.
90-91: Convenient constructorDelegation from
newtonew_multiretains DRY consistency, preventing logic duplication for single-prover vs. multi-prover configurations.
100-101: Write lock for chunk proofSimilar to DarwinHandler, using
prover.write().awaitbefore modifying the chunk prover’s state. This is an essential concurrency safeguard.
109-111: Deserialization from ProveRequestParsing JSON input for chunk traces fosters extensibility and lowers coupling to specific data shapes. Good approach for maintaining loose coupling.
127-133: Protocol validation with RwLockEnsuring chunk proofs conform to protocol specs under a write lock helps avoid partial modifications from concurrent tasks. This approach is consistent with concurrency best practices.
150-153: Extending async pattern to bundle proofsThe method signature for
gen_bundle_proof_rawunifies all proof generation tasks (chunk, batch, bundle) under the same async pattern.
155-155: Consolidated concurrency approach for bundle proofsAcquiring a write lock ensures no overlapping writes to the same batch prover instance, preventing race conditions or data corruption.
166-168: Simplified request-based interfaceAccepting
ProveRequestingen_bundle_proofis simpler than manually constructing tasks. This fosters a uniform request pattern across proof types.
179-195: Concurrent read for VK retrievalMultiple asynchronous read locks allow concurrent VK retrieval requests, reducing lock contention where no state mutations are needed.
200-204: Consolidated proof entry point
get_proof_datagating logic remains consistent withdarwin.rs, preventing code divergence between Darwin versions.
253-254: Async tests bolster reliabilityUsing
#[tokio::test]ensures the test environment matches the concurrency patterns used in production code. This helps catch async-specific bugs earlier.
262-264: Retrieving VK ensures robust coverageBy checking the chunk VK in tests, you validate the new concurrency model does not affect the loading and usage of verification keys.
363-369: VK consistency checks remain a crucial safeguardEnsuring that the VK on disk matches the runtime VK is vital to preventing silent circuit mismatches. This is an essential final verification step.
prover/src/utils.rs (3)
4-5: New imports align well with updated task types
These imports support the shift toCircuitType. Everything looks consistent and correct here.
28-33: Clean rename to get_circuit_types
Renaming fromget_task_typestoget_circuit_typesclarifies intent and aligns code with the new type system. The return values forProverType::ChunkandProverType::Batchare straightforward and valid.
35-40: Reverse mapping from CircuitType to ProverType
The reverse mapping logic is consistent, withCircuitType::Undefinedleading toNoneandBundlesharingProverType::Batch. No issues found.prover/src/coordinator_client/types.rs (4)
2-5: Appropriate import for CircuitType
Replacing the oldTaskTypeimport withCircuitTypematches the broader refactor.
62-62: Field updated to CircuitType
Updatingtask_typestoVec<CircuitType>inGetTaskRequestis consistent with the new refactor and naming conventions.
70-70: Task type field uses CircuitType
GetTaskResponseDatanow storestask_typeasCircuitType, aligning with other changes.
79-79: SubmitProofRequest uses CircuitType
Makingtask_typeaCircuitTypeinSubmitProofRequestcompletes the transition fromTaskTypetoCircuitTypeacross the codebase.coordinator/internal/config/config.go (1)
19-20: New external prover threshold
AddingExternalProverThresholdprovides clear, configurable control over external prover activation. This appears well-documented and neatly integrated.coordinator/internal/logic/provertask/prover_task.go (1)
30-33: Simple, clear prefix for external prover naming
DefiningExternalProverNamePrefix = "external"is straightforward. If usage grows more complex, consider elevating this to a configurable value.prover/config.json (1)
6-6: Confirm multi-prover support downstream.
Changing from a singleprover_typeto an arrayprover_typesis a great step to allow multiple types. Ensure that all affected code handles this array properly (e.g., reading multiple values, passing them along, etc.).coordinator/conf/config.json (1)
5-5: Verify external prover threshold validation.The hardcoded threshold value needs validation checks in the code to prevent potential system instability.
✅ Verification successful
Let me gather more information about the validation of this threshold.
The external_prover_threshold is properly used as a control mechanism
The threshold value is used as intended in the codebase to control task assignment behavior:
- In
chunk_prover_task.go, it prevents task assignment when unassigned chunks are below the threshold- In
bundle_prover_task.go, it controls bundle task assignment based on the threshold- In
batch_prover_task.go, it manages batch task assignment using the same threshold logicWhile there's no explicit validation of the threshold value itself, this is acceptable because:
- The value is used in simple numerical comparisons that are safe regardless of the configured number
- The threshold acts as a control gate rather than a critical safety parameter
- The implementation properly handles the configured value in all relevant prover task types
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for validation of external_prover_threshold in the codebase rg -A 5 "external_prover_threshold" # Look for config validation patterns ast-grep --pattern 'func $_(config $_) error { $$$ if config.ProverManager.$_ { $$$ } $$$ }'Length of output: 1077
Script:
#!/bin/bash # Search for validation or usage of ExternalProverThreshold rg -A 10 "ExternalProverThreshold" # Look for config validation functions in config.go ast-grep --pattern 'func $_(config $_) { $$$ }' # Search for validation patterns specifically in coordinator package rg -A 5 "validate|check.*config" coordinator/Length of output: 11543
prover/Cargo.toml (1)
34-34: 🛠️ Refactor suggestionUse tagged versions instead of git revisions for better reproducibility.
The
scroll-proving-sdkdependency is pinned to a specific git revision (6dd13b6). While this ensures reproducibility, using tagged versions is preferred for better maintainability and security tracking.-scroll-proving-sdk = { git = "https://github.com/scroll-tech/scroll-proving-sdk.git", rev = "6dd13b6"} +scroll-proving-sdk = { git = "https://github.com/scroll-tech/scroll-proving-sdk.git", tag = "v0.1.0" } # Use appropriate version tagAlso, verify if this dependency has been audited for security vulnerabilities.
…igning failing task to same prover" This reverts commit 791fcaf.
…m/scroll-tech/scroll into feat-prover-integrate-proving-sdk
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
prover/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
prover/src/main.rs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: fmt
- GitHub Check: tests
🔇 Additional comments (3)
prover/src/main.rs (3)
12-13: LGTM! Clean integration of scroll-proving-sdk.The imports are well-organized and correctly bring in the necessary components from the scroll-proving-sdk.
Line range hint
15-30: LGTM! Well-structured argument parsing.The command-line argument handling is clean and follows Rust best practices using clap.
31-32: LGTM! Robust error handling implementation.The code properly handles errors using anyhow::Result and safely handles the optional local prover configuration, addressing the previous review comment about unwrap().
Also applies to: 44-50
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
prover/src/prover.rs (1)
180-181:⚠️ Potential issueAddress TODO comments in error response builders.
Both response builders use
CircuitType::Undefinedas a placeholder. This should be replaced with the actual circuit type from the request.Also applies to: 203-204
🧹 Nitpick comments (2)
prover/src/prover.rs (2)
24-30: Remove commented-out code.Line 129 contains commented-out code that should be removed if no longer needed.
- // result: Arc::new(Mutex::new(Err(anyhow::Error::msg("prover not started")))),
138-142: Consider using a more robust task ID generation strategy.The current task ID generation might not be suitable for distributed environments. Consider using UUIDs or a more robust distributed ID generation system.
- let task_id = { - let mut next_task_id = self.next_task_id.lock().unwrap(); - *next_task_id += 1; - *next_task_id - }; + let task_id = uuid::Uuid::new_v4().to_string();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
prover/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
prover/Cargo.toml(2 hunks)prover/src/prover.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- prover/Cargo.toml
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: clippy
- GitHub Check: tests
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
prover/src/zk_circuits_handler.rs (4)
24-28: Add documentation for the CircuitsHandler trait.Consider adding documentation comments to describe:
- The purpose of the trait
- Expected behavior of each method
- Error conditions and handling
+/// Handles circuit-related operations for the prover. #[async_trait] pub trait CircuitsHandler: Send + Sync { + /// Retrieves the verification key for the given circuit type. + /// Returns None if the key is not available. async fn get_vk(&self, task_type: CircuitType) -> Option<Vec<u8>>; + /// Generates proof data for the given prove request. + /// Returns the proof as a string or an error if proving fails. async fn get_proof_data(&self, prove_request: ProveRequest) -> Result<String>; }
47-49: Improve panic error message.The error message could be more descriptive about what failed during initialization.
- panic!("AssetsDirEnvConfig init failed: {:#}", e); + panic!("Failed to initialize AssetsDirEnvConfig. This is required for circuit proving. Error: {:#}", e);
124-129: Improve error handling in handler creation.The current error handling could provide more context about what failed.
- let handler = builder(prover_types, &self.config) - .expect("failed to build circuits handler"); + let handler = builder(prover_types, &self.config) + .map_err(|e| anyhow::anyhow!( + "Failed to build circuits handler for fork '{}': {}", + hard_fork_name, e + ))?;
137-163: Improve VK initialization robustness and performance.The current implementation has several areas for improvement:
- No error propagation from the build operation
- Silent failures when VK is empty
- Nested loops could be refactored for better readability
Consider refactoring to:
pub async fn init_vks( &self, config: &LocalProverConfig, prover_types: Vec<ProverType>, - ) -> Vec<String> { + ) -> Result<Vec<String>> { let mut vks = Vec::new(); for (hard_fork_name, build) in self.circuits_handler_builder_map.iter() { - let handler = - build(prover_types.clone(), config).expect("failed to build circuits handler"); + let handler = build(prover_types.clone(), config) + .map_err(|e| anyhow::anyhow!( + "Failed to build circuits handler for fork '{}': {}", + hard_fork_name, e + ))?; for prover_type in prover_types.iter() { for task_type in get_circuit_types(*prover_type).into_iter() { let vk = handler .get_vk(task_type) .await - .map_or("".to_string(), utils::encode_vk); + .ok_or_else(|| anyhow::anyhow!( + "Missing VK for fork '{}', task type '{:?}'", + hard_fork_name, task_type + ))?; + let encoded_vk = utils::encode_vk(vk); log::info!( "vk for {hard_fork_name}, is {vk}, task_type: {:?}", task_type ); - if !vk.is_empty() { - vks.push(vk) - } + vks.push(encoded_vk); } } } - vks + Ok(vks) }prover/src/prover.rs (2)
73-112: Simplify task status management.The current implementation has several areas for improvement:
- Multiple lock acquisitions
- Complex nested if-else structure
- Repetitive response building
Consider using a more concise approach:
async fn query_task(&self, req: QueryTaskRequest) -> QueryTaskResponse { - let handle = self.current_task.lock().unwrap().take(); - if let Some(handle) = handle { - if handle.is_finished() { - return match handle.await { - Ok(Ok(proof)) => QueryTaskResponse { - task_id: req.task_id, - status: TaskStatus::Success, - proof: Some(proof), - ..Default::default() - }, - Ok(Err(e)) => QueryTaskResponse { - task_id: req.task_id, - status: TaskStatus::Failed, - error: Some(format!("proving task failed: {}", e)), - ..Default::default() - }, - Err(e) => QueryTaskResponse { - task_id: req.task_id, - status: TaskStatus::Failed, - error: Some(format!("proving task panicked: {}", e)), - ..Default::default() - }, - }; - } else { - *self.current_task.lock().unwrap() = Some(handle); - return QueryTaskResponse { - task_id: req.task_id, - status: TaskStatus::Proving, - ..Default::default() - }; - } - } - // If no handle is found - QueryTaskResponse { - task_id: req.task_id, - status: TaskStatus::Failed, - error: Some("no proving task is running".to_string()), - ..Default::default() - } + let mut response = QueryTaskResponse { + task_id: req.task_id, + ..Default::default() + }; + + let mut guard = self.current_task.lock().expect("Failed to acquire task lock"); + + match guard.take() { + Some(handle) => { + if handle.is_finished() { + match handle.await { + Ok(Ok(proof)) => { + response.status = TaskStatus::Success; + response.proof = Some(proof); + } + Ok(Err(e)) => { + response.status = TaskStatus::Failed; + response.error = Some(format!("Proving task failed: {}", e)); + } + Err(e) => { + response.status = TaskStatus::Failed; + response.error = Some(format!("Proving task panicked: {}", e)); + } + } + } else { + *guard = Some(handle); + response.status = TaskStatus::Proving; + } + } + None => { + response.status = TaskStatus::Failed; + response.error = Some("No proving task is running".to_string()); + } + } + + response }
130-165: Improve error handling and timing accuracy in proof generation.The current implementation has several areas for improvement:
- Unwrap usage for system time
- Manual timestamp calculation
- No error handling for mutex operations
Consider these improvements:
async fn do_prove( &self, req: ProveRequest, handler: Arc<Box<dyn CircuitsHandler>>, ) -> Result<ProveResponse> { let task_id = { - let mut next_task_id = self.next_task_id.lock().unwrap(); + let mut next_task_id = self.next_task_id.lock() + .map_err(|e| anyhow::anyhow!("Failed to acquire task ID lock: {}", e))?; *next_task_id += 1; *next_task_id }; - let duration = SystemTime::now().duration_since(UNIX_EPOCH).unwrap(); - let created_at = duration.as_secs() as f64 + duration.subsec_nanos() as f64 * 1e-9; + let created_at = SystemTime::now() + .duration_since(UNIX_EPOCH) + .map_err(|e| anyhow::anyhow!("Failed to get system time: {}", e))? + .as_secs_f64(); // ... rest of the implementation ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
prover/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
prover/Cargo.toml(2 hunks)prover/src/prover.rs(1 hunks)prover/src/zk_circuits_handler.rs(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- prover/Cargo.toml
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: fmt
- GitHub Check: tests
🔇 Additional comments (1)
prover/src/prover.rs (1)
144-148:⚠️ Potential issueCritical: Avoid blocking the tokio runtime.
Using
block_oninside aspawn_blockingtask can lead to deadlocks and performance issues. This needs to be redesigned to properly handle async operations.Consider restructuring to:
- let req_clone = req.clone(); - let handle = Handle::current(); - let task_handle = - tokio::task::spawn_blocking(move || handle.block_on(handler.get_proof_data(req_clone))); + let task_handle = tokio::spawn(handler.get_proof_data(req));This change:
- Removes the blocking operation
- Properly utilizes tokio's async runtime
- Prevents potential deadlocks
Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
prover/src/prover.rs (1)
141-142: Use chrono for timestamp handling.Using raw
SystemTimeand manual float conversion is error-prone. Consider using thechronocrate for better timestamp handling.- let duration = SystemTime::now().duration_since(UNIX_EPOCH).unwrap(); - let created_at = duration.as_secs() as f64 + duration.subsec_nanos() as f64 * 1e-9; + let created_at = chrono::Utc::now().timestamp_nanos() as f64 * 1e-9;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
prover/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
prover/Cargo.toml(2 hunks)prover/src/prover.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- prover/Cargo.toml
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: fmt
- GitHub Check: tests
🔇 Additional comments (3)
prover/src/prover.rs (3)
24-29: LGTM! Well-designed thread-safe state management.The struct uses appropriate synchronization primitives:
RwLockforcircuits_handler_providerenables concurrent readsArc<Mutex>for task management fields ensures safe sharing across threads
56-61:⚠️ Potential issueReplace expect() with proper error handling.
Using
expect()can cause panics. Consider propagating the error using?operator with a descriptive error message.- .expect("failed to get circuit handler"); + .map_err(|e| anyhow::anyhow!("failed to get circuit handler: {}", e))?;Likely invalid or redundant comment.
73-112: 🛠️ Refactor suggestionImprove task state management in query_task.
The current implementation has several issues:
- Multiple
unwrap()calls that could panic- Taking ownership of the handle on every query is inefficient
- Complex nested match structure
Consider this safer implementation:
async fn query_task(&self, req: QueryTaskRequest) -> QueryTaskResponse { - let handle = self.current_task.lock().unwrap().take(); + let handle = self.current_task + .lock() + .map_err(|e| format!("failed to acquire lock: {}", e))? + .take(); + if let Some(handle) = handle { if handle.is_finished() { return match handle.await { Ok(Ok(proof)) => QueryTaskResponse { task_id: req.task_id, status: TaskStatus::Success, proof: Some(proof), ..Default::default() }, Ok(Err(e)) => QueryTaskResponse { task_id: req.task_id, status: TaskStatus::Failed, error: Some(format!("proving task failed: {}", e)), ..Default::default() }, Err(e) => QueryTaskResponse { task_id: req.task_id, status: TaskStatus::Failed, error: Some(format!("proving task panicked: {}", e)), ..Default::default() }, }; } else { - *self.current_task.lock().unwrap() = Some(handle); + if let Ok(mut guard) = self.current_task.lock() { + *guard = Some(handle); + } return QueryTaskResponse { task_id: req.task_id, status: TaskStatus::Proving, ..Default::default() }; } }Likely invalid or redundant comment.
|
raised a pr here to resolve the ci failing. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
prover/src/main.rs (1)
45-57: Improve error handling in circuit type initialization.The code could benefit from more descriptive error handling when processing circuit types.
Consider this improvement:
- let mut prover_types = vec![]; - cfg.sdk_config - .prover - .circuit_types - .iter() - .for_each(|circuit_type| { - if let Some(pt) = get_prover_type(*circuit_type) { - if !prover_types.contains(&pt) { - prover_types.push(pt); - } - } - }); + let prover_types: Vec<_> = cfg.sdk_config + .prover + .circuit_types + .iter() + .filter_map(|&circuit_type| get_prover_type(circuit_type)) + .collect::<std::collections::HashSet<_>>() + .into_iter() + .collect(); + + if prover_types.is_empty() { + return Err(anyhow::anyhow!("No valid prover types found in configuration")); + }prover/src/zk_circuits_handler.rs (1)
137-163: Improve error handling and performance in init_vks.The current implementation silently ignores errors and could be more efficient.
Consider this improvement:
pub async fn init_vks( &self, config: &LocalProverConfig, prover_types: Vec<ProverType>, - ) -> Vec<String> { - let mut vks = Vec::new(); + ) -> Result<Vec<String>> { + let mut vks = Vec::with_capacity( + self.circuits_handler_builder_map.len() * prover_types.len() + ); for (hard_fork_name, build) in self.circuits_handler_builder_map.iter() { let handler = - build(prover_types.clone(), config).expect("failed to build circuits handler"); + build(prover_types.clone(), config) + .map_err(|e| anyhow::anyhow!( + "Failed to build circuits handler for {}: {}", + hard_fork_name, e + ))?; for prover_type in prover_types.iter() { for task_type in get_circuit_types(*prover_type).into_iter() { let vk = handler .get_vk(task_type) .await .map_or("".to_string(), utils::encode_vk); if !vk.is_empty() { vks.push(vk) } else { + log::warn!( + "No VK found for {}, task_type: {:?}", + hard_fork_name, task_type + ); } } } } - vks + Ok(vks) }prover/src/prover.rs (1)
85-100: Improve error handling in prove method.The error handling could be more descriptive and the clone operations could be optimized.
Apply these improvements:
async fn prove(&self, req: ProveRequest) -> ProveResponse { let handler = self .circuits_handler_provider .write() .await - .get_circuits_handler(&req.hard_fork_name, self.prover_types.clone()) - .expect("failed to get circuit handler"); + .get_circuits_handler(&req.hard_fork_name, &self.prover_types) + .map_err(|e| { + log::error!("Failed to get circuit handler: {}", e); + anyhow::anyhow!("Circuit handler initialization failed: {}", e) + })?; match self.do_prove(req, handler).await { Ok(resp) => resp, Err(e) => { + log::error!("Proving failed: {}", e); ProveResponse { status: TaskStatus::Failed, error: Some(format!("failed to request proof: {}", e)), ..Default::default() } } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
prover/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
prover/Cargo.toml(3 hunks)prover/src/main.rs(2 hunks)prover/src/prover.rs(1 hunks)prover/src/zk_circuits_handler.rs(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- prover/Cargo.toml
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: fmt
- GitHub Check: tests
🔇 Additional comments (1)
prover/src/zk_circuits_handler.rs (1)
47-49:⚠️ Potential issueReplace panic with proper error handling.
Using
panic!in library code makes it harder to handle errors gracefully in the calling code.Apply this diff to propagate the error instead:
- if let Err(e) = AssetsDirEnvConfig::init() { - panic!("AssetsDirEnvConfig init failed: {:#}", e); - } + AssetsDirEnvConfig::init().map_err(|e| { + anyhow::anyhow!("AssetsDirEnvConfig initialization failed: {:#}", e) + })?;Likely invalid or redundant comment.
omerfirmak
force-pushed
the
feat-prover-integrate-proving-sdk
branch
from
7c38ac6 to
5770194
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
prover/src/zk_circuits_handler/darwin.rs (1)
321-321:⚠️ Potential issueBundle VK case needs proper implementation.
The
todo!()forProofType::Bundleshould be properly implemented.- ProofType::Bundle => todo!(), + ProofType::Bundle => bail!("Bundle verification key reading not yet implemented"),prover/src/zk_circuits_handler/darwin_v2.rs (3)
131-131: 🛠️ Refactor suggestionUse read lock for protocol check.
The protocol check operation only needs read access to the prover. Using a write lock here unnecessarily blocks other readers.
- let is_valid = prover.write().await.check_protocol_of_chunks(&chunk_proofs); + let is_valid = prover.read().await.check_protocol_of_chunks(&chunk_proofs);
185-199:⚠️ Potential issueReplace unwrap calls with proper error handling.
Using
unwrap()could lead to runtime panics if the provers aren't initialized.match task_type { - ProofType::Chunk => self.chunk_prover.as_ref().unwrap().read().await.get_vk(), + ProofType::Chunk => self.chunk_prover + .as_ref() + .ok_or_else(|| anyhow::anyhow!("ChunkProver not initialized"))? + .read() + .await + .get_vk() + .ok_or_else(|| anyhow::anyhow!("Failed to get chunk verification key"))?, ProofType::Batch => self .batch_prover .as_ref() - .unwrap() + .ok_or_else(|| anyhow::anyhow!("BatchProver not initialized"))? .read() .await - .get_batch_vk(), + .get_batch_vk() + .ok_or_else(|| anyhow::anyhow!("Failed to get batch verification key"))?, ProofType::Bundle => self .batch_prover .as_ref() - .unwrap() + .ok_or_else(|| anyhow::anyhow!("BatchProver not initialized"))? .read() .await - .get_bundle_vk(), + .get_bundle_vk() + .ok_or_else(|| anyhow::anyhow!("Failed to get bundle verification key"))?,
379-379:⚠️ Potential issueBundle VK case needs proper implementation.
The
todo!()forProofType::Bundleshould be properly implemented.- ProofType::Bundle => todo!(), + ProofType::Bundle => bail!("Bundle verification key reading not yet implemented"),
🧹 Nitpick comments (3)
prover/src/zk_circuits_handler/darwin.rs (3)
88-88: Consider using a more descriptive unreachable message.The unreachable branch should include context about which proof types are actually supported.
- _ => unreachable!(), + _ => unreachable!("Unsupported proof type. Only Chunk, Batch, and Bundle are supported."),
110-110: Improve error message for uninitialized prover.The unreachable error message should be more specific about the initialization issue.
- unreachable!("please check errors in proof_type logic") + unreachable!("ChunkProver not initialized. Ensure proof_type Chunk was included during initialization")
114-114: Enhance error handling for JSON deserialization.The deserialization error should provide context about the failure.
- let chunk_traces: Vec<BlockTrace> = serde_json::from_str(&prove_request.input)?; + let chunk_traces: Vec<BlockTrace> = serde_json::from_str(&prove_request.input) + .context("Failed to deserialize chunk traces from prove request")?;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
prover/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
prover/Cargo.toml(3 hunks)prover/src/main.rs(2 hunks)prover/src/prover.rs(1 hunks)prover/src/types.rs(1 hunks)prover/src/utils.rs(0 hunks)prover/src/zk_circuits_handler.rs(5 hunks)prover/src/zk_circuits_handler/common.rs(2 hunks)prover/src/zk_circuits_handler/darwin.rs(9 hunks)prover/src/zk_circuits_handler/darwin_v2.rs(9 hunks)
💤 Files with no reviewable changes (1)
- prover/src/utils.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- prover/Cargo.toml
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: clippy
- GitHub Check: tests
🔇 Additional comments (8)
prover/src/zk_circuits_handler/common.rs (1)
6-6: LGTM! The changes align with the proving-sdk integration.The update to use
ProofTypefromscroll_proving_sdkinstead ofProverTypeis consistent with the PR objectives.Also applies to: 22-24
prover/src/main.rs (1)
51-51: Handle the result from prover.run().The result from
prover.run().awaitis being ignored. This could hide potential errors during execution.Apply this diff to properly handle the result:
- prover.run().await; + prover.run().await?;prover/src/types.rs (1)
4-4: LGTM! The changes align with the proving-sdk integration.The update to use
CircuitTypefromscroll_proving_sdkis consistent with the PR objectives.Also applies to: 11-11, 21-21
prover/src/zk_circuits_handler.rs (1)
42-44:⚠️ Potential issueAvoid using panic! in library code.
Using
panic!makes the code harder to test and maintain. Consider propagating the error instead:- if let Err(e) = AssetsDirEnvConfig::init() { - panic!("AssetsDirEnvConfig init failed: {:#}", e); - } + AssetsDirEnvConfig::init().map_err(|e| { + anyhow::anyhow!("Failed to initialize AssetsDirEnvConfig: {}", e) + })?;Likely invalid or redundant comment.
prover/src/prover.rs (2)
159-162: Critical: Avoid blocking operations in async context.Using
block_oninside aspawn_blockingtask can block other tasks in the runtime. This needs to be redesigned to properly handle async operations.Consider this non-blocking approach:
- let req_clone = req.clone(); - let handle = Handle::current(); - let task_handle = - tokio::task::spawn_blocking(move || handle.block_on(handler.get_proof_data(req_clone))); + let task_handle = tokio::spawn(async move { + handler.get_proof_data(req).await + });For CPU-intensive operations, consider using a dedicated thread pool:
let (tx, rx) = tokio::sync::oneshot::channel(); let req_clone = req.clone(); let handler_clone = handler.clone(); std::thread::spawn(move || { let runtime = tokio::runtime::Builder::new_current_thread() .enable_all() .build() .expect("Failed to create runtime"); let result = runtime.block_on(handler_clone.get_proof_data(req_clone)); let _ = tx.send(result); }); let task_handle = tokio::spawn(async move { rx.await.expect("thread panicked") });
90-127: Improve task state management.The current implementation has several issues:
- Taking ownership of the handle on every query is inefficient
- Error states could be more descriptive
- Task status transitions could be clearer
Consider implementing a proper state machine:
#[derive(Debug)] enum TaskState { NotStarted, Running(JoinHandle<Result<String>>), Completed(Result<String>), Failed(String), } impl TaskState { fn is_terminal(&self) -> bool { matches!(self, Self::Completed(_) | Self::Failed(_)) } } // Then modify query_task to use this state machineprover/src/zk_circuits_handler/darwin.rs (2)
3-8: LGTM! Good transition to async/concurrent design.The switch from
RefCelltoRwLockfor prover fields and the addition of async-related imports improve concurrent access patterns.Also applies to: 41-42
131-131: 🛠️ Refactor suggestionUse read lock for protocol check.
The protocol check operation only needs read access to the prover. Using a write lock here unnecessarily blocks other readers.
- let is_valid = prover.write().await.check_protocol_of_chunks(&chunk_proofs); + let is_valid = prover.read().await.check_protocol_of_chunks(&chunk_proofs);Likely invalid or redundant comment.
omerfirmak
force-pushed
the
feat-prover-integrate-proving-sdk
branch
from
5770194 to
81b17a0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
prover/src/zk_circuits_handler.rs (2)
Line range hint
59-63: Add error handling for unsupported proof types.The match statement uses
unreachable!()for the default case, which could cause a panic.let degrees: Vec<u32> = get_degrees(&proof_types_set, |proof_type| match proof_type { ProofType::Chunk => ZKEVM_DEGREES.clone(), ProofType::Batch => AGG_DEGREES.clone(), ProofType::Bundle => AGG_DEGREES.clone(), - _ => unreachable!(), + _ => bail!("Unsupported proof type: {:?}", proof_type), });
Line range hint
185-199: Replace unwrap() calls with proper error handling.Using
unwrap()could lead to runtime panics if the provers aren't initialized.match task_type { - ProofType::Chunk => self.chunk_prover.as_ref().unwrap().read().await.get_vk(), + ProofType::Chunk => self.chunk_prover + .as_ref() + .ok_or_else(|| anyhow::anyhow!("ChunkProver not initialized"))? + .read() + .await + .get_vk(), ProofType::Batch => self .batch_prover .as_ref() - .unwrap() + .ok_or_else(|| anyhow::anyhow!("BatchProver not initialized"))? .read() .await .get_batch_vk(), ProofType::Bundle => self .batch_prover .as_ref() - .unwrap() + .ok_or_else(|| anyhow::anyhow!("BatchProver not initialized"))? .read() .await .get_bundle_vk(),
♻️ Duplicate comments (2)
prover/src/zk_circuits_handler/darwin.rs (2)
185-199:⚠️ Potential issueReplace unwrap calls with proper error handling.
Using
unwrap()could lead to runtime panics if the provers aren't initialized.match task_type { - ProofType::Chunk => self.chunk_prover.as_ref().unwrap().read().await.get_vk(), + ProofType::Chunk => self.chunk_prover + .as_ref() + .ok_or_else(|| anyhow::anyhow!("ChunkProver not initialized"))? + .read() + .await + .get_vk(), ProofType::Batch => self .batch_prover .as_ref() - .unwrap() + .ok_or_else(|| anyhow::anyhow!("BatchProver not initialized"))? .read() .await .get_batch_vk(), ProofType::Bundle => self .batch_prover .as_ref() - .unwrap() + .ok_or_else(|| anyhow::anyhow!("BatchProver not initialized"))? .read() .await .get_bundle_vk(),
321-321:⚠️ Potential issueImplement Bundle VK reading.
The
read_vkfunction has an unimplemented match arm forCircuitType::Bundle.Would you like me to help implement the Bundle VK reading functionality?
🧹 Nitpick comments (4)
prover/src/main.rs (1)
43-45: Improve error handling for configuration loading.The configuration loading and prover initialization could be more robust with better error messages.
- let cfg = LocalProverConfig::from_file(args.config_file)?; - let sdk_config = cfg.sdk_config.clone(); - let local_prover = LocalProver::new(cfg); + let cfg = LocalProverConfig::from_file(&args.config_file) + .map_err(|e| anyhow::anyhow!("Failed to load config from {}: {}", args.config_file, e))?; + let sdk_config = cfg.sdk_config.clone(); + let local_prover = LocalProver::new(cfg) + .map_err(|e| anyhow::anyhow!("Failed to initialize local prover: {}", e))?;prover/src/zk_circuits_handler/darwin_v2.rs (2)
59-63: Consider handling unknown proof types explicitly.The
unreachable!()arm could be replaced with a more explicit error handling approach.- _ => unreachable!(), + _ => vec![], // or return Result::Err for invalid proof types
81-86: Document shared prover usage for Batch and Bundle types.The code reuses the same prover instance for both Batch and Bundle proof types. This should be documented for maintainability.
+ // Batch and Bundle proofs share the same prover instance ProofType::Batch | ProofType::Bundle => {prover/src/zk_circuits_handler/darwin.rs (1)
59-63: Consider handling future proof types.The
unreachable!()macro in the match arms suggests that new proof types might cause runtime panics. Consider using a more graceful error handling approach.- _ => unreachable!(), + _ => vec![], // or return a Result with an errorAlso applies to: 88-89
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
prover/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
prover/Cargo.toml(3 hunks)prover/src/main.rs(2 hunks)prover/src/prover.rs(1 hunks)prover/src/types.rs(1 hunks)prover/src/utils.rs(0 hunks)prover/src/zk_circuits_handler.rs(5 hunks)prover/src/zk_circuits_handler/common.rs(2 hunks)prover/src/zk_circuits_handler/darwin.rs(9 hunks)prover/src/zk_circuits_handler/darwin_v2.rs(9 hunks)
💤 Files with no reviewable changes (1)
- prover/src/utils.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- prover/Cargo.toml
- prover/src/zk_circuits_handler/common.rs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: tests
🔇 Additional comments (8)
prover/src/main.rs (1)
51-51:⚠️ Potential issueHandle the result from prover.run().
The result from
prover.run().awaitis being ignored. This could hide potential errors during execution.- prover.run().await; + prover.run().await?;Likely invalid or redundant comment.
prover/src/types.rs (1)
11-11: Consider adding validation for CircuitType.The
task_typeandproof_typefields now useCircuitType, but there's no validation to ensure only supported circuit types are used.Also applies to: 21-21
prover/src/prover.rs (1)
159-162:⚠️ Potential issueCritical: Avoid blocking operations in async context.
Using
block_oninside aspawn_blockingtask can block other tasks in the runtime. This needs to be redesigned to properly handle async operations.- let req_clone = req.clone(); - let handle = Handle::current(); - let task_handle = - tokio::task::spawn_blocking(move || handle.block_on(handler.get_proof_data(req_clone))); + let task_handle = tokio::spawn(async move { + handler.get_proof_data(req).await + });Likely invalid or redundant comment.
prover/src/zk_circuits_handler/darwin_v2.rs (3)
41-42: LGTM! Good use of RwLock for thread-safe concurrent access.The transition from
RefCelltoRwLockis a good improvement for concurrent access to the provers.
379-379:⚠️ Potential issueImplement Bundle VK case properly.
The
todo!()forProofType::Bundleshould be properly implemented or documented.- ProofType::Bundle => todo!(), + ProofType::Bundle => bail!("Bundle verification key reading not yet implemented"),Likely invalid or redundant comment.
185-192:⚠️ Potential issueReplace unwrap with proper error handling.
Using
unwrap()could lead to runtime panics if the provers aren't initialized.- ProofType::Chunk => self.chunk_prover.as_ref().unwrap().read().await.get_vk(), + ProofType::Chunk => self.chunk_prover + .as_ref() + .ok_or_else(|| anyhow::anyhow!("ChunkProver not initialized"))? + .read() + .await + .get_vk(),Likely invalid or redundant comment.
prover/src/zk_circuits_handler/darwin.rs (2)
3-8: LGTM! Well-structured async transition.The transition from
RefCelltoRwLockand the addition of async-related imports are well-implemented, providing proper concurrency support.Also applies to: 41-42
98-111: 🛠️ Refactor suggestionImprove error handling for missing provers.
Using
unreachable!()for missing provers could lead to runtime panics. Consider returning a proper error instead.- unreachable!("please check errors in proof_type logic") + bail!("Chunk prover not initialized for the requested proof type")Likely invalid or redundant comment.
omerfirmak
force-pushed
the
feat-prover-integrate-proving-sdk
branch
from
81b17a0 to
40b15f7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
prover/src/main.rs (1)
38-41: Consider usinganyhow::Resultfor version check exit.Instead of directly calling
std::process::exit(0), consider returningOk(())to maintain consistent error handling throughout the function.- println!("version is {}", get_version()); - std::process::exit(0); + println!("version is {}", get_version()); + return Ok(());prover/src/prover.rs (1)
128-129: Improve error handling in new().Using
expectfor error handling is not ideal in a library context.- let circuits_handler_provider = CircuitsHandlerProvider::new(config.clone()) - .expect("failed to create circuits handler provider"); + let circuits_handler_provider = CircuitsHandlerProvider::new(config.clone()) + .map_err(|e| anyhow::anyhow!("failed to create circuits handler provider: {}", e))?;prover/src/zk_circuits_handler/darwin_v2.rs (2)
59-63: Simplify the degrees calculation using a match expression.The current implementation can be simplified for better readability.
- let degrees: Vec<u32> = get_degrees(&proof_types_set, |prover_type| match prover_type { - ProofType::Chunk => ZKEVM_DEGREES.clone(), - ProofType::Batch => AGG_DEGREES.clone(), - ProofType::Bundle => AGG_DEGREES.clone(), - _ => unreachable!(), - }); + let degrees: Vec<u32> = get_degrees(&proof_types_set, |proof_type| match proof_type { + ProofType::Chunk => ZKEVM_DEGREES.clone(), + _ => AGG_DEGREES.clone(), + });
3-8: LGTM! Good improvements to concurrency support.The transition to async/await and RwLock improves thread safety and concurrent access capabilities. The code structure is consistent throughout the file.
Consider adding documentation about the concurrent access patterns and thread safety guarantees provided by these changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
prover/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
prover/Cargo.toml(3 hunks)prover/src/main.rs(2 hunks)prover/src/prover.rs(1 hunks)prover/src/types.rs(1 hunks)prover/src/utils.rs(0 hunks)prover/src/zk_circuits_handler.rs(5 hunks)prover/src/zk_circuits_handler/common.rs(2 hunks)prover/src/zk_circuits_handler/darwin.rs(9 hunks)prover/src/zk_circuits_handler/darwin_v2.rs(9 hunks)
💤 Files with no reviewable changes (1)
- prover/src/utils.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- prover/src/zk_circuits_handler/common.rs
- prover/Cargo.toml
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: fmt
- GitHub Check: tests
🔇 Additional comments (9)
prover/src/main.rs (1)
48-48:⚠️ Potential issueCritical: Handle the result from prover.run().
The result from
prover.run().awaitis being ignored. This could hide potential errors during execution.- prover.run().await; + prover.run().await?;Likely invalid or redundant comment.
prover/src/types.rs (1)
11-11: LGTM!The update to use
CircuitTypefrom scroll_proving_sdk is a good improvement, making the types more consistent with the SDK.Also applies to: 21-21
prover/src/zk_circuits_handler.rs (1)
21-22: LGTM!The trait has been properly updated to be async and thread-safe. The use of
RwLockfor synchronization is appropriate.Also applies to: 23-25
prover/src/prover.rs (1)
148-151:⚠️ Potential issueCritical: Avoid blocking operations in async context.
Using
block_oninside aspawn_blockingtask can block other tasks in the runtime. This needs to be redesigned to properly handle async operations.Consider this non-blocking approach:
- let req_clone = req.clone(); - let handle = Handle::current(); - let task_handle = - tokio::task::spawn_blocking(move || handle.block_on(handler.get_proof_data(req_clone))); + let task_handle = tokio::spawn(async move { + handler.get_proof_data(req).await + });Likely invalid or redundant comment.
prover/src/zk_circuits_handler/darwin.rs (2)
Line range hint
254-290: LGTM! Well-structured async test.The test has been properly updated to use tokio and async/await. The test structure is clear and follows good practices.
185-199:⚠️ Potential issueReplace unwrap calls with proper error handling.
Using
unwrap()could lead to runtime panics if the provers aren't initialized.match task_type { - ProofType::Chunk => self.chunk_prover.as_ref().unwrap().read().await.get_vk(), + ProofType::Chunk => self.chunk_prover + .as_ref() + .ok_or_else(|| anyhow::anyhow!("ChunkProver not initialized"))? + .read() + .await + .get_vk(), ProofType::Batch => self .batch_prover .as_ref() - .unwrap() + .ok_or_else(|| anyhow::anyhow!("BatchProver not initialized"))? .read() .await .get_batch_vk(), ProofType::Bundle => self .batch_prover .as_ref() - .unwrap() + .ok_or_else(|| anyhow::anyhow!("BatchProver not initialized"))? .read() .await .get_bundle_vk(),Likely invalid or redundant comment.
prover/src/zk_circuits_handler/darwin_v2.rs (3)
41-42: LGTM! Good transition to RwLock for thread safety.The change from
RefCelltoRwLockimproves thread safety by allowing concurrent access to the provers.
185-199: Replace unwrap calls with proper error handling in get_vk.Using
unwrap()could lead to runtime panics if the provers aren't initialized.
379-379: Bundle VK case needs proper implementation in read_vk.The Bundle verification key reading should be properly implemented following the same pattern as chunk and batch VKs.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
prover/config.json (5)
2-4: Revise and Validate Initial SDK Configuration Fields
The renaming from"prover_name"to"prover_name_prefix"in thesdk_configobject aligns with the updated configuration approach. Please ensure that all parts of the codebase that reference this field are updated accordingly. Additionally, the new"keys_dir"field should be validated (e.g., checking that the directory exists or is writable) during configuration loading.
5-10: Review Coordinator Configuration Settings
The"coordinator"block looks standard; however, the"base_url"is currently hardcoded to"http://localhost:8555". To enhance flexibility across environments (development, staging, production), consider allowing an override via environment variables or external configuration sources.
14-17: Document and Validate Circuit Configuration Fields
Within the"prover"object, fields such as"circuit_type","supported_proof_types", and"circuit_version"are set using literal numeric codes and a version string. It is recommended to:
- Document what the numeric values for
"circuit_type"and"supported_proof_types"represent, or replace them with constants/enums.- Validate that
"circuit_version"adheres to semantic versioning (MAJOR.MINOR.PATCH).
21-25: Low Version Circuit Configuration Review
The newly added"low_version_circuit"block includes"hard_fork_name","params_path", and"assets_path". This is a positive move for handling different circuit versions. Consider adding validation to ensure that:
- The file paths (for
"params_path"and"assets_path") exist and are accessible.- The
"hard_fork_name"is among the supported options (e.g., by using an enum or a whitelisting approach).
26-30: High Version Circuit Configuration Review
Similarly, the"high_version_circuit"block is well-structured. Ensure that the configuration loading logic validates the correctness of the"hard_fork_name"and that the paths provided for"params_path"and"assets_path"exist and are valid.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
prover/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
prover/Cargo.toml(3 hunks)prover/config.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- prover/Cargo.toml
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: fmt
- GitHub Check: tests
🔇 Additional comments (1)
prover/config.json (1)
19-19: Verify Relocation of Database Path Setting
The"db_path"field has been moved inside thesdk_configblock. Confirm that the configuration parser and any dependent logic are updated to reflect this new structure.
This reverts commit 40b15f7.
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (3)
prover/src/types.rs (1)
15-23: 🛠️ Refactor suggestionConsider using Result instead of panic for invalid ProverType.
The
from_u8function panics on invalid input, which could make the code less robust.Consider returning a Result:
- fn from_u8(v: u8) -> Self { + fn from_u8(v: u8) -> Result<Self, &'static str> { match v { - 1 => ProverType::Chunk, - 2 => ProverType::Batch, - _ => { - panic!("invalid prover_type") - } + 1 => Ok(ProverType::Chunk), + 2 => Ok(ProverType::Batch), + _ => Err("invalid prover_type") } }prover/src/prover.rs (1)
179-197: 🛠️ Refactor suggestionReplace unwrap calls with proper error handling in get_vk.
Using
unwrap()could lead to runtime panics if the provers aren't initialized.- CircuitType::Chunk => self.chunk_prover.as_ref().unwrap().read().await.get_vk(), + CircuitType::Chunk => { + self.chunk_prover + .as_ref() + .ok_or_else(|| anyhow::anyhow!("ChunkProver not initialized"))? + .read() + .await + .get_vk() + }prover/src/zk_circuits_handler/darwin.rs (1)
370-376:⚠️ Potential issueBundle VK case needs proper implementation in read_vk.
The codebase actively uses bundle verification keys in the coordinator, with a consistent naming pattern (
vk_bundle.vkey).Until implemented:
CircuitType::Bundle => bail!("Bundle verification key reading not yet implemented"),
♻️ Duplicate comments (2)
prover/src/main.rs (1)
65-65:⚠️ Potential issueHandle the result from prover.run().
The result from
prover.run().awaitis being ignored, which could hide potential errors during execution.Apply this diff to properly handle the result:
- prover.run().await; + prover.run().await?;prover/src/prover.rs (1)
174-177:⚠️ Potential issueCritical: Avoid blocking operations in async context.
Using
block_oninside aspawn_blockingtask can block other tasks in the runtime. This needs to be redesigned to properly handle async operations.Consider this non-blocking approach:
- let req_clone = req.clone(); - let handle = Handle::current(); - let task_handle = - tokio::task::spawn_blocking(move || handle.block_on(handler.get_proof_data(req_clone))); + let task_handle = tokio::spawn(async move { + handler.get_proof_data(req).await + });
🧹 Nitpick comments (2)
prover/src/zk_circuits_handler.rs (1)
137-164: Consider parallel initialization of verification keys.The
init_vksfunction processes verification keys sequentially. Since it's async, consider usingfutures::stream::StreamExtto process them concurrently.Example implementation:
use futures::stream::{self, StreamExt}; pub async fn init_vks( &self, config: &LocalProverConfig, prover_types: Vec<ProverType>, ) -> Vec<String> { let mut vks = Vec::new(); let handlers = stream::iter(self.circuits_handler_builder_map.iter()) .map(|(hard_fork_name, build)| async move { let handler = build(prover_types.clone(), config) .expect("failed to build circuits handler"); (hard_fork_name, handler) }) .buffer_unwind(); while let Some((hard_fork_name, handler)) = handlers.next().await { for prover_type in &prover_types { for task_type in get_circuit_types(*prover_type) { if let Some(vk) = handler.get_vk(task_type).await { let encoded = utils::encode_vk(vk); log::info!( "vk for {}, is {}, task_type: {:?}", hard_fork_name, encoded, task_type ); vks.push(encoded); } } } } vks }prover/config.json (1)
6-9: Environment-specific configuration needs implementation.The coordinator's
base_urlis set to localhost, which works for development but may need to be configurable for different environments.Consider implementing environment variable overrides for critical configuration values like
base_url.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
prover/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
prover/Cargo.toml(2 hunks)prover/config.json(1 hunks)prover/src/main.rs(2 hunks)prover/src/prover.rs(1 hunks)prover/src/types.rs(2 hunks)prover/src/utils.rs(1 hunks)prover/src/zk_circuits_handler.rs(5 hunks)prover/src/zk_circuits_handler/darwin.rs(11 hunks)prover/src/zk_circuits_handler/darwin_v2.rs(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- prover/Cargo.toml
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: fmt
- GitHub Check: tests
🔇 Additional comments (7)
prover/src/utils.rs (2)
4-9: LGTM! Clear mapping between ProverType and CircuitType.The function provides a clear and straightforward mapping from ProverType to CircuitType.
11-18: Consider handling potential edge cases in the mapping.The mapping from CircuitType to ProverType is not fully symmetric with
get_circuit_types. For example,ProverType::Batchmaps to bothCircuitType::BatchandCircuitType::Bundle, but both circuit types map back toProverType::Batch. This could lead to confusion if the mappings are used bidirectionally.Consider documenting this behavior or adding tests to verify the expected behavior of these mappings.
prover/src/main.rs (1)
34-37: LGTM! Proper async main setup with tracing.The async main function is properly set up with tokio and includes tracing initialization.
prover/src/types.rs (1)
48-55: LGTM! Clean transition to CircuitType.The Task struct has been cleanly updated to use CircuitType instead of TaskType.
prover/src/zk_circuits_handler.rs (1)
24-29: LGTM! Well-designed async trait.The CircuitsHandler trait is properly marked with async_trait and includes Send + Sync bounds for thread safety.
prover/src/zk_circuits_handler/darwin_v2.rs (2)
370-376: Bundle VK case needs proper implementation in read_vk.The codebase actively uses bundle verification keys in the coordinator, with a consistent naming pattern (
vk_bundle.vkey).
94-107: Improve error handling for uninitialized provers.Using
unreachable!()for uninitialized provers is not ideal.
Purpose or design rationale of this PR
Describe your change. Make sure to answer these three questions: What does this PR do? Why does it do it? How does it do it?
PR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
Deployment tag versioning
Has
tagincommon/version.gobeen updated or have you addedbump-versionlabel to this PR?Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit
New Features
Refactor
Chore