-
Notifications
You must be signed in to change notification settings - Fork 15
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
Migrate to Plonk v0.2.0 #54
Conversation
\## [0.5.0] - 11-05-20 \### Added - `dusk-plonk_v0.1.0` as proving system. - `dusk-bls12_381_v0.1.0` as curve-backend \### Changed - `GadgetStrategy` structure refactor & optimization. - tests updated & refactored with the new proving system. \### Fixed \### Removed - `Bulletproofs` is no longer the proving system used. - `Curve25519-dalek` is no longer used as curve-backend. Also: - Update Cargo.toml to bump v0.5 & remove old deps - Removed `num-traits` dep which is no longer needed.
[0.5.0] - 11-05-20 ### Added - `dusk-plonk_v0.1.0` as proving system. - `dusk-bls12_381_v0.1.0` as curve-backend ### Changed - `GadgetStrategy` structure refactor & optimization. - tests updated & refactored with the new proving system. ### Fixed ### Removed - `Bulletproofs` is no longer the proving system used. - `Curve25519-dalek` is no longer used as curve-backend.
- We only use dusk-plonk as dep and the re-exported jubjub and bls crate-structures that come with it. - Use plonk `prelude.rs` file for the imports everywhere. - Refactor the tests to work with the Prover&Verifier abstraction. Closes #38
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few nits, plus a question (about zero_var
)
src/strategies/gadget.rs
Outdated
prover.preprocess(&ck).unwrap(); | ||
let proof = prover.prove(&ck).unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make the tests returns Error
(as is now done in the blind bid if you need a reference).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a common practice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so there's a couple of things to mention related to that.
- PLONK is using
failure
crate for error handling. The problem is that is deprecated, so I don't want to include failure here just for that. - I can add a follow-up issue in respect of the error stuff that you mention and go for it once PLONK is refactored to use
anyhow
as error handling crate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know failure
is deprecated, I was the one to point it out :)
FFR, As discussed, I thought Hades was also handle the error from PLONK but apparently is something only we had in tests, so as suggested for the time being we can just use expect
instead.
We'll refactor the tests in the future (low priority tech debt) to handle the error properly, once we have a new unified approach for error handling – possibly something not experimental such as the failure crate.
src/strategies/gadget.rs
Outdated
// Verifying | ||
let mut verifier = Verifier::new(b"hades_gadget_tester"); | ||
let _ = hades_gadget_tester(i, o, verifier.mut_cs()); | ||
verifier.preprocess(&ck).unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WDYM?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto – the same thing again (used in lists and accounts and often indicated by a ditto mark under the word or figure to be repeated).
src/strategies/gadget.rs
Outdated
let (mut composer_2, pi2) = new_composer(e, e_perm); | ||
let proof2 = composer_2.prove(&ck, &circuit, &mut transcript.clone()); | ||
let pi2 = hades_gadget_tester(e, e_perm, prover.mut_cs()); | ||
let proof2 = prover.prove(&ck).unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same.
src/strategies/gadget.rs
Outdated
let proof3 = composer_3.prove(&ck, &circuit, &mut transcript.clone()); | ||
prover.clear_witness(); | ||
let pi3 = hades_gadget_tester(x, h, prover.mut_cs()); | ||
let proof3 = prover.prove(&ck).unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And here too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
We only use dusk-plonk as dep and the re-exported jubjub
and bls crate-structures that come with it.
Use plonk
prelude.rs
file for the imports everywhere.Refactor the tests to work with the Prover&Verifier abstraction.
Closes #53
Once this is merged I'll bump to 0.6.0