-
Notifications
You must be signed in to change notification settings - Fork 36
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
HyperKZG+Shplonk primary PCS #348
Conversation
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
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is starting to look pretty good to me!
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of points on this smorgasboard of constraints:
- the bounds on
E::Fr
andE::G1Affine
are implied by that onE::G1
, and should be auto-satisfied. They should no longer be needed whenassociated_type_bounds
are 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>,
@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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great!
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