Proof deserialization

[lazovicff]

Summary of Bug

Facing issues while serializing and deserializing the proof. The proof is valid and satisfies all the constraints.

let proof: Proof<Bls12_381> = prove_groth16(&pk, circuit.clone(), &mut rng);
let mut proof_bytes = vec![0u8; proof.serialized_size()];
proof.serialize(&mut proof_bytes).unwrap();
let proof_anew = Proof::<Bls12_381>::deserialize(&proof_bytes[..]).unwrap();

The error I'm getting is SerializationError::InvalidData on line: https://github.com/webb-tools/arkworks-gadgets/blob/bug-reprodution/src/setup/mixer.rs#L625

If I call Proof::deserialize_unchecked instead of Proof::deserialize I'm getting a SerializationError:: UnexpectedFlags

Version

ark-groth16 = {version = "^0.2.0", default-features = false }

Steps to Reproduce

Clone this repo: https://github.com/webb-tools/arkworks-gadgets Checkout to bug-reprodution branch Run cargo test should_handle_proof_deserialization --features r1cs Which will run this test: https://github.com/webb-tools/arkworks-gadgets/blob/bug-reprodution/src/setup/mixer.rs#L607-L639

[lazovicff]

Also reproducible on the simpler example: https://github.com/webb-tools/arkworks-gadgets/blob/bug-reprodution/src/circuit/basic.rs#L133-L150

by running cargo test should_deserialize_basic_circuit_groth16 --features r1cs

[weikengchen]

I will take a look. This could be an issue that comes from something larger, as Proof's CanonicalDeserialize comes from PairingEngine::G1Affine or PairingEngine::G2Affine.

[weikengchen]

(Updated: nope, unchecked by default pointed to uncompressed.)

~The first error discovered is that serialize_unchecked and deserialize_unchecked are mismatched in the SW curve implementations. This explains: "If I call Proof::deserialize_unchecked instead of Proof::deserialize I'm getting a SerializationError:: UnexpectedFlags"~

---

I will think about the first issue, which is very likely related to is_in_correct_subgroup_assuming_on_curve.

Do you have any more complete error information?

[lazovicff]

Hmm, I'm not getting a lot of info about the errors, but I have 4 scenarios that I have tested:

1. Using to_bytes macro to convert proof to bytes:

   let proof_bytes = to_bytes![proof].unwrap();
   let proof_anew = Proof::<Bls12_381>::deserialize(&proof_bytes[..]).unwrap();

   which gives I/O error: Custom { kind: Other, error: "FromBytes::read failed" }

2. Using serialize + desirialize that gives me SerializationError::InvalidData

3. Using serialize + desirialize_unchecked which gives me SerializationError:: UnexpectedFlags

4. Using serialize_unchecked + desirialize_unchecked which gives me SerializationError:: UnexpectedIdentity

If you want anything else specific, let me know

[lazovicff]

Works if I pass &mut proof_bytes[..] instead of &mut proof_bytes into serialize for the case 2. Other cases still fail, but I have something to work with for now.

[weikengchen]

For all scenarios, it is expected that cases 1 and 3 to fail.

The reason for case 4 to fail is because serialize_unchecked needs a size of uncompressed_size instead of serialized_size. (This should be documented)

But case 2 is expected to pass easily.

[weikengchen]

I find the following: if we pass &mut proof_bytes instead of &mut proof_bytes[..] in your case, the proof_bytes are still all zeros. I suspect that this is something interesting related to Rust...

[weikengchen]

OK. I think we find the reason.

If you write to &mut proof_bytes, all the data goes to the END of the vector. So, the vector is indeed longer. https://github.com/arkworks-rs/utils/blob/fb1f90a14369a62f50e599a7a15f04284fee075f/src/io/mod.rs#L327

This is related to Arkwork's no_std version of Write implementation for Vec<u8>. And it may cause confusion...

@Pratyush do you think we should change the behavior of this? Should Write first clear the vector before extending it with slices?

[weikengchen]

Ok, nope, it seems that this is to be consistent with Rust's implementation for Vec<u8>. https://doc.rust-lang.org/src/std/io/impls.rs.html#361-393

So I think we will not change that since ark_std is aimed to reproduce std.

@filiplazovic We likely should just remember the existence of this trap in Rust.