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

Reviews

See the guideline for information on the review process.

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.

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.

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:

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

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

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

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

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

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

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.

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

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.

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

closed for now