the result of CWallet::IsHDEnabled() was initialized with true.
But in case of no keys or a blank hd wallet the iterator would be skipped and not set to false but true, since the loop would be not entered.
That had resulted in a wrong return and subsequent false HD and watch-only icon display in GUi when reloading a wallet after closing.
While testing the GUI I noticed that after reloading a blank HD wallet or a key less wallet the icons are displayed not blanked 50%, and also not the correct strike thru HD icon.
When we create the wallet that is handled correct since we init IsHDEnabled there with 0. (false)
CWallet::IsHDEnabled while working on https://github.com/bitcoin-core/gui/pull/403.
1370- result &= spk_man->IsHDEnabled();
1371+ if (spk_man->IsHDEnabled()) {
1372+ result = true;
1373+ } else {
1374+ return false;
1375+ }
0 if (!spk_man->IsHDEnabled()) return false;
1 result = true;
Or, even better:
0--- a/src/wallet/wallet.cpp
1+++ b/src/wallet/wallet.cpp
2@@ -1364,11 +1364,11 @@ CAmount CWallet::GetDebit(const CTransaction& tx, const isminefilter& filter) co
3 bool CWallet::IsHDEnabled() const
4 {
5 // All Active ScriptPubKeyMans must be HD for this to be true
6- bool result = true;
7+ if (GetActiveScriptPubKeyMans().empty()) return false;
8 for (const auto& spk_man : GetActiveScriptPubKeyMans()) {
9- result &= spk_man->IsHDEnabled();
10+ if (!spk_man->IsHDEnabled()) return false;
11 }
12- return result;
13+ return true;
14 }
15
16 bool CWallet::CanGetAddresses(bool internal) const
?
I thought too about using a check if keys are empty, but then was not sure if empty will work correctly. So the second suggestion might work, the first not, since we will never enter the loop when wallet is empty, and subsequently return true, when indeed false would be correct.
I saw btw the discussion https://github.com/bitcoin-core/gui/pull/403 and found that a review statement there **…the pr works and the behavior after refraktor has not changed_**__ quite revealing, i tested too that PR and saw that it would not change this vital aspect, that is wrong since a lot of releases, but i guess it is not only a GUI issue and to some extent this PR is also a hint to the implications of constructions in the form of bare
for (const auto& x : X()) .... when we can no be sure about the size of x and make vital decisions only in the loop.
… the first not, since we will never enter the loop when wallet is empty, and subsequently return true, when indeed false would be correct.
I don’t think this is correct description of the suggested change. Both lines are within a loop body, and there is no return true; statement at all.
But in case of no keys or a blank hd wallet the iterator would be skipped
and not set to false but true, since the loop would be not entered.
That had resulted in a wrong return and subsequent false HD and watch-only
icon display in gui when reloading a wallet after closing.
Update src/wallet/wallet.cpp
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
