key.cpp: fail with a friendlier message on missing ssl EC support #4277

apoelstra commented at 11:25 PM on June 2, 2014: contributor

Previously if bitcoind is linked with an OpenSSL which is compiled without EC support, this is seen as an assertion failure "pKey != NULL" at key.cpp:134. It is an esoteric piece of knowledge to interpret this as "oops, I linked with the wrong OpenSSL".

in src/key.cpp:None in 5429143a9e outdated

 130 | @@ -131,7 +131,10 @@ class CECKey {
 131 |  public:
 132 |      CECKey() {
 133 |          pkey = EC_KEY_new_by_curve_name(NID_secp256k1);
 134 | -        assert(pkey != NULL);
 135 | +        if(pkey == NULL) {
 136 | +            fprintf(stderr, "Fatal error: OpenSSL appears to lack support for curve secp256k1.\n");
 137 | +            assert(0);

luke-jr commented at 11:28 PM on June 2, 2014:

assert(0) is not abort()

apoelstra commented at 11:40 PM on June 2, 2014: contributor

Thanks Luke, I didn't know about <code>abort</code>. Fixed.

in src/key.cpp:None in 0c057e2560 outdated

 130 | @@ -131,7 +131,10 @@ class CECKey {
 131 |  public:
 132 |      CECKey() {
 133 |          pkey = EC_KEY_new_by_curve_name(NID_secp256k1);
 134 | -        assert(pkey != NULL);
 135 | +        if(pkey == NULL) {
 136 | +            fprintf(stderr, "Fatal error: OpenSSL appears to lack support for curve secp256k1.\n");

luke-jr commented at 2:16 AM on June 3, 2014:

Should probably use the logging functions... Maybe explain a bit about how the user may consider solving it?

apoelstra commented at 4:04 AM on June 3, 2014: contributor

Thanks for the suggestion, @luke-jr. I've updated the PR to use <code>uiInterface.ThreadSafeMessageBox</code> which is the logging function used for other failures in <code>init.cpp</code>, and also added a URL to a wiki page describing the problem and available workarounds.

laanwj commented at 6:52 AM on June 3, 2014: member

Instead of deferring this to the last moment and create a dependency of the key.c/h on UI interface, I'd prefer adding this check (and potentially other sanity checks, see #4081) earlier in the init sequence, for example in AppInit2 before any key objects are instantiated.

key.cpp: fail with a friendlier message on missing ssl EC support

Previously if bitcoind is linked with an OpenSSL which is compiled
without EC support, this is seen as an assertion failure "pKey !=
NULL" at key.cpp:134, which occurs after several seconds. It is an
esoteric piece of knowledge to interpret this as "oops, I linked
with the wrong OpenSSL", and because of the delay it may not even
be noticed.

The new output is

: OpenSSL appears to lack support for elliptic curve cryptography. For
more information, visit
https://en.bitcoin.it/wiki/OpenSSL_and_EC_Libraries
: Initialization sanity check failed. Bitcoin Core is shutting down.

which occurs immediately after attempted startup.

This also blocks in an InitSanityCheck() function which currently only
checks for EC support but should eventually do more. See #4081.

4a09e1df51

apoelstra commented at 7:15 PM on June 3, 2014: contributor

Ok, I've refreshed the PR; I left the assertion in place and added an <code>InitSanityCheck</code> function to init.cpp which does the check. The failure is now both clean and immediate (the old assertion failure took several seconds to trigger on my machine). The function <code>InitSanityCheck</code> is also a clear place to add the additional checks suggested in #4081.

sipa commented at 7:47 PM on June 3, 2014: member

Untested ACK

BitcoinPullTester commented at 8:06 PM on June 3, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/4a09e1df51267c6d2dec219c6f96a24b716cc251 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.

apoelstra commented at 8:13 PM on June 3, 2014: contributor

Oh, I should probably mention that I've tested this (and every candidate along the way) with a good and bad OpenSSL library. It works correctly in both cases.

laanwj commented at 6:48 AM on June 4, 2014: member

Code changes look good to me.

laanwj merged this on Jun 11, 2014

laanwj closed this on Jun 11, 2014

laanwj referenced this in commit d01574f792 on Jun 11, 2014

MarcoFalke locked this on Sep 8, 2021