-
Notifications
You must be signed in to change notification settings - Fork 231
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Semaphore V4 #480
Semaphore V4 #480
Conversation
c0aae37
to
c2f12d6
Compare
Semaphore V4
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
Issues regarding the Merkle Tree circuit:
|
What happens exactly? Could you add a test for it, or share the code you used here?
Good catch! I feel like this should be done here: https://github.com/privacy-scaling-explorations/zk-kit/blob/main/packages/circuits/circom/binary-merkle-root.circom. What do you think? |
@cedoor Yes, sure, I will create an example of test for it. Some changes should be in the zk-kit package and others in the semaphore proof package. For 2, yes, all the changes are in zk-kit. |
Actually, this issue is related to variable naming in the Merkle Tree circuit because the name depth works for the IMT but not for the LeanIMT. A test case could be to use the leaves:
And generate the merkle proof of the position 4. Replace this part (https://github.com/privacy-scaling-explorations/zk-kit/blob/main/packages/circuits/tests/binary-merkle-root.test.ts#L12-L20) with this: const leaf = BigInt(5)
for (let i = 1; i < 6; i += 1) {
tree.insert(BigInt(i))
}
const { siblings, index } = tree.generateProof(4) With that, the test will not pass. To pass the test, we can add const indices: number[] = []
const siblingsLength = siblings.length
for (let i = 0; i < MAX_DEPTH; i += 1) {
indices.push((index >> i) & 1)
if (siblings[i] === undefined) {
siblings[i] = BigInt(0)
}
} And update the input: const INPUT = {
leaf,
depth: siblingsLength,
indices,
siblings
} To solve this issue, I think that it is better to change the name of the variable depth here and use another to refer to the siblings' length. |
The LeanIMT works a bit differently compared to the old IMT. The Merkle proof could contain a different index and number of siblings due to the tree properties. I'm not sure if the variable name should be something different than The
It could be different when the leaf is on the right side of the tree. Proposal: https://github.com/privacy-scaling-explorations/zk-kit/blob/main/packages/circuits/circom/binary-merkle-root.circom could still use depth, as it's not necessary related to the LeanIMT. Semaphore could use other names, like |
406bbc4
to
0c596d8
Compare
Regarding this: I like this proposal 👍. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Outstanding job 🥇 - Thank you @cedoor.
Left some comments and minor nits around.
28727f9
to
3c7ce0e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! ✨
5d9ed8a
to
aa1bca6
Compare
More documentation on Identity lib code
Co-authored-by: Vivian Plasencia <v.pcalana@gmail.com>
Co-authored-by: Vivian Plasencia <v.pcalana@gmail.com>
More documentation on Circom code
More documentation on proof lib code
The commit adds more documentation to the group package functions and updates the TypeDoc link. re #483
Upgrade Yarn to v4
…mpiling contracts
Add code comments to contracts
More documentation on group lib code
Correct TypeDoc links
…n-version New OpenZeppelin version
Description
This PR updates the Semaphore codebase based on the findings from the analysis and research period described in the research repository.
The only supported tree depth is 10 at the moment. Another PR (based on issue #482) will be created to support a tree depth range from 1 to 32.
This is not the final version of the PR. The tasks that will follow are listed below. However, an initial review is necessary before proceeding.
ZK circuits
@zk-kit/circuits
-BinaryMerkleRoot
) and supports dynamic depths.@semaphore-protocol/circuit
)JS libraries
wasm
/zkey
artifacts on browsers and also on NodeJS.src
folder. A newtests
folder has been created for them.@semaphore-protocol/heyauthn
tests have been updated to support the new core libraries@semaphore-protocol/hardhat
has been updated to support the new contractsSolidity contracts
SemaphoreGroups.sol
has been adapted to the new data structure, which does not require zero nodes and has a static depth.SemaphoreGroups.sol
now also manages access to functions (group admins).VerifiedProof
event now includes all the parameters of the proof, allowing anyone to verify the proof off-chain (Add privateproof
values to theVerifiedProof
event #328).Related Issue
Closes #357
Does this introduce a breaking change?
Other information
Next tasks
Before merging this PR to the
main
branch it is necessary to work on other tasks, which will be merged to this branch first.merkleTreeSize
instead fomerkleTreeDepth
#530ISemaphore
typo for custom error #533@semaphore-protocol/data
according to Semphore V4 changes #544