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

Remove openssl info from init/log and from Qt debug window #7605

Merged
merged 1 commit into from Mar 3, 2016

Conversation

Projects
None yet
6 participants
@jonasschnelli
Member

jonasschnelli commented Feb 26, 2016

Alternative solution for #7586

Removes openssl information log dump during startup as well as from the QT debug window.
Openssl is no longer consensus critical, though, it still in use for random number generation, AES encryption and BIP70 (payment protocol, GUI only).

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Feb 26, 2016

Contributor

I think that logging SSL library used for generating random numbers is still worth it. We do not need it in the GUI though, I agree.

Contributor

paveljanik commented Feb 26, 2016

I think that logging SSL library used for generating random numbers is still worth it. We do not need it in the GUI though, I agree.

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Feb 26, 2016

Member

Great to see this removed from the GUI.

utACK 5ecfa36. (building right now)

Tested ACK 5ecfa36

(I am ok to keep it in the debug.log, but I don't have a strong opinion here)

Member

MarcoFalke commented Feb 26, 2016

Great to see this removed from the GUI.

utACK 5ecfa36. (building right now)

Tested ACK 5ecfa36

(I am ok to keep it in the debug.log, but I don't have a strong opinion here)

@AliceWonderMiscreations

This comment has been minimized.

Show comment
Hide comment
@AliceWonderMiscreations

AliceWonderMiscreations Feb 27, 2016

Contributor

I also think it should be in the debug log but I do not think it serves a purpose in the Qt Information dialog.

Way off topic but in my dialog, the Build date appears to be wrong, reporting 17 Feb which is before 0.12.0 was released. Wonder if that is a bug in my chroot build environment.

Contributor

AliceWonderMiscreations commented Feb 27, 2016

I also think it should be in the debug log but I do not think it serves a purpose in the Qt Information dialog.

Way off topic but in my dialog, the Build date appears to be wrong, reporting 17 Feb which is before 0.12.0 was released. Wonder if that is a bug in my chroot build environment.

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Feb 27, 2016

Member

I also think it should be in the debug log

Imo it's just not worth the effort to maintain. (We've had more than 3 pull request about "fixing" this in the last month). There are other trivial ways to determine the version of the library you are using

Way off topic but in my dialog, the Build date appears to be wrong, reporting 17 Feb which is before 0.12.0 was released. Wonder if that is a bug in my chroot build environment.

The build date for releases is the date when the last commit was commited:

$ git log  --format=%cD 188ca9c305d3dd0fb462b9d6a44048b1d99a05f3 -1
Wed, 17 Feb 2016 09:40:03 +0100
Member

MarcoFalke commented Feb 27, 2016

I also think it should be in the debug log

Imo it's just not worth the effort to maintain. (We've had more than 3 pull request about "fixing" this in the last month). There are other trivial ways to determine the version of the library you are using

Way off topic but in my dialog, the Build date appears to be wrong, reporting 17 Feb which is before 0.12.0 was released. Wonder if that is a bug in my chroot build environment.

The build date for releases is the date when the last commit was commited:

$ git log  --format=%cD 188ca9c305d3dd0fb462b9d6a44048b1d99a05f3 -1
Wed, 17 Feb 2016 09:40:03 +0100
@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Feb 27, 2016

Contributor

@MarcoFalke The sentence "There are other trivial ways to determine the version of the library you are using" unfortunately applies only to users with deep technical knowledge. Our users mix different versions of SSL libraries, at compile time and at runtime. The fixes applied to this part of our code only handle such strange instances. It is very useful to know what was used at runtime, really.

Contributor

paveljanik commented Feb 27, 2016

@MarcoFalke The sentence "There are other trivial ways to determine the version of the library you are using" unfortunately applies only to users with deep technical knowledge. Our users mix different versions of SSL libraries, at compile time and at runtime. The fixes applied to this part of our code only handle such strange instances. It is very useful to know what was used at runtime, really.

@AliceWonderMiscreations

