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

Perturbing/add msm bls #514

Open
wants to merge 26 commits into
base: master
Choose a base branch
from

Conversation

perturbing
Copy link
Contributor

Description

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 and others added 9 commits January 23, 2025 14:30
…1/Internal.hs

Co-authored-by: Alexey Kuleshevich <lehins@yandex.ru>
…1/Internal.hs

Co-authored-by: Alexey Kuleshevich <lehins@yandex.ru>
…1/Internal.hs

Co-authored-by: Alexey Kuleshevich <lehins@yandex.ru>
…1/Internal.hs

Co-authored-by: Alexey Kuleshevich <lehins@yandex.ru>
…1/Internal.hs

Co-authored-by: Alexey Kuleshevich <lehins@yandex.ru>
Co-authored-by: Alexey Kuleshevich <lehins@yandex.ru>
Co-authored-by: Alexey Kuleshevich <lehins@yandex.ru>
Co-authored-by: Alexey Kuleshevich <lehins@yandex.ru>
@perturbing perturbing force-pushed the perturbing/add-msm-bls branch from e51ee19 to 53daf8e Compare February 4, 2025 11:01
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