Circuit improvements #198

enricobottazzi · 2023-11-28T16:56:41Z

Subject of this PR:

Apply audit fixes Add circuit audit comments #191
Apply improvement on range checks ZK Proof of Inclusion Efficiency improvement #167

On the last part there's still two missing things:

Enforcing that no overflow can happen inside the circuit, based on the chosen parameters N_BYTES and LEVELS on the smart contract level. (this is better described in the issue) cc: @alxkzmn
Update the benchmarks. For now the benchmarks are broken due to the new csv format. I opened an issue for that (Fix csv files folder for Benchmark #197). I think we can still go on and merge this PR and update the benchmarks later @sifnoc

enricobottazzi · 2023-11-29T08:24:27Z

@alxkzmn to add possibility of recording the generic parameters of the [MstInclusionCircuit](https://github.com/summa-dev/summa-solvency/blob/master/zk_prover/src/circuits/merkle_sum_tree.rs#L29) on the Summa contract.
@enricobottazzi to create a script to verify that the combination circuit + generic parameters + trusted setup leads to the same smart contract

enricobottazzi · 2023-11-30T07:20:11Z

@alxkzmn thanks for sorting this out! I think that the value that you hardcoded for the merkle tree levels is not correct.

These are the parameters that we are using for building the verifier (https://github.com/summa-dev/summa-solvency/blob/circuit-improvement-enrico/zk_prover/examples/gen_inclusion_verifier.rs).

Can you update them, adding N_ASSETS as well and add in the comments a link to the script that we use to generate such verifier contract

alxkzmn · 2023-12-01T05:41:22Z

zk_prover/src/chips/merkle_sum_tree.rs

@@ -20,7 +20,7 @@ pub struct MerkleSumTreeConfig {
 /// Chip that performs various constraints related to a Merkle Sum Tree data structure such as:
 ///
 /// * `s * swap_bit * (1 - swap_bit) = 0` (if `bool_and_swap_selector` is toggled). It basically enforces that swap_bit is either a 0 or 1.
-/// * `s * (swap_bit * 2 * (elelment_r_cur - elelment_l_cur)  - (elelment_l_next - elelment_l_cur) - (elelment_r_cur - elelment_r_next)) = 0`. Enforces that if the swap_bit is equal to 1, the values will be swapped on the next row (if `bool_and_swap_selector` is toggled).
+/// * `s * (swap_bit * 2 * (element_r_cur - element_l_cur)  - (element_l_next - element_l_cur) - (element_r_cur - element_r_next)) = 0`. Enforces that if the swap_bit is equal to 1, the values will be swapped on the next row (if `bool_and_swap_selector` is toggled).


The description is outdated

sifnoc

Looks good to me! but I left small thing what i found.

sifnoc · 2023-11-30T09:41:40Z

zk_prover/src/circuits/traits.rs

@@ -30,4 +30,24 @@ pub trait CircuitBase {
            |mut region| region.assign_advice(|| "value", advice_col, 0, || Value::known(value)),
        )
    }
+
+    /// /// Loads the lookup table with values from `0` to `2^8 - 1`


Duplicated ///

sifnoc · 2023-11-30T09:48:58Z

zk_prover/src/circuits/traits.rs

+
+    /// /// Loads the lookup table with values from `0` to `2^8 - 1`
+    fn load(&self, layouter: &mut impl Layouter<Fp>, column: Column<Fixed>) -> Result<(), Error> {
+        let range = 1 << (8);


May not need parentheses for 8

enricobottazzi · 2023-12-01T08:38:34Z

Thanks @sifnoc, applied changes

alxkzmn · 2023-12-01T09:10:18Z

zk_prover/src/circuits/traits.rs

@@ -30,4 +30,24 @@ pub trait CircuitBase {
            |mut region| region.assign_advice(|| "value", advice_col, 0, || Value::known(value)),
        )
    }
+
+    /// Loads the lookup table with values from `0` to `2^8 - 1`
+    fn load(&self, layouter: &mut impl Layouter<Fp>, column: Column<Fixed>) -> Result<(), Error> {


Technically this is not a problem, but this method is very specific to the implementation of our MST. It makes CircuitBase not a common trait anymore but a specific API for this case, so either naming and description don't correspond to the function anymore, or the method should be inside the MST circuit implementation.

I guess the trait is a leftover from the time when we had two circuits

* chore: apply clippy fixes * fix: audit comment 1 - Lookup table lives outside of `RangeCheckChip` * fix: audit comment 2 - `swap_constraint` * feat: optimize number of balances to be range checked * chore: update `InclusionVerifier` contract * chore: comment * Record circuit config parameters in the contract * Fix contract deployment parameters * Fix typo * chore: rename to `assetsCount` * chore: update backend * chore: fix comments * chore: minor changes --------- Co-authored-by: Alex Kuzmin <[email protected]>

enricobottazzi added 5 commits November 28, 2023 15:43

chore: apply clippy fixes

76d4b5e

fix: audit comment 1 - Lookup table lives outside of RangeCheckChip

08468e9

fix: audit comment 2 - swap_constraint

b2803a9

feat: optimize number of balances to be range checked

454cfb3

chore: update InclusionVerifier contract

17058be

enricobottazzi changed the base branch from master to v1-improvements-and-consolidation November 28, 2023 16:57

enricobottazzi requested a review from alxkzmn November 28, 2023 16:57

This was linked to issues Nov 28, 2023

ZK Proof of Inclusion Efficiency improvement #167

Closed

Apply Circuit Audit Fixeing #195

Closed

chore: comment

ecfd746

Record circuit config parameters in the contract

a9d7c9b

alxkzmn and others added 4 commits November 30, 2023 16:35

Fix contract deployment parameters

f7fbd1a

Fix typo

48f975d

chore: rename to assetsCount

3efd2d7

chore: update backend

2a3667c

enricobottazzi requested a review from sifnoc November 30, 2023 08:55

enricobottazzi marked this pull request as ready for review November 30, 2023 08:55

alxkzmn reviewed Dec 1, 2023

View reviewed changes

chore: fix comments

eabeb6b

sifnoc approved these changes Dec 1, 2023

View reviewed changes

chore: minor changes

65bd0ec

enricobottazzi requested a review from alxkzmn December 1, 2023 08:38

alxkzmn reviewed Dec 1, 2023

View reviewed changes

alxkzmn approved these changes Dec 1, 2023

View reviewed changes

enricobottazzi merged commit 0ce4a0d into v1-improvements-and-consolidation Dec 1, 2023
5 checks passed

enricobottazzi deleted the circuit-improvement-enrico branch December 1, 2023 09:15

enricobottazzi mentioned this pull request Dec 1, 2023

Mantainance #202

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Circuit improvements #198

Circuit improvements #198

enricobottazzi commented Nov 28, 2023 •

edited

Loading

enricobottazzi commented Nov 29, 2023

enricobottazzi commented Nov 30, 2023

alxkzmn Dec 1, 2023

enricobottazzi Dec 1, 2023

sifnoc left a comment

sifnoc Nov 30, 2023

sifnoc Nov 30, 2023

enricobottazzi commented Dec 1, 2023

alxkzmn Dec 1, 2023

alxkzmn Dec 1, 2023

Circuit improvements #198

Circuit improvements #198

Conversation

enricobottazzi commented Nov 28, 2023 • edited Loading

enricobottazzi commented Nov 29, 2023

enricobottazzi commented Nov 30, 2023

alxkzmn Dec 1, 2023

Choose a reason for hiding this comment

enricobottazzi Dec 1, 2023

Choose a reason for hiding this comment

sifnoc left a comment

Choose a reason for hiding this comment

sifnoc Nov 30, 2023

Choose a reason for hiding this comment

sifnoc Nov 30, 2023

Choose a reason for hiding this comment

enricobottazzi commented Dec 1, 2023

alxkzmn Dec 1, 2023

Choose a reason for hiding this comment

alxkzmn Dec 1, 2023

Choose a reason for hiding this comment

enricobottazzi commented Nov 28, 2023 •

edited

Loading