Early subscribe core signals in transaction table model #121

promag · 2020-10-28T02:01:49Z

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 bitcoin/bitcoin#20241.

promag · 2020-10-28T18:22:09Z

Please ignore whitespaces when reviewing.

promag · 2020-11-01T13:01:13Z

Rebased due to conflict with updated #120.

hebasto · 2020-12-16T15:04:51Z

Concept ACK.

hebasto · 2021-01-04T17:15:46Z

This fixes the case where transaction notifications arrive between getWalletTxs and subscribeToCoreSignals.

Yes, the current behavior is indeed buggy. It is easy to verify by patching TransactionTableModel ctor with a delay before subscribeToCoreSignals call.

hebasto

Approach ACK 5d7d363.

src/qt/transactiontablemodel.cpp

ryanofsky

Code review ACK 5d7d363. 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 (comment).

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.

promag

Thanks for reviewing, I'll improve readability.

src/qt/transactiontablemodel.cpp

laanwj · 2021-01-27T19:17:18Z

This is also a requirement to call getWalletTxs in a background thread.

Concept ACK, thanks for working on that.

hebasto

ACK 5d7d363, agree with #121 (review)

this PR does seems fine to merge in its current form.

promag

Rebased and addressed @hebasto review.

src/qt/transactiontablemodel.cpp

jonatack

Approach ACK modulo that it's a fairly confusing change and separating into more commits may help here (as mentioned above).

src/qt/transactiontablemodel.cpp

Review with --ignore-all-space

promag · 2021-04-28T23:21:11Z

Ok, I've tried to split in decent commits.

jonatack · 2021-04-29T17:55:02Z

src/qt/transactiontablemodel.cpp

                }
            }
        }
+        m_loaded = true;


3bccd50 nit, can drop extra brackets

diff --git a/src/qt/transactiontablemodel.cpp b/src/qt/transactiontablemodel.cpp index 0175c88e7..8dc6d4c40 100644 --- a/src/qt/transactiontablemodel.cpp +++ b/src/qt/transactiontablemodel.cpp @@ -110,11 +110,9 @@ public: void refreshWallet(interfaces::Wallet& wallet) { assert(!m_loaded); - { - for (const auto& wtx : wallet.getWalletTxs()) { - if (TransactionRecord::showTransaction()) { - cachedWallet.append(TransactionRecord::decomposeTransaction(wtx)); - } + for (const auto& wtx : wallet.getWalletTxs()) { + if (TransactionRecord::showTransaction()) { + cachedWallet.append(TransactionRecord::decomposeTransaction(wtx)); } } m_loaded = true;

(similar to the change in the last commit, git show cafef080a2e --ignore-all-space)

jonatack · 2021-04-29T18:21:34Z

src/qt/transactiontablemodel.cpp

+        assert(invoked);
+    }
+    for (unsigned int i = 0; i < vQueueNotifications.size(); ++i)
    {


cafef08 nit, while removing the brackets in this commit could also

diff --git a/src/qt/transactiontablemodel.cpp b/src/qt/transactiontablemodel.cpp index 0175c88e7..734605a47 100644 --- a/src/qt/transactiontablemodel.cpp +++ b/src/qt/transactiontablemodel.cpp @@ -739,8 +739,7 @@ void TransactionTablePriv::DispatchNotifications() bool invoked = QMetaObject::invokeMethod(parent, "setProcessingQueuedTransactions", Qt::QueuedConnection, Q_ARG(bool, true)); assert(invoked); } - for (unsigned int i = 0; i < vQueueNotifications.size(); ++i) - { + for (unsigned int i = 0; i < vQueueNotifications.size(); ++i) {

jonatack

tACK cafef08

happy to re-ACK for the above

hebasto

Thanks for splitting commits! Much easier to review.

hebasto · 2021-04-29T18:33:13Z

src/qt/transactiontablemodel.cpp

-    bool fQueueNotifications = false;
+    /** True when model finishes loading all wallet transactions on start */
+    bool m_loaded = false;
+    /** 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?

Right, I'll rephrase.

In commit "qt: Refactor ShowProgress to DispatchNotifications" (c6cbdf1)

re: #121 (comment)

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.

hebasto · 2021-04-29T18:33:30Z

src/qt/transactiontablemodel.cpp


    void NotifyTransactionChanged(const uint256 &hash, ChangeType status);
-    void ShowProgress(const std::string &title, int nProgress);
+    void DispatchNotifications();


A doxygen comment?

hebasto · 2021-04-29T18:34:20Z

src/qt/transactiontablemodel.cpp

+    if (!m_loaded || m_loading)
    {


style nit:

Suggested change

if (!m_loaded || m_loading)

{

if (!m_loaded || m_loading) {

ryanofsky

Code review ACK cafef08. 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?

ryanofsky · 2021-05-04T01:06:18Z

src/qt/transactiontablemodel.cpp

-    bool fQueueNotifications = false;
+    /** True when model finishes loading all wallet transactions on start */
+    bool m_loaded = false;
+    /** True when transactions are being notified, for instance when scanning */


In commit "qt: Refactor ShowProgress to DispatchNotifications" (c6cbdf1)

re: #121 (comment)

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.

meshcollider

Code review ACK cafef08

promag force-pushed the 2020-10-missing-transaction-notifications branch from fc0803b to 9267d2a Compare October 28, 2020 11:46

promag mentioned this pull request Oct 28, 2020

Call interfaces::Wallet::getWalletTxs asynchronous bitcoin/bitcoin#20241

Closed

promag force-pushed the 2020-10-missing-transaction-notifications branch from 9267d2a to 8b65985 Compare November 1, 2020 13:00

promag requested review from hebasto, jonasschnelli and ryanofsky November 12, 2020 18:08

promag force-pushed the 2020-10-missing-transaction-notifications branch from 8b65985 to 0b0d91e Compare November 12, 2020 18:11

fanquake mentioned this pull request Dec 2, 2020

Unexpected test fail: wallet/wallet.cpp:3113:51: runtime error: division by zero bitcoin/bitcoin#20542

Closed

promag force-pushed the 2020-10-missing-transaction-notifications branch from 0b0d91e to 5d7d363 Compare December 2, 2020 08:58

hebasto requested changes Jan 4, 2021

View reviewed changes

src/qt/transactiontablemodel.cpp Outdated Show resolved Hide resolved

src/qt/transactiontablemodel.cpp Outdated Show resolved Hide resolved

src/qt/transactiontablemodel.cpp Outdated Show resolved Hide resolved

src/qt/transactiontablemodel.cpp Show resolved Hide resolved

ryanofsky approved these changes Jan 25, 2021

View reviewed changes

promag commented Jan 27, 2021

View reviewed changes

src/qt/transactiontablemodel.cpp Show resolved Hide resolved

hebasto approved these changes Mar 24, 2021

View reviewed changes

hebasto added the Bug Something isn't working label Mar 25, 2021

promag force-pushed the 2020-10-missing-transaction-notifications branch from 5d7d363 to 8f16516 Compare April 19, 2021 12:10

promag commented Apr 19, 2021

View reviewed changes

src/qt/transactiontablemodel.cpp Outdated Show resolved Hide resolved

src/qt/transactiontablemodel.cpp Outdated Show resolved Hide resolved

src/qt/transactiontablemodel.cpp Outdated Show resolved Hide resolved

hebasto added the Wallet label Apr 19, 2021

hebasto changed the title ~~qt: Early subscribe core signals in transaction table model~~ Early subscribe core signals in transaction table model Apr 19, 2021

jonatack reviewed Apr 23, 2021

View reviewed changes

src/qt/transactiontablemodel.cpp Outdated Show resolved Hide resolved

src/qt/transactiontablemodel.cpp Outdated Show resolved Hide resolved

promag added 4 commits April 28, 2021 23:58

qt: Set flag after inital load on transaction table model

3bccd50

qt: Refactor ShowProgress to DispatchNotifications

c6cbdf1

qt: Early subscribe core signals in transaction table model

57785fb

qt: Refactor to remove unnecessary block in DispatchNotifications

cafef08

Review with --ignore-all-space

promag force-pushed the 2020-10-missing-transaction-notifications branch from 8f16516 to cafef08 Compare April 28, 2021 23:19

jonatack reviewed Apr 29, 2021

View reviewed changes

hebasto reviewed Apr 29, 2021

View reviewed changes

ryanofsky approved these changes May 4, 2021

View reviewed changes

meshcollider reviewed May 25, 2021

View reviewed changes

hebasto merged commit 3ec033e into bitcoin-core:master May 26, 2021

promag deleted the 2020-10-missing-transaction-notifications branch May 26, 2021 10:38

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 27, 2021

Merge bitcoin-core/gui#121: Early subscribe core signals in transacti…

8bd2ce3

gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022

Merge 3ec033e into merged_master (Bitcoin PR bitcoin-core/gui#121)

2893856

bitcoin-core locked as resolved and limited conversation to collaborators Aug 16, 2022

Early subscribe core signals in transaction table model #121

Early subscribe core signals in transaction table model #121

Uh oh!

Conversation

promag commented Oct 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Oct 28, 2020

Uh oh!

promag commented Nov 1, 2020

Uh oh!

hebasto commented Dec 16, 2020

Uh oh!

hebasto commented Jan 4, 2021

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

laanwj commented Jan 27, 2021

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

promag commented Apr 28, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto Apr 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

promag commented Oct 28, 2020 •

edited

Loading

hebasto Apr 29, 2021 •

edited

Loading