Use batch commitment scheme and commit only polynomial linear combination #144
Conversation
akinovak
commented
May 18, 2022
akinovak
commented
- Create linear combination of "aw" and "saw" polynomials and commit to it instead of every single polynomial
- Use batched version of polynomial commitment scheme to reduce number of pairing checks
akinovak
added
T-feature
akinovak
requested review from
lopeetall,
davidnevadoc,
LukePearson1 and
joebebel
|
If this PR will also incorporate changes for the IPA as PCS too, could we turn to draft for the meantime? @akinovak |
There was a problem hiding this comment.
Thanks for this work @akinovak !! ❤️
It would be nice if we could before merging to get benchmarks to check previous and actual approach to see the batching benefits.
Also, as mentioned in a comment:
Getting rid of the Transcript from merlin and just using the FiatShamir internally prevents the parties to setup some original randomness on the transcript before starting the protocol.
On the other hand, we get rid off the labes which we saw was a bit annoying to workarround in #133 .
Also, it's much easier for what refers to the next refactor we have planned where we want to support arbitrary ammount of wires.
This is a big change and I would really like others to take part and express their opinions.
From my side I would be OK with the changes as long as they represent a performance difference.
| use ark_serialize::*; | ||
|
|
||
| // Copy(bound = "PC::Commitment: Copy"), |
| // #[derive(CanonicalDeserialize, CanonicalSerialize, derivative::Derivative)] | ||
| // #[derivative( | ||
| // Clone(bound = ""), | ||
| // Debug( | ||
| // bound = "arithmetic::VerifierKey<F,PC>: core::fmt::Debug, | ||
| // PC::Commitment: core::fmt::Debug" ), | ||
| // Eq(bound = "arithmetic::VerifierKey<F,PC>: Eq, PC::Commitment: Eq"), | ||
| // PartialEq( | ||
| // bound = "arithmetic::VerifierKey<F,PC>: PartialEq, PC::Commitment: | ||
| // PartialEq" ) | ||
| // )] |
| #[test] | ||
| fn test_serialise_deserialise_prover_key() { | ||
| type F = ark_bls12_381::Fr; | ||
| // TODO make test to call this |
There was a problem hiding this comment.
The test is done.
The comment can be removed.
| @@ -64,6 +64,9 @@ merlin = { version = "3.0", default-features = false } | |||
| num-traits = { version = "0.2.14" } | |||
| rand_core = {version = "0.6", default-features=false, features = ["getrandom"] } | |||
|
|
|||
| ark-marlin = "0.3.0" | |||
There was a problem hiding this comment.
In regards of ark-marlin, we are just getting FiatShamirRng. Don't we have an alternative to save us this dep and more complexity?
There was a problem hiding this comment.
Yes, I think that in the newer version FiatShamirRng is separated from ark-marlin, will make sure to check and fix :)
There was a problem hiding this comment.
If possible, I would not recommend using ark-marlin as a dependency to use FiatShamirRng, because as CPerezz pointed out, there would be significant refactoring around FiatShamirRng in arkworks-rs.
There was a problem hiding this comment.
What do you recommend using until then?
plonk-core/src/proof_system/proof.rs
Outdated
| let mut fs_rng = | ||
| FiatShamirRng::<D>::from_seed(&to_bytes![&PROTOCOL_NAME].unwrap()); | ||
|
|
||
| //Append Public Inputs to the transcript |
| transcript: &mut Transcript, | ||
| _transcript: &mut Transcript, |
There was a problem hiding this comment.
It does not make any sense to pass the Transcript anymore.
This is the part that I don't really like TBH. Getting rid of the Transcript from merlin and just using the FiatShamir internally prevents the parties to setup some original randomness on the transcript before starting the protocol.
On the other hand, we get rid off the labes which we saw was a bit annoying to workarround in #133 .
Also, it's much easier for what refers to the next refactor we have planned where we want to support arbitrary ammount of wires.
There was a problem hiding this comment.
Sorry for the miscommunication, I spoke with @davidnevadoc and @LukePearson1. I intentionally didn't remove this transcript, as you said change is already really big and I wanted to check if that makes sense at all before introducing even more changes.
Instead, rng will be passed as argument and it will be also preprocessed with same values that are being inserted in transcript now.
Happy to chat more about arbitrary number of wires.
Co-authored-by: Carlos Pérez <37264926+CPerezz@users.noreply.github.com>
| @@ -388,44 +403,35 @@ where | |||
| PC::commit(commit_key, &[label_polynomial!(z_2_poly)], None) | |||
| .map_err(to_pc_error::<F, PC>)?; | |||
|
|
|||
| // TODO this should be added to transcript? | |||
| // fs_rng.absorb(&to_bytes![z_2_poly_commit].unwrap()); | |||
There was a problem hiding this comment.
Flag this for other people to provide inputs.
