Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use a bellman::Proof instead of a byte array in Groth16Proof #3179

Closed
conradoplg opened this issue Dec 9, 2021 · 2 comments
Closed

Use a bellman::Proof instead of a byte array in Groth16Proof #3179

conradoplg opened this issue Dec 9, 2021 · 2 comments
Labels
A-cryptography Area: Cryptography related C-cleanup Category: This is a cleanup C-enhancement Category: This is an improvement

Comments

@conradoplg
Copy link
Collaborator

conradoplg commented Dec 9, 2021

Motivation

We use a byte array in the Groth16Proof struct. However, not every byte array is a well-formed proof (i.e. the encoding of three field elements, as enforced by the bellman package). For improved type safety, it would be better to use bellman::Proof inside it and parse the proof while parsing the transaction.

However, this requires a lot of code changes:

  • A lot of tests expect it to be a byte array
  • The tests vectors from https://github.com/zcash-hackworks/zcash-test-vectors/ generate random byte array as proofs. We'd need to change the test vector generators to generate random well-formed proofs.
  • In the same way, the Arbitrary implementation must be changed to generate random well-formed proofs.

For this reason, it was postponed for later.

Specifications

Designs

To generate a well-formed proof, you will need to generate 3 field elements: one in G1, second in G2, third in G1 again. See this snippet for reference:

// A proof is composed of three field elements, the first and last having 48 bytes.
// (The middle one has 96 bytes.) To corrupt the proof without making it malformed,
// simply swap those first and last elements.
let (first, rest) = joinsplit.zkproof.0.split_at_mut(48);
first.swap_with_slice(&mut rest[96..144]);

Related Work

@conradoplg conradoplg added C-enhancement Category: This is an improvement S-needs-triage Status: A bug report needs triage labels Dec 9, 2021
@mpguerra mpguerra added C-cleanup Category: This is a cleanup P-Medium labels Dec 15, 2021
@mpguerra
Copy link
Contributor

@ftm1000 ftm1000 removed the S-needs-triage Status: A bug report needs triage label Mar 10, 2022
@conradoplg conradoplg added the A-cryptography Area: Cryptography related label Jun 1, 2022
@teor2345
Copy link
Contributor

Making this change would increase the cost of serving blocks to peers, because the proofs would be redundantly re-validated when deserializing from the database.

We're already having this performance problem with some curve types.

So I think it's ok for now.

@teor2345 teor2345 closed this as not planned Won't fix, can't repro, duplicate, stale Aug 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cryptography Area: Cryptography related C-cleanup Category: This is a cleanup C-enhancement Category: This is an improvement
Projects
None yet
Development

No branches or pull requests

4 participants