mirror of
https://github.com/bitcoin/bitcoin.git
synced 2025-02-03 09:56:38 -05:00
qt: lock cs_main, m_cached_tip_mutex in that order
Always lock the mutexes `cs_main` and `m_cached_tip_mutex` in the same order: `cs_main`, `m_cached_tip_mutex`. Otherwise we may end up in a deadlock. `ClientModel::m_cached_tip_blocks` is protected by `ClientModel::m_cached_tip_mutex`. There are two access paths that lock the two mutexes in opposite order: ``` validation.cpp:2868 CChainState::ActivateBestChain(): lock cs_main validation.cpp:2916 CChainState::ActivateBestChain(): call uiInterface.NotifyBlockTip() ui_interface.cpp:52 CClientUIInterface::NotifyBlockTip(): go deep in boost ... qt/clientmodel.cpp:255 BlockTipChanged(): lock m_cached_tip_mutex ``` and ``` qt/clientmodel.cpp:119 ClientModel::getBestBlockHash(): lock m_cached_tip_mutex qt/clientmodel.cpp:121 ClientModel::getBestBlockHash(): call m_node.getBestBlockHash() interfaces/node.cpp:200 NodeImpl::getBestBlockHash(): lock cs_main ``` From `debug.log`: ``` POTENTIAL DEADLOCK DETECTED Previous lock order was: m_cs_chainstate validation.cpp:2851 (1) cs_main validation.cpp:2868 ::mempool.cs validation.cpp:2868 (2) clientmodel->m_cached_tip_mutex qt/clientmodel.cpp:255 Current lock order is: (2) m_cached_tip_mutex qt/clientmodel.cpp:119 (1) ::cs_main interfaces/node.cpp:200 ``` The possible deadlock was introduced in https://github.com/bitcoin/bitcoin/pull/17993
This commit is contained in:
parent
9bc7751cad
commit
f46b678acf
1 changed files with 15 additions and 1 deletions
|
@ -116,9 +116,23 @@ int ClientModel::getNumBlocks() const
|
||||||
|
|
||||||
uint256 ClientModel::getBestBlockHash()
|
uint256 ClientModel::getBestBlockHash()
|
||||||
{
|
{
|
||||||
|
uint256 tip{WITH_LOCK(m_cached_tip_mutex, return m_cached_tip_blocks)};
|
||||||
|
|
||||||
|
if (!tip.IsNull()) {
|
||||||
|
return tip;
|
||||||
|
}
|
||||||
|
|
||||||
|
// Lock order must be: first `cs_main`, then `m_cached_tip_mutex`.
|
||||||
|
// The following will lock `cs_main` (and release it), so we must not
|
||||||
|
// own `m_cached_tip_mutex` here.
|
||||||
|
tip = m_node.getBestBlockHash();
|
||||||
|
|
||||||
LOCK(m_cached_tip_mutex);
|
LOCK(m_cached_tip_mutex);
|
||||||
|
// We checked that `m_cached_tip_blocks` is not null above, but then we
|
||||||
|
// released the mutex `m_cached_tip_mutex`, so it could have changed in the
|
||||||
|
// meantime. Thus, check again.
|
||||||
if (m_cached_tip_blocks.IsNull()) {
|
if (m_cached_tip_blocks.IsNull()) {
|
||||||
m_cached_tip_blocks = m_node.getBestBlockHash();
|
m_cached_tip_blocks = tip;
|
||||||
}
|
}
|
||||||
return m_cached_tip_blocks;
|
return m_cached_tip_blocks;
|
||||||
}
|
}
|
||||||
|
|
Loading…
Add table
Reference in a new issue