net: Use log categories when logging events that P2P peers can trigger arbitrarily by practicalswift · Pull Request #17828 · bitcoin/bitcoin

practicalswift · 2019-12-29T21:45:45Z

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.

hebasto · 2019-12-29T21:47:50Z

Concept ACK.

DrahtBot · 2019-12-30T00:05:50Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

promag

Concept ACK. nit, error_debug() looks strange to me.

src/util/system.h

practicalswift · 2020-01-02T17:25:25Z

@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(…)?

practicalswift · 2020-01-08T12:46:28Z

Now using error_with_debug_log(…) instead of error_debug(…).

Please re-review :)

practicalswift · 2020-01-10T13:45:07Z

Rebased! :)

…r arbitrarily Github-Pull: bitcoin#17828 Rebased-From: 443e105f7aed5ae6ac820682fb2f2215d8163ca0

…r arbitrarily

maflcko

ACK on the mempool category. Not sure about BCLog::VALIDATION.

maflcko · 2020-03-05T22:00:38Z

src/validation.cpp

        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());


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

maflcko · 2020-03-06T15:13:36Z

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.

practicalswift · 2020-03-06T15:22:15Z

@MarcoFalke Sounds good, but what changes do you suggest for this PR? :)

maflcko · 2020-03-06T15:23:18Z

This PR should be revisited after a guideline has been established

practicalswift · 2020-03-06T15:26:45Z

@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?

maflcko · 2020-03-06T19:34:09Z

@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.

fanquake added P2P Utils/log/libs labels Dec 29, 2019

promag reviewed Jan 2, 2020

View reviewed changes

src/util/system.h Outdated Show resolved Hide resolved

practicalswift force-pushed the log-categories branch from e148126 to 594f17d Compare January 8, 2020 12:45

practicalswift force-pushed the log-categories branch from 594f17d to 443e105 Compare January 10, 2020 13:42

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jan 12, 2020

net: Use log categories when logging events that P2P peers can trigge…

6f2ecd9

…r arbitrarily Github-Pull: bitcoin#17828 Rebased-From: 443e105f7aed5ae6ac820682fb2f2215d8163ca0

practicalswift requested a review from laanwj January 14, 2020 22:39

This was referenced Jan 22, 2020

[0.18] net: Log to net category for exceptions in ProcessMessages #17974

Merged

net: Log to net category for exceptions in ProcessMessages #17762

Merged

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Feb 9, 2020

net: Use log categories when logging events that P2P peers can trigge…

0015578

…r arbitrarily Github-Pull: bitcoin#17828 Rebased-From: 443e105f7aed5ae6ac820682fb2f2215d8163ca0

DrahtBot mentioned this pull request Feb 11, 2020

Return BlockValidationState from ProcessNewBlock if CheckBlock/AcceptBlock fails #17479

Closed

DrahtBot mentioned this pull request Feb 28, 2020

validation: Templatize ValidationState instead of subclassing #17399

Merged

DrahtBot added the Needs rebase label Mar 1, 2020

net: Use log categories when logging events that P2P peers can trigge…

0496062

…r arbitrarily

practicalswift force-pushed the log-categories branch from 443e105 to 0496062 Compare March 5, 2020 21:27

DrahtBot removed the Needs rebase label Mar 5, 2020

maflcko approved these changes Mar 5, 2020

View reviewed changes

practicalswift closed this Mar 6, 2020

practicalswift deleted the log-categories branch April 10, 2021 19:39

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

maflcko added the Up for grabs label Feb 20, 2023

Conversation

practicalswift commented Dec 29, 2019

Uh oh!

hebasto commented Dec 29, 2019

Uh oh!

DrahtBot commented Dec 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

practicalswift commented Jan 2, 2020

Uh oh!

practicalswift commented Jan 8, 2020

Uh oh!

practicalswift commented Jan 10, 2020

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Mar 5, 2020

Choose a reason for hiding this comment

Uh oh!

practicalswift Mar 6, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko commented Mar 6, 2020

Uh oh!

practicalswift commented Mar 6, 2020

Uh oh!

maflcko commented Mar 6, 2020

Uh oh!

practicalswift commented Mar 6, 2020

Uh oh!

maflcko commented Mar 6, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

DrahtBot commented Dec 30, 2019 •

edited

Loading