Refac/groth16 mpc #1372

Tabaie · 2024-12-25T21:37:18Z

Description

#1297, but using tools from Consensys/gnark-crypto#589.

Fixes # (issue)

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How has this been tested?

Test A
Test B

How has this been benchmarked?

Benchmark A, on Macbook pro M1, 32GB RAM
Benchmark B, on x86 Intel xxx, 16GB RAM

Checklist:

I have performed a self-review of my code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
I did not modify files generated from templates
golangci-lint does not output errors locally
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules

gbotrel

lgtm, minor changes maybe

internal/generator/backend/template/zkpschemes/groth16/mpcsetup/utils.go.tmpl

gbotrel · 2025-01-21T21:30:23Z

internal/generator/backend/template/zkpschemes/groth16/mpcsetup/utils.go.tmpl

-		r[i].SetRandom()
+func areInSubGroupG1(s []curve.G1Affine) bool {
+	for i := range s {
+		if !s[i].IsInSubGroup() {


that's not cheap, we don't have a batch subgroup check yet (@yelhousni wanted to add one) but shouldn't we parallelize since we assume the nominal case is everything is in subgroup ?

internal/utils/test_utils/test_utils.go

gbotrel · 2025-01-21T21:32:43Z

internal/utils/slices.go

+package utils
+
+// AppendRefs returns append(s, &v[0], &v[1], ...).
+func AppendRefs[T any](s []any, v []T) []any {


is this still needed with golang slices generic stuff ?

Just went through the package docs. Nothing there does it compactly.

ivokub

Thanks Arya! It looks really nice. Imo I understood it mostly. There are a few comments though, see inline.

A few of the comments are nice to have and you don't need to address now. We could do it in separate PR:

optional randomness source for contribution
being able to change hash function for challenge computation for cool tricks a la providing SNARK of ceremony correctness

ivokub · 2025-01-22T12:14:53Z

backend/groth16/bls12-377/mpcsetup/utils.go

+	/*if a[0].IsOne() {
+		A = A[1:]
+		a = a[1:]
+	}*/


In all usages in the package we have len(A) == len(a) I would add the check explicitly as a defensive measure. First to avoid out-of-bounds reads due to some calling error and secondly to ensure that all A are scaled. Compared to scalar muls below it is negligible cost, but adds clarity.

My concern with this is that its uses will become verbose and redundant. We'll get things like

scaleG1InPlace(c.G1.Tau[1:], tauUpdates[1:len(c.G1.Tau]) scaleG1InPlace(c.G1.AlphaTau, alphaUpdates[:len(c.G1.AlphaTau)]) scaleG1InPlace(c.G1.BetaTau, betaUpdates[:len(c.G1.BetaTau)])

where we previously had

scaleG1InPlace(c.G1.Tau[1:], tauUpdates[1:]) scaleG1InPlace(c.G1.AlphaTau, alphaUpdates) scaleG1InPlace(c.G1.BetaTau, betaUpdates)

Indeed - but currently the functions scaleG1InPlace and scaleG2InPlace are used only in one method in phase1.go where we know anyway that len(tauUpdates) == len(c.G1.Tau) (etc for AlphaTau and BetaTau).

Even more, now having a second look - I think we could just inline powers and scaleG1InPlace without defining utility methods at all and avoid too early generefication. Then we can avoid allocating the slices tauUpdates, betaUpdates and alphaUpdates.

And in order to make it parallel, we can use utils.Parallelize and then within compute s = f^start as the starting point and then at every loop multiply s by f and then do a scalar multiplication.

ivokub · 2025-01-22T12:16:02Z

backend/groth16/bls12-377/mpcsetup/utils.go

+	/*if a[0].IsOne() {
+		A = A[1:]
+		a = a[1:]
+	}*/


Similarly would add check len(A) == len(a) explicitly. And imo it is OK to panic here, no need to handle errors.

ivokub · 2025-01-22T12:19:24Z

backend/groth16/bls12-377/mpcsetup/setup.go

 )

-func ExtractKeys(srs1 *Phase1, srs2 *Phase2, evals *Phase2Evaluations, nConstraints int) (pk groth16.ProvingKey, vk groth16.VerifyingKey) {
+// Seal performs the final contribution and outputs the proving and verifying keys.


Add documentation what the beaconChallenge should be. Is it an identifier for the current ceremony? What happens if I run Seal independently on same Phase2 with different beaconChallenge - is there any dependence between the output Groth16 SRS?

backend/groth16/bls12-377/mpcsetup/setup_test.go

ivokub · 2025-01-22T12:31:38Z

backend/groth16/bls12-377/mpcsetup/setup_test.go


+func testAll(t *testing.T, nbContributionsPhase1, nbContributionsPhase2 int) {
 	assert := require.New(t)


There is probably some import cycle involved, but couldn't you just do assert := test.NewAssert(t)? Then the returned test.Assert object has helper methods for proving etc.

Which methods do you have in mind? Since the setup has to be in a custom way (imitating how an actual multiparty ceremony would go with objects being written to and read from the network) there is only the proof and verification stages to get help with, and those are short enough imo.

There is method below proveVerifyCircuit, but indeed it uses fixed proving and verifying key and test.Assert doesn't currently have a way to test it like this. But still, test.Assert is a very simple wrapper around require.Assertions, keeping all the methods what require.Assertions has. And imo this unifies the testing style if we use same instance everywhere.

Ah so we generally better not use the testify repo directly?

At least I would use the wrapper we have - then the test implementations across different gnark packages are more coherent imo.

ivokub · 2025-01-22T23:18:19Z

backend/groth16/bn254/mpcsetup/unit_test.go

@@ -0,0 +1,286 @@
+package mpcsetup


File only generated for BN254? I would code-generate just in case.

The idea was not to clog the CI with unit tests. The full test is still generated for all curves. Ignoring the unit tests saves 5 seconds per curve on my laptop. Perhaps not worth it?

In that case, I would still code generate, but maybe conditionally add test.Skip() in case the curve isn't BN254. Then atleast we ensure that everything is syntactically correct and the unit test is covered in code generation.

ivokub · 2025-01-22T23:43:18Z

backend/groth16/bls12-377/mpcsetup/phase1.go

 }

-func (phase1 *Phase1) hash() []byte {
+func (p *Phase1) hash() []byte {


And imo, the method could also be public and defined:
func (p *Phase) Challenge(previous []byte) []byte

Would it perhaps be better to remove the challenge field entirely, and just feed it as input to the Contribute and Verify functions?

The downside of this approach is that when sending contributions on the wire the user has to read/write two objects rather than one.

But in practice do we need to send challenge on the wire? When the user returns contribution to the sequencer, then it doesn't need to send the challenge, sequencer can compute it on its own (and it actually already does it, assuming that it verifies the contributions).

Now, the question imo is if the user is sent the tip of the ceremony for starting their contribution, then should it verify the full state of the current ceremony. Imo for me it would make sense it should, to ensure that it doesn't contribute to some fork etc. From practical point of view, it is probably too heavy for the users to verify and it could only contribute based on current trusted tip (i.e. ceremony tip and running challenge). And imo then it makes sense that challenge is separate, to indicate its optionality. And in this case imo we can provide it separately to Contribute and Verify

But its only my gut feeling, maybe it is okay to keep it in, but add comment that it is "trusted" value and should be verfied in full.

But in practice do we need to send challenge on the wire?

Yes, any new contributor need to receive the challenge used by the last contribution in order to compute its own challenge (so that the channel reflects the entire protocol transcript)
You are right that the field can be omitted when a contribution is being sent back to the sequencer.

I don't think we should require contributors to verify the whole protocol transcript. That goes against two of the protocol's major design goals:

Low entry barrier for contributions: I should be able to contribute relatively easily on a small-ish machine in a short amount of time

Indefinite run: The protocol is never truly over. We should always be able to enhance our SRS with more contributions, so the number of contributions is not a constant. So especially in light of (1) the verification effort gets prohibitive.

but add comment that it is "trusted" value

A trusted value from the contributor's perspective you mean? Because the verifier does reconstruct it, in the same way as you described in the first paragraph:
https://github.com/Consensys/gnark/blob/refac/groth16-mpc/backend/groth16/bn254/mpcsetup/phase1.go#L168

Yep, I think from practical point of view it doesn't make sense for the contributor to verify the whole chain, only the previous contribution on top it builds its contribution. And I guess it is fine to keep Challenge field inside the structs, makes easier to serialize and transport.

ivokub · 2025-01-22T23:47:08Z

io/roundtrip.go

@@ -21,8 +22,8 @@ func RoundTripCheck(from any, to func() any) error {
 			if err != nil {
 				return err
 			}
-			if !reflect.DeepEqual(from, r) {
-				return errors.New("reconstructed object don't match original (ReadFrom)")
+			if err = equal(from, r); err != nil {


I would keep it as is, imo there may be unwanted side-effects of using Equal if exists - there are already a few types in gnark which have the Equal method defined and they may be more strict than needed.

It seems like it is used to avoid comparing Challenge field in a test. But at least for me it would be clearer to manually empty the Challenge field manually when needed in the test. This current approach may break imo other roundtrip tests.

ivokub · 2025-01-22T23:52:25Z

backend/groth16/bls12-377/mpcsetup/phase1.go

 	}
-	if !sameRatio(contribution.Parameters.G1.AlphaTau[0], current.Parameters.G1.AlphaTau[0], alphaR, contribution.PublicKeys.Alpha.XR) {
-		return errors.New("couldn't verify that [α]₁ is based on previous contribution")
+	next.Challenge = challenge


My assumption was that verification wouldn't change the inputs, but it does as we write next.Challenge. This imo may have practical side effects that the contributions are not static (a la when hashing serialized proofs). Instead, I would provide the challenge as argument to Verify and then VerifyPhase1 automatically carries the continuation over to next round (with computing it using newChallenge := Phase1.Hash(challenge))

It's only changed if it is empty.

ivokub · 2025-01-22T23:53:12Z

backend/groth16/bls12-377/mpcsetup/phase2.go

+	if len(next.Challenge) != 0 && !bytes.Equal(next.Challenge, challenge) {
+		return errors.New("the challenge does not match the previous contribution's hash")
+	}
+	next.Challenge = challenge


Similarly as for Phase1 imo Phase2.Verify shouldn't change the inputs. See the corresponding comment.

ivokub · 2025-01-24T10:43:32Z

backend/groth16/bls12-377/mpcsetup/setup_test.go

+	return ccs.(*cs.R1CS)
+})
+
+func getTestCircuit() *cs.R1CS {


And now, we don't even need to have getTestCircuit function separately, we can just remove the current getTestCircuit function, rename onceCircuit variable to getTestCircuit and it works exactly the same.

Tabaie and others added 30 commits October 10, 2024 11:50

remove zombie code: rand in InitPhase1

2858a22

docs comment genR

3477517

revert bring init back

2e425fc

refactor: updateValue and verify

2719edb

feat new phase struct

7c93ec0

refactor phase1

c350336

feat complete phase1 verification

fcde665

fix check without assuming alpha, beta in G2

6fc1ffe

refactor initPhase1

4601ed7

feat phase1 marshal

9b82466

refactor "Parameters"

c352cd9

chore some comments and name changes

90f4ed7

feat phase2 init

7afabbf

Merge branch 'master' into chore/groth16-mpc

0510bd3

checkpoint

b4bbb2d

Merge remote-tracking branch 'origin/master' into chore/groth16-mpc

317ae6d

feat phase2 verification

282484a

docs lemma

b37e663

docs new ratio check method

50cd7e9

feat newLinearCombinationsG1

527dd22

clean phase1 ver

18411e5

docs and fixes

849f9d5

feat key extraction - commitments

34b3967

reface p *Phase2

81fdfb6

feat phase2 final contribution

ebaff87

feat marshal

c2f3b8d

test integration full - fails

5708047

test value update/pok

143c2b2

test setup no contributors

2afebf1

fix setOne

cd1d7a3

Tabaie added 7 commits December 25, 2024 14:44

refactor: groth16 mpcsetup to use gnark-crypto tools

b3dc401

refactor: use max instead of utils.Max

70fc948

fix: hash() panic on error

8d06fc5

refactor: clean up unused funcs

f63da80

chore: generify changes

8f5e982

chore: generify

73dd286

build: update gnark-crypto dep

94a58c5

Tabaie requested review from gbotrel and ivokub December 25, 2024 21:37

Tabaie mentioned this pull request Dec 25, 2024

Groth 16 MPC setup refactor #1297

Closed

Tabaie added 3 commits December 25, 2024 22:50

fix: utils.Max -> max

0f6d281

fix staticcheck

67cebb2

chore: generify staticcheck fix

c549cbc

ivokub mentioned this pull request Jan 13, 2025

Does gnark plan to add secret-sharing related primitive #1377

Closed

Tabaie added 2 commits January 21, 2025 13:01

Merge branch 'master' into refac/groth16-mpc

d7af186

build: update gnark-crypto

f0e446f

gbotrel requested changes Jan 21, 2025

View reviewed changes

Tabaie and others added 5 commits January 22, 2025 13:22

Merge branch 'master' into refac/groth16-mpc

f768fd1

perf: parallelize subgroup checks

2766ed7

build: generify parallel subgroup checks

6542b21

fix: error channels

3042482

refactor: simplify "powers"

27a82d5

Tabaie requested a review from gbotrel January 22, 2025 21:38

ivokub requested changes Jan 22, 2025

View reviewed changes

ivokub mentioned this pull request Jan 23, 2025

release: v0.12.0 #1399

Merged

3 tasks

Tabaie added 4 commits January 23, 2025 10:25

revert: remove conditionallog

75993d6

remove deadcode

3e8c434

docs: beacon

be3c909

fix: use sync.Once correctly

b70d0ae

ivokub reviewed Jan 24, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refac/groth16 mpc #1372

Refac/groth16 mpc #1372

Tabaie commented Dec 25, 2024

gbotrel left a comment

gbotrel Jan 21, 2025

gbotrel Jan 21, 2025

Tabaie Jan 22, 2025

ivokub left a comment

ivokub Jan 22, 2025

Tabaie Jan 23, 2025 •

edited

Loading

ivokub Jan 24, 2025

ivokub Jan 24, 2025

ivokub Jan 22, 2025

ivokub Jan 22, 2025

ivokub Jan 22, 2025

Tabaie Jan 23, 2025

ivokub Jan 24, 2025

Tabaie Jan 24, 2025

ivokub Jan 24, 2025

ivokub Jan 22, 2025

Tabaie Jan 23, 2025 •

edited

Loading

ivokub Jan 24, 2025

ivokub Jan 22, 2025

Tabaie Jan 23, 2025

Tabaie Jan 23, 2025

ivokub Jan 24, 2025

Tabaie Jan 27, 2025

ivokub Jan 27, 2025

ivokub Jan 22, 2025

ivokub Jan 22, 2025

Tabaie Jan 23, 2025

ivokub Jan 22, 2025

ivokub Jan 24, 2025


		func testAll(t *testing.T, nbContributionsPhase1, nbContributionsPhase2 int) {
		assert := require.New(t)

Refac/groth16 mpc #1372

Are you sure you want to change the base?

Refac/groth16 mpc #1372

Conversation

Tabaie commented Dec 25, 2024

Description

Type of change

How has this been tested?

How has this been benchmarked?

Checklist:

gbotrel 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

ivokub left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tabaie Jan 23, 2025 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tabaie Jan 23, 2025 • 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

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

Tabaie Jan 23, 2025 •

edited

Loading

Tabaie Jan 23, 2025 •

edited

Loading