secp256k1

[AdamISZ]

I'm opening this issue to keep a log/update of progress on libsecp256k1 integration.

The branch is at https://github.com/AdamISZ/joinmarket/tree/secp256k1 .

An overview: First, the motivation: we need to use the best secure, tested, maintained, and conforming-to-standards code for Bitcoin signing and key management; in particular, the code for elliptic curve calculations and ECDSA operations. Hence the desire to use this library. However, this code does only that, it does not replace the code for transaction creation/manipulation, or for BIP32 (the HD style of wallets used by joinmarket).

So in this branch, all EC and ECDSA operations are moved to the module ec_ecdsa.py, which is a wrapper for the this python binding of the libsecp256k1 shared library. I've submitted a PR to add exposure of needed functionality here.

This will allow us, hopefully, to no longer use snapshots of nacl or secp256k1 (the latter is currently added into this branch) and use pip install instead. For pybitcointools, this will no longer be relevant since the library has been cannibalised.

Further in line with improving security/testing, I've added the bip32 test vectors to the library here (they passed).

There is other irrelevant code that could be moved from this version of bitcoin/ , for example blocks.py and composite.py (?). Opinions welcome, but not important.

I'm going to log the results of testing here. I plan to merge current master of joinmarket into this branch today.

[AdamISZ]

Tests so far:

Basic test/regtest.py : passing. ("Basic" means the version currently in master; I have a more sophisticated version with more coins in more coin depths, randomised, and more bot participants which can get merged sometime).

test/wallet-test.py : passing. (These tests for now are mostly about password and seed recovery. I want to add more, especially e.g. private key dump).

test/tumbler-test.py : passing.

The above 3 tests are of course running of a regtest blockchain.

I have also just tried a 3 bot test (2yg, 1sp) on testnet using blockr. Also passed.

I'll add to this when I have more. There are quite a lot of 'corner use cases' nowadays like using multisig, using Core wallet features. If you can add to this or help me test, please do.

[AdamISZ]

A note about installation of secp256k1 before I forget: secp256k1-py references all the components in libsecp256k1. So do:

$ ./autogen.sh
$ ./configure --enable-module-ecdh --enable-module-schnorr --enable-module-recovery
$ make
$ ./tests
$ sudo make install

This should put libsecp256k1.so into your shared library path.

[AdamISZ]

Auto-merged latest master. All above tests passing, will add newer regtest code into the branch also. I want to check (a) multisig and (b) Core wallet usage, as these seem very likely to have issues. Basic functionality is pretty clearly working at this point.

The other outstanding issue is donation code, which had to be commented out as it was using EC manipulations separate from BIP32. We'll have to figure out the right way to implement that.

[AdamISZ]

The changes needed for the donation code are committed here: https://github.com/AdamISZ/joinmarket/commit/b2cba5959152533a480713d9684cab46c355c65e

I want to make a few notes here as it's rather involved. In order for the mechanism to work, it's necessary to pass a value for the nonce ('k') into secp256k1. Understandably, this is not directly exposed in the interface (a big part of the point is to make signatures safer by using a standardised nonce generation function, for the moment this is only rfc6979 style). However, the interface does expose the ability to pass a function pointer and nonce data, which are then used to generate a nonce. In this commit I've used the cffi module (as in secp256k1-py) to create a nonce generating function that simply passes in the value in the variable ndata and then copies it into the nonce. This has been called via our python code using a nonce value of just os.urandom(32).

This of course makes things messier (although it only affects donation txs so is no danger to the rest of the code). Messier in the sense that I had to add code changes to secp256k1.py in the python binding which aren't probably going to get merged, so we might need to preserve our own custom copy.

I would have preferred not to have done this, but I don't see a better way for the moment.

The bright side is clear enough: there was no need to use pybitcointools version of rfc6979 (which is not complete), nor to reintroduce any EC or ECDSA calculations into the Python code. Only passing in os.urandom() as a nonce to the ecdsa signature, which strikes me as very unlikely to be an issue.

