libsecp256k1 integration by theuni · Pull Request #4312 · bitcoin/bitcoin

theuni · 2014-06-09T16:47:00Z

@sipa suggested I go ahead and PR this to get discussion going. It's his work, I've just integrated it into our build-system.
This was done in a few stages:

Add libtool dependency. I've tried to update all relevant docs, but very possible I missed some. Note that we don't actually make real use of libtool yet, it's only used when linking the subproject.
Pull in libsecp256k1 subtree
Add configure option "--enable-libsecp256k1". This is disabled by default.
Update the makefiles to link against the lib where necessary
Finally, add in the actual implementation.

sipa · 2014-06-09T19:37:06Z

Seems to work here, but pulltester is unhappy :(

laanwj · 2014-06-10T06:28:33Z

Pulltester problem doesn't look too bad:

make[2]: Entering directory `/mnt/bitcoin/src/secp256k1'
make[2]: *** No rule to make target `distdir'.  Stop.

sipa · 2014-06-10T07:31:55Z

By the way, at least initially I would prefer it if --enable-libsecp256k1 was mutually incompatible with --enable-wallet. Ideally, there would be a way to remove the mining code too :)

luke-jr · 2014-06-10T07:37:49Z

NACK, just pull in libsecp256k1 as a normal dependency. No subtree garbage necessary.

Define "normal dependency"?

Including it as subtree is better IMO. It leaves us control over the code (which is one of the reasons for moving away from OpenSSL in the first place!), and avoids that the person building it has to install a dependency that's pretty much unknown right now.

@sipa Like Qt, boost, or any other dependency.

@laanwj We already have "control over the code" - @sipa is upstream! People building code are prepared to deal with dependencies.

Subtree messes just _de_modularise the code into one big ugly blob. Our goal should be modularising, not the opposite!

But something could be said that secp256k is in exactly the same spot as leveldb. It's critical to the consensus.
Sure, @sipa is upstream here which helps, but by including the code we avoid having to take version mismatches into account, for example. We test with one bitcoind-secp256k-leveldb combination and everyone will use that.

There is plenty to be gained by modularising it. Other software can use bits.

Other software can use libsecp256k1 regardless of whether we subtree it or not.

Not quite, it will reside in memory twice.

It's 30 kB of object code.

And 600 kB of precomputed data (which wouldn't be shared across processes anyway).

I'm pretty indifferent on the matter, I just hooked it up as requested. Either outcome is fine by me.

theuni · 2014-06-10T17:16:53Z

The pull-tester issue is actually a pretty nasty problem. I'm still debating which "fix" to use, I'll push something up soon.

laanwj · 2014-06-11T12:13:33Z

I get the following error with

#!/bin/bash
PARALLELISM=6
./autogen.sh && ./configure --with-gui=qt5 --enable-libsecp256k1 --disable-wallet && make clean && make -j${PARALLELISM}

make[2]: Entering directory `/store/orion/projects/bitcoin/bitcoin/src'
  AR       libbitcoin_server.a
  CXX      libbitcoin_common_a-key.o
key.cpp: In function ‘bool ECC_InitSanityCheck()’:
key.cpp:740:5: error: ‘EC_KEY’ was not declared in this scope
     EC_KEY *pkey = EC_KEY_new_by_curve_name(NID_secp256k1);
     ^
key.cpp:740:13: error: ‘pkey’ was not declared in this scope
     EC_KEY *pkey = EC_KEY_new_by_curve_name(NID_secp256k1);
             ^
key.cpp:740:45: error: ‘NID_secp256k1’ was not declared in this scope
     EC_KEY *pkey = EC_KEY_new_by_curve_name(NID_secp256k1);
                                             ^
key.cpp:740:58: error: ‘EC_KEY_new_by_curve_name’ was not declared in this scope
     EC_KEY *pkey = EC_KEY_new_by_curve_name(NID_secp256k1);
                                                          ^
key.cpp:743:21: error: ‘EC_KEY_free’ was not declared in this scope
     EC_KEY_free(pkey);
                     ^
make[2]: *** [libbitcoin_common_a-key.o] Error 1

(Ubuntu 14.04 AMD64)
Edit: oops, yes, as @sipa notes below I've tested a version that was merged into master, instead of your branch directly, I should mention that.

sipa · 2014-06-11T12:16:10Z

Yes, this needs rebasing on top of #4277.

theuni · 2014-06-11T18:37:02Z

rebased. I made a minimal change for @laanwj's error and squashed it into the first commit.

sipa · 2014-06-11T23:37:53Z

Idea: call secp256k1_start from ECC_InitSanityCheck?

That way, we don't spend the time for building the precomputation tables in bitcoin-cli...

laanwj · 2014-06-12T02:48:07Z

@sipa Good point, though I'm not sure the sanity check function is the best place.
I think in general it would be good to move from 'hidden' static classes with constructors and destructors (CInit in util.cpp, CMainCleanup in main.cpp, and now CSecp256k1Init) and implicit initialization (GetDataDir and al) to explicit initialization and shutdown sequences. It's easier (well: possible!) to track of the sequence and what depends on what.

Something could be said that bitcoin-cli shouldn't be linking key.cpp at all. But that's an issue for another day :)

sipa · 2014-06-12T08:13:28Z

Agree, I'm fine with an explicit initializer/finalizer as well, independent from the ecc check.

And of course, bitcoin-cli shouldn't use key.cpp at all, but I don't know how far we're away from that.

pstratem · 2014-06-15T21:18:43Z

*** Error in `bitcoin/src/bitcoind': munmap_chunk(): invalid pointer: 0x0000000000a2e430 ***

#0 0x00007ffff5d71f79 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56
#1 0x00007ffff5d75388 in __GI_abort () at abort.c:89
#2 0x00007ffff5daf1d4 in __libc_message (do_abort=do_abort@entry=1, fmt=fmt@entry=0x7ffff5ebda10 "* Error in `%s': %s: 0x%s _\n") at ../sysdeps/posix/libc_fatal.c:175
#3 0x00007ffff5db9f37 in malloc_printerr (action=, str=0x7ffff5ebdd90 "munmap_chunk(): invalid pointer", ptr=) at malloc.c:4996
#4 0x00007ffff6b9ef4d in CRYPTO_free (str=0xa2e430) at mem.c:397
#5 0x00007ffff6bd7a05 in BN_free (a=0xa46670) at bn_lib.c:261
#6 0x00000000005f9a86 in secp256k1_num_free (r=0xa46670) at src/num_openssl_impl.h:21
#7 secp256k1_ge_stop () at src/group_impl.h:396
#8 secp256k1_stop () at src/secp256k1.c:19
#9 0x00007ffff5d77509 in __run_exit_handlers (status=0, listp=0x7ffff60fa6c8 <__exit_funcs>, run_list_atexit=run_list_atexit@entry=true) at exit.c:82
#10 0x00007ffff5d77555 in __GI_exit (status=) at exit.c:104
#11 0x00007ffff5d5cecc in _libc_start_main (main=0x41bf10 <main(int, char*)>, argc=2, argv=0x7fffffffe408, init=, fini=, rtld_fini=, stack_end=0x7fffffffe3f8) at libc-start.c:321
#12 0x0000000000424d19 in _start ()

sipa · 2014-06-15T21:45:31Z

See bitcoin-core/secp256k1#28 for a fix.

theuni · 2014-06-15T22:05:36Z

@sipa Would you prefer that I sync and rebase this as these come up? Or leave it as-is with the understanding that a sync will be done before merge?

sipa · 2014-06-15T23:04:46Z

@theuni Up to you. Constantly rebasing will take work, but may make some things easier to review.

laanwj · 2014-06-23T09:28:21Z

Needs rebase.
Strange: I tried a rebase to see where the conflict is with, but rebase gets confused between src/util.h and src/secp256k1/util.h!

theuni · 2014-06-24T20:55:15Z

Rebased.

wtogami · 2014-06-24T21:36:42Z

2014-06-24 21:30:27 Bitcoin version v0.9.99.0-2e04a3d (2014-06-24 16:52:34 -0400)
2014-06-24 21:30:27 Using OpenSSL version OpenSSL 1.0.1e-fips 11 Feb 2013
2014-06-24 21:30:27 Using BerkeleyDB version Berkeley DB 4.8.30: (August  3, 2013)

Could you add a print during runtime to display the fact that it is using libsecp256k1 similar to these others? Otherwise it is difficult to know which library you are using.

laanwj · 2014-06-25T08:07:58Z

I want to merge this as soon as possible due to the build system changes. Is it ready @theuni?

Improvements like @wtogami's (which I fully agree with) can be done later.

theuni · 2014-06-25T21:27:06Z

@laanwj From the build-side, I think it's ok to merge. Up to @sipa on the readiness of the lib itself.

Are you just worried about the (time) cost of rebasing this for build-system changes? If so, I can split it into 2 parts: Build-system changes that are useless without the 2nd part, and the actual secp and configure switch addition. We could merge the first now to prevent future rebasing, then do the 2nd whenever @sipa is happy.

laanwj · 2014-06-26T05:29:47Z

If there is a clear condition when the lib is ready we can wait for that, I just want to avoid keeping this open up to some arbitrary time. My belief at the start of this project was that secp256k1 was ready for experimental use in bitcoin core.

So once the code review comments with regard to the integration are processed, and things are not obviously crashing or failing, I think we should aim to merge this. We can then periodically update the secp256k1 lib from upstream.

Splitting it into two parts, a build system change and the rest, also sounds good to me.

theuni · 2014-06-26T16:32:02Z

Ok, I'll split this into 2 PRs. The build-side can be merged, then the discussion can be about the readiness of secp256k1 itself.

theuni · 2014-06-26T22:09:37Z

Split up. @laanwj You can pull as much of this one as you want whenever you're ready. It's a complete no-op without the lib itself and the configure option.

The rest is here (on top of this PR) for reference: https://github.com/theuni/bitcoin/commits/secp256k1-integration-part2

sipa · 2014-06-26T23:49:24Z

Sorry for commenting late.

If we want to merge for experimental use, fine by me. I'd like to make it disable wallet and mining when doing so, but perhaps people compiling from source could easily bypass that anyway.

I do plan to change the API significantly at some point, but everything is functional now, and that won't be immediately. If we want to wait for something like an actual "release" of libsecp256k1... that may take a while, and the glue code would probably need to be different too.

laanwj · 2014-06-29T08:22:36Z

Will merge this after rebase.

I definitely don't think we should wait for an actual "release" of libsecp256k1. The more testing it gets the better.

Note: This is added to our existing automake targets rather than as a libtool-style lib. The switch to libtool-style targets can come later if it proves to not add any complications.

theuni · 2014-07-01T16:33:33Z

Rebased and ready for merge afaik.
@sipa Would you prefer to take the top commits here and PR it yourself so that I'm not in the way of it? https://github.com/theuni/bitcoin/commits/secp256k1-integration-part2

Otherwise, if you're ok with it as-is, I can just PR those once this one is merged.

BitcoinPullTester · 2014-07-01T16:49:06Z

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4312_fda3fed18aedc4bfc8ccffe89d8d2cabb12677ab/ for binaries and test log.
This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.

fda3fed libsecp256k1 integration (Pieter Wuille) 5566826 secp256k1: Add build-side changes for libsecp256k1 (Cory Fields) b150b09 secp256k1: add libtool as a dependency (Cory Fields)

luke-jr reviewed Jun 10, 2014
View reviewed changes

theuni and others added 3 commits July 1, 2014 12:27

secp256k1: add libtool as a dependency

b150b09

secp256k1: Add build-side changes for libsecp256k1

5566826

Note: This is added to our existing automake targets rather than as a libtool-style lib. The switch to libtool-style targets can come later if it proves to not add any complications.

libsecp256k1 integration

fda3fed

laanwj merged commit fda3fed into bitcoin:master Jul 2, 2014

laanwj added a commit that referenced this pull request Jul 2, 2014

Merge pull request #4312

c9600ce

fda3fed libsecp256k1 integration (Pieter Wuille) 5566826 secp256k1: Add build-side changes for libsecp256k1 (Cory Fields) b150b09 secp256k1: add libtool as a dependency (Cory Fields)

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Uh oh!

Conversation

theuni commented Jun 9, 2014

Uh oh!

sipa commented Jun 9, 2014

Uh oh!

laanwj commented Jun 10, 2014

Uh oh!

sipa commented Jun 10, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theuni commented Jun 10, 2014

Uh oh!

laanwj commented Jun 11, 2014

Uh oh!

sipa commented Jun 11, 2014

Uh oh!

theuni commented Jun 11, 2014

Uh oh!

sipa commented Jun 11, 2014

Uh oh!

laanwj commented Jun 12, 2014

Uh oh!

sipa commented Jun 12, 2014

Uh oh!

pstratem commented Jun 15, 2014

Uh oh!

sipa commented Jun 15, 2014

Uh oh!

theuni commented Jun 15, 2014

Uh oh!

sipa commented Jun 15, 2014

Uh oh!

laanwj commented Jun 23, 2014

Uh oh!

theuni commented Jun 24, 2014

Uh oh!

wtogami commented Jun 24, 2014

Uh oh!

laanwj commented Jun 25, 2014

Uh oh!

theuni commented Jun 25, 2014

Uh oh!

laanwj commented Jun 26, 2014

Uh oh!

theuni commented Jun 26, 2014

Uh oh!

theuni commented Jun 26, 2014

Uh oh!

sipa commented Jun 26, 2014

Uh oh!

laanwj commented Jun 29, 2014

Uh oh!

theuni commented Jul 1, 2014

Uh oh!

BitcoinPullTester commented Jul 1, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects