refactor: encapsulate wallet's address book access #25337

furszy · 2022-06-11T15:58:47Z

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

fanquake · 2022-06-12T13:01:45Z

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

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

furszy · 2022-06-13T19:02:35Z

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

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

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

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

in the "refactor: use ForEachAddrBookEntry in interfaces::wallet::getAddresses" commit (c629bef) apply the following patch:

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

and maybe skip the last commit?

…y function

furszy · 2022-06-14T15:08:03Z

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.

src/wallet/wallet.h

src/wallet/wallet.cpp

furszy · 2022-06-14T21:09:59Z

Thanks for the review achow101

Updated per feedback. Removed ForEachAddrBookEntry default parameter.

achow101 · 2022-06-14T23:25:12Z

ACK 21c6342

theStack

Concept ACK

src/wallet/wallet.h

DrahtBot · 2022-06-21T00:44:52Z

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.

…estinations lookup

furszy · 2022-06-21T13:25:51Z

Thanks for the feedback!

Only modification is a variable rename filter_change -> ignore_change.

theStack

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

src/wallet/wallet.cpp

src/wallet/rpc/transactions.cpp

… functionality Mainly to not access 'm_address_book' externally.

Plus remove open bracket jump line

furszy · 2022-06-22T16:01:37Z

Thanks! feedback tackled, changes:

Added missing "change" addrs skip in ListReceived (Squashed inside b459fc1).
Added test coverage for point (1) --> verifying that no "change" address is returned by listreceivedbyaddress (d69045e)

theStack

ACK d69045e ✅

achow101 · 2022-07-06T15:10:02Z

ACK d69045e

w0xlt

ACK d69045e

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.

d69045e test: add coverage for 'listreceivedbyaddress' no change addrs return (furszy) 324f00a refactor: 'ListReceived' use optional for filtered address (furszy) b459fc1 refactor: RPC 'ListReceived', encapsulate m_address_book access (furszy) fa9f2ab refactor: RPC 'listlabels', encapsulate 'CWallet::ListAddrBookLabels' functionality (furszy) 83e42c4 refactor: use 'ForEachAddrBookEntry' in RPC 'getaddressesbylabel' (furszy) 2b48642 refactor: use ForEachAddrBookEntry in interfaces::getAddresses (furszy) 032842a wallet: implement ForEachAddrBookEntry method (furszy) 09649bc refactor: implement general 'ListAddrBookAddresses' for addressbook destinations lookup (furszy) 192eb1e refactor: getAddress don't access m_address_book, use FindAddressEntry function (furszy) Pull request description: ### 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). ACKs for top commit: achow101: ACK d69045e theStack: ACK d69045e ✅ w0xlt: ACK bitcoin@d69045e Tree-SHA512: dba17acd86f171b4e9af0223bbbcad380048570f6a2f6a92732a51f01abe8806debaf65c9e9e5569fa76a541903cbb50adcb5f56ef77858151c698ae6b218e2a

ryanofsky · 2022-07-08T00:23:21Z

src/interfaces/wallet.h

@@ -114,7 +114,7 @@ class Wallet
        std::string* purpose) = 0;

    //! Get wallet address list.
-    virtual std::vector<WalletAddress> getAddresses() = 0;
+    virtual std::vector<WalletAddress> getAddresses() const = 0;


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.

k 👍🏼, got it.

DrahtBot added the Refactoring label Jun 11, 2022

fanquake requested a review from achow101 June 12, 2022 13:01

hebasto reviewed Jun 14, 2022

View reviewed changes

refactor: getAddress don't access m_address_book, use FindAddressEntr…

192eb1e

…y function

furszy force-pushed the 2022_encapsulate_addressbook_access branch from 61b4654 to 005d111 Compare June 14, 2022 14:59

achow101 reviewed Jun 14, 2022

View reviewed changes

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

src/wallet/wallet.cpp Show resolved Hide resolved

