PI handling & Circuit Builder trait refactor #423

CPerezz · 2021-03-03T13:08:14Z

As it was mentioned in the latest iteration, for plonk v0.6 we wanted to improve the API that the Circuit trait was exposing and forcing users to implement as well as find better ways of handling the public inputs.

Finally with this PR, we've been able to close some issues related to the problems mentioned above. Here is a summary of the problems that the PR addresses:

Public Inputs were being stored inside of the Composer in a dense way. While we're still not sure that storing them inside is the right thing, we know that is helpful to have them for quick testing and also as a support for the Circuit trait public inputs management. Therefore, we're now storing the sparse repr of the Values and positions in a BTreeMap instead. Closes Store Public Inputs inside of the Composer on a sparse way #427
We've changed the way on which the trim size and the transcript initialization label are handled. trim_size is mostly extracted from the keys but also sent as parameter to the prove and compile functions. As for the label it is always a parameter of proof and verify. Closes Remove set_trim_size from Circuit #351. Closes Remove trim_size related fns for circuit and use trait associated constants istead + min_const_generics #389
We've introduced a new type that unifies the Public Inputs PublicInputValue which is responsible of also implement the conversions from the Public Input types to the BlsScalar "lingua franca". Closes Create a new struct for the Circuit Builder in order to serialize the Public Input position #416
We've also refactored the API and the usage that the users need to do inside their Circuit implementations by abstracting the PI management and moving it inside of the `Composer. Closes Take profit of the Composer and use it as a backend manager of the Circuit Public Inputs #428
Circuit::verify_proof() no longer calls gadget() inside which was unnecessary and a waste of time and resources. Closes Circuit::verify_proof() should not call gadget inside of it #426
Since these two functions didn't need Self or self and also were independent, we've been able to remove them from the trait. Also allows us to have a generic method that can verify Proofs of any Circuit without needing to have access to the type of it. Closes Circuit builder - verify proof should not require &self #396

vlopes11

Few nits

vlopes11 · 2021-03-03T20:38:46Z

src/constraint_system/arithmetic.rs

