feat: verify commitments in groth16 recursion verifier #1057
Conversation
fix: emulated MarshalG1 for BN254
|
Thanks for the contribution! I started reviewing the PR, but it takes a bit time. I'm proposing some refactors related to this PR and some which I wanted to include as part of a cleanup of Groth16 verifier (simpler init of groth16 verifier instance for example). I'll have a longer response with specific items/suggestions soon. |
ivokub
left a comment
ivokub
left a comment
There was a problem hiding this comment.
Thanks for the contribution! I refactored to be more aligned with PLONK verifier and allow for independent Pedersen commitment verification.
Could you have a look that I hadn't missed something and the refactored version works for you. If you give a go-ahead, then I'll merge.
test/assert_options.go
Outdated
| } | ||
|
|
||
| // WithProverChecks forces to run prover checks and disables solver checks. | ||
| func WithProverChecks() TestingOption { |
There was a problem hiding this comment.
I'd not add the test property. The command assert.CheckCircuit is usually meant for end-to-end testing in CI and its internal behaviour is toggled with build tags:
// default options;
// go test -short --> testEngineOnly
// go test --> constraintOnlyProfile
// go test -tags=prover_checks --> proverOnlyProfile
// go test -tags=release_checks --> releaseProfile
Now, if we define and use WithProverCheck option, then when running tests with -short tag or without any tags, we would still be running the prover (which may be really slow).
Better option for testing prover is to use tags=provers_checks tag like
go test -timeout 10m -tags debug,prover_checks -run ^TestBN254InBN254Commitment$ github.com/consensys/gnark/std/recursion/groth16 -v -count=1
|
I run our tests with the refactored version. Everything seems fine. I left a comment about |
2 participants
Description
This PR adds commitment verification to groth16 recursion
AssertProofmethod. This implementation only supports at most one commitment.TODO:
go.modafter that PR mergedType of change
How has this been tested?
TestBN254InBN254Commitmentto test inner circuit with commitmentHow has this been benchmarked?
Checklist:
golangci-lintdoes not output errors locally