Add importmulti RPC call #7551

pedrobranco · 2016-02-17T17:45:12Z

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 · 2016-02-17T18:22:08Z

Concept ACK

promag · 2016-02-17T20:24:48Z

src/wallet/rpcdump.cpp

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

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

instagibbs · 2016-02-18T02:39:32Z

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 · 2016-02-18T14:33:27Z

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 · 2016-02-18T20:00:30Z

qa/rpc-tests/importmulti.py

This is never used?

True, i will remove.

laanwj · 2016-02-19T09:51:18Z

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 · 2016-02-19T10:29:50Z

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

luke-jr · 2016-02-25T11:45:51Z

GBT also uses an options Object

Github-Pull: bitcoin#7551 Rebased-From: 182bcc26d71dd8d25058d06c8c144cfebb8ec2e3

jtimon · 2016-02-25T23:10:40Z

concept ACK

pedrobranco · 2016-03-10T14:44:48Z

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 · 2016-03-10T14:51:12Z

Passing along error messages would be hugely helpful, yes.

pedrobranco · 2016-03-10T15:32:55Z

@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 · 2016-03-14T15:12:41Z

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

mrbandrews · 2016-03-17T18:55:28Z

qa/rpc-tests/importmulti.py

Nit: only need to initialize 2 nodes

pedrobranco · 2016-10-19T21:44:23Z

Rebased.

jonasschnelli · 2016-10-20T06:57:22Z

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

215caba Add consistency check to RPC call importmulti (Pedro Branco) cb08fdb Add importmulti rpc call (Pedro Branco)

laanwj · 2016-10-20T07:05:27Z

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.

Github-Pull: bitcoin#7551 Rebased-From: cb08fdb

Github-Pull: bitcoin#7551 Rebased-From: 215caba

rebroad · 2016-10-25T02:31:44Z

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 · 2016-10-25T02:53:58Z

src/wallet/rpcdump.cpp

+                    CPubKey pubkey = key.GetPubKey();
+                    assert(key.VerifyPubKey(pubkey));
+
+                    CKeyID vchAddress = pubkey.GetID();


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

laanwj · 2016-10-25T05:34:50Z

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.

215caba Add consistency check to RPC call importmulti (Pedro Branco) cb08fdb Add importmulti rpc call (Pedro Branco)

