-
Notifications
You must be signed in to change notification settings - Fork 38.7k
fixes/refactoring for building against LibreSSL #7586
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
fixes/refactoring for building against LibreSSL #7586
Conversation
|
Added a commit that fixes a logical text-issue when compiling with LibreSSL. |
|
In main.cpp we explicitly cede for LibreSSL, do we want a similar construction here? (or even: factor it out somehow so it exists only in one place, so that we don't have this inconsistency next time) |
|
Yes. Let me factor it out. But not sure about extra treating LibreSSL, IMO we should just use I could imaging allow compiling against different OpenSSL alternatives (Apple also has it's own IIRC). |
|
IIRC support for (or we have the |
|
It's not LibreSSL's mistake. They forked at a specific version of OpenSSL that they guarantee API compatibility with. That version uses SSLEAY_VERSION. So LibreSSL has to keep that in order to keep the API compatibility promise. The change in OpenSSL is after the fork. |
|
Added a commit that refactors the SSL detection. |
That's true but they do report a newer OpenSSL version in the |
|
@jonasschnelli just a note, CentOS is not a LibreSSL distribution. I built and run LibreSSL on it, as the OpenSSL it has is old and has poor ECC support. |
Aside, as it is no reason to have worse support for LibreSSL, but as of 0.12, ECC support in OpenSSL is no longer a requirement for Bitcoin Core. |
|
@laanwj yeah, but while I can have both openssl and libressl installed at the same time I can't have the header files to both installed at the same time, at least not with both installed in /usr/include |
| <widget class="QLabel" name="label_14"> | ||
| <property name="text"> | ||
| <string>Using OpenSSL version</string> | ||
| <string>Using SSL version</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add library to make clear it's not the protocol?
|
Concept ACK |
| #elif defined(SSLEAY_VERSION_NUMBER) || defined(OPENSSL_VERSION_NUMBER) | ||
| return SSLeay_version(SSLEAY_VERSION); | ||
| #else | ||
| return NULL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about returning _("unknown") and further simplify the code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Soon, there could be an option to run without SSL (after rand and aes implementation).
Then we might want to skip/hide the SSL log info?
|
Debug log: before: after: I propose to change the text to "Using SSL library" |
|
|
||
| std::string CopyrightHolders(const std::string& strPrefix); | ||
|
|
||
| /* returns the current SSL product and version info */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this file is completely documented, can you please extend the comments? E.g. like this:
/**
* Return the product name with the version number of the SSL library being used.
* @note Bitcoin Core supports OpenSSL and LibreSSL.
*/There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Will use you comment.
|
Since its no longer part of consensus does it make sense to log it? Bitcoind doesn't log the boost version or the QT version. |
Github-Pull: bitcoin#7586 Rebased-From: 16605c6
|
Agree with @gmaxwell. This way we could get rid of the hacky logic and also tidy up the GUI a bit, keeping in mind there are other ways to detect the library version, if this is ever desired. |
|
I think it could be useful in the debug log. But I agree it can be removed in the UI. |
|
Closing (#7605 merged) |
Alternative solution for #7585. Re-Enables LibreSSL compatibility.
Commit count and credit goes to @AliceWonderMiscreations