apk-proofs icon indicating copy to clipboard operation
apk-proofs copied to clipboard

Review notes

Open burdges opened this issue 4 years ago • 4 comments

It's quite beautiful code :)

7 June 2021:

  • It's possible to initialize some parts of the prover once per epoch, via Prover: Clone.
  • It's possible to extract a linear accountable proof from a packed proof? oops, no
  • IWewant to think through why 128 bit suffices everywhere
  • missing newline https://github.com/w3f/apk-proofs/blob/main/bw6/src/utils.rs#L141
  • odd I guess this is a scalar multiplication of a polynomial https://github.com/w3f/apk-proofs/blob/main/bw6/src/utils.rs#L149
  • Can you explain deg(n) = r on https://github.com/w3f/apk-proofs/blob/main/bw6/src/piop/affine_addition.rs#L288
  • Jeff should better understand plonk linearization polynomial trick https://hackmd.io/CdZkCe2PQuy7XG7CLOBRbA?view#Summary-of-the-efficiency-of-the-SNARK and final proof step
  • unnecessary lifetime? https://github.com/w3f/apk-proofs/blob/main/bw6/src/kzg/mod.rs#L214
  • Oana shall look A(x) (DONE)

burdges avatar Jun 07 '21 10:06 burdges

Just for my convenience, our common tabs/bookmarks while reading:

https://github.com/w3f/research-internal/blob/master/papers/LightClient/LightClient_stable.pdf https://github.com/w3f/apk-proofs/issues/23 https://github.com/w3f/apk-proofs/tree/main/bw6/src https://github.com/w3f/apk-proofs/blob/main/bw6/src/lib.rs#L120 https://github.com/w3f/apk-proofs/blob/main/bw6/src/prover.rs#L170 https://github.com/w3f/apk-proofs/blob/main/bw6/src/piop/mod.rs https://github.com/w3f/apk-proofs/blob/main/bw6/src/piop/basic.rs https://github.com/w3f/apk-proofs/blob/main/bw6/src/piop/affine_addition.rs#L288 https://hackmd.io/CdZkCe2PQuy7XG7CLOBRbA / https://hackmd.io/@Oana/ByKu5Utdw https://github.com/w3f/apk-proofs/blob/main/bw6/src/verifier.rs https://github.com/w3f/apk-proofs/blob/main/bw6/src/kzg/mod.rs#L214 https://docs.rs/ark-poly/0.3.0/ark_poly/polynomial/univariate/struct.DensePolynomial.html?search=aggregate_polynomials#method.divide_by_vanishing_poly https://hackmd.io/CdZkCe2PQuy7XG7CLOBRbA?view#Summary-of-the-efficiency-of-the-SNARK

https://hackmd.io/NlES9mnjTr-GPFFtNU6AbA?view / https://hackmd.io/@Oana/BkZaAdxcw

burdges avatar Jun 07 '21 12:06 burdges

Additional useful links: https://hackmd.io/@aztec-network/plonk-arithmetiization-air

InaOana avatar Jun 08 '21 11:06 InaOana

What does https://github.com/w3f/apk-proofs/blob/main/bw6/src/piop/mod.rs#L91 mean? Also can you be clearer about https://github.com/w3f/apk-proofs/blob/main/bw6/src/piop/mod.rs#L94 ?

These stay zero afterwards https://github.com/w3f/apk-proofs/blob/main/bw6/src/piop/affine_addition.rs#L312-L314 ? OK :)

Yes arkworks needs a transcript abstraction or ark_merlin crate that works with the reader and writer stuff it inherited from zcash https://github.com/w3f/apk-proofs/blob/main/bw6/src/transcript.rs#L74 ref. https://merlin.cool/transcript/ops.html

Bugs:

  • https://github.com/w3f/apk-proofs/blob/main/bw6/src/verifier.rs#L181-L182 ignore their return values. It's too easy to ignore bool returns in Rust and other languages so folks prefer Result<_,_> which is #[must_use], but internally I think that doesn't matter.
  • https://github.com/w3f/apk-proofs/blob/main/bw6/src/verifier.rs#L127 returns nothing. At first I thought it worked purely by adding to the transcript, but fsrng never gets used again and https://github.com/w3f/apk-proofs/blob/main/bw6/src/verifier.rs#L176 should return something.

burdges avatar Jun 18 '21 09:06 burdges

We glanced at https://github.com/w3f/apk-proofs/blob/main/bw6/src/signer_set.rs#L25 today:

As you have a Vec that owns the keys, you should use batch_normalize when converting to affine on https://github.com/w3f/apk-proofs/blob/main/bw6/src/signer_set.rs#L31

Should there be an comments on padding for the IFFT? Is one specified by the ark_poly::domain::EvaluationDomain trait somehow? I guess you always minimize the degree.. Never mind your code is not generic.

Why do we pad with zeros and not the identity of the curve? It's likely fine with the signer set size being enforced, but not sure what happens if that gets corrupted somehow. We cannot pad with the identity since it's the point at infinity. In many cases, (0,0) is a small multiple of the identity, but not sure in all cases.

refs. https://docs.rs/ark-poly/0.3.0/ark_poly/evaluations/univariate/struct.Evaluations.html and https://docs.rs/ark-poly/0.3.0/ark_poly/polynomial/univariate/struct.DensePolynomial.html

burdges avatar Jul 19 '21 08:07 burdges