wallet: Introduce AddressBookManager #25620

furszy commented at 1:06 PM on July 15, 2022: member

Finishing the encapsulation work started in #25337. Please review #26836 first.

Context

We are currently using the address book for different purposes and features in the wallet such us:

Store and retrieve own receive addresses with a label.
Store and retrieve external addresses with a label.
Store and retrieve payment requests.
Mark addresses/destinations as 'used' if they appear on the blockchain (part of the “avoid re-use” feature).
Know whether an output is a change or not based on the encoded script.

Some of these points aren’t well known by many because this code has been spread and obfuscated across the wallet’s sources. (and created in a Frankenstein style).

PR Goals

Encapsulate the address book functionalities into its own AddressBookManager class, so we are able to distinguish properly the address book responsibilities and capabilities.

Be able to decouple the address book dependency on the wallet’s main mutex (up to a certain point, because we are still using the same db connection).

Clean up the wallet sources further.

Be able to unit/fuzz test the address book manager implementation isolated from the wallet flows.

And create a new class that can be easily upgraded/replaced, and even different implementations can be used in parallel in different local wallets, by just providing a different manager instance to the wallet in the constructor.

Next steps

After this PR, will be working on improve the ‘change output’ flow which, at least for now, is strictly connected to this process (goal will be to remove the strict dependency).

Pending work (still under WIP, almost there)

Add manager class description.
[ ] Clean CAddressBookData.
Cover the class with unit tests.
Double-check commits order.
Add addrbook purposes enum.
Make cs_addrbook RecursiveMutex a Mutex?

refactor: walletdb load, encapsulate 'm_address_book' access a17e591cd8

wallet: clean redundancies in DelAddressBook

1) Call IsMine only once (instead of two).
2) Encode destination only once (instead of three).
3) Don't remove the entry from the map if db write failed.
4) Notify GUI only if removal succeeded

b76c38e3f7

refactor: SetAddrBookWithDB, signal only if write succeeded 728530be7c

refactor: encapsulate wallet addrbook size c5dd3f901e

refactor: move CAddressBookData to addressbookman.h 4543ff6359

addressbook: introduce addressbook manager

