Skip to content

Commit d167cc6

Browse files
fanquakevijaydasmp
fanquake
authored and
vijaydasmp
committed
Merge bitcoin#25077: Fix chain tip data race and corrupt rest response
fac04cb refactor: Add lock annotations to Active* methods (MacroFake) fac15ff Fix logical race in rest_getutxos (MacroFake) fa97a52 Fix UB/data-race in RPCNotifyBlockChange (MacroFake) fa530bc Add ChainstateManager::GetMutex(), an alias for ::cs_main (MacroFake) Pull request description: This fixes two issues: * A data race in `ActiveChain`, which returns a reference to the chain (a `std::vector`), which is not thread safe. See also below traceback. * A corrupt rest response, which returns a blockheight and blockhash, which are unrelated to each other and to the result, as the chain might advance between each call without cs_main held. The issues are fixed by taking cs_main and holding it for the required time. ``` ================== WARNING: ThreadSanitizer: data race (pid=32335) Write of size 8 at 0x7b3c000008f0 by thread T22 (mutexes: write M131626, write M151, write M131553): #0 std::__1::enable_if<(is_move_constructible<CBlockIndex**>::value) && (is_move_assignable<CBlockIndex**>::value), void>::type std::__1::swap<CBlockIndex**>(CBlockIndex**&, CBlockIndex**&) /usr/lib/llvm-13/bin/../include/c++/v1/__utility/swap.h:39:7 (bitcoind+0x501239) #1 std::__1::vector<CBlockIndex*, std::__1::allocator<CBlockIndex*> >::__swap_out_circular_buffer(std::__1::__split_buffer<CBlockIndex*, std::__1::allocator<CBlockIndex*>&>&) /usr/lib/llvm-13/bin/../include/c++/v1/vector:977:5 (bitcoind+0x501239) #2 std::__1::vector<CBlockIndex*, std::__1::allocator<CBlockIndex*> >::__append(unsigned long) /usr/lib/llvm-13/bin/../include/c++/v1/vector:1117:9 (bitcoind+0x501239) #3 std::__1::vector<CBlockIndex*, std::__1::allocator<CBlockIndex*> >::resize(unsigned long) /usr/lib/llvm-13/bin/../include/c++/v1/vector:2046:15 (bitcoind+0x4ffe29) #4 CChain::SetTip(CBlockIndex*) src/chain.cpp:19:12 (bitcoind+0x4ffe29) #5 CChainState::ConnectTip(BlockValidationState&, CBlockIndex*, std::__1::shared_ptr<CBlock const> const&, ConnectTrace&, DisconnectedBlockTransactions&) src/validation.cpp:2748:13 (bitcoind+0x475d00) #6 CChainState::ActivateBestChainStep(BlockValidationState&, CBlockIndex*, std::__1::shared_ptr<CBlock const> const&, bool&, ConnectTrace&) src/validation.cpp:2884:18 (bitcoind+0x47739e) #7 CChainState::ActivateBestChain(BlockValidationState&, std::__1::shared_ptr<CBlock const>) src/validation.cpp:3011:22 (bitcoind+0x477baf) #8 node::ThreadImport(ChainstateManager&, std::__1::vector<fs::path, std::__1::allocator<fs::path> >, ArgsManager const&) src/node/blockstorage.cpp:883:30 (bitcoind+0x23cd74) #9 AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7::operator()() const src/init.cpp:1657:9 (bitcoind+0x15863e) #10 decltype(static_cast<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&>(fp)()) std::__1::__invoke<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&>(AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&) /usr/lib/llvm-13/bin/../include/c++/v1/type_traits:3918:1 (bitcoind+0x15863e) #11 void std::__1::__invoke_void_return_wrapper<void, true>::__call<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&>(AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&) /usr/lib/llvm-13/bin/../include/c++/v1/__functional/invoke.h:61:9 (bitcoind+0x15863e) #12 std::__1::__function::__alloc_func<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7, std::__1::allocator<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7>, void ()>::operator()() /usr/lib/llvm-13/bin/../include/c++/v1/__functional/function.h:171:16 (bitcoind+0x15863e) #13 std::__1::__function::__func<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7, std::__1::allocator<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7>, void ()>::operator()() /usr/lib/llvm-13/bin/../include/c++/v1/__functional/function.h:345:12 (bitcoind+0x15863e) #14 std::__1::__function::__value_func<void ()>::operator()() const /usr/lib/llvm-13/bin/../include/c++/v1/__functional/function.h:498:16 (bitcoind+0x88891f) #15 std::__1::function<void ()>::operator()() const /usr/lib/llvm-13/bin/../include/c++/v1/__functional/function.h:1175:12 (bitcoind+0x88891f) #16 util::TraceThread(char const*, std::__1::function<void ()>) src/util/thread.cpp:18:9 (bitcoind+0x88891f) #17 decltype(static_cast<void (*>(fp)(static_cast<char const*>(fp0), static_cast<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7>(fp0))) std::__1::__invoke<void (*)(char const*, std::__1::function<void ()>), char const*, AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7>(void (*&&)(char const*, std::__1::function<void ()>), char const*&&, AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&&) /usr/lib/llvm-13/bin/../include/c++/v1/type_traits:3918:1 (bitcoind+0x157e6a) #18 void std::__1::__thread_execute<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (*)(char const*, std::__1::function<void ()>), char const*, AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7, 2ul, 3ul>(std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (*)(char const*, std::__1::function<void ()>), char const*, AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7>&, std::__1::__tuple_indices<2ul, 3ul>) /usr/lib/llvm-13/bin/../include/c++/v1/thread:280:5 (bitcoind+0x157e6a) #19 void* std::__1::__thread_proxy<std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (*)(char const*, std::__1::function<void ()>), char const*, AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7> >(void*) /usr/lib/llvm-13/bin/../include/c++/v1/thread:291:5 (bitcoind+0x157e6a) Previous read of size 8 at 0x7b3c000008f0 by main thread: #0 std::__1::vector<CBlockIndex*, std::__1::allocator<CBlockIndex*> >::size() const /usr/lib/llvm-13/bin/../include/c++/v1/vector:680:61 (bitcoind+0x15179d) #1 CChain::Tip() const src/./chain.h:449:23 (bitcoind+0x15179d) #2 ChainstateManager::ActiveTip() const src/./validation.h:927:59 (bitcoind+0x15179d) #3 AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*) src/init.cpp:1841:35 (bitcoind+0x15179d) #4 AppInit(node::NodeContext&, int, char**) src/bitcoind.cpp:231:43 (bitcoind+0x133fd2) #5 main src/bitcoind.cpp:275:13 (bitcoind+0x133fd2) Location is heap block of size 232 at 0x7b3c00000870 allocated by main thread: #0 operator new(unsigned long) <null> (bitcoind+0x132668) #1 ChainstateManager::InitializeChainstate(CTxMemPool*, std::__1::optional<uint256> const&) src/validation.cpp:4851:21 (bitcoind+0x48e26b) #2 node::LoadChainstate(bool, ChainstateManager&, CTxMemPool*, bool, Consensus::Params const&, bool, long, long, long, bool, bool, std::__1::function<bool ()>, std::__1::function<void ()>) src/node/chainstate.cpp:31:14 (bitcoind+0x24de07) #3 AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*) src/init.cpp:1438:32 (bitcoind+0x14e994) #4 AppInit(node::NodeContext&, int, char**) src/bitcoind.cpp:231:43 (bitcoind+0x133fd2) #5 main src/bitcoind.cpp:275:13 (bitcoind+0x133fd2) Mutex M131626 (0x7b3c00000898) created at: #0 pthread_mutex_lock <null> (bitcoind+0xda898) #1 std::__1::mutex::lock() <null> (libc++.so.1+0x49f35) #2 node::ThreadImport(ChainstateManager&, std::__1::vector<fs::path, std::__1::allocator<fs::path> >, ArgsManager const&) src/node/blockstorage.cpp:883:30 (bitcoind+0x23cd74) #3 AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7::operator()() const src/init.cpp:1657:9 (bitcoind+0x15863e) #4 decltype(static_cast<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&>(fp)()) std::__1::__invoke<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&>(AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&) /usr/lib/llvm-13/bin/../include/c++/v1/type_traits:3918:1 (bitcoind+0x15863e) #5 void std::__1::__invoke_void_return_wrapper<void, true>::__call<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&>(AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&) /usr/lib/llvm-13/bin/../include/c++/v1/__functional/invoke.h:61:9 (bitcoind+0x15863e) #6 std::__1::__function::__alloc_func<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7, std::__1::allocator<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7>, void ()>::operator()() /usr/lib/llvm-13/bin/../include/c++/v1/__functional/function.h:171:16 (bitcoind+0x15863e) #7 std::__1::__function::__func<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7, std::__1::allocator<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7>, void ()>::operator()() /usr/lib/llvm-13/bin/../include/c++/v1/__functional/function.h:345:12 (bitcoind+0x15863e) #8 std::__1::__function::__value_func<void ()>::operator()() const /usr/lib/llvm-13/bin/../include/c++/v1/__functional/function.h:498:16 (bitcoind+0x88891f) #9 std::__1::function<void ()>::operator()() const /usr/lib/llvm-13/bin/../include/c++/v1/__functional/function.h:1175:12 (bitcoind+0x88891f) #10 util::TraceThread(char const*, std::__1::function<void ()>) src/util/thread.cpp:18:9 (bitcoind+0x88891f) #11 decltype(static_cast<void (*>(fp)(static_cast<char const*>(fp0), static_cast<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7>(fp0))) std::__1::__invoke<void (*)(char const*, std::__1::function<void ()>), char const*, AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7>(void (*&&)(char const*, std::__1::function<void ()>), char const*&&, AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&&) /usr/lib/llvm-13/bin/../include/c++/v1/type_traits:3918:1 (bitcoind+0x157e6a) #12 void std::__1::__thread_execute<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (*)(char const*, std::__1::function<void ()>), char const*, AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7, 2ul, 3ul>(std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (*)(char const*, std::__1::function<void ()>), char const*, AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7>&, std::__1::__tuple_indices<2ul, 3ul>) /usr/lib/llvm-13/bin/../include/c++/v1/thread:280:5 (bitcoind+0x157e6a) #13 void* std::__1::__thread_proxy<std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (*)(char const*, std::__1::function<void ()>), char const*, AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7> >(void*) /usr/lib/llvm-13/bin/../include/c++/v1/thread:291:5 (bitcoind+0x157e6a) Mutex M151 (0x55aacb8ea030) created at: #0 pthread_mutex_init <null> (bitcoind+0xbed2f) #1 std::__1::recursive_mutex::recursive_mutex() <null> (libc++.so.1+0x49fb3) #2 __libc_start_main <null> (libc.so.6+0x29eba) Mutex M131553 (0x7b4c000042e0) created at: #0 pthread_mutex_init <null> (bitcoind+0xbed2f) #1 std::__1::recursive_mutex::recursive_mutex() <null> (libc++.so.1+0x49fb3) #2 std::__1::__unique_if<CTxMemPool>::__unique_single std::__1::make_unique<CTxMemPool, CBlockPolicyEstimator*, int const&>(CBlockPolicyEstimator*&&, int const&) /usr/lib/llvm-13/bin/../include/c++/v1/__memory/unique_ptr.h:728:32 (bitcoind+0x15c81d) #3 AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*) src/init.cpp:1426:24 (bitcoind+0x14e7b4) #4 AppInit(node::NodeContext&, int, char**) src/bitcoind.cpp:231:43 (bitcoind+0x133fd2) #5 main src/bitcoind.cpp:275:13 (bitcoind+0x133fd2) Thread T22 'b-loadblk' (tid=32370, running) created by main thread at: #0 pthread_create <null> (bitcoind+0xbd5bd) #1 std::__1::__libcpp_thread_create(unsigned long*, void* (*)(void*), void*) /usr/lib/llvm-13/bin/../include/c++/v1/__threading_support:443:10 (bitcoind+0x155e06) #2 std::__1::thread::thread<void (*)(char const*, std::__1::function<void ()>), char const (&) [8], AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7, void>(void (*&&)(char const*, std::__1::function<void ()>), char const (&) [8], AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&&) /usr/lib/llvm-13/bin/../include/c++/v1/thread:307:16 (bitcoind+0x155e06) #3 AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*) src/init.cpp:1656:29 (bitcoind+0x150164) #4 AppInit(node::NodeContext&, int, char**) src/bitcoind.cpp:231:43 (bitcoind+0x133fd2) #5 main src/bitcoind.cpp:275:13 (bitcoind+0x133fd2) SUMMARY: ThreadSanitizer: data race /usr/lib/llvm-13/bin/../include/c++/v1/__utility/swap.h:39:7 in std::__1::enable_if<(is_move_constructible<CBlockIndex**>::value) && (is_move_assignable<CBlockIndex**>::value), void>::type std::__1::swap<CBlockIndex**>(CBlockIndex**&, CBlockIndex**&) ================== ``` From https://cirrus-ci.com/task/5612886578954240?logs=ci#L4868 ACKs for top commit: achow101: re-ACK fac04cb theStack: Code-review ACK fac04cb Tree-SHA512: 9d619f99ff6373874c7ffe1db20674575605646b4b54b692fb54515a4a49f110a770026d7320ed6dfeaa7976be4cd89e93f821acdbf22c7662bd1c5be0cedcd2
1 parent 2e6a225 commit d167cc6

