(WIP)feat/upgrade5 prover #1557
Conversation
WalkthroughThe changes in this pull request involve updates to the Changes
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: 12
🧹 Outside diff range and nitpick comments (8)
prover/src/types.rs (1)
62-64: Consider documenting the purpose of ChunkAll variant.The purpose of the
ChunkAllvariant is not immediately clear from its name. Consider adding documentation to explain when this variant should be used.prover/src/prover.rs (2)
32-35: LGTM! Consider extracting the ProverType match condition.The change to use
matches!macro improves readability and maintainability. The logic correctly handles initialization for all chunk-based prover types.Consider extracting the repeated match condition into a helper function since it's used in multiple places:
impl ProverType { pub fn is_chunk_based(&self) -> bool { matches!( self, ProverType::ChunkHalo2 | ProverType::ChunkSp1 | ProverType::ChunkAll ) } }This would simplify both this check and the similar one in
fetch_task.
Line range hint
1-182: Consider documenting the prover type selection strategy.The changes effectively support multiple prover types while maintaining a clean architecture. However, the selection strategy between different chunk-based provers (Halo2, Sp1, All) isn't immediately clear from the code.
Consider:
- Adding documentation explaining when each prover type should be used
- Creating a configuration guide for operators to understand prover selection implications
- Adding metrics/logging to track which prover types are being selected in production
prover/src/zk_circuits_handler/darwin_v2.rs (1)
Line range hint
324-328: Consider adding negative test cases.While the test updates correctly reflect the ProverType changes, consider adding test cases that verify:
- Error handling for unsupported prover types
- Validation of the new ChunkHalo2 proof format
prover/src/zk_circuits_handler/euclid.rs (4)
22-23: Address TODO: Enablesp1_proverintegrationThere is a TODO comment indicating that
sp1_proverintegration needs to be enabled. Consider uncommenting and updating the import statement when ready to integratesp1_prover.Would you like assistance in implementing
sp1_proveror opening a GitHub issue to track this task?
39-41: Address TODO: Enablesp1_proverinChunkProverstructThe
ChunkProverstruct contains a commented-out fieldsp1_proverwith a TODO comment. Consider enabling and initializingsp1_proverwhen appropriate.Would you like assistance in implementing
sp1_proverwithin theChunkProverstruct or opening a GitHub issue to track this task?
189-195: Handle potential deserialization errors ingen_batch_proofIn
gen_batch_proof, the task data is deserialized without handling possible errors that may arise from invalid or malformed JSON. Althoughserde_json::from_strreturns aResult, it might be useful to add context to the error for easier debugging.Consider adding context to the deserialization error:
- let batch_task_detail: BatchTaskDetail = serde_json::from_str(&task.task_data)?; + let batch_task_detail: BatchTaskDetail = serde_json::from_str(&task.task_data) + .context("Failed to deserialize batch task detail")?;
217-219: Clarify the usage ofOUTPUT_DIRThe
get_output_dirmethod returns theOUTPUT_DIRenvironment variable if set. It might be helpful to document or log when the output directory is used for clarity.Consider adding a comment or log statement to indicate when the output directory is utilized.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
prover/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
- prover/Cargo.toml (1 hunks)
- prover/rust-toolchain (0 hunks)
- prover/rust-toolchain.toml (1 hunks)
- prover/src/main.rs (1 hunks)
- prover/src/prover.rs (2 hunks)
- prover/src/types.rs (5 hunks)
- prover/src/utils.rs (1 hunks)
- prover/src/zk_circuits_handler.rs (4 hunks)
- prover/src/zk_circuits_handler/darwin.rs (0 hunks)
- prover/src/zk_circuits_handler/darwin_v2.rs (8 hunks)
- prover/src/zk_circuits_handler/euclid.rs (1 hunks)
💤 Files with no reviewable changes (2)
- prover/rust-toolchain
- prover/src/zk_circuits_handler/darwin.rs
✅ Files skipped from review due to trivial changes (2)
- prover/rust-toolchain.toml
- prover/src/main.rs
🔇 Additional comments (7)
prover/src/utils.rs (1)
29-31: LGTM! The changes align with the PR objectives.The updated match arms correctly handle the new prover types, maintaining backward compatibility while adding support for Sp1 tasks. The
ChunkAllvariant provides a convenient way to handle both Halo2 and Sp1 chunk tasks.Let's verify the consistency with ProverType enum definition:
✅ Verification successful
✅ The match arms are consistent with the ProverType enum definition
The verification confirms that the match arms in
utils.rsperfectly align with theProverTypeenum defined intypes.rs. The enum contains exactly the variantsChunkHalo2,ChunkSp1,ChunkAll, andBatch, and these are handled consistently across the codebase, including:
- Correct serialization/deserialization in
types.rs- Proper task type mapping in
utils.rs- Consistent usage in circuit handlers for both Darwin and Euclid implementations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that ProverType enum matches these variants # Expected: Should find the enum definition with these variants ast-grep --pattern 'enum ProverType { $$$ ChunkHalo2, ChunkSp1, ChunkAll, $$$ }'Length of output: 92
Script:
#!/bin/bash # Let's try a different approach to find the ProverType enum definition # First, let's use ripgrep to find the enum definition rg "enum ProverType" -A 10 # Also search for any usage of ProverType to ensure consistency rg "ProverType::" -A 2Length of output: 5441
prover/Cargo.toml (2)
33-33: Verify the transition strategy from Darwin to Euclid.Since both
prover_darwin_v2andprover_euclidare present, please ensure there's a clear transition strategy and that both provers can coexist without conflicts during the migration period.Run the following script to analyze the migration pattern:
#!/bin/bash # Description: Analyze the migration pattern from Darwin to Euclid # Test 1: Look for conditional compilation flags or feature gates echo "Checking for feature gates..." rg "#\[cfg\(feature|#\[cfg_attr" # Test 2: Search for migration-related code patterns echo "Checking migration patterns..." ast-grep --pattern 'if $_ { $$$_DARWIN$$$ } else { $$$_EUCLID$$$ }'
33-33: Consider using a tagged version instead of a feature branch.Using a feature branch (
feat/hybrid-snark-agg) forprover_euclidin production code could lead to instability if the branch is rebased or force-pushed. Consider switching to a tagged version once the feature is stable.Run the following script to check the dependency graph and verify the coexistence of both provers:
prover/src/zk_circuits_handler.rs (1)
3-3: LGTM: Clean transition to Euclid moduleThe module declaration and import changes align well with the PR objective to upgrade to Euclid.
Also applies to: 13-13
prover/src/zk_circuits_handler/darwin_v2.rs (3)
Line range hint
13-21: LGTM! Good encapsulation practice.The imports are well-organized, and reducing the visibility of
BatchTaskDetailimproves encapsulation since it's only used internally within this module.Also applies to: 27-31
Line range hint
252-264: LGTM! Consistent TaskType handling.The changes in
get_vkandget_proof_datamethods maintain consistency with the ProverType changes while preserving the error handling behavior.Also applies to: 270-274
69-71: Verify ProverType changes across the codebase.The renaming from
ChunktoChunkHalo2looks correct. However, we should verify that all usages of this enum have been updated consistently.Also applies to: 84-94
| ProverType::ChunkHalo2 => 1, | ||
| ProverType::Batch => 2, | ||
| ProverType::ChunkSp1 => 3, | ||
| ProverType::ChunkAll => 4, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a const array to define variant mappings.
The serialization values are duplicated across multiple methods. Consider defining them once to improve maintainability:
impl ProverType {
const VARIANT_MAPPINGS: [(ProverType, u8); 4] = [
(ProverType::ChunkHalo2, 1),
(ProverType::Batch, 2),
(ProverType::ChunkSp1, 3),
(ProverType::ChunkAll, 4),
];
fn to_u8(self) -> u8 {
Self::VARIANT_MAPPINGS
.iter()
.find(|(variant, _)| *variant == self)
.map(|(_, value)| *value)
.unwrap()
}
}Also applies to: 97-100
| 1 => ProverType::ChunkHalo2, | ||
| 2 => ProverType::Batch, | ||
| 3 => ProverType::ChunkSp1, | ||
| 4 => ProverType::ChunkAll, | ||
| _ => { | ||
| panic!("invalid prover_type") | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider returning Result instead of panicking.
The from_u8 method currently panics on invalid input. Consider returning a Result type instead, which would allow callers to handle invalid values gracefully:
- fn from_u8(v: u8) -> Self {
+ fn from_u8(v: u8) -> Result<Self, &'static str> {
match v {
- 1 => ProverType::ChunkHalo2,
- 2 => ProverType::Batch,
- 3 => ProverType::ChunkSp1,
- 4 => ProverType::ChunkAll,
- _ => {
- panic!("invalid prover_type")
- }
+ 1 => Ok(ProverType::ChunkHalo2),
+ 2 => Ok(ProverType::Batch),
+ 3 => Ok(ProverType::ChunkSp1),
+ 4 => Ok(ProverType::ChunkAll),
+ _ => Err("invalid prover_type")
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 1 => ProverType::ChunkHalo2, | |
| 2 => ProverType::Batch, | |
| 3 => ProverType::ChunkSp1, | |
| 4 => ProverType::ChunkAll, | |
| _ => { | |
| panic!("invalid prover_type") | |
| } | |
| match v { | |
| 1 => Ok(ProverType::ChunkHalo2), | |
| 2 => Ok(ProverType::Batch), | |
| 3 => Ok(ProverType::ChunkSp1), | |
| 4 => Ok(ProverType::ChunkAll), | |
| _ => Err("invalid prover_type") | |
| } |
| ChunkHalo2, | ||
| ChunkSp1, |
There was a problem hiding this comment.
💡 Codebase verification
Update required: Several files still use the old TaskType::Chunk variant
The verification has found multiple files that need to be updated to use the new ChunkHalo2 and ChunkSp1 variants instead of the old Chunk variant:
prover/src/zk_circuits_handler/darwin_v2.rs: Lines containingchunk_vk = chunk_handler.get_vk(TaskType::Chunk)andcheck_vk(TaskType::Chunk, chunk_vk, ...)common/libzkp/impl/src/lib.rs: UsesTaskType::Chunkin proof verificationcommon/libzkp/impl/src/verifier/darwin.rsanddarwin_v2.rs: Match arms usingTaskType::Chunk
These references need to be updated to use either TaskType::ChunkHalo2 or TaskType::ChunkSp1 based on the prover type being used. The changes are not just simple renames but require determining the correct variant to use in each context.
🔗 Analysis chain
Verify the impact of TaskType enum changes.
The renaming of Chunk to ChunkHalo2 and addition of ChunkSp1 could impact external systems that depend on these values. Please ensure all consumers are updated accordingly.
Let's verify the usage of these enum variants:
Also applies to: 20-23, 36-39
Update remaining references to TaskType::Chunk to use the new variants
The verification reveals several files still using the old TaskType::Chunk variant that need to be updated:
prover/src/zk_circuits_handler/darwin_v2.rs: UsesTaskType::Chunkin verification logiccommon/libzkp/impl/src/lib.rs: UsesTaskType::Chunkin proof verification functionscommon/libzkp/impl/src/verifier/darwin.rsanddarwin_v2.rs: UsesTaskType::Chunkin match arms
Let's verify if these are the only instances and check their context:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old Chunk variant
rg -l "TaskType::Chunk"
# Search for places where TaskType is used to ensure they handle the new ChunkSp1 variant
rg -A 5 "TaskType::"
Length of output: 11203
Script:
#!/bin/bash
# Show the context around TaskType::Chunk usage
rg -B 3 -A 3 "TaskType::Chunk"
# Check if there are any other enum variants we might have missed
ast-grep --pattern 'enum TaskType {
$$$
}'
Length of output: 7946
| if matches!( | ||
| self.config.prover_type, | ||
| ProverType::ChunkHalo2 | ProverType::ChunkSp1 | ProverType::ChunkAll | ||
| ) { |
There was a problem hiding this comment.
💡 Codebase verification
Error handling needs improvement in task fetching
The verification revealed some concerns in the error handling:
- The
fetch_taskresult is unwrapped intask_processor.rswithout proper error context - The error handling in the task fetching logic could be more robust, especially for the new prover types
Key locations that need attention:
prover/src/task_processor.rs: Replacefetch_result.unwrap()with proper error handlingprover/src/prover.rs: Consider adding more specific error types for the new prover type cases
🔗 Analysis chain
LGTM! Verify error handling for edge cases.
The task fetching logic correctly handles the new prover types and includes appropriate error handling.
Let's verify the error handling paths are tested:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for tests covering error cases in task fetching
rg -A 5 "fetch_task.*Error|test.*fetch_task" prover/src/
# Look for potential panic scenarios in error paths
ast-grep --pattern 'unwrap()'
Length of output: 90
Script:
#!/bin/bash
# Search for tests covering task fetching with more context
rg -A 10 "fetch_task" prover/src/
# Look for error handling in task fetching implementation
rg -A 10 "fn fetch_task" prover/src/
# Look for unwrap calls specifically in prover.rs
rg "unwrap\(\)" prover/src/prover.rs
Length of output: 2325
| }; | ||
| let degrees: Vec<u32> = get_degrees(&prover_types_set, |prover_type| match prover_type { | ||
| ProverType::ChunkHalo2 | ProverType::ChunkAll => ZKEVM_DEGREES.clone(), | ||
| ProverType::ChunkSp1 => ZKEVM_DEGREES.clone(), // TODO(rohit) |
There was a problem hiding this comment.
Clarify or resolve TODO comment for ProverType::ChunkSp1 degrees
There's a TODO comment // TODO(rohit) indicating that the degrees for ProverType::ChunkSp1 may need adjustment. Ensure that the correct degrees are used for ChunkSp1 and update the code accordingly.
Would you like assistance in determining the appropriate degrees for ProverType::ChunkSp1 or opening a GitHub issue to track this task?
| unreachable!("please check errors in proof_type logic") | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using unreachable!() for error handling
Using unreachable!() can cause a panic if this code path is ever reached due to logic errors. Consider returning a proper error using bail!() or another error handling mechanism to provide better error reporting.
Apply this diff to replace unreachable!() with an error return:
- unreachable!("please check errors in proof_type logic")
+ bail!("No suitable chunk prover found; please check the prover type logic")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| unreachable!("please check errors in proof_type logic") | |
| } | |
| bail!("No suitable chunk prover found; please check the prover type logic") | |
| } |
| _ => unreachable!(), | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle unexpected TaskType variants instead of using unreachable!()
Using unreachable!() may lead to panics if new TaskType variants are added in the future. Consider returning a clear error message to improve maintainability.
Apply this diff to return an error:
- _ => unreachable!(),
+ other => bail!("Unsupported task type: {:?}", other),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _ => unreachable!(), | |
| } | |
| other => bail!("Unsupported task type: {:?}", other), | |
| } |
| .as_ref() | ||
| .and_then(|p| p.borrow().get_vk()) | ||
| }), | ||
| TaskType::ChunkSp1 => unimplemented!(), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using unimplemented!() in get_vk method
Using unimplemented!() for TaskType::ChunkSp1 can cause a panic if this variant is encountered. Consider handling this case appropriately by returning None or an error message.
Apply this diff to handle the unsupported task type:
- TaskType::ChunkSp1 => unimplemented!(),
+ TaskType::ChunkSp1 => {
+ // Return None or handle accordingly
+ None
+ },Committable suggestion was skipped due to low confidence.
| fn gen_sp1_chunk_proof(&self, task: &crate::types::Task) -> Result<String> { | ||
| let chunk_trace = self.gen_chunk_traces(task)?; | ||
| todo!() | ||
| } |
There was a problem hiding this comment.
Implement gen_sp1_chunk_proof or handle unimplemented function appropriately
The function gen_sp1_chunk_proof contains a todo!() macro, which will cause a panic if invoked. This may lead to runtime errors if TaskType::ChunkSp1 tasks are processed. Consider implementing this function or returning an appropriate error if ChunkSp1 is not yet supported.
Would you like assistance in implementing gen_sp1_chunk_proof or updating the code to handle unsupported task types? Alternatively, consider returning an error to prevent panics.
Apply this diff to return an error instead of panicking:
- todo!()
+ bail!("SP1 chunk proof generation is not yet implemented")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn gen_sp1_chunk_proof(&self, task: &crate::types::Task) -> Result<String> { | |
| let chunk_trace = self.gen_chunk_traces(task)?; | |
| todo!() | |
| } | |
| fn gen_sp1_chunk_proof(&self, task: &crate::types::Task) -> Result<String> { | |
| let chunk_trace = self.gen_chunk_traces(task)?; | |
| bail!("SP1 chunk proof generation is not yet implemented") | |
| } |
| fn get_sorted_traces_by_hashes(&self, block_hashes: &[CommonHash]) -> Result<Vec<BlockTrace>> { | ||
| if block_hashes.is_empty() { | ||
| log::error!("[prover] failed to get sorted traces: block_hashes are empty"); | ||
| bail!("block_hashes are empty") | ||
| } | ||
|
|
||
| let mut block_traces = Vec::new(); | ||
| for hash in block_hashes.iter() { | ||
| let trace = self | ||
| .geth_client | ||
| .as_ref() | ||
| .unwrap() | ||
| .borrow_mut() | ||
| .get_block_trace_by_hash(hash)?; | ||
| block_traces.push(trace); | ||
| } | ||
|
|
||
| block_traces.sort_by(|a, b| { | ||
| if get_block_number(a).is_none() { | ||
| Ordering::Less | ||
| } else if get_block_number(b).is_none() { | ||
| Ordering::Greater | ||
| } else { | ||
| get_block_number(a) | ||
| .unwrap() | ||
| .cmp(&get_block_number(b).unwrap()) | ||
| } | ||
| }); | ||
|
|
||
| let block_numbers: Vec<u64> = block_traces | ||
| .iter() | ||
| .map(|trace| get_block_number(trace).unwrap_or(0)) | ||
| .collect(); | ||
| let mut i = 0; | ||
| while i < block_numbers.len() - 1 { | ||
| if block_numbers[i] + 1 != block_numbers[i + 1] { | ||
| log::error!( | ||
| "[prover] block numbers are not continuous, got {} and {}", | ||
| block_numbers[i], | ||
| block_numbers[i + 1] | ||
| ); | ||
| bail!( | ||
| "block numbers are not continuous, got {} and {}", | ||
| block_numbers[i], | ||
| block_numbers[i + 1] | ||
| ) | ||
| } | ||
| i += 1; | ||
| } | ||
|
|
||
| Ok(block_traces) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and logging in get_sorted_traces_by_hashes
The method get_sorted_traces_by_hashes could benefit from enhanced error handling and logging:
- If
get_block_numberreturnsNone, it defaults to 0 inblock_numbers. This may cause unexpected behavior in sorting and validation. - The while loop uses manual indexing, which can be more idiomatically handled using iterator methods.
Consider refactoring the code for clarity and robustness:
- let block_numbers: Vec<u64> = block_traces
- .iter()
- .map(|trace| get_block_number(trace).unwrap_or(0))
- .collect();
- let mut i = 0;
- while i < block_numbers.len() - 1 {
- if block_numbers[i] + 1 != block_numbers[i + 1] {
+ let block_numbers: Vec<u64> = block_traces
+ .iter()
+ .map(|trace| get_block_number(trace))
+ .collect::<Option<Vec<_>>>()
+ .ok_or_else(|| anyhow!("Failed to get block numbers from traces"))?;
+
+ for window in block_numbers.windows(2) {
+ if window[0] + 1 != window[1] {
log::error!(
"[prover] block numbers are not continuous, got {} and {}",
- block_numbers[i],
- block_numbers[i + 1]
+ window[0],
+ window[1]
);
bail!(
"block numbers are not continuous, got {} and {}",
- block_numbers[i],
- block_numbers[i + 1]
+ window[0],
+ window[1]
)
}
- i += 1;
}This change:
- Ensures that all block numbers are present and valid.
- Uses iterator methods for better readability.
- Eliminates manual indexing.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn get_sorted_traces_by_hashes(&self, block_hashes: &[CommonHash]) -> Result<Vec<BlockTrace>> { | |
| if block_hashes.is_empty() { | |
| log::error!("[prover] failed to get sorted traces: block_hashes are empty"); | |
| bail!("block_hashes are empty") | |
| } | |
| let mut block_traces = Vec::new(); | |
| for hash in block_hashes.iter() { | |
| let trace = self | |
| .geth_client | |
| .as_ref() | |
| .unwrap() | |
| .borrow_mut() | |
| .get_block_trace_by_hash(hash)?; | |
| block_traces.push(trace); | |
| } | |
| block_traces.sort_by(|a, b| { | |
| if get_block_number(a).is_none() { | |
| Ordering::Less | |
| } else if get_block_number(b).is_none() { | |
| Ordering::Greater | |
| } else { | |
| get_block_number(a) | |
| .unwrap() | |
| .cmp(&get_block_number(b).unwrap()) | |
| } | |
| }); | |
| let block_numbers: Vec<u64> = block_traces | |
| .iter() | |
| .map(|trace| get_block_number(trace).unwrap_or(0)) | |
| .collect(); | |
| let mut i = 0; | |
| while i < block_numbers.len() - 1 { | |
| if block_numbers[i] + 1 != block_numbers[i + 1] { | |
| log::error!( | |
| "[prover] block numbers are not continuous, got {} and {}", | |
| block_numbers[i], | |
| block_numbers[i + 1] | |
| ); | |
| bail!( | |
| "block numbers are not continuous, got {} and {}", | |
| block_numbers[i], | |
| block_numbers[i + 1] | |
| ) | |
| } | |
| i += 1; | |
| } | |
| Ok(block_traces) | |
| } | |
| fn get_sorted_traces_by_hashes(&self, block_hashes: &[CommonHash]) -> Result<Vec<BlockTrace>> { | |
| if block_hashes.is_empty() { | |
| log::error!("[prover] failed to get sorted traces: block_hashes are empty"); | |
| bail!("block_hashes are empty") | |
| } | |
| let mut block_traces = Vec::new(); | |
| for hash in block_hashes.iter() { | |
| let trace = self | |
| .geth_client | |
| .as_ref() | |
| .unwrap() | |
| .borrow_mut() | |
| .get_block_trace_by_hash(hash)?; | |
| block_traces.push(trace); | |
| } | |
| block_traces.sort_by(|a, b| { | |
| if get_block_number(a).is_none() { | |
| Ordering::Less | |
| } else if get_block_number(b).is_none() { | |
| Ordering::Greater | |
| } else { | |
| get_block_number(a) | |
| .unwrap() | |
| .cmp(&get_block_number(b).unwrap()) | |
| } | |
| }); | |
| let block_numbers: Vec<u64> = block_traces | |
| .iter() | |
| .map(|trace| get_block_number(trace)) | |
| .collect::<Option<Vec<_>>>() | |
| .ok_or_else(|| anyhow!("Failed to get block numbers from traces"))?; | |
| for window in block_numbers.windows(2) { | |
| if window[0] + 1 != window[1] { | |
| log::error!( | |
| "[prover] block numbers are not continuous, got {} and {}", | |
| window[0], | |
| window[1] | |
| ); | |
| bail!( | |
| "block numbers are not continuous, got {} and {}", | |
| window[0], | |
| window[1] | |
| ) | |
| } | |
| } | |
| Ok(block_traces) | |
| } |
amoylan2
force-pushed
the
feat/upgrade5-prover
branch
from
238d903
to
df6f687
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
prover/src/zk_circuits_handler.rs (1)
64-67: Document the dual-handler migration strategy.The code maintains two different handlers:
DarwinV2Handlerfor low version circuitsEuclidHandlerfor high version circuitsConsider adding documentation to explain:
- The criteria for handler selection based on circuit versions
- Any behavioral differences between handlers
- The migration timeline and deprecation plans for
DarwinV2HandlerAlso applies to: 87-91
prover/src/zk_circuits_handler/darwin_v2.rs (1)
Line range hint
252-274: Consider handling all TaskType variants explicitly.The match expressions use an unreachable arm (
_ => unreachable!()), which could mask potential new TaskType variants. Consider either:
- Explicitly listing all possible variants, or
- Providing a more descriptive error message in the unreachable arm.
fn get_vk(&self, task_type: TaskType) -> Option<Vec<u8>> { match task_type { TaskType::ChunkHalo2 => self .chunk_prover .as_ref() .and_then(|prover| prover.borrow().get_vk()), TaskType::Batch => self .batch_prover .as_ref() .and_then(|prover| prover.borrow().get_batch_vk()), TaskType::Bundle => self .batch_prover .as_ref() .and_then(|prover| prover.borrow().get_bundle_vk()), - _ => unreachable!(), + TaskType::Undefined => None, } }prover/src/zk_circuits_handler/euclid.rs (2)
47-53: Add documentation comments toEuclidHandlerstructAdding documentation comments (
///) to theEuclidHandlerstruct and its fields can improve code maintainability and readability for other developers interacting with this code.Example:
/// `EuclidHandler` manages different types of provers for proof generation. #[derive(Default)] pub struct EuclidHandler { /// Prover for chunk tasks. chunk_prover: Option<ChunkProver>, /// Prover for batch tasks. batch_prover: Option<RefCell<BatchProver<'static>>>, /// Client to interact with Geth for fetching block traces. geth_client: Option<Rc<RefCell<GethClient>>>, }
217-219: Simplifyget_output_dirmethod by inliningThe method
get_output_dirsimply returnsOUTPUT_DIR.as_deref(). Consider inlining this method or makingOUTPUT_DIRdirectly accessible if there's no additional logic required.Alternatively, if you anticipate adding more logic to this method in the future, you can leave it as is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- prover/Cargo.toml (1 hunks)
- prover/src/zk_circuits_handler.rs (4 hunks)
- prover/src/zk_circuits_handler/darwin_v2.rs (7 hunks)
- prover/src/zk_circuits_handler/euclid.rs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- prover/Cargo.toml
🔇 Additional comments (10)
prover/src/zk_circuits_handler.rs (1)
3-3: LGTM: Clean module organization for Euclid integration.The module declarations and imports are properly organized for the transition from Darwin to Euclid prover.
Also applies to: 13-13
prover/src/zk_circuits_handler/darwin_v2.rs (3)
Line range hint
324-329: LGTM! Test changes are consistent with the type renaming.The test has been properly updated to use
ProverType::ChunkHalo2instead ofProverType::Chunk, maintaining consistency with the type changes.
69-71: Verify consistent renaming of ProverType::Chunk across the codebase.The prover type has been renamed from
ChunktoChunkHalo2. While the changes in this file are consistent, we should verify this renaming across the entire codebase to ensure no references to the old name remain.Also applies to: 84-94
✅ Verification successful
Based on the search results, I can generate the final response as we have comprehensive information about the ProverType usage across the codebase.
The codebase is already consistently using ProverType::ChunkHalo2
The search results show that there are no remaining references to
ProverType::Chunk. Instead, the codebase consistently uses an expanded set of prover types:
ProverType::ChunkHalo2ProverType::ChunkSp1ProverType::ChunkAllProverType::BatchThe changes in the reviewed file are part of a broader, already completed refactoring that introduced multiple chunk prover variants. All relevant files (types.rs, utils.rs, prover.rs, and zk_circuits handlers) are consistently using the new prover type names.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to ProverType::Chunk rg --type rust "ProverType::Chunk" -B 2 -A 2 # Search for new references to ProverType::ChunkHalo2 rg --type rust "ProverType::ChunkHalo2" -B 2 -A 2Length of output: 10269
Line range hint
27-31: Verify the impact of BatchTaskDetail visibility change.The struct's visibility has been reduced from
pubto private. While this change follows Rust's principle of minimal exposure, we should verify that this doesn't break any external dependencies.✅ Verification successful
Let me generate another script to specifically search for
BatchTaskDetailusage since the previous script's output was not relevant to our verification.
Visibility change of BatchTaskDetail is safe
The struct
BatchTaskDetailis only used internally within the same module (darwin_v2.rs) and has a parallel private implementation ineuclid.rs. The visibility change frompubto private aligns with Rust's principle of minimal exposure and doesn't impact any external dependencies.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any external usage of BatchTaskDetail rg --type rust "BatchTaskDetail" -B 2 -A 2Length of output: 10269
Script:
#!/bin/bash # Search for any external usage of BatchTaskDetail rg --type rust "BatchTaskDetail" -B 2 -A 2Length of output: 3430
prover/src/zk_circuits_handler/euclid.rs (6)
73-73: Confirm the degrees forProverType::ChunkSp1The
ProverType::ChunkSp1is assignedZKEVM_DEGREES, and there is a// TODO(rohit)comment indicating that this may need adjustment. Ensure that the correct degree parameters are used forChunkSp1to prevent potential issues during proof generation.Do you need help determining the appropriate degrees for
ProverType::ChunkSp1or opening a GitHub issue to track this?
308-309: 🛠️ Refactor suggestion
⚠️ Potential issueHandle unexpected
TaskTypevariants instead of usingunreachable!()Using
unreachable!()may lead to panics if newTaskTypevariants are added in the future. Consider returning a clear error message to improve maintainability.Apply this diff to return an error:
- _ => unreachable!(), + other => bail!("Unsupported task type: {:?}", other),Likely invalid or redundant comment.
289-289: 🛠️ Refactor suggestion
⚠️ Potential issueAvoid using
unimplemented!()inget_vkmethodUsing
unimplemented!()forTaskType::ChunkSp1can cause a panic if this variant is encountered. Consider handling this case appropriately by returningNoneor an error message.Apply this diff to handle the unsupported task type:
- TaskType::ChunkSp1 => unimplemented!(), + TaskType::ChunkSp1 => { + // Return None or handle accordingly + None + },Likely invalid or redundant comment.
155-158:⚠️ Potential issueHandle unimplemented
gen_sp1_chunk_prooffunction appropriatelyThe
gen_sp1_chunk_prooffunction contains atodo!()macro, which will cause a panic if invoked. This may lead to runtime errors ifTaskType::ChunkSp1tasks are processed. Consider implementing this function or returning an appropriate error ifChunkSp1is not yet supported.Would you like assistance in implementing
gen_sp1_chunk_proofor updating the code to handle unsupported task types?Apply this diff to return an error instead of panicking:
- todo!() + bail!("SP1 chunk proof generation is not yet implemented")Likely invalid or redundant comment.
146-147: 🛠️ Refactor suggestion
⚠️ Potential issueAvoid using
unreachable!()for error handlingUsing
unreachable!()can cause a panic if this code path is ever reached due to logic errors. Consider returning a proper error usingbail!()or another error handling mechanism to provide better error reporting.Apply this diff to replace
unreachable!()with an error return:- unreachable!("please check errors in proof_type logic") + bail!("No suitable chunk prover found; please check the prover type logic")Likely invalid or redundant comment.
226-277: 🛠️ Refactor suggestionImprove error handling and logic in
get_sorted_traces_by_hashesThe method
get_sorted_traces_by_hashescan be improved for better error handling and code clarity:
- Handling
Nonevalues: Currently, ifget_block_numberreturnsNone, it defaults to0, which may not reflect the real situation and could lead to incorrect sorting or validations.- Using iterator methods: Instead of manually indexing with a
whileloop, you can use iterator methods for cleaner and more idiomatic code.Apply this diff to enhance the method:
let block_numbers: Vec<u64> = block_traces .iter() - .map(|trace| get_block_number(trace).unwrap_or(0)) + .map(|trace| get_block_number(trace)) + .collect::<Option<Vec<_>>>() + .ok_or_else(|| anyhow!("Failed to get block numbers from traces"))?; - let mut i = 0; - while i < block_numbers.len() - 1 { - if block_numbers[i] + 1 != block_numbers[i + 1] { + for window in block_numbers.windows(2) { + if window[0] + 1 != window[1] { log::error!( "[prover] block numbers are not continuous, got {} and {}", - block_numbers[i], - block_numbers[i + 1] + window[0], + window[1] ); bail!( "block numbers are not continuous, got {} and {}", - block_numbers[i], - block_numbers[i + 1] + window[0], + window[1] ) } - i += 1; }This refactoring:
- Ensures all block numbers are valid by collecting into an
Option<Vec<_>>and handling theNonecase.- Uses
windows(2)to iterate over pairs of block numbers without manual indexing.- Enhances code readability and reliability.
Likely invalid or redundant comment.
| DarwinV2Handler::new( | ||
| prover_type, | ||
| &config.low_version_circuit.params_path, | ||
| &config.low_version_circuit.assets_path, |
There was a problem hiding this comment.
💡 Codebase verification
Implementation for Sp1 task type is incomplete
The codebase shows several issues with the Sp1 task type implementation:
gen_sp1_chunk_proofis not implemented (containstodo!())ChunkProverstruct has commented out Sp1 prover field- The Sp1 prover initialization is commented out with TODO
This indicates that while the task type is defined, its actual implementation is missing or incomplete.
🔗 Analysis chain
Verify task type handling in both handlers.
Since this PR aims to support Sp1 tasks, let's verify the implementation of task type handling in both handlers.
Also applies to: 87-91
Let me gather more information about the implementation of task handlers, particularly focusing on the Sp1 chunk proof generation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify task type handling in both handlers
# Test 1: Search for TaskType handling in both handlers
echo "Searching for TaskType handling..."
rg -A 5 "match.*TaskType" prover/src/
# Test 2: Search for Sp1 specific handling
echo "Searching for Sp1 task handling..."
rg -A 5 "ChunkSp1" prover/src/
Length of output: 5091
Script:
#!/bin/bash
# Search for Sp1 chunk proof generation implementation
echo "Searching for Sp1 chunk proof generation..."
rg -A 10 "fn gen_sp1_chunk_proof" prover/src/
# Search for ChunkProver struct implementation
echo "Searching for ChunkProver implementation..."
rg -A 10 "struct ChunkProver" prover/src/
Length of output: 1968
| struct ChunkProver { | ||
| halo2_prover: Option<RefCell<Halo2Prover<'static>>>, | ||
| // TODO: enable_post_sp1_prover | ||
| // sp1_prover: Option<RefCell<Sp1Prover>>, | ||
| } |
There was a problem hiding this comment.
Consider removing commented code and addressing TODO
The ChunkProver struct contains commented-out code and a TODO comment regarding enabling sp1_prover. Leaving commented code can reduce code readability and may lead to confusion. Consider removing the commented lines or implementing the sp1_prover functionality if it's required.
Would you like assistance in implementing the sp1_prover field or opening a GitHub issue to track this task?
| handler.chunk_prover = Some(ChunkProver { | ||
| halo2_prover: Some(RefCell::new(Halo2Prover::from_params_and_assets( | ||
| params_map, assets_dir, | ||
| ))), | ||
| // TODO: enable_post_sp1_prover | ||
| // sp1_prover: None, | ||
| }); | ||
| } | ||
| ProverType::ChunkSp1 => { | ||
| handler.chunk_prover = Some(ChunkProver { | ||
| halo2_prover: None, | ||
| // TODO: enable_post_sp1_prover | ||
| // sp1_prover: Some(RefCell::new(Sp1Prover::new())), | ||
| }); | ||
| } | ||
| ProverType::ChunkAll => { | ||
| handler.chunk_prover = Some(ChunkProver { | ||
| halo2_prover: Some(RefCell::new(Halo2Prover::from_params_and_assets( | ||
| params_map, assets_dir, | ||
| ))), | ||
| // TODO: enable_post_sp1_prover | ||
| // sp1_prover: Some(RefCell::new(Sp1Prover::new())), | ||
| }); | ||
| } |
There was a problem hiding this comment.
Enable sp1_prover in ChunkProver initialization
In the initialization of ChunkProver for ProverType::ChunkSp1 and ProverType::ChunkAll, the sp1_prover field is commented out with associated TODO comments. To support these prover types fully, consider enabling and initializing sp1_prover where applicable.
Would you like assistance in implementing sp1_prover for these ProverType variants or opening a GitHub issue to track this task?
amoylan2
force-pushed
the
feat/upgrade5-prover
branch
from
df6f687
to
d25bca8
Compare
Purpose or design rationale of this PR
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
Release Notes
New Features
EuclidHandlerfor managing zero-knowledge proof tasks with support for new prover types:ChunkHalo2,ChunkSp1, andChunkAll.prover_eucliddependency to enhance functionality.Changes
darwinmodule and its associated handler.Bug Fixes
Documentation