wallet, spkm: Move key management from DescriptorScriptPubKeyMan to wallet level KeyManager

[achow101]

This PR changes DescriptorScriptPubKeyMan to no longer handle relevant keys directly. Instead all keys for all DescriptorSPKMs will be handled by a new KeyManager class which exists within CWallet (a reference to it is passed to each DescriptorSPKM). This allows us to have a concept of a wallet HD key for descriptor wallets. This makes it easier to add new single key descriptors that use the same HD master key as the rest of the autogenerated descriptors (e.g. for taproot). Multisigs will also be easier as an xpub belonging to the wallet can be exported without needing to do weird things like descriptor introspection and guessing about which descriptor's key to use.

KeyManager is a class which handles all of the keys for DescriptorSPKMs. It contains the maps that hold the keys, deals with writing those keys to disk, and handles their encryption. Encryption keys are still managed by CWallet but provided to KeyManager through the WalletStorage interface. Signing is still done through DescriptorScriptPubKeyMan::SignTransaction however this will fetch the keys from KeyManager rather than storing keys in the DescriptorSPKM to be used.

This change is backwards compatible. Although KeyManager writes and uses keys in new keyman_key and keyman_ckey records, it will still write keys for each descriptor in walletdescriptorkey and walletdescriptorckey records. This allows a descriptor wallet created using this change to be opened by 22.0 and 0.21. Additionally, wallets created with older software will automatically be upgraded to using the KeyManager at first loading. This is done in the background and does not require any user interaction (i.e. no passphrase required).

[Sjors]

Concept ACK. This will make #22341 a lot easier.

[DrahtBot]

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #26627 (wallet: Migrate non-HD keys with single combo containing a list of keys by achow101)
• #26596 (wallet: Migrate legacy wallets to descriptor wallets without requiring BDB by achow101)
• #26008 (wallet: cache IsMine scriptPubKeys to improve performance of wallets with a lot of non-ranged descriptors by achow101)
• #25991 (Wallet: Add foreign_outputs metadata to support CoinJoin transactions by luke-jr)
• #25907 (wallet: rpc to add automatically generated descriptors by achow101)
• #25766 (wallet: Include a signature with encrypted keys to mitigate a wallet scam by achow101)
• #24914 (wallet: Load database records in a particular order by achow101)
• #22838 (descriptors: Be able to specify change and receiving in a single descriptor string 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.

[Sjors]

I rebased #22341 on top of this.

Some initial review remarks:

wallet_transactiontime_rescan.py fails consistently for me

2021-11-05T14:52:37.129000Z TestFramework (INFO): Restore user wallet on another node without rescan
2021-11-05T14:52:37.154000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/Users/sjors/dev/bitcoin/test/functional/test_framework/test_framework.py", line 132, in main
    self.run_test()
  File "test/functional/wallet_transactiontime_rescan.py", line 134, in run_test
    assert_equal(restorewo_wallet.getbalance(), 0)
  File "/Users/sjors/dev/bitcoin/test/functional/test_framework/util.py", line 50, in assert_equal
    raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
AssertionError: not(1.00000000 == 0)

Some thread safety warnings.

wallet/keyman.cpp:241:5: warning: calling function 'AssertLockHeldInternal<AnnotatedMixin<std::recursive_mutex>>' requires holding mutex 'cs_keyman' exclusively [-Wthread-safety-analysis]
    AssertLockHeld(cs_keyman);
    ^
./sync.h:83:28: note: expanded from macro 'AssertLockHeld'
#define AssertLockHeld(cs) AssertLockHeldInternal(#cs, __FILE__, __LINE__, &cs)
                           ^
wallet/keyman.cpp:244:41: warning: reading variable 'm_map_crypted_keys' requires holding mutex 'cs_keyman' [-Wthread-safety-analysis]
        for (const auto& [id, key_pair] : m_map_crypted_keys) {
                                        ^
wallet/keyman.cpp:244:41: warning: reading variable 'm_map_crypted_keys' requires holding mutex 'cs_keyman' [-Wthread-safety-analysis]
wallet/keyman.cpp:253:12: warning: reading variable 'm_map_keys' requires holding mutex 'cs_keyman' [-Wthread-safety-analysis]
    return m_map_keys;
           ^
wallet/keyman.cpp:264:9: warning: reading variable 'm_map_crypted_keys' requires holding mutex 'cs_keyman' [-Wthread-safety-analysis]
    if (m_map_crypted_keys.count(id) == 0) {
        ^
wallet/keyman.cpp:267:12: warning: reading variable 'm_map_crypted_keys' requires holding mutex 'cs_keyman' [-Wthread-safety-analysis]
    return m_map_crypted_keys.at(id);
           ^
6 warnings generated.

...

wallet/walletdb.cpp:813:38: warning: calling function 'LoadActiveHDKey' requires holding mutex 'pwallet->GetKeyManager().cs_keyman' exclusively [-Wthread-safety-analysis]
            pwallet->GetKeyManager().LoadActiveHDKey(extpub);
                                     ^
wallet/walletdb.cpp:1150:38: warning: calling function 'SetActiveHDKey' requires holding mutex 'pwallet->GetKeyManager().cs_keyman' exclusively [-Wthread-safety-analysis]
            pwallet->GetKeyManager().SetActiveHDKey(best_xpub);

Shameless plug: it would be useful is #19013 was merged first, so this PR can include more wallet backward and forward compatibility tests.

[achow101]

Thread safety warnings are fixed, not seeing the wallet_transactiontime_rescan.py failure.

[Sjors]

Some more comments after light-weight code review...

[Sjors]

Let's clarify here, or at the documentation for GetActiveHDKey, that the corresponding extended private key is reconstructed using this extended public key, which includes the chain code, and the right KEYMAN_KEY or KEYMAN_CKEY private key.

Still this process seems rather complicated, why not just store the (encrypted) extended private key?

[achow101]

A lot of places in the codebase do not expect CExtKeys when fetching private keys, even in places where BIP 32 derivation ends up being done. They instead take CKeys and combine them with CExtPubKeys to get the necessary CExtKeys. We maintain this same paradigm for ease of implementation.

Additionally, having all private keys be universally CKeys makes it easier to support non-ranged descriptors. Instead of having to store and fetch different data types depending on whether the descriptor is ranged, and then converting them into the same data type for expansion and signing, we can use the same datatype throughout. When we do need the extended key, it can be reconstructed, but that (currently) happens rarely.

[Sjors]

Storing both CExtKey and Ckey (for non-ranged descriptors) could be an approach, but I'm also not sure if that makes the implementation any easier to understand. This is probably the only opportunity to break with the past convention, if we want to.

[Sjors]

Not seeing any thread warning anymore, so that's good.

wallet_transactiontime_rescan.py only fails for me with configuring --without-bdb.

[aureleoules]

reACK c9af030 - minor rebase, added KeyManager m_keyman to CWallet

[Sjors]

re-utACK c9af030

[S3RK]

Concept ACK. Started code review

[achow101]

I had a discussion with @S3RK about this PR last week, and we concluded that this might not be the best approach to tackle the issue that it targets. While it is a complete solution that would probably work well if descriptor wallets had been implemented this way in the first place, it seems like it doesn't work well with having to deal with backwards compatibility and various combinations of upgrading and downgrading that may occur.

Since the goal of this PR is to enable key rotation (via re-enabling sethdseed) and the addition of new automatically generated descriptors, this implementation is probably overkill in addition to the backwards compatibility issues that it introduces.

I've opened #26728 which implements a much simpler solution of just having CWallet store the master key and any ones that get rotated out. It still enables the things that we want.