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

Fixes #283 : ensure test.Assert compile cache handles different objects of same type #284

Merged
merged 4 commits into from
Mar 18, 2022

Conversation

gbotrel
Copy link
Collaborator

@gbotrel gbotrel commented Mar 18, 2022

additionally, adds a bit of parallelism in the Assert object, after the compile phase.
Not extremely efficient since most algorithms after the compile phase are already parallel.

@gbotrel gbotrel requested a review from ivokub March 18, 2022 15:00
@ivokub
Copy link
Collaborator

ivokub commented Mar 18, 2022

Suggested edit:

diff --git a/frontend/compile.go b/frontend/compile.go
index c6d1140b..a0f98c19 100644
--- a/frontend/compile.go
+++ b/frontend/compile.go
@@ -81,7 +81,6 @@ func parseCircuit(builder Builder, circuit Circuit) (err error) {
 	// leafs are Constraints that need to be initialized in the context of compiling a circuit
 	var handler schema.LeafHandler = func(visibility schema.Visibility, name string, tInput reflect.Value) error {
 		if tInput.CanSet() {
-			// log.Trace().Str("name", name).Str("visibility", visibility.String()).Msg("init input wire")
 			switch visibility {
 			case schema.Secret:
 				tInput.Set(reflect.ValueOf(builder.AddSecretVariable(name)))
diff --git a/test/assert.go b/test/assert.go
index b3556491..ce390b13 100644
--- a/test/assert.go
+++ b/test/assert.go
@@ -443,7 +443,7 @@ func (assert *Assert) compile(circuit frontend.Circuit, curveID ecc.ID, backendI
 		return nil, ErrCompilationNotDeterministic
 	}
 
-	// // add the compiled circuit to the cache
+	// add the compiled circuit to the cache
 	assert.compiled[key] = ccs
 
 	return ccs, nil

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.

Looks good.

@gbotrel gbotrel merged commit 858ec0f into develop Mar 18, 2022
@gbotrel gbotrel deleted the fix/compilecache branch March 18, 2022 16:38
gbotrel added a commit that referenced this pull request Mar 22, 2022
* build: updated to latest gnark-crypto

* build: updated to latest gnark-crypto

* refactor: introduce Curve interface in std/ and updated eddsa tests

* feat: added std/eddsa publicKey and signature assign helpers

* refactor(std): merged twistededwards and bandersnatch. IsOnCurve failing for bandersnatch

* fix: closes #283. ensure test.Assert compile cache handles different object of same type

* fix: use UnsafeAddr instead of UnsafePointer to be retro compatible

* fix: fix previous commit

* test: test all twisted ed curve operations

* Fixes #283 : ensure test.Assert compile cache handles different objects of same type (#284)

* fix: closes #283. ensure test.Assert compile cache handles different object of same type

* fix: use UnsafeAddr instead of UnsafePointer to be retro compatible

* fix: fix previous commit

* fix: apply pr patch

* style: make twistededwards/Point methods package private
gbotrel added a commit that referenced this pull request Mar 24, 2022
* perf(std/tEd): first bit in ScalarMul handled separately

* perf(std/tEd): rearrange Double --> less constraints

* perf(std/EdDSA): rearrange eddsa verify (-1 addtion, -1 MustBeOnCurve)

* perf(std/tEd): Lookup2 for first 2 bits in ScalarMulFixedBase

* perf(std/tEd): FixedPoint should be hidden by the API

* test(tEd): test scalarMul for all curves and schemes

* fix(tEd): case when scalar size is odd

* fix(tEd): case when scalar size is odd

* refactor(eddsa): rearrange eddsa verif as cofactor clearing counts

* feat(tEd): implements double-base scalar mul

* perf(EdDSA): eddsa gadget using double-base scalar mul

* perf(bandersnatch): apply tEd perf changes to Bandersnatch

* fix: fixed wrong bigInt op in plonk api

* style(eddsa, tEd): no benchmarks

* style(eddsa, tEd): no benchmarks

* perf(bandersnatch): GLV scalar mul in-circuit

* test(twistededwards): randomise test

* refactor(bandersnatch): review PR 263

* fix(bandersnatch): curveID in hint not checked

* fix(bandersnatch): check curveID for endomorphism availability

* style(bandersnatch): correct comment

* style(bandersnatch): correct comment about negative scalars

* fix(bandersnatch): increase scalars size bound to 129 + comments

* fix: hint signature in bandersnatch matches new format

* refactor: eddsa factorizing and code cleaning (#285)

* build: updated to latest gnark-crypto

* build: updated to latest gnark-crypto

* refactor: introduce Curve interface in std/ and updated eddsa tests

* feat: added std/eddsa publicKey and signature assign helpers

* refactor(std): merged twistededwards and bandersnatch. IsOnCurve failing for bandersnatch

* fix: closes #283. ensure test.Assert compile cache handles different object of same type

* fix: use UnsafeAddr instead of UnsafePointer to be retro compatible

* fix: fix previous commit

* test: test all twisted ed curve operations

* Fixes #283 : ensure test.Assert compile cache handles different objects of same type (#284)

* fix: closes #283. ensure test.Assert compile cache handles different object of same type

* fix: use UnsafeAddr instead of UnsafePointer to be retro compatible

* fix: fix previous commit

* fix: apply pr patch

* style: make twistededwards/Point methods package private

* style: fix gosec errors in std/eddsa

* feat: disable GLV mul in bandersnatch until #268 is fixed

Co-authored-by: Thomas Piellard <thomas.piellard@consensys.net>
Co-authored-by: Gautam Botrel <gautam.botrel@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants