Skip to content
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

Non Membership Proofs #76

Merged
merged 9 commits into from Feb 2, 2023
Merged

Non Membership Proofs #76

merged 9 commits into from Feb 2, 2023

Conversation

BlakeMScurr
Copy link
Collaborator

This adds a circuit for making zk proofs of non membership in a merkle tree.
It also provides a TS class for preprocessing the tree to easily get the relevant data.
The tests show that our circuits accept some arbitrary valid non membership proof.

This stackexchange post explains the algorithm well.

@coveralls
Copy link
Collaborator

coveralls commented Jan 10, 2023

Coverage Status

Coverage: 88.525%. Remained the same when pulling f311f3f on non-membership into fbf5707 on main.

@sripwoud sripwoud added the enhancement New feature or request label Jan 10, 2023
@sripwoud sripwoud linked an issue Jan 10, 2023 that may be closed by this pull request
Copy link
Collaborator

@sripwoud sripwoud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK
@wanseob Could you help finding an additional reviewer to give feedbacks to Blake and approve this circuit PR?

@sripwoud sripwoud requested a review from wanseob January 10, 2023 11:53
@sripwoud sripwoud force-pushed the non-membership branch 2 times, most recently from bcc06e2 to f311f3f Compare February 1, 2023 10:34
@sripwoud
Copy link
Collaborator

sripwoud commented Feb 1, 2023

@BlakeMScurr
Please the resolve conflict
I tried to do it myself because the conflict as shown in the web editor is trivial.
But it seems to mess with your previous PR about pub key check afterwards.

error[T2008]: Duplicated callable symbol
 ... 
ECDSACheckPubKey is already in use

@@ -2,6 +2,8 @@ pragma circom 2.0.2;

include "node_modules/circom-ecdsa/circuits/zk-identity/eth.circom";
include "node_modules/circom-ecdsa/circuits/ecdsa.circom";
include "node_modules/circom-ecdsa/node_modules/circomlib/circuits/bitify.circom";
include "node_modules/circom-ecdsa/node_modules/circomlib/circuits/comparators.circom"; // TODO: fix insane dependency twiddling
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe comparators already include bitify. That's why you are getting duplicate dependency error.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔
I see that comparators does include bitify...
It doesn't cause an error locally though.
So I am not sure...

Copy link
Collaborator

@socathie socathie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Circuit looks good to me. Just a question on the practical side though, how do we actually obtain the two adjacent addresses in raw form for comparison in a non-inclusion proof? Does that mean we have a server that actually stores these addresses in raw (instead of hashed) form?

@BlakeMScurr
Copy link
Collaborator Author

Hey @socathie, yes the idea is that someone would be storing this in full unhashed form. The main use case for a non-membership proof that we could think of is a blacklist for a mixer (like tornado cash). In that case some kind of censor would publish a list of disallowed addresses and freely share them.

Now that I think about it, maybe there needs to be a mechanism to make sure they're sharing the full tree, but that would probably be a smart contract where they can manually add addresses to a merkle tree. But either way, for the purposes of withdrawing, you can assume that the blacklist would be published somewhere.

@socathie
Copy link
Collaborator

socathie commented Feb 1, 2023

I see. It makes a lot of sense now!

@BlakeMScurr
Copy link
Collaborator Author

@r1oga I had the same issue with ECDSACheckPubKey being duplicated, but I actually just fixed it by rerunning pnpm install. The issue was the this non-membership branch was using my fork of circom-ecdsa (back when we thought I could add the public key check into the depdency), whereas the main branch was not using a fork, and we had ECDSACheckPubKey in our repo instead. So these tests should pass.

@BlakeMScurr
Copy link
Collaborator Author

@r1oga So we have a timeout issue here. We have so many tests that there we seem to be hitting a ~30 minute limit on the github action. I have set the internal jest timeout to over 2 hours (lmao), so it's not that. It takes ~50 minutes for me to run the whole test suite locally, and it all passes:
ECDSACheckPubKey: 842s
Inclusion test: 1017s
Exclusion test: 1012s
Poseidon tests: 5s

@sripwoud
Copy link
Collaborator

sripwoud commented Feb 2, 2023

@BlakeMScurr Github Action jobs can execute for up to 6h so the test should be able to complete within that time.
These are the specs for default runners.
Paid plans have larger runners.
Or later we could config GitHub actions to run on a self hosted runner.

For now let's ignore this and merge. I can confirm I could run the test successfully locally too.

@sripwoud sripwoud merged commit f3cff14 into main Feb 2, 2023
@sripwoud sripwoud deleted the non-membership branch February 2, 2023 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Non-Membership Circuit
4 participants