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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
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.
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);
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?
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",
GetNotifications().fatalError() next, I think this log line can just be removed?
err.what(), so the debug log in this line is still needed.
Can’t we just merge them?
0diff --git a/src/validation.cpp b/src/validation.cpp
1index 89aa84a551..ac95824443 100644
2--- a/src/validation.cpp
3+++ b/src/validation.cpp
4@@ -6303,12 +6303,10 @@ bool ChainstateManager::ValidatedSnapshotCleanup()
5 fs::path p_old,
6 fs::path p_new,
7 const fs::filesystem_error& err) {
8- LogError("[snapshot] Error renaming path (%s) -> (%s): %s\n",
9- fs::PathToString(p_old), fs::PathToString(p_new), err.what());
10 GetNotifications().fatalError(strprintf(_(
11- "Rename of '%s' -> '%s' failed. "
12+ "[snapshot] Rename of '%s' -> '%s' failed: %s. "
13 "Cannot clean up the background chainstate leveldb directory."),
14- fs::PathToString(p_old), fs::PathToString(p_new)));
15+ fs::PathToString(p_old), fs::PathToString(p_new), err.what()));
16 };
17
18 try {
fatalError to use the error level. So I pushed that other change in the last push.
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>
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
