Skip to content

Feat/marshal g1 scalar #891

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.

Sign up for GitHub

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

Jump to bottom
Merged
ThomasPiellard merged 9 commits into from
Oct 27, 2023
Merged

Feat/marshal g1 scalar #891

ThomasPiellard merged 9 commits into from
Oct 27, 2023

Conversation

@ThomasPiellard
Copy link
Collaborator

@ThomasPiellard ThomasPiellard commented Oct 25, 2023

Description

Adds method MarshalG1(G1El, int) []frontend.Variable, MarshalScalar(S, int) []frontend.Variable on the Curve interface, following gnark-crypto's Marshal (the result a slice of bits while it is a []byte in gnark-crypto but the layout is the same, same endianness).

Type of change

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

How has this been tested?

For both emulated and native curves, see:

  • TestMarshalG1
  • TestMarshalScalar

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

@ThomasPiellard ThomasPiellard requested a review from ivokub October 25, 2023 17:06
@github-actions
Copy link

github-actions bot commented Oct 25, 2023

📦 github.com/consensys/gnark/std/gkr
❌ TestMiMCFullDepthNoDepSolve 0s

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0xb762ea]

goroutine 107427 [running]:
github.com/consensys/gnark-crypto/ecc/bn254/fr/iop.(*Polynomial).ToCanonical(0x0, 0xc0002ac900?, {0xc0005c0f60?, 0xc0?, 0xfc6e20?})
	/home/runner/go/pkg/mod/github.com/consensys/[email protected]/ecc/bn254/fr/iop/polynomial.go:338 +0x2a
github.com/consensys/gnark/backend/plonk/bn254.(*instance).computeNumerator.func6(0x0)
	/home/runner/work/gnark/gnark/backend/plonk/bn254/prove.go:996 +0xc5
github.com/consensys/gnark/backend/plonk/bn254.batchApply.func1(0xc000150000?)
	/home/runner/work/gnark/gnark/backend/plonk/bn254/prove.go:1092 +0x31
created by github.com/consensys/gnark/backend/plonk/bn254.batchApply in goroutine 106564
	/home/runner/work/gnark/gnark/backend/plonk/bn254/prove.go:1091 +0x12b

ivokub
ivokub reviewed Oct 25, 2023
View reviewed changes
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.

Right now the MarshalScalar and MarshalG1 methods seem to be returning bits. In the short-mimc the fieldhasher interfaces is taking in field elements.

I'll try if it would be possible to here instead return compatible fr.Elements (by recomposing bits such that the high byte is 0). Then we can also have compatibility when hashing the bits (not only bitwise compatibility with native marshaling)

@ThomasPiellard
Copy link
Collaborator Author

ThomasPiellard commented Oct 26, 2023

Yes my plan was to return bits in the exact same layout as in gnark-crypto and let the user reconstruct the variable according to gnark-crypto. But it makes sense to return frontendVariables now that there is the short hash.

@github-actions
Copy link

github-actions bot commented Oct 27, 2023

📦 github.com/consensys/gnark/std/gkr
❌ TestMiMCFullDepthNoDepSolve 0s

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0xb762ea]

goroutine 413958 [running]:
github.com/consensys/gnark-crypto/ecc/bn254/fr/iop.(*Polynomial).ToCanonical(0x0, 0xc00023eb58?, {0xc00047a760?, 0x54?, 0xfc6e20?})
	/home/runner/go/pkg/mod/github.com/consensys/[email protected]/ecc/bn254/fr/iop/polynomial.go:338 +0x2a
github.com/consensys/gnark/backend/plonk/bn254.(*instance).computeNumerator.func6(0x0)
	/home/runner/work/gnark/gnark/backend/plonk/bn254/prove.go:996 +0xc5
github.com/consensys/gnark/backend/plonk/bn254.batchApply.func1(0xc0001600e0?)
	/home/runner/work/gnark/gnark/backend/plonk/bn254/prove.go:1092 +0x31
created by github.com/consensys/gnark/backend/plonk/bn254.batchApply in goroutine 413062
	/home/runner/work/gnark/gnark/backend/plonk/bn254/prove.go:1091 +0x12b

@ivokub ivokub self-requested a review October 27, 2023 11:41
ivokub
ivokub approved these changes Oct 27, 2023
View reviewed changes
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.

I changed a bit not to include the number of bits for marshalling and are now deducing from the parameters. Also added bitmode to short hash so that we can have compatible hash values.

I'll implement custom FS transcript binding (with BindBits separately).

From my side good to go. Have a look yourself and you can merge if seems good.

@ThomasPiellard ThomasPiellard merged commit 24d850c into master Oct 27, 2023
@ThomasPiellard ThomasPiellard deleted the feat/marshal_g1_scalar branch October 27, 2023 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ivokub ivokub ivokub approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ThomasPiellard @ivokub