Support creating an empty wallet

Sjors commented at 5:02 pm on December 12, 2018: member

Add as new argument (empty) to createwallet to create an empty wallet.

This can be followed by sethdseed to create a clean wallet with a custom seed.

Followup PRs can make this significantly more useful, e.g.

#15006 adds a password argument to createwallet
TopUpKeyPool() should look for imported keys
allow import of a receive and change descriptor with private keys, which also sets the hd seed (custom BIP32 derivation paths, limiting the address type at the wallet level)
TopUpKeyPool() should look for public keys (if WALLET_FLAG_DISABLE_PRIVATE_KEYS is set)

fanquake added the label Wallet on Dec 12, 2018

Sjors force-pushed on Dec 12, 2018

Sjors commented at 5:14 pm on December 12, 2018: member

Some context:

#14075 (comment) “Import watch only pubkeys to the keypool if private keys are disabled” currently has importmulti add stuff to the keypool
I had difficulty creating a test for handling PSBT in watch-only wallets, where I wanted to mock the signed PSBT by importing the same private private keys into a second wallet and signing it with that: https://github.com/bitcoin/bitcoin/pull/14912/files

Sjors force-pushed on Dec 12, 2018

DrahtBot commented at 9:31 pm on December 12, 2018: 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:

#15226 (Allow creating blank (empty) wallets (alternative) by achow101)
#15006 (Add option to create an encrypted wallet by achow101)
#13756 (wallet: “avoid_reuse” wallet flag for improved privacy by kallewoof)

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 doc/release-notes-14936.md:4 in 9822ff692f outdated

0@@ -0,0 +1,4 @@
1+Miscellaneous RPC changes
2+------------
3+
4+- `createwallet` can now create an empty wallet

promag commented at 10:01 pm on December 12, 2018:

nit

0- The RPC `createwallet` has now the optional `empty` argument that allows to create empty wallets.

in test/functional/wallet_createwallet.py:39 in 9822ff692f outdated

34+        assert_raises_rpc_error(-4,"Error: Private keys are disabled for this wallet", w1.getnewaddress)
35+        assert_raises_rpc_error(-4,"Error: Private keys are disabled for this wallet", w1.getrawchangeaddress)
36+        w1.importpubkey(w0.getaddressinfo(address1)['pubkey'])
37+
38+        self.log.info("Test empty creation with private keys disabled.")
39+        self.nodes[0].createwallet('w2', True, True)

promag commented at 10:02 pm on December 12, 2018:

nit, could use named arguments.

Sjors commented at 3:17 pm on December 20, 2018:

Good idea

in src/wallet/wallet.cpp:192 in 9822ff692f outdated

186@@ -187,9 +187,13 @@ CPubKey CWallet::GenerateNewKey(WalletBatch &batch, bool internal)
187     int64_t nCreationTime = GetTime();
188     CKeyMetadata metadata(nCreationTime);
189 
190-    // use HD key derivation if HD was enabled during wallet creation
191+    // use HD key derivation if HD was enabled during wallet creation and a seed is present
192     if (IsHDEnabled()) {
193-        DeriveNewChildKey(batch, metadata, secret, (CanSupportFeature(FEATURE_HD_SPLIT) ? internal : false));
194+        if (HasHDSeed()) {

promag commented at 10:07 pm on December 12, 2018:

nit

0if (!HasHDSeed()) throw std::runtime_error(std::string(__func__) + ": No HD Seed");
1
2DeriveNewChildKey(batch, metadata, secret, (CanSupportFeature(FEATURE_HD_SPLIT) ? internal : false));

in src/wallet/rpcwallet.cpp:2567 in 9822ff692f outdated

2563@@ -2563,6 +2564,11 @@ static UniValue createwallet(const JSONRPCRequest& request)
2564         disable_privatekeys = request.params[1].get_bool();
2565     }
2566 
2567+    bool empty = false;

promag commented at 10:08 pm on December 12, 2018:

nit, create_empty.

promag commented at 10:09 pm on December 12, 2018: member

Concept ACK.

hebasto commented at 6:19 am on December 13, 2018: member

Concept ACK.

laanwj commented at 9:50 am on December 13, 2018: member

Concept ACK @jonasschnelli is this in line with your idea of supporting hardware wallets?

Sjors commented at 6:39 pm on December 13, 2018: member

It’s definitely in line with my idea of adding hardware wallets :-)

See also #14145.

jonasschnelli commented at 7:23 pm on December 13, 2018: contributor

I’m not opposed against this,… but this seems not to be in line with hardware wallet support (yet). Because sethdseed does set the secret master key rather than the extended public account key. Exposing the seed defeats the purpose of a hardware wallet.

