Refactor CWallet's inheritance chain #16227

achow101 commented at 10:46 PM on June 17, 2019: member

This PR compresses the CWallet chain of inheritance from 5 classes to 3 classes. CBasicKeyStore is renamed to FillableSigningProvider and some parts of it (the watchonly parts) are moved into CWallet. CKeyStore and CCrypoKeyStore are completely removed. CKeyStore's Have* functions are moved into SigningProvider and the Add* moved into FillableSigningProvider, thus allowing it to go away entirely. CCryptoKeyStore's functionality is moved into CWallet. The new inheritance chain is:

SigningProvider -> FillableSigningProvider -> CWallet

SigningProvider now is the class the provides keys and scripts and indicates whether keys and scripts are present. FillableSigningProvider allows keys and scripts to be added to the signing provider via Add* functions. CWallet handles all of the watchonly stuff (AddWatchOnly, HaveWatchOnly, RemoveWatchOnly which were previously in CKeyStore) and key encryption (previously in CCryptoKeyStore).

Implements the 2nd prerequisite from the wallet restructure.

fanquake added the label Wallet on Jun 17, 2019

fanquake added the label Refactoring on Jun 17, 2019

fanquake requested review from meshcollider on Jun 18, 2019

DrahtBot commented at 4:20 AM on June 18, 2019: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16341 (WIP: Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes) by achow101)
#16273 (refactor: Remove unused includes by practicalswift)
#15845 (wallet: Fast rescan with BIP157 block filters by MarcoFalke)
#14144 (Refactoring: Clarify code using encrypted_batch in CWallet by domob1812)

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.

DrahtBot added the label Needs rebase on Jun 18, 2019

jnewbery added this to the "PRs" column in a project

achow101 commented at 10:09 PM on June 19, 2019: member

Rebased

achow101 force-pushed on Jun 19, 2019

DrahtBot removed the label Needs rebase on Jun 19, 2019

DrahtBot added the label Needs rebase on Jun 21, 2019

achow101 force-pushed on Jun 21, 2019

achow101 commented at 5:55 PM on June 21, 2019: member

Rebased

DrahtBot removed the label Needs rebase on Jun 21, 2019

in src/wallet/wallet.cpp:3270 in c2975f193b outdated

3266 | @@ -3220,7 +3267,7 @@ DBErrors CWallet::LoadWallet(bool& fFirstRunRet)
3267 |      }
3268 |  
3269 |      {
3270 | -        LOCK(cs_KeyStore);
3271 | +        LOCK(cs_wallet);

MarcoFalke commented at 6:56 PM on June 21, 2019:

wallet/wallet.cpp:3270:9: error: acquiring mutex 'cs_wallet' that is already held [-Werror,-Wthread-safety-analysis]

And

./wallet/wallet.h:1009:10: warning: 'AddKeyPubKey' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
    bool AddKeyPubKey(const CKey& key, const CPubKey &pubkey) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);

achow101 commented at 7:55 PM on June 21, 2019:

Fixed

achow101 force-pushed on Jun 21, 2019

in src/wallet/crypter.h:14 in 30c0be7550 outdated

  10 |  #include <support/allocators/secure.h>
  11 | +#include <script/signingprovider.h>
  12 |  
  13 |  #include <atomic>
  14 |  
  15 | +#include <boost/signals2/signal.hpp>

MarcoFalke commented at 2:15 PM on June 24, 2019:

This should probably move to wallet.h as well, since NotifyStatusChanged moved there.

achow101 commented at 2:52 PM on June 24, 2019:

Fixed

MarcoFalke commented at 2:18 PM on June 24, 2019: member

Looks like this doesn't compile with ./configure CC=clang CXX=clang++

achow101 force-pushed on Jun 24, 2019

achow101 commented at 2:53 PM on June 24, 2019: member

There were some lock order issues (I guess bad copy paste?) that I fixed.

Looks like this doesn't compile with ./configure CC=clang CXX=clang++

Seems to be working for me now although I didn't actually do anything to try to fix it. Does it work for you now?

MarcoFalke commented at 3:45 PM on June 24, 2019: member

You can see the failure here: https://travis-ci.org/bitcoin/bitcoin/jobs/549788027#L3052 (in the build step)

wallet/wallet.cpp:4653:12: warning: reading variable 'vMasterKey' requires holding mutex 'cs_wallet' [-Wthread-safety-analysis]
    return vMasterKey.empty();
           ^
wallet/wallet.cpp:4663:9: warning: reading variable 'vMasterKey' requires holding mutex 'cs_wallet' [-Wthread-safety-analysis]
        vMasterKey.clear();
        ^
