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

secp256k1: replace local version with nixpkgs #371

Closed
wants to merge 1 commit into from

Conversation

prusnak
Copy link
Contributor

@prusnak prusnak commented Jul 16, 2021

nixpkgs contains newer version, so we can upgrade

Copy link
Member

@jonasnick jonasnick left a comment

Choose a reason for hiding this comment

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

ACK f00061f

@nixbitcoin
Copy link
Member

We use this for JoinMarket because JoinMarket uses rev 0d9540b13ffcd7cd44cc361b8744b93d88aa76ba. Could using a different secp256k1 version lead to problems @kristapsk @AdamISZ?

@kristapsk
Copy link

@nixbitcoin Likely not, but somebody needs to go through changes and test it carefully.

@AdamISZ
Copy link

AdamISZ commented Jul 20, 2021

(Background: I'll have to go back and check but as I recall fixing that version because coincurve did, and in the absence of secp256k1 releases there was no simple step-change after that. So we should double check if coincurve ever changed it in an update. But complicating this issue considerably: we've almost completely removed the coincurve dependency in Joinmarket (replaced with Simplexum/python-bitcointx), but unfortunately not yet 100%. The main thing that could go wrong is in that dependency; our use of the secp256k1 API is pretty simple.
Honestly if I had more time I think I could both answer your question, and remove the coincurve dependency which over complicates our back end, as concrete steps but it'd require some work and I'd love someone else to do it).

In short: I agree with @kristapsk . It needs to be tested. As well as checking/updating the install script, the test suite is probably sufficient for this though (since it's low level, manual user-level testing isn't needed imo).

@kristapsk
Copy link

Coincurve has updated secp256k1 to f2d9aeae6d5a7c7fbbba8bbb38b1849b784beef7 in January, so guess that should be safe to use.

@prusnak
Copy link
Contributor Author

prusnak commented Jul 20, 2021

Related: the library maintainers are considering providing tagged versions soon (bitcoin-core/secp256k1#286 + bitcoin-core/secp256k1#856), so the situation with packaging the library hopefully gets better soon.

@jonasnick
Copy link
Member

@nixbitcoin makes a good point. Since libsecp is unreleased, the libraries should come with their own, vendored version of libsecp. I think coincurve does and python-bitcointx doesn't. I hear that python-bitcointx doesn't recommend a specific commit of libsecp and it's test coverage is only "decent". So, I think at this point it's best is to go with whatever joinmarket suggests using. Thanks for your input @kristapsk @AdamISZ and thanks for opening this PR @prusnak.

I hope the libsecp maintainers get their act together.

@jonasnick jonasnick closed this Jul 20, 2021
@prusnak
Copy link
Contributor Author

prusnak commented Jul 20, 2021

I agree with the decision to close this PR and I will re-open the PR when libsecp256k1 has a proper versioned release.

@nixbitcoin
Copy link
Member

I think coincurve does and python-bitcointx doesn't

Do you know how coincurve gets its libsecp?

@jonasnick
Copy link
Member

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.

5 participants