[Consensus] Fix compilation with OpenSSL 1.1

[Warrows]

What prompted me to address the situation is related to issue #149
Let's give a bit of context:
In bitcoin, openssl was used in consensus critical code. Following the infamous heartbleed vulnerability discovered in openssl code, libressl was created as a fork to refactor and improve ssl code. To this day, libressl is used as default instead of openssl in the os openBSD. As far as crypto are concerned: the openssl hotfix was integrated into bitcoin promptly. The change in consensus code triggered a fork. bip 66 was then decided, removing openssl code from consensus critical area and using custom implementation by core team instead. This bip was backported in dash commit 3d004ab which we inherited. So good so far.
The thing is, before this bip a limitation was put into the autoconf to avoid compiling consensus critical code with the new lib libressl. We directly inherited this limitation from bitcoin core 0.10. It was removed from bitcoin core in commit a3874c7. Too late for us. Since then, anyone wanting to compile PIVX with libressl will get the warning as seen in #149. And this has never been removed. But this warning is not cleanly looking for libressl, it is using a workaround, testing the absence of a specific function existing in openssl 1.0 but removed in both libressl and openssl 1.1 prompting you to get a libressl warning if you are using openssl 1.1. The simple workaround would be to altogether remove the warning, as done by bitcoin.
But the thing which leaves me wondering in this situation is that when implementing libzerocoin, we came to depend on openssl again. Depend so much that we can't compile with openssl 1.1. Even by ignoring the warning thanks to the --with-libressl argument. This pull request aim to fix this compilation problem. I am not fully aware of the internal workings of the zerocoin implementation but surely, since it's burning and creating coins, it is influencing consensus.

My advice would be: after merging this, either try to totally remove openssl dependency from any consensus critical code (especially related to zerocoin). If not possible, and anyway, as a short term fix: change the alert message and argument when compiling as to include openssl 1.1 beside libressl as potentially consensus breaking. This would allow to quickly close issue #149

About testing: I compiled and ran unit tests on debian with both openssl 1.0 and 1.1. I did not test libressl and I'd prefer if someone is able to try it on openbsd. I also minted and spent some zPIV on testnet without problem.

[presstab]

Hmm interesting.

@rejectedpromise please take a look at this PR, you have been dealing with the crypto side of the wallet recently, see if anything jumps out at you.

[rejectedpromise]

• added to my todo list

[rejectedpromise]

Changes to CBigNum scare me because of the heavy zPIV dependency on it. However, I do agree with moving towards removing dependencies on openssl, as bitcoin and others have done.

utACK, hope to pull and test this weekend, but have a long todo list ;)

[Fuzzbawls]

While moving away from OpenSSL may be a good long-term goal, it certainly resides out of the scope of this PR.

That said, I think this PR would need to actually allow compiling against OpenSSL 1.1 without the use of --with-libressl argument as you've noted that it has become an inappropriate check. I don't think we actually want to support libressl, so changes to the check in the build system would be necessary.

[Warrows]

@Fuzzbawls We can agree --with-libressl is conceptually not the appropriate argument to compile with openssl 1.1. However I am not sure we want to support ossl1.1 any more than lssl. Bitcoin did not when their consensus was depending on it. Given the dependency for zPIV I think we need an argument for both lssl and ossl1.1.
And I am not (yet) aware of a way to distinguish them efficiently in the configure process. Maybe I should just change argument name and warning texts to include both for now?

[Fuzzbawls]

I will try to do some more testing with this on a system that has OpenSSL 1.1 native (AFAIK Ubuntu 14.04 fully updated does this). I agree that we probably don't want to encourage OpenSSL 1.1 any more than LibreSSL, but if this PR is intended to provide OpenSSL 1.1 compatibility, then the build system will absolutely need to recognize that, if even for the short term.

[Warrows]

@Fuzzbawls Done and rebased. It's now simply stating "unsupported ssl version" instead of "libressl" when you compile with something else than OpenSSL 1.0.

[Fuzzbawls]

from a build-system perspective, the recent change is good. Would still like feedback from @rejectedpromise regarding the changes to bignum and crypter when forcing a compile against openssl 1.1

[Mrs-X]

@rejectedpromise Any news on this?

[rejectedpromise]

sorry for the delay, been crunching for next release :)

Changes Needed:

When building this PR, the regular make command seems to succeed just fine.
When issuing a make check however, there is a failure during the building and running of test_pivx-qt.

Looks like a tACK, IMO, once that gets fixed

Tested Linux Mint 18.3:

Manual testing results:

• minted 777 tzPIV
• 2.5 seconds for tzPIV mint
• spent 1.33 from two 1 denomination mints, without re-mint or minimize change; to another wallet
• spend with 100 security successful in 3.685 sec
• proper change amount returned to wallet
• other wallet promptly received the spend; both users had to wait their respective amounts of time (maturity/confirmations)
• waited for the 500 denomination minted above (777) to become confirmed and mature
• spend 333 from 500 denomination with 100 security with converting and minimizing change.
• spend successful in 5.2 sec

Boost testing results

• failed to build tests completely but only in the qt binary
• execution of test_pivx pass with no errors
• execution of test_pivx-qt failed with core dump
• ran PR through my non-merged Zerocoin encryption tests and all checked out

[Warrows]

@rejectedpromise I found the problem and fixed it. It was a bad initialization of context in payment request plus.

[rejectedpromise]

Sweet!
I will try to test again either tonight or tomorrow 👍

[rejectedpromise]

Regular build and test build now passing

[Warrows]

So @presstab, @Mrs-X, @Fuzzbawls, are we waiting for more reviews or is it mergeable in your view ?

[Fuzzbawls]

@Warrows getting a compile-time error with this when using OpenSSL 1.1.0g

qt/paymentrequestplus.cpp: In member function ‘bool PaymentRequestPlus::getMerchant(X509_STORE*, QString&) const’:
qt/paymentrequestplus.cpp:161:20: error: aggregate ‘EVP_MD_CTX _ctx’ has incomplete type and cannot be defined
         EVP_MD_CTX _ctx;
                    ^~~~
Makefile:5606: recipe for target 'qt/qt_libbitcoinqt_a-paymentrequestplus.o' failed

[Warrows]

@Fuzzbawls nice catch, I introduced it when fixing @rejectedpromise case. Both should be good now.

[Fuzzbawls]

Fixed now. Compiles/syncs.

[Mrs-X]

@Fuzzbawls Do you get it to build on High Sierra?
Because here configure still stops with the unsupported SSL error.

Edit: ./configure LDFLAGS='-L/usr/local/opt/openssl/lib' CPPFLAGS='-I/usr/local/opt/openssl/include' PKG_CONFIG_PATH='/usr/local/opt/openssl/lib/pkgconfig' does the job on OSX.

Not ideal, but at least I can test now.

[Warrows]

@Mrs-X I guess High Sierra is using OpenSSL 1.1. The point of this PR is to allow compilation with it, but it's still potentially dangerous in terms of consensus, hence the unsupported SSL error. You should be able to bypass it by using --with-unsupported-ssl with ./configure instead of these flags.
I just pushed 094fa11 to give this information directly when the error happens (you'll need to rerun autogen).

[Fuzzbawls]

@Mrs-X I did my testing on Debian Stretch, which uses OpenSSL 1.1 by default.

Also, ACK 094fa11 with the changed error message.

Note: It should be well understood that this PR does NOT aim to ensure consensus compatibility with OpenSSL 1.1, just that compiling is possible.

[Mrs-X]

@Fuzzbawls Does your remark mean that we will not merge this?

[Fuzzbawls]

@Mrs-X not at all. I think this should be merged (hence the ACK), but just wanted to make it clear that this is only a compile-time fix and that OpenSSL 1.1 is still unsupported and largely untested for consensus critical code.

[Mrs-X]

Okay, since I successfully tested 094fa11 as well I'll merge...