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

achow101 commented at 7:49 PM on August 26, 2018: member

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.

achow101 force-pushed on Aug 26, 2018

in test/functional/wallet_importmulti.py:693 in d7d9ac9654 outdated

 466 | @@ -467,6 +467,67 @@ def run_test (self):
 467 |          import_pub2 = self.nodes[0].getaddressinfo(addr2)['pubkey']
 468 |          assert_equal(pub2, import_pub2)
 469 |  
 470 | +        # Import some public keys to the keypool of a no privkey wallet

promag commented at 11:02 PM on August 26, 2018:

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

Could move test to a different commit?

achow101 commented at 12:01 AM on August 28, 2018:

Done

in test/functional/wallet_importmulti.py:472 in d7d9ac9654 outdated

 466 | @@ -467,6 +467,67 @@ def run_test (self):
 467 |          import_pub2 = self.nodes[0].getaddressinfo(addr2)['pubkey']
 468 |          assert_equal(pub2, import_pub2)
 469 |  
 470 | +        # Import some public keys to the keypool of a no privkey wallet
 471 | +        self.log.info("Adding pubkey to keypool of disableprivkey wallet")
 472 | +        self.nodes[1].createwallet("noprivkeys", True)

promag commented at 11:02 PM on August 26, 2018:

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

Use named argument disable_private_keys=True?

achow101 commented at 12:01 AM on August 28, 2018:

Done

in src/wallet/rpcdump.cpp:1222 in 803a91650d outdated

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

promag commented at 11:04 PM on August 26, 2018:

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

Default false.

achow101 commented at 12:02 AM on August 28, 2018:

Fixed

in src/wallet/rpcdump.cpp:887 in 803a91650d outdated

 882 | +            throw JSONRPCError(RPC_INVALID_PARAMETER, "Keys can only be imported to the keypool when private keys are disabled");
 883 | +        }
 884 | +
 885 | +        // Add to keypool only works with pubkeys
 886 | +        if (add_keypool && pubKeys.size() == 0) {
 887 | +            throw JSONRPCError(RPC_INVALID_PARAMETER, "Only pubkeys be imported to the keypool");

promag commented at 11:04 PM on August 26, 2018:

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

... can be ...

achow101 commented at 12:02 AM on August 28, 2018:

Fixed

fanquake added the label Wallet on Aug 26, 2018

in src/qt/walletmodel.cpp:562 in d7d9ac9654 outdated

 558 | @@ -559,7 +559,7 @@ bool WalletModel::isWalletEnabled()
 559 |  
 560 |  bool WalletModel::privateKeysDisabled() const
 561 |  {
 562 | -    return m_wallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS);
 563 | +    return m_wallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) && m_wallet->KeypoolCountExternalKeys() == 0;

instagibbs commented at 3:15 PM on August 27, 2018:

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

WalletModel::KeypoolEmptyAndPrivkeysDisabled or something obnoxiously explicit.

achow101 commented at 12:02 AM on August 28, 2018:

Done

in src/wallet/rpcwallet.cpp:179 in d7d9ac9654 outdated

 168 | +    if (pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) && pwallet->KeypoolCountExternalKeys() == 0) {
 169 | +        throw JSONRPCError(RPC_WALLET_ERROR, "Error: Private keys are disabled for this wallet and no keys in the external keypool");
 170 |      }
 171 |  
 172 | -    LOCK(pwallet->cs_wallet);
 173 | +    if (!pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) && !pwallet->IsLocked()) {

instagibbs commented at 3:26 PM on August 27, 2018:

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.

achow101 commented at 11:24 PM on August 27, 2018:

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.

in src/wallet/wallet.cpp:3426 in d7d9ac9654 outdated

3422 | @@ -3417,7 +3423,7 @@ bool CWallet::ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& keypool, bool fRe
3423 |          if (!IsLocked())
3424 |              TopUpKeyPool();
3425 |  
3426 | -        bool fReturningInternal = IsHDEnabled() && CanSupportFeature(FEATURE_HD_SPLIT) && fRequestedInternal;
3427 | +        bool fReturningInternal = ((IsHDEnabled() && CanSupportFeature(FEATURE_HD_SPLIT)) || IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS))  && fRequestedInternal;

