Implement Verifiers for KZG and SNARK Proofs in Contracts #267

sifnoc · 2024-02-20T17:41:20Z

Verifier test code is located in contracts/test/Verifiers.ts.
The pre-generated proofs for testing are located at kzg_prover/bin as a JSON file.
- The proofs are generated by these two scripts: gen_commitment.rs and gen_inclusion_proof.rs.
Only the SNARK verifier is generated from the gen_verifier.rs script in kzg_prover.
Retained ProverGWC for further benchmarks on a higher number of currencies.

Todo

Fix the revert error while verifying the Inclusion proof.
Align terminology in verifier comments with that used in other references.
Update README for generating verifer and proofs.

…est script in the kzg_prover

… on Solidity verifier

…uction for generating verifier, commitment and proofs

sifnoc · 2024-02-22T19:59:45Z

I removed revert message in the InclusionVerifier due to the bug in ethers: ethers-io/ethers.js#3605

alxkzmn · 2024-02-23T03:47:44Z

contracts/src/GrandsumVerifier.sol

+    // Memory positions for the verifying key.
+    // The memory location starts at 0x200 due to the maximum operation on the ec_pairing function being 0x180, marking the maximum memory location used
+    uint256 internal constant                VK_MPTR = 0x200;
+    uint256 internal constant         VK_DIGEST_MPTR = 0x200;


VK_DIGEST_MPTR is not used

True, I will address this by copying only the values needed in the verifier from the VerifyingKey.

Fixed at eb067b9

alxkzmn · 2024-02-23T06:41:25Z

contracts/src/Summa.sol

+
+        bytes memory slicedSnarkProof = new bytes(grandsumProof.length);
+        for (uint256 i = 0; i < grandsumProof.length; i++) {
+            slicedSnarkProof[i] = snarkProof[i + 64]; // Skip first 64 bytes, it's not for total balance.


Solidity now supports array slicing, maybe we can try to take advantage of that? https://docs.soliditylang.org/en/v0.6.0/types.html#array-slices

Saved 146,361 gas by applying the array slice to the submitCommitment method. Thanks!
It's updated at eb067b9

Wow, I didn't even expect such an improvement, sounds great!

alxkzmn · 2024-02-23T06:43:20Z

contracts/src/Summa.sol

+
+        require(grandSumVerifier.verifyProof(verifyingKey, combinedProofs, totalBalances), "Invalid grandsum proof");
+
+        // Slice the proof aa length as grandsumProof 


The comment seems to be unrelated to the code

It's removed at 0230050

alxkzmn · 2024-02-23T06:45:23Z

contracts/src/Summa.sol

+        // Slice the proof aa length as grandsumProof 
+        commitments[timestamp] = slicedSnarkProof;
+
+        emit LiabilitiesCommitmentSubmitted(timestamp, totalBalances, slicedSnarkProof);


I think that we can actually put the combined proof into the event as it binds together the range check and the grand sum opening proofs, and also store the combined proof in the commitments mapping.

I agree with including both the snark proof and the grand sum proof in the event. However, I'm unsure about storing the grand sum proof in the commitment mapping. Is there a potential use for it in the future?

Agreed, no need for the grand sum proof in the commitments mapping :)

It's updated at 6db5fb7

alxkzmn · 2024-02-23T06:47:22Z

contracts/src/Summa.sol

+        uint256[] memory challenges,
+        uint256[] memory values
+    ) public view returns (bool) {
+        require(values.length > 1, "Invalid values length");


We can check the exact values length here because we know the cryptocurrencyNames length that was passed in the constructor.

Ah yes, It been fixed at 0230050

contracts/test/Summa.ts

…Proof

alxkzmn · 2024-02-23T07:00:14Z

contracts/test/Summa.ts

    });