Zcash: Excludes changes to importwallet (missing bitcoin/bitcoin#11667) and ProcessImport (missing bitcoin/bitcoin#7551).

Bitcoin wallet PRs 3 Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#7687 - bitcoin/bitcoin#11116 - Excludes SegWit tests - Excludes `isInvalid` code (which is part of the SegWit support structure) - bitcoin/bitcoin#13002 - Excludes changes to `importwallet` (missing bitcoin/bitcoin#11667) - Excludes changes to `ProcessImport` (missing bitcoin/bitcoin#7551) - Third commit was rewritten to add an internal SigVersion argument (which we didn't have because it was added to support SegWit logic).

7e0c709 Clean up importmulti help text (Fuzzbawls) 1cacda6 [Tests] Fix and Re-enable wallet_importmulti.py (random-zebra) 4ff0b4a [Trivial] NULL --> nullptr in skiplist_tests (random-zebra) b53a986 Add unit test for FindEarliestAtLeast (Suhas Daftuar) d1bb973 [RPC] Add importmulti call (random-zebra) Pull request description: Based on top of: - [x] #2240 Essentially backport bitcoin#7551 and bitcoin#9490, with more recent changes to adapt to the current sources. Fix and re-enable the `wallet_importmulti.py` functional test. ACKs for top commit: Fuzzbawls: ACK 7e0c709 furszy: ACK 7e0c709 and merging.. Tree-SHA512: 47a4452bfaeae16d09268d3eaa0c1a5d94a39accf86b7345818f8436222650515a8bf66ef331ba478058aabcbcfcf978192f39b349027e189b7ac41cd3e69a6e

laanwj added the Wallet label Feb 17, 2016

laanwj added the Feature label Feb 17, 2016

promag reviewed Feb 17, 2016
View reviewed changes

pedrobranco force-pushed the feature/rpc-import-multi branch 3 times, most recently from 4f9bbbf to 3f2e87e Compare February 18, 2016 12:09

instagibbs reviewed Feb 18, 2016
View reviewed changes

pedrobranco force-pushed the feature/rpc-import-multi branch from 3f2e87e to 182bcc2 Compare February 19, 2016 09:42

promag mentioned this pull request Feb 25, 2016

Add option to specify rescan starting timestamp to RPC import calls #6570

Closed

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Feb 25, 2016

Add importmulti RPC call

f259592

Github-Pull: bitcoin#7551 Rebased-From: 182bcc26d71dd8d25058d06c8c144cfebb8ec2e3

pedrobranco force-pushed the feature/rpc-import-multi branch from 182bcc2 to 795adf4 Compare March 3, 2016 11:47

pedrobranco force-pushed the feature/rpc-import-multi branch 2 times, most recently from 4719852 to 960f086 Compare March 16, 2016 17:28

mrbandrews reviewed Mar 17, 2016
View reviewed changes

qa/rpc-tests/importmulti.py Outdated

Copy link

Contributor

mrbandrews Mar 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: only need to initialize 2 nodes

Pedro Branco added 2 commits October 19, 2016 15:17

Add importmulti rpc call

cb08fdb

Add consistency check to RPC call importmulti

215caba

pedrobranco force-pushed the feature/rpc-import-multi branch from e4d03e7 to 215caba Compare October 19, 2016 14:20

laanwj merged commit 215caba into bitcoin:master Oct 20, 2016

laanwj added a commit that referenced this pull request Oct 20, 2016

Merge #7551: Add importmulti RPC call

f2d7056

215caba Add consistency check to RPC call importmulti (Pedro Branco) cb08fdb Add importmulti rpc call (Pedro Branco)

laanwj mentioned this pull request Oct 20, 2016

TODO for release notes 0.14.0 #8455

Closed

18 tasks

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Oct 20, 2016

Add importmulti rpc call

eb039ce

Github-Pull: bitcoin#7551 Rebased-From: cb08fdb

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Oct 20, 2016

Add consistency check to RPC call importmulti

12e852a

Github-Pull: bitcoin#7551 Rebased-From: 215caba

rebroad reviewed Oct 25, 2016

View reviewed changes

unsystemizer mentioned this pull request Nov 11, 2016

importprivkey RPC call locks the whole client while rescanning #1011

Closed

sipa mentioned this pull request Feb 6, 2017

Importmulti cannot import bare private keys #9694

Closed

codablock pushed a commit to codablock/dash that referenced this pull request Jan 13, 2018

Merge bitcoin#7551: Add importmulti RPC call

efded3c

215caba Add consistency check to RPC call importmulti (Pedro Branco) cb08fdb Add importmulti rpc call (Pedro Branco)

sipa mentioned this pull request Nov 20, 2018

Overhaul importmulti logic #14565

Merged

andvgal pushed a commit to energicryptocurrency/gen2-energi that referenced this pull request Jan 6, 2019

Merge bitcoin#7551: Add importmulti RPC call

c282ca9

215caba Add consistency check to RPC call importmulti (Pedro Branco) cb08fdb Add importmulti rpc call (Pedro Branco)

CryptoCentric pushed a commit to absolute-community/absolute that referenced this pull request Feb 15, 2019

Merge bitcoin#7551: Add importmulti RPC call

f550288

215caba Add consistency check to RPC call importmulti (Pedro Branco) cb08fdb Add importmulti rpc call (Pedro Branco)

jonasschnelli mentioned this pull request Jul 16, 2019

Use CWallet::Import* functions in all import* RPCs #16301

Merged

str4d pushed a commit to str4d/zcash that referenced this pull request Dec 19, 2019

Make CScript -> CScriptID conversion explicit

df00e51

Zcash: Excludes changes to importwallet (missing bitcoin/bitcoin#11667) and ProcessImport (missing bitcoin/bitcoin#7551).

str4d mentioned this pull request Dec 19, 2019

Bitcoin wallet PRs 3 zcash/zcash#4271

Merged

random-zebra mentioned this pull request Mar 30, 2021

[RPC] Add importmulti RPC command and functional test PIVX-Project/PIVX#2277

Merged

1 task

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Add importmulti RPC call #7551

Add importmulti RPC call #7551

Uh oh!

Conversation

pedrobranco commented Feb 17, 2016

Uh oh!

laanwj commented Feb 17, 2016

Uh oh!

promag Feb 17, 2016

Choose a reason for hiding this comment

Uh oh!

pedrobranco Feb 19, 2016

Choose a reason for hiding this comment

Uh oh!

instagibbs commented Feb 18, 2016

Uh oh!

jonasschnelli commented Feb 18, 2016

Uh oh!

instagibbs Feb 18, 2016

Choose a reason for hiding this comment

Uh oh!

pedrobranco Feb 18, 2016

Choose a reason for hiding this comment

Uh oh!

laanwj commented Feb 19, 2016

Uh oh!

promag commented Feb 19, 2016

Uh oh!

luke-jr commented Feb 25, 2016

Uh oh!

jtimon commented Feb 25, 2016

Uh oh!

pedrobranco commented Mar 10, 2016

Uh oh!

instagibbs commented Mar 10, 2016

Uh oh!

pedrobranco commented Mar 10, 2016

Uh oh!

instagibbs commented Mar 14, 2016

Uh oh!

mrbandrews Mar 17, 2016

Choose a reason for hiding this comment

Uh oh!

pedrobranco commented Oct 19, 2016

Uh oh!

jonasschnelli commented Oct 20, 2016

Uh oh!

laanwj commented Oct 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rebroad commented Oct 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rebroad Oct 25, 2016

Choose a reason for hiding this comment

Uh oh!

laanwj commented Oct 25, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

laanwj commented Oct 20, 2016 •

edited

Loading

rebroad commented Oct 25, 2016 •

edited

Loading