w3f / apk-proofs

Apache License 2.0
56 stars 19 forks source link

Review notes #23

Open burdges opened 3 years ago

burdges commented 3 years ago

It's quite beautiful code :)

7 June 2021:

burdges commented 3 years ago

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

InaOana commented 3 years ago

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

burdges commented 3 years ago

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:

burdges commented 3 years ago

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