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

libsecp256k1 integration #4312

Merged
merged 3 commits into from Jul 2, 2014
Merged

libsecp256k1 integration #4312

merged 3 commits into from Jul 2, 2014

Conversation

@theuni
Copy link
Member

theuni commented Jun 9, 2014

@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

This comment has been minimized.

Copy link
Member

sipa commented Jun 9, 2014

Seems to work here, but pulltester is unhappy :(

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Jun 10, 2014

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

This comment has been minimized.

Copy link
Member

sipa commented Jun 10, 2014

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
luke-jr reviewed Jun 10, 2014
View changes
configure.ac Outdated
export -n LDFLAGS

ac_configure_args="${ac_configure_args} --disable-shared --with-pic"
AC_CONFIG_SUBDIRS([src/secp256k1])

This comment has been minimized.

Copy link
@luke-jr

luke-jr Jun 10, 2014

Member

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

This comment has been minimized.

Copy link
@sipa

sipa Jun 10, 2014

Member

Define "normal dependency"?

This comment has been minimized.

Copy link
@laanwj

laanwj Jun 10, 2014

Member

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.

This comment has been minimized.

Copy link
@luke-jr

luke-jr Jun 10, 2014

Member

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

This comment has been minimized.

Copy link
@laanwj

laanwj Jun 10, 2014

Member

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.

This comment has been minimized.

Copy link
@luke-jr

luke-jr Jun 10, 2014

Member

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

This comment has been minimized.

Copy link
@sipa

sipa Jun 10, 2014

Member

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

This comment has been minimized.

Copy link
@luke-jr

luke-jr Jun 10, 2014

Member

Not quite, it will reside in memory twice.

This comment has been minimized.

Copy link
@sipa

sipa Jun 10, 2014

Member

It's 30 kB of object code.

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

This comment has been minimized.

Copy link
@theuni

theuni Jun 10, 2014

Author Member

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

@luke-jr
luke-jr reviewed Jun 10, 2014
View changes
configure.ac Outdated
@@ -62,6 +63,12 @@ AC_ARG_ENABLE([upnp-default],
[use_upnp_default=$enableval],
[use_upnp_default=no])

AC_ARG_ENABLE([libsecp256k1],
[AS_HELP_STRING([--enable-libsecp256k1],

This comment has been minimized.

Copy link
@luke-jr

luke-jr Jun 10, 2014

Member

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

privkey.resize(279);
int privkeylen = 279;
int ret = secp256k1_ecdsa_privkey_export(begin(), (unsigned char*)&privkey[0], &privkeylen, fCompressed);
assert(ret);

This comment has been minimized.

Copy link
@luke-jr

luke-jr Jun 10, 2014

Member

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...

This comment has been minimized.

Copy link
@sipa

sipa Jun 10, 2014

Member

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

@theuni

This comment has been minimized.

Copy link
Member Author

theuni commented Jun 10, 2014

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

This comment has been minimized.

Copy link
Member

laanwj commented Jun 11, 2014

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

This comment has been minimized.

Copy link
Member

sipa commented Jun 11, 2014

Yes, this needs rebasing on top of #4277.

@theuni

This comment has been minimized.

Copy link
Member Author

theuni commented Jun 11, 2014

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

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Jun 11, 2014

Idea: call secp256k1_start from ECC_InitSanityCheck?

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

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Jun 12, 2014

@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

This comment has been minimized.

Copy link
Member

sipa commented Jun 12, 2014

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

This comment has been minimized.

Copy link
Contributor

pstratem commented Jun 15, 2014

*** 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

This comment has been minimized.

Copy link
Member

sipa commented Jun 15, 2014

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

@theuni

This comment has been minimized.

Copy link
Member Author

theuni commented Jun 15, 2014

@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

This comment has been minimized.

Copy link
Member

sipa commented Jun 15, 2014

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

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Jun 23, 2014

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

This comment has been minimized.

Copy link
Member Author

theuni commented Jun 24, 2014

Rebased.

@wtogami

This comment has been minimized.

Copy link
Contributor

wtogami commented Jun 24, 2014

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

This comment has been minimized.

Copy link
Member

laanwj commented Jun 25, 2014

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

This comment has been minimized.

Copy link
Member Author

theuni commented Jun 25, 2014

@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

This comment has been minimized.

Copy link
Member

laanwj commented Jun 26, 2014

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

This comment has been minimized.

Copy link
Member Author

theuni commented Jun 26, 2014

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

This comment has been minimized.

Copy link
Member Author

theuni commented Jun 26, 2014

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

This comment has been minimized.

Copy link
Member

sipa commented Jun 26, 2014

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

This comment has been minimized.

Copy link
Member

laanwj commented Jun 29, 2014

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.

theuni and others added 3 commits Jun 6, 2014
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

This comment has been minimized.

Copy link
Member Author

theuni commented Jul 1, 2014

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

This comment has been minimized.

Copy link

BitcoinPullTester commented Jul 1, 2014

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 laanwj merged commit fda3fed into bitcoin:master Jul 2, 2014
laanwj added a commit that referenced this pull request Jul 2, 2014
fda3fed libsecp256k1 integration (Pieter Wuille)
5566826 secp256k1: Add build-side changes for libsecp256k1 (Cory Fields)
b150b09 secp256k1: add libtool as a dependency (Cory Fields)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.