succinctlabs / sp1

The fastest, most feature-complete zkVM for developers.
https://succinctlabs.github.io/sp1
Apache License 2.0
1.02k stars 334 forks source link

fix(sdk): `.bytes()` inconsistent behavior between groth16 and plonk #1802

Closed ratankaliani closed 5 days ago

ratankaliani commented 6 days ago

Motivation

Inconsistent behavior in bytes between mock PLONK and Groth16 proofs. This caused an issue in op-succinct.

In the future, especially for PR's from external folks, we should have simple tests added in the relevant file to test behavior. This prevents issues such as this which lead to inconsistent API behavior.

Reversion introduced by https://github.com/succinctlabs/sp1/pull/1685. Likely because base dev branch was not up to date.

Solution

Return an empty byte array from .bytes() if it's a mock Groth16 proof.

Add testing to confirm that .bytes() returns the correct output in the future.

Note: Did not test SP1Proof::Compressed because constructing an empty ReduceProof was difficult.

PR Checklist

github-actions[bot] commented 6 days ago

SP1 Performance Test Results

Branch: ratan/return-empty-bytes-groth16 Commit: 64753139 Author: ratankaliani

program cycles execute (mHz) core (kHZ) compress (KHz) time success
fibonacci 11291 0.18 2.78 0.46 25s
ssz-withdrawals 2757356 17.48 128.61 35.09 1m19s
tendermint 12593597 6.71 267.42 98.68 2m9s