Implemented nullifier tree wrapper over Smt
#275
Conversation
There was a problem hiding this comment.
Looks good! Thank you!
I've also tagged @hackaugusto to do another review.
store/src/nullifier_tree.rs
Outdated
| Err(NullifierTreeError::NullifierAlreadyExists { | ||
| nullifier: *nullifier, | ||
| block_num: Self::leaf_value_to_block_num(value), | ||
| }) |
There was a problem hiding this comment.
This error is misleading. It seems to imply the insert failed, because the nullifier already exists. But the existing nullifier is being replaced, since insert behaves like an insert_or_replace.
What was the intention here? To me, it seems that if we want the same behaviour, we should not have this error. OR, if we want to Err, then we have to do a check before the insert.
There was a problem hiding this comment.
Oh! Great catch! Yes, definitely we should do a check and only then do the insert. Basically, if the error is returned, the state of the nullifier tree should not change.
store/src/nullifier_tree.rs
Outdated
| let value = self.0.get_value(&nullifier.inner()); | ||
| if value == EMPTY_WORD { | ||
| return None; | ||
| } |
There was a problem hiding this comment.
This feels like a design flaw in SparseMerkleTree. We are required to encode the lack of a value as an EMPTY_WORD, but that implementation detail is leaking, and now on higher level abstractions we are having to special case it.
There was a problem hiding this comment.
Building on 0xPolygonMiden/crypto#289 (comment), the correct check here would be if value == Smt::EMPTY_VALUE, since Smt::EMPTY_VALUE is part of the public API, but the fact that Smt::EMPTY_VALUE == EMPTY_WORD is an implementation detail.
There was a problem hiding this comment.
Yes, let's change this to if value == Smt::EMPTY_VALUE.
Resolves: #269