With Put, Find, Has and Delete methods
(Same as it's implemented in the wallet)

59e71aba01

addressbookman: implement ForEachAddrBookEntry 5ac3243c87

addressbookman: implement GetSize, LoadDestData, LoadEntryLabel and LoadEntryPurpose aef6336843

addressbookman: implement SetDestUsed and IsDestUsed methods

Same flow as CWallet::SetAddressUsed and CWallet::IsAddressUsed

6efd225bbf

wallet: connect AddressBookManager 61dfed379a

addressbookman: add GetEntriesByDestDataPrefix and SetEntryDestData 9c406ac53d

wallet: refactor FindAddressBookEntry to return std::optional 20705cc080

DrahtBot added the label Wallet on Jul 15, 2022

DrahtBot commented at 1:43 AM on July 16, 2022: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	S3RK

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#bitcoin-core/gui/723 (Add warnings for non-active addresses in receive tab and address book by pinheadmz)
#27286 (wallet: Keep track of the wallet's own transaction outputs in memory by achow101)
#27224 (refactor: Remove CAddressBookData::destdata by ryanofsky)
#27217 (wallet: Replace use of purpose strings with an enum by achow101)
#26951 (wallet: improve rescan performance 640% by pstratem)
#26836 (wallet: finish addressbook encapsulation by furszy)
#25297 (wallet: group independent db writes on single batched db transaction by furszy)
#24914 (wallet: Load database records in a particular order by achow101)

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.

achow101 commented at 8:13 PM on July 18, 2022: member

The first four commits seem fine and could stand alone, but I'm not sure that introducing an entire class for managing the address book is really necessary. I don't really see how it is more beneficial to have such a class vs just cleaning up existing address book access and making m_address_book private. I don't really foresee a situation where we would want to have different implementations of the address book.

furszy commented at 9:37 PM on July 19, 2022: member

The first four commits seem fine and could stand alone, but I'm not sure that introducing an entire class for managing the address book is really necessary. I don't really see how it is more beneficial to have such a class vs just cleaning up existing address book access and making m_address_book private. I don't really foresee a situation where we would want to have different implementations of the address book.

That point is more a "plus" than part of the short term benefits.

The manager decoupling could worth it mainly for three reasons:

It's another extra responsibility removed from the wallet class. Class that, even with the spend/receive files decoupling, is still a long file that keeps growing. Handling more responsibilities that what should ideally handle.

The CWallet class description says: “CWallet maintains a set of transactions and balances, and provides the ability to create new transactions.”. And.. that is not even 1/10 of what the wallet class does (there is a good phrase that says something like: "Try to sum up what a class does in a single phrase. If this phrase includes “AND” it’s probably doing too much.").

Plus, I’m unsure about how many devs know all the current address book functionalities, the code was a bit spread across the wallet without any proper description.
The new manager cuts the address book dependency on the wallet mutex. So, every walk-through the address book entries (which we do in several RPC commands and on the GUI too), single entry searches, the isAddressUsed and the payment requests retrieval can run wallet mutex free. Which, as we use the wallet mutex for practically everything that happens inside the wallet, seems to be an even nicer property. An easy example is the RPC command ´listlabels´, which will run cs_wallet lock free.
Then a separated class let us add test coverage isolated from the whole wallet class. So, bugs like an unknown address book entry purpose, an entry purpose change from "receive" to "send" (which would be pretty bad), a removal of a “receive” entry, among other negative test scenarios, could be guarded inside the class and test covered without requiring to setup an entire wallet, the signals etc. Note: the purpose field instead of be a hardcoded string on the caller side, could be an enum that internally (only inside the manager class) maps to the string value. Which will be less error prone.

PD: we could still implement all of this inside the wallet class as well (including methods descriptions and what people shouldn't do), but.. is that the best for a maintainability point of view?. Shouldn't the wallet class ideally act more as a flow dispatcher, calling the appropriate classes/functions based on the contextual information? So each inner class contains few properly documented responsibilities and we prevent problems that classes with a large span of responsibilities and activities have (plus, we make the wallet sources more friendly to review).

Saying that, I'm weighting approaches out loud.. I would be fine either way for this. Maybe it's too early to implement the AddressBookManager.

S3RK commented at 6:28 AM on July 21, 2022: contributor

ConceptACK. Separating this responsibility from CWallet class is reducing complexity and introduces useful abstractions. @furszy I noticed that you left original methods to deal with address book in CWallet. Some of the are pure proxies. Is that on purpose? Do you have any further plans here?

furszy commented at 3:32 PM on July 21, 2022: member

@furszy I noticed that you left original methods to deal with address book in CWallet. Some of the are pure proxies. Is that on purpose? Do you have any further plans here?

Yeah, I left them for two main reason: (1) To keep the same wallet interface and (2) To not give callers free will to do anything that want with the AddressBookManager object. Preventing external code to call any write function without triggering the wallet internal signals which are useful to notify to the upper layers (example: the SetAddressBook method triggers the wallet's NotifyAddressBookChanged signal when an entry gets added or updated in the addressbook).

achow101 commented at 8:48 PM on August 2, 2022: member

The new manager cuts the address book dependency on the wallet mutex. So, every walk-through the address book entries (which we do in several RPC commands and on the GUI too), single entry searches, the isAddressUsed and the payment requests retrieval can run wallet mutex free. Which, as we use the wallet mutex for practically everything that happens inside the wallet, seems to be an even nicer property. An easy example is the RPC command ´listlabels´, which will run cs_wallet lock free.

I don't think the presence of cs_wallet precludes having another cs_address_book (or whatever) also in CWallet.

we could still implement all of this inside the wallet class as well (including methods descriptions and what people shouldn't do), but.. is that the best for a maintainability point of view?

I don't really think that having the separate class helps though. AddressBookMan is entirely internal to the wallet, so no callers will have access to its functions (as it should be). It also largely doesn't have its own private functions that its functions regularly call, so it just ends up being a wrapper around m_address_book. We end up keeping all of the same address book functions in CWallet, and actually end up adding even more functions. Those functions in CWallet just pass straight through to AddressBookMan and largely don't do anything interesting. So any change to function signatures now requires changing 4 places, rather than 2 places. IMO that is not more maintainable.

S3RK commented at 6:25 AM on August 8, 2022: contributor

@achow101 why do you think no callers should have access to AddressBookMan?

I agree that having proxy functions doesn't help with maintainability, but I'd just say we need to drop the wrappers in CWallet and let the callers interact with the address book.

furszy commented at 3:48 PM on February 8, 2023: member

Have decoupled the commits that have no conceptual opposition within #26836. So we can move forward with them first. Afterwards, we can re-evaluate whether to rework this PR or not, based on a smaller set of patches.

furszy marked this as a draft on Feb 8, 2023

DrahtBot added the label Needs rebase on Apr 12, 2023

DrahtBot commented at 11:28 AM on April 12, 2023: contributor

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

DrahtBot commented at 1:47 AM on July 11, 2023: contributor

There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

maflcko commented at 4:53 PM on September 20, 2023: member

Are you still working on this? If not, maybe close for now

furszy closed this on Sep 20, 2023

furszy commented at 5:34 PM on September 20, 2023: member

closed for now

bitcoin locked this on Sep 19, 2024