Use log categories when logging events that P2P peers can trigger arbitrarily.
Rationale similar to that of PR #17762 (net: Log to net category for exceptions in ProcessMessages):
It is not good to panick end users with verbose errors (let alone writing to stderr) when any peer can generate them.
Concept ACK.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
No conflicts as of last run.
51 | @@ -52,6 +52,13 @@ bool error(const char* fmt, const Args&... args) 52 | return false; 53 | } 54 | 55 | +template<typename... Args> 56 | +bool error_debug(const BCLog::LogFlags& category, const char* fmt, const Args&... args)
Drop reference in 1st arg?
Fixed!
Concept ACK. nit, error_debug() looks strange to me.
@promag I agree the name does not feel perfect. Have any suggestions w.r.t. naming? :) I'm trying to mimic bool error(…). Perhaps bool error_with_category(…) or bool error_with_debug(…)?
Now using error_with_debug_log(…) instead of error_debug(…).
Please re-review :)
Rebased! :)
3814 | @@ -3815,7 +3815,7 @@ bool ProcessNewBlock(const CChainParams& chainparams, const std::shared_ptr<cons 3815 | } 3816 | if (!ret) { 3817 | GetMainSignals().BlockChecked(*pblock, state); 3818 | - return error("%s: AcceptBlock FAILED (%s)", __func__, state.ToString()); 3819 | + return error_with_debug_log(BCLog::VALIDATION, "%s: AcceptBlock FAILED (%s)", __func__, state.ToString());
BCLog::VALIDATION is used for the validation interface. Is the same category applicable here?
I'd be happy to change but I'm not sure to which TBH: which BCLog category would be appropriate in these cases?
FWIW, these are the location of BCLog uses in current master:
$ git grep -E '^ *LogPrint\(BCLog::' | tr -d " " | cut -f1 -d, | \
sed 's/:LogPrint(/ /g' | sort -u | awk '{ print $2 " " $1 }' | sort -u
BCLog::ADDRMAN src/addrman.cpp
BCLog::ADDRMAN src/addrman.h
BCLog::BENCH src/miner.cpp
BCLog::BENCH src/validation.cpp
BCLog::CMPCTBLOCK src/blockencodings.cpp
BCLog::COINDB src/txdb.cpp
BCLog::ESTIMATEFEE src/policy/fees.cpp
BCLog::HTTP src/httpserver.cpp
BCLog::LEVELDB src/dbwrapper.cpp
BCLog::LIBEVENT src/httpserver.cpp
BCLog::MEMPOOLREJ src/net_processing.cpp
BCLog::MEMPOOL src/net_processing.cpp
BCLog::MEMPOOL src/txmempool.cpp
BCLog::MEMPOOL src/validation.cpp
BCLog::NET src/addrman.cpp
BCLog::NET src/banman.cpp
BCLog::NET src/netbase.cpp
BCLog::NET src/net.cpp
BCLog::NET src/net_processing.cpp
BCLog::NET src/timedata.cpp
BCLog::PROXY src/netbase.cpp
BCLog::PRUNE src/validation.cpp
BCLog::QT src/qt/bitcoin.cpp
BCLog::RAND src/random.cpp
BCLog::REINDEX src/validation.cpp
BCLog::RPC src/httprpc.cpp
BCLog::RPC src/init.cpp
BCLog::RPC src/rpc/blockchain.cpp
BCLog::RPC src/rpc/request.cpp
BCLog::RPC src/rpc/server.cpp
BCLog::SELECTCOINS src/wallet/coinselection.cpp
BCLog::TOR src/torcontrol.cpp
BCLog::VALIDATION src/validation.cpp
BCLog::VALIDATION src/validationinterface.cpp
BCLog::WALLETDB src/wallet/db.cpp
BCLog::WALLETDB src/wallet/walletdb.cpp
BCLog::ZMQ src/zmq/zmqnotificationinterface.cpp
BCLog::ZMQ src/zmq/zmqpublishnotifier.cpp
ACK on the mempool category. Not sure about BCLog::VALIDATION.
I think there is a tradeoff. IIRC -debug is disabled by default, disabling all debug categories. Thus, the debug log is pretty much useless to find the root cause of an issue after a crash or other misbehavior if all log statements are put into a category. Still, it makes sense to put log statments into categories to make it possible to disable or enable them specifically.
I think we should rethink what categories are enabled/disabled by default and come up with a broader guideline on debug categories.
@MarcoFalke Sounds good, but what changes do you suggest for this PR? :)
This PR should be revisited after a guideline has been established
@MarcoFalke I'm not sure I follow: which of the error messages touched in this PR do you think should be shown to users not running -debug?
@practicalswift Any of them. Assume a user has a crashed node (or just high memory usage, high CPU usage, or any other anomaly). In that case those debug messages can give a hint which peer or which message type or which code part/module is responsible for the misbehavior.
