Skip to content

feat: add typed field hasher interface in MiMC package#752

Merged
gbotrel merged 3 commits intomasterfrom
feat/mimc_fieldhasher
Oct 1, 2025
Merged

feat: add typed field hasher interface in MiMC package#752
gbotrel merged 3 commits intomasterfrom
feat/mimc_fieldhasher

Conversation

@gbotrel
Copy link
Copy Markdown
Collaborator

@gbotrel gbotrel commented Sep 30, 2025

Adds a interface in mimc packages to enable callers to directly work with field elements instead of dancing with bytes (e.g. linea monorepo).


Note

Introduce a typed field-element MiMC hasher with WriteElement/SumElement/SumElements and supporting factories, update internals to reuse buffers, and add tests/templates for all curves.

  • API:
    • Add FieldHasher interface with WriteElement, SumElement, and SumElements.
    • New constructors: NewFieldHasher (field elements) and NewBinaryHasher (bytes).
  • Implementation:
    • Implement WriteElement, SumElement, and SumElements in digest for bls12-377, bls12-381, bls24-315, bls24-317, bn254, bw6-633, bw6-761, and grumpkin.
    • Change Sum to reset internal buffer with d.data = d.data[:0].
  • Tests:
    • Add TestFieldHasher validating equivalence of byte-based and element-based hashing, and SumElement/SumElements behavior for all curves.
  • Codegen:
    • Update templates (internal/generator/.../mimc.go.tmpl, tests) to generate the new API and behavior across curves.

Written by Cursor Bugbot for commit f87fe4a. This will update automatically on new commits. Configure here.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds a new FieldHasher interface to MiMC packages to enable direct work with field elements instead of requiring byte conversion. This provides a more convenient API for cryptographic operations that naturally work with field elements.

  • Introduces the FieldHasher interface with WriteElement and SumElement methods
  • Updates NewMiMC() to return FieldHasher instead of hash.StateStorer
  • Improves memory efficiency by using slice reslicing instead of setting to nil

Reviewed Changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.

File Description
internal/generator/crypto/hash/mimc/template/mimc.go.tmpl Template for MiMC implementation with new FieldHasher interface and methods
internal/generator/crypto/hash/mimc/template/tests/mimc_test.go.tmpl Template for tests verifying FieldHasher functionality
ecc/*/fr/mimc/mimc.go Generated MiMC implementations for various elliptic curves with FieldHasher interface
ecc/*/fr/mimc/mimc_test.go Generated tests for FieldHasher functionality across different curves

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link
Copy Markdown
Collaborator

@ivokub ivokub left a comment

Choose a reason for hiding this comment

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

I think the PR makes sense and I think the implementation is good.

I'm just not 100% sure if the interfaces make sense. The problem could be if someone has some kind of switch to initialize the hasher, then because NewMiMC in every pacakge now returns different interfaces, then it wouldn't work anymore.

I'd rather instead would define new constructor NewFieldHasher which returns FieldHasher interface (which we can now simplify to be Interface {Write(fr.Element) Sum() fr.Element} and for clarity maybe NewBinaryHasher which is the same as NewMiMC (and this we keep around).

This also should cover potential pain-point when people would try to do mixed hashing (i.e. bytes and field elements).

@gbotrel gbotrel merged commit 8310aae into master Oct 1, 2025
7 checks passed
@gbotrel gbotrel deleted the feat/mimc_fieldhasher branch October 1, 2025 02:52
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.

3 participants