Replace the ark-zkey witness calculator with the one of iden3 #273
Conversation
Benchmark for 2a22ee6Click to view benchmark
|
Benchmark for a194f3dClick to view benchmark
|
Benchmark for 2a22ee6Click to view benchmark
|
Benchmark for a194f3dClick to view benchmark
|
Benchmark for 2a22ee6Click to view benchmark
|
Benchmark for a194f3dClick to view benchmark
|
|
|
||
| let mut nodes = Vec::new(); | ||
| let nodes_num = br.read_u64::<LittleEndian>()?; | ||
| for _ in 0..nodes_num { |
There was a problem hiding this comment.
Probably a one-liner using collect would be more idiomatic
There was a problem hiding this comment.
Agree.
UPDATE: Impossible to collect because of "read_message(&mut br)?;" cannot be within the map closure due to "?". Let's leave this as is.
There was a problem hiding this comment.
Make sense.
Btw, do we know the estimate for the number of nodes? I mean that we know the size and it would be nice to allocate memory to the vector at once, instead of copying it every time we push it.
Like this:
let mut nodes = Vec::with_capacity(nodes_num as usize);
There was a problem hiding this comment.
Make sense.
Btw, do we know the estimate for the number of nodes? I mean that we know the size and it would be nice to allocate memory to the vector at once, instead of copying it every time we push it.
Like this:
let mut nodes = Vec::with_capacity(nodes_num as usize);
Agree. I will add this.
| if bytes_read == 0 { | ||
| return Err(std::io::Error::new( | ||
| std::io::ErrorKind::UnexpectedEof, | ||
| "Unexpected EOF", |
There was a problem hiding this comment.
Probably better to have these literals by name, reuse them and put them in a different file. Even better would be a more holistic approach to error handling, but that could be topic of another PR
There was a problem hiding this comment.
No need in this, because the iden3 code contains only 2 such literals, which are used only 3 times, and the original zerokit code uses the same approach to error handling.
There was a problem hiding this comment.
I still think that processing is necessary because, if you pay attention, all external functions that are called by rln have this handling and this one don't leave utils lib in this form
Moreover, as far as I can see you use unwrap in all calls, which is not very good either:
Line 24 in d8232a5
| if bytes_read != ln { | ||
| return Err(std::io::Error::new( | ||
| std::io::ErrorKind::UnexpectedEof, | ||
| "Unexpected EOF", |
|
|
||
| // Find all nodes with the same value. | ||
| let mut value_map = HashMap::new(); | ||
| for (i, &value) in values.iter().enumerate() { |
There was a problem hiding this comment.
All of these for cycles below are not very idiomatic
There was a problem hiding this comment.
The cycle with HashMap does not need modifications, see the second part of https://stackoverflow.com/a/30441736 . The only loop in this function to be eliminated is the one at the line 596.
| for (key, value) in input_list { | ||
| let (offset, len) = inputs_info[key]; | ||
| if len != value.len() { | ||
| panic!("Invalid input length for {}", key); |
There was a problem hiding this comment.
What is the justification for a panic?
There was a problem hiding this comment.
I agree, we need to get the results back, not panic
There was a problem hiding this comment.
Agree.
UPDATE: I have studied all 3 places, where panic! is used, and I think that these case are justified. It seems that these pieces of code panic in situations, which cannot happen with correctly written code and correctly generated execution graph regardless of the user's input. So the condition checks leading to these "panics" can be considered as some fundamental sanity checks.
The same is true regarding those unwraps that I have studied. By the way, the project contains 3 iden3 files with unwraps and 20 previous files with them. Thus, using these unwraps seems to be consistent with the code style.
In most cases the iden3 code uses returning Results, which is the evidence of their understanding the principles of using panic!.
Thus, there is no need in rewriting these panic! and unwrap fragments.
| )), | ||
| Node::Input(_) => Err(NodeConstErr::InputSignal), | ||
| Node::MontConstant(_) => { | ||
| panic!("MontConstant should not be used here") |
There was a problem hiding this comment.
What is the justification for a panic here?
There was a problem hiding this comment.
See #273 (comment)
rln/src/iden3calc/graph.rs
Outdated
| *output = renumber[*output].unwrap(); | ||
| } | ||
|
|
||
| eprintln!("Removed {removed} unused nodes"); |
There was a problem hiding this comment.
Maybe a good idea to think about a more holistic approach to logging
There was a problem hiding this comment.
I guess there is no need in these logging fragments, so I am going to remove them.
Thank you for reviewing! Almost all the fragments of the code you mentioned have been written by iden3 and can be found there: https://github.com/philsippl/circom-witness-rs . Thus, I think it is better to contact them regarding your remarks, and then the updated code from iden3 repository can be integrated. So I have responded to the comment regarding the part of code rewritten by me. |
There was a problem hiding this comment.
- Agree about errors handling - no panics and unwrap, use `thiserror as in all code
- Fix clippy error
- Return wasm file into resources - rln_wasm right now depend on this file especially in tests. And also check that you don't remove functionality around rln-wasm
| for (key, value) in input_list { | ||
| let (offset, len) = inputs_info[key]; | ||
| if len != value.len() { | ||
| panic!("Invalid input length for {}", key); |
There was a problem hiding this comment.
I agree, we need to get the results back, not panic
| .collect(); | ||
|
|
||
| let (nodes, signals, input_mapping): (Vec<Node>, Vec<usize>, InputSignalsInfo) = | ||
| deserialize_witnesscalc_graph(std::io::Cursor::new(graph_data)).unwrap(); |
There was a problem hiding this comment.
Add error return, not unwrap
There was a problem hiding this comment.
Agree.
See #273 (comment)
As for me, all remarks are on point and it's worth fixing them because they affect the general style of the code. Just because the code is taken from another source doesn't mean it can't be changed, especially since it's taken without pulling dependencies (by the way, why not?). |
Benchmark for 3aa142cClick to view benchmark
|
Benchmark for a59fa6dClick to view benchmark
|
Benchmark for 3aa142cClick to view benchmark
|
Benchmark for a59fa6dClick to view benchmark
|
Benchmark for 3aa142cClick to view benchmark
|
If we are copying any code as is from other repositories, I would suppose it is mandatory to put doc that specifies it, that would include links to files we are copying. There were issues in such occasions |
Benchmark for a59fa6dClick to view benchmark
|
|
Thanks for participating in the first round of reviewing and welcome to the second! |
Thanks, Ekaterina and I support your idea. The corresponding changes have been included into the latest commit. |
Benchmark for 4f9d8d4Click to view benchmark
|
Benchmark for 4f9d8d4Click to view benchmark
|
Benchmark for 3f36961Click to view benchmark
|
Benchmark for 4f9d8d4Click to view benchmark
|
Benchmark for 3f36961Click to view benchmark
|
Benchmark for 3f36961Click to view benchmark
|
Motivation
See this comment.
Methods
Use circom-witness-rs to create the execution graph file for the rln circuit used in zerokit. Add this file to the zerokit resources.
Remove the execution graph generator code from circom-witness-rs and simplify its architecture to facilitate integration of its witness calculating code into zerokit.
Rewrite the corresponding parts of the cut version of circom-witness-rs to make its input and output data format consistent with zerokit.
Integrate the modified cut version of circom-witness-rs into zerokit, remove the code expressing the involvement of the ark-zkey witness calculator and WASM binary file used by it, involve the integrated code into the witness calculation and remove the redundant dependencies.
Modify the code related to testing and measurements to facilitate studying the performance of the witness calculation for the release compilation mode.
Results