-
Notifications
You must be signed in to change notification settings - Fork 20
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
Implement SolidityGenerator
#2
Implement SolidityGenerator
#2
Conversation
9440187
to
f8240eb
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.
Did a first review round with some questions. Still need to review half codegen
module & the tests
and transcript
files.
Hope to do this within the next days. This is quite a big PR so sorry if some questions are dumb.
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.
Didn't mean to approve yet. Fixing this.
ed33ced
to
d4d2ad0
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.
Thanks for the review and suggestion, commits to resolve the comment have been submitted, hope it's more clear now.
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.
I'll apporve the PR as I don't want to keep it stucked.
TBH, I;d need more time to be able to sit down and propperly review all the logic. In any case, this is not meant to be used yet. So we can make an audit any week later this year if we feel like deploying this.
I'd add a huge disclaimer mentioning that this should not be used in production as it hasn't been yet been audited nor properly reviewed.
Aside from that, this is an awesome work Han. Amazing effort! 👏 👏
Make senses, thanks for reminding! I've added that in c51fe30 and hope it's clear! |
Description
Currently it allows to generate verifier in 2 way:
k
and preprocessed commitments from verifying key so we could reuse verifier with different verifying key as long as theconfigure
is same (with one caveat, see the second bullet in Caveats).But it assumes:
Also the transcript behaves exactly same as the
EvmTranscript
insnark-verifier
.Benchmark
For
halo2wrong
main gate with range chip, the separated verifier contract has size only 38% of the output ofsnark-verifier
, and surprisingly, the gas cost is 10k lesssnark-verifier
even it has many for-loops.Caveats
--via-ir
is necessary when compiling the generated contract, otherwise it'd cause stack too deep. However,--via-ir
is not allowed to be used with--standard-json
, not sure how to workaround this.configure
is same, the selector compression might lead to different configuration when selector assignments are different. To avoid this we might need to update halo2 to support disabling selector compression.Acknowledgement
The template is heavily inspired by Aztec's verifier contract https://github.com/AztecProtocol/barretenberg/blob/master/sol/src/ultra/BaseUltraVerifier.sol.