gui: grey out used address in address book #17355

za-kk commented at 6:12 PM on November 2, 2019: contributor

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 <img width="840" alt="Screenshot 2019-11-02 at 17 36 51" src="https://user-images.githubusercontent.com/31032215/68075122-897dec00-fd9b-11e9-95ac-4a8f36635cc9.png">

pr <img width="836" alt="Screenshot 2019-11-02 at 17 33 41" src="https://user-images.githubusercontent.com/31032215/68075130-9e5a7f80-fd9b-11e9-93e3-6d687b982c34.png">

za-kk renamed this:
~~Grey out used address in address book~~
gui: grey out used address in address book
on Nov 2, 2019

fanquake added the label GUI on Nov 2, 2019

in src/interfaces/wallet.cpp:205 in 7d1d94d1fe outdated

 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

promag commented at 6:37 PM on November 2, 2019:

There was some discussion elsewhere which, IIRC, say you should use isUsedDestination instead.

za-kk commented at 7:25 PM on November 2, 2019:

yes, makes sense as it follows the same convention as the rest of the file. will update along with other nits

za-kk commented at 8:11 PM on November 2, 2019:

fixed

in src/interfaces/wallet.h:135 in 7d1d94d1fe outdated

 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.

promag commented at 6:39 PM on November 2, 2019:

The comment is not clear to me, what you mean by "this"?

za-kk commented at 7:07 PM on November 2, 2019:

this comment was carried over from the original IsUsedDestination method in the wallet/wallet.h file

in src/interfaces/wallet.cpp:207 in 7d1d94d1fe outdated

 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();

promag commented at 6:39 PM on November 2, 2019:

Why are you locking this?

za-kk commented at 7:28 PM on November 2, 2019:

sorry, think i misunderstood what this line did, will remove

za-kk commented at 8:11 PM on November 2, 2019:

this has now been removed

in src/qt/addresstablemodel.cpp:231 in 7d1d94d1fe outdated

 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)

promag commented at 6:41 PM on November 2, 2019:

nit, here and below if (...) {.

za-kk commented at 8:11 PM on November 2, 2019:

fixed

in src/qt/addresstablemodel.cpp:233 in 7d1d94d1fe outdated

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

promag commented at 6:41 PM on November 2, 2019:

nit, space after if.

za-kk commented at 8:11 PM on November 2, 2019:

fixed

luke-jr commented at 6:41 PM on November 2, 2019: member

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

za-kk force-pushed on Nov 2, 2019

za-kk commented at 10:50 AM on November 4, 2019: contributor

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 commented at 12:30 PM on November 4, 2019: member

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 commented at 7:30 PM on November 4, 2019: contributor

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

maflcko commented at 7:51 PM on November 4, 2019: member

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 commented at 9:36 PM on November 4, 2019: member

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 closed this on Feb 10, 2020

za-kk force-pushed on Feb 10, 2020

za-kk reopened this on Feb 11, 2020

za-kk commented at 12:08 AM on February 11, 2020: contributor

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 commented at 8:08 AM on February 11, 2020: none

ack

DrahtBot commented at 8:35 AM on February 11, 2020: contributor

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

za-kk requested review from promag on Feb 18, 2020

luke-jr commented at 1:55 AM on February 23, 2020: member

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 commented at 8:41 PM on February 24, 2020: contributor

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?

DrahtBot added the label Needs rebase on Apr 17, 2020

za-kk force-pushed on May 26, 2020

za-kk commented at 11:05 PM on May 26, 2020: contributor

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

DrahtBot removed the label Needs rebase on May 26, 2020

jonasschnelli commented at 7:48 AM on May 29, 2020: contributor

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

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

maflcko commented at 11:57 AM on May 29, 2020: member

Open-Close to re-run ci. See #15847 (comment)

maflcko closed this on May 29, 2020

maflcko reopened this on May 29, 2020

za-kk force-pushed on May 30, 2020

in src/wallet/wallet.cpp:791 in 3893e266c5 outdated

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

promag commented at 12:29 AM on June 1, 2020:

Please update code format accordingly to developer notes.

za-kk commented at 9:12 PM on June 6, 2020:

Updated according to developer notes (added space after if), hopefully I haven't missed anything, Thanks!

jonatack commented at 7:13 AM on June 7, 2020:

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

za-kk commented at 1:22 PM on June 7, 2020:

Thanks @jonatack, that will definitely be useful for me to use in the future!

in src/wallet/wallet.cpp:805 in 3893e266c5 outdated

 800 | +}
 801 | +
 802 | +bool CWallet::IsUsedAddress(const CTxDestination& dst) const
 803 | +{
 804 | +    LOCK(cs_wallet);
 805 | +    return IsMine(dst) && GetDestData(dst, "address_used", nullptr);

promag commented at 12:31 AM on June 1, 2020:

Is it me or IsMine is irrelevant?

Either way call GetDestData since it's faster and most likely to fail.

za-kk commented at 9:11 PM on June 6, 2020:

Yes IsMine would be irrelevant in this case, have now removed to just return the output of GetDestData as suggested, Thanks!

promag commented at 12:34 AM on June 1, 2020: contributor

Maybe add a tooltip on the greyed rows?

za-kk force-pushed on Jun 6, 2020

za-kk commented at 10:24 PM on June 6, 2020: contributor

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

address-tooltip

in src/wallet/wallet.cpp:887 in 53f0d0b428 outdated

 795 | +            } else if (!used && GetDestData(dst, "address_used", nullptr)) {
 796 | +                EraseDestData(batch, dst, "address_used");
 797 | +            }
 798 | +        }
 799 | +    }
 800 | +}

jonatack commented at 8:23 AM on June 7, 2020:

perhaps simplify

 {
     AssertLockHeld(cs_wallet);
     CTxDestination dst;
-    if (ExtractDestination(cout.scriptPubKey, dst)) {
-        if (IsMine(dst)) {
-            if (used && !GetDestData(dst, "address_used", nullptr)) {
-                AddDestData(batch, dst, "address_used", "p"); // p for "present", opposite of absent (null)
-            } else if (!used && GetDestData(dst, "address_used", nullptr)) {
-                EraseDestData(batch, dst, "address_used");
-            }
+    if (ExtractDestination(cout.scriptPubKey, dst) && IsMine(dst)) {
+        if (used && !GetDestData(dst, "address_used", nullptr)) {
+            AddDestData(batch, dst, "address_used", "p"); // p for "present", opposite of absent (null)
+        } else if (!used && GetDestData(dst, "address_used", nullptr)) {
+            EraseDestData(batch, dst, "address_used");
         }
     }
 }

za-kk commented at 1:28 PM on June 7, 2020:

Yes makes sense, this has now been simplified as suggested

in src/qt/addresstablemodel.cpp:233 in 53f0d0b428 outdated

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

jonatack commented at 8:37 AM on June 7, 2020:

Suggestion: perhaps gray to be more compatible with dark mode.

gray: Screenshot from 2020-06-07 10-47-18

light gray: Screenshot from 2020-06-07 10-41-39

za-kk commented at 1:29 PM on June 7, 2020:

Thanks for testing with dark mode! have now updated the code to use gray instead of lightGray to accommodate both light and dark modes

in src/wallet/wallet.cpp:860 in 53f0d0b428 outdated

 799 | +    }
 800 | +}
 801 | +
 802 | +bool CWallet::IsUsedAddress(const CTxDestination& dst) const
 803 | +{
 804 | +    LOCK(cs_wallet);

jonatack commented at 8:51 AM on June 7, 2020:

Is this lock needed?

za-kk commented at 1:32 PM on June 7, 2020:

Probably not needed as GetDestData is just accessing the address book data. I have now removed the lock.

jonatack commented at 9:05 AM on June 8, 2020:

Sorry about that; it appears to be needed. Clang build output:

wallet/wallet.cpp:803:12: error: calling function 'GetDestData' requires
holding mutex 'cs_wallet' exclusively [-Werror,-Wthread-safety-analysis]
    return GetDestData(dst, "address_used", nullptr);
           ^
1 error generated.

za-kk commented at 6:54 PM on June 8, 2020:

My bad, should have checked properly myself before removing! This has now been updated to include the lock again.

jonatack commented at 8:55 AM on June 7, 2020: member

Concept ACK -- nice!

Builds cleanly and works as advertised, including the tooltip.

za-kk force-pushed on Jun 7, 2020

za-kk commented at 1:34 PM on June 7, 2020: contributor

Thanks for reviewing the code @jonatack! I have now made the changes suggested and squashed my commits.

jonatack commented at 1:53 PM on June 7, 2020: member

Thanks for the color change. I only use the GUI in dark mode :). I don't think you needed to squash the two commits and 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.

promag commented at 9:17 AM on June 8, 2020: contributor

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 force-pushed on Jun 8, 2020

za-kk commented at 6:55 PM on June 8, 2020: contributor

Thanks for reviewing @promag! makes sense to avoid locking up the GUI wherever possible, I will look into updating the code to follow the process you mentioned instead.

hebasto commented at 2:05 PM on June 29, 2020: member

Concept ACK. @za-kk Are you still working on this? Are you going to implement @promag's suggestion?

za-kk commented at 2:09 PM on June 29, 2020: contributor

Thanks for the ACK @hebasto, yes I am still working on this and plan to implement the suggestions made by @promag soon 👍🏻

jonatack commented at 5:23 PM on July 12, 2020: member

Hi @za-kk, let us know if you're having any issue with progress here :rocket:

fanquake added the label Wallet on Jul 15, 2020

jarolrod commented at 1:46 AM on November 19, 2020: member

@za-kk are you going to implement promags suggestion, or can I pick this up?

hebasto commented at 5:36 AM on November 19, 2020: member

@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:

introduce new CWallet member functions and add tests for them (in this repo)
GUI changes (in the gui repo)

za-kk commented at 11:07 AM on November 19, 2020: contributor

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

promag commented at 11:47 AM on November 19, 2020: contributor

@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 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 commented at 1:24 PM on November 19, 2020: member

@promag

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

I'm fine with keeping all changes and discussion here.

New introduced functions still require test coverage (probably, in a follow up).

za-kk force-pushed on Nov 25, 2020

DrahtBot added the label Needs rebase on Dec 1, 2020

za-kk force-pushed on Dec 2, 2020

DrahtBot removed the label Needs rebase on Dec 2, 2020

in src/wallet/wallet.cpp:880 in a5ee7558b3 outdated

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

adamjonas commented at 10:36 PM on December 9, 2020:

I'm getting compiler warnings consistent with the CI:

wallet/wallet.cpp:847:5: warning: calling function 'AssertLockHeldInternal<AnnotatedMixin<std::__1::recursive_mutex> >' requires holding mutex 'cs_wallet' exclusively [-Wthread-safety-analysis]
    AssertLockHeld(cs_wallet);
    ^
./sync.h:81:28: note: expanded from macro 'AssertLockHeld'
#define AssertLockHeld(cs) AssertLockHeldInternal(#cs, __FILE__, __LINE__, &cs)
                           ^
wallet/wallet.cpp:849:55: warning: calling function 'IsMine' requires holding mutex 'cs_wallet' exclusively [-Wthread-safety-analysis]
    if (ExtractDestination(cout.scriptPubKey, dst) && IsMine(dst)) {
                                                      ^
wallet/wallet.cpp:850:22: warning: calling function 'GetDestData' requires holding mutex 'cs_wallet' exclusively [-Wthread-safety-analysis]
        if (used && !GetDestData(dst, "address_used", nullptr)) {
                     ^
wallet/wallet.cpp:851:13: warning: calling function 'AddDestData' requires holding mutex 'cs_wallet' exclusively [-Wthread-safety-analysis]
            AddDestData(batch, dst, "address_used", "p"); // p for "present", opposite of absent (null)
            ^
wallet/wallet.cpp:852:29: warning: calling function 'GetDestData' requires holding mutex 'cs_wallet' exclusively [-Wthread-safety-analysis]
        } else if (!used && GetDestData(dst, "address_used", nullptr)) {
                            ^
wallet/wallet.cpp:853:13: warning: calling function 'EraseDestData' requires holding mutex 'cs_wallet' exclusively [-Wthread-safety-analysis]
            EraseDestData(batch, dst, "address_used");
            ^
6 warnings generated.

za-kk commented at 12:32 AM on February 7, 2021:

Thanks for taking a look @adamjonas, this should now be resolved in latest commit

promag commented at 12:57 AM on December 11, 2020: contributor

I think should do the following:

start by adding a new is_used member in interfaces::WalletAddress
then in getAddresses() (src/wallet/interfaces.cpp) is_used should be set - you have the wallet lock there
add a is_used member to AddressTableEntry
copy is_used to entry in AddressTablePriv::refreshAddressTable
ensure is_used is updated accordingly in AddressTablePriv::updateEntry
expose in the model (like you do in this PR) but read data only from the record.

I hope this helps.

in src/wallet/wallet.cpp:851 in 9c1aa72752 outdated

 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)

promag commented at 12:04 AM on December 12, 2020:

Could save txid instead of "p" and maybe reword the key to "first_txid" so that we could show the user that transaction.

za-kk commented at 12:32 AM on February 7, 2021:

This should be implemented in latest commit

in src/wallet/wallet.cpp:845 in 9c1aa72752 outdated

 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)

promag commented at 12:04 AM on December 12, 2020:

used is always true, maybe drop it?

za-kk commented at 5:22 PM on July 4, 2021:

This has been resolved in the latest commit, thanks!

in src/wallet/wallet.cpp:861 in 9c1aa72752 outdated

 856 | +}
 857 | +
 858 | +bool CWallet::IsUsedAddress(const CTxDestination& dst) const
 859 | +{
 860 | +    AssertLockHeld(cs_wallet);
 861 | +    return GetDestData(dst, "address_used", nullptr);

promag commented at 12:06 AM on December 12, 2020:

An existing wallet will not have these set, ignore this fact? Or just add a release note saying that a rescan fixes this?

za-kk commented at 12:33 AM on February 7, 2021:

Yes I think the best solution would be adding a release note as you suggested, I will look into doing this shortly

hebasto commented at 9:20 PM on December 26, 2020: member

Hi @za-kk, let us know if you're having any issue with progress here

DrahtBot added the label Needs rebase on Feb 1, 2021

za-kk force-pushed on Feb 6, 2021

za-kk commented at 12:31 AM on February 7, 2021: contributor

Hi All, I finally had chance to implement the changes suggested by @promag. I notice that the test wallet_avoidreuse.py is currently failing in CI, I will look into resolving this.

DrahtBot removed the label Needs rebase on Feb 7, 2021

za-kk force-pushed on Feb 8, 2021

hebasto commented at 3:15 AM on March 26, 2021: member

@za-kk Are you still working on this?

Any update?

DrahtBot added the label Needs rebase on May 25, 2021

hebasto commented at 7:55 PM on May 29, 2021: member

@MarcoFalke @meshcollider

Up for grabs?

za-kk commented at 8:03 PM on May 29, 2021: contributor

Apologies for the delay on this PR, I do plan on picking it back up very shortly.

Always willing to accept help/advice to move things forward @hebasto 👍🏻

za-kk force-pushed on May 31, 2021

DrahtBot removed the label Needs rebase on May 31, 2021

za-kk commented at 5:24 PM on July 4, 2021: contributor

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

za-kk requested review from jonatack on Jul 4, 2021

za-kk requested review from promag on Jul 4, 2021

ghost commented at 6:05 PM on July 4, 2021: none

Concept ACK

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

Will test by tomorrow

in src/wallet/wallet.h:459 in 8c9917b86a outdated

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

jonatack commented at 10:07 PM on July 4, 2021:

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

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

https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#f16-for-in-parameters-pass-cheaply-copied-types-by-value-and-others-by-reference-to-const

za-kk commented at 11:43 AM on July 5, 2021:

Thanks @jonatack, this should now be resolved in bb4790816d84f850367c99134ea6a16659fbb180

in src/wallet/wallet.cpp:863 in 8c9917b86a outdated

 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)

