-
Notifications
You must be signed in to change notification settings - Fork 856
Implement RootCircuit
for SuperCircuit
aggregation
#1000
Implement RootCircuit
for SuperCircuit
aggregation
#1000
Conversation
@lispc can you help us get another reviewer for this task. Thanks! |
eb76f8f
to
b0fd687
Compare
Does the evm-verifier-with-accumulator in that snark-verifier repo work for this root circuit?? Thanks! |
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 work is awesome @han0110
It's quite clear what you're doing all the time. I really like it!
I left some questions and minor nits.
Also, are there any specs for the Root circuit somewhere? As I think we should have some. And also, to have them related to the code we see here.
Again, awesome work mate!
use itertools::Itertools; | ||
use rand::rngs::OsRng; | ||
|
||
#[ignore = "Due to high memory requirement"] |
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.
Would this be a candidate to run in the serial
tests section of our testsuite? Or even with this the machine would OOM?
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.
Since it's trying to create a SuperCircuit
proof with k >= 18
, so I believe it requires a tons of memory to run, so I didn't bother to try enable this to test the CI machine.
@smtmfft Yes I think so. |
@smtmfft Sorry that I haven't added too much document yet, but I'm planning to do that soon, the paper I followed is this one https://eprint.iacr.org/2020/499. Simply speaking it's trying to verify it without doing pairing, and defer the final pairing input to the evm verifier. |
Note: Han is working on documentation tasks first that will help to review this one. |
Note: after the document is finished, this review can continue. privacy-scaling-explorations/snark-verifier#22 |
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.
Sorry for the delay on the review @han0110 .
Awesome job. I'll anyway dive deeper into the paper implementation inside snark-verifier
once I get a bit more time.
Thanks for this awesome work! 👏
b0fd687
to
1435a30
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.
Amazing work! LGTM. I added a comment.
let a = region.assign_advice(|| "", w_l, 0, || Value::known(self.0))?; | ||
region.assign_fixed(|| "", q_l, 0, || Value::known(-F::one()))?; | ||
a.copy_advice(|| "", &mut region, w_r, 1)?; | ||
a.copy_advice(|| "", &mut region, w_o, 2)?; | ||
region.assign_advice(|| "", w_l, 3, || Value::known(-F::from(5)))?; | ||
for (column, idx) in [q_l, q_r, q_o, q_m, q_c].iter().zip(1..) { | ||
region.assign_fixed(|| "", *column, 3, || Value::known(F::from(idx)))?; | ||
} |
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 don't follow the cell assignments here. Can we add a comment on why we are assigning this way?
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.
It's because currently there is some problem for halo2wrong
if there is identity point in preprocessed or proof (kind of forget to investigate for a long time), so here it's trying to make sure the fixed columns are not identity by assigning some random values.
I have added some comments and error handling in 54935c5, and will try to dig into that issue.
…roof, and comments why we need to assign some random values in testing circuit `StandardPlonk`
c0ba028
to
54935c5
Compare
* create_op: fix memory size (privacy-scaling-explorations#990) * create: refactor lifetime * fmt * add more tests * update access list in create * refactor create * build * fix * try * ci
This PR aims to implement
RootCircuit
forSuperCircuit
aggregation, to replace current one inzkevm-chain
repo.It uses the latest
snark-verifier
(previouslyplonk-verifier
) so it requires to upgrade thehalo2
andhalo2wrong
tag, tho it doesn't require any change (it's just that we could use stable channel rust now butzkevm-circuits
itself still requires nightly).Resolves #664.