Allow creating blank (empty) wallets (alternative)

achow101 commented at 3:31 am on January 22, 2019: member

Alternative (kind of) to #14938

This PR adds a blank parameter to the createwallet RPC to create a wallet that has no private keys initially. sethdseed can then be used to make a clean wallet with a custom seed. encryptwallet can also be used to make a wallet that is born encrypted.

Instead of changing the version number as done in #14938, a wallet flag is used to indicate that the wallet should be blank. This flag is set at creation, and then unset when the wallet is no longer blank. A wallet becomes non-blank when a HD seed is set or anything is imported. The main change to create a blank wallet is primarily taken from #14938.

Also with this, the term “blank wallet” is used instead of “empty wallet” to avoid confusion with wallets that have balance which would also be referred to as “empty”.

This is built on top of #15225 in order to fix GUI issues.

fanquake added the label Wallet on Jan 22, 2019

achow101 force-pushed on Jan 22, 2019

DrahtBot commented at 4:19 am on January 22, 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:

#15288 (Remove wallet -> node global function calls by ryanofsky)
#15006 (Add option to create an encrypted wallet by achow101)
#14918 (RPCHelpMan: Check default values are given at compile-time by MarcoFalke)
#14912 ([WIP] External signer support (e.g. hardware wallet) by Sjors)
#14075 (Import watch only pubkeys to the keypool if private keys are disabled by achow101)
#13756 (wallet: “avoid_reuse” wallet flag for improved privacy by kallewoof)
#10973 (Refactor: separate wallet from node by ryanofsky)

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.

in src/wallet/wallet.cpp:717 in c0bfc2d716 outdated

