Fix OpenSSL v1.1 compatibility #153
Conversation
hrobeers
requested review from
sigmike,
peerchemist,
saeveritt,
backpacker69 and
sunnyking
|
Is this code change from Bitcoin, if so is it possible to reference a bitcoin change here? Also, is there impact to build requirements? |
|
@sunnyking The fix should be very straightforward to review for anyone familiar with OpenSSL. In the mean time I tested it to fully validate both the mainnet & testnet chain. |
|
Compiled on Debian 9.1 and running with OpenSSL 1.1.0f on testnet. |
src/key.cpp
Outdated
| if (sig->r) BN_clear_free(sig->r); | ||
| if (sig->s) BN_clear_free(sig->s); | ||
| sig->r = sig_r; | ||
| sig->s = sig_s; |
There was a problem hiding this comment.
This changes the behavior a bit. I don't know the internals of openssl, but if it ever keeps another reference to r and s then this code breaks it. It's most likely not the case, but keeping the old code as it was would be simpler and less risky.
There was a problem hiding this comment.
I wanted to avoid duplication of the BN_bin2bn function calls.
But I agree keeping the old code here might be safer. Will change.
|
The code looks good to me, except for all the new |
|
Thanks, @sigmike great feedback. How did you think to handle a possible allocation failure in the Bignum ctor? |
|
Yes. |
|
I decided to reuse the bignum_error exception for consistency. |
|
Ok this should be it guys. Rewrote history squashed everything in one commit for clarity. |
Runs and tests pass using openssl 1.1.0.f on Archlinux
Also tested for backwards compatibility with openssl 1.0
Please test if it compiles and runs on your system, heavy testing will be done on the second fork client.