But once this is turned into “watch-only-HD”/ public-key-derivation, then I guess this is useful.

The question then would be, if we don’t want to directly add an xpub argument to the createwallet call.

in test/functional/wallet_createwallet.py:34 in 9822ff692f outdated

29+        address1 = w0.getnewaddress()
30+
31+        self.log.info("Test disableprivatekeys creation.")
32+        self.nodes[0].createwallet('w1', True)
33+        w1 = node.get_wallet_rpc('w1')
34+        assert_raises_rpc_error(-4,"Error: Private keys are disabled for this wallet", w1.getnewaddress)

practicalswift commented at 10:10 am on December 14, 2018:

Missing whitespace after , – applies throughout this PR :-)

Sjors commented at 5:32 pm on December 14, 2018:

Thanks @practicalswift. What Python linter are you running that Travis isn’t?

practicalswift commented at 5:45 pm on December 14, 2018:

@Sjors We run flake8 in Travis but the rule E231 (“missing whitespace after ‘,’”) isn’t enabled. Please consider opening a PR fixing that :-)

in src/wallet/wallet.cpp:1391 in 9822ff692f outdated

1441+    AssertLockHeld(cs_wallet);
1442+    // -usehd was removed in 0.16, so all wallets with FEATURE_PRE_SPLIT_KEYPOOL are HD
1443+    // For older wallets the presence of seed_id indicates if HD is enabled.
1444+    // Support for empty HD capable wallets was added in 0.18
1445+    return CanSupportFeature(FEATURE_PRE_SPLIT_KEYPOOL) || !hdChain.seed_id.IsNull();
1446+}

achow101 commented at 5:44 pm on December 19, 2018:

Call HasHDSeed instead of doing the same check here

in src/wallet/wallet.cpp:4068 in 9822ff692f outdated

