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

Migrate from secp256k1 to secp256k1prp. #75

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

jhtitor
Copy link
Contributor

@jhtitor jhtitor commented Jan 10, 2019

secp256k1 is a python wrapper around a secp256k1 C library, which speeds up and provides various crypto functions to private key generation.

secp256k1prp is a fork, which wraps a similar C library, secp256k1-zkp, from bitshares-core, which adds support for Pederesen commitments / Range Proofs (hence, "prp").

As of time of this writing secp256k1prp is also more maintained than secp256k1, with pip wheel files provided for all major python platforms (wheel files contain the pre-compiled C library, for a specific platform, so the end-user doesn't have to compile it himself), see here: https://pypi.org/project/secp256k1prp/#files

This PR simply replaces secp256k1 with secp256k1prp, but it could possibly look for either.

@xeroc
Copy link
Owner

xeroc commented Feb 4, 2019

I would really love to add this patch. However, the issue is that secp256k1 is generally known, well maintained and trusted. Not saying that I don't trust you, but in order to be able to merge this patch, I'd need to be sure that the secp256k1 stuff is untouched and thus need ti review the entire fork. The impact on tampering with secp256k1 whould be much more massive.

How about using some sort of trigger that allows to switch the actual implementation from outside.
A simple USE_sec256k1prp=True for instance would do the trick.
I merely want to ensure that devs that don't need it, can stick with the regular sec256k1 implementation.

@jhtitor
Copy link
Contributor Author

jhtitor commented Feb 6, 2019

That makes total sense, I'll do it.

Regarding reviewing my fork, it's not that hard: the python-wrapper code I changed is nothing drastic, just version bumping and some build option adjustments. See for yourself! ludbb/secp256k1-py@master...jhtitor:master

Now, there are cffi bindings to the C code, and again, you may plainly see, that nothing suspicious is going on here, it's a 1:1 mapping.

Then, in setup.py, you will find this:

# Version of libsecp256k1 to download if none exists in the `libsecp256k1`
# directory
LIB_TARBALL_URL = "https://github.com/sipa/secp256k1-zkp/archive/35932bb24e83257b737a8ab4da0816972f4c252a.tar.gz"

That is a reference to a very specific commit to the zkp library (mind you -- the same library that is used in bitshares-core), the commit is sipa/secp256k1-zkp@35932bb . It is authored by Gregory Maxwell and is the defacto method for pedersen range proofs. There are other forks of the library, that also add this, but I've picked this commit specifically, because gmaxwell is a know and trusted member of the crypto community, and we basically live off his code anyways. (And I had the very same "fears" as you, I didn't want some random implementation, I wanted the implementation).

Also, for argument sake, if I turn evil, and release a bad future version, all this won't help. So I think maybe you could fork my repo and maintain it yourself? I've included build scripts to make windows and manylinux wheel files, it's pretty sweet.

That being said, I don't see myself ever touching this code again, it's stable, the pip files are built, and there's nothing more to do, really.

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