Introduce interface for signing providers #12714

CKeyStore is a rich interface that provides many features, including knowledge of scripts and pubkeys for solving, private keys for signing, in addition to watch-only keys and scripts, and distinguishing lack of keys from them just being encrypted.

The signing logic in script/sign does not actually need most of these features. Here we introduce a simpler interface (SigningProvider) which only provides keys and scripts. This is actually sufficient for signing.

In addtion, we swap the dependency between keystore and script/sign (keystore now depends on script/script with CKeyStore deriving from SigningProvider, rather than CKeyStore being the interface that signing relies on).

This is a very early step towards the design in https://gist.github.com/sipa/125cfa1615946d0c3f3eec2ad7f250a2, separating the concern between deciding what outputs are ours and signing.

Makes sense. utACK b6cb981e9e62a9886891b200fcf6b144f1e2e2f5

Concept ACK

utACK b6cb981e9e62a9886891b200fcf6b144f1e2e2f5.

Strong concept ACK.

I very much appreciate this PR as it promotes the separation of concerns and the use of loosely coupled components.

Some thoughts:

• CKeyStore can become a pure interface by itself by moving its cs_KeyStore member and the AddKey implementation to CBasicKeyStore (I am happy to follow up on this if this is desired, but out of scope);
• Declare SigningProvider in its own header file;
• Rename the variable names where its type has been refactored from CKeyStore to SigningProvider to signingProvider-like equivalents.

CKeyStore can become a pure interface by itself by moving its cs_KeyStore member and the AddKey implementation to CBasicKeyStore (I am happy to follow up on this if this is desired, but out of scope);

Indeed, this is possible (and has been possible for a while). I have some bigger changes planned, though (see below).

Declare SigningProvider in its own header file;

I disagree. SigningProvider is part of the signing module. It's the interface you need to implement to use the signing logic. Making keystore a separate module was partly responsible for it gaining many of its unnecessary responsibilities.

Rename the variable names where its type has been refactored from CKeyStore to SigningProvider to signingProvider-like equivalents.

Indeed, but there are many more changes coming here, and I'd like to avoid code churn.

In a way, I think, SigningProvider is what CKeyStore should have been. At the time it was created, it was necessary to break a cyclic dependency between script and wallet, and SigningProvider would have been sufficient for that. However, instead of just being the interface to provide information to the signing logic, it became the class that was actually responsible for maintaining that information. Later is was extended with even more responsibilities (maintaining all information to determine whether an output is ours, rather than just information for signing).

Effectively keystore now manages a rather arbitrary subset of the wallet's data (keys and script and watchonly, but no key chains, pool, labels, metadata) - which isn't even enough to fully control the "which outputs are ours" concern now that HD derivation etc are implemented. I think keystore should either be fully merged back into wallet, or be extended to become the storage of all "record" related information (see the gist linked to in the PR description).

Thanks for the feedback @sipa, appreciate it!

Just to elaborate on the suggestion to decouple the pure interface SigningProvider:

As far as I can tell with b6cb981 there is a compilation dependency between implementations of the SigningProvider and the signing module, meaning that each time when the signing module needs to be compiled, implementations of the SigningProvider - which may not have been changed nor care about the recompiled signing module - will be recompiled as a result.

I took notice of the comment below:

SigningProvider is part of the signing module. It's the interface you need to implement to use the signing logic.

CKeyStore and CBasicKeyStore only seem to use the signing module by way of implementing the SigningProvider interface class. Decoupling the SigningProvider interface class doesn't conceptually change that design if you would keep the signingprovider.h and sign.h header files together, but it will break the compilation dependency between the signing module and implementations of the SigningProvider interface class.

Please note that this is just a friendly clarification. I am not pushing back on the concept (it wouldn't surprise me if I am still missing something very obvious).

@251Labs Sorry if I came across as claiming you're wrong, I'm just voicing my opinion about design. This is a useful discussion in any case.

You're certainly right that keeping SigningProvider part of the signing module means a possibly unnecessary compilation dependency of provider implementations on script/sign.h, but I don't think this is a big issue.

The point I was trying to make is that we should see SigningProvider as part of the "script signing interface", and not a separate thing. This is purely a code organization issue, not more. I believe that having keystore a separate module is partially responsible for it having become overloaded with unnecessary features.

concept ACK, will review

utACK b6cb981.

This seams an incomplete change while reading code like SigningProvider* keystore, but I guess who cares.

utACK https://github.com/bitcoin/bitcoin/pull/12714/commits/b6cb981e9e62a9886891b200fcf6b144f1e2e2f5

follow-on renamings always nice in subsequent PRs

Updated to include a few non-invasive renames.

reACK https://github.com/bitcoin/bitcoin/pull/12714/commits/d40f06a3da5e6b1fd065885b08513263fa930cb8

(m_)provider renaming

utACK d40f06a3da5e6b1fd065885b08513263fa930cb8