refactor: encapsulate wallet's address book access

furszy commented at 3:58 pm on June 11, 2022: member

Context

The wallet’s m_address_book field is being accessed directly from several places across the sources.

Problem

Code structure wise, we shouldn’t be accessing it directly. It could end up being modified by mistake (from a place that has nothing to do with the wallet like an RPC command or the GUI) and cause a bigger issue: like an address book entry ‘purpose’ string change, which if done badly (from ‘send’ to ‘receive’), could end up in a user sharing a “receive” address that he/she doesn’t own.

Solution

Encapsulate m_address_book access inside the wallet.

Extra Note:

This is the initial step towards decoupling the address book functionality from the wallet’s sources. In other words, the creation of the AddressBookManager (which will be coming in a follow-up PR).

DrahtBot added the label Refactoring on Jun 11, 2022

fanquake requested review from achow101 on Jun 12, 2022

fanquake commented at 1:01 pm on June 12, 2022: member

https://github.com/bitcoin/bitcoin/pull/25337/checks?check_run_id=6844212884:

0  CXX      wallet/rpc/libbitcoin_wallet_a-coins.o
1wallet/interfaces.cpp:213:49: error: calling function 'IsMine' requires holding mutex 'm_wallet->cs_wallet' exclusively [-Werror,-Wthread-safety-analysis]
2            result.emplace_back(dest, m_wallet->IsMine(dest), label, purpose);
3                                                ^

furszy commented at 7:02 pm on June 13, 2022: member

Thanks @fanquake 👍🏼.

Strangely, cs_wallet is actually locked there.. but well, It should be fine now, ended up cleaning a good number of external cs_wallet locks with 61b4654.

hebasto commented at 12:40 pm on June 14, 2022: member

Wrt thread-safety annotations, I’d suggest to:

rebase this PR to include bitcoin/bitcoin#24931
ensure that every commit does not introduce new -Wthread-safety warnings when compiling with clang
in the “wallet: implement ForEachAddrBookEntry method” commit (ead512a3d4079b60cc14b88e9076d51f15a6f7e6) apply the following patch:

 0--- a/src/wallet/wallet.h
 1+++ b/src/wallet/wallet.h
 2@@ -653,7 +653,8 @@ public:
 3      * Stops when the provided 'ListAddrBookFunc' returns false.
 4      */
 5     using ListAddrBookFunc = std::function<void(const CTxDestination& dest, const std::string& label, const std::string& purpose, bool is_change)>;
 6-    void ForEachAddrBookEntry(const ListAddrBookFunc& func = [](const CTxDestination&, const std::string&, const std::string&, bool is_change){ return; }) const;
 7+    void ForEachAddrBookEntry(const ListAddrBookFunc& func = [](const CTxDestination&, const std::string&, const std::string&, bool is_change){ return; }) const
 8+        EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
 9 
10     /**
11      * Marks all outputs in each one of the destinations dirty, so their cache is

in the “refactor: use ForEachAddrBookEntry in interfaces::wallet::getAddresses” commit (c629bef63135719f4dc6b301788198f557a2ac7d) apply the following patch:

 0--- a/src/wallet/interfaces.cpp
 1+++ b/src/wallet/interfaces.cpp
 2@@ -208,7 +208,7 @@ public:
 3     {
 4         LOCK(m_wallet->cs_wallet);
 5         std::vector<WalletAddress> result;
 6-        m_wallet->ForEachAddrBookEntry([&](const CTxDestination& dest, const std::string& label, const std::string& purpose, bool is_change) {
 7+        m_wallet->ForEachAddrBookEntry([&](const CTxDestination& dest, const std::string& label, const std::string& purpose, bool is_change) EXCLUSIVE_LOCKS_REQUIRED(m_wallet->cs_wallet) {
 8             if (is_change) return;
 9             result.emplace_back(dest, m_wallet->IsMine(dest), label, purpose);
10         });

and maybe skip the last commit?

refactor: getAddress don't access m_address_book, use FindAddressEntry function 192eb1e61c

furszy force-pushed on Jun 14, 2022

furszy commented at 3:08 pm on June 14, 2022: member

Thanks for the review hebasto!

Great suggestions 👌🏼 , have applied them all.

ensure that every commit does not introduce new -Wthread-safety warnings when compiling with clang

Ensured. Have compiled every commit with –enable-debug`.

and maybe skip the last commit?

Done. Agree that it’s not related to the scope of this work. Will push it in another PR.

in src/wallet/wallet.h:656 in 1db894772f outdated

647@@ -648,6 +648,14 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
648      */
649     std::vector<CTxDestination> ListAddrBookAddresses(const std::optional<AddrBookFilter>& filter) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
650 
651+    /**
652+     * Walk-through the address book entries.
653+     * Stops when the provided 'ListAddrBookFunc' returns false.
654+     */
655+    using ListAddrBookFunc = std::function<void(const CTxDestination& dest, const std::string& label, const std::string& purpose, bool is_change)>;
656+    void ForEachAddrBookEntry(const ListAddrBookFunc& func = [](const CTxDestination&, const std::string&, const std::string&, bool is_change){ return; }) const

achow101 commented at 8:31 pm on June 14, 2022:

In 1db894772fb76d3469410683fc25f1c9721327cd “wallet: implement ForEachAddrBookEntry method”

I don’t think it’s necessary to have this default parameter for func. It isn’t used and logically there is no reason why ForEachAddrBookEntry would ever be called without a specific func.

furszy commented at 9:01 pm on June 14, 2022:

yeah, good catch

in src/wallet/wallet.cpp:2365 in 38b871da82 outdated

2361@@ -2362,7 +2362,7 @@ std::vector<CTxDestination> CWallet::ListAddrBookAddresses(const std::optional<A
2362     AssertLockHeld(cs_wallet);
2363     std::vector<CTxDestination> result;
2364     AddrBookFilter filter = _filter ? *_filter : AddrBookFilter();
2365-    ForEachAddrBookEntry([&result, &filter](const CTxDestination& dest, const std::string& label, const std::string& purpose, bool is_change){
2366+    ForEachAddrBookEntry([&result, &filter](const CTxDestination& dest, const std::string& label, const std::string& purpose, bool is_change) {

achow101 commented at 8:33 pm on June 14, 2022:

In 38b871da82cfc7fe74cc7f844c8c6636a0a0234c “refactor: RPC ’listlabels’, encapsulate ‘CWallet::ListAddrBookLabels’ functionality”

nit: This change should be in 1db894772fb76d3469410683fc25f1c9721327cd “wallet: implement ForEachAddrBookEntry method”

furszy force-pushed on Jun 14, 2022

furszy commented at 9:09 pm on June 14, 2022: member

Thanks for the review achow101

Updated per feedback. Removed ForEachAddrBookEntry default parameter.

achow101 commented at 11:25 pm on June 14, 2022: member

ACK 21c6342555f4e037f51644c4b9e77caa0e3c5fa3

in src/wallet/wallet.h:643 in 21c6342555 outdated

639+    // Filter struct for 'ListAddrBookAddresses'
640+    struct AddrBookFilter {
641+        // Fetch addresses with the provided label
642+        std::optional<std::string> m_op_label{std::nullopt};
643+        // Always filter change addresses by default
644+        bool filter_change{true};

theStack commented at 8:15 pm on June 20, 2022:

nit: The usage of the verb “filter” is potentially confusing here, as it’s not clear whether it means inclusion or exclusion; if the filter structure has m_op_label set, it means to only end up with addresses of that label, but if filter_change is true, all change addresses are ignored (i.e. the opposite logic). Probably use something like

0        // Never include change addresses by default
1        bool ignore_change{true};

furszy commented at 1:24 pm on June 21, 2022:

sure, updated.

theStack commented at 8:19 pm on June 20, 2022: contributor

Concept ACK

DrahtBot commented at 0:44 am on June 21, 2022: 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:

#25297 (WIP, wallet: speedup transactions sync, rescan and load not flushing to db constantly by furszy)

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.

refactor: implement general 'ListAddrBookAddresses' for addressbook destinations lookup 09649bc95d

wallet: implement ForEachAddrBookEntry method 032842ae41

refactor: use ForEachAddrBookEntry in interfaces::getAddresses 2b48642499

refactor: use 'ForEachAddrBookEntry' in RPC 'getaddressesbylabel' 83e42c4b94

furszy force-pushed on Jun 21, 2022

furszy commented at 1:25 pm on June 21, 2022: member

Thanks for the feedback!

Only modification is a variable rename filter_change -> ignore_change.

in src/wallet/wallet.cpp:2387 in 4be2c0daaa outdated

2391+        if (_is_change) return;
2392+        if (purpose.empty() || _purpose == purpose) {
2393+            label_set.insert(_label);
2394+        }
2395+    });
2396+   return label_set;

theStack commented at 0:29 am on June 22, 2022:

nit: missing space

0    return label_set;

furszy commented at 3:53 pm on June 22, 2022:

done

in src/wallet/rpc/transactions.cpp:140 in 4be2c0daaa outdated

150-    {
151-        if (item_it->second.IsChange()) continue;
152-        const CTxDestination& address = item_it->first;
153-        const std::string& label = item_it->second.GetLabel();
154+    const auto& func = [&](const CTxDestination& address, const std::string& label, const std::string& purpose, bool is_change) {
155         auto it = mapTally.find(address);

theStack commented at 0:37 am on June 22, 2022:

To match the previous behaviour of the iteration loop, I think change addresses should be ignored?

0        if (is_change) return;
1        auto it = mapTally.find(address);

furszy commented at 3:55 pm on June 22, 2022:

Good catch 👌🏼 Added fix inside b459fc12, and test coverage for it in d69045e

theStack commented at 0:38 am on June 22, 2022: contributor

Almost-ACK, changes look good to me. Just two suggestions below, one nit, one potential behaviour change.

furszy force-pushed on Jun 22, 2022

refactor: RPC 'listlabels', encapsulate 'CWallet::ListAddrBookLabels' functionality

Mainly to not access 'm_address_book' externally.

fa9f2ab8fd

refactor: RPC 'ListReceived', encapsulate m_address_book access b459fc122f

refactor: 'ListReceived' use optional for filtered address

Plus remove open bracket jump line

324f00a642

test: add coverage for 'listreceivedbyaddress' no change addrs return d69045e291

furszy force-pushed on Jun 22, 2022

furszy commented at 4:01 pm on June 22, 2022: member

Thanks! feedback tackled, changes:

Added missing “change” addrs skip in ListReceived (Squashed inside b459fc12).
Added test coverage for point (1) –> verifying that no “change” address is returned by listreceivedbyaddress (d69045e)

theStack approved

theStack commented at 3:47 pm on June 23, 2022: contributor

ACK d69045e291e32e02d105d1b5ff1c8b86db0ae69e ✅

achow101 commented at 3:10 pm on July 6, 2022: member

ACK d69045e291e32e02d105d1b5ff1c8b86db0ae69e

w0xlt approved

w0xlt commented at 11:51 pm on July 7, 2022: contributor

ACK https://github.com/bitcoin/bitcoin/pull/25337/commits/d69045e291e32e02d105d1b5ff1c8b86db0ae69e

It’s an improvement in code organization, as ideally only CWallet would be aware of m_address_book. This field is still called in walletdb.cpp and wallettool.cpp, but may eventually be handled in another PR.

in src/interfaces/wallet.h:117 in d69045e291

113@@ -114,7 +114,7 @@ class Wallet
114         std::string* purpose) = 0;
115 
116     //! Get wallet address list.
117-    virtual std::vector<WalletAddress> getAddresses() = 0;
118+    virtual std::vector<WalletAddress> getAddresses() const = 0;

ryanofsky commented at 0:23 am on July 8, 2022:

Just for future reference, virtual src/interfaces/ methods should not be const since it breaks multiprocess code in #10102, which adds alternate implementation of these interfaces which do IPC, and don’t wrap local CWallets.

In general virtual const methods don’t make sense unless you can guarantee that every override of the virtual method can const, which is not something you normally know.

furszy commented at 1:20 am on July 15, 2022:

k 👍🏼, got it.

achow101 merged this on Jul 8, 2022

achow101 closed this on Jul 8, 2022

sidhujag referenced this in commit 79de186eda on Jul 8, 2022

furszy deleted the branch on May 27, 2023

bitcoin locked this on May 26, 2024

refactor: encapsulate wallet’s address book access #25337

Context

Problem

Solution

Conflicts