wallet/wallet.cpp:4735:9: warning: writing variable 'vMasterKey' requires holding mutex 'cs_wallet' exclusively [-Wthread-safety-analysis]
        vMasterKey = vMasterKeyIn;
        ^
wallet/wallet.cpp:4788:40: warning: reading variable 'mapCryptedKeys' requires holding mutex 'cs_KeyStore' [-Wthread-safety-analysis]
    CryptedKeyMap::const_iterator mi = mapCryptedKeys.find(address);
                                       ^
wallet/wallet.cpp:4789:15: warning: reading variable 'mapCryptedKeys' requires holding mutex 'cs_KeyStore' [-Wthread-safety-analysis]
    if (mi != mapCryptedKeys.end())
              ^
5 warnings generated.

achow101 force-pushed on Jun 24, 2019

achow101 commented at 6:04 PM on June 24, 2019: member

Fixed those warnings.

in src/keystore.cpp:203 in b8f6f3f25f outdated

 195 | @@ -196,10 +196,3 @@ CKeyID GetKeyForDestination(const CBasicKeyStore& store, const CTxDestination& d
 196 |      }
 197 |      return CKeyID();
 198 |  }
 199 | -
 200 | -bool HaveKey(const CBasicKeyStore& store, const CKey& key)
 201 | -{
 202 | -    CKey key2;
 203 | -    key2.Set(key.begin(), key.end(), !key.IsCompressed());

ryanofsky commented at 8:36 PM on June 24, 2019:

In commit "Remove HaveKey static function from keystore" (b8f6f3f25f9fd45d8659bfa15800c09bda951872)

Would be good to mention why removing this doesnt' change behavior in the commit description (I'd assume because hd keys are always compressed).

achow101 commented at 10:09 PM on June 24, 2019:

Actually, looking at it now, think this does actually change behavior. I think I will move this function into rpcwallet.cpp where it is used.

Sjors commented at 1:39 PM on July 9, 2019:

nit: don't forget to rename the commit

in src/script/signingprovider.h:69 in 8ce8d6a320 outdated

  65 | @@ -66,14 +66,10 @@ class FillableSigningProvider : public SigningProvider
  66 |      mutable CCriticalSection cs_KeyStore;
  67 |  
  68 |      using KeyMap = std::map<CKeyID, CKey>;
  69 | -    using WatchKeyMap = std::map<CKeyID, CPubKey>;

ryanofsky commented at 9:13 PM on June 24, 2019:

In commit "Move WatchOnly stuff from SigningProvider to CWallet" (8ce8d6a3200921c960d41bca0ba5bf0ab176c5ee)

The changes in this commit seem undesirable and I don't understand the motivation for them. Why move watch only storage code away from script and key storage code that it's actually similar to, into the cwallet class which less related and more complicated? I could understand if this was actually simplifying something, for example by removing ability to store watch only keys alongside signing keys. But this doesn't seem to be simplifying anything.

I think i'd advise dropping this commit from the PR. Even if it is doing something useful (which it might be), it seems like it's going in the opposite direction as the other changes by splitting things up instead of consolidating.

achow101 commented at 10:08 PM on June 24, 2019:

The point of this was that SigningProviders provide things for signing which watchonly things are not. Those are just scripts and keys that CWallet needs to know and are not related to the things that a SigningProvider needs to provide. So that's why I moved them into CWallet. Later these will get moved in the the LegacyScryptPubKeyManager.

Sjors commented at 2:37 PM on July 9, 2019:

How does solvability play into this?

ryanofsky approved

ryanofsky commented at 9:25 PM on June 24, 2019: member

utACK first seven commits (last commit seems harmless but not desirable). I left some comments, but feel free to ignore them. This PR is easy to understand and very nicely broken up. I confirmed the relevant parts are move only and that this doesn't change behavior.

faa513ea3dc376a69d382b943e3b1358d8baed6d Add HaveKey and HaveCScript to SigningProvider (1/8)
4f1b9d808efab7562099a2d07c406aeb29e8516e Remove CKeyStore and squash into CBasicKeyStore (2/8)
b8f6f3f25f9fd45d8659bfa15800c09bda951872 Remove HaveKey static function from keystore (3/8)
ec83adc1aed0503c62d9a3cb397508097b2b95a0 scripted-diff: rename CBasicKeyStore to FillableSigningProvider (4/8)
39659203e9104fbce270d7dd62790e954d55f55d Move KeyOriginInfo to its own header file (5/8)
a96967901b2b8b4a36a06b63581f28ae0ea3f145 Move various SigningProviders to signingprovider.{cpp,h} (6/8)
9a65a1e55ec5ac141c3b5a7e4d167f48712dc3cf Remove CCryptoKeyStore and move all of it's functionality into CWallet (7/8)
8ce8d6a3200921c960d41bca0ba5bf0ab176c5ee Move WatchOnly stuff from SigningProvider to CWallet (8/8)