This comment has been minimized.

Show comment
Hide comment
@AliceWonderMiscreations

AliceWonderMiscreations Feb 27, 2016

Contributor

Thanks @MarcoFalke so it isn't really the "build" date as I think of "build" date (compile)
-=-
sorry for OT

Contributor

AliceWonderMiscreations commented Feb 27, 2016

Thanks @MarcoFalke so it isn't really the "build" date as I think of "build" date (compile)
-=-
sorry for OT

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Feb 29, 2016

Member

I like this solution.

The build date should probably go away as well, it's confusing at least for the executables built in gitian, as the date/time is overridden to create deterministic executables. I don't think it adds very much in any case.

Imo it's just not worth the effort to maintain. (We've had more than 3 pull request about "fixing" this in the last month).

Yes, that's kind of annoying, and usually means a deeper solution is desirable.
OTOH we probably got it right now, finally. Removing it from just the GUI is fine with me too.

Member

laanwj commented Feb 29, 2016

I like this solution.

The build date should probably go away as well, it's confusing at least for the executables built in gitian, as the date/time is overridden to create deterministic executables. I don't think it adds very much in any case.

Imo it's just not worth the effort to maintain. (We've had more than 3 pull request about "fixing" this in the last month).

Yes, that's kind of annoying, and usually means a deeper solution is desirable.
OTOH we probably got it right now, finally. Removing it from just the GUI is fine with me too.

@laanwj laanwj added the GUI label Feb 29, 2016

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Feb 29, 2016

Member

Agree with removing the build-date (in another PR), an alternative solution would be to only show the build date in non release builds (!CLIENT_VERSION_IS_RELEASE).

I think we should merge this PR and remove openssl from the logs and the GUI. IMO there is not much value in logging the openssl version regarding to PRNG, and, eventually we'll end up with a custom PRNG implementation (#5885) anyways.

Member

jonasschnelli commented Feb 29, 2016

Agree with removing the build-date (in another PR), an alternative solution would be to only show the build date in non release builds (!CLIENT_VERSION_IS_RELEASE).

I think we should merge this PR and remove openssl from the logs and the GUI. IMO there is not much value in logging the openssl version regarding to PRNG, and, eventually we'll end up with a custom PRNG implementation (#5885) anyways.

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Feb 29, 2016

Member

It may actually be useful to log the version/hash of every library being used, but probably no reason to single out OpenSSL.

Member

luke-jr commented Feb 29, 2016

It may actually be useful to log the version/hash of every library being used, but probably no reason to single out OpenSSL.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Mar 1, 2016

Member

@luke-jr Well we log BerkeleyDB version as well. But at least there's a good reason for that, to troubleshoot wallet incompatibility.

Member

laanwj commented Mar 1, 2016

@luke-jr Well we log BerkeleyDB version as well. But at least there's a good reason for that, to troubleshoot wallet incompatibility.

@laanwj laanwj merged commit 5ecfa36 into bitcoin:master Mar 3, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Mar 3, 2016

Merge #7605: Remove openssl info from init/log and from Qt debug window
5ecfa36 Remove openssl info from init/log and from Qt debug window (Jonas Schnelli)

@laanwj laanwj removed the Needs backport label Mar 24, 2016

laanwj added a commit that referenced this pull request Mar 24, 2016

Remove openssl info from init/log and from Qt debug window
Conflicts:
	src/init.cpp

Github-Merge: #7605
Rebased-From: 5ecfa36

zander added a commit to zander/bitcoinclassic that referenced this pull request Jun 16, 2016

Remove openssl info from init/log and from Qt debug window
Conflicts:
	src/init.cpp

Github-Merge: #7605
Rebased-From: 5ecfa36

thokon00 added a commit to faircoin/faircoin that referenced this pull request Jun 28, 2016

Remove openssl info from init/log and from Qt debug window
Conflicts:
	src/init.cpp

Github-Merge: #7605
Rebased-From: 5ecfa36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment