Skip to content

Perturbing/add msm bls #514

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

Closed

Conversation

perturbing
Copy link
Contributor

@perturbing perturbing commented Nov 26, 2024

Description

✨ PR: Add and Test Two MSM Implementations for BLS12-381

Summary

This PR introduces multi-scalar multiplication (MSM) for BLS12-381 elliptic curve points:


Changes

⚙️ Internal Module (BLS12_381/Internal.hs)

  • Added types for projective/affine points and scalars, including array/block representations
  • Added FFI bindings for 6 C functions:
    • blst_{p1s,p2s}_mult_pippenger, blst_{p1s,p2s}_to_affine, and c_blst_{p1s,p2s}_mult_pippenger_scratch_sizeof
  • Added helper marshalling functions:
    • withPointArray, withScalarArray, withAffineBlockArrayPtr
  • Implemented blsMSM with filtering of points-at-infinity and zero scalars for safety and efficiency

✅ Tests (Test/EllipticCurve.hs)

  • Added property tests comparing blsMSM against naive implementations
  • Fixed prop_randomFailsFinalVerify to use group-based logic instead of point inequality
  • Updated Arbitrary instance for Point
  • Increased probability of generating blsZero for better edge-case coverage

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • New tests are added if needed and existing tests are updated
  • All visible changes are prepended to the latest section of a CHANGELOG.md for the affected packages.
    New section is never added with the code changes. (See RELEASING.md)
  • When applicable, versions are updated in .cabal and CHANGELOG.md files according to the
    versioning process.
  • The version bounds in .cabal files for all affected packages are updated.
    If you change the bounds in a cabal file, that package itself must have a version increase. (See RELEASING.md)
  • Commits that only contain large amounts of formatting changes were added to .git-blame-ignore-revs
  • Self-reviewed the diff

Copy link
Collaborator

@lehins lehins left a comment

Choose a reason for hiding this comment

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

Haven't looked into ffi calls, just at the haskell portion of it.
I'll look into this in more depth once PR is out of draft

@perturbing
Copy link
Contributor Author

Thank you for taking a first look @lehins, much appreciated!

The FFI is not working yet, I am getting some segmentation faults that I am trying to debug with valgrind.

@perturbing perturbing marked this pull request as ready for review January 21, 2025 14:50
@perturbing
Copy link
Contributor Author

Hi, I fixed the bug I encountered with the memory layout (I overlooked how C code wanted the pointer).

I also added a property test, but got some weird behavior where my test's success depends on the running of other tests. See my comments here and here.

Copy link
Collaborator

@lehins lehins left a comment

Choose a reason for hiding this comment

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

I have quickly looked through the PR, but haven't spotted exactly what is wrong with this PR, but there is definitely something seriously wrong with memory management in this PR. All the non-deterministic test failures, which should not be the case with functionality that pretends to be "pure" serve as a good indicator that the functionality has a bug. Moreover, the fact that unrelated tests are affected is another strong indicator that something is seriously wrong!

I'll try to dig deeper into this functionality some time this week. Maybe I can help you get to the bottom of this.

@perturbing perturbing force-pushed the perturbing/add-msm-bls branch from e51ee19 to 53daf8e Compare February 4, 2025 11:01
Copy link
Contributor

@kwxm kwxm left a comment

Choose a reason for hiding this comment

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

This looks OK, but there's a problem with inputs involving the zero point which needs to be fixed.

@perturbing perturbing force-pushed the perturbing/add-msm-bls branch from 76dccab to 368e488 Compare April 28, 2025 12:46
Copy link
Contributor Author

@perturbing perturbing left a comment

Choose a reason for hiding this comment

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

Hi, I am ready for another round of review (see also the PR description for an overview of the changes).

Thanks in advance, much appreciated.

I think it's best to not yet merge this PR, best to first let @kwxm implement it in plutus via an SRP stanza.

@perturbing perturbing requested review from kwxm, lehins and tdammers May 2, 2025 11:27
@perturbing perturbing requested a review from a team as a code owner May 15, 2025 07:34
@perturbing perturbing force-pushed the perturbing/add-msm-bls branch from 04f0f30 to 03c0c4e Compare May 15, 2025 07:41
Copy link
Contributor

@tdammers tdammers left a comment

Choose a reason for hiding this comment

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

Looks mostly good to me.

@perturbing perturbing requested a review from tdammers May 15, 2025 14:56
Copy link
Collaborator

@lehins lehins left a comment

Choose a reason for hiding this comment

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

I had some questions and minor suggestions, but other than that it is looking good.

Needs a rebase on master. Also could you please add all visible changes to the Changelog and bump minor version in both cabal file and in the latest section in the changlog file to 2.2.3.0.

Copy link
Collaborator

@lehins lehins left a comment

Choose a reason for hiding this comment

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

This PR looks ready to go. Thank you for the hard work @perturbing!

@lehins lehins enabled auto-merge May 30, 2025 22:03
@lehins lehins disabled auto-merge May 30, 2025 22:03
@lehins lehins enabled auto-merge (squash) May 30, 2025 22:04
@lehins
Copy link
Collaborator

lehins commented May 30, 2025

@perturbing Could I ask you to cleanup the git commit history. Otherwise I am just gonna squash the whole 73 commits into one big one with very little info.

@lehins lehins disabled auto-merge May 30, 2025 22:05
@lehins lehins enabled auto-merge (squash) May 30, 2025 22:06
@lehins lehins disabled auto-merge May 31, 2025 01:11
erikd and others added 22 commits June 3, 2025 10:12
* Remove cborg allow-newer stanza

* Nix updates

* Update haskellNix flake input

---------

Co-authored-by: Neil Mayhew <[email protected]>
`cardano-binary-test` has been removed long time ago. It was converted
to a `testlib` in `cardano-binary`
* Disable some C compiler warnings for basement on Windows
* Use `ucrt64` for windows

Co-Authored-By: Hamish Mackenzie <[email protected]>
@perturbing perturbing force-pushed the perturbing/add-msm-bls branch from db3815b to dc34187 Compare June 3, 2025 08:13
@perturbing perturbing requested review from erikd, nfrisby and a team as code owners June 3, 2025 08:13
@perturbing
Copy link
Contributor Author

I think the above is too messy, since the impact of this PR is small, I made a new PR here #536. Closing this one!

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.

10 participants