[RFC] Wallet Class Structure Changes #16165

A writeup of the proposed changes to the wallet class structure can be found on the devwiki: https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Class-Structure-Changes

Please make comment about it here and suggest/make changes if necessary.

The wiki page mentions "address types" lots of places. Can it be updated to clarify specifically what the address types are in each context? I can't figure out which places it is referring to legacy/bech32/p2sh segwit types, or change / non-change types, or both.

@ryanofsky there would be option for all six combinations of change/address. They might all point to the same SPKManager, or all different.

Thanks, and concept ACK from me! I think the design description is clear and seems workable. The prerequisites section of the page is probably the weakest part and might be confusing to anyone who didn't overhear achow and meshcollider debating it. But it's less important because it's only describing temporary steps. @sipa, it's probably be good if you could read the wiki page and provide basic feedback. Like whether it seems fine, or has any obvious problems. And whether it's clear, or are has parts that are confusing or ambiguous.

It might be worth incorporating information from sipa's wallet_and_segwit.md gist https://gist.github.com/sipa/125cfa1615946d0c3f3eec2ad7f250a2 to the new document. Or linking to it. Or updating it and making it a wiki page of its own.

I think a lot of things in the wiki page cover stuff discussed in that gist. I've added a link to it in the wiki page.

Concept ACK.

I'm not sure that ScriptPubKeyManager needs to inherit SigningProvider directly; doing so may require having a lot of information permanently cached (in order to be able to derive all scripts on the fly, for example).

An alternative is to have the box directly expose an IsMine(scriptPubKey) function, but instead of directly implementing SigningProvider, have it expose a GetSigningProvider(scriptPubKey), which would (for descriptor wallets) look up which index the scriptPubKey is constructed with (using the same precomputed table as IsMine uses), and then populate a FlatSigningProvider with just the keys/scripts necessary for that index.

I think inheritance is over abused in general, so Concept ACK since we are getting rid of inheritance layers. Passing interfaces around is all you really need for 99% of use cases, otherwise you tend to have uncompositional and complect code because your design gets tangled in the inheritance hierarchy.

Concept ACK

From the Wallet-Class-Structure-Changes document:

The inheritance chain from SigningProvider should be condensed and the scopes slightly changed ... SigningProvider -> CBasicKeyStore (renamed) -> CWallet

Any thoughts on using composition of CBasicKeyStore (renamed) over inheritance by making CBasicKeyStore (renamed) a member of CWallet that is owned by CWallet?

Using composition over inheritance feels like a much better design (but it could be that I am missing key design considerations to keep the inheritance).

An alternative is to have the box directly expose an IsMine(scriptPubKey) function, but instead of directly implementing SigningProvider, have it expose a GetSigningProvider(scriptPubKey), which would (for descriptor wallets) look up which index the scriptPubKey is constructed with (using the same precomputed table as IsMine uses), and then populate a FlatSigningProvider with just the keys/scripts necessary for that index.

I've decided to go with this approach. ScriptPubKeyMan will not inherit anything.

Concept ACK. Love this design direction which makes things more modular and gets rid of inheritance hell.

I have a few questions. From the proposal:

At the end of this refactoring, the stack will be: SigningProvider -> CBasicKeyStore (renamed) -> CWallet

Also:

CWallet will need to be standalone there is no need for CWallet to be a SigningProvider

So if CWallet "is-NOT-a" SigningProvider, then why have it as part of the inheritance tree at all?

Taking it further, could we maybe get rid of this inheritance tree altogether by either (a) merging CBasicKeyStore into SigningProvider, just like what @achow101 is proposing for CKeyStore? or (b) converting it to a "has-a" relationship? So a SigningProvider could have a CBasicKeyStore to help manage its own keys. But a CBasicKeyStore is-NOT-a SigningProvider (which makes more sense to me, it's not intuitive that a KeyStore is also a SigningProvider).

(a) makes more sense if a KeyStore always has to be a part of a SigningProvider (and nowhere else). (b) makes more sense if a stand-alone KeyStore could be a useful thing, without the ability to sign.

In general, I think most of the class relationships modeled here sound like classic has-a, not is-a relationships.

Completed in #17261