Skip to content
New issue

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

#

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

Already on GitHub? # to your account

Jump to bottom

feat: Add signature_verification_result to schnorr stdlib #173

Merged
merged 4 commits into from
Mar 2, 2023
Merged

feat: Add signature_verification_result to schnorr stdlib #173

merged 4 commits into from
Mar 2, 2023

Conversation

phated
Copy link
Contributor

@phated phated commented Feb 21, 2023 •
edited
Loading

Description

Noir needs verify_signature in the schnorr stdlib to return a bool_t indicating whether the signature was valid or not.

Currently a draft because I tried to update @kevaundray's code to add a constraint but I have no idea if I did it correctly.

Checklist:

  • I have reviewed my diff in github, line by line.
  • Every change is related to the PR description.
  • I have linked this pull request to the issue(s) that it resolves.
  • There are no unexpected formatting changes, superfluous debug logs, or commented-out code.
  • There are no circuit changes, OR specifications in /markdown/specs have been updated.
  • There are no circuit changes, OR a cryptographer has been assigned for review.
  • I've updated any terraform that needs updating (e.g. environment variables) for deployment.
  • The branch has been rebased against the head of its merge target.
  • I'm happy for the PR to be merged at the reviewer's next convenience.
  • New functions, classes, etc. have been documented according to the doxygen comment format. Classes and structs must have @brief describing the intended functionality.
  • If existing code has been modified, such documentation has been added or updated.

@phated phated changed the title feat: Make stdlib schnorr verify_signature return a bool_t constraint feat!: Make stdlib schnorr verify_signature return a bool_t constraint Feb 21, 2023
@phated phated force-pushed the phated/noir-needs-4 branch from 4e06695 to a98abe2 Compare February 27, 2023 15:30
zac-williamson
zac-williamson previously requested changes Feb 27, 2023
View reviewed changes
Copy link
Contributor

@zac-williamson zac-williamson left a comment

Choose a reason for hiding this comment

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

I can see how it would be useful to have a verification method that does not assert but instead returns a boolean. It gives more flexibility to developers and makes it easier to evaluate branching paths where one wishes to conditionally verify a signature.

We have a lot of tests and circuit code that relies on the signature verification algorithm to produce unsatisfiable constraints if the signature is invalid.

I think the fastest path forward is to create a new independent method that returns a bool_t, which does not interfere with existing tests.

@codygunton
Copy link
Collaborator

Rebased and resolved conflicts with waffle removal PR.

@phated phated marked this pull request as ready for review February 27, 2023 19:22
@phated phated changed the title feat!: Make stdlib schnorr verify_signature return a bool_t constraint feat: Add signature_verification_result to schnorr stdlib Feb 27, 2023
@codygunton
Copy link
Collaborator

@zac-williamson looks good? Merging is blocked until you approve 😇

@codygunton codygunton dismissed zac-williamson’s stale review March 1, 2023 14:03

Zac is with me now and he say it's ok.

@codygunton codygunton force-pushed the phated/noir-needs-4 branch from ffa0396 to 6f3830f Compare March 2, 2023 16:49
@codygunton
Copy link
Collaborator

codygunton commented Mar 2, 2023 •
edited
Loading

^ rebase

@codygunton codygunton merged commit 7ae381e into master Mar 2, 2023
@codygunton codygunton deleted the phated/noir-needs-4 branch March 2, 2023 19:34
@Savio-Sou Savio-Sou added this to the UltraPlonk in Noir milestone Mar 6, 2023
@github-actions github-actions bot mentioned this pull request Jun 15, 2023
Merged
@github-actions github-actions bot mentioned this pull request Jul 11, 2023
Merged
ludamad pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Jul 22, 2023
@phated @kevaundray @codygunton
feat: Add signature_verification_result to schnorr stdlib (AztecPro…
75ff77f 
…tocol/barretenberg#173)

* Add more flexible Schnorr verification methods

---------

Co-authored-by: kevaundray <kevtheappdev@gmail.com>
Co-authored-by: codygunton <codygunton@gmail.com>
ludamad pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Jul 24, 2023
@phated @kevaundray @codygunton
feat: Add signature_verification_result to schnorr stdlib (AztecPro…
187e25c 
…tocol/barretenberg#173)

* Add more flexible Schnorr verification methods

---------

Co-authored-by: kevaundray <kevtheappdev@gmail.com>
Co-authored-by: codygunton <codygunton@gmail.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@codygunton codygunton codygunton approved these changes

@charlielye charlielye Awaiting requested review from charlielye

@kevaundray kevaundray Awaiting requested review from kevaundray

@dbanks12 dbanks12 Awaiting requested review from dbanks12

@zac-williamson zac-williamson Awaiting requested review from zac-williamson

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@phated @codygunton @zac-williamson @Savio-Sou @kevaundray