Add `Relation` trait #348

Pratyush · 2021-04-20T01:16:33Z

Description

Adds two traits: Relation and NPRelation. Modifies the SNARK trait to use these, instead of the ConstraintSystem infrastructure.

TBD: How to integrate the old ConstraintSystem-based APIs into this. See #348 (comment) for an idea.

closes #323
closes #321
closes #341
closes #342

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (master)
Linked to Github issue with discussion and accepted design OR have an explanation in the PR that describes this work.
Wrote unit tests --- N/A
Updated relevant documentation in the code
Added a relevant changelog entry to the Pending section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer

relations/src/r1cs/constraint_system.rs

Pratyush · 2021-04-20T01:18:00Z

snark/src/lib.rs

        circuit_pk: &Self::ProvingKey,
-        circuit: C,
-        rng: &mut R,
+        instance: &R::Instance,
+        witness: &R::Witness,


Matches the API from the formal definition better.

Pratyush · 2021-04-20T01:18:41Z

snark/src/lib.rs

-    /// Takes in a description of a computation (specified in R1CS constraints),
-    /// and samples proving and verification keys for that circuit.
-    fn circuit_specific_setup<C: ConstraintSynthesizer<F>, R: RngCore + CryptoRng>(
-        circuit: C,
-        rng: &mut R,
-    ) -> Result<(Self::ProvingKey, Self::VerifyingKey), Self::Error>;
-


I moved this algorithm to CircuitSpecificSetupSNARK; it doesn't really belong here.

Pratyush · 2021-04-20T01:19:19Z

snark/src/lib.rs

 }

 /// A helper type for universal-setup SNARKs, which must infer their computation
 /// size bounds.