achow101 force-pushed on Jun 24, 2019

achow101 force-pushed on Jun 27, 2019

laanwj added this to the "Blockers" column in a project

l2a5b1 commented at 3:33 PM on June 28, 2019: contributor

@achow101 per @MarcoFalke's request please see #16303 (comment)

achow101 force-pushed on Jun 28, 2019

achow101 commented at 3:58 PM on June 28, 2019: member

Fixed the include mentioned in #16303

in src/script/signingprovider.h:18 in f67758de35 outdated

  13 | +#include <sync.h>
  14 | +
  15 | +struct KeyOriginInfo;
  16 | +
  17 | +/** An interface to be implemented by keystores that support signing. */
  18 | +class SigningProvider

l2a5b1 commented at 11:14 AM on June 30, 2019:

Any thoughts on:

(1) turning SigningProvider into a "pure interface" that only declares pure virtual functions? (2) isolating SigningProvider and define it in its own header file?

These changes could make sense because we are using SigningProvider as an interface.

achow101 commented at 2:04 PM on June 30, 2019:

I think it is fine to leave it here. Making it a pure interface does make sense, but I don't think I will do that here.

in src/wallet/wallet.h:714 in f67758de35 outdated

 710 | @@ -709,9 +711,35 @@ class WalletRescanReserver; //forward declarations for ScanForWalletTransactions
 711 |   * A CWallet is an extension of a keystore, which also maintains a set of transactions and balances,
 712 |   * and provides the ability to create new transactions.
 713 |   */
 714 | -class CWallet final : public CCryptoKeyStore, private interfaces::Chain::Notifications
 715 | +class CWallet final : public FillableSigningProvider, private interfaces::Chain::Notifications

l2a5b1 commented at 11:15 AM on June 30, 2019:

In the spirit of this pull request it could make sense to choose composition of FillableSigningProvider over inheritance.

achow101 commented at 2:15 PM on June 30, 2019:

We do that later with the rest of the restructure with the introduction of the ScriptPubKeyMan. I'm not sure that it makes sense to make it a member now rather than later where a bunch of things will also change.

l2a5b1 commented at 11:55 AM on June 30, 2019: contributor

On a concept level I was wondering if it makes sense to:

make SigningProvider a "pure interface" (preferably defined in its own header file).
choose composition of FillableSigningProvider over inheritance in CWallet.

The names of the SigningProvider subtypes didn't really resonate with me. Perhaps there are better alternatives. I am obviously nitpicking here.

DrahtBot added the label Needs rebase on Jul 3, 2019

Add HaveKey and HaveCScript to SigningProvider 1b699a5083

achow101 force-pushed on Jul 3, 2019

DrahtBot removed the label Needs rebase on Jul 4, 2019

in src/keystore.h:51 in 3408fd1426 outdated

  55 | -    bool HaveWatchOnly(const CScript &dest) const override;
  56 | -    bool HaveWatchOnly() const override;
  57 | +    virtual bool AddWatchOnly(const CScript &dest);
  58 | +    virtual bool RemoveWatchOnly(const CScript &dest);
  59 | +    virtual bool HaveWatchOnly(const CScript &dest) const;
  60 | +    virtual bool HaveWatchOnly() const;

kallewoof commented at 5:55 AM on July 9, 2019:

In 3408fd14267cd0d3f57af73f6ba870edd805ecb0

Is there a reason why some of the methods here are non-virtual? Writing a subclass of this one could easily assume that an override works where it sometimes won't. Recommend adding virtual to all of them for simplicity.

achow101 commented at 8:10 PM on July 9, 2019:

Made them virtual.

in src/bench/ccoins_caching.cpp:7 in df22f16a14 outdated

   3 | @@ -4,8 +4,8 @@
   4 |  
   5 |  #include <bench/bench.h>
   6 |  #include <coins.h>
   7 | +#include <script/signingprovider.h>

kallewoof commented at 6:03 AM on July 9, 2019:

Nit: p comes before s (move this one line down).

achow101 commented at 8:10 PM on July 9, 2019:

Fixed the order here and everywhere.

in src/bitcoin-tx.cpp:14 in df22f16a14 outdated

  10 | @@ -11,7 +11,7 @@
  11 |  #include <consensus/consensus.h>
  12 |  #include <core_io.h>
  13 |  #include <key_io.h>
  14 | -#include <keystore.h>
  15 | +#include <script/signingprovider.h>

kallewoof commented at 6:03 AM on July 9, 2019:

Nit: p comes before s (move this 4 lines down).

in src/rpc/rawtransaction_util.cpp:15 in df22f16a14 outdated

  12 |  #include <policy/policy.h>
  13 |  #include <primitives/transaction.h>
  14 |  #include <rpc/protocol.h>
  15 |  #include <rpc/util.h>
  16 | +#include <script/signingprovider.h>
  17 | +#include <script/sign.h>

kallewoof commented at 6:05 AM on July 9, 2019:

Nit: swap order for alphabeticality (as in other places).

in src/rpc/rawtransaction.cpp:13 in df22f16a14 outdated

   9 | @@ -10,7 +10,7 @@
  10 |  #include <core_io.h>
  11 |  #include <index/txindex.h>
  12 |  #include <key_io.h>
  13 | -#include <keystore.h>
  14 | +#include <script/signingprovider.h>

kallewoof commented at 6:05 AM on July 9, 2019:

Nit: move to L27 for alphabeticality.

in src/rpc/util.cpp:7 in df22f16a14 outdated

   2 | @@ -3,8 +3,8 @@
   3 |  // file COPYING or http://www.opensource.org/licenses/mit-license.php.
   4 |  
   5 |  #include <key_io.h>
   6 | -#include <keystore.h>
   7 |  #include <outputtype.h>
   8 | +#include <script/signingprovider.h>

kallewoof commented at 6:06 AM on July 9, 2019:

Nit: 2 lines down.

in src/script/sign.cpp:11 in df22f16a14 outdated

   8 | @@ -9,6 +9,7 @@
   9 |  #include <policy/policy.h>
  10 |  #include <primitives/transaction.h>
  11 |  #include <script/standard.h>
  12 | +#include <script/signingprovider.h>

kallewoof commented at 6:06 AM on July 9, 2019:

Nit: i comes before t

(I am leaving the ones so far as is, as you seem to have paid some attention to ordering of includes, but I am not bringing this up in future occurrences during this review.)

in src/script/signingprovider.cpp:23 in df22f16a14 outdated

  20 | +    if (it != map.end()) {
  21 | +        value = it->second;
  22 | +        return true;
  23 | +    }
  24 | +    return false;
  25 | +}

kallewoof commented at 6:07 AM on July 9, 2019:

Alternative, feel free to ignore:

if (it == map.end()) return false;
value = it->second;
return true;

achow101 commented at 7:53 PM on July 9, 2019:

It's moved, so I'll just leave it as is.

kallewoof approved

kallewoof commented at 6:14 AM on July 9, 2019: member

utACK f76c4819f6f37711f2b35562131ede4fe0438671

