Skip to content

Commit 8bbe589

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 32dbb2f commit 8bbe589

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
@@ -2187,7 +2187,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
21872187
// Either install a handler to notify us when genesis activates, or set fHaveGenesis directly.
21882188
// No locking, as this happens before any background thread is started.
21892189
boost::signals2::connection block_notify_genesis_wait_connection;
2190-
if (chainman.ActiveChain().Tip() == nullptr) {
2190+
if (WITH_LOCK(chainman.GetMutex(), return chainman.ActiveChain().Tip() == nullptr)) {
21912191
block_notify_genesis_wait_connection = uiInterface.NotifyBlockTip_connect(std::bind(BlockNotifyGenesisWait, std::placeholders::_2));
21922192
} else {
21932193
fHaveGenesis = true;
@@ -2426,12 +2426,12 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
24262426
// At this point, the RPC is "started", but still in warmup, which means it
24272427
// cannot yet be called. Before we make it callable, we need to make sure
24282428
// that the RPC's view of the best block is valid and consistent with
2429-
// ChainstateManager's ActiveTip.
2429+
// ChainstateManager's active tip.
24302430
//
24312431
// If we do not do this, RPC's view of the best block will be height=0 and
24322432
// hash=0x0. This will lead to erroroneous responses for things like
24332433
// waitforblockheight.
2434-
RPCNotifyBlockChange(chainman.ActiveTip());
2434+
RPCNotifyBlockChange(WITH_LOCK(chainman.GetMutex(), return chainman.ActiveTip()));
24352435
SetRPCWarmupFinished();
24362436

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

‎src/net_processing.cpp‎

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

‎src/qt/test/wallettests.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -127,14 +127,14 @@ void TestGUI(interfaces::Node& node)
127127
if (!wallet->AddWalletDescriptor(w_desc, provider, "", false)) assert(false);
128128
CTxDestination dest = PKHash(test.coinbaseKey.GetPubKey());
129129
wallet->SetAddressBook(dest, "", "receive");
130-
wallet->SetLastBlockProcessed(105, node.context()->chainman->ActiveChain().Tip()->GetBlockHash());
130+
wallet->SetLastBlockProcessed(105, WITH_LOCK(node.context()->chainman->GetMutex(), return node.context()->chainman->ActiveChain().Tip()->GetBlockHash()));
131131
}
132132
{
133133
WalletRescanReserver reserver(*wallet);
134134
reserver.reserve();
135135
CWallet::ScanResult result = wallet->ScanForWalletTransactions(Params().GetConsensus().hashGenesisBlock, 0 /* start_height */, {} /* max_height */, reserver, true /* fUpdate */);
136136
QCOMPARE(result.status, CWallet::ScanResult::SUCCESS);
137-
QCOMPARE(result.last_scanned_block, node.context()->chainman->ActiveChain().Tip()->GetBlockHash());
137+
QCOMPARE(result.last_scanned_block, WITH_LOCK(node.context()->chainman->GetMutex(), return node.context()->chainman->ActiveChain().Tip()->GetBlockHash()));
138138
QVERIFY(result.last_failed_block.IsNull());
139139
}
140140
wallet->SetBroadcastTransactions(true);

‎src/rest.cpp‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -809,14 +809,18 @@ static bool rest_getutxos(const CoreContext& context, HTTPRequest* req, const st
809809
ChainstateManager* maybe_chainman = GetChainman(context, req);
810810
if (!maybe_chainman) return false;
811811
ChainstateManager& chainman = *maybe_chainman;
812+
decltype(chainman.ActiveHeight()) active_height;
813+
uint256 active_hash;
812814
{
813-
auto process_utxos = [&vOutPoints, &outs, &hits](const CCoinsView& view, const CTxMemPool& mempool) {
815+
auto process_utxos = [&vOutPoints, &outs, &hits, &active_height, &active_hash, &chainman](const CCoinsView& view, const CTxMemPool& mempool) EXCLUSIVE_LOCKS_REQUIRED(chainman.GetMutex()) {
814816
for (const COutPoint& vOutPoint : vOutPoints) {
815817
Coin coin;
816818
bool hit = !mempool.isSpent(vOutPoint) && view.GetCoin(vOutPoint, coin);
817819
hits.push_back(hit);
818820
if (hit) outs.emplace_back(std::move(coin));
819821
}
822+
active_height = chainman.ActiveHeight();
823+
active_hash = chainman.ActiveTip()->GetBlockHash();
820824
};
821825

822826
if (fCheckMemPool) {
@@ -844,7 +848,7 @@ static bool rest_getutxos(const CoreContext& context, HTTPRequest* req, const st
844848
// serialize data
845849
// use exact same output as mentioned in Bip64
846850
CDataStream ssGetUTXOResponse(SER_NETWORK, PROTOCOL_VERSION);
847-
ssGetUTXOResponse << chainman.ActiveChain().Height() << chainman.ActiveChain().Tip()->GetBlockHash() << bitmap << outs;
851+
ssGetUTXOResponse << active_height << active_hash << bitmap << outs;
848852
std::string ssGetUTXOResponseString = ssGetUTXOResponse.str();
849853

850854
req->WriteHeader("Content-Type", "application/octet-stream");
@@ -854,7 +858,7 @@ static bool rest_getutxos(const CoreContext& context, HTTPRequest* req, const st
854858

855859
case RetFormat::HEX: {
856860
CDataStream ssGetUTXOResponse(SER_NETWORK, PROTOCOL_VERSION);
857-
ssGetUTXOResponse << chainman.ActiveChain().Height() << chainman.ActiveChain().Tip()->GetBlockHash() << bitmap << outs;
861+
ssGetUTXOResponse << active_height << active_hash << bitmap << outs;
858862
std::string strHex = HexStr(ssGetUTXOResponse) + "\n";
859863

860864
req->WriteHeader("Content-Type", "text/plain");
@@ -867,8 +871,8 @@ static bool rest_getutxos(const CoreContext& context, HTTPRequest* req, const st
867871

868872
// pack in some essentials
869873
// use more or less the same output as mentioned in Bip64
870-
objGetUTXOResponse.pushKV("chainHeight", chainman.ActiveChain().Height());
871-
objGetUTXOResponse.pushKV("chaintipHash", chainman.ActiveChain().Tip()->GetBlockHash().GetHex());
874+
objGetUTXOResponse.pushKV("chainHeight", active_height);
875+
objGetUTXOResponse.pushKV("chaintipHash", active_hash.GetHex());
872876
objGetUTXOResponse.pushKV("bitmap", bitmapStringRepresentation);
873877

874878
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
@@ -231,7 +231,7 @@ BOOST_AUTO_TEST_CASE(mempool_locks_reorg)
231231

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

236236
// Later on split from here
237237
const uint256 split_hash{last_mined->hashPrevBlock};
@@ -320,7 +320,7 @@ BOOST_AUTO_TEST_CASE(mempool_locks_reorg)
320320
ProcessBlock(b);
321321
}
322322
// Check that the reorg was eventually successful
323-
BOOST_CHECK_EQUAL(last_mined->GetHash(), m_node.chainman->ActiveChain().Tip()->GetBlockHash());
323+
BOOST_CHECK_EQUAL(last_mined->GetHash(), WITH_LOCK(Assert(m_node.chainman)->GetMutex(), return m_node.chainman->ActiveChain().Tip()->GetBlockHash()));
324324

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

‎src/test/validation_chainstatemanager_tests.cpp‎

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

59-
auto& active_chain = manager.ActiveChain();
59+
auto& active_chain = WITH_LOCK(manager.GetMutex(), return manager.ActiveChain());
6060
BOOST_CHECK_EQUAL(&active_chain, &c1.m_chain);
6161

62-
BOOST_CHECK_EQUAL(manager.ActiveHeight(), -1);
62+
BOOST_CHECK_EQUAL(WITH_LOCK(manager.GetMutex(), return manager.ActiveHeight()), -1);
6363

64-
auto active_tip = manager.ActiveTip();
64+
auto active_tip = WITH_LOCK(manager.GetMutex(), return manager.ActiveTip());
6565
auto exp_tip = c1.m_chain.Tip();
6666
BOOST_CHECK_EQUAL(active_tip, exp_tip);
6767

@@ -101,12 +101,12 @@ BOOST_AUTO_TEST_CASE(chainstatemanager)
101101
auto all2 = manager.GetAll();
102102
BOOST_CHECK_EQUAL_COLLECTIONS(all2.begin(), all2.end(), chainstates.begin(), chainstates.end());
103103

104-
auto& active_chain2 = manager.ActiveChain();
104+
auto& active_chain2 = WITH_LOCK(manager.GetMutex(), return manager.ActiveChain());
105105
BOOST_CHECK_EQUAL(&active_chain2, &c2.m_chain);
106106

107-
BOOST_CHECK_EQUAL(manager.ActiveHeight(), 0);
107+
BOOST_CHECK_EQUAL(WITH_LOCK(manager.GetMutex(), return manager.ActiveHeight()), 0);
108108

109-
auto active_tip2 = manager.ActiveTip();
109+
auto active_tip2 = WITH_LOCK(manager.GetMutex(), return manager.ActiveTip());
110110
auto exp_tip2 = c2.m_chain.Tip();
111111
BOOST_CHECK_EQUAL(active_tip2, exp_tip2);
112112

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

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

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

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

360360
CBlockIndex* validated_tip{nullptr};
361-
CBlockIndex* assumed_tip{chainman.ActiveChain().Tip()};
361+
CBlockIndex* assumed_tip{WITH_LOCK(chainman.GetMutex(), return chainman.ActiveChain().Tip())};
362362

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

‎src/validation.h‎

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

960960
//! The most-work chain.
961961
CChainState& ActiveChainstate() const;
962-
CChain& ActiveChain() const { return ActiveChainstate().m_chain; }
963-
int ActiveHeight() const { return ActiveChain().Height(); }
964-
CBlockIndex* ActiveTip() const { return ActiveChain().Tip(); }
962+
CChain& ActiveChain() const EXCLUSIVE_LOCKS_REQUIRED(GetMutex()) { return ActiveChainstate().m_chain; }
963+
int ActiveHeight() const EXCLUSIVE_LOCKS_REQUIRED(GetMutex()) { return ActiveChain().Height(); }
964+
CBlockIndex* ActiveTip() const EXCLUSIVE_LOCKS_REQUIRED(GetMutex()) { return ActiveChain().Tip(); }
965965

966966
BlockMap& BlockIndex() EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
967967
{

‎src/wallet/test/spend_tests.cpp‎

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

2020
// Check that a subtract-from-recipient transaction slightly less than the
2121
// 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
@@ -104,17 +104,18 @@ static void AddKey(CWallet& wallet, const CKey& key)
104104
BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup)
105105
{
106106
// Cap last block file size, and mine new block in a new block file.
107-
CBlockIndex* oldTip = m_node.chainman->ActiveChain().Tip();
107+
CBlockIndex* oldTip = WITH_LOCK(Assert(m_node.chainman)->GetMutex(), return m_node.chainman->ActiveChain().Tip());
108108
WITH_LOCK(::cs_main, m_node.chainman->m_blockman.GetBlockFileInfo(oldTip->GetBlockPos().nFile)->nSize = MAX_BLOCKFILE_SIZE);
109109
CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey()));
110-
CBlockIndex* newTip = m_node.chainman->ActiveChain().Tip();
110+
CBlockIndex* newTip = WITH_LOCK(Assert(m_node.chainman)->GetMutex(), return m_node.chainman->ActiveChain().Tip());
111111

112112
// Verify ScanForWalletTransactions fails to read an unknown start block.
113113
{
114114
CWallet wallet(m_node.chain.get(), m_node.coinjoin_loader.get(), "", CreateDummyWalletDatabase());
115115
wallet.SetupLegacyScriptPubKeyMan();
116116
{
117117
LOCK(wallet.cs_wallet);
118+
LOCK(Assert(m_node.chainman)->GetMutex());
118119
wallet.SetWalletFlag(WALLET_FLAG_DESCRIPTORS);
119120
wallet.SetLastBlockProcessed(m_node.chainman->ActiveChain().Height(), m_node.chainman->ActiveChain().Tip()->GetBlockHash());
120121
}
@@ -135,6 +136,7 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup)
135136
CWallet wallet(m_node.chain.get(), m_node.coinjoin_loader.get(), "", CreateDummyWalletDatabase());
136137
{
137138
LOCK(wallet.cs_wallet);
139+
LOCK(Assert(m_node.chainman)->GetMutex());
138140
wallet.SetWalletFlag(WALLET_FLAG_DESCRIPTORS);
139141
wallet.SetLastBlockProcessed(m_node.chainman->ActiveChain().Height(), m_node.chainman->ActiveChain().Tip()->GetBlockHash());
140142
}
@@ -164,6 +166,7 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup)
164166
CWallet wallet(m_node.chain.get(), m_node.coinjoin_loader.get(), "", CreateDummyWalletDatabase());
165167
{
166168
LOCK(wallet.cs_wallet);
169+
LOCK(Assert(m_node.chainman)->GetMutex());
167170
wallet.SetWalletFlag(WALLET_FLAG_DESCRIPTORS);
168171
wallet.SetLastBlockProcessed(m_node.chainman->ActiveChain().Height(), m_node.chainman->ActiveChain().Tip()->GetBlockHash());
169172
}
@@ -191,6 +194,7 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup)
191194
CWallet wallet(m_node.chain.get(), m_node.coinjoin_loader.get(), "", CreateDummyWalletDatabase());
192195
{
193196
LOCK(wallet.cs_wallet);
197+
LOCK(Assert(m_node.chainman)->GetMutex());
194198
wallet.SetWalletFlag(WALLET_FLAG_DESCRIPTORS);
195199
wallet.SetLastBlockProcessed(m_node.chainman->ActiveChain().Height(), m_node.chainman->ActiveChain().Tip()->GetBlockHash());
196200
}
@@ -209,10 +213,10 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup)
209213
BOOST_FIXTURE_TEST_CASE(importmulti_rescan, TestChain100Setup)
210214
{
211215
// Cap last block file size, and mine new block in a new block file.
212-
CBlockIndex* oldTip = m_node.chainman->ActiveChain().Tip();
216+
CBlockIndex* oldTip = WITH_LOCK(Assert(m_node.chainman)->GetMutex(), return m_node.chainman->ActiveChain().Tip());
213217
WITH_LOCK(::cs_main, m_node.chainman->m_blockman.GetBlockFileInfo(oldTip->GetBlockPos().nFile)->nSize = MAX_BLOCKFILE_SIZE);
214218
CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey()));
215-
CBlockIndex* newTip = m_node.chainman->ActiveChain().Tip();
219+
CBlockIndex* newTip = WITH_LOCK(Assert(m_node.chainman)->GetMutex(), return m_node.chainman->ActiveChain().Tip());
216220

217221
// Prune the older block file.
218222
int file_number;
@@ -276,7 +280,7 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup)
276280
{
277281
// Create two blocks with same timestamp to verify that importwallet rescan
278282
// will pick up both blocks, not just the first.
279-
const int64_t BLOCK_TIME = m_node.chainman->ActiveChain().Tip()->GetBlockTimeMax() + 5;
283+
const int64_t BLOCK_TIME = WITH_LOCK(Assert(m_node.chainman)->GetMutex(), return m_node.chainman->ActiveChain().Tip()->GetBlockTimeMax() + 5);
280284
SetMockTime(BLOCK_TIME);
281285
m_coinbase_txns.emplace_back(CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())).vtx[0]);
282286
m_coinbase_txns.emplace_back(CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())).vtx[0]);
@@ -301,6 +305,7 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup)
301305
spk_man->AddKeyPubKey(coinbaseKey, coinbaseKey.GetPubKey());
302306

