[RELEASE-Proposal] PRs to merge before we cut a new release for halo2

[CPerezz]

Considering the amount of changes we currently have on HEAD + the upcomming PRs we should consider to cut a new release and integrate into zkevm-circuits.

This is the list of PRs I would merge before we cut the release:

• Commitment scheme abstraction #77 (Contains upstream rebase too. So comes with quite some changes).
• Optional zero-knowledge by const generic #76 We can basically get rid of the blinders.
• Negate the subtrahend when processing Expression::Sum #81 Fixes an expression evaluation issue.
• Reduce memory use in h evaluation #78
• fft gpu optimization #79 (If we can have gpu in our CI. Otherwise, this one can wait) cc: @AronisAt79 @ntampakas @barryWhiteHat

cc: @kilic @han0110

[kilic]

@CPerezz below there is a list of merged PRs that should be moved on top of abstraction.

It might be good to expect original authors to do so. Otherwise someone else for example @kilic could just port after waiting a short while. Would that be ok?

• [MockProver] Use a hash map to track cell assignment #71 @Brechtpd
• Remove bench regression workflow #60 @ntampakas
• Small changes for EVM circuit updates #54
• introduce gh-actions workflow unmerged changes from closed branch dum… #48 @AronisAt79
• Parallelized eval_polynomial/evaluate_vanishing_polynomial #47
• Poly optimizations #44
• test: add verify_at_rows in MockProver for improve test performance #38
• Halo2benchmarking #31
• dev: add name for lookup #29 @lispc
• lookup_any #8 @noctrlz

@Brechtpd what would you think moving some of polynomial and evaluation related improvements to the upstream?

[CPerezz]

@kilic I'm not sure I follow. All these PRs were merged into main. So I thought we were planning to keep merging into main the ones I listed above.

But it seems that you have a different plan. I guess that the Commitment Scheme abstraction is too hard to merge into main?

[kilic]

@CPerezz even without the abstraction stuff diff for main 70 commits ahead 198 commits behind against upstream. So this abstraction effort is like base for our usual manual rebase. I know it's horrible to manually rebase each time but it looked like most comfortable way to avoid huge rebase conflicts while synching with upstream.

[CPerezz]

@kilic no, it's fine. Sometimes there's no better way sadly..

Let's see if @Brechtpd @noctrlz @lispc @ntampakas @AronisAt79 and @han0110 can take some time to do this.

I can also help to migrate one or two of these if that helps.

[AronisAt79]

@CPerezz not sure if the above PR-lists contain everything that is associated to halo2 benchmarking via CI. But afaiu, the idea here is to port the complete CI (benchmarking) functionality of our halo2 main to abstraction branch. correct?

[Brechtpd]

@Brechtpd what would you think moving some of polynomial and evaluation related improvements to the upstream?

Sure. With upstream you mean zcash/halo2 and not the abstraction branch right?

[CPerezz]

@AronisAt79 I think so. But would be nice if @kilic could confirm

[kilic]

@CPerezz not sure if the above PR-lists contain everything that is associated to halo2 benchmarking via CI. But afaiu, the idea here is to port the complete CI (benchmarking) functionality of our halo2 main to abstraction branch. correct?

@AronisAt79 Yes and I think whole benchmarking work can be a single PR if you would also agree.

Sure. With upstream you mean zcash/halo2 and not the abstraction branch right?

@Brechtpd Yes I mean zcash/halo2 if you also think it make sense and it's not so much zkevm focused. However we can expect that PR in the upstream might take a while to get merged. So depending to the response it gets we might want to have it on abstraction first before it is accepted

[kilic]

Is there any zcash/halo2 PRs that we are exited about and expected to be merged soon? @CPerezz @therealyingtong @han0110

[CPerezz]

@kilic zcash#593 is essential for us to be able to forge keccak and other RLC-dependant circuits.

I think @han0110 has to address some review comments. But once merged we would like to have it ASAP IMO.

[han0110]

For challenge API I think I have addressed all addressable comments, and left some I don't have clear idea how to deal with. Not sure when it would be reviewed again, but if we want to use it right now I can port it to our fork, it should be really easy after #77 is merged because they are on the same base then.

[CPerezz]

For challenge API I think I have addressed all addressable comments, and left some I don't have clear idea how to deal with. Not sure when it would be reviewed again, but if we want to use it right now I can port it to our fork, it should be really easy after #77 is merged because they are on the same base then.

I'm not sure we need to rush it. What I meant in the call is that this PR should make it into the next release we cut. As otherwise, we will be constructing a lot more circuits without the challenge abstraction which is needed for the RLC almost everywhere across the zkevm-circuits.

Let's rebase all the other PRs listed and wait a week or so to see if your PR makes it into zcash's ~HEAD

[str4d]

Not sure when it would be reviewed again

Reviewing it now 🙂

[kilic]

1. How bad we want to include last 7 PRs from upstream?. I've tried to rebase but meet with some pesky conflicts. Should we park where abstraction is based or we want to fetch them too for this release?

2. For the mothod of moving our PRs onto abstraction. Option 1. push to the abstraction. Option 2. open PR that targets abstraction. I feel like Option 2 is better to because it gives more clear view of diffs

[CPerezz]

@kilic

zcash#608 Might be the only one. But since we have a bunch of parallelization and GPU opts incoming, I think we can skip all 7 for this release.

2. I think @AronisAt79 already pushed into abstraction. Whatever it is, please mark that is done in the PR here. *The checkbox)

[therealyingtong]

Do we want Circuit::Value (zcash#598) as well? This will mean updating the types from Option<V> to Value<V> in every circuit, but I don't think it would introduce many rebase conflicts.

[CPerezz]

Do we want Circuit::Value (zcash#598) as well? This will mean updating the types from Option<V> to Value<V> in every circuit, but I don't think it would introduce many rebase conflicts.

Indeed this is in the current HEAD of upstream halo2. And since we're rebasing on top of it + adding the PRs listed above, this would get included too.

[han0110]

I have picked all necessary changes and squash them into branch feature/abstraction-squashed. I didn't force update abstraction branch because halo2wrong depends on it and #29 add another argument name to API ConstraintSystem::lookup as a breaking change.

I think a better way to process this release is to open another PR and leave abstraction untouched, then after the release tag is ready, we can start to update everything in halo2wrong and zkevm-circuits. Will open a PR soon if this makes sense.

[CPerezz]

Closed via #93