Skip to content
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

Halo2 circuit in mopro (redesigned) #214

Merged
merged 44 commits into from
Jul 16, 2024

Conversation

ElusAegis
Copy link
Contributor

Proposition on how we could add Halo2 Circuits to redesigned Mopro.

Overview

I attempted to be as consistent as possible to Circom implementation, resulting in defining two types Halo2ProveFn and Halo2VerifyFn which we expect the user to implement locally and then use the key_halo2_circuit_map as a way to map between different circuits and their implementations of prove and verify functions.

_Note that as Halo2 is a library, there is very little shared components, hence Mopro only defines these two types as well as uses key_halo2_circuit_map to invoke correct function at run time. _

Halo2 Module in test-e2e

I have also added a halo2 module in the test-e2e crate. It does not contain a halo2 circuit logic (I have extracted it into another repository as discussed before - https://github.com/ElusAegis/halo2-fibonacci-sample.git, which can be forked into Mopro organisation). What it has is logic to interface with the circuit, implementing logic to parse and prove the values.

As each circuit can use its own types, I did not see a way to abstract it away without enforcing strict user limitations. So halo2 is an example which users would need to reproduce. (Halo2 requires quite a bit of background knowledge).

Halo2/Circom Features

Currently, all users must use both Halo2 and Circom (define key_halo2_circuit_map and zkey_witness_map. I have given a thought to how we could be able to disable and enable each of them, however with current design that does not seem to be possible unless we add something like halo2_placeholder!() which users need to specify if they do not use Halo2. This does not seem ideal UX.

Hence @vimwitch @vivianjeng , perhaps you have some views on how we could be able to do this? We may need to do something different than the current approach of making a user define a particular function.

@ElusAegis ElusAegis force-pushed the feat/halo2-redesign branch from 48fb2eb to 048e534 Compare July 11, 2024 16:35
@ElusAegis ElusAegis marked this pull request as ready for review July 11, 2024 16:39
@ElusAegis
Copy link
Contributor Author

@vimwitch @vivianjeng I have finished the PR. Can you please have a look.

Key changes were to app macros, to enable to have different adapters.

I have it now recursively call halo2_app and circom_app, which each have a fallback implementation when their respective feature is not implemented.

I have also played with how the test-e2e/lib.rs would look like, considering we would need conditionally compile it based on the flags, and I ended up doing it using macros, which are conditionally expanded, thus avoiding to write #[cfg(feature = ...)] before each line.

I have added tests for iOS, however I have not updated Android as I am still unable to compile it.

Copy link
Collaborator

@chancehudson chancehudson left a comment

Choose a reason for hiding this comment

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

Definitely moving in the right direction. Just need to pull some logic out of test-e2e and get rid of a couple macros in favor of written functions.

.github/workflows/build-and-test.yml Outdated Show resolved Hide resolved
mopro-ffi/src/circom/mod.rs Outdated Show resolved Hide resolved
test-e2e/src/halo2/mod.rs Outdated Show resolved Hide resolved
test-e2e/src/halo2/mod.rs Outdated Show resolved Hide resolved
test-e2e/src/halo2/mod.rs Outdated Show resolved Hide resolved
mopro-ffi/src/circom/mod.rs Show resolved Hide resolved
mopro-ffi/src/halo2/mod.rs Outdated Show resolved Hide resolved
mopro-ffi/src/halo2/mod.rs Outdated Show resolved Hide resolved
test-e2e/src/lib.rs Show resolved Hide resolved
@ElusAegis
Copy link
Contributor Author

@vimwitch thank you for the review, I agree with all of it besides how we show the setup to users. Will get to improving the

@ElusAegis ElusAegis force-pushed the feat/halo2-redesign branch from 04fb870 to 8eea240 Compare July 12, 2024 09:05
@ElusAegis ElusAegis requested a review from chancehudson July 12, 2024 12:53
@ElusAegis
Copy link
Contributor Author

ElusAegis commented Jul 12, 2024

Halo2 Mopro Samples:

@ElusAegis
Copy link
Contributor Author

ElusAegis commented Jul 12, 2024

Benchmarking:

Keccak256:

  1. M1 Pro iPhone simulator: 10.1s to prove and 0.13s to verify
  2. iPhone 15 Pro: 11.0s prove time, 0.12s verify time

RSA:

  1. M1 Pro iPhone simulator: ~64.5s proving , ~9s verifying time (with proving/verifying key generation included)
  2. iPhone 15 Pro: Crashes due to memory limit exceeded (needs around 5GB RAM, with iPhone limiting to 3GB)

Copy link
Collaborator

@chancehudson chancehudson left a comment

Choose a reason for hiding this comment

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

Good work, this is just about ready to merge

test-e2e/Cargo.toml Outdated Show resolved Hide resolved
test-vectors/halo2/fibonacci_pk Outdated Show resolved Hide resolved
@chancehudson chancehudson requested a review from vivianjeng July 12, 2024 22:55
@ElusAegis ElusAegis requested a review from chancehudson July 12, 2024 23:53
Copy link
Collaborator

@vivianjeng vivianjeng left a comment

Choose a reason for hiding this comment

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

Looks good to me!
Some places I think it can be simplified as I commented
and some comment messages can be written in docs (can be in the following PRs)

.github/workflows/build-and-test.yml Outdated Show resolved Hide resolved
mopro-ffi/src/halo2/mod.rs Outdated Show resolved Hide resolved
@ElusAegis
Copy link
Contributor Author

@vivianjeng thank you for the review, I have implemented the suggestions. However, after I have merged with the main and updated Cargo.lock it no longer runs the iOS-app-simulator successfully. Yet it does on my local machine with a clean start.

I do remember we had some problems with non-determinism, could it be one of them? Or was there something in the recent PR that was merged that could have impacted it? I looked at it and nothing seems to be able to break the process at that point.

@chancehudson
Copy link
Collaborator

The iOS simulator on github has intermittent failures 😭. Usually if you read the log it will say something like "timed out" or similar.

@chancehudson chancehudson merged commit 5525c77 into zkmopro:main Jul 16, 2024
10 checks passed
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.

3 participants