HomePhabricator

qt/test: Reset chainman in ~ChainstateManager instead

Description

qt/test: Reset chainman in ~ChainstateManager instead

Summary:
There are some mutable, global state variables that are currently reseti by UnloadBlockIndex such as pindexBestHeader which should be cleaned up whenever the ChainstateManager is unloaded/reset/destructed/etc.

Not cleaning them up leads to bugs like a use-after-free that happens like so:

  1. At the end of a test, ChainstateManager is destructed, which also destructs BlockManager, which calls BlockManager::Unload to free all CBlockIndexes in its BlockMap
  2. Since pindexBestHeader is not cleaned up, it now points to an invalid0 location
  3. Another test starts to init, and calls LoadGenesisBlock, which calls AddToBlockIndex, which compares the genesis block with an invalid location
  4. Cute puppies perish by the hundreds

Previously, for normal codepaths (e.g. bitcoind), we relied on the fact that our program will be unloaded by the operating system which effectively resets these variables. The one exception is in QT tests, where these variables had to be manually reset.

Since now ChainstateManager is no longer a global, we can just put this logic in its destructor to make sure that callers are always correct.

Over time, we should probably move these mutable global state variables into ChainstateManager or CChainState so it's easier to reason about their lifecycles.

This is a backport of core#21866 [11/12]
https://github.com/bitcoin/bitcoin/pull/21866/commits/972c5166ee685447a6d4bf5e501b07a0871fba85

Depends on D11751 and D11744

Backport note: it is not possible to drop the chainman reset code in apptests.cpp at this point, as the unit tests still use g_chainman. This has to be removed in the next commit in D11753, when we actually make the unit tests use their own chain state manager instead of the global one.

Test Plan: ninja all check-all

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D11752

Details

Provenance
Carl Dong <contact@carldong.me>Authored on Oct 6 2020, 21:35
PiRKCommitted on Jul 15 2022, 10:45
PiRKPushed on Jul 15 2022, 10:45
Reviewer
Restricted Project
Differential Revision
D11752: qt/test: Reset chainman in ~ChainstateManager instead
Parents
rABC9d99cccc3809: make FindBlockToFinalize a CChainState member
Branches
Unknown
Tags
Unknown