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

achow101 · 2019-01-22T03:31:53Z

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.

DrahtBot · 2019-01-22T04:19:28Z

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.

src/wallet/wallet.cpp

src/wallet/wallet.h

jonasschnelli · 2019-01-22T06:32:44Z

Concept ACK

Sjors · 2019-01-22T17:14:55Z

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.

ryanofsky · 2019-01-22T17:41:38Z

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

Sjors

Other than my questions and remarks c0bfc2d7160e7c861d9a3e678e1dd9b6213a8a86 looks good.

src/wallet/wallet.cpp

achow101 · 2019-01-22T21:07:15Z

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 · 2019-01-22T21:59:59Z

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

ryanofsky · 2019-01-22T23:22:01Z

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 · 2019-01-23T12:02:32Z

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 a8da482 and released in v0.17.0, should already prevent 0.16 nodes from opening a >=0.17.0 wallet:

FEATURE_PRE_SPLIT_KEYPOOL = 169900,
FEATURE_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 · 2019-01-23T19:34:14Z

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.

laanwj · 2019-01-24T15:23:05Z

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.

test/functional/test_framework/test_node.py

achow101 · 2019-01-29T19:57:15Z

Rebased

ryanofsky

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.

src/wallet/wallet.cpp

src/interfaces/wallet.cpp

achow101 · 2019-02-07T03:54:21Z

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

ryanofsky

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

src/wallet/wallet.cpp

jonasschnelli · 2019-02-07T20:00:37Z

Tested ACK 8061ea4611696b22d5d5dcdd73c1e053ffecc09d

gwillen · 2019-02-07T23:02:06Z

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.

Sjors · 2019-02-08T13:14:45Z

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

ryanofsky

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

src/interfaces/wallet.cpp

src/wallet/wallet.cpp

doc/release-notes-15226.md

src/wallet/wallet.cpp

ryanofsky · 2019-02-08T22:07:02Z

src/wallet/rpcwallet.cpp

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))

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.

There's already a check for that flag above.

src/wallet/wallet.cpp

src/wallet/wallet.h

ryanofsky · 2019-02-09T14:30:57Z

src/wallet/rpcwallet.cpp

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.

src/qt/walletmodel.cpp

achow101 · 2019-02-09T18:43:15Z

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.

ryanofsky

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

ryanofsky · 2019-02-09T19:36:00Z

src/wallet/rpcwallet.cpp

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.

I've added a comment

ryanofsky · 2019-02-09T19:37:52Z

src/wallet/rpcwallet.cpp

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.

src/wallet/wallet.cpp

ryanofsky · 2019-02-09T19:52:56Z

src/wallet/wallet.cpp

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.

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.

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.

meshcollider · 2019-02-10T10:02:17Z

utACK 707eaba

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

Sjors

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.

Sjors · 2019-02-10T16:13:10Z

src/wallet/wallet.cpp

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.

Sjors · 2019-02-10T16:32:00Z

src/wallet/wallet.h

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.

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

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

Sjors · 2019-02-10T16:35:51Z

src/wallet/wallet.cpp

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

Outdated comment. I've changed it back.

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 :-).

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.

Sjors · 2019-02-10T18:58:54Z

re-utACK 7687f78

7687f78 [wallet] Support creating a blank wallet (Andrew Chow) Pull request description: 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. Tree-SHA512: 824d685e11ac2259a26b5ece99c67a7bda94a570cd921472c464243ee356b7734595ad35cc439b34357135df041ed9cba951e6edac194935c3a55a1dc4fcbdea

Updated the address format in the new rpc_deriveaddresses.py regtest to Xaya's address versions. After bitcoin/bitcoin#15226, we have to explicitly set HDs seeds in some of the wallet unit tests, specifically for Xaya. That is the case because in Xaya, the initial / default wallet version already supports HD, and then the wallet refuses the generate keys without an HD seed set.

662d117 Add option to create an encrypted wallet (Andrew Chow) Pull request description: This PR adds a new `passphrase` argument to `createwallet` which will create a wallet that is encrypted with that passphrase. This is built on #15226 because it needs to first create an empty wallet, then encrypt the empty wallet and generate new keys that have only been stored in an encrypted state. ACKs for commit 662d11: laanwj: utACK 662d117 jnewbery: Looks great. utACK 662d117 Tree-SHA512: a53fc9a0f341eaec1614eb69abcf2d48eb4394bc89041ab69bfc05a63436ed37c65ad586c07fd37dc258ac7c7d5e4f7f93b4191407f5824bbf063b4c50894c4a

fanquake added the Wallet label Jan 22, 2019

achow101 force-pushed the blank-wallets branch from 9233a56 to 575dd54 Compare January 22, 2019 03:45

