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

Refactor Composer into trait #700

Merged
merged 2 commits into from
Aug 17, 2022
Merged

Refactor Composer into trait #700

merged 2 commits into from
Aug 17, 2022

Conversation

vlopes11
Copy link
Contributor

A composer is a set of functionalities that can be extended into
different implementators, such as debuggers or even PLONKup.

This commit introduces the composer as trait, and targets to reduce the
complexity of the implementation of multiple components of the composer.

With this simplification, we achieved significant performance boost for
public inputs evaluation.

We also removed some of the exported types that are leaked internals.
The user shouldn't be aware of public inputs indexes, kzg commitments,
keys or any permutation argument. Instead, he should work with proofs,
public inputs, public parameters, labels, circuits, provers and
verifiers.

The proof generation was largely simplified, reducing its arguments to a
random number generator for the proof blinders, and a circuit instance
for its witnesses.

The proof verification was simplified to take only the proof and its
public inputs. The public inputs are not encoded into the proof because
they are often used from external sources, such as blockchain payload.

A debugger was introduced and it will output CDF files if the feature
flag is on, and the CDF_OUTPUT environment variable is set. These CDF
files are expected to be read from the TCDB debugger - a CLI application
inspired in gdb, for zk-SNARKS Plonk circuits.

Finally, a type-safe constraint was introduced, binding the prover
and verifier with their concrete circuit implementation. This allowed
great simplification of the verification process since the user don't
need to manage verification keys anymore.

@codecov
Copy link

codecov bot commented Aug 15, 2022

Codecov Report

Merging #700 (c2be965) into master (66accb8) will increase coverage by 3.10%.
The diff coverage is 85.21%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #700      +/-   ##
==========================================
+ Coverage   80.14%   83.25%   +3.10%     
==========================================
  Files          47       46       -1     
  Lines        3526     3630     +104     
==========================================
+ Hits         2826     3022     +196     
+ Misses        700      608      -92     
Impacted Files Coverage Δ
src/commitment_scheme/kzg10/key.rs 85.64% <0.00%> (+1.85%) ⬆️
src/composer/polynomial.rs 0.00% <0.00%> (ø)
src/constraint_system/witness.rs 50.00% <ø> (ø)
src/error.rs 0.00% <0.00%> (ø)
src/fft/polynomial.rs 42.14% <ø> (+1.65%) ⬆️
src/lib.rs 100.00% <ø> (ø)
src/proof_system/proof.rs 95.73% <ø> (ø)
src/proof_system/widget.rs 92.75% <ø> (+0.26%) ⬆️
src/debugger.rs 27.36% <27.36%> (ø)
src/runtime.rs 38.46% <38.46%> (ø)
... and 35 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 66accb8...c2be965. Read the comment docs.

A composer is a set of functionalities that can be extended into
different implementators, such as debuggers or even PLONKup.

This commit introduces the composer as trait, and targets to reduce the
complexity of the implementation of multiple components of the composer.

With this simplification, we achieved significant performance boost for
public inputs evaluation.

We also removed some of the exported types that are leaked internals.
The user shouldn't be aware of public inputs indexes, kzg commitments,
keys or any permutation argument. Instead, he should work with proofs,
public inputs, public parameters, labels, circuits, provers and
verifiers.

The proof generation was largely simplified, reducing its arguments to a
random number generator for the proof blinders, and a circuit instance
for its witnesses.

The proof verification was simplified to take only the proof and its
public inputs. The public inputs are not encoded into the proof because
they are often used from external sources, such as blockchain payload.

A debugger was introduced and it will output CDF files if the feature
flag is on, and the `CDF_OUTPUT` environment variable is set. These CDF
files are expected to be read from the TCDB debugger - a CLI application
inspired in gdb, for zk-SNARKS Plonk circuits.

Finally, a type-safe constraint was introduced, binding the prover
and verifier with their concrete circuit implementation. This allowed
great simplification of the verification process since the user don't
need to manage verification keys anymore.
Copy link
Member

@moCello moCello left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generally the changes LGTM, the only remark (apart from the few thingies I commented about) is that there are quite a big amount of comments missing compared to the old version of plonk.
In my opinion the protocol and implementation are already complicated enough and if in doubt I would rather opt for too many comments than too few.

benches/plonk.rs Show resolved Hide resolved
pub trait Composer: Sized + Index<Witness, Output = BlsScalar> {
/// Zero representation inside the constraint system.
///
/// A turbo composer expects the first witness to be always present and to
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove 'turbo'


/// `One` representation inside the constraint system.
///
/// A turbo composer expects the 2nd witness to be always present and to
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove 'turbo'

//
// Copyright (c) DUSK NETWORK. All rights reserved.

//! PLONK turbo composer definitions
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove 'turbo'

src/composer.rs Show resolved Hide resolved
src/composer/prover.rs Show resolved Hide resolved
Comment on lines +128 to +129
pub fn serialized_size(&self) -> usize {
self.prepare_serialize().0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i would calculate the size in this function and not in prepare_serialize. in that case we don't prepare for serialization when we simply want to get the size

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is to avoid duplicated code. If we repeat the calculation here, then we are duplicating the implementation for when we actually serialize

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would the code be duplicated? We can call serialized_size where we need the size

src/composer/prover.rs Show resolved Hide resolved
src/composer/prover.rs Show resolved Hide resolved
src/composer/verifier.rs Show resolved Hide resolved
Copy link
Member

@autholykos autholykos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving on @vlopes11 request and allowing the merge

Copy link
Member

@ureeves ureeves left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! I like the API simplification. Not sure about those #[deprecated] attributes, but I also couldn't find another way to just emit a compiler warning.
I do second the removal of the word "turbo" everywhere, as @moCello suggested, but it's really just a nit and can be addressed at any time.

@vlopes11 vlopes11 merged commit 65db5f0 into master Aug 17, 2022
@vlopes11 vlopes11 deleted the feature/vlopes11/trait-tc branch August 17, 2022 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants