ofek / coincurve

Cross-platform Python bindings for libsecp256k1
https://ofek.dev/coincurve/
Apache License 2.0
153 stars 46 forks source link

Update for SECP256K1 v0.4.1 #131

Closed MementoRC closed 10 months ago

MementoRC commented 11 months ago

Mainly add support for the new features of SECP256K1.

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

codecov[bot] commented 11 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files ```diff @@ 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](https://app.codecov.io/gh/ofek/coincurve/pull/131?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Ofek+Lev) | Coverage Δ | | |---|---|---| | [coincurve/keys.py](https://app.codecov.io/gh/ofek/coincurve/pull/131?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Ofek+Lev#diff-Y29pbmN1cnZlL2tleXMucHk=) | `76.76% <100.00%> (ø)` | | | [coincurve/utils.py](https://app.codecov.io/gh/ofek/coincurve/pull/131?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Ofek+Lev#diff-Y29pbmN1cnZlL3V0aWxzLnB5) | `87.93% <ø> (ø)` | |
ofek commented 11 months ago

Looks like some style errors

ofek commented 11 months ago

Let me know when to review/merge and I will fix documentation after

MementoRC commented 11 months ago

@ofek , sure. coincurve itself is ready, but adding linking to the existing libsecp256k1.lib fails for windows, so i kept trying to find out why and made so many commit when actually I should just patch the recipe to figure it out and make a final commit, hopefully it can be resolved today

MementoRC commented 11 months ago

@ofek , I had forgotten about it, but there is a more fundamental issue with coincurve, it links against a static libsecp156k1.lib, which we don't have as a conda package. So can I add more significant changes to try to generate a _libsecp256k1.abi that expects a dynamic library? (This is what I did for the libsecp256k1-py-bindings)

This issue only arises with windows (of course), so I could hide it under an if and it should not affect your deliverables since you use cross-compilation for windows

Any other suggestions are welcome of course, (@carterbox )

carterbox commented 11 months ago

The linking models for Unix and Windows are different. .lib files are sometimes not static libraries. Please use the following information to solve your problems setting the correct linkages for Windows.

Unix

Naming

Where the library name is "foo" and "1" is the lib version (for the ABI): Shared libaries are called libfoo.[so,dylib] or libfoo.so.1 or libfoo.1.dylib Static libraries are called libfoo.a. Naming is very standarized. The linker is aware of lib versions.

Windows

Naming

Where the library name is "foo" and "1" is the lib version (for the ABI): Shared libraries are called foo.dll or foo-1.dll Import libraries are called foo.lib or foo-1.lib or something else Static libraries are called foo.lib or foo_static.lib or foo.lib.a or foo.dll.a or something else. Naming is not standardized because no one agrees how to differentiate between import and static libraries. The linker is not aware of lib versions AFAIK; adding the lib version to the name of the file is a hack that people use to emulate a behavior that is standard on unix.

What is an import library!?

Import libraries are like a machine readable header that the Windows linker uses to create linkages for a DLL. Only the LIB file is needed at link time; the DLL is not needed at link time. On unix, the SO file is used directly to create the linkage; it is needed both at link and run time. Import libraries have the same file extension as static libraries; this causes a lot of confusion because you cannot tell them apart without opening them.

ofek commented 11 months ago

That's fine. While you're at it, I don't know if this is feasible, but is there a way to get rid of the cruft in setup.py and simply build what we need by calling CFFI directly and then ship wherever the paths end up? I speak about this approach in the second item here https://hatch.pypa.io/latest/blog/2023/12/11/hatch-v180/#future

It looks like this function can compile and outputs to the specific path https://cffi.readthedocs.io/en/stable/cdef.html#ffibuilder-compile-etc-compiling-out-of-line-modules

At that point, we can completely get rid of setuptools actually and switch to a build backend like Hatchling with simpler config.

MementoRC commented 11 months ago

@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 commented 11 months ago

@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

ofek commented 11 months ago

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 commented 11 months ago

@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 commented 11 months ago

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

MementoRC commented 11 months ago

@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 commented 10 months ago

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

MementoRC commented 10 months ago

@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 commented 10 months ago

@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 commented 10 months ago

Yeah go ahead and open another!