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

achow101 · 2018-08-26T19:49:20Z

If the wallet has private keys disabled, allow importing public keys into the keypool. A keypool option has been added to importmulti in order to signal that the keys should be added to the keypool.

promag · 2018-08-26T23:02:19Z

test/functional/wallet_importmulti.py

@@ -467,6 +467,67 @@ def run_test (self):
        import_pub2 = self.nodes[0].getaddressinfo(addr2)['pubkey']
        assert_equal(pub2, import_pub2)

+        # Import some public keys to the keypool of a no privkey wallet


Commit "Fetch keys from keypool when private keys are disabled"

Could move test to a different commit?

promag · 2018-08-26T23:02:26Z

test/functional/wallet_importmulti.py

@@ -467,6 +467,67 @@ def run_test (self):
        import_pub2 = self.nodes[0].getaddressinfo(addr2)['pubkey']
        assert_equal(pub2, import_pub2)

+        # Import some public keys to the keypool of a no privkey wallet
+        self.log.info("Adding pubkey to keypool of disableprivkey wallet")
+        self.nodes[1].createwallet("noprivkeys", True)


Commit "Fetch keys from keypool when private keys are disabled"

Use named argument disable_private_keys=True?

promag · 2018-08-26T23:04:02Z

src/wallet/rpcdump.cpp

@@ -1198,6 +1219,7 @@ UniValue importmulti(const JSONRPCRequest& mainRequest)
            "      \"internal\": <true>                                    , (boolean, optional, default: false) Stating whether matching outputs should be treated as not incoming payments\n"
            "      \"watchonly\": <true>                                   , (boolean, optional, default: false) Stating whether matching outputs should be considered watched even when they're not spendable, only allowed if keys are empty\n"
            "      \"label\": <label>                                      , (string, optional, default: '') Label to assign to the address (aka account name, for now), only allowed with internal=false\n"
+            "      \"keypool\": <true|false>                               , (boolean, optional, default: true) If true, adds the pubkeys to the keypool if private keys are disabled for the wallet.\n"


Commit "Add option to importmulti add an imported pubkey to the keypool"

Default false.

promag · 2018-08-26T23:04:33Z

src/wallet/rpcdump.cpp

+
+        // Add to keypool only works with pubkeys
+        if (add_keypool && pubKeys.size() == 0) {
+            throw JSONRPCError(RPC_INVALID_PARAMETER, "Only pubkeys be imported to the keypool");


Commit "Add option to importmulti add an imported pubkey to the keypool"

... can be ...

instagibbs · 2018-08-27T15:15:45Z

src/qt/walletmodel.cpp

@@ -559,7 +559,7 @@ bool WalletModel::isWalletEnabled()

 bool WalletModel::privateKeysDisabled() const
 {
-    return m_wallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS);
+    return m_wallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) && m_wallet->KeypoolCountExternalKeys() == 0;


This function should be renamed or called into from a new one.

WalletModel::KeypoolEmptyAndPrivkeysDisabled or something obnoxiously explicit.

instagibbs · 2018-08-27T15:26:36Z

src/wallet/rpcwallet.cpp

    }

