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

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

Merged
merged 5 commits into from Feb 14, 2019

Conversation

Projects
None yet
10 participants
@achow101
Copy link
Member

commented Aug 26, 2018

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.

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

This comment has been minimized.

Copy link
@promag

promag Aug 26, 2018

Member

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

Could move test to a different commit?

This comment has been minimized.

Copy link
@achow101

achow101 Aug 28, 2018

Author Member

Done

test/functional/wallet_importmulti.py Outdated
@@ -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)

This comment has been minimized.

Copy link
@promag

promag Aug 26, 2018

Member

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

Use named argument disable_private_keys=True?

This comment has been minimized.

Copy link
@achow101

achow101 Aug 28, 2018

Author Member

Done

src/wallet/rpcdump.cpp Outdated
@@ -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"

This comment has been minimized.

Copy link
@promag

promag Aug 26, 2018

Member

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

Default false.

This comment has been minimized.

Copy link
@achow101

achow101 Aug 28, 2018

Author Member

Fixed

src/wallet/rpcdump.cpp Outdated

// 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");

This comment has been minimized.

Copy link
@promag

promag Aug 26, 2018

Member

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

... can be ...

This comment has been minimized.

Copy link
@achow101

achow101 Aug 28, 2018

Author Member

Fixed

@fanquake fanquake added the Wallet label Aug 26, 2018

@DrahtBot DrahtBot referenced this pull request Aug 27, 2018

Merged

Remove accounts rpcs #14023

src/qt/walletmodel.cpp Outdated
@@ -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 comment has been minimized.

Copy link
@instagibbs

instagibbs Aug 27, 2018

Member

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

WalletModel::KeypoolEmptyAndPrivkeysDisabled or something obnoxiously explicit.

This comment has been minimized.

Copy link
@achow101

achow101 Aug 28, 2018

Author Member

Done

src/wallet/rpcwallet.cpp Outdated
}

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

This comment has been minimized.

Copy link
@instagibbs

instagibbs Aug 27, 2018

Member

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.

This comment has been minimized.

Copy link
@achow101

achow101 Aug 27, 2018

Author Member

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.

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 achow101 force-pushed the achow101:watch-only-keypool branch Aug 28, 2018

@achow101

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2018

Rebased. The first commit is still from #14019

@DrahtBot DrahtBot removed the Needs rebase label Aug 28, 2018

@achow101 achow101 force-pushed the achow101:watch-only-keypool branch Aug 28, 2018

