-
Notifications
You must be signed in to change notification settings - Fork 630
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 16 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,74 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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)] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| target_path: String, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| impl Dumper { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fn dump(&self, input_string: &str) -> eyre::Result<()> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let task: ProvingTaskExt = serde_json::from_str(input_string)?; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let task = ProvingTask::from(task); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // 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::serde::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); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| bincode::serde::encode_into_std_write( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| &task.aggregated_proofs, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| &mut agg_writer, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| bincode::config::standard(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| )?; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+66
to
+78
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. 🧩 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: # Search for TaskStatus enum definition
rg "enum TaskStatus" -A 10Repository: scroll-tech/scroll Length of output: 44 🏁 Script executed: # Search for how prove() is used
rg "\.prove\(" -B 2 -A 2Repository: scroll-tech/scroll Length of output: 44 🏁 Script executed: # Look for any comments or documentation about Dumper's purpose
rg -i "dumper" -B 2 -A 2 | head -100Repository: scroll-tech/scroll Length of output: 1372 🏁 Script executed: # Search for TaskStatus definition in different ways
rg "TaskStatus" -B 2 -A 5 | head -150Repository: scroll-tech/scroll Length of output: 7859 🏁 Script executed: # Look for tests related to dumper
fd "test" -path "*dumper*" -o -name "*test*" | grep -i dumpRepository: scroll-tech/scroll Length of output: 230 🏁 Script executed: # Search for TaskStatus in proving_sdk or prover modules
fd "proving_service" -o -name "*.rs" | xargs rg "enum TaskStatus\|pub enum TaskStatus" -A 10Repository: scroll-tech/scroll Length of output: 294 🏁 Script executed: # Search for TaskStatus enum definition in scroll_proving_sdk
find . -name "*.rs" -type f | xargs rg "pub enum TaskStatus" -A 10Repository: scroll-tech/scroll Length of output: 46 🏁 Script executed: # Also check if there's documentation about Dumper's purpose
find . -name "*.md" -o -name "README*" | xargs grep -l -i "dump" 2>/dev/null | head -5Repository: scroll-tech/scroll Length of output: 44 🏁 Script executed: # Search for TaskStatus definition more broadly
rg "enum TaskStatus|struct TaskStatus" -A 15Repository: scroll-tech/scroll Length of output: 44 🏁 Script executed: # Check if scroll_proving_sdk is in the repo or external
find . -name "scroll_proving_sdk" -type dRepository: scroll-tech/scroll Length of output: 44 🏁 Script executed: # Look at Cargo.toml to understand dependencies
cat Cargo.toml 2>/dev/null || find . -name "Cargo.toml" -path "*/prover-bin/*" -exec cat {} \;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: # Search for any local definition of TaskStatus or proving_service
find . -name "*.rs" -type f -exec grep -l "proving_service\|ProvingService" {} \; | head -20Repository: scroll-tech/scroll Length of output: 129 Status should reflect dump success or failure, not always return Failed. The 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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| async fn query_task(&mut self, _: QueryTaskRequest) -> QueryTaskResponse { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| unreachable!("for one_shot routine, we should be returned in prove call"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| BEGIN_BLOCK?=26653680 | ||
| END_BLOCK?=26653686 | ||
| SCROLL_FORK_NAME=galileo |
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.
Unused
target_pathfield while output paths are hardcoded.The
target_pathfield is marked as dead code and never used. However, thedump()method writes to hardcoded file paths"input_task.bin"and"agg_proofs.bin"in the current working directory (lines 24 and 33). This appears to be an incomplete implementation.Consider using
target_pathto control the output directory:🔎 Suggested fix to use `target_path` for output files:
impl Dumper { fn dump(&self, input_string: &str) -> eyre::Result<()> { let task: ProvingTaskExt = serde_json::from_str(input_string)?; let task = ProvingTask::from(task); // stream-encode serialized_witness to input_task.bin using bincode 2.0 - let input_file = std::fs::File::create("input_task.bin")?; + let input_path = std::path::Path::new(&self.target_path).join("input_task.bin"); + let input_file = std::fs::File::create(input_path)?; let mut input_writer = std::io::BufWriter::new(input_file); bincode::serde::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 agg_path = std::path::Path::new(&self.target_path).join("agg_proofs.bin"); + let agg_file = std::fs::File::create(agg_path)?; let mut agg_writer = std::io::BufWriter::new(agg_file); bincode::serde::encode_into_std_write( &task.aggregated_proofs, &mut agg_writer, bincode::config::standard(), )?; Ok(()) } }And remove the
#[allow(dead_code)]attribute:#[derive(Default)] pub struct Dumper { - #[allow(dead_code)] target_path: String, }📝 Committable suggestion