303307
AddWallet(context, wallet);
308+
LOCK(Assert(m_node.chainman)->GetMutex());
304309
wallet->SetLastBlockProcessed(m_node.chainman->ActiveChain().Height(), m_node.chainman->ActiveChain().Tip()->GetBlockHash());
305310
}
306311
JSONRPCRequest request;
@@ -326,6 +331,7 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup)
326331
request.params.setArray();
327332
request.params.push_back(backup_file);
328333
AddWallet(context, wallet);
334+
LOCK(Assert(m_node.chainman)->GetMutex());
329335
wallet->SetLastBlockProcessed(m_node.chainman->ActiveChain().Height(), m_node.chainman->ActiveChain().Tip()->GetBlockHash());
330336
::importwallet().HandleRequest(request);
331337
RemoveWallet(context, wallet, /*load_on_start=*/std::nullopt);
@@ -352,6 +358,8 @@ BOOST_FIXTURE_TEST_CASE(coin_mark_dirty_immature_credit, TestChain100Setup)
352358
CWalletTx wtx(&wallet, m_coinbase_txns.back());
353359

354360
LOCK(wallet.cs_wallet);
361+
LOCK(Assert(m_node.chainman)->GetMutex());
362+
CWalletTx wtx{m_coinbase_txns.back(), TxStateConfirmed{m_node.chainman->ActiveChain().Tip()->GetBlockHash(), m_node.chainman->ActiveChain().Height(), /*index=*/0}};
355363
wallet.SetWalletFlag(WALLET_FLAG_DESCRIPTORS);
356364
wallet.SetupDescriptorScriptPubKeyMans();
357365

