-
Notifications
You must be signed in to change notification settings - Fork 133
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
Use a tree to compute the result commitment #89
Use a tree to compute the result commitment #89
Conversation
ff2b106
to
193c0fc
Compare
193c0fc
to
aeb9e69
Compare
…ved the snark download script; fixed bug in genMaciStateFromContract
ac2bd6b
to
10fca58
Compare
This PR should be OK to merge even though the latest test failed (as of 5pm today -https://circleci.com/workflow-run/c9a47f2f-c1a4-43be-9119-33fb3b25ce14). This is due to the low performance of the CircleCI VM, not a problem with the code. |
There is one issue which this PR does not address: the final invocation of |
In lieu of #91, this PR will also remove the requirement that the coordinator must publish the final tally on-chain. |
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.
contracts/sol/MACI.sol
Outdated
// Calculate and store a commitment to 2 ** voteOptionTreeDepth zeros, | ||
// and the salt of 0. | ||
currentResultsCommitment = hashLeftRight( | ||
computeEmptyRoot(_treeDepths.voteOptionTreeDepth, 0), |
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.
This should be emptyVoteOptionTreeRoot
?
computeEmptyRoot(_treeDepths.voteOptionTreeDepth, 0), | |
emptyVoteOptionTreeRoot, |
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.
Oops! Computing the root twice wastes gas. Thank you for spotting this. I'll push a commit to fix it.
Updated the spec :D |
Currently, during the tally process, we use MiMCSponge to hash all the results and a salt to create a result commitment:
commitment = hash([...results, salt])
When we switch to Poseidon hashes, however, we will need a different way. This is because the Poseidon EVM code supports up to 5 elements, while we need to support many more than 5 vote options. correction, 3 May 2020: this is incorrect; we can still safely use Poseidon for more than 5 elements
The solution presented in this PR is to generate the commitment by using a Merkle tree:
commitment = hashLeftRight(computeRoot(results), salt)
This means that we can simply use PoseidonT3 (to hash 2 elements) for the Merkle tree and for
hashLeftRight
, so we won't need to write new EVM code or a PoseidonSponge.edit, 3 May 2020: This also means that a client application can verifiably prove that they know the value of any leaf by providing a Merkle path from the leaf to the root.
Original QVT circuit: 121466 constraints
New QVT circuit: 141266 constraints
Original
proveVoteTallyBatch()
: 740772 gasNew
proveVoteTallyBatch()
: 740860 gasUsing
zkutil
, the difference in proof generation time is negligible.There is also a bugfix in
cli/ts/utils.ts
which makesgenMaciStateFromContract
correctly read the tree depths from the contract.Related to #88