From 442887f27fb5809e72862d4385eba86588bc97d0 Mon Sep 17 00:00:00 2001
From: Russell Yanofsky <russ@yanofsky.org>
Date: Fri, 3 Feb 2017 16:23:13 -0500
Subject: [PATCH 1/3] 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
---
 qa/rpc-tests/import-rescan.py |  1 +
 qa/rpc-tests/importmulti.py   | 37 ++++++++++++++++++++++++++++++++---
 src/wallet/rpcdump.cpp        | 35 +++++++++++++++++++++++++++------
 3 files changed, 64 insertions(+), 9 deletions(-)

diff --git a/qa/rpc-tests/import-rescan.py b/qa/rpc-tests/import-rescan.py
index e683df26d..8f60e63e2 100755
--- a/qa/rpc-tests/import-rescan.py
+++ b/qa/rpc-tests/import-rescan.py
@@ -33,6 +33,7 @@ def call_import_rpc(call, data, address, scriptPubKey, pubkey, key, label, node,
             "scriptPubKey": {
                 "address": address
             },
+            "timestamp": "now",
             "pubkeys": [pubkey] if data == Data.pub else [],
             "keys": [key] if data == Data.priv else [],
             "label": label,
diff --git a/qa/rpc-tests/importmulti.py b/qa/rpc-tests/importmulti.py
index e100a3af9..b4d4b6c5b 100755
--- a/qa/rpc-tests/importmulti.py
+++ b/qa/rpc-tests/importmulti.py
@@ -52,7 +52,8 @@ class ImportMultiTest (BitcoinTestFramework):
         result = self.nodes[1].importmulti([{
             "scriptPubKey": {
                 "address": address['address']
-            }
+            },
+            "timestamp": "now",
         }])
         assert_equal(result[0]['success'], True)
         address_assert = self.nodes[1].validateaddress(address['address'])
@@ -65,6 +66,7 @@ class ImportMultiTest (BitcoinTestFramework):
         address = self.nodes[0].validateaddress(self.nodes[0].getnewaddress())
         result = self.nodes[1].importmulti([{
             "scriptPubKey": address['scriptPubKey'],
+            "timestamp": "now",
             "internal": True
         }])
         assert_equal(result[0]['success'], True)
@@ -76,7 +78,8 @@ class ImportMultiTest (BitcoinTestFramework):
         print("Should not import a scriptPubKey without internal flag")
         address = self.nodes[0].validateaddress(self.nodes[0].getnewaddress())
         result = self.nodes[1].importmulti([{
-            "scriptPubKey": address['scriptPubKey']
+            "scriptPubKey": address['scriptPubKey'],
+            "timestamp": "now",
         }])
         assert_equal(result[0]['success'], False)
         assert_equal(result[0]['error']['code'], -8)
@@ -93,6 +96,7 @@ class ImportMultiTest (BitcoinTestFramework):
             "scriptPubKey": {
                 "address": address['address']
             },
+            "timestamp": "now",
             "pubkeys": [ address['pubkey'] ]
         }])
         assert_equal(result[0]['success'], True)
@@ -106,6 +110,7 @@ class ImportMultiTest (BitcoinTestFramework):
         address = self.nodes[0].validateaddress(self.nodes[0].getnewaddress())
         request = [{
             "scriptPubKey": address['scriptPubKey'],
+            "timestamp": "now",
             "pubkeys": [ address['pubkey'] ],
             "internal": True
         }]
@@ -120,6 +125,7 @@ class ImportMultiTest (BitcoinTestFramework):
         address = self.nodes[0].validateaddress(self.nodes[0].getnewaddress())
         request = [{
             "scriptPubKey": address['scriptPubKey'],
+            "timestamp": "now",
             "pubkeys": [ address['pubkey'] ]
         }]
         result = self.nodes[1].importmulti(request)
@@ -137,6 +143,7 @@ class ImportMultiTest (BitcoinTestFramework):
             "scriptPubKey": {
                 "address": address['address']
             },
+            "timestamp": "now",
             "keys": [ self.nodes[0].dumpprivkey(address['address']) ]
         }])
         assert_equal(result[0]['success'], True)
@@ -151,6 +158,7 @@ class ImportMultiTest (BitcoinTestFramework):
             "scriptPubKey": {
                 "address": address['address']
             },
+            "timestamp": "now",
             "keys": [ self.nodes[0].dumpprivkey(address['address']) ],
             "watchonly": True
         }])
@@ -166,6 +174,7 @@ class ImportMultiTest (BitcoinTestFramework):
         address = self.nodes[0].validateaddress(self.nodes[0].getnewaddress())
         result = self.nodes[1].importmulti([{
             "scriptPubKey": address['scriptPubKey'],
+            "timestamp": "now",
             "keys": [ self.nodes[0].dumpprivkey(address['address']) ],
             "internal": True
         }])
@@ -179,6 +188,7 @@ class ImportMultiTest (BitcoinTestFramework):
         address = self.nodes[0].validateaddress(self.nodes[0].getnewaddress())
         result = self.nodes[1].importmulti([{
             "scriptPubKey": address['scriptPubKey'],
+            "timestamp": "now",
             "keys": [ self.nodes[0].dumpprivkey(address['address']) ]
         }])
         assert_equal(result[0]['success'], False)
@@ -203,7 +213,8 @@ class ImportMultiTest (BitcoinTestFramework):
         result = self.nodes[1].importmulti([{
             "scriptPubKey": {
                 "address": multi_sig_script['address']
-            }
+            },
+            "timestamp": "now",
         }])
         assert_equal(result[0]['success'], True)
         address_assert = self.nodes[1].validateaddress(multi_sig_script['address'])
@@ -229,6 +240,7 @@ class ImportMultiTest (BitcoinTestFramework):
             "scriptPubKey": {
                 "address": multi_sig_script['address']
             },
+            "timestamp": "now",
             "redeemscript": multi_sig_script['redeemScript']
         }])
         assert_equal(result[0]['success'], True)
@@ -253,6 +265,7 @@ class ImportMultiTest (BitcoinTestFramework):
             "scriptPubKey": {
                 "address": multi_sig_script['address']
             },
+            "timestamp": "now",
             "redeemscript": multi_sig_script['redeemScript'],
             "keys": [ self.nodes[0].dumpprivkey(sig_address_1['address']), self.nodes[0].dumpprivkey(sig_address_2['address'])]
         }])
@@ -277,6 +290,7 @@ class ImportMultiTest (BitcoinTestFramework):
             "scriptPubKey": {
                 "address": multi_sig_script['address']
             },
+            "timestamp": "now",
             "redeemscript": multi_sig_script['redeemScript'],
             "keys": [ self.nodes[0].dumpprivkey(sig_address_1['address']), self.nodes[0].dumpprivkey(sig_address_2['address'])],
             "watchonly": True
@@ -294,6 +308,7 @@ class ImportMultiTest (BitcoinTestFramework):
             "scriptPubKey": {
                 "address": address['address']
             },
+            "timestamp": "now",
             "pubkeys": [ address2['pubkey'] ]
         }])
         assert_equal(result[0]['success'], False)
@@ -310,6 +325,7 @@ class ImportMultiTest (BitcoinTestFramework):
         address2 = self.nodes[0].validateaddress(self.nodes[0].getnewaddress())
         request = [{
             "scriptPubKey": address['scriptPubKey'],
+            "timestamp": "now",
             "pubkeys": [ address2['pubkey'] ],
             "internal": True
         }]
@@ -330,6 +346,7 @@ class ImportMultiTest (BitcoinTestFramework):
             "scriptPubKey": {
                 "address": address['address']
             },
