Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Feb 10, 2019

Conversation

Projects
None yet
9 participants
@achow101
Copy link
Member

commented Jan 22, 2019

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 fanquake added the Wallet label Jan 22, 2019

@achow101 achow101 force-pushed the achow101:blank-wallets branch Jan 22, 2019

@achow101 achow101 force-pushed the achow101:blank-wallets branch Jan 22, 2019

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2019

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.

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

This comment has been minimized.

Copy link
Member

commented Jan 22, 2019

Concept ACK

@achow101 achow101 force-pushed the achow101:blank-wallets branch 2 times, most recently Jan 22, 2019

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

@Sjors

This comment has been minimized.

Copy link
Member

commented Jan 22, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2019

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

@Sjors
Copy link
Member

left a comment

Other than my questions and remarks c0bfc2d7160e7c861d9a3e678e1dd9b6213a8a86 looks good.

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

This comment has been minimized.

Copy link
Member Author

commented Jan 22, 2019

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

This comment has been minimized.

Copy link
Member

commented Jan 22, 2019

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

@ryanofsky

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2019

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

This comment has been minimized.

Copy link
Member

commented Jan 23, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Jan 23, 2019

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 achow101 force-pushed the achow101:blank-wallets branch to ac4d021 Jan 23, 2019

@Sjors Sjors referenced this pull request Jan 24, 2019

Open

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

0 of 2 tasks complete
@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 24, 2019

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.

@achow101 achow101 force-pushed the achow101:blank-wallets branch from ac4d021 to 2021dfa Jan 29, 2019

@achow101

This comment has been minimized.

Copy link
Member Author

commented Jan 29, 2019

Rebased

@Sjors

This comment has been minimized.

Copy link
Member

commented Feb 1, 2019

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.

@ryanofsky
Copy link
Contributor

left a comment

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.

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

@achow101 achow101 force-pushed the achow101:blank-wallets branch from 4d2b8b4 to 1cdb8d8 Feb 7, 2019

@achow101

This comment has been minimized.

Copy link
Member Author

commented Feb 7, 2019

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
Copy link
Contributor

left a comment

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

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

@achow101 achow101 force-pushed the achow101:blank-wallets branch from 1cdb8d8 to 8061ea4 Feb 7, 2019

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Feb 7, 2019

Tested ACK 8061ea4

@gwillen

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2019

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 meshcollider added this to the 0.18.0 milestone Feb 8, 2019

@Sjors

This comment has been minimized.

Copy link
Member

commented Feb 8, 2019

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

@ryanofsky
Copy link
Contributor

left a comment

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

Show resolved Hide resolved src/interfaces/wallet.cpp
Show resolved Hide resolved src/wallet/wallet.cpp Outdated
Show resolved Hide resolved src/wallet/wallet.cpp Outdated
Show resolved Hide resolved doc/release-notes-15226.md Outdated
Show resolved Hide resolved src/wallet/wallet.cpp Outdated
@@ -3880,7 +3891,7 @@ UniValue sethdseed(const JSONRPCRequest& request)
LOCK(pwallet->cs_wallet);

