HyperKZG+Shplonk primary PCS #348
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Folks suggest using 'measurement_time' equals to 100s in order to make measurements more resilient to transitory peak loads caused by external programs. bheisler/criterion.rs#322
|
!benchmark --bench pcs |
Benchmark for 9c4b1ecClick to view benchmark
|
|
|
huitseeker
reviewed
Feb 27, 2024
src/provider/hyperkzg.rs
Outdated
| let d = f_x.coeffs.len(); | ||
|
|
||
| // Compute h(x) = f(x)/(x - u) | ||
| let mut h = vec![E::Fr::ZERO; d]; |
There was a problem hiding this comment.
I remember you had doubts about the divide_with_q_and_r function which is indeed used in non_hiding_zeromorph::UVKZGPCS::open, which you could use here instead of reimplementing. FYI, I added tests of correctness in #344
|
|
||
| // Phase 3 -- create response | ||
| let (w, evals) = kzg_open_batch(polys, &u, transcript); | ||
| // TODO: since this is a usual KZG10 we should use it as utility instead |
There was a problem hiding this comment.
And in a way that utility is there. I'm happy with code that mode or edits divide_with_q_and_r if you find it slower than it should be.
src/provider/hyperkzg.rs
Outdated
| .collect::<Vec<E::Fr>>(), | ||
| ); | ||
| let mut K_x = batched_Pi.clone(); | ||
| K_x += &tmp; |
There was a problem hiding this comment.
I'm not opposed to adding impl<Fr> SubAssign<&Self> for UniPoly<Fr>, which would perhaps allow us to avoid the somewhat pedestrian iteration above by doing K_x -= &tmp here.
There was a problem hiding this comment.
Added SubAssign in 2017748, using AddAssign's code actually with just changing plus on minus))
src/provider/hyperkzg.rs
Outdated
| .collect::<Vec<_>>(); | ||
|
|
||
| let pairing_result = E::multi_miller_loop(pairing_input_refs.as_slice()).final_exponentiation(); | ||
| if pairing_result.is_identity().unwrap_u8() == 0x00 { |
There was a problem hiding this comment.
Oh, this is not the best way to unwrap a subtle::Choice: just use impl From<Choice> for bool, by calling .into()` here.
There was a problem hiding this comment.
We have Result<(), NovaError> as an output in verify function for EvaluationEngineTrait, so I can suggest:
let successful: bool = pairing_result.is_identity().into();
if !successful {
return Err(NovaError::ProofVerifyError);
}
Ok(())
src/provider/hyperkzg.rs
Outdated
Comment on lines
65
to
71
| NE: NovaEngine<GE = E::G1, Scalar = E::Fr, CE = KZGCommitmentEngine<E>>, | ||
| E::Fr: Serialize + DeserializeOwned, | ||
| E::G1Affine: Serialize + DeserializeOwned, | ||
| E::G2Affine: Serialize + DeserializeOwned, | ||
| E::G1: DlogGroup<ScalarExt = E::Fr, AffineExt = E::G1Affine>, | ||
| E::Fr: PrimeFieldBits, | ||
| <E::G1 as Group>::Base: TranscriptReprTrait<E::G1>, |
There was a problem hiding this comment.
A couple of points on this smorgasboard of constraints:
- the bounds on
E::FrandE::G1Affineare implied by that onE::G1, and should be auto-satisfied. They should no longer be needed whenassociated_type_boundsare possible. - we probably want to put first the two sets of bounds that are actually defining what we need, then comment on what's going on with the rest:
E: Engine,
NE: NovaEngine<GE = E::G1, Scalar = E::Fr, CE = KZGCommitmentEngine<E>>,
E::G1: DlogGroup<ScalarExt = E::Fr, AffineExt = E::G1Affine>,
// the following bounds repeat existing, satisfied bounds on associated types of the above
// but are required since the equality constraints we use in the above do not transitively carry bounds
// we should be able to remove most of those constraints when rust supports associated_type_bounds
E::Fr: Serialize + DeserializeOwned,
E::G1Affine: Serialize + DeserializeOwned,
E::G2Affine: Serialize + DeserializeOwned,
E::Fr: PrimeFieldBits,
<E::G1 as Group>::Base: TranscriptReprTrait<E::G1>,
storojs72
changed the title
|
@huitseeker , I remember you initial request to keep PCS codebase as a separate module + utilising common code as much as possible, so I leave these tasks to myself for future patches in context of #304 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #302
I tried to merge Shplonk changes carefully, but patch appeared to be a little bit weird, sorry...
Specifically for this PR, I run benchmarks for comparing HyperKZG, Shplonk, HyperKZG + Shplonk as a separate PCSs.
On my dev machine, prover HyperKZG + Shplonk has the best prover performance in 4 cases (12, 14, 16, 20), in 1 case (10) Shplonk is better.
As for the verifier, HyperKZG + Shplonk is better than Shplonk in all cases (10, 12, 14, 18, 20) and better than HyperKZG in the 3 cases (12, 14, 16).
Those results give me some confidence that HyperKZG + Shplonk provides better prover and at more or less comparable verifier, so I'm removing leaving it as a default (dropping rest from the codebase).
It would be nice to see HyperKZG + Shplonk benches results executed on dedicated machine and compared with previously stored ones for pure HyperKZG