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

furszy commented at 6:39 pm on January 23, 2020: member

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, whatever is presented to fix it, this should use the same flow too.

[Edit - 24/01/2020]

Have added #17905 comment fix here too.

furszy requested review from ryanofsky on Jan 23, 2020

ryanofsky commented at 7:17 pm on January 23, 2020: member

Concept ACK. Will look more closely but I think this change seems good. It would also be good to follow up with #17905 (comment) and switch to hashes instead of heights, so GUI is updated reliably if there are reorgs

DrahtBot commented at 7:32 pm on January 23, 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:

#19101 (refactor: remove ::vpwallets and related global variables by ryanofsky)
#19099 (refactor: Move wallet methods out of chain.h and node.h by ryanofsky)

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.

furszy commented at 7:38 pm on January 23, 2020: member

agree @ryanofsky . Once you cache the best tip hash in clientModel, I can just code a getter there and replace the cachedNumBlocks here for the hash.

ryanofsky commented at 7:51 pm on January 23, 2020: member

Once you cache the best tip hash in clientModel

To be sure, I didn’t start to implement this at all. It’d be a welcome followup to this PR and #17905, or something that could be done alongside

furszy commented at 8:10 pm on January 23, 2020: member

k. if you didn’t start it (and want me doing it), i can tackle it over night. Just give me green light to not end up with both doing the same work.

DrahtBot added the label GUI on Jan 23, 2020

DrahtBot added the label Tests on Jan 23, 2020

ryanofsky commented at 8:17 pm on January 23, 2020: member

k. if you didn’t start it, i can tackle it over night. Just give me green light to not end up with both doing the same work.

🚥

Feel free to go ahead! I don’t have immediate plans to work on this, and have no problem closing/rebasing/replacing my other PR if your changes supercede or conflict with it

fanquake removed the label Tests on Jan 23, 2020

furszy commented at 0:57 am on January 24, 2020: member

Done. The last commit is not really needed, it’s just wrapping the tip data. I looked for a more manageable structure than continue adding arguments to the functions.

[Edit: seems that travis is still failing, will check it tomorrow]

furszy force-pushed on Jan 24, 2020

in src/interfaces/node.cpp:186 in 4948d4ee50 outdated

182@@ -183,6 +183,17 @@ class NodeImpl : public Node
183         LOCK(::cs_main);
184         return ::ChainActive().Height();
185     }
186+

ryanofsky commented at 6:10 pm on January 24, 2020:

In commit “Added best block hash to the NotifyHeaderTip and NotifyBlockTip signals.” (4948d4ee50d9960b23f43e14938845b0bb21772b)

Should remove extra newlines between methods (here and below) for consistency with existing code

furszy commented at 4:55 am on January 25, 2020:

done

in src/interfaces/node.cpp:193 in 4948d4ee50 outdated

188+    {
189+        const CBlockIndex* tip = nullptr;
190+        {
191+            LOCK(::cs_main);
192+            tip = ::ChainActive().Tip();
193+        }

ryanofsky commented at 6:11 pm on January 24, 2020:

In commit “Added best block hash to the NotifyHeaderTip and NotifyBlockTip signals.” (4948d4ee50d9960b23f43e14938845b0bb21772b)

Maybe shorten these lines to

0const CBlockIndex* tip = WITH_LOCK(::cs_main,  return ::ChainActive().Tip());

furszy commented at 4:54 am on January 25, 2020:

done

in src/interfaces/node.cpp:194 in 4948d4ee50 outdated

189+        const CBlockIndex* tip = nullptr;
190+        {
191+            LOCK(::cs_main);
192+            tip = ::ChainActive().Tip();
193+        }
194+        return (tip) ? tip->GetBlockHash() : Params().GenesisBlock().GetHash();

ryanofsky commented at 6:11 pm on January 24, 2020:

In commit “Added best block hash to the NotifyHeaderTip and NotifyBlockTip signals.” (4948d4ee50d9960b23f43e14938845b0bb21772b)

Maybe drop extra parentheses around (tip)

furszy commented at 4:54 am on January 25, 2020:

done

in src/interfaces/node.h:254 in 4948d4ee50 outdated

250@@ -248,12 +251,12 @@ class Node
251 
252     //! Register handler for block tip messages.
253     using NotifyBlockTipFn =
254-        std::function<void(bool initial_download, int height, int64_t block_time, double verification_progress)>;
255+        std::function<void(bool initial_download, const uint256 blockHash, int height, int64_t block_time, double verification_progress)>;

ryanofsky commented at 6:13 pm on January 24, 2020:

In commit “Added best block hash to the NotifyHeaderTip and NotifyBlockTip signals.” (4948d4ee50d9960b23f43e14938845b0bb21772b)

Maybe use reference type const uint256& to be a little more efficient and avoid a copy here and below

in src/qt/clientmodel.cpp:249 in 4948d4ee50 outdated

245@@ -235,7 +246,7 @@ static void BannedListChanged(ClientModel *clientmodel)
246     assert(invoked);
247 }
248 
249-static void BlockTipChanged(ClientModel *clientmodel, bool initialSync, int height, int64_t blockTime, double verificationProgress, bool fHeader)
250+static void BlockTipChanged(ClientModel *clientmodel, bool initialSync, const uint256 blockHash, int height, int64_t blockTime, double verificationProgress, bool fHeader)

ryanofsky commented at 6:16 pm on January 24, 2020:

In commit “Added best block hash to the NotifyHeaderTip and NotifyBlockTip signals.” (4948d4ee50d9960b23f43e14938845b0bb21772b)

Maybe use reference type const uint256& to be a little more efficient and avoid a copy here

furszy commented at 4:54 am on January 25, 2020:

done

in src/qt/clientmodel.h:85 in 4948d4ee50 outdated

80@@ -79,6 +81,9 @@ class ClientModel : public QObject
81     mutable std::atomic<int64_t> cachedBestHeaderTime;
82     mutable std::atomic<int> cachedNumBlocks;
83 
84+    mutable std::atomic<uint256> cachedBestHeaderHash;
85+    mutable std::atomic<uint256> cachedBestBlockHash;

ryanofsky commented at 6:20 pm on January 24, 2020:

In commit “Added best block hash to the NotifyHeaderTip and NotifyBlockTip signals.” (4948d4ee50d9960b23f43e14938845b0bb21772b)

These lines don’t seem to work with my compiler version (clang version 6.0.0-1ubuntu2). I don’t think it’s possible to use std::atomic with the uint256 class because it will only work for primitive int and pointer types. You probably need something like:

0Mutex m_cached_tip_mutex;
1uint256 m_cached_tip_headers;
2uint256 m_cached_tip_blocks;

furszy commented at 4:53 am on January 25, 2020:

done

in src/qt/transactionrecord.cpp:165 in 997129bdb4 outdated

161@@ -162,7 +162,7 @@ QList<TransactionRecord> TransactionRecord::decomposeTransaction(const interface
162     return parts;
163 }
164 
165-void TransactionRecord::updateStatus(const interfaces::WalletTxStatus& wtx, int numBlocks, int64_t block_time)
166+void TransactionRecord::updateStatus(const interfaces::WalletTxStatus& wtx, uint256 block_hash, int numBlocks, int64_t block_time)

ryanofsky commented at 6:24 pm on January 24, 2020:

In commit “[WalletModel] Balance and Txrecord status change polling check based on the block hash and not the height.” (997129bdb45c01390bffce991b680d58c9e7d18b)

Maybe use const uint256& type to avoid copy here, similarly for other functions in this commit

furszy commented at 4:53 am on January 25, 2020:

done

in src/qt/walletmodel.cpp:42 in 997129bdb4 outdated

