Add importmulti RPC call #7551

pedrobranco commented at 5:45 PM on February 17, 2016: contributor

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 added the label Wallet on Feb 17, 2016

laanwj commented at 6:22 PM on February 17, 2016: member

Concept ACK

laanwj added the label Feature on Feb 17, 2016

in src/wallet/rpcdump.cpp:None in b974523012 outdated

 616 | +            } else
 617 | +                throw;
 618 | +
 619 | +            result.pushKV("result", UniValue(true));
 620 | +            fRunScan = true;
 621 | +        } catch (...) {

promag commented at 8:24 PM on February 17, 2016:

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

pedrobranco commented at 2:33 PM on February 19, 2016:

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

in src/wallet/rpcdump.cpp:None in b974523012 outdated

 571 | +        return NullUniValue;
 572 | +
 573 | +    if (params.size() == 1)
 574 | +        RPCTypeCheck(params, boost::assign::list_of(UniValue::VARR));
 575 | +    else
 576 | +        RPCTypeCheck(params, boost::assign::list_of(UniValue::VARR)(UniValue::VBOOL));

promag commented at 8:26 PM on February 17, 2016:

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

pedrobranco commented at 10:09 PM on February 17, 2016:

Will fix.

in src/wallet/rpcdump.cpp:None in b974523012 outdated

 538 | +
 539 | +UniValue importmulti(const UniValue& params, bool fHelp)
 540 | +{
 541 | +    if (fHelp || params.size() < 1 || params.size() > 2)
 542 | +        throw runtime_error(
 543 | +            "importmulti '[{\"type\":\"privkey\",\"value\":\"mkjjX...\"},...]' (rescan) \n\n"

promag commented at 10:42 PM on February 17, 2016:

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.

in src/wallet/rpcdump.cpp:None in b974523012 outdated

  72 | @@ -72,6 +73,40 @@ std::string DecodeDumpString(const std::string &str) {
  73 |      return ret.str();
  74 |  }
  75 |  
  76 | +bool ImportPrivateKey(const string& strPrivkey, const string& strLabel)

promag commented at 10:43 PM on February 17, 2016:

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

instagibbs commented at 2:39 AM on February 18, 2016: member

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

pedrobranco force-pushed on Feb 18, 2016

jonasschnelli commented at 2:33 PM on February 18, 2016: contributor

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

in qa/rpc-tests/importmulti.py:None in 3f2e87ef7d outdated

   6 | +from test_framework.test_framework import BitcoinTestFramework
   7 | +from test_framework.util import *
   8 | +
   9 | +class ImportMultiTest (BitcoinTestFramework):
  10 | +
  11 | +    def check_fee_amount(self, curr_balance, balance_with_fee, fee_per_byte, tx_size):

instagibbs commented at 8:00 PM on February 18, 2016:

This is never used?

pedrobranco commented at 8:05 PM on February 18, 2016:

True, i will remove.

pedrobranco force-pushed on Feb 19, 2016

laanwj commented at 9:51 AM on February 19, 2016: member

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 commented at 10:29 AM on February 19, 2016: member

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

luke-jr commented at 11:45 AM on February 25, 2016: member

GBT also uses an options Object

luke-jr referenced this in commit f25959231d on Feb 25, 2016

jtimon commented at 11:10 PM on February 25, 2016: contributor

concept ACK

pedrobranco force-pushed on Mar 3, 2016

pedrobranco commented at 2:44 PM on March 10, 2016: contributor

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 commented at 2:51 PM on March 10, 2016: member

Passing along error messages would be hugely helpful, yes.

pedrobranco commented at 3:32 PM on March 10, 2016: contributor

@instagibbs :+1:

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 commented at 3:12 PM on March 14, 2016: member

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

pedrobranco force-pushed on Mar 16, 2016

in qa/rpc-tests/importmulti.py:None in 960f08667b outdated

   8 | +
   9 | +class ImportMultiTest (BitcoinTestFramework):
  10 | +    def setup_chain(self):
  11 | +        print("Initializing test directory "+self.options.tmpdir)
  12 | +        initialize_chain_clean(self.options.tmpdir, 4)
  13 | +

mrbandrews commented at 6:55 PM on March 17, 2016:

Nit: only need to initialize 2 nodes

in qa/rpc-tests/importmulti.py:None in 960f08667b outdated

  11 | +        print("Initializing test directory "+self.options.tmpdir)
  12 | +        initialize_chain_clean(self.options.tmpdir, 4)
  13 | +
  14 | +    def setup_network(self, split=False):
  15 | +        self.nodes = start_nodes(2, self.options.tmpdir)
  16 | +        connect_nodes_bi(self.nodes,0,1)

mrbandrews commented at 6:56 PM on March 17, 2016:

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

in qa/rpc-tests/importmulti.py:None in 960f08667b outdated

  13 | +
  14 | +    def setup_network(self, split=False):
  15 | +        self.nodes = start_nodes(2, self.options.tmpdir)
  16 | +        connect_nodes_bi(self.nodes,0,1)
  17 | +        self.is_network_split=False
  18 | +        self.sync_all()

mrbandrews commented at 6:56 PM on March 17, 2016:

See above, no need to connect/sync

in qa/rpc-tests/importmulti.py:None in 960f08667b outdated

  48 | +
  49 | +        #Check only one address
  50 | +        address_info = self.nodes[0].validateaddress(address1)
  51 | +        assert_equal(address_info['ismine'], True)
  52 | +
  53 | +        self.sync_all()

mrbandrews commented at 6:56 PM on March 17, 2016:

Same comment

mrbandrews commented at 6:59 PM on March 17, 2016: contributor

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 force-pushed on Mar 22, 2016

pedrobranco force-pushed on Mar 28, 2016

pedrobranco commented at 1:10 PM on March 28, 2016: contributor

@mrbandrews Test added.

pedrobranco force-pushed on Mar 28, 2016

in qa/rpc-tests/importmulti.py:None in 7fd4248fa3 outdated

 106 | +        assert_equal(result2[4]['success'], True)
 107 | +
 108 | +        # empty json case
 109 | +        try:
 110 | +            self.nodes[1].importmulti()
 111 | +            raise

MarcoFalke commented at 1:31 PM on March 28, 2016:

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 commented at 2:41 PM on March 28, 2016:

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 commented at 3:15 PM on March 29, 2016:

Yes.

in qa/rpc-tests/importmulti.py:None in 7fd4248fa3 outdated

 122 | +        assert_equal(result3[0]['success'], True)
 123 | +        try:    #JSON field "error" doesn't exist in success:true
 124 | +            result3[0]['error']
 125 | +            raise
 126 | +        except:
 127 | +            pass

MarcoFalke commented at 2:47 PM on March 28, 2016:

Nit: Some more dead code

MarcoFalke commented at 2:49 PM on March 28, 2016: member

Concept ACK

laanwj commented at 9:16 AM on March 29, 2016: member

Needs rebase after #7558

pedrobranco force-pushed on Mar 29, 2016

pedrobranco commented at 3:30 PM on March 29, 2016: contributor

Nit: Would be nice to be more verbose here. @MarcoFalke What do you suggest? @laanwj Rebased.

pedrobranco commented at 3:31 PM on April 4, 2016: contributor

@laanwj Are you considering merging this soon?

pedrobranco force-pushed on Apr 6, 2016

sipa commented at 9:40 AM on April 14, 2016: member

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.

in src/wallet/rpcdump.cpp:None in 90b6183fa8 outdated

 668 | +            "      \"type\": \"privkey | pubkey | address | script\", (string, required) Type of address\n"
 669 | +            "      \"value\": \"...\",                                (string, required) Value of the address\n"
 670 | +            "      \"timestamp\": 1454686740,                         (integer, optional) Timestamp\n"
 671 | +            "      \"label\": \"...\"                                 (string, optional) Label\n"
 672 | +            "      \"p2sh\": true | false                             (bool, optional, default=false) Value is a P2SH (type=script only)\n"
 673 | +            "    }\n"

jonasschnelli commented at 9:54 AM on April 14, 2016:

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 commented at 9:57 AM on April 14, 2016: member

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 commented at 10:51 AM on April 14, 2016: contributor

concept ACK

pedrobranco commented at 12:26 PM on April 18, 2016: contributor

@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 commented at 1:30 PM on April 25, 2016: member

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 commented at 1:33 PM on April 25, 2016: member

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 commented at 1:34 PM on April 25, 2016: member

Then add a different type for either case?

laanwj commented at 1:36 PM on April 25, 2016: member

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 commented at 1:38 PM on April 25, 2016: member

@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 commented at 3:32 PM on April 25, 2016: member

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 commented at 1:41 PM on May 2, 2016: member

@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 commented at 1:47 PM on May 2, 2016: member

@sipa replace output by scriptPubKey ?

sipa commented at 1:52 PM on May 2, 2016: member

@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 commented at 2:59 PM on May 2, 2016: contributor

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

in qa/rpc-tests/importmulti.py:None in 90b6183fa8 outdated

   0 | @@ -0,0 +1,147 @@
   1 | +#!/usr/bin/env python2

MarcoFalke commented at 9:37 AM on May 6, 2016:

Mind to change this to py3?

pedrobranco commented at 10:04 AM on May 6, 2016:

Sure.

promag commented at 9:27 AM on May 25, 2016: member

@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 commented at 9:28 AM on May 25, 2016: member

@arowser this is still work in progress.

sipa commented at 1:57 PM on May 25, 2016: member

@promag Looks good to me.

laanwj added this to the milestone 0.13.0 on Jun 8, 2016

laanwj commented at 12:04 PM on June 8, 2016: member

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 commented at 1:54 PM on June 8, 2016: contributor

@laanwj Yes, i think is realistic.

laanwj commented at 9:24 AM on June 13, 2016: member

Ping...

pedrobranco commented at 11:43 AM on June 13, 2016: contributor

@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 commented at 10:21 AM on June 15, 2016: contributor

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

pedrobranco force-pushed on Jun 15, 2016

in qa/rpc-tests/importmulti.py:None in 8255b35f6c outdated

   7 | +from test_framework.util import *
   8 | +
   9 | +class ImportMultiTest (BitcoinTestFramework):
  10 | +    def setup_chain(self):
  11 | +        print("Initializing test directory "+self.options.tmpdir)
  12 | +        initialize_chain_clean(self.options.tmpdir, 2)

MarcoFalke commented at 7:41 PM on June 15, 2016:

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 commented at 9:28 AM on June 16, 2016:

Sure, and looks way better.

in qa/pull-tester/rpc-tests.py:None in 8255b35f6c outdated

 136 | @@ -137,6 +137,7 @@
 137 |      'p2p-versionbits-warning.py',
 138 |      'importprunedfunds.py',
 139 |      'signmessages.py',
 140 | +    'importmulti.py'

MarcoFalke commented at 9:32 AM on June 16, 2016:

micro-Nit: missing trailing ,

in qa/rpc-tests/importmulti.py:None in 8255b35f6c outdated

  14 | +    def setup_network(self, split=False):
  15 | +        self.nodes = start_nodes(2, self.options.tmpdir)
  16 | +        self.is_network_split=False
  17 | +
  18 | +    def run_test (self):
  19 | +        import time

MarcoFalke commented at 9:33 AM on June 16, 2016:

Nit: Might as well place this in the header

pedrobranco force-pushed on Jun 16, 2016

laanwj commented at 1:51 PM on June 20, 2016: member

Looks good to me - utACK https://github.com/bitcoin/bitcoin/pull/7551/commits/d72a88616f3ad0578ca6e6d8f025db5574f9bf07

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 commented at 2:29 PM on June 20, 2016: contributor

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 commented at 2:31 PM on June 20, 2016: member

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

pedrobranco commented at 2:56 PM on June 20, 2016: contributor

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 commented at 8:32 AM on June 21, 2016: member

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

MarcoFalke removed this from the milestone 0.13.0 on Jun 21, 2016

luke-jr referenced this in commit ac96079d55 on Jun 28, 2016

luke-jr referenced this in commit e69c0b5be2 on Jun 28, 2016

sipa commented at 10:04 AM on July 1, 2016: member

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 commented at 10:11 AM on July 1, 2016: contributor

@sipa ACK.

pedrobranco force-pushed on Jul 1, 2016

pedrobranco force-pushed on Jul 18, 2016

pedrobranco commented at 5:12 PM on July 18, 2016: contributor

Consistency check added + internal + watchonly.

pedrobranco commented at 9:31 AM on July 28, 2016: contributor

Can someone please review the PR?

jonasschnelli commented at 9:32 AM on July 28, 2016: contributor

Will test today.

in src/chain.cpp:None in 7832208760 outdated

  69 | +    }
  70 | +};
  71 | +
  72 | +CBlockIndex* CChain::FindLatestBefore(int64_t nTime) const
  73 | +{
  74 | +    std::vector<CBlockIndex*>::const_iterator lower = std::lower_bound(vChain.begin(), vChain.end(), nTime, SmallerThanTimestamp());

jonasschnelli commented at 10:03 AM on July 28, 2016:

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.

in src/wallet/rpcdump.cpp:None in 7832208760 outdated

 988 | +        const UniValue result = processImport(data);
 989 | +        response.push_back(result);
 990 | +    }
 991 | +
 992 | +    if (fRescan && fRunScan && request.size()) {
 993 | +        CBlockIndex* pindex = nLowestTimestamp > 0 ? chainActive.FindLatestBefore(nLowestTimestamp) : chainActive.Genesis();

jonasschnelli commented at 10:10 AM on July 28, 2016:

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 commented at 11:03 AM on July 29, 2016:

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 commented at 11:50 AM on July 29, 2016:

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

in src/wallet/rpcdump.cpp:None in 7832208760 outdated

 983 | +        fRescan = false;
 984 | +
 985 | +    UniValue response(UniValue::VARR);
 986 | +
 987 | +    BOOST_FOREACH (const UniValue& data, request.getValues()) {
 988 | +        const UniValue result = processImport(data);

jonasschnelli commented at 10:25 AM on July 28, 2016:

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

in src/wallet/rpcdump.cpp:None in 7832208760 outdated

 750 | +                    pwalletMain->MarkDirty();
 751 | +                    pwalletMain->SetAddressBook(vchAddress, label, "receive");
 752 | +
 753 | +                    if (pwalletMain->HaveKey(vchAddress))
 754 | +                        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Already have this key");
 755 | +                    ;

jonasschnelli commented at 10:27 AM on July 28, 2016:

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

in src/wallet/rpcdump.cpp:None in 7832208760 outdated

 972 | +    }
 973 | +
 974 | +    LOCK2(cs_main, pwalletMain->cs_wallet);
 975 | +    EnsureWalletIsUnlocked();
 976 | +
 977 | +    bool fRunScan = false;

jonasschnelli commented at 6:21 PM on July 28, 2016:

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

pedrobranco commented at 10:47 AM on July 29, 2016:

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

jonasschnelli commented at 11:49 AM on July 29, 2016:

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 commented at 1:15 PM on July 29, 2016:

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 commented at 6:22 PM on July 28, 2016: contributor

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.

jonasschnelli added this to the milestone 0.14 on Jul 28, 2016

pedrobranco force-pushed on Jul 29, 2016

pedrobranco force-pushed on Aug 2, 2016

pedrobranco commented at 9:31 AM on August 3, 2016: contributor

@jonasschnelli Can you please test the rescan behavior?

in src/wallet/rpcdump.cpp:None in 51fd0d6a61 outdated

1054 | +            nLowestTimestamp = timestamp;
1055 | +        }
1056 | +    }
1057 | +
1058 | +    if (fRescan && fRunScan && request.size()) {
1059 | +        CBlockIndex* pindex = nLowestTimestamp > 0 ? chainActive.FindLatestBefore(nLowestTimestamp) : chainActive.Genesis();

jonasschnelli commented at 3:34 PM on August 4, 2016:

I guess nLowestTimestamp is always > 0.

pedrobranco force-pushed on Aug 4, 2016

luke-jr referenced this in commit b48d08b325 on Aug 7, 2016

luke-jr referenced this in commit 81a7fab0cb on Aug 9, 2016

laanwj commented at 10:17 AM on September 9, 2016: member

What needs to be done here?

pedrobranco commented at 11:03 AM on September 9, 2016: contributor

What needs to be done here?

Maybe add some tests for the rescan feature.

laanwj commented at 2:03 PM on September 19, 2016: member

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.

in src/wallet/rpcdump.cpp:None in 1bcc021f7c outdated

1026 | +    bool fRunScan = false;
1027 | +    const int64_t minimumTimestamp = 1;
1028 | +    int64_t nLowestTimestamp;
1029 | +
1030 | +    if (fRescan && chainActive.Tip()) {
1031 | +        nLowestTimestamp = chainActive.Tip()->GetBlockTime();

jonasschnelli commented at 2:28 PM on September 28, 2016:

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 commented at 2:54 PM on September 28, 2016:

We can prevent rescanning if nLowestTimestamp is in the future:

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

in src/wallet/rpcdump.cpp:None in 1bcc021f7c outdated

1029 | +
1030 | +    if (fRescan && chainActive.Tip()) {
1031 | +        nLowestTimestamp = chainActive.Tip()->GetBlockTime();
1032 | +    } else {
1033 | +        fRescan = false;
1034 | +    }

jonasschnelli commented at 2:32 PM on September 28, 2016:

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

pedrobranco commented at 2:44 PM on September 28, 2016:

fRescan is initially set to true.

jonasschnelli commented at 2:46 PM on September 28, 2016:

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

pedrobranco commented at 2:57 PM on September 28, 2016:

No problem @jonasschnelli .

jonasschnelli approved

jonasschnelli commented at 2:43 PM on September 28, 2016: contributor

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

pedrobranco force-pushed on Sep 28, 2016

pedrobranco commented at 9:21 AM on September 29, 2016: contributor

Rebased and nits addressed.

in src/wallet/rpcdump.cpp:None in e4d03e7ab0 outdated

 840 | +                // add to address book or update label
 841 | +                if (pubKeyAddress.IsValid()) {
 842 | +                    pwalletMain->SetAddressBook(pubKeyAddress.Get(), label, "receive");
 843 | +                }
 844 | +
 845 | +                // TODO Is this necessary?

laanwj commented at 2:46 PM on October 5, 2016:

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

pedrobranco commented at 9:47 AM on October 6, 2016:

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 commented at 1:33 PM on October 19, 2016: member

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

Add importmulti rpc call cb08fdbf78

Add consistency check to RPC call importmulti 215caba4ed

pedrobranco force-pushed on Oct 19, 2016

pedrobranco commented at 9:44 PM on October 19, 2016: contributor

Rebased.

jonasschnelli commented at 6:57 AM on October 20, 2016: contributor

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

laanwj merged this on Oct 20, 2016

laanwj closed this on Oct 20, 2016

laanwj referenced this in commit f2d705629b on Oct 20, 2016

laanwj commented at 7:05 AM on October 20, 2016: member

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.

luke-jr referenced this in commit eb039ce0c5 on Oct 20, 2016

luke-jr referenced this in commit 12e852a616 on Oct 20, 2016

rebroad commented at 2:31 AM on October 25, 2016: contributor

bisect identifies cb08fdbf78685b55029768524ca867772711c32b 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..

in src/wallet/rpcdump.cpp:None in 215caba4ed

 766 | +                    }
 767 | +
 768 | +                    CPubKey pubkey = key.GetPubKey();
 769 | +                    assert(key.VerifyPubKey(pubkey));
 770 | +
 771 | +                    CKeyID vchAddress = pubkey.GetID();

rebroad commented at 2:53 AM on October 25, 2016:

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

laanwj commented at 5:34 AM on October 25, 2016: member

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.

codablock referenced this in commit 0de540fd3e on Sep 19, 2017

codablock referenced this in commit efded3ca9c on Jan 13, 2018

andvgal referenced this in commit c282ca9ead on Jan 6, 2019

CryptoCentric referenced this in commit f550288a38 on Feb 15, 2019

str4d referenced this in commit 33293c7586 on Dec 6, 2019

str4d referenced this in commit 6e8f11cde0 on Dec 9, 2019

str4d referenced this in commit df00e51e3e on Dec 19, 2019

zkbot referenced this in commit 900ed4555f on Dec 19, 2019

furszy referenced this in commit a279df6e61 on Apr 6, 2021

DrahtBot locked this on Sep 8, 2021