Implements issue #17174, to grey out used addresses in the address book when the wallet has the avoid_reuse flag set.
this commit brings the IsUsedDestination method into the Wallet.h interface. It is then called in addresstablemodel.cpp to determine whether the address has been used or not whilst setting the font colour
master
pr
201@@ -202,6 +202,12 @@ class WalletImpl : public Wallet
202 LOCK(m_wallet->cs_wallet);
203 return m_wallet->ListLockedCoins(outputs);
204 }
205+ bool IsUsedDestination(const CTxDestination& dst) override
isUsedDestination instead.
131@@ -132,6 +132,9 @@ class Wallet
132 //! List locked coins.
133 virtual void listLockedCoins(std::vector<COutPoint>& outputs) = 0;
134
135+ // Whether this or any UTXO with the same CTxDestination has been spent.
IsUsedDestination method in the wallet/wallet.h file
201@@ -202,6 +202,12 @@ class WalletImpl : public Wallet
202 LOCK(m_wallet->cs_wallet);
203 return m_wallet->ListLockedCoins(outputs);
204 }
205+ bool IsUsedDestination(const CTxDestination& dst) override
206+ {
207+ auto locked_chain = m_wallet->chain().lock();
227@@ -228,6 +228,13 @@ QVariant AddressTableModel::data(const QModelIndex &index, int role) const
228 default: break;
229 }
230 }
231+ else if (role == Qt::ForegroundRole)
if (...) {.
227@@ -228,6 +228,13 @@ QVariant AddressTableModel::data(const QModelIndex &index, int role) const
228 default: break;
229 }
230 }
231+ else if (role == Qt::ForegroundRole)
232+ {
233+ if(walletModel->wallet().IsUsedDestination(DecodeDestination(rec->address.toStdString())))
if.
Addresses are used when they receive. Not when the UTXO created is later spent.
Understood, I can update it to grey out just addresses that have received (Used) and not ones that have received and been spent if desired?
Understood, I can update it to grey out just addresses that have received (Used) and not ones that have received and been spent if desired?
Yes (it would be meaningless/erroneous to grey out addresses based on actions of the UTXOs created when they receive).
Revisiting this pr…
I have now re-implemented by saving a flag in the address book (address_used) when we see an address used. I took the same approach as the IsUsedDestination methods and therefor this will only work when the avoid_reuse flag is set on the wallet.
This essentially checks the outputs when a transaction is added to the wallet (AddToWallet) and if it belongs to us, sets a flag in the address book (SetUsedAddressState). It will then grey out any addresses with this flag set when viewing them in the ‘Receiving Addresses’ dialog box.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
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.
IMO this should work for all wallets, and when loading an old wallet (or one that has been loaded by an older version since the last use of a supporting version), should populate the flag as necessary. @luke-jr I would be inclined to agree with you on this, it was only initially constrained to
avoid_reusewallets asIsUsedDestinationwas used in the first integration (although wrongly used due to confusion). So pivoting back to working for all wallets would make sense.
Would like to get others opinions on this before progressing with making the changes, thoughts @MarcoFalke @jonasschnelli @promag?
783@@ -784,6 +784,27 @@ bool CWallet::IsSpentKey(const uint256& hash, unsigned int n) const
784 return false;
785 }
786
787+void CWallet::SetUsedAddressState(WalletBatch& batch, const CTxOut& cout, bool used)
788+{
789+ AssertLockHeld(cs_wallet);
790+ CTxDestination dst;
791+ if(ExtractDestination(cout.scriptPubKey, dst)) {
if), hopefully I haven’t missed anything, Thanks!
If helpful, you can use Clang in the future to check for some things:
Use Clang formatting to clean up a file:
clang-format -i <file>
To format the last commit with 0 lines of context, call the
clang-format-diff.py script from root as follows:
git diff -U0 HEAD~1.. | ./contrib/devtools/clang-format-diff.py -p1 -i -v
800+}
801+
802+bool CWallet::IsUsedAddress(const CTxDestination& dst) const
803+{
804+ LOCK(cs_wallet);
805+ return IsMine(dst) && GetDestData(dst, "address_used", nullptr);
Is it me or IsMine is irrelevant?
Either way call GetDestData since it’s faster and most likely to fail.
IsMine would be irrelevant in this case, have now removed to just return the output of GetDestData as suggested, Thanks!
Maybe add a tooltip on the greyed rows?
Added tooltip as suggested, not sure on suitable wording so suggestions welcome (I went with “This address has been used” followed by the original tooltip in brackets (screenshot below).
795+ } else if (!used && GetDestData(dst, "address_used", nullptr)) {
796+ EraseDestData(batch, dst, "address_used");
797+ }
798+ }
799+ }
800+}
perhaps simplify
0 {
1 AssertLockHeld(cs_wallet);
2 CTxDestination dst;
3- if (ExtractDestination(cout.scriptPubKey, dst)) {
4- if (IsMine(dst)) {
5- if (used && !GetDestData(dst, "address_used", nullptr)) {
6- AddDestData(batch, dst, "address_used", "p"); // p for "present", opposite of absent (null)
7- } else if (!used && GetDestData(dst, "address_used", nullptr)) {
8- EraseDestData(batch, dst, "address_used");
9- }
10+ if (ExtractDestination(cout.scriptPubKey, dst) && IsMine(dst)) {
11+ if (used && !GetDestData(dst, "address_used", nullptr)) {
12+ AddDestData(batch, dst, "address_used", "p"); // p for "present", opposite of absent (null)
13+ } else if (!used && GetDestData(dst, "address_used", nullptr)) {
14+ EraseDestData(batch, dst, "address_used");
15 }
16 }
17 }
227@@ -228,6 +228,16 @@ QVariant AddressTableModel::data(const QModelIndex &index, int role) const
228 default: break;
229 }
230 }
231+ else if (role == Qt::ForegroundRole) {
232+ if (rec->type == AddressTableEntry::Receiving && walletModel->wallet().isUsedAddress(DecodeDestination(rec->address.toStdString()))) {
233+ return QColor(Qt::lightGray);
Suggestion: perhaps gray to be more compatible with dark mode.
gray:
light gray:
gray instead of lightGray to accommodate both light and dark modes
799+ }
800+}
801+
802+bool CWallet::IsUsedAddress(const CTxDestination& dst) const
803+{
804+ LOCK(cs_wallet);
GetDestData is just accessing the address book data. I have now removed the lock.
Sorry about that; it appears to be needed. Clang build output:
0wallet/wallet.cpp:803:12: error: calling function 'GetDestData' requires
1holding mutex 'cs_wallet' exclusively [-Werror,-Wthread-safety-analysis]
2 return GetDestData(dst, "address_used", nullptr);
3 ^
41 error generated.
Concept ACK – nice!
Builds cleanly and works as advertised, including the tooltip.
git diff 94cbeaa 481d3df would have been cleaner and easier to re-review by not squashing. But wait until someone else reviews before re-touching.
I’m sorry to NACK current approach. You shouldn’t be adding blocking calls in AddressTableModel::data, these will make the GUI unresponsive. I think you should add a is_used member to AddressTableEntry and keep it updated accordingly.
Edit: I mean blocking mainly because of process separation - in that case some IPC is necessary between the GUI process and wallet process. But even without process separation, calling GetDestCall for all visible addresses can disturb the GUI event loop.
@za-kk are you going to implement promags suggestion, or can I pick this up?
Probably, the better approach is to split this change into two PRs:
CWallet member functions and add tests for them (in this repo)
Sorry, this PR has been quite stale from me over the last several months as I haven’t had the chance to take a look yet. I do still plan on implementing the suggestions over the next week or so but always willing to accept help on this!
Thanks @hebasto, I was wondering what the preferred way of doing this would be now that we have the GUI repo. I will look into introducing the initial CWallet functions over the coming days
CWallet changes then I think they belong there. It might make sense to do what you suggest but not in this early stage - too much effort to sync both PR’s, to have split discussions etc.
@hebasto I don’t quite agree with that approach. The goal is to add a GUI change and that PR should be in gui repo. If that PR requires
CWalletchanges then I think they belong there. It might make sense to do what you suggest but not in this early stage - too much effort to sync both PR’s, to have split discussions etc.
I’m fine with keeping all changes and discussion here.
New introduced functions still require test coverage (probably, in a follow up).
841@@ -842,6 +842,25 @@ bool CWallet::IsSpentKey(const uint256& hash, unsigned int n) const
842 return false;
843 }
844
845+void CWallet::SetUsedAddressState(WalletBatch& batch, const CTxOut& cout, bool used)
846+{
847+ AssertLockHeld(cs_wallet);
I’m getting compiler warnings consistent with the CI:
0wallet/wallet.cpp:847:5: warning: calling function 'AssertLockHeldInternal<AnnotatedMixin<std::__1::recursive_mutex> >' requires holding mutex 'cs_wallet' exclusively [-Wthread-safety-analysis]
1 AssertLockHeld(cs_wallet);
2 ^
3./sync.h:81:28: note: expanded from macro 'AssertLockHeld'
4#define AssertLockHeld(cs) AssertLockHeldInternal(#cs, __FILE__, __LINE__, &cs)
5 ^
6wallet/wallet.cpp:849:55: warning: calling function 'IsMine' requires holding mutex 'cs_wallet' exclusively [-Wthread-safety-analysis]
7 if (ExtractDestination(cout.scriptPubKey, dst) && IsMine(dst)) {
8 ^
9wallet/wallet.cpp:850:22: warning: calling function 'GetDestData' requires holding mutex 'cs_wallet' exclusively [-Wthread-safety-analysis]
10 if (used && !GetDestData(dst, "address_used", nullptr)) {
11 ^
12wallet/wallet.cpp:851:13: warning: calling function 'AddDestData' requires holding mutex 'cs_wallet' exclusively [-Wthread-safety-analysis]
13 AddDestData(batch, dst, "address_used", "p"); // p for "present", opposite of absent (null)
14 ^
15wallet/wallet.cpp:852:29: warning: calling function 'GetDestData' requires holding mutex 'cs_wallet' exclusively [-Wthread-safety-analysis]
16 } else if (!used && GetDestData(dst, "address_used", nullptr)) {
17 ^
18wallet/wallet.cpp:853:13: warning: calling function 'EraseDestData' requires holding mutex 'cs_wallet' exclusively [-Wthread-safety-analysis]
19 EraseDestData(batch, dst, "address_used");
20 ^
216 warnings generated.
I think should do the following:
is_used member in interfaces::WalletAddressgetAddresses() (src/wallet/interfaces.cpp) is_used should be set - you have the wallet lock thereis_used member to AddressTableEntryis_used to entry in AddressTablePriv::refreshAddressTableis_used is updated accordingly in AddressTablePriv::updateEntryI hope this helps.
846+{
847+ AssertLockHeld(cs_wallet);
848+ CTxDestination dst;
849+ if (ExtractDestination(cout.scriptPubKey, dst) && IsMine(dst)) {
850+ if (used && !GetDestData(dst, "address_used", nullptr)) {
851+ AddDestData(batch, dst, "address_used", "p"); // p for "present", opposite of absent (null)
841@@ -842,6 +842,25 @@ bool CWallet::IsSpentKey(const uint256& hash, unsigned int n) const
842 return false;
843 }
844
845+void CWallet::SetUsedAddressState(WalletBatch& batch, const CTxOut& cout, bool used)
used is always true, maybe drop it?
856+}
857+
858+bool CWallet::IsUsedAddress(const CTxDestination& dst) const
859+{
860+ AssertLockHeld(cs_wallet);
861+ return GetDestData(dst, "address_used", nullptr);
Up for grabs?
Concept ACK
nit: grey if used once and red if used twice or more would have been better
Will test by tomorrow
454@@ -455,6 +455,9 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
455 void SetSpentKeyState(WalletBatch& batch, const uint256& hash, unsigned int n, bool used, std::set<CTxDestination>& tx_destinations) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
456
457 std::vector<OutputGroup> GroupOutputs(const std::vector<COutput>& outputs, const CoinSelectionParams& coin_sel_params, const CoinEligibilityFilter& filter, bool positive_only) const;
458+ //! Whether this address has been used.
459+ void SetUsedAddressState(WalletBatch& batch, const CTxOut& cout, uint256 hash) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
Here and in the definition in the .cpp file, uint256 hash ought to be passed by reference to const, as it is not a cheaply passed type (see usage elsewhere in the codebase).
0 void SetUsedAddressState(WalletBatch& batch, const CTxOut& cout, const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
859@@ -860,6 +860,22 @@ bool CWallet::IsSpentKey(const uint256& hash, unsigned int n) const
860 return false;
861 }
862
863+void CWallet::SetUsedAddressState(WalletBatch& batch, const CTxOut& cout, uint256 hash)
0void CWallet::SetUsedAddressState(WalletBatch& batch, const CTxOut& cout, const uint256& hash)
Thanks for updating! This is a bit of a drive-by review as the hour is late, but I was unable to build (with clang 11 on debian) after rebasing on current master at 7a49fdc :
0wallet/wallet.cpp:878:15: error: out-of-line definition of 'SetUsedAddressState' does not match any declaration in 'CWallet'
1void CWallet::SetUsedAddressState(WalletBatch& batch, const CTxOut& cout, const uint256& hash)
2 ^~~~~~~~~~~~~~~~~~~
3./wallet/wallet.h:460:70: note: type of 3rd parameter of member declaration does not match definition ('uint256' vs 'const uint256 &')
4 void SetUsedAddressState(WalletBatch& batch, const CTxOut& cout, uint256 hash) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
5 ^
6wallet/wallet.cpp:883:9: error: use of undeclared identifier 'AddDestData'
7 AddDestData(batch, dst, "first_txid", hash.ToString());
8 ^
9wallet/wallet.cpp:884:9: error: no matching function for call to object of type 'boost::signals2::signal<void (const CTxDestination &, const std::string &, bool, const std::string &, ChangeType)>' (aka 'signal<void (const variant<CNoDestination, PKHash, ScriptHash, WitnessV0ScriptHash, WitnessV0KeyHash, WitnessV1Taproot, WitnessUnknown> &, const basic_string<char> &, bool, const basic_string<char> &, ChangeType)>')
10 NotifyAddressBookChanged(this, dst, m_address_book[dst].GetLabel(), IsMine(dst) != ISMINE_NO, "receive", CT_UPDATED );
11 ^~~~~~~~~~~~~~~~~~~~~~~~
12/usr/include/boost/signals2/detail/signal_template.hpp:720:19: note: candidate function not viable: requires 5 arguments, but 6 were provided
13 result_type operator ()(BOOST_SIGNALS2_SIGNATURE_FULL_ARGS(BOOST_SIGNALS2_NUM_ARGS))
14 ^
15/usr/include/boost/signals2/detail/signal_template.hpp:724:19: note: candidate function not viable: requires 5 arguments, but 6 were provided
16 result_type operator ()(BOOST_SIGNALS2_SIGNATURE_FULL_ARGS(BOOST_SIGNALS2_NUM_ARGS)) const
17 ^
18wallet/wallet.cpp:891:12: error: use of undeclared identifier 'GetDestData'
19 return GetDestData(dst, "first_txid", nullptr);
20 ^
214 errors generated.
Compiled successfully on Linux (Pop!_OS). Works as expected. Addresses were greyed out once I sent bitcoin to first 2 addresses from the ‘receiving addresses’:
Although if a user cares about privacy, should never open ‘received addresses’ for using. Once a receiving address is created, it should be considered used. Best practice is to create a new address every time you need an address for receiving bitcoin.
879+{
880+ AssertLockHeld(cs_wallet);
881+ CTxDestination dst;
882+ if (ExtractDestination(cout.scriptPubKey, dst) && IsMine(dst)) {
883+
884+ m_address_book[dst].destdata.insert(std::make_pair("first_txid", hash.ToString()));
0 m_address_book[dst].destdata.emplace(std::make_pair("first_txid", hash.ToString()));
233@@ -231,6 +234,16 @@ QVariant AddressTableModel::data(const QModelIndex &index, int role) const
234 } // no default case, so the compiler can warn about missing cases
235 assert(false);
236 }
237+ else if (role == Qt::ForegroundRole) {
238+ if (rec->is_used == true) {
0 if (rec->is_used) {
238+ if (rec->is_used == true) {
239+ return QColor(Qt::gray);
240+ }
241+ }
242+ else if (role == Qt::ToolTipRole) {
243+ if (rec->is_used == true) {
0 if (rec->is_used) {
455@@ -456,6 +456,9 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
456 void SetSpentKeyState(WalletBatch& batch, const uint256& hash, unsigned int n, bool used, std::set<CTxDestination>& tx_destinations) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
457
458 std::vector<OutputGroup> GroupOutputs(const std::vector<COutput>& outputs, const CoinSelectionParams& coin_sel_params, const CoinEligibilityFilter& filter, bool positive_only) const;
459+ //! Whether this address has been used (has received).
460+ void SetAddressReceivedState(WalletBatch& batch, const CTxOut& cout, const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
The first argument of SetAddressReceivedState() seems unused unless I’m missing something.
0 void SetAddressReceivedState(const CTxOut& cout, const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
898+ {
899+ return true;
900+ }
901+ }
902+ return false;
903+}
style nit: clang format and also can simplify
0 bool CWallet::HasAddressReceived(const CTxDestination& dst) const
1 {
2-
3 const std::string key{"first_txid"};
4- std::map<CTxDestination, CAddressBookData>::const_iterator i = m_address_book.find(dst);
5- if(i != m_address_book.end())
6- {
7- CAddressBookData::StringMap::const_iterator j = i->second.destdata.find(key);
8- if(j != i->second.destdata.end())
9- {
10- return true;
11- }
12- }
13- return false;
14+ const std::map<CTxDestination, CAddressBookData>::const_iterator it{m_address_book.find(dst)};
15+ return it != m_address_book.end() && it->second.destdata.find(key) != it->second.destdata.end() ? true : false;
16 }
874@@ -875,6 +875,33 @@ bool CWallet::IsSpentKey(const uint256& hash, unsigned int n) const
875 return false;
876 }
877
878+void CWallet::SetAddressReceivedState(WalletBatch& batch, const CTxOut& cout, const uint256& hash)
879+{
880+ AssertLockHeld(cs_wallet);
881+ CTxDestination dst;
882+ if (ExtractDestination(cout.scriptPubKey, dst) && IsMine(dst)) {
883+
Reviewing bb4790816d84f850367c99134ea6a16659fbb180. This seems pretty good but:
SetAddressReceivedState should be called in LoadToWallet as well as AddToWallet, not just AddToWallet so old and new addresses are displayed correctly, not just ~old~new addresses.SetAddressReceivedState should be called regardless of WALLET_FLAG_AVOID_REUSE flag. The avoid reuse feature is for detecting addresses that we previously spent from (and avoiding spending from them again). This feature is about detecting which addresses we previously received to, which is not directly related, and probably doesn’t need a feature flag since it doesn’t affect the wallet database storage.CAddressBookData::received member instead of adding a "first_txid" entry and transaction hash to the CAddressBookData::destdata mapEDIT: corrected old/new above
It seems like SetAddressReceivedState should be called in LoadToWallet as well as AddToWallet, not just AddToWallet so old and new addresses are displayed correctly, not just old addresses.
I relaunched bitcoin-qt today and my old addresses which had received bitcoin, were marked grey yesterday are not greyed out now:
It seems like SetAddressReceivedState should be called regardless of WALLET_FLAG_AVOID_REUSE flag. The avoid reuse feature is for detecting addresses that we previously spent from (and avoiding spending from them again). This feature is about detecting which addresses we previously received to, which is not directly related, and probably doesn’t need a feature flag since it doesn’t affect the wallet database storage.
Agree
🐙 This pull request conflicts with the target branch and needs rebase.
Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.