From f323248aba5088c9630e5cdfe5ce980f21633fe8 Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Fri, 18 Sep 2020 13:35:27 -0400 Subject: qt/test: Use existing chainman in ::TestGUI (can be scripted-diff) --- src/qt/test/wallettests.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'src/qt') diff --git a/src/qt/test/wallettests.cpp b/src/qt/test/wallettests.cpp index ea35f80cf5..3e1a0e0fa9 100644 --- a/src/qt/test/wallettests.cpp +++ b/src/qt/test/wallettests.cpp @@ -146,14 +146,14 @@ void TestGUI(interfaces::Node& node) LOCK2(wallet->cs_wallet, spk_man->cs_KeyStore); wallet->SetAddressBook(GetDestinationForKey(test.coinbaseKey.GetPubKey(), wallet->m_default_address_type), "", "receive"); spk_man->AddKeyPubKey(test.coinbaseKey, test.coinbaseKey.GetPubKey()); - wallet->SetLastBlockProcessed(105, ::ChainActive().Tip()->GetBlockHash()); + wallet->SetLastBlockProcessed(105, node.context()->chainman->ActiveChain().Tip()->GetBlockHash()); } { WalletRescanReserver reserver(*wallet); reserver.reserve(); CWallet::ScanResult result = wallet->ScanForWalletTransactions(Params().GetConsensus().hashGenesisBlock, 0 /* block height */, {} /* max height */, reserver, true /* fUpdate */); QCOMPARE(result.status, CWallet::ScanResult::SUCCESS); - QCOMPARE(result.last_scanned_block, ::ChainActive().Tip()->GetBlockHash()); + QCOMPARE(result.last_scanned_block, node.context()->chainman->ActiveChain().Tip()->GetBlockHash()); QVERIFY(result.last_failed_block.IsNull()); } wallet->SetBroadcastTransactions(true); -- cgit v1.2.3 From 972c5166ee685447a6d4bf5e501b07a0871fba85 Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Tue, 6 Oct 2020 17:35:53 -0400 Subject: qt/test: Reset chainman in ~ChainstateManager instead There are some mutable, global state variables that are currently reset 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 invalid 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. --- src/qt/test/apptests.cpp | 5 ----- 1 file changed, 5 deletions(-) (limited to 'src/qt') diff --git a/src/qt/test/apptests.cpp b/src/qt/test/apptests.cpp index cb3dbd2267..9c31cd50df 100644 --- a/src/qt/test/apptests.cpp +++ b/src/qt/test/apptests.cpp @@ -85,11 +85,6 @@ void AppTests::appTests() // Reset global state to avoid interfering with later tests. LogInstance().DisconnectTestLogger(); AbortShutdown(); - { - LOCK(cs_main); - UnloadBlockIndex(/* mempool */ nullptr, g_chainman); - g_chainman.Reset(); - } } //! Entry point for BitcoinGUI tests. -- cgit v1.2.3