These are mostly followups from #30968, making the code, log lines, error messages, and comments more consistent.
The last commit is an attempt at improving the error reporting when loading the chainstate. It aims to more cleanly distinguish between errors arising from a specific database, and errors where the culprit may be less clear.
It is bad, because it is both printed for non-GUI users and does not
convey additional information.
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
Shutdown is indeed called, and it being overkill does not make sense
either.
<!--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 | mzumsande, BrandonOdiwuor, stickies-v, achow101 |
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.
52 | + .path = chainman.m_options.datadir / "blocks" / "index", 53 | + .cache_bytes = static_cast<size_t>(cache_sizes.block_tree_db), 54 | + .memory_only = options.block_tree_db_in_memory, 55 | + .wipe_data = options.wipe_block_tree_db, 56 | + .options = chainman.m_options.block_tree_db}); 57 | + } catch (dbwrapper_error&) {
Note: instantiating a CDBWrapper can throw other errors such as a fs::filesystem_error from here, but I suppose that is not something the user can recover from with a reindex so not catching that here seems correct.
I think it logging the dbwrapper_error would be useful though?
<details> <summary>git diff on 1bec418e77</summary>
diff --git a/src/node/chainstate.cpp b/src/node/chainstate.cpp
index 76094105fa..b9c3981910 100644
--- a/src/node/chainstate.cpp
+++ b/src/node/chainstate.cpp
@@ -48,8 +48,10 @@ static ChainstateLoadResult CompleteChainstateInitialization(
.memory_only = options.block_tree_db_in_memory,
.wipe_data = options.wipe_block_tree_db,
.options = chainman.m_options.block_tree_db});
- } catch (dbwrapper_error&) {
- return {ChainstateLoadStatus::FAILURE, _("Error opening block database")};
+ } catch (dbwrapper_error& err) {
+ auto msg{_("Error opening block database")};
+ LogError("%s: %s\n", msg.original, err.what());
+ return {ChainstateLoadStatus::FAILURE, msg};
}
if (options.wipe_block_tree_db) {
@@ -116,8 +118,10 @@ static ChainstateLoadResult CompleteChainstateInitialization(
/*cache_size_bytes=*/chainman.m_total_coinsdb_cache * init_cache_fraction,
/*in_memory=*/options.coins_db_in_memory,
/*should_wipe=*/options.wipe_chainstate_db);
- } catch (dbwrapper_error&) {
- return {ChainstateLoadStatus::FAILURE, _("Error opening coins database")};
+ } catch (dbwrapper_error& err) {
+ auto msg{_("Error opening coins database")};
+ LogError("%s: %s\n", msg.original, err.what());
+ return {ChainstateLoadStatus::FAILURE, msg};
}
if (options.coins_error_cb) {
</details>
Approach ACK 1bec418e77482a4c53c8f1e7c89a151fdeae0f8e
one more small thing that I noticed reviewing #30968 (it wasn't really on-topic there, but fits with the last commit here): I don't think that we should return ChainstateLoadStatus::FAILURE in https://github.com/bitcoin/bitcoin/blob/5837e3463fe188a65d67f96e84cbcca674d61d9e/src/node/chainstate.cpp#L239 (snapshot validation failure). This is not a problem where a general reindex would typically help, and is handled inside of validation (InvalidateCoinsDBOnDisk) anyway, so something like node::ChainstateLoadStatus::FAILURE_FATAL might be bettter.
They should name the correct source of an error, or be generic if no
clear source can be ascertained.
A general reindex won't typically help in this case, and there is
already some action being taken with the call to
`InvalidateCoinsDBOnDisk`.
Thank you for the reviews @mzumsande and @stickies-v,
Updated 1bec418e77482a4c53c8f1e7c89a151fdeae0f8e -> a7a02b09fafe9d03e5db1fb9d9600f6a13aaa852 (chainstate_init_followup_0 -> chainstate_init_followup_1, compare)
Code Review / lightly tested ACK 31cc5006c3de4dd6a1f7a238684163956604df45
Code Review ACK 31cc5006c3de4dd6a1f7a238684163956604df45.
Nice work on the chainstate loading improvements, especially the enhanced handling of database-specific errors.
245 | @@ -236,7 +246,7 @@ ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSize
246 | return {init_status, init_error};
unrelated nit / probably scope creep but it is a "small chainstate load improvement" so leaving here anyway, can be simplified:
<details> <summary>git diff on 31cc5006c3</summary>
diff --git a/src/node/chainstate.cpp b/src/node/chainstate.cpp
index fa8d7a386f..4837135dee 100644
--- a/src/node/chainstate.cpp
+++ b/src/node/chainstate.cpp
@@ -240,11 +240,7 @@ ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSize
// A reload of the block index is required to recompute setBlockIndexCandidates
// for the fully validated chainstate.
chainman.ActiveChainstate().ClearBlockIndexCandidates();
-
- auto [init_status, init_error] = CompleteChainstateInitialization(chainman, cache_sizes, options);
- if (init_status != ChainstateLoadStatus::SUCCESS) {
- return {init_status, init_error};
- }
+ return CompleteChainstateInitialization(chainman, cache_sizes, options);
} else {
return {ChainstateLoadStatus::FAILURE_FATAL, _(
"UTXO snapshot failed to validate. "
</details>
Hmm, then again, that breaks the pattern here of only (explicitly) returning early for failure, so could also leave as-is.
I think your suggestion is a bit better. I'll push it if something else comes up. Looks to me like that code iterated over the chainstates in a previous un-pushed version, that would explain why only the errors are surfaced here. There is also ryanofsky's assumeutxo state cleanup PR that could catch this later if we don't get it here
ACK 31cc5006c3de4dd6a1f7a238684163956604df45
ACK 31cc5006c3de4dd6a1f7a238684163956604df45