bug: Groth16Commitments is empty after trusted setup

mattstam commented 7 months ago

Description

semaphore-mtb-setup allows you to run an MPC ceremony for Groth16. They've previously used Gnark v0.8.0 (because at the time v0.9.0 was unaudited). Since then, I have been attempting to update it to v0.9.1.

This repo provides custom functions for exporting the pk and vk:

The purpose of these functions is to export the keys in a way that can be read using Gnark's provided ReadFrom() functions. However, when the exported PK is read in this way after migrating to v0.9.1., it throws an error.

The particular error thrown is "must have as many value vectors as bases" from https://github.com/Consensys/gnark-crypto/blob/2e4aaaaefdbfdf06515663986ed884fed1b2177e/internal/generator/pedersen/template/pedersen.go.tmpl#L104 - I've manually checked the values here, its len(pk) == 1 vs. len(values) == 0, causing the error.

This led me to debug the cause of why the length of the passed in values [][]fr.Element are 0, and it turns out that when bn254.Prove() runs: https://github.com/Consensys/gnark/blob/9b8efdac514400a4100888b3cd5279e207f4a193/backend/groth16/bn254/icicle/icicle.go#L152 it has an empty commitmentInfo.

Expected Behavior

It's expected that commitmentInfo is non-empty, and that the passed in length of the values privateCommittedValues matches the pk.CommitmentKeys

Actual Behavior

commitmentInfo is empty

Possible Fix

In this PR to semaphore-mtb-setup, updating from v0.8 to v0.9 required adjustments due to changes in Gnark's API.

Most of these changes can be found in utils.go and keys.go -- if any of these changes from v0.8 -> v0.9 are not 1:1, it could be lead to the unexpected behavior observed.

Steps to Reproduce

git clone https://github.com/worldcoin/semaphore-mtb-setup.git && cd semaphore-mtb-setup
git fetch origin pull/2/head:mattstam-update-gnark
git checkout mattstam-update-gnark
go test ./...

This will lead to error panic: must have as many value vectors as base

Context

In semaphore-mtb-setup I was attempting to migrate v0.8.0 to 0.9.1 in PR. Although the issue described is not directly in https://github.com/Consensys/gnark, semaphore-mtb-setup is a very useful utility for anyone working with Groth16.

Your Environment

gnark version used: v0.9.1
gnark-crypto version used: v0.12.2-0.20231013160410-1f65e75b6dfb
go version: 1.19
Operating System and version: MacOS 13.4.1

ivokub commented 7 months ago

Hmm, there may be several aspects here which may be related, but would have to debug further to figure out the issue:

commitment info is quite recent addition to gnark, and it may be that for circuits compiled with older versions of gnark the compiled circuit info doesn't contain the data.
gnark included MPC imo doesnt support commitments, but would have to double check
I see you referenced icicle backend. We have the code path included and ran the tests at merge time, but we're not running continuous tests for it. There may be some updates to gnark CPU backends not merged to GPU backend.

All in all, will look into it, but relies on several non-default code paths.

ivokub commented 7 months ago

@mattstam - looking at the failing test TestProveAndVerify, I see that the proving and verification key is deserialized from file. For which circuit the keys have been generated? Is there a test for generating the keys also?

mattstam commented 7 months ago

@mattstam - looking at the failing test TestProveAndVerify, I see that the proving and verification key is deserialized from file. For which circuit the keys have been generated? Is there a test for generating the keys also?

Sorry it's a little confusing, the keys are generated here https://github.com/worldcoin/semaphore-mtb-setup/blob/d46ef6be3eb0c43303d7e817f7d0c005530addf0/test/example_test.go#L106 in TestSetup's ExtractKeys().

So the files from TestSetup are used during TestProveAndVerify, the latter of which demonstrates a standard trusted setup procedure using the program.

ivokub commented 7 months ago

But in this case, how does the proving key and verification key depend on the circuit? In TestSetup

// Compile the circuit
    var myCircuit Circuit
    ccs, err := frontend.Compile(bn254.ID.ScalarField(), r1cs.NewBuilder, &myCircuit)
    if err != nil {
        t.Error(err)
    }
    writer, err := os.Create("circuit.r1cs")
    if err != nil {
        t.Error(err)
    }
    defer writer.Close()
    ccs.WriteTo(writer)

is the only place either writer or ccs is used. Most importantly, when we write files pk and vk they do not depend on the circuit at all?

Now in TestProveAndVerify, the line which fails is https://github.com/worldcoin/semaphore-mtb-setup/blob/d46ef6be3eb0c43303d7e817f7d0c005530addf0/test/example_test.go#L131C22-L131C27 (using proving key for proving a compiled circuit), but it cannot match to the circuit itself?

ivokub commented 7 months ago

All in all, I do not understand the MPC setup code very well. But something seems to be off here, maybe the proving key is generated for some particular circuit? I'll try to have a look, but I'm not very hopeful.

Consensys / gnark