Optimization for prover (override #1761) #1774
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,81 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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::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 { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| bincode::serde::encode_into_std_write( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| &proof.proofs, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| &mut agg_writer, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| bincode::config::standard(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| )?; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for proof in &task.aggregated_proofs { | |
| bincode::serde::encode_into_std_write( | |
| &proof.proofs, | |
| &mut agg_writer, | |
| bincode::config::standard(), | |
| )?; | |
| } | |
| for proof in &task.aggregated_proofs { | |
| bincode::serde::encode_into_std_write( | |
| &task.aggregated_proofs, | |
| &mut agg_writer, | |
| bincode::config::standard(), | |
| )?; | |
| } |
🤖 Prompt for AI Agents
In crates/prover-bin/src/dumper.rs around lines 35 to 41, the code currently
encodes each proof.proofs in a loop which concatenates multiple bincode blobs
into agg_proofs.bin and makes deserialization ambiguous; replace the loop by
encoding the entire aggregated_proofs collection once (i.e., serialize
task.aggregated_proofs as a single container) so a reader can decode the whole
collection in one call, or if you must stream individual entries, write a length
prefix before each encoded item (serialize the length then the item) so
deserializers can know where each entry ends.
There was a problem hiding this comment.
🧩 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 prove() method always sets status: TaskStatus::Failed regardless of whether the dump succeeds. When error is None (successful dump), the status should be TaskStatus::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
‼️ 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.
| 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 prove(&mut self, req: ProveRequest) -> ProveResponse { | |
| 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, | |
| error, | |
| ..Default::default() | |
| } | |
| } |
🤖 Prompt for AI Agents
In crates/prover-bin/src/dumper.rs around lines 57 to 69, the prove() method
always returns TaskStatus::Failed even when dump succeeds; change the status to
be conditional on the dump result: call self.dump(&req.input) and if it returns
Err set error to Some(...) and status to TaskStatus::Failed, otherwise set error
to None and status to TaskStatus::Success, then construct ProveResponse using
that conditional status and error (keep the rest of the default fields).
There was a problem hiding this comment.
Misleading task status despite intentional exit behavior.
The query_task() method returns TaskStatus::Failed with message "dump file finished but need a fail return to exit" for all queries. While this appears intentional to force process termination (per PR objectives about ensuring exit), it violates expected API semantics—the status should reflect the actual task outcome, not be used as a process control mechanism.
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.
| 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