709@@ -700,8 +710,8 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase)
710         Lock();
711         Unlock(strWalletPassphrase);
712 
713-        // if we are using HD, replace the HD seed with a new one
714-        if (IsHDEnabled()) {
715+        // if we are using HD, create a fresh seed or replace the existing seed with a new one
716+        if (IsHDEnabled() && !IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {

jonasschnelli commented at 6:28 am on January 22, 2019:

Does this mean we are currently generating a new seed if one encrypts a wallet where private keys are disabled?

achow101 commented at 6:47 am on January 22, 2019:

No. ~~Looking at what we currently do, this should actually be checking for HasHDSeed(). Will change.~~ Actually for the behavior that I want, this is correct. The reason this is changed is because IsHDEnabled() is different now.

Sjors commented at 5:40 pm on January 22, 2019:

So this would add a seed to an empty wallet? That should be made more clear in the documentation.

What if I want to import private keys and encrypt those? Should we add a bool to encryptwallet to determine if it should generate a key?

ryanofsky commented at 11:02 pm on February 6, 2019:

re: #15226 (review)

In commit “If there are no encrypted keys, default is passing” (e4ffedbb91e129ca282e47a5f5836dc0212deecf)

So this would add a seed to an empty wallet?

I also think this is a confusing UI. What’s the advantage of requiring a seed in order to use encryption?

Why not simply change the condition to if (HasHDSeed()) keeping the previous behavior and keeping the encryption and blank wallet features independent?

in src/wallet/wallet.h:150 in c0bfc2d716 outdated

136@@ -137,9 +137,12 @@ enum WalletFlags : uint64_t {
137 
138     // will enforce the rule that the wallet can't contain any private keys (only watch-only/pubkeys)
139     WALLET_FLAG_DISABLE_PRIVATE_KEYS = (1ULL << 32),
140+
141+    // Will enforce that the wallet should be blank
142+    WALLET_FLAG_BLANK_WALLET = (1ULL << 33),

jonasschnelli commented at 6:31 am on January 22, 2019:

Should we mention in the release notes that blank wallets can not be used in older bitcoin core versions?

jonasschnelli commented at 6:32 am on January 22, 2019: contributor

Concept ACK

achow101 force-pushed on Jan 22, 2019

laanwj renamed this:
~~Allow creating blank (empty) wallets~~
Allow creating blank (empty) wallets (alternative)
on Jan 22, 2019

Sjors commented at 5:14 pm on January 22, 2019: member

Concept ACK, thanks for saving me some work :-)

Are you sure older clients will understand this mandatory flag? Testing this is a good use of #12134, though such test should be in a separate PR to prevent a long wait.

in src/wallet/wallet.cpp:1434 in c125d04c45 outdated

1425+bool CWallet::IsHDEnabled() const
1426+{
1427+    LOCK(cs_wallet);
1428+    // -usehd was removed in 0.16, so all wallets with FEATURE_PRE_SPLIT_KEYPOOL are HD
1429+    // For older wallets the presence of seed_id indicates if HD is enabled.
1430+    // Support for empty HD capable wallets was added in 0.18

Sjors commented at 5:31 pm on January 22, 2019:

Nit: “blank capable”.

in src/wallet/wallet.cpp:1444 in c125d04c45 outdated

1431+    return CanSupportFeature(FEATURE_PRE_SPLIT_KEYPOOL) || HasHDSeed();
1432+}
1433+
1434+bool CWallet::IsBlank() const
1435+{
1436+    return IsHDEnabled() && !HasHDSeed();

Sjors commented at 5:34 pm on January 22, 2019:

Nit: document that for older wallets this relies on IsHDEnabled() checking HasHDSeed(), so there’s no way this returns a false positive.

ryanofsky commented at 5:41 pm on January 22, 2019: member

This should probably replace #14938 on the high priority list: https://github.com/bitcoin/bitcoin/projects/8

Sjors commented at 5:48 pm on January 22, 2019: member

Other than my questions and remarks c0bfc2d7160e7c861d9a3e678e1dd9b6213a8a86 looks good.

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

achow101 commented at 9:07 pm on January 22, 2019: member

Are you sure older clients will understand this mandatory flag? Testing this is a good use of #12134, though such test should be in a separate PR to prevent a long wait.

Just tested with 0.17 and 0.16. 0.17 knows what the wallet flags are so it does understand the mandatory flag and refuses to load wallets with the blank flag set. However 0.16 does not know about wallet flags and will actually load the wallet and generate keys for it. It does this for both wallets with private keys disabled and blank wallets, which might be an issue. However I noticed it is a bit harder to move back to 0.16 due to the wallet directory change where wallets are now directories instead of just files. But this is an issue in general for wallet flags. 0.16 does not open these wallets because the version number was bumped for 0.17.

jonasschnelli commented at 9:59 pm on January 22, 2019: contributor

Would it had made (or still make) sense to raise the wallet version number to prevent ignoring wallet flags?

ryanofsky commented at 11:22 pm on January 22, 2019: member

Would it had made (or still make) sense to raise the wallet version number to prevent ignoring wallet flags?

It makes sense that if you are going to set a mandatory flag, you also need to increase the minimum version to whatever bitcoin version started checking mandatory flags.

Sjors commented at 12:02 pm on January 23, 2019: member

Wallet feature flags were introduced in 0.17.0, and as @achow101 and the source comment points out, “wallet flags in the upper section (> 1 « 31) will lead to not opening the wallet if flag is unknown”.

The following, added in a8da482a8bc87ff26194612727d4a7b86b2fb60d and released in v0.17.0, should already prevent 0.16 nodes from opening a >=0.17.0 wallet:

0FEATURE_PRE_SPLIT_KEYPOOL = 169900,
1FEATURE_LATEST = FEATURE_PRE_SPLIT_KEYPOOL;

@achow101 I’m surprised you were able to open such a wallet in 0.16. Are you sure you compiled it right? I just tried using the v0.16.3 binary and it refused to open a wallet with WALLET_FLAG_DISABLE_PRIVATE_KEYS, because the version was too new.

By the way, let’s make sure we don’t get in the way of #13756 which introduces MUTABLE_WALLET_FLAGS, flags that once set can’t be changed.

achow101 commented at 7:34 pm on January 23, 2019: member

Oh, I made a mistake. Just tested it again and yes, 0.16 does not open wallets that have flags set, so there is no need to do another version bump.

achow101 force-pushed on Jan 23, 2019

laanwj commented at 3:23 pm on January 24, 2019: member

Also with this, the term “blank wallet” is used instead of “empty wallet” to avoid confusion with wallets that have balance which would also be referred to as “empty”.

Thank you, I really like this distinct terminology.

in test/functional/test_framework/test_node.py:437 in ac4d021ded outdated

433@@ -434,7 +434,7 @@ def batch(self, requests):
434     def send_cli(self, command=None, *args, **kwargs):
435         """Run bitcoin-cli command. Deserializes returned string as python object."""
436         pos_args = [str(arg).lower() if type(arg) is bool else str(arg) for arg in args]
437-        named_args = [str(key) + "=" + str(value) for (key, value) in kwargs.items()]
438+        named_args = [str(key) + "=" + (str(value).lower() if type(value) is bool else str(value)) for (key, value) in kwargs.items()]

laanwj commented at 3:42 pm on January 24, 2019:

Same comment here as #15235 (review)

achow101 commented at 3:39 pm on January 31, 2019:

Since this issue affects multiple PRs, I’ve split it into a separate PR: #15301

DrahtBot added the label Needs rebase on Jan 29, 2019

achow101 force-pushed on Jan 29, 2019

achow101 commented at 7:57 pm on January 29, 2019: member

Rebased

DrahtBot removed the label Needs rebase on Jan 29, 2019

in src/qt/walletmodel.cpp:588 in 2021dfa737 outdated

582@@ -583,9 +583,10 @@ bool WalletModel::canGetAddresses() const
583     // The wallet can provide a fresh address if:
584     // * hdEnabled(): an HD seed is present; or
585     // * it is a legacy wallet, because:
586-    //     * !hdEnabled(): an HD seed is not present; and
587-    //     * !IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS): private keys have not been disabled (which results in hdEnabled() == true)
588-    return m_wallet->hdEnabled() || (!m_wallet->hdEnabled() && !m_wallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS));
589+    //     * !hdEnabled(): no HD seed is present; and
590+    //     * !IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS): private keys have not been disabled (which results in hdEnabled() == true); and
591+    //     * !!IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET): the wallet is not blank

Sjors commented at 10:43 am on January 30, 2019:

Should be a single !?

Sjors commented at 10:47 am on January 30, 2019:

With this additional flag, it makes more sense to fix hdEnabled() in interfaces::Wallet.

achow101 commented at 7:58 pm on January 31, 2019:

Fixed.

in src/interfaces/wallet.cpp:460 in f595d9d89e outdated

456@@ -457,7 +457,7 @@ class WalletImpl : public Wallet
457         return result;
458     }
459     unsigned int getConfirmTarget() override { return m_wallet.m_confirm_target; }
460-    bool hdEnabled() override { return m_wallet.IsHDEnabled(); }
461+    bool hdEnabled() override { return m_wallet.HasHDSeed(); }

Sjors commented at 11:15 am on January 30, 2019:

Let’s fix hdEnabled in WalletModel instead, otherwise things are going to get confusing fast.

Sjors commented at 4:30 pm on January 31, 2019:

Now both 6382e96ab9b8d9f7f9ad4ab1b318262ff3890916 and 2774bec3aa2bc20b578453b103fbe04cf34ecbdf change bool hdEnabled() in interfaces/wallet.cpp .

I don’t like it that hdEnabled() on interfaces wallet calls m_wallet.HasHDSeed, even though m_wallet also has a ruction called IsHDEnabled.

achow101 commented at 7:58 pm on January 31, 2019:

I’ve changed it so that hdEnabled() does the same thing as before and instead added a hasHDSeed() which calls HasHDSeed. hasHDSeed is used instead of hdEnabled in the walletmodel.cpp changes.

Sjors commented at 11:19 am on January 30, 2019: member

2021dfa is getting close. It just needs some changes to prevent even more confusion around how we’re deciding if a wallet is HD.

Also regarding encrypting a blank wallet, whatever strategy you pick should be documented. See my earlier comment:

So this would add a seed to an empty wallet? That should be made more clear in the documentation.

What if I want to import private keys and encrypt those? Should we add a bool to encryptwallet to determine if it should generate a key?

DrahtBot added the label Needs rebase on Jan 31, 2019

achow101 force-pushed on Jan 31, 2019

DrahtBot removed the label Needs rebase on Jan 31, 2019

in test/functional/wallet_createwallet.py:72 in 6382e96ab9 outdated

58@@ -59,5 +59,27 @@ def run_test(self):
59         w3.getnewaddress()
60         w3.getrawchangeaddress()
61 
62+        self.log.info("Test blank creation with privkeys enabled and then encryption")

Sjors commented at 4:26 pm on January 31, 2019:

Introducing these tests in 6382e96ab9b8d9f7f9ad4ab1b318262ff3890916 seems too early (I assume they’re fail, which hurts git-bisect).

achow101 commented at 6:14 pm on January 31, 2019:

No, the order is correct. Github is just showing it in the wrong order because of the dates. I’ll fix the dates so it shows correctly.

achow101 force-pushed on Jan 31, 2019

achow101 commented at 7:59 pm on January 31, 2019: member

I’ve updated the docs and added a few code comments as suggested by earlier comments (which I can’t respond to for some reason).

in src/qt/walletmodel.cpp:587 in 24ff3c70c3 outdated

586     // * it is a legacy wallet, because:
587-    //     * !hdEnabled(): an HD seed is not present; and
588-    //     * !IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS): private keys have not been disabled (which results in hdEnabled() == true)
589-    return m_wallet->hdEnabled() || (!m_wallet->hdEnabled() && !m_wallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS));
590+    //     * !hasHDSeed(): an HD seed is not present; and
591+    //     * !IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS): private keys have not been disabled (which results in hasHDSeed() == true)

Sjors commented at 8:31 pm on January 31, 2019:

nit: missing ; and

in src/wallet/wallet.h:1138 in 06e7122e0f outdated

1133+    bool HasHDSeed() const;
1134+
1135     /* Returns true if HD is enabled */
1136     bool IsHDEnabled() const;
1137 
1138+    /* Returns true if the wallet is blank */

Sjors commented at 8:43 pm on January 31, 2019:

nit: , i.e. it has no private keys (because it might have scripts, and the keypool isn’t taken into account)

Sjors approved

Sjors commented at 8:45 pm on January 31, 2019: member

utACK 24ff3c7

DrahtBot added the label Needs rebase on Feb 1, 2019

achow101 force-pushed on Feb 1, 2019

achow101 commented at 3:40 pm on February 1, 2019: member

Rebased

DrahtBot removed the label Needs rebase on Feb 1, 2019

Sjors commented at 5:39 pm on February 1, 2019: member

tACK 4d2b8b4, since last check: rebase and moved some tests to another commit

I checked again that v0.17.1 refuses to open a blank wallet, and that after sethdseed it does.

in src/wallet/wallet.cpp:3326 in 96476657a0 outdated

3310@@ -3295,6 +3311,9 @@ bool CWallet::TopUpKeyPool(unsigned int kpSize)
3311         if (IsLocked())
3312             return false;
3313 
3314+        // If wallet supports HD but no seed is present, do not top up keypool
3315+        if (IsBlank()) return true;

ryanofsky commented at 1:02 am on February 7, 2019:

In commit “[wallet] support creating a blank wallet” (96476657a06797913edffd42d3fdae320b86e9df):

This isn’t checking the WALLET_FLAG_BLANK_WALLET flag at all. It seems weird that to allow topping up keypool or generating keys (in GetKeyFromPool) in a wallet that supports hd but intentionally has no seed. Am I understanding this correctly, or is there another check somewhere else?

jonasschnelli commented at 7:59 pm on February 7, 2019:

I can’t follow your comment. The IsBlank() check here stops the refill-process if the wallet is either blank or if no HD seed is available (while HD is enabled). I also tested this by creating a blank wallet, import a watch-only and then calling keypoolrefill.

ryanofsky commented at 8:34 pm on February 7, 2019:

re: #15226 (review)

I can’t follow your comment.

This comment was wrong about the keypool, but it was added reviewing a previous version of this PR (commit hash mentioned was 96476657a06797913edffd42d3fdae320b86e9df) before isBlank checked the blank flag, which is probably why you were confused.

I actually think now that this shouldn’t directly be checking the blank flag, but I want to look more closely and follow up with an other review.

in src/interfaces/wallet.cpp:465 in 4d2b8b4d93 outdated

462@@ -463,6 +463,7 @@ class WalletImpl : public Wallet
463     }
464     unsigned int getConfirmTarget() override { return m_wallet.m_confirm_target; }
465     bool hdEnabled() override { return m_wallet.IsHDEnabled(); }

