This fixes the case where transaction notifications arrive between getWalletTxs and subscribeToCoreSignals. Basically notifications are queued until getWalletTxs and wallet rescan complete.
This is also a requirement to call getWalletTxs in a background thread.
Motivated by https://github.com/bitcoin/bitcoin/issues/20241.
This fixes the case where transaction notifications arrive between
getWalletTxsandsubscribeToCoreSignals.
Yes, the current behavior is indeed buggy. It is easy to verify by patching TransactionTableModel ctor with a delay before subscribeToCoreSignals call.
94@@ -95,25 +95,27 @@ class TransactionTablePriv
95 */
96 QList<TransactionRecord> cachedWallet;
97
98- bool fQueueNotifications = false;
99+ bool m_loaded = false;
100+ int m_progress = 100;
int when we are only interested in bool if progress < 100?
108 */
109 void refreshWallet(interfaces::Wallet& wallet)
110 {
111- qDebug() << "TransactionTablePriv::refreshWallet";
112- cachedWallet.clear();
113+ assert(cachedWallet.empty());
With new logic I’d prefer to assert(!m_loaded); or
0 assert(cachedWallet.empty() && !m_loaded);
106
107 /* Query entire wallet anew from core.
108 */
109 void refreshWallet(interfaces::Wallet& wallet)
110 {
111- qDebug() << "TransactionTablePriv::refreshWallet";
refreshWallet is called? Seems one of those logs you add when developing, but now it’s useless. I’ll revert this change if you think otherwise. It’s unrelated to this PR but committed since I was touching nearby code.
To know when
refreshWalletis called?
Rather to know that refreshWallet is called, for example for old wallets that were not synced for a long time, no?
TransactionTableModel constructor.
But it’s called unconditionally in
TransactionTableModelconstructor.
Right. But it is an implementation detail that (as we assume) is hidden from users.
Anyway, feel free to ignore this concern and mark it as resolved :)
But it is an implementation detail that (as we assume) is hidden from users.
Right, so why log it? Following the same idea there are many things we could also log.
Code review ACK 5d7d36344c1241fb940766905265747492570446. This looks good but is a pretty confusing change. I think it could be clearer split up into two commits: one pure refactoring commit replacing ShowProgress with DispatchNotifications, and another commit changing initialization order moving the subscribeToCoreSignals call before the refreshWallet call and adding the new DispatchNotifications call at the end of the refreshWallet implementation.
I will say I don’t fully understand the motivation for the two refreshMethod related changes. They seem safe but I don’t know if they are actually changing behavior now or just setting something up for the future. If they were moved to standalone commit, purpose of the changes could be made more clear either in the commit description or code comments.
In case it helps other reviewers, I found the refactoring changes replacing ShowProgress with DispatchNotifications easier to understand after ignoring whitespace and rereading a previous explanation of the code #120 (review).
Hebasto’s suggestions also all seem reasonable and good. Basically a lot could be made clearer here, but this PR does seems fine to merge in its current form.
This PR might be interesting to people dealing with large wallets that have a lot of transactions, since it seems like it could help move to loading transactions asynchronously without locking up the GUI.
This is also a requirement to call getWalletTxs in a background thread.
Concept ACK, thanks for working on that.
95@@ -96,25 +96,29 @@ class TransactionTablePriv
96 */
97 QList<TransactionRecord> cachedWallet;
98
99- bool fQueueNotifications = false;
100+ /** True when model finishes loading all wallet transaction on start */
95@@ -96,25 +96,29 @@ class TransactionTablePriv
96 */
97 QList<TransactionRecord> cachedWallet;
98
99- bool fQueueNotifications = false;
100+ /** True when model finishes loading all wallet transaction on start */
101+ bool m_loaded = false;
102+ /** True when transactions are being notified, for instance when scanning */
103+ int m_loading = false;
(maybe a leftover from an initial version?) this can work but unless I’m misreading m_loading will only be set to true or false (1 or 0)
0 bool m_loading = false;
Review with --ignore-all-space
119 if (TransactionRecord::showTransaction()) {
120 cachedWallet.append(TransactionRecord::decomposeTransaction(wtx));
121 }
122 }
123 }
124+ m_loaded = true;
3bccd50ad2f38 nit, can drop extra brackets
0diff --git a/src/qt/transactiontablemodel.cpp b/src/qt/transactiontablemodel.cpp
1index 0175c88e7..8dc6d4c40 100644
2--- a/src/qt/transactiontablemodel.cpp
3+++ b/src/qt/transactiontablemodel.cpp
4@@ -110,11 +110,9 @@ public:
5 void refreshWallet(interfaces::Wallet& wallet)
6 {
7 assert(!m_loaded);
8- {
9- for (const auto& wtx : wallet.getWalletTxs()) {
10- if (TransactionRecord::showTransaction()) {
11- cachedWallet.append(TransactionRecord::decomposeTransaction(wtx));
12- }
13+ for (const auto& wtx : wallet.getWalletTxs()) {
14+ if (TransactionRecord::showTransaction()) {
15+ cachedWallet.append(TransactionRecord::decomposeTransaction(wtx));
16 }
17 }
18 m_loaded = true;
git show cafef080a2e --ignore-all-space)
743+ if (vQueueNotifications.size() > 10) { // prevent balloon spam, show maximum 10 balloons
744+ bool invoked = QMetaObject::invokeMethod(parent, "setProcessingQueuedTransactions", Qt::QueuedConnection, Q_ARG(bool, true));
745+ assert(invoked);
746+ }
747+ for (unsigned int i = 0; i < vQueueNotifications.size(); ++i)
748 {
cafef080a2e59c2 nit, while removing the brackets in this commit could also
0diff --git a/src/qt/transactiontablemodel.cpp b/src/qt/transactiontablemodel.cpp
1index 0175c88e7..734605a47 100644
2--- a/src/qt/transactiontablemodel.cpp
3+++ b/src/qt/transactiontablemodel.cpp
4@@ -739,8 +739,7 @@ void TransactionTablePriv::DispatchNotifications()
5 bool invoked = QMetaObject::invokeMethod(parent, "setProcessingQueuedTransactions", Qt::QueuedConnection, Q_ARG(bool, true));
6 assert(invoked);
7 }
8- for (unsigned int i = 0; i < vQueueNotifications.size(); ++i)
9- {
10+ for (unsigned int i = 0; i < vQueueNotifications.size(); ++i) {
tACK cafef080a2e59c2bcae6baeee3c3c7e187e931ae
happy to re-ACK for the above
95@@ -96,25 +96,29 @@ class TransactionTablePriv
96 */
97 QList<TransactionRecord> cachedWallet;
98
99- bool fQueueNotifications = false;
100+ /** True when model finishes loading all wallet transactions on start */
101+ bool m_loaded = false;
102+ /** True when transactions are being notified, for instance when scanning */
It could be just my bad English, but it seems should be “… are not being notified”, no?
If I’m correct, maybe re-word the comment in positive one?
Or maybe we think about different notifications: (a) from the node vs (b) to a user?
In commit “qt: Refactor ShowProgress to DispatchNotifications” (c6cbdf1a90a253fef0259b365a782bf88cd437f2)
re: #121 (review)
Right, I’ll rephrase.
If you’re still planning to update this, my attempt at a comment would be: “This will be true when blocks in the chain are being scanned for new transactions, and transaction notification popups should be suppressed until the scan is done. It can be true even after m_loaded is true if a rescan has been triggered.” I think I would also call this m_delay_notifications instead of m_loading because it can be true when m_loaded is also true, but feel free to stick with whatever name you prefer.
103+ bool m_loading = false;
104 std::vector< TransactionNotification > vQueueNotifications;
105
106 void NotifyTransactionChanged(const uint256 &hash, ChangeType status);
107- void ShowProgress(const std::string &title, int nProgress);
108+ void DispatchNotifications();
722@@ -719,44 +723,42 @@ void TransactionTablePriv::NotifyTransactionChanged(const uint256 &hash, ChangeT
723
724 TransactionNotification notification(hash, status, showTransaction);
725
726- if (fQueueNotifications)
727+ if (!m_loaded || m_loading)
728 {
style nit:
0 if (!m_loaded || m_loading) {
Code review ACK cafef080a2e59c2bcae6baeee3c3c7e187e931ae. Only change since last review is splitting commits and replacing m_progress with m_loading.
Thanks for the updates! I think this change is much easier to understand now.
It would be good to clarify in the PR description what exactly is meant by “This fixes the case where transaction notifications arrive between getWalletTxs and subscribeToCoreSignals.” It’s not clear from the description what the old behavior was and what the new behavior is.
IIUC, the problem was that notifications about new transactions discovered while the wallet was being loaded were dropped previously, and now they are shown?
