Skip to content
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

net: Use log categories when logging events that P2P peers can trigger arbitrarily #17828

Closed
wants to merge 1 commit into from

Conversation

practicalswift
Copy link
Contributor

@practicalswift practicalswift commented Dec 29, 2019

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
Copy link
Member

@hebasto hebasto commented Dec 29, 2019

Concept ACK.

@DrahtBot
Copy link
Contributor

@DrahtBot DrahtBot commented Dec 30, 2019

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

Conflicts

No conflicts as of last run.

Copy link
Member

@promag promag left a comment

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

src/util/system.h Outdated Show resolved Hide resolved
@practicalswift
Copy link
Contributor Author

@practicalswift practicalswift commented Jan 2, 2020

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

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this issue Jan 5, 2020
@practicalswift
Copy link
Contributor Author

@practicalswift practicalswift commented Jan 8, 2020

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

Please re-review :)

@practicalswift
Copy link
Contributor Author

@practicalswift practicalswift commented Jan 10, 2020

Rebased! :)

Copy link
Member

@MarcoFalke MarcoFalke left a comment

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

@@ -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());
Copy link
Member

@MarcoFalke MarcoFalke Mar 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BCLog::VALIDATION is used for the validation interface. Is the same category applicable here?

Copy link
Contributor Author

@practicalswift practicalswift Mar 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Mar 6, 2020

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
Copy link
Contributor Author

@practicalswift practicalswift commented Mar 6, 2020

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

@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Mar 6, 2020

This PR should be revisited after a guideline has been established

@practicalswift
Copy link
Contributor Author

@practicalswift practicalswift commented Mar 6, 2020

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

@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Mar 6, 2020

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

@practicalswift practicalswift deleted the log-categories branch Apr 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants