fix(merkle-tree): make sibling path generic over its length#647
Merged
Conversation
cleanup fix: missed occurences
LHerskind
approved these changes
May 23, 2023
LHerskind
left a comment
LHerskind
left a comment
Contributor
There was a problem hiding this comment.
Tiny nits. Looks good to me.
🧚
|
|
||
| // ensure the committed state is correct | ||
| const initialSiblingPath = new SiblingPath([initialLeafHash, level1ZeroHash, level2ZeroHash]); | ||
| const initialSiblingPath = new SiblingPath(3, [initialLeafHash, level1ZeroHash, level2ZeroHash]); |
Contributor
There was a problem hiding this comment.
Suggested change
| const initialSiblingPath = new SiblingPath(3, [initialLeafHash, level1ZeroHash, level2ZeroHash]); | |
| const initialSiblingPath = new SiblingPath(TEST_TREE_DEPTH, [initialLeafHash, level1ZeroHash, level2ZeroHash]); |
| height, | ||
| ), | ||
| ); | ||
| return new MembershipWitness(height, index, assertLength(path.toFieldArray(), height)); |
Contributor
There was a problem hiding this comment.
Do you know about this todo? Not fully clear to me what conversion it is related to.
Member
Author
There was a problem hiding this comment.
I dont think it applies anymore? The only number type here is the generic that will never be that big? @spalladino do you know if this is resolved?
Contributor
There was a problem hiding this comment.
Yep, it seems it doesn't apply anymore. IIRC there was an explicit Number(bigintVariable) here, but seems like it's gone (or moved elsewhere).
This was referenced Jul 25, 2023
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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
Try to prevent: #639
More broadly part of: #645
Issues occurred when migrating the Nullifier Tree to a new location when the subtree and full tree heights were mixed up, leading to hard to decipher circuit messages.
This pr aims to prevent this issue by strongly typing the nullifier tree batch insertion method.
MembershipWitnessfile in his msgpack work, SiblingPaths are now generic over their height and this height is checked.Buffer[]->Fr[](path.data.map(f => Fr.fromBuffer(f))yuck) have been replaced withSiblingPath.toFieldArray()Checklist: