gui: Avoid redundant tx status updates

ryanofsky commented at 8:38 pm on January 10, 2020: member

This PR is part of the process separation project.

In TransactionTablePriv::index, avoid calling interfaces::Wallet::tryGetTxStatus if the status is up to date as of the most recent NotifyBlockTip notification. Store height from the most recent notification in a new ClientModel::cachedNumBlocks variable in order to check this.

This avoids floods of IPC traffic from tryGetTxStatus with #10102 when there are a lot of transactions. It might also make the GUI a little more efficient even when there is no IPC.

In TransactionTablePriv::index, avoid calling
interfaces::Wallet::tryGetTxStatus if the status is up to date as of the most
recent NotifyBlockTip notification.  Store height from the most recent
notification in a new ClientModel::cachedNumBlocks variable in order to check
this.

This avoids floods of IPC traffic from tryGetTxStatus with #10102 when there
are a lot of transactions. It might also make the GUI a little more efficient
even when there is no IPC.

96cb597325

promag commented at 9:28 pm on January 10, 2020: member

Concept ACK.

fanquake added the label GUI on Jan 10, 2020

DrahtBot commented at 10:46 pm on January 10, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17877 (qt, refactor: Make enums in BitcoinUnits class scoped by hebasto)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

in src/qt/transactiontablemodel.cpp:670 in 2b3d6bf0e4 outdated