4065         if ((wallet_creation_flags & WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
4066             //selective allow to set flags
4067             walletInstance->SetWalletFlag(WALLET_FLAG_DISABLE_PRIVATE_KEYS);
4068+        } else if (create_empty) {
4069+            // Do not generate a new seed
4070         } else {

achow101 commented at 5:46 pm on December 19, 2018:

Instead of an empty else if block, change the else to be else if (!create_empty).

achow101 commented at 7:21 pm on December 19, 2018: member

Calling encryptwallet on an empty wallet does not work. I’m getting;

DeriveNewSeed: AddKeyPubKey failed

I would expect either that we can’t encrypt an empty wallet (bummer, because then we can’t have a wallet encrypted from the very beginning), or that it would encrypt and have a newly generated seed (which is what I think you were trying to do).

Edit: https://github.com/achow101/bitcoin/commit/d3b5d7bc27b68e77696e0599a38c1851128c226c fixes this issue.

Sjors force-pushed on Dec 20, 2018

Sjors commented at 3:31 pm on December 20, 2018: member

@achow101 thanks, I see you included that commit in #15006

Addressed @promag’s, @practicalswift ’s and @achow101’s comments.

Rebased because Travis was going nuts.

Sjors force-pushed on Dec 20, 2018

in test/functional/wallet_createwallet.py:32 in 0cc35e0a0b outdated

27+        self.nodes[0].createwallet(wallet_name='w0')
28+        w0 = node.get_wallet_rpc('w0')
29+        address1 = w0.getnewaddress()
30+
31+        self.log.info("Test disableprivatekeys creation.")
32+        self.nodes[0].createwallet(wallet_name='w1', disable_private_keys='true')

Sjors commented at 5:03 pm on December 20, 2018:

@jnewbery this test fails with --usecli if I use a boolean True rather than a string 'true'. Is that by design? Two of the Travis machines don’t seem to like this syntax either.

jnewbery commented at 7:27 pm on December 20, 2018:

Looks like this is a variation of the issue fixed here: https://github.com/bitcoin/bitcoin/commit/a3fa4d6a6acf19d640a1d5879a00aa1f059e2380 but for named arguments. Fix it with something like:

 0diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py
 1index 031a8824b..8225992c2 100755
 2--- a/test/functional/test_framework/test_node.py
 3+++ b/test/functional/test_framework/test_node.py
 4@@ -434,7 +434,7 @@ class TestNodeCLI():
 5     def send_cli(self, command=None, *args, **kwargs):
 6         """Run bitcoin-cli command. Deserializes returned string as python object."""
 7         pos_args = [str(arg).lower() if type(arg) is bool else str(arg) for arg in args]
 8-        named_args = [str(key) + "=" + str(value) for (key, value) in kwargs.items()]
 9+        named_args = [str(key) + "=" + (str(value).lower() if type(value) is bool else str(value)) for (key, value) in kwargs.items()]
10         assert not (pos_args and named_args), "Cannot use positional arguments and named arguments in the same bitcoin-cli call"
11         p_args = [self.binary, "-datadir=" + self.datadir] + self.options
12         if named_args:

jnewbery commented at 7:28 pm on December 20, 2018: member

Concept ACK. Had a very quick skim and changes look sensible.

Will review once Travis is happy.

in src/wallet/wallet.cpp:4002 in 0cc35e0a0b outdated

3995@@ -3979,20 +3996,25 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,
3996 
3997     if (fFirstRun)
3998     {
3999-        // ensure this wallet.dat can only be opened by clients supporting HD with chain split and expects no default key
4000-        walletInstance->SetMinVersion(FEATURE_LATEST);
4001+        // ensure this wallet can only be opened by clients supporting HD with chain split and expects no default key
4002+        if (!create_empty) {
4003+            walletInstance->SetMinVersion(FEATURE_PRE_SPLIT_KEYPOOL);

Empact commented at 10:26 pm on December 20, 2018:

Why not stay with FEATURE_LATEST here? It is currently equal to FEATURE_PRE_SPLIT_KEYPOOL, and presumably we will want to stick with current feature support when we make the next breaking change.

Sjors commented at 12:58 pm on December 21, 2018:

@Empact ~~in an earlier version I added a new feature version for this, but then I realized it wasn’t necessary. Forgot to remove this bit. Fixed.~~ On second thought I’m worried that previous versions would treat this as a non-HD wallet and upgrade it, which may not be what the user intended. So it seems safer to add a new version number.

achow101 commented at 4:19 pm on January 7, 2019:

I think instead of using a version number you should use a wallet flag that is required. The flag can be unset once the wallet is no longer empty. This would remove the need for a new version number while still preventing older software from opening the wallet.

Sjors force-pushed on Dec 21, 2018

QA: Fix -usecli converting named bool args to non-lowercase e12df93e59

Sjors force-pushed on Dec 21, 2018

[wallet] support creating an empty wallet 8ce17c0fb0

Sjors force-pushed on Dec 21, 2018

Sjors commented at 4:51 pm on December 21, 2018: member

Travis is happy now.

Added LOCK(cs_wallet) to IsHDEnabled because it uses CanSupportFeature which has an AssertLockHeld.

I added a new wallet version FEATURE_ALLOW_EMPTY which is set as the minimum when creating an empty wallet. When creating a non-empty wallet we stick to FEATURE_PRE_SPLIT_KEYPOOL as the minimum version.

achow101 commented at 3:32 pm on December 24, 2018: member

I don’t think it is necessary to set different version numbers depending on whether the wallet can be empty. This is effectively reintroducing the optional version stuff that we had done for HD wallets and HD chain split, and reconciling those was a major pain. I don’t see why it is necessary to do that again.

Sjors commented at 4:23 pm on December 25, 2018: member

What do you mean by “optional version stuff”?

The alternative seems to be to disallow <= v0.17 nodes from opening a v0.18 wallet, even though only a minitory of users need the empty wallet feature.

achow101 commented at 4:43 pm on December 25, 2018: member

What do you mean by “optional version stuff”?

The original HD wallet stuff was optional and the option was determined by the wallet version number. Non-HD wallets had a lower version number than HD wallets. So an optional feature was determined by the version number. But later when a change to the wallet affected both non-HD and HD wallets and required a version number bump, this optional feature thing switched via the version number had to go away, and making it go away was a pain. In general, I don’t think that we shouldn’t have optional features be indicated by the version number. Since we have wallet flags now, I think that instead optional features should be indicated through the flags, and if a flag is set that we don’t know about, we should throw an error. If you set a flag in the upper 32 bits, an error will be thrown if the flag is set but the flag is unknown, so I think you should use a flag there instead of a version number. The flag for disable private keys does the same thing.

The alternative seems to be to disallow <= v0.17 nodes from opening a v0.18 wallet, even though only a minitory of users need the empty wallet feature.

That certainly wouldn’t be ideal, but I don’t think that is really that big of a concern. I think it is far more likely for someone who upgraded to 0.18 from 0.17 to then downgrade to 0.17 than for someone who starts with 0.18 (and thus makes a new wallet) to then downgrade to 0.17.

in src/wallet/wallet.cpp:3228 in 8ce17c0fb0

3223@@ -3214,6 +3224,9 @@ bool CWallet::TopUpKeyPool(unsigned int kpSize)
3224         if (IsLocked())
3225             return false;
3226 
3227+        // If wallet supports HD but no seed is present, do not top up keypool
3228+        if(IsHDEnabled() && !HasHDSeed()) return true;

laanwj commented at 12:49 pm on January 2, 2019:

maybe roll this logic (which is repeated below) into a function IsEmpty()?

Edit: hmm I guess empty is a bad name here as it could also imply that the wallet has no transactions, or no funds…

laanwj commented at 12:54 pm on January 2, 2019: member

Concept ACK as this is a dependency for #15006

utACK 8ce17c0fb0ea51e8047db3d5a23bef747bbedf3a

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

promag commented at 7:11 pm on January 3, 2019: member

Should this option be exposed in the GUI (after #13100)?

achow101 commented at 7:35 pm on January 3, 2019: member

@promag I think it should

jonasschnelli commented at 8:05 pm on January 3, 2019: contributor

Tested a bit. If I create an empty createwallet dummy false true and generate an address via GUI I get this (invalid) state:

getnewaddress gives me Error: Keypool ran out, please call keypoolrefill first (code -12) but I would expect that the key pool gets refilled automatically? not?

Sjors commented at 3:08 pm on January 7, 2019: member

@jonasschnelli an empty wallet does not have an HD seed, so there is no way to refill the keypool. Calling sethdseed will add such a seed.

I’m surprised the GUI doesn’t handle this correctly, since the keypool could already run out before this change. Normally it just greys out the new address button. I’ll try to reproduce.

Sjors commented at 1:35 pm on January 9, 2019: member

@achow101 wrote inline:

I think instead of using a version number you should use a wallet flag that is required. The flag can be unset once the wallet is no longer empty. This would remove the need for a new version number while still preventing older software from opening the wallet.

I’m happy with either option. Mandatory flags explicitly say what a wallet can’t do, which is arguably better than inferring capabilities from version numbers. Any other takes?

laanwj commented at 12:49 pm on January 16, 2019: member

I’m surprised the GUI doesn’t handle this correctly, since the keypool could already run out before this change. Normally it just greys out the new address button. I’ll try to reproduce.

Right—apparently the GUI doesn’t do correct error handling here, it should handle the case where it’s impossible to generate an address.

in src/wallet/wallet.cpp:4020 in 8ce17c0fb0

4020-        }
4021+        } // Otherwise, do not generate a new seed
4022 
4023         // Top up the keypool
4024-        if (!walletInstance->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) && !walletInstance->TopUpKeyPool()) {
4025+        if (!create_empty && !walletInstance->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) && !walletInstance->TopUpKeyPool()) {

ryanofsky commented at 4:59 pm on January 18, 2019:

Why do you need to check create_empty here if TopUpKeyPool already returns true when there’s no seed? I think you should add a code comment here if checking create_empty is necessary for some reason, and probably drop it otherwise to simplify this code and be consistent with other TopUpKeyPool calls.

ryanofsky approved

ryanofsky commented at 5:00 pm on January 18, 2019: member

utACK 8ce17c0fb0ea51e8047db3d5a23bef747bbedf3a, but I agree with @achow101 in preferring a new feature flag to a new version number here.

In this case, since it is unlikely there’d ever be a new version of the wallet which dropped support for unset HD seeds, there’s little practical difference between using a new flag or a new version. I prefer flags anyway just for sanity when reading code, because flags make it possible to reason about one feature at a time without having to think about the history of the project. The counterargument in favor of preferring versions to flags is that using flags leads to a combinatorial explosion of configurations that can’t all be tested and make the project less stable. I think this argument is spurious because you can always disallow combinations of flags you don’t want to test, without the need to confuse future maintainers of the code forever with version number comparisons.

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

I’m surprised the GUI doesn’t handle this correctly, since the keypool could already run out before this change. Normally it just greys out the new address button. I’ll try to reproduce.

This error is because the grey out condition is that private keys are disable.

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

Closing in favour of #15226.

Sjors closed this on Jan 22, 2019

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

laanwj referenced this in commit 7c09e209ef on Jan 31, 2019

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

Munkybooty referenced this in commit 2fa3899964 on Aug 21, 2021

Munkybooty referenced this in commit a1cef14cce on Aug 21, 2021

Munkybooty referenced this in commit 8c6c481d4a on Aug 23, 2021

Munkybooty referenced this in commit 0a3ce36ba6 on Aug 24, 2021

Munkybooty referenced this in commit 9c696c5a14 on Aug 24, 2021

Munkybooty referenced this in commit 81bfd0c463 on Aug 24, 2021

UdjinM6 referenced this in commit 8d38af1289 on Aug 24, 2021

Munkybooty referenced this in commit baa76aa331 on Aug 24, 2021

MarcoFalke locked this on Dec 16, 2021

Support creating an empty wallet #14938

Conflicts