@@ -524,7 +532,7 @@ class ListCoinsTestingSetup : public TestChain100Setup
524532
ListCoinsTestingSetup()
525533
{
526534
CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey()));
527-
wallet = CreateSyncedWallet(*m_node.chain, *m_node.coinjoin_loader, m_node.chainman->ActiveChain(), coinbaseKey);
535+
wallet = CreateSyncedWallet(*m_node.chain, *m_node.coinjoin_loader, WITH_LOCK(Assert(m_node.chainman)->GetMutex(), return m_node.chainman->ActiveChain()), coinbaseKey);
528536
}
529537

530538
~ListCoinsTestingSetup()
@@ -550,6 +558,7 @@ class ListCoinsTestingSetup : public TestChain100Setup
550558
CreateAndProcessBlock({CMutableTransaction(blocktx)}, GetScriptForRawPubKey(coinbaseKey.GetPubKey()));
551559

552560
LOCK(wallet->cs_wallet);
561+
LOCK(Assert(m_node.chainman)->GetMutex());
553562
wallet->SetLastBlockProcessed(wallet->GetLastBlockHeight() + 1, m_node.chainman->ActiveChain().Tip()->GetBlockHash());
554563
auto it = wallet->mapWallet.find(tx->GetHash());
555564
BOOST_CHECK(it != wallet->mapWallet.end());

0 commit comments

Comments
 (0)