HomePhabricator

Added best block hash to the NotifyHeaderTip and NotifyBlockTip signals.

Description

Added best block hash to the NotifyHeaderTip and NotifyBlockTip signals.

Summary:
PR description

Rationale:

The height based polling in the GUI is an issue on chain reorgs. Any new tip signal with the same height as the one that it's cached in the model was not triggering the GUI update (interpreting it as the same same block when it could receive a different one). Ending up with bad information presented in the GUI.

This PR essentially changes the last cached height to be a last cached block hash.

https://github.com/bitcoin/bitcoin/pull/17993/commits/2f867203b0c7a4438ce484be4cfa2b29dbf1abf0

[ClientModel] best header/block hash cached.

https://github.com/bitcoin/bitcoin/pull/19132/commits/f46b678acff0b2e75e26aa50b14d935b3d251a2a

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 is a backport of Core PR17993 [1/2] and PR19132

Test Plan:
ninja && src/qt/bitcoin-qt
Send a transaction, wait for a few block and check that the tx status is correctly updated in the interface.

Reviewers: #bitcoin_abc, majcosta

Reviewed By: #bitcoin_abc, majcosta

Subscribers: majcosta

Differential Revision: https://reviews.bitcoinabc.org/D9253

Details

Provenance
furszy <matiasfurszyfer@protonmail.com>Authored on Jan 23 2020, 22:31
PiRKCommitted on Tue, Feb 23, 12:53
PiRKPushed on Tue, Feb 23, 12:53
Reviewer
Restricted Project
Differential Revision
D9253: Added best block hash to the NotifyHeaderTip and NotifyBlockTip signals.
Parents
rABC5c69fab896b6: [buildbot] make bot link to backports of core-gui repo
Branches
Unknown
Tags
Unknown