Require timestamps for importmulti keys #9682

ryanofsky · 2017-02-03T21:42:42Z

Require timestamps for importmulti keys

Additionally, accept a "now" timestamp, to allow avoiding rescans for keys
which are known never to have been used.

Note that the behavior when "now" is specified is slightly different than the
previous behavior when no timestamp was specified at all. Previously, when no
timestamp was specified, it would avoid rescanning during the importmulti call,
but set the key's nCreateTime value to 1, which would not prevent future block
reads in later ScanForWalletTransactions calls. With this change, passing a
"now" timestamp will set the key's nCreateTime to the current block time
instead of 1.

Fixes #9491

TheBlueMatt · 2017-02-03T21:53:15Z

Needs 0.14 tag.

achow101 · 2017-02-03T22:34:37Z

src/wallet/rpcdump.cpp

@@ -970,13 +984,16 @@ UniValue importmulti(const JSONRPCRequest& mainRequest)
            "  [     (array of json objects)\n"
            "    {\n"
            "      \"scriptPubKey\": \"<script>\" | { \"address\":\"<address>\" }, (string / json, required) Type of scriptPubKey (string for script, json for address)\n"
+            "      \"timestamp\": 1454686740 | \"now\"                       , (integer / string) Integer timestamp, or the string \"now\" to substitute the current synced\n"


You should say whether it is required or optional

If we are at this, why do we have the number 1454686740 here? Can't we just say <timestamp>?

Updated in af06509. Personally I think "timestamp": 1454686740 is more informative than "timestamp": <timestamp>, but made the requested change anyway.

gmaxwell · 2017-02-04T22:35:54Z

Concept ACK but people with more opinions (and knowledge about json rpc norms) should comment on the type of the timestamp argument being variable.

jonasschnelli · 2017-02-05T19:22:12Z

Concept ACK, will test.

I liked the idea from @sipa where he mentioned that "unknown" as timestamp for avoiding rescans seems to be the better choice (instead of "now"). "Now" would imply that the key was generated during import which is (almost) impossible.

maflcko · 2017-02-05T19:51:41Z

sipa proposed `timestamp: "unused"` instead of "now". #9491 (comment)

laanwj · 2017-02-06T11:00:28Z

Concept ACK

Concept ACK but people with more opinions (and knowledge about json rpc norms) should comment on the type of the timestamp argument being variable.

It's hard to wrap in bindings for statically typed languages - could be worked around with an extra flag that specifies how to handle the timestamp.

Update timestamp option documentation as suggested bitcoin#9682 (comment) bitcoin#9682 (comment)

ryanofsky · 2017-02-06T16:31:02Z

Added RPC test to verify that this is correctly interpreting the "now" value and saving the current time as the key creation time in 2cd875a.

ryanofsky · 2017-02-06T16:41:11Z

Squashed 2cd875a -> c07ebb6 (multinow.3 -> multinow.4)

TheBlueMatt · 2017-02-07T15:30:20Z

src/wallet/rpcdump.cpp

@@ -1015,6 +1032,12 @@ UniValue importmulti(const JSONRPCRequest& mainRequest)
    LOCK2(cs_main, pwalletMain->cs_wallet);
    EnsureWalletIsUnlocked();

+    // Verify all timestamps are present before importing any keys.
+    const int64_t now = chainActive.Tip() ? chainActive.Tip()->GetBlockTime() : 0;


Can we use the MTP here?

Switched to MTP in dc402a4

TheBlueMatt · 2017-02-07T15:41:06Z

utACK c07ebb6 if we switch to using MTP for the "now" timestamp. Did not review tests.

ryanofsky

Now using MTP in dc402a4

ryanofsky · 2017-02-07T16:10:47Z

src/wallet/rpcdump.cpp

@@ -1015,6 +1032,12 @@ UniValue importmulti(const JSONRPCRequest& mainRequest)
    LOCK2(cs_main, pwalletMain->cs_wallet);
    EnsureWalletIsUnlocked();

+    // Verify all timestamps are present before importing any keys.
+    const int64_t now = chainActive.Tip() ? chainActive.Tip()->GetBlockTime() : 0;


Switched to MTP in dc402a4

TheBlueMatt · 2017-02-08T16:18:54Z

utACK dc402a4

laanwj · 2017-02-09T06:34:47Z

I liked the idea from @sipa where he mentioned that "unknown" as timestamp for avoiding rescans seems to be the better choice (instead of "now"). "Now" would imply that the key was generated during import which is (almost) impossible.
sipa proposed timestamp: "unused" instead of "now".

A "unused" flag makes sense, however that is not a property of the timestamp.

kallewoof · 2017-02-09T08:16:42Z

Timestamp = unused sounds strange to me as well. Timestamp = now makes more sense.

kallewoof

utACK, did not review tests

kallewoof · 2017-02-09T08:19:03Z

src/rpc/misc.cpp

@@ -167,6 +167,7 @@ UniValue validateaddress(const JSONRPCRequest& request)
            "  \"pubkey\" : \"publickeyhex\",    (string) The hex value of the raw public key\n"
            "  \"iscompressed\" : true|false,  (boolean) If the address is compressed\n"
            "  \"account\" : \"account\"         (string) DEPRECATED. The account associated with the address, \"\" is the default account\n"
+            "  \"timestamp\" : <timestamp>,      (number, optional) The creation time of the key if available\n"


Nit: Non-string arguments tend to be represented as simply name, not <name>, e.g. https://github.com/bitcoin/bitcoin/blame/master/src/rpc/blockchain.cpp#L649

Fixed in 38c01b5

kallewoof · 2017-02-09T08:20:00Z

src/wallet/rpcdump.cpp

@@ -640,7 +640,8 @@ UniValue dumpwallet(const JSONRPCRequest& request)
 }


-UniValue processImport(const UniValue& data) {
+UniValue processImport(const UniValue& data, int64_t timestamp)


Nit: const int64_t timestamp
Nit 2 / coding standard: Functions in Bitcoin generally start with a capital letter, i.e. ProcessImport here. (Exception being the RPC commands, which match the CLI command names.) I know you didn't name this one, but it coincides with the new function below (getImportTimestamp). Renaming this one should not be a big issue, as it is used in one place only.

Fixed in 6524d40

kallewoof · 2017-02-09T08:22:11Z

src/wallet/rpcdump.cpp

@@ -958,6 +958,20 @@ UniValue processImport(const UniValue& data) {
    }
 }

+int64_t getImportTimestamp(const UniValue& data, int64_t now)


Nit / coding standard / repeat: GetImportTimestamp (see above)

Fixed in 6524d40

kallewoof · 2017-02-09T08:23:16Z

src/wallet/rpcdump.cpp

@@ -970,13 +984,16 @@ UniValue importmulti(const JSONRPCRequest& mainRequest)
            "  [     (array of json objects)\n"
            "    {\n"
            "      \"scriptPubKey\": \"<script>\" | { \"address\":\"<address>\" }, (string / json, required) Type of scriptPubKey (string for script, json for address)\n"
+            "      \"timestamp\": <timestamp> | \"now\"                      , (integer / string, required) Integer timestamp, or the string \"now\" to substitute the current synced\n"


Nit / repeat: <timestamp> -> timestamp (see above comment)

Fixed in 6524d40

kallewoof · 2017-02-09T08:25:14Z

src/wallet/rpcdump.cpp

@@ -1015,6 +1032,12 @@ UniValue importmulti(const JSONRPCRequest& mainRequest)
    LOCK2(cs_main, pwalletMain->cs_wallet);
    EnsureWalletIsUnlocked();

+    // Verify all timestamps are present before importing any keys.
+    const int64_t now = chainActive.Tip() ? chainActive.Tip()->GetMedianTimePast() : 0;


I don't know if 0 is a good default for now, but it doesn't seem harmful either. I wonder if the function shouldn't simply throw a runtime_exception if !chainActive.Tip() saying to sync up first..

Would be ok with that change, but prefer the current behavior of not aborting just to give a notification that the chain is not synced when the outcome is safe either way.

OK, yeah I think a notification would be useful.

kallewoof · 2017-02-09T08:38:51Z

src/wallet/rpcdump.cpp

+            "      \"timestamp\": <timestamp> | \"now\"                      , (integer / string, required) Integer timestamp, or the string \"now\" to substitute the current synced\n"
+            "                                                              blockchain time. The timestamp of the oldest key will determine how far back blockchain rescans\n"
+            "                                                              for missing wallet transactions need to begin. \"now\" can be specified to bypass scanning, for\n"
+            "                                                              keys which are known to never have been used, and 0 can be specified to scan the entire blockchain.\n"


Language: "The timestamp of the oldest key will determine how far back blockchain rescans need to begin for finding missing wallet transactions." maybe?

Fixed in 6524d40

Fix validateaddress timestamp argument documentation as suggested bitcoin#9682 (comment)

Style and languages fixes suggested in bitcoin#9682 (comment) bitcoin#9682 (comment) bitcoin#9682 (comment) bitcoin#9682 (comment)

ryanofsky · 2017-02-09T11:38:48Z

Squashed 6524d40 -> 715f05d (multinow.6 -> multinow.7)

jonasschnelli

Tested ACK 715f05d

Additionally, accept a "now" timestamp, to allow avoiding rescans for keys which are known never to have been used. Note that the behavior when "now" is specified is slightly different than the previous behavior when no timestamp was specified at all. Previously, when no timestamp was specified, it would avoid rescanning during the importmulti call, but set the key's nCreateTime value to 1, which would not prevent future block reads in later ScanForWalletTransactions calls. With this change, passing a "now" timestamp will set the key's nCreateTime to the current block time instead of 1. Fixes bitcoin#9491

Easiest way to test this was to expose the timestamp via the validateaddress RPC (which was already looking up and returning key metadata).

ryanofsky · 2017-02-10T20:48:18Z

Rebased 715f05d -> 266a811 (multinow.7 -> multinow.8) following the #9227 merge.

This PR didn't conflict with #9227, but I'm about to rebase #9108 on top of this, and #9108 did conflict with #9227.

morcos · 2017-02-13T19:47:25Z

utACK 266a811