+            "timestamp": "now",
             "keys": [ self.nodes[0].dumpprivkey(address2['address']) ]
         }])
         assert_equal(result[0]['success'], False)
@@ -346,6 +363,7 @@ class ImportMultiTest (BitcoinTestFramework):
         address2 = self.nodes[0].validateaddress(self.nodes[0].getnewaddress())
         result = self.nodes[1].importmulti([{
             "scriptPubKey": address['scriptPubKey'],
+            "timestamp": "now",
             "keys": [ self.nodes[0].dumpprivkey(address2['address']) ],
             "internal": True
         }])
@@ -356,5 +374,18 @@ class ImportMultiTest (BitcoinTestFramework):
         assert_equal(address_assert['iswatchonly'], False)
         assert_equal(address_assert['ismine'], False)
 
+        # Bad or missing timestamps
+        print("Should throw on invalid or missing timestamp values")
+        assert_raises_message(JSONRPCException, 'Missing required timestamp field for key',
+            self.nodes[1].importmulti, [{
+                "scriptPubKey": address['scriptPubKey'],
+            }])
+        assert_raises_message(JSONRPCException, 'Expected number or "now" timestamp value for key. got type string',
+            self.nodes[1].importmulti, [{
+                "scriptPubKey": address['scriptPubKey'],
+                "timestamp": "",
+            }])
+
+
 if __name__ == '__main__':
     ImportMultiTest ().main ()
diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp
index 7d4ed70ed..4674c95bf 100644
--- a/src/wallet/rpcdump.cpp
+++ b/src/wallet/rpcdump.cpp
@@ -640,7 +640,8 @@ UniValue dumpwallet(const JSONRPCRequest& request)
 }
 
 
-UniValue processImport(const UniValue& data) {
+UniValue ProcessImport(const UniValue& data, const int64_t timestamp)
+{
     try {
         bool success = false;
 
@@ -659,7 +660,6 @@ UniValue processImport(const UniValue& data) {
         const bool& internal = data.exists("internal") ? data["internal"].get_bool() : false;
         const bool& watchOnly = data.exists("watchonly") ? data["watchonly"].get_bool() : false;
         const string& label = data.exists("label") && !internal ? data["label"].get_str() : "";
-        const int64_t& timestamp = data.exists("timestamp") && data["timestamp"].get_int64() > 1 ? data["timestamp"].get_int64() : 1;
 
         bool isScript = scriptPubKey.getType() == UniValue::VSTR;
         bool isP2SH = strRedeemScript.length() > 0;
@@ -958,6 +958,20 @@ UniValue processImport(const UniValue& data) {
     }
 }
 
+int64_t GetImportTimestamp(const UniValue& data, int64_t now)
+{
+    if (data.exists("timestamp")) {
+        const UniValue& timestamp = data["timestamp"];
+        if (timestamp.isNum()) {
+            return timestamp.get_int64();
+        } else if (timestamp.isStr() && timestamp.get_str() == "now") {
+            return now;
+        }
+        throw JSONRPCError(RPC_TYPE_ERROR, strprintf("Expected number or \"now\" timestamp value for key. got type %s", uvTypeName(timestamp.type())));
+    }
+    throw JSONRPCError(RPC_TYPE_ERROR, "Missing required timestamp field for key");
+}
+
 UniValue importmulti(const JSONRPCRequest& mainRequest)
 {
     // clang-format off
@@ -970,13 +984,17 @@ 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) Creation time of the key in seconds since epoch (Jan 1 1970 GMT),\n"
+            "                                                              or the string \"now\" to substitute the current synced blockchain time. The timestamp of the oldest\n"
+            "                                                              key will determine how far back blockchain rescans need to begin for missing wallet transactions.\n"
+            "                                                              \"now\" can be specified to bypass scanning, for keys which are known to never have been used, and\n"
+            "                                                              0 can be specified to scan the entire blockchain.\n"
             "      \"redeemscript\": \"<script>\"                            , (string, optional) Allowed only if the scriptPubKey is a P2SH address or a P2SH scriptPubKey\n"
             "      \"pubkeys\": [\"<pubKey>\", ... ]                         , (array, optional) Array of strings giving pubkeys that must occur in the output or redeemscript\n"
             "      \"keys\": [\"<key>\", ... ]                               , (array, optional) Array of strings giving private keys whose corresponding public keys must occur in the output or redeemscript\n"
             "      \"internal\": <true>                                    , (boolean, optional, default: false) Stating whether matching outputs should be be treated as not incoming payments\n"
             "      \"watchonly\": <true>                                   , (boolean, optional, default: false) Stating whether matching outputs should be considered watched even when they're not spendable, only allowed if keys are empty\n"
             "      \"label\": <label>                                      , (string, optional, default: '') Label to assign to the address (aka account name, for now), only allowed with internal=false\n"
-            "      \"timestamp\": 1454686740,                                (integer, optional, default now) Timestamp\n"
             "    }\n"
             "  ,...\n"
             "  ]\n"
@@ -1015,6 +1033,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;
+    for (const UniValue& data : requests.getValues()) {
+        GetImportTimestamp(data, now);
+    }
+
     bool fRunScan = false;
     const int64_t minimumTimestamp = 1;
     int64_t nLowestTimestamp = 0;
@@ -1028,7 +1052,8 @@ UniValue importmulti(const JSONRPCRequest& mainRequest)
     UniValue response(UniValue::VARR);
 
     BOOST_FOREACH (const UniValue& data, requests.getValues()) {
-        const UniValue result = processImport(data);
+        const int64_t timestamp = std::max(GetImportTimestamp(data, now), minimumTimestamp);
+        const UniValue result = ProcessImport(data, timestamp);
         response.push_back(result);
 
         if (!fRescan) {
@@ -1041,8 +1066,6 @@ UniValue importmulti(const JSONRPCRequest& mainRequest)
         }
 
         // Get the lowest timestamp.
-        const int64_t& timestamp = data.exists("timestamp") && data["timestamp"].get_int64() > minimumTimestamp ? data["timestamp"].get_int64() : minimumTimestamp;
-
         if (timestamp < nLowestTimestamp) {
             nLowestTimestamp = timestamp;
         }

From 3cf991756cf357b651c6415e3d950e5aa9d355ab Mon Sep 17 00:00:00 2001
From: Russell Yanofsky <russ@yanofsky.org>
Date: Mon, 6 Feb 2017 11:13:05 -0500
Subject: [PATCH 2/3] Add test to check new importmulti "now" value

Easiest way to test this was to expose the timestamp via the validateaddress
RPC (which was already looking up and returning key metadata).
---
 qa/rpc-tests/importmulti.py |  2 ++
 src/rpc/misc.cpp            | 15 +++++++++++----
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/qa/rpc-tests/importmulti.py b/qa/rpc-tests/importmulti.py
index b4d4b6c5b..b9874ccb8 100755
--- a/qa/rpc-tests/importmulti.py
+++ b/qa/rpc-tests/importmulti.py
@@ -139,6 +139,7 @@ class ImportMultiTest (BitcoinTestFramework):
         # Address + Private key + !watchonly
         print("Should import an address with private key")
         address = self.nodes[0].validateaddress(self.nodes[0].getnewaddress())
+        timestamp = self.nodes[1].getblock(self.nodes[1].getbestblockhash())['time']
         result = self.nodes[1].importmulti([{
             "scriptPubKey": {
                 "address": address['address']
@@ -150,6 +151,7 @@ class ImportMultiTest (BitcoinTestFramework):
         address_assert = self.nodes[1].validateaddress(address['address'])
         assert_equal(address_assert['iswatchonly'], False)
         assert_equal(address_assert['ismine'], True)
+        assert_equal(address_assert['timestamp'], timestamp)
 
         # Address + Private key + watchonly
         print("Should not import an address with private key and with watchonly")
diff --git a/src/rpc/misc.cpp b/src/rpc/misc.cpp
index 480c45516..25fad3c2e 100644
--- a/src/rpc/misc.cpp
+++ b/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 in seconds since epoch (Jan 1 1970 GMT)\n"
             "  \"hdkeypath\" : \"keypath\"       (string, optional) The HD keypath if the key is HD and available\n"
             "  \"hdmasterkeyid\" : \"<hash160>\" (string, optional) The Hash160 of the HD master pubkey\n"
             "}\n"
@@ -204,10 +205,16 @@ UniValue validateaddress(const JSONRPCRequest& request)
         if (pwalletMain && pwalletMain->mapAddressBook.count(dest))
             ret.push_back(Pair("account", pwalletMain->mapAddressBook[dest].name));
         CKeyID keyID;
-        if (pwalletMain && address.GetKeyID(keyID) && pwalletMain->mapKeyMetadata.count(keyID) && !pwalletMain->mapKeyMetadata[keyID].hdKeypath.empty())
-        {
-            ret.push_back(Pair("hdkeypath", pwalletMain->mapKeyMetadata[keyID].hdKeypath));
-            ret.push_back(Pair("hdmasterkeyid", pwalletMain->mapKeyMetadata[keyID].hdMasterKeyID.GetHex()));
+        if (pwalletMain) {
+            const auto& meta = pwalletMain->mapKeyMetadata;
+            auto it = address.GetKeyID(keyID) ? meta.find(keyID) : meta.end();
+            if (it != meta.end()) {
+                ret.push_back(Pair("timestamp", it->second.nCreateTime));
+                if (!it->second.hdKeypath.empty()) {
+                    ret.push_back(Pair("hdkeypath", it->second.hdKeypath));
+                    ret.push_back(Pair("hdmasterkeyid", it->second.hdMasterKeyID.GetHex()));
+                }
+            }
         }
 #endif
     }

From 266a8114cbe2a87a6c84d7690a7716a18d782c56 Mon Sep 17 00:00:00 2001
From: Russell Yanofsky <russ@yanofsky.org>
Date: Tue, 7 Feb 2017 11:08:08 -0500
Subject: [PATCH 3/3] Use MTP for importmulti "now" timestamps

---
 qa/rpc-tests/importmulti.py | 2 +-
 src/wallet/rpcdump.cpp      | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/qa/rpc-tests/importmulti.py b/qa/rpc-tests/importmulti.py
index b9874ccb8..52e40d6c1 100755
--- a/qa/rpc-tests/importmulti.py
+++ b/qa/rpc-tests/importmulti.py
@@ -139,7 +139,7 @@ class ImportMultiTest (BitcoinTestFramework):
         # Address + Private key + !watchonly
         print("Should import an address with private key")
         address = self.nodes[0].validateaddress(self.nodes[0].getnewaddress())
-        timestamp = self.nodes[1].getblock(self.nodes[1].getbestblockhash())['time']
+        timestamp = self.nodes[1].getblock(self.nodes[1].getbestblockhash())['mediantime']
         result = self.nodes[1].importmulti([{
             "scriptPubKey": {
                 "address": address['address']
diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp
index 4674c95bf..9310a320c 100644
--- a/src/wallet/rpcdump.cpp
+++ b/src/wallet/rpcdump.cpp
@@ -1034,7 +1034,7 @@ UniValue importmulti(const JSONRPCRequest& mainRequest)
     EnsureWalletIsUnlocked();
 
     // Verify all timestamps are present before importing any keys.
-    const int64_t now = chainActive.Tip() ? chainActive.Tip()->GetBlockTime() : 0;
+    const int64_t now = chainActive.Tip() ? chainActive.Tip()->GetMedianTimePast() : 0;
     for (const UniValue& data : requests.getValues()) {
         GetImportTimestamp(data, now);
     }