Show resolved Hide resolved src/interfaces/wallet.h Outdated
src/wallet/wallet.cpp Outdated
{
LOCK(cs_wallet);
if (internal) {
return setInternalKeyPool.size() == 0;

This comment has been minimized.

Copy link
@instagibbs

instagibbs Aug 29, 2018

Member

mu-nit: add commit for KeypoolCountInternalKeys

This comment has been minimized.

Copy link
@achow101

achow101 Aug 29, 2018

Author Member

I think that's unnecssary.

@instagibbs

This comment has been minimized.

Copy link
Member

commented Aug 29, 2018

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

@achow101 achow101 force-pushed the achow101:watch-only-keypool branch 2 times, most recently Aug 29, 2018

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

This comment has been minimized.

Copy link
Member

commented Sep 13, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Sep 13, 2018

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.

@achow101 achow101 force-pushed the achow101:watch-only-keypool branch to 5022a35 Sep 13, 2018

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

This comment has been minimized.

Copy link
Member

commented Oct 4, 2018

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

@achow101

This comment has been minimized.

Copy link
Member Author

commented Oct 4, 2018

@Sjors You can use dumpwallet

@Sjors

This comment has been minimized.

Copy link
Member

commented Oct 5, 2018

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

I'll try with getaddressinfo.

@achow101 achow101 referenced this pull request Oct 12, 2018

Closed

Add Coldcard #2612

@achow101

This comment has been minimized.

Copy link
Member Author

commented Oct 15, 2018

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

@DrahtBot DrahtBot referenced this pull request Oct 20, 2018

Closed

Rpc help helper class #14502

@Sjors

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

I used a rather unpractical descriptor in my example above, with hardened derivation after the xpub. Let's try that again...

It works fine, so tACK a6e2c33, but I'm still confused why it works :-)

Here's a wallet where one address has received funds:

bitcoin-cli createwallet keypool true
bitcoin-cli -rpcwallet=keypool importmulti '[{"desc": "wpkh(tpubD6NzVbkrYhZ4YNXVQbNhMK1WqguFsUXceaVJKbmno2aZ3B6QfbMeraaYvnBSGpV3vxLyTTK9DYT1yoEck4XUScMzXoQ2U2oSmE2JyMedq3H/0/*)", "range": {"end": 2}, "timestamp": 1548979200, "watchonly": true, "keypool": true}]'

The transaction immediately appears in QT, although the balance is incorrect until the next restart.

bitcoin-cli -rpcwallet=keypool getaddressinfo tb1qsh65n95t9myg5xn44h9gmlmh2kej0l0yppqkqx
{
  "address": "tb1qsh65n95t9myg5xn44h9gmlmh2kej0l0yppqkqx",
  "scriptPubKey": "001485f549968b2ec88a1a75adca8dff7755b327fde4",
  "ismine": false,
  "solvable": true,
  "desc": "wpkh([85f54996]0245fa928afe25e7b1c38d3bc16aafd933dda8ea8c653396cfc1d87859ef14331d)",
  "iswatchonly": true,
  "isscript": false,
  "iswitness": true,
  "witness_version": 0,
  "witness_program": "85f549968b2ec88a1a75adca8dff7755b327fde4",
  "pubkey": "0245fa928afe25e7b1c38d3bc16aafd933dda8ea8c653396cfc1d87859ef14331d",
  "label": "",
  "ischange": false,
  "timestamp": 1548979200,
  "labels": [
    {
      "name": "",
      "purpose": "receive"
    }
  ]
}

Remember when @instagibbs added:

The second commit has watch keys imported to the keypool be marked as used when transactions are seen using them.

You then dropped that commit, because it was superseded by #14821.

The problem is that AddToWalletIfInvolvingMe only considers IsMine https://github.com/bitcoin/bitcoin/blame/master/src/wallet/wallet.cpp#L972.

But these imported public keys are merely Solvable so why aren't they skipped? Is the keypool somehow treated as IsMine even though getaddressinfo disagrees?

@achow101

This comment has been minimized.

Copy link
Member Author

commented Feb 11, 2019

But these imported public keys are merely Solvable so why aren't they skipped?

IsMine() returns true for things that are watch only, solvable, or spendable.

@instagibbs

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

More concretely, IsMine does a last-minute check if the exact scriptPubKey exists in the watchonly store if it fails at everything else.

@laanwj laanwj added this to the 0.18.0 milestone Feb 13, 2019

@instagibbs

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

Feel free to remove "built on" comment in OP so I don't keep checking if it's time to review in earnest :)

@instagibbs

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

There is no test for keys being imported in order, and it appears to not actually be implemented for descriptors with ranges? You're iterating over a map's keys, which will be sorted in CKeyID order.

@instagibbs

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

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

Test and I'm happy.

@achow101 achow101 force-pushed the achow101:watch-only-keypool branch from a6e2c33 to 0f9f035 Feb 14, 2019

]
result = wrpc.importmulti(
[{
'desc': 'wpkh(' + xpub + '/0h/0h/*' + ')',

This comment has been minimized.

Copy link
@instagibbs

instagibbs Feb 14, 2019

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

This comment has been minimized.

Copy link
@achow101

achow101 Feb 14, 2019

Author Member

Whoops

@Sjors

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

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

@achow101 achow101 force-pushed the achow101:watch-only-keypool branch from 0f9f035 to 32fd3bb Feb 14, 2019

@instagibbs

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

Test fix only change.

reACK 32fd3bb

@achow101 achow101 force-pushed the achow101:watch-only-keypool branch from 32fd3bb to 38fbc10 Feb 14, 2019

@instagibbs

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

re-ACK 38fbc10

Ordering test fixed, at least locally.

@Sjors

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

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

@meshcollider

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

utACK 38fbc10

achow101 added some commits Nov 6, 2018

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.

@achow101 achow101 force-pushed the achow101:watch-only-keypool branch from 38fbc10 to 037e695 Feb 14, 2019

achow101 added some commits Nov 6, 2018

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

@achow101 achow101 force-pushed the achow101:watch-only-keypool branch from 037e695 to f4b00b7 Feb 14, 2019

@meshcollider

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

LGTM f4b00b7

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

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

meshcollider added a commit to meshcollider/bitcoin that referenced this pull request Feb 14, 2019

Merge bitcoin#14075: Import watch only pubkeys to the keypool if priv…
…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
@Sjors

This comment has been minimized.

Copy link
Member

commented Feb 15, 2019

Post merge utACK f4b00b7

laanwj added a commit that referenced this pull request Feb 17, 2019

Merge #15426: [Doc] importmulti: add missing description of keypool o…
…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
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.