This picks up the first commit from #29231, but extends it to also cover cases that were missed in it.
As per https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#logging, LogError should be used for severe problems that require the node to shut down.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For detailed information about the code coverage, see the test coverage report.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | stickies-v, kevkevinpal, achow101 |
| Stale ACK | BrandonOdiwuor |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
<!--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.
5519 | @@ -5520,7 +5520,7 @@ Chainstate& ChainstateManager::InitializeChainstate(CTxMemPool* mempool) 5520 | const bool destroyed = DestroyDB(path_str); 5521 | 5522 | if (!destroyed) { 5523 | - LogPrintf("error: leveldb DestroyDB call failed on %s\n", path_str); 5524 | + LogError("[snapshot] leveldb DestroyDB call failed on %s\n", path_str);
I don't think this should be LogError as this can be recovered from, e.g. here. Ideally, we just return error messages here and let the callsite log - but barring that I'd just leave as is since all the call sites that do lead to node termination will already log on the error level?
Good point, deferred for the follow-up that handles warnings.
6302 | @@ -6303,7 +6303,7 @@ bool ChainstateManager::ValidatedSnapshotCleanup() 6303 | fs::path p_old, 6304 | fs::path p_new, 6305 | const fs::filesystem_error& err) { 6306 | - LogPrintf("Error renaming path (%s) -> (%s): %s\n", 6307 | + LogError("[snapshot] Error renaming path (%s) -> (%s): %s\n",
nit: since we're calling GetNotifications().fatalError() next, I think this log line can just be removed?
My understanding is that the notification does not include err.what(), so the debug log in this line is still needed.
Can't we just merge them?
<details> <summary>git diff on b90f1c36ff</summary>
diff --git a/src/validation.cpp b/src/validation.cpp
index 89aa84a551..ac95824443 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -6303,12 +6303,10 @@ bool ChainstateManager::ValidatedSnapshotCleanup()
fs::path p_old,
fs::path p_new,
const fs::filesystem_error& err) {
- LogError("[snapshot] Error renaming path (%s) -> (%s): %s\n",
- fs::PathToString(p_old), fs::PathToString(p_new), err.what());
GetNotifications().fatalError(strprintf(_(
- "Rename of '%s' -> '%s' failed. "
+ "[snapshot] Rename of '%s' -> '%s' failed: %s. "
"Cannot clean up the background chainstate leveldb directory."),
- fs::PathToString(p_old), fs::PathToString(p_new)));
+ fs::PathToString(p_old), fs::PathToString(p_new), err.what()));
};
try {
</details>
I think it is fine to have this log, but happy to review a separate pull removing it :)
This reminded me to change the two other log lines before fatalError to use the error level. So I pushed that other change in the last push.
ACK b90f1c36ff0e469bc64830ff35e591a9aab63fb2 - more consistent logging
re-ACK 2222f2573c8de0a7f7171e6fc9e844ad448e1f65
As per doc/developer-notes#logging, LogError should be used for
severe problems that require the node to shut down.
Co-Authored-By: stickies-v <stickies-v@protonmail.com>
ACK 2222f2573c8de0a7f7171e6fc9e844ad448e1f65 - Replacing LogPrintf with Logerror to enhance critical error logging
re-ACK fae3a1f0065064d80ab4c0375a9eaeb666c5dd55, I'm ~0 on the latest force push as user_error was already logged at the right level through GetNotifications().fatalError(user_error); so I'd be in favour of deduplicating/cleaning up this logging logic but can be done in follow-up.
user_errorwas already logged at the right level throughGetNotifications().fatalError(user_error);
Happy to review a follow-up for the two cases (https://github.com/bitcoin/bitcoin/pull/30255#discussion_r1633421463). For now I think it is risk-free and more consistent to log the fatal error twice in the error log category.
ACK fae3a1f
agree with #30255 (review) but that should be good for a follow up
ACK fae3a1f0065064d80ab4c0375a9eaeb666c5dd55