-pub enum UniversalSetupIndexError<Bound, E> {
+pub enum IndexingError<Bound, E> {


Indexing is an operation that happens only for UniversalSetupSNARKs, so we don't need to specify that separately here.

Pratyush · 2021-04-20T01:19:33Z

snark/src/lib.rs

    /// Specifies how to bound the size of public parameters required to
    /// generate the index proving and verification keys for a given
    /// circuit.
-    type ComputationBound: Clone + Default + Debug;
+    type IndexBound: Clone + Default + Debug;


Slightly more descriptive name.

Pratyush · 2021-04-20T01:19:56Z

snark/src/lib.rs

    /// Specifies the type of universal public parameters.
    type PublicParameters: Clone + Debug;

+    /// Specifies the bound size that is necessary and sufficient to 
+    /// generate public parameters for `index`.
+    fn bound_for_index(index: &R::Index) -> Self::IndexBound;


New API that enables computing the bound for a given index.

Pratyush · 2021-04-20T01:20:22Z

snark/src/lib.rs

        pp: &Self::PublicParameters,
-        circuit: C,
-        rng: &mut R,


Indexing is now deterministic, as per the formal definition.

Pratyush · 2021-04-20T01:20:55Z

snark/src/lib.rs

+impl<R: NPRelation, S> CircuitSpecificSetupSNARK<R> for S
+where
+    S: UniversalSetupSNARK<R> 
+{


Every UniversalSetupSNARK is also a CircuitSpecificSetupSNARK, so I added this blanket impl here.

Pratyush · 2021-04-20T01:21:59Z

snark/src/r1cs.rs

@@ -0,0 +1,70 @@
+use ark_relations::r1cs::{R1CS, ConstraintSynthesizer};


These are specialized APIs for working with the existing ConstraintSystem-based API

Instead of having these segregated APIs, another idea would be to add a RelationProvider trait that is a generalization of RelationProvider.

pub trait RelationProvider<R: Relation> { // These are all optional because they might not exist always. // For example, in indexing phase, there is no assignment, // while during proving there may be no index. fn index(&self) -> Option<R::Index>; // Maybe this should be `Rc<RefCell<R::Index>>`? to avoid copying. fn instance(&self) -> Option<R::Instance>; fn witness(&self) -> Option<R::Witness>; } pub trait SNARK<R: Relation> { fn prove<RP: RelationProvider<R>, ...>(pk, rp: RP) // similar for other methods. } impl<F: Field> RelationProvider<R1CS<F>> for ConstraintSynthesizer<F> { // do stuff. }

Pratyush · 2021-04-20T01:31:45Z

snark/src/r1cs.rs

+    /// specified as R1CS constraints.
+    fn verify_with_processed_vk<C: ConstraintSynthesizer<F>>(
+        circuit_pvk: &Self::ProcessedVerifyingKey,
+        public_input_generator: C,


The verifier now takes in a ConstraintSynthesizer that outputs the assignment to instance variables.

Pratyush · 2021-04-20T01:33:07Z

relations/src/r1cs/constraint_system.rs

@@ -20,7 +20,7 @@ use ark_std::{
 // TODO: Think: should we replace this with just a closure?
 pub trait ConstraintSynthesizer<F: Field> {


Probably we should rename this trait and add new methods like generate_instance_assignment and generate_witness_assignment

Alternatively, we could replace the trait with three closures that do these tasks, as the TODO suggests.

snark/src/r1cs.rs

snark/src/lib.rs

snark/src/r1cs.rs

ValarDragon · 2021-04-20T21:26:52Z

snark/src/r1cs.rs

+    /// Generate inputs for the SNARK prover from [`cs`].
+    fn prover_inputs<CS: ConstraintSynthesizer<F>>(cs: &CS) -> (Instance<F>, Witness<F>);
+
+    /// Generate inputs for the SNARK verifier from [`cs`].


Suggested change

/// Generate inputs for the SNARK verifier from [`cs`].

/// Generate inputs for the SNARK verifier from [`cs`].

/// Assumes the verifier has a version of the matrices from elsewhere.

/// (Either preprocessed, or a direct copy of the matrices)

snark/src/r1cs.rs

ValarDragon

LGTM! This seems like a significant improvement to the interfaces here

ValarDragon · 2021-04-20T21:28:52Z

Accidentally just reviewed the last few commits lol

relations/src/lib.rs

relations/src/r1cs/mod.rs

ValarDragon · 2021-04-20T21:34:12Z

relations/src/r1cs/mod.rs

+}
+
+/// An R1CS instance consists of variable assignments to the instance variables.
+/// The first variable must be assigned a value of `F::one()`.


I wonder if we should do the converse, and make the second variable onwards what goes into the struct?

what do other sources of R1CS do, like zkinterface?

relations/src/r1cs/mod.rs

Pratyush · 2021-04-21T07:55:53Z

Updated the PR further, I think it's now in the final form and is ready for merging as is (modulo the CHANGELOG)

snark/src/lib.rs

Co-authored-by: Dev Ojha <[email protected]>

jon-chuang · 2021-04-25T15:23:52Z

So the Relation trait is pretty generic, and allows for a generic Snark interface. Which is nice.

So I'm wondering if it's a good time to try to do, or at least plan for, a draft migration of the halo2 code, although @daira seemed to be saying its not exactly ready for that. But from my perspective, it maybe would help put an extra pair of eyes on zcash's code and help arkworks advance to being yet more featured and flexible.

I'll post an issue soon regarding an approach, also outlining potential friction areas and incompatibilities, and ideas for how to solve them.

I'll also try to see if there are any tweaks that could be made regarding this PR.

I'm also wondering if relatedly, a common "plonk description language" ought to be established to make at the very least, relations, which can consist of multiple sub-relations, compatible between various libraries. The description language can also contain meta data, such as subcircuit names and component names, or not.

If R1CS is a general-purpose processor, plonk/plookup is a sort of FPGA/ASIC, with reuseable components, lookup tables, wiring, fan-in/fan-out and locality constraints, custom gates. The glue that holds it all together is the wiring (permutation argument). There are also subtle tradeoffs between the maximum relation multiplicative depth and proof size/proving time.

Just like the differences between programming a CPU and programming an FPGA are huge, there is a need to manage, and hide, a lot more complexity when it comes to Plonk/Plookup.

relations/src/lib.rs

relations/src/r1cs/constraint_system.rs

weikengchen · 2021-06-04T00:32:27Z

When should we merge this one (and plan for the next/next release)? And how many code changes to the other repos should we expect?

Pratyush · 2021-06-04T00:50:14Z

I still have to make corresponding PRs for Groth16 and Marlin; let's push merging this until after 0.3 (so in 0.4).

Pratyush commented Apr 20, 2021

View reviewed changes

relations/src/r1cs/constraint_system.rs Outdated Show resolved Hide resolved

Pratyush commented Apr 20, 2021

View reviewed changes

Pratyush requested review from ValarDragon and weikengchen April 20, 2021 01:22

Pratyush commented Apr 20, 2021

View reviewed changes

weikengchen reviewed Apr 20, 2021

View reviewed changes

snark/src/r1cs.rs Outdated Show resolved Hide resolved