Skip to content

Conversation

@han0110
Copy link

@han0110 han0110 commented Jul 8, 2022

This PR aims to pick all previous improvements base on abstraction, to make further integration easier. Each improvement is squashed into a single commit for easier rebasing in the future, and some are slightly modified to make as less changes as possible, they includes (in order):

With 4 more changes I made/picked:

  • Use same nightly rust version as privacy-scaling-explorations/zkevm-circuits
  • In MockProver::verify_*, cache the table if its identifier is same to previous one (which in most case several lookup to same table will be allocated adjacently), and check the cell indexes first in permutation check.
  • MockProver::verify_at_rows_par (modified) by @spherel
  • Expose necessary immutable reference fields for aggregation circuit usage.

@han0110 han0110 force-pushed the feature/abstraction-squashed branch 2 times, most recently from 6b5bd07 to 2d45008 Compare July 8, 2022 11:04
@han0110 han0110 force-pushed the feature/abstraction-squashed branch 2 times, most recently from 8d60da2 to 70c11c1 Compare July 10, 2022 05:41
@han0110 han0110 force-pushed the feature/abstraction-squashed branch from 70c11c1 to d88de9a Compare July 18, 2022 17:19
Copy link

@therealyingtong therealyingtong left a comment

Choose a reason for hiding this comment

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

LGTM.

I only reviewed the final three commits; I did not review the Abstraction PR, or the already-merged PRs.

/// the reasons that the circuit is not satisfied.
/// Constraints are only checked at `gate_row_ids`,
/// and lookup inputs are only checked at `lookup_input_row_ids`, parallelly.
pub fn verify_at_rows_par<I: Clone + Iterator<Item = usize>>(

Choose a reason for hiding this comment

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

Directly editing iter() -> par_iter() in verify_at_rows would make for a smaller diff. But I suppose that might cause conflicts in future rebases.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, was trying to just add things and avoid editing things for easier rebasing.

@pinkiebell
Copy link

Can we maybe include: #86 ?
Some not yet merged features in the prover crate depend on that and that's still a blocker for merging more recent changes.

@han0110 han0110 force-pushed the feature/abstraction-squashed branch 2 times, most recently from be78085 to 801135d Compare August 9, 2022 06:50
Copy link

@davidnevadoc davidnevadoc left a comment

Choose a reason for hiding this comment

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

Great work!
I went through all the changes paying special attention to the commits that are not part of a reviewed and approved PR. Overall everything LGTM :)
The only comment I'd make is that we maybe should parallelize compute_inner_product and include that change as part of this PR.

@han0110 han0110 force-pushed the feature/abstraction-squashed branch from 801135d to 470831c Compare August 12, 2022 16:37
@han0110
Copy link
Author

han0110 commented Aug 12, 2022

Can we maybe include: #86 ?
Some not yet merged features in the prover crate depend on that and that's still a blocker for merging more recent changes.

@pinkiebell In trait Param it provides read access to k and n, do you think it's sufficient? I also had an impression that we might need to truncate SRS for smaller circuit, how does that handled currently?

EDIT: I just add another method Param::downsize for the above use case.

@han0110 han0110 force-pushed the feature/abstraction-squashed branch from 470831c to 558a89c Compare August 12, 2022 16:43
@han0110
Copy link
Author

han0110 commented Aug 12, 2022

The only comment I'd make is that we maybe should parallelize compute_inner_product and include that change as part of this PR.

@davidnevadoc I just realized that compute_inner_product is only used for IPA but currently we haven't needed it yet, is that possible that we spend time implementing this when we are going to use it? Since I'm not sure how to pick the threshold to parallelize and might need to investigate some time to see.

@ashWhiteHat
Copy link

ashWhiteHat commented Aug 13, 2022

I just realized that compute_inner_product is only used for IPA but currently we haven't needed it yet, is that possible that we spend time implementing this when we are going to use it? Since I'm not sure how to pick the threshold to parallelize and might need to investigate some time to see.

I think it's unnecessary to introduce parallelize to compute_inner_product.
I introduced the parallelize before but it didn't affect so much and also other solution which affects more than parallelize.
zkcrypto/bls12_381#84

This PR is enough large so we should merge this and after that, improve more.

@han0110 han0110 force-pushed the feature/abstraction-squashed branch from 558a89c to 8926366 Compare August 13, 2022 09:46
@pinkiebell
Copy link

Can we maybe include: #86 ?
Some not yet merged features in the prover crate depend on that and that's still a blocker for merging more recent changes.

@pinkiebell In trait Param it provides read access to k and n, do you think it's sufficient?

Yes! That's all I needed :)

I also had an impression that we might need to truncate SRS for smaller circuit, how does that handled currently?

At the moment, I generated different keys for each K. I also tried a similar downsizing method by truncating, but keys generated with unsafe_setup didn't work after truncating - It basically broke commit_lagrange. Does it correctly work with the abstraction-squashed branch?

@han0110
Copy link
Author

han0110 commented Aug 13, 2022

also tried a similar downsizing method by truncating, but keys generated with unsafe_setup didn't work after truncating - It basically broke commit_lagrange.

Did you truncate the g_lagrange by g_lagrange.truncate(smaller_n) directly?

Does it correctly work with the abstraction-squashed branch?

Yes I think so, I have tried Params::downsize in tests/plonk_api and it works.

@pinkiebell
Copy link

also tried a similar downsizing method by truncating, but keys generated with unsafe_setup didn't work after truncating - It basically broke commit_lagrange.

Did you truncate the g_lagrange by g_lagrange.truncate(smaller_n) directly?

Yes. And after I took a look at unsafe_setup it became evident why that didn't work :). I defaulted to just work with different keys instead of changing halo2 too much, but good to know that the upcoming halo2 update ships that functionality.

@han0110 han0110 force-pushed the feature/abstraction-squashed branch 2 times, most recently from 50b8819 to ac9839e Compare August 15, 2022 07:53
Copy link

@CPerezz CPerezz left a comment

Choose a reason for hiding this comment

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

Awesome job mate! Thanks for working on this!

@han0110 han0110 force-pushed the feature/abstraction-squashed branch from ac9839e to ebfb648 Compare August 15, 2022 09:36
@han0110 han0110 mentioned this pull request Aug 15, 2022
@han0110 han0110 force-pushed the feature/abstraction-squashed branch from ebfb648 to 7724683 Compare August 15, 2022 17:20
This was referenced Aug 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants