fix(lean-imt): After a sequence of updates a proof is invalid. #355
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
0xDatapunk
reviewed
Dec 3, 2024
Hi 0xDatapunk. The poseidon method accepts bigint|number|string, if you pass an undefined parameter the process crashes with the error TypeError: Cannot convert undefined to a BigInt. |
cedoor
approved these changes
Dec 3, 2024
When a member is removed from the tree (update a leaf with 0n) and then another member is updated, the root that is saved is not correct, so the proof that is generated is not valid.This was caused by a type validation failure. This fix the behavior described above. I detected this when I was testing in Semphore groups.
due to a bug in type checking, the root is not generated correctly.
due to a bug in type checking, the root is not generated correctly.
|
@cedoor I had to rebase my commit because they weren't verified. I've fixed it now. |
cedoor
approved these changes
Dec 4, 2024
cedoor
approved these changes
Dec 4, 2024
|
Congrats, your important contribution to this open-source project has earned you a GitPOAP! GitPOAP: 2024 ZK-KIT Contributor: Head to gitpoap.io & connect your GitHub account to mint! Learn more about GitPOAPs here. |
7 tasks
github-merge-queue bot
pushed a commit
to proofcarryingdata/zupass
that referenced
this pull request
Dec 10, 2024
Just a cleanliness update while I check for changes in our various dependencies. Here's the story behind the updated comment, for posterity: I originally planned to use 0 as the hash value for PODNull, but decided against it because LeanIMT behaved badly. Turns out that behavior was a bug, fixed here: privacy-scaling-explorations/zk-kit#355 However, the test for that bug points out that people might assume that 0 has special meaning in many situations (including using it to mean removal from a Merkle tree), so it's still a good idea to use a unique non-zero value for PODNull's hash. In this PR I've updated the comment on the related utest to describe the general reasoning, rather than point to LeanIMT in particular.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
When a member is removed from the tree (update a leaf with 0n) and then another member is updated, the root that is saved is not correct, so the proof that is generated is not valid.This was caused by a type validation failure.
This fix the behavior described above.
I detected this when I was testing in Semphore groups.
Checklist
yarn stylewithout getting any errors