init: Remove retry for loop

[sedited]

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
  • https://github.com/bitcoin/bitcoin/pull/30132#issuecomment-2120741121
• 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:
  • https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1601589963

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.

[DrahtBot]

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.

[sedited]

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.

[kevkevinpal]

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

[maflcko]

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

review ACK e9d60af9889c12b4d427adefa53fd234e417f8f6 🚸

Show signature

Signature:

untrusted 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}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK e9d60af9889c12b4d427adefa53fd234e417f8f6 🚸
iATg595aO+/TUwqgVqt7+bt2dNjd2Ai91/AKhFJ+R83tf9xxI+uDnPy6qo038kgbtoy4KXt7egIDP7XzmzLcDQ==

[josibake]

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

Came here from reviewing https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1776649353, haven't tested but did review and the code change is relatively straightforward (mostly a move-only change) and definitely improves the readability here.

[maflcko]

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.

[ryanofsky]

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]

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]

ACK e9d60af9889c12b4d427adefa53fd234e417f8f6

[sedited]

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