instagibbs commented at 3:31 PM on August 27, 2018:

assumption here being using importmulti means you want to support hdsplit?

Perhaps break this into two lines:

bool fReturningInternal = fRequestedInternal;
fReturningInternal &= (IsHDEnabled() && CanSupportFeature(FEATURE_HD_SPLIT)) || IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS);

achow101 commented at 12:02 AM on August 28, 2018:

Done

in src/wallet/wallet.cpp:3443 in d7d9ac9654 outdated

3439 | @@ -3434,7 +3440,7 @@ bool CWallet::ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& keypool, bool fRe
3440 |          if (!batch.ReadPool(nIndex, keypool)) {
3441 |              throw std::runtime_error(std::string(__func__) + ": read failed");
3442 |          }
3443 | -        if (!HaveKey(keypool.vchPubKey.GetID())) {
3444 | +        if (!IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) && !HaveKey(keypool.vchPubKey.GetID())) {

instagibbs commented at 3:34 PM on August 27, 2018:

Belt and suspenders, we should be checking that this exists in our wallet regardless yes? Just not asking if we have the private key itself.

achow101 commented at 12:02 AM on August 28, 2018:

I changed this line to check for the pubkey in the wallet instead of the privkey.

in src/wallet/wallet.cpp:3488 in d7d9ac9654 outdated

3483 | @@ -3478,23 +3484,21 @@ void CWallet::ReturnKey(int64_t nIndex, bool fInternal, const CPubKey& pubkey)
3484 |  
3485 |  bool CWallet::GetKeyFromPool(CPubKey& result, bool internal)
3486 |  {
3487 | -    if (IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
3488 | +    LOCK(cs_wallet);
3489 | +    if (IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) && KeypoolCountExternalKeys() == 0) {

instagibbs commented at 3:37 PM on August 27, 2018:

This should be checking the keypool corresponding to internal, yes?

achow101 commented at 12:02 AM on August 28, 2018:

Fixed

DrahtBot added the label Needs rebase on Aug 27, 2018

achow101 force-pushed on Aug 28, 2018

achow101 commented at 12:03 AM on August 28, 2018: member

Rebased. The first commit is still from #14019

DrahtBot removed the label Needs rebase on Aug 28, 2018

achow101 force-pushed on Aug 28, 2018

in src/interfaces/wallet.h:276 in 85b6075ec7 outdated

 271 | @@ -272,6 +272,9 @@ class Wallet
 272 |      //! Register handler for watchonly changed messages.
 273 |      using WatchOnlyChangedFn = std::function<void(bool have_watch_only)>;
 274 |      virtual std::unique_ptr<Handler> handleWatchOnlyChanged(WatchOnlyChangedFn fn) = 0;
 275 | +
 276 | +    // Get the number of external keypool keys

instagibbs commented at 1:27 PM on August 29, 2018:

generalize comment

achow101 commented at 8:59 PM on August 29, 2018:

Done

in src/wallet/wallet.cpp:3277 in 85b6075ec7 outdated

3318 | +    }
3319 | +
3320 | +    {
3321 | +        LOCK(cs_wallet);
3322 | +        if (internal) {
3323 | +            return setInternalKeyPool.size() == 0;

instagibbs commented at 1:29 PM on August 29, 2018:

mu-nit: add commit for KeypoolCountInternalKeys

achow101 commented at 8:55 PM on August 29, 2018:

I think that's unnecssary.

instagibbs commented at 2:28 PM on August 29, 2018: member

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

achow101 force-pushed on Aug 29, 2018

achow101 force-pushed on Aug 30, 2018

in test/functional/wallet_importmulti.py:458 in acb05a1820 outdated

 453 | +        pub1 = self.nodes[1].getaddressinfo(addr1)['pubkey']
 454 | +        pub2 = self.nodes[1].getaddressinfo(addr2)['pubkey']
 455 | +        ms = self.nodes[1].createmultisig(2, [pub1, pub2])
 456 | +        result = self.nodes[0].importmulti(
 457 | +            [{
 458 | +                'scriptPubKey' : { 'address' : ms['address']},

practicalswift commented at 6:44 AM on September 10, 2018:

FWIW, PEP-8 whitespace nits:

./test/functional/wallet_importmulti.py:458:31: E203 whitespace before ':'
./test/functional/wallet_importmulti.py:458:35: E201 whitespace after '{'
./test/functional/wallet_importmulti.py:458:45: E203 whitespace before ':'
./test/functional/wallet_importmulti.py:459:31: E203 whitespace before ':'
./test/functional/wallet_importmulti.py:460:26: E203 whitespace before ':'
./test/functional/wallet_importmulti.py:482:31: E203 whitespace before ':'
./test/functional/wallet_importmulti.py:482:35: E201 whitespace after '{'
./test/functional/wallet_importmulti.py:482:45: E203 whitespace before ':'
./test/functional/wallet_importmulti.py:483:31: E203 whitespace before ':'
./test/functional/wallet_importmulti.py:484:26: E203 whitespace before ':'
./test/functional/wallet_importmulti.py:485:26: E203 whitespace before ':'
./test/functional/wallet_importmulti.py:504:31: E203 whitespace before ':'
./test/functional/wallet_importmulti.py:504:35: E201 whitespace after '{'
./test/functional/wallet_importmulti.py:504:45: E203 whitespace before ':'
./test/functional/wallet_importmulti.py:505:31: E203 whitespace before ':'
./test/functional/wallet_importmulti.py:506:26: E203 whitespace before ':'
./test/functional/wallet_importmulti.py:507:26: E203 whitespace before ':'
./test/functional/wallet_importmulti.py:508:27: E203 whitespace before ':'
./test/functional/wallet_importmulti.py:523:27: E203 whitespace before ':'
./test/functional/wallet_importmulti.py:523:31: E201 whitespace after '{'
./test/functional/wallet_importmulti.py:523:41: E203 whitespace before ':'
./test/functional/wallet_importmulti.py:524:27: E203 whitespace before ':'
./test/functional/wallet_importmulti.py:525:22: E203 whitespace before ':'
./test/functional/wallet_importmulti.py:526:22: E203 whitespace before ':'

achow101 commented at 11:36 PM on September 13, 2018:

Fixed

Sjors commented at 5:57 PM on September 13, 2018: member

Concept ACK. Lightly tested via your combined branch 3fa968ea343: 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 commented at 11:32 PM on September 13, 2018: member

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 force-pushed on Sep 13, 2018

in src/wallet/rpcdump.cpp:939 in 5022a35a6b outdated

 934 | +
 935 | +                if (!IsHex(pubkey)) {
 936 | +                    throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Pubkey must be a hex string");
 937 | +                }
 938 | +
 939 | +                std::vector<unsigned char> vData(ParseHex(pubkey));

practicalswift commented at 3:45 PM on September 30, 2018:

This vData shadows another local variable vData :-)

Sjors commented at 3:31 AM on October 4, 2018: member

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

achow101 commented at 12:16 PM on October 4, 2018: member

@Sjors You can use dumpwallet

Sjors commented at 3:50 AM on October 5, 2018: member

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

I'll try with getaddressinfo.

achow101 commented at 9:07 PM on October 15, 2018: member

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

DrahtBot added the label Needs rebase on Oct 31, 2018

Sjors commented at 12:18 PM on November 5, 2018: member

Probably best rebased on #14491 or its predecessor.

Here's my attempt at it: https://github.com/Sjors/bitcoin/tree/2018/11/watch-only-keypool

start with #14021
cherry-pick 0abb0b28e76224fb5b163b29937b7a08ed60d20a, bcb8b4834ada2fcdbdcdeeed7cbc95ee0b81f9b0
manually cherry-pick bcb8b4834ada2fcdbdcdeeed7cbc95ee0b81f9b0

It seems to work for a simple case, but haven't thoroughly tested it.

achow101 force-pushed on Nov 6, 2018

achow101 commented at 3:04 PM on November 6, 2018: member

Rebased onto #14491

achow101 force-pushed on Nov 6, 2018

DrahtBot removed the label Needs rebase on Nov 6, 2018

achow101 force-pushed on Nov 6, 2018

achow101 force-pushed on Nov 7, 2018

achow101 commented at 7:08 PM on November 8, 2018: member

I've added two commits contributed by @instagibbs. The first one makes it so that things are imported in the order they are listed in the import command or in the order expanded for ranged descriptors. The second commit has watch keys imported to the keypool be marked as used when transactions are seen using them.

achow101 force-pushed on Nov 8, 2018

DrahtBot commented at 9:30 AM on November 9, 2018: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#14021 (Import key origin data through descriptors in importmulti by achow101)

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.

fanquake deleted a comment on Nov 9, 2018

achow101 force-pushed on Nov 10, 2018

DrahtBot added the label Needs rebase on Nov 13, 2018

meshcollider commented at 10:29 AM on November 16, 2018: contributor

Light utACK https://github.com/bitcoin/bitcoin/pull/14075/commits/8e6b7bd5e9aeb8efe85068de8fc38cf0b22f1185

Will review more in-depth after the dependent PRs are merged and this is rebased

Sjors commented at 11:01 AM on December 12, 2018: member

I think we should make keypoolrefill and TopUpKeyPool work with just public keys.

When WALLET_FLAG_DISABLE_PRIVATE_KEYS is set we can skip the EnsureWalletIsUnlocked(pwallet) check.

TopUpKeyPool currently only creates random fresh keys (for legacy wallets) or derives new keys from the wallet HD seed. We could change it to first walk through solvable keys in the wallet.

For safety we could prevent it from putting keys without private keys into the keypool for wallets without WALLET_FLAG_DISABLE_PRIVATE_KEYS.

In that case we don't need the keypool option in importmulti.

This might make it easier to refactor towards a descriptor based wallet without a global keypool (cc @sipa), because we avoid adding another code path where stuff can be added to the keypool. In such a refactor a RPC method like getnewaddress will have to reference a descriptor. That would be the wallet receive descriptor by default. importmulti would then be able to set the wallet receive descriptor (in case the wallet is empty). It doesn't do keypool management stuff.

This would also let you create a new wallet without keys and then import private keys yourself via importmulti, something that currently doesn't work (at least not with getnewaddress and the GUI equivalent).

achow101 force-pushed on Dec 16, 2018

achow101 commented at 7:36 PM on December 16, 2018: member

Rebased.

I dropped the last commit as #14821 should handle that.

achow101 force-pushed on Dec 16, 2018

DrahtBot removed the label Needs rebase on Dec 16, 2018

DrahtBot added the label Needs rebase on Dec 24, 2018

achow101 force-pushed on Dec 24, 2018

achow101 commented at 3:29 PM on December 24, 2018: member

Rebased

DrahtBot removed the label Needs rebase on Dec 28, 2018

achow101 force-pushed on Jan 18, 2019

achow101 commented at 10:10 PM on January 18, 2019: member

I've added a commit which fixed an issue where the Request payment button was not being enabled once keys were imported to the keypool. That commit sends a signal to the GUI every time the keypool changes so that the button will be enabled and disabled as there keys are added and removed from the keypool.

achow101 force-pushed on Jan 19, 2019

achow101 force-pushed on Jan 22, 2019

achow101 commented at 12:01 AM on January 22, 2019: member

I split out 28c89d1 into it's own PR: #15225

achow101 force-pushed on Jan 22, 2019

Sjors commented at 5:54 PM on January 22, 2019: member

In light of #15226 (specifically the test comment "Imported private keys are currently ignored by the keypool") I'd like to revisit my [earlier remark]( #14075 (comment)):

I'd like to avoid messing with the keypool inside of importmulti. Instead I would prefer if TopUpKeyPool can deal with imported keys.

TopUpKeyPool currently only creates random fresh keys (for legacy wallets) or derives new keys from the wallet HD seed. We could change it to first walk through solvable keys in the wallet.

In that case we don't need the keypool option in importmulti.

achow101 force-pushed on Jan 23, 2019

jonasschnelli commented at 8:19 PM on January 29, 2019: contributor

Concept ACK

Ideally – but orthogonal to this PR – would be if someone could provide a xpub or range based descriptor during sethdseed (or new command) that would make the keypool act similar to the default HD keypool but derive watch only pub keys.

laanwj referenced this in commit 7c09e209ef on Jan 31, 2019

DrahtBot added the label Needs rebase on Jan 31, 2019

achow101 force-pushed on Jan 31, 2019

DrahtBot removed the label Needs rebase on Jan 31, 2019

Sjors commented at 8:24 PM on January 31, 2019: member

Tested a4bd25d, import works and GUI new address button updates as expected.

Sjors commented at 8:27 PM on January 31, 2019: member

if someone could provide a xpub or range based descriptor during sethdseed (or new command) that would make the keypool act similar to the default HD keypool but derive watch only pub keys.

I would prefer to hold off on that until we have a descriptor based wallet, which would make that trivial.

In the mean time you can set the import range argument for the descriptor extremely long for a (probably) good enough result.

DrahtBot added the label Needs rebase on Feb 1, 2019

achow101 force-pushed on Feb 5, 2019

DrahtBot removed the label Needs rebase on Feb 5, 2019

achow101 force-pushed on Feb 8, 2019

meshcollider added this to the milestone 0.18.0 on Feb 8, 2019

Sjors commented at 11:45 AM on February 9, 2019: member

I had some difficulty combining this with #15226 (blank wallet) and made a commit that does that, poorly: https://github.com/Sjors/bitcoin/commit/8988a96e2eff8bd6344f9e93032e86742e7dc3f8 However, the blank wallet PR has since changed again, so my commit is merely useful for inspiration and some extra tests. Don't let it hold back merging here.

I had to choose between the two PR's, I prefer this one, because it's needed for HWI.

meshcollider commented at 3:58 PM on February 9, 2019: contributor

I don't think we can get both this and #14021 in to 0.18 so I'll remove this from the milestone

meshcollider removed this from the milestone 0.18.0 on Feb 9, 2019

achow101 force-pushed on Feb 9, 2019

achow101 commented at 7:51 PM on February 9, 2019: member

I've actually decided to base this on #15226. In the version of #15226, I introduced CanGetAddresses and CanGenerateKeys which I am using here for determining whether there are things in the keypool to fetch keys instead of NoPrivkeysAndKeypoolEmpty (which did a similar thing but was less robust). Because of this, I have also replaced WalletModel::privateKeysDisabled with canGetaddresses.

in test/functional/wallet_importmulti.py:743 in 7369e1a459 outdated

 672 | +                'internal': True,
 673 | +                "timestamp": "now",
 674 | +            }]
 675 | +        )
 676 | +        assert result[0]['success']
 677 | +        assert result[1]['success']

Sjors commented at 4:52 PM on February 10, 2019:

Nit: assert_equal(wrpc.getwalletinfo()["keypoolsize"],2)

achow101 commented at 8:43 PM on February 10, 2019:

Done

in src/qt/bitcoingui.cpp:1205 in 5c73073d18 outdated

1201 | @@ -1202,7 +1202,7 @@ void BitcoinGUI::updateWalletStatus()
1202 |      }
1203 |      WalletModel * const walletModel = walletView->getWalletModel();
1204 |      setEncryptionStatus(walletModel->getEncryptionStatus());
1205 | -    setHDStatus(walletModel->privateKeysDisabled(), walletModel->wallet().hdEnabled());
1206 | +    setHDStatus(!walletModel->canGetAddresses(), walletModel->wallet().hdEnabled());

Sjors commented at 5:15 PM on February 10, 2019:

Nit: update documentation and argument name for setHDStatus.

achow101 commented at 8:43 PM on February 10, 2019:

I've reverted this change.

in src/qt/overviewpage.cpp:164 in 5c73073d18 outdated

 160 | @@ -161,7 +161,7 @@ void OverviewPage::setBalance(const interfaces::WalletBalances& balances)
 161 |  {
 162 |      int unit = walletModel->getOptionsModel()->getDisplayUnit();
 163 |      m_balances = balances;
 164 | -    if (walletModel->privateKeysDisabled()) {
 165 | +    if (!walletModel->canGetAddresses()) {

Sjors commented at 5:55 PM on February 10, 2019:

This is weird; whether we treat watch-only balances as ours shouldn't depend on if the keypool is empty. The original privateKeysDisabled() is more appropriate, because it's the only wallet type where we can be certain no private keys will be added at any time (saving us from having to deal with a bunch of UI glitches described below).

Fortunately you need to do something fairly contrived to make it misbehave:

Using my patch that allows adding private keys to the keypool, create a blank wallet with private keys enabled and import:

 importmulti '[{"desc": "sh(wpkh(tprv8ZgxMBicQKsPeuVhWwi6wuMQGfPKi9Li5GtX35jVNknACgqe3CY4g5xgkfDDJcmtF7o1QnxWDRYw4H5P26PXq7sbcUkEqeR4fg3Kxp2tigg/0h/0h/*))", "range": {"end": 2},  "keys": ["cSWSqykYtfkrWQ5FX6rCQPEME9iBaJcpqAgsCBeJDGQdrdS8V6G4", "cUHbt88Mu51u2Wz4xfuEPYCiAiNFRzuNZqDEDVGfGSz12DvPMRJu", "cPR5WCsjJr5vM3BTcwUji7u9fCW5pQPMsywS1fCxPbe4bf2XJEKG"], "timestamp": "now", "keypool": true}]'

I sent testnet coins to the first two of those keys. This means the keypool should be 1.

Now add a watch-only address:

importmulti '[{"desc": "sh(wpkh(tpubD6NzVbkrYhZ4YNXVQbNhMK1WqguFsUXceaVJKbmno2aZ3B6QfbMeraaYvnBSGpV3vxLyTTK9DYT1yoEck4XUScMzXoQ2U2oSmE2JyMedq3H/0h/0h/*))", "range": {"start": 3, "end": 3}, "timestamp": "now", "keypool": true, "watchonly": true}]'

Restart QT and notice there's a watch-only column. Generate addresses to deplete the keypool and restart QT, notice the watch-only column goes away and your balance now shows zero.

There's a few other QT glitches too, but not necessarily new:

balances don't update after importmulti, you have to restart QT
HD indicated doesn't update

achow101 commented at 8:44 PM on February 10, 2019:

I reverted that change

in src/qt/overviewpage.cpp:187 in 5c73073d18 outdated

 183 | @@ -184,7 +184,7 @@ void OverviewPage::setBalance(const interfaces::WalletBalances& balances)
 184 |      // for symmetry reasons also show immature label when the watch-only one is shown
 185 |      ui->labelImmature->setVisible(showImmature || showWatchOnlyImmature);
 186 |      ui->labelImmatureText->setVisible(showImmature || showWatchOnlyImmature);
 187 | -    ui->labelWatchImmature->setVisible(!walletModel->privateKeysDisabled() && showWatchOnlyImmature); // show watch-only immature balance
 188 | +    ui->labelWatchImmature->setVisible(walletModel->canGetAddresses() && showWatchOnlyImmature); // show watch-only immature balance

Sjors commented at 6:06 PM on February 10, 2019:

Same as above, but even more rare.

achow101 commented at 8:44 PM on February 10, 2019:

Reverted

in src/wallet/wallet.cpp:2760 in 5c73073d18 outdated

2755 | @@ -2756,7 +2756,7 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std
2756 |                  //  post-backup change.
2757 |  
2758 |                  // Reserve a new key pair from key pool
2759 | -                if (IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
2760 | +                if (!CanGetAddresses(true)) {
2761 |                      strFailReason = _("Can't generate a change-address key. Private keys are disabled for this wallet.");

Sjors commented at 6:07 PM on February 10, 2019:

Nit: drop / tweak "Private keys are disabled for this wallet."

achow101 commented at 8:54 PM on February 10, 2019:

Changed.

in src/wallet/wallet.cpp:3491 in 5c73073d18 outdated

3416 | @@ -3417,7 +3417,8 @@ bool CWallet::ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& keypool, bool fRe
3417 |          if (!IsLocked())
3418 |              TopUpKeyPool();
3419 |  
3420 | -        bool fReturningInternal = IsHDEnabled() && CanSupportFeature(FEATURE_HD_SPLIT) && fRequestedInternal;
3421 | +        bool fReturningInternal = fRequestedInternal;
3422 | +        fReturningInternal &= (IsHDEnabled() && CanSupportFeature(FEATURE_HD_SPLIT)) || IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS);

Sjors commented at 6:12 PM on February 10, 2019:

nit, add HasHDSeed() to complement IsHDEnabled(), and make it more clear what you're checking:

// All WALLET_FLAG_DISABLE_PRIVATE_KEYS wallets were created after HD_SPLIT was added.
const bool fSupportsInternal = (HasHDSeed() && IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) || CanSupportFeature(FEATURE_HD_SPLIT)
bool fReturningInternal = fRequestedInternal & fSupportsInternal;

achow101 commented at 8:48 PM on February 10, 2019:

We can do that later.

in src/wallet/wallet.h:1004 in dffff150e9 outdated

 992 | @@ -993,6 +993,7 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface
 993 |      bool NewKeyPool();
 994 |      size_t KeypoolCountExternalKeys() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
 995 |      bool TopUpKeyPool(unsigned int kpSize = 0);
 996 | +    void AddKeypoolPubkey(const CPubKey& pubkey, const bool internal);

Sjors commented at 6:34 PM on February 10, 2019:

Nit: call this new function from TopUpKeyPool, so it's more clear this function reuses existing code. It could take a error_message and an optional WalletBatch argument. If WalletBatch is set it should skip NotifyCanGetAddressesChanged(), though that gets a bit ugly.

achow101 commented at 8:45 PM on February 10, 2019:

I added a function AddKeypoolPubkeyWithDB which TopUpKeypool calls. AddKeypoolPubkey will make the WalletBatch, call AddKeypoolPubkeyWithDB and then NotifyCanGetAddressesChanged.

Sjors approved

Sjors commented at 6:38 PM on February 10, 2019: member

I'd still like to get this into 0.18 even if it's not tagged as such.

~tACK 8484a10~ (see newer comment) modulo privateKeysDisabled needs to be restored on the QT overview page (I can live with punting that).

For a followup I'd like to loosen the restriction of not adding imported private keys to the keypool. That's not necessary for HWI, so can wait until after the branching. Implemented here: https://github.com/Sjors/bitcoin/commits/2019/02/add_privkey_to_keypool

Consider squashing dffff150e98fc96c98e9e8caf40cec7391bbebfb into 39c95977b2435110114cffd620a2efdb02357360

DrahtBot added the label Needs rebase on Feb 10, 2019

achow101 force-pushed on Feb 10, 2019

DrahtBot removed the label Needs rebase on Feb 10, 2019

in test/functional/wallet_importmulti.py:693 in 70ec7598a3 outdated

 624 | @@ -625,6 +625,76 @@ def run_test(self):
 625 |                       ismine=False,
 626 |                       iswatchonly=False)
 627 |  
 628 | +        # Import some public keys to the keypool of a no privkey wallet

instagibbs commented at 4:01 AM on February 11, 2019:

A test showing that importing some non-single-pubkey addresses using descriptors doesn't cause the keypool to grow seems apt.

achow101 commented at 4:45 AM on February 11, 2019:

Done

achow101 force-pushed on Feb 11, 2019

Sjors commented at 10:14 AM on February 11, 2019: member

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 commented at 3:28 PM on February 11, 2019: member

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 commented at 3:35 PM on February 11, 2019: member

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

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

instagibbs commented at 6:01 PM on February 14, 2019: member

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

instagibbs commented at 6:14 PM on February 14, 2019: member

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 commented at 7:19 PM on February 14, 2019: member

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

Test and I'm happy.

achow101 force-pushed on Feb 14, 2019

in test/functional/wallet_importmulti.py:730 in 0f9f035449 outdated

 725 | +            'bcrt1qau64272ymawq26t90md6an0ps99qkrse58m640', # m/0'/0'/3
 726 | +            'bcrt1qsg97266hrh6cpmutqen8s4s962aryy77jp0fg0', # m/0'/0'/4
 727 | +        ]
 728 | +        result = wrpc.importmulti(
 729 | +            [{
 730 | +                'desc': 'wpkh(' + xpub + '/0h/0h/*' + ')',

instagibbs commented at 7:26 PM on February 14, 2019:

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

achow101 commented at 7:29 PM on February 14, 2019:

Whoops

Sjors commented at 7:28 PM on February 14, 2019: member

@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 force-pushed on Feb 14, 2019

instagibbs commented at 7:33 PM on February 14, 2019: member

Test fix only change.

reACK https://github.com/bitcoin/bitcoin/pull/14075/commits/32fd3bb627fbd3c44b4f45434ead56f207dffa06

achow101 force-pushed on Feb 14, 2019

instagibbs commented at 7:43 PM on February 14, 2019: member

re-ACK https://github.com/bitcoin/bitcoin/pull/14075/commits/38fbc101d1342222565b8670b7cd9c2485a57c74

Ordering test fixed, at least locally.

Sjors commented at 7:56 PM on February 14, 2019: member

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

meshcollider commented at 10:32 PM on February 14, 2019: contributor

utACK https://github.com/bitcoin/bitcoin/pull/14075/commits/38fbc101d1342222565b8670b7cd9c2485a57c74

Add a method to add a pubkey to the keypool

Introduces AddKeypoolPubkey in order to add a pubkey to the keypool

99cccb900b

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.

9b81fd19ac

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.

513719c5f8

achow101 force-pushed on Feb 14, 2019

Test pubkey import to keypool 9e1551b9ce

Import public keys in order

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

f4b00b70e8

achow101 force-pushed on Feb 14, 2019

meshcollider commented at 11:51 PM on February 14, 2019: contributor

LGTM https://github.com/bitcoin/bitcoin/pull/14075/commits/f4b00b70e811d3772589ab3c64d7658f4dbdab69

meshcollider merged this on Feb 14, 2019

meshcollider closed this on Feb 14, 2019

Sjors commented at 7:41 AM on February 15, 2019: member

Post merge utACK f4b00b7

laanwj referenced this in commit 2d0337992d on Feb 17, 2019

deadalnix referenced this in commit e54d9beb63 on Jun 4, 2020

Munkybooty referenced this in commit 2fa3899964 on Aug 21, 2021

Munkybooty referenced this in commit a1cef14cce on Aug 21, 2021

Munkybooty referenced this in commit 8c6c481d4a on Aug 23, 2021

Munkybooty referenced this in commit 0a3ce36ba6 on Aug 24, 2021

Munkybooty referenced this in commit 9c696c5a14 on Aug 24, 2021

Munkybooty referenced this in commit 81bfd0c463 on Aug 24, 2021

UdjinM6 referenced this in commit 8d38af1289 on Aug 24, 2021

Munkybooty referenced this in commit baa76aa331 on Aug 24, 2021

vijaydasmp referenced this in commit 288b5a711f on Sep 5, 2021

6293 referenced this in commit 10f52b3d06 on Nov 27, 2021

PastaPastaPasta referenced this in commit fd9b92e95e on Dec 22, 2021

PastaPastaPasta referenced this in commit 7d72c07722 on Dec 22, 2021

Munkybooty referenced this in commit 92b09e6dbb on Dec 23, 2021

Munkybooty referenced this in commit bbd6e2bb12 on Dec 23, 2021

Munkybooty referenced this in commit 26b127da84 on Dec 23, 2021

Munkybooty referenced this in commit 335b147b00 on Dec 23, 2021

Munkybooty referenced this in commit ea0eee60c4 on Dec 23, 2021

Munkybooty referenced this in commit 73bdcba760 on Dec 23, 2021

Munkybooty referenced this in commit be6a10ad5b on Dec 24, 2021

Munkybooty referenced this in commit 10977471e7 on Dec 24, 2021

Munkybooty referenced this in commit 9465bea31d on Dec 27, 2021

Munkybooty referenced this in commit 89c2005454 on Dec 27, 2021

Munkybooty referenced this in commit 49e16b0a09 on Jan 3, 2022

Munkybooty referenced this in commit cb743d990e on Jan 3, 2022

Munkybooty referenced this in commit 9ae9a791b8 on Jan 21, 2022

Munkybooty referenced this in commit 342aa6f33a on Jan 24, 2022

DrahtBot locked this on Feb 15, 2022