Page MenuHomePhabricator

Added best block hash to the NotifyHeaderTip and NotifyBlockTip signals.
ClosedPublic

Authored by PiRK on Feb 22 2021, 14:44.

Details

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.

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

PiRK requested review of this revision.Feb 22 2021, 14:44
majcosta requested changes to this revision.Feb 22 2021, 18:33
majcosta added a subscriber: majcosta.
majcosta added inline comments.
src/qt/transactionrecord.h
85–86 ↗(On Diff #27768)

this is from PR10449, remove pls

This revision now requires changes to proceed.Feb 22 2021, 18:33
PiRK edited the summary of this revision. (Show Details)

remove unnecessary line added accidentaly when fixing conflict

majcosta requested changes to this revision.Feb 23 2021, 08:10

now you reverted PR19132's changes completely

This revision now requires changes to proceed.Feb 23 2021, 08:10

now you reverted PR19132's changes completely

OK I see what happened. Branch pr19132 was initially on top of pr17993, but ended up on a separate branch altogether when I rebased to squash the commits, yesterday. And today I amended a commit on outdated branch pr17993 instead of branch pr19132. Will fix this.

Good catch.

fix accidental revert of core#19132

This revision is now accepted and ready to land.Feb 23 2021, 09:40