jonatack commented at 10:08 PM on July 4, 2021:

void CWallet::SetUsedAddressState(WalletBatch& batch, const CTxOut& cout, const uint256& hash)

za-kk commented at 11:44 AM on July 5, 2021:

Thanks @jonatack, this should now be resolved in bb4790816d84f850367c99134ea6a16659fbb180

jonatack commented at 10:14 PM on July 4, 2021: member

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.

grey out used addresses in address book if avoid_reuse flag has been set on wallet 71587f4f43

rework to avoid using AddDestData so it builds bb4790816d

za-kk force-pushed on Jul 5, 2021

za-kk commented at 11:51 AM on July 5, 2021: contributor

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 (bb4790816d84f850367c99134ea6a16659fbb180)

unknown approved

unknown commented at 6:25 PM on July 5, 2021: none

tACK https://github.com/bitcoin/bitcoin/pull/17355/commits/bb4790816d84f850367c99134ea6a16659fbb180 There are few issues which should be fixed as mentioned in #17355#pullrequestreview-699943793 and confirmed below

ghost commented at 6:25 PM on July 5, 2021: none

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.

in src/wallet/wallet.cpp:884 in bb4790816d

 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()));

jonatack commented at 12:09 PM on July 6, 2021:

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

in src/qt/addresstablemodel.cpp:238 in bb4790816d

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

jonatack commented at 12:09 PM on July 6, 2021:

        if (rec->is_used) {

in src/qt/addresstablemodel.cpp:243 in bb4790816d

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

jonatack commented at 12:09 PM on July 6, 2021:

        if (rec->is_used) {

in src/wallet/wallet.h:460 in bb4790816d

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

jonatack commented at 12:18 PM on July 6, 2021:

The first argument of SetAddressReceivedState() seems unused unless I'm missing something.

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

in src/wallet/wallet.cpp:903 in bb4790816d

 898 | +        {
 899 | +            return true;
 900 | +        }
 901 | +    }
 902 | +    return false;
 903 | +}

jonatack commented at 12:21 PM on July 6, 2021:

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 commented at 12:30 PM on July 6, 2021: member

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.

in src/wallet/wallet.cpp:883 in bb4790816d

 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 | +

jonatack commented at 12:39 PM on July 6, 2021:

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

ryanofsky commented at 1:20 PM on July 6, 2021: contributor

Reviewing bb4790816d84f850367c99134ea6a16659fbb180. 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 commented at 2:10 PM on July 6, 2021: none

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 added the label Needs rebase on Sep 3, 2021

DrahtBot commented at 9:28 AM on September 3, 2021: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

<sub>Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".</sub>

za-kk commented at 3:02 PM on November 14, 2021: contributor

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 😀

za-kk closed this on Nov 14, 2021

maflcko added the label Up for grabs on Nov 14, 2021

bitcoin locked this on Nov 14, 2022

fanquake removed the label Up for grabs on Jul 23, 2025

fanquake commented at 10:40 AM on July 23, 2025: member

Removing "Up for grabs" here. Followup could start in https://github.com/bitcoin-core/gui/issues/528, or the QML repo.