init: Remove retry for loop

TheCharlatan commented at 1:16 pm on September 25, 2024: contributor

The for loop around the chain loading logic in init.cpp allows users of the GUI to retry once on failure with reindexing without having to manually set the reindex flag on startup. However this current mechanism has problems:

It is badly documented and has led to confusion among developers and bugs making it into master. Examples:
- https://github.com/bitcoin/bitcoin/pull/28830/files#r1598392660
- #30132 (comment)
It can only ever iterate once, making the choice of a for loop questionable.
With its large scope it is easy for re-entry bugs to sneak in. Example:
- #28830 (review)

Attempt to fix this by moving the bulk of the logic into a separate function and replacing the for loop with a simpler if statement.

The diff’s in this pull request are best reviewed with --color-moved-ws=ignore-all-space --color-moved=dimmed-zebra. The error behaviour can be tested by either manually making LoadChainstate return a failure, or deleting some of the block index database files.

init: Check mempool arguments in AppInitParameterInteractions

This makes the handling more consistent and reports errors sooner.

781c01f580

DrahtBot commented at 1:16 pm on September 25, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	ryanofsky, maflcko, josibake, achow101
Concept ACK	theuni, ismaelsadeeq, kevkevinpal, stickies-v, mzumsande

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#30967 (refactor: Replace g_genesis_wait_cv with m_tip_block_cv by maflcko)
#30965 (kernel: Move block tree db open to block manager by TheCharlatan)
#29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)

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.

ryanofsky approved

ryanofsky commented at 3:42 pm on September 25, 2024: contributor

Code review ACK 705105de4f84f2ce3bb1d754c88c32349e01bb3b

Nice cleanup. Previous for loop was very confusing and new code is straightforward.

