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( | ||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. 🧩 Analysis chain🏁 Script executed: find . -name "dumper.rs" -type f 2>/dev/null | head -20Repository: scroll-tech/scroll Length of output: 95 🏁 Script executed: cat -n ./crates/prover-bin/src/dumper.rs | head -60Repository: scroll-tech/scroll Length of output: 2381 🏁 Script executed: # Search for ProvingTask struct definition to understand field types
rg -A 20 "struct ProvingTask" --type rustRepository: scroll-tech/scroll Length of output: 1025 🏁 Script executed: # Find where ProvingTask is defined (it's imported from scroll_zkvm_types)
rg "pub struct ProvingTask" --type rust -A 15Repository: scroll-tech/scroll Length of output: 820 🏁 Script executed: # Find ProvingTask definition in scroll_zkvm_types crate
find . -name "*.rs" -type f -exec grep -l "pub struct ProvingTask" {} \; | head -5Repository: scroll-tech/scroll Length of output: 90 🏁 Script executed: # Check the import statement and look for where ProvingTask comes from
rg "use.*ProvingTask|from scroll_zkvm_types" crates/prover-bin/src/dumper.rsRepository: scroll-tech/scroll Length of output: 125 🏁 Script executed: # Find scroll_zkvm_types crate and its ProvingTask definition
find . -path "*/scroll_zkvm_types/*" -name "*.rs" -type f | head -10Repository: scroll-tech/scroll Length of output: 44 🏁 Script executed: # Check Cargo.toml for prover-bin to understand bincode configuration
cat crates/prover-bin/Cargo.tomlRepository: scroll-tech/scroll Length of output: 1152 🏁 Script executed: # Check if scroll_zkvm_types is a workspace member
rg "scroll.zkvm.types|scroll-zkvm-types" workspace.members -A 2 -B 2 2>/dev/null || cat Cargo.toml | grep -A 50 "\[workspace\]"Repository: scroll-tech/scroll Length of output: 1695 🏁 Script executed: # Search for deserialization or loading code for these binary files
rg "aggregated_proofs|serialized_witness" crates/ -B 2 -A 2 --type rust | head -60Repository: scroll-tech/scroll Length of output: 4156 🏁 Script executed: # Try to get struct definitions from the external git dependency documentation
# Let's check what types serialized_witness and proof.proofs are
rg "struct.*Proof|serialized_witness.*:|proofs.*:" crates/libzkp/src/ --type rust -B 1 -A 3 | head -80Repository: scroll-tech/scroll Length of output: 4761 🏁 Script executed: # Look for deserialization code that reads these files back
rg "decode_from_std_read|decode_from" crates/ --type rust -B 2 -A 2 | head -60Repository: scroll-tech/scroll Length of output: 44 🏁 Script executed: # Search for any code that reads input_task.bin or agg_proofs.bin files
rg "input_task\.bin|agg_proofs\.bin" crates/ --type rust -B 3 -A 3Repository: scroll-tech/scroll Length of output: 1346 Clarify the bincode API usage and fix the loop encoding pattern. Line 26 uses However, lines 35-41 encode each proof individually in a loop, concatenating multiple bincode values to 🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||
| &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(), | ||||||||||||||||||||||||||||||
| )?; | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Loop encodes individual proofs, not the aggregated collection. The code encodes each The comment "stream-encode aggregated_proofs" suggests encoding the entire collection once. Encoding individual items without length prefixes or delimiters will likely cause deserialization failures. 🔎 Consider encoding the entire collection at once: // 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(),
- )?;
-}
+bincode::serde::encode_into_std_write(
+ &task.aggregated_proofs,
+ &mut agg_writer,
+ bincode::config::standard(),
+)?;Alternatively, if individual encoding is required, prefix each entry with its length or use a proper container format. 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| 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() | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
noel2004 marked this conversation as resolved.
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| 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() | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
noel2004 marked this conversation as resolved.
|
||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| 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