net: Use log categories when logging events that P2P peers can trigger arbitrarily #17828
Conversation
|
Concept ACK. |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
|
@promag I agree the name does not feel perfect. Have any suggestions w.r.t. naming? :) I'm trying to mimic |
…r arbitrarily Github-Pull: bitcoin#17828 Rebased-From: e148126
e148126 to
594f17d
Compare
|
Now using Please re-review :) |
594f17d to
443e105
Compare
|
Rebased! :) |
…r arbitrarily Github-Pull: bitcoin#17828 Rebased-From: 443e105
…r arbitrarily Github-Pull: bitcoin#17828 Rebased-From: 443e105
443e105 to
0496062
Compare
| @@ -3815,7 +3815,7 @@ bool ProcessNewBlock(const CChainParams& chainparams, const std::shared_ptr<cons | |||
| } | |||
| if (!ret) { | |||
| GetMainSignals().BlockChecked(*pblock, state); | |||
| return error("%s: AcceptBlock FAILED (%s)", __func__, state.ToString()); | |||
| return error_with_debug_log(BCLog::VALIDATION, "%s: AcceptBlock FAILED (%s)", __func__, state.ToString()); | |||
There was a problem hiding this comment.
BCLog::VALIDATION is used for the validation interface. Is the same category applicable here?
There was a problem hiding this comment.
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
|
I think there is a tradeoff. IIRC 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 |
|
@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. |
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):