36@@ -37,9 +37,9 @@ WalletModel::WalletModel(std::unique_ptr<interfaces::Wallet> wallet, interfaces:
37     QObject(parent), m_wallet(std::move(wallet)), m_node(node), optionsModel(_optionsModel), clientModel(_clientModel), addressTableModel(nullptr),
38     transactionTableModel(nullptr),
39     recentRequestsTableModel(nullptr),
40-    cachedEncryptionStatus(Unencrypted),
41-    cachedNumBlocks(0)
42+    cachedEncryptionStatus(Unencrypted)
43 {
44+    cachedLastBalanceUpdate = uint256();

ryanofsky commented at 6:27 pm on January 24, 2020:

In commit “[WalletModel] Balance and Txrecord status change polling check based on the block hash and not the height.” (997129bdb45c01390bffce991b680d58c9e7d18b)

Suggest dropping initialization in the constructor body and using member initialization

in src/qt/walletmodel.h:184 in 997129bdb4 outdated

178@@ -179,7 +179,9 @@ class WalletModel : public QObject
179     // Cache some values to be able to detect changes
180     interfaces::WalletBalances m_cached_balances;
181     EncryptionStatus cachedEncryptionStatus;
182-    int cachedNumBlocks;
183+
184+    // Block hash denoting when the last balance, polling, update was done.
185+    mutable std::atomic<uint256> cachedLastBalanceUpdate;

ryanofsky commented at 6:29 pm on January 24, 2020:

In commit “[WalletModel] Balance and Txrecord status change polling check based on the block hash and not the height.” (997129bdb45c01390bffce991b680d58c9e7d18b)

Again std::atomic<uint256> doesn’t seem to be a valid type. You probably need something like

0Mutex m_cached_tip_mutex;
1uint256 m_cached_tip;

furszy commented at 4:51 am on January 25, 2020:

I’m thinking on this one.

No synchronization is needed here. Only pollBalanceChanged modifies the cached tip, which is launched by the timer only. And, If the execution takes longer than the timer schedule, the new event will be queued. So, no parallel code execution.

in src/interfaces/node.h:278 in 7e8e03743f outdated

273+    int block_height;
274+    int64_t block_time;
275+    uint256 block_hash;
276+};
277+
278+BlockTip MakeTip(uint256 block_hash, int block_height, int64_t block_time);

ryanofsky commented at 6:46 pm on January 24, 2020:

In commit “[WalletModel] Balance and Txrecord status change polling check based on the block hash and not the height.” (997129bdb45c01390bffce991b680d58c9e7d18b)

I don’t think you need this function, since you can create instances just as easily with BlockHash{height, time, hash}

furszy commented at 4:52 am on January 25, 2020:

done

ryanofsky approved

ryanofsky commented at 6:53 pm on January 24, 2020: member

Conditional code review ACK cacb52feb81b22801a2747c3ae40369972825f5d if std::atomic<uint256> errors are fixed. The caching logic in the GUI all seems correct though.

furszy force-pushed on Jan 25, 2020

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

Concept ACK.

in src/qt/walletmodel.cpp:60 in d851606432 outdated

63 {
64     // This timer will be fired repeatedly to update the balance
65-    QTimer* timer = new QTimer(this);
66-    connect(timer, &QTimer::timeout, this, &WalletModel::pollBalanceChanged);
67-    timer->start(MODEL_UPDATE_DELAY);
68+    pollingTimer = new QTimer(this);

promag commented at 1:08 pm on January 30, 2020:

These changes to the timer are unnecessary. The timer is stopped and destroyed automatically on WalletModel destruction.

furszy commented at 5:42 pm on January 30, 2020:

double checked and yep, cleaning them 👍 .

furszy force-pushed on Jan 30, 2020

in src/interfaces/node.cpp:192 in 3d03bac543 outdated

182@@ -183,6 +183,11 @@ class NodeImpl : public Node
183         LOCK(::cs_main);
184         return ::ChainActive().Height();
185     }
186+    uint256 getBestBlockHash() override
187+    {
188+        const CBlockIndex* tip = WITH_LOCK(::cs_main,  return ::ChainActive().Tip());

promag commented at 3:54 pm on January 31, 2020:

nit double space

furszy commented at 2:15 pm on February 6, 2020:

done.

in src/interfaces/node.cpp:193 in 3d03bac543 outdated

182@@ -183,6 +183,11 @@ class NodeImpl : public Node
183         LOCK(::cs_main);
184         return ::ChainActive().Height();
185     }
186+    uint256 getBestBlockHash() override
187+    {
188+        const CBlockIndex* tip = WITH_LOCK(::cs_main,  return ::ChainActive().Tip());
189+        return tip ? tip->GetBlockHash() : Params().GenesisBlock().GetHash();

promag commented at 3:54 pm on January 31, 2020:

When can tip be null?

furszy commented at 1:36 pm on February 6, 2020:

The else path is stating it, only can be null when no blocks were connected to the chain a.k.a genesis block.

DrahtBot added the label Needs rebase on Feb 13, 2020

hebasto commented at 1:49 pm on April 22, 2020: member

@furszy Mind rebasing if you are still working on it?

furszy commented at 10:20 pm on April 22, 2020: member

Yep sure, will check the PRs that have been merged over the area and update/rebase this one. If my memory doesn’t fail me, the first commit will not be needed anymore and this PR will be mainly focused on moving the best tip cached height to the block hash.

furszy force-pushed on Apr 23, 2020

furszy renamed this:
~~gui: Avoid redundant cs_main locks in balance polling.~~
gui: Balance/TxStatus polling update based on last block hash.
on Apr 23, 2020

furszy commented at 1:39 am on April 23, 2020: member

Work rebased and updated to latest master changes. Plus, PR title and description updated too.

DrahtBot removed the label Needs rebase on Apr 23, 2020

furszy commented at 8:27 pm on April 25, 2020: member

@ryanofsky @promag would you mind re reviewing this? .

hebasto commented at 12:46 pm on May 16, 2020: member

Concept ACK a33fd3fa4e668b19495a1d11e44c7d405aaa6e10.

Here are my non-blocking suggestions to the a33fd3fa4e668b19495a1d11e44c7d405aaa6e10 “Trivial: getLastBlockTime inline with the latest WITH_LOCK macro.” commit:

please do not touch getBestBlockHash() function; removing double space should be done in the “Added best block hash to the NotifyHeaderTip and NotifyBlockTip signals.” commit
if you decided to inline with the WITH_LOCK macro in getLastBlockTime(), why do not make the same in the getVerificationProgress()?
please in commit message change s/Trivial/refactor/ (see https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#contributor-workflow)

in src/qt/clientmodel.cpp:39 in 6002d1185d outdated

34@@ -35,6 +35,7 @@ ClientModel::ClientModel(interfaces::Node& node, OptionsModel *_optionsModel, QO
35 {
36     cachedBestHeaderHeight = -1;
37     cachedBestHeaderTime = -1;
38+    m_cached_tip_blocks = uint256();

hebasto commented at 1:08 pm on May 16, 2020:

6002d1185db89af124a257c894195a7f1b04c1f2

Mind using a default member initializer:

0    uint256 m_cached_tip_blocks GUARDED_BY(m_cached_tip_mutex) {};

?

furszy commented at 9:53 pm on May 18, 2020:

sure np. Can move the other members initializations (cachedBestHeaderXXX) if that is needed in another commit too.

in src/qt/clientmodel.h:13 in 6002d1185d outdated

 9@@ -10,6 +10,8 @@
10 
11 #include <atomic>
12 #include <memory>
13+#include <sync.h>     // for Mutex

hebasto commented at 1:09 pm on May 16, 2020:

Not sure if the comment is required here.

hebasto changes_requested

hebasto commented at 1:25 pm on May 16, 2020: member

In the 5db27e3fcc7dc0154869291482a324672eb7a251 “[WalletModel] Balance and Txrecord status…” commit uint256 hash is passed by value to some functions, and as const reference to others. A suggest consistently pass uint256 hash as const reference.

in src/qt/walletmodel.cpp:48 in 5db27e3fcc outdated

44@@ -45,7 +45,7 @@ WalletModel::WalletModel(std::unique_ptr<interfaces::Wallet> wallet, ClientModel
45     transactionTableModel(nullptr),
46     recentRequestsTableModel(nullptr),
47     cachedEncryptionStatus(Unencrypted),
48-    cachedNumBlocks(0)
49+    m_cached_last_update_tip(uint256())

hebasto commented at 1:32 pm on May 16, 2020:

Mind using a default member initializer?

hebasto commented at 1:41 pm on May 16, 2020: member

The motivation of the de8221e6c0d7e360caa62baf7591bd2ee3670e79 “[ClientModel][Interfaces] BlockTip struct created and connected to notifyHeaderTip and notifyBlockTip signals.” commit is not clear for me. Could elaborate?

in src/qt/walletmodel.cpp:83 in ae5775a16e outdated

78@@ -79,6 +79,10 @@ void WalletModel::updateStatus()
79 
80 void WalletModel::pollBalanceChanged()
81 {
82+    // Do not even try to poll if shutdown was requested.
83+    if (m_node.shutdownRequested())

ryanofsky commented at 3:16 pm on May 18, 2020:

In commit “[WalletModel] Stop polling balance if shutdown was requested.” (ae5775a16e85144e73e999ef87dec26e22236a86)

Is it necessary to include this change in the PR? I think it’d be nicer to drop this commit if it’s not strictly necessary. Reasons:

I think it is cleaner to cancel the poll timer outright when shutdown is initiated instead of continuing to poll but just returning immediately each poll interval. I already have a change which does this out for review: 2bc9b92ed8b7736ad67876398a0bb8287f57e9b3 from #18587. I think that approach is better because it lets the GUI just handle shutdown one place in qt/bitcoin.cpp, and not be querying for shutdown state in other parts of GUI code.
Adding the m_node method call here is bad for performance and debugging with #10102 because m_node method calls require IPC between gui and node processes

If we can drop this commit from the PR, that’d be great. But it’s ok to keep this if it’s needed to avoid crashes. I can always revert it as part of #18587 later. (I’d appreciate review on #18587, by the way, if anyone wants to take a look.)

furszy commented at 11:07 pm on May 18, 2020:

Yeah absolutely, will drop it. it’s not necessary anymore and fully agree on the timer cancel from outside and performance reasons (and well, because it’s an ugly patch too..).

If my memory doesn’t fail me, i made it because there was a race condition over the shutdown process before (for the first commit of this PR, the one that was removed due #17905 merge) and even closing the timer properly as you did there was not good enough.

ryanofsky approved

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

Code review ACK a33fd3fa4e668b19495a1d11e44c7d405aaa6e10. Thank you @furszy for a really nice first contribution! Also thank you for being so patient. I don’t think I noticed when you updated this almost a month ago, and definitely wish I reviewed it sooner.

re: #17993#issue-366506660

[Edit - 22/04/2020]

Final (hopefully) PR Description:

If it’s not too much trouble, it’d be great if you could move the newer PR description to the top of the comment. You can use --- horizontal line markdown to separate it from the rest of the comment. This should help the PR make more sense to other reviewers, and should also make git history more readable because we have a script that adds github PR descriptions to git merge commit messages.

Extra topic:

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

Probably best to save renaming for a future PR, but we are slowly migrating class members to be prefixed with m_ and be lower case. Personally, I would probably do a number of other renames as well:

getBestBlockHash -> getLastBlockHash
getNumBlocks -> getLastBlockHeight
cachedNumBlocks -> m_last_block_height
m_cached_tip_blocks -> m_last_block_hash
cur_block_hash -> last_block_hash

re: #17993 (comment)

The motivation of the de8221e “[ClientModel][Interfaces] BlockTip struct created and connected to notifyHeaderTip and notifyBlockTip signals.” commit is not clear for me. Could elaborate?

It’s just supposed to be an aesthetic improvment: instead of having lots of parameters, combining related parameters into a simple struct. Not strictly necessary, but seems like a pretty good change.

ryanofsky commented at 7:00 pm on May 18, 2020: member

Travis error https://travis-ci.org/github/bitcoin/bitcoin/jobs/678420543#L3130 “AssertionError: Mempool sync timed out after 60s” in mempool_reorg.py in native_valgrind build a seems like it might have been already reported and fixed in #18617 (comment)

furszy force-pushed on May 18, 2020

furszy commented at 11:13 pm on May 18, 2020: member

Hey guys, thanks for the feedback. All of the code comments were addressed.

Going through @hebasto’s feedback first. Answering the remaining unanswered questions:

if you decided to inline with the WITH_LOCK macro in getLastBlockTime(), why do not make the same in the getVerificationProgress()?

Because the GuessVerificationProgress internal method accepts a CblockIndex and not a CBlock like the one returning by Params().GenesisBlock() and i didn’t want to add more off-topic changes to this PR.

In the 5db27e3 “[WalletModel] Balance and Txrecord status…” commit uint256 hash is passed by value to some functions, and as const reference to others. A suggest consistently pass uint256 hash as const reference.

Could you point me exactly where? The only place passing it differently is tryGetBalances because it’s used as a return object.

Travis error https://travis-ci.org/github/bitcoin/bitcoin/jobs/678420543#L3130 “AssertionError: Mempool sync timed out after 60s” in mempool_reorg.py in native_valgrind build a seems like it might have been already reported and fixed in #18617 (comment) @ryanofsky yeah. This PR is not touching a single line of anything tested there.

in src/qt/clientmodel.cpp:1 in 6002d1185d outdated

0@@ -1,4 +1,4 @@
1-// Copyright (c) 2011-2020 The Bitcoin Core developers
2+// Copyright (c) 2011-2019 The Bitcoin Core developers

ryanofsky commented at 12:20 pm on May 19, 2020:

In commit “Added best block hash to the NotifyHeaderTip and NotifyBlockTip signals.” (6002d1185db89af124a257c894195a7f1b04c1f2)

Probably unintended change this line

furszy commented at 1:23 pm on May 21, 2020:

yeah, rebase issue. Good catch.

in src/qt/clientmodel.h:13 in 20049ec7ea outdated

 9@@ -10,7 +10,7 @@
10 
11 #include <atomic>
12 #include <memory>
13-#include <sync.h>     // for Mutex
14+#include <sync.h>

ryanofsky commented at 12:22 pm on May 19, 2020:

In commit “Added best block hash to the NotifyHeaderTip and NotifyBlockTip signals.” (20049ec7eac1f555dde04026594c4da4b1b80c8b)

Would recommend squashing the two commits to a single one

6002d1185db89af124a257c894195a7f1b04c1f2 Added best block hash to the NotifyHeaderTip and NotifyBlockTip signals. (1/5)
20049ec7eac1f555dde04026594c4da4b1b80c8b Added best block hash to the NotifyHeaderTip and NotifyBlockTip signals. (2/5)

furszy commented at 1:38 pm on May 21, 2020:

yeah absolutely. Same as the other point, rebase issue that i totally missed. Squashing it.

ryanofsky approved

ryanofsky commented at 12:32 pm on May 19, 2020: member

Code review ACK 6a3d1a51064a3cc1697f73893efc8fd350653ffe, but I think this needs to bring back the dropped shutdownrequested commit https://github.com/bitcoin/bitcoin/commit/ae5775a16e85144e73e999ef87dec26e22236a86, contra #17993 (review), because now address sanitizer is failing in WalletModel::pollBalanceChanged: https://travis-ci.org/github/bitcoin/bitcoin/jobs/688603552#L4244.

Main change since my previous review was dropping the shutdownrequested commit. Other changes were minor commit reordering, spacing comment, initialization tweaks

EDIT: strikethrough, see #17993 (comment)

DrahtBot added the label Needs rebase on May 19, 2020

ryanofsky commented at 11:12 am on May 20, 2020: member

re: #17993#pullrequestreview-414386972

but I think this needs to bring back the dropped shutdownrequested commit ae5775a, contra #17993 (comment), because now address sanitizer is failing in WalletModel::pollBalanceChanged: https://travis-ci.org/github/bitcoin/bitcoin/jobs/688603552#L4244.

Update: I think the ae5775a16e85144e73e999ef87dec26e22236a86 workaround should no longer be needed again after rebasing now that 2bc9b92ed8b7736ad67876398a0bb8287f57e9b3 from #18587 is merged

furszy force-pushed on May 21, 2020

DrahtBot removed the label Needs rebase on May 21, 2020

furszy commented at 2:46 pm on May 21, 2020: member

Done, feedback + rebased on top of master (there were lot of conflicts :p). Travis should be happy now too. @ryanofsky @hebasto all yours. If you need anything else shoot.

hebasto commented at 6:42 pm on May 22, 2020: member

@furszy I’ve collected my suggestions into this branch – feel free to use it :)

Also I think the latest 03ef9300e8514896e44b070c4c009ceacd36d39b commit changes behavior as return statement is not protected by the ::cs_main mutex now.

furszy force-pushed on May 22, 2020

furszy commented at 11:31 pm on May 22, 2020: member

k checked 👍 . Cherry-picked few code styling changes and left the other name changes for another PR as @ryanofsky wrote above #17993#pullrequestreview-413693885 . I agree with him there about the overall members naming needing further discussion to follow the same convention everywhere.

Plus, didn’t cherry-picked the getBestBloskHash rename to getLastBlockHash of the node.h interface to follow the current convention —> to get the tip, the sources are using getBestBlockHash in the RPC, which is the same action and there by should have the same name. And, the only place using the lastBlock method name is the wallet which doesn’t have the same meaning there, the wallet record the last seen hash.

Also I think the latest 03ef930 commit changes behavior as return statement is not protected by the ::cs_main mutex now.

The critical section is the ChainActive.Tip method call as the tip can change at any time and chainActive will return a different pointer. The CBlockIndex data being accessed shouldn’t change or would break other areas of the code and not only this one (at least for now, there is no CBlockIndex pointers delete policy).

But.. dropped the last commit to not block this PR. No big deal, tiny change there that has nothing to do with this PR anyway. Better to just move on :) .

in src/qt/transactionrecord.h:65 in 4062b9e05e outdated

60@@ -61,8 +61,8 @@ class TransactionStatus
61                       finalization */
62     /**@}*/
63 
64-    /** Current number of blocks (to know whether cached status is still valid) */
65-    int cur_num_blocks;
66+    /** Current block hash (to know whether cached status is still valid) */
67+    uint256 cur_block_hash;

hebasto commented at 4:35 am on May 23, 2020:

@furszy may I suggest the default member initialization again

0    /** Current block hash (to know whether cached status is still valid) */
1    uint256 cur_block_hash{};

? And drop cur_block_hash(uint256()) in the ctor initialization list.

hebasto commented at 4:47 am on May 23, 2020:

And use m_ prefix for the data member cur_block_hash name?

furszy commented at 11:04 pm on May 23, 2020:

yeah done.

hebasto changes_requested

hebasto commented at 4:43 am on May 23, 2020: member

ACK 4062b9e05e251eea3366c010c9b75fe5119b76a5, tested on Linux Mint 19.3 (x86_64).

In fe92e47ac09c86a0e7bb28f4b5c4c2f19d889774 “Added best block hash to the NotifyHeaderTip and NotifyBlockTip signals.” mind following our naming conventions and replacing s/blockHash/block_hash/ in function parameter and local variable names?

Note 1: The clang-format-diff.py tool makes following coding style conventions very easy :) I believe it is important in the long term, especially for new added code.

Note 2. If you will update commits, mind not using strings like “[ClientModel][Interfaces]” in commit title and message?

Added best block hash to the NotifyHeaderTip and NotifyBlockTip signals.

[ClientModel] best header/block hash cached.

2f867203b0

BlockTip struct created and connected to notifyHeaderTip and notifyBlockTip signals. a06e845e82

furszy force-pushed on May 23, 2020

furszy commented at 11:08 pm on May 23, 2020: member

Thanks for the quick feedback @hebasto. PR updated per repo’s conventions suggestions 👍 .

hebasto approved

hebasto commented at 4:32 am on May 24, 2020: member

re-ACK a06e845e826acaeb0db7cf02b2519c177e94dee5, suggested style changes implemented since the previous review.

in src/qt/transactiontablemodel.cpp:667 in 2f867203b0 outdated

663@@ -664,7 +664,7 @@ QVariant TransactionTableModel::headerData(int section, Qt::Orientation orientat
664 QModelIndex TransactionTableModel::index(int row, int column, const QModelIndex &parent) const
665 {
666     Q_UNUSED(parent);
667-    TransactionRecord *data = priv->index(walletModel->wallet(), walletModel->getNumBlocks(), row);
668+    TransactionRecord* data = priv->index(walletModel->wallet(), walletModel->clientModel().getBestBlockHash(), row);

ryanofsky commented at 7:31 pm on May 26, 2020:

In commit “Added best block hash to the NotifyHeaderTip and NotifyBlockTip signals.” (2f867203b0c7a4438ce484be4cfa2b29dbf1abf0)

Note: there is a change in behavior here. Previously the transaction would be updated when WalletModel::cachedNumBlocks changed (when the wallet model balance polling timer detected the wallet processed a new block). Now the transaction will get updated ClientModel::m_cached_tip_blocks changes (when there is a new node BlockTipChanged event). This means transaction display should now get updated a little faster since it won’t depend on the wallet balance timer, but now it will do some wasted work during the time it takes for the wallet to update its last processed block after the node tip changes.

Ideally the wallet model would have its own wallet last block processed member similar CWallet::m_last_block_processed, that could be used here, and which would get updated from a wallet notification as described #16874 (comment), not requiring polling or locking cs_wallet to get an up to date value.

EDIT: To be clear, this is all good for the current PR, just wanted to document what changed here and what future steps could be

ryanofsky approved

ryanofsky commented at 8:01 pm on May 26, 2020: member

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

jonasschnelli commented at 8:36 am on May 29, 2020: contributor

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).

jonasschnelli merged this on May 29, 2020

jonasschnelli closed this on May 29, 2020

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

vasild referenced this in commit 367c8a6df7 on Jun 1, 2020

vasild referenced this in commit f46b678acf on Jun 1, 2020

jonasschnelli referenced this in commit f4f2220456 on Jun 5, 2020

sidhujag referenced this in commit e70bd9a484 on Jun 5, 2020

hebasto commented at 1:16 pm on June 19, 2020: member

The regression, that was supposedly introduced by this PR, has been detected in https://github.com/bitcoin-core/gui/issues/7.

MarcoFalke referenced this in commit 4946400470 on Jun 26, 2020

MarcoFalke referenced this in commit d906aaa117 on Jun 26, 2020

stackman27 referenced this in commit e89c0b5572 on Jun 26, 2020

stackman27 referenced this in commit 287aabfdea on Jun 26, 2020

sidhujag referenced this in commit 4dfbb17dc3 on Jul 7, 2020

janus referenced this in commit 01ed57dea6 on Nov 15, 2020

Fabcien referenced this in commit 9ad2859f2e on Feb 23, 2021

Fabcien referenced this in commit 1730a7452c on Feb 23, 2021

DrahtBot locked this on Feb 15, 2022

furszy deleted the branch on Nov 29, 2022

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

Conflicts