Each 250ms the slot WalletModel::pollBalanceChanged is called which, at worst case, calls Wallet::GetBalance. This is a waste of resources since most of the time there aren't new transactions or new blocks. Fix this by early checking if cache is dirty or not.
The actual balance computation can still hang the GUI thread but that is tracked in #16874 and should be fixed with a solution similar to #17135.
@MarcoFalke considering #18123 this should be backported too (if change is correct).
Concept ACK about not wasting resources.
97 | - if(transactionTableModel) 98 | - transactionTableModel->updateConfirmations(); 99 | - } 100 | + checkBalanceChanged(new_balances); 101 | + if(transactionTableModel) 102 | + transactionTableModel->updateConfirmations();
nit: While these lines are touched already, why not apply correct format style?
Forgot to mention to review with &w=1.
ACK 0933a37078e1ce3a3d70983c3e7f4b3ac6c3fa37, I have not tested the code, but I have reviewed it and it looks OK, I agree it can be merged.
Early return from tryGetBalances() also minimizes holding of cs_wallet lock.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
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.
See also #10251 (could the discussion there be relevant here?)
The goal here is to avoid unnecessary calls to GetBalances and to be an easy backport.
Refactoring to make it asynchronous can also be done but I don't think it invalidates this change.
utACK 0933a37078e1ce3a3d70983c3e7f4b3ac6c3fa37
200 | @@ -201,8 +201,11 @@ class Wallet 201 | //! Get balances. 202 | virtual WalletBalances getBalances() = 0; 203 | 204 | - //! Get balances if possible without blocking. 205 | - virtual bool tryGetBalances(WalletBalances& balances, int& num_blocks) = 0; 206 | + //! Get balances if possible without waiting for chain and wallet locks.
This should mention that it won't attempt to get balances if height is same.
ACK 0933a37078e1ce3a3d70983c3e7f4b3ac6c3fa37
Unrelated to this PR: Checking for staleness should be done via checking total chain work, not height changes. A re-org across a difficulty change threshold could result in balances not updating when they should.
Can someone explain to me how this is supposed to work? Why does the balance not change when the block height stays the same? Can't we send out funds or receive them in the meantime (they might be untrusted, but still, the balance should change)?
@MarcoFalke look at updateTransaction which sets the "force" parameter. I'll have to dig more to completely understand on my side
In that case it seems easier to just call it with the force parameter on every connected block and then remove the polling?
The real deal should be async balance computation to not block the GUI thread. What you suggest is a bigger change and requires locks instead of try locks.
I think this PR would be equivalent to adding:
if (!fForceCheckBalanceChanged && cachedNumBlocks == m_client_model.getNumBlocks()) return;
to the top of the WalletModel::pollBalanceChanged() function after #17905.
Assuming #17905 is a change we want to make anyway, keeping the changed detection in pollBalanceChanged() instead of tryGetBalances seems better because:
tryGetBalances method the same and its usage simplertryGetBalances IPC call (with #10102) in addition to avoiding the overhead of balance computation.tryGetBalances cached_num_blocks argument should leave one less thing that needs to be updated in #17993, which switches polling to check the chain tip instead of the chain height
@ryanofsky If we call getBalances async when the tip changes or a wallet transaction is updated then we can drop tryGetBalances (it only exists because of polling).
So this PR tunes tryGetBalances for polling, maybe it should rename it tryGetBalancesIfNeeded.
Code review ACK 0933a37078e1ce3a3d70983c3e7f4b3ac6c3fa37, but I would prefer (not strongly) for #17905 to be merged first. This PR can be simpler if it is based on #17905, so tryGetBalances can just be left alone instead of changing into to a more complicated tryGetBalancesIfNeeded function, and then getting changed back later when we want to optimize it out.
@ryanofsky is the alternative easy to backport?
@ryanofsky is the alternative easy to backport?
The suggested alternative makes this PR smaller and probably easier to backport, but it builds on #17905 which is bigger and does have minor conflicts with 0.18 and 0.19.
If the goal is to backport #18160 without #17905, I'd suggest leaving this PR in its current form so it is easier to backport. If the goal is to backport both PRs, then adopting the suggested alternative won't have an impact on backporting. The alternative would just make this PR simpler and let these changes be entirely contained in src/qt/ instead of requiring a complication to src/interfaces/ that will be unnecessary and confusing after #17905
But again no strong preference on what order things are merged. I am just saying there will be less to clean up if #17905 is merged first and src/interfaces can be left alone.
IMO this should go into 0.20. @ryanofsky @jonasschnelli @laanwj
ACK 0933a37078e based primarily on code review, despite a lot of manual testing with a large 177MB wallet.
I did a fair amount of manually testing this PR versus master, back on forth, on debian and macOS, loading small wallets and a 177MB wallet in the GUI, and left a comment here: #15015 (comment). The results are variable: When all is snappy and well, I am not seeing a difference. When the GUI is laggy/scanning, this PR does seems better, but I don't have much confidence given the variability. This ACK is mainly based on code review.
Made a wallet with 34176 transactions. Then ran instruments before and after this PR for around 90 seconds.
Before:
After:
So before 8.44 seconds (of 90s) in total was spent in CWallet::GetBalance. Note that after no significant CPU was spent.
Milestone
0.20.0