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 coincurve dependency, use python-bitcointx #1134

Merged
merged 1 commit into from
Jan 3, 2022
Merged

Conversation

AdamISZ
Copy link
Member

@AdamISZ AdamISZ commented Dec 30, 2021

This commit changes the underlying functions used in
jmbitcoin from the private and public key primitives
in coincurve and replaces them with equivalent
primitives CKey and CPubKey from python-bitcointx,
this removes the need to install coincurve and its
own bundled libsecp256k1 dynamic library.
Note that an additional pubkey_tweak_mul function
is exposed with ctypes from python-bitcointx's
bundled libsecp256k1 library.

@AdamISZ
Copy link
Member Author

AdamISZ commented Dec 30, 2021

This is marked [WIP] while a few things still need to be done:

  • Remove coincurve from the setup.py of jmbitcoin
  • Remove the libffi build dependency in install.sh (unless I forgot something, this is only needed for coincurve) edit: oh bad memory, it was originally put in place for libsodium, so that won't be removed.
  • Test installation is still functioning correctly
  • Cross-compatibility tests of old versions of joinmarket with ones using this version (specifically to make sure that the inter-participant message sign/verify is compatible, but also other functions).

This commit changes the underlying functions used in
jmbitcoin from the private and public key primitives
in coincurve and replaces them with equivalent
primitives CKey and CPubKey from python-bitcointx,
this removes the need to install coincurve and its
own bundled libsecp256k1 dynamic library.
Note that an additional pubkey_tweak_mul function
is exposed with ctypes from python-bitcointx's
bundled libsecp256k1 library.
@AdamISZ AdamISZ changed the title [WIP] Remove coincurve dependency, use python-bitcointx Remove coincurve dependency, use python-bitcointx Dec 30, 2021
@AdamISZ
Copy link
Member Author

AdamISZ commented Dec 30, 2021

As of 537e317 I'm now happy with merge of this.
Tested installation; tested cross compatibility of coinjoin in both directions, i.e. with taker side being this branch and maker side being master branch, and then vice versa.

Will do a few more tests of minor functions, i.e. various different wallet methods and payjoin, before actually merging.

Other review and test contributions would be very much appreciated!

@AdamISZ
Copy link
Member Author

AdamISZ commented Dec 30, 2021

Have now tested all the relevant wallet-tool methods (i.e. ones involving keys or transactions somehow), and payjoin between a master-branch receiver and a this-branch sender (which also tests the PSBT signing code under the hood). Everything seems to be working correctly.

@kristapsk
Copy link
Member

Concept ACK on removing extra dependencies. Will do some testing.

Copy link
Member

@kristapsk kristapsk left a comment

Choose a reason for hiding this comment

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

ACK 537e317. Didn't notice any problems in my tests.

@AdamISZ
Copy link
Member Author

AdamISZ commented Jan 3, 2022

Thanks for review, merging, will probably add #1098 after some more checking and testing.

@AdamISZ AdamISZ merged commit b161b10 into master Jan 3, 2022
@AdamISZ AdamISZ deleted the remove-coincurve branch January 3, 2022 11:06
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.

2 participants