Not important, but I think it would be a little better if InitAndLoadChainstate simply returned ChainstateLoadStatus instead of std::tuple</*may_retry*/ bool, /*error*/ bilingual_str>. This way retry logic would be implemented in one function, not split across two functions. And it would avoid introducing a new way to represent success/interrupted/failure status, and just reuse the status type we already have:

  0--- a/src/init.cpp
  1+++ b/src/init.cpp
  2@@ -123,6 +123,7 @@ using node::ApplyArgsManOptions;
  3 using node::BlockManager;
  4 using node::CacheSizes;
  5 using node::CalculateCacheSizes;
  6+using node::ChainstateLoadStatus;
  7 using node::DEFAULT_PERSIST_MEMPOOL;
  8 using node::DEFAULT_PRINT_MODIFIED_FEE;
  9 using node::DEFAULT_STOPATHEIGHT;
 10@@ -1185,23 +1186,24 @@ bool CheckHostPortOptions(const ArgsManager& args) {
 11 
 12 // A GUI user may opt to retry once if there is a failure during chainstate initialization.
 13 // The function therefore has to support re-entry.
 14-static std::tuple</*may_retry*/ bool, /*error*/ bilingual_str> InitAndLoadChainstate(
 15+static ChainstateLoadStatus InitAndLoadChainstate(
 16     NodeContext& node,
 17     bool do_reindex,
 18     const bool do_reindex_chainstate,
 19     CacheSizes& cache_sizes,
 20-    const ArgsManager& args)
 21+    const ArgsManager& args,
 22+    bilingual_str& error)
 23 {
 24+    error.clear();
 25     const CChainParams& chainparams = Params();
 26     CTxMemPool::Options mempool_opts{
 27         .check_ratio = chainparams.DefaultConsistencyChecks() ? 1 : 0,
 28         .signals = node.validation_signals.get(),
 29     };
 30     Assert(ApplyArgsManOptions(args, chainparams, mempool_opts)); // no error can happen, already checked in AppInitParameterInteraction
 31-    bilingual_str mempool_error;
 32-    node.mempool = std::make_unique<CTxMemPool>(mempool_opts, mempool_error);
 33-    if (!mempool_error.empty()) {
 34-        return {/*may_retry*/ false, /*error*/ mempool_error};
 35+    node.mempool = std::make_unique<CTxMemPool>(mempool_opts, error);
 36+    if (!error.empty()) {
 37+        return ChainstateLoadStatus::FAILURE_FATAL;
 38     }
 39     LogPrintf("* Using %.1f MiB for in-memory UTXO set (plus up to %.1f MiB of unused mempool space)\n", cache_sizes.coins * (1.0 / 1024 / 1024), mempool_opts.max_size_bytes * (1.0 / 1024 / 1024));
 40     ChainstateManager::Options chainman_opts{
 41@@ -1220,7 +1222,8 @@ static std::tuple</*may_retry*/ bool, /*error*/ bilingual_str> InitAndLoadChains
 42     try {
 43         node.chainman = std::make_unique<ChainstateManager>(*Assert(node.shutdown), chainman_opts, blockman_opts);
 44     } catch (std::exception& e) {
 45-        return {/*may_retry*/ false, /*error*/ strprintf(Untranslated("Failed to initialize ChainstateManager: %s"), e.what())};
 46+        error = Untranslated(strprintf("Failed to initialize ChainstateManager: %s", e.what()));
 47+        return ChainstateLoadStatus::FAILURE_FATAL;
 48     }
 49     ChainstateManager& chainman = *node.chainman;
 50     // This is defined and set here instead of inline in validation.h to avoid a hard
 51@@ -1266,7 +1269,8 @@ static std::tuple</*may_retry*/ bool, /*error*/ bilingual_str> InitAndLoadChains
 52             return std::make_tuple(node::ChainstateLoadStatus::FAILURE, _("Error opening block database"));
 53         }
 54     };
 55-    auto [status, error] = catch_exceptions([&] { return LoadChainstate(chainman, cache_sizes, options); });
 56+    ChainstateLoadStatus status;
 57+    std::tie(status, error) = catch_exceptions([&] { return LoadChainstate(chainman, cache_sizes, options); });
 58     if (status == node::ChainstateLoadStatus::SUCCESS) {
 59         uiInterface.InitMessage(_("Verifying blocks…").translated);
 60         if (chainman.m_blockman.m_have_pruned && options.check_blocks > MIN_BLOCKS_TO_KEEP) {
 61@@ -1276,13 +1280,9 @@ static std::tuple</*may_retry*/ bool, /*error*/ bilingual_str> InitAndLoadChains
 62         std::tie(status, error) = catch_exceptions([&] { return VerifyLoadedChainstate(chainman, options); });
 63         if (status == node::ChainstateLoadStatus::SUCCESS) {
 64             LogPrintf(" block index %15dms\n", Ticks<std::chrono::milliseconds>(SteadyClock::now() - load_block_index_start_time));
 65-            return {/*may_retry*/ false, /*error*/ bilingual_str{}};
 66         }
 67     }
 68-    if (status == node::ChainstateLoadStatus::FAILURE_FATAL || status == node::ChainstateLoadStatus::FAILURE_INCOMPATIBLE_DB || status == node::ChainstateLoadStatus::FAILURE_INSUFFICIENT_DBCACHE) {
 69-        return {/*may_retry*/ false, /*error*/ error};
 70-    }
 71-    return {/*may_retry*/ true, /*error*/ error};
 72+    return status;
 73 };
 74 
 75 bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
 76@@ -1638,13 +1638,9 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
 77     const bool do_reindex_chainstate{args.GetBoolArg("-reindex-chainstate", false)};
 78 
 79     // Chainstate initialization and loading may be retried once with reindexing by GUI users
 80-    auto [maybe_retry, error] = InitAndLoadChainstate(
 81-        node,
 82-        do_reindex,
 83-        do_reindex_chainstate,
 84-        cache_sizes,
 85-        args);
 86-    if (maybe_retry && !do_reindex && !ShutdownRequested(node)) {
 87+    bilingual_str error;
 88+    ChainstateLoadStatus status = InitAndLoadChainstate(node, do_reindex, do_reindex_chainstate, cache_sizes, args, error);
 89+    if (status == ChainstateLoadStatus::FAILURE && !do_reindex && !ShutdownRequested(node)) {
 90         // suggest a reindex
 91         bool do_retry = uiInterface.ThreadSafeQuestion(
 92             error + Untranslated(".\n\n") + _("Do you want to rebuild the block database now?"),
 93@@ -1658,14 +1654,9 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
 94         if (!Assert(node.shutdown)->reset()) {
 95             LogError("Internal error: failed to reset shutdown signal.\n");
 96         }
 97-        std::tie(std::ignore, error) = InitAndLoadChainstate(
 98-            node,
 99-            do_reindex,
100-            do_reindex_chainstate,
101-            cache_sizes,
102-            args);
103+        status = InitAndLoadChainstate(node, do_reindex, do_reindex_chainstate, cache_sizes, args, error);
104     }
105-    if (!error.empty()) {
106+    if (status != ChainstateLoadStatus::SUCCESS && status != ChainstateLoadStatus::INTERRUPTED) {
107         return InitError(error);
108     }
109

theuni commented at 4:34 pm on September 25, 2024: member

Concept ACK. I’ve also spent a good amount of time (more than once) scratching my head trying to understand this loop.

refactor: Move most of init retry for loop to a function

This makes it clearer which state is being mutated by the function and
facilitates getting rid of the for loop in the following commit. Move
creation of the required options into the function too, such that the
function takes fewer arguments and is more self-contained.

Co-Authored-By: Ryan Ofsky <ryan@ofsky.org>

c1d8870ea4

refactor: Replace init retry for loop with if statement

The for loop has been a long standing source of confusion and bugs, both
because its purpose is not clearly documented and because the body of
the for loop contains a lot of logic.

Co-Authored-By: Ryan Ofsky <ryan@ofsky.org>

e9d60af988

TheCharlatan force-pushed on Sep 25, 2024

TheCharlatan commented at 5:43 pm on September 25, 2024: contributor

Thank you for your suggestion @ryanofsky,

Updated 705105de4f84f2ce3bb1d754c88c32349e01bb3b -> e9d60af9889c12b4d427adefa53fd234e417f8f6 (removeInitForLoop_0 -> removeInitForLoop_1, compare)

Took @ryanofsky’s suggestion, making the new InitLoadChainstate function return a ChainstateLoadResult and using it instead of a retry and error tuple.

ismaelsadeeq commented at 6:02 pm on September 25, 2024: member

Nice Concept ACK

in src/init.cpp:1644 in c1d8870ea4 outdated

1720+        auto [status, error] = InitAndLoadChainstate(
1721+            node,
1722+            do_reindex,
1723+            do_reindex_chainstate,
1724+            cache_sizes,
1725+            args);

ryanofsky commented at 7:35 pm on September 25, 2024:

Not important, for but both of the InitAndLoadChainstate calls in this PR, I’m surprised someone would prefer newline separated arguments over space separated:

0InitAndLoadChainstate(node, do_reindex, do_reindex_chainstate, cache_sizes, args);

clang-format has a nice “BinPack” and “Align” options for packing arguments neatly without having to take up large amounts of space

TheCharlatan commented at 7:53 pm on September 25, 2024:

Mmh, I think this must have been something I did out of old habit. It would be good to have these stricter clang format options documented in the dev notes so we can at least all try to write similarly formatted new code.

maflcko commented at 8:50 am on September 26, 2024:

clang-format has a nice “BinPack” and “Align” options

Last time I checked they wouldn’t take any effect with ColumnLimit: 0, but maybe I am wrong, or this may have changed. However, once there is a line break, I think using a line break for all is cleaner (cargo fmt agrees with me and consistently either uses a line-separator or space-separator, but never mixes them [1]). It would be nice if there was a clang-format config that did what cargo fmt does by default.

[1] https://github.com/bitcoin/bitcoin/blob/65f6e7078b17e6e73d74dfeb00159878099fee1e/test/lint/test_runner/src/main.rs#L350 and https://github.com/bitcoin/bitcoin/blob/65f6e7078b17e6e73d74dfeb00159878099fee1e/test/lint/test_runner/src/main.rs#L272-L278

ryanofsky approved

ryanofsky commented at 7:40 pm on September 25, 2024: contributor

Code review ACK e9d60af9889c12b4d427adefa53fd234e417f8f6. Nice change to make AppInitMain shorter and more understandable.

DrahtBot requested review from ismaelsadeeq on Sep 25, 2024

DrahtBot requested review from theuni on Sep 25, 2024

kevkevinpal commented at 10:10 pm on September 25, 2024: contributor

Concept ACK the for loop is confusing and refactoring it to something better is a welcomed change

maflcko commented at 8:51 am on September 26, 2024: member

Reviewed, but did not test the retry flow in the GUI

review ACK e9d60af9889c12b4d427adefa53fd234e417f8f6 🚸

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: review ACK e9d60af9889c12b4d427adefa53fd234e417f8f6 🚸
3iATg595aO+/TUwqgVqt7+bt2dNjd2Ai91/AKhFJ+R83tf9xxI+uDnPy6qo038kgbtoy4KXt7egIDP7XzmzLcDQ==

josibake commented at 5:12 pm on September 26, 2024: member

crACK https://github.com/bitcoin/bitcoin/commit/e9d60af9889c12b4d427adefa53fd234e417f8f6

Came here from reviewing #30965 (review), haven’t tested but did review and the code change is relatively straightforward (mostly a move-only change) and definitely improves the readability here.

in src/init.cpp:1670 in e9d60af988

1773+    }
1774+    if (status != ChainstateLoadStatus::SUCCESS && status != ChainstateLoadStatus::INTERRUPTED) {
1775+        return InitError(error);
1776     }
1777 
1778     // As LoadBlockIndex can take several minutes, it's possible the user

maflcko commented at 7:03 pm on September 26, 2024:

Unrelated to this pull, the comment is wrong. Shutdown() is called. So it would be good (in a follow-up), or if you re-touch, to remove the incorrect and confusing comment:

 0diff --git a/src/init.cpp b/src/init.cpp
 1index ebe48dc307..5f786fd4df 100644
 2--- a/src/init.cpp
 3+++ b/src/init.cpp
 4@@ -1649,7 +1649,6 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
 5 
 6     // As LoadBlockIndex can take several minutes, it's possible the user
 7     // requested to kill the GUI during the last operation. If so, exit.
 8-    // As the program has not fully started yet, Shutdown() is possibly overkill.
 9     if (ShutdownRequested(node)) {
10         LogPrintf("Shutdown requested. Exiting.\n");
11         return false;

maflcko commented at 7:43 am on September 27, 2024: member

The error behaviour can be tested by either manually making LoadChainstate return a failure, or deleting some of the block index database files.

Upgrading my review: I tested using the instructions.

in src/init.cpp:1079 in e9d60af988

1073@@ -1072,6 +1074,13 @@ bool AppInitParameterInteraction(const ArgsManager& args)
1074         if (!blockman_result) {
1075             return InitError(util::ErrorString(blockman_result));
1076         }
1077+        CTxMemPool::Options mempool_opts{
1078+            .check_ratio = chainparams.DefaultConsistencyChecks() ? 1 : 0,
1079+        };

stickies-v commented at 4:09 pm on September 27, 2024:

Is there a reason to set check_ratio? It doesn’t seem necessary?

0        CTxMemPool::Options mempool_opts{};

stickies-v commented at 4:32 pm on September 27, 2024: contributor

Strong concept ACK!

in src/init.cpp:1188 in c1d8870ea4 outdated

1184@@ -1183,6 +1185,104 @@ bool CheckHostPortOptions(const ArgsManager& args) {
1185     return true;
1186 }
1187 
1188+// A GUI user may opt to retry once if there is a failure during chainstate initialization.

mzumsande commented at 9:59 pm on September 27, 2024:

nit: might use “retry with do_reindex set”. Otherwise it might sounds a bit like “try turning it off and on again”.

in src/init.cpp:1652 in e9d60af988

1649+        bool do_retry = uiInterface.ThreadSafeQuestion(
1650+            error + Untranslated(".\n\n") + _("Do you want to rebuild the block database now?"),
1651+            error.original + ".\nPlease restart with -reindex or -reindex-chainstate to recover.",
1652+            "", CClientUIInterface::MSG_ERROR | CClientUIInterface::BTN_ABORT);
1653+        if (!do_retry) {
1654+            LogError("Aborted block database rebuild. Exiting.\n");

mzumsande commented at 10:16 pm on September 27, 2024:

This log error is also shown in the non-GUI case, where it doesn’t make any sense because the user didn’t get the option to rebuild the block database. Also, to be pedantic, it’s misleading in the GUI because we didn’t abort a rebuild, but aborted instead of trying it. Maybe it’s not necessary at all?

mzumsande commented at 10:20 pm on September 27, 2024: contributor

Concept ACK

I think that one minor change in behavior is that with the old code we could retry many times if, for some reason, we’d repeatedly get ChainstateLoadStatus::FAILURE messages (which shouldn’t happen normally, but might in some exotic failure scenarios?). But even if it was possible to get these messages repeatedly, it doesn’t really make sense to retry more than once.

ryanofsky commented at 10:59 pm on September 27, 2024: contributor

I think that one minor change in behavior is that with the old code we could retry many times if, for some reason, we’d repeatedly get ChainstateLoadStatus::FAILURE messages (which shouldn’t happen normally, but might in some exotic failure scenarios?)

I think it wouldn’t retry multiple times because the old code sets do_reindex = true

https://github.com/bitcoin/bitcoin/blob/d812cf11896a2214467b6fa72d7b763bac6077c5/src/init.cpp#L1643

At least that was my interpretation reviewing this earlier (could be mistaken)

mzumsande commented at 0:59 am on September 28, 2024: contributor

At least that was my interpretation reviewing this earlier (could be mistaken)

Yes, you are right. I thought that maybe there could be some scenarios where even with do_reindex == true we could maybe get ChainstateLoadStatus::FAILURE again, but after looking more closely I don’t see how this could be the case.

achow101 commented at 8:50 pm on September 30, 2024: member

ACK e9d60af9889c12b4d427adefa53fd234e417f8f6

DrahtBot requested review from stickies-v on Sep 30, 2024

DrahtBot requested review from mzumsande on Sep 30, 2024

achow101 merged this on Sep 30, 2024

achow101 closed this on Sep 30, 2024

TheCharlatan commented at 10:18 am on October 7, 2024: contributor

Going to address the left-over comments in a follow-up.

init: Remove retry for loop #30968

Code Coverage

Reviews

Conflicts