Skip to content

Commit a75b779

Browse files
fanquakefanquake
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) bitcoin#16 util::TraceThread(char const*, std::__1::function<void ()>) src/util/thread.cpp:18:9 (bitcoind+0x88891f) bitcoin#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) bitcoin#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) bitcoin#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
2 parents aa774f3 + fac04cb commit a75b779

12 files changed

+68
-35
lines changed

‎src/bitcoin-chainstate.cpp‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -115,12 +115,14 @@ int main(int argc, char* argv[])
115115
// Main program logic starts here
116116
std::cout
117117
<< "Hello! I'm going to print out some information about your datadir." << std::endl
118-
<< "\t" << "Path: " << gArgs.GetDataDirNet() << std::endl
118+
<< "\t" << "Path: " << gArgs.GetDataDirNet() << std::endl;
119+
{
120+
LOCK(chainman.GetMutex());
121+
std::cout
119122
<< "\t" << "Reindexing: " << std::boolalpha << node::fReindex.load() << std::noboolalpha << std::endl
120123
<< "\t" << "Snapshot Active: " << std::boolalpha << chainman.IsSnapshotActive() << std::noboolalpha << std::endl
121124
<< "\t" << "Active Height: " << chainman.ActiveHeight() << std::endl
122125
<< "\t" << "Active IBD: " << std::boolalpha << chainman.ActiveChainstate().IsInitialBlockDownload() << std::noboolalpha << std::endl;
123-
{
124126
CBlockIndex* tip = chainman.ActiveTip();
125127
if (tip) {
126128
std::cout << "\t" << tip->ToString() << std::endl;

‎src/init.cpp‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1547,7 +1547,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
15471547
// Either install a handler to notify us when genesis activates, or set fHaveGenesis directly.
15481548
// No locking, as this happens before any background thread is started.
15491549
boost::signals2::connection block_notify_genesis_wait_connection;
1550-
if (chainman.ActiveChain().Tip() == nullptr) {
1550+
if (WITH_LOCK(chainman.GetMutex(), return chainman.ActiveChain().Tip() == nullptr)) {
15511551
block_notify_genesis_wait_connection = uiInterface.NotifyBlockTip_connect(std::bind(BlockNotifyGenesisWait, std::placeholders::_2));
15521552
} else {
15531553
fHaveGenesis = true;
@@ -1751,12 +1751,12 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
17511751
// At this point, the RPC is "started", but still in warmup, which means it
17521752
// cannot yet be called. Before we make it callable, we need to make sure
17531753
// that the RPC's view of the best block is valid and consistent with
1754-
// ChainstateManager's ActiveTip.
1754+
// ChainstateManager's active tip.
17551755
//
17561756
// If we do not do this, RPC's view of the best block will be height=0 and
17571757
// hash=0x0. This will lead to erroroneous responses for things like
17581758
// waitforblockheight.
1759-
RPCNotifyBlockChange(chainman.ActiveTip());
1759+
RPCNotifyBlockChange(WITH_LOCK(chainman.GetMutex(), return chainman.ActiveTip()));
17601760
SetRPCWarmupFinished();
17611761

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

‎src/net_processing.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2510,7 +2510,7 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, Peer& peer,
25102510
// Consider fetching more headers.
25112511
if (nCount == MAX_HEADERS_RESULTS) {
25122512
// Headers message had its maximum size; the peer may have more headers.
2513-
if (MaybeSendGetHeaders(pfrom, m_chainman.ActiveChain().GetLocator(pindexLast), peer)) {
2513+
if (MaybeSendGetHeaders(pfrom, WITH_LOCK(m_chainman.GetMutex(), return m_chainman.ActiveChain().GetLocator(pindexLast)), peer)) {
25142514
LogPrint(BCLog::NET, "more getheaders (%d) to end to peer=%d (startheight:%d)\n",
25152515
pindexLast->nHeight, pfrom.GetId(), peer.m_starting_height);
25162516
}

‎src/qt/test/wallettests.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -175,14 +175,14 @@ void TestGUI(interfaces::Node& node)
175175
if (!wallet->AddWalletDescriptor(w_desc, provider, "", false)) assert(false);
176176
CTxDestination dest = GetDestinationForKey(test.coinbaseKey.GetPubKey(), wallet->m_default_address_type);
177177
wallet->SetAddressBook(dest, "", "receive");
178-
wallet->SetLastBlockProcessed(105, node.context()->chainman->ActiveChain().Tip()->GetBlockHash());
178+
wallet->SetLastBlockProcessed(105, WITH_LOCK(node.context()->chainman->GetMutex(), return node.context()->chainman->ActiveChain().Tip()->GetBlockHash()));
179179
}
180180
{
181181
WalletRescanReserver reserver(*wallet);
182182
reserver.reserve();
183183
CWallet::ScanResult result = wallet->ScanForWalletTransactions(Params().GetConsensus().hashGenesisBlock, /*start_height=*/0, /*max_height=*/{}, reserver, /*fUpdate=*/true, /*save_progress=*/false);
184184
QCOMPARE(result.status, CWallet::ScanResult::SUCCESS);
185-
QCOMPARE(result.last_scanned_block, node.context()->chainman->ActiveChain().Tip()->GetBlockHash());
185+
QCOMPARE(result.last_scanned_block, WITH_LOCK(node.context()->chainman->GetMutex(), return node.context()->chainman->ActiveChain().Tip()->GetBlockHash()));
186186
QVERIFY(result.last_failed_block.IsNull());
187187
}
188188
wallet->SetBroadcastTransactions(true);

‎src/rest.cpp‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -784,14 +784,18 @@ static bool rest_getutxos(const std::any& context, HTTPRequest* req, const std::
784784
ChainstateManager* maybe_chainman = GetChainman(context, req);
785785
if (!maybe_chainman) return false;
786786
ChainstateManager& chainman = *maybe_chainman;
787+
decltype(chainman.ActiveHeight()) active_height;
788+
uint256 active_hash;
787789
{
788-
auto process_utxos = [&vOutPoints, &outs, &hits](const CCoinsView& view, const CTxMemPool* mempool) {
790+
auto process_utxos = [&vOutPoints, &outs, &hits, &active_height, &active_hash, &chainman](const CCoinsView& view, const CTxMemPool* mempool) EXCLUSIVE_LOCKS_REQUIRED(chainman.GetMutex()) {
789791
for (const COutPoint& vOutPoint : vOutPoints) {
790792
Coin coin;
791793
bool hit = (!mempool || !mempool->isSpent(vOutPoint)) && view.GetCoin(vOutPoint, coin);
792794
hits.push_back(hit);
793795
if (hit) outs.emplace_back(std::move(coin));
794796
}
797+
active_height = chainman.ActiveHeight();
798+
active_hash = chainman.ActiveTip()->GetBlockHash();
795799
};
796800

797801
if (fCheckMemPool) {
@@ -819,7 +823,7 @@ static bool rest_getutxos(const std::any& context, HTTPRequest* req, const std::
819823
// serialize data
820824
// use exact same output as mentioned in Bip64
821825
CDataStream ssGetUTXOResponse(SER_NETWORK, PROTOCOL_VERSION);
822-
ssGetUTXOResponse << chainman.ActiveChain().Height() << chainman.ActiveChain().Tip()->GetBlockHash() << bitmap << outs;
826+
ssGetUTXOResponse << active_height << active_hash << bitmap << outs;
823827
std::string ssGetUTXOResponseString = ssGetUTXOResponse.str();
824828

825829
req->WriteHeader("Content-Type", "application/octet-stream");
@@ -829,7 +833,7 @@ static bool rest_getutxos(const std::any& context, HTTPRequest* req, const std::
829833

830834
case RESTResponseFormat::HEX: {
831835
CDataStream ssGetUTXOResponse(SER_NETWORK, PROTOCOL_VERSION);
832-
ssGetUTXOResponse << chainman.ActiveChain().Height() << chainman.ActiveChain().Tip()->GetBlockHash() << bitmap << outs;
836+
ssGetUTXOResponse << active_height << active_hash << bitmap << outs;
833837
std::string strHex = HexStr(ssGetUTXOResponse) + "\n";
834838

835839
req->WriteHeader("Content-Type", "text/plain");
@@ -842,8 +846,8 @@ static bool rest_getutxos(const std::any& context, HTTPRequest* req, const std::
842846

843847
// pack in some essentials
844848
// use more or less the same output as mentioned in Bip64
845-
objGetUTXOResponse.pushKV("chainHeight", chainman.ActiveChain().Height());
846-
objGetUTXOResponse.pushKV("chaintipHash", chainman.ActiveChain().Tip()->GetBlockHash().GetHex());
849+
objGetUTXOResponse.pushKV("chainHeight", active_height);
850+
objGetUTXOResponse.pushKV("chaintipHash", active_hash.GetHex());
847851
objGetUTXOResponse.pushKV("bitmap", bitmapStringRepresentation);
848852

849853
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: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -234,7 +234,7 @@ BOOST_AUTO_TEST_CASE(mempool_locks_reorg)
234234

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

239239
// Later on split from here
240240
const uint256 split_hash{last_mined->hashPrevBlock};
@@ -316,7 +316,7 @@ BOOST_AUTO_TEST_CASE(mempool_locks_reorg)
316316
ProcessBlock(b);
317317
}
318318
// Check that the reorg was eventually successful
319-
BOOST_CHECK_EQUAL(last_mined->GetHash(), m_node.chainman->ActiveChain().Tip()->GetBlockHash());
319+
BOOST_CHECK_EQUAL(last_mined->GetHash(), WITH_LOCK(Assert(m_node.chainman)->GetMutex(), return m_node.chainman->ActiveChain().Tip()->GetBlockHash()));
320320

321321
// We can join the other thread, which returns when the reorg was successful
322322
rpc_thread.join();
@@ -325,6 +325,7 @@ BOOST_AUTO_TEST_CASE(mempool_locks_reorg)
325325

326326
BOOST_AUTO_TEST_CASE(witness_commitment_index)
327327
{
328+
LOCK(Assert(m_node.chainman)->GetMutex());
328329
CScript pubKey;
329330
pubKey << 1 << OP_TRUE;
330331
auto ptemplate = BlockAssembler{m_node.chainman->ActiveChainstate(), m_node.mempool.get()}.CreateNewBlock(pubKey);

‎src/test/validation_chainstatemanager_tests.cpp‎

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

52-
auto& active_chain = manager.ActiveChain();
52+
auto& active_chain = WITH_LOCK(manager.GetMutex(), return manager.ActiveChain());
5353
BOOST_CHECK_EQUAL(&active_chain, &c1.m_chain);
5454

55-
BOOST_CHECK_EQUAL(manager.ActiveHeight(), -1);
55+
BOOST_CHECK_EQUAL(WITH_LOCK(manager.GetMutex(), return manager.ActiveHeight()), -1);
5656

57-
auto active_tip = manager.ActiveTip();
57+
auto active_tip = WITH_LOCK(manager.GetMutex(), return manager.ActiveTip());
5858
auto exp_tip = c1.m_chain.Tip();
5959
BOOST_CHECK_EQUAL(active_tip, exp_tip);
6060

@@ -84,12 +84,12 @@ BOOST_AUTO_TEST_CASE(chainstatemanager)
8484
auto all2 = manager.GetAll();
8585
BOOST_CHECK_EQUAL_COLLECTIONS(all2.begin(), all2.end(), chainstates.begin(), chainstates.end());
8686

87-
auto& active_chain2 = manager.ActiveChain();
87+
auto& active_chain2 = WITH_LOCK(manager.GetMutex(), return manager.ActiveChain());
8888
BOOST_CHECK_EQUAL(&active_chain2, &c2.m_chain);
8989

90-
BOOST_CHECK_EQUAL(manager.ActiveHeight(), 0);
90+
BOOST_CHECK_EQUAL(WITH_LOCK(manager.GetMutex(), return manager.ActiveHeight()), 0);
9191

92-
auto active_tip2 = manager.ActiveTip();
92+
auto active_tip2 = WITH_LOCK(manager.GetMutex(), return manager.ActiveTip());
9393
auto exp_tip2 = c2.m_chain.Tip();
9494
BOOST_CHECK_EQUAL(active_tip2, exp_tip2);
9595

@@ -236,7 +236,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_activate_snapshot, TestChain100Setup)
236236
BOOST_CHECK(WITH_LOCK(::cs_main, return !chainman.ActiveChain().Genesis()->IsAssumedValid()));
237237

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

241241
BOOST_CHECK_EQUAL(tip->nChainTx, au_data.nChainTx);
242242

@@ -335,7 +335,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_loadblockindex, TestChain100Setup)
335335
const int assumed_valid_start_idx = last_assumed_valid_idx - expected_assumed_valid;
336336

337337
CBlockIndex* validated_tip{nullptr};
338-
CBlockIndex* assumed_tip{chainman.ActiveChain().Tip()};
338+
CBlockIndex* assumed_tip{WITH_LOCK(chainman.GetMutex(), return chainman.ActiveChain().Tip())};
339339

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

‎src/validation.h‎

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -868,6 +868,19 @@ class ChainstateManager
868868
const CChainParams& GetParams() const { return m_chainparams; }
869869
const Consensus::Params& GetConsensus() const { return m_chainparams.GetConsensus(); }
870870

871+
/**
872+
* Alias for ::cs_main.
873+
* Should be used in new code to make it easier to make ::cs_main a member
874+
* of this class.
875+
* Generally, methods of this class should be annotated to require this
876+
* mutex. This will make calling code more verbose, but also help to:
877+
* - Clarify that the method will acquire a mutex that heavily affects
878+
* overall performance.
879+
* - Force call sites to think how long they need to acquire the mutex to
880+
* get consistent results.
881+
*/
882+
RecursiveMutex& GetMutex() const LOCK_RETURNED(::cs_main) { return ::cs_main; }
883+
871884
std::thread m_load_block;
872885
//! A single BlockManager instance is shared across each constructed
873886
//! chainstate to avoid duplicating block metadata.
@@ -938,9 +951,9 @@ class ChainstateManager
938951

939952
//! The most-work chain.
940953
CChainState& ActiveChainstate() const;
941-
CChain& ActiveChain() const { return ActiveChainstate().m_chain; }
942-
int ActiveHeight() const { return ActiveChain().Height(); }
943-
CBlockIndex* ActiveTip() const { return ActiveChain().Tip(); }
954+
CChain& ActiveChain() const EXCLUSIVE_LOCKS_REQUIRED(GetMutex()) { return ActiveChainstate().m_chain; }
955+
int ActiveHeight() const EXCLUSIVE_LOCKS_REQUIRED(GetMutex()) { return ActiveChain().Height(); }
956+
CBlockIndex* ActiveTip() const EXCLUSIVE_LOCKS_REQUIRED(GetMutex()) { return ActiveChain().Tip(); }
944957

945958
node::BlockMap& BlockIndex() EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
946959
{

‎src/wallet/test/availablecoins_tests.cpp‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ class AvailableCoinsTestingSetup : public TestChain100Setup
4444
CreateAndProcessBlock({CMutableTransaction(blocktx)}, GetScriptForRawPubKey(coinbaseKey.GetPubKey()));
4545

4646
LOCK(wallet->cs_wallet);
47+
LOCK(m_node.chainman->GetMutex());
4748
wallet->SetLastBlockProcessed(wallet->GetLastBlockHeight() + 1, m_node.chainman->ActiveChain().Tip()->GetBlockHash());
4849
auto it = wallet->mapWallet.find(tx->GetHash());
4950
BOOST_CHECK(it != wallet->mapWallet.end());

0 commit comments

Comments
 (0)