@@ -74,7 +74,9 @@ impl StandardComposer {
        self.q_fixed_group_add.push(BlsScalar::zero());
        self.q_variable_group_add.push(BlsScalar::zero());

-        self.public_inputs.push(pi);
+        if let Some(pi) = pi {
+            assert!(self.public_inputs_sparse_store.insert(self.n, pi).is_none());


I believe we should go in a direction to reduce the number of assertions inside plonk functions, not increase it. If the PI must fail in this case, then the function should return a Result

It's unclear to me the reason why we have such assertion, so I cannot judge it properly.
So, first of all I would add a comment about why this is needed.
Basically it should clarify:

Why assert! is needed?

Why assert! instead of debug_assert!?

If we're going to keep the it, we also should add a message so if it fails we know what failed and why.

This should never happen. That's why it has an assert! it's like unreachable!() for me here. I just have it so that if at some point this assertion fails. We know that we made a change in a really wrong direction.

vlopes11 · 2021-03-03T20:39:02Z

src/constraint_system/arithmetic.rs

@@ -153,7 +155,9 @@ impl StandardComposer {
        self.q_fixed_group_add.push(BlsScalar::zero());
        self.q_variable_group_add.push(BlsScalar::zero());

-        self.public_inputs.push(pi);
+        if let Some(pi) = pi {
+            assert!(self.public_inputs_sparse_store.insert(self.n, pi).is_none());


Unnecessary assertion, check above

Same as mentioned before.

vlopes11 · 2021-03-03T20:42:13Z

src/constraint_system/composer.rs

+        self.public_inputs_sparse_store
+            .keys()
+            .map(|pos| *pos)
+            .collect::<Vec<usize>>()


Unnecessary type annotation

vlopes11 · 2021-03-03T20:42:35Z

src/constraint_system/composer.rs

@@ -206,7 +229,9 @@ impl StandardComposer {
        self.q_fixed_group_add.push(BlsScalar::zero());
        self.q_variable_group_add.push(BlsScalar::zero());

-        self.public_inputs.push(pi);
+        if let Some(pi) = pi {
+            assert!(self.public_inputs_sparse_store.insert(self.n, pi).is_none());


Unnecessary assertion, check above

Same comment I made in the previous one.

vlopes11 · 2021-03-03T20:44:34Z

src/circuit_builder.rs

+#[cfg_attr(feature = "canon", derive(Canon))]
+/// Structure that represents a PLONK Circuit Public Input
+/// structure converted into it's &[BlsScalar] repr.
+pub struct PublicInputValue(pub(crate) Vec<BlsScalar>);


The name suggests its a single public input when its in fact a collection

I think is correct: A single public input value is defined by one or more BlsScalar.

And in fact it is a Public Input. But expressed on it's BlsScalar form. And as you know. Since we have 3 types that are Public Inputs and one of them (AffinePoint) gets represented as 2 BlsScalar instead of one.

Therefore the best way we've found with @ZER0 for now is to return a Vec<BlsScalar> which will be 1 item for BlsScalar and JubJubScalar and 2 for AffinePoint.

Before it was an enum. Do we have a gain to convert from enum to this Vec structure? Also, if we know the size to be either 1 or 2, any of these simpler structures will also do:

(BlsScalar, Option<BlsScalar>)

[Option<BlsScalar>; 2]

[BlsScalar; 2]

But Vec seemed much simpler than that. The only one that I might adopt is the Array. But is more missleading since makes it look like it's always 2 arrays instead of one.

As alternative we could use const generic also here, you can play with this if you want and see how the API looks like.

vlopes11 · 2021-03-03T20:45:09Z

src/circuit_builder.rs

-                bytes
-            }
-        }
+impl From<BlsScalar> for PublicInputValue {


Why should we reconstruct a collection of public inputs from a single scalar?

It's not a collection of public input, is a single public input. Maybe we could add a more descriptive documentation.

Its confusing to have a Vec and not expect a variable size collection

vlopes11 · 2021-03-03T20:45:28Z

src/circuit_builder.rs

-                _ => unreachable!(),
-            }
-        }
+impl From<JubJubScalar> for PublicInputValue {


Single scalar should not result in a collection; check above

In this case yes. Basically a PublicInputValue in our case can be one or more (currently two) BlsScalar. But it doesn't matter the underling representation, the entity is still a single PublicInputValue.

vlopes11 · 2021-03-03T20:45:45Z

src/circuit_builder.rs

-            PublicInput::JubJubScalar(_, pos) => [*pos, 0],
-            PublicInput::AffinePoint(_, pos_x, pos_y) => [*pos_x, *pos_y],
-        }
+impl From<JubJubAffine> for PublicInputValue {


Single point should not result in a collection, check above

Actually, yes. A single point can be decoded as two BlsScalar (x and y in this case).

vlopes11 · 2021-03-03T20:46:37Z

src/circuit_builder.rs

+    /// Initialization string used to fill the transcript for both parties.
+    const TRANSCRIPT_INIT: &'static [u8];
+    /// Trimming size for the keys of the circuit.
+    const TRIM_SIZE: usize;


TRIM_SIZE can't be constant for a circuit. One example is the transfer circuit that has variable circuit size depending on the number of inputs

I asked the same question today, if it could be a constant or not. Maybe @CPerezz was misleaded by #351 ? I don't know. Maybe tomorrow morning at the stand up we should discuss this point.

I thought about it when I was implementing it.
What if we pass it with a const generic? SO that the trim size also constrains the type @ZER0 @vlopes11 ?

Let's talk about it tomorrow morning. It's crucial to fully understand the edge case of transfer circuit, it's the only anomaly there.

#351 is only about the set_trim_size that should never be called, as in the proposal inside the issue

Proposal: remove Circuit::set_trim_size

However, the get_trim_size (or any other method name with the same effect) will return the current size for that circuit. There is the "con" to tie the circuit instance to a fixed size trim without any clear "pro"

vlopes11 · 2021-03-03T20:46:58Z

src/circuit_builder.rs

+        pub_input_pos: &PublicInputPositions,
+    ) -> Vec<BlsScalar> {
+        let mut pi = vec![BlsScalar::zero(); Self::TRIM_SIZE];
+        println!("{:?}", pub_input_pos);


Remove println

ZER0

Few things to fix, but overall is good.

ZER0 · 2021-03-03T21:31:42Z

src/constraint_system/arithmetic.rs

@@ -74,7 +74,9 @@ impl StandardComposer {
        self.q_fixed_group_add.push(BlsScalar::zero());
        self.q_variable_group_add.push(BlsScalar::zero());

-        self.public_inputs.push(pi);
+        if let Some(pi) = pi {
+            assert!(self.public_inputs_sparse_store.insert(self.n, pi).is_none());


It's unclear to me the reason why we have such assertion, so I cannot judge it properly.
So, first of all I would add a comment about why this is needed.
Basically it should clarify:

Why assert! is needed?

Why assert! instead of debug_assert!?

If we're going to keep the it, we also should add a message so if it fails we know what failed and why.

ZER0 · 2021-03-03T21:31:56Z

src/constraint_system/arithmetic.rs

@@ -153,7 +155,9 @@ impl StandardComposer {
        self.q_fixed_group_add.push(BlsScalar::zero());
        self.q_variable_group_add.push(BlsScalar::zero());

-        self.public_inputs.push(pi);
+        if let Some(pi) = pi {
+            assert!(self.public_inputs_sparse_store.insert(self.n, pi).is_none());


Same as mentioned before.

ZER0 · 2021-03-03T21:35:04Z

src/circuit_builder.rs

+#[cfg_attr(feature = "canon", derive(Canon))]
+/// Structure that represents a PLONK Circuit Public Input
+/// structure converted into it's &[BlsScalar] repr.
+pub struct PublicInputValue(pub(crate) Vec<BlsScalar>);


I think is correct: A single public input value is defined by one or more BlsScalar.

ZER0 · 2021-03-03T21:35:53Z

src/circuit_builder.rs

-                bytes
-            }
-        }
+impl From<BlsScalar> for PublicInputValue {


It's not a collection of public input, is a single public input. Maybe we could add a more descriptive documentation.

ZER0 · 2021-03-03T21:37:05Z

src/circuit_builder.rs

-                _ => unreachable!(),
-            }
-        }
+impl From<JubJubScalar> for PublicInputValue {


In this case yes. Basically a PublicInputValue in our case can be one or more (currently two) BlsScalar. But it doesn't matter the underling representation, the entity is still a single PublicInputValue.

ZER0 · 2021-03-03T22:13:11Z

src/constraint_system/composer.rs

+            .keys()
+            .map(|pos| *pos)


Suggested change

.keys()

.map(|pos| *pos)

.keys()

.copied()

ZER0 · 2021-03-03T22:18:12Z

src/constraint_system/arithmetic.rs

+            + (q_r * b_eval)
+            + (q_4 * d_eval)
+            + q_c
+            + pi.unwrap_or(BlsScalar::zero());


Suggested change

+ pi.unwrap_or(BlsScalar::zero());

+ pi.unwrap_or_default();

or:

Suggested change

+ pi.unwrap_or(BlsScalar::zero());

+ pi.unwrap_or_else(BlsScalar::zero);

ZER0 · 2021-03-03T22:21:55Z

src/constraint_system/arithmetic.rs

@@ -268,7 +276,8 @@ impl StandardComposer {
        let a_eval = self.variables[&a];
        let b_eval = self.variables[&b];
        let d_eval = self.variables[&d];
-        let c_eval = (q_m * a_eval * b_eval) + (q_4 * d_eval) + q_c + pi;
+        let c_eval =
+            (q_m * a_eval * b_eval) + (q_4 * d_eval) + q_c + pi.unwrap_or(BlsScalar::zero());


As mentioned in the comment above.

ZER0 · 2021-03-03T22:24:48Z

src/constraint_system/composer.rs

+        self.public_inputs_sparse_store
+            .keys()
+            .zip(self.public_inputs_sparse_store.values())
+            .for_each(|(pos, value)| {
+                pi[*pos] = *value;
+            });


You're zipping yourself. :) It make sense when you have the keys and the values on two different objects, but here the object is just one. You should be able just to iterate it to get the entries (a tuple (key, value)).

ZER0 · 2021-03-03T22:25:17Z

src/constraint_system/composer.rs

@@ -206,7 +229,9 @@ impl StandardComposer {
        self.q_fixed_group_add.push(BlsScalar::zero());
        self.q_variable_group_add.push(BlsScalar::zero());

-        self.public_inputs.push(pi);
+        if let Some(pi) = pi {
+            assert!(self.public_inputs_sparse_store.insert(self.n, pi).is_none());


Same comment I made in the previous one.

vlopes11

LGTM after the nits pointed by @ZER0 are addressed.

Looking forward to this merge; will help a lot the circuits development

It wasted a lot of time and also was useless to call the `gadget` fn again for the verify fn in the Circuit trait. Thanks to the refactor done to the PublicInputs and how they're handled now we no longer need to call it.

Previously we were storing the dense representation of the public inputs that were used with the `Composer` instance. This was constly, since most of the public inputs were zero and therefore storing them was useless but also we needed to collect the position information in the Circuit trait in such difficult ways. With this refactor we currently have: - Public Inputs are now passed to gate-functions as `Option<BlsScalar>`. - The `Composer` no longer stores the dense public input vector. Instead we store two sparse vectors which contain the values and the positions. - We include a function that allows the `Composer` to construct the dense PI vector from it's sparse ones. - We've added ways for the consumer to get access to the positions vector. This will allow to simplify the public input management on the `Circuit` trait.

@ZER0

After discussions with @ZER0 we realized there's not a need to have a really big trait to handle the public inputs and all we need is a simple structure that implements the conversions from the different PI types that we can have in PLONK circuits into the format that PLONK requires which is `&[BlsScalar]`. - Created `PublicInputValue` which is implemented for `BlsScalar`, `JubJubScalar` and `JubJubAffine`. - Forced the `Circuit` trait to use this struct as source of the public input values needed in the verification step.

Instead of storing the sparse representation of the non-zero PublicInputs as two different `Vec` which are logically connected but not tecnollogically(code-wise). With this change we can have them strictly correlated since they're stored linked in the `HashMap`.

We need to return the PI positons(and store them) in order since otherways when consumers ask for these data, it would be returned wrongly if we don't return it ordered.

This also allows to test wether the support for the PI handling of the Circuit trait is correct.

To allow (although it's not recommended to do) variable-size circuits, we need a way to express different circuit sizes (TRIM_SIZE) for the same circuit. By setting the `Circuit::TRIM_SIZE` inherit the value from the const generic parameter N, we enable this behaviour leaving an API that is not bad.

Since these two functions didn't need `Self` or `self` and also were independent, we've been able to remove them from the trait. This closes #396 and also allows us to have a generic method that can verify Proofs of any `Circuit` without needing to have access to the type of it.

CPerezz added the team:Core Low Level Core Development Team (Rust) label Mar 3, 2021

CPerezz changed the title ~~[WIP] PI handling & Circuit Builder trait refactor~~ PI handling & Circuit Builder trait refactor Mar 3, 2021

CPerezz marked this pull request as ready for review March 3, 2021 18:18

CPerezz requested review from vlopes11, ZER0 and LukePearson1 and removed request for vlopes11 March 3, 2021 18:51

CPerezz changed the base branch from master to release-0.6 March 3, 2021 19:29

vlopes11 requested changes Mar 3, 2021

View reviewed changes

CPerezz requested a review from vlopes11 March 3, 2021 22:03

ZER0 requested changes Mar 3, 2021

View reviewed changes

CPerezz requested a review from ZER0 March 4, 2021 08:49

vlopes11 approved these changes Mar 4, 2021

View reviewed changes

CPerezz force-pushed the circ_builder_refactor branch from 13769e4 to d2358f7 Compare March 4, 2021 15:45

CPerezz added 15 commits March 4, 2021 16:46

Move PI to separated file

403dc05

Include TRANSCRIPT_INIT & TRIM_SIZE consts for trait

7ece4c4

Add PiPositionsHolder trait as Circuit supertrait

0dfcbaa

Remove gadget call from Circuit::verify() fn

8fd34cb

It wasted a lot of time and also was useless to call the `gadget` fn again for the verify fn in the Circuit trait. Thanks to the refactor done to the PublicInputs and how they're handled now we no longer need to call it.

Use extend instead of append to reduce allocations

b2d12df

Declare zeros array only once

560e528

Implement bytes-PI conversion trait

9db99c9

Use BTreeMap instead of HashMap

c067ce0

We need to return the PI positons(and store them) in order since otherways when consumers ask for these data, it would be returned wrongly if we don't return it ordered.

Extend circuit tests to use ECC ops

43531ac

This also allows to test wether the support for the PI handling of the Circuit trait is correct.

Remove prints

3f1de47

Address reveiw comments

626bbb4

CPerezz added 9 commits March 4, 2021 16:46

Use copied() in iters instead of *

46166aa

Update README.md with code examples

fd77f02

Change circuit module name

479711b

Check invariants for pi BTreeMap insertion

62cd93c

Impl padded_circuit_size for VerifierKey

88e74cc

Update CHANGELOG

5a3171b

Address @ZER0's PR suggestions

62176dc

Add canonical_derive as dep behind canon feature

d2358f7

CPerezz mentioned this pull request Mar 4, 2021

Remove trim_size related fns for circuit and use trait associated constants istead + min_const_generics #389

Closed

CPerezz merged commit 0efaed6 into release-0.6 Mar 4, 2021

CPerezz deleted the circ_builder_refactor branch March 4, 2021 15:56

autholykos mentioned this pull request Mar 5, 2021

Transfer Contract circuit panics during the host verification dusk-network/rusk#229

Closed

	+ pi.unwrap_or(BlsScalar::zero());
	+ pi.unwrap_or_else(BlsScalar::zero);

PI handling & Circuit Builder trait refactor #423

PI handling & Circuit Builder trait refactor #423

Conversation

CPerezz commented Mar 3, 2021 • edited

vlopes11 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZER0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlopes11 left a comment

Choose a reason for hiding this comment

CPerezz commented Mar 3, 2021 •

edited