gui: grey out used address in address book #17355

za-kk · 2019-11-02T18:12:59Z

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

luke-jr · 2019-11-02T18:41:48Z

Addresses are used when they receive. Not when the UTXO created is later spent.

src/interfaces/wallet.cpp

src/interfaces/wallet.h

src/interfaces/wallet.cpp

src/qt/addresstablemodel.cpp

za-kk · 2019-11-04T10:50:09Z

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?

luke-jr · 2019-11-04T12:30:18Z

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

jonasschnelli · 2019-11-04T19:30:01Z

Agree with @luke-jr. This is currently conceptual wrong.

maflcko · 2019-11-04T19:51:37Z

So if the existing used-flag is supposed to do what it does (indicate true when the coin sent to that destination has been spent), it could make sense to add a new flag (indicate true when a tx has been received that has the destination in one of its outputs).

luke-jr · 2019-11-04T21:36:58Z

See also #15987, but I'm not sure it's the best approach to use here. If we're going to check every address (as this does), we should probably save a flag in the wallet/address book when we see an address used...

za-kk · 2020-02-11T00:08:56Z

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.

dannmat · 2020-02-11T08:08:12Z

ack

DrahtBot · 2020-02-11T08:35:34Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22100 (refactor: Clean up new wallet spend, receive files added MOVEONLY: CWallet transaction code out of wallet.cpp/.h #21207 by ryanofsky)
#20591 (wallet, bugfix: fix ComputeTimeSmart function during rescanning process. by BitcoinTsunami)
#19461 (multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky)
#19460 (multiprocess: Add bitcoin-wallet -ipcconnect option 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.

luke-jr · 2020-02-23T01:55:53Z

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.

za-kk · 2020-02-24T20:41:02Z

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_reuse wallets as IsUsedDestination was 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?

za-kk · 2020-05-26T23:05:25Z

Rebased with master and squashed commits.

Thinking of picking this back up if it is still a desirable change/feature? Currently it's limited to watch only wallets but as @luke-jr mentioned, this may be desirable to all wallet types

jonasschnelli · 2020-05-29T07:48:41Z

Concept ACK on how this works today. Slightly reviewed the code and it looks good.

Can you please review @luke-jr, @promag?

za-kk · 2021-07-04T17:24:02Z

This should now be ready for review again, I have implemented the suggested changes made by @promag.

ghost · 2021-07-04T18:05:52Z

Concept ACK

nit: grey if used once and red if used twice or more would have been better

Will test by tomorrow

jonatack

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 :

wallet/wallet.cpp:878:15: error: out-of-line definition of 'SetUsedAddressState' does not match any declaration in 'CWallet'
void CWallet::SetUsedAddressState(WalletBatch& batch, const CTxOut& cout, const uint256& hash)
              ^~~~~~~~~~~~~~~~~~~
./wallet/wallet.h:460:70: note: type of 3rd parameter of member declaration does not match definition ('uint256' vs 'const uint256 &')
    void SetUsedAddressState(WalletBatch& batch, const CTxOut& cout, uint256 hash) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
                                                                     ^
wallet/wallet.cpp:883:9: error: use of undeclared identifier 'AddDestData'
        AddDestData(batch, dst, "first_txid", hash.ToString());
        ^
wallet/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)>')
        NotifyAddressBookChanged(this, dst, m_address_book[dst].GetLabel(), IsMine(dst) != ISMINE_NO, "receive", CT_UPDATED );
        ^~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/boost/signals2/detail/signal_template.hpp:720:19: note: candidate function not viable: requires 5 arguments, but 6 were provided
      result_type operator ()(BOOST_SIGNALS2_SIGNATURE_FULL_ARGS(BOOST_SIGNALS2_NUM_ARGS))
                  ^
/usr/include/boost/signals2/detail/signal_template.hpp:724:19: note: candidate function not viable: requires 5 arguments, but 6 were provided
      result_type operator ()(BOOST_SIGNALS2_SIGNATURE_FULL_ARGS(BOOST_SIGNALS2_NUM_ARGS)) const
                  ^
wallet/wallet.cpp:891:12: error: use of undeclared identifier 'GetDestData'
    return GetDestData(dst, "first_txid", nullptr);
           ^
4 errors generated.

src/wallet/wallet.h

src/wallet/wallet.cpp

…set on wallet

za-kk · 2021-07-05T11:51:57Z