achow101 mentioned this pull request Jan 22, 2019

Add option to create an encrypted wallet #15006

Merged

achow101 force-pushed the blank-wallets branch from 575dd54 to c0bfc2d Compare January 22, 2019 04:10

jonasschnelli reviewed Jan 22, 2019

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

jonasschnelli reviewed Jan 22, 2019

View reviewed changes

src/wallet/wallet.h Outdated Show resolved Hide resolved

achow101 force-pushed the blank-wallets branch 2 times, most recently from d93a8df to c0bfc2d Compare January 22, 2019 06:54

laanwj changed the title ~~Allow creating blank (empty) wallets~~ Allow creating blank (empty) wallets (alternative) Jan 22, 2019

Sjors mentioned this pull request Jan 22, 2019

Support creating an empty wallet #14938

Closed

Sjors reviewed Jan 22, 2019

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

Sjors mentioned this pull request Jan 22, 2019

Import watch only pubkeys to the keypool if private keys are disabled #14075

Merged

achow101 force-pushed the blank-wallets branch from c0bfc2d to ac4d021 Compare January 23, 2019 20:56

Sjors mentioned this pull request Jan 24, 2019

[WIP] External signer support (e.g. hardware wallet) #14912

Closed

laanwj reviewed Jan 24, 2019

View reviewed changes

test/functional/test_framework/test_node.py Outdated Show resolved Hide resolved

DrahtBot added the Needs rebase label Jan 29, 2019

achow101 force-pushed the blank-wallets branch from ac4d021 to 2021dfa Compare January 29, 2019 19:55

jonasschnelli mentioned this pull request Jan 29, 2019

GUI: Change the receive button to respond to keypool state changing #15225

Merged

DrahtBot removed the Needs rebase label Jan 29, 2019

Sjors mentioned this pull request Feb 2, 2019

Build previous releases and run functional tests #12134

Merged

ryanofsky reviewed Feb 7, 2019

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

src/interfaces/wallet.cpp Outdated Show resolved Hide resolved

achow101 force-pushed the blank-wallets branch from 4d2b8b4 to 1cdb8d8 Compare February 7, 2019 03:51

ryanofsky reviewed Feb 7, 2019

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

achow101 force-pushed the blank-wallets branch from 1cdb8d8 to 8061ea4 Compare February 7, 2019 06:02

meshcollider added this to the 0.18.0 milestone Feb 8, 2019

ryanofsky reviewed Feb 8, 2019

View reviewed changes

ryanofsky reviewed Feb 9, 2019

View reviewed changes

achow101 force-pushed the blank-wallets branch from 8061ea4 to 49044a4 Compare February 9, 2019 18:38

achow101 force-pushed the blank-wallets branch from 49044a4 to 8b5533e Compare February 9, 2019 18:57

ryanofsky reviewed Feb 9, 2019

View reviewed changes

achow101 force-pushed the blank-wallets branch from 8b5533e to 707eaba Compare February 9, 2019 21:34

Sjors approved these changes Feb 10, 2019

View reviewed changes

achow101 force-pushed the blank-wallets branch from 707eaba to 7687f78 Compare February 10, 2019 17:25

bitcoin deleted a comment Feb 10, 2019

meshcollider merged commit 7687f78 into bitcoin:master Feb 10, 2019

maflcko mentioned this pull request Aug 19, 2019

[RFC] Long term plan for wallet command-line args #13044

Closed

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

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

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

Uh oh!

Conversation

achow101 commented Jan 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jan 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Uh oh!

Uh oh!

jonasschnelli commented Jan 22, 2019

Uh oh!

Sjors commented Jan 22, 2019

Uh oh!

ryanofsky commented Jan 22, 2019

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

achow101 commented Jan 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonasschnelli commented Jan 22, 2019

Uh oh!

ryanofsky commented Jan 22, 2019

Uh oh!

Sjors commented Jan 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented Jan 23, 2019

Uh oh!

laanwj commented Jan 24, 2019

Uh oh!

Uh oh!

achow101 commented Jan 29, 2019

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

achow101 commented Feb 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jonasschnelli commented Feb 7, 2019

Uh oh!

gwillen commented Feb 7, 2019

Uh oh!

Sjors commented Feb 8, 2019

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achow101 commented Jan 22, 2019 •

edited

Loading

DrahtBot commented Jan 22, 2019 •

edited

Loading

achow101 commented Jan 22, 2019 •

edited

Loading

Sjors commented Jan 23, 2019 •

edited

Loading

achow101 commented Feb 7, 2019 •

edited

Loading

Sjors Feb 10, 2019 •

edited

Loading