40da275a86d81a41b4a331efce33cd3e2074e592: Typo in commit message (it's should be its). 48f95eed20dae512a1ad62070d9d67771464ec74: Verified move-only.

I point out a number of include orderings; there are more; I left them there because it looked like attention was paid to this in some places.

in src/keystore.h:57 in 76032ecfba outdated

  53 | @@ -54,7 +54,4 @@ class CBasicKeyStore : public SigningProvider
  54 |  /** Return the CKeyID of the key involved in a script (if there is a unique one). */
  55 |  CKeyID GetKeyForDestination(const CBasicKeyStore& store, const CTxDestination& dest);
  56 |  
  57 | -/** Checks if a CKey is in the given CBasicKeyStore compressed or otherwise*/

Sjors commented at 1:41 PM on July 9, 2019:

nit: don't lose this comment

achow101 commented at 8:10 PM on July 9, 2019:

Added it back

in src/script/signingprovider.cpp:15 in f76c4819f6 outdated

  72 | @@ -73,8 +73,6 @@ void FillableSigningProvider::ImplicitlyLearnRelatedKeyScripts(const CPubKey& pu
  73 |  {
  74 |      AssertLockHeld(cs_KeyStore);
  75 |      CKeyID key_id = pubkey.GetID();
  76 | -    // We must actually know about this key already.
  77 | -    assert(HaveKey(key_id) || mapWatchKeys.count(key_id));

Sjors commented at 2:24 PM on July 9, 2019:

Why is this safe to remove?

instagibbs commented at 8:24 PM on July 11, 2019:

This is only called directly after adding the privkey or watchonly key:

CCryptoKeyStore::AddCryptedKey
CBasicKeyStore::AddKeyPubKey
CBasicKeyStore::AddWatchOnly

perhaps leave a comment at the top of this function to the effect.

Sjors approved

Sjors commented at 2:36 PM on July 9, 2019: member

ACK f76c481

Nit: you can remove "Built on" from the description.

Maybe keep EncryptSecret, DecryptSecret and DecryptKey in wallet/crypter.cpp? It makes me slightly sad to see wallet/wallet.cpp grow.

I checked the scripted diff (to my surprise, it works on macOS).

git show [hash] --color-moved=dimmed-zebra is quite handy for some of these commits.

It could make sense to turn the last commit, which moves watch-only stuff to CWallet, into a separate PR. See this discussion.

Remove CKeyStore and squash into CBasicKeyStore c7797ec655

achow101 force-pushed on Jul 9, 2019

Move HaveKey static function from keystore to rpcwallet where it is used a913e3f2fb

scripted-diff: rename CBasicKeyStore to FillableSigningProvider

-BEGIN VERIFY SCRIPT-
git grep -l "CBasicKeyStore" | xargs sed -i -e 's/CBasicKeyStore/FillableSigningProvider/g'
-END VERIFY SCRIPT-

d9becff4e1

Move KeyOriginInfo to its own header file 16f8096e91

Move various SigningProviders to signingprovider.{cpp,h}

Moves all of the various SigningProviders out of sign.{cpp,h} and
keystore.{cpp,h}. As such, keystore.{cpp,h} is also removed.

Includes and the Makefile are updated to reflect this. Includes were largely
changed using:
git grep -l "keystore.h" | xargs sed -i -e 's;keystore.h;script/signingprovider.h;g'

37a79a4fcc

Remove CCryptoKeyStore and move all of it's functionality into CWallet

Instead of having a separate CCryptoKeyStore that handles the encryption
stuff, just roll it all into CWallet.

8f5b81e6ed

Move WatchOnly stuff from SigningProvider to CWallet 93ce4a0b6f

achow101 force-pushed on Jul 9, 2019

Sjors commented at 2:30 PM on July 10, 2019: member

re-ACK 93ce4a0; it keeps EncryptSecret, DecryptSecret and DecryptKey in wallet/crypter.cpp, but makes them not static. It improves alphabetical includes, reorders some function definitions, fixes commit message, brings back lost code comment.

in src/keystore.h:26 in 1b699a5083 outdated

  22 | @@ -23,12 +23,10 @@ class CKeyStore : public SigningProvider
  23 |      virtual bool AddKeyPubKey(const CKey &key, const CPubKey &pubkey) =0;
  24 |  
  25 |      //! Check whether a key corresponding to a given address is present in the store.
  26 | -    virtual bool HaveKey(const CKeyID &address) const =0;

instagibbs commented at 8:06 PM on July 11, 2019:

mu-nit: code commend above should be removed as well

instagibbs commented at 8:37 PM on July 11, 2019: member

utACK https://github.com/bitcoin/bitcoin/pull/16227/commits/93ce4a0b6fb54efb1f424a71dfc09cc33307e5b9

easy to follow

laanwj commented at 8:42 PM on July 11, 2019: member

code review ACK 93ce4a0b6fb54efb1f424a71dfc09cc33307e5b9

laanwj removed this from the "Blockers" column in a project

laanwj merged this on Jul 11, 2019

laanwj closed this on Jul 11, 2019

laanwj referenced this in commit 735d6b57e7 on Jul 11, 2019

sidhujag referenced this in commit b72e30bedf on Jul 11, 2019

jnewbery moved this from the "PRs" to the "Done" column in a project

deadalnix referenced this in commit ce0e8e7332 on Jun 13, 2020

deadalnix referenced this in commit b2849c3abc on Jun 13, 2020

deadalnix referenced this in commit f8cec7b82a on Jun 13, 2020

deadalnix referenced this in commit 19779479d7 on Jun 13, 2020

deadalnix referenced this in commit 8a6ea7a8fe on Jun 13, 2020

deadalnix referenced this in commit 33483a05b6 on Jun 13, 2020

deadalnix referenced this in commit cbe04c002b on Jun 13, 2020

deadalnix referenced this in commit 4a64e22d3a on Jun 15, 2020

ftrader referenced this in commit 078f55b744 on Aug 17, 2020

MarcoFalke locked this on Dec 16, 2021