Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes)

achow101 commented at 3:01 am on July 5, 2019: member

Introducing the ScriptPubKeyMan (short for ScriptPubKeyManager) for managing scriptPubKeys and their associated scripts and keys. This functionality is moved over from CWallet. Instead, CWallet will have a pointer to a ScriptPubKeyMan for every possible address type, internal and external. It will fetch the correct ScriptPubKeyMan as necessary. When fetching new addresses, it chooses the ScriptPubKeyMan based on address type and whether it is change. For signing, it takes the script and asks each ScriptPubKeyMan for whether that ScriptPubKeyMan considers that script IsMine, whether it has that script, or whether it is able to produce a signature for it. If so, the ScriptPubKeyMan will provide a SigningProvider to the caller which will use that in order to sign.

There is currently one ScriptPubKeyMan - the LegacyScriptPubKeyMan. Each CWallet will have only one LegacyScriptPubKeyMan with the pointers for all of the address types and change pointing to this LegacyScriptPubKeyMan. It is created when the wallet is loaded and all keys and metadata are loaded into it instead of CWallet. The LegacyScriptPubKeyMan is primarily made up of all of the key and script management that used to be in CWallet. For convenience, CWallet has a GetLegacyScriptPubKeyMan which will return the LegacyScriptPubKeyMan or a nullptr if it does not have one (not yet implemented, but callers will check for the nullptr). For purposes of signing, LegacyScriptPubKeyMan’s GetSigningProvider will return itself rather than a separate SigningProvider. This will be different for future ScriptPubKeyMans.

The LegacyScriptPubKeyMan will also handle the importing and exporting of keys and scripts instead of CWallet. As such, a number of RPCs have been limited to work only if a LegacyScriptPubKeyMan can be retrieved from the wallet. These RPCs are sethdseed, addmultisigaddress, importaddress, importprivkey, importpubkey, importmulti, dumpprivkey, and dumpwallet. Other RPCs which relied on the wallet for scripts and keys have been modified in order to take the SigningProvider retrieved from the ScriptPubKeyMan for a given script.

Overall, these changes should not effect how everything actually works and the user should experience no difference between having this change and not having it. As such, no functional tests were changed, and the only unit tests changed were those that were directly accessing CWallet functions that have been removed.

This PR is the last step in the Wallet Structure Changes.

The commits are organized as follows:

Miscellaneous changes that don’t necessarily make sense outside of this PR
- Move wallet enums to walletutil.h
- List output types in an array in order to be iterated over
- Always try to sign for all pubkeys in multisig
Interface definitions and miscellaneous changes in preparation for ScriptPubKeyMan integration
- Introduce both ScriptPubKeyMan as an interface and LegacyScriptPubKeyMan as a dummy class
- Add LegacyScriptPubKeyMan to CWallet
- Add function callbacks for wallet flags and versions and wallet database
- Fetch the SigningProvider for a script from the wallet
- Fetch the ScriptPubKeyMan for given output type and internal-ness, or a given script, or ScriptPubKeyMan id
Implementation of LegacyScriptPubKeyMan by copying existing code from CWallet. These will pass all tests and do not affect CWallet
- Implement GetSigningProvider in LegacyScriptPubKeyMan
- Implement function to connect ScriptPubKeyMan’s NotifyCanGetAddessesChanged and NotifyWatchOnlyChanged to CWallet’s
- Implement IsLocked and IsCrypted in LegacyScriptPubKeyMan
- Implement LoadCryptedKey and AddCryptedKey in LegacyScriptPubKeyMan
- Implement UpdateTimeFirstKey, and GetTimeFirstKey in LegacyScriptPubKeyMan
- Implement AddWatchOnly, RemoveWatchOnly, HaveWatchOnly, and LoadWatchOnly in LegacyScriptPubKeyMan
- Implement AddKeyPubKey and LoadKey in LegacyScriptPubKeyMan
- Implement WalletLogPrintf in LegacyScriptPubKeyMan
- Implement SetHDCHain, and IsHDEnabled in LegacyScriptPubKeyMan
- Implement LoadCScript in LegacyScriptPubKeyMan
- Implement LoadKeyMetadata and LoadScriptMetadata in LegacyScriptPubKeyMan
- Implement GetKey, HaveKey, and GetPubKey in LegacyScriptPubKeyMan
- Implement GenerateNewKey in LegacyScriptPubKeyMan
- Implement LoadKeyPool in LegacyScriptPubKeyMan
- Implement GetOldestKeyPoolTime, KeypoolCountExternalKeys, and GetKeypoolSize in LegacyScriptPubKeyMan
- Implement CanGetAddresses, CanGenerateKeys, and HavePrivateKeys in LegacyScriptPubKeyMan
- Implement GenerateNewSeed, DeriveNewSeed, and SetHDSeed for LegacyScriptPubKeyMan
- Implement TopUpKeypool, TopUp, and NewKeyPool in LegacyScriptPubKeyMan
- Implement ReturnAddress, and KeepKey in LegacyScriptPubKeyMan
- Implement GetNewAddress, and GetReservedAddress in LegacyScriptPubKeyMan
- Implement MarkUnusedAddresses in LegacyScriptPubKeyMan
- Implement IsMine in LegacyScriptPubKeyMan
- Implement UpgradeKeyMetaData, SetupGeneration, IsFirstRun, Upgrade, RewriteDB in LegacyScriptPubKeyMan
- Implement Unlock, Lock, and Encrypt and LegacyScriptPubKeyMan
- Implement ImporScripts, ImportPrivKeys, ImportPubKeys, and ImportScriptPubKeys in LegacyScriptPubKeyMan
- Implement GetMetadata in LegacyScriptPubKeyMan
- Implement GetKeyOrigin in LegacyScriptPubKeyMan
- Implement actually loading everything into LegacyScriptPubKeyMan
- Implement CanProvide in LegacyScriptPubKeyMan
Replacing CWallet functions and RPC things with calls to ScriptPubKeyMan or LegacyScriptPubKeyMan. These will compile but are not expected to pass tests hence the [ci skip].
- [ci skip] Remove CWallet from IsMine and have CWallet always use ScriptPubKeyMan’s IsMine
- [ci skip] moveonly: move ismine stuff to be a module of LegacyScriptPubKeyMan
- [ci skip] Have GetNewAddress, GetNewChangeAddress, and ReserveAddress use ScriptPubKeyMan
- [ci skip] Mark used addresses in ScriptPubKeyMan
- [ci skip] Call UpgradeKeyMetaData for each ScriptPubKeyMan
- [ci skip] Sign using SigningProvider from ScriptPubKeyMan when signing within CWallet
- [ci skip] Do not allow import*, dump*, and addmultisigaddress RPCs when wallet is not backed by LegacyScriptPubKeyMan
- [ci skip] Change Imports to use LegacyScriptPubKeyMan Imports
- [ci skip] Use SigningProviders and ScriptPubKeyMans in listunspent, signmessage, signrawtransactionwithwallet, and getaddressinfo
- [ci skip] Use LegacyScriptPubKeyMan in addmultisigaddress and sethdseed
- [ci skip] Use LegacyScriptPubKeyMan for hdseedid in getwalletinfo
- [ci skip] Change KeypoolCountExternal and GetKeypoolSize to get aggregate sizes from ScriptPubKeyMans
- [ci skip] Have IsHDEnabled fetch from ScriptPubKeyMans
- [ci skip] Fetch oldest keypool time from ScriptPubKeyMans
- [ci skip] have TopUpKeyPool call TopUp in each ScriptPubKeyMan
- [ci skip] Have EncryptWallet, Lock, and Unlock call their respective functions in ScriptPubKeyMans
- [ci skip] Use LegacyScriptPubKeyMan throughout psbt_wallet_tests
- [ci skip] Use LegacyScriptPubKeyMan throughout wallettool
- [ci skip] Use ScriptPubKeyMans’ Setup and Upgrade functions when loading or creating a wallet
- [ci skip] Define first run as having no ScriptPubKeyMans
- [ci skip] Use RewriteDB action when DB needs rewrite
- [ci skip] Use GetTimeFirstKey instead of nTimeFirstKey
- [ci skip] Use LegacyScriptPubKeyMan for in wallet_tests
- [ci skip] Use LegacyScriptPubKeyMan in dumpprivkey and dumpwallet
- [ci skip] Change CanGetAddresses to fetch from ScriptPubKeyMan
- [ci skip] Fetch the correct SigningProvider for signing PSBTs
- [ci skip] Use LegacyScriptPubKeyMan in test util
- [ci skip] Use LegacyScriptPubKeyMan in some parts of getbalances and createwallet
- [ci skip] Have getPubKey and getPrivKey use SigningProvider
- [ci skip] Use LegacyScriptPubKeyMan in benchmarks involving the wallet
- [ci skip] Store p2sh scripts in AddAndGetDestinationForScript
Tying everything together and removing the CWallet functions.
- Remove unused functions and switch CWallet to use ScriptPubKeyMan

achow101 commented at 3:01 am on July 5, 2019: member

In the coming days I will try to squash together some of these commits so there aren’t so many of them. I just wanted to get this open now for people to begin reviewing the changes.

fanquake added the label Wallet on Jul 5, 2019

fanquake added the label Refactoring on Jul 5, 2019

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

DrahtBot commented at 4:41 am on July 5, 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:

#17246 (wallet: avoid knapsack when there’s no change by Sjors)
#17237 (wallet: LearnRelatedScripts only if KeepDestination by promag)
#17219 (wallet: allow transaction without change if keypool is empty by Sjors)
#17211 (Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs by achow101)
#16946 (wallet: include a checksum of encrypted private keys by achow101)
#16910 (wallet: reduce loading time by using unordered maps by achow101)
#16839 (Replace Connman and BanMan globals with Node local by ryanofsky)
#16440 (BIP-322: Generic signed message format by kallewoof)
#16037 (rpc: Enable wallet import on pruned nodes by promag)
#15931 (Remove GetDepthInMainChain dependency on locked chain interface by ariard)
#15761 (Replace -upgradewallet startup option with upgradewallet RPC by achow101)
#15294 ([moveonly] wallet: Extract RipeMd160 by Empact)
#14942 (wallet: Make scan / abort status private to CWallet by Empact)
#14707 ([RPC] Include coinbase transactions in receivedby RPCs by andrewtoth)
#14384 (Fire TransactionRemovedFromMempool callbacks from mempool by l2a5b1)
#14144 (Refactoring: Clarify code using encrypted_batch in CWallet by domob1812)
#10785 (Serialization improvements by sipa)

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.

achow101 force-pushed on Jul 5, 2019

in src/interfaces/wallet.cpp:168 in 1404f8ca28 outdated

192@@ -170,7 +193,7 @@ class WalletImpl : public Wallet
193             *name = it->second.name;
194         }
195         if (is_mine) {
196-            *is_mine = IsMine(*m_wallet, dest);
197+            *is_mine = m_wallet->IsMine(dest);

fqlx commented at 11:55 pm on July 5, 2019:

I’m very suspect at this line. We are assigning a dereference pointer. I think we should prefer is_mine = &m_wallet->IsMine(dest);

sipa commented at 3:18 am on July 6, 2019:

Please try to understand the code you’re commenting on.

dandv commented at 4:12 am on July 6, 2019:

@sipa: Clarifying what you’re referring to would be both constructive and polite :)

achow101 commented at 4:31 am on July 6, 2019:

This line is merely changing how IsMine is called. In order to avoid difficulties for reviewers, the existing code (the assignment itself) is not being changed.

in src/interfaces/wallet.h:84 in 1404f8ca28 outdated

83+    // Get a new address.
84+    virtual bool getNewAddress(const OutputType type, const std::string label, CTxDestination& dest) = 0;
85 
86     //! Get public key.
87-    virtual bool getPubKey(const CKeyID& address, CPubKey& pub_key) = 0;
88+    virtual bool getPubKey(const CScript& script, const CKeyID& address, CPubKey& pub_key) = 0;

fqlx commented at 11:56 pm on July 5, 2019:

This name doesn’t indicate a boolean return value. 🤔 Why not isPubKey()?

dandv commented at 4:14 am on July 6, 2019:

The old get...Key returning bool smells: either the pubKey is a boolean, or getPubKey has a side effect of getting the key somewhere else instead of as a result. Could this PR fix that by renaming it to isPubKey()?

achow101 commented at 4:32 am on July 6, 2019:

I am trying to avoid changing existing code as much as possible in order to avoid additional review burden. Feel free to change this later, but changing it here is out of scope for this PR.

in src/script/signingprovider.cpp:143 in 1404f8ca28 outdated

136@@ -83,23 +137,23 @@ bool CBasicKeyStore::GetKey(const CKeyID &address, CKey &keyOut) const
137     return false;
138 }
139 
140-bool CBasicKeyStore::AddCScript(const CScript& redeemScript)
141+bool FillableSigningProvider::AddCScript(const CScript& redeemScript)
142 {
143     if (redeemScript.size() > MAX_SCRIPT_ELEMENT_SIZE)
144-        return error("CBasicKeyStore::AddCScript(): redeemScripts > %i bytes are invalid", MAX_SCRIPT_ELEMENT_SIZE);
145+        return error("FillableSigningProvider::AddCScript(): redeemScripts > %i bytes are invalid", MAX_SCRIPT_ELEMENT_SIZE);

fqlx commented at 0:08 am on July 6, 2019:

Include redeemScript.size() size in the logging so we can debug easier

achow101 commented at 4:33 am on July 6, 2019:

This is part of a scripted diff. Such a change would both not part of the script and be unrelated to this PR.

in src/script/keyorigin.h:13 in 1404f8ca28 outdated

 9+#include <streams.h>
10+#include <vector>
11+
12+struct KeyOriginInfo
13+{
14+    unsigned char fingerprint[4]; //!< First 32 bits of the Hash160 of the public key at the root of the path

fqlx commented at 0:09 am on July 6, 2019:

Can we use BYTE to be more informative of the type

sipa commented at 3:19 am on July 6, 2019:

C++11 does not have such a type.

in src/wallet/rpcwallet.cpp:3101 in 1404f8ca28 outdated

3096@@ -3083,7 +3097,8 @@ static UniValue listunspent(const JSONRPCRequest& request)
3097         entry.pushKV("spendable", out.fSpendable);
3098         entry.pushKV("solvable", out.fSolvable);
3099         if (out.fSolvable) {
3100-            auto descriptor = InferDescriptor(scriptPubKey, *pwallet);
3101+            const SigningProvider* provider = pwallet->GetSigningProvider(scriptPubKey);
3102+            auto descriptor = InferDescriptor(scriptPubKey, *provider);

fqlx commented at 0:17 am on July 6, 2019:

provider should be checked before dereferencing

achow101 commented at 4:59 am on July 6, 2019:

Done

in src/wallet/wallet.cpp:1346 in 1404f8ca28 outdated

1383+    return GetLegacyScriptPubKeyMan()->ImportPubKeys(ordered_pubkeys, pubkey_map, key_origins, add_keypool, internal, timestamp);
1384 }
1385 
1386 bool CWallet::ImportScriptPubKeys(const std::string& label, const std::set<CScript>& script_pub_keys, const bool have_solving_data, const bool internal, const int64_t timestamp)
1387 {
1388+    auto spk_man =GetLegacyScriptPubKeyMan();

fqlx commented at 0:31 am on July 6, 2019:

Please add space around operators

dandv commented at 4:16 am on July 6, 2019:

Such key code would certainly benefit from linting.

achow101 commented at 4:59 am on July 6, 2019:

Done

in src/script/signingprovider.cpp:105 in 1404f8ca28 outdated

108     vchPubKeyOut = key.GetPubKey();
109     return true;
110 }
111 
112-bool CBasicKeyStore::AddKeyPubKey(const CKey& key, const CPubKey &pubkey)
113+bool FillableSigningProvider::AddKeyPubKey(const CKey& key, const CPubKey &pubkey)

fqlx commented at 3:02 am on July 6, 2019:

Could be void since we always return true.

achow101 commented at 4:34 am on July 6, 2019:

This is existing code and is merely a rename. Changing what it returns is out of scope for this PR.

in src/interfaces/wallet.cpp:118 in 1404f8ca28 outdated

139@@ -140,14 +140,37 @@ class WalletImpl : public Wallet
140     void abortRescan() override { m_wallet->AbortRescan(); }
141     bool backupWallet(const std::string& filename) override { return m_wallet->BackupWallet(filename); }
142     std::string getWalletName() override { return m_wallet->GetName(); }
143-    bool getKeyFromPool(bool internal, CPubKey& pub_key) override
144+    bool getNewAddress(const OutputType type, const std::string label, CTxDestination& dest) override
145     {
146-        return m_wallet->GetKeyFromPool(pub_key, internal);
147+        LOCK(m_wallet->cs_wallet);
148+        std::string error;

fqlx commented at 3:05 am on July 6, 2019:

Looks like we could remove error and use empty string "" below.

achow101 commented at 4:39 am on July 6, 2019:

No, GetNewAddress takes a reference which "" is not.

in src/wallet/wallet.cpp:4002 in 1404f8ca28 outdated

3948-    CKeyMetadata meta;
3949+    LOCK(cs_wallet);
3950+    if (fUseCrypto)
3951+        return true;
3952+    fUseCrypto = true;
3953+    return true;

fqlx commented at 3:14 am on July 6, 2019:

Is this done as an optimization? I’d prefer:

0void CWallet::SetCrypted()
1{ 
2    LOCK(cs_wallet);
3    fUseCrypto = true;
4}

achow101 commented at 4:36 am on July 6, 2019:

This is moved code and I will not be changing it in order to avoid overburdening reviewers with ensuring that behavior hasn’t changed.

fanquake renamed this:
~~Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes)~~
WIP: Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes)
on Jul 6, 2019

fanquake added the label Needs Conceptual Review on Jul 6, 2019

fanquake commented at 4:28 am on July 6, 2019: member

Everyone commenting here, note that this PR is a work in progress, and is built on top of multiple open PRs.

For now we should try and keep the discussion here at a Concept ACK/NACK & design level, rather than individual nit-picks and code review.

If you would like to review the code, please do so in the open base PRs. That will not only save the concept level discussion here from being drowned out in inline comments, but will also prevent duplicate review. i.e What you’re commenting on here might have already been addressed/pointed out in the base PRs.

achow101 force-pushed on Jul 6, 2019

meshcollider commented at 3:03 am on July 7, 2019: contributor

Concept ACK, this abstraction is definitely the right path for the wallet to take IMO as we work toward descriptor wallets.

achow101 force-pushed on Jul 9, 2019

DrahtBot added the label Needs rebase on Jul 10, 2019

achow101 force-pushed on Jul 10, 2019

DrahtBot removed the label Needs rebase on Jul 10, 2019

DrahtBot added the label Needs rebase on Jul 10, 2019

achow101 force-pushed on Jul 10, 2019

DrahtBot removed the label Needs rebase on Jul 10, 2019

Sjors commented at 2:56 pm on July 11, 2019: member

Approach ACK.

To make some of the commits easier to review, instead of having one commit that adds a function to LegacyScriptPubKeyMan (e.g. LegacyScriptPubKeyMan::LoadKey), one commit that deletes it from CWallet (Remove unused functions and switch CWallet to use ScriptPubKeyMan) and one commit that moves the call over (Implement actually loading everything into LegacyScriptPubKeyMan), try to combine those. That lets you get rid of the CI Skip commits, and makes it easier to verify move-only parts with --color-moved=dimmed-zebra.

In other words, start with empty (Legacy)ScriptPubKeyMan and then migrate one function per commit into it.

Are you sure you want to move WalletFeature and WalletFlags into LegacyScriptPubKeyMan? I would prefer to keep these settings global to the wallet. It does make sense for each ScriptPubKeyMan to have its own IsLocked() state, so that brings up the question what to do if a single one fails to unlock. But I’d rather not add RPC methods to unlock specific PubKeyMans in a given wallet.

I brainstormed how to apply this to hardware wallets: #14912 (comment). The unsollicited keypool topup in GetReservedDestination is annoying in that context. Can ScriptPubKeyMan have a flag to opt out of that?

achow101 commented at 5:57 pm on July 11, 2019: member

To make some of the commits easier to review, instead of having one commit that adds a function to LegacyScriptPubKeyMan (e.g. LegacyScriptPubKeyMan::LoadKey), one commit that deletes it from CWallet (Remove unused functions and switch CWallet to use ScriptPubKeyMan) and one commit that moves the call over (Implement actually loading everything into LegacyScriptPubKeyMan), try to combine those. That lets you get rid of the CI Skip commits, and makes it easier to verify move-only parts with --color-moved=dimmed-zebra.

In other words, start with empty (Legacy)ScriptPubKeyMan and then migrate one function per commit into it.

Doing that will cause the entirety of the PR to fail all tests (so all need ci skip) except for the very last commit. If other reviewers want that, I could reorganize it.

Are you sure you want to move WalletFeature and WalletFlags into LegacyScriptPubKeyMan? I would prefer to keep these settings global to the wallet.

They are global to the wallet. They are moved to walletutil.h. Both LegacyScriptPubKeyMan and CWallet require access to both of these enums, but having scriptpubkeyman including wallet.h would be a circular dependency, so I moved it to walletutil.h.

It does make sense for each ScriptPubKeyMan to have its own IsLocked() state, so that brings up the question what to do if a single one fails to unlock.

I guess it should fail to unlock everything? That would indicate some corruption has occurred.

But I’d rather not add RPC methods to unlock specific PubKeyMans in a given wallet.

Agreed.

The unsollicited keypool topup in GetReservedDestination is annoying in that context. Can ScriptPubKeyMan have a flag to opt out of that?\

I suppose it should actually be in LegacyScriptPubKeyMan::GetReservedDestination rather than CWallet’s. However, TopUp can just do nothing and that won’t effect this behavior. Every ScriptPubKeyMan needs to implement TopUp anyways.

achow101 force-pushed on Jul 11, 2019

Sjors commented at 2:18 pm on July 12, 2019: member

In other words, start with empty (Legacy)ScriptPubKeyMan and then migrate one function per commit into it.

Doing that will cause the entirety of the PR to fail all tests (so all need ci skip) except for the very last commit. If other reviewers want that, I could reorganize it.

Why would any of the test fail if ScriptPubKeyMan is just an empty shell, and/or a bunch of NOOP functions?

The unsollicited keypool topup in GetReservedDestination is annoying in that context. Can ScriptPubKeyMan have a flag to opt out of that?

I suppose it should actually be in LegacyScriptPubKeyMan::GetReservedDestination rather than CWallet’s. However, TopUp can just do nothing and that won’t effect this behavior. Every ScriptPubKeyMan needs to implement TopUp anyways.

In the context of a hardware wallet I want TopUp to fetch keys from the device. But it should only try that if the user explicitly calls topupkeypool (and the UI equivalent). Problem is that TopUp doesn’t know where it’s called from.

achow101 force-pushed on Jul 16, 2019

achow101 force-pushed on Jul 17, 2019

achow101 force-pushed on Jul 18, 2019

achow101 force-pushed on Jul 19, 2019

achow101 force-pushed on Jul 22, 2019

DrahtBot added the label Needs rebase on Jul 24, 2019

achow101 force-pushed on Jul 25, 2019

DrahtBot removed the label Needs rebase on Jul 25, 2019

achow101 force-pushed on Jul 25, 2019

laanwj added this to the "Chasing Concept ACK" column in a project

achow101 force-pushed on Jul 26, 2019

achow101 renamed this:
~~WIP: Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes)~~
Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes)
on Jul 26, 2019

achow101 commented at 7:51 pm on July 26, 2019: member

The PRs this depended on have been merged. I don’t foresee making large changes to this anymore, so this is no longer WIP.

Please review!

fanquake removed the label Needs Conceptual Review on Jul 26, 2019

achow101 force-pushed on Jul 29, 2019

DrahtBot added the label Needs rebase on Jul 30, 2019

achow101 force-pushed on Jul 30, 2019

DrahtBot removed the label Needs rebase on Jul 30, 2019

achow101 force-pushed on Jul 30, 2019

DrahtBot added the label Needs rebase on Jul 31, 2019

achow101 force-pushed on Jul 31, 2019

DrahtBot removed the label Needs rebase on Jul 31, 2019

DrahtBot added the label Needs rebase on Aug 1, 2019

achow101 force-pushed on Aug 1, 2019

DrahtBot removed the label Needs rebase on Aug 1, 2019

laanwj moved this from the "Chasing Concept ACK" to the "Blockers" column in a project

DrahtBot added the label Needs rebase on Aug 2, 2019

achow101 force-pushed on Aug 2, 2019

DrahtBot removed the label Needs rebase on Aug 2, 2019

in src/script/sign.cpp:151 in ca02ac2532 outdated

