Calling LogPrint with a category that is not enabled results in
evaluating the remaining function arguments, which may be arbitrarily
complex (and possibly expensive) expressions. Defining LogPrint as a
macro prevents this unnecessary expression evaluation.
This is a partial revert of #14209. The decision to revert is discussed in #16688, which adds verbose logging for validation event notification.
Is it known to be true that every call to LogPrint has no expressions with side effects?
On Tue, Oct 22, 2019, 11:28 AM Jeffrey Czyz notifications@github.com wrote:
Calling LogPrint with a category that is not enabled results in evaluating the remaining function arguments, which may be arbitrarily complex (and possibly expensive) expressions. Defining LogPrint as a macro prevents this unnecessary expression evaluation.
This is a partial revert of #14209 https://github.com/bitcoin/bitcoin/pull/14209. The decision to revert is discussed in #16688 https://github.com/bitcoin/bitcoin/pull/16688, which adds verbose logging for validation event notification.
You can view, comment on, or merge this pull request online at:
#17218 Commit Summary
- Replace the LogPrint function with a macro
File Changes
- M src/logging.h https://github.com/bitcoin/bitcoin/pull/17218/files#diff-0 (12)
Patch Links:
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/17218?email_source=notifications&email_token=AAGYN67LZZ4C7NCSPBAFEVDQP5A4VA5CNFSM4JDTSZXKYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4HTSVB2A, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAGYN67C6XATDCTWFXGPE6TQP5A4VANCNFSM4JDTSZXA .
160-{
161- if (LogAcceptCategory((category))) {
162- LogPrintf(args...);
163- }
164-}
165+#define LogPrint(category, ...) do { \
LOG_CATEGORY(category, format, ...) macro. This would avoid issue the Jeremy raised about side effects in existing code, and also be nicer in my opinion because uppercase naming would be more consistent with other macros.
I’d prefer to see a developer comment to explain why this is a macro (not a function like LogPrintf just a few lines up).
optional: You may install clang-format and run the https://github.com/bitcoin/bitcoin/tree/master/contrib/devtools#clang-format-diffpy script to align the newline continuation chars \.
My feeling is that providing two ways to log where one is subtly different than the other adds unnecessary cognitive load to readers and reviewers. Will address Jeremey’s comment in a follow-up.
If the concern is cognitive load, I’d think the current:
LogPrintf - log unconditionallyLogPrint - log for categoryis more confusing than what I’m proposing:
LogPrintf - log unconditionallyLOG_CATEGORY - log for category with macro magicBut if it would take a long time to deprecate LogPrint (not sure why it would), then I agree inconsistency would not look nice in the interim.
I don’t feel strongly any way about this, so please ignore this suggestion if doesn’t suit you.
Ah, I thought you were suggesting LogPrint, LogPrintf, and LOG_CATEGORY.
I’m not completely opposed to changing call sites to LOG_CATEGORY (and checking for side effects in the process). But I would like to be sure such a change would be welcome before putting in the work. :)
I would like to be sure such a change would be welcome before putting in the work. :)
To be clear, suggestion is not to do more work. My suggestion is:
LOG_CATEGORY, to be clear that it is a macro and not something that evaluated like a normal function.LogPrint alone, mark it deprecated and add a note like “LOG_CATEGORY is preferred over LogPrint in new code as a more performant alternative”Separately after that, I do not think it would take a lot of work to replace instances of LogPrint and remove it. We could start with a simple scripted-diff to replace simple variable references that obviously don’t have side effects.
0git grep -l LogPrint | xargs sed -i 's/LogPrint\((BCLog::[A-Z]\+, "[^"]\+"\(, [A-Za-z_]\+\)*)\)/LOG_CATEGORY\1/g'
As for organizing these changes, I might consider closing this PR, and instead adding the new macro directly where you intend to use it in #16688.
LogPrint should be deprecated in favor of the LOG_CATEGORY macro.
Is this a pure optimisation PR, or can we think of reasons beyond that?
Do we have any examples of slow LogPrint(…) calls where this would make a significant difference?
Can we benchmark this in any way?
Calling LogPrint with a category that is not enabled results in
evaluating the remaining function arguments, which may be arbitrarily
complex (and possibly expensive) expressions. Defining LogPrint as a
macro prevents this unnecessary expression evaluation.
This is a partial revert of #14209. The decision to revert is discussed
in #16688, which adds verbose logging for validation event notification.
I’ve thought about it a bit and I think for the reason I gave above I’d want to NACK this change.
If there are motivating examples for not evaluating the calls, they should be fixed one by one.
It would also seem like perfect forwarding and inline can help with some of these concerns too to nudge the compiler to defer constructing strings or whatever.
On Tue, Oct 22, 2019, 1:47 PM Jeffrey Czyz notifications@github.com wrote:
@jkczyz commented on this pull request.
In src/logging.h https://github.com/bitcoin/bitcoin/pull/17218#discussion_r337741803:
@@ -155,12 +155,10 @@ static inline void LogPrintf(const char* fmt, const Args&… args) } }
-template <typename… Args> -static inline void LogPrint(const BCLog::LogFlags& category, const Args&… args) -{
- if (LogAcceptCategory((category))) {
LogPrintf(args...);- } -} +#define LogPrint(category, …) do { \
Done in 8734c85 https://github.com/bitcoin/bitcoin/commit/8734c856f85cb506fa97596383dd7e7b9edd7e03 .
— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/17218?email_source=notifications&email_token=AAGYN6ZCEVD7RSKFT4WLQPTQP5RGBA5CNFSM4JDTSZXKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCI2YLLA#discussion_r337741803, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAGYN636IOUCRCYGJ2VKPFDQP5RGBANCNFSM4JDTSZXA .
Is it known to be true that every call to LogPrint has no expressions with side effects?
I grepped over call sites to check if anything suspicious stood out, but I did not thoroughly check if each expression was free of side effects.
Note that this PR simply reverts the unintended behavioral change of #14209. Any side effects would have been introduced by that PR or subsequent changes.
If there are motivating examples for not evaluating the calls, they should be fixed one by one.
The motivating example is #16688. I think this change is relatively low risk and restores the previous behavior of LogPrint. The alternative of adding a separate logging function would move the codebase into a worse state, IMHO. That should be weighed against the perceived risk of removing any unintended side effects.
It would also seem like perfect forwarding and inline can help with some of these concerns too to nudge the compiler to defer constructing strings or whatever.
I don’t believe perfect forwarding will prevent an argument expression from being evaluated, which is the reason for the PR. The compiler is also free to ignore inline.
Is this a pure optimisation PR, or can we think of reasons beyond that?
Do we have any examples of slow
LogPrint(…)calls where this would make a significant difference?Can we benchmark this in any way?
I see it more as restoring the previous behavior of only paying for the logging that you want. Thus, anyone is free to add very verbose logging to their module without others incurring a cost when logging for that category isn’t enabled.
Ah didn’t catch that it’s reverting behavior.
I think that given that it’s a revert it’s more ok.
It’s tricky to tell by glance if there are no side effects because of atomics autoderef loading and stuff, which is why I felt it deserves careful consideration.
I don’t care to bikeshed too much, but I think that making it IF_CAT(cat, expr) would be semantic in terms of implying the arguments may not be evaluated, and then expr can be an unconditional Logprint. But that’s a few extra characters… I wouldn’t object if expr is passed to LogPrint in the macro, but feel it’s a bit harder to comprehend at a glance.
On Tue, Oct 22, 2019, 3:34 PM Jeffrey Czyz notifications@github.com wrote:
@jkczyz commented on this pull request.
In src/logging.h https://github.com/bitcoin/bitcoin/pull/17218#discussion_r337779900:
@@ -155,12 +155,10 @@ static inline void LogPrintf(const char* fmt, const Args&… args) } }
-template <typename… Args> -static inline void LogPrint(const BCLog::LogFlags& category, const Args&… args) -{
- if (LogAcceptCategory((category))) {
LogPrintf(args...);- } -} +#define LogPrint(category, …) do { \
Ah, I thought you were suggesting LogPrint, LogPrintf, and LOG_CATEGORY.
I’m not completely opposed to changing call sites to LOG_CATEGORY (and checking for side effects in the process). But I would like to be sure such a change would be welcome before putting in the work. :)
— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/17218?email_source=notifications&email_token=AAGYN67PZ3X5Z2MVSRXTJFDQP55W5A5CNFSM4JDTSZXKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCI3EIYQ#discussion_r337779900, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAGYN6Z3TFDBZLZOUXAEQITQP55W5ANCNFSM4JDTSZXA .
I see it more as restoring the previous behavior of only paying for the logging that you want. Thus, anyone is free to add very verbose logging to their module without others incurring a cost when logging for that category isn’t enabled.
In other types of software that is universally a good idea, but it could be argued that in Bitcoin Core we want to make sure that execution of consensus critical code should be as identical as possible across clients. See sipas arguments in #4576 regarding always evaluating arguments to not have uncertainty what performance impact consistency checks.
When it comes to consensus critical code it could be argued that consistency in taken code paths is often more important than raw execution speed.
If we want to put that principle aside we should have good reasons for doing so: that’s why it is important to benchmark a change like this. Is the performance impact measurable in practice?
Concept NACK, this used to be a macro in the past. We changed it to a function for argument hygiene as is common in modern C++.
If you really need to log in performance-critical contexts, then you could do a special thing there, but I’m very much against changing the general log system for this.
Never mind, I was confused here. It still uses an internal function for that. The whole point here was to be able to shortcut in case logging for the category was disabled…
I think the assumption in #14209 was that an inline was just as good as a macro in this regard. Have you benchmarked this?
I think the assumption in #14209 was that an inline was just as good as a macro in this regard. Have you benchmarked this?
Practicalswift asked the same question yesterday and this hasn’t been benchmarked yet. It might be the case that performance is roughly equivalent, but if by “just as good” you mean actually equivalent, it’s probably not a good assumption that compilers are going to be able to deduce that calls like pindex->GetBlockHash().GetHex() have no side-effects and inline them out.
I see it more as restoring the previous behavior of only paying for the logging that you want. Thus, anyone is free to add very verbose logging to their module without others incurring a cost when logging for that category isn’t enabled.
In other types of software that is universally a good idea, but it could be argued that in Bitcoin Core we want to make sure that execution of consensus critical code should be as identical as possible across clients. See sipas arguments in #4576 regarding always evaluating arguments to not have uncertainty what performance impact consistency checks.
When it comes to consensus critical code it could be argued that consistency in taken code paths is often more important than raw execution speed.
Hmmm, in that case the current behavior is actually inconsistent. If the compiler decides to inline the code at a particular call site, then the arguments will not be evaluated whereas they will be evaluated at call sites that are not inlined.
I think the assumption in #14209 was that an inline was just as good as a macro in this regard. Have you benchmarked this?
Practicalswift asked the same question yesterday and this hasn’t been benchmarked yet. It might be the case that performance is roughly equivalent, but if by “just as good” you mean actually equivalent, it’s probably not a good assumption that compilers are going to be able to deduce that calls like
pindex->GetBlockHash().GetHex()have no side-effects and inline them out.
Correct, I have not benchmarked the code. I’m not even sure how to adequately devise such a benchmark given the compiler can decide which call sites will have the code inlined. At very least, use of a macro will result in consistent behavior with regards to argument evaluation.
t’s probably not a good assumption that compilers are going to be able to deduce that calls like pindex->GetBlockHash().GetHex() have no side-effects and inline them out.
Maybe we should look at the gnu::pure attribute (which have pretty wide support in gcc/clang/llvm and others, except MSVC) and noexcept, which would allow the compiler to better optimize such functions if this is an issue https://stackoverflow.com/questions/2798188/pure-const-function-attributes-in-different-compilers
So, uh, no real direct input but at one point while reworking the logging for categories I changed the handling around and benchmarking as a function vs macro vs a few other things and found measurably slower validation (in particular early chain IBD which is already pretty fast), it was also easy to see it doing string parsing in the disassembly.
There is another option beyond making this a macro: find the few performance critical logging spots and block them off with category tests (potentially just copying the current logging state into a local variable). This should be even faster than the macro, since you could eliminate many logging lines with potentially just one access to the contended global state.
I think it’s fine to do this. I strongly doubt there is any logging (especially categorized debug logging) with important side-effects. Not too long ago this was a macro. At least I’ve always treated it as one.
If @marcofalke’s concerns in #14209 no longer hold, ACK.
My motivation for #14209 was to get code coverage test run, which was impossible prior to my fix. I don’t care whether it is a function or macro.
Someone should do a benchmark, though if the rationale for this change is “speed up”.
Someone should do a benchmark, though if the rationale for this change is “speed up”.
From what I understand it’s not a benchmark issue but a conceptual one. Macros don’t have the guarantee to evaluate their arguments, but functions (even inline functions) do. Given arguments that are expensive to compute (e.g. string formatting, allocation, conversions), that’s will always make a difference in the disabled case.
LogPrint* could be microbenched
ACK 8734c856f85cb506fa97596383dd7e7b9edd7e03
My motivation for #14209 was to get code coverage test run, which was impossible prior to my fix. I don’t care whether it is a function or macro.
Whether it is a function or macro seems completely orthogonal to this. The simple fix in #14209 would have just been to remove the ifdef preprocessor directive added here: https://github.com/bitcoin/bitcoin/commit/c8914b9dbbf6106dac3c62769f7ce3bacd8fbf9b#diff-772f489c7d0a32de3badbfbcb5fd200dR133.
The renaming LogPrintf -> LOG_CATEGORY suggested here #17218 (review) seems like a good follow-up.
Here are all the LogPrintf changes since #14209 was merged:
0git log fae3fbd61a89c7a35bc0eda91b1df61371dc0936..HEAD -SLogPrintf -p | grep "^[+-]" | grep LogPrintf
1- pwallet->WalletLogPrintf("Skipping import of %s (key already present)\n", EncodeDestination(PKHash(keyid)));
2+ pwallet->WalletLogPrintf("Error importing key for %s\n", EncodeDestination(PKHash(keyid)));
3- pwallet->WalletLogPrintf("Skipping import of %s (script already present)\n", HexStr(script));
4+ LogPrintf("Error reading from database: %s\n", e.what());
5- LogPrintf("Error reading from database: %s\n", e.what());
6+ WalletLogPrintf("Already have script %s, skipping\n", HexStr(entry));
7+ WalletLogPrintf("Already have key with pubkey %s, skipping\n", HexStr(pubkey));
8+ WalletLogPrintf("Already have pubkey %s, skipping\n", HexStr(temp));
9- LogPrintf("The wallet is probably corrupted: Some keys decrypt but not all.\n");
10+ LogPrintf("The wallet is probably corrupted: Some keys decrypt but not all.\n");
11- // This can be called during exceptions by LogPrintf(), so we cache the
12- // This can be called during exceptions by LogPrintf(), so we cache the
13- LogPrintf("\n\n\n\n\n");
14- LogPrintf("%s: Connecting genesis block...\n", __func__);
15- LogPrintf("%s: failed to activate chain (%s)\n", __func__, FormatStateMessage(state));
16- LogPrintf("Using wallet directory %s\n", GetWalletDir().string());
17+ LogPrintf("Using wallet directory %s\n", GetWalletDir().string());
18+ LogPrintf("* Using %.1f MiB for %s block filter index database\n",
19+ LogPrintf("%s: Failed to open filter file %d\n", __func__, pos.nFile);
20+ LogPrintf("%s: Failed to truncate filter file %d\n", __func__, pos.nFile);
21+ LogPrintf("%s: Failed to commit filter file %d\n", __func__, pos.nFile);
22+ LogPrintf("%s: out of disk space\n", __func__);
23+ LogPrintf("%s: Failed to open filter file %d\n", __func__, pos.nFile);
24+ if (ec) LogPrintf("%s: %s %s\n", __func__, ec.message(), path.string());
25+ LogPrintf("%s: %s %s\n", __func__, ec.message(), it->path().string());
26+ LogPrintf("%s\n", e.what());
27+ LogPrintf("Pre-allocating up to position 0x%x in %s%05u.dat\n", new_size, m_prefix, pos.nFile);
28- LogPrintf("Pre-allocating up to position 0x%x in blk%05u.dat\n", nNewChunks * BLOCKFILE_CHUNK_SIZE, pos.nFile);
29- LogPrintf("Pre-allocating up to position 0x%x in rev%05u.dat\n", nNewChunks * UNDOFILE_CHUNK_SIZE, pos.nFile);
30+ LogPrintf("Unable to open file %s\n", path.string());
31+ LogPrintf("Unable to seek to position %u of %s\n", pos.nPos, path.string());
32- LogPrintf("Unable to open file %s\n", path.string());
33- LogPrintf("Unable to seek to position %u of %s\n", pos.nPos, path.string());
34+ LogPrintf("Using RdSeed as additional entropy source\n");
35+ LogPrintf("%s: Unable to remove PID file: File does not exist\n", __func__);
36- LogPrintf("%s: Unable to remove pidfile: %s\n", __func__, e.what());
37+ LogPrintf("%s: Unable to remove PID file: %s\n", __func__, e.what());
38+ wallet->WalletLogPrintf("Releasing wallet\n");
39- LogPrintf("%s: Warning: RegQueryValueExA(HKEY_PERFORMANCE_DATA) failed with code %i\n", __func__, ret);
40+ LogPrintf("Invalid or missing banlist.dat; recreating\n");
41- LogPrintf("Invalid or missing banlist.dat; recreating\n");
42- LogPrintf("HTTP event loop did not exit within allotted time, sending loopbreak\n");
43+ LogPrintf("WARNING: the RPC server is not safe to expose to untrusted networks such as the public internet\n");
44+ LogPrintf("WARNING: option -rpcallowip was specified without -rpcbind; this doesn't usually make sense\n");
45- LogPrintf("Warning: Config setting for %s only applied on %s network when in [%s] section.\n", arg, m_network, m_network);
46+- [#10265](/bitcoin-bitcoin/10265/) `ff13f59` Make sure pindex is non-null before possibly referencing in LogPrintf call. (Karl-Johan Alm)
47+ LogPrintf("Setting version bits activation parameters for %s to start=%ld, timeout=%ld\n", vDeploymentParams[0], nStartTime, nTimeout);
48- LogPrintf("Setting version bits activation parameters for %s to start=%ld, timeout=%ld\n", vDeploymentParams[0], nStartTime, nTimeout);
49+ LogPrintf("[%d%%]...", percentageDone); /* Continued */
None of the new logs with printf args have side-effects:
0+ WalletLogPrintf("Already have script %s, skipping\n", HexStr(entry)); // HexStr() has no side-effects
1+ WalletLogPrintf("Already have key with pubkey %s, skipping\n", HexStr(pubkey)); // HexStr() has no side-effects
2+ WalletLogPrintf("Already have pubkey %s, skipping\n", HexStr(temp)); // HexStr() has no side-effects
3+ LogPrintf("* Using %.1f MiB for %s block filter index database\n", // BlockFilterTypeName() has no side-effects
4+ LogPrintf("%s: Failed to open filter file %d\n", __func__, pos.nFile); // no side-effects
5+ LogPrintf("%s: Failed to truncate filter file %d\n", __func__, pos.nFile); // no side-effects
6+ LogPrintf("%s: Failed to commit filter file %d\n", __func__, pos.nFile); // no side-effects
7+ LogPrintf("%s: out of disk space\n", __func__); // no side-effects
8+ LogPrintf("%s: Failed to open filter file %d\n", __func__, pos.nFile); // no side-effects
9+ if (ec) LogPrintf("%s: %s %s\n", __func__, ec.message(), path.string()); // boost::system::error_code::message() and boost::filesystem::path::string() have no side-effects
10+ LogPrintf("%s: %s %s\n", __func__, ec.message(), it->path().string()); // boost::system::error_code::message() and boost::filesystem::path::string() have no side-effects
11+ LogPrintf("%s\n", e.what()); // std::exception::what() has no side-effects
12+ LogPrintf("Pre-allocating up to position 0x%x in %s%05u.dat\n", new_size, m_prefix, pos.nFile); // no side-effects
13+ LogPrintf("[%d%%]...", percentageDone); /* Continued */ // no side-effects
So merging this is at least as safe as we were prior to #14209.
Our bench runner has a full testing setup, so in theory
LogPrint*could be microbenched
I ran the following benchmark configured with –enable-debug:
0#include <bench/bench.h>
1#include <logging.h>
2#include <validation.h>
3
4static void BenchmarkLogPrint(benchmark::State& state)
5{
6 LOCK(cs_main);
7 CBlockIndex* tip = ::ChainActive().Tip();
8 assert(tip != nullptr);
9
10 while (state.KeepRunning()) {
11 LogPrint(BCLog::NET, "%s: new block hash=%s", __func__, tip->GetBlockHash().ToString());
12 }
13}
14
15BENCHMARK(BenchmarkLogPrint, 10 * 1000 * 1000);
LogPrint as an inline function:
0# Benchmark, evals, iterations, total, min, max, median
1BenchmarkLogPrint, 5, 10000000, 28.4183, 5.64534e-07, 5.76267e-07, 5.66845e-07
LogPrint as a macro:
0# Benchmark, evals, iterations, total, min, max, median
1BenchmarkLogPrint, 5, 10000000, 0.356953, 6.8946e-09, 7.80663e-09, 6.92364e-09
Looks like it’s a couple of orders of magnitude faster as a macro. Let me know if there is something more representative to run.
The renaming
LogPrintf->LOG_CATEGORYsuggested here #17218 (comment) seems like a good follow-up.Here are all the
LogPrintfchanges since #14209 was merged:
I believe we want LogPrint here as LogPrintf is the unconditional one.
I believe we want LogPrint here as LogPrintf is the unconditional one.
Oops. You’re right. Same exercise for LogPrint:
0+ LogPrint(BCLog::NET, "peer=%d: %s\n", nodeid, message); // no side-effects
1+ LogPrint(BCLog::NET, "CHECKSUM ERROR (%s, %u bytes), expected %s was %s\n", // no side-effects (SanitizeString() and HexString() args are const)
2+ LogPrint(BCLog::NET, "Ignoring \"getaddr\" from block-relay-only connection. peer=%d\n", pfrom->GetId()); // no side-effects (GetId() is const)
3+ LogPrint(BCLog::NET, "timeout of inflight tx %s from peer=%d\n", it->first.ToString(), pto->GetId()); // no side-effects (GetId() is const)
4+ LogPrint(BCLog::NET, "%s(%s, %u bytes): Exception '%s' caught\n", __func__, SanitizeString(strCommand), nMessageSize, e.what()); // no side-effects (std::exception::what() is const)
5+ LogPrint(BCLog::NET, "%s(%s, %u bytes): Exception '%s' caught\n", __func__, SanitizeString(strCommand), nMessageSize, e.what()); // no side-effects (std::exception::what() is const)
6+ LogPrint(BCLog::NET, "peer=%d: %s\n", nodeid, message); // no side-effects
7+ LogPrint(BCLog::NET, "Unexpected cmpctblock message received from peer %d\n", pfrom->GetId()); // no side-effects (GetId() is const)
8+ LogPrint(BCLog::NET, "Unexpected blocktxn message received from peer %d\n", pfrom->GetId()); // no side-effects (GetId() is const)
9+ LogPrint(BCLog::NET, "Unexpected headers message received from peer %d\n", pfrom->GetId()); // no side-effects (GetId() is const)
10+ LogPrint(BCLog::NET, "Unexpected block message received from peer %d\n", pfrom->GetId()); // no side-effects (GetId() is const)
11+ LogPrint(BCLog::ADDRMAN, "Unable to test; swapping %s for %s in tried table anyway\n", info_new.ToString(), info_old.ToString()); // no side-effects (ToString() is const)
12+ LogPrint(BCLog::ZMQ, "zmq: Outbound message high water mark for %s at %s is %d\n", type, address, outbound_message_high_water_mark); // no side-effects
13+ LogPrint(BCLog::HTTP, "HTTP request from %s rejected: Client network is not allowed RPC access\n", // no side-effects (GetPeer() and ToString() are const)
14+ LogPrint(BCLog::HTTP, "HTTP request from %s rejected: Unknown HTTP request method\n", // no side-effects (GetPeer() and ToString() are const)
15+ LogPrint(BCLog::ZMQ, "zmq: Outbound message high water mark for %s at %s is %d\n", type, address, outbound_message_high_water_mark); // no side-effects
16+ LogPrint(BCLog::ZMQ, "zmq: version %d.%d.%d\n", major, minor, patch); // no side-effects