init: [gui] Avoid UB/crash in InitAndLoadChainstate #32987

pull maflcko wants to merge 2 commits into bitcoin:master from maflcko:2507-less-ub changing 4 files +52 −6
  1. maflcko commented at 5:18 am on July 16, 2025: member

    InitAndLoadChainstate is problematic, when called twice in the GUI. This can happen when it returns a failure and the user selects an interactive reindex.

    There are several bugs that have been introduced since the last time this was working correctly:

    • The first one is a crash (assertion failure), which happens due to a cached tip block in the notifiications from the previous run. See #31346 (review)
    • The second one is UB (use-after-free), which happens because the block index db in the blockmanager is not reset. See #30965 (review)

    Fix both bugs by resetting any dirty state in InitAndLoadChainstate.

    Also, add a test, because I don’t really want to keep testing this manually every time. (A failing test run can be seen in https://github.com/bitcoin/bitcoin/pull/32979/checks)

  2. init: [gui] Avoid UB/crash in InitAndLoadChainstate faaaddaaf8
  3. DrahtBot commented at 5:18 am on July 16, 2025: contributor

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

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32987.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK TheCharlatan, achow101, mzumsande
    Concept ACK hebasto

    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:

    • #26022 (Add util::ResultPtr class by ryanofsky)
    • #25665 (refactor: Add util::Result failure values, multiple error and warning messages by ryanofsky)

    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. maflcko force-pushed on Jul 16, 2025
  5. DrahtBot added the label CI failed on Jul 16, 2025
  6. DrahtBot commented at 5:22 am on July 16, 2025: contributor

    🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/runs/46066804349 LLM reason (✨ experimental): The CI failure is caused by a lint error: an unused import of ‘os’ in a Python test script.

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  7. maflcko force-pushed on Jul 16, 2025
  8. DrahtBot removed the label CI failed on Jul 16, 2025
  9. TheCharlatan approved
  10. TheCharlatan commented at 2:24 pm on July 16, 2025: contributor
    Thanks for fixing, should have thought of this when I started making the destructors more powerful! ACK on the fixes, the functional test is a bit hacky, but I don’t have a better idea either. Maybe we could hook into the ui interface instead, but that’s not necessarily cleaner and we only have one caller of ThreadSafeQuestion anyway.
  11. hebasto commented at 4:19 pm on July 17, 2025: member
    Concept ACK.
  12. fanquake commented at 4:41 pm on July 17, 2025: member
    Is there any way to write an actual GUI test for this? Having to add an obscure, single-use option to bitcoind, to simulate crashing behaviour in the GUI, seems like a last resort?
  13. mzumsande commented at 4:55 pm on July 17, 2025: contributor

    I wonder if this GUI interactive reindex feature is worth all the troubles - seems like it got broken multiple times in the past without users really noticing / complaining?! (Just from memory, and I didn’t check how many releases were affected)

    Given that the need for a reindex should be somewhat rare anyway, would it really be much of a degradation of user experience to remove the feature and have GUI users restart with -reindex non-interactively like they would have to with bitcoind?

  14. hebasto commented at 4:58 pm on July 17, 2025: member

    Given that the need for a reindex should be somewhat rare anyway, would it really be much of a degradation of user experience to remove the feature and have GUI users restart with -reindex non-interactively like they would have to with bitcoind?

    I believe it would.

  15. in src/init.cpp:510 in fae8ac91d5 outdated
    506@@ -507,6 +507,9 @@ void SetupServerArgs(ArgsManager& argsman, bool can_listen_ipc)
    507             "(default: 0 = disable pruning blocks, 1 = allow manual pruning via RPC, >=%u = automatically prune block files to stay under the specified target size in MiB)", MIN_DISK_SPACE_FOR_BLOCK_FILES / 1024 / 1024), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
    508     argsman.AddArg("-reindex", "If enabled, wipe chain state and block index, and rebuild them from blk*.dat files on disk. Also wipe and rebuild other optional indexes that are active. If an assumeutxo snapshot was loaded, its chainstate will be wiped as well. The snapshot can then be reloaded via RPC.", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
    509     argsman.AddArg("-reindex-chainstate", "If enabled, wipe chain state, and rebuild it from blk*.dat files on disk. If an assumeutxo snapshot was loaded, its chainstate will be wiped as well. The snapshot can then be reloaded via RPC.", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
    510+    argsman.AddArg("-reindex_after_failure_noninteractive_yes",
    


    mzumsande commented at 5:07 pm on July 17, 2025:
    could use -test=<option> instead.
  16. test: Check that the GUI interactive reindex works fac90e5261
  17. maflcko force-pushed on Jul 17, 2025
  18. maflcko commented at 5:16 am on July 18, 2025: member

    seems like it got broken multiple times in the past without users really noticing / complaining?

    They’d still see the error message about the -reindex, before the crash. It seems easy to dismiss a crash as hardware fault and not software fault after you’ve seen a corruption warning.

    Is there any way to write an actual GUI test for this? Having to add an obscure, single-use option to bitcoind, to simulate crashing behaviour in the GUI, seems like a last resort?

    Switched to the test option, which is now an overhead of two lines of “real” code. A gui unit test may theoretically be possible, but it has the downside that it won’t be covered if the gui is disabled in the build, so a dev hitting an error here may have a harder time to reproduce?

  19. TheCharlatan approved
  20. TheCharlatan commented at 9:32 am on July 19, 2025: contributor
    ACK fac90e5261b811739ada56e06ea793a12f9c2c3d
  21. DrahtBot requested review from hebasto on Jul 19, 2025
  22. achow101 commented at 11:01 pm on July 22, 2025: member
    ACK fac90e5261b811739ada56e06ea793a12f9c2c3d
  23. luke-jr commented at 6:37 pm on July 29, 2025: member
    Alternative to the test option, Core could just add support for reindex=auto (#22072 / #26674)
  24. luke-jr referenced this in commit ebcf68681b on Jul 29, 2025
  25. luke-jr referenced this in commit 8037e5af17 on Jul 29, 2025
  26. mzumsande commented at 10:27 pm on July 29, 2025: contributor

    Tested ACK fac90e5261b811739ada56e06ea793a12f9c2c3d

    Unrelated, but just mentioning that reindex with the GUI is not great: I tried it with a regtest chain of 1000 empty blocks and the app froze for a few seconds / my OS asked me whether it should terminate the non-responding program. Didn’t try it with mainnet, but it’s probably not a smooth experience.

  27. achow101 merged this on Jul 30, 2025
  28. achow101 closed this on Jul 30, 2025

  29. maflcko deleted the branch on Jul 31, 2025

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: 2025-08-13 09:12 UTC

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