arkworks-rs / snark

Interfaces for Relations and SNARKs for these relations
https://www.arkworks.rs
Apache License 2.0
776 stars 209 forks source link

Implementing deserialization for Proof, VerifyingKey, Parameters in GM17 #237

Closed ChaitanyaKonda closed 4 years ago

ChaitanyaKonda commented 4 years ago

Implemented fn read<R: Read>(mut _reader: R, _checked: bool) -> io::Result<Self> for Proof, VerifyingKey, Parameters in GM17. Currently they are unimplemented as shown below.

https://github.com/scipr-lab/zexe/blob/9b04691267687bc93beb96a1170d414e3b9182ef/gm17/src/lib.rs#L93 https://github.com/scipr-lab/zexe/blob/9b04691267687bc93beb96a1170d414e3b9182ef/gm17/src/lib.rs#L157 https://github.com/scipr-lab/zexe/blob/9b04691267687bc93beb96a1170d414e3b9182ef/gm17/src/lib.rs#L201

PRing this code on behalf EY.

fixes #224

Pratyush commented 4 years ago

Hmm I think from a deployment perspective it's safer/cleaner to go via the Canonical{Serialize,Deserialize} traits, because the output of those is guaranteed to be stable, whereas ToBytes and FromBytes are more for internal use (I might even remove/forward {To,From}Bytes to Canonical{Serialize,Deserialize} in the future)

ChaitanyaKonda commented 4 years ago

Before implementing ToBytes and FromBytes, I used serialize_uncompressed and deserialize_uncompressed in the code to add zexe gm17 backend to zokrates. It works well for serializing parameters and deserializing parameters for small circuits. If a circuit has 100000 constraints on BLS12_377, while using deserialize_uncompressed on parameters it takes a very long time (more than an hour). So I tested deserialize_uncompressed on parameters directly in zexe tests. It seems to be rather quick. I wonder if I am missing out turning on an optimisation parameter while using zexe library externally

Pratyush commented 4 years ago

Hmm were you using --release? Another issue might be in buffering, if you're writing to disk at any point

ChaitanyaKonda commented 4 years ago

--release seems to have solved that. I noticed this profile by default as well in the cargo.toml in root has optimisation set to 3

iAmMichaelConnor commented 4 years ago

@Pratyush I've been helping out on the project @ChaitanyaKonda's been working on; we're adding a zexe backend to Zokrates. The current workflow is as follows: 1) Via Zokrates, do a trusted setup of a circuit, using zexe as the backend. 2) Zexe will return a verification key and a proving key to Zokrates, which we serialise and store in a volume. 3) Some time later, a user will request to generate a proof, via Zokrates, using the stored proving key. 4) Zokrates 'deserialises' the proving key (using zexe) for use in generating the proof.

The deserialize_uncompressed function does some extra safety checks on the proving key's points (such as checking for points at infinity, and checking values are below the field modulus, etc). We believe it's these extra checks which are causing a significant slow-down in our proof generation time.

For our use-case, I'd suggest that we don't need the extra 'safety checks' when deserialising, because we're using a proving key which was originally produced and serialised by zexe (see step 2 above); and therefore zexe will already have validated the proving key's data when it was generated.

Therefore, for our use case, it would be much more efficient to have access to basic read/write functionality without the superfluous safety checks.

Some rough experimental values (from Chait's 16gb MacBook Pro) using zokrates to generate a proof (with a zexe backend) for a 100,000 constraint circuit:

This PR's Read Write, compiled with Debug - ~ 88 seconds
This PR's Read Write, compiled with Release - ~ 3 seconds

Canonical Serialize/Deserialize, compiled with with Debug - ~ over an hour
Canonical Serialize/Deserialize, compiled with Release - ~ 210 seconds 

It would be great to get your expert thoughts on this.

Pratyush commented 4 years ago

Ah, I see, in this case deserialization is from a trusted source, so you don't need to perform any checks. I opened PR #253 that should solve this use case more generally (and also for Groth16).

iAmMichaelConnor commented 4 years ago

Ah, I see, in this case deserialization is from a trusted source, so you don't need to perform any checks

That's a lovely, succinct way of explaining it, yes. Thanks for doing a PR to add this functionality!

Pratyush commented 4 years ago

Superseded by #253