Add importmulti RPC call

[pedrobranco]

As discussed in PR #6570 this is a new rpc call importmulti that receives an array of JSON objects representing the intention of importing a public key, a private key, an address and script/p2sh:

bitcoin-cli importmulti '[
  {
    "timestamp": 1455191478,
    "type": "privkey",
    "value": "<private key>"
  },
  {
    "label": "example 1",
    "timestamp": 1455191480,
    "type": "pubkey",
    "value": "<public key>"
  }
]' '{"rescan":true}'

and rescans (if not disabled in second argument) the chain from the block that has a timestamp lowest than the lowest timestamp found in all requests, preventing scanning from genesis.

The output is an array with the status of each request:

output: [ { "result": true } , { "result": true } ]

Arguments:

1. json request array     (json, required) Data to be imported
  [
    {
      "type": "privkey | pubkey | address | script", (string, required) Type of address
      "value": "...",                       (string, required) Value of the address
      "timestamp": 1454686740,              (integer, optional) Timestamp
      "label": "..."                        (string, optional) Label
      "p2sh": true | false                  (bool, optional, default=false) Value is a P2SH (type=script only)
    }
  ,...
  ]
2. json options                 (json, optional) Options

Some notes:

• If one of the import requests has no timestamp then it should rescan from Genesis (if rescan is not disabled).
• If all requests fail then it won't rescan the chain.

Edit: As suggested by @promag i've replaced the second argument (optional) bool to JSON to be easier specify new options and added a new type="script" to support only script values, so can type="address" work only with addresses.

[laanwj]

Concept ACK

[promag]

@laanwj should we catch specific exceptions to return the error?

[pedrobranco]

IMO should catch the JSONRPCError and be included on the response, something like:

output: [
  {
    "result": true
  },
  {
    "result": false,
    "error": {
      "code": -5,
      "message": "Invalid Bitcoin address"
    }
  }
]

and catch other exceptions as "missing required fields" (runtime_error). Also maybe changing from "result" to "success".

[promag]

Only this line is necessary because RPCTypeCheck doesn't enforce the length. See implementation.

[pedrobranco]

Will fix.

[promag]

What about the second argument be a JSON object? Here it would be { "rescan": true }. But it could have more options, for instance, one that would rejects addresses with timestamps belonging to pruned blocks.

[promag]

I would move up all auxiliary functions (the ones in title case), then the RPC handlers. WDYT?

[instagibbs]

concept ACK

I have implemented a method to also simply just import an spv proof + transaction rather than rescan. Would be nice to include that in this as well. (need to add tests and PR...)

[jonasschnelli]

Concept ACK.
IIRC, this would be the first RPC command that accept a associative array parameter list (json options (json, optional) Options). IMO this is good but I would prefer a general way of providing key/value parameters for RPC commands (probably out-of-scope for this PR).

[instagibbs]

This is never used?

[pedrobranco]

True, i will remove.

[laanwj]

IIRC, this would be the first RPC command that accept a associative array parameter list

I agree. Though I've done a similar thing in #7552. Use positional arguments only for the 'essential' arguments, and an associative array for everything optional or future-extensible.

Much less hassle and confusing than APIs with tons of positional arguments, especially optional ones.

[promag]

@jonasschnelli there is also #7518 that accepts options as a JSON object.

[luke-jr]

GBT also uses an options Object

[jtimon]

concept ACK

[pedrobranco]

Replaced the output result from [ { "result": true } , ... ] to [ { "success": true } , ... ].

In case of giving a exception should we show any information of the reason in the result? Something like:
[ { "success": false , error : { "code": -5, "message": "Invalid private key encoding" } } , ... ]

[instagibbs]

Passing along error messages would be hugely helpful, yes.

[pedrobranco]

@instagibbs 👍

Another thing, currently we match the input to the output status by the order given of the input array.
Shouldn't we put some information of the input (unique enough) in the output to be easier take some action in case of success/failure?

Example:

input = [ { type: "address", value: "myaddress1"}, { type: "address", value: "myaddress2"}, ... ]
output = [ { success: true }, { success: true }, ... ]

to something like:

input = [ { type: "address", value: "myaddress1"}, { type: "address", value: "myaddress2"}, ... ]
output = [ { success: true , reference : { value: "myaddress1"} }, { success: true , reference : { "value":"myaddress2"} } }, ... ]

Or maybe include in the reference the complete input node . In this case when the user calls the importmulti it can pass any type of extra information that later can be useful to take some action.

