[WIP] Importmulti private key support #12705

1. 
   kallewoof commented at 7:40 am on March 16, 2018: member

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

2. fanquake added the label Wallet on Mar 16, 2018
3. fanquake added the label RPC/REST/ZMQ on Mar 16, 2018
4. kallewoof force-pushed on Mar 16, 2018
5. kallewoof force-pushed on Mar 16, 2018
6. in src/wallet/rpcdump.cpp:1135 in 1f9e8da2a9 outdated

   1131@@ -1132,7 +1132,7 @@ UniValue importmulti(const JSONRPCRequest& mainRequest)
   1132             "  ]\n"
   1133             "2. options                 (json, optional)\n"
   1134             "  {\n"
   1135-            "     \"rescan\": <false>,         (boolean, optional, default: true) Stating if should rescan the blockchain after all imports\n"
   1136+            "     \"rescan\": <bool>,          (boolean, optional, default: true) Whether a rescan of the blockchain after all imports should be executed\n"
   

   ---

   ---

   

   promag commented at 12:30 pm on March 16, 2018:

   In commit “Importmulti private key support”,

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

7. in src/wallet/rpcdump.cpp:99 in 1bfac83da7 outdated

   94+    if (!key.IsValid()) throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid private key encoding");
   95+
   96+    CPubKey pubkey = key.GetPubKey();
   97+    assert(key.VerifyPubKey(pubkey));
   98+    CKeyID address = pubkey.GetID();
   99+    {
   

   ---

   ---

   

   promag commented at 12:32 pm on March 16, 2018:

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

   Remove this scope?

8. in src/wallet/rpcdump.cpp:94 in 1bfac83da7 outdated

   87@@ -88,6 +88,38 @@ bool GetWalletAddressesForKey(CWallet * const pwallet, const CKeyID &keyid, std:
   88     return fLabelFound;
   89 }
   90 
   91+bool ImportPrivateKey(CWallet * const pwallet, const UniValue& privkey, const std::string& label = "", const int64_t timestamp = 1)
   92+{
   93+    CKey key = DecodeSecret(privkey.get_str());
   94+    if (!key.IsValid()) throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid private key encoding");
   

   ---

   ---

   

   promag commented at 12:33 pm on March 16, 2018:

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

   Nit, move throw to new line.

   ---

   

   kallewoof commented at 4:33 am on March 19, 2018:

   Why?

   ---

   

   cdecker commented at 1:39 pm on March 26, 2018:

   Consistency?

   ---

   

   kallewoof commented at 2:34 am on April 16, 2018:

   Ahh, because of the other one below. Fixed.
9. in src/wallet/rpcdump.cpp:91 in 1bfac83da7 outdated

   87@@ -88,6 +88,38 @@ bool GetWalletAddressesForKey(CWallet * const pwallet, const CKeyID &keyid, std:
   88     return fLabelFound;
   89 }
   90 
   91+bool ImportPrivateKey(CWallet * const pwallet, const UniValue& privkey, const std::string& label = "", const int64_t timestamp = 1)
   

   ---

   ---

   

   promag commented at 12:34 pm on March 16, 2018:

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

   static bool ...?

10. in src/wallet/rpcdump.cpp:826 in 472afcce62 outdated

    821+            if (pubKeys.size() > 0 || keys.size() > 0) throw JSONRPCError(RPC_INVALID_PARAMETER, "Keys/pubkeys not supported for privkey import");
    822+            if (watchOnly)                             throw JSONRPCError(RPC_INVALID_PARAMETER, "Privkeys cannot be imported watch-only");
    823+            if (!ImportPrivateKey(pwallet, privkey, label, timestamp)) {
    824+                throw JSONRPCError(RPC_WALLET_ERROR, "The wallet already contains the given private key");
    825+            }
    826+            UniValue result = UniValue(UniValue::VOBJ);
    

    ---

    ---

    

    promag commented at 12:37 pm on March 16, 2018:

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

    Nit, UniValue result(UniValue::VOBJ);

11. 
    promag commented at 12:37 pm on March 16, 2018: member

    Concept ACK. Light review.
12. 
    meshcollider commented at 10:53 pm on March 17, 2018: contributor

    Concept ACK
13. 
    jonasschnelli commented at 5:06 am on March 18, 2018: contributor

    Concept ACK
14. 
    sipa commented at 5:15 am on March 18, 2018: member

    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.
15. 
    sipa commented at 7:18 am on March 18, 2018: member

    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.

16. 
    kallewoof commented at 3:46 am on March 19, 2018: member

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

17. 
    kallewoof commented at 4:39 am on March 19, 2018: member

    @promag Thanks for the feedback, see e896ba0, 6408244.
18. 
    kallewoof commented at 4:40 am on March 19, 2018: member

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

19. kallewoof renamed this:
    Importmulti private key support
    [WIP] Importmulti private key support
    on Mar 19, 2018
20. 
    cdecker commented at 1:45 pm on March 26, 2018: contributor

    Concept ACK

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

21. wallet: [rpc] [doc] Fix rescan description e98cf031e1
22. wallet: [rpc] Move private key import functionality out of importprivkey

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

    b01a8d41a7
23. wallet: [rpc] Add privkey support to importmulti 7977f1f7e2
24. test: Add tests for privkey-style importmulti da0b1f85b2
25. wallet: [rpc] [doc] importmulti: standardize defaults in help text 158269878a
26. kallewoof force-pushed on Jun 15, 2018
27. f'rpcdump.cpp: assert locks held a5ff7631a9
28. f'rpcdump.cpp: lock explicitly ca8c953293
29. 
    DrahtBot commented at 3:00 pm on July 25, 2018: member
30. DrahtBot added the label Needs rebase on Jul 25, 2018
31. 
    kallewoof commented at 3:15 am on August 10, 2018: member

    Closing as too many aspects of this are up in the air.
32. kallewoof closed this on Aug 10, 2018

    ---

33. kallewoof deleted the branch on Oct 17, 2019
34. laanwj removed the label Needs rebase on Oct 24, 2019
35. DrahtBot locked this on Dec 16, 2021

Contributors
kallewoof promag cdecker meshcollider jonasschnelli sipa DrahtBot

Labels
Wallet RPC/REST/ZMQ