Merge #17993: gui: Balance/TxStatus polling update based on last block hash.

a06e845e826acaeb0db7cf02b2519c177e94dee5 BlockTip struct created and connected to notifyHeaderTip and notifyBlockTip signals. (furszy)
2f867203b0c7a4438ce484be4cfa2b29dbf1abf0 Added best block hash to the NotifyHeaderTip and NotifyBlockTip signals. (furszy)

Pull request 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.

  ---

  Old historical information of this PR.

  As the tip height is cached and updated via signaling in clientModel, there is no need to continue locking `cs_main` on every balance poll (`m_node.getNumBlocks()` method call).

  Extra topic:

  Would suggest to change the `cachedNumBlocks` field inside `walletModel` to a more understandable name, maybe `nLastBalanceUpdateHeight`.

  And finally, this will have the equal height reorg issue mentioned [here](https://github.com/bitcoin/bitcoin/pull/17905#issuecomment-577324304), whatever is presented to fix it, this should use the same flow too.

  **[Edit - 24/01/2020]**

  Have added #[17905](https://github.com/bitcoin/bitcoin/pull/17905#issuecomment-577324304) comment fix here too.

ACKs for top commit:
  jonasschnelli:
    utACK a06e845e826acaeb0db7cf02b2519c177e94dee5 - it would be great to have QT unit tests  (in this case for a reorg) that either automatically inspect the window content based on accessibility and tests for expected values or at least allow for quick manual re-testing (screenshots, automatically create UI situations).
  hebasto:
    re-ACK a06e845e826acaeb0db7cf02b2519c177e94dee5, suggested style changes implemented since the [previous](https://github.com/bitcoin/bitcoin/pull/17993#pullrequestreview-417249705) review.
  ryanofsky:
    Code review ACK a06e845e826acaeb0db7cf02b2519c177e94dee5. A lot of changes since the last review: rebase after sync_state introduction #18152 and tryGetBalances revert #18587, reverting getLastBlockTime change, fixing spacing and initializations and renaming some variables

Tree-SHA512: 835e587a8296df9899cccd7b3e598a5970942b640e432e6a32de0b4eaea5b40f9271258f089ec033595311707b74a0f7187ecf8ed397c713e1153e2714072975

11 files changed, 71 insertions, 40 deletions

diff --git a/src/interfaces/node.cpp b/src/interfaces/node.cpp
index 3c94e44b53..ca1435fe60 100644
--- a/src/interfaces/node.cpp
+++ b/src/interfaces/node.cpp
@@ -187,6 +187,11 @@ public:
         LOCK(::cs_main);
         return ::ChainActive().Height();
     }
+    uint256 getBestBlockHash() override
+    {
+        const CBlockIndex* tip = WITH_LOCK(::cs_main, return ::ChainActive().Tip());
+        return tip ? tip->GetBlockHash() : Params().GenesisBlock().GetHash();
+    }
     int64_t getLastBlockTime() override
     {
         LOCK(::cs_main);
@@ -310,7 +315,7 @@ public:
     std::unique_ptr<Handler> handleNotifyBlockTip(NotifyBlockTipFn fn) override
     {
         return MakeHandler(::uiInterface.NotifyBlockTip_connect([fn](SynchronizationState sync_state, const CBlockIndex* block) {
-            fn(sync_state, block->nHeight, block->GetBlockTime(),
+            fn(sync_state, BlockTip{block->nHeight, block->GetBlockTime(), block->GetBlockHash()},
                 GuessVerificationProgress(Params().TxData(), block));
         }));
     }
@@ -318,7 +323,7 @@ public:
     {
         return MakeHandler(
             ::uiInterface.NotifyHeaderTip_connect([fn](SynchronizationState sync_state, const CBlockIndex* block) {
-                fn(sync_state, block->nHeight, block->GetBlockTime(),
+                fn(sync_state, BlockTip{block->nHeight, block->GetBlockTime(), block->GetBlockHash()},
                     /* verification progress is unused when a header was received */ 0);
             }));
     }
diff --git a/src/interfaces/node.h b/src/interfaces/node.h
index 45b0e18fae..0b7fb6736a 100644
--- a/src/interfaces/node.h
+++ b/src/interfaces/node.h
@@ -36,6 +36,7 @@ struct bilingual_str;
 namespace interfaces {
 class Handler;
 class Wallet;
+struct BlockTip;
 
 //! Top-level interface for a bitcoin node (bitcoind process).
 class Node
@@ -149,6 +150,9 @@ public:
     //! Get num blocks.
     virtual int getNumBlocks() = 0;
 
+    //! Get best block hash.
+    virtual uint256 getBestBlockHash() = 0;
+
     //! Get last block time.
     virtual int64_t getLastBlockTime() = 0;
 
@@ -250,12 +254,12 @@ public:
 
     //! Register handler for block tip messages.
     using NotifyBlockTipFn =
-        std::function<void(SynchronizationState, int height, int64_t block_time, double verification_progress)>;
+        std::function<void(SynchronizationState, interfaces::BlockTip tip, double verification_progress)>;
     virtual std::unique_ptr<Handler> handleNotifyBlockTip(NotifyBlockTipFn fn) = 0;
 
     //! Register handler for header tip messages.
     using NotifyHeaderTipFn =
-        std::function<void(SynchronizationState, int height, int64_t block_time, double verification_progress)>;
+        std::function<void(SynchronizationState, interfaces::BlockTip tip, double verification_progress)>;
     virtual std::unique_ptr<Handler> handleNotifyHeaderTip(NotifyHeaderTipFn fn) = 0;
 
     //! Return pointer to internal chain interface, useful for testing.
@@ -265,6 +269,13 @@ public:
 //! Return implementation of Node interface.
 std::unique_ptr<Node> MakeNode();
 
+//! Block tip (could be a header or not, depends on the subscribed signal).
+struct BlockTip {
+    int block_height;
+    int64_t block_time;
+    uint256 block_hash;
+};
+
 } // namespace interfaces
 
 #endif // BITCOIN_INTERFACES_NODE_H
diff --git a/src/interfaces/wallet.cpp b/src/interfaces/wallet.cpp
index 349dce0247..cec75030ad 100644
--- a/src/interfaces/wallet.cpp
+++ b/src/interfaces/wallet.cpp
@@ -351,13 +351,13 @@ public:
         }
         return result;
     }
-    bool tryGetBalances(WalletBalances& balances, int& num_blocks) override
+    bool tryGetBalances(WalletBalances& balances, uint256& block_hash) override
     {
         TRY_LOCK(m_wallet->cs_wallet, locked_wallet);
         if (!locked_wallet) {
             return false;
         }
-        num_blocks = m_wallet->GetLastBlockHeight();
+        block_hash = m_wallet->GetLastBlockHash();
         balances = getBalances();
         return true;
     }
diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h
index 421d35af15..67569a3e55 100644
--- a/src/interfaces/wallet.h
+++ b/src/interfaces/wallet.h
@@ -203,7 +203,7 @@ public:
     virtual WalletBalances getBalances() = 0;
 
     //! Get balances if possible without blocking.
-    virtual bool tryGetBalances(WalletBalances& balances, int& num_blocks) = 0;
+    virtual bool tryGetBalances(WalletBalances& balances, uint256& block_hash) = 0;
 
     //! Get balance.
     virtual CAmount getBalance() = 0;
diff --git a/src/qt/clientmodel.cpp b/src/qt/clientmodel.cpp
index 159b0d3df3..f15921c5bc 100644
--- a/src/qt/clientmodel.cpp
+++ b/src/qt/clientmodel.cpp
@@ -114,6 +114,15 @@ int ClientModel::getNumBlocks() const
     return m_cached_num_blocks;
 }
 
+uint256 ClientModel::getBestBlockHash()
+{
+    LOCK(m_cached_tip_mutex);
+    if (m_cached_tip_blocks.IsNull()) {
+        m_cached_tip_blocks = m_node.getBestBlockHash();
+    }
+    return m_cached_tip_blocks;
+}
+
 void ClientModel::updateNumConnections(int numConnections)
 {
     Q_EMIT numConnectionsChanged(numConnections);
@@ -235,14 +244,15 @@ static void BannedListChanged(ClientModel *clientmodel)
     assert(invoked);
 }
 
-static void BlockTipChanged(ClientModel* clientmodel, SynchronizationState sync_state, int height, int64_t blockTime, double verificationProgress, bool fHeader)
+static void BlockTipChanged(ClientModel* clientmodel, SynchronizationState sync_state, interfaces::BlockTip tip, double verificationProgress, bool fHeader)
 {
     if (fHeader) {
         // cache best headers time and height to reduce future cs_main locks
-        clientmodel->cachedBestHeaderHeight = height;
-        clientmodel->cachedBestHeaderTime = blockTime;
+        clientmodel->cachedBestHeaderHeight = tip.block_height;
+        clientmodel->cachedBestHeaderTime = tip.block_time;
     } else {
-        clientmodel->m_cached_num_blocks = height;
+        clientmodel->m_cached_num_blocks = tip.block_height;
+        WITH_LOCK(clientmodel->m_cached_tip_mutex, clientmodel->m_cached_tip_blocks = tip.block_hash;);
     }
 
     // Throttle GUI notifications about (a) blocks during initial sync, and (b) both blocks and headers during reindex.
@@ -254,8 +264,8 @@ static void BlockTipChanged(ClientModel* clientmodel, SynchronizationState sync_
     }
 
     bool invoked = QMetaObject::invokeMethod(clientmodel, "numBlocksChanged", Qt::QueuedConnection,
-        Q_ARG(int, height),
-        Q_ARG(QDateTime, QDateTime::fromTime_t(blockTime)),
+        Q_ARG(int, tip.block_height),
+        Q_ARG(QDateTime, QDateTime::fromTime_t(tip.block_time)),
         Q_ARG(double, verificationProgress),
         Q_ARG(bool, fHeader),
         Q_ARG(SynchronizationState, sync_state));
@@ -271,8 +281,8 @@ void ClientModel::subscribeToCoreSignals()
     m_handler_notify_network_active_changed = m_node.handleNotifyNetworkActiveChanged(std::bind(NotifyNetworkActiveChanged, this, std::placeholders::_1));
     m_handler_notify_alert_changed = m_node.handleNotifyAlertChanged(std::bind(NotifyAlertChanged, this));
     m_handler_banned_list_changed = m_node.handleBannedListChanged(std::bind(BannedListChanged, this));
-    m_handler_notify_block_tip = m_node.handleNotifyBlockTip(std::bind(BlockTipChanged, this, std::placeholders::_1, std::placeholders::_2, std::placeholders::_3, std::placeholders::_4, false));
-    m_handler_notify_header_tip = m_node.handleNotifyHeaderTip(std::bind(BlockTipChanged, this, std::placeholders::_1, std::placeholders::_2, std::placeholders::_3, std::placeholders::_4, true));
+    m_handler_notify_block_tip = m_node.handleNotifyBlockTip(std::bind(BlockTipChanged, this, std::placeholders::_1, std::placeholders::_2, std::placeholders::_3, false));
+    m_handler_notify_header_tip = m_node.handleNotifyHeaderTip(std::bind(BlockTipChanged, this, std::placeholders::_1, std::placeholders::_2, std::placeholders::_3, true));
 }
 
 void ClientModel::unsubscribeFromCoreSignals()
diff --git a/src/qt/clientmodel.h b/src/qt/clientmodel.h
index ace77f5972..7f12cce1d9 100644
--- a/src/qt/clientmodel.h
+++ b/src/qt/clientmodel.h
@@ -10,6 +10,8 @@
 
 #include <atomic>
 #include <memory>
+#include <sync.h>
+#include <uint256.h>
 
 class BanTableModel;
 class CBlockIndex;
@@ -57,6 +59,7 @@ public:
     //! Return number of connections, default is in- and outbound (total)
     int getNumConnections(unsigned int flags = CONNECTIONS_ALL) const;
     int getNumBlocks() const;
+    uint256 getBestBlockHash();
     int getHeaderTipHeight() const;
     int64_t getHeaderTipTime() const;
 
@@ -74,11 +77,14 @@ public:
 
     bool getProxyInfo(std::string& ip_port) const;
 
-    // caches for the best header, number of blocks
+    // caches for the best header: hash, number of blocks and block time
     mutable std::atomic<int> cachedBestHeaderHeight;
     mutable std::atomic<int64_t> cachedBestHeaderTime;
     mutable std::atomic<int> m_cached_num_blocks{-1};
 
+    Mutex m_cached_tip_mutex;
+    uint256 m_cached_tip_blocks GUARDED_BY(m_cached_tip_mutex){};
+
 private:
     interfaces::Node& m_node;
     std::unique_ptr<interfaces::Handler> m_handler_show_progress;
diff --git a/src/qt/transactionrecord.cpp b/src/qt/transactionrecord.cpp
index a32d218fc9..01dff8069c 100644
--- a/src/qt/transactionrecord.cpp
+++ b/src/qt/transactionrecord.cpp
@@ -162,7 +162,7 @@ QList<TransactionRecord> TransactionRecord::decomposeTransaction(const interface
     return parts;
 }
 
-void TransactionRecord::updateStatus(const interfaces::WalletTxStatus& wtx, int numBlocks, int64_t block_time)
+void TransactionRecord::updateStatus(const interfaces::WalletTxStatus& wtx, const uint256& block_hash, int numBlocks, int64_t block_time)
 {
     // Determine transaction status
 
@@ -174,7 +174,7 @@ void TransactionRecord::updateStatus(const interfaces::WalletTxStatus& wtx, int
         idx);
     status.countsForBalance = wtx.is_trusted && !(wtx.blocks_to_maturity > 0);
     status.depth = wtx.depth_in_main_chain;
-    status.cur_num_blocks = numBlocks;
+    status.m_cur_block_hash = block_hash;
 
     const bool up_to_date = ((int64_t)QDateTime::currentMSecsSinceEpoch() / 1000 - block_time < MAX_BLOCK_TIME_GAP);
     if (up_to_date && !wtx.is_final) {
@@ -233,9 +233,9 @@ void TransactionRecord::updateStatus(const interfaces::WalletTxStatus& wtx, int
     status.needsUpdate = false;
 }
 
-bool TransactionRecord::statusUpdateNeeded(int numBlocks) const
+bool TransactionRecord::statusUpdateNeeded(const uint256& block_hash) const
 {
-    return status.cur_num_blocks != numBlocks || status.needsUpdate;
+    return status.m_cur_block_hash != block_hash || status.needsUpdate;
 }
 
 QString TransactionRecord::getTxHash() const
diff --git a/src/qt/transactionrecord.h b/src/qt/transactionrecord.h
index 3f64cefd09..c983c527c0 100644
--- a/src/qt/transactionrecord.h
+++ b/src/qt/transactionrecord.h
@@ -23,9 +23,8 @@ struct WalletTxStatus;
 class TransactionStatus
 {
 public:
-    TransactionStatus():
-        countsForBalance(false), sortKey(""),
-        matures_in(0), status(Unconfirmed), depth(0), open_for(0), cur_num_blocks(-1)
+    TransactionStatus() : countsForBalance(false), sortKey(""),
+                          matures_in(0), status(Unconfirmed), depth(0), open_for(0)
     { }
 
     enum Status {
@@ -61,8 +60,8 @@ public:
                       finalization */
     /**@}*/
 
-    /** Current number of blocks (to know whether cached status is still valid) */
-    int cur_num_blocks;
+    /** Current block hash (to know whether cached status is still valid) */
+    uint256 m_cur_block_hash{};
 
     bool needsUpdate;
 };
@@ -138,11 +137,11 @@ public:
 
     /** Update status from core wallet tx.
      */
-    void updateStatus(const interfaces::WalletTxStatus& wtx, int numBlocks, int64_t block_time);
+    void updateStatus(const interfaces::WalletTxStatus& wtx, const uint256& block_hash, int numBlocks, int64_t block_time);
 
     /** Return whether a status update is needed.
      */
-    bool statusUpdateNeeded(int numBlocks) const;
+    bool statusUpdateNeeded(const uint256& block_hash) const;
 };
 
 #endif // BITCOIN_QT_TRANSACTIONRECORD_H
diff --git a/src/qt/transactiontablemodel.cpp b/src/qt/transactiontablemodel.cpp
index 7a15503228..0867ae8105 100644
--- a/src/qt/transactiontablemodel.cpp
+++ b/src/qt/transactiontablemodel.cpp
@@ -176,7 +176,7 @@ public:
         return cachedWallet.size();
     }
 
-    TransactionRecord *index(interfaces::Wallet& wallet, const int cur_num_blocks, const int idx)
+    TransactionRecord* index(interfaces::Wallet& wallet, const uint256& cur_block_hash, const int idx)
     {
         if(idx >= 0 && idx < cachedWallet.size())
         {
@@ -192,8 +192,8 @@ public:
             interfaces::WalletTxStatus wtx;
             int numBlocks;
             int64_t block_time;
-            if (rec->statusUpdateNeeded(cur_num_blocks) && wallet.tryGetTxStatus(rec->hash, wtx, numBlocks, block_time)) {
-                rec->updateStatus(wtx, numBlocks, block_time);
+            if (rec->statusUpdateNeeded(cur_block_hash) && wallet.tryGetTxStatus(rec->hash, wtx, numBlocks, block_time)) {
+                rec->updateStatus(wtx, cur_block_hash, numBlocks, block_time);
             }
             return rec;
         }
@@ -664,7 +664,7 @@ QVariant TransactionTableModel::headerData(int section, Qt::Orientation orientat
 QModelIndex TransactionTableModel::index(int row, int column, const QModelIndex &parent) const
 {
     Q_UNUSED(parent);
-    TransactionRecord *data = priv->index(walletModel->wallet(), walletModel->getNumBlocks(), row);
+    TransactionRecord* data = priv->index(walletModel->wallet(), walletModel->clientModel().getBestBlockHash(), row);
     if(data)
     {
         return createIndex(row, column, data);
diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp
index f63ffd9018..671b5e1ce6 100644
--- a/src/qt/walletmodel.cpp
+++ b/src/qt/walletmodel.cpp
@@ -46,7 +46,6 @@ WalletModel::WalletModel(std::unique_ptr<interfaces::Wallet> wallet, ClientModel
     transactionTableModel(nullptr),
     recentRequestsTableModel(nullptr),
     cachedEncryptionStatus(Unencrypted),
-    cachedNumBlocks(0),
     timer(new QTimer(this))
 {
     fHaveWatchOnly = m_wallet->haveWatchOnly();
@@ -88,24 +87,23 @@ void WalletModel::pollBalanceChanged()
 {
     // Avoid recomputing wallet balances unless a TransactionChanged or
     // BlockTip notification was received.
-    if (!fForceCheckBalanceChanged && cachedNumBlocks == m_client_model->getNumBlocks()) return;
+    if (!fForceCheckBalanceChanged && m_cached_last_update_tip == m_client_model->getBestBlockHash()) return;
 
     // Try to get balances and return early if locks can't be acquired. This
     // avoids the GUI from getting stuck on periodical polls if the core is
     // holding the locks for a longer time - for example, during a wallet
     // rescan.
     interfaces::WalletBalances new_balances;
-    int numBlocks = -1;
-    if (!m_wallet->tryGetBalances(new_balances, numBlocks)) {
+    uint256 block_hash;
+    if (!m_wallet->tryGetBalances(new_balances, block_hash)) {
         return;
     }
 
-    if(fForceCheckBalanceChanged || numBlocks != cachedNumBlocks)
-    {
+    if (fForceCheckBalanceChanged || block_hash != m_cached_last_update_tip) {
         fForceCheckBalanceChanged = false;
 
         // Balance and number of transactions might have changed
-        cachedNumBlocks = numBlocks;
+        m_cached_last_update_tip = block_hash;
 
         checkBalanceChanged(new_balances);
         if(transactionTableModel)
diff --git a/src/qt/walletmodel.h b/src/qt/walletmodel.h
index 7bd6d9cde0..38e8a14556 100644
--- a/src/qt/walletmodel.h
+++ b/src/qt/walletmodel.h
@@ -144,8 +144,8 @@ public:
 
     interfaces::Node& node() const { return m_node; }
     interfaces::Wallet& wallet() const { return *m_wallet; }
+    ClientModel& clientModel() const { return *m_client_model; }
     void setClientModel(ClientModel* client_model);
-    int getNumBlocks() const { return cachedNumBlocks; }
 
     QString getWalletName() const;
     QString getDisplayName() const;
@@ -181,9 +181,11 @@ private:
     // Cache some values to be able to detect changes
     interfaces::WalletBalances m_cached_balances;
     EncryptionStatus cachedEncryptionStatus;
-    int cachedNumBlocks;
     QTimer* timer;
 
+    // Block hash denoting when the last balance update was done.
+    uint256 m_cached_last_update_tip{};
+
     void subscribeToCoreSignals();
     void unsubscribeFromCoreSignals();
     void checkBalanceChanged(const interfaces::WalletBalances& new_balances);