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

Support optimized witness generation for circuit2. #190

Closed
wants to merge 7 commits into from

Conversation

porcuquine
Copy link
Collaborator

@porcuquine porcuquine commented Jun 2, 2023

[Do not merge until updated for bellpepper.]

UPDATE: This PR now uses bellpepper. However, we intend to retain bellperson compatibilty for those requiring it. This PR switches to bellpepper to expedite upstream changes. Therefore it should also add a mechanism for using bellperson instead, when needed.

UPDATE2: I have added a bellperson feature, so now this only needs to be updated to exercise the feature flag on CI.

UPDATE3: Actually, the new work still depends on bellperson git dependency, so maybe the answer is to make the new work unsupported when bellperson feature is selected.

This PR makes use of filecoin-project/bellperson#309 to implement SizedWitness for poseidon. The resulting witness is for the circuit2 variant, with final result allocated. Several entry points are provided, to support usages where the witness should be, for example, generated directly into the constraint system, or returned for later use.

The synthesis benchmark is extended to allow comparison with previous synthesis (using BenchCS, which carries minimal overhead). Note that the hash_allocated_witness runs are about 16x faster than the Optimal Allocated Poseidon Circuit runs.

➜  neptune git:(witness-gen) ✗ cargo criterion --bench synthesis
   Compiling neptune v9.0.0 (/Users/clwk/fil/neptune)
    Finished bench [optimized] target(s) in 5.82s
synthesis-8/Legacy Poseidon Circuit/arity: 8, count: 1                                                                            
                        time:   [606.56 µs 608.75 µs 612.32 µs]
                        change: [-0.6349% -0.2242% +0.2068%] (p = 0.37 > 0.05)
                        No change in performance detected.
synthesis-8/Optimal Allocated Poseidon Circuit,/arity: 8, count: 1                                                                            
                        time:   [591.16 µs 591.63 µs 592.15 µs]
                        change: [-1.3293% -0.5826% -0.1104%] (p = 0.08 > 0.05)
                        No change in performance detected.
synthesis-8/hash_allocated_witness/arity: 8, count: 1                                                                            
                        time:   [36.666 µs 36.695 µs 36.747 µs]
                        change: [+0.6121% +0.8506% +1.1294%] (p = 0.00 < 0.05)
                        Change within noise threshold.
synthesis-8/Legacy Poseidon Circuit/arity: 8, count: 10                                                                           
                        time:   [6.1382 ms 6.1424 ms 6.1468 ms]
                        change: [-1.9551% -1.0453% -0.2484%] (p = 0.02 < 0.05)
                        Change within noise threshold.
synthesis-8/Optimal Allocated Poseidon Circuit,/arity: 8, count: 10                                                                           
                        time:   [5.9172 ms 5.9187 ms 5.9217 ms]
                        change: [+0.8503% +1.1880% +1.5227%] (p = 0.00 < 0.05)
                        Change within noise threshold.
synthesis-8/hash_allocated_witness/arity: 8, count: 10                                                                           
                        time:   [365.80 µs 366.54 µs 367.76 µs]
                        change: [-15.570% -14.962% -14.400%] (p = 0.00 < 0.05)
                        Performance has improved.
synthesis-8/Legacy Poseidon Circuit/arity: 8, count: 100                                                                           
                        time:   [61.153 ms 61.457 ms 61.860 ms]
                        change: [+2.2618% +2.5457% +2.9353%] (p = 0.00 < 0.05)
                        Performance has regressed.
synthesis-8/Optimal Allocated Poseidon Circuit,/arity: 8, count: 100                                                                           
                        time:   [58.389 ms 58.418 ms 58.446 ms]
                        change: [-32.491% -32.425% -32.362%] (p = 0.00 < 0.05)
                        Performance has improved.
synthesis-8/hash_allocated_witness/arity: 8, count: 100                                                                            
                        time:   [3.6248 ms 3.6411 ms 3.6632 ms]
                        change: [-95.978% -95.962% -95.943%] (p = 0.00 < 0.05)
                        Performance has improved.
Benchmarking synthesis-8/Legacy Poseidon Circuit/arity: 8, count: 1000: Warming up for 3.0000 s
Warning: Unable to complete 10 samples in 5.0s. You may wish to increase target time to 6.1s.
synthesis-8/Legacy Poseidon Circuit/arity: 8, count: 1000                                                                          
                        time:   [605.64 ms 607.31 ms 609.05 ms]
                        change: [+1.1024% +1.3995% +1.7238%] (p = 0.00 < 0.05)
                        Performance has regressed.
