Import watch only pubkeys to the keypool if private keys are disabled by achow101 · Pull Request #14075 · bitcoin/bitcoin

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

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

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

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

Default false.

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

src/wallet/rpcdump.cpp

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

... can be ...

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

src/qt/walletmodel.cpp

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

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

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)

instagibbs · 2019-02-14T19:19:18Z

Good news: I'm wrong, and I'm the one who wrote the logic originally :(

Test and I'm happy.

instagibbs · 2019-02-14T19:26:41Z

test/functional/wallet_importmulti.py

./test/functional/wallet_importmulti.py:730:35: F821 undefined name 'xpub'

Sjors · 2019-02-14T19:28:41Z

@instagibbs keys from a ranged descriptor are imported in order. Feel free to replace this test:

        # Test importing of a ranged descriptor without keys, check order
        self.log.info("Should import the ranged descriptor with specified range as solvable")
        self.test_importmulti({"desc": desc,
                               "timestamp": "now",
                               "range": {"end": 9}},
                              success=True,
                              warnings=["Some private keys are missing, outputs will be considered watchonly. If this is intentional, specify the watchonly flag."])
        # Check the first two addresses:
        for address in addresses:
            test_address(self.nodes[1],
                         key.p2sh_p2wpkh_addr,
                         solvable=True)

        # Check keys were imported in the correct order:
        for i in range(10):
            address = self.nodes[1].getnewaddress()
            assert_equal(self.nodes[1].getaddressinfo(address)["hdkeypath"], "m/0'/0'/%s'" % i)

instagibbs · 2019-02-14T19:33:27Z

Test fix only change.

reACK 32fd3bb

instagibbs · 2019-02-14T19:43:32Z

re-ACK 38fbc10

Ordering test fixed, at least locally.

Sjors · 2019-02-14T19:56:31Z

utACK a6e2c3331023d11112f8e9d176aa823d84b3f5ce (only adds a test since my last ACK)

meshcollider · 2019-02-14T22:32:22Z

utACK 38fbc10

Introduces AddKeypoolPubkey in order to add a pubkey to the keypool

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.

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.

Do public key imports in the order that they are specified in the import or in the descriptor range.

meshcollider · 2019-02-14T23:51:19Z

LGTM f4b00b7

Sjors · 2019-02-15T07:41:58Z

Post merge utACK f4b00b7

@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

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

achow101 force-pushed the watch-only-keypool branch from a6e2c33 to 0f9f035 Compare February 14, 2019 19:22

instagibbs reviewed Feb 14, 2019

View reviewed changes

achow101 force-pushed the watch-only-keypool branch from 0f9f035 to 32fd3bb Compare February 14, 2019 19:29

achow101 force-pushed the watch-only-keypool branch from 32fd3bb to 38fbc10 Compare February 14, 2019 19:35

achow101 added 3 commits February 14, 2019 18:14

Add a method to add a pubkey to the keypool

99cccb9

Introduces AddKeypoolPubkey in order to add a pubkey to the keypool

Fetch keys from keypool when private keys are disabled

9b81fd1

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

513719c

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.

achow101 force-pushed the watch-only-keypool branch from 38fbc10 to 037e695 Compare February 14, 2019 23:18

achow101 added 2 commits February 14, 2019 18:33

Test pubkey import to keypool

9e1551b

Import public keys in order

f4b00b7

Do public key imports in the order that they are specified in the import or in the descriptor range.

achow101 force-pushed the watch-only-keypool branch from 037e695 to f4b00b7 Compare February 14, 2019 23:34

meshcollider merged commit f4b00b7 into bitcoin:master Feb 14, 2019

Sjors mentioned this pull request Feb 15, 2019

[wallet] allow adding pubkeys from imported private keys to keypool #15414

Closed

harding mentioned this pull request Feb 16, 2019

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

Merged

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!

instagibbs commented Feb 14, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sjors commented Feb 14, 2019

Uh oh!

instagibbs commented Feb 14, 2019

Uh oh!

instagibbs commented Feb 14, 2019

Uh oh!

Sjors commented Feb 14, 2019

Uh oh!

meshcollider commented Feb 14, 2019

Uh oh!

meshcollider commented Feb 14, 2019

Uh oh!

Sjors commented Feb 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

achow101 commented Aug 26, 2018 •

edited

Loading

Sjors commented Oct 5, 2018 •

edited

Loading