[wallet] allow adding pubkeys from imported private keys to keypool #15414
Conversation
|
This might interact with #15024. |
|
Concept ACK |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
Sjors
force-pushed
the
2019/02/keypool_import_private_keys
branch
from
cacf2f3 to
9f63410
Compare
Sjors
force-pushed
the
2019/02/keypool_import_private_keys
branch
from
9f63410 to
7f23872
Compare
|
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.
Why can't you create the wallet with the private keys and then import the pubkeys into another wallet? |
|
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. |
|
@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. |
Sjors
force-pushed
the
2019/02/keypool_import_private_keys
branch
from
7f23872 to
dc44b45
Compare
Sjors
force-pushed
the
2019/02/keypool_import_private_keys
branch
from
dc44b45 to
524f76a
Compare
|
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. |
| Needs rebase |
7 participants
#14075 added a
keypoolparam toimportmultiwhich imports public keys into the keypool, but this was restricted to watch-only wallets (wallets created withdisable_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
getnewaddresscommand 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.