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

1. 
   Sjors commented at 10:01 am on February 15, 2019: member

   #14075 added a keypool param to importmulti which imports public keys into the keypool, but this was restricted to watch-only wallets (wallets created with disable_private_keys).

   This PR relaxes that constraint to also allow private keys.

   This is a step towards #14449, being able to import any BIP44/49/84 compatible external wallet. Once imported it’s not safe to go back though, because the getnewaddress command is unaware of the intended address type. For that to really work we either need descriptor based wallets, or import could descriptors as metadata to individual keys in the wallet and make the wallet enforce those when generating a new (change) address.

   This also enables me to write a test in #14912 which creates an unsigned PSBT in a wallet without private keys and then signs it in a wallet with those private keys (which simulates an external hardware wallet). There are probably other ways I can build that test though.

2. 
   Sjors commented at 10:02 am on February 15, 2019: member

   This might interact with #15024.
3. fanquake added the label Wallet on Feb 15, 2019
4. 
   meshcollider commented at 10:39 am on February 15, 2019: contributor

   Concept ACK
5. 
   DrahtBot commented at 7:54 am on February 16, 2019: 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:

   • #16341 (WIP: Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes) by achow101)
   • #16301 (Use CWallet::Import* functions in all import* RPCs 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.

6. DrahtBot added the label Needs rebase on Feb 16, 2019
7. Sjors force-pushed on Feb 22, 2019
8. DrahtBot removed the label Needs rebase on Feb 22, 2019
9. Sjors force-pushed on Mar 8, 2019
10. 
    achow101 commented at 7:28 pm on March 14, 2019: member

    NACK. I don’t think that any imported private keys should be added to the keypool. A key that has been imported is one which has been exposed in plaintext and should thus be considered compromised. It would be unsafe to continue to use such a private key. Furthermore, private keys which are imported are often ones that have already been used. If a used key were imported to the keypool, it will lead to accidental address reuse which we should avoid.

    creates an unsigned PSBT in a wallet without private keys and then signs it in a wallet with those private keys (which simulates an external hardware wallet). There are probably other ways I can build that test though.

    Why can’t you create the wallet with the private keys and then import the pubkeys into another wallet?

11. 
    Sjors commented at 9:11 pm on March 14, 2019: member

    The test needs a signed PSBT, which is easiest with a keypool. It might be possible to sign without having a keypool by manually passing in the private keys to a PSBT method.
12. 
    luke-jr commented at 8:05 am on April 17, 2019: member

    @achow101 A compromised key is never safe to import, so the assumption that an imported key is compromised doesn’t really hold. The only time I think it really makes sense to import private keys are restoring a wallet backup, converting a foreign wallet, or vanitygen. The latter case definitely could benefit from adding to the keypool: someone could leave a vanitygen process constantly filling their keypool with a given prefix, for example.
13. Sjors force-pushed on Apr 24, 2019
14. DrahtBot added the label Needs rebase on May 29, 2019
15. [wallet] allow adding pubkeys from imported private keys to keypool 524f76a316
16. Sjors force-pushed on Jun 6, 2019
17. 
    Sjors commented at 9:45 am on June 6, 2019: member

    I’m tempted to close this in order to allow more focus on descriptor based wallets (e.g. #15764). First I’m going to see if I can make my test work without this.
18. DrahtBot removed the label Needs rebase on Jun 6, 2019
19. DrahtBot added the label Needs rebase on Jul 26, 2019
20. 
    DrahtBot commented at 8:49 pm on July 26, 2019: member
21. Sjors closed this on Aug 2, 2019

    ---

22. laanwj removed the label Needs rebase on Oct 24, 2019
23. DrahtBot locked this on Dec 16, 2021

Contributors
Sjors meshcollider DrahtBot achow101 luke-jr

Labels
Wallet