LogPrintf has many issues:
LogInfo, according to the dev notes.Fix all issues by removing the deprecated alias.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/29641.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | stickies-v, ajtowns, rkrux |
| Concept ACK | sedited, l0rinc |
| Stale ACK | kevkevinpal, fanquake |
If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
I noticed that there are helper functions such as the following using the LogPrintf naming scheme
WalletLogPrintf in src/wallet/wallet.hLogPrintfCategoryWithThreadNames, LogPrintfCategory, LogPrintfCategoryWithoutThreadNames, LogPrintfWithoutThreadNames, LogPrintfWithThreadNames in ./src/bench/logging.cppUsing this grep grep -nri "\<LogPrintLevel\>" ./src --binary-files=without-match
I also noticed that we are using LogPrintLevel when we could be using LogInfo, LogWarning, LogError, LogDebug and LogTrace
these might want to be addressed in a separate PR though
Concept -0. Not a strong opinion, but I think just mechanically replacing s/LogPrint/LogDebug/ and s/LogPrintf/LogInfo/ everywhere would not provide a major benefit, and while it may be true that “this will have to be done at some point” I think the point where it’d be nicest to do this would be after #29256 when we are able to define log sources to make LogDebug calls less verbose, and add missing category information to LogInfo calls.
I wouldn’t object to this PR if other reviewers think it’s a worthwhile improvement and are ok with the long list of conflicts. I just think there are other improvements we should make to log prints besides this one, and it would be better to change each individual log print once instead of changing it multiple times.
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.
Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
Leave a comment here, if you need help tracking down a confusing failure.
I suggest #29256 (comment) instead.
It may also make sense to finish consensus and work on the simplest possible consistent user-facing API and developer API before doing a mass migration.
I suggest #29256 (comment) instead.
It may also make sense to finish consensus and work on the simplest possible consistent user-facing API and developer API before doing a mass migration.
I don’t think the changes here conceptually conflict with any open pull request. There are discussions around the naming and functionality around LogWarning and LogError, but I don’t think they are changed or touched in this pull request?
However, the changes here conflict with quite a few open pull requests because the same lines (or adjacent lines) are touched, which is why this is marked as “draft” (not ready for merge).
Personally, I don’t think this change is high priority and it can wait. While I like the new macro names and functionality (and all reviewers who approved the pull request introducing them, seemed to be liking them as well?), due to the number of conflicts and the low priority, this will be probably be sitting for a while.
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/27330763084
Make sure to run all tests locally, according to the documentation.
The failure may happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
182@@ -183,7 +183,7 @@ std::optional<std::vector<int>> CalculatePrevHeights(
183 const CTxIn& txin = tx.vin[i];
184 Coin coin;
185 if (!coins.GetCoin(txin.prevout, coin)) {
186- LogPrintf("ERROR: %s: Missing input %d in transaction \'%s\'\n", __func__, i, tx.GetHash().GetHex());
187+ LogInfo("ERROR: %s: Missing input %d in transaction \'%s\'\n", __func__, i, tx.GetHash().GetHex());
Based on the message and
LogError(fmt, params...)should be used in place ofLogInfofor severe problems that require the node (or a subsystem) to shut down entirely (e.g., insufficient storage space).
and the discussion in #30849 (review), this looks like a LogError instead (will throw in https://github.com/bitcoin/bitcoin/blob/master/src/validation.cpp#L895-L898 if nullopt is returned).
Edit: based on the comments in the above thread this may be a LogWarn instead:
LogWarning(fmt, params...)should be used in place ofLogInfofor severe problems that the node admin should address, but are not severe enough to warrant shutting down the node (e.g., system time appears to be wrong, unknown soft fork appears to have activated).
It is an internal bug, so it is dead code and doesn’t really matter much. I think internal bugs should be using Assume(false). Logs could go with LogError("%s", STR_INTERNAL_BUG("...")), but I wanted to keep the changes here minimal. Happy to go with LogError("%s", STR_INTERNAL_BUG( if you feel strongly.
edit: the diff would look like:
0diff --git a/src/validation.cpp b/src/validation.cpp
1index eafe19c767..99271a3358 100644
2--- a/src/validation.cpp
3+++ b/src/validation.cpp
4@@ -186,12 +186,12 @@ std::optional<std::vector<int>> CalculatePrevHeights(
5 std::vector<int> prev_heights;
6 prev_heights.resize(tx.vin.size());
7 for (size_t i = 0; i < tx.vin.size(); ++i) {
8- if (auto coin{coins.GetCoin(tx.vin[i].prevout)}) {
9+ if (auto coin{Assume(coins.GetCoin(tx.vin[i].prevout))}) {
10 prev_heights[i] = coin->nHeight == MEMPOOL_HEIGHT
11 ? tip.nHeight + 1 // Assume all mempool transaction confirm in the next block.
12 : coin->nHeight;
13 } else {
14- LogInfo("ERROR: %s: Missing input %d in transaction \'%s\'\n", __func__, i, tx.GetHash().GetHex());
15+ LogError("%s", STR_INTERNAL_BUG(strprintf("Missing input %d in transaction '%s'!", i, tx.GetHash().GetHex())));
16 return std::nullopt;
17 }
18 }
19@@ -1196,7 +1196,7 @@ bool MemPoolAccept::ConsensusScriptChecks(const ATMPArgs& args, Workspace& ws)
20 script_verify_flags currentBlockScriptVerifyFlags{GetBlockScriptFlags(*m_active_chainstate.m_chain.Tip(), m_active_chainstate.m_chainman)};
21 if (!CheckInputsFromMempoolAndCache(tx, state, m_view, m_pool, currentBlockScriptVerifyFlags,
22 ws.m_precomputed_txdata, m_active_chainstate.CoinsTip(), GetValidationCache())) {
23- LogError("BUG! PLEASE REPORT THIS! CheckInputScripts failed against latest-block but not STANDARD flags %s, %s", hash.ToString(), state.ToString());
24+ LogError("%s", STR_INTERNAL_BUG(strprintf("CheckInputScripts failed against latest-block but not STANDARD flags %s, %s", hash.ToString(), state.ToString())));
25 return Assume(false);
26 }
27
28@@ -1277,8 +1277,8 @@ bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector<Workspace>&
29 Assume(false);
30 all_submitted = false;
31 package_state.Invalid(PackageValidationResult::PCKG_MEMPOOL_ERROR,
32- strprintf("BUG! PolicyScriptChecks succeeded but ConsensusScriptChecks failed: %s",
33- ws.m_ptx->GetHash().ToString()));
34+ STR_INTERNAL_BUG(strprintf("PolicyScriptChecks succeeded but ConsensusScriptChecks failed: %s",
35+ ws.m_ptx->GetHash().ToString())));
36 // Remove the transaction from the mempool.
37 if (!m_subpackage.m_changeset) m_subpackage.m_changeset = m_pool.GetChangeSet();
38 m_subpackage.m_changeset->StageRemoval(m_pool.GetIter(ws.m_ptx->GetHash()).value());
8@@ -9,7 +9,7 @@
9
10 namespace bitcoin {
11
12-// Warn about any use of LogPrintf that does not end with a newline.
13+// Warn about any use of LogInfo that does not end with a newline.
92+ LogInfo("hello world!...");
93 }
94 void bad_func4_ignored()
95 {
96- LogPrintf("hello world!"); // NOLINT(bitcoin-unterminated-logprintf)
97+ LogInfo("hello world!"); // NOLINT(bitcoin-unterminated-logprintf)
19@@ -20,11 +20,11 @@ static inline void LogPrintf_(const std::string& logging_function, const std::st
20 }
21
22 #define LogPrintLevel_(category, level, ...) LogPrintf_(__func__, __FILE__, __LINE__, category, level, __VA_ARGS__)
23-#define LogPrintf(...) LogPrintLevel_(LogFlags::NONE, Level::None, __VA_ARGS__)
24+#define LogInfo(...) LogPrintLevel_(LogFlags::NONE, Level::None, __VA_ARGS__)
LogInfo examples now, we should rename the file as well.
24+#define LogInfo(...) LogPrintLevel_(LogFlags::NONE, Level::None, __VA_ARGS__)
25
26 #define LogDebug(category, ...) \
27 do { \
28- LogPrintf(__VA_ARGS__); \
29+ LogInfo(__VA_ARGS__); \
debug as Info
(This draft pull request isn’t ready for review. The previous 5 comments are on stuff that will be deleted anyway.)
For now, please focus review on non-draft pull requests. There should be plenty right now.
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/30702762713
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
108@@ -109,7 +109,8 @@ jobs:
109 run: |
110 # A workaround for "The `brew link` step did not complete successfully" error.
111 brew install --quiet python@3 || brew link --overwrite python@3
112- brew install --quiet coreutils ninja pkg-config gnu-getopt ccache boost libevent zeromq qt@5 qrencode
113+ brew unlink pkg-config
114+ brew install --quiet coreutils ninja gnu-getopt ccache boost libevent zeromq qt@5 qrencode
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/40077970072
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
| File | commit cdc32994feadf3f15df3cfac5baae36b4b011462(master) | commit 2e5836b57203006270fa40bdb975e89de9ab94bc(pull/$29641/merge) |
|---|---|---|
| *-arm-linux-gnueabihf-debug.tar.gz | 03657941dd88df39... |
ddbb7b8ec70bcff8... |
| *-x86_64-linux-gnu.tar.gz | 38e919eeeb4cf11f... |
9f686ad3168769f3... |
| *-x86_64-linux-gnu-debug.tar.gz | 25c5b9ff38beb61e... |
8c58fa69424ad61f... |
| *-arm64-apple-darwin-unsigned.zip | c8bff07d08664e09... |
da401efb70000a03... |
| *-x86_64-apple-darwin-unsigned.tar.gz | 7f68411d59e4da91... |
8101df692fe6c29d... |
| *-powerpc64-linux-gnu.tar.gz | 9963c6483b222801... |
6b183ed36014e527... |
| *-arm64-apple-darwin-codesigning.tar.gz | 553ab37d14d2371e... |
8091ed9153f9a094... |
| *-riscv64-linux-gnu-debug.tar.gz | 759af97886144bd7... |
035bbdb8bf38e766... |
| *-riscv64-linux-gnu.tar.gz | 0f2bd80a8c005f26... |
17ec67af43b37c68... |
| *-aarch64-linux-gnu.tar.gz | 5aea0cec71322bcc... |
24ead07f5563df6d... |
| *-arm64-apple-darwin-unsigned.tar.gz | 80bfbae3aa772f74... |
574b9576f6f4fbf8... |
| guix_build.log | 92bb3587787aeaeb... |
fff1c68567b9b161... |
| guix_build.log.diff | 3914f7c75e076193... |
|
| *-aarch64-linux-gnu-debug.tar.gz | 362ced3e339f1812... |
542e9bfa188c82ec... |
| *-x86_64-apple-darwin-unsigned.zip | 53e1009e277faffc... |
ab59c62a900a0777... |
| SHA256SUMS.part | d82163924bc84482... |
d3d6a6b229301d85... |
| *-x86_64-apple-darwin-codesigning.tar.gz | ea0aa1b091751f39... |
031dbd717c0283d9... |
| *-powerpc64-linux-gnu-debug.tar.gz | b579128b84939d87... |
2800088216047b98... |
| *-arm-linux-gnueabihf.tar.gz | 6a284e891567742b... |
3c28fadb23963d9e... |
| *.tar.gz | 0964f14427b3bc12... |
4e5477d443991af8... |
| File | commit cdc32994feadf3f15df3cfac5baae36b4b011462(master) | commit 2e5836b57203006270fa40bdb975e89de9ab94bc(pull/29641/merge) |
|---|---|---|
| *-aarch64-linux-gnu-debug.tar.gz | 362ced3e339f1812... |
542e9bfa188c82ec... |
| *-aarch64-linux-gnu.tar.gz | 5aea0cec71322bcc... |
24ead07f5563df6d... |
| *-arm-linux-gnueabihf-debug.tar.gz | 03657941dd88df39... |
ddbb7b8ec70bcff8... |
| *-arm-linux-gnueabihf.tar.gz | 6a284e891567742b... |
3c28fadb23963d9e... |
| *-arm64-apple-darwin-codesigning.tar.gz | 553ab37d14d2371e... |
8091ed9153f9a094... |
| *-arm64-apple-darwin-unsigned.tar.gz | 80bfbae3aa772f74... |
574b9576f6f4fbf8... |
| *-arm64-apple-darwin-unsigned.zip | c8bff07d08664e09... |
da401efb70000a03... |
| *-powerpc64-linux-gnu-debug.tar.gz | b579128b84939d87... |
2800088216047b98... |
| *-powerpc64-linux-gnu.tar.gz | 9963c6483b222801... |
6b183ed36014e527... |
| *-riscv64-linux-gnu-debug.tar.gz | 759af97886144bd7... |
035bbdb8bf38e766... |
| *-riscv64-linux-gnu.tar.gz | 0f2bd80a8c005f26... |
17ec67af43b37c68... |
| *-x86_64-apple-darwin-codesigning.tar.gz | ea0aa1b091751f39... |
031dbd717c0283d9... |
| *-x86_64-apple-darwin-unsigned.tar.gz | 7f68411d59e4da91... |
8101df692fe6c29d... |
| *-x86_64-apple-darwin-unsigned.zip | 53e1009e277faffc... |
ab59c62a900a0777... |
| *-x86_64-linux-gnu-debug.tar.gz | 25c5b9ff38beb61e... |
8c58fa69424ad61f... |
| *-x86_64-linux-gnu.tar.gz | 38e919eeeb4cf11f... |
9f686ad3168769f3... |
| *.tar.gz | 0964f14427b3bc12... |
4e5477d443991af8... |
| SHA256SUMS.part | d82163924bc84482... |
d3d6a6b229301d85... |
| guix_build.log | 92bb3587787aeaeb... |
fff1c68567b9b161... |
| guix_build.log.diff | 3914f7c75e076193... |
🚧 At least one of the CI tasks failed. Debug: previous releases, depends DEBUG https://github.com/bitcoin/bitcoin/runs/41251947428 LLM reason (✨ experimental): The CI failure is caused by a compilation error in src/init.cpp due to a syntax mistake with GetPi and incomplete line, leading to a broken function.
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
🚧 At least one of the CI tasks failed.
Task ARM, unit tests, no functional tests: https://github.com/bitcoin/bitcoin/runs/41651827597
LLM reason (✨ experimental): The CI failure is due to a build error: a member function was incorrectly named.
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
🚧 At least one of the CI tasks failed.
Task lint: https://github.com/bitcoin/bitcoin/runs/44024372509
LLM reason (✨ experimental): The CI failure is due to errors encountered during the linting process.
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
🚧 At least one of the CI tasks failed.
Task ARM, unit tests, no functional tests: https://github.com/bitcoin/bitcoin/runs/45922957541
LLM reason (✨ experimental): The CI failure is caused by a compilation error due to the use of a non-declared function ‘RemovePidFileeeeeeeeeeeee’ and the compiler being set to treat all warnings as errors.
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
Is this ready for review now?
No. Someone would have to pick up #29231, because most of the logs are alerts, and not info logs.
So i went ahead and did that in https://github.com/bitcoin/bitcoin/pull/33960
This refactor does not change behavior.
-BEGIN VERIFY SCRIPT-
sed --in-place 's/\<LogPrintf\>/LogInfo/g' \
$( git grep -l '\<LogPrintf\>' -- ./contrib/ ./src/ ./test/ ':(exclude)src/logging.h' )
-END VERIFY SCRIPT-
ACK fa4395dffd432b999002dfd24eb6f8d7384fbcbe
nit: would be nice to update the scripted-diff to also remove trailing newlines if that’s not a huge pita
lgtm ACK fa4395dffd432b999002dfd24eb6f8d7384fbcbe
Thanks for the cleanup, best to remove the deprecated LogPrintf alias.
It wastes review cycles, because reviewers sometimes point out that it is deprecated.
I reviewed #34008 where exactly this^ happened: #34008 (review)
nit: would be nice to update the scripted-diff to also remove trailing newlines if that’s not a huge pita
Thx, I can take a look, if I have to re-touch.
Labels
Refactoring
Milestone
31.0