Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat: BW6-761 KZG gadget #866

Merged
merged 6 commits into from
Oct 18, 2023
Merged

Feat: BW6-761 KZG gadget #866

merged 6 commits into from
Oct 18, 2023

Conversation

yelhousni
Copy link
Contributor

@yelhousni yelhousni commented Oct 16, 2023

Description

Adds BW6-761 KZG according to #840 (need #846 first to me merged).

Type of change

  • New feature (non-breaking change which adds functionality)

How has this been tested?

The tests in std/algebra/emulated/sw_emulated and std/commitments/kzg are generic for all curves. But weirdly the BW6 KZG test fails when testing with any other curve than BN254 as the SNARK curve! It throws an error in the division needed to compute the slope of doubleStep at exactly the iteration i=15. I need to deeply look at this. So for now testing with test.WithCurves(ecc.BN254) only.

Edit: the issue was identified by @ivokub to be related to field emulation (edge case in division when limbs are big e.g. bw6-761 field). Will be fixed in a separate PR (issue #867).

How has this been benchmarked?

KZG over BW6-761 in a BN254 SNARK:

Verify
R1CS 4,992,635
SCS 49,249,398

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

@yelhousni yelhousni added the consolidate strengthen an existing feature label Oct 16, 2023
@yelhousni yelhousni requested a review from ivokub October 16, 2023 22:18
@yelhousni yelhousni self-assigned this Oct 16, 2023
@yelhousni yelhousni marked this pull request as draft October 16, 2023 22:19
@github-actions
Copy link

📦 github.com/consensys/gnark/std/algebra/emulated/sw_bw6761
TestFinalExponentiationTestSolve 6.33s

    pairing_test.go:61: 
        	Error Trace:	/home/runner/work/gnark/gnark/std/algebra/emulated/sw_bw6761/pairing_test.go:61
        	Error:      	Received unexpected error:
        	            	[assertIsEqual] 4043086562086568115189170845891238752565310120857718483635404800 == 4043086562086568115189170865208720454583709724433885051062469808
        	            	emulated.(*Field[...]).rsh
        	            		field_assert.go:56
        	            	emulated.(*Field[...]).assertLimbsEqualitySlow
        	            		field_assert.go:35
        	            	emulated.(*Field[...]).AssertLimbsEquality
        	            		field_assert.go:86
        	            	emulated.(*Field[...]).AssertIsEqual
        	            		field_assert.go:144
        	            	fields_bw6761.Ext3.AssertIsEqual
        	            		e3.go:382
        	            	fields_bw6761.Ext6.AssertIsEqual
        	            		e6.go:362
        	            	sw_bw6761.Pairing.AssertIsEqual
        	            		pairing.go:136
        	            	sw_bw6761.(*FinalExponentiationCircuit).Define
        	            		pairing_test.go:47
        	Test:       	TestFinalExponentiationTestSolve

TestMultiPairTestSolve 14.78s

    pairing_test.go:141: 
        	Error Trace:	/home/runner/work/gnark/gnark/std/algebra/emulated/sw_bw6761/pairing_test.go:141
        	Error:      	Received unexpected error:
        	            	[assertIsEqual] 4043103644193227505160688951496192019616594702585708108085985280 == 4043103644193227505160689028132802940650037215199111543855022211
        	            	emulated.(*Field[...]).rsh
        	            		field_assert.go:56
        	            	emulated.(*Field[...]).assertLimbsEqualitySlow
        	            		field_assert.go:35
        	            	emulated.(*Field[...]).AssertLimbsEquality
        	            		field_assert.go:86
        	            	emulated.(*Field[...]).AssertIsEqual
        	            		field_assert.go:144
        	            	fields_bw6761.Ext3.AssertIsEqual
        	            		e3.go:382
        	            	fields_bw6761.Ext6.AssertIsEqual
        	            		e6.go:362
        	            	sw_bw6761.Pairing.AssertIsEqual
        	            		pairing.go:136
        	            	sw_bw6761.(*MultiPairCircuit).Define
        	            		pairing_test.go:118
        	Test:       	TestMultiPairTestSolve

TestPairTestSolve 12.27s

    pairing_test.go:94: 
        	Error Trace:	/home/runner/work/gnark/gnark/std/algebra/emulated/sw_bw6761/pairing_test.go:94
        	Error:      	Received unexpected error:
        	            	[assertIsEqual] 4043085924335764517335693622064575039337582645263476405563293696 == 4043085924335764517335693832531137221026007763305316447077703495
        	            	emulated.(*Field[...]).rsh
        	            		field_assert.go:56
        	            	emulated.(*Field[...]).assertLimbsEqualitySlow
        	            		field_assert.go:35
        	            	emulated.(*Field[...]).AssertLimbsEquality
        	            		field_assert.go:86
        	            	emulated.(*Field[...]).AssertIsEqual
        	            		field_assert.go:144
        	            	fields_bw6761.Ext3.AssertIsEqual
        	            		e3.go:382
        	            	fields_bw6761.Ext6.AssertIsEqual
        	            		e6.go:362
        	            	sw_bw6761.Pairing.AssertIsEqual
        	            		pairing.go:136
        	            	sw_bw6761.(*PairCircuit).Define
        	            		pairing_test.go:79
        	Test:       	TestPairTestSolve

@yelhousni yelhousni marked this pull request as ready for review October 17, 2023 14:54
@ivokub
Copy link
Collaborator

ivokub commented Oct 17, 2023

Should work with #870.

Copy link
Collaborator

@ivokub ivokub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All perfect!

@ivokub
Copy link
Collaborator

ivokub commented Oct 17, 2023

And for historical note, with #749 PLONK cost is 20760085.

@ivokub
Copy link
Collaborator

ivokub commented Oct 17, 2023

Suggested edit:

diff --git a/std/commitments/kzg/verifier_test.go b/std/commitments/kzg/verifier_test.go
index 2fed4f36..29d2694a 100644
--- a/std/commitments/kzg/verifier_test.go
+++ b/std/commitments/kzg/verifier_test.go
@@ -175,7 +175,7 @@ func TestKZGVerificationEmulated3(t *testing.T) {
 		Commitment:   wCmt,
 		OpeningProof: wProof,
 	}
-	assert.CheckCircuit(&KZGVerificationCircuit[sw_bw6761.Scalar, sw_bw6761.G1Affine, sw_bw6761.G2Affine, sw_bw6761.GTEl]{}, test.WithValidAssignment(&assignment), test.WithCurves(ecc.BN254))
+	assert.CheckCircuit(&KZGVerificationCircuit[sw_bw6761.Scalar, sw_bw6761.G1Affine, sw_bw6761.G2Affine, sw_bw6761.GTEl]{}, test.WithValidAssignment(&assignment))
 }
 
 func TestKZGVerificationTwoChain(t *testing.T) {

@yelhousni
Copy link
Contributor Author

Suggested edit:

diff --git a/std/commitments/kzg/verifier_test.go b/std/commitments/kzg/verifier_test.go
index 2fed4f36..29d2694a 100644
--- a/std/commitments/kzg/verifier_test.go
+++ b/std/commitments/kzg/verifier_test.go
@@ -175,7 +175,7 @@ func TestKZGVerificationEmulated3(t *testing.T) {
 		Commitment:   wCmt,
 		OpeningProof: wProof,
 	}
-	assert.CheckCircuit(&KZGVerificationCircuit[sw_bw6761.Scalar, sw_bw6761.G1Affine, sw_bw6761.G2Affine, sw_bw6761.GTEl]{}, test.WithValidAssignment(&assignment), test.WithCurves(ecc.BN254))
+	assert.CheckCircuit(&KZGVerificationCircuit[sw_bw6761.Scalar, sw_bw6761.G1Affine, sw_bw6761.G2Affine, sw_bw6761.GTEl]{}, test.WithValidAssignment(&assignment))
 }
 
 func TestKZGVerificationTwoChain(t *testing.T) {

Should we wait for #870 to merge with the suggested edit (otherwise CI will fail) or could we merge as-is and in #870 merge master and there apply the edit?

@ivokub
Copy link
Collaborator

ivokub commented Oct 18, 2023

Suggested edit:

diff --git a/std/commitments/kzg/verifier_test.go b/std/commitments/kzg/verifier_test.go
index 2fed4f36..29d2694a 100644
--- a/std/commitments/kzg/verifier_test.go
+++ b/std/commitments/kzg/verifier_test.go
@@ -175,7 +175,7 @@ func TestKZGVerificationEmulated3(t *testing.T) {
 		Commitment:   wCmt,
 		OpeningProof: wProof,
 	}
-	assert.CheckCircuit(&KZGVerificationCircuit[sw_bw6761.Scalar, sw_bw6761.G1Affine, sw_bw6761.G2Affine, sw_bw6761.GTEl]{}, test.WithValidAssignment(&assignment), test.WithCurves(ecc.BN254))
+	assert.CheckCircuit(&KZGVerificationCircuit[sw_bw6761.Scalar, sw_bw6761.G1Affine, sw_bw6761.G2Affine, sw_bw6761.GTEl]{}, test.WithValidAssignment(&assignment))
 }
 
 func TestKZGVerificationTwoChain(t *testing.T) {

Should we wait for #870 to merge with the suggested edit (otherwise CI will fail) or could we merge as-is and in #870 merge master and there apply the edit?

Yes, we can merge current as-is and then in #870 make the change. #870 is a more generic fix and not particularly blocking this one.

I'm trying to keep in mind to rebase 870 and add the change there :)

@yelhousni yelhousni merged commit 73146fd into master Oct 18, 2023
7 checks passed
@yelhousni yelhousni deleted the feat/bw6761-kzg branch October 18, 2023 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consolidate strengthen an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants