webb-tools / zero-knowledge-gadgets

Zero-knowledge gadgets for Webb's cross-chain blockchain applications.
Apache License 2.0
89 stars 29 forks source link

[TEMPORARY SPEC] PLONK inputs API #153

Closed lazovicff closed 2 years ago

lazovicff commented 2 years ago

In this spec we are making a recommendation for updating the PLONK API regarding private/public/constant inputs.

This is inspired by the Arkworks R1CS API:

We will have an enum with variants representing which type of input do we want:

enum InputType {
    Public,
    Private,
    Constant
}

We would modify the add_input (on StandardComposer) to accept the InputType as an argument:

fn add_input(&self, value: F, type: InputType);

Inside the add_input different functions will be called to support these types. 1) For InputType::Private, the logic remains the same 2) For InputType::Public, we will call underlying function: constrain_to_public 3) For InputType::Constant, we will call underlying function: constrain_to_constant

drewstone commented 2 years ago

Is this meant to be presented to the PLONK repo or for our own use?

drewstone commented 2 years ago

Did you consider the following?

enum InputType<F> {
    Public(F),
    Private(F),
    Constant(F)
}
fn add_input(&self, value: InputType<F>);
lazovicff commented 2 years ago

This will be presented to PLONK people, once we land on a good spec. But yeah, that sound good, keep the suggestions coming in. @GhostOfGauss also

lazovicff commented 2 years ago

Actually, this might be tricky to implement, for example here: https://github.com/webb-tools/arkworks-gadgets/blob/master/arkworks-plonk-circuits/src/merkle_tree/mod.rs#L28-L48

We are passing the native path, which field values are not wrapped in InputType(F)

The idea was to pass InputType into from_native so that you could control how these values are inserted into the circuit from a higher (circuit) level

GhostOfGauss commented 2 years ago

Okay... I started thinking about this on the train and writing down my thoughts and the thing kinda grew into a book lol 

Feel no pressure to read all this, I can probably explain it in a call more quickly. I'll just put it here in case tho

GhostOfGauss commented 2 years ago

After some thought I’m not sure that having an InputType makes sense. I think that the key question is this: What should a wire ever carry aside from a secret?

I thought a bit about what public inputs/constants are potentially used for in circuits. I think the only two purposes they serve are:

Either way, the public input/constant value is consumed by some gate and so that gate ought to be its point of entry into the circuit. I was unable to think of a situation in which it made sense to carry the public input/constant into the gate on a wire. In other words, wires are only for secrets.

I think that we got thrown off track by the add_witness_to_circuit_description method, which I’ve come to believe just simply should not exist because it puts a publicly known constant on a wire to feed that constant into the circuit. This costs a gate and is unnecessary because that constant could have been manually entered into the gate itself.

So I’m not sure that the plonk library is missing InputType. I think what it’s probably missing is more custom gates. For example, we have assert_equal. This constrains two wires to carry equal values. If we go with my claim that wires should only carry secrets then this gate is appropriate only for asserting the equality of two secret values.

If you want to assert that a wire carries some publicly known value like a Merkle root then you should use a gate like assert_equal_to_public_input(secret: Variable, public_input: F) . This gate would do something like composer.poly_gate( secret, secret, self.zero_var, F::zero(), -F::one(), F::zero(), F::zero(), F::zero(), public_input) (I know that’s ugly, that’s just a gate that imposes the constraint secret + public_input = 0 ).

Similarly, there would be a gate for asserting that a wire carries some public constant. Like if the wire carries the output variable of a tree membership check then you’d want to constrain that value to 1. There should be a gate assert_equal_to_constant that does this. (It would differ slightly from the above gate in that assert_equal_to_public_input will require prover/verifier to provide a public input value, whereas the assert_equal_to_constant gate has this value hard-coded to the circuit.)

A related functionality would be optimizing matrix multiplication in plonk. The way that I wrote matrix multiplication in my poseidon implementation is inefficient because it handles 1 multiplication per gate, when in fact a gate is capable of handling 3 at a time. Moreover, I used a wire to carry each constant to the gate. This is again a place where the operation should be coded in such a way as to have these constants enter the circuit directly in a gate as the selector values (q_l, q_r, etc).

So getting back to the problematic from_native:

Maybe the issue here goes all the way back to the PoseidonParametersVar struct. In this struct we decided to carry a bunch of constants along wires, when it would be more efficient to keep encoding them with the PoseidonParameters struct and use these values directly in gates like the ones I mentioned above. Concretely that would mean a gate that adds round constants to the state vector (treating constants as constants and secrets as secrets) and a function for matrix multiplication that again treats the matrix entries properly as constants instead of variables. I think this is something I could take care of reasonably quickly.

Then the from_native method would no longer be needed because the parameters for Poseidon implementing FieldHasher are the same as those of PoseidonGadget implementing FieldHasherGadget.

lazovicff commented 2 years ago

I think we had similar conversations before, that's why I added constrain_to_public and constrain_to_constant (although names could be better) which are equivalent to assert_equal_to_constant and assert_equal_to_public_input.

Adding InputType in add_inputs was purely to match the style from R1CS gadgets, but I'm hearing more and more that this is not desired in PLONK.

We also talked about Poseidon optimizations, but decided to wait it out until the PLONK library supports it.

So, to not end up somewhere in between, maybe we should focus on: 1) Implement optimized version of Poseidon 2) Ditch R1CS-like from_native, and pass values directly to assert_equal_to_constant and assert_equal_to_public

before continuing with our circuits?

lazovicff commented 2 years ago

(Did more thinking on this over the weekend)

I wouldn't limit public inputs/constants to specified use cases.

Also, if we ditch from_native, and create public inputs/constants inside the implementation, we would have to hard code it. For example, for set gadget, self.set should be configurable to be either public or private (or maybe even constants in rare cases)

lazovicff commented 2 years ago

We decided to ditch the r1cs-like style and handle things differently. Every gadget should have different implementation for different kinds of inputs. For example, this is what the struct should look like for public/constant inputs:

struct SetGadget<F: PrimeField> {
    set: Vec<F>
}

impl<F: PrimeField> SetGadget<F> {
    pub fn new(set: Vec<F>) -> Self {
        Self { set }
    }
}

This is how it should be for private inputs:

struct SetGadget {
    set: Variable
}

impl<F: PrimeField> SetGadget {
    pub fn from_native(composer: StandardComposer<F, P>, set: Vec<F>) -> Self {
        let set_private = Vec::new();
        for item in set {
            let var = composer.add_input(item);
            set_private.push(var);
        }
        Self {
            set: set_private
        }
    }
}

This means we should remove from_native from our trait requirements, and keep everything else as is.

Things to update:

Still there are proposals to PLONK library that we want to make, namely adding these two functions: assert_equal_to_constant, assert_equal_to_public_input