143@@ -144,8 +144,10 @@ static bool SignStep(const SigningProvider& provider, const BaseSignatureCreator
144         ret.push_back(valtype()); // workaround CHECKMULTISIG bug
145         for (size_t i = 1; i < vSolutions.size() - 1; ++i) {
146             CPubKey pubkey = CPubKey(vSolutions[i]);
147-            if (ret.size() < required + 1 && CreateSig(creator, sigdata, provider, sig, pubkey, scriptPubKey, sigversion)) {
148-                ret.push_back(std::move(sig));
149+            if (CreateSig(creator, sigdata, provider, sig, pubkey, scriptPubKey, sigversion)) {
150+                if (ret.size() < required + 1) {

elichai commented at 2:12 pm on August 5, 2019:

The nice thing about the way it was done before is that if this expr returns false it would get short circuited and CreateSig wouldn’t be called too.

Now it’s the opposite. which I think makes less sense when we know that CreateSig is more work than that simple expr, so it might do CreateSig just to fail that if afterwards.

achow101 commented at 2:49 pm on August 5, 2019:

This is necessary for LegacyScriptPubKeyMan::CanProvide to pass. That uses the DUMMY_SIGNATURE_CREATOR which creates dummy signatures without checking whether the SigningProvider has the private keys (as it should). However, in CanProvide, we want to make sure that we do actually have the private keys, and if the dummy signing returns us dummy signatures that correspond to keys that we don’t have, CanProvide will fail. So we need it to give us all of the signatures (by putting them in the sigdata) so that CanProvide can check for whether it actually could sign.

ryanofsky commented at 6:33 pm on August 22, 2019:

In commit “Always try to sign for all pubkeys in multisig” (d0b41dc26a74e158bd22259aed50869c9676e3fd)

I’m not deeply familiar with this code, and I found the explanation a little hard to follow, but in case it helps anyone else, the key seems to be that calling CreateSig unconditionally is needed to fill sigdata. I can confirm that rpc_psbt.py test fails without this change.

instagibbs commented at 3:21 pm on August 27, 2019:

I also found this explanation difficult. Is there any way to succinctly say the desired effect, and put it in a comment in the code?

achow101 commented at 7:49 pm on August 30, 2019:

I added a comment.

Sjors commented at 1:59 pm on September 17, 2019:

Can you add the bit about LegacyScriptPubKeyMan::CanProvide as well to the comment?

achow101 commented at 5:30 pm on September 18, 2019:

I don’t think it makes sense to have a comment with such specific information about the wallet in the non-wallet signing code. There’s already a comment there explaining why we try to sign for all keys.

in src/outputtype.cpp:95 in ca02ac2532 outdated

93         CScript witprog = GetScriptForDestination(witdest);
94         // Check if the resulting program is solvable (i.e. doesn't use an uncompressed key)
95-        if (!IsSolvable(keystore, witprog)) return ScriptHash(script);
96+        if (!IsSolvable(keystore, witprog)) {
97+            keystore.AddCScript(GetScriptForDestination(sh));
98+            return ScriptHash(script);

elichai commented at 2:40 pm on August 5, 2019:

shouldn’t this be return sh;?

achow101 commented at 3:05 pm on August 5, 2019:

Yes, done.

achow101 force-pushed on Aug 5, 2019

in src/wallet/wallettool.cpp:100 in 12a411b8a7 outdated

 97+    LOCK2(wallet_instance->cs_wallet, spk_man->cs_KeyStore);
 98 
 99     tfm::format(std::cout, "Wallet info\n===========\n");
100     tfm::format(std::cout, "Encrypted: %s\n", wallet_instance->IsCrypted() ? "yes" : "no");
101-    tfm::format(std::cout, "HD (hd seed available): %s\n", wallet_instance->GetHDChain().seed_id.IsNull() ? "no" : "yes");
102+    tfm::format(std::cout, "HD (hd seed available): %s\n", (spk_man && spk_man->GetHDChain().seed_id.IsNull()) ? "no" : "yes");

elichai commented at 3:09 pm on August 5, 2019:

is checking that spk_man isn’t null needed here assuming you already call spk_man->cs_KeyStore which is a deference?

sipa commented at 3:20 pm on August 5, 2019:

&& short-circuits; it doesn’t evaluate its right hand is the left hand was false.

elichai commented at 3:24 pm on August 5, 2019:

Yes. My point was that he deferences it a few lines above when he acquires a lock.

achow101 commented at 3:30 pm on August 5, 2019:

wallettool looks to be broken when there are other ScriptPubKeyMan classes. I’ll investigate in the Descriptor wallets PR.

achow101 commented at 3:53 pm on August 5, 2019:

This change to using spk_man here is not actually needed. I’ve changed it to use CWallet::IsHDEnabled

in src/rpc/rawtransaction_util.cpp:150 in 12a411b8a7 outdated

146@@ -147,9 +147,8 @@ static void TxInErrorToJSON(const CTxIn& txin, UniValue& vErrorsRet, const std::
147     vErrorsRet.push_back(entry);
148 }
149 
150-UniValue SignTransaction(CMutableTransaction& mtx, const UniValue& prevTxsUnival, FillableSigningProvider* keystore, std::map<COutPoint, Coin>& coins, bool is_temp_keystore, const UniValue& hashType)
151+void ParsePrevouts(const UniValue& prevTxsUnival, FillableSigningProvider* keystore, std::map<COutPoint, Coin>& coins)

elichai commented at 3:16 pm on August 5, 2019:

nit. snake case for the function arguments?

ryanofsky commented at 4:55 pm on August 27, 2019:

In commit “Refactor rawtransaction_util’s SignTransaction to have previous tx parsing be separate” (b49bbb939be92a67ff77c3f7bca5bb94dd141906)

nit. snake case for the function arguments?

Better in a separate PR, this PR is huge already and the change would make this commit a lot harder to follow.

Sjors commented at 8:43 am on August 31, 2019:

I think this commit deserves its own PR anyway; as does any refactor that’s a prequisite for the main PR.

achow101 force-pushed on Aug 5, 2019

Sjors commented at 1:03 pm on August 13, 2019: member

Found a bug (try in QT):

create a wallet blank wallet with private keys disabled
see that new address button in receive screen is disabled, as it should
close QT and start again, load the wallet
see that new address button is enabled and creates addresses (panic)

Maybe Always try to sign for all pubkeys in multisig can be its own PR?

I didn’t pay precise attention to which methods are private / public / etc, because that’s easy to change later as required.

Shameless plug for #12134 so we can test for accidental breakage of serialization. The existing test passes on top of this PR, but it only checks that a default “master” wallet can still be opened in earlier versions).

in src/wallet/scriptpubkeyman.h:119 in bae5b518ec outdated

115@@ -85,7 +116,9 @@ class ScriptPubKeyMan
116 class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProvider
117 {
118 public:
119-    LegacyScriptPubKeyMan() {}
120+    LegacyScriptPubKeyMan(FlagSetFunc is_set_func, FlagFunc set_flag_func, FlagFuncWithDB unset_flag_func, VersionFunc feature_sup_func, NameFunc wallet_name_func, SetVersionFunc set_version_func, std::shared_ptr<WalletDatabase> database)

ryanofsky commented at 6:49 pm on August 22, 2019:

In commit “Add function callbacks for wallet flags and versions and wallet database” (bae5b518ec507d071a54ce4810dd24b016bcfe12)

This could just say using ScriptPubKeyMan::ScriptPubKeyMan to inherit the parent constructor instead of repeating it all. Even at the end of this PR, this constructor is empty except for some member initializations which would be better to do at the points of declaration.

achow101 commented at 7:49 pm on August 30, 2019:

Done

ryanofsky commented at 7:05 pm on August 22, 2019: member

Started review (will update list below with progress). Heroic effort here by achow! I’ve started to review a few times, but put it off for too long. Time to seriously jump in.

My initial impression here is in line with Sjors, that it would have been wiser to have sequence of commits gradually moving bits of code and bits of data with all tests passing at every commit, rather than a sequence of commits copying then a sequence of commits deleting. Moving is nicer than copying for review, because you can see the original code right in the diff instead of having to search for it manually. But I know there was an earlier attempt at this, and it didn’t go well for whatever reason, and I don’t think it’s worth putting in more effort just to restructure the PR.

d0b41dc26a74e158bd22259aed50869c9676e3fd Always try to sign for all pubkeys in multisig (1/70)
e892a72f22c3359a12085f0e93d519acaa711bcd Introduce both ScriptPubKeyMan as an interface and LegacyScriptPubKeyMan as a dummy class (2/70)
8a8e141fe93bf887630df9681daa9cd8b2500f55 List output types in an array in order to be iterated over (3/70)
7a27857f91ad585f82bb784faf092e8fe7e30946 Add LegacyScriptPubKeyMan to CWallet (4/70)
16e82f0b9252cddb50b0aeeaedb13a2ebf532c29 Move wallet enums to walletutil.h (5/70)
bae5b518ec507d071a54ce4810dd24b016bcfe12 Add function callbacks for wallet flags and versions and wallet database (6/70)
493486c32ad084099636c8696c10f4e21f56326e Fetch the SigningProvider for a script from the wallet (7/70)
bec9b6c2f00113c4bd789562f1333d30c959ed00 Fetch the ScriptPubKeyMan for given output type and internal-ness, or a given script, or ScriptPubKeyMan id (8/70)
b49bbb939be92a67ff77c3f7bca5bb94dd141906 Refactor rawtransaction_util’s SignTransaction to have previous tx parsing be separate (9/70)
a31374df794d709cf8acfbafc7d5b57f74b1d75a Implement GetSigningProvider in LegacyScriptPubKeyMan (10/70)
7b57deac879499e4b0ee99f1efc320600907bc59 Implement function to connect ScriptPubKeyMan’s NotifyCanGetAddessesChanged and NotifyWatchOnlyChanged to CWallet’s (11/70)
72a44f23a9000765bac8102f53d6606c3f0b0fe0 Implement IsLocked and IsCrypted in LegacyScriptPubKeyMan (12/70)
fce5df284b2fc1b0b7f9582275c11dcce8dded86 Implement LoadCryptedKey and AddCryptedKey in LegacyScriptPubKeyMan (13/70)
6685ff5f9c0265094b97aa5aaa03d6b10ec398bb Implement UpdateTimeFirstKey, and GetTimeFirstKey in LegacyScriptPubKeyMan (14/70)
eeff18e1a8e2240a9e0d0fdd252ae05f36e7f502 Implement AddWatchOnly, RemoveWatchOnly, HaveWatchOnly, and LoadWatchOnly in LegacyScriptPubKeyMan (15/70)
95522e7f84fc311521a5b4246a44a314cdc9a5ed Implement AddKeyPubKey and LoadKey in LegacyScriptPubKeyMan (16/70)
c88536aaab4af1d54b42d8e95d7f9a493d1919f7 Implement WalletLogPrintf in LegacyScriptPubKeyMan (17/70)
977cb6552f033f826bc06198b1855790e61a0e8a Implement SetHDCHain, and IsHDEnabled in LegacyScriptPubKeyMan (18/70)
bf224a9ccc477aa7c20c3bfd6b448b1cdf7f15c0 Implement LoadCScript in LegacyScriptPubKeyMan (19/70)
e205c2c933ebde193cce7b926be240ed3f226e31 Implement LoadKeyMetadata and LoadScriptMetadata in LegacyScriptPubKeyMan (20/70)
7432db927f5990b13539999508e47a8c8d8fec1e Implement GetKey, HaveKey, and GetPubKey in LegacyScriptPubKeyMan (21/70)
0922f3f9370f00feb84690bc7942ecac9737d492 Implement GenerateNewKey in LegacyScriptPubKeyMan (22/70)
c0b6d2b211a39a50e32728e4d8f52c2bf2ad7e79 Implement LoadKeyPool in LegacyScriptPubKeyMan (23/70)
d3e3cc9e9f5ecd9512f4eeb8b51a789f05ac0529 Implement GetOldestKeyPoolTime, KeypoolCountExternalKeys, and GetKeypoolSize in LegacyScriptPubKeyMan (24/70)
acbd549766a89c759b3a1aa435bd001c7082a323 Implement CanGetAddresses, CanGenerateKeys, and HavePrivateKeys in LegacyScriptPubKeyMan (25/70)
5e7d3149895228c4b7b7c7c582f877938b217b23 Implement GenerateNewSeed, DeriveNewSeed, and SetHDSeed for LegacyScriptPubKeyMan (26/70)
d229452d6dbd94331986d267b46d13af745bf98d Implement TopUpKeypool, TopUp, and NewKeyPool in LegacyScriptPubKeyMan (27/70)
4500e0af8f964289acb680bd76031c59ffa8b968 Implement ReturnAddress, and KeepKey in LegacyScriptPubKeyMan (28/70)
a7855dd0cf12314ad4a99ff77de22e60f8a9f435 Implement GetNewAddress, and GetReservedAddress in LegacyScriptPubKeyMan (29/70)
9947416fef270701c9d22fc914830448112e6bb8 Implement MarkUnusedAddresses in LegacyScriptPubKeyMan (30/70)
0ff797c105a3917c4b2fdcf0bb172125fe91349a Implement IsMine in LegacyScriptPubKeyMan (31/70)
eee7b80e8c8a43b7a41bdb6df68735f295f777d2 Implement UpgradeKeyMetaData, SetupGeneration, IsFirstRun, Upgrade, RewriteDB in LegacyScriptPubKeyMan (32/70)
7a5ad98e1733f44f7b71b7247f540dd97acdcb4e Implement Unlock, Lock, and Encrypt and LegacyScriptPubKeyMan (33/70)
f489836fdd5ced869babe04352c473c678d2a9a3 Implement ImporScripts, ImportPrivKeys, ImportPubKeys, and ImportScriptPubKeys in LegacyScriptPubKeyMan (34/70)
cb1519c317ebcf5762567018f881d1faa5957734 Implement GetMetadata in LegacyScriptPubKeyMan (35/70)
b893f0d053f1b5be1229d4fb77ced46b222f9993 Implement GetKeyOrigin in LegacyScriptPubKeyMan (36/70)
93322bf756fd8991fb445e327c878e2695fe9ca9 Implement actually loading everything into LegacyScriptPubKeyMan (37/70)
ae87c89f2e66dc7cb07c541261069c08ad809ddb Implement CanProvide in LegacyScriptPubKeyMan (38/70)
6ac3cf53cca1714f6d6c9fb3ebe16824b9ae49ea [ci skip] Remove CWallet from IsMine and have CWallet always use ScriptPubKeyMan’s IsMine (39/70)
d33724a6ca11134fa4ea1f80cacbc5b873c837e3 [ci skip] moveonly: move ismine stuff to be a module of LegacyScriptPubKeyMan (40/70)
6efeaa2ea30a4c128101ddf4cb9ee1eebb08aa9a [ci skip] Have GetNewAddress, GetNewChangeAddress, and ReserveAddress use ScriptPubKeyMan (41/70)
9603366a7879c225aeef97988323fc482eef5f44 [ci skip] Mark used addresses in ScriptPubKeyMan (42/70)
bc6d7687f9db39577d280c500896493de1525f2f [ci skip] Call UpgradeKeyMetaData for each ScriptPubKeyMan (43/70)
18602887c67b68f141d43f187d52ce32ae3af02b [ci skip] Sign using SigningProvider from ScriptPubKeyMan when signing within CWallet (44/70)
f95622c8db42713e617dea3af79be0531c8c69ec [ci skip] Do not allow import*, dump*, and addmultisigaddress RPCs when wallet is not backed by LegacyScriptPubKeyMan (45/70)
08f509b7d99cf3e5a6e4654c45e411dafd3c94f7 [ci skip] Change Imports to use LegacyScriptPubKeyMan Imports (46/70)
086225731c5bb99438d5cda6174a459332e1abdf [ci skip] Use SigningProviders and ScriptPubKeyMans in listunspent, signmessage, signrawtransactionwithwallet, and getaddressinfo (47/70)
74777d18512f5ff38c0af86cc648fcf60c8d1ea4 [ci skip] Use LegacyScriptPubKeyMan in addmultisigaddress and sethdseed (48/70)
bac9fda14e9bf597d85d51cebfae210a03c33c07 [ci skip] Use LegacyScriptPubKeyMan for hdseedid in getwalletinfo (49/70)
0a23a92785a2bc0eef233ad4c2ac58b7c354ff88 [ci skip] Change KeypoolCountExternal and GetKeypoolSize to get aggregate sizes from ScriptPubKeyMans (50/70)
5db4f1fbf38cf7ad55f358d1fe918cb776e72fc9 [ci skip] Have IsHDEnabled fetch from ScriptPubKeyMans (51/70)
29642268998127a7e46ad4ccf04cbf2bc66e1653 [ci skip] Fetch oldest keypool time from ScriptPubKeyMans (52/70)
662d0002a3bc31be2553800b1ac9d2666df7b680 [ci skip] have TopUpKeyPool call TopUp in each ScriptPubKeyMan (53/70)
e32509e49d5f4301d6dd677d682b929caa3a2494 [ci skip] Have EncryptWallet, Lock, and Unlock call their respective functions in ScriptPubKeyMans (54/70)
cb12ea9cc8f0df83f9e29b0ac8abe48567ce4fa1 [ci skip] Use LegacyScriptPubKeyMan throughout psbt_wallet_tests (55/70)
39e9d613aebfa611b3f599916cd595a66500c595 [ci skip] Use LegacyScriptPubKeyMan throughout wallettool (56/70)
dbb0d8f6af94300dbe84e0dd1595e0a9d45d3d07 [ci skip] Use ScriptPubKeyMans’ Setup and Upgrade functions when loading or creating a wallet (57/70)
c09a7967dab25904218e6b850e871f49b4f9ea78 [ci skip] Define first run as having no ScriptPubKeyMans (58/70)
d188254886cf7ac429c1d6ebcb1e18f09f215a61 [ci skip] Use RewriteDB action when DB needs rewrite (59/70)
5a837cbec18b6e7bf6903816f479b96fc31f0b7e [ci skip] Use GetTimeFirstKey instead of nTimeFirstKey (60/70)
04456084ef2600909a2d162dbfae9197afcb94bb [ci skip] Use LegacyScriptPubKeyMan for in wallet_tests (61/70)
2dd631c391fee7fccaba4d536d08306de867accd [ci skip] Use LegacyScriptPubKeyMan in dumpprivkey and dumpwallet (62/70)
dadcbef6226ed1cddcf18ebbbe12ca8470d56946 [ci skip] Change CanGetAddresses to fetch from ScriptPubKeyMan (63/70)
f353a3ca99600e6e25b92e478cb5592c69a62ed8 [ci skip] Fetch the correct SigningProvider for signing PSBTs (64/70)
fef2ffa7ac0df3373ea3146e11057a1932736336 [ci skip] Use LegacyScriptPubKeyMan in test util (65/70)
6aac37c38c5d8d3aea67209931a7de5a14fe92bb [ci skip] Use LegacyScriptPubKeyMan in some parts of getbalances and createwallet (66/70)
43819a86b248c089bcfc342e7c7ce43060f507c1 [ci skip] Have getPubKey and getPrivKey use SigningProvider (67/70)
4e7c2d8fc7835133c39ad8b8c7a805d49679c0f4 [ci skip] Use LegacyScriptPubKeyMan in benchmarks involving the wallet (68/70)
dc262606b03486af8045b71e78b8292ebf225536 [ci skip] Store p2sh scripts in AddAndGetDestinationForScript (69/70)
c5f900358e582607c8386430ea3a63f02533b64e Remove unused functions and switch CWallet to use ScriptPubKeyMan (70/70)

in src/wallet/scriptpubkeyman.h:145 in e892a72f22 outdated

13+
14+#include <boost/signals2/signal.hpp>
15+
16+enum class OutputType;
17+
18+class ScriptPubKeyMan

instagibbs commented at 3:27 pm on August 27, 2019:

Please add a short comment on what this is in charge of and what it’s not.

achow101 commented at 7:49 pm on August 30, 2019:

Done

in src/outputtype.h:31 in 8a8e141fe9 outdated

27@@ -27,6 +28,8 @@ enum class OutputType {
28     CHANGE_AUTO,
29 };
30 
31+const std::array<OutputType, 3> output_types = {OutputType::LEGACY, OutputType::P2SH_SEGWIT, OutputType::BECH32};

instagibbs commented at 3:31 pm on August 27, 2019:

s/output_types/OUTPUT_TYPES/

achow101 commented at 7:49 pm on August 30, 2019:

Done

in src/wallet/wallet.h:695 in 7a27857f91 outdated

833@@ -833,6 +834,11 @@ class CWallet final : public FillableSigningProvider, private interfaces::Chain:
834     //! Fetches a key from the keypool
835     bool GetKeyFromPool(CPubKey &key, bool internal = false);
836 
837+    std::map<OutputType, std::shared_ptr<ScriptPubKeyMan>> m_external_spk_managers;
838+    std::map<OutputType, std::shared_ptr<ScriptPubKeyMan>> m_internal_spk_managers;
839+
840+    std::map<uint256, std::shared_ptr<ScriptPubKeyMan>> m_spk_managers;

instagibbs commented at 3:38 pm on August 27, 2019:

Comment on how these are indexed?

achow101 commented at 7:53 pm on August 30, 2019:

Done

in src/wallet/wallet.cpp:4120 in 7a27857f91 outdated

4934@@ -4935,3 +4935,32 @@ bool CWallet::AddCryptedKeyInner(const CPubKey &vchPubKey, const std::vector<uns
4935     ImplicitlyLearnRelatedKeyScripts(vchPubKey);
4936     return true;
4937 }
4938+
4939+std::shared_ptr<LegacyScriptPubKeyMan> CWallet::GetLegacyScriptPubKeyMan()
4940+{
4941+    SetupLegacyScriptPubKeyMan();
4942+    return std::dynamic_pointer_cast<LegacyScriptPubKeyMan>(m_internal_spk_managers[OutputType::LEGACY]);

instagibbs commented at 3:42 pm on August 27, 2019:

m_internal_spk_managers[OutputType::LEGACY]

Please leave a comment noting that any output type is the same in this context.

achow101 commented at 7:49 pm on August 30, 2019:

Done

in src/wallet/wallet.cpp:4949 in 7a27857f91 outdated

4944+
4945+void CWallet::SetupLegacyScriptPubKeyMan()
4946+{
4947+    if (m_internal_spk_managers.empty() && m_external_spk_managers.empty() && m_spk_managers.empty()) {
4948+        auto spk_manager = std::shared_ptr<ScriptPubKeyMan>(new LegacyScriptPubKeyMan());
4949+        m_internal_spk_managers[OutputType::LEGACY] = spk_manager;

instagibbs commented at 3:49 pm on August 27, 2019:

we have the list of output types, let’s just iterate through them here to build this

achow101 commented at 7:49 pm on August 30, 2019:

Done

in src/wallet/wallet.cpp:4960 in 7a27857f91 outdated

4955+        m_spk_managers[spk_manager->GetID()] = spk_manager;
4956+    }
4957+    // These all need to exist and be the same
4958+    assert(m_internal_spk_managers.count(OutputType::LEGACY) > 0);
4959+    std::shared_ptr<ScriptPubKeyMan> spk_man = m_internal_spk_managers.at(OutputType::LEGACY);
4960+    assert(m_internal_spk_managers.at(OutputType::P2SH_SEGWIT) == spk_man);

instagibbs commented at 3:49 pm on August 27, 2019:

we have the list of output types, let’s just iterate through them here to assert these

achow101 commented at 7:50 pm on August 30, 2019:

Done

in src/wallet/scriptpubkeyman.h:174 in a31374df79 outdated

166@@ -167,4 +167,20 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv
167     uint256 GetID() const override;
168 };
169 
170+/** Wraps a LegacyScriptPubKeyMan so that it can be returned in a new unique_ptr */
171+class LegacySigningProvider : public SigningProvider
172+{
173+private:
174+    const LegacyScriptPubKeyMan* spk_man;

ryanofsky commented at 5:01 pm on August 27, 2019:

In commit “Implement GetSigningProvider in LegacyScriptPubKeyMan” (a31374df794d709cf8acfbafc7d5b57f74b1d75a)

Not important, but this should never be null so a reference makes more sense than a pointer in this class

achow101 commented at 7:49 pm on August 30, 2019:

Done

in src/wallet/wallet.cpp:5039 in 7b57deac87 outdated

5032@@ -5032,3 +5033,11 @@ void CWallet::SetupLegacyScriptPubKeyMan()
5033     assert(m_external_spk_managers.at(OutputType::BECH32) == spk_man);
5034     assert(m_spk_managers.size() == 1);
5035 }
5036+
5037+void CWallet::ConnectScriptPubKeyManNotifiers()
5038+{
5039+    for (auto spk_man_pair : m_spk_managers) {

ryanofsky commented at 5:04 pm on August 27, 2019:

In commit “Implement function to connect ScriptPubKeyMan’s NotifyCanGetAddessesChanged and NotifyWatchOnlyChanged to CWallet’s” (7b57deac879499e4b0ee99f1efc320600907bc59)

const auto& instead of auto would avoid copying and incrementing / decrementing shared_ptr atomic reference counts, would also make it clear that the std::pair is not changing

achow101 commented at 7:49 pm on August 30, 2019:

Changed all of these iterations to const auto&

instagibbs commented at 4:05 pm on August 28, 2019: member

dropping some comments on a partial review

DrahtBot added the label Needs rebase on Aug 30, 2019

in src/wallet/test/coinselector_tests.cpp:126 in b893f0d053 outdated

122@@ -123,6 +123,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)
123 {
124 
125     LOCK(testWallet.cs_wallet);
126+    testWallet.SetupLegacyScriptPubKeyMan();

ryanofsky commented at 6:24 pm on August 30, 2019:

In commit “Implement GetKeyOrigin in LegacyScriptPubKeyMan” (b893f0d053f1b5be1229d4fb77ced46b222f9993)

It’s unclear why this change is in this commit. Is it supposed to be part of the next one 93322bf756fd8991fb445e327c878e2695fe9ca9?

achow101 commented at 7:50 pm on August 30, 2019:

Yeah. Moved it

instagibbs commented at 6:43 pm on August 30, 2019: member

Suggestion just to see what others think: Split up this PR into a couple PRs, first of which is the non-test-breaking changes, the 2nd all the breaking changes+test fixes at the end?

ryanofsky commented at 6:48 pm on August 30, 2019: member

Suggestion just to see what others think: Split up this PR into a couple PRs, first of which is the non-test-breaking changes, the 2nd all the breaking changes+test fixes at the end?

I definitely would not want this because it would temporarily add a huge amount of duplicate code to the wallet. If this needs to be broken up to be reviewed (and I don’t think it does), I’d either go with sjors approach moving things gradually, or just have one giant move-only commit followed by the actual code changes.

ryanofsky commented at 6:52 pm on August 30, 2019: member

Or, in case I misunderstood, if you are just talking about splitting the first 9 commits into their own PR (the commits before the “Implement…” commits and the “ci skip” commits), that seems perfectly fine and would help a little.

achow101 force-pushed on Aug 30, 2019

achow101 commented at 8:12 pm on August 30, 2019: member

Rebased and addressed comments.

Will look at the bug @Sjors reported next.

achow101 force-pushed on Aug 30, 2019

achow101 commented at 8:52 pm on August 30, 2019: member

The bug @Sjors found should be fixed now.

DrahtBot removed the label Needs rebase on Aug 30, 2019

Sjors commented at 9:03 am on August 31, 2019: member

The blank wallet bug is indeed fixed.

I like the idea of splitting the first 9 commits out, maybe into two or three PRs. The refactors touch very different spots, so could be multiple PRs. At minimum I would put the pure refactors and new ScriptPubKeyman stuff separate PRs. Right now the commits alternate between refactors and adding new stuff.

d9ea84699fc0c63003b759ec8e41802a3e1a2cec Always try to sign for all pubkeys in multisig (haven’t reviewed yet)
cce926c57 Refactor rawtransaction_util's SignTransaction code review ACK
d2d43ab141e7f27b3c965ebbf5e7db10476837f3 Move wallet enums to walletutil.h ACK, confirmed move only

In general I suggest splitting as much as possible out into followup, prerequisite and parallel PRs. Then let’s see if we can review the remaining blob as a whole, or keep chipping small bits off.

Just make sure to add all these PRs to https://github.com/bitcoin/bitcoin/projects/12; I find that work flow pretty efficient in the assume UTXO protect https://github.com/bitcoin/bitcoin/projects/11

achow101 commented at 7:54 pm on September 3, 2019: member

I split cce926c into #16798

I’m unsure about splitting the other two “unrelated” commits since they don’t actually do anything useful outside of this PR.

d2d43ab is purely to avoid a circular dependency, and d9ea846 is only to fix a side effect of this PR.

Also reordered the first several commits so the “unrelated” commits come first.

achow101 force-pushed on Sep 3, 2019

in src/wallet/wallet.h:1324 in c4cbc90efe outdated

1318@@ -1319,6 +1319,16 @@ class CWallet final : public FillableSigningProvider, private interfaces::Chain:
1319     /** Implement lookup of key origin information through wallet key metadata. */
1320     bool GetKeyOrigin(const CKeyID& keyid, KeyOriginInfo& info) const override;
1321 
1322+    std::set<std::shared_ptr<ScriptPubKeyMan>> GetActiveScriptPubKeyMans() const;
1323+
1324+    //! Get the ScriptPubKeyMan for the given OutputType and internal/external chain. If it doesn't exist, instantiate one

instagibbs commented at 8:06 pm on September 6, 2019:

this function doesn’t create one, just returns null

achow101 commented at 11:17 pm on September 6, 2019:

Edited comment.

in src/wallet/scriptpubkeyman.h:237 in f504e8a537 outdated

122@@ -123,6 +123,15 @@ class ScriptPubKeyMan
123 
124 class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProvider
125 {
126+private:
127+    //! if fUseCrypto is true, mapKeys must be empty
128+    //! if fUseCrypto is false, vMasterKey must be empty
129+    std::atomic<bool> fUseCrypto{false};

instagibbs commented at 8:10 pm on September 6, 2019:

I know this is from CWallet but I’d rather avoid hungarian notation for new variables: m_use_crypto

instagibbs commented at 9:03 pm on September 6, 2019:

on second thought, nevermind. for now straight copy is good.

in src/wallet/scriptpubkeyman.h:244 in f504e8a537 outdated

128+    //! if fUseCrypto is false, vMasterKey must be empty
129+    std::atomic<bool> fUseCrypto{false};
130+
131+    bool SetCrypted();
132+
133+    CKeyingMaterial vMasterKey GUARDED_BY(cs_KeyStore);

instagibbs commented at 8:13 pm on September 6, 2019:

I know there’s no documentation where you got this from but could there be an explanation of this field?

achow101 commented at 11:18 pm on September 6, 2019:

Done

in src/wallet/scriptpubkeyman.cpp:210 in f504e8a537 outdated

15@@ -16,12 +16,27 @@ isminetype LegacyScriptPubKeyMan::IsMine(const CScript& script) const
16 
17 bool LegacyScriptPubKeyMan::IsCrypted() const
18 {
19-    return false;
20+    return fUseCrypto;

instagibbs commented at 8:15 pm on September 6, 2019:

for reviewers: this whole commit is cloned from CWallet’s implementation

in src/wallet/scriptpubkeyman.cpp:164 in d5bec330bd outdated

159+    LOCK(cs_KeyStore);
160+    if (!SetCrypted()) {
161+        return false;
162+    }
163+
164+    mapCryptedKeys[vchPubKey.GetID()] = make_pair(vchPubKey, vchCryptedSecret);

instagibbs commented at 8:17 pm on September 6, 2019:

I know this is cloning but std::make_pair like everywhere else

achow101 commented at 11:18 pm on September 6, 2019:

Done

in src/wallet/scriptpubkeyman.h:272 in aba2ac2b7e outdated

161@@ -161,6 +162,9 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv
162     //! Adds a watch-only address to the store, and saves it to disk.
163     bool AddWatchOnlyWithDB(WalletBatch &batch, const CScript& dest, int64_t create_time) EXCLUSIVE_LOCKS_REQUIRED(cs_KeyStore);
164 
165+    //! Adds a key to the store, and saves it to disk.

instagibbs commented at 8:28 pm on September 6, 2019:

please change all these “store” references to pubkeyman references

achow101 commented at 11:11 pm on September 6, 2019:

I don’t think it’s necessary to change all of these.

in src/wallet/scriptpubkeyman.cpp:485 in 4d53fa7f1c outdated

480+        secret.MakeNewKey(fCompressed);
481+    }
482+
483+    // Compressed public keys were introduced in version 0.6.0
484+    if (fCompressed) {
485+        SetMinVersion(FEATURE_COMPRPUBKEY, nullptr, false);

instagibbs commented at 8:50 pm on September 6, 2019:

this seems to add the optional params for no reason vs master. remove them, or at least annotate what they mean.

achow101 commented at 11:18 pm on September 6, 2019:

Removed the optional params. They still need to be specified in when binding the function though.

in src/wallet/scriptpubkeyman.h:351 in c43d7572a2 outdated

124+
125+    void RewriteDB() override;
126+
127+    int64_t GetOldestKeyPoolTime() override;
128+    size_t KeypoolCountExternalKeys() override EXCLUSIVE_LOCKS_REQUIRED(cs_KeyStore);
129+    unsigned int GetKeypoolSize() const override;

instagibbs commented at 8:58 pm on September 6, 2019:

renamed to GetTotalKeypoolSize to make it clear it counts both?

achow101 commented at 11:10 pm on September 6, 2019:

It’s always counted both, and I would prefer to keep this consistent with CWallet::GetKeypoolSize.

instagibbs commented at 9:03 pm on September 6, 2019: member

reviewed through d7ff25c778ee1fbafa349b4623b3c5241ca0097c : “Implement CanGetAddresses, CanGenerateKeys, and HavePrivateKeys in LegacyScriptPubKeyMan”

achow101 force-pushed on Sep 6, 2019

fanquake referenced this in commit 46494b08e2 on Sep 7, 2019

DrahtBot added the label Needs rebase on Sep 7, 2019

achow101 force-pushed on Sep 7, 2019

DrahtBot removed the label Needs rebase on Sep 7, 2019

instagibbs commented at 2:42 pm on September 9, 2019: member

only changes through “Implement CanGetAddresses, CanGenerateKeys, and HavePrivateKeys in LegacyScriptPubKeyMan” were the suggested ones in above review.

in src/wallet/scriptpubkeyman.cpp:17 in a007c8ac72 outdated

 7 #include <wallet/scriptpubkeyman.h>
 8 
 9 bool LegacyScriptPubKeyMan::GetNewDestination(const OutputType type, CTxDestination& dest, std::string& error)
10 {
11-    return false;
12+    TopUpKeyPool();

instagibbs commented at 3:14 pm on September 9, 2019:

missing error.clear() that CWallet has?

achow101 commented at 6:00 pm on September 9, 2019:

Done

in src/wallet/scriptpubkeyman.h:295 in a007c8ac72 outdated

283@@ -284,6 +284,25 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv
284     std::map<CKeyID, int64_t> m_pool_key_to_index;
285     std::map<int64_t, CKeyID> m_reserved_key_to_index;

instagibbs commented at 3:16 pm on September 9, 2019:

realized this doesn’t exist in master, please put explanation in comment on what it’s used for

achow101 commented at 6:00 pm on September 9, 2019:

Done.

in src/wallet/scriptpubkeyman.cpp:154 in c83bb3c7ad outdated

149+            if (GetPubKey(meta_pair.first, pubkey)) {
150+                batch->WriteKeyMetadata(meta, pubkey, true);
151+            }
152+        }
153+    }
154+    batch.reset(); //write before setting the flag

instagibbs commented at 3:33 pm on September 9, 2019:

there’s no flag being set here anymore. If intentional, I’d like an explanation and fixup of this comment

achow101 commented at 6:00 pm on September 9, 2019:

Removed the comment.

in src/wallet/scriptpubkeyman.h:181 in 365879e9bd outdated

176+    virtual void MarkUnusedAddresses(const CScript& script) {}
177+
178+    //! Upgrade stored CKeyMetadata objects to store key origin info as KeyOriginInfo
179+    virtual void UpgradeKeyMetadata() {}
180+
181+    /** Sets up the key generation stuff.

instagibbs commented at 3:37 pm on September 9, 2019:

please elaborate on stuff :P

it’s important for spotting missing functionality since I don’t think there’s an equiv block in old wallet code

instagibbs commented at 3:38 pm on September 9, 2019:

seems a bit of a misnomer even based on my reading

achow101 commented at 5:37 pm on September 9, 2019:

The equivalent blocks in old wallet code are the various SetHDSeed(GenerateNewSeed()) lines scattered around in a bunch of places.

How is it a misnomer?

achow101 commented at 6:00 pm on September 9, 2019:

I updated the comment.

in src/wallet/scriptpubkeyman.cpp:411 in c83bb3c7ad outdated

203+        // generate a new master key
204+        SetHDSeed(GenerateNewSeed());
205+        hd_upgrade = true;
206+    }
207+    // Upgrade to HD chain split if necessary
208+    if (CanSupportFeature(FEATURE_HD_SPLIT) && hdChain.nVersion < 2 /* VERSION_HD_CHAIN_SPLIT */) {

instagibbs commented at 3:41 pm on September 9, 2019:

any reason you’re not actually referencing VERSION_HD_CHAIN_SPLIT directly?

instagibbs commented at 3:42 pm on September 9, 2019:

and why the logic change here? master doesn’t have this version check

achow101 commented at 5:44 pm on September 9, 2019:

Referencing VERSION_HD_CHAIN_SPLIT directly requires including walletdb.h which is a circular dependency.

The logic is slightly different here because without it, it would end up marking the newly generated keys as pre-split on later runs on the same LegacyScriptPubKeyMan. This is an issue because for a LegacyScriptPubKeyMan, Upgrade() will be called 6 times (once for each address type and internalness). If you were to run -upgradewallet multiple times on master, you would see the same problem.

in src/wallet/scriptpubkeyman.cpp:436 in 365879e9bd outdated

400+{
401+    LOCK(cs_KeyStore);
402+    return !mapKeys.empty() || !mapCryptedKeys.empty();
403+}
404+
405+void LegacyScriptPubKeyMan::RewriteDB()

instagibbs commented at 3:45 pm on September 9, 2019:

this has nothing to do with the DB, other than it’s called right after database->Rewrite("\x04pool") by the caller.

Please rename it to something sensible e.g., ClearKeypool

achow101 commented at 5:46 pm on September 9, 2019:

It’s supposed to be post-processing that is done by ScriptPubKeyMan when the db had to be rewritten, hence the name. Other ScriptPubKeyMans may do something different.

in src/wallet/scriptpubkeyman.cpp:638 in b5cb8f59ca outdated

426@@ -429,6 +427,36 @@ bool LegacyScriptPubKeyMan::AddKeyPubKeyInner(const CKey& key, const CPubKey &pu
427     return true;
428 }
429 
430+bool LegacyScriptPubKeyMan::Encrypt(CKeyingMaterial& vMasterKeyIn, WalletBatch* batch)
431+{
432+    LOCK(cs_KeyStore);
433+    encrypted_batch = batch;

instagibbs commented at 3:52 pm on September 9, 2019:

can you explain the diff in code from master with the batch pointer being set in this function, when it’s set to null, etc?

achow101 commented at 5:50 pm on September 9, 2019:

encrypted_batch used to be set from within EncryptWallet before EncryptKeys was called. Since EncryptKeys is now ScriptPubKeyMan::Encrypt, it doesn’t have access to CWallet’s encrypted_batch, so we need to pass it in and set it when the ScriptPubKeyMan is encrypting.

in src/wallet/scriptpubkeyman.cpp:1427 in c02e6daf18 outdated

1236+    for (const CScript& script : script_pub_keys) {
1237+        if (!have_solving_data || !IsMine(script)) { // Always call AddWatchOnly for non-solvable watch-only, so that watch timestamp gets updated
1238+            if (!AddWatchOnlyWithDB(batch, script, timestamp)) {
1239+                return false;
1240+            }
1241+        }

instagibbs commented at 3:58 pm on September 9, 2019:

Can you explain where the SetAddressBookWithDB call went for reviewers?

achow101 commented at 5:22 pm on September 9, 2019:

It remains in wallet.cpp since the address book isn’t part of ScriptPubKeyMan.

in src/wallet/scriptpubkeyman.h:211 in 365879e9bd outdated

203+    virtual size_t KeypoolCountExternalKeys() { return 0; }
204+    virtual unsigned int GetKeypoolSize() const { return 0; }
205+
206+    virtual int64_t GetTimeFirstKey() const { return 0; }
207+
208+    virtual const CKeyMetadata* GetMetadata(uint160 id) const { return nullptr; }

instagibbs commented at 4:02 pm on September 9, 2019:

Brief comment on what this does? (I don’t think similar existed before in CWallet)

achow101 commented at 5:53 pm on September 9, 2019:

It just returns a CKeyMetadata as the function name suggests.

It is needed so that getting metadata no longer requires direct access to mapKeyMetadata which no longer exists in CWallet and is not guaranteed to exist in other ScriptPubKeyMans (e.g. it is generated on the fly in native descriptor wallets).

in src/script/signingprovider.h:75 in 6806ad09a1 outdated

71@@ -74,6 +72,8 @@ class FillableSigningProvider : public SigningProvider
72     void ImplicitlyLearnRelatedKeyScripts(const CPubKey& pubkey) EXCLUSIVE_LOCKS_REQUIRED(cs_KeyStore);
73 
74 public:
75+    mutable CCriticalSection cs_KeyStore;

instagibbs commented at 4:05 pm on September 9, 2019:

why this move? is this preparatory?

achow101 commented at 5:54 pm on September 9, 2019:

Yes. cs_KeyStore sometimes needs to be locked by the caller.

in src/wallet/walletdb.cpp:314 in 6806ad09a1 outdated

311@@ -310,6 +312,11 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
312                 strErr = "Error reading wallet database: LoadKey failed";
313                 return false;
314             }
315+            if (!pwallet->GetLegacyScriptPubKeyMan()->LoadKey(key, vchPubKey))
316+            {
317+                strErr = "Error reading wallet database: LegacyScriptPubKeyMan::LoadKey failed";
318+                return false;
319+            }
320         } else if (strType == DBKeys::MASTER_KEY) {

instagibbs commented at 4:26 pm on September 9, 2019:

Maybe leave a comment in this block that the pubkeyman has no use for this key, it’s just an encryption key. I was tripped up on this. Bad naming! bad!

achow101 commented at 6:02 pm on September 9, 2019:

Done

in src/wallet/scriptpubkeyman.cpp:502 in 6c7c36276e outdated

295@@ -296,7 +296,22 @@ std::unique_ptr<SigningProvider> LegacyScriptPubKeyMan::GetSigningProvider(const
296 
297 bool LegacyScriptPubKeyMan::CanProvide(const CScript& script, SignatureData& sigdata)

instagibbs commented at 4:28 pm on September 9, 2019:

please explain in header what it CanProvide

achow101 commented at 6:02 pm on September 9, 2019:

Done

in src/wallet/scriptpubkeyman.cpp:482 in 6c7c36276e outdated

302+    } else if (HaveCScript(CScriptID(script))) {
303+        return true;
304+    } else {
305+        ProduceSignature(*this, DUMMY_SIGNATURE_CREATOR, script, sigdata);
306+        if (!sigdata.signatures.empty()) {
307+            // If we could make signatures, make sure we have the private key

instagibbs commented at 4:30 pm on September 9, 2019:

why?

achow101 commented at 5:26 pm on September 9, 2019:

It could be watch only (as DUMMY_SIGNATURE_CREATOR works for watch only things), which then causes issues elsewhere when a private key is assumed to exist but doesn’t for watch only things. So this prevents those code paths being taken for watch only things.

Sjors commented at 2:11 pm on September 17, 2019:

Should this say “we have any private key” instead of the private key? “DUMMY_SIGNATURE_CREATOR works for watch only things” is useful to add as a comment. Can you also explain under what circumstance DUMMY_SIGNATURE_CREATOR does not produce a signature? It’s also useful to reiterate here that ProduceSignature doesn’t necessarily produce an ECDSA signature, but a script signature, which in turn may contain a (dummy?) ECDSA signature.

achow101 commented at 6:20 pm on September 17, 2019:

I’m not writing an essay. It is completely unnecessary to describe what DUMMY_SIGNATURE_CREATOR does here as you can just look at the sign.cpp/h where it is defined and read the comments and code there yourself. It’s less than 10 lines.

instagibbs commented at 4:32 pm on September 9, 2019: member

reviewed through 6c7c36276e26539edc8b281129bb30f1cd4a4fb8 “Implement CanProvide in LegacyScriptPubKeyMan”

in src/wallet/wallet.cpp:3093 in 6a91646790 outdated

3716-    CPubKey new_key;
3717-    if (!GetKeyFromPool(new_key)) {
3718-        error = "Error: Keypool ran out, please call keypoolrefill first";
3719-        return false;
3720+    bool result = false;
3721+    auto spk_man = GetScriptPubKeyMan(type, false);

instagibbs commented at 5:14 pm on September 9, 2019:

nit: annotate the bool arg, or make it an enum

achow101 commented at 11:34 pm on September 9, 2019:

Done

in src/wallet/wallet.cpp:3283 in 6a91646790 outdated

3921+    spk_man = pwallet->GetScriptPubKeyMan(type, internal);
3922+    if (!spk_man) {
3923+        return false;
3924+    }
3925+
3926+    if (!spk_man->IsLocked()) {

instagibbs commented at 5:18 pm on September 9, 2019:

no need to gate here, TopUp already checks for this in TopUpKeyPool

achow101 commented at 11:34 pm on September 9, 2019:

Done

in src/wallet/wallet.cpp:3287 in 6a91646790 outdated

3925+
3926+    if (!spk_man->IsLocked()) {
3927+        spk_man->TopUp();
3928+    }
3929+
3930+    if (!spk_man->CanGetAddresses(internal)) {

instagibbs commented at 5:19 pm on September 9, 2019:

LegacyScriptPubKeyMan::GetReservedDestination does this check as well

achow101 commented at 11:35 pm on September 9, 2019:

Done

in src/wallet/wallet.cpp:3922 in 6a91646790 outdated

3940         }
3941-        vchPubKey = keypool.vchPubKey;
3942         fInternal = keypool.fInternal;
3943     }
3944-    assert(vchPubKey.IsValid());
3945-    pwallet->LearnRelatedScripts(vchPubKey, type);

instagibbs commented at 5:24 pm on September 9, 2019:

not immediately obvious to me how this gets accomplished in new flow? GetNewDestination case is covered, just not this one?

achow101 commented at 11:24 pm on September 9, 2019:

It’s handled internally by spk_man in the same way that GetNewDestination does.

instagibbs commented at 1:22 pm on September 10, 2019:

Please be specific. GetNewDestination literally has the call to LearnRelatedScripts, I can’t find the corresponding one for this.

achow101 commented at 3:55 pm on September 10, 2019:

I’ve added a call to LearnRelatedScripts in GetReservedDestination

in src/wallet/wallet.cpp:1385 in c78b003846 outdated

1766+        return false;
1767     }
1768-
1769-    return true;
1770+    LOCK(spk_man->cs_KeyStore);
1771+    return GetLegacyScriptPubKeyMan()->ImportScripts(scripts, timestamp);

instagibbs commented at 5:37 pm on September 9, 2019:

just use spk_man here (if you cannot, please annotate why in comments)

achow101 commented at 11:35 pm on September 9, 2019:

Done

in src/wallet/wallet.cpp:1395 in c78b003846 outdated

1794+    if (!spk_man) {
1795+        return false;
1796     }
1797-    return true;
1798+    LOCK(spk_man->cs_KeyStore);
1799+    return GetLegacyScriptPubKeyMan()->ImportPrivKeys(privkey_map, timestamp);

instagibbs commented at 5:38 pm on September 9, 2019:

just use spk_man here (if you cannot, please annotate why in comments)

achow101 commented at 11:35 pm on September 9, 2019:

Done

in src/wallet/wallet.cpp:1405 in c78b003846 outdated

1831+    if (!spk_man) {
1832+        return false;
1833     }
1834-    return true;
1835+    LOCK(spk_man->cs_KeyStore);
1836+    return GetLegacyScriptPubKeyMan()->ImportPubKeys(ordered_pubkeys, pubkey_map, key_origins, add_keypool, internal, timestamp);

instagibbs commented at 5:38 pm on September 9, 2019:

just use spk_man here (if you cannot, please annotate why in comments)

achow101 commented at 11:35 pm on September 9, 2019:

Done

in src/wallet/wallet.cpp:1415 in c78b003846 outdated

1841+    auto spk_man = GetLegacyScriptPubKeyMan();
1842+    if (!spk_man) {
1843+        return false;
1844+    }
1845+    LOCK(spk_man->cs_KeyStore);
1846+    if (!GetLegacyScriptPubKeyMan()->ImportScriptPubKeys(script_pub_keys, have_solving_data, timestamp)) {

instagibbs commented at 5:39 pm on September 9, 2019:

just use spk_man here (if you cannot, please annotate why in comments)

achow101 commented at 11:35 pm on September 9, 2019:

Done

achow101 force-pushed on Sep 9, 2019

in src/wallet/wallet.cpp:3052 in f4ff4c5f1a outdated

3540-        WalletBatch batch(*database);
3541-        for (int64_t i = missingInternal + missingExternal; i--;)
3542-        {
3543-            if (i < missingInternal) {
3544-                internal = true;
3545+    bool res = true;

instagibbs commented at 6:03 pm on September 9, 2019:

why not just iterate over GetActiveScriptPubKeyMans result, should be more complete and less brittle to updates?

achow101 commented at 11:35 pm on September 9, 2019:

Done

in src/wallet/wallet.cpp:4266 in 3e6cd81d3d outdated

4287-        if (hd_upgrade) {
4288-            if (!walletInstance->TopUpKeyPool()) {
4289-                chain.initError(_("Unable to generate keys").translated);
4290-                return nullptr;
4291+
4292+        for (bool internal : {false, true}) {

instagibbs commented at 6:08 pm on September 9, 2019:

GetActiveScriptPubKeyMans?

achow101 commented at 11:35 pm on September 9, 2019:

Done

in src/wallet/wallet.cpp:4453 in e3b5a71ae0 outdated

4448@@ -4449,8 +4449,17 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,
4449 
4450         // No need to read and scan block if block was created before
4451         // our wallet birthday (as adjusted for block time variability)
4452-        if (walletInstance->nTimeFirstKey) {
4453-            if (Optional<int> first_block = locked_chain->findFirstBlockWithTimeAndHeight(walletInstance->nTimeFirstKey - TIMESTAMP_WINDOW, rescan_height, nullptr)) {
4454+        int64_t time_first_key = std::numeric_limits<int64_t>::max();
4455+        for (bool internal : {false, true}) {

instagibbs commented at 6:11 pm on September 9, 2019:

GetActiveScriptPubKeyMans?

achow101 commented at 11:35 pm on September 9, 2019:

Done

in src/wallet/wallet.cpp:219 in 595fb67238 outdated

213@@ -214,9 +214,17 @@ WalletCreationStatus CreateWallet(interfaces::Chain& chain, const SecureString&
214             }
215 
216             // Set a seed for the wallet
217-            CPubKey master_pub_key = wallet->GenerateNewSeed();
218-            wallet->SetHDSeed(master_pub_key);
219-            wallet->NewKeyPool();
220+            {
221+                LOCK(wallet->cs_wallet);
222+                for (bool internal : {false, true}) {

instagibbs commented at 6:16 pm on September 9, 2019:

GetActiveScriptPubKeyMans?

achow101 commented at 11:35 pm on September 9, 2019:

Done

in src/outputtype.cpp:81 in 592177c771 outdated

77@@ -78,22 +78,30 @@ CTxDestination AddAndGetDestinationForScript(FillableSigningProvider& keystore,
78 {
79     // Add script to keystore
80     keystore.AddCScript(script);
81+    ScriptHash sh = ScriptHash(script);

instagibbs commented at 6:18 pm on September 9, 2019:

comment for this whole commit: why is this here? I don’t see direct connection with anything. Could this be moved earlier in PR or split out?

achow101 commented at 11:56 pm on September 9, 2019:

it was needed for some tests to pass. I’ve moved it up near the front of the PR.

Sjors commented at 3:11 pm on September 17, 2019:

I think you can drop [ci-skip]; tests pass for me locally on this commit.

It’s still not clear to me what this is doing. The function already calls keystore.AddCScript, so why do this twice?

achow101 commented at 6:18 pm on September 17, 2019:

It’s still not clear to me what this is doing.

Nor is it to me. It makes the tests pass. That’s. it.

instagibbs commented at 8:30 pm on October 11, 2019:

micro-nit: ScriptHash sh(script);

achow101 commented at 9:47 pm on October 11, 2019:

Done

Sjors commented at 6:16 pm on February 11, 2020:

Now we know :-) #18067

ryanofsky commented at 6:37 pm on February 11, 2020:

re: #16341 (review)

Wow, missed this thread. Reviewers: “I don’t know what this this doing.” Author: “Me either, but it makes tests pass.” Reviewers: “ACK” “ACK”

You guys! :heart:

Sjors commented at 7:21 pm on February 11, 2020:

There was a bit more back and forth, but yeah… :-(

instagibbs commented at 6:21 pm on September 9, 2019: member

Briefly reviewed the rest of the commits as well for revision 365879e9bdba93a84bd2ef4a39816a5eb8fd31b0

One concern I have that I’d have to take a harder look at is that some places seem to eagerly try whatever scriptpubkeyman are loaded, and some simply load the Legacy one by name. I’ll more carefully go through the exposed RPC interface later.

achow101 force-pushed on Sep 9, 2019

instagibbs commented at 1:51 pm on September 10, 2019: member

tentative utACK https://github.com/bitcoin/bitcoin/pull/16341/commits/1ccf487912fd6abb2a289f4933eebd06ad4e5116 if https://github.com/bitcoin/bitcoin/pull/16341/files#r322739005 is answered

The RPC-based changes seem correct as well. Any import/dump type functionality is locked in to legacy box, and as new boxes are created we can either activate those, or make new RPC calls. For more general info-gathering-on-keys based queries, any box will do, which is what the implementation does.

achow101 force-pushed on Sep 10, 2019

instagibbs commented at 4:08 pm on September 10, 2019: member

re-utACK fb0058ed68a18053959001a0191f3fbcfec50526 :)

sidhujag referenced this in commit 9282d8beb9 on Sep 10, 2019

in src/wallet/scriptpubkeyman.h:244 in fb0058ed68 outdated

241+    //! keeps track of whether Unlock has run a thorough check before
242+    bool fDecryptionThoroughlyChecked = false;
243+
244+    //! Master encryption key for the wallet. Keys are encrypted with this.
245+    //! Wallet is unlocked if this is not null.
246+    CKeyingMaterial vMasterKey GUARDED_BY(cs_KeyStore);

ryanofsky commented at 6:21 pm on September 12, 2019:

The state variables vMasterKey, fUseCrypto, and fDecryptionThoroughlyChecked seem to end up duplicated in both the CWallet and in KeyMan classes:

https://github.com/bitcoin/bitcoin/blob/fb0058ed68a18053959001a0191f3fbcfec50526/src/wallet/wallet.h#L591 https://github.com/bitcoin/bitcoin/blob/fb0058ed68a18053959001a0191f3fbcfec50526/src/wallet/scriptpubkeyman.h#L246

Is this by design, or could KeyMan instances store references or pointers instead? It seems not ideal for different KeyMan instances to potentially be storing their own copies of encryption state and maybe having variables like fDecryptionThoroughlyChecked set inconsistently.

Also, the SetCrypted and IsLocked functions are duplicated almost but not quite verbatim across both classes:

https://github.com/bitcoin/bitcoin/blob/fb0058ed68a18053959001a0191f3fbcfec50526/src/wallet/wallet.cpp#L3996-L4012 https://github.com/bitcoin/bitcoin/blob/fb0058ed68a18053959001a0191f3fbcfec50526/src/wallet/scriptpubkeyman.cpp#L213-L231

I don’t know if any of this is harmful, but it does seem more messy than it needs to be. What would you think about introducing an interface like:

 0class WalletStorage
 1{
 2    virtual ~WalletStorage() = default;
 3    virtual const std::string& GetDisplayName() = 0;
 4    virtual WalletDatabase& GetDatabase() = 0;
 5    virtual const CKeyingMaterial& GetMasterKey() = 0;
 6    virtual void IsWalletFlagSet(uint64_t)> = 0;
 7    virtual void SetWalletFlag(WalletBatch&, uint64_t)> = 0;
 8    virtual bool UnsetWalletFlagWithDB(uint64_t) = 0;
 9    virtual bool CanSupportFeature(enum WalletFeature) = 0;
10    virtual void SetMinVersion(enum WalletFeature) = 0;
11};

that would allow ScriptPubKeyMan to get access to database and crypto functionality without creating a circular dependency on CWallet, and without needing the current std::function trampolines and duplication of crypto code.

The idea would be to declare this interface in scriptpubkeyman.h or a standalone header, pass it to scriptpubkeymanager instances, and have CWallet inherit and implement it.

achow101 commented at 9:08 pm on September 13, 2019:

I’ve added WalletStorage as you suggested. However I did not change vMasterKey, fUseCrypto, and fDecryptionThorouglyChecked to be references or pointers as allowing ScriptPubKeyMans to modify these variables in CWallet itself may result in inconsistent states where one ScriptPubKeyMan has finished en/decrypting but another has not. Properly changing them to use references could be done in the future, but doing it now would cause the places that use them to differ even more and thus make review harder.

Sjors commented at 4:54 pm on September 17, 2019:

It makes sense to me to have WalletStorage only cover stuff that’s global to the wallet, rather than unique to each ScriptPubKeyMan. The end result is that this commit is now manageably small.

However, at the PR level, shouldn’t CWallet’s IsCrypted() be looping over all ScriptPubKeyMans, just like SetCrypted() does?

update: IsCrypted() doesn’t loop, I was looking at CWallet::EncryptWallet

achow101 commented at 5:46 pm on September 17, 2019:

However, at the PR level, shouldn’t CWallet’s IsCrypted() be looping over all ScriptPubKeyMans, just like SetCrypted() does?

SetCrypted doesn’t loop.

The thought I had here was that it could be possible that a wallet is considered encrypted (by virtue of having an encryption key set) without any of the ScriptPubKeyMans having any of them actually encrypting anything and thus be IsCrypted() == false.

ryanofsky commented at 3:46 pm on September 20, 2019:

The encryption stuff here is making my head spin. Wallet encryption was already confusing before, but now with CWallet and ScriptPubKeyMan both having duplicate fUseCrypto, SetCrypted(), IsCrypted(), vMasterKey, IsLocked() states and code, it’s hard to tell which combinations of state are expected, and which are bugs, and whether the code supposed to be keeping states in sync actually works.

I have a few suggestions to try clarify things. These should also simplify the code.

Drop the CWallet::fUseCrypto member and CWallet::SetCrypted() method. Change CWallet::IsCrypted() method to just loop over keyman objects and call IsCrypted on them. As far as I can tell the CWallet::fUseCrypto state is broken in the current PR. CWallet::SetCrypted() used to be called whenever any encrypted data was present, but now that now longer happens, and CWallet::IsCrypted() returns false if the wallet hasn’t been explicitly encrypted or locked since startup, regardless of what encrypted keys or data are present.
If the intent is to consider a wallet encrypted “by virtue of having an encryption key set”, it’d be better to add a new method bool HasEncryptionKey() { return !mapCryptedKeys.empty(); } instead of overloading IsCrypted(). It might also make sense to rename the existing IsCrypted() function HasEncryptedData() in that case so the meaning is not confused.
This last suggestion is less important, but I think it’d be better to lock and unlock the entire wallet, instead of parts of it. The keyman vMasterKey member and IsLocked(), Locked(), and Unlocked() implementations copied from wallet code could all be dropped. Keyman instances should have a fDecryptionThoroughlyChecked member and have CheckDecryption() methods, but otherwise would be simpler if they just called IsLocked() and GetMasterKey() from WalletStorage instead of implementing locking and unlocking themselves.

achow101 commented at 4:42 pm on September 20, 2019:

I’ll take a look at reworking the encryption state stuff.

achow101 commented at 4:10 pm on September 23, 2019:

I’ve changed the encryption stuff to be mostly in line with @ryanofsky’s suggestions.

As far as I can tell the CWallet::fUseCrypto state is broken in the current PR. CWallet::SetCrypted() used to be called whenever any encrypted data was present, but now that now longer happens, and CWallet::IsCrypted() returns false if the wallet hasn’t been explicitly encrypted or locked since startup, regardless of what encrypted keys or data are present.

FWIW it wasn’t broken. SetCrypted was changed to be called during wallet loading so fUseCrypto would be true and thus IsCrypted() returned true. Otherwise tests wouldn’t pass.

achow101 force-pushed on Sep 13, 2019

in src/wallet/scriptpubkeyman.h:460 in 5f641a5c8f outdated

460+     */
461+    void MarkReserveKeysAsUsed(int64_t keypool_id) EXCLUSIVE_LOCKS_REQUIRED(cs_KeyStore);
462+    const std::map<CKeyID, int64_t>& GetAllReserveKeys() const { return m_pool_key_to_index; }
463+};
464+
465+/** Wraps a LegacyScriptPubKeyMan so that it can be returned in a new unique_ptr */

ryanofsky commented at 6:29 pm on September 16, 2019:

What’s the reason for using unique_ptr and shared_ptr so many places to begin with? It doesn’t seem like we’d ever want KeyMan instances to outlive their CWallet instance, or to be shared between multiple CWallet instances. So wouldn’t it be simplest to store KeyMan instances in CWallet unique_ptr<> or set<unique_ptr<>> member variables so they get cleaned up when the wallet does, but otherwise just use plain & references or * pointers everywhere else (address types maps, accessors, and higher level code)?

achow101 commented at 8:43 pm on September 16, 2019:

The use of shared_ptr was because ScriptPubKeyMan objects were being passed around a lot. I’ve changed this to store them as a unique_ptr and instead just pass around the ScriptPubKeyMan* as you suggested.

ryanofsky commented at 9:40 pm on September 16, 2019:

Thanks! If desired, I think you could take it further and drop LegacySigningProvider wrapper class if the CWallet::GetSigningProvider methods are returning plain pointers. Also, I think there’s no need anymore to change CWallet::database from a unique_ptr to a shared_ptr. (It’s a little nicer as a unique_ptr to be sure the database is closed when a wallet’s unloaded and not kept open with a dangling reference.)

Sjors commented at 4:07 pm on September 17, 2019:

I think LegacySigningProvider class adds some clarity.

achow101 force-pushed on Sep 16, 2019

in src/wallet/scriptpubkeyman.cpp:503 in 803e3250f6 outdated

297@@ -298,7 +298,22 @@ std::unique_ptr<SigningProvider> LegacyScriptPubKeyMan::GetSigningProvider(const
298 
299 bool LegacyScriptPubKeyMan::CanProvide(const CScript& script, SignatureData& sigdata)
300 {

Sjors commented at 2:15 pm on September 17, 2019:

What’s the equivalent pre-box code for this? If nothing, can you explain in a comment why you need these three distinct checks?

achow101 commented at 5:51 pm on September 17, 2019:

Of course there is no pre-box equivalent, CWallet was just passed in as the SiginingProvider always!

Sjors commented at 7:22 am on September 18, 2019:

This function has no tests, half a dozen different code paths and calls really confusing legacy code like IsMine. There’s probably less than 5 people on the planet who understand it, and even fewer who understand the ways this can break for some weird use cases. The only excuse for not documenting it is if it’s copied from a legacy function.

achow101 commented at 4:44 pm on September 18, 2019:

I’ve added some comments.

in src/wallet/scriptpubkeyman.h:166 in 1e572c14d8 outdated

25+class ScriptPubKeyMan
26+{
27+public:
28+    virtual ~ScriptPubKeyMan() {};
29+    virtual bool GetNewDestination(const OutputType type, CTxDestination& dest, std::string& error) { return false; }
30+    virtual isminetype IsMine(const CScript& script) const { return ISMINE_NO; }

Sjors commented at 3:24 pm on September 17, 2019:

I suppose we can’t burry IsMine deep inside a private method? :-)

achow101 commented at 5:52 pm on September 17, 2019:

No

in src/wallet/scriptpubkeyman.h:197 in 1e572c14d8 outdated

59+
60+    /* Returns true if the wallet can give out new addresses. This means it has keys in the keypool or can generate new keys */
61+    virtual bool CanGetAddresses(bool internal = false) { return false; }
62+
63+    /** Upgrades the wallet to the specified version */
64+    virtual bool Upgrade(int prev_version, int new_version, std::string& error) { return false; }

Sjors commented at 3:27 pm on September 17, 2019:

Nit: move closer to UpgradeKeyMetadata

achow101 commented at 5:52 pm on September 17, 2019:

No

in src/wallet/scriptpubkeyman.h:229 in 1e572c14d8 outdated

85+    virtual bool CanProvide(const CScript& script, SignatureData& sigdata) { return false; }
86+
87+    virtual uint256 GetID() const { return uint256(); }
88+
89+    /** Watch-only address added */
90+    boost::signals2::signal<void (bool fHaveWatchOnly)> NotifyWatchonlyChanged;

Sjors commented at 3:29 pm on September 17, 2019:

Shouldn’t this be part of the legacy box?

achow101 commented at 6:12 pm on September 17, 2019:

It makes everything else more complicated to do so.

in src/wallet/scriptpubkeyman.h:24 in 957ac1c094 outdated

14 
15 #include <boost/signals2/signal.hpp>
16 
17 enum class OutputType;
18 
19+class WalletStorage

Sjors commented at 3:45 pm on September 17, 2019:

Why does this live in scriptpubkeyman.h rather than wallet.h? Can you add a few comment lines to explain the purpose of WalletStorage and how it relates to CWallet and ScriptPubKeyMan?

In particular I don’t get why (in the next commit) each ScriptPubKeyMan instance needs this.

Maybe move this commit closer to where you first need it.

achow101 commented at 5:53 pm on September 17, 2019:

To avoid circular dependencies.

I am not moving this commit. It belongs right there.

It serves the exact same purpose as the commits it replaces does, to provide access to some functions that require reading and/or modifying some wallet storage things like the database handle, the wallet version number, and wallet flags. It’s there to avoid having callbacks and binding functions.

in src/wallet/wallet.cpp:4118 in 04aba6b8af outdated

4970+    ScriptPubKeyMan* spk_man = m_internal_spk_managers.at(OutputType::LEGACY);
4971+    for (const auto& type : OUTPUT_TYPES) {
4972+        assert(m_internal_spk_managers.at(type) == spk_man);
4973+        assert(m_external_spk_managers.at(type) == spk_man);
4974+    }
4975+    assert(m_spk_managers.size() == 1);

Sjors commented at 3:55 pm on September 17, 2019:

Maybe point out that having only 1 entry in m_spk_managers here means that functions like GetSigningProvider that iterate over “all” PubKeyMans really only query once.

achow101 commented at 5:55 pm on September 17, 2019:

I don’t think that’s necessary to point out.

in src/wallet/wallet.cpp:4020 in 15a633b8dc outdated

4947@@ -4948,6 +4948,58 @@ bool CWallet::AddCryptedKeyInner(const CPubKey &vchPubKey, const std::vector<uns
4948     return true;
4949 }
4950 
4951+std::set<ScriptPubKeyMan*> CWallet::GetActiveScriptPubKeyMans() const

Sjors commented at 4:00 pm on September 17, 2019:

How is this different from m_spk_managers?

achow101 commented at 5:56 pm on September 17, 2019:

m_spk_managers can have inactive ScriptPubKeyMans.

in src/wallet/wallet.h:1102 in 15a633b8dc outdated

1374@@ -1375,6 +1375,16 @@ class CWallet final : public FillableSigningProvider, public WalletStorage, priv
1375     /** Implement lookup of key origin information through wallet key metadata. */
1376     bool GetKeyOrigin(const CKeyID& keyid, KeyOriginInfo& info) const override;
1377 
1378+    std::set<ScriptPubKeyMan*> GetActiveScriptPubKeyMans() const;

Sjors commented at 4:01 pm on September 17, 2019:

Maybe clarify the meaning of “Active”, and how this is different from m_spk_managers. Presumably the latter may, in a descriptor wallet, include PubKeyMan that we don’t draw new address for.

Maybe rename commit to Introduce GetScriptPubKeyMan methods.

achow101 commented at 5:56 pm on September 17, 2019:

Active means that we actively draw addresses from them, i.e. they are in m_internal_spk_managers and m_external_spk_managers.

in src/wallet/wallet.cpp:4147 in 65b9a1376d outdated

5042@@ -5042,3 +5043,11 @@ void CWallet::SetupLegacyScriptPubKeyMan()
5043     }
5044     assert(m_spk_managers.size() == 1);
5045 }
5046+
5047+void CWallet::ConnectScriptPubKeyManNotifiers()
5048+{
5049+    for (const auto& spk_man_pair : m_spk_managers) {
5050+        spk_man_pair.second->NotifyWatchonlyChanged.connect(NotifyWatchonlyChanged);
5051+        spk_man_pair.second->NotifyCanGetAddressesChanged.connect(NotifyCanGetAddressesChanged);

Sjors commented at 4:51 pm on September 17, 2019:

Shouldn’t this only loop over active ScriptPubKeyMans?

achow101 commented at 5:14 pm on September 18, 2019:

Yes. Changed

Sjors commented at 5:02 pm on September 17, 2019: member

Review in progress, borrowing @ryanofsky’s fancy tools (see my gist):

eb428d89d09e12b9528518f6a73daeeec4845d12 Move wallet enums to walletutil.h (1/70)
4aade7bceeb5092d6a322daec61c1af07ed9775e List output types in an array in order to be iterated over (2/70)
d8abfae4e93b3023d1d62b4dd10f109eb55f56bd Always try to sign for all pubkeys in multisig (3/70)
9899064017a2864ae0b8eb41c3c7e69822752129 Store p2sh scripts in AddAndGetDestinationForScript (4/70)
fbb6d0142ac98a315fc230d186a4a7b673f5f5cb Introduce both ScriptPubKeyMan as an interface and LegacyScriptPubKeyMan as a dummy class (5/70)
86c2a670d6e169f2b5058946818331d354ed4f3a Add LegacyScriptPubKeyMan to CWallet (6/70)
85b38c74d7642a65af3f447322c31fd77d36780b Add WalletStorage class and have CWallet inherit it (7/70)
d6b6e0bab5552916dcd166e60c5b02d31ba853e7 Add WalletStorage to ScriptPubKeyMan and passthrough some functions to it (8/70)
a8e7461254b5964fc7452e5ceabfbcd303d9fbfb Fetch the SigningProvider for a script from the wallet (9/70)
1127cf6c45c860950a02f85d964b98feb03b1abe Fetch the ScriptPubKeyMan for given output type and internal-ness, or a given script, or ScriptPubKeyMan id (10/70)
501c0830a4bd6bfe3949482170f57d6e137f931c Implement GetSigningProvider in LegacyScriptPubKeyMan (11/70)
e423df81761a83da3f55723f616f90705fe188b5 Implement function to connect ScriptPubKeyMan’s NotifyCanGetAddessesChanged and NotifyWatchOnlyChanged to CWallet’s (12/70)
7af540932d3e405c3886a382d66bda0cac443585 Implement IsLocked and IsCrypted in LegacyScriptPubKeyMan (13/70)
07b616b4b04dc29936cdf5fb87c5b3201bc01445 Implement LoadCryptedKey and AddCryptedKey in LegacyScriptPubKeyMan (14/70)
1c21d4e2ce2c698b9ca06c7c428d7300d33184ab Implement UpdateTimeFirstKey, and GetTimeFirstKey in LegacyScriptPubKeyMan (15/70)
17d32d65c125078063cccf58b72cb6041e6b6828 Implement AddWatchOnly, RemoveWatchOnly, HaveWatchOnly, and LoadWatchOnly in LegacyScriptPubKeyMan (16/70)
7b83f49531be8e22c45b107bcdad879ecd9a834d Implement AddKeyPubKey and LoadKey in LegacyScriptPubKeyMan (17/70)
276adc1ebc3d185d14cfc12abc4f057aaa4f164d Implement WalletLogPrintf in LegacyScriptPubKeyMan (18/70)
6ecc20b2d98f02ad4218f3924c7de88f375fa983 Implement SetHDCHain, and IsHDEnabled in LegacyScriptPubKeyMan (19/70)
8ccc29a594b34c0b6447e58b0732ee9f0840df4c Implement LoadCScript in LegacyScriptPubKeyMan (20/70)
540e60b06265fa3ea52181256ee729ce7a50c3fe Implement LoadKeyMetadata and LoadScriptMetadata in LegacyScriptPubKeyMan (21/70)
251201acb529fc5de68efa13d01b6e5882d2776f Implement GetKey, HaveKey, and GetPubKey in LegacyScriptPubKeyMan (22/70)
2760f2160c2ea437a8c4c67dbb435577cd5e956a Implement GenerateNewKey in LegacyScriptPubKeyMan (23/70)
ce1ed8df731f5770f34a8579a05fa7c5a8effb72 Implement LoadKeyPool in LegacyScriptPubKeyMan (24/70)
ab15835a6bc41946e9821ae68b465fab6bec4a00 Implement GetOldestKeyPoolTime, KeypoolCountExternalKeys, and GetKeypoolSize in LegacyScriptPubKeyMan (25/70)
add7cbd861d29f492d2796325c51ddc8f6e34d5f Implement CanGetAddresses, CanGenerateKeys, and HavePrivateKeys in LegacyScriptPubKeyMan (26/70)
dda4bffd91d569a1a0ddac46d93a99db819e5034 Implement GenerateNewSeed, DeriveNewSeed, and SetHDSeed for LegacyScriptPubKeyMan (27/70)
b8af54b943ff52b259571fd458ed63d20b5a2e95 Implement TopUpKeypool, TopUp, and NewKeyPool in LegacyScriptPubKeyMan (28/70)
8611d0122a8747ba4570afa3113b4b5cca92d721 Implement ReturnAddress, and KeepKey in LegacyScriptPubKeyMan (29/70)
ae9354b7694056cfecc8517e8657f811bf0c7842 Implement GetNewAddress, and GetReservedAddress in LegacyScriptPubKeyMan (30/70)
2ccffda892e4e3c8d7642b6658e5ace84357714b Implement MarkUnusedAddresses in LegacyScriptPubKeyMan (31/70)
5b6f5b926457e2423d6a4fcf0b52e0b454a20676 Implement IsMine in LegacyScriptPubKeyMan (32/70)
205502215e1085b7f94f36d4255b9ee833f0f55a Implement UpgradeKeyMetaData, SetupGeneration, IsFirstRun, Upgrade, RewriteDB in LegacyScriptPubKeyMan (33/70)
84470984430c34f0ad51dc3dc56e4c0d27367bf5 Implement Unlock, Lock, and Encrypt and LegacyScriptPubKeyMan (34/70)
5c03c7e5cba3e7abe00a0eca8889a5b14e9c67b6 Implement ImporScripts, ImportPrivKeys, ImportPubKeys, and ImportScriptPubKeys in LegacyScriptPubKeyMan (35/70)
3099b7c2dbfd31f4a1747f7e38629e44988d6ef7 Implement GetMetadata in LegacyScriptPubKeyMan (36/70)
b1efc0af2845bb57714951613dcdfc70ccf63b47 Implement GetKeyOrigin in LegacyScriptPubKeyMan (37/70)
f3739076feb69c99a6d93d6218e3b628d2e4cc57 Implement actually loading everything into LegacyScriptPubKeyMan (38/70)
c2f6d4b40b8c508aa14f27f5e860cdc271a0b6b4 Implement CanProvide in LegacyScriptPubKeyMan (39/70)
842f501ead54f66143d85c5b44d9defb0f1e22d4 [ci skip] Remove CWallet from IsMine and have CWallet always use ScriptPubKeyMan’s IsMine (40/70)
8a2d51c741b896842e093fa35590ef36abb907c4 [ci skip] moveonly: move ismine stuff to be a module of LegacyScriptPubKeyMan (41/70)
2d4c3de17c140cfc84df45fb9cb5d86f184b004a [ci skip] Have GetNewAddress, GetNewChangeAddress, and ReserveAddress use ScriptPubKeyMan (42/70)
73f7f54b1aad82c26240cffaa1ae7afb7cc602ee [ci skip] Mark used addresses in ScriptPubKeyMan (43/70)
99a802afd08fe4535b4a4bc65ff644186f5a0131 [ci skip] Call UpgradeKeyMetaData for each ScriptPubKeyMan (44/70)
8398e91488779baf282d7d692630f3348cb0b3fa [ci skip] Sign using SigningProvider from ScriptPubKeyMan when signing within CWallet (45/70)
d8dd23fdaed2170ed41c8a702620777115483ae7 [ci skip] Do not allow import*, dump*, and addmultisigaddress RPCs when wallet is not backed by LegacyScriptPubKeyMan (46/70)
5bd936494ed2b6afe5e0cc332ae4e74f0066b061 [ci skip] Change Imports to use LegacyScriptPubKeyMan Imports (47/70)
47e43141262c07a39522361628dab30c7d7625bd [ci skip] Use SigningProviders and ScriptPubKeyMans in listunspent, signmessage, signrawtransactionwithwallet, and getaddressinfo (48/70)
46747500672cf985fd015cd8609e7b39aa175306 [ci skip] Use LegacyScriptPubKeyMan in addmultisigaddress and sethdseed (49/70)
4837bc6fcf8e165dd3ee606f46aa5316e007556c [ci skip] Use LegacyScriptPubKeyMan for hdseedid in getwalletinfo (50/70)
12d51a85b65f1f080cc483b273b43dc7596b9a53 [ci skip] Change KeypoolCountExternal and GetKeypoolSize to get aggregate sizes from ScriptPubKeyMans (51/70)
657e8f7df530ac9c5b70e337e4c2bf2ec062c7fc [ci skip] Have IsHDEnabled fetch from ScriptPubKeyMans (52/70)
d3dde67e4aba051bea38daae60ed80fd4db69d3b [ci skip] Fetch oldest keypool time from ScriptPubKeyMans (53/70)
f3b7ab7a6f443acac0bfc253a380a14f8f17646e [ci skip] have TopUpKeyPool call TopUp in each ScriptPubKeyMan (54/70)
2492a85e988f1ac07655754af2f99ea2b45d9bc3 [ci skip] Have EncryptWallet, Lock, and Unlock call their respective functions in ScriptPubKeyMans (55/70)
0dc9eaffa4a9a306d0b97432ad108be39e4c3184 [ci skip] Use LegacyScriptPubKeyMan throughout psbt_wallet_tests (56/70)
0eab7717e4969a25560e776ee06593710ee0395d [ci skip] Use LegacyScriptPubKeyMan throughout wallettool (57/70)
d027cbb056c1bb2057a0be4a8356f85910768838 [ci skip] Use ScriptPubKeyMans’ Setup and Upgrade functions when loading or creating a wallet (58/70)
ef95b21d408b3b279a8a7c3d5f866f1bccf86c63 [ci skip] Define first run as having no ScriptPubKeyMans (59/70)
dcee75f6dc2a26697a56d5741890acace3f81f06 [ci skip] Use RewriteDB action when DB needs rewrite (60/70)
d1c5d16d0b338e8cdb16ec2d14e23a3c678ece6f [ci skip] Use GetTimeFirstKey instead of nTimeFirstKey (61/70)
9822895bc7cb870c356a77bc6b3763cd37409bee [ci skip] Use LegacyScriptPubKeyMan for in wallet_tests (62/70)
cb56b322890273629de253b12ac4083f046112b3 [ci skip] Use LegacyScriptPubKeyMan in dumpprivkey and dumpwallet (63/70)
f505d6c36e153c0560362aa297cd85bcef62c4de [ci skip] Change CanGetAddresses to fetch from ScriptPubKeyMan (64/70)
6f51d198952a4ae8146fd0947110ba03c1a711ae [ci skip] Fetch the correct SigningProvider for signing PSBTs (65/70)
e2575a1f46d98261c85687956875e0c2a7ef0f31 [ci skip] Use LegacyScriptPubKeyMan in test util (66/70)
fcb35328be8ac5183a216229c8e594e51aadea07 [ci skip] Use LegacyScriptPubKeyMan in some parts of getbalances and createwallet (67/70)
1573967b4aeca99357195546f2903437fa393849 [ci skip] Have getPubKey and getPrivKey use SigningProvider (68/70)
0cca3802d911d5526f96ac7325030fe39e70e3e6 [ci skip] Use LegacyScriptPubKeyMan in benchmarks involving the wallet (69/70)
1c01da74d33033b0aef4a30dc305a03594ff7d33 Remove unused functions and switch CWallet to use ScriptPubKeyMan (70/70)

Nit: maybe split scriptpubkeyman_legacy.h out from scriptpubkeyman.h.

in src/wallet/wallet.h:146 in 059824a35f outdated

141@@ -283,10 +142,10 @@ class ReserveDestination
142 protected:
143     //! The wallet to reserve from
144     CWallet* pwallet;
145+    //! The ScriptPubKeyMan to reserve from. Based on type when GetReservedDestination is called
146+    ScriptPubKeyMan* spk_man;

ryanofsky commented at 5:20 pm on September 17, 2019:

Would be good to prefix with m_

achow101 commented at 6:30 pm on September 17, 2019:

Done

achow101 force-pushed on Sep 17, 2019

in src/wallet/scriptpubkeyman.cpp:967 in 0d701bd42d outdated

552@@ -553,3 +553,24 @@ void LegacyScriptPubKeyMan::DeriveNewChildKey(WalletBatch &batch, CKeyMetadata&
553     if (!batch.WriteHDChain(hdChain))
554         throw std::runtime_error(std::string(__func__) + ": Writing HD chain model failed");
555 }
556+

ryanofsky commented at 6:17 pm on September 17, 2019:

In commit “Implement LoadKeyPool in LegacyScriptPubKeyMan” (0d701bd42d0d9db9da858a4e6ae346ae0b1dda90)

Empty src/qt/stqG2SM file in this commit should be dropped

achow101 commented at 6:30 pm on September 17, 2019:

How’d that get there? Removed it.

achow101 force-pushed on Sep 17, 2019

achow101 force-pushed on Sep 18, 2019

in src/wallet/walletdb.cpp:519 in 95aca1f9be outdated

516@@ -514,10 +517,6 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
517     pwallet->WalletLogPrintf("Keys: %u plaintext, %u encrypted, %u w/ metadata, %u total. Unknown wallet records: %u\n",
518            wss.nKeys, wss.nCKeys, wss.nKeyMeta, wss.nKeys + wss.nCKeys, wss.m_unknown_records);
519 
520-    // nTimeFirstKey is only reliable if all keys have metadata
521-    if ((wss.nKeys + wss.nCKeys + wss.nWatchKeys) != wss.nKeyMeta)
522-        pwallet->UpdateTimeFirstKey(1);

ryanofsky commented at 4:40 pm on September 18, 2019:

Did this call move somewhere? Or is it not important and just dropped?

achow101 commented at 4:52 pm on September 18, 2019:

No.. but we should probably keep it, so I’ve added it back in.

achow101 force-pushed on Sep 18, 2019

in src/wallet/wallet.cpp:2919 in df689d5973 outdated

2930-        LOCK(cs_KeyStore);
2931-        // This wallet is in its first run if all of these are empty
2932-        fFirstRunRet = mapKeys.empty() && mapCryptedKeys.empty() && mapWatchKeys.empty() && setWatchOnly.empty() && mapScripts.empty()
2933-            && !IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) && !IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET);
2934+    // This wallet is in its first run if all of these are empty and this isn't blank or no privkeys
2935+    fFirstRunRet = m_spk_managers.empty() && !IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) && !IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET);

ryanofsky commented at 5:56 pm on September 18, 2019:

I wasn’t sure if the new m_spk_managers.empty() condition here is equivalent to the previous mapKeys.empty() && mapCryptedKeys.empty() && mapWatchKeys.empty() && setWatchOnly.empty() && mapScripts.empty() condition. I think it is equivalent because the LoadWallet(*this) call above will only call GetLegacyScriptPubKeyMan() internally if keys are found, and if it does not, SetupLegacyScriptPubKeyMan() will not have been called yet and m_spk_managers will be empty. But correct me if I’m wrong.

I guess it’s a little confusing that GetLegacyScriptPubKeyMan right now will actually create a legacy keyman if one doesn’t exist, and never return null, even though there are checks for it returning null throughout the code. Maybe there should be two different methods for clarity:

0LegacyScriptPubKeyMan* GetLegacyScriptPubKeyMan();
1LegacyScriptPubKeyMan& GetOrCreateLegacyScriptPubKeyMan();

Just a suggestion, though, and correct me if I missed anything.

achow101 commented at 8:43 pm on September 19, 2019:

Yes, that is correct. That’s pretty much the only reason GetLegacyScriptPubKeyMan also creates a LegacyScriptPubKeyMan.

The checks for null are for the future when we have more ScriptPubKeyMan classes.

achow101 commented at 11:22 pm on September 19, 2019:

I’ve added GetOrCreateLegacyScriptPubKeyMan

in src/wallet/wallet.cpp:4791 in df689d5973 outdated

4052+        for (const auto& spk_man_pair : m_spk_managers) {
4053+            if (!spk_man_pair.second->Unlock(vMasterKeyIn, accept_no_keys)) {
4054+                return false;
4055             }
4056-            keyPass = true;
4057-            if (fDecryptionThoroughlyChecked)

ryanofsky commented at 6:11 pm on September 19, 2019:

CWallet::fDecryptionThoroughlyChecked variable isn’t actually used anymore, only assigned to. It should probably either be used or removed. Maybe it’s not necessary anymore since keyman has the same variable.

achow101 commented at 11:22 pm on September 19, 2019:

Removed it

in src/wallet/wallet.cpp:573 in df689d5973 outdated

570@@ -919,10 +571,10 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase)
571 
572         // if we are using HD, replace the HD seed with a new one
573         if (IsHDEnabled()) {

ryanofsky commented at 7:08 pm on September 19, 2019:

It’d make more sense to call spk_man_pair.second->IsHDEnabled on the individual keyman object than IsHDEnabled on the wallet object, which is the intersection of all IsHDEnabled statuses.

achow101 commented at 11:22 pm on September 19, 2019:

Done

achow101 force-pushed on Sep 19, 2019

in src/wallet/scriptpubkeyman.cpp:128 in 8a2d51c741 outdated

122+        if (keystore.HaveKey(keyID)) {
123+            ret = std::max(ret, IsMineResult::SPENDABLE);
124+        }
125+        break;
126+    }
127+    case TX_SCRIPTHASH:

Sjors commented at 8:30 am on September 21, 2019:

In 8a2d51c741b896842e093fa35590ef36abb907c4: case TX_PUBKEYHASH contains a bug fix. Maybe move that to a seperate commit so this commit is actually move-only.

achow101 commented at 5:30 pm on September 23, 2019:

Moved into its own commit

in src/wallet/scriptpubkeyman.cpp:528 in 3099b7c2db outdated

302@@ -303,6 +303,16 @@ bool LegacyScriptPubKeyMan::CanProvide(const CScript& script, SignatureData& sig
303 
304 const CKeyMetadata* LegacyScriptPubKeyMan::GetMetadata(uint160 id) const
305 {
306+    LOCK(cs_KeyStore);
307+    auto it = mapKeyMetadata.find(CKeyID(id));

Sjors commented at 8:57 am on September 21, 2019:

In 3099b7c2dbfd31f4a1747f7e38629e44988d6ef7: for other reviewers, this is more or less lifted from getaddressinfo in rpcwallet.cpp.

in src/wallet/wallet.cpp:3112 in d3dde67e4a outdated

3721-        if (!set_pre_split_keypool.empty()) {
3722-            oldestKey = std::max(GetOldestKeyTimeInPool(set_pre_split_keypool, batch), oldestKey);
3723-        }
3724+    int64_t oldestKey = 0;
3725+    for (const auto& spk_man_pair : m_spk_managers) {
3726+        oldestKey = std::max(oldestKey, spk_man_pair.second->GetOldestKeyPoolTime());

Sjors commented at 10:17 am on September 21, 2019:

In d3dde67e4aba051bea38daae60ed80fd4db69d3b: GetOldestKeyPoolTime was introduced in #10235. It calls GetOldestKeyTimeInPool for the internal and external keypools. IIUC this gets the timestamp from keypool entry at index 0. For legacy wallets those are the same, so std::max is fine. But if we add descriptor wallets, and allow importing them after wallet creation, then std::max would return the newest one. So maybe we should switch to std::min here and initialize oldestKey with Time()?

Alternatively we could drop keypoololdest in the RPC wallet for non-legacy spk_managers, and show it for each individual descriptor instead of the whole wallet.

achow101 commented at 5:30 pm on September 23, 2019:

I’ve changed it to use min. Looking at the original implementation in master, it actually looks like it’s broken as it uses max.

Sjors approved

Sjors commented at 10:47 am on September 21, 2019: member

Comparing function bodies side by side was less painful than I feared.

ACK 1c01da74d33033b0aef4a30dc305a03594ff7d33, modulo two parts I did not review yet:

wallet encryption stuff, still in flux, see discussion here
wallet creation and upgrade: see discussion here

I also ran the (very minimal) backwards compatibility & upgrade sanity checks in #12134.

When all reviewers are satisfied, I suggest merging as early possible after 0.19 branch-off.

achow101 force-pushed on Sep 23, 2019

in src/wallet/wallet.h:738 in 3b05946857 outdated

739@@ -942,8 +740,8 @@ class CWallet final : public FillableSigningProvider, private interfaces::Chain:
740         encrypted_batch = nullptr;

ryanofsky commented at 7:51 pm on September 24, 2019:

I don’t think there’s any reason for CWallet::encrypted_batch to exist as a member variable anymore. It could just be a local in CWallet::EncryptWallet

achow101 commented at 8:08 pm on September 24, 2019:

Done

in src/wallet/walletdb.cpp:521 in 3b05946857 outdated

516@@ -515,8 +517,13 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
517            wss.nKeys, wss.nCKeys, wss.nKeyMeta, wss.nKeys + wss.nCKeys, wss.m_unknown_records);
518 
519     // nTimeFirstKey is only reliable if all keys have metadata
520-    if ((wss.nKeys + wss.nCKeys + wss.nWatchKeys) != wss.nKeyMeta)
521-        pwallet->UpdateTimeFirstKey(1);
522+    if ((wss.nKeys + wss.nCKeys + wss.nWatchKeys) != wss.nKeyMeta) {
523+        auto spk_man = pwallet->GetOrCreateLegacyScriptPubKeyMan();

ryanofsky commented at 8:02 pm on September 24, 2019:

Seems like this should use GetLegacyScriptPubKeyMan not GetOrCreateLegacyScriptPubKeyMan to make more sense and be in line with the previous code. Or if GetOrCreateLegacyScriptPubKeyMan is actually better here, probably should drop if (spk_man) condition which would never be false

achow101 commented at 8:08 pm on September 24, 2019:

Changed to GetLegacyScriptPubKeyMan.

achow101 force-pushed on Sep 24, 2019

in src/wallet/wallet.h:146 in 47b342db0d outdated

141@@ -283,10 +142,10 @@ class ReserveDestination
142 protected:
143     //! The wallet to reserve from
144     CWallet* pwallet;
145+    //! The ScriptPubKeyMan to reserve from. Based on type when GetReservedDestination is called
146+    ScriptPubKeyMan* m_spk_man;

ryanofsky commented at 4:59 pm on September 25, 2019:

It’d be good to initialize this to nullptr since it’s not set in the constructor like pwallet is

achow101 commented at 8:08 pm on September 26, 2019:

Done

in src/wallet/scriptpubkeyman.h:31 in 47b342db0d outdated

22+public:
23+    virtual ~WalletStorage() = default;
24+    virtual const std::string GetDisplayName() const = 0;
25+    virtual std::shared_ptr<WalletDatabase> GetDatabase() = 0;
26+    virtual bool IsWalletFlagSet(uint64_t) const = 0;
27+    virtual void SetWalletFlag(uint64_t) = 0;

ryanofsky commented at 7:35 pm on September 25, 2019:

SetWalletFlag method and override don’t seem to be called anymore could be removed.

achow101 commented at 7:48 pm on September 26, 2019:

It’s used in descriptor wallets (next PR).

in src/wallet/wallet.h:598 in 47b342db0d outdated

604     using WatchKeyMap = std::map<CKeyID, CPubKey>;
605 
606-    bool SetCrypted();
607-
608     //! will encrypt previously unencrypted keys
609     bool EncryptKeys(CKeyingMaterial& vMasterKeyIn);

ryanofsky commented at 5:20 pm on September 26, 2019:

Looks like this should be removed. (Calls and definition were removed but not this declaration.)

achow101 commented at 8:08 pm on September 26, 2019:

Done

in src/wallet/wallet.cpp:4865 in 47b342db0d outdated

4093-    }
4094-    // Check for watch-only pubkeys
4095-    return GetWatchPubKey(address, vchPubKeyOut);
4096-}
4097-
4098-std::set<CKeyID> CWallet::GetKeys() const

ryanofsky commented at 6:29 pm on September 26, 2019:

In commit “Remove unused functions and switch CWallet to use ScriptPubKeyMan” (47b342db0ddb1bc9bc595635e36bb2f16921a605)

There’s no replacement for this function and I think losing it might break CWallet::GetKeyBirthTimes for encrypted wallets. Regardless, this should probably become a keyman method instead of being removed so the SigningProvider interface returns consistent results.

achow101 commented at 8:09 pm on September 26, 2019:

Added LegacyScriptPubKeyMan::GetKeys

in src/wallet/scriptpubkeyman.h:167 in 47b342db0d outdated

162+    virtual isminetype IsMine(const CScript& script) const { return ISMINE_NO; }
163+
164+    virtual bool HasEncryptedData() const { return false; }
165+
166+    virtual bool CheckDecryptionKey(const CKeyingMaterial& vMasterKeyIn, bool accept_no_keys = false) { return false; }
167+    virtual bool Encrypt(CKeyingMaterial& vMasterKeyIn, WalletBatch* batch) { return false; }

ryanofsky commented at 6:48 pm on September 26, 2019:

Master key argument should be a const reference, not mutable. Also these arguments could be named master_key, since there’s no longer any vMasterKeyIn/vMasterKey conflict.

achow101 commented at 8:09 pm on September 26, 2019:

Done

in src/wallet/scriptpubkeyman.cpp:211 in 47b342db0d outdated

206+}
207+
208+bool LegacyScriptPubKeyMan::HasEncryptedData() const
209+{
210+    LOCK(cs_KeyStore);
211+    return fUseCrypto || !mapCryptedKeys.empty();

ryanofsky commented at 7:24 pm on September 26, 2019:

The fUseCrypto variable seems completely pointless now that IsCrypted is locking cs_KeyStore and checking mapCryptedKeys.empty(). As far as I can tell it only barely made sense before as an optimization to avoid locking.

Would suggest dropping fUseCrypto and dropping the SetCrypted method, just replacing if(!SetCrypted()) return false calls with if (!mapKeys.empty()) return false

achow101 commented at 7:39 pm on September 26, 2019:

fUseCrypto is needed for wallet loading in order to load the initial encrypted key. In that situation, mapCryptedKeys will be empty because no keys have been loaded into memory.

ryanofsky commented at 5:26 pm on September 27, 2019:

re: #16341 (review)

fUseCrypto is needed for wallet loading in order to load the initial encrypted key. In that situation, mapCryptedKeys will be empty because no keys have been loaded into memory.

I think since you removed the SetCrypted call at the beginning of CWallet::Lock, the fUseCrypto variable no longer contains any useful information (it’s just a stale sometimes out-of-date cache of !mapCryptedKeys.empty()), and that’s why HasEncryptedData now has to take a lock and return fUseCrypto || !mapCryptedKeys.empty() instead of just the previous fUseCrypto.

If you got rid of the fUseCrypto variable and the SetCrypted() method as described #16341 (review), I think it would be equivalent to the current code and more comprehensible.

achow101 commented at 5:55 pm on September 27, 2019:

It doesn’t feel like that is safe. A blank wallet would have mapKeys empty so would that be considered encrypted?

ryanofsky commented at 6:04 pm on September 27, 2019:

It doesn’t feel like that is safe. A blank wallet would have mapKeys empty so would that be considered encrypted?

No, but I’m not suggesting that. HasEncryptedData() could return !mapCryptedKeys.empty() and fUseCrypto and SetCrypted could both be deleted. The two if(!SetCrypted()) return false lines would just be replaced by if (!mapKeys.empty()) return false

achow101 commented at 7:22 pm on September 27, 2019:

No, but I’m not suggesting that. HasEncryptedData() could return !mapCryptedKeys.empty() and fUseCrypto and SetCrypted could both be deleted. The two if(!SetCrypted()) return false lines would just be replaced by if (!mapKeys.empty()) return false

This does not work since mapKeys is not empty utnil after a wallet is encrypted, but the places the SetCrypted are used are AddCryptedKey which is used during Encrypt.

ryanofsky commented at 7:48 pm on September 27, 2019:

This does not work since mapKeys is not empty utnil after a wallet is encrypted, but the places the SetCrypted are used are AddCryptedKey which is used during Encrypt.

Wow, that’s confusing. It makes the meaning of the fUseCrypto member very difficult to describe. How about just clearing mapKeys at the beginning instead of the end of the encrypt function?

0-    fUseCrypto = true;
1-    for (const KeyMap::value_type& mKey : mapKeys)
2+    KeyMap keys_to_encrypt;
3+    keys_to_encrypt.swap(mapKeys); // Clear mapKeys so AddCryptedKeyInner will succeed.
4+    for (const KeyMap::value_type& mKey : keys_to_encrypt)

achow101 commented at 8:23 pm on September 27, 2019:

This breaks blank encrypted wallets (necessary for born encrypted wallets).

achow101 commented at 9:16 pm on September 27, 2019:

I was able to remove fUseCrypto and instead used a check for whether there were any encryption keys present (required adding HasEncrytionKeys to WalletStorage).

achow101 force-pushed on Sep 26, 2019

achow101 force-pushed on Sep 27, 2019

achow101 force-pushed on Sep 30, 2019

in src/wallet/scriptpubkeyman.h:206 in 4abd055ce1 outdated

201+    virtual void RewriteDB() {}
202+
203+    virtual int64_t GetOldestKeyPoolTime() { return GetTime(); }
204+
205+    virtual size_t KeypoolCountExternalKeys() { return 0; }
206+    virtual unsigned int GetKeypoolSize() const { return 0; }

ryanofsky commented at 4:26 pm on October 2, 2019:

Might be good to standardize on GetKeyPoolSize pr GetKeypoolSize (both are used after this change)

achow101 commented at 10:44 pm on October 2, 2019:

Standardized on GetKeyPoolSize as that was already being used.

in src/wallet/scriptpubkeyman.cpp:488 in 26c1ebe6a7 outdated

128 
129 unsigned int LegacyScriptPubKeyMan::GetKeypoolSize() const
130 {
131-    return 0;
132+    LOCK(cs_KeyStore);
133+    return setInternalKeyPool.size() + setExternalKeyPool.size() + set_pre_split_keypool.size();

ryanofsky commented at 5:12 pm on October 2, 2019:

In commit “Implement GetOldestKeyPoolTime, KeypoolCountExternalKeys, and GetKeypoolSize in LegacyScriptPubKeyMan” (26c1ebe6a7bea812860636f9d7316fc71b8d90dc)

Is it a bugfix to include set_pre_split_keypool size in this return value? Wallet method doesn’t do this, so behavior seems to change in later commit 937cf1e2265ace44147475dc9da8a5ec0b1dda47

https://github.com/bitcoin/bitcoin/blob/f4a0d27e85754d60804ffa36e415b67c263180b9/src/wallet/wallet.h#L1251-L1255

achow101 commented at 9:36 pm on October 2, 2019:

Yes, that is a bug fix.

in src/wallet/wallet.cpp:3832 in 4abd055ce1 outdated

3826@@ -4543,8 +3827,12 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,
3827 
3828         // No need to read and scan block if block was created before
3829         // our wallet birthday (as adjusted for block time variability)
3830-        if (walletInstance->nTimeFirstKey) {
3831-            if (Optional<int> first_block = locked_chain->findFirstBlockWithTimeAndHeight(walletInstance->nTimeFirstKey - TIMESTAMP_WINDOW, rescan_height, nullptr)) {
3832+        int64_t time_first_key = std::numeric_limits<int64_t>::max();
3833+        for (auto spk_man : walletInstance->GetActiveScriptPubKeyMans()) {
3834+            std::min(time_first_key, spk_man->GetTimeFirstKey());

ryanofsky commented at 5:33 pm on October 2, 2019:

In commit “[ci skip] Use GetTimeFirstKey instead of nTimeFirstKey” (4b7effe37f5efeddb54523b95e58d464bc317e7f)

Is this a bug? It seems like this should say

0time_first_key = std::min(time_first_key, spk_man->GetTimeFirstKey());

If this is a bug, I’m a little surprised there isn’t any test coverage for this.

achow101 commented at 10:44 pm on October 2, 2019:

Seems like a bug, fixed. Changing it has no effect on any tests…

achow101 force-pushed on Oct 2, 2019

in src/wallet/wallet.cpp:4000 in 0b66a22b85 outdated

3997-    {
3998-        LOCK(cs_wallet);
3999-        auto it = mapKeyMetadata.find(keyID);
4000-        if (it != mapKeyMetadata.end()) {
4001-            meta = it->second;
4002+    for (const auto& entry : m_spk_managers) {

ryanofsky commented at 5:42 pm on October 3, 2019:

I think it’d make sense to add if (HasEncryptionKeys()) return true; before this loop so IsCrypted meaning is unchanged and it’ll reliably return true if wallet encryption is present, regardless of what keyman instances are created and how they function internally.

achow101 commented at 7:02 pm on October 3, 2019:

Done

in src/wallet/scriptpubkeyman.cpp:212 in 0b66a22b85 outdated

206+}
207+
208+bool LegacyScriptPubKeyMan::HasEncryptedData() const
209+{
210+    LOCK(cs_KeyStore);
211+    return HasEncryptionKeys() || !mapCryptedKeys.empty();

ryanofsky commented at 5:54 pm on October 3, 2019:

Not asking for this change here since it could be a pain to backport, but in the future I think it’d be nicer if HasEncryptedData simply returned !mapCryptedKeys.empty() and HasEncryptionKeys simply returned !mapMasterKeys.empty() so the two functions would be independent and not call each other.

At call sites, I think it would be an improvement to write:

0if (!m_storage.HasEncryptionKeys() && !HasEncryptedData())

instead of

0if (!HasEncryptedData())

because otherwise it’s not clear that the presence of wallet encryption keys is a controlling factor (this is confusing in AddKeyPubKeyInner when you’d expect to be checking for the presense of keys to encypt new data with, not the presence of already encrypted data)

in src/wallet/scriptpubkeyman.h:144 in 0b66a22b85 outdated

139+ * and it's related scripts and keys, including encryption.
140+ */
141+class ScriptPubKeyMan
142+{
143+private:
144+    WalletStorage& storage;

ryanofsky commented at 6:06 pm on October 3, 2019:

Should this be called m_storage instead of storage? Also any particular reason this is private and not protected so keyman implementations could access the interface directly without having to go through an extra hop?

achow101 commented at 7:02 pm on October 3, 2019:

Changed to m_storage and made protected.

in src/wallet/scriptpubkeyman.h:160 in 0b66a22b85 outdated

155+    void UnsetWalletFlagWithDB(WalletBatch& batch, uint64_t flag) { return storage.UnsetWalletFlagWithDB(batch, flag); }
156+    bool CanSupportFeature(enum WalletFeature wf) const { return storage.CanSupportFeature(wf); }
157+    void SetMinVersion(enum WalletFeature wf) { return storage.SetMinVersion(wf); }
158+    const CKeyingMaterial& GetEncryptionKey() const { return storage.GetEncryptionKey(); }
159+    bool HasEncryptionKeys() const { return storage.HasEncryptionKeys(); }
160+    bool IsLocked() const { return storage.IsLocked(); }

ryanofsky commented at 6:12 pm on October 3, 2019:

Not asking for this change here since it could be a pain to backport, but in the future I think it’d be nicer to drop these wrapper functions and for keyman implementations to explicitly call storage methods, so the split between what data is managed by the top level wallet class and what data is managed by individual keyman implementations is clearer.

achow101 commented at 6:31 pm on October 3, 2019:

Sure. I used the wrappers so that moved code wouldn’t need to change.

in src/wallet/scriptpubkeyman.cpp:276 in 0b66a22b85 outdated

271+        if (!AddCryptedKey(vchPubKey, vchCryptedSecret)) {
272+            encrypted_batch = nullptr;
273+            return false;
274+        }
275+    }
276+    mapKeys.clear();

ryanofsky commented at 6:23 pm on October 3, 2019:

Should drop this clear. The map is already cleared above, so having this extra clear separated by a long loop seems confusing and maybe dangerous if the code is changed again in the future

achow101 commented at 7:02 pm on October 3, 2019:

Done

in src/wallet/scriptpubkeyman.cpp:288 in 0b66a22b85 outdated

284+        return false;
285+    }
286+
287+    {
288+        LOCK(cs_KeyStore);
289+        if (!ReserveKeyFromKeyPool(index, keypool, internal) && !IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {

ryanofsky commented at 6:58 pm on October 3, 2019:

Why is && !IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) added here? This seems broken because why would you want to return true from GetReserved if reserving a key failed? But it also seems like it should not be able to make any difference because the CanGetAddresses call above would return false if private keys were disabled.

I’d suggest either dropping this condition or adding an explanatory comment. Tests do seem to pass if this is dropped.

Also, I’m not sure if the CanGetAddresses check added above is more than just a sanity check, but if it is actually an expected condition, it’d be useful to have a comment saying when it’s expected to trigger. It seems like a fine check, but it’s not clear what the motivation was for adding it. Tests do seem to pass without it.

achow101 commented at 8:30 pm on October 3, 2019:

Removed. I don’t remember why they were added, could have been due to some previous change that was since changed so they are no longer needed.

I believe the CanGetAddresses check is for sanity.

achow101 force-pushed on Oct 3, 2019

in src/wallet/wallet.cpp:3254 in c14ad7a898 outdated

3266+    m_spk_man = pwallet->GetScriptPubKeyMan(type, internal);
3267+    if (!m_spk_man) {
3268         return false;
3269     }
3270 
3271+    m_spk_man->TopUp();

ryanofsky commented at 7:50 pm on October 3, 2019:

This TopUp seems newly added rather than moved, and the return value isn’t checked. Tests also seem to pass without it. I also noticed TopUp calls were removed in GetNewChangeDestination and CreateWalletFromFile, and I don’t know why.

These changes seem ok, but I guess the things I’d like to know would be:

If something in particular motivated these changes, or if they just seemed like a good ideas.
In general, if there’s some kind of reasoning about where the keypool should be topped up. For example, I’d expect either keyman objects to be responible for topping up internally, or for the wallet to be responsible, but not for calls to happen both places. If there is intent behind this, maybe the Topup method could have a comment saying when it’s supposed to be called.

Could also consider reverting top up changes that aren’t necessary here. I think I’ll be done with my neverending review of this PR soon, but little changes like this I don’t understand do not make the end seem closer! :smiley:

achow101 commented at 8:10 pm on October 3, 2019:

This particular TopUp was moved from GetNewChangeDestination into GetReservedDestination because GetNewChangeDestination does not determine which ScriptPubKeyMan to use, but GetReservedDestination does.

Where in CreateWalletFromFile were TopUps removed? It’s likely that those were rolled into SetupGeneration.

ryanofsky commented at 8:31 pm on October 3, 2019:

Ok, thanks for the explanation. The changes here do make sense since they’re just moves. I didn’t realize the GetNewChangeDestination and GetReservedDestination changes were related, or that SetupGeneration calls TopUp (indirectly through LegacyScriptPubKeyMan::NewKeyPool).

achow101 force-pushed on Oct 3, 2019

in src/wallet/scriptpubkeyman.cpp:1187 in ce944a2f30 outdated

1182+
1183+    CKeyPool keypool;
1184+    {
1185+        LOCK(cs_KeyStore);
1186+        int64_t nIndex;
1187+        if (!ReserveKeyFromKeyPool(nIndex, keypool, internal)) {

ryanofsky commented at 7:29 pm on October 4, 2019:

In the original line that this was copied from there was a && !DISABLE_PRIVATE_KEYS check, I believe to avoid the GenerateNewKey call below. I think this got accidentally deleted in a recent update and would be good to restore:

https://github.com/bitcoin/bitcoin/blob/7b701fef58f627956d597817a1f9422edd890cdc/src/wallet/wallet.cpp#L3694

achow101 commented at 7:53 pm on October 4, 2019:

Done

achow101 force-pushed on Oct 4, 2019

in src/wallet/wallet.h:595 in 2d8785dd61 outdated

599-    bool fDecryptionThoroughlyChecked;
600+    CKeyingMaterial vMasterKey GUARDED_BY(cs_wallet);
601 
602     using CryptedKeyMap = std::map<CKeyID, std::pair<CPubKey, std::vector<unsigned char>>>;
603     using WatchOnlySet = std::set<CScript>;
604     using WatchKeyMap = std::map<CKeyID, CPubKey>;

ryanofsky commented at 10:42 am on October 6, 2019:

Should remove CryptedKeyMap, WatchOnlySet, and WatchKeyMap definitions from wallet.h since they are now unused and duplicated in scriptpubkeyman.h

achow101 commented at 3:25 pm on October 6, 2019:

Done

in src/wallet/wallet.cpp:3109 in 2d8785dd61 outdated

3128-    if (IsHDEnabled() && CanSupportFeature(FEATURE_HD_SPLIT)) {
3129-        oldestKey = std::max(GetOldestKeyTimeInPool(setInternalKeyPool, batch), oldestKey);
3130-        if (!set_pre_split_keypool.empty()) {
3131-            oldestKey = std::max(GetOldestKeyTimeInPool(set_pre_split_keypool, batch), oldestKey);
3132-        }
3133+    int64_t oldestKey = GetTime();

ryanofsky commented at 11:01 am on October 6, 2019:

Can this be changed to start with max int rather than the current time, similar to the other oldest key loop:

https://github.com/bitcoin/bitcoin/blob/2d8785dd618b85dd8714d70a47e066be9b6e1969/src/wallet/wallet.cpp#L3830-L3833

It seems nicer as an API if GetOldestKeyPoolTime would return a fixed value you could check against, rather than an ever changing current time.

achow101 commented at 3:25 pm on October 6, 2019:

Done

achow101 force-pushed on Oct 6, 2019

ryanofsky referenced this in commit 022a3a1ea6 on Oct 7, 2019

ryanofsky referenced this in commit ded789da14 on Oct 7, 2019

ryanofsky referenced this in commit fbc33c8222 on Oct 7, 2019

ryanofsky referenced this in commit 6469adeafa on Oct 7, 2019

ryanofsky referenced this in commit 1532876c5e on Oct 7, 2019

ryanofsky referenced this in commit 6ac26975f0 on Oct 7, 2019

ryanofsky referenced this in commit 454cc4ef82 on Oct 7, 2019

ryanofsky referenced this in commit 12b0b2bf15 on Oct 7, 2019

ryanofsky approved

ryanofsky commented at 7:33 pm on October 7, 2019: member

[Edited 10/12: Removed ACK]

utACK 0fb52e481f411fdf67257965a9144522b4bac87c. Nice work on this @achow101 and thank you for being so responsive to suggestions!

To review this, I created my own branch pr/keyman with the same end result as this PR but with a different commit structure that made more sense to me. You could consider adopting this branch if you think it’d be easier for others to review. The branch consists of a pure moveonly commit followed by a renaming commit followed by smaller refactoring and bugfix commits. Unlike the current PR, moved code is never added in one commit and deleted in a separate commit, and every commit compiles and passes tests, and refactoring changes are in different commits than bugfix changes. It’s possible the branch might be helpful to other reviewers even if not adopted, and in any case I’m happy with the PR in its current form.

`pr/keyman`
3d38bf3d0c620755ff9ce9a53eb161e2d5ffd055 MOVEONLY: Move key handling code out of wallet to keyman file	+24 -6 ⇔1375
530d4d4e08826f0d28e34e81ceaf341b58d29982 Refactor: Split up CWallet and LegacyScriptPubKeyMan and classes	+591 -350 ⇔295
156ae0e3efe7ecf972856cfb97bc7dd947979e76 MOVEONLY: Reorder LegacyScriptPubKeyMan methods	+4 -5 ⇔87
797b4ab016335ec67a158ca2012d80d1ac65e867 Refactor: Declare LegacyScriptPubKeyMan methods as virtual	+21 -6 ⇔1
51acb04eded9501ffd082f798f03266e5b0f5b04 Refactor: Add new ScriptPubKeyMan virtual methods	+46 -8 ⇔0
5a4a5bcae4bcf3da1075d6c24928b6f3790bc8f2 Refactor: Move SetAddressBook call out of LegacyScriptPubKeyMan::GetNewDestination	+7 -4 ⇔2
7c71c029010556fc76c5f660c6c8f489ff9275d0 Refactor: Move SetWalletFlag out of LegacyScriptPubKeyMan::UpgradeKeyMetadata	+2 -2 ⇔0
bd5b4351377a432333e0e20842a704fc3e7582fb Refactor: Remove UnsetWalletFlag call from LegacyScriptPubKeyMan::SetHDSeed	+2 -1 ⇔0
addfa4fd8c2e49bb23ebe5f7a1e05f8c0afa737f Refactor: Move SetAddressBookWithDB call out of LegacyScriptPubKeyMan::ImportScriptPubKeys	+6 -4 ⇔5
9d6999c51c991b38fb837f72662dcb2576072485 Refactor: Move LoadKey LegacyScriptPubKeyMan method definition	+6 -1 ⇔0
f18b5e8d08268f4c41be0db9e96633dbfbc1dc54 Refactor: Move GetMetadata code out of getaddressinfo	+21 -8 ⇔5
34aa63af4446ed3b064fcf2093991be934f3ffb4 Refactor: Move MarkUnusedAddresses code out of CWallet::AddToWalletIfInvolvingMe	+12 -5 ⇔14
ac0080cf211eb4873ada28092511f5cc99ea66c6 Refactor: Move Upgrade code out of CWallet::CreateWalletFromFile	+24 -12 ⇔20
1b78923a5fcdf4423112eed3c2748cfe9e4dd6b0 Refactor: Move HavePrivateKeys code out of CWallet::CreateWalletFromFile	+13 -2 ⇔0
c59ca1ecde4426e4de3cb1c817207ec68453b812 Refactor: Move SetupGeneration code out of CWallet	+22 -11 ⇔12
33c8232f0a39a355726c7f01167071a917454391 Refactor: Move RewriteDB code out of CWallet	+20 -4 ⇔5
511ee46de0fad2eba114a2ae4445087a4c8fef46 Refactor: Move GetKeypoolSize code out of CWallet	+12 -3 ⇔8
3f3fad5dd471a60abbe3c7ee6ef56465f2ec4f9e Refactor: Move nTimeFirstKey accesses out of CWallet	+16 -3 ⇔0
8aa21da6b73ddb4bcd3de65a9d51062c40865991 Refactor: Move encryption code between KeyMan and Wallet	+68 -49 ⇔23
6192746344ab4565ae6f46af177d7fdce6b5b22f Locking: Lock cs_KeyStore instead of cs_main in legacy keyman	+51 -75 ⇔40
0c50a72ac4c6192c4cfcc228306d9dc6b7e6a561 Refactor: Allow LegacyScriptPubKeyMan to be null	+72 -27 ⇔0
360cc0c4d4d8244df796b33275e3884ecd469107 Refactor: Require scriptPubKey to get wallet SigningProvider	+101 -24 ⇔0
022a3a1ea675949cada71f70b2c7b365be9ed6ad Key pool: Move CanGetAddresses call	+2 -1 ⇔2
be91169dc313ea9af9b84a1002a466a6d864949c Key pool: Move LearnRelated and GetDestination calls	+8 -8 ⇔0
ded789da149cbe2c5dbac7be9d03a562e63ed073 Key pool: Move TopUp call	+0 -1 ⇔1
65654f2bfbbee1735a72af699d403d8817b4ab07 Key pool: Change ReturnDestination interface to take address instead of key	+12 -14 ⇔0
1ef5245a4e6cd082a191fb144d896f8e21124daa Key pool: Make TopUp fail if unexpected wallet flags are set	+3 -0 ⇔0
fbc33c8222b6127cbcf168f50d580b1973dee7a9 Key pool: Fix omitted pre-split count in GetKeyPoolSize	+1 -1 ⇔0
76dcd952ec383543b8c80e8c89eb3e1f18abf6ad IsMine: Set state to WATCH_ONLY if we can get the pubkey	+6 -2 ⇔0
6469adeafa54e709b027bbf58478bed5f6faf986 HD Split: Avoid redundant upgrades	+1 -1 ⇔0
34b50f459e0bab550553a8e6be12f3be1b4717c7 Box the wallet: Add multiple keyman maps and loops	+162 -47 ⇔22
b59be76a60dfab381dc742e4a74e6c051f4693bf Refactor: Copy CWallet signals and print function to LegacyScriptPubKeyMan	+22 -16 ⇔6
3319222232df36179372985f3bbf03939973fbbe Cleanup: Update message strings and comments	+4 -5 ⇔3
2fa05963c04bcb30bfa6f4ceb622144d8fcb7e7b Cleanup: Drop unused GUI learnRelatedScripts method	+0 -5 ⇔0
1532876c5ea6c01c0a62d7d03d99373f35cacb3a Refactor: Replace SigningProvider pointers with unique_ptrs	+45 -29 ⇔0
6ac26975f05f39faf26649c263984ea18e782210 Refactor: Make LegacyScriptPubKeyMan::HasEncryptedData() call WalletStorage::HasEncryptionKeys()	+7 -7 ⇔0
454cc4ef82387d3f05b4bf4907307182da8f5671 Refactor: Add storage wrapper functions	+44 -33 ⇔0
12b0b2bf15c08576a080d43373a04b42e9897f50 Refactor: Make WalletDatabase a shared_ptr instead of a unique_ptr	+28 -24 ⇔0

in src/wallet/scriptpubkeyman.h:319 in 0fb52e481f outdated

314+    void ReturnKey(int64_t nIndex, bool fInternal, const CKeyID& pubkey_id);
315+
316+public:
317+    using ScriptPubKeyMan::ScriptPubKeyMan;
318+
319+    bool GetNewDestination(const OutputType type, CTxDestination& dest, std::string& error) override EXCLUSIVE_LOCKS_REQUIRED(cs_KeyStore);

ryanofsky commented at 3:03 pm on October 9, 2019:

The EXCLUSIVE_LOCKS_REQUIRED(cs_KeyStore) annotation appears to be a mistake. Wallet::GetNewDestination calls this method without acquiring cs_KeyStore:

https://github.com/bitcoin/bitcoin/blob/0fb52e481f411fdf67257965a9144522b4bac87c/src/wallet/wallet.cpp#L3082

The only reason there’s no compile error is that the call is happening through the overridden declaration above which is missing EXCLUSIVE_LOCKS_REQUIRED:

https://github.com/bitcoin/bitcoin/blob/0fb52e481f411fdf67257965a9144522b4bac87c/src/wallet/scriptpubkeyman.h#L161

Probably what should happen here is that EXCLUSIVE_LOCKS_REQUIRED(cs_KeyStore) should be dropped and cs_KeyStore get locked internally instead.

achow101 commented at 5:22 pm on October 9, 2019:

Changed to lock internally.

in src/wallet/scriptpubkeyman.h:370 in 0fb52e481f outdated

365+
366+    // Map from Script ID to key metadata (for watch-only keys).
367+    std::map<CScriptID, CKeyMetadata> m_script_metadata GUARDED_BY(cs_KeyStore);
368+
369+    //! Adds a key to the store, and saves it to disk.
370+    bool AddKeyPubKey(const CKey& key, const CPubKey &pubkey) override EXCLUSIVE_LOCKS_REQUIRED(cs_KeyStore);

ryanofsky commented at 3:11 pm on October 9, 2019:

The EXCLUSIVE_LOCKS_REQUIRED annotation here is a little dodgy because the parent virtual method doesn’t have the same annotation, so it will be bypassed if this is called through the parent class.

To address this, it might be a good idea to add EXCLUSIVE_LOCKS_REQUIRED to the parent declaration in SigningProvider, or to add an AssertLockHeld in LegacyScriptPubKeyMan::AddKeyPubKey for more enforcement, or to remove the annotation and just lock cs_KeyStore internally in AddKeyPubKey.

It might be a good idea to AssertLockHeld(cs_KeyStore) in LegacyScriptPubKeyMan::AddKeyPubKey, because the EXCLUSIVE_LOCKS_REQUIRED annotation won’t be enforced if AddKeyPubKey is called through the overriden virtual method. Other options would be to EXCLUSIVE_LOCKS_REQUIRED

achow101 commented at 5:22 pm on October 9, 2019:

Changed to lock internally.

in src/wallet/wallet.cpp:3923 in 0fb52e481f outdated

3280-        vchPubKey = keypool.vchPubKey;
3281         fInternal = keypool.fInternal;
3282     }
3283-    assert(vchPubKey.IsValid());
3284-    pwallet->LearnRelatedScripts(vchPubKey, type);
3285-    address = GetDestinationForKey(vchPubKey, type);

ryanofsky commented at 3:40 pm on October 9, 2019:

Seems like a bug to no longer set ReserveDestination::address here, since it might be needed later to call ReturnDestination:

https://github.com/bitcoin/bitcoin/blob/0fb52e481f411fdf67257965a9144522b4bac87c/src/wallet/wallet.cpp#L3294

Should there be a this->address = dest line after the GetReservedDestination call above?

Sjors commented at 4:16 pm on October 9, 2019:

I think the address property is unused, so we could just drop it: https://github.com/Sjors/bitcoin/commit/aa5ea3768a2858d7776d798edf63b421849b9714

achow101 commented at 5:22 pm on October 9, 2019:

While it may be unused, it may also be useful in the future with other ScriptPubKeyMans. I think it also makes more sense for ReserveDestination to explicitly have the destination stored as that was part of why that was added. I’ve added address = dest back in.

Sjors commented at 3:58 pm on October 9, 2019: member

I can confirm that git diff achow101/box-the-wallet ryanofsky/pr/keyman returns nothing for 0fb52e4 vs. 12b0b2bf15. ACK both.

A few nits in @ryanofsky’s version:

in 530d4d4 the This type of wallet does not support this command lines could be introduced in their own commit, preserving eff07eb9cca498e2942a417545938eb773dbc513
commit 530d4d4e08826f0d28e34e81ceaf341b58d29982 complains about a missing lock: https://gist.github.com/Sjors/2b5d6cb4a7df4d96217c11ffc1f97c6f. You can use --enable-debug again as of 8aa21da (Refactor: Move encryption code between KeyMan and Wallet).
I don’t have strong feelings about the last couple “Unclear if this change is necessary or useful” commits.

Would be nice to get one final rebase of #16528, to do some testing with descriptor wallets.

achow101 force-pushed on Oct 9, 2019

achow101 commented at 5:23 pm on October 9, 2019: member

I’m going to stick with the current commit structure.

ryanofsky referenced this in commit 3e0b0e476e on Oct 9, 2019

ryanofsky referenced this in commit 721b341079 on Oct 9, 2019

ryanofsky referenced this in commit 5afee37fdf on Oct 9, 2019

ryanofsky referenced this in commit 6cfb305e01 on Oct 9, 2019

ryanofsky referenced this in commit 6da50b7ab6 on Oct 9, 2019

ryanofsky referenced this in commit b4be0917a0 on Oct 9, 2019

ryanofsky referenced this in commit fa8f61cd37 on Oct 9, 2019

ryanofsky referenced this in commit d590e6b8d4 on Oct 9, 2019

ryanofsky commented at 8:17 pm on October 9, 2019: member

[Edited 10/12: Removed ACK]

Code review ACK 9556f59ad83690640a047192771600600c1550dc. Only changes since last review were suggested address and lock annotation fixes.

I also updated my branch. If any other reviewers struggling with this PR, I’d encourage them to use it to see a breakdown of changes in the main PR:

`pr/keyman`
3d38bf3d0c620755ff9ce9a53eb161e2d5ffd055 MOVEONLY: Move key handling code out of wallet to keyman file	+24 -6 ⇔1375
c80a4c7787ec84873e90a101cb4e5a99e4d797f0 Refactor: Split up CWallet and LegacyScriptPubKeyMan and classes	+590 -350 ⇔296
1529a4bc58035c3f1768830a4a7376086c93b9c6 MOVEONLY: Reorder LegacyScriptPubKeyMan methods	+4 -5 ⇔87
fdc32d358c38e8719f1249d42ab8f2f77d4ce270 Refactor: Declare LegacyScriptPubKeyMan methods as virtual	+21 -6 ⇔1
b4a321dd967124c9c52f68ae4b3e40a1f7861606 Refactor: Add new ScriptPubKeyMan virtual methods	+46 -8 ⇔0
46fe6e240acfe947ee8100ac9cdfcf9e6234a3ea Refactor: Move SetAddressBook call out of LegacyScriptPubKeyMan::GetNewDestination	+7 -4 ⇔2
5982116cd8db96e05782ecc5f7825cfd9ec04498 Refactor: Move SetWalletFlag out of LegacyScriptPubKeyMan::UpgradeKeyMetadata	+2 -2 ⇔0
9b121c57197d96ffdf79740b2b703b2aa89e53f2 Refactor: Remove UnsetWalletFlag call from LegacyScriptPubKeyMan::SetHDSeed	+2 -1 ⇔0
f7d1a7d50b78671e48f269030739d46ed75656d9 Refactor: Move SetAddressBookWithDB call out of LegacyScriptPubKeyMan::ImportScriptPubKeys	+6 -4 ⇔5
01dd97cb9f15f2ea8d86f86a4bf4be4a39972bbe Refactor: Move LoadKey LegacyScriptPubKeyMan method definition	+6 -1 ⇔0
dda6d03e4969e6b2a71b7f38c602d900c6faca49 Refactor: Move GetMetadata code out of getaddressinfo	+21 -8 ⇔5
00ee9e9467c66a634700bc134f21d83eb429354b Refactor: Move MarkUnusedAddresses code out of CWallet::AddToWalletIfInvolvingMe	+12 -5 ⇔14
80b3a0763f643af7d9c9be5ce6b8d6bd3c42938f Refactor: Move Upgrade code out of CWallet::CreateWalletFromFile	+24 -12 ⇔20
f733cbc54693d1a1efffbe0613d53d606c2464ad Refactor: Move HavePrivateKeys code out of CWallet::CreateWalletFromFile	+13 -2 ⇔0
0d5f789e6fc249dba4125e3d22a7c365b3440789 Refactor: Move SetupGeneration code out of CWallet	+22 -11 ⇔12
993d1f1a63d0cd837aab06a7dcb03272b5b02f03 Refactor: Move RewriteDB code out of CWallet	+20 -4 ⇔5
20b68aefd252c4273ea82d672ae1459274c3e48e Refactor: Move GetKeypoolSize code out of CWallet	+12 -3 ⇔8
ee77be668f61a9703e22d15cc14f60e68fb2b4e4 Refactor: Move nTimeFirstKey accesses out of CWallet	+16 -3 ⇔0
045f7de4f6d7d5e0aa927b726d277dccaee7fb5e Refactor: Move encryption code between KeyMan and Wallet	+67 -49 ⇔24
6a622f3e94193b5d8da19904a6c2ba7d1fe4b003 Locking: Lock cs_KeyStore instead of cs_main in legacy keyman	+55 -73 ⇔37
f2d0b6bf38f51d7a584db9ba0556981ab5051efc Refactor: Allow LegacyScriptPubKeyMan to be null	+72 -28 ⇔0
713ff35d756c2c082a8b0bfdd6789f9673bd07ed Refactor: Require scriptPubKey to get wallet SigningProvider	+101 -24 ⇔0
3e0b0e476e4ebf59d688af009900ed5cb8528221 Key pool: Move CanGetAddresses call	+2 -1 ⇔2
7126df82214e0d1e5ea47ce43de49b021f359070 Key pool: Move LearnRelated and GetDestination calls	+9 -8 ⇔0
721b34107984104e1f2b232016e456a7c76a05a6 Key pool: Move TopUp call	+0 -1 ⇔1
22524983096745e8df808efa57671c86f7d93245 Key pool: Change ReturnDestination interface to take address instead of key	+12 -14 ⇔0
df7f357fb81837edbf0e977ad319af773d10d5af Key pool: Make TopUp fail if unexpected wallet flags are set	+3 -0 ⇔0
5afee37fdfc10e7cd210e68f3052d1fb1ef658f5 Key pool: Fix omitted pre-split count in GetKeyPoolSize	+1 -1 ⇔0
a71dc57498345e8e506fc52bcb1f6723035f8082 IsMine: Set state to WATCH_ONLY if we can get the pubkey	+6 -2 ⇔0
6cfb305e01113325ec0811fea8a0104536cbf967 HD Split: Avoid redundant upgrades	+1 -1 ⇔0
4040cc10d67c87aa942f618213bb1195c7b4db4e Box the wallet: Add multiple keyman maps and loops	+161 -46 ⇔22
6394d8f92c67f180cb25352a36073cf563315760 Refactor: Copy CWallet signals and print function to LegacyScriptPubKeyMan	+22 -16 ⇔6
85b944bde87bcc54f0cfc25305dc2d8073a88941 Cleanup: Update message strings and comments	+4 -5 ⇔3
f25201cac1aa2e8d4191786177da9458c79a4e86 Cleanup: Drop unused GUI learnRelatedScripts method	+0 -5 ⇔0
6da50b7ab6b68c6607e9f9cfc78ea1bf7669d3f0 Refactor: Replace SigningProvider pointers with unique_ptrs	+45 -29 ⇔0
b4be0917a0ccf4fd844a462c4039ce7a760448c4 Refactor: Make LegacyScriptPubKeyMan::HasEncryptedData() call WalletStorage::HasEncryptionKeys()	+7 -7 ⇔0
fa8f61cd3748b3b5fa9b6ffbe33d9262248b127c Refactor: Add storage wrapper functions	+44 -33 ⇔0
d590e6b8d41499928910e13e4d2da9b8fd8a954d Refactor: Make WalletDatabase a shared_ptr instead of a unique_ptr	+28 -24 ⇔0

achow101 force-pushed on Oct 9, 2019

achow101 commented at 9:47 pm on October 9, 2019: member

While rebasing #16528, I noticed that we actually don’t need HasEncryptedData check both HasEncryptionKeys and mapCryptedKeys.empty(). It is sufficient to just check HasEncryptionKeys because having any encryption keys is a better indicator of encryption. So I’ve removed HasEncryptedData and the things that called it now call HasEncryptedData.

This also avoids some issues with locking and recursion from calling IsLocked.

ryanofsky referenced this in commit bd7c03d495 on Oct 10, 2019

ryanofsky referenced this in commit db94ae7b38 on Oct 10, 2019

ryanofsky referenced this in commit 1831faea1f on Oct 10, 2019

ryanofsky referenced this in commit 2382133e90 on Oct 10, 2019

ryanofsky referenced this in commit f887adad00 on Oct 10, 2019

ryanofsky referenced this in commit f3b11c61e7 on Oct 10, 2019

ryanofsky referenced this in commit d18e2cd864 on Oct 10, 2019

ryanofsky commented at 3:13 pm on October 10, 2019: member

[Edited 10/12: Removed ACK]

Code review ACK 39bd4db227d8c771502184389db97f9d7e7ac3c0. Only change since last review was removing HasEncryptedData.

I also updated my branch. If any other reviewers struggling with this PR, I’d encourage them to use it to see a breakdown of changes in the main PR:

`pr/keyman`
3d38bf3d0c620755ff9ce9a53eb161e2d5ffd055 MOVEONLY: Move key handling code out of wallet to keyman file	+24 -6 ⇔1375
c80a4c7787ec84873e90a101cb4e5a99e4d797f0 Refactor: Split up CWallet and LegacyScriptPubKeyMan and classes	+590 -350 ⇔296
1529a4bc58035c3f1768830a4a7376086c93b9c6 MOVEONLY: Reorder LegacyScriptPubKeyMan methods	+4 -5 ⇔87
fdc32d358c38e8719f1249d42ab8f2f77d4ce270 Refactor: Declare LegacyScriptPubKeyMan methods as virtual	+21 -6 ⇔1
b4a321dd967124c9c52f68ae4b3e40a1f7861606 Refactor: Add new ScriptPubKeyMan virtual methods	+46 -8 ⇔0
46fe6e240acfe947ee8100ac9cdfcf9e6234a3ea Refactor: Move SetAddressBook call out of LegacyScriptPubKeyMan::GetNewDestination	+7 -4 ⇔2
5982116cd8db96e05782ecc5f7825cfd9ec04498 Refactor: Move SetWalletFlag out of LegacyScriptPubKeyMan::UpgradeKeyMetadata	+2 -2 ⇔0
9b121c57197d96ffdf79740b2b703b2aa89e53f2 Refactor: Remove UnsetWalletFlag call from LegacyScriptPubKeyMan::SetHDSeed	+2 -1 ⇔0
f7d1a7d50b78671e48f269030739d46ed75656d9 Refactor: Move SetAddressBookWithDB call out of LegacyScriptPubKeyMan::ImportScriptPubKeys	+6 -4 ⇔5
01dd97cb9f15f2ea8d86f86a4bf4be4a39972bbe Refactor: Move LoadKey LegacyScriptPubKeyMan method definition	+6 -1 ⇔0
dda6d03e4969e6b2a71b7f38c602d900c6faca49 Refactor: Move GetMetadata code out of getaddressinfo	+21 -8 ⇔5
00ee9e9467c66a634700bc134f21d83eb429354b Refactor: Move MarkUnusedAddresses code out of CWallet::AddToWalletIfInvolvingMe	+12 -5 ⇔14
80b3a0763f643af7d9c9be5ce6b8d6bd3c42938f Refactor: Move Upgrade code out of CWallet::CreateWalletFromFile	+24 -12 ⇔20
f733cbc54693d1a1efffbe0613d53d606c2464ad Refactor: Move HavePrivateKeys code out of CWallet::CreateWalletFromFile	+13 -2 ⇔0
0d5f789e6fc249dba4125e3d22a7c365b3440789 Refactor: Move SetupGeneration code out of CWallet	+22 -11 ⇔12
993d1f1a63d0cd837aab06a7dcb03272b5b02f03 Refactor: Move RewriteDB code out of CWallet	+20 -4 ⇔5
20b68aefd252c4273ea82d672ae1459274c3e48e Refactor: Move GetKeypoolSize code out of CWallet	+12 -3 ⇔8
ee77be668f61a9703e22d15cc14f60e68fb2b4e4 Refactor: Move nTimeFirstKey accesses out of CWallet	+16 -3 ⇔0
c6bb1b333e5ceccfef6bcc6258ceb821c7d1e11f Refactor: Move encryption code between KeyMan and Wallet	+55 -49 ⇔19
54584860885425efdba2ff65e3463b63a665c732 Locking: Lock cs_KeyStore instead of cs_main in legacy keyman	+55 -73 ⇔37
b6eae875d35dd678465ae75afc5eb0bef4524f54 Refactor: Allow LegacyScriptPubKeyMan to be null	+72 -28 ⇔0
f0d7bf12681c67d65949192023527bd22a81a38c Refactor: Require scriptPubKey to get wallet SigningProvider	+101 -24 ⇔0
bd7c03d4957344c801f9f78a88e058d37e61924a Key pool: Move CanGetAddresses call	+2 -1 ⇔2
9193af03599c250c2290a3e66973d4956d83bd0a Key pool: Move LearnRelated and GetDestination calls	+9 -8 ⇔0
db94ae7b384299c76832d55d29e118abeda504db Key pool: Move TopUp call	+0 -1 ⇔1
11ddab8aac81337e548e3fcb94f8ee175326a465 Key pool: Change ReturnDestination interface to take address instead of key	+12 -14 ⇔0
b8ecc970ea4095aa6764847b70385c38d0cab7e8 Key pool: Make TopUp fail if unexpected wallet flags are set	+3 -0 ⇔0
1831faea1fbc98509c5b325efb2573f58a736928 Key pool: Fix omitted pre-split count in GetKeyPoolSize	+1 -1 ⇔0
7a6d930c21520e165fb0fed803e7b7131fab5f93 IsMine: Set state to WATCH_ONLY if we can get the pubkey	+6 -2 ⇔0
2382133e9067c5c564363b77d80ea22f92c62423 HD Split: Avoid redundant upgrades	+1 -1 ⇔0
01e94deafbe11dde66589e385a2933260f8feec4 Box the wallet: Add multiple keyman maps and loops	+159 -44 ⇔22
268bcd400439ba35c8ffeff5b4b8fa5de8f53ed7 Refactor: Copy CWallet signals and print function to LegacyScriptPubKeyMan	+22 -16 ⇔6
8ec2f6fabe72ef0f869fb1b93b722fd33d5dab8a Cleanup: Update message strings and comments	+4 -5 ⇔3
aee3a1e9dda36a5d98bc6850037dc48c10fc0e96 Cleanup: Drop unused GUI learnRelatedScripts method	+0 -5 ⇔0
f887adad00daf4b4f73a46050e484b37a7f46af6 Refactor: Replace SigningProvider pointers with unique_ptrs	+45 -29 ⇔0
f3b11c61e74f5f6bd12f417d3f50a5f98472b348 Refactor: Add storage wrapper functions	+49 -38 ⇔0
d18e2cd8642a6b4e73be0482ef19f76698f1ef62 Refactor: Make WalletDatabase a shared_ptr instead of a unique_ptr	+28 -24 ⇔0

Sjors commented at 5:44 pm on October 10, 2019: member

I tested 39bd4db227d8c771502184389db97f9d7e7ac3c0 with the follow descriptor wallet PR.

Are you sure your last change handles non-HD wallets correctly? IIUC mapCryptedKeys is non-empty, but mapMasterKeys is, so HasEncryptionKeys() will return false. I tested with an encrypted -usehd=0 wallet from v0.15.2 and can’t find any problems though.

achow101 commented at 5:46 pm on October 10, 2019: member

I tested 39bd4db with the follow descriptor wallet PR.

Are you sure your last change handles non-HD wallets correctly? IIUC mapCryptedKeys is non-empty, but mapMasterKeys is, so HasEncryptionKeys() will return false. I tested with an encrypted -usehd=0 wallet from v0.15.2 and can’t find any problems though.

Huh? Encryption has nothing to do with HD wallets. mapMasterKeys is named poorly and refers to encryption keys, not HD master keys. We don’t even use HD master keys, we an HD seed which then becomes a master key for key derivation.

Sjors commented at 6:14 pm on October 10, 2019: member

Ah yes, it’s the master encryption key map (typedef std::map<unsigned int, CMasterKey> MasterKeyMap), not the master hd key (CHDChain).

ACK 39bd4db. I like that with @ryanofsky’s alternative history we now have a DAG :-) If you can link to it in the PR description, then it should be preserved.

in src/wallet/scriptpubkeyman.h:169 in 39bd4db227 outdated

159+
160+    virtual ~ScriptPubKeyMan() {};
161+    virtual bool GetNewDestination(const OutputType type, CTxDestination& dest, std::string& error) { return false; }
162+    virtual isminetype IsMine(const CScript& script) const { return ISMINE_NO; }
163+
164+    virtual bool CheckDecryptionKey(const CKeyingMaterial& master_key, bool accept_no_keys = false) { return false; }

instagibbs commented at 8:34 pm on October 11, 2019:

nit: Check for what about the decryption check?

achow101 commented at 9:47 pm on October 11, 2019:

Added a comment

in src/wallet/wallet.cpp:3977 in 39bd4db227 outdated

4009-    }
4010-    return true;
4011-}
4012-
4013-bool CWallet::AddKeyOriginWithDB(WalletBatch& batch, const CPubKey& pubkey, const KeyOriginInfo& info)
4014+bool CWallet::IsCrypted() const

instagibbs commented at 8:55 pm on October 11, 2019:

future cleanup: Redundant with HasEncryptionKeys

in src/wallet/scriptpubkeyman.h:19 in 39bd4db227 outdated

14+
15+#include <boost/signals2/signal.hpp>
16+
17+enum class OutputType;
18+
19+// Wallet storage things that ScriptPubKeyMans need in order to be able to store things to the wallet database

instagibbs commented at 8:57 pm on October 11, 2019:

CWallet also needs this apparently?

instagibbs commented at 9:00 pm on October 11, 2019:

Can this explanation be made more concrete? Why are these things set here versus elsewhere in ScriptPubKeyMans/CWallet?

“storage” vs “database”, why does the database in ScriptPubKeyMan and referenced inside m_storage as well?

ryanofsky commented at 9:30 pm on October 11, 2019:

re: #16341 (review)

“storage” vs “database”, why does the database in ScriptPubKeyMan and referenced inside m_storage as well?

There isn’t a good reason for this. I marked the change adding m_database as one of the “Unclear if this change is necessary or useful” changes in my branch: d18e2cd8642a6b4e73be0482ef19f76698f1ef62

instagibbs commented at 9:31 pm on October 11, 2019:

The underlying relations were explained to me offline. Incoming expanded comment hopefully.

achow101 commented at 9:49 pm on October 11, 2019:

Expanded the comment.

in src/wallet/wallet.h:585 in 39bd4db227 outdated

584@@ -726,34 +585,12 @@ class WalletRescanReserver; //forward declarations for ScanForWalletTransactions
585  * A CWallet is an extension of a keystore, which also maintains a set of transactions and balances,

instagibbs commented at 8:58 pm on October 11, 2019:

non-blocking nit: No longer an extension of a keystore, which isn’t found anywhere anymore

achow101 commented at 9:49 pm on October 11, 2019:

Shortened.

instagibbs commented at 9:09 pm on October 11, 2019: member

A few clarifying questions and some non-blocking nits, while I go look at descriptor wallet PR…

“WalletStorage” is the new thing for me.

re-reviewed all the way to https://github.com/bitcoin/bitcoin/commit/39bd4db227d8c771502184389db97f9d7e7ac3c0

instagibbs commented at 9:32 pm on October 11, 2019: member

utACK https://github.com/bitcoin/bitcoin/commit/39bd4db227d8c771502184389db97f9d7e7ac3c0

contingent on expanded explanation of role of WalletStorage between the various concepts

achow101 force-pushed on Oct 11, 2019

achow101 force-pushed on Oct 12, 2019

achow101 commented at 0:44 am on October 12, 2019: member

There was a hidden merge conflict with master, so I had to rebase this. resolving that conflict required changing the WatchOnlyPubKeys test case in the wallet tests to use LegacyScriptPubKeyMan,

in src/wallet/scriptpubkeyman.h:19 in e418a29df3 outdated

14+
15+#include <boost/signals2/signal.hpp>
16+
17+enum class OutputType;
18+
19+// Wallet storage things that ScriptPubKeyMans need in order to be able to store things to the wallet database.

instagibbs commented at 12:22 pm on October 12, 2019:

“Wallet storage for things”

achow101 commented at 4:38 pm on October 12, 2019:

I think it is correct as is. It’s things for wallet storage that ScriptPubKeyMans need.

ryanofsky commented at 2:35 pm on October 12, 2019: member

Apologies to @achow101 because the work here is great and very promising, but I want to remove my ACK from this PR. With Sjors and Instagibbs ACKs coming so quickly after mine, it seems possible my review might be being given more weight than it warrants, and I want to avoid this possibility.

Normally when I ACK something I want to at least be able to say “I’d be surprised if this PR introduced a bug”, but with all the changes broken out, and several I haven’t deeply looked into, I might be more surprised if this PR didn’t introduce any bugs than if it did.

I think it’d be fine to merge this PR with reviews and ACKs from other people. And personally I’d feel comfortable adding my ACK to smaller, more broken down PRs. But just given size of this PR, the time I’ve spent on it, and my level of understanding, I don’t think my review should count for that much and I want to make sure this change gets in depth review and more confident ACKs from other people if it will be merged.

Sjors commented at 2:56 pm on October 12, 2019: member

@ryanofsky note that my ACK was mostly based on my own previous review (i.e. @achow101’s commits). I used your variant to sanity check. But I agree this PR needs more people looking at it. Lots of moving parts. More tests would also be useful, because a lot of things can be changed without breaking any. @meshcollider mentioned on IRC he’s also in the process of reviewing.

Confirming that e418a29 is a rebase of my previous ACK, but I haven’t re-tested it. It takes into account the new bech32 default, the new watch-only tests added in #16786 and adds a bunch of comments.

instagibbs commented at 3:22 pm on October 12, 2019: member

There were only a couple significant changes since my last ACK. I only reviewed after Russel because I didn’t feel like reviewing 60 different versions I do agree it needs more ACKs than typical due to size of changes.

On Sat, Oct 12, 2019, 10:59 AM Sjors Provoost notifications@github.com wrote:

@ryanofsky https://github.com/ryanofsky note that my ACK was mostly based on my own previous review. I used your variant to sanity check. But I agree this PR needs more people looking at it. Lots of moving parts.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/16341?email_source=notifications&email_token=ABMAFUZLFL666DUPQUPBF3LQOHQ43A5CNFSM4H6EPYF2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBCBFKQ#issuecomment-541332138, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABMAFU3XNAG7XFM7NJ3ZLTDQOHQ43ANCNFSM4H6EPYFQ .

ryanofsky referenced this in commit e425848214 on Oct 15, 2019

ryanofsky referenced this in commit e27c8409b6 on Oct 15, 2019

ryanofsky referenced this in commit 6ff45aaaa6 on Oct 15, 2019

ryanofsky referenced this in commit efff76918a on Oct 15, 2019

ryanofsky referenced this in commit 34a2abc2b0 on Oct 15, 2019

ryanofsky referenced this in commit 362484c7d1 on Oct 15, 2019

ryanofsky referenced this in commit d5dd89fdc2 on Oct 15, 2019

in src/wallet/wallet.cpp:4955 in e3efac9239 outdated

4948@@ -4949,3 +4949,35 @@ bool CWallet::AddCryptedKeyInner(const CPubKey &vchPubKey, const std::vector<uns
4949     ImplicitlyLearnRelatedKeyScripts(vchPubKey);
4950     return true;
4951 }
4952+
4953+LegacyScriptPubKeyMan* CWallet::GetLegacyScriptPubKeyMan() const
4954+{
4955+    // All active ScriptPubKeyMans are the same LegacyScriptPubKeyMan

meshcollider commented at 7:08 am on October 16, 2019:

This comment isn’t the clearest, its only right now that this is true. But I guess you’re changing this soon in the other PR anyway.

achow101 commented at 4:43 am on October 17, 2019:

I’ve updated the comment to be a bit clearer. I don’t think it will be changed with descriptor wallets.

in src/wallet/scriptpubkeyman.h:23 in a9f852f494 outdated

18+/*
19+ * A class implementing ScriptPubKeyMan manages some (or all) scriptPubKeys used in a wallet.
20+ * It contains the scripts and keys related to the scriptPubKeys it manages.
21+ * A ScriptPubKeyMan will be able to give out scriptPubKeys to be used, as well as marking
22+ * when a scriptPubKey has been used. It also handles when and how to store a scriptPubKey
23+ * and it's related scripts and keys, including encryption.

meshcollider commented at 7:10 am on October 16, 2019:

nit: its

achow101 commented at 4:44 am on October 17, 2019:

Fixed.

in src/wallet/scriptpubkeyman.h:151 in d89011ffca outdated

51+    std::shared_ptr<WalletDatabase> m_database;
52+
53 public:
54+    ScriptPubKeyMan(WalletStorage& storage) : m_storage(storage), m_database(storage.GetDatabase()) {}
55+
56+    const std::string GetDisplayName() const { return m_storage.GetDisplayName(); }

meshcollider commented at 7:12 am on October 16, 2019:

All of these are properties of the wallet itself as you describe in the comment above. Why make them accessible from each scriptpubkeymanager? (d89011ffca9144ddd28e53177d5e722fadaf2164 Add WalletStorage class and have CWallet inherit it)

Sjors commented at 9:02 am on October 25, 2019:

@meshcollider several of these (protected) methods are used by each scriptpubkeymanager. E.g. IsWalletFlagSet() is checked in LegacyScriptPubKeyMan::UpgradeKeyMetadata(). UnsetWalletFlagWithDB() is used to mark a wallet as no longer blank when a key is added. GetDisplayName() and SetWalletFlag are unused, but I’m fine with keeping them for completeness.

ryanofsky commented at 12:57 pm on October 25, 2019:

re: #16341 (review)

@meshcollider several of these (protected) methods are used by each scriptpubkeymanager. E.g. IsWalletFlagSet() is checked in LegacyScriptPubKeyMan::UpgradeKeyMetadata(). UnsetWalletFlagWithDB() is used to mark a wallet as no longer blank when a key is added. GetDisplayName() and SetWalletFlag are unused, but I’m fine with keeping them for completeness.

I think it would be best to eliminate all these methods because they obscure the difference between wallet and key manager objects and between storage and key management functionality. They also make call sites less clear about what the calls are intended to do.

In my pr/keyman branch I’ve marked the change adding these methods (2d08c308a3d9936c389c86786e666c0b71c81052) as one of the questionable “Unclear if this change is necessary or useful” changes and put it at the end of the branch so it could be easily dropped or reverted.

Sjors commented at 2:06 pm on October 25, 2019:

I don’t have strong feelings on whether the left or right side of 2d08c30 is more clear. Explicit references to m_storage might discourage relying on it more; is the long term goal to get rid of m_storage usage in ScriptPubKeyMans?

ryanofsky commented at 2:48 pm on October 25, 2019:

is the long term goal to get rid of m_storage usage in ScriptPubKeyMans?

This would be a surprise to me. As far as I know, key managers will continue to need a way to read and store data in the wallet database, and WalletStorage provides a limited and controlled interface for doing that.

Even if the wallet storage interface is going to be eliminated or cut back at some point, having wrapper functions that imply a key manager is storing its own data now when it isn’t muddles the code and adds pointless hops in the meantime.

I don’t think this is an important issue at all. Just surprised if getting rid of the indirection here wouldn’t be the obvious choice.

achow101 commented at 3:53 pm on October 25, 2019:

They can be removed in a follow up PR. The intention of wrapping up each function was to make the moves less painful and more clear that they were moves.

in src/wallet/scriptpubkeyman.cpp:911 in ff90cccd99 outdated

466+        throw std::runtime_error(std::string(__func__) + ": AddKey failed");
467+    }
468+    return pubkey;
469+}
470+
471+const uint32_t BIP32_HARDENED_KEY_LIMIT = 0x80000000;

meshcollider commented at 8:00 am on October 16, 2019:

This constant is used elsewhere in the code too (e.g. BIP32PubkeyProvider), it would be good to factor it out everywhere (not important for this PR though)

achow101 commented at 4:30 am on October 17, 2019:

Yes, should be cleaned up in a follow up.

in src/wallet/scriptpubkeyman.cpp:1004 in d9e9ae8fff outdated

601+    CKey key;
602+    key.MakeNewKey(true);
603+    return DeriveNewSeed(key);
604+}
605+
606+CPubKey LegacyScriptPubKeyMan::DeriveNewSeed(const CKey& key)

meshcollider commented at 10:07 am on October 16, 2019:

I don’t really understand why this is a function in LegacyScriptPubKeyMan. IMO these should be moved out into standalone functions and just SetHDSeed should be part of the LegacyScriptPubKeyMan. Or just combine these functions with SetHDSeed so it derives+sets at the same time.

achow101 commented at 4:21 am on October 17, 2019:

They were originally together but later split apart to allow for the sethdseed RPC. I suppose it should be DeriveAndSetHDSeed which calls SetHDSeed. But since this is moved from how it is currently, I will leave it as is.

in src/wallet/scriptpubkeyman.cpp:1264 in 23ea5cfafd outdated

903+void LegacyScriptPubKeyMan::MarkReserveKeysAsUsed(int64_t keypool_id)
904+{
905+    AssertLockHeld(cs_KeyStore);
906+    bool internal = setInternalKeyPool.count(keypool_id);
907+    if (!internal) assert(setExternalKeyPool.count(keypool_id) || set_pre_split_keypool.count(keypool_id));
908+    std::set<int64_t> *setKeyPool = internal ? &setInternalKeyPool : (set_pre_split_keypool.empty() ? &setExternalKeyPool : &set_pre_split_keypool);

meshcollider commented at 10:24 am on October 16, 2019:

This could be simplified for readability

achow101 commented at 4:22 am on October 17, 2019:

Maybe for a followup? Would prefer to leave this as a move

in src/wallet/scriptpubkeyman.cpp:1273 in 23ea5cfafd outdated

912+    while (it != std::end(*setKeyPool)) {
913+        const int64_t& index = *(it);
914+        if (index > keypool_id) break; // set*KeyPool is ordered
915+
916+        CKeyPool keypool;
917+        if (batch.ReadPool(index, keypool)) { //TODO: This should be unnecessary

meshcollider commented at 10:30 am on October 16, 2019:

Why is this TODO here?

achow101 commented at 4:22 am on October 17, 2019:

It was moved from the original implementation.

in src/wallet/scriptpubkeyman.cpp:13 in e99778c18c outdated

2@@ -3,11 +3,24 @@
3 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
4 
5 #include <key_io.h>
6+#include <outputtype.h>
7 #include <wallet/scriptpubkeyman.h>
8 
9 bool LegacyScriptPubKeyMan::GetNewDestination(const OutputType type, CTxDestination& dest, std::string& error)

meshcollider commented at 10:40 am on October 16, 2019:

The commit title is wrong, Address -> Destination

in src/wallet/wallet.cpp:1656 in 7bfed77bbe outdated

1661+    if (m_spk_managers.empty()) return false;
1662+    bool result = false;
1663+    for (OutputType t : OUTPUT_TYPES) {
1664+        auto spk_man = GetScriptPubKeyMan(t, internal);
1665+        if (spk_man && spk_man->CanGetAddresses(internal)) {
1666+            result = true;

meshcollider commented at 11:23 am on October 16, 2019:

why not just return true and get rid of result

achow101 commented at 4:44 am on October 17, 2019:

Done. Got rid of result.

DrahtBot added the label Needs rebase on Oct 16, 2019

in src/wallet/scriptpubkeyman.cpp:165 in c2bdabae3e outdated

163@@ -122,7 +164,36 @@ bool LegacyScriptPubKeyMan::CanGetAddresses(bool internal)
164 
165 bool LegacyScriptPubKeyMan::Upgrade(int prev_version, int new_version, std::string& error)

meshcollider commented at 3:23 am on October 17, 2019:

new_version isn’t used? What situation did you have in mind for this? I haven’t checked to see if this changes in descriptor wallets yet so I won’t be surprised if its here for the future, but it seems weird not to check it before upgrading to HD (new_version = prev_version should be a noop)

achow101 commented at 4:25 am on October 17, 2019:

IIRC I had that there originally for upgrading a legacy wallet to a descriptor wallet. But since we are no longer doing that, it can be removed. I believe it was going to be used in combination with #15761 (or rather #15761 was split from an earlier implementation).

achow101 commented at 4:46 am on October 17, 2019:

Removed new_version.

For anyone who is wondering why this is works and/or is safe, the new version is set as the wallet maximum version so all of the CanSupportFeature calls will fail if the specified new version is too low.

meshcollider commented at 4:11 am on October 17, 2019: contributor

Sorry for not doing this sooner, I’ve finally finished reviewing it. Awesome work Andrew. ACK e418a29df300fc2d33ac333b00579f577d2bdf5c without building GUI

3dc782fb556b9e65b9c35977c1bc8bd94cee671f Move wallet enums to walletutil.h
223882da23084a8206415b0b2a238c618654a718 List output types in an array in order to be iterated over
4e0b7dd5946a10d4b809702104c9f5486800b3f5 Always try to sign for all pubkeys in multisig
0e83a5d43fe95be7bcf251791cbef015c751b85e Store p2sh scripts in AddAndGetDestinationForScript
a9f852f494b1c6f72333aa15770b3e119080e578 Introduce both ScriptPubKeyMan as an interface and LegacyScriptPubKeyMan as a dummy class
e3efac92393d6c366c0a6be7561eb2a3c54855e0 Add LegacyScriptPubKeyMan to CWallet
d89011ffca9144ddd28e53177d5e722fadaf2164 Add WalletStorage class and have CWallet inherit it
790f8398db115d53e521aeed4cde3b192a0c3a14 Fetch the SigningProvider for a script from the wallet
dc0611244f18a4cac814f29f2951e46e43a89bba Fetch the ScriptPubKeyMan for given output type and internal-ness, or a given script, or ScriptPubKeyMan id
274340f8234a124612fdd9d7100cc8d3a8191184 Implement GetSigningProvider in LegacyScriptPubKeyMan
b79da31e8c4f2920a28c297d4554df7564cdcee9 Implement function to connect ScriptPubKeyMan’s NotifyCanGetAddessesChanged and NotifyWatchOnlyChanged to CWallet’s
da1a68fb28b1c85cbbf1981f71f316901222de22 Implement HasEncryptedData, SetCrypted, LoadCryptedKey, and AddCryptedKey in LegacyScriptPubKeyMan
713cf419d51837ca17a44bc16069a9b43ec93c04 Implement UpdateTimeFirstKey, and GetTimeFirstKey in LegacyScriptPubKeyMan
e7254f0f2e02f764ce5fe26ff35be1499a441bfb Implement AddWatchOnly, RemoveWatchOnly, HaveWatchOnly, and LoadWatchOnly in LegacyScriptPubKeyMan
dd59ea1c78848766f0166c251ce83ae8d6d3eabf Implement AddKeyPubKey and LoadKey in LegacyScriptPubKeyMan
5343a9f0273b25fdd8d236d31b78aa039216bbf4 Implement WalletLogPrintf in LegacyScriptPubKeyMan
ab9d81eaecfbd02e0af8c8582094dd6bcefdf48a Implement SetHDCHain, and IsHDEnabled in LegacyScriptPubKeyMan
fc136aab3366e5a41304e54526b59f144deaa2b2 Implement LoadCScript in LegacyScriptPubKeyMan
0e71f718133b2e0bf7a7965692c6433bb328b229 Implement LoadKeyMetadata and LoadScriptMetadata in LegacyScriptPubKeyMan
fe1ebe542c3d8f8e44c86f6fdc0aa0d4c54a544d Implement GetKey, HaveKey, and GetPubKey in LegacyScriptPubKeyMan
ff90cccd99705a1d1f163fa99f7b3b314ad42695 Implement GenerateNewKey in LegacyScriptPubKeyMan
1fedc29d7e4c8828939d3c71da3750f12ef7e0c4 Implement LoadKeyPool in LegacyScriptPubKeyMan
e22f2616be40eeb9b554e8dc44bb650d6dea30a4 Implement GetOldestKeyPoolTime, KeypoolCountExternalKeys, and GetKeypoolSize in LegacyScriptPubKeyMan
d7eb32b58abdb782b2b770eab25e83806915deff Implement CanGetAddresses, CanGenerateKeys, and HavePrivateKeys in LegacyScriptPubKeyMan
d9e9ae8fffc51accaa7c8ac59bf2732d81f8cde4 Implement GenerateNewSeed, DeriveNewSeed, and SetHDSeed for LegacyScriptPubKeyMan
c836a6cad69dc54b0c4a5f2cf3b0de9b249fa8ad Implement TopUpKeypool, TopUp, and NewKeyPool in LegacyScriptPubKeyMan
a25d83f4c2abb5d58c64b3ee5ba5be6df04cf381 Implement ReturnAddress, and KeepKey in LegacyScriptPubKeyMan
e99778c18ceb0809e4a68ea5688e6c26aba14294 Implement GetNewAddress, and GetReservedAddress in LegacyScriptPubKeyMan
23ea5cfafd01f202573d6915c4b901caf8fa160d Implement MarkUnusedAddresses in LegacyScriptPubKeyMan
bf80f299b8544a4330b5b468495e71ecfd7b44e9 Implement IsMine in LegacyScriptPubKeyMan
c2bdabae3e98b3a7e1b2795f66a202fc5c0b60cc Implement UpgradeKeyMetaData, SetupGeneration, IsFirstRun, Upgrade, RewriteDB in LegacyScriptPubKeyMan
c5895f35a998664141beda2f0c4a58a3969f954e Implement Unlock, Lock, and Encrypt and LegacyScriptPubKeyMan
9c4ba789aa6a671045ebf37d7b566a147bf6ac09 Implement ImporScripts, ImportPrivKeys, ImportPubKeys, and ImportScriptPubKeys in LegacyScriptPubKeyMan
e2c664d2e6d3739256a9706958c26cedef978c4e Implement GetMetadata in LegacyScriptPubKeyMan
0b8af682533d0e766ea7e6b951c6217a557e6af8 Implement GetKeyOrigin in LegacyScriptPubKeyMan
8972801f344fc2366987bfd8668a06dd56546061 Implement actually loading everything into LegacyScriptPubKeyMan
fb60065c64617ff1ec3914ffd9de31c8e2af02c7 Implement CanProvide in LegacyScriptPubKeyMan
b77a6774fe690cf51f660bcc40b85cbdfe4e4464 Implement GetKeys in LegacyScriptPubKeyMan
81bcdc126ac73dccd077e1ce9fec6278ddea3128 [ci skip] Remove CWallet from IsMine and have CWallet always use ScriptPubKeyMan’s IsMine
5293b57fd5bbcb114f2ddda5630fa5bd21dfbb59 [ci skip] moveonly: move ismine stuff to be a module of LegacyScriptPubKeyMan
4821f3b52c702ba06ae6ef626e8eb2ab1792ec7b [ci skip] IsMine set state to WATCH_ONLY if we can get the pubkey
404f182bda8977f9abc573584acdce81e6fb219c [ci skip] Have GetNewAddress, GetNewChangeAddress, and ReserveAddress use ScriptPubKeyMan
890db7a782e598bdfc75b0fa9768f9be7442a28a [ci skip] Mark used addresses in ScriptPubKeyMan
458c2449fc1d113afe3694b7a3eb37a2b9b2e9bc [ci skip] Call UpgradeKeyMetaData for each ScriptPubKeyMan
fa5f552cbee969c8d86a5239e9e8d65ddefb764c [ci skip] Sign using SigningProvider from ScriptPubKeyMan when signing within CWallet
6c5b3187aac3a033e512979009145436feaa1823 [ci skip] Do not allow import*, dump*, and addmultisigaddress RPCs when wallet is not backed by LegacyScriptPubKeyMan
1ba7d942f45eb1997f0e036acff2b85351ff8e29 [ci skip] Change Imports to use LegacyScriptPubKeyMan Imports
f853ad2af5ad2fad9a54fa37c7ae0d8846a5cf6f [ci skip] Use SigningProviders and ScriptPubKeyMans in listunspent, signmessage, signrawtransactionwithwallet, and getaddressinfo
f44fe2573459a32106a5af7e1edbad8e90365a28 [ci skip] Use LegacyScriptPubKeyMan in addmultisigaddress and sethdseed
4791b5e33df28a593f0e47d4983fd56f5dbe938f [ci skip] Use LegacyScriptPubKeyMan for hdseedid in getwalletinfo
ba2740be33f77fc6ad02a9069032117fa8e89efa [ci skip] Change KeypoolCountExternal and GetKeypoolSize to get aggregate sizes from ScriptPubKeyMans
de8036ecde165b32df2671db0fa0fbb2339d6e19 [ci skip] Have IsHDEnabled fetch from ScriptPubKeyMans
22e10490b3eb51096d47f98b1aee5fa2143bf559 [ci skip] Fetch oldest keypool time from ScriptPubKeyMans
83748acf031436dceb3c0eeb60c65a2aadb05592 [ci skip] have TopUpKeyPool call TopUp in each ScriptPubKeyMan
02b7221e42d9a081d21aab6b03d1ebdbdc1bdcd6 [ci skip] Have EncryptWallet, Lock, and Unlock call their respective functions in ScriptPubKeyMans
cb719eb88af5c88579c7ad8faa3d83fbf26d6e5c [ci skip] Use LegacyScriptPubKeyMan throughout psbt_wallet_tests
f493b0a12343385da5ea5e29a934be740819bd86 [ci skip] Use LegacyScriptPubKeyMan throughout wallettool
f9144d83d97ea6f1207d080662f504c63f9d9408 [ci skip] Use ScriptPubKeyMans’ Setup and Upgrade functions when loading or creating a wallet
018a77ab490923c433563ca3552ea13147a85f3d [ci skip] Define first run as having no ScriptPubKeyMans
f43d480b24076a763f61689159e42da801fe43a3 [ci skip] Use RewriteDB action when DB needs rewrite
2d108185054a45f68230d8e641df15a9a12bce3a [ci skip] Use GetTimeFirstKey instead of nTimeFirstKey
208dd51e4b8e0abd062d21ccc12e57a3d4289b41 [ci skip] Use LegacyScriptPubKeyMan for in wallet_tests
ccd70050f970c099a6d7fd6cdbc9d1ecde82f426 [ci skip] Use LegacyScriptPubKeyMan in dumpprivkey and dumpwallet
7bfed77bbe6b09aded26a053b5e7651cc1b86bcd [ci skip] Change CanGetAddresses to fetch from ScriptPubKeyMan
0e5177d2c85a34b4e8596a20bc0e21927e027bac [ci skip] Fetch the correct SigningProvider for signing PSBTs
312d3610b0bd0a7ef812ace9e30cb9c9e9d000a1 [ci skip] Use LegacyScriptPubKeyMan in test util
ac65f820104f3c6973a1718a07f1003dcf2d8793 [ci skip] Use LegacyScriptPubKeyMan in some parts of getbalances and createwallet
0442e54abf075b7bf9bb3a7522406085e2253f1e [ci skip] Have getPubKey and getPrivKey use SigningProvider
6622ca6dbde936ad1436afe404165870412bcace [ci skip] Use LegacyScriptPubKeyMan in benchmarks involving the wallet
e418a29df300fc2d33ac333b00579f577d2bdf5c Remove unused functions and switch CWallet to use ScriptPubKeyMan

With reviews from instagibbs, Sjors, ryanofsky and myself, I think this is fairly ready to merge. I know promag said on IRC that he is currently reviewing it too so I will wait for him. A review from @jnewbery or @sipa would be awesome but this is a large PR so understandable if they don’t have the time at the moment. With 0.19 only just branched off I think its good to merge this soon, with plenty of time before 0.20, so that bugs can be found and squashed long before the next release.

achow101 force-pushed on Oct 17, 2019

DrahtBot removed the label Needs rebase on Oct 17, 2019

achow101 force-pushed on Oct 17, 2019

achow101 commented at 5:50 am on October 17, 2019: member

Latest push adds #include <functional> to util/translation.h as it was causing some travis builds to fail for some reason.

DrahtBot added the label Needs rebase on Oct 21, 2019

Move wallet enums to walletutil.h f44a1f4202

List output types in an array in order to be iterated over 3a5fb0135d

Always try to sign for all pubkeys in multisig 36eaf9e739

Store p2sh scripts in AddAndGetDestinationForScript 67891ae7f3

Introduce both ScriptPubKeyMan as an interface and LegacyScriptPubKeyMan as a dummy class fb7647a6fc

Add LegacyScriptPubKeyMan to CWallet 43c43ff4d4

Add WalletStorage class and have CWallet inherit it b363de355d

Fetch the SigningProvider for a script from the wallet ee6bacd7b5

Fetch the ScriptPubKeyMan for given output type and internal-ness, or a given script, or ScriptPubKeyMan id

Co-authored-by: Hugo Nguyen <hugh.hn@gmail.com>

f4958a4ee7

Implement GetSigningProvider in LegacyScriptPubKeyMan d8831ee5e0

Implement function to connect ScriptPubKeyMan's NotifyCanGetAddessesChanged and NotifyWatchOnlyChanged to CWallet's 1557bb64a8

Implement HasEncryptedData, SetCrypted, LoadCryptedKey, and AddCryptedKey in LegacyScriptPubKeyMan 3946efcde6

Implement UpdateTimeFirstKey, and GetTimeFirstKey in LegacyScriptPubKeyMan d67b8f41fe

Implement AddWatchOnly, RemoveWatchOnly, HaveWatchOnly, and LoadWatchOnly in LegacyScriptPubKeyMan e333336503

Implement AddKeyPubKey and LoadKey in LegacyScriptPubKeyMan 4626daacd4

Implement WalletLogPrintf in LegacyScriptPubKeyMan 8bfe9e5bbe

Implement SetHDCHain, and IsHDEnabled in LegacyScriptPubKeyMan a6d1b2d5ba

Implement LoadCScript in LegacyScriptPubKeyMan 3900abb6e3

Implement LoadKeyMetadata and LoadScriptMetadata in LegacyScriptPubKeyMan 642a802b36

Implement GetKey, HaveKey, and GetPubKey in LegacyScriptPubKeyMan 739ace8144

Implement GenerateNewKey in LegacyScriptPubKeyMan 0161c0c20f

Implement LoadKeyPool in LegacyScriptPubKeyMan 96801f0d56

Implement GetOldestKeyPoolTime, KeypoolCountExternalKeys, and GetKeypoolSize in LegacyScriptPubKeyMan 84b294259c

Implement CanGetAddresses, CanGenerateKeys, and HavePrivateKeys in LegacyScriptPubKeyMan 62d57108b8

Implement GenerateNewSeed, DeriveNewSeed, and SetHDSeed for LegacyScriptPubKeyMan b88b27d0c3

Implement TopUpKeypool, TopUp, and NewKeyPool in LegacyScriptPubKeyMan cc11df1843

Implement ReturnAddress, and KeepKey in LegacyScriptPubKeyMan 8653db5ed5

Implement GetNewDestination, and GetReservedDestination in LegacyScriptPubKeyMan 334bb17340

Implement MarkUnusedAddresses in LegacyScriptPubKeyMan e15037cbf9

Implement IsMine in LegacyScriptPubKeyMan 0045e372e7

Implement UpgradeKeyMetaData, SetupGeneration, IsFirstRun, Upgrade, RewriteDB in LegacyScriptPubKeyMan 666ff362b6

Implement Unlock, Lock, and Encrypt and LegacyScriptPubKeyMan 8550e365ca

Implement ImportScripts, ImportPrivKeys, ImportPubKeys, and ImportScriptPubKeys in LegacyScriptPubKeyMan d1834ac064

Implement GetMetadata in LegacyScriptPubKeyMan dc2642e1de

Implement GetKeyOrigin in LegacyScriptPubKeyMan 335ede966b

Implement actually loading everything into LegacyScriptPubKeyMan 94d0ea0ebc

Implement CanProvide in LegacyScriptPubKeyMan 4cd8392f29

Implement GetKeys in LegacyScriptPubKeyMan 280e8c6b61

[ci skip] Remove CWallet from IsMine and have CWallet always use ScriptPubKeyMan's IsMine cec6828cc1

[ci skip] moveonly: move ismine stuff to be a module of LegacyScriptPubKeyMan 150dbffde5

[ci skip] IsMine set state to WATCH_ONLY if we can get the pubkey 7dc1b6e221

[ci skip] Have GetNewDestination, GetNewChangeDestination, and ReserveDestination use ScriptPubKeyMan af0f740ae9

[ci skip] Mark used addresses in ScriptPubKeyMan 8e736f1092

[ci skip] Call UpgradeKeyMetaData for each ScriptPubKeyMan b3e712a72b

[ci skip] Sign using SigningProvider from ScriptPubKeyMan when signing within CWallet 95e9343b0d

[ci skip] Do not allow import*, dump*, and addmultisigaddress RPCs when wallet is not backed by LegacyScriptPubKeyMan 9417a68047

[ci skip] Change Imports to use LegacyScriptPubKeyMan Imports 86c05b10ae

[ci skip] Use SigningProviders and ScriptPubKeyMans in listunspent, signmessage, signrawtransactionwithwallet, and getaddressinfo 3bf4cd4245

[ci skip] Use LegacyScriptPubKeyMan in addmultisigaddress and sethdseed ecc4371c46

[ci skip] Use LegacyScriptPubKeyMan for hdseedid in getwalletinfo d03ed2c77f

[ci skip] Change KeypoolCountExternal and GetKeypoolSize to get aggregate sizes from ScriptPubKeyMans 473df837aa

[ci skip] Have IsHDEnabled fetch from ScriptPubKeyMans 921261e245

[ci skip] Fetch oldest keypool time from ScriptPubKeyMans bf8491c5f0

[ci skip] have TopUpKeyPool call TopUp in each ScriptPubKeyMan bdfac28a22

[ci skip] Have EncryptWallet, Lock, and Unlock call their respective functions in ScriptPubKeyMans da4ca24ced

[ci skip] Use LegacyScriptPubKeyMan throughout psbt_wallet_tests 6fa358591c

[ci skip] Use LegacyScriptPubKeyMan throughout wallettool 6480910fba

[ci skip] Use ScriptPubKeyMans' Setup and Upgrade functions when loading or creating a wallet 9585527a56

[ci skip] Define first run as having no ScriptPubKeyMans 4c97a6ba51

[ci skip] Use RewriteDB action when DB needs rewrite 111a094791

[ci skip] Use GetTimeFirstKey instead of nTimeFirstKey 1ee0c5a87e

[ci skip] Use LegacyScriptPubKeyMan for in wallet_tests e5b834e8ee

[ci skip] Use LegacyScriptPubKeyMan in dumpprivkey and dumpwallet 0c1a3cd0ce

[ci skip] Change CanGetAddresses to fetch from ScriptPubKeyMan

Co-authored-by: Hugo Nguyen <hugh.hn@gmail.com>

01fa6dc1a5

[ci skip] Fetch the correct SigningProvider for signing PSBTs 77417b4360

[ci skip] Use LegacyScriptPubKeyMan in test util ecd7054f3e

[ci skip] Use LegacyScriptPubKeyMan in some parts of getbalances and createwallet 3cb41d78f8

[ci skip] Have getPubKey and getPrivKey use SigningProvider f86100c8bf

[ci skip] Use LegacyScriptPubKeyMan in benchmarks involving the wallet 9c452b0e86

Remove unused functions and switch CWallet to use ScriptPubKeyMan c37be15785

achow101 force-pushed on Oct 21, 2019

achow101 commented at 3:49 pm on October 21, 2019: member

Rebased

DrahtBot removed the label Needs rebase on Oct 21, 2019

ryanofsky referenced this in commit 9ee565d7b1 on Oct 21, 2019

ryanofsky referenced this in commit e91b42f9f6 on Oct 21, 2019

ryanofsky referenced this in commit 37301e5cb4 on Oct 21, 2019

ryanofsky referenced this in commit af89fe136b on Oct 21, 2019

ryanofsky referenced this in commit a9ee4ca806 on Oct 21, 2019

ryanofsky referenced this in commit 2d08c308a3 on Oct 21, 2019

ryanofsky referenced this in commit 1d089c6d88 on Oct 21, 2019

Sjors commented at 9:16 am on October 25, 2019: member

Code review re-ACK c37be15

ryanofsky referenced this in commit 66e675ec1e on Oct 25, 2019

ryanofsky referenced this in commit c968ca5a16 on Oct 25, 2019

ryanofsky referenced this in commit 11ee7b705b on Oct 25, 2019

ryanofsky referenced this in commit 4eb8c892d7 on Oct 25, 2019

ryanofsky referenced this in commit 2e20c13cec on Oct 25, 2019

ryanofsky referenced this in commit 952ba99a44 on Oct 25, 2019

ryanofsky referenced this in commit 299296e51f on Oct 25, 2019

achow101 referenced this in commit ce1f7cde68 on Oct 25, 2019

achow101 referenced this in commit 5ca467def2 on Oct 25, 2019

achow101 referenced this in commit 9ee0f740ec on Oct 25, 2019

achow101 referenced this in commit 0800db90d8 on Oct 25, 2019

achow101 commented at 9:09 pm on October 25, 2019: member

As per the wallet meeting today, we will be using @ryanofsky’s branch (with some modification maybe) and split it up into multiple PRs in an attempt to reduce review burden.

achow101 closed this on Oct 25, 2019

achow101 referenced this in commit e69d048559 on Oct 26, 2019

achow101 referenced this in commit de888ec3d9 on Oct 26, 2019

achow101 referenced this in commit 5bd8339ea0 on Oct 26, 2019

achow101 referenced this in commit 5f086a1262 on Oct 26, 2019

ryanofsky referenced this in commit 9e30fef671 on Oct 26, 2019

ryanofsky referenced this in commit e19bacbdc8 on Oct 26, 2019

ryanofsky referenced this in commit 44bd915ca0 on Oct 26, 2019

ryanofsky referenced this in commit 9875c89220 on Oct 26, 2019

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

achow101 referenced this in commit c1a4093648 on Oct 29, 2019

achow101 referenced this in commit 9cef9ab9a9 on Oct 29, 2019

achow101 referenced this in commit aadcaf2e43 on Oct 29, 2019

achow101 referenced this in commit 3af7b5d4fa on Oct 29, 2019

achow101 referenced this in commit 5d160fa0b7 on Oct 29, 2019

achow101 referenced this in commit a0cf2b646b on Oct 29, 2019

achow101 referenced this in commit 0a682f3eef on Oct 29, 2019

achow101 referenced this in commit ac51a6d634 on Oct 29, 2019

ryanofsky referenced this in commit ef7e9ec083 on Oct 29, 2019

ryanofsky referenced this in commit da93efc30a on Oct 29, 2019

ryanofsky referenced this in commit 16a22682d0 on Oct 29, 2019

ryanofsky referenced this in commit 3a09dd5a8e on Oct 29, 2019

achow101 referenced this in commit 9134d9089a on Oct 30, 2019

achow101 referenced this in commit 187064f917 on Oct 30, 2019

achow101 referenced this in commit df1cdab349 on Oct 30, 2019

achow101 referenced this in commit 5402e001db on Oct 30, 2019

Sjors referenced this in commit 8afbe97853 on Oct 31, 2019

Sjors referenced this in commit b3b53e404b on Oct 31, 2019

Sjors referenced this in commit 7cd863f7d8 on Oct 31, 2019

Sjors referenced this in commit 7ee67e3d14 on Oct 31, 2019

ryanofsky referenced this in commit 71f1a7ff10 on Nov 4, 2019

ryanofsky referenced this in commit 922252b6ff on Nov 4, 2019

ryanofsky referenced this in commit 68a35f25bf on Nov 4, 2019

ryanofsky referenced this in commit 85a1bb8e3a on Nov 4, 2019

achow101 referenced this in commit 7a0cc863d4 on Nov 4, 2019

achow101 referenced this in commit 7fff46e2a6 on Nov 4, 2019

achow101 referenced this in commit 236a6440d6 on Nov 4, 2019

achow101 referenced this in commit c4f33a2c16 on Nov 4, 2019

achow101 referenced this in commit f6285f6bcd on Nov 4, 2019

achow101 referenced this in commit 32321348e2 on Nov 4, 2019

achow101 referenced this in commit ef5cb38fbe on Nov 4, 2019

achow101 referenced this in commit a9a1b186cf on Nov 4, 2019

achow101 referenced this in commit 13925af2c6 on Nov 4, 2019

achow101 referenced this in commit 17baba8248 on Nov 4, 2019

achow101 referenced this in commit eba7066b48 on Nov 4, 2019

achow101 referenced this in commit 86ad24f6a4 on Nov 4, 2019

achow101 referenced this in commit 4e9c2a59f5 on Nov 4, 2019

achow101 referenced this in commit 79c0a7c0d6 on Nov 4, 2019

achow101 referenced this in commit d7affb5fc2 on Nov 4, 2019

achow101 referenced this in commit 8655224ab7 on Nov 5, 2019

achow101 referenced this in commit f034e5748a on Nov 6, 2019

achow101 referenced this in commit 8c0367050f on Nov 6, 2019

achow101 referenced this in commit c810114b27 on Nov 6, 2019

achow101 referenced this in commit 8b9a1ed9fb on Nov 6, 2019

achow101 referenced this in commit 228a316e63 on Nov 6, 2019

achow101 referenced this in commit 6b23558a70 on Nov 6, 2019

achow101 referenced this in commit a8a61f655d on Nov 6, 2019

achow101 referenced this in commit c592354dc2 on Nov 6, 2019

achow101 referenced this in commit ff4c545c3e on Nov 6, 2019

Sjors referenced this in commit 062768df72 on Nov 7, 2019

Sjors referenced this in commit 7daf7ae1ce on Nov 7, 2019

Sjors referenced this in commit 5ac919d1ec on Nov 7, 2019

Sjors referenced this in commit 16fa4935b5 on Nov 7, 2019

achow101 referenced this in commit 60b57a3da6 on Nov 8, 2019

achow101 referenced this in commit 2135d369db on Nov 8, 2019

achow101 referenced this in commit e9651318a1 on Nov 8, 2019

achow101 referenced this in commit 0af77494fe on Nov 8, 2019

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

achow101 referenced this in commit b118878bb1 on Nov 14, 2019

achow101 referenced this in commit a175268f0e on Nov 14, 2019

achow101 referenced this in commit eb9c260137 on Nov 14, 2019

achow101 referenced this in commit 68dbf52719 on Nov 20, 2019

achow101 referenced this in commit 69533f2193 on Nov 20, 2019

achow101 referenced this in commit 42ddfe9125 on Nov 20, 2019

achow101 referenced this in commit 596f6460f9 on Nov 23, 2019

achow101 referenced this in commit 5daefe31c3 on Nov 23, 2019

achow101 referenced this in commit a477750ad2 on Nov 23, 2019

achow101 referenced this in commit 0bd9dd413c on Nov 23, 2019

achow101 referenced this in commit 1e0a4bde0a on Nov 23, 2019

ryanofsky referenced this in commit e2e4042808 on Nov 26, 2019

ryanofsky referenced this in commit 0670042d7e on Nov 26, 2019

achow101 referenced this in commit 562a4b0e1c on Nov 26, 2019

achow101 referenced this in commit dc63fce9d9 on Nov 26, 2019

achow101 referenced this in commit f540bbe3cc on Nov 26, 2019

achow101 referenced this in commit d17100df2e on Nov 26, 2019

achow101 referenced this in commit 24b25eee17 on Nov 27, 2019

achow101 referenced this in commit 66b82896d5 on Nov 27, 2019

achow101 referenced this in commit 244f5c23f8 on Dec 2, 2019

achow101 referenced this in commit 886f1731be on Dec 2, 2019

achow101 referenced this in commit 4099344168 on Dec 6, 2019

achow101 referenced this in commit f28008ce64 on Dec 12, 2019

achow101 referenced this in commit a3e5ffe199 on Dec 13, 2019

achow101 referenced this in commit 216f65542f on Dec 17, 2019

achow101 referenced this in commit eca2793f71 on Dec 17, 2019

achow101 referenced this in commit ecdbacc563 on Dec 30, 2019

achow101 referenced this in commit c6a061d10e on Jan 6, 2020

achow101 referenced this in commit 05e08b8a3b on Jan 6, 2020

achow101 referenced this in commit b890cac265 on Jan 6, 2020

achow101 referenced this in commit c5842c171b on Jan 15, 2020

achow101 referenced this in commit 8540dcb0bb on Jan 15, 2020

achow101 referenced this in commit d27e13f505 on Jan 17, 2020

achow101 referenced this in commit 7a9fef7655 on Jan 17, 2020

achow101 referenced this in commit 5b580074ba on Jan 17, 2020

achow101 referenced this in commit 415afcccd3 on Jan 23, 2020

JeremyRubin referenced this in commit 7f08a0e801 on Feb 7, 2020

furszy referenced this in commit d58fd6fee1 on Mar 11, 2020

HashUnlimited referenced this in commit b329fd5fc1 on Apr 17, 2020

HashUnlimited referenced this in commit 6c2fbd7cba on Apr 17, 2020

HashUnlimited referenced this in commit 5a0000aa15 on Apr 17, 2020

meshcollider referenced this in commit eef90c14ed on Apr 27, 2020

sidhujag referenced this in commit 46ed584a4a on Apr 27, 2020

jasonbcox referenced this in commit 343d199ad1 on Oct 10, 2020

van-orton referenced this in commit 1affd98073 on Oct 30, 2020

backpacker69 referenced this in commit 80c0e7739e on Mar 28, 2021

backpacker69 referenced this in commit de5717dd91 on Mar 28, 2021

backpacker69 referenced this in commit 88cdbf163a on Mar 28, 2021

DrahtBot locked this on Feb 15, 2022

Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes) #16341

Conflicts