-    LOCK(pwallet->cs_wallet);
+    if (!pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) && !pwallet->IsLocked()) {


Might be time to make a TryToTopUpKeyPool function for when we will continue regardless. We do this in a number of places in the PR and probably elsewhere.

TopUpKeypool actually already has this check in it so it won't do anything when the disable private keys flag is set. This is just an extra belt and suspenders check.

src/wallet/wallet.cpp

achow101 · 2018-08-28T00:03:11Z

Rebased. The first commit is still from #14019

src/interfaces/wallet.h

instagibbs · 2018-08-29T13:29:15Z

src/wallet/wallet.cpp

+    {
+        LOCK(cs_wallet);
+        if (internal) {
+            return setInternalKeyPool.size() == 0;


mu-nit: add commit for KeypoolCountInternalKeys

I think that's unnecssary.

instagibbs · 2018-08-29T14:28:52Z

CWallet::CreateTransaction needs to check for keys in change keypool even if WALLET_FLAG_DISABLE_PRIVATE_KEYS is set.

test/functional/wallet_importmulti.py

Sjors · 2018-09-13T17:57:33Z

Concept ACK. Lightly tested via your combined branch 3fa968e: I was able to import a bunch of receive and change keys. I was also able to receive coins on a bech32 address and send from it.

For some reason when I composed the transaction it picked change address with index 1 instead of with index 0. Is that an existing rule?

What I also found confusing, though I don't know if that's this PR, or the other two or just existing weirdness, is how dumpwallet shows p2sh-p2wpkh addresses under # addr, even though I launched bitcoind with -addresstype=bech32.

achow101 · 2018-09-13T23:32:36Z

For some reason when I composed the transaction it picked change address with index 1 instead of with index 0. Is that an existing rule?

Maybe your import command was weird? It should be adding them in the order of the import command so you should be getting them in that order too.

src/wallet/rpcdump.cpp

Sjors · 2018-10-04T03:31:00Z

What's a good way to inspect the wallet / keypool?

achow101 · 2018-10-04T12:16:47Z

@Sjors You can use dumpwallet

Sjors · 2018-10-05T03:50:49Z

@achow101 dumpwallet doesn't contain the bip32 derivation paths though.

I'll try with getaddressinfo.

achow101 · 2018-10-15T21:07:12Z

I will rework this to not require #14019 as soon as I have time (in a few days)

@achow101

…ption a607c9a [Doc] importmulti: add missing description of keypool option (David A. Harding) Pull request description: Option was added in #14075 but not documented there. CC: @achow101 Tree-SHA512: dcb6421fa1be3d733d7a00c1b57ffd591fe76c02d1c479e729089c118bec52f53bd7ebdb5454b3b1c7603ab189e91682a688b673a7f6b04fa8610c4249711217

Summary: * Add a method to add a pubkey to the keypool Introduces AddKeypoolPubkey in order to add a pubkey to the keypool * Fetch keys from keypool when private keys are disabled When private keys are disabled, still fetch keys from the keypool if the keypool has keys. Those keys come from importing them and adding them to the keypool. * Add option to importmulti add an imported pubkey to the keypool Adds a new option to importmulti where the pubkeys specified in the import object can be added to the keypool. This only works if the wallet has private keys disabled. * Test pubkey import to keypool * Import public keys in order Do public key imports in the order that they are specified in the import or in the descriptor range. This is a backport of Core [[bitcoin/bitcoin#14075 | PR14075]] Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D6366

…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

@achow101

…ypool option a607c9a [Doc] importmulti: add missing description of keypool option (David A. Harding) Pull request description: Option was added in bitcoin#14075 but not documented there. CC: @achow101 Tree-SHA512: dcb6421fa1be3d733d7a00c1b57ffd591fe76c02d1c479e729089c118bec52f53bd7ebdb5454b3b1c7603ab189e91682a688b673a7f6b04fa8610c4249711217

@achow101

…ypool option a607c9a [Doc] importmulti: add missing description of keypool option (David A. Harding) Pull request description: Option was added in bitcoin#14075 but not documented there. CC: @achow101 Tree-SHA512: dcb6421fa1be3d733d7a00c1b57ffd591fe76c02d1c479e729089c118bec52f53bd7ebdb5454b3b1c7603ab189e91682a688b673a7f6b04fa8610c4249711217

@achow101

…ypool option a607c9a [Doc] importmulti: add missing description of keypool option (David A. Harding) Pull request description: Option was added in bitcoin#14075 but not documented there. CC: @achow101 Tree-SHA512: dcb6421fa1be3d733d7a00c1b57ffd591fe76c02d1c479e729089c118bec52f53bd7ebdb5454b3b1c7603ab189e91682a688b673a7f6b04fa8610c4249711217

@achow101

…ypool option a607c9a [Doc] importmulti: add missing description of keypool option (David A. Harding) Pull request description: Option was added in bitcoin#14075 but not documented there. CC: @achow101 Tree-SHA512: dcb6421fa1be3d733d7a00c1b57ffd591fe76c02d1c479e729089c118bec52f53bd7ebdb5454b3b1c7603ab189e91682a688b673a7f6b04fa8610c4249711217

@achow101

…ypool option a607c9a [Doc] importmulti: add missing description of keypool option (David A. Harding) Pull request description: Option was added in bitcoin#14075 but not documented there. CC: @achow101 Tree-SHA512: dcb6421fa1be3d733d7a00c1b57ffd591fe76c02d1c479e729089c118bec52f53bd7ebdb5454b3b1c7603ab189e91682a688b673a7f6b04fa8610c4249711217

…ate keys are disabled f4b00b7 Import public keys in order (Andrew Chow) 9e1551b Test pubkey import to keypool (Andrew Chow) 513719c Add option to importmulti add an imported pubkey to the keypool (Andrew Chow) 9b81fd1 Fetch keys from keypool when private keys are disabled (Andrew Chow) 99cccb9 Add a method to add a pubkey to the keypool (Andrew Chow) Pull request description: If the wallet has private keys disabled, allow importing public keys into the keypool. A `keypool` option has been added to `importmulti` in order to signal that the keys should be added to the keypool. Tree-SHA512: e88ea7bf726c13031aa739389a0c2662e6b22a4f9a4dc45b042418c692a950d98f170e0db80eb59e9c9063cda8765eaa85b2927d1790b9625744f7a87bad5fc8

@achow101

…ypool option a607c9a [Doc] importmulti: add missing description of keypool option (David A. Harding) Pull request description: Option was added in bitcoin#14075 but not documented there. CC: @achow101 Tree-SHA512: dcb6421fa1be3d733d7a00c1b57ffd591fe76c02d1c479e729089c118bec52f53bd7ebdb5454b3b1c7603ab189e91682a688b673a7f6b04fa8610c4249711217

…ate keys are disabled f4b00b7 Import public keys in order (Andrew Chow) 9e1551b Test pubkey import to keypool (Andrew Chow) 513719c Add option to importmulti add an imported pubkey to the keypool (Andrew Chow) 9b81fd1 Fetch keys from keypool when private keys are disabled (Andrew Chow) 99cccb9 Add a method to add a pubkey to the keypool (Andrew Chow) Pull request description: If the wallet has private keys disabled, allow importing public keys into the keypool. A `keypool` option has been added to `importmulti` in order to signal that the keys should be added to the keypool. Tree-SHA512: e88ea7bf726c13031aa739389a0c2662e6b22a4f9a4dc45b042418c692a950d98f170e0db80eb59e9c9063cda8765eaa85b2927d1790b9625744f7a87bad5fc8

@achow101

…ypool option a607c9a [Doc] importmulti: add missing description of keypool option (David A. Harding) Pull request description: Option was added in bitcoin#14075 but not documented there. CC: @achow101 Tree-SHA512: dcb6421fa1be3d733d7a00c1b57ffd591fe76c02d1c479e729089c118bec52f53bd7ebdb5454b3b1c7603ab189e91682a688b673a7f6b04fa8610c4249711217

…ate keys are disabled f4b00b7 Import public keys in order (Andrew Chow) 9e1551b Test pubkey import to keypool (Andrew Chow) 513719c Add option to importmulti add an imported pubkey to the keypool (Andrew Chow) 9b81fd1 Fetch keys from keypool when private keys are disabled (Andrew Chow) 99cccb9 Add a method to add a pubkey to the keypool (Andrew Chow) Pull request description: If the wallet has private keys disabled, allow importing public keys into the keypool. A `keypool` option has been added to `importmulti` in order to signal that the keys should be added to the keypool. Tree-SHA512: e88ea7bf726c13031aa739389a0c2662e6b22a4f9a4dc45b042418c692a950d98f170e0db80eb59e9c9063cda8765eaa85b2927d1790b9625744f7a87bad5fc8

@achow101

…ypool option a607c9a [Doc] importmulti: add missing description of keypool option (David A. Harding) Pull request description: Option was added in bitcoin#14075 but not documented there. CC: @achow101 Tree-SHA512: dcb6421fa1be3d733d7a00c1b57ffd591fe76c02d1c479e729089c118bec52f53bd7ebdb5454b3b1c7603ab189e91682a688b673a7f6b04fa8610c4249711217

…ate keys are disabled f4b00b7 Import public keys in order (Andrew Chow) 9e1551b Test pubkey import to keypool (Andrew Chow) 513719c Add option to importmulti add an imported pubkey to the keypool (Andrew Chow) 9b81fd1 Fetch keys from keypool when private keys are disabled (Andrew Chow) 99cccb9 Add a method to add a pubkey to the keypool (Andrew Chow) Pull request description: If the wallet has private keys disabled, allow importing public keys into the keypool. A `keypool` option has been added to `importmulti` in order to signal that the keys should be added to the keypool. Tree-SHA512: e88ea7bf726c13031aa739389a0c2662e6b22a4f9a4dc45b042418c692a950d98f170e0db80eb59e9c9063cda8765eaa85b2927d1790b9625744f7a87bad5fc8

achow101 force-pushed the watch-only-keypool branch from 56ab4be to d7d9ac9 Compare August 26, 2018 20:06

DrahtBot mentioned this pull request Aug 26, 2018

Import key origin data through descriptors in importmulti #14021

Merged

promag reviewed Aug 26, 2018

View reviewed changes

fanquake added the Wallet label Aug 26, 2018

DrahtBot mentioned this pull request Aug 27, 2018

Remove accounts rpcs #14023

Merged

instagibbs reviewed Aug 27, 2018

View reviewed changes

DrahtBot added the Needs rebase label Aug 27, 2018

achow101 force-pushed the watch-only-keypool branch from d7d9ac9 to 5f2a499 Compare August 28, 2018 00:01

DrahtBot mentioned this pull request Aug 28, 2018

Import pubkeys when importing p2sh with importmulti #14019

Closed

DrahtBot removed the Needs rebase label Aug 28, 2018

achow101 force-pushed the watch-only-keypool branch from 5f2a499 to 85b6075 Compare August 28, 2018 02:16

instagibbs reviewed Aug 29, 2018

View reviewed changes

achow101 force-pushed the watch-only-keypool branch 2 times, most recently from 50cff8c to acb05a1 Compare August 30, 2018 14:31

achow101 mentioned this pull request Sep 4, 2018

Hardware wallet support #14145

Closed

practicalswift reviewed Sep 10, 2018

View reviewed changes

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

achow101 force-pushed the watch-only-keypool branch from acb05a1 to 5022a35 Compare September 13, 2018 23:36

DrahtBot mentioned this pull request Sep 27, 2018

Multiprocess bitcoin #10102

Draft

practicalswift reviewed Sep 30, 2018

View reviewed changes

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

DrahtBot mentioned this pull request Oct 10, 2018

Add SegWit support to importmulti #14454

Merged

achow101 mentioned this pull request Oct 12, 2018

Add Coldcard bitcoin-dot-org/Bitcoin.org#2612

Closed

DrahtBot mentioned this pull request Oct 20, 2018

Rpc help helper class #14502

Closed

harding mentioned this pull request Feb 16, 2019

[Doc] importmulti: add missing description of keypool option #15426

Merged

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

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

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

Uh oh!

Conversation

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

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!

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!

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!

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!

Uh oh!

achow101 commented Aug 28, 2018

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

instagibbs commented Aug 29, 2018

Uh oh!

Uh oh!

Sjors commented Sep 13, 2018

Uh oh!

achow101 commented Sep 13, 2018

Uh oh!

Uh oh!

Sjors commented Oct 4, 2018

Uh oh!

achow101 commented Oct 4, 2018

Uh oh!

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

Uh oh!

Uh oh!

achow101 commented Oct 15, 2018

Uh oh!

Uh oh!

achow101 commented Aug 26, 2018 •

edited

Loading

Sjors commented Oct 5, 2018 •

edited

Loading