furszy force-pushed the 2022_encapsulate_addressbook_access branch 2 times, most recently from 3648db9 to 21c6342 Compare June 14, 2022 21:09

theStack reviewed Jun 20, 2022

View reviewed changes

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

furszy added 4 commits June 21, 2022 10:23

refactor: implement general 'ListAddrBookAddresses' for addressbook d…

09649bc

…estinations lookup

wallet: implement ForEachAddrBookEntry method

032842a

refactor: use ForEachAddrBookEntry in interfaces::getAddresses

2b48642

refactor: use 'ForEachAddrBookEntry' in RPC 'getaddressesbylabel'

83e42c4

furszy force-pushed the 2022_encapsulate_addressbook_access branch from 21c6342 to 4be2c0d Compare June 21, 2022 13:24

theStack reviewed Jun 22, 2022

View reviewed changes

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

src/wallet/rpc/transactions.cpp Show resolved Hide resolved

furszy force-pushed the 2022_encapsulate_addressbook_access branch from 4be2c0d to 2e94d08 Compare June 22, 2022 15:49

furszy added 4 commits June 22, 2022 12:51

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

fa9f2ab

… functionality Mainly to not access 'm_address_book' externally.

refactor: RPC 'ListReceived', encapsulate m_address_book access

b459fc1

refactor: 'ListReceived' use optional for filtered address

324f00a

Plus remove open bracket jump line

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

d69045e

furszy force-pushed the 2022_encapsulate_addressbook_access branch from 2e94d08 to d69045e Compare June 22, 2022 15:51

theStack approved these changes Jun 23, 2022

View reviewed changes

DrahtBot mentioned this pull request Jun 29, 2022

wallet: group independent db writes on single batched db transaction #25297

Closed

w0xlt approved these changes Jul 7, 2022

View reviewed changes

achow101 merged commit b9f9ed4 into bitcoin:master Jul 8, 2022

furszy mentioned this pull request Jul 8, 2022

refactor: introduce generic 'Result' class and connect it to CreateTransaction and GetNewDestination #25218

Merged

ryanofsky reviewed Jul 11, 2022

View reviewed changes

furszy mentioned this pull request Jul 15, 2022

wallet: Introduce AddressBookManager #25620

Closed

6 tasks

ryanofsky mentioned this pull request Sep 14, 2022

RPC/Wallet: Access wallets via interfaces::Wallet #26082

Closed

furszy mentioned this pull request Jan 6, 2023

wallet: batch and simplify addressbook migration process #26836

Merged

furszy mentioned this pull request Mar 28, 2023

refactor: Remove CAddressBookData::destdata #27224

Merged

TheCharlatan mentioned this pull request May 14, 2023

kernel: Remove util/system from kernel library, interface_ui from validation. #27636

Merged

furszy deleted the 2022_encapsulate_addressbook_access branch May 27, 2023 01:50

bitcoin locked and limited conversation to collaborators May 26, 2024

refactor: encapsulate wallet's address book access #25337

refactor: encapsulate wallet's address book access #25337

Uh oh!

Conversation

furszy commented Jun 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Problem

Solution

Uh oh!

fanquake commented Jun 12, 2022

Uh oh!

furszy commented Jun 13, 2022

Uh oh!

hebasto 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!

furszy commented Jun 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

furszy commented Jun 14, 2022

Uh oh!

achow101 commented Jun 14, 2022

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented Jun 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

furszy commented Jun 21, 2022

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

furszy commented Jun 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Jul 6, 2022

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Jul 8, 2022

Choose a reason for hiding this comment

Uh oh!

furszy Jul 15, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

furszy commented Jun 11, 2022 •

edited

Loading

hebasto left a comment •

edited

Loading

furszy commented Jun 14, 2022 •

edited

Loading

DrahtBot commented Jun 21, 2022 •

edited

Loading

furszy commented Jun 22, 2022 •

edited

Loading