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

Integrate ECDSA + Keccak #1307

Merged
merged 19 commits into from
Dec 13, 2023
Merged

Integrate ECDSA + Keccak #1307

merged 19 commits into from
Dec 13, 2023

Conversation

mitschabaude
Copy link
Member

@mitschabaude mitschabaude commented Dec 12, 2023

Changes

  • Refactor Keccak
    • Note: This started as a big refactor with the goal of returning 64-bit words. Then I realized that this is not compatible with ECDSA and we really need bytes. This path might explain the large amount of changes introduced here. I think most of them are valuable
    • A beneficial side-effect is that we now use the minimal amount of constraints for to/from bytes conversion
    • A bad side-effect is that the 224 bit variant of SHA3 is no longer supported since the core sponge() function was rewritten with the assumption that length and capacity are multiples of 64 bits 😬 With a bit of extra work it should be possible to bring back 224 and keep constraints optimal. Honestly, I doubt 224 will be needed.
  • Add simple conversion circuit which prepares Keccak output for ECDSA
  • Change ECDSA sign() and verify() to take message bytes and do the Keccak hash
  • Update ECDSA doccomments
  • Change example to contain ECDSA and Keccak circuits in combination and individually
  • Expose Keccak as an export
  • Save some constraints by not xoring with zero constants

TODOs not covered by this PR

Results

image

@mitschabaude mitschabaude changed the title ECDSA + Keccak Integrate ECDSA + Keccak Dec 12, 2023
@mitschabaude mitschabaude marked this pull request as ready for review December 12, 2023 17:19
@mitschabaude mitschabaude requested a review from a team as a code owner December 13, 2023 07:20
@mitschabaude
Copy link
Member Author

closes #61

@@ -1,31 +1,23 @@
import { Field } from './field.js';
Copy link
Member

Choose a reason for hiding this comment

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

Actually, does this fit into src/lib/gadgets better than src/lib? Similar to ECDSA

Copy link
Contributor

Choose a reason for hiding this comment

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

It probably does! IIRC, I put it there before ECDSA was done, but it should probably live wherever that lives!

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed, I can move it in the later PR!

@mitschabaude mitschabaude merged commit 6a6c615 into main Dec 13, 2023
13 checks passed
@mitschabaude mitschabaude deleted the feature/keccak-and-ecdsa branch December 13, 2023 19:34
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.

None yet

3 participants