664@@ -663,10 +665,10 @@ QVariant TransactionTableModel::headerData(int section, Qt::Orientation orientat
665 QModelIndex TransactionTableModel::index(int row, int column, const QModelIndex &parent) const
666 {
667     Q_UNUSED(parent);
668-    TransactionRecord *data = priv->index(walletModel->wallet(), row);
669+    TransactionRecord *data = priv->index(walletModel->wallet(), clientModel->getNumBlocks(), row);
670     if(data)
671     {
672-        return createIndex(row, column, priv->index(walletModel->wallet(), row));
673+        return createIndex(row, column, data);

promag commented at 6:07 pm on January 12, 2020:

Well this change alone already improves!

in src/qt/transactiontablemodel.cpp:179 in 2b3d6bf0e4 outdated

175@@ -175,7 +176,7 @@ class TransactionTablePriv
176         return cachedWallet.size();
177     }
178 
179-    TransactionRecord *index(interfaces::Wallet& wallet, int idx)
180+    TransactionRecord *index(interfaces::Wallet& wallet, int cur_num_blocks, int idx)

promag commented at 6:12 pm on January 12, 2020:

nit, use numBlocks and drop L193? Or make this const.

ryanofsky commented at 9:46 pm on January 13, 2020:

re: #17905 (review)

nit, use numBlocks and drop L193? Or make this const.

Declared const

promag commented at 6:38 pm on January 12, 2020: member

Code review ACK 2b3d6bf0e4e8a815b960d82c07f3b94cc753bd28. Restarted the travis job, I think it’s an unrelated failure:

 0interfaces/node.cpp:167:51: runtime error: implicit conversion from type 'unsigned long' of value 13744632839234567870 (64-bit, unsigned) to type 'int64_t' (aka 'long') changed the value to -4702111234474983746 (64-bit, signed)
 1    [#0](/bitcoin-bitcoin/0/) 0x55b40a49492c in interfaces::(anonymous namespace)::NodeImpl::getTotalBytesRecv() /home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-pc-linux-gnu/src/interfaces/node.cpp:167:51
 2    [#1](/bitcoin-bitcoin/1/) 0x55b40a02986f in ClientModel::ClientModel(interfaces::Node&, OptionsModel*, QObject*)::$_0::operator()() const /home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-pc-linux-gnu/src/qt/clientmodel.cpp:48:36
 3    [#2](/bitcoin-bitcoin/2/) 0x55b40a02986f in QtPrivate::FunctorCall<QtPrivate::IndexesList<>, QtPrivate::List<>, void, ClientModel::ClientModel(interfaces::Node&, OptionsModel*, QObject*)::$_0>::call(ClientModel::ClientModel(interfaces::Node&, OptionsModel*, QObject*)::$_0&, void**) /usr/include/x86_64-linux-gnu/qt5/QtCore/qobjectdefs_impl.h:130
 4    [#3](/bitcoin-bitcoin/3/) 0x55b40a029404 in void QtPrivate::Functor<ClientModel::ClientModel(interfaces::Node&, OptionsModel*, QObject*)::$_0, 0>::call<QtPrivate::List<>, void>(ClientModel::ClientModel(interfaces::Node&, OptionsModel*, QObject*)::$_0&, void*, void**) /usr/include/x86_64-linux-gnu/qt5/QtCore/qobjectdefs_impl.h:240:13
 5    [#4](/bitcoin-bitcoin/4/) 0x55b40a029404 in QtPrivate::QFunctorSlotObject<ClientModel::ClientModel(interfaces::Node&, OptionsModel*, QObject*)::$_0, 0, QtPrivate::List<>, void>::impl(int, QtPrivate::QSlotObjectBase*, QObject*, void**, bool*) /usr/include/x86_64-linux-gnu/qt5/QtCore/qobject_impl.h:168
 6    [#5](/bitcoin-bitcoin/5/) 0x7f6de37ee75e in QMetaObject::activate(QObject*, int, int, void**) (/usr/lib/x86_64-linux-gnu/libQt5Core.so.5+0x2b975e)
 7    [#6](/bitcoin-bitcoin/6/) 0x7f6de37fb0b6 in QTimer::timeout(QTimer::QPrivateSignal) (/usr/lib/x86_64-linux-gnu/libQt5Core.so.5+0x2c60b6)
 8    [#7](/bitcoin-bitcoin/7/) 0x7f6de37fb417 in QTimer::timerEvent(QTimerEvent*) (/usr/lib/x86_64-linux-gnu/libQt5Core.so.5+0x2c6417)
 9    [#8](/bitcoin-bitcoin/8/) 0x7f6de37ef16a in QObject::event(QEvent*) (/usr/lib/x86_64-linux-gnu/libQt5Core.so.5+0x2ba16a)
10    [#9](/bitcoin-bitcoin/9/) 0x7f6de287c83b in QApplicationPrivate::notify_helper(QObject*, QEvent*) (/usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x15483b)
11    [#10](/bitcoin-bitcoin/10/) 0x7f6de2884103 in QApplication::notify(QObject*, QEvent*) (/usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x15c103)
12    [#11](/bitcoin-bitcoin/11/) 0x7f6de37bf9c7 in QCoreApplication::notifyInternal2(QObject*, QEvent*) (/usr/lib/x86_64-linux-gnu/libQt5Core.so.5+0x28a9c7)
13    [#12](/bitcoin-bitcoin/12/) 0x7f6de3817e1d in QTimerInfoList::activateTimers() (/usr/lib/x86_64-linux-gnu/libQt5Core.so.5+0x2e2e1d)
14    [#13](/bitcoin-bitcoin/13/) 0x7f6de38185e0  (/usr/lib/x86_64-linux-gnu/libQt5Core.so.5+0x2e35e0)
15    [#14](/bitcoin-bitcoin/14/) 0x7f6dde8dd416 in g_main_context_dispatch (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x4c416)
16    [#15](/bitcoin-bitcoin/15/) 0x7f6dde8dd64f  (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x4c64f)
17    [#16](/bitcoin-bitcoin/16/) 0x7f6dde8dd6db in g_main_context_iteration (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x4c6db)
18    [#17](/bitcoin-bitcoin/17/) 0x7f6de381897e in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) (/usr/lib/x86_64-linux-gnu/libQt5Core.so.5+0x2e397e)
19    [#18](/bitcoin-bitcoin/18/) 0x7f6de37bd9f9 in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) (/usr/lib/x86_64-linux-gnu/libQt5Core.so.5+0x2889f9)
20    [#19](/bitcoin-bitcoin/19/) 0x7f6de35dc239 in QThread::exec() (/usr/lib/x86_64-linux-gnu/libQt5Core.so.5+0xa7239)
21    [#20](/bitcoin-bitcoin/20/) 0x7f6de35e117c  (/usr/lib/x86_64-linux-gnu/libQt5Core.so.5+0xac17c)
22    [#21](/bitcoin-bitcoin/21/) 0x7f6de41166da in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x76da)
23    [#22](/bitcoin-bitcoin/22/) 0x7f6de036988e in clone (/lib/x86_64-linux-gnu/libc.so.6+0x12188e)
24SUMMARY: UndefinedBehaviorSanitizer: implicit-integer-sign-change interfaces/node.cpp:167:51 in 
25FAIL qt/test/test_bitcoin-qt (exit status: 1)

in src/qt/test/addressbooktests.cpp:7 in 2b3d6bf0e4 outdated

4@@ -5,6 +5,7 @@
5 #include <interfaces/chain.h>
6 #include <interfaces/node.h>
7 #include <qt/editaddressdialog.h>
8+#include <qt/clientmodel.h>

emilengler commented at 2:59 pm on January 13, 2020:

nit: Move this one line up, alphabetical order

ryanofsky commented at 9:41 pm on January 13, 2020:

re: #17905 (review)

nit: Move this one line up, alphabetical order

Done

ryanofsky force-pushed on Jan 16, 2020

ryanofsky commented at 4:54 pm on January 16, 2020: member

Updated 2b3d6bf0e4e8a815b960d82c07f3b94cc753bd28 -> 26cde384af1d4790e17760e87b26e1c4ba51d5fd (pr/ipc-txup.1 -> pr/ipc-txup.2, compare) with suggested changes

Restarted the travis job, I think it’s an unrelated failure

Travis failure was just the #17906 failure. Should be fixed now that is merged

laanwj commented at 6:35 pm on January 22, 2020: member

Concept ACK

One remark: Is block height enough as an identifier of the current state? Might this miss a 1-block reorg that ends up in the same height? If so, wouldn’t “tip blockhash” be better for this?

ryanofsky commented at 7:21 pm on January 22, 2020: member

One remark: Is block height enough as an identifier of the current state? Might this miss a 1-block reorg that ends up in the same height? If so, wouldn’t “tip blockhash” be better for this?

This is true, and if you’d like I could add a followup commit or open a new PR to address this. But the current PR isn’t changing anything there because updateStatus is already never called when statusUpdateNeeded returns false. The only thing this PR does is additionally avoid wasted slow wallet.tryGetTxStatus calls.

in src/qt/transactiontablemodel.h:89 in 26cde384af outdated

85@@ -85,6 +86,7 @@ class TransactionTableModel : public QAbstractTableModel
86 
87 private:
88     WalletModel *walletModel;
89+    ClientModel *clientModel;

promag commented at 7:03 pm on January 26, 2020:

nit, ClientModel* const m_client_model;

ryanofsky commented at 9:27 pm on February 7, 2020:

nit, ClientModel* const m_client_model;

Updated and switched to reference

promag commented at 7:08 pm on January 26, 2020: member

ClientModel gives access to interfaces::Node& and OptionsModel* so maybe take advantage of this and simplify the constructors.

promag commented at 5:02 pm on February 4, 2020: member

ClientModel gives access to interfaces::Node& and OptionsModel* so maybe take advantage of this and simplify the constructors. @ryanofsky unless this makes breaking circular dependencies harder.

ryanofsky force-pushed on Feb 10, 2020

ryanofsky commented at 4:53 pm on February 10, 2020: member

Thanks for suggestions!

Updated 26cde384af1d4790e17760e87b26e1c4ba51d5fd -> 689424a679502c3ce1742538279223db83cea645 (pr/ipc-txup.2 -> pr/ipc-txup.3, compare) implementing suggestions

in src/qt/clientmodel.h:80 in 689424a679 outdated

73@@ -73,9 +74,10 @@ class ClientModel : public QObject
74 
75     bool getProxyInfo(std::string& ip_port) const;
76 
77-    // caches for the best header
78+    // caches for the best header, number of blocks
79     mutable std::atomic<int> cachedBestHeaderHeight;
80     mutable std::atomic<int64_t> cachedBestHeaderTime;
81+    mutable std::atomic<int> m_cached_num_blocks;

promag commented at 9:34 pm on February 26, 2020:

nit

0    mutable std::atomic<int> m_cached_num_blocks{-1};

ryanofsky commented at 10:03 pm on February 26, 2020:

re: #17905 (review)

nit

Added

in src/qt/clientmodel.cpp:111 in 689424a679 outdated

105@@ -105,6 +106,14 @@ int64_t ClientModel::getHeaderTipTime() const
106     return cachedBestHeaderTime;
107 }
108 
109+int ClientModel::getNumBlocks() const
110+{
111+    if (m_cached_num_blocks == -1) {
112+        m_cached_num_blocks = m_node.getNumBlocks();

promag commented at 9:44 pm on February 26, 2020:

Maybe just initialize m_cached_num_blocks after subscribeToCoreSignals() and drop the -1 case?

ryanofsky commented at 10:03 pm on February 26, 2020:

re: #17905 (review)

Maybe just initialize m_cached_num_blocks after subscribeToCoreSignals() and drop the -1 case?

It seems a little more fragile to rely on another code path being executed if this is going to be initialized to -1. Also it’s more consistent with existing getHeaderTipHeight and getHeaderTipTime to be checking this way

promag commented at 10:18 pm on February 26, 2020:

I mean this:

0     m_thread->start();
1
2     subscribeToCoreSignals();
3+    m_cached_num_blocks = m_node.getNumBlocks();
4 }
5
6 ClientModel::~ClientModel()

It’s the constructor so it’s always executed?

ryanofsky commented at 3:54 pm on February 27, 2020:

re: #17905 (review)

Thanks again for the review!

I mean this:

Yes, I did understand the suggestion. I like having getNumBlocks implemented the same way as getHeaderTipHeight and getHeaderTipTime, and I like m_cached_num_blocks being touched as few places as possible (initialized to -1 and updated only as needed) and not needing to interact with the more complicated constructor.

I also think this case is similar to #18160 where I am trying to avoid an unnecessary interface method call (in that case interfaces::Wallet::tryGetBalances, in this case interfaces::Node::getNumBlocks), because I see these calls as potentially costly (with #10102 requiring serializing arguments, sending them across a socket and optionally logging them to debug.log, waiting for reply on the socket, deserializing and optionally logging that), but I think you see them more like ordinary method calls.

If other reviewers agree with your suggestion, though, I’m ok with changing this. I don’t think it is very significant.

Meta point: When I leave comment on PRs I try be be explicit about (1) What my suggestion is (2) What the benefits of the suggestion are or underlying problem is.

Some reviewers are great at saying what they want but not why they want it, others are great at finding things that should be improved without being specific enough about what kind of change they want to see. In this case, I understand the suggestion but don’t understand what’s driving it. Readability, avoiding a potential bug, efficiency, or just style?

promag commented at 4:03 pm on February 27, 2020:

Sorry for not explaining my POV. I’m suggesting to just make ClientModel::getNumBlocks lock free - not sure what would be the first call stack hitting the lock and if it would hurt the event loop, but with my suggestion that wouldn’t be a concern. But like you say, it’s not very significant so feel free to ignore it, FWIW I did ACK.

promag commented at 9:47 pm on February 26, 2020: member

Code review ACK 689424a679502c3ce1742538279223db83cea645.

Just two comments for your consideration.

ryanofsky force-pushed on Feb 26, 2020

ryanofsky commented at 10:14 pm on February 26, 2020: member

Updated 689424a679502c3ce1742538279223db83cea645 -> 96cb597325f64cadb3cf43e2cdb3d7c1e2e49891 (pr/ipc-txup.3 -> pr/ipc-txup.4, compare) with suggested change

promag commented at 5:30 pm on March 2, 2020: member

Code review ACK 96cb597325f64cadb3cf43e2cdb3d7c1e2e49891.

ryanofsky added this to the "In progress" column in a project

hebasto commented at 6:54 pm on March 23, 2020: member

Concept ACK.

hebasto approved

hebasto commented at 5:25 am on March 29, 2020: member

ACK 96cb597325f64cadb3cf43e2cdb3d7c1e2e49891

laanwj referenced this in commit f2880e21ef on Mar 31, 2020

sidhujag referenced this in commit b857234f16 on Mar 31, 2020

ryanofsky referenced this in commit 31c732f036 on Apr 10, 2020

ryanofsky commented at 6:17 pm on April 10, 2020: member

This PR might be mergeable: has 2 acks, is a pretty limited gui change

MarcoFalke merged this on Apr 10, 2020

MarcoFalke closed this on Apr 10, 2020

ryanofsky moved this from the "In progress" to the "Done" column in a project

sidhujag referenced this in commit c486c7458a on Apr 13, 2020

ryanofsky referenced this in commit 148ac68174 on Apr 25, 2020

ryanofsky referenced this in commit 83f69fab3a on May 1, 2020

ryanofsky referenced this in commit 86883eaeb2 on May 1, 2020

ryanofsky referenced this in commit e0886b9da3 on May 1, 2020

ryanofsky referenced this in commit 6642b2a237 on May 8, 2020

ryanofsky commented at 3:53 pm on May 18, 2020: member

re: @laanwj #17905 (comment)

One remark: Is block height enough as an identifier of the current state? Might this miss a 1-block reorg that ends up in the same height? If so, wouldn’t “tip blockhash” be better for this?

This issue is finally fixed in #17993 by @furszy, which cleanly replaces cur_num_blocks comparisons here and other places with cur_block_hash comparisons

jonasschnelli referenced this in commit f4b603cff6 on May 29, 2020

sidhujag referenced this in commit 5b92706601 on May 31, 2020

Fabcien referenced this in commit 1d7377e74d on Jan 11, 2021

PhotoshiNakamoto referenced this in commit 89effc4ae5 on Dec 11, 2021

DrahtBot locked this on Feb 15, 2022

gui: Avoid redundant tx status updates #17905

Conflicts