Benchmarking synthesis-8/Optimal Allocated Poseidon Circuit,/arity: 8, count: 1000: Warming up for 3.0000 s
Warning: Unable to complete 10 samples in 5.0s. You may wish to increase target time to 5.9s.
synthesis-8/Optimal Allocated Poseidon Circuit,/arity: 8, count: 1000                                                                          
                        time:   [585.13 ms 587.99 ms 591.48 ms]
                        change: [-32.349% -31.993% -31.619%] (p = 0.00 < 0.05)
                        Performance has improved.
synthesis-8/hash_allocated_witness/arity: 8, count: 1000                                                                           
                        time:   [36.332 ms 36.387 ms 36.419 ms]
                        change: [-95.984% -95.973% -95.962%] (p = 0.00 < 0.05)
                        Performance has improved.

@porcuquine porcuquine force-pushed the witness-gen branch 4 times, most recently from 21013c6 to 4eee9a4 Compare June 2, 2023 11:20
src/circuit2.rs Outdated Show resolved Hide resolved
src/circuit2.rs Outdated Show resolved Hide resolved
src/circuit2_witness.rs Outdated Show resolved Hide resolved
src/circuit2_witness.rs Show resolved Hide resolved
src/circuit2_witness.rs Show resolved Hide resolved
src/circuit2_witness.rs Outdated Show resolved Hide resolved
src/circuit2_witness.rs Show resolved Hide resolved
src/circuit2_witness.rs Show resolved Hide resolved
src/circuit.rs Outdated Show resolved Hide resolved
src/circuit.rs Outdated Show resolved Hide resolved
src/circuit2_witness.rs Outdated Show resolved Hide resolved
Copy link
Member

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

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

Other TODO: this line should be updated when bellperson releases:

gbench/Cargo.toml
10:bellperson = { version = "0.25.0", default-features = false }

@porcuquine
Copy link
Collaborator Author

I pushed a commit that makes the witness-generation optimization automatically available to SAFE.

Copy link
Member

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

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

This LGTM apart for the bellperson dependency of course (to be updated in 2 places). The rebase should be pretty trivial.
This is a nit: #200

@huitseeker
Copy link
Member

FYI: #199 is changing the required tests upon merging (removing “rustfmt”, which is now integrated in “clippy” and adding “msrv”, which requires source compatibility with the declared rust-version). The PR in its current form has not run the second check, hence the “Some checks haven’t completed yet”, but that should resolve when you rebase.

huitseeker added a commit to huitseeker/neptune that referenced this pull request Jul 28, 2023
This is nothing but a rebased and refreshed argumentcomputer#190 Please refer to the authoritative commit message there.

- Import and implement witness-generation for optimal Poseidon hash circuit into new file `circuit2_witness.rs` and update file `circuit2.rs` to accommodate these changes.
- Private methods in `circuit2.rs` are now public and deprecated function descriptions have been updated.
- The `BenchCircuit` struct in `synthesis.rs` has been refactored to include new functionalities and handle more cases.
- A module `circuit2_witness` is added to `lib.rs` for exclusively witness generation..
- The `bellpepper` dependency is moved from dev-dependencies to dependencies in `Cargo.toml` (due to using witness generation).
- Unit tests are introduced for the new functionalities in `circuit2_witness.rs`.
huitseeker added a commit to huitseeker/neptune that referenced this pull request Jul 28, 2023
This is nothing but a rebased and refreshed argumentcomputer#190 Please refer to the authoritative commit message there.

- Import and implement witness-generation for optimal Poseidon hash circuit into new file `circuit2_witness.rs` and update file `circuit2.rs` to accommodate these changes.
- Private methods in `circuit2.rs` are now public and deprecated function descriptions have been updated.
- The `BenchCircuit` struct in `synthesis.rs` has been refactored to include new functionalities and handle more cases.
- A module `circuit2_witness` is added to `lib.rs` for exclusively witness generation..
- The `bellpepper` dependency is moved from dev-dependencies to dependencies in `Cargo.toml` (due to using witness generation).
- Unit tests are introduced for the new functionalities in `circuit2_witness.rs`.
github-merge-queue bot pushed a commit that referenced this pull request Jul 29, 2023
This is nothing but a rebased and refreshed #190 Please refer to the authoritative commit message there.

- Import and implement witness-generation for optimal Poseidon hash circuit into new file `circuit2_witness.rs` and update file `circuit2.rs` to accommodate these changes.
- Private methods in `circuit2.rs` are now public and deprecated function descriptions have been updated.
- The `BenchCircuit` struct in `synthesis.rs` has been refactored to include new functionalities and handle more cases.
- A module `circuit2_witness` is added to `lib.rs` for exclusively witness generation..
- The `bellpepper` dependency is moved from dev-dependencies to dependencies in `Cargo.toml` (due to using witness generation).
- Unit tests are introduced for the new functionalities in `circuit2_witness.rs`.
@huitseeker
Copy link
Member

Made obsolete by #219.

@huitseeker huitseeker closed this Jul 30, 2023
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.

2 participants