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: Add ECDSA secp256k1 builtin test #1294

Merged
merged 20 commits into from
May 12, 2023
Merged

feat: Add ECDSA secp256k1 builtin test #1294

merged 20 commits into from
May 12, 2023

Conversation

kevaundray
Copy link
Contributor

@kevaundray kevaundray commented May 4, 2023

Related issue(s)

Resolves #

Description

This adds back the ECDSA secp256k1 test, to see what the status of it is since UltraPlonk

Summary of changes

Dependency additions / changes

Test additions / changes

Checklist

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt with default settings.
  • I have linked this PR to the issue(s) that it resolves.
  • I have reviewed the changes on GitHub, line by line.
  • I have ensured all changes are covered in the description.

Documentation needs

  • This PR requires documentation updates when merged.

Additional context

BEGIN_COMMIT_OVERRIDE
chore: Add ECDSA secp256k1 builtin test (#1294)
END_COMMIT_OVERRIDE

@Savio-Sou
Copy link
Collaborator

Closes #1106 and closes #12

@TomAFrench
Copy link
Member

It could be helpful to move an equivalent of this across to aztec_backend as ideally we'd have a test for each black box function over there.

@kevaundray
Copy link
Contributor Author

This now fails with a constraint not satisfied error which is perhaps because of a misconfiguration in acvm/pwg.

Before it was crashing -- Note, we are currently committed to a branch of barretenberg, so this is not mergable until that branch gets merged into bb/master

flake.lock Outdated Show resolved Hide resolved
@kevaundray kevaundray requested review from TomAFrench and vezenovm May 11, 2023 21:44
@kevaundray
Copy link
Contributor Author

Hmm tests taking so long implies that the nix cache may not be working fully

Copy link
Contributor

@vezenovm vezenovm left a comment

Choose a reason for hiding this comment

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

Just some nits, good to merge with tests passing

@kevaundray kevaundray added this pull request to the merge queue May 11, 2023
Merged via the queue into master with commit 8a36611 May 12, 2023
@kevaundray kevaundray deleted the kw/add-bck-ecdsa branch May 12, 2023 00:13
TomAFrench added a commit that referenced this pull request May 17, 2023
* master: (66 commits)
  feat(nargo)!: retire print-acir in favour of flag (#1328)
  chore(ssa): enable cse for assert (#1350)
  chore(ssa refactor): Add basic instruction simplification (#1329)
  chore(noir): Release 0.6.0 (#1279)
  feat: enable to_radix for any field element (#1343)
  chore(ssa refactor): Simplify inlining pass and fix inlining failure (#1337)
  chore!: Update to acvm 0.11.0 (#1322)
  feat: Add ECDSA secp256k1 builtin test (#1294)
  chore: add support for encoding/decoding inputs from JSON (#1325)
  feat: Issue an error when attempting to use a `return` expression (#1330)
  chore(ssa refactor): Fix inlining bug (#1335)
  fix: to-bits and to-radix for > 128 bits (#1312)
  chore(parser): Parser error optimisation (#1292)
  chore(ssa refactor): Implement function inlining (#1293)
  chore: fix installation link in readme (#1326)
  chore: fix installation link in readme (#1326)
  feat(stdlib): Add keccak (#1249)
  fix: Parsing nested generics (#1319)
  chore(ssa refactor): Document some SSA-gen functions (#1321)
  fix: Assigning to tuple fields (#1318)
  ...
spalladino pushed a commit to spalladino/noir that referenced this pull request May 24, 2023
* Copy ECDSA from commit 92ef9b4

* rework for new syntax

* commit since github was down

* [NOT MERGEABLE] - Skip all tests except ecdsa

* change parameter order

* update flake.lock

* update test

* update flake.lock

* update flake.lock

* modify test

* add message + hashed_message to program

* remove underscore

* modify input to use low-S

* commit s variant 1

* commit variant S2

* modify s value in signature to value that passes in acvm/pwg

* update flake.lock

* remove if statement to make tests go faster

* cleanup test
sirasistant pushed a commit that referenced this pull request May 31, 2023
* feat: enable to_radix for any field element (#1343)

* Enable to_radix for any field element

* add integration test

* use proper bound during modulo (and small optimisation)

* update integration test

* feat: Add ECDSA secp256k1 builtin test (#1294)

* Copy ECDSA from commit 92ef9b4

* rework for new syntax

* commit since github was down

* [NOT MERGEABLE] - Skip all tests except ecdsa

* change parameter order

* update flake.lock

* update test

* update flake.lock

* update flake.lock

* modify test

* add message + hashed_message to program

* remove underscore

* modify input to use low-S

* commit s variant 1

* commit variant S2

* modify s value in signature to value that passes in acvm/pwg

* update flake.lock

* remove if statement to make tests go faster

* cleanup test

---------

Co-authored-by: guipublic <47281315+guipublic@users.noreply.github.com>
Co-authored-by: kevaundray <kevtheappdev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants