libsecp256k1 integration #4312

theuni commented at 4:47 PM on June 9, 2014: member

@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 commented at 7:37 PM on June 9, 2014: member

Seems to work here, but pulltester is unhappy :(

laanwj commented at 6:28 AM on June 10, 2014: member

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 commented at 7:31 AM on June 10, 2014: member

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 :)

in configure.ac:None in 98ad8d7024 outdated

 725 | +  export -n CPPFLAGS
 726 | +  export -n LIBS
 727 | +  export -n LDFLAGS
 728 | +
 729 | +  ac_configure_args="${ac_configure_args} --disable-shared --with-pic"
 730 | +  AC_CONFIG_SUBDIRS([src/secp256k1])

luke-jr commented at 7:37 AM on June 10, 2014:

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

sipa commented at 7:39 AM on June 10, 2014:

Define "normal dependency"?

laanwj commented at 7:39 AM on June 10, 2014:

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.

luke-jr commented at 7:42 AM on June 10, 2014:

@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!

laanwj commented at 7:47 AM on June 10, 2014:

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.

luke-jr commented at 7:53 AM on June 10, 2014:

LevelDB shouldn't be subtree'd either. If you want to tie release code to a specific version, just put it in the configure.ac as a requirement... but that's a bad idea IMO. Especially for libsecp256k1 since upstream understands consensus-critical needs.

laanwj commented at 8:53 AM on June 10, 2014:

I'm all for modularizing where it makes sense (eject the wallet today!). But the consensus logic by nature is an ugly atomic blob. There's nothing to be gained by modularizing it.

luke-jr commented at 9:16 AM on June 10, 2014:

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

sipa commented at 9:36 AM on June 10, 2014:

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

luke-jr commented at 9:37 AM on June 10, 2014:

Not quite, it will reside in memory twice.

sipa commented at 11:19 AM on June 10, 2014:

It's 30 kB of object code.

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

theuni commented at 8:05 PM on June 10, 2014:

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

in configure.ac:None in 98ad8d7024 outdated

  62 | @@ -62,6 +63,12 @@ AC_ARG_ENABLE([upnp-default],
  63 |    [use_upnp_default=$enableval],
  64 |    [use_upnp_default=no])
  65 |  
  66 | +AC_ARG_ENABLE([libsecp256k1],
  67 | +  [AS_HELP_STRING([--enable-libsecp256k1],

luke-jr commented at 7:38 AM on June 10, 2014:

Dependencies are supposed to be AC_ARG_WITH/--with-libsecp256k1

in src/key.cpp:None in 98ad8d7024 outdated

 433 | @@ -408,99 +434,167 @@ bool CKey::SetPrivKey(const CPrivKey &privkey, bool fCompressedIn) {
 434 |  
 435 |  CPrivKey CKey::GetPrivKey() const {
 436 |      assert(fValid);
 437 | +    CPrivKey privkey;
 438 | +#ifdef USE_SECP256K1
 439 | +    privkey.resize(279);
 440 | +    int privkeylen = 279;
 441 | +    int ret = secp256k1_ecdsa_privkey_export(begin(), (unsigned char*)&privkey[0], &privkeylen, fCompressed);
 442 | +    assert(ret);

luke-jr commented at 7:39 AM on June 10, 2014:

Should secp256k1_ecdsa_privkey_export really NEVER return NULL in ordinary circumstances here? If there's a normal condition it might, this shouldn't be an assert...

sipa commented at 7:44 AM on June 10, 2014:

Currently there is no code path that could make it return 0 even.

theuni commented at 5:16 PM on June 10, 2014: member

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 commented at 12:13 PM on June 11, 2014: member

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 commented at 12:16 PM on June 11, 2014: member

Yes, this needs rebasing on top of #4277.

theuni commented at 6:37 PM on June 11, 2014: member

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

sipa commented at 11:37 PM on June 11, 2014: member

Idea: call secp256k1_start from ECC_InitSanityCheck?

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

laanwj commented at 2:48 AM on June 12, 2014: member

@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 commented at 8:13 AM on June 12, 2014: member

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 commented at 9:18 PM on June 15, 2014: contributor

*** 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=<optimized out>, str=0x7ffff5ebdd90 "munmap_chunk(): invalid pointer", ptr=<optimized out>) 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=<optimized out>) at exit.c:104 #11 0x00007ffff5d5cecc in __libc_start_main (main=0x41bf10 <main(int, char_*)>, argc=2, argv=0x7fffffffe408, init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>, stack_end=0x7fffffffe3f8) at libc-start.c:321 #12 0x0000000000424d19 in _start ()

sipa commented at 9:45 PM on June 15, 2014: member

See bitcoin/secp256k1#28 for a fix.

theuni commented at 10:05 PM on June 15, 2014: member

@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 commented at 11:04 PM on June 15, 2014: member

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

laanwj commented at 9:28 AM on June 23, 2014: member

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 commented at 8:55 PM on June 24, 2014: member

Rebased.

wtogami commented at 9:36 PM on June 24, 2014: contributor

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 commented at 8:07 AM on June 25, 2014: member

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 commented at 9:27 PM on June 25, 2014: member

@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 commented at 5:29 AM on June 26, 2014: member

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 commented at 4:32 PM on June 26, 2014: member

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 commented at 10:09 PM on June 26, 2014: member

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 commented at 11:49 PM on June 26, 2014: member

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 commented at 8:22 AM on June 29, 2014: member

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.

secp256k1: add libtool as a dependency b150b09edc

secp256k1: Add build-side changes for libsecp256k1

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.

5566826635

libsecp256k1 integration fda3fed18a

theuni commented at 4:33 PM on July 1, 2014: member

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 commented at 4:49 PM on July 1, 2014: none

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.

laanwj merged this on Jul 2, 2014

laanwj closed this on Jul 2, 2014

laanwj referenced this in commit c9600ce640 on Jul 2, 2014

MarcoFalke locked this on Sep 8, 2021