[AdamISZ]

(Related to the above, but this is ultra technical and probably irrelevant, so...) The nonce generating function is designed to receive a 'count' argument, so that if the nonce generated fails to generate a valid signature, it can be called again with a higher value of 'count'. The issue then is, if your nonce generating function pays no attention to that variable, and the signature it generates against the msg/privkey is invalid, then it will go into an infinite loop (see: https://github.com/bitcoin/secp256k1/blob/master/src/secp256k1.c#L357). As far as I can see, the only cases where it will fail is if r=0 or s=0 which is essentially impossible for a randomly generated input as we use. So as far as I can tell you hit an infinite loop with vanishingly small likelihood, and we can ignore it.

[AdamISZ]

Tests OK on tumbler-test, regtest and on testnet with blockr. I also tried sending to multisig on testnet just as a sanity check.

I believe this code is now ready for use - but how to get it to be easy to install is a different matter.

[AdamISZ]

This commit: https://github.com/AdamISZ/joinmarket/commit/698400d90c6baaf61c23127406c53f2d06118415 addresses a nasty compatibility issue.

In pybitcointools, message signing (as distinct from bitcoin transaction signing) was done with a different format. Until and unless all bots convert to the new version of the code, the message signatures and verifications have to use the 'legacy' style signature format; see the comments for more details here.

[AdamISZ]

Final (for now) test: In the real (mainnet pit): Ran sendpayment twice, and ran a yieldgenerator for a while, which did 6 joins OK.

This job is basically done now, a few things still to consider: a) the dump private key option in wallet-tool.py as I recall is currently only printing a hex privkey, not a bip32 format or WIF format. need to fix. b) clean up the file structure under bitcoin/ - remove unused modules and maybe merge ec_ecdsa.py into main.py, there is no reason really to have two separate modules. c) the big one - how to package for install. we need to run noncefunc_build.py to build the noncefunc.so (and need cffi module for that). We need the libsecp256k1.so library installed. Ideally we could use pip for secp256k1-py and nacl as mentioned above but the noncefunc makes that more problematic (maybe not a big deal, but anyway). d) I have not looked at how joinmarket is used in combination with a Bitcoin Core wallet. If someone could investigate that it'd be appreciated.

[AdamISZ]

a) the dump private key option in wallet-tool.py as I recall is currently only printing a hex privkey, not a bip32 format or WIF format. need to fix.

Done; back to WIF compressed.

[AdamISZ]

Progress in recent commits: Some tidy up, removed a lot of clutter in lib/bitcoin. Travis install and test working. Improvements in the regtest.py testing script. Comes a bit closer to being a full simulated pit. We need more extensive wallet tests though, but that's not really a matter for this thread. An install.sh script, just bare bones version, although it tested OK on debian already. arubi is working on polishing it, initially the goal would be for it to work on Linuces. Added dependencies: mainly libgmp-dev, libffi-dev and the secp256k1 repo.

I have repeated the above tests and things are working well. @chris-belcher I would like to merge in the next week if possible. Obv I will need to squash and do some rebasing I guess.

[AdamISZ]

303 - I am running this now. @fivepiece is working on the install script, it's mostly done I think. This (plus instructions in readme) can be added in here or added after.

[AdamISZ]

I have added the milestone but the status seems somewhat in doubt as long as no one has any suggestions on how to improve the installation.

[chris-belcher]

It might be worth writing about this issue on reddit/bitcointalk/twitter. Even if not everyone uses libsecp256k1 it still reduces any damage caused in the event of a crypto disaster from pybitcointools. Plus we have somewhat of a responsibility to warn people.

The yield generator operators may be more likely to update by hinting that pybitcointools is broken enough that they're about to get their investment stolen.

I've written this text which could be posted

So a couple of weeks ago, a patch was merged which made JoinMarket use libsecp256k1 as a cryptography library instead of pybitcointools. It wasn't quite ready, some people couldn't install it, so we reverted it.

You should all know why libsecp256k1 is a good idea, and maybe use it yourself, reporting any problems.

The current crypto library that JoinMarket uses is pybitcointools. Many believe that pybitcointools is a completely naive implementation, written by a single person, which has never been peer reviewed, which is internally undocumented and completely without tests, which was the authors first cryptographic code, by an author who'd previously written a number of completely broken wallet tools (e.g. reading the mouse position three times in a tight loop and adding math.random() to generate a private key)

libsecp256k1 by contrast is written by several experienced developers: gmaxwell, sipa, luke-jr, etc. It has been under development for two years and has recently been merged into Bitcoin Core.

The branch using libsecp256k1 is found here https://github.com/JoinMarket-Org/joinmarket/tree/secp256k1

[AdamISZ]

Yes. The text looks fine, perhaps link to https://github.com/bitcoin/secp256k1 . (side note: was luke-jr involved? no matter). I'd be happy to re-base it to the new develop branch with the refactoring, but would rather wait until the install issue was sorted out.

(The alternative approach of using python-bitcoinlib seems problematic although desirable; don't want to introduce an openssl dependency which would doubtless be superceded at some point anyway. When it switches to libsecp256k1 It could either pull in ludbb's python binding or build it's own from scratch; something that fundamental would just be @petertodd's decision anyway. And would have to get a bip32 PR pulled into that code. And it still needs more code (or we could continue to use the existing) to replicate the functionality we need in transaction.py.)

Perhaps the right thing to do is, to finish the 0.1.0 milestone without it, but to do the rebasing and keep the secp256k1 branch in sync.

[AdamISZ]

Removing this from 0.1.0 due to the installation problem.

[mb300sd]

Has there been any consideration of re-merging this with a cleaner install method? I've been using this version for a couple months now, and just pulled the latest master to try out. First thing I noticed was how much slower it was, which wasn't that much of an issue before the secp256k1 merge, but my yield generator wallet has grown significantly since then.

[AdamISZ]

@mb300sd As you can see from the above, that was the intention. See here for an issue that arose from the proposed process being unreliable.

I don't have the skillset to make an install process that is reliable even across Linuces, not to speak of other OSs. I was hoping that there would be more progress on this, but so far it hasn't happened.

[chris-belcher]

Discussion today

<belcher> waxwing iv been playing with your secp256k1 branch
<belcher> is it enough to just run install.sh ? do you remember
<waxwing> belcher: well that's the part that proved problematic. it was working for me, yeah, it should install dependencies.
<waxwing> but at least 1 or 2 other people, the install failed somewhere.
<belcher> right now i get ImportError: /home/username/coding/joinmarket/secp256k1/lib/bitcoin/_libsecp256k1.so: undefined symbol: secp256k1_ecdsa_signature_normalize
<waxwing> also libsecp256k1 will have moved forwards
<belcher> oh right
<waxwing> ah yeah, doubtless that's connected to the python binding code.
<belcher> i actually meant to just use libsecp256k1 for something else
* rdymac (uid31665@gateway/web/irccloud.com/x-tapeviewurnfjhfh) has joined #joinmarket
<waxwing> part of the issue was i couldn't pull the ludbb binding cleanly for two separate reasons
<waxwing> one was the nonce requirement for the donation code and the other was there's something wrong with the ludbb code itself
<waxwing> or is it luddb whatever :)
<belcher> luddb is python bindings for libsecp256k1
<waxwing> i should make a PR for that performance error it had, that's simple in principle
<belcher> im just trying to understanding python binding
<waxwing> but the nonce thing is more substantial, somehow.
<waxwing> it uses ctypes or that other one, i forget the name (2 ways to make C calls from Python)
<waxwing> if there's something specific you want to do now, tell me what it is, i might be able to help
<belcher> oh wait it just worked
<belcher> i added the stuff you write in .bashrc
<waxwing> arubi did that stuff
<arubi> I did some bash stuff, yes.
<belcher> waxwing so just to remind me, the nonce stuff is about making secp256k1 access a python function that returns a nonce ?
<waxwing> cffi was the one they use instead of ctypes
<waxwing> belcher: yeah you're allowed to pass a custom nonce function instead of the standard 6979
<belcher> yep, and that needs some bindings which dont appear in luddb
<belcher> thats not specific to joinmarket is it? that feature is in secp256k1 itself
<waxwing> yes, but it's not in the binding
<waxwing> https://github.com/AdamISZ/secp256k1-py/blob/master/secp256k1.py#L320-L321
<waxwing> oh sorry that's my fork, doh
<belcher> what is it instead of the binding ?
<waxwing> https://github.com/ludbb/secp256k1-py/blob/master/secp256k1/__init__.py#L387-L388
<waxwing> you mean what's the default nonce function? rfc6979
<waxwing> it's just passing null in the above, so uses default. iirc.
<belcher> so secp256k1 has a feature to pass in your own nonce function, but luddb doesnt support it for use with python
<belcher> ?
<belcher> here is it in c https://github.com/bitcoin/secp256k1/blob/master/include/secp256k1.h#L464
<waxwing> yes it's not in there, unless they added it recently and i missed it 
<waxwing> i put it in in my secp256k1 branch code
<belcher> just checking when you say "secp256k1 branch code" you mean the python bindings from luddb ? not the c code from github.com/bitcoin/secp256k1
<waxwing> yes of course it's in the libsecp256k1 library :)
<waxwing> here's how i put it in the secp256k1 branch
<belcher> ok so, could we write a patch for luddb's bindings and try to get them included with a PR ?
<waxwing> https://github.com/AdamISZ/joinmarket/blob/issue291/lib/bitcoin/secp256k1.py#L384
<belcher> since its already a feature of the bitcoin/libsecp256k1 library
<waxwing> yes we could, i just couldn't bring myself to do it at that stage. i already made a small PR there. i just put it off. the whole thing was a lot of work.
<belcher> ok sure, it doesnt have to be you it can be anyone
<belcher> thats an issue right now isnt it? users need their own copy of luddb's bindings and also compile their own ?
<waxwing> i'd first want to make a PR to fix the performance hit of recreating contexts unnecessarily.
<waxwing> (which is a much bigger thing than it seems)
<belcher> sure
<belcher> wait am i remember right? some stuff needs to be compiled
<belcher> i just compiled something on ubuntu with install.sh i think
<waxwing> i think there's actually been some process recently on that binding library; somebody made a PR to do a binary build I think.
<belcher> the easy way to install libsodium on windows was just to download the binary
<waxwing> yes belcher we had it set up to build libsecp56k1, there's no binary release currently
<belcher> ah i see
<belcher> yes no wonder it was hard
<belcher> compiling stuff on windows is difficult iv always found, it doesnt have gcc or visual studios by default
<waxwing> yes, i can't even remember how i did it, but there are some notes.
<belcher> you made it work on windows ?
<waxwing> oh yes i remember now you can compile on linux with certain parameters to make the windows dll.
<belcher> hah ok
<belcher> also another thing we can do to make it easier for people is keep pybitcointools too in case people really cant make libsecp256k1 work
<waxwing> if you want to push forward with that again, i think the first thing to do might be to investigate that PR to ludbb that produced a binary install (i hope I read that right..)
<belcher> not right now, but eventually
<waxwing> it really seems like making robust installs for linux distros is a pain.

[Ghesmati]

Installing JM V.0.8.2 on windows 10 pip install -r requirements\base.txt and pip install -r requirements\gui.txt were successfully done.

running ~ /scripts>python joinmarket-qt.py results in the following error:

 _secp256k1 = load_secp256k1_library(bitcointx.util._secp256k1_library_path)
  File "C:\Users\...\AppData\Local\Programs\Python\Python36\lib\site-packages\bitcointx\core\secp256k1.py", line 232, in load_secp256k1_library
    raise ImportError('secp256k1 library not found')
ImportError: secp256k1 library not found

[AdamISZ]

Please reopen on the other repository (see README for link).