-    it("should not submit an invalid commitment", async () => {
+    it("should reject submit an invalid total balances", async () => {


I'd say either "reject" or "not submit"

It's updated at 6db5fb7

alxkzmn · 2024-02-23T07:00:55Z

contracts/test/Summa.ts

    });

-    it("should revert if the caller is not the owner", async () => {
+    it("should reject submit when grandsum proof length mismatches with total balances", async () => {


Same here with reject/submit

It's updated at 6db5fb7

alxkzmn · 2024-02-23T08:01:24Z

contracts/test/Verifiers.ts

+      expect(await snarkVerifier.verifyProof(verifyingKey.address, commitmentCalldata.range_check_snark_proof, [1])).to.be.true;
+    });
+
+    it("should fail to verify snark proof without the number of instances", async () => {


I think this test is not necessary because the instance issue should be fixed by merging the upstream changes.

Okay, I will remove it.

Replaced another test case instead of this at 6db5fb7

alxkzmn · 2024-02-23T08:02:34Z

contracts/test/Verifiers.ts

+      commitmentCalldata = deploymentInfo.commitmentCalldata;
+
+      // InclusionVerifier requires BN256G2 contract for performing elliptic curve operations on G2 subgroup
+      // const bn256g2 = await deployBN256G2();


BN256G2 mention can be removed

It removed at 6db5fb7

alxkzmn · 2024-02-23T08:13:38Z

kzg_prover/bin/gen_commit_and_proofs.rs

+    let mut grand_sums_kzg_proof = Vec::new();
+    for balance_column in 1..(N_CURRENCIES + 1) {
+        let f_poly = advice_polys.advice_polys.get(balance_column).unwrap();
+        let kzg_commitment = commit_kzg(&params, f_poly);


This can be replaced with the committed point taken from the zk-proof transcript.

Yes, Indeed! Thanks for point out!

https://github.com/summa-dev/summa-solvency/pull/268/files#diff-c68e26b4e20170b2d56ef3b1f9675c7b9ed5e2414278ec2edcd90f3fa1331474R99 here's the code that does that

Great tip! It was much helped
It updated at 6db5fb7

alxkzmn · 2024-02-23T08:15:13Z

kzg_prover/bin/gen_commit_and_proofs.rs

+    let mut inclusion_proof: Vec<Vec<u8>> = Vec::new();
+    for column_index in column_range {
+        let f_poly = advice_polys.advice_polys.get(column_index).unwrap();
+        let kzg_commitment = commit_kzg(&params, f_poly);


Same here, the commitment can be taken from the zk-proof transcript.

It updated at 6db5fb7

alxkzmn · 2024-02-23T08:16:55Z

kzg_prover/bin/gen_commit_and_proofs.rs

+struct InclusionProofCallData {
+    proof: String,
+    challenges: Vec<U256>,
+    username: String,


Let's rename "username" to "userId" because it's not a plain-text username but is supposed to be a hash.

Yes, that makes sense. Okay, I will replace 'username' with 'userId'

It updated at 6db5fb7

alxkzmn · 2024-02-23T08:20:36Z

kzg_prover/bin/gen_verifier.rs

@@ -73,6 +72,10 @@ fn main() {
    );
    assert!(result.is_ok());

+    let result_unwrapped = result.unwrap();
+    result_unwrapped.0.expect("prover should not fail");
+    let _advice_polys = result_unwrapped.1.clone();


_advice_polys is not used

It removed at 6db5fb7

alxkzmn

Awesome job! Let's follow the consistent naming pattern for grand sum-related things (separate "grand sum" in text and proper snake/camel case in code, e.g., "grand_sum" and "grandSum", including the file names).

sifnoc · 2024-02-23T11:16:34Z

Awesome job! Let's follow the consistent naming pattern for grand sum-related things (separate "grand sum" in text and proper snake/camel case in code, e.g., "grand_sum" and "grandSum", including the file names).

Thanks, That's great naming idea

alxkzmn · 2024-02-26T04:17:02Z

contracts/test/Verifiers.ts

-    it("should fail to verify snark proof without the number of instances", async () => {
-      await expect(snarkVerifier.verifyProof(verifyingKey.address, commitmentCalldata.range_check_snark_proof, [])).to.be.reverted;
+    it("should revert grand sum proof", async () => {
+      await expect(snarkVerifier.verifyProof(verifyingKey.address, commitmentCalldata.grand_sums_batch_proof, [1])).to.be.reverted;


Why should it revert here?

Because the verifier is for verifying snark proof, not for verifying grand sum proof.

I would rename the test to something like "should revert with invalid proof".

alxkzmn · 2024-02-26T04:27:09Z

contracts/test/Verifiers.ts

+      inclusionProof = inclusionCalldata.proof;
+      userId = inclusionCalldata.user_id;
+      challenges = inclusionCalldata.challenges;
+      userIdBigUint = inclusionCalldata.user_values[0];


Do we still need the userId if we have it in inclusionCalldata.user_values[0]?

Actually 'userId' is not used anywhere in the test code. I will remove it on the test code.
Would it be better to remove it from the csv file as well?

I don't think we can remove it from the csv, it is used in some tests (e.g., test_valid_univariate_grand_sum_full_prover) and should be used in production as well.

alxkzmn

LGTM! Can merge after resolving the conflicts.

…tract test

sifnoc added 10 commits February 8, 2024 05:41

feat: replaced transacript to Keccak256 from Blake2b

55802a0

feat: added solidity snark verifier and generator

0495953

fix: invalid test case for opening verifier

32a44cf

feat: added kzg verifier for grand sum proof

09fdcfd

feat: added kzg verifiers for both grandsum and inclusion in Contracts

0b31129

fix: name convention in ts; test cases; removed unncessary verifier t…

77dbae1

…est script in the kzg_prover

fix: generating inclusion proofs with same params for verifying proof…

ff1bba3

… on Solidity verifier

Merge branch 'v2' into v2-kzg-verifier

8a32fe7

fix: removed unused script in kzg_prover; updated README with a instr…

4b7f49b

…uction for generating verifier, commitment and proofs

feat: added verifying functions and test cases on Summa contract

37bb1bd

sifnoc marked this pull request as ready for review February 22, 2024 19:59

alxkzmn reviewed Feb 23, 2024

View reviewed changes

contracts/test/Summa.ts Show resolved Hide resolved

feat: read snark proof from contract while performing verifyInclusion…

0230050

…Proof

alxkzmn reviewed Feb 23, 2024

View reviewed changes

fix: test case about commitment on contract

46f330c

alxkzmn reviewed Feb 23, 2024

View reviewed changes

alxkzmn requested changes Feb 23, 2024

View reviewed changes

sifnoc added 3 commits February 23, 2024 16:03

fix: following review comments

6db5fb7

fix: gas optimization on verifier contracts

eb067b9

fix: remove 'callStatic' method in test

6a82630

sifnoc requested a review from alxkzmn February 23, 2024 17:09

alxkzmn reviewed Feb 26, 2024

View reviewed changes

alxkzmn approved these changes Feb 26, 2024

View reviewed changes

sifnoc added 3 commits February 26, 2024 06:25

fix: renamed description of test case; removed unused variable in con…

3559ad2

…tract test

Merge branch 'v2' into v2-kzg-verifier

441160a

fix: removed unused

6e52b9d

sifnoc force-pushed the v2-kzg-verifier branch from a48ea0b to 6e52b9d Compare February 26, 2024 07:12

sifnoc merged commit f411d4d into v2 Feb 26, 2024
6 checks passed

sifnoc deleted the v2-kzg-verifier branch February 26, 2024 07:22

This was referenced Feb 28, 2024

Consider using a Solidity KZG verifier for user inclusion #241

Closed

Refactor smart contract for the polynomial encoding version (aka Summa v2) #242

Closed


		require(grandSumVerifier.verifyProof(verifyingKey, combinedProofs, totalBalances), "Invalid grandsum proof");

		// Slice the proof aa length as grandsumProof

Implement Verifiers for KZG and SNARK Proofs in Contracts #267

Implement Verifiers for KZG and SNARK Proofs in Contracts #267

Conversation

sifnoc commented Feb 20, 2024 • edited Loading

sifnoc commented Feb 22, 2024

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

alxkzmn Feb 23, 2024 • edited Loading

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

alxkzmn Feb 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alxkzmn Feb 23, 2024 • edited Loading

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

alxkzmn Feb 23, 2024 • edited Loading

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

alxkzmn left a comment

Choose a reason for hiding this comment

sifnoc commented Feb 23, 2024

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

alxkzmn left a comment

Choose a reason for hiding this comment

sifnoc commented Feb 20, 2024 •

edited

Loading

alxkzmn Feb 23, 2024 •

edited

Loading

alxkzmn Feb 23, 2024 •

edited

Loading

alxkzmn Feb 23, 2024 •

edited

Loading

alxkzmn Feb 23, 2024 •

edited

Loading