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

Remove SECP256K1 deprecated context flags #135

Closed
wants to merge 15 commits into from

Conversation

MementoRC
Copy link
Collaborator

Proposal to remove the deprecated CONTEXT flags

Source SECP256K1 include/secp256k1.h:
https://github.com/bitcoin-core/secp256k1/blob/e4af41c61b0bb55fc9614cb39df8e455715b4dd4/include/secp256k1.h#L207

Copy link

codecov bot commented Jan 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (8bfb756) 85.45% compared to head (f6f48e2) 85.86%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #135      +/-   ##
==========================================
+ Coverage   85.45%   85.86%   +0.40%     
==========================================
  Files          10       11       +1     
  Lines         557      573      +16     
  Branches       66       66              
==========================================
+ Hits          476      492      +16     
  Misses         45       45              
  Partials       36       36              
Files Coverage Δ
coincurve/context.py 80.00% <100.00%> (ø)
coincurve/flags.py 100.00% <100.00%> (ø)
coincurve/keys.py 76.76% <100.00%> (ø)
tests/test_ecdsa.py 100.00% <100.00%> (ø)
tests/test_flags.py 100.00% <100.00%> (ø)
tests/test_keys.py 100.00% <100.00%> (ø)
tests/test_utils.py 100.00% <100.00%> (ø)

@ofek
Copy link
Owner

ofek commented Jan 24, 2024

What was the issue?

@MementoRC
Copy link
Collaborator Author

MementoRC commented Jan 24, 2024

@ofek The #define xxx (value | value) may not be interpreted by CFFI on windows. Looking at older version I see that you had already hardcoded the integer value in _windows_libsecp256k1.py
Well, I have not been able to confirm that it is the issue

Also, for the missing cffi, I had moved it to the pyproject.toml, instead of in the setup.py with setup_requires, maybe that is the issue that darosior is having? meaning, not using requirements.txt (I thought he was doing a pip install -r requirements.txt)

@carterbox
Copy link

IMHO, these changes seem too extensive. The conda package which uses an external dynamically linked libsecp256k1 instead of a revended statically linked libsecp256k1 is able to be imported on Windows. That means the problem with the PYPI package (the difference between the PYPI and conda package) should be related to building and linking libsecp256k1 only.

@MementoRC
Copy link
Collaborator Author

There are several changes that are related but not necessary. I think the key issue was in the _windows_libsecp256k1.py, which is used in the PyPI build process and not in the conda build. I actually wonder if it is needed, perhaps it was needed only for previous version of CFFI

How to check that this will solve the issue? - My way was to implement a windows-wheel build/test, but it requires a lot of try/error, I don't mind going through it, but if a simpler way is available, it would be great

@MementoRC
Copy link
Collaborator Author

This PR is a bit too messy. Resubmitting in a more orderly fashion

@MementoRC MementoRC closed this Jan 27, 2024
@MementoRC MementoRC deleted the feat/deprecated_context_flags branch February 3, 2024 00:43
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