// Do not do anything to non-HD wallets
if (!pwallet->IsHDEnabled()) {
if (!pwallet->IsBlank() && !pwallet->IsHDEnabled()) {

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 8, 2019

Contributor

In commit "[wallet] Support creating a blank wallet" (8061ea4)

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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 9, 2019

Contributor

In commit "[wallet] Support creating a blank wallet" (8061ea4)

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.

This comment has been minimized.

Copy link
@achow101

achow101 Feb 9, 2019

Author Member

There's already a check for that flag above.

Show resolved Hide resolved src/wallet/wallet.cpp Outdated
Show resolved Hide resolved src/wallet/wallet.h Outdated
@@ -3880,7 +3891,7 @@ UniValue sethdseed(const JSONRPCRequest& request)
LOCK(pwallet->cs_wallet);

// Do not do anything to non-HD wallets
if (!pwallet->IsHDEnabled()) {
if (!pwallet->IsBlank() && !pwallet->IsHDEnabled()) {

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 9, 2019

Contributor

In commit "[wallet] Support creating a blank wallet" (8061ea4)

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.

Show resolved Hide resolved src/qt/walletmodel.cpp Outdated

@achow101 achow101 force-pushed the achow101:blank-wallets branch from 8061ea4 to 49044a4 Feb 9, 2019

@achow101

This comment has been minimized.

Copy link
Member Author

commented Feb 9, 2019

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 achow101 force-pushed the achow101:blank-wallets branch from 49044a4 to 8b5533e Feb 9, 2019

@ryanofsky
Copy link
Contributor

left a comment

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

@@ -176,6 +176,11 @@ static UniValue getnewaddress(const JSONRPCRequest& request)

LOCK(pwallet->cs_wallet);

if (!pwallet->CanGetAddresses()) {

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 9, 2019

Contributor

In commit "[wallet] Support creating a blank wallet" (8b5533e)

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.

This comment has been minimized.

Copy link
@achow101

achow101 Feb 9, 2019

Author Member

I've added a comment

@@ -237,6 +242,10 @@ static UniValue getrawchangeaddress(const JSONRPCRequest& request)

LOCK(pwallet->cs_wallet);

if (!pwallet->CanGetAddresses(true)) {

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 9, 2019

Contributor

In commit "[wallet] Support creating a blank wallet" (8b5533e)

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

Show resolved Hide resolved src/wallet/wallet.cpp
@@ -3416,7 +3460,7 @@ void CWallet::ReturnKey(int64_t nIndex, bool fInternal, const CPubKey& pubkey)

bool CWallet::GetKeyFromPool(CPubKey& result, bool internal)
{
if (IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
if (!CanGetAddresses(internal)) {

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 9, 2019

Contributor

In commit "[wallet] Support creating a blank wallet" (8b5533e)

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.

This comment has been minimized.

Copy link
@achow101

achow101 Feb 9, 2019

Author Member

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.

This comment has been minimized.

Copy link
@Sjors

Sjors Feb 10, 2019

Member

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.

@achow101 achow101 force-pushed the achow101:blank-wallets branch from 8b5533e to 707eaba Feb 9, 2019

@meshcollider

This comment has been minimized.

Copy link
Member

commented Feb 10, 2019

utACK 707eaba

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

@Sjors

Sjors approved these changes Feb 10, 2019

Copy link
Member

left a comment

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.

@@ -3416,7 +3460,7 @@ void CWallet::ReturnKey(int64_t nIndex, bool fInternal, const CPubKey& pubkey)

bool CWallet::GetKeyFromPool(CPubKey& result, bool internal)
{
if (IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
if (!CanGetAddresses(internal)) {

This comment has been minimized.

Copy link
@Sjors

Sjors Feb 10, 2019

Member

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.

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

This comment has been minimized.

Copy link
@Sjors

Sjors Feb 10, 2019

Member

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.

This comment has been minimized.

Copy link
@achow101

achow101 Feb 10, 2019

Author Member

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

This comment has been minimized.

Copy link
@Sjors

Sjors Feb 10, 2019

Member

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

@@ -703,7 +713,7 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase)
Lock();
Unlock(strWalletPassphrase);

// if we are using HD, replace the HD seed with a new one
// if we are using HD, create a fresh seed or replace the existing seed with a new one

This comment has been minimized.

Copy link
@Sjors

Sjors Feb 10, 2019

Member

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

This comment has been minimized.

Copy link
@achow101

achow101 Feb 10, 2019

Author Member

Outdated comment. I've changed it back.

This comment has been minimized.

Copy link
@Sjors

Sjors Feb 10, 2019

Member

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

[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.

@achow101 achow101 force-pushed the achow101:blank-wallets branch from 707eaba to 7687f78 Feb 10, 2019

@Sjors

This comment has been minimized.

Copy link
Member

commented Feb 10, 2019

re-utACK 7687f78

@bitcoin bitcoin deleted a comment Feb 10, 2019

@bitcoin bitcoin deleted a comment Feb 10, 2019

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

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

meshcollider added a commit that referenced this pull request Feb 10, 2019

Merge #15226: Allow creating blank (empty) wallets (alternative)
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

@meshcollider meshcollider removed this from Blockers in High-priority for review Feb 10, 2019

domob1812 added a commit to xaya/xaya that referenced this pull request Feb 11, 2019

Merge branch 'namecoin'
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.

laanwj added a commit that referenced this pull request May 16, 2019

Merge #15006: Add option to create an encrypted wallet
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.