Ruanpc/production #81
Conversation
| } | ||
|
|
||
| None => { | ||
| println!("============Validation started============"); |
There was a problem hiding this comment.
Rename validation to verification.
There was a problem hiding this comment.
This is part of requirement from production team to make the output as identical to v1
| let equity_offset = RecursiveTargets::<RECURSION_BRANCHOUT_NUM>::pub_input_equity_offset(); | ||
| let equity_sum = root_proof.proof.public_inputs[equity_offset].to_canonical_u64(); | ||
|
|
||
| let debt_offset = RecursiveTargets::<RECURSION_BRANCHOUT_NUM>::pub_input_debt_offset(); |
There was a problem hiding this comment.
offsets should be constants? It seems a bit weird logically to get offsets from targets.
There was a problem hiding this comment.
offsets is determined by targets. So it is better to put offset close to targets.
crates/zk-por-cli/src/verifier.rs
Outdated
| let invalid_proof_paths = user_proof_paths | ||
| .chunks(chunk_size) | ||
| .map(|chunks| { | ||
| let invalid_proof_paths = chunks | ||
| .par_iter() | ||
| .map(|user_proof_path| { | ||
| let merkle_path = File::open(&user_proof_path).unwrap(); | ||
| let reader = std::io::BufReader::new(merkle_path); | ||
| let proof: MerkleProof = from_reader(reader).unwrap(); | ||
| let result = proof.verify_merkle_proof(root_hash); | ||
| (result, user_proof_path) | ||
| }) | ||
| .filter(|(result, _)| result.is_err()) | ||
| .map(|(_, invalid_proof_path)| invalid_proof_path.to_str().unwrap().to_owned()) | ||
| .collect::<Vec<String>>(); | ||
| if verbose { | ||
| bar.inc(chunks.len() as u64); | ||
| } |
There was a problem hiding this comment.
You dont need to manually select chunk size to be num cpus, the way the rayon threading works, it will automatically schedule num_cpus number of threads and will spin up a new thread every time one completes. Unless you specify the number of threads in a threadpool rayon will use num cpu threads at a time.
There was a problem hiding this comment.
cliff determines the chunk size as the number of cpus. So I think I will better keep it unchanged.
There was a problem hiding this comment.
Yeah but this version is more inefficient and perhaps cliff is unaware about this feature of rayon. Its a pretty trivial change.
Btw I didnt notice that this was how we were doing parallelism in other sections. There is no need to concern ourselves with thread nums, especially since rayons threading is optimised for exactly this kind of task (work stealing greedy scheduler). We should change it because it is less efficient that simply using rayons own scheduler.
There was a problem hiding this comment.
There was a problem hiding this comment.
just push a commit to remove chunking as you suggested. Pls approve this pr.
No description provided.