init: Remove retry for loop #30968

pull TheCharlatan wants to merge 3 commits into bitcoin:master from TheCharlatan:removeInitForLoop changing 1 files +137 −124
  1. 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:

    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.

  2. init: Check mempool arguments in AppInitParameterInteractions
    This makes the handling more consistent and reports errors sooner.
    781c01f580
  3. 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.

    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.

  4. ryanofsky approved
  5. 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 
    
  6. 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.
  7. 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
  8. 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
  9. TheCharlatan force-pushed on Sep 25, 2024
  10. 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.
  11. ismaelsadeeq commented at 6:02 pm on September 25, 2024: member
    Nice Concept ACK
  12. 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

  13. ryanofsky approved
  14. ryanofsky commented at 7:40 pm on September 25, 2024: contributor
    Code review ACK e9d60af9889c12b4d427adefa53fd234e417f8f6. Nice change to make AppInitMain shorter and more understandable.
  15. DrahtBot requested review from ismaelsadeeq on Sep 25, 2024
  16. DrahtBot requested review from theuni on Sep 25, 2024
  17. 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
  18. 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==
    
  19. 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.

  20. 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;
    
  21. 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.

  22. 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{};
    
  23. stickies-v commented at 4:32 pm on September 27, 2024: contributor
    Strong concept ACK!
  24. 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”.
  25. 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?
  26. 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.

  27. 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)

  28. 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.

  29. achow101 commented at 8:50 pm on September 30, 2024: member
    ACK e9d60af9889c12b4d427adefa53fd234e417f8f6
  30. DrahtBot requested review from stickies-v on Sep 30, 2024
  31. DrahtBot requested review from mzumsande on Sep 30, 2024
  32. achow101 merged this on Sep 30, 2024
  33. achow101 closed this on Sep 30, 2024

  34. TheCharlatan commented at 10:18 am on October 7, 2024: contributor
    Going to address the left-over comments in a follow-up.

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-08 16:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me