File tree

10 files changed

+49
-32
lines changed

10 files changed

+49
-32
lines changed

‎src/init.cpp‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2375,7 +2375,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
23752375
// Either install a handler to notify us when genesis activates, or set fHaveGenesis directly.
23762376
// No locking, as this happens before any background thread is started.
23772377
boost::signals2::connection block_notify_genesis_wait_connection;
2378-
if (chainman.ActiveChain().Tip() == nullptr) {
2378+
if (WITH_LOCK(chainman.GetMutex(), return chainman.ActiveChain().Tip() == nullptr)) {
23792379
block_notify_genesis_wait_connection = uiInterface.NotifyBlockTip_connect(std::bind(BlockNotifyGenesisWait, std::placeholders::_2));
23802380
} else {
23812381
fHaveGenesis = true;
@@ -2682,12 +2682,12 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
26822682
// At this point, the RPC is "started", but still in warmup, which means it
26832683
// cannot yet be called. Before we make it callable, we need to make sure
26842684
// that the RPC's view of the best block is valid and consistent with
2685-
// ChainstateManager's ActiveTip.
2685+
// ChainstateManager's active tip.
26862686
//
26872687
// If we do not do this, RPC's view of the best block will be height=0 and
26882688
// hash=0x0. This will lead to erroroneous responses for things like
26892689
// waitforblockheight.
2690-
RPCNotifyBlockChange(chainman.ActiveTip());
2690+
RPCNotifyBlockChange(WITH_LOCK(chainman.GetMutex(), return chainman.ActiveTip()));
26912691
SetRPCWarmupFinished();
26922692

26932693
uiInterface.InitMessage(_("Done loading").translated);

‎src/net_processing.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3327,7 +3327,7 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, Peer& peer,
33273327
const std::string msg_type = uses_compressed ? NetMsgType::GETHEADERS2 : NetMsgType::GETHEADERS;
33283328
if (nCount == GetHeadersLimit(pfrom, uses_compressed)) {
33293329
// Headers message had its maximum size; the peer may have more headers.
3330-
if (MaybeSendGetHeaders(pfrom, msg_type, m_chainman.ActiveChain().GetLocator(pindexLast), peer)) {
3330+
if (MaybeSendGetHeaders(pfrom, msg_type, WITH_LOCK(m_chainman.GetMutex(), return m_chainman.ActiveChain().GetLocator(pindexLast)), peer)) {
33313331
LogPrint(BCLog::NET, "more %s (%d) to end to peer=%d (startheight:%d)\n",
33323332
msg_type, pindexLast->nHeight, pfrom.GetId(), peer.m_starting_height);
33333333
}

‎src/qt/test/wallettests.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -138,14 +138,14 @@ void TestGUI(interfaces::Node& node)
138138
if (!wallet->AddWalletDescriptor(w_desc, provider, "", false)) assert(false);
139139
CTxDestination dest = PKHash(test.coinbaseKey.GetPubKey());
140140
wallet->SetAddressBook(dest, "", "receive");
141-
wallet->SetLastBlockProcessed(105, node.context()->chainman->ActiveChain().Tip()->GetBlockHash());
141+
wallet->SetLastBlockProcessed(105, WITH_LOCK(node.context()->chainman->GetMutex(), return node.context()->chainman->ActiveChain().Tip()->GetBlockHash()));
142142
}
143143
{
144144
WalletRescanReserver reserver(*wallet);
145145
reserver.reserve();
146146
CWallet::ScanResult result = wallet->ScanForWalletTransactions(Params().GetConsensus().hashGenesisBlock, /*start_height=*/0, /*max_height=*/{}, reserver, /*fUpdate=*/true, /*save_progress=*/false);
147147
QCOMPARE(result.status, CWallet::ScanResult::SUCCESS);
148-
QCOMPARE(result.last_scanned_block, node.context()->chainman->ActiveChain().Tip()->GetBlockHash());
148+
QCOMPARE(result.last_scanned_block, WITH_LOCK(node.context()->chainman->GetMutex(), return node.context()->chainman->ActiveChain().Tip()->GetBlockHash()));
149149
QVERIFY(result.last_failed_block.IsNull());
150150
}
151151
wallet->SetBroadcastTransactions(true);

‎src/rest.cpp‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -820,14 +820,18 @@ static bool rest_getutxos(const CoreContext& context, HTTPRequest* req, const st
820820
ChainstateManager* maybe_chainman = GetChainman(context, req);
821821
if (!maybe_chainman) return false;
822822
ChainstateManager& chainman = *maybe_chainman;
823+
decltype(chainman.ActiveHeight()) active_height;
824+
uint256 active_hash;
823825
{
824-
auto process_utxos = [&vOutPoints, &outs, &hits](const CCoinsView& view, const CTxMemPool* mempool) {
826+
auto process_utxos = [&vOutPoints, &outs, &hits, &active_height, &active_hash, &chainman](const CCoinsView& view, const CTxMemPool* mempool ) EXCLUSIVE_LOCKS_REQUIRED(chainman.GetMutex()) {
825827
for (const COutPoint& vOutPoint : vOutPoints) {
826828
Coin coin;
827829
bool hit = (!mempool || !mempool->isSpent(vOutPoint)) && view.GetCoin(vOutPoint, coin);
828830
hits.push_back(hit);
829831
if (hit) outs.emplace_back(std::move(coin));
830832
}
833+
active_height = chainman.ActiveHeight();
834+
active_hash = chainman.ActiveTip()->GetBlockHash();
831835
};
832836

833837
if (fCheckMemPool) {
@@ -855,7 +859,7 @@ static bool rest_getutxos(const CoreContext& context, HTTPRequest* req, const st
855859
// serialize data
856860
// use exact same output as mentioned in Bip64
857861
CDataStream ssGetUTXOResponse(SER_NETWORK, PROTOCOL_VERSION);
858-
ssGetUTXOResponse << chainman.ActiveChain().Height() << chainman.ActiveChain().Tip()->GetBlockHash() << bitmap << outs;
862+
ssGetUTXOResponse << active_height << active_hash << bitmap << outs;
859863
std::string ssGetUTXOResponseString = ssGetUTXOResponse.str();
860864

861865
req->WriteHeader("Content-Type", "application/octet-stream");
@@ -865,7 +869,7 @@ static bool rest_getutxos(const CoreContext& context, HTTPRequest* req, const st
865869

866870
case RESTResponseFormat::HEX: {
867871
CDataStream ssGetUTXOResponse(SER_NETWORK, PROTOCOL_VERSION);
868-
ssGetUTXOResponse << chainman.ActiveChain().Height() << chainman.ActiveChain().Tip()->GetBlockHash() << bitmap << outs;
872+
ssGetUTXOResponse << active_height << active_hash << bitmap << outs;
869873
std::string strHex = HexStr(ssGetUTXOResponse) + "\n";
870874

871875
req->WriteHeader("Content-Type", "text/plain");
@@ -878,8 +882,8 @@ static bool rest_getutxos(const CoreContext& context, HTTPRequest* req, const st
878882

879883
// pack in some essentials
880884
// use more or less the same output as mentioned in Bip64
881-
objGetUTXOResponse.pushKV("chainHeight", chainman.ActiveChain().Height());
882-
objGetUTXOResponse.pushKV("chaintipHash", chainman.ActiveChain().Tip()->GetBlockHash().GetHex());
885+
objGetUTXOResponse.pushKV("chainHeight", active_height);
886+
objGetUTXOResponse.pushKV("chaintipHash", active_hash.GetHex());
883887
objGetUTXOResponse.pushKV("bitmap", bitmapStringRepresentation);
884888

885889
UniValue utxos(UniValue::VARR);

‎src/test/interfaces_tests.cpp‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ BOOST_FIXTURE_TEST_SUITE(interfaces_tests, TestChain100Setup)
1717

1818
BOOST_AUTO_TEST_CASE(findBlock)
1919
{
20+
LOCK(Assert(m_node.chainman)->GetMutex());
2021
auto& chain = m_node.chain;
2122
const CChain& active = Assert(m_node.chainman)->ActiveChain();
2223

@@ -61,6 +62,7 @@ BOOST_AUTO_TEST_CASE(findBlock)
6162

6263
BOOST_AUTO_TEST_CASE(findFirstBlockWithTimeAndHeight)
6364
{
65+
LOCK(Assert(m_node.chainman)->GetMutex());
6466
auto& chain = m_node.chain;
6567
const CChain& active = Assert(m_node.chainman)->ActiveChain();
6668
uint256 hash;
@@ -73,6 +75,7 @@ BOOST_AUTO_TEST_CASE(findFirstBlockWithTimeAndHeight)
7375

7476
BOOST_AUTO_TEST_CASE(findAncestorByHeight)
7577
{
78+
LOCK(Assert(m_node.chainman)->GetMutex());
7679
auto& chain = m_node.chain;
7780
const CChain& active = Assert(m_node.chainman)->ActiveChain();
7881
uint256 hash;
@@ -83,6 +86,7 @@ BOOST_AUTO_TEST_CASE(findAncestorByHeight)
8386

8487
BOOST_AUTO_TEST_CASE(findAncestorByHash)
8588
{
89+
LOCK(Assert(m_node.chainman)->GetMutex());
8690
auto& chain = m_node.chain;
8791
const CChain& active = Assert(m_node.chainman)->ActiveChain();
8892
int height = -1;
@@ -94,7 +98,7 @@ BOOST_AUTO_TEST_CASE(findAncestorByHash)
9498
BOOST_AUTO_TEST_CASE(findCommonAncestor)
9599
{
96100
auto& chain = m_node.chain;
97-
const CChain& active = Assert(m_node.chainman)->ActiveChain();
101+
const CChain& active = WITH_LOCK(Assert(m_node.chainman)->GetMutex(), return Assert(m_node.chainman)->ActiveChain());
98102
auto* orig_tip = active.Tip();
99103
for (int i = 0; i < 10; ++i) {
100104
BlockValidationState state;

‎src/test/validation_block_tests.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -233,7 +233,7 @@ BOOST_AUTO_TEST_CASE(mempool_locks_reorg)
233233

234234
// Run the test multiple times
235235
for (int test_runs = 3; test_runs > 0; --test_runs) {
236-
BOOST_CHECK_EQUAL(last_mined->GetHash(), m_node.chainman->ActiveChain().Tip()->GetBlockHash());
236+
BOOST_CHECK_EQUAL(last_mined->GetHash(), WITH_LOCK(Assert(m_node.chainman)->GetMutex(), return m_node.chainman->ActiveChain().Tip()->GetBlockHash()));
237237

238238
// Later on split from here
239239
const uint256 split_hash{last_mined->hashPrevBlock};
@@ -322,7 +322,7 @@ BOOST_AUTO_TEST_CASE(mempool_locks_reorg)
322322
ProcessBlock(b);
323323
}
324324
// Check that the reorg was eventually successful
325-
BOOST_CHECK_EQUAL(last_mined->GetHash(), m_node.chainman->ActiveChain().Tip()->GetBlockHash());
325+
BOOST_CHECK_EQUAL(last_mined->GetHash(), WITH_LOCK(Assert(m_node.chainman)->GetMutex(), return m_node.chainman->ActiveChain().Tip()->GetBlockHash()));
326326

327327
// We can join the other thread, which returns when the reorg was successful
328328
rpc_thread.join();

‎src/test/validation_chainstatemanager_tests.cpp‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -59,12 +59,12 @@ BOOST_AUTO_TEST_CASE(chainstatemanager)
5959
auto all = manager.GetAll();
6060
BOOST_CHECK_EQUAL_COLLECTIONS(all.begin(), all.end(), chainstates.begin(), chainstates.end());
6161

62-
auto& active_chain = manager.ActiveChain();
62+
auto& active_chain = WITH_LOCK(manager.GetMutex(), return manager.ActiveChain());
6363
BOOST_CHECK_EQUAL(&active_chain, &c1.m_chain);
6464

65-
BOOST_CHECK_EQUAL(manager.ActiveHeight(), -1);
65+
BOOST_CHECK_EQUAL(WITH_LOCK(manager.GetMutex(), return manager.ActiveHeight()), -1);
6666

67-
auto active_tip = manager.ActiveTip();
67+
auto active_tip = WITH_LOCK(manager.GetMutex(), return manager.ActiveTip());
6868
auto exp_tip = c1.m_chain.Tip();
6969
BOOST_CHECK_EQUAL(active_tip, exp_tip);
7070

@@ -103,12 +103,12 @@ BOOST_AUTO_TEST_CASE(chainstatemanager)
103103
auto all2 = manager.GetAll();
104104
BOOST_CHECK_EQUAL_COLLECTIONS(all2.begin(), all2.end(), chainstates.begin(), chainstates.end());
105105

106-
auto& active_chain2 = manager.ActiveChain();
106+
auto& active_chain2 = WITH_LOCK(manager.GetMutex(), return manager.ActiveChain());
107107
BOOST_CHECK_EQUAL(&active_chain2, &c2.m_chain);
108108

109-
BOOST_CHECK_EQUAL(manager.ActiveHeight(), 0);
109+
BOOST_CHECK_EQUAL(WITH_LOCK(manager.GetMutex(), return manager.ActiveHeight()), 0);
110110

111-
auto active_tip2 = manager.ActiveTip();
111+
auto active_tip2 = WITH_LOCK(manager.GetMutex(), return manager.ActiveTip());
112112
auto exp_tip2 = c2.m_chain.Tip();
113113
BOOST_CHECK_EQUAL(active_tip2, exp_tip2);
114114

@@ -260,7 +260,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_activate_snapshot, TestChain100Setup)
260260
BOOST_CHECK(WITH_LOCK(::cs_main, return !chainman.ActiveChain().Genesis()->IsAssumedValid()));
261261

262262
const AssumeutxoData& au_data = *ExpectedAssumeutxo(snapshot_height, ::Params());
263-
const CBlockIndex* tip = chainman.ActiveTip();
263+
const CBlockIndex* tip = WITH_LOCK(chainman.GetMutex(), return chainman.ActiveTip());
264264

265265
BOOST_CHECK_EQUAL(tip->nChainTx, au_data.nChainTx);
266266

@@ -359,7 +359,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_loadblockindex, TestChain100Setup)
359359
const int assumed_valid_start_idx = last_assumed_valid_idx - expected_assumed_valid;
360360

361361
CBlockIndex* validated_tip{nullptr};
362-
CBlockIndex* assumed_tip{chainman.ActiveChain().Tip()};
362+
CBlockIndex* assumed_tip{WITH_LOCK(chainman.GetMutex(), return chainman.ActiveChain().Tip())};
363363

364364
auto reload_all_block_indexes = [&]() {
365365
for (CChainState* cs : chainman.GetAll()) {

‎src/validation.h‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -995,9 +995,9 @@ class ChainstateManager
995995

996996
//! The most-work chain.
997997
CChainState& ActiveChainstate() const;
998-
CChain& ActiveChain() const { return ActiveChainstate().m_chain; }
999-
int ActiveHeight() const { return ActiveChain().Height(); }
1000-
CBlockIndex* ActiveTip() const { return ActiveChain().Tip(); }
998+
CChain& ActiveChain() const EXCLUSIVE_LOCKS_REQUIRED(GetMutex()) { return ActiveChainstate().m_chain; }
999+
int ActiveHeight() const EXCLUSIVE_LOCKS_REQUIRED(GetMutex()) { return ActiveChain().Height(); }
1000+
CBlockIndex* ActiveTip() const EXCLUSIVE_LOCKS_REQUIRED(GetMutex()) { return ActiveChain().Tip(); }
10011001

10021002
node::BlockMap& BlockIndex() EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
10031003
{

‎src/wallet/test/spend_tests.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ BOOST_FIXTURE_TEST_SUITE(spend_tests, WalletTestingSetup)
1717
BOOST_FIXTURE_TEST_CASE(SubtractFee, TestChain100Setup)
1818
{
1919
CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey()));
20-
auto wallet = CreateSyncedWallet(*m_node.chain, *m_node.coinjoin_loader, m_node.chainman->ActiveChain(), m_args, coinbaseKey);
20+
auto wallet = CreateSyncedWallet(*m_node.chain, *m_node.coinjoin_loader, WITH_LOCK(Assert(m_node.chainman)->GetMutex(), return m_node.chainman->ActiveChain(), m_args, coinbaseKey);
2121

2222
// Check that a subtract-from-recipient transaction slightly less than the
2323
// coinbase input amount does not create a change output (because it would

‎src/wallet/test/wallet_tests.cpp‎

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -105,17 +105,18 @@ static void AddKey(CWallet& wallet, const CKey& key)
105105
BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup)
106106
{
107107
// Cap last block file size, and mine new block in a new block file.
108-
CBlockIndex* oldTip = m_node.chainman->ActiveChain().Tip();
108+
CBlockIndex* oldTip = WITH_LOCK(Assert(m_node.chainman)->GetMutex(), return m_node.chainman->ActiveChain().Tip());
109109
WITH_LOCK(::cs_main, m_node.chainman->m_blockman.GetBlockFileInfo(oldTip->GetBlockPos().nFile)->nSize = MAX_BLOCKFILE_SIZE);
110110
CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey()));
111-
CBlockIndex* newTip = m_node.chainman->ActiveChain().Tip();
111+
CBlockIndex* newTip = WITH_LOCK(Assert(m_node.chainman)->GetMutex(), return m_node.chainman->ActiveChain().Tip());
112112

113113
// Verify ScanForWalletTransactions fails to read an unknown start block.
114114
{
115115
CWallet wallet(m_node.chain.get(), m_node.coinjoin_loader.get(), "", m_args, CreateDummyWalletDatabase());
116116
wallet.SetupLegacyScriptPubKeyMan();
117117
{
118118
LOCK(wallet.cs_wallet);
119+
LOCK(Assert(m_node.chainman)->GetMutex());
119120
wallet.SetWalletFlag(WALLET_FLAG_DESCRIPTORS);
120121
wallet.SetLastBlockProcessed(m_node.chainman->ActiveChain().Height(), m_node.chainman->ActiveChain().Tip()->GetBlockHash());
121122
}
@@ -136,6 +137,7 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup)
136137
CWallet wallet(m_node.chain.get(), m_node.coinjoin_loader.get(), "", m_args, CreateMockWalletDatabase());
137138
{
138139
LOCK(wallet.cs_wallet);
140+
LOCK(Assert(m_node.chainman)->GetMutex());
139141
wallet.SetWalletFlag(WALLET_FLAG_DESCRIPTORS);
140142
wallet.SetLastBlockProcessed(m_node.chainman->ActiveChain().Height(), m_node.chainman->ActiveChain().Tip()->GetBlockHash());
141143
}
@@ -180,6 +182,7 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup)
180182
CWallet wallet(m_node.chain.get(), m_node.coinjoin_loader.get(), "", m_args, CreateDummyWalletDatabase());
181183
{
182184
LOCK(wallet.cs_wallet);
185+
LOCK(Assert(m_node.chainman)->GetMutex());
183186
wallet.SetWalletFlag(WALLET_FLAG_DESCRIPTORS);
184187
wallet.SetLastBlockProcessed(m_node.chainman->ActiveChain().Height(), m_node.chainman->ActiveChain().Tip()->GetBlockHash());
185188
}
@@ -207,6 +210,7 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup)
207210
CWallet wallet(m_node.chain.get(), m_node.coinjoin_loader.get(), "", m_args, CreateDummyWalletDatabase());
208211
{
209212
LOCK(wallet.cs_wallet);
213+
LOCK(Assert(m_node.chainman)->GetMutex());
210214
wallet.SetWalletFlag(WALLET_FLAG_DESCRIPTORS);
211215
wallet.SetLastBlockProcessed(m_node.chainman->ActiveChain().Height(), m_node.chainman->ActiveChain().Tip()->GetBlockHash());
212216
}
@@ -225,10 +229,10 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup)
225229
BOOST_FIXTURE_TEST_CASE(importmulti_rescan, TestChain100Setup)
226230
{
227231
// Cap last block file size, and mine new block in a new block file.
228-
CBlockIndex* oldTip = m_node.chainman->ActiveChain().Tip();
232+
CBlockIndex* oldTip = WITH_LOCK(Assert(m_node.chainman)->GetMutex(), return m_node.chainman->ActiveChain().Tip());
229233
WITH_LOCK(::cs_main, m_node.chainman->m_blockman.GetBlockFileInfo(oldTip->GetBlockPos().nFile)->nSize = MAX_BLOCKFILE_SIZE);
230234
CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey()));
231-
CBlockIndex* newTip = m_node.chainman->ActiveChain().Tip();
235+
CBlockIndex* newTip = WITH_LOCK(Assert(m_node.chainman)->GetMutex(), return m_node.chainman->ActiveChain().Tip());
232236

233237
// Prune the older block file.
234238
int file_number;
@@ -292,7 +296,7 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup)
292296
{
293297
// Create two blocks with same timestamp to verify that importwallet rescan
294298
// will pick up both blocks, not just the first.
295-
const int64_t BLOCK_TIME = m_node.chainman->ActiveChain().Tip()->GetBlockTimeMax() + 5;
299+
const int64_t BLOCK_TIME = WITH_LOCK(Assert(m_node.chainman)->GetMutex(), return m_node.chainman->ActiveChain().Tip()->GetBlockTimeMax() + 5);
296300
SetMockTime(BLOCK_TIME);
297301
m_coinbase_txns.emplace_back(CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())).vtx[0]);
298302
m_coinbase_txns.emplace_back(CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())).vtx[0]);
@@ -317,6 +321,7 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup)
317321
spk_man->AddKeyPubKey(coinbaseKey, coinbaseKey.GetPubKey());
318322

319323
AddWallet(context, wallet);
324+
LOCK(Assert(m_node.chainman)->GetMutex());
320325
wallet->SetLastBlockProcessed(m_node.chainman->ActiveChain().Height(), m_node.chainman->ActiveChain().Tip()->GetBlockHash());
321326
}
322327
JSONRPCRequest request;
@@ -342,6 +347,7 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup)
342347
request.params.setArray();
343348
request.params.push_back(backup_file);
344349
AddWallet(context, wallet);
350+
LOCK(Assert(m_node.chainman)->GetMutex());
345351
wallet->SetLastBlockProcessed(m_node.chainman->ActiveChain().Height(), m_node.chainman->ActiveChain().Tip()->GetBlockHash());
346352
wallet::importwallet().HandleRequest(request);
347353
RemoveWallet(context, wallet, /*load_on_start=*/std::nullopt);
@@ -368,6 +374,8 @@ BOOST_FIXTURE_TEST_CASE(coin_mark_dirty_immature_credit, TestChain100Setup)
368374
CWalletTx wtx{m_coinbase_txns.back(), TxStateConfirmed{m_node.chainman->ActiveChain().Tip()->GetBlockHash(), m_node.chainman->ActiveChain().Height(), /*index=*/0}};
369375

370376
LOCK(wallet.cs_wallet);
377+
LOCK(Assert(m_node.chainman)->GetMutex());
378+
CWalletTx wtx{m_coinbase_txns.back(), TxStateConfirmed{m_node.chainman->ActiveChain().Tip()->GetBlockHash(), m_node.chainman->ActiveChain().Height(), /*index=*/0}};
371379
wallet.SetWalletFlag(WALLET_FLAG_DESCRIPTORS);
372380
wallet.SetupDescriptorScriptPubKeyMans("", "");
373381

@@ -537,7 +545,7 @@ class ListCoinsTestingSetup : public TestChain100Setup
537545
ListCoinsTestingSetup()
538546
{
539547
CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey()));
540-
wallet = CreateSyncedWallet(*m_node.chain, *m_node.coinjoin_loader, m_node.chainman->ActiveChain(), m_args, coinbaseKey);
548+
wallet = CreateSyncedWallet(*m_node.chain, *m_node.coinjoin_loader, WITH_LOCK(Assert(m_node.chainman)->GetMutex(), return m_node.chainman->ActiveChain()), m_args, coinbaseKey);
541549
}
542550

543551
~ListCoinsTestingSetup()
@@ -563,6 +571,7 @@ class ListCoinsTestingSetup : public TestChain100Setup
563571
CreateAndProcessBlock({CMutableTransaction(blocktx)}, GetScriptForRawPubKey(coinbaseKey.GetPubKey()));
564572

565573
LOCK(wallet->cs_wallet);
574+
LOCK(Assert(m_node.chainman)->GetMutex());
566575
wallet->SetLastBlockProcessed(wallet->GetLastBlockHeight() + 1, m_node.chainman->ActiveChain().Tip()->GetBlockHash());
567576
auto it = wallet->mapWallet.find(tx->GetHash());
568577
BOOST_CHECK(it != wallet->mapWallet.end());

0 commit comments

Comments
 (0)