Adds sparse merkle tree in noname stdlib
#249
Conversation
|
Hey @katat can you take a look? |
|
Thanks! Will take a look soon. |
|
hey it seems the checks failed due to enforce formating |
There was a problem hiding this comment.
Nice work!
I just had a round of review, and will need another rounds. I left some comments.
I think some of the arithmetic logic can be simplified using ifelse block.
More documentation on the arithmetic logic in the compute_roots and next_state, if they can't be simplified ifelse, would be useful.
| /// Field: mimc7 hash for key = 1 and values = [key,value] | ||
| fn compute_leaf_hash(key: Field, value: Field) -> Field { | ||
| let hash_values = [key, value]; | ||
| return mimc::mimc7_hash(hash_values, 1); |
There was a problem hiding this comment.
is it correct to provide 1 as the secret key?
There was a problem hiding this comment.
Yeah this was in the iden3 specification I think this is mainly for domain seperation of the internal and leaf hash functions other than that I cannot find a reason why
| /// `is_update`: whether the operation is an update operation | ||
| /// # Returns | ||
| /// `[Bool;6]`: the next state | ||
| fn next_state( |
There was a problem hiding this comment.
it'd better to have more documentation on the logic for each transition.
There was a problem hiding this comment.
Got it. It was written at the top but I will repeat it below
| let new_key_bits = bits::to_bits(LEN , new_key); | ||
|
|
||
| let mut level_inserted = old_level_inserted(enabled,siblings); | ||
| let mut states = [[false;6]; LEN]; |
There was a problem hiding this comment.
would it be easier if it is a field value to represent the different states intead of using an array of booleans?
There was a problem hiding this comment.
yeah but that makes the states computation a bit more difficult as in this case I can directly do boolean operations as my state transitions are dictated by booleans.
Also the assertion that there is only one state at a time
I tried the single state variable earlier but it got too difficult but when we do #248 it will be way easier maybe we can refactor then?
One more point is that circomlib also uses 6 variables for some reason maybe it has less constraints this way?
This pr adds the
sparse merkle treetostdliband closes #223The testing was done against the output of
circomlibjsyou can find the script here.The approach taken is almost identical to the circom's implementation mentioned in the issue.