ryanofsky commented at 1:11 am on February 7, 2019:

In commit “Set a wallet flag for blank wallets” (4d2b8b4d93cb979fba81182258d93d562608a632)

I don’t understand the GUI part of this change at all. Does it actually make sense to show an HD enabled logo for a wallet that doesn’t have an HD key? And will generate non-HD keys if an address is requested (IIUC)?

ryanofsky commented at 8:45 pm on February 8, 2019:

Resolved when the PR was reworked

ryanofsky commented at 1:28 am on February 7, 2019: member

I really like the feature, but would NACK the PR in it’s current form just because it so pointlessly confusing to review! The worst part is the renaming of IsHDEnabled to HasHDSeed followed by an addition of a new IsHDEnabled with a different meaning. So all the calls where the previous behavior isn’t changing show up in the diff, while all the calls where the behavior is changing don’t show up at all! I’d really suggest dropping the IsHDEnabled rename or doing a scripted diff commit, and then adding a new function like IsHDSupported or IsHDSafe for the new version-based check.

achow101 force-pushed on Feb 7, 2019

achow101 commented at 3:54 am on February 7, 2019: member

I’ve reworked IsHDEnabled(), HasHDSeed(), and IsBlank(). I have decided to lave IsHDEnabled() as it is and dropped HasHDSeed(). Instead IsBlank() will now check whether the blank wallet flag is set or if HD support is specified in the version but no HD seed is set. This change should clear up any confusion.

I’ve also squashed down the commits as I don’t think the original commit structure would pass tests.

I changed how EncryptWallet() works with blank wallets. It will just set the encryption master key and not create a HD seed. If the user wants a seed, they can use sethdseed or create a born encrypted wallet with the change in #15006

in src/wallet/wallet.cpp:1436 in 1cdb8d8346 outdated

1428@@ -1418,6 +1429,13 @@ bool CWallet::IsHDEnabled() const
1429     return !hdChain.seed_id.IsNull();
1430 }
1431 
1432+bool CWallet::IsBlank()
1433+{
1434+    // A Blank wallet either has the flag set or it has a version number requiring HD but no HD seed is set
1435+    LOCK(cs_wallet);
1436+    return IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET) || (CanSupportFeature(FEATURE_HD) && !IsHDEnabled());

ryanofsky commented at 5:14 am on February 7, 2019:

In commit “[wallet] Support creating a blank wallet” (1cdb8d8346649434450e410971cc3d68c3504a25)

If there a wallet was created after the FEATURE_HD version but with -usehd=0 set, would it cause this function to return true and no longer let GetKeyFromPool and TopUpKeyPool work?

achow101 commented at 5:34 am on February 7, 2019:

I don’t think so because those wallets would have a version less than FEATURE_HD.

ryanofsky commented at 9:22 pm on February 8, 2019:

You’re right. Apparently the SetMinVersion(FEATURE_LATEST) that now happens on first run didn’t used to happen on first run.

ryanofsky commented at 10:02 pm on February 8, 2019:

In commit “[wallet] Support creating a blank wallet” (8061ea4611696b22d5d5dcdd73c1e053ffecc09d)

I think there is no point to checking WALLET_FLAG_BLANK_WALLET here. It’s redundant and just complicates the logic.

Also, I don’t think this function should be called IsBlank, because this creates two slightly different definitions of blank, one where a wallet stops being blank when a watch-only address is added (WALLET_FLAG_BLANK_WALLET), and one which remains blank (IsBlank). I think a more accurate name for this function would be CannotGenerateKeys, but even better would be:

0bool CWallet::CanGenerateKeys()
1{
2    // Wallet can generate keys if it has an HD seed, or is a pre-hd wallet.
3    return IsHDEnabled() || !CanSupportFeature(FEATURE_HD);
4}

which could simplify all the places where this is called, as well as the canGetAddresses GUI function.

EDIT: Changed suggested name from CanGenerateAddresses to CanGenerateKeys in light of #14075

ryanofsky commented at 2:03 pm on February 9, 2019:

re: #15226 (review)

After seeing #14075, I’d tweak my suggestion above and call this CanGenerateKeys or CanTopUpKeyPool instead of CanGenerateAddresses. After #14075 it looks like it will be possible to generate addresses with imported keys even if the wallet can’t generate new keys.

in src/wallet/wallet.cpp:384 in 1cdb8d8346 outdated

379@@ -374,7 +380,11 @@ bool CWallet::AddWatchOnly(const CScript& dest)
380     const CKeyMetadata& meta = m_script_metadata[CScriptID(dest)];
381     UpdateTimeFirstKey(meta.nCreateTime);
382     NotifyWatchonlyChanged(true);
383-    return WalletBatch(*database).WriteWatchOnly(dest, meta);
384+    if (WalletBatch(*database).WriteWatchOnly(dest, meta)) {
385+        UnsetWalletFlag(WALLET_FLAG_BLANK_WALLET);

ryanofsky commented at 5:20 am on February 7, 2019:

In commit “[wallet] Support creating a blank wallet” (1cdb8d8346649434450e410971cc3d68c3504a25)

It looks like if you import a watch-only key into a blank wallet WalletModel::canGetAddresses() will start returning true. I wonder if canGetAddresses should be using IsBlank.

achow101 commented at 6:02 am on February 7, 2019:

I think it should. I’ve added isBlank() to the wallet interface and canGetAddresses() calls it instead of checking the wallet flag.

ryanofsky commented at 7:39 pm on February 9, 2019:

Thread could be could be marked resolved.

ryanofsky commented at 5:32 am on February 7, 2019: member

Thanks for reworking and squashing. I think I understand the PR better now. I do have two questions below.

achow101 force-pushed on Feb 7, 2019

jonasschnelli commented at 8:00 pm on February 7, 2019: contributor

Tested ACK 8061ea4611696b22d5d5dcdd73c1e053ffecc09d

gwillen commented at 11:02 pm on February 7, 2019: contributor

Tested on OS X, reviewed and looks good but I don’t feel qualified to ACK as I don’t understand the intricacies of the wallet well enough.

meshcollider added this to the milestone 0.18.0 on Feb 8, 2019

Sjors commented at 1:14 pm on February 8, 2019: member

I’ll try to give this another review in the next few days.

in src/wallet/wallet.cpp:4113 in 8061ea4611 outdated

4110-        }
4111+        } // Otherwise, do not generate a new seed
4112 
4113         // Top up the keypool
4114-        if (!walletInstance->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) && !walletInstance->TopUpKeyPool()) {
4115+        if (!walletInstance->IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET) && !walletInstance->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) && !walletInstance->TopUpKeyPool()) {

ryanofsky commented at 8:49 pm on February 8, 2019:

In commit “[wallet] Support creating a blank wallet” (8061ea4611696b22d5d5dcdd73c1e053ffecc09d)

Would suggest replacing this line with if (walletInstance->CanGenerateKeys() && !walletInstance->TopUpKeyPool()) if you use my other suggestion.

in doc/release-notes-15226.md:5 in 8061ea4611 outdated

0@@ -0,0 +1,8 @@
1+Miscellaneous RPC changes
2+------------
3+
4+- The RPC `createwallet` now has an optional `blank` argument that can be used to create a blank wallet.
5+Blank wallets do not have any private keys or HD seed.

ryanofsky commented at 9:52 pm on February 8, 2019:

In commit “[wallet] Support creating a blank wallet” (8061ea4611696b22d5d5dcdd73c1e053ffecc09d)

I think just “keys” here would be more accurate than “private keys”. If you import a watchonly address the wallet isn’t really blank in the sense you’re describing here (it can be opened in 0.17.x).

in src/wallet/rpcwallet.cpp:3894 in 8061ea4611 outdated

3890@@ -3880,7 +3891,7 @@ UniValue sethdseed(const JSONRPCRequest& request)
3891     LOCK(pwallet->cs_wallet);
3892 
3893     // Do not do anything to non-HD wallets
3894-    if (!pwallet->IsHDEnabled()) {
3895+    if (!pwallet->IsBlank() && !pwallet->IsHDEnabled()) {

ryanofsky commented at 10:07 pm on February 8, 2019:

In commit “[wallet] Support creating a blank wallet” (8061ea4611696b22d5d5dcdd73c1e053ffecc09d)

This is pretty convoluted. It would be more straightforward to avoid all the cancelling conditions and just write if (!CanSupportFeature(FEATURE_HD))

ryanofsky commented at 2:30 pm on February 9, 2019:

In commit “[wallet] Support creating a blank wallet” (8061ea4611696b22d5d5dcdd73c1e053ffecc09d)

This will now allow calling sethdseed on a wallet with WALLET_FLAG_DISABLE_PRIVATE_KEYS set. That seems like a mistake because I’m not sure the resulting state would make sense, and at the very least GenerateNewSeed would assert false in this case.

Would suggest adding a check for WALLET_FLAG_DISABLE_PRIVATE_KEYS here.

achow101 commented at 5:12 pm on February 9, 2019:

There’s already a check for that flag above.

in src/wallet/wallet.cpp:3449 in 8061ea4611 outdated

3445@@ -3416,7 +3446,7 @@ void CWallet::ReturnKey(int64_t nIndex, bool fInternal, const CPubKey& pubkey)
3446 
3447 bool CWallet::GetKeyFromPool(CPubKey& result, bool internal)
3448 {
3449-    if (IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
3450+    if (IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) || IsBlank()) {

ryanofsky commented at 10:10 pm on February 8, 2019:

In commit “[wallet] Support creating a blank wallet” (8061ea4611696b22d5d5dcdd73c1e053ffecc09d)

Would suggest replacing this line with if (!CanGenerateKeys()) return false if you use my other suggestion.

in src/wallet/wallet.h:140 in 8061ea4611 outdated

135@@ -136,9 +136,12 @@ enum WalletFlags : uint64_t {
136 
137     // will enforce the rule that the wallet can't contain any private keys (only watch-only/pubkeys)
138     WALLET_FLAG_DISABLE_PRIVATE_KEYS = (1ULL << 32),
139+
140+    // Will enforce that the wallet should be blank

ryanofsky commented at 10:17 pm on February 8, 2019:

In commit “[wallet] Support creating a blank wallet” (8061ea4611696b22d5d5dcdd73c1e053ffecc09d)

It would be nice to explain how the flag works. I’d suggest.

0//! Flag set when a wallet contains no HD seed and no private keys, scripts,
1//! addresses, and other watch only things, and is therefore "blank."
2//!
3//! The only function this flag serves is to distinguish a blank wallet from
4//! a newly created wallet when the wallet database is loaded, to avoid
5//! initialization that should only happen on first run.
6//!
7//! This flag is also a mandatory flag to prevent previous versions of
8//! bitcoin from opening the wallet, thinking it was newly created, and
9//! then improperly reinitializing it.

ryanofsky commented at 10:21 pm on February 8, 2019: member

This is pretty good and I think it will work but I think it can be simplified more. Left suggestions below.

in src/qt/walletmodel.cpp:589 in 8061ea4611 outdated

588-    //     * !IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS): private keys have not been disabled (which results in hdEnabled() == true)
589-    return m_wallet->hdEnabled() || (!m_wallet->hdEnabled() && !m_wallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS));
590+    //     * !hdEnabled(): a HD seed is not present; and
591+    //     * !IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS): private keys have not been disabled
592+    //     * !isBlank(): the wallet is not blank
593+    return m_wallet->hdEnabled() || (!m_wallet->hdEnabled() && !m_wallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) && !m_wallet->isBlank());

ryanofsky commented at 2:33 pm on February 9, 2019:

In commit “[wallet] Support creating a blank wallet” (8061ea4611696b22d5d5dcdd73c1e053ffecc09d)

I think this entire line is equivalent to return !m_wallet->isBlank().

achow101 force-pushed on Feb 9, 2019

achow101 commented at 6:43 pm on February 9, 2019: member

I have taken @ryanofsky’s suggestions and reworked this a bit more. This should also help with #14075.

I got rid of IsBlank and replaced it with two functions: CanGenerateKeys and CanGetAddresses. CanGenerateKeys is as @ryanofsky commented; it checks for whether there is an HD seed or whether the wallet is non-HD. CanGetAddresses checks whether there are keys in the keypool (defaults to external keypool but can check the internal one), and if not, checks CanGenerateKeys. This is used as a guard wherever a key from the keypool is fetched for addresses. Additionally this is called by WalletModel::CanGetAddresses() and replaces the logic that was there.

achow101 force-pushed on Feb 9, 2019

in src/wallet/rpcwallet.cpp:180 in 8b5533ee28 outdated

175@@ -176,6 +176,11 @@ static UniValue getnewaddress(const JSONRPCRequest& request)
176 
177     LOCK(pwallet->cs_wallet);
178 
179+    if (!pwallet->CanGetAddresses()) {

ryanofsky commented at 7:36 pm on February 9, 2019:

In commit “[wallet] Support creating a blank wallet” (8b5533ee281ba114b8bb5c195f4a9b715b02a644)

Curious what other people think but I think it would be great to remove WALLET_FLAG_DISABLE_PRIVATE_KEYS check above in light of this check.

One thing that makes flag code hard to understand is that you can grep and have to dig through 20 usages of a flag where checking it is redundant or has little effect, which makes it harder to identify the places where the flag is actually playing an important role

I think if WALLET_FLAG_DISABLE_PRIVATE_KEYS check above should be kept it should at least have a comment saying it’s belt and suspenders or something like that.

achow101 commented at 9:34 pm on February 9, 2019:

I’ve added a comment

in src/wallet/rpcwallet.cpp:247 in 8b5533ee28 outdated

241@@ -237,6 +242,10 @@ static UniValue getrawchangeaddress(const JSONRPCRequest& request)
242 
243     LOCK(pwallet->cs_wallet);
244 
245+    if (!pwallet->CanGetAddresses(true)) {

ryanofsky commented at 7:37 pm on February 9, 2019:

In commit “[wallet] Support creating a blank wallet” (8b5533ee281ba114b8bb5c195f4a9b715b02a644)

There’s another WALLET_FLAG_DISABLE_PRIVATE_KEYS check above that could be removed or noted belt and suspenders.

in src/wallet/wallet.cpp:3463 in 8b5533ee28 outdated

3459@@ -3416,7 +3460,7 @@ void CWallet::ReturnKey(int64_t nIndex, bool fInternal, const CPubKey& pubkey)
3460 
3461 bool CWallet::GetKeyFromPool(CPubKey& result, bool internal)
3462 {
3463-    if (IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
3464+    if (!CanGetAddresses(internal)) {

ryanofsky commented at 7:52 pm on February 9, 2019:

In commit “[wallet] Support creating a blank wallet” (8b5533ee281ba114b8bb5c195f4a9b715b02a644)

Here and other three places where wallet code is calling CanGetAddresses(), I think it should be calling CanGenerateKeys() instead. CanGetAddresses() is definitely useful for the GUI, because the GUI has to continuously keep its display state up to date with state of the keypool, and this is inherently racy and approximate. But actual wallet code shouldn’t be doing racy, redundant checks like this. There’s no point to locking cs_wallet, checking the number of available keys in the keypool, releasing the lock, reacquiring the lock and then doing the same check again below in ReserveKey.

achow101 commented at 9:28 pm on February 9, 2019:

The reason I have the CanGetAddresses check here is for future work where you can get a key from the keypool without the ability to generate addresses.

Sjors commented at 4:13 pm on February 10, 2019:

Agree with the usefulness of CanGetAddresses for followup stuff. Longer term I also think it’s a useful distinction in a descriptor based wallet, which may support a lot more (semi) watch-only (solvable, but not IsMine) things like being part of a multisig group.

Regarding the “racy” stuff, maybe it helps to add AssertLockHeld to CWallet CanGetAddresses and CanGenerateKeys? That way places that (indirectly) call both can get a lock once.

ryanofsky commented at 8:06 pm on February 9, 2019: member

utACK 8b5533ee281ba114b8bb5c195f4a9b715b02a644. Thanks for being so responsive. I think this is in good shape, though I did leave some more suggestions.

achow101 force-pushed on Feb 9, 2019

meshcollider commented at 10:02 am on February 10, 2019: contributor

utACK https://github.com/bitcoin/bitcoin/pull/15226/commits/707eabab827250cc1b726179cabe97d3e0c100de

I’ll give @Sjors a chance to finish his last review of this, then merge tomorrow

in src/wallet/wallet.h:1144 in 707eabab82 outdated

1143@@ -1132,6 +1144,12 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface
1144     /* Returns true if HD is enabled */

Sjors commented at 4:32 pm on February 10, 2019:

IsHDEnabled() should be renamed to HasHDSeed() because that’s actually what it checks (as opposed to CanSupportFeature(FEATURE_HD)). Can perhaps wait until a followup, since it’s clear enough from the comments.

achow101 commented at 5:23 pm on February 10, 2019:

I would prefer to leave it as-is for now. That can be changed in a followup.

Sjors commented at 6:50 pm on February 10, 2019:

That’s fine, but I’ll keep stalking you :-)

in src/wallet/wallet.cpp:716 in 707eabab82 outdated

712@@ -703,7 +713,7 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase)
713         Lock();
714         Unlock(strWalletPassphrase);
715 
716-        // if we are using HD, replace the HD seed with a new one
717+        // if we are using HD, create a fresh seed or replace the existing seed with a new one

Sjors commented at 4:35 pm on February 10, 2019:

I don’t see how “create a fresh seed” happens here, because if there’s no seed IsHDEnabled() returns false.

achow101 commented at 5:25 pm on February 10, 2019:

Outdated comment. I’ve changed it back.

Sjors commented at 7:00 pm on February 10, 2019:

Might be useful to add “If no HD seed is set, e.g. because it is a blank wallet, do not add one.”, can be done in the same followup as renaming IsHDEnabled() (though that rename would make the comment unnecessary :-).

Sjors approved

Sjors commented at 4:41 pm on February 10, 2019: member

utACK 707eaba, but please double check my “create a fresh seed” comment before merging.

I like the new CanGenerateKeys and CanGetAddresses approach, and the fact that the blank flag now really indicates the wallet is empty, rather than the more complicated condition we had before.

I checked again that v0.17.1 refuses to open a blank wallet, and that after sethdseed it does.

[wallet] Support creating a blank wallet

A blank wallet is a wallet that has no keys, script or watch only things.
A new wallet flag indicating that it is blank will be set when the wallet
is blank. Once it is no longer blank (a seed has been generated, keys or
scripts imported, etc), the flag will be unset.

7687f7873b

achow101 force-pushed on Feb 10, 2019

Sjors commented at 6:58 pm on February 10, 2019: member

re-utACK 7687f78

meshcollider deleted a comment on Feb 10, 2019

meshcollider merged this on Feb 10, 2019

meshcollider closed this on Feb 10, 2019

meshcollider referenced this in commit 6f4e0d1542 on Feb 10, 2019

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

domob1812 referenced this in commit 5b7650b528 on Feb 11, 2019

laanwj referenced this in commit 1c719f78d3 on May 16, 2019

DrahtBot locked this on Dec 16, 2021

Allow creating blank (empty) wallets (alternative) #15226

Conflicts