-
Notifications
You must be signed in to change notification settings - Fork 627
Optimization for prover (override #1761) #1774
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from all commits
bdf7826
9839bf7
d262a63
dc29c8c
af63bc0
5c2803c
5ae31bc
98be0a0
b244fa8
e852915
b833468
930a12a
3b174f8
74a3d7a
ede29c7
b270d96
79d79ed
d306b38
03b992f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,88 @@ | ||
| use async_trait::async_trait; | ||
| use libzkp::ProvingTaskExt; | ||
| use scroll_proving_sdk::prover::{ | ||
| proving_service::{ | ||
| GetVkRequest, GetVkResponse, ProveRequest, ProveResponse, QueryTaskRequest, | ||
| QueryTaskResponse, TaskStatus, | ||
| }, | ||
| ProvingService, | ||
| }; | ||
| use scroll_zkvm_types::ProvingTask; | ||
|
|
||
| #[derive(Default)] | ||
| pub struct Dumper { | ||
| #[allow(dead_code)] | ||
| pub target_path: String, | ||
| pub json_mode: bool, | ||
| } | ||
|
|
||
| impl Dumper { | ||
| fn dump(&self, input_string: &str) -> eyre::Result<()> { | ||
| let task: ProvingTaskExt = serde_json::from_str(input_string)?; | ||
| let task = ProvingTask::from(task); | ||
|
|
||
| if self.json_mode { | ||
| let file = std::fs::File::create("input_task.json")?; | ||
| serde_json::to_writer(std::io::BufWriter::new(file), &task)?; | ||
| } else { | ||
| // stream-encode serialized_witness to input_task.bin using bincode 2.0 | ||
| let input_file = std::fs::File::create("input_task.bin")?; | ||
| let mut input_writer = std::io::BufWriter::new(input_file); | ||
| bincode::encode_into_std_write( | ||
| &task.serialized_witness, | ||
| &mut input_writer, | ||
| bincode::config::standard(), | ||
| )?; | ||
|
|
||
| // stream-encode aggregated_proofs to agg_proofs.bin using bincode 2.0 | ||
| let agg_file = std::fs::File::create("agg_proofs.bin")?; | ||
| let mut agg_writer = std::io::BufWriter::new(agg_file); | ||
| for proof in &task.aggregated_proofs { | ||
| let sz = bincode::serde::encode_into_std_write( | ||
| &proof.proofs, | ||
| &mut agg_writer, | ||
| bincode::config::standard(), | ||
| )?; | ||
| println!("written {sz} bytes for proof"); | ||
| } | ||
| } | ||
|
|
||
| Ok(()) | ||
| } | ||
| } | ||
|
|
||
| #[async_trait] | ||
| impl ProvingService for Dumper { | ||
| fn is_local(&self) -> bool { | ||
| true | ||
| } | ||
| async fn get_vks(&self, _: GetVkRequest) -> GetVkResponse { | ||
| // get vk has been deprecated in new prover with dynamic asset loading scheme | ||
| GetVkResponse { | ||
| vks: vec![], | ||
| error: None, | ||
| } | ||
| } | ||
| async fn prove(&mut self, req: ProveRequest) -> ProveResponse { | ||
| let error = if let Err(e) = self.dump(&req.input) { | ||
| Some(format!("failed to dump: {}", e)) | ||
| } else { | ||
| None | ||
| }; | ||
|
|
||
| ProveResponse { | ||
| status: TaskStatus::Failed, | ||
| error, | ||
| ..Default::default() | ||
| } | ||
| } | ||
|
|
||
| async fn query_task(&mut self, req: QueryTaskRequest) -> QueryTaskResponse { | ||
| QueryTaskResponse { | ||
| task_id: req.task_id, | ||
| status: TaskStatus::Failed, | ||
| error: Some("dump file finished but need a fail return to exit".to_string()), | ||
| ..Default::default() | ||
| } | ||
| } | ||
|
Comment on lines
+80
to
+87
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Misleading task status despite intentional exit behavior. The Consider separating concerns: track actual task status (Success/Failed based on dump result) and use a different mechanism (e.g., explicit process exit in the caller) to ensure termination. |
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the full dumper.rs file to understand the context cat crates/prover-bin/src/dumper.rsRepository: scroll-tech/scroll
Length of output: 2301
🏁 Script executed:
Repository: scroll-tech/scroll
Length of output: 44
🏁 Script executed:
Repository: scroll-tech/scroll
Length of output: 44
🏁 Script executed:
Repository: scroll-tech/scroll
Length of output: 1372
🏁 Script executed:
Repository: scroll-tech/scroll
Length of output: 7859
🏁 Script executed:
Repository: scroll-tech/scroll
Length of output: 230
🏁 Script executed:
Repository: scroll-tech/scroll
Length of output: 294
🏁 Script executed:
Repository: scroll-tech/scroll
Length of output: 46
🏁 Script executed:
Repository: scroll-tech/scroll
Length of output: 44
🏁 Script executed:
Repository: scroll-tech/scroll
Length of output: 44
🏁 Script executed:
Repository: scroll-tech/scroll
Length of output: 44
🏁 Script executed:
Repository: scroll-tech/scroll
Length of output: 2786
🏁 Script executed:
# Check prover-bin's Cargo.toml for dependencies cat crates/prover-bin/Cargo.tomlRepository: scroll-tech/scroll
Length of output: 1129
🏁 Script executed:
Repository: scroll-tech/scroll
Length of output: 129
Status should reflect dump success or failure, not always return Failed.
The
prove()method always setsstatus: TaskStatus::Failedregardless of whether the dump succeeds. WhenerrorisNone(successful dump), the status should beTaskStatus::Success. This causes all dump operations to be incorrectly reported as failed.Update the implementation to conditionally set the status:
async fn prove(&mut self, req: ProveRequest) -> ProveResponse { - let error = if let Err(e) = self.dump(&req.input) { - Some(format!("failed to dump: {}", e)) - } else { - None - }; + let (status, error) = if let Err(e) = self.dump(&req.input) { + (TaskStatus::Failed, Some(format!("failed to dump: {}", e))) + } else { + (TaskStatus::Success, None) + }; ProveResponse { - status: TaskStatus::Failed, + status, error, ..Default::default() } }📝 Committable suggestion
🤖 Prompt for AI Agents