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

impl multiplication for polyval #55

Merged
merged 23 commits into from
Sep 13, 2024
Merged

impl multiplication for polyval #55

merged 23 commits into from
Sep 13, 2024

Conversation

thor314
Copy link
Contributor

@thor314 thor314 commented Aug 21, 2024

Closes #25.

Basing multiplication of the RustCrytpo software implementation. This PR is about halfway finished, implements bmul64, and most of the main galois mul subroutine.

Still need to:

  • create a test vector (and hope for the bit-twiddling best)
  • impl 64-bit overflowing multiplication
  • finish the last chunk in mul

there's a broken test I wrote that isn't actually supposed to pass (and i have no test vectors for it yet; I might generate some from rust-crypto), but triggers circom's compiler errors, which has been useful (if painful).

@thor314 thor314 marked this pull request as draft August 21, 2024 00:38
Copy link
Contributor

@devloper devloper left a comment

Choose a reason for hiding this comment

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

Damn!! This is good work! It's a tricky algorithm. One thought is that his algo appears to be optimized for CPU execution, but I wonder if that maps over to snark efficiency.

Regardless, starting with a working algorithm seems like the right idea.

Added only one comment, I'll wait until the PR is out of draft for further review.

//
// ref: https://github.com/RustCrypto/universal-hashes/blob/master/polyval/src/backend/soft64.rs#L151
template MUL() {
signal input a[2][64];
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the intuition behind doing [2][64] vs [128]? Given each is represented as a field element internally, my hunch is we don't gain efficiency out of this (tho not certain) while having worse readability.

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe to use REV64()?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this file needed?

@thor314
Copy link
Contributor Author

thor314 commented Sep 13, 2024

All gfmul tests should be logically passing, but I'm having issues getting circomkit to correctly recognize the out[2][64] outputs, seems to only be capturing 64 of the 128 bits, causing these tests to fail.

Only thing left blocking this PR is figuring out that circomkit quirk and we're flying.

@Autoparallel @0xJepsen @KaiGeffen would you take a look?

Run npx mocha -g GF_MUL to reproduce.

I used https://github.com/thor314/universal-hashes to generate test vectors (see soft64.rs). Unsure whether that should be better documented.

@thor314 thor314 marked this pull request as ready for review September 13, 2024 23:24
@thor314 thor314 requested a review from 0xJepsen September 13, 2024 23:24
Copy link
Contributor

@0xJepsen 0xJepsen left a comment

Choose a reason for hiding this comment

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

lgtm

@0xJepsen 0xJepsen merged commit 8616f12 into main Sep 13, 2024
2 checks passed
@0xJepsen 0xJepsen deleted the impl-polymul branch September 13, 2024 23:25
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.

feat: GHASH - implement galois multiplication
5 participants