LogPrintf has many issues:
LogInfo, according to the dev notes.Fix all issues by removing the deprecated alias.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/29641.
<!--021abf342d371248e50ceaed478a90ca-->
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 <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
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.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
concept ACK fae5751
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.
Ok, makes sense. I'll put it in draft for now, to allow for more time, if people want to change log messages further.
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 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.
<sub>Debug: https://github.com/bitcoin/bitcoin/runs/25181673072</sub>
Concept ACK
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.
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed. <sub>Debug: https://github.com/bitcoin/bitcoin/runs/27330763084</sub>
<details><summary>Hints</summary>
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.
</details>
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:
diff --git a/src/validation.cpp b/src/validation.cpp
index eafe19c767..99271a3358 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -186,12 +186,12 @@ std::optional<std::vector<int>> CalculatePrevHeights(
std::vector<int> prev_heights;
prev_heights.resize(tx.vin.size());
for (size_t i = 0; i < tx.vin.size(); ++i) {
- if (auto coin{coins.GetCoin(tx.vin[i].prevout)}) {
+ if (auto coin{Assume(coins.GetCoin(tx.vin[i].prevout))}) {
prev_heights[i] = coin->nHeight == MEMPOOL_HEIGHT
? tip.nHeight + 1 // Assume all mempool transaction confirm in the next block.
: coin->nHeight;
} else {
- LogInfo("ERROR: %s: Missing input %d in transaction \'%s\'\n", __func__, i, tx.GetHash().GetHex());
+ LogError("%s", STR_INTERNAL_BUG(strprintf("Missing input %d in transaction '%s'!", i, tx.GetHash().GetHex())));
return std::nullopt;
}
}
@@ -1196,7 +1196,7 @@ bool MemPoolAccept::ConsensusScriptChecks(const ATMPArgs& args, Workspace& ws)
script_verify_flags currentBlockScriptVerifyFlags{GetBlockScriptFlags(*m_active_chainstate.m_chain.Tip(), m_active_chainstate.m_chainman)};
if (!CheckInputsFromMempoolAndCache(tx, state, m_view, m_pool, currentBlockScriptVerifyFlags,
ws.m_precomputed_txdata, m_active_chainstate.CoinsTip(), GetValidationCache())) {
- LogError("BUG! PLEASE REPORT THIS! CheckInputScripts failed against latest-block but not STANDARD flags %s, %s", hash.ToString(), state.ToString());
+ LogError("%s", STR_INTERNAL_BUG(strprintf("CheckInputScripts failed against latest-block but not STANDARD flags %s, %s", hash.ToString(), state.ToString())));
return Assume(false);
}
@@ -1277,8 +1277,8 @@ bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector<Workspace>&
Assume(false);
all_submitted = false;
package_state.Invalid(PackageValidationResult::PCKG_MEMPOOL_ERROR,
- strprintf("BUG! PolicyScriptChecks succeeded but ConsensusScriptChecks failed: %s",
- ws.m_ptx->GetHash().ToString()));
+ STR_INTERNAL_BUG(strprintf("PolicyScriptChecks succeeded but ConsensusScriptChecks failed: %s",
+ ws.m_ptx->GetHash().ToString())));
// Remove the transaction from the mempool.
if (!m_subpackage.m_changeset) m_subpackage.m_changeset = m_pool.GetChangeSet();
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.
We should likely rename the file and class as well to match
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)
does the custom lint name need an update now?
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__)
Since the file contains 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__); \
this line seems weird now, defining debug as Info
Concept ACK
(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.
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed. <sub>Debug: https://github.com/bitcoin/bitcoin/runs/30702762713</sub>
<details><summary>Hints</summary>
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.
</details>
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
forgot to add pkgconf?
<!--9cd9c72976c961c55c7acef8f6ba82cd-->
| File | commit 144f98db85eb35aaf4f55849f45a18247edf5902<br>(master) | commit 7a4af2bf82261b536459fc657a1862a32b0ff332<br>(master and this pull) |
|---|---|---|
| SHA256SUMS.part | 1c004827138ad8a3... |
02a507287cbc2ec9... |
| *-aarch64-linux-gnu-debug.tar.gz | 4f390b9052bee4eb... |
2530fbb6b6728af6... |
| *-aarch64-linux-gnu.tar.gz | 6f9e3bc3176b932c... |
206798944f0ca6bd... |
| *-arm-linux-gnueabihf-debug.tar.gz | 2bc81317aafce85f... |
06e809f8524ee22a... |
| *-arm-linux-gnueabihf.tar.gz | 8e54bb98ef268e66... |
f1e8e7ed781d9728... |
| *-arm64-apple-darwin-unsigned.tar.gz | 760ff2eff02bb070... |
fddc9f74cc5cd479... |
| *-arm64-apple-darwin-unsigned.zip | aeb60be790e2b8f3... |
1d68bde83e62a413... |
| *-arm64-apple-darwin.tar.gz | cb6f5d76ee3dda4f... |
2ad15cc1018a8b29... |
| *-powerpc64-linux-gnu-debug.tar.gz | 4a8daf44ebe9714e... |
9d11157e0d086977... |
| *-powerpc64-linux-gnu.tar.gz | 030347c206f6d8ef... |
16f238a750613fce... |
| *-riscv64-linux-gnu-debug.tar.gz | 373b3273685e4d51... |
2a4aa5bad8612e6d... |
| *-riscv64-linux-gnu.tar.gz | e9be777ab634838f... |
6f938de398963d53... |
| *-x86_64-apple-darwin-unsigned.tar.gz | fb7477fd0c2531d0... |
3ce7528226fe9fe5... |
| *-x86_64-apple-darwin-unsigned.zip | 9c45cc0383b71309... |
970eba0c751f91c5... |
| *-x86_64-apple-darwin.tar.gz | 5601d925876fd50a... |
3479956bc59b7acf... |
| *-x86_64-linux-gnu-debug.tar.gz | d18f37f54bf31654... |
9e799c33ec810764... |
| *-x86_64-linux-gnu.tar.gz | 7f9028b87f0ff7e8... |
36607df6e962fbfa... |
| *.tar.gz | ad7543a15fee40fe... |
1fb69a13f48ea83b... |
| guix_build.log | bfede8657a978bd6... |
86c8239909dd5c6d... |
| guix_build.log.diff | 7a0df0371fb92b57... |
<!--9cd9c72976c961c55c7acef8f6ba82cd-->
| File | commit 22723c809a8abce415195693546e2a7c03e516c4<br>(master) | commit 8160e11e8bf36429f03c7ea7193cb6205d23398e<br>(master and this pull) |
|---|---|---|
| SHA256SUMS.part | 9f62b39763c37372... |
6f4643c09b4a08a7... |
| *-aarch64-linux-gnu-debug.tar.gz | de1ca4d38be6158f... |
7cdb07136c161db4... |
| *-aarch64-linux-gnu.tar.gz | 65e51aff97fbd383... |
e549909863348352... |
| *-arm-linux-gnueabihf-debug.tar.gz | 4bbc38eac723fb02... |
c1e95cd83abfd84c... |
| *-arm-linux-gnueabihf.tar.gz | 16c178e224449c6f... |
4ad24c6272053ea6... |
| *-arm64-apple-darwin-unsigned.tar.gz | 4d7781dd06264c88... |
296554aac939aa81... |
| *-arm64-apple-darwin-unsigned.zip | ea6c5a60996c003a... |
5ab183e6e007ffd3... |
| *-arm64-apple-darwin.tar.gz | bfce458364631d02... |
dd29e60ac23ed8e6... |
| *-powerpc64-linux-gnu-debug.tar.gz | 05f0daf8e1bc09e5... |
12b14a9ed348b505... |
| *-powerpc64-linux-gnu.tar.gz | f3665dc6745666d8... |
069b314f355931f6... |
| *-riscv64-linux-gnu-debug.tar.gz | 798cb191cce683f7... |
abdf79b769297d0b... |
| *-riscv64-linux-gnu.tar.gz | fd5703407bf26dd7... |
d5a1a63d5b802883... |
| *-x86_64-apple-darwin-unsigned.tar.gz | d2f432f3aa99a5a5... |
372f17632d548386... |
| *-x86_64-apple-darwin-unsigned.zip | 01974c25640fd599... |
da00421d29b6ef02... |
| *-x86_64-apple-darwin.tar.gz | 91a46abb27c44ee5... |
a015f1148c8426bd... |
| *-x86_64-linux-gnu-debug.tar.gz | a463ef88e05bcdc3... |
bf6c4148b5327b42... |
| *-x86_64-linux-gnu.tar.gz | 793cc9fbf1a319fc... |
da68d7a29c87a91f... |
| *.tar.gz | 478531d86c4e0bfa... |
9106bc777d1074b1... |
| guix_build.log | f7c69749ca6bde97... |
1634a4f619597adb... |
| guix_build.log.diff | b892903a164121fa... |
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed. <sub>Debug: https://github.com/bitcoin/bitcoin/runs/40077970072</sub>
<details><summary>Hints</summary>
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.
</details>
<!--9cd9c72976c961c55c7acef8f6ba82cd-->
| File | commit cdc32994feadf3f15df3cfac5baae36b4b011462<br>(master) | commit 2e5836b57203006270fa40bdb975e89de9ab94bc<br>(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... |
<!--9cd9c72976c961c55c7acef8f6ba82cd-->
| File | commit cdc32994feadf3f15df3cfac5baae36b4b011462<br>(master) | commit 2e5836b57203006270fa40bdb975e89de9ab94bc<br>(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... |
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed. <sub>Debug: previous releases, depends DEBUG https://github.com/bitcoin/bitcoin/runs/41251947428</sub> <sub>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.</sub>
<details><summary>Hints</summary>
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.
</details>
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed.
<sub>Task ARM, unit tests, no functional tests: https://github.com/bitcoin/bitcoin/runs/41651827597</sub>
<sub>LLM reason (✨ experimental): The CI failure is due to a build error: a member function was incorrectly named.
</sub>
<details><summary>Hints</summary>
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.
</details>
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed.
<sub>Task lint: https://github.com/bitcoin/bitcoin/runs/44024372509</sub>
<sub>LLM reason (✨ experimental): The CI failure is due to errors encountered during the linting process.</sub>
<details><summary>Hints</summary>
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.
</details>
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed.
<sub>Task ARM, unit tests, no functional tests: https://github.com/bitcoin/bitcoin/runs/45922957541</sub>
<sub>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.</sub>
<details><summary>Hints</summary>
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.
</details>
ACK 317b71bcd2e552f93ac2131b16a8acab414567dd - I think the conflicts here are small, and a number are themselves draft / WIP.
Is this ready for review now?
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
ACK fa4395dffd432b999002dfd24eb6f8d7384fbcbe
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.
Milestone
31.0
