[WIP] Importmulti private key support #12705

kallewoof · 2018-03-16T07:40:00Z

Partially addresses #12703.
Fully addresses #9694.

This implementation simply uses the existing importprivkey code (moved into a separate ImportPrivateKey method), and does not support redeem scripts, internal, pubkeys, or keys options.

One question is if this should be simplified to where a user can put private keys in keys and leave scriptPubKey empty, or if it makes more sense to add a new field as I do here.

Another question is whether this should be incorporated more into how importmulti works, rather than basing it off of importprivkey (although with the timestamp argument, it seems like it does all we want it to).

(I was initially planning on deriving the arguments for ProcessImport and letting it do its thing, but realized there can be multiple scriptPubKeys for one private key, so I'd have to call ProcessImport multiple times, with the same private key.. which will trigger an error.)

promag

Concept ACK. Light review.

promag · 2018-03-16T12:30:34Z

src/wallet/rpcdump.cpp

In commit "Importmulti private key support",

Also fix default=true to be consistent with most of the code.

promag · 2018-03-16T12:32:34Z

src/wallet/rpcdump.cpp

In commit "[wallet] [rpc] Move private key import functionality out of importprivkey",

Remove this scope?

promag · 2018-03-16T12:33:02Z

src/wallet/rpcdump.cpp

In commit "[wallet] [rpc] Move private key import functionality out of importprivkey",

Nit, move throw to new line.

Consistency?

Ahh, because of the other one below. Fixed.

promag · 2018-03-16T12:34:25Z

src/wallet/rpcdump.cpp

In commit "[wallet] [rpc] Move private key import functionality out of importprivkey",

static bool ...?

promag · 2018-03-16T12:37:04Z

src/wallet/rpcdump.cpp

In commit "[wallet] [rpc] Add privkey support to importmulti",

Nit, UniValue result(UniValue::VOBJ);

meshcollider · 2018-03-17T22:53:15Z

Concept ACK

jonasschnelli · 2018-03-18T05:06:46Z

Concept ACK

sipa · 2018-03-18T05:15:48Z

Thank you for working on this!

I think we should be careful here though. importprivkey has a number a quirks that are for historical or compatibility reasons, that we shouldn't maintain:

importprivkey does not know what the corresponding address is, and thus can't know what type to use (p2pkh, p2sh-p2wpkh, p2wpkh), and thus has to assume all are possible (which results in at least labelling all 3). For importmulti we generally do have the address available. The goal here could be to provide a default address type (or explicitly name the type); not keep assuming it can be any.
Relatedly, we don't have an encoding for "private key whose address is supposed to be P2SH-P2WPKH". My suggestion would be to add one (I believe Electrum has some sort of standard for this). importprivkey can't really use these, because it already has to assume it can be any type, but importmulti does not need to repeat this. It could assume just P2PKH for a legacy WIP encoding, and P2SH/P2WPKH when one of those novel encodings is used.
importprivkey doesn't support key birth times; but this is not inherent, keys can have birth times.

sipa · 2018-03-18T07:18:59Z

One question is if this should be simplified to where a user can put private keys in keys and leave scriptPubKey empty, or if it makes more sense to add a new field as I do here.

I think it should absolutely use keys - there is no need to add new fields to just provide the same functionality in a different way.

The way it should work IMO is that it integrates into all the functionality that exists (including labels, birth times), but simply permit the scriptpubkey/address (and for p2sh-p2wpkh, also the redeemscript) to be omitted and computed by default from the private key - provided we have figured out how to deal with the encoding of different types of address derivation.

kallewoof · 2018-03-19T03:46:05Z

@sipa It seems Electrum 3.0 included an extension to WIF for segwit stuff:

https://github.com/spesmilo/electrum/blob/5fef1e7980e6c9811448ad7d9fb6afa4460ac7fc/RELEASE-NOTES#L181-L194

A good starting point might be to rework this PR to adopt that and see what people think?

kallewoof · 2018-03-19T04:39:19Z

@promag Thanks for the feedback, see e896ba0, 6408244.

kallewoof · 2018-03-19T04:40:34Z

@sipa Small note:

importprivkey doesn't support key birth times; but this is not inherent, keys can have birth times.

The method implementation does support birth times, in the timestamp argument which is always 1 for importprivkey, but uses the provided timestamp for importmulti.

cdecker · 2018-03-26T13:45:29Z

Concept ACK

Other than the quirks that importprivkey has to support mentioned by @sipa, which would be nice to avoid for a new call.

The new ImportPrivateKey method is to be used by importmulti when user provides a private key.

DrahtBot · 2018-07-25T15:00:43Z

Needs rebase

kallewoof · 2018-08-10T03:15:00Z

Closing as too many aspects of this are up in the air.

fanquake added Wallet RPC/REST/ZMQ labels Mar 16, 2018

kallewoof force-pushed the importmulti-wif-support branch 2 times, most recently from 6c839f8 to 32e70ab Compare March 16, 2018 07:52

promag reviewed Mar 16, 2018

View reviewed changes

kallewoof changed the title ~~Importmulti private key support~~ [WIP] Importmulti private key support Mar 19, 2018

kallewoof mentioned this pull request Apr 3, 2018

BIP-178: Extend WIF format with key type #12869

Closed

kallewoof added 5 commits June 15, 2018 13:35

wallet: [rpc] [doc] Fix rescan description

e98cf03

wallet: [rpc] Move private key import functionality out of importprivkey

b01a8d4

The new ImportPrivateKey method is to be used by importmulti when user provides a private key.

wallet: [rpc] Add privkey support to importmulti

7977f1f

test: Add tests for privkey-style importmulti

da0b1f8

wallet: [rpc] [doc] importmulti: standardize defaults in help text

1582698

kallewoof force-pushed the importmulti-wif-support branch from da1e008 to 1582698 Compare June 15, 2018 04:39

kallewoof added 2 commits June 15, 2018 14:06

f'rpcdump.cpp: assert locks held

a5ff763

f'rpcdump.cpp: lock explicitly

ca8c953

DrahtBot mentioned this pull request Jul 13, 2018

[moveonly] Extract RescanWallet to handle a simple rescan #13658

Merged

DrahtBot added the Needs rebase label Jul 25, 2018

kallewoof closed this Aug 10, 2018

kallewoof deleted the importmulti-wif-support branch October 17, 2019 08:55

laanwj removed the Needs rebase label Oct 24, 2019

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

[WIP] Importmulti private key support #12705

[WIP] Importmulti private key support #12705

Uh oh!

Conversation

kallewoof commented Mar 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag left a comment

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!

meshcollider commented Mar 17, 2018

Uh oh!

jonasschnelli commented Mar 18, 2018

Uh oh!

sipa commented Mar 18, 2018

Uh oh!

sipa commented Mar 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kallewoof commented Mar 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kallewoof commented Mar 19, 2018

Uh oh!

kallewoof commented Mar 19, 2018

Uh oh!

cdecker commented Mar 26, 2018

Uh oh!

DrahtBot commented Jul 25, 2018

Uh oh!

kallewoof commented Aug 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

kallewoof commented Mar 16, 2018 •

edited

Loading

sipa commented Mar 18, 2018 •

edited

Loading

kallewoof commented Mar 19, 2018 •

edited

Loading