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

Update for SECP256K1 v0.4.1 #131

Closed
wants to merge 43 commits into from

Conversation

MementoRC
Copy link
Collaborator

Mainly add support for the new features of SECP256K1.

  • Replaced deprecated functions.
  • Moved project description to TOML
  • An odd issue arises from egg_info creating absolute paths in SOURCES.txt -> use prune in MANIFEST.in
  • Added tox test for python 3.12

The .h are created automatically from the sec256pk1 with gcc -E (and a few tricks) so it formats a little oddly, creating lots of changes (like int* a -> int *a) which may make the review tedious
In the conda recipe (and the github repo for libsecp256k1-py-bindings), the build.py was modified to make it more automated and able to add either dynamic or static Clib. The compilation also uses an ABI version of CFFI lib

Copy link

codecov bot commented Dec 20, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2dfea67) 85.45% compared to head (ec5f8ff) 85.45%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #131   +/-   ##
=======================================
  Coverage   85.45%   85.45%           
=======================================
  Files          10       10           
  Lines         557      557           
  Branches       57       57           
=======================================
  Hits          476      476           
  Misses         45       45           
  Partials       36       36           
Files Coverage Δ
coincurve/keys.py 76.76% <100.00%> (ø)
coincurve/utils.py 87.93% <ø> (ø)

pyproject.toml Outdated Show resolved Hide resolved
@ofek
Copy link
Owner

ofek commented Dec 20, 2023

Looks like some style errors

pyproject.toml Outdated Show resolved Hide resolved
@MementoRC
Copy link
Collaborator Author

@ofek, @carterbox I seem to have finally got it going, though it might not be very elegant. It just passed with a patched coincurve, so I added the solution this PR. I will update the conda recipe to see if with the coincurve PR it will build (without patching)

@MementoRC
Copy link
Collaborator Author

MementoRC commented Dec 22, 2023

@ofek, I did use PY_LIMITED_API for the libsecp256k1-py-bindings repository. It does simplify the setup.py. I only created the py-bindings because it seemed like a major change to coincurve and I wanted to have the packages finally released on conda-forge, it's been like 3 months since I first started that recipe, other recipes have typically a few days turnover. It has not been a healthy experience for me, but it is likely from my own shortcomings

@carterbox , right, thanks, I had stumbled upon this info in the pst. The issue here is that a specific syntax has to be used with CFFI for it to compile the object for linkage to a shared library, by default it assumes the library would be static (or at least that's what i experienced

I just checked and all runs have passed, we should be close to LGTM after review and needed corrections

Thanks for all the help, it was a long journey

tox.ini Outdated Show resolved Hide resolved
@ofek
Copy link
Owner

ofek commented Dec 24, 2023

The compilation also uses an ABI version of CFFI lib

Is there a way to avoid that? I would much prefer API after reading https://cffi.readthedocs.io/en/stable/overview.html#abi-versus-api

@MementoRC
Copy link
Collaborator Author

MementoRC commented Dec 24, 2023

@ofek, this is what I was referring to with ABI: https://cffi.readthedocs.io/en/stable/cdef.html#ffibuilder-compile-etc-compiling-out-of-line-modules, it is CPython independent - I suspect it is still a concern? (I am over my head with that part, I just thought CPython independent was a good feature, but I don't understand the details)

So actually, it seem to generate a CPython dependent library:
copying build\lib.win-amd64-cpython-38\coincurve\_libsecp256k1.cp38-win_amd64.pyd -> build\bdist.win-amd64\wheel\.\coincurve

Whereas for libsecp256k1-py-bindings, it is CPython independent: INFO:root:copying build\lib.win-amd64-cpython-310\libsecp256k1_py_bindings_libsecp256k1.pyd -> build\bdist.win-amd64\wheel.\libsecp256k1_py_bindings`

I am not understanding why, though

@ofek
Copy link
Owner

ofek commented Dec 24, 2023

So am I correct in understanding that actually the mode hasn't changed with this PR?

@MementoRC
Copy link
Collaborator Author

@ofek, Well, you are correct. I realized I used build_ext instead of _build_ext, so maybe that's why the ABI was not effective. I will change the py-bindings one as well on the recipe and verify that both are using API mode

@ofek
Copy link
Owner

ofek commented Dec 25, 2023

Is pkgconfig required? I've hardly ever seen that used

@MementoRC
Copy link
Collaborator Author

@ofek, I use it to find whether libsecp256k1 is installed. The issue seems to be that setup.py does not know it is needed since I put the info in the pyproject.toml

@MementoRC MementoRC changed the title Update for SECP256K1 v0.4.0 Update for SECP256K1 v0.4.1 Dec 26, 2023
@MementoRC MementoRC closed this Dec 26, 2023
@MementoRC MementoRC deleted the feat/update_v0.4.0 branch December 26, 2023 18:11
@MementoRC
Copy link
Collaborator Author

@ofek Oops, I changed the name of my branch, I did not foresee that it would close this PR ... maybe make a new PR?

@ofek
Copy link
Owner

ofek commented Dec 26, 2023

Yeah go ahead and open another!

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