Thanks for taking a look @jonatack! seems that some latest merges to master has resulted in changes to destdata (#21353) which was causing the build issues. I have now refactored the code to deal with this (bb47908)

ghost

~~tACK bb47908~~ There are few issues which should be fixed as mentioned in #17355 (review) and confirmed below

ghost · 2021-07-05T18:25:32Z

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.

jonatack

Some comments for your consideration. The pull doesn't necessarily need to be done in one commit depending on how you organise the changes and provided they are hygienic, but the "fix up to make it build" commit probably should be squashed into the relevant changes. Don't hesitate to run clang-format on your changes.

jonatack · 2021-07-06T12:09:21Z

src/wallet/wallet.cpp

+    CTxDestination dst;
+    if (ExtractDestination(cout.scriptPubKey, dst) && IsMine(dst)) {
+
+        m_address_book[dst].destdata.insert(std::make_pair("first_txid", hash.ToString()));


Suggested change

m_address_book[dst].destdata.insert(std::make_pair("first_txid", hash.ToString()));

m_address_book[dst].destdata.emplace(std::make_pair("first_txid", hash.ToString()));

jonatack · 2021-07-06T12:09:44Z

src/qt/addresstablemodel.cpp

@@ -231,6 +234,16 @@ QVariant AddressTableModel::data(const QModelIndex &index, int role) const
        } // no default case, so the compiler can warn about missing cases
        assert(false);
    }
+    else if (role == Qt::ForegroundRole) {
+        if (rec->is_used == true) {


Suggested change

if (rec->is_used == true) {

if (rec->is_used) {

jonatack · 2021-07-06T12:09:53Z

src/qt/addresstablemodel.cpp

+        }
+    }
+    else if (role == Qt::ToolTipRole) {
+        if (rec->is_used == true) {


Suggested change

if (rec->is_used == true) {

if (rec->is_used) {

jonatack · 2021-07-06T12:18:34Z

src/wallet/wallet.h

@@ -456,6 +456,9 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
    void SetSpentKeyState(WalletBatch& batch, const uint256& hash, unsigned int n, bool used, std::set<CTxDestination>& tx_destinations) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);

    std::vector<OutputGroup> GroupOutputs(const std::vector<COutput>& outputs, const CoinSelectionParams& coin_sel_params, const CoinEligibilityFilter& filter, bool positive_only) const;
+    //! Whether this address has been used (has received).
+    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.

Suggested change

void SetAddressReceivedState(WalletBatch& batch, const CTxOut& cout, const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);

void SetAddressReceivedState(const CTxOut& cout, const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);

jonatack · 2021-07-06T12:21:28Z

src/wallet/wallet.cpp

+        }
+    }
+    return false;
+}


style nit: clang format and also can simplify

bool CWallet::HasAddressReceived(const CTxDestination& dst) const { - const std::string key{"first_txid"}; - std::map<CTxDestination, CAddressBookData>::const_iterator i = m_address_book.find(dst); - if(i != m_address_book.end()) - { - CAddressBookData::StringMap::const_iterator j = i->second.destdata.find(key); - if(j != i->second.destdata.end()) - { - return true; - } - } - return false; + const std::map<CTxDestination, CAddressBookData>::const_iterator it{m_address_book.find(dst)}; + return it != m_address_book.end() && it->second.destdata.find(key) != it->second.destdata.end() ? true : false; }

jonatack · 2021-07-06T12:39:55Z

src/wallet/wallet.cpp

+    AssertLockHeld(cs_wallet);
+    CTxDestination dst;
+    if (ExtractDestination(cout.scriptPubKey, dst) && IsMine(dst)) {
+


nit, omit extra line break here and also line 891 below

ryanofsky

Reviewing bb47908. This seems pretty good but:

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~~new addresses.
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.
Would seem more efficient and simpler to a add new bool CAddressBookData::received member instead of adding a "first_txid" entry and transaction hash to the CAddressBookData::destdata map
Could be nice to have some test coverage for this in https://github.com/bitcoin/bitcoin/blob/master/src/qt/test/wallettests.cpp

EDIT: corrected old/new above

ghost · 2021-07-06T14:10:26Z

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

DrahtBot · 2021-09-03T09:28:47Z

🐙 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".}

za-kk · 2021-11-14T15:02:54Z

Thanks for the feedback everyone! Closing this PR for now until I have more time to come back and implement the changes, anyone else is welcome to pick it up in the meantime if desired 😀

fanquake · 2025-07-23T10:40:51Z

Removing "Up for grabs" here. Followup could start in bitcoin-core/gui#528, or the QML repo.

za-kk changed the title ~~Grey out used address in address book~~ gui: grey out used address in address book Nov 2, 2019

fanquake added the GUI label Nov 2, 2019

promag reviewed Nov 2, 2019

View reviewed changes

za-kk force-pushed the oct-19-17174 branch from 05f8cf0 to cf4cb7d Compare November 2, 2019 20:10

za-kk closed this Feb 10, 2020

za-kk force-pushed the oct-19-17174 branch from 69b9443 to 646f0ad Compare February 10, 2020 22:58

za-kk reopened this Feb 11, 2020

This was referenced Feb 11, 2020

wallet: Prefer full destination groups in coin selection #17824

Merged

Remove CWalletTx merging logic from AddToWallet #9381

Merged

za-kk requested a review from promag February 18, 2020 09:38

za-kk mentioned this pull request Feb 22, 2020

scripted-diff: Wallet: Rename incorrectly named *UsedDestination #18193

Merged

DrahtBot mentioned this pull request Mar 24, 2020

wallet: Increase OUTPUT_GROUP_MAX_ENTRIES to 100 #18418

Merged

DrahtBot added the Needs rebase label Apr 17, 2020

za-kk force-pushed the oct-19-17174 branch 3 times, most recently from adb14be to 8ff78cf Compare May 26, 2020 23:01

DrahtBot removed the Needs rebase label May 26, 2020

za-kk force-pushed the oct-19-17174 branch 3 times, most recently from a054712 to 8c9917b Compare May 31, 2021 21:06

DrahtBot removed the Needs rebase label May 31, 2021

DrahtBot mentioned this pull request Jun 1, 2021

refactor: Clean up new wallet spend, receive files added #21207 #22100

Merged

za-kk requested review from jonatack and promag July 4, 2021 17:24

jonatack reviewed Jul 4, 2021

View reviewed changes

src/wallet/wallet.h Outdated Show resolved Hide resolved

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

CoinCornerZakk added 2 commits July 4, 2021 23:32

grey out used addresses in address book if avoid_reuse flag has been …

71587f4

…set on wallet

rework to avoid using AddDestData so it builds

bb47908

za-kk force-pushed the oct-19-17174 branch from 8c9917b to bb47908 Compare July 5, 2021 11:41

ghost approved these changes Jul 5, 2021

View reviewed changes

jonatack reviewed Jul 6, 2021

View reviewed changes

ryanofsky reviewed Jul 6, 2021

View reviewed changes

ghost mentioned this pull request Jul 10, 2021

Sub wallet for dirty coins #22424

Closed

DrahtBot added the Needs rebase label Sep 3, 2021

za-kk closed this Nov 14, 2021

maflcko added the Up for grabs label Nov 14, 2021

fanquake mentioned this pull request Jan 21, 2022

Hide or grey out used addresses bitcoin-core/gui#528

Open

bitcoin locked and limited conversation to collaborators Nov 14, 2022

fanquake removed the Up for grabs label Jul 23, 2025

	m_address_book[dst].destdata.insert(std::make_pair("first_txid", hash.ToString()));
	m_address_book[dst].destdata.emplace(std::make_pair("first_txid", hash.ToString()));

	void SetAddressReceivedState(WalletBatch& batch, const CTxOut& cout, const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
	void SetAddressReceivedState(const CTxOut& cout, const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);

gui: grey out used address in address book #17355

gui: grey out used address in address book #17355

Uh oh!

Conversation

za-kk commented Nov 2, 2019

Uh oh!

luke-jr commented Nov 2, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

za-kk commented Nov 4, 2019

Uh oh!

luke-jr commented Nov 4, 2019

Uh oh!

jonasschnelli commented Nov 4, 2019

Uh oh!

maflcko commented Nov 4, 2019

Uh oh!

luke-jr commented Nov 4, 2019

Uh oh!

za-kk commented Feb 11, 2020

Uh oh!

dannmat commented Feb 11, 2020

Uh oh!

DrahtBot commented Feb 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

luke-jr commented Feb 23, 2020

Uh oh!

za-kk commented Feb 24, 2020

Uh oh!

za-kk commented May 26, 2020

Uh oh!

jonasschnelli commented May 29, 2020

Uh oh!

za-kk commented Jul 4, 2021

Uh oh!

ghost commented Jul 4, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

za-kk commented Jul 5, 2021

Uh oh!

ghost left a comment • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Jul 5, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack left a comment • 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!

jonatack Jul 6, 2021

Choose a reason for hiding this comment

Uh oh!

jonatack Jul 6, 2021

Choose a reason for hiding this comment

Uh oh!

jonatack Jul 6, 2021

Choose a reason for hiding this comment

Uh oh!

jonatack Jul 6, 2021

Choose a reason for hiding this comment

Uh oh!

jonatack Jul 6, 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!

jonatack Jul 6, 2021

Choose a reason for hiding this comment

DrahtBot commented Feb 11, 2020 •

edited

Loading

ghost commented Jul 4, 2021 •

edited by ghost

Loading

ghost left a comment •

edited by ghost

Loading

ghost commented Jul 5, 2021 •

edited by ghost

Loading

jonatack left a comment •

edited

Loading

jonatack Jul 6, 2021 •

edited

Loading

ryanofsky left a comment •

edited

Loading