Support creating an empty wallet #14938

Sjors · 2018-12-12T17:02:33Z

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.

Add option to create an encrypted wallet #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)

Sjors · 2018-12-12T17:14:44Z

Some context:

Import watch only pubkeys to the keypool if private keys are disabled #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

DrahtBot · 2018-12-12T21:31:40Z

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.

promag

Concept ACK.

doc/release-notes-14936.md

test/functional/wallet_createwallet.py

src/wallet/wallet.cpp

src/wallet/rpcwallet.cpp

hebasto · 2018-12-13T06:19:41Z

Concept ACK.

laanwj · 2018-12-13T09:50:51Z

Concept ACK

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

Sjors · 2018-12-13T18:39:12Z

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

See also #14145.

jonasschnelli · 2018-12-13T19:23:30Z

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.

test/functional/wallet_createwallet.py

src/wallet/wallet.cpp

achow101 · 2018-12-19T19:21:34Z

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: achow101@d3b5d7b fixes this issue.

Sjors · 2018-12-20T15:31:59Z

@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 · 2018-12-20T17:03:16Z

test/functional/wallet_createwallet.py

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

Looks like this is a variation of the issue fixed here: a3fa4d6 but for named arguments. Fix it with something like:

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

jnewbery · 2018-12-20T19:28:50Z

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

Will review once Travis is happy.

Empact · 2018-12-20T22:26:57Z

src/wallet/wallet.cpp

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.

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

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 · 2018-12-21T16:51:13Z

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 · 2018-12-24T15:32:58Z

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 · 2018-12-25T16:23:45Z

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 · 2018-12-25T16:43:57Z

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.

laanwj · 2019-01-02T12:49:05Z

src/wallet/wallet.cpp

            return false;

+        // If wallet supports HD but no seed is present, do not top up keypool
+        if(IsHDEnabled() && !HasHDSeed()) return true;


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 · 2019-01-02T12:54:34Z

Concept ACK as this is a dependency for #15006

utACK 8ce17c0

promag · 2019-01-03T19:11:14Z

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

achow101 · 2019-01-03T19:35:30Z

@promag I think it should

jonasschnelli · 2019-01-03T20:05:21Z

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 · 2019-01-07T15:08:56Z

@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 · 2019-01-09T13:35:38Z

@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 · 2019-01-16T12:49:13Z

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.

ryanofsky

utACK 8ce17c0, 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.

ryanofsky · 2019-01-18T16:59:04Z

src/wallet/wallet.cpp


        // Top up the keypool
-        if (!walletInstance->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) && !walletInstance->TopUpKeyPool()) {
+        if (!create_empty && !walletInstance->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) && !walletInstance->TopUpKeyPool()) {


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.

achow101 · 2019-01-21T19:31:11Z

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 · 2019-01-22T17:15:32Z

Closing in favour of #15226.

…ate changing 2bc4c3e Notify the GUI that the keypool has changed to set the receive button (Andrew Chow) 14bcdbe Check for more than private keys disabled to show receive button (Andrew Chow) Pull request description: Currently the Receive button in the GUI is displayed enabled or disabled by the initial state of the wallet when the wallet is first loaded. The button is only enabled or disabled depending on whether the disable private keys flag is set when the wallet is loaded. However, future changes to the wallet means that this initial state and check may no longer be accurate. #14938 introduces empty wallets which do not have private keys. An empty wallet that is loaded should have the Receive button disabled, and then it should become enabled once `sethdseed` is used so that a keypool can be generated and new keys generated. Likewise, with #14075, a wallet can be loaded with no keypool initially, so the button should be disabled. Later, public keys can be imported into the keypool, at which time the button should become enabled. When the keypool runs out again (no new keys are generated as the keypool only consists of imports), the button should become disabled. This PR makes it so that the button becomes enabled and disabled as the keypool state changes. The check for whether to enable or disable the receive button has changed to checking whether it is possible to get new keys. It now checks for whether the wallet has an HD seed and, if not, whether the private keys are disabled. When an action happens which would make it possible for a new address to be retrieved or make it possible for a no more addresses to be retrieved, a signal is emitted which has the GUI recheck the conditions for the Receive button. These actions are setting a new HD seed, topping up the keypool, retrieving a key from the keypool, and returning a key to the keypool. Tree-SHA512: eff15a5337f4c64ecd7169414fb47053c04f6a0f0130341b6dd9799ac4d79f451e25284701c668971fca33f0909d5352a474a2c12349375bedfdb59b63077d50

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

…pool state changing 2bc4c3e Notify the GUI that the keypool has changed to set the receive button (Andrew Chow) 14bcdbe Check for more than private keys disabled to show receive button (Andrew Chow) Pull request description: Currently the Receive button in the GUI is displayed enabled or disabled by the initial state of the wallet when the wallet is first loaded. The button is only enabled or disabled depending on whether the disable private keys flag is set when the wallet is loaded. However, future changes to the wallet means that this initial state and check may no longer be accurate. bitcoin#14938 introduces empty wallets which do not have private keys. An empty wallet that is loaded should have the Receive button disabled, and then it should become enabled once `sethdseed` is used so that a keypool can be generated and new keys generated. Likewise, with bitcoin#14075, a wallet can be loaded with no keypool initially, so the button should be disabled. Later, public keys can be imported into the keypool, at which time the button should become enabled. When the keypool runs out again (no new keys are generated as the keypool only consists of imports), the button should become disabled. This PR makes it so that the button becomes enabled and disabled as the keypool state changes. The check for whether to enable or disable the receive button has changed to checking whether it is possible to get new keys. It now checks for whether the wallet has an HD seed and, if not, whether the private keys are disabled. When an action happens which would make it possible for a new address to be retrieved or make it possible for a no more addresses to be retrieved, a signal is emitted which has the GUI recheck the conditions for the Receive button. These actions are setting a new HD seed, topping up the keypool, retrieving a key from the keypool, and returning a key to the keypool. Tree-SHA512: eff15a5337f4c64ecd7169414fb47053c04f6a0f0130341b6dd9799ac4d79f451e25284701c668971fca33f0909d5352a474a2c12349375bedfdb59b63077d50

fanquake added the Wallet label Dec 12, 2018

Sjors force-pushed the 2018/12/create-empty-wallet branch from dddff68 to 2c60129 Compare December 12, 2018 17:06

Sjors force-pushed the 2018/12/create-empty-wallet branch 2 times, most recently from 6d603dd to 9822ff6 Compare December 12, 2018 17:34

promag reviewed Dec 12, 2018

View reviewed changes

doc/release-notes-14936.md Outdated Show resolved Hide resolved

test/functional/wallet_createwallet.py Outdated Show resolved Hide resolved

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

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

practicalswift reviewed Dec 14, 2018

View reviewed changes

test/functional/wallet_createwallet.py Outdated Show resolved Hide resolved

Sjors mentioned this pull request Dec 15, 2018

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

Closed

achow101 reviewed Dec 19, 2018

View reviewed changes

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

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

achow101 mentioned this pull request Dec 19, 2018

Add option to create an encrypted wallet #15006

Merged

Sjors force-pushed the 2018/12/create-empty-wallet branch from 9822ff6 to a3755ca Compare December 20, 2018 15:31

Sjors force-pushed the 2018/12/create-empty-wallet branch 2 times, most recently from efbfc9e to 0cc35e0 Compare December 20, 2018 17:02

Sjors commented Dec 20, 2018

View reviewed changes

Empact reviewed Dec 20, 2018

View reviewed changes

Sjors force-pushed the 2018/12/create-empty-wallet branch from 0cc35e0 to 65f7276 Compare December 21, 2018 13:17

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

e12df93

Sjors force-pushed the 2018/12/create-empty-wallet branch from 65f7276 to 1bc6a02 Compare December 21, 2018 14:08

[wallet] support creating an empty wallet

8ce17c0

Sjors force-pushed the 2018/12/create-empty-wallet branch from 1bc6a02 to 8ce17c0 Compare December 21, 2018 14:42

laanwj reviewed Jan 2, 2019

View reviewed changes

This was referenced Jan 17, 2019

gui: Add dynamic wallets support #13100

Closed

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

Closed

ryanofsky approved these changes Jan 18, 2019

View reviewed changes

This was referenced Jan 21, 2019

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

Merged

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

Merged

Sjors closed this Jan 22, 2019

jnewbery mentioned this pull request Jan 24, 2019

Do not import private keys to wallets with private keys disabled #15235

Merged

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

Support creating an empty wallet #14938

Support creating an empty wallet #14938

Conversation

Sjors commented Dec 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Dec 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Dec 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hebasto commented Dec 13, 2018

Uh oh!

laanwj commented Dec 13, 2018

Uh oh!

Sjors commented Dec 13, 2018

Uh oh!

jonasschnelli commented Dec 13, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

achow101 commented Dec 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Dec 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors Dec 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery Dec 20, 2018

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Dec 20, 2018

Uh oh!

Empact Dec 20, 2018

Choose a reason for hiding this comment

Uh oh!

Sjors Dec 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achow101 Jan 7, 2019

Choose a reason for hiding this comment

Uh oh!

Sjors commented Dec 21, 2018

Uh oh!

achow101 commented Dec 24, 2018

Uh oh!

Sjors commented Dec 25, 2018

Uh oh!

achow101 commented Dec 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

promag commented Jan 3, 2019

Uh oh!

achow101 commented Jan 3, 2019

Uh oh!

jonasschnelli commented Jan 3, 2019

Uh oh!

Sjors commented Jan 7, 2019

Uh oh!

Sjors commented Dec 12, 2018 •

edited

Loading

Sjors commented Dec 12, 2018 •

edited

Loading

DrahtBot commented Dec 12, 2018 •

edited

Loading

achow101 commented Dec 19, 2018 •

edited

Loading

Sjors commented Dec 20, 2018 •

edited

Loading

Sjors Dec 20, 2018 •

edited

Loading

Sjors Dec 21, 2018 •

edited

Loading

achow101 commented Dec 25, 2018 •

edited

Loading

laanwj Jan 2, 2019 •

edited

Loading

laanwj commented Jan 2, 2019 •

edited

Loading