[instagibbs]

I'll have to defer to others on the value of that. I don't see much value-add personally but unsure.

[mrbandrews]

Nit: only need to initialize 2 nodes

[mrbandrews]

I don't believe it's necessary to connect the nodes for this test

[mrbandrews]

See above, no need to connect/sync

[mrbandrews]

Same comment

[mrbandrews]

Lightly tested ACK.
I made a few nit comments on the python test.
If you wanted you could add a test that trying to import a private key and scriptpubKey for the same address fails.

[pedrobranco]

@mrbandrews Test added.

[MarcoFalke]

Nit: Would be nice to be more verbose here. Maybe move the comment from above into an AssertionError?

Edit: Also, I don't like the pass. Effectively the current try-except is a noop. Am I missing something?

[MarcoFalke]

This is indeed a noop, please remove the code or replace it with something else. Maybe?

diff --git a/qa/rpc-tests/importmulti.py b/qa/rpc-tests/importmulti.py
index 845bcfe..243e70e 100755
--- a/qa/rpc-tests/importmulti.py
+++ b/qa/rpc-tests/importmulti.py
@@ -108,7 +108,3 @@ class ImportMultiTest (BitcoinTestFramework):
         # empty json case
-        try:
-            self.nodes[1].importmulti()
-            raise
-        except:
-            pass
+        assert_raises(JSONRPCException, self.nodes[1].importmulti)

[pedrobranco]

Yes.

[MarcoFalke]

Nit: Some more dead code

[MarcoFalke]

Concept ACK

[laanwj]

Needs rebase after #7558

[pedrobranco]

Nit: Would be nice to be more verbose here.

@MarcoFalke What do you suggest?

@laanwj Rebased.

[pedrobranco]

@laanwj Are you considering merging this soon?

[sipa]

Sorry for the very late response here, but I think it's undesirable to have the "p2sh" flag in this call. The fact that it's present in importscript is historical, but quite confusing I think (see #7687).

Here is what happens:

importaddress "script" p2sh=false

will mark scriptPubKeys that exactly match script as ismine, but without the ability to spend them, and there is no address added to the wallet (so it will likely be treated as change by the wallet, though show up in listunspent).

importaddress "script" p2sh=true

Will do the same as the call above, AND also add script as a known script to the keystore, meaning that it will start treating scriptPubKeys that send to P2SH(script) as ismine too, AND in addition associate the passed label with that P2SH address (but not with the raw script).

importaddress "address"

Will mark scriptPubKeys that match the exact script corresponding to address as ismine, AND associate the passed label with it.

There is a much cleaner distinction here, where importing of scripts and importing of addresses is completely separate. That would mean that where you'd use import script p2sh=true, instead, you'd import both a script and an address.

[jonasschnelli]

nit formating:

    {
      "type": "privkey | pubkey | address | script", (string, required) Type of address
      "value": "...",                                (string, required) Value of the address
      "timestamp": 1454686740,                         (integer, optional) Timestamp
      "label": "..."                                 (string, optional) Label
      "p2sh": true | false                             (bool, optional, default=false) Value is a P2SH (type=script only)
    }

[sipa]

To add some more background: ultimately, there are 3 internal calls that can be issued:

• AddCScript(redeemscript) allows the wallet to construct scriptSigs for scriptPubKeys that send to P2SH(redeemscript). This only affects ismine when redeemscript itself can be signed for.
• AddWatchOnly(scriptPubKey) marks a particular scriptPubKey as unconditionally ismine (whether we know how to construct a scriptSig for such an output or not).
• SetAddressBook(destination) marks outputs set to a scriptPubKey that matches destination as 'incoming payment' with a label (rather than our own change). Unfortunately, that does not work for arbitrary scriptPubKeys, only P2SH or P2PKH ones.

So right now, importaddress "script" p2sh=true calls AddWatchOnly(script), AddCScript(script), SetAddressBook(P2SH(script)). This is a bit inconsistent, as it has effects on outputs. paying to script directly and through P2SH

[btcdrak]

concept ACK

[pedrobranco]

@sipa Thanks for the explanation. I've added p2sh support only because importaddress rpc call already had it. Do you agree that we should remove p2sh flag from importmulti call?

[laanwj]

Maybe I'm misunderstanding this, but I think the type should fully specify what kind of object is imported:

      "type": "privkey | pubkey | address | script", (string, required) Type of address

This makes an extra P2SH flag redundant.

[sipa]

script is ambiguous: it can mean "i'm importing this script, so that when it is used as a P2SH redeemscript, the client knows how to sign it", or "i want outputs spending to this exact script to be counting towards my balance and listunspent".

[laanwj]

Then add a different type for either case?

[laanwj]

eh, the "i'm importing this script, so that when it is used as a P2SH redeemscript, the client knows how to sign it" case probably doesn't belong here at all, as it relates to signing and doesn't affect the balance/rescan?

[sipa]

@laanwj It does, because if that script itself uses keys that are known to the wallet, it will make such outputs be considered spendable.

[sipa]

I'm sorry for just mentioning some background and complaints, and not offering a good solution. But I think we have a mess that is caused by having half a dozen different ways through which a script can be considered spendable/solvable/ismine, and having magic import commands with many edge cases (like importaddress now) that just try to do as much as possible, and we should not maintain that in newer APIs.

One solution is to break it down to the lowest-level CKeyStore changes, and providing a means to individually add:

• privkey (which helps spendability, causing outputs always to be treated as ismine)
• pubkeys/redeemscripts (which helps solvability, and if that indirectly leads to a spendable script, it causes outputs to be treated as ismine)
• watched scripts (which always causes outputs to be treated as ismine, regardless of spendability)
• labels (which makes the wallet treat such transactions as incoming funds, which is only possible for scripts that map to a CTxDestination)

However, it would be highly inconvenient to use (you'd sometimes need 3 separate entries to get the behaviour of a single call now).

Here is a proposal, but it's quite a bit different from the existing design: the input is a list of Objects, each of which has the following keys:

• output mandatory string an address or a hex-encoded script, describing exactly what outputs we're talking about, this is mandatory
• redeemscript optional string and only allowed if the address is a P2SH address or a P2SH scriptPubKey, and it must correspond to that output.
• pubkeys optional array of strings giving pubkeys that must occur in the output or redeemscript
• keys: optional array of strings, giving private keys whose corresponding public keys must occur in the output or redeemscript
• internal optional bool (default false) stating whether matching outputs should be be treated as not incoming payments. Must be explicitly set to false for outputs that don't have a corresponding CTxDestination (so a warning/error can be given)
• watchonly optional bool (default false) stating whether matching outputs should be considered watched even when they're not spendable. If this is not given, importing this entry must result in the specified outputs becoming spendable.
• label optional string (default "" if internal=false, none if internal=true) to assign to the address (aka account name, for now), only allowed with internal=false
• timestamp as now (though perhaps we should consider that the timestamp for an individual key should be stored in mapKeyMetaData, and not just in nTimeFirstKey.

This is a bit redundant and less convenient (it always requires specifying a script or address, and either keys), but I think it allows for very accurate error messages whenever the behaviour wouldn't match the user's expectations. It also makes it obvious where the current codebase's restrictions are (you can't make a non-standard scriptPubKey non-internal, only individual keys can be assigned a birth time, ...), which can be loosened in future iterations if those restrictions are solved.

Open for discussion of course!

[promag]

@sipa thanks for these insights!

Totally agree with @sipa regarding the object keys. Specially because there is no type key 👏 With these keys we can be sure the caller knows his business.

[promag]

@sipa replace output by scriptPubKey ?

[sipa]

@promag I considered that, but if it's an address, it's not really a scriptPubKey (especially if there is ever support for things like stealth addresses (which contain cryptographic information that never actually directly ends up in the scriptPubKey).

Perhaps there should be two separate fields, address or scriptPubKey, and you're required to exactly provide one of them.

[pedrobranco]

@sipa ACK and thanks for the proposal.

Also, when calling with the private keys and watchonly=true. Should we throw an error / warn the user that he should use pubkeys instead if he wants watch-only? Or should we allow importing private keys in watch-only (deriving the pubkey)?

[MarcoFalke]

Mind to change this to py3?

[pedrobranco]

Sure.

[promag]

@promag I considered that, but if it's an address, it's not really a scriptPubKey (especially if there is ever support for things like stealth addresses (which contain cryptographic information that never actually directly ends up in the scriptPubKey).

@sipa maybe we could go with:

• { "scriptPubKey": <script> }
• { "scriptPubKey": { "address": <address> } }

It's scalable and unambiguous. WDYT?

[promag]

@arowser this is still work in progress.

[sipa]

@promag Looks good to me.

[laanwj]

As this is very often requested functionality I'd really like to get this in for 0.13.

Note that the deadline for features for 0.13 is June 16, so that would mean we'll have to get it to a mergeable state in a week. Is that realistic?

[pedrobranco]

@laanwj Yes, i think is realistic.

[laanwj]

Ping...

[pedrobranco]

@laanwj I'm working on this with @promag and we'll try to push a solution as fastest as we can (probably by tomorrow).

[pedrobranco]

@laanwj We have a partial solution but is not complete:

• Does not checks the consistency between private/pub keys and scriptPubKey / redeemScript.
• Does not support internal and watchonly modes.

We've made a major refactor and unfortunately we think we are late for the 0.13 milestone.

[MarcoFalke]

Could you use __init__ for that, instead of overwriting setup_chain?

I.e. something like

    def __init__(self):
        super().__init__()
        self.num_nodes = 2
        self.setup_clean_chain = True

[pedrobranco]

Sure, and looks way better.

[MarcoFalke]

micro-Nit: missing trailing ,

[MarcoFalke]

Nit: Might as well place this in the header

[laanwj]

Looks good to me - utACK d72a886

Does not support internal and watchonly modes.

Seems acceptable for a first version, it's not documented either so that's consistent.

We've made a major refactor and unfortunately we think we are late for the 0.13 milestone.

I'm afraid so too, if we would merge it now it would be very last-minute, I don't think this code is well-tested enough. Code does get more testing on master, but so close to a release it's more risky, we don't want to release with something broken.

[pedrobranco]

Code does get more testing on master, but so close to a release it's more risky, we don't want to release with something broken.

I totally agree.

Still we need help on how to implement internal mode and watch-only, and the consistency checking (technical doubts, so we'll need to do some researching).

[sipa]

I promise to help with this (nag me if I forget), but after 0.13 is branched off.

[pedrobranco]

I promise to help with this (nag me if I forget), but after 0.13 is
branched off.

Thanks for the support.

I have some doubts about the expected result in some situations:

• When we have private keys on keys and watch-only=true what should we do:
  • import the public key of each given key?
  • import the private key and mark it somehow to unspentable / watchable only?
• The watch-only concept should not import and save any private key, @sipa agree?

About the internal concept I'm a bit confused on how it should work.

[laanwj]

@pedrobranco Removing the 0.13 milestone then - thanks for your patience!

[sipa]

I think if private keys are given and watchonly, you should fail. The reason for the watchonly option is to indicate that you intentionally are not providing a private key, and are not expecting the ability to spend. It's there so that if someone just forgets to provide a private key (as opposed to making it intentionally watch-only), the call can warn them by failing.

The internal concept is similar to watchonly. When it is present, you're not adding the address to the addressbook (so no label, not even an empty label). This makes it incompatible with the label option. But also for example, when the scriptPubKey is given in hex form and not in a standard type that can be internally converted to a CTxDestination, internal must be set, as there would be no way to add it to the address book (at this point).

[pedrobranco]

@sipa ACK.

[pedrobranco]

Consistency check added + internal + watchonly.

[pedrobranco]

Can someone please review the PR?

[jonasschnelli]

Will test today.

[jonasschnelli]

Using c++11 lambda here?

Something like:

std::lower_bound(vChain.begin(), vChain.end(), nTime,
   [](CBlockIndex* pBlock, const int64_t& time) -> bool { return pBlock->GetBlockTime() < time; }
);

Would probably be less confusing to read.

[jonasschnelli]

Are you sure this is correct?
I can't see a link between the input timestamp and nLowestTimestamp.
In case of a rescan, nLowestTimestamp is always chainActive.Tip()->GetBlockTime() which is incorrect IMO.

Also, we should add a time- or block-delta between nLowestTimestamp and the actual rescan height. Timestamps could be slightly wrong (timezones, failed implementations, accuracy). Maybe we should go down another 144 blocks.

[pedrobranco]

It was left out in this latest proposal, so I will fix that.

Maybe we should go down another 144 blocks.

I'm not quite sure about rescanning in a different timestamp that the user specifies, given that he knows what's he doing. Is there any reason for the 144 blocks?

[jonasschnelli]

144 blocks = 1day. I think going back/deeper one day is reasonable.

[jonasschnelli]

To bad we need to hold cs_main for this. But I think its not possible without it with the current lock structure.

[jonasschnelli]

nit: ; can be removed;
mini-nit: You are using "ifs without brackets" together with some "ifs with brackets".

[jonasschnelli]

fRunScan is always false which result in never triggering a rescan.

[pedrobranco]

I will check if any of the requests returns success=true, then allow the rescan (fRunScan = true) if not disabled via options.

[jonasschnelli]

I think that is not correct.
There are two appearance of the variable fRunScan, one on L977 (bool fRunScan = false;) and one on L992 (if (fRescan && fRunScan && request.size()) {).

I can't see a code part where fRunScan will be set to true.

[pedrobranco]

What I have meant in the comment is that after pushing the fix it it will do what I've described:

I will check if any of the requests returns success=true, then allow the rescan (fRunScan = true) if not disabled via options.

[jonasschnelli]

The rescan behavior seems not to work, I'm happy to test again once this is fixed.
We really want to have importmulti in 0.14.

[pedrobranco]

@jonasschnelli Can you please test the rescan behavior?

[jonasschnelli]

I guess nLowestTimestamp is always > 0.

[laanwj]

What needs to be done here?

[pedrobranco]

What needs to be done here?

Maybe add some tests for the rescan feature.

[laanwj]

Personally, I don't know the opinion of others, I'd really like to move forward and merge this if the base functionality is agreed on. There is still enough time before 0.14 to fix problems, add tests, add features etc.

[jonasschnelli]

nit: using chainActive.Tip()s blocktime with the later FindLatestBefore(nLowestTimestamp) will probably always result in re-scaning a couple 1-2 blocks when importing with a timestamp in future.

[pedrobranco]

We can prevent rescanning if nLowestTimestamp is in the future:

if (fRescan && fRunScan && request.size() && nLowestTimestamp <= chainActive.Tip()->GetBlockTime()) {
(...)

[jonasschnelli]

This else should be removed because it can only set a already false fRescan again to false.

[pedrobranco]

fRescan is initially set to true.

[jonasschnelli]

Argh. Your right. Got fooled by fRunScan, fRescan.
Nevermind then.

[pedrobranco]

No problem @jonasschnelli .

[jonasschnelli]

Tested ACK 1bcc021
(trivial rebase issue [only rpc-tests.py] can be done during merging), nits can be addresses after merging.

[pedrobranco]

Rebased and nits addressed.

[laanwj]

It looks like you're doing the IsMine/HaveWatchOnly/AddWatchOnly dance twice here. Why?

[pedrobranco]

I was wondering the same when I checked the RPC call importpubkey, which calls the same dance in here.

In doubt, and since I did want to change the behavior of importing pub keys, I've added the TODO mark for revision of this part of the code.

[laanwj]

Needs (easy) rebase in rpcwallet.cpp after #8788

[pedrobranco]

Rebased.

[jonasschnelli]

Re-Tested ACK 215caba
I think this is ready for merge. Possible optimizations and nit-fixing can be done separately.

[laanwj]

I think this is ready for merge. Possible optimizations and nit-fixing can be done separately.

Yes, agreed. The code is self-contained so the risk of regressions is minimal. The importmulti RPC call should be considered experimental for now, but 0.14 is still a few months away anyhow.

[rebroad]

bisect identifies cb08fdb as the commit that causes the compile to fail with:-

make src/qt/bitcoin-qt
Makefile:1169: warning: overriding commands for target `src/qt/bitcoin-qt'
Makefile:1107: warning: ignoring old commands for target `src/qt/bitcoin-qt'
make -C src qt/bitcoin-qt
make[1]: Entering directory `/home/rebroad/src/bitcoin/src'
  CXX      wallet/libbitcoin_wallet_a-rpcdump.o
wallet/rpcdump.cpp: In function ‘UniValue processImport(const UniValue&)’:
wallet/rpcdump.cpp:869:37: error: ‘pubkey’ was not declared in this scope
                 CKeyID vchAddress = pubkey.GetID();
                                     ^
make[1]: *** [wallet/libbitcoin_wallet_a-rpcdump.o] Error 1
make[1]: Leaving directory `/home/rebroad/src/bitcoin/src'
make: *** [src/qt/bitcoin-qt] Error 2

I am confused as to how this tested ok and got merged when it doesn't compile..

[rebroad]

pubKey is not defined.... pubkey is though!

[laanwj]

This has not given any compile issues for anyone else besides a boost issue which was fixed later ( #8980).
Also when I look at the source, pubkey is simply defined as:

CPubKey pubkey = key.GetPubKey();

If you are trying to merge this into some other code base do not complain here if the build breaks.