Use importmulti timestamp when importing watch only keys (on top of #9682)

ryanofsky commented at 10:05 pm on November 8, 2016: member

(This is based on #9682 to avoid merge conflicts in importmulti.py. Only the two commits “Dedup nTimeFirstKey update logic” and “Use importmulti timestamp when importing watch only keys” pertain to this PR. The other three commits belong to #9682).

This fixes issue #9034.

When importing a watch-only address over importmulti with a specific timestamp, the wallet’s nTimeFirstKey is currently set to 1. After this change, the provided timestamp will be used and stored as metadata associated with watch-only key. This can improve wallet performance because it can avoid the need to scan the entire blockchain for watch only addresses when timestamps are provided.

fanquake added the label Wallet on Nov 9, 2016

fanquake added the label RPC/REST/ZMQ on Nov 9, 2016

fanquake commented at 5:37 am on November 9, 2016: member

The fundrawtransaction test failed on two linux builds, same error for both:

 0.........
 1fundrawtransaction.py:
 2Initializing test directory /tmp/testz6xi8e_p/637
 3start_node: bitcoind started, waiting for RPC to come up
 4start_node: RPC succesfully started
 5start_node: bitcoind started, waiting for RPC to come up
 6start_node: RPC succesfully started
 7start_node: bitcoind started, waiting for RPC to come up
 8start_node: RPC succesfully started
 9start_node: bitcoind started, waiting for RPC to come up
10start_node: RPC succesfully started
11Mining blocks...
12start_node: bitcoind started, waiting for RPC to come up
13start_node: RPC succesfully started
14start_node: bitcoind started, waiting for RPC to come up
15start_node: RPC succesfully started
16start_node: bitcoind started, waiting for RPC to come up
17start_node: RPC succesfully started
18start_node: bitcoind started, waiting for RPC to come up
19start_node: RPC succesfully started
20Stopping nodes
21Cleaning up
22Tests successful
23stderr:
24Warning: Error reading wallet.dat! All keys read correctly, but transaction data or address book entries might be missing or incorrect.
25Pass: False, Duration: 41 s

ryanofsky force-pushed on Nov 10, 2016

ryanofsky commented at 7:54 pm on November 10, 2016: member

Fixed fundrawtransaction test, and added importmulti tests.

ryanofsky renamed this:
~~WIP: Use importmulti timestamp when importing watch only keys~~
Use importmulti timestamp when importing watch only keys
on Nov 10, 2016

jonasschnelli commented at 10:13 am on January 1, 2017: contributor

Concept ACK. Plans to test this soon.

laanwj added this to the milestone 0.14.0 on Jan 19, 2017

gmaxwell commented at 8:28 pm on January 19, 2017: contributor

utACK.

in src/wallet/wallet.cpp: in 6a52c4de03 outdated

218@@ -222,6 +219,17 @@ bool CWallet::LoadCryptedKey(const CPubKey &vchPubKey, const std::vector<unsigne
219     return CCryptoKeyStore::AddCryptedKey(vchPubKey, vchCryptedSecret);
220 }
221 
222+void CWallet::UpdateTimeFirstKey(int nCreateTime)

jonasschnelli commented at 9:43 am on January 20, 2017:

shouldn’t this be int64_t nCreateTime.

ryanofsky commented at 7:05 pm on January 23, 2017:

Good catch, fixed in 6891d5e1f24e74561aeba45c4752756a760c281d.

in src/wallet/walletdb.cpp: in 6a52c4de03 outdated

456@@ -454,20 +457,27 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
457             }
458             wss.fIsEncrypted = true;
459         }
460-        else if (strType == "keymeta")
461+        else if (strType == "keymeta" || strType == "watchmeta")
462         {
463-            CPubKey vchPubKey;
464-            ssKey >> vchPubKey;
465+            uint160 keyId;

jonasschnelli commented at 9:45 am on January 20, 2017:

Is there a reason for not using CKeyID here?

ryanofsky commented at 6:45 pm on January 23, 2017:

Again this can hold CScriptID values, and is changed to CTxDestination in 777b3db229e162c524d49ab7b448421f51bfa5ca.

in src/wallet/wallet.cpp: in 6a52c4de03 outdated

3261@@ -3247,7 +3262,7 @@ void CWallet::GetKeyBirthTimes(std::map<CKeyID, int64_t> &mapKeyBirth) const {
3262     mapKeyBirth.clear();
3263 
3264     // get birth times for keys with metadata
3265-    for (std::map<CKeyID, CKeyMetadata>::const_iterator it = mapKeyMetadata.begin(); it != mapKeyMetadata.end(); it++)
3266+    for (std::map<uint160, CKeyMetadata>::const_iterator it = mapKeyMetadata.begin(); it != mapKeyMetadata.end(); it++)

jonasschnelli commented at 9:46 am on January 20, 2017:

I don’t see the reason for changing this from CKeyID to uint160?

ryanofsky commented at 5:41 pm on January 23, 2017:

Changed from uint160 to CTxDestination in 777b3db229e162c524d49ab7b448421f51bfa5ca. The map isn’t keyed by CKeyID anymore because it can also contain CScriptID entries for watch only keys.

jonasschnelli changes_requested

morcos commented at 9:20 pm on January 23, 2017: member

utACK 6891d5e

jtimon commented at 11:34 pm on January 23, 2017: contributor

Concept ACK

in src/rpc/misc.cpp: in 6891d5e1f2 outdated

204@@ -204,10 +205,20 @@ UniValue validateaddress(const JSONRPCRequest& request)
205         if (pwalletMain && pwalletMain->mapAddressBook.count(dest))
206             ret.push_back(Pair("account", pwalletMain->mapAddressBook[dest].name));
207         CKeyID keyID;
208-        if (pwalletMain && address.GetKeyID(keyID) && pwalletMain->mapKeyMetadata.count(keyID) && !pwalletMain->mapKeyMetadata[keyID].hdKeypath.empty())
209+        if (pwalletMain)
210         {
211-            ret.push_back(Pair("hdkeypath", pwalletMain->mapKeyMetadata[keyID].hdKeypath));
212-            ret.push_back(Pair("hdmasterkeyid", pwalletMain->mapKeyMetadata[keyID].hdMasterKeyID.GetHex()));
213+            auto it = pwalletMain->mapKeyMetadata.find(CScriptID(scriptPubKey));

TheBlueMatt commented at 6:31 pm on January 25, 2017:

Shouldn’t you try address.GetKeyID(keyID) first before you give up and use the CScriptID? Otherwise you end up using the P2SH of the P2PH.

ryanofsky commented at 4:29 pm on February 1, 2017:

Changed lookup order in 1ea9543ac25cd8976430beab2f913d4875767e8b. I didn’t understand what you mean by P2SH of the P2PH, but I think it does make sense to prefer regular key metadata over watchonly key metadata if both exist.

in src/wallet/wallet.cpp: in 6891d5e1f2 outdated

258@@ -251,11 +259,18 @@ bool CWallet::AddWatchOnly(const CScript &dest)
259 {

TheBlueMatt commented at 7:01 pm on January 25, 2017:

I’d significantly prefer to not expose this version, making it explicit to callers that the key’s nCreateTime is being set to 0 (which, after this PR, implies the wallet’s nTimeFirstKey is set to 1).

TheBlueMatt commented at 7:13 pm on January 25, 2017:

The second two calls to AddWatchOnly in processImport probably could take the timestamp argument.

TheBlueMatt commented at 4:04 pm on January 26, 2017:

Grr, meant for this to be on AddWatchOnly at line https://github.com/bitcoin/bitcoin/pull/9108/files#diff-b2bb174788c7409b671c46ccc86034bdL251

ryanofsky commented at 6:58 pm on February 2, 2017:

Thanks, added the two missing timestamp arguments in f87008a8b0edc677db6ec16036f7e9940a3c904d, and expanded the test to verify the fix.

I very much agree that it’s not good to expose the AddWatchOnly version not taking a timestamp, so I made it private in 1b9969682e59e082855179e4c50fb1dd38227d67. I also experimented with getting rid of the overload by adding the timestamp to AddWatchOnly methods in the keystore superclasses, but this was a bigger change and made the keystore and walletdb code more confusing, so I went with this simpler fix.

in src/wallet/walletdb.cpp: in 6891d5e1f2 outdated

127 }
128 
129 bool CWalletDB::EraseWatchOnly(const CScript &dest)
130 {
131     nWalletDBUpdated++;
132+    if (!Erase(std::make_pair(std::string("watchmeta"), *(const CScriptBase*)(&dest))))

TheBlueMatt commented at 7:04 pm on January 25, 2017:

I think we need to ignore this error - otherwise if a watchonly has been added in a previous version we will fail to erase here (as it might have no watchmeta)?

ryanofsky commented at 4:29 pm on February 1, 2017:

Having no watchmeta shouldn’t be a problem. Erase still returns true if the key is not found. It just false if there is a database error.

in src/wallet/walletdb.cpp: in 6891d5e1f2 outdated

115@@ -116,15 +116,20 @@ bool CWalletDB::WriteCScript(const uint160& hash, const CScript& redeemScript)
116     return Write(std::make_pair(std::string("cscript"), hash), *(const CScriptBase*)(&redeemScript), false);
117 }
118 
119-bool CWalletDB::WriteWatchOnly(const CScript &dest)
120+bool CWalletDB::WriteWatchOnly(const CScript &dest, const CKeyMetadata& keyMeta)
121 {
122     nWalletDBUpdated++;
123+    if (!Write(std::make_pair(std::string("watchmeta"), *(const CScriptBase*)(&dest)),
124+               keyMeta, false))

TheBlueMatt commented at 7:11 pm on January 25, 2017:

Maybe do overwrite here? Seems strange that we update in-wallet-state without updating on-disk state (and its probably a supported use-case to re-add something to wallet to set its date differently).

ryanofsky commented at 4:29 pm on February 1, 2017:

Enabled overwrite in 50474f1305693bf22a995f0841ee27dad545d01a. This was just a mistake copying the line of code from taken from CWalletDB::WriteKey.

in src/wallet/walletdb.cpp: in 99c1aa1695 outdated

462@@ -463,11 +463,6 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
463             wss.nKeyMeta++;
464 
465             pwallet->LoadKeyMetadata(vchPubKey, keyMeta);
466-

jtimon commented at 1:56 am on February 1, 2017:

Missing this here?

0pwallet->UpdateTimeFirstKey(keyMeta.nCreateTime);

EDIT: no, LoadKeyMetadata calls it.

jtimon commented at 2:02 am on February 1, 2017: contributor

utACK individual commits: 99c1aa169592a71c24e7a2475c50ad7598eafb65 6891d5e1f24e74561aeba45c4752756a760c281d

Can we squash already?

ryanofsky force-pushed on Feb 1, 2017

ryanofsky commented at 4:27 pm on February 1, 2017: member

Squashed 6891d5e1f24e74561aeba45c4752756a760c281d -> 89011a3032f635b6536a476bfa48502c5dcc7a65, and will look into Matt’s comments.

ryanofsky referenced this in commit 1ea9543ac2 on Feb 2, 2017

ryanofsky referenced this in commit 50474f1305 on Feb 2, 2017

ryanofsky referenced this in commit f87008a8b0 on Feb 2, 2017

ryanofsky force-pushed on Feb 2, 2017

ryanofsky commented at 7:18 pm on February 2, 2017: member

Squashed 1b9969682e59e082855179e4c50fb1dd38227d67 -> 3d6dbed2c3132d53650b86f09d42e339149d3210 (watchtime.3 -> watchtime.4)

ryanofsky force-pushed on Feb 2, 2017

ryanofsky commented at 7:39 pm on February 2, 2017: member

Latest changes conflicted (trivially) with #9607 and #9121, so rebased 3d6dbed2c3132d53650b86f09d42e339149d3210 -> b78ec3cc3dbf5e1fc2dc105c609574101e462ae8 (watchtime.4 -> watchtime.5)

in src/wallet/wallet.h: in b78ec3cc3d outdated

609+    /**
610+     * Private version of AddWatchOnly method which does not accept a timestamp.
611+     * Because this is an inherited virtual method, it is accessible despite
612+     * being marked private, but it is marked private anyway to encourage use of
613+     * the other AddWatchOnly which accepts a timestamp and sets nTimeFirstKey
614+     * more intelligently for more efficient rescans.

TheBlueMatt commented at 9:45 pm on February 3, 2017:

Ugh, I totally missed that its an override, thanks for catching that. Can you further document here that this method implies setting the first key time to 1?

ryanofsky commented at 10:04 pm on February 3, 2017:

Added in 5dbd05d8f7c4fc86d1296a200a00a834b61fb5c3

TheBlueMatt commented at 9:51 pm on February 3, 2017: member

utACK b78ec3cc3dbf5e1fc2dc105c609574101e462ae8 +/- wanting another comment, Didn’t look too closely at tests.

ryanofsky referenced this in commit 5dbd05d8f7 on Feb 3, 2017

ryanofsky force-pushed on Feb 3, 2017

ryanofsky commented at 10:08 pm on February 3, 2017: member

Added comment in 5dbd05d8f7c4fc86d1296a200a00a834b61fb5c3.

Squashed 5dbd05d8f7c4fc86d1296a200a00a834b61fb5c3 -> b8fca20399c3528e5c293509629907f6874575e3 (watchtime.6 -> watchtime.7)

ryanofsky force-pushed on Feb 6, 2017

ryanofsky commented at 6:11 pm on February 6, 2017: member

Rebased b8fca20399c3528e5c293509629907f6874575e3 -> be820d8ad761b87ca61427ce8b3005b0f6db0d06 (watchtime.7 -> watchtime.8) to fix conflict with #9227.

TheBlueMatt commented at 3:13 pm on February 7, 2017: member

be820d8ad761b87ca61427ce8b3005b0f6db0d06 appears to be equivalent to my previous utACK (b78ec3cc3dbf5e1fc2dc105c609574101e462ae8) plus a fixup for the comments I requested.

laanwj commented at 6:41 am on February 9, 2017: member

@jonasschnelli Looks like your suggestions have been carried out by @ryanofsky and the “changes requested” can be cancelled?

in src/rpc/misc.cpp: in be820d8ad7 outdated

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

kallewoof commented at 7:37 am on February 9, 2017:

Nit: JSON represents integer values without quotes, so probably " \"timestamp\" : int64 (number, optional) The crea..."

ryanofsky commented at 10:59 am on February 9, 2017:

Fixed in 6a56f6d77a4e0679ab88b354dea7143ec8539c4e

in src/wallet/rpcdump.cpp: in be820d8ad7 outdated

160@@ -161,7 +161,7 @@ void ImportScript(const CScript& script, const string& strLabel, bool isRedeemSc
161 
162     pwalletMain->MarkDirty();
163 
164-    if (!pwalletMain->HaveWatchOnly(script) && !pwalletMain->AddWatchOnly(script))
165+    if (!pwalletMain->HaveWatchOnly(script) && !pwalletMain->AddWatchOnly(script, 0 /* nCreateTime */))

kallewoof commented at 7:39 am on February 9, 2017:

Is there a reason why this is not simply defaulting to 0 in AddWatchOnly() which would leave this line untouched?

ryanofsky commented at 10:59 am on February 9, 2017:

There was a lot of discussion about this above. We think it is safer to require the timestamp.

in src/wallet/rpcdump.cpp: in be820d8ad7 outdated

583 
584     // sort time/key pairs
585     std::vector<std::pair<int64_t, CKeyID> > vKeyBirth;
586-    for (std::map<CKeyID, int64_t>::const_iterator it = mapKeyBirth.begin(); it != mapKeyBirth.end(); it++) {
587-        vKeyBirth.push_back(std::make_pair(it->second, it->first));
588+    for (auto it = mapKeyBirth.begin(); it != mapKeyBirth.end(); it++) {

kallewoof commented at 7:44 am on February 9, 2017:

Nit:

0-    for (auto it = mapKeyBirth.begin(); it != mapKeyBirth.end(); it++) {
1-        if (const CKeyID* keyID = boost::get<CKeyID>(&it->first)) {
2-            vKeyBirth.push_back(std::make_pair(it->second, *keyID));
3+    for (const auto& it : mapKeyBirth) {
4+        if (const CKeyID* keyID = boost::get<CKeyID>(&it.first)) {
5+            vKeyBirth.push_back(std::make_pair(it.second, *keyID));

ryanofsky commented at 11:00 am on February 9, 2017:

Fixed in 6a56f6d77a4e0679ab88b354dea7143ec8539c4e

in src/wallet/rpcdump.cpp: in be820d8ad7 outdated

584     // sort time/key pairs
585     std::vector<std::pair<int64_t, CKeyID> > vKeyBirth;
586-    for (std::map<CKeyID, int64_t>::const_iterator it = mapKeyBirth.begin(); it != mapKeyBirth.end(); it++) {
587-        vKeyBirth.push_back(std::make_pair(it->second, it->first));
588+    for (auto it = mapKeyBirth.begin(); it != mapKeyBirth.end(); it++) {
589+        if (const CKeyID* keyID = boost::get<CKeyID>(&it->first)) {

kallewoof commented at 7:45 am on February 9, 2017:

Nit: worthwhile to add // set and test or similar as the single = might be mistaken for == by someone reading the code.

ryanofsky commented at 11:00 am on February 9, 2017:

Fixed in 6a56f6d77a4e0679ab88b354dea7143ec8539c4e

in src/wallet/wallet.cpp: in be820d8ad7 outdated

3436     AssertLockHeld(cs_wallet); // mapKeyMetadata
3437     mapKeyBirth.clear();
3438 
3439     // get birth times for keys with metadata
3440-    for (std::map<CKeyID, CKeyMetadata>::const_iterator it = mapKeyMetadata.begin(); it != mapKeyMetadata.end(); it++)
3441+    for (auto it = mapKeyMetadata.begin(); it != mapKeyMetadata.end(); it++)

kallewoof commented at 7:49 am on February 9, 2017:

Nit: as above, can be made to look a bit nicer with for (const auto& it : mapKeyMetadata) and s/it->/it./.

ryanofsky commented at 11:00 am on February 9, 2017:

Fixed in 6a56f6d77a4e0679ab88b354dea7143ec8539c4e

in src/wallet/wallet.h: in be820d8ad7 outdated

762@@ -748,7 +763,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
763     bool GetDestData(const CTxDestination &dest, const std::string &key, std::string *value) const;
764 
765     //! Adds a watch-only address to the store, and saves it to disk.
766-    bool AddWatchOnly(const CScript &dest);
767+    bool AddWatchOnly(const CScript &dest, int64_t nCreateTime);

kallewoof commented at 7:50 am on February 9, 2017:

Nit: (see above) make 2nd argument optional, Nit 2: CScript& dest

0bool AddWatchOnly(const CScript& dest, int64_t nCreateTime = 0);

ryanofsky commented at 11:00 am on February 9, 2017:

Nit 2 fixed in b3ba65488d575899c6df2a98f9dc6447c93ae22d

in src/wallet/wallet.cpp: in be820d8ad7 outdated

218@@ -222,6 +219,17 @@ bool CWallet::LoadCryptedKey(const CPubKey &vchPubKey, const std::vector<unsigne
219     return CCryptoKeyStore::AddCryptedKey(vchPubKey, vchCryptedSecret);
220 }
221 
222+void CWallet::UpdateTimeFirstKey(int64_t nCreateTime)
223+{
224+    AssertLockHeld(cs_wallet);
225+    // Cannot determine birthday information, so set the wallet birthday to the
226+    // beginning of time.
227+    if (nCreateTime <= 1)

kallewoof commented at 8:07 am on February 9, 2017:

Maybe I’m misreading the code, but the comment Cannot determine birthday information seems to refer to the if case below it being true; I would prefer comments being inside the block in this case. I would probably also include brackets after doing so as the comment makes the single-line conditional state less obvious.

ryanofsky commented at 11:00 am on February 9, 2017:

Fixed in f95843a294dad786117bd40801c8bf40afcb8784

kallewoof approved

kallewoof commented at 8:09 am on February 9, 2017: member

Slight/weak ACK (some testing but not a lot)

ryanofsky referenced this in commit f95843a294 on Feb 9, 2017

ryanofsky referenced this in commit 6a56f6d77a on Feb 9, 2017

ryanofsky referenced this in commit b3ba65488d on Feb 9, 2017

ryanofsky force-pushed on Feb 9, 2017

ryanofsky commented at 11:35 am on February 9, 2017: member

Squashed b3ba65488d575899c6df2a98f9dc6447c93ae22d -> f35ce5172bb9c6bec6bb636ea349104b559d36de (watchtime.10 -> watchtime.11)

jonasschnelli approved

jonasschnelli commented at 12:15 pm on February 9, 2017: contributor

UtACK f35ce5172bb9c6bec6bb636ea349104b559d36de

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

442887f27f

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

3cf991756c

Use MTP for importmulti "now" timestamps 266a8114cb

Dedup nTimeFirstKey update logic

Also make nTimeFirstKey member variable private.

This is just a cleanup change, it doesn't change behavior in any significant
way.

a58370e6a2

Use importmulti timestamp when importing watch only keys

When importing a watch-only address over importmulti with a specific timestamp,
the wallet's nTimeFirstKey is currently set to 1. After this change, the
provided timestamp will be used and stored as metadata associated with
watch-only key. This can improve wallet performance because it can avoid the
need to scan the entire blockchain for watch only addresses when timestamps are
provided.

Also adds timestamp to validateaddress return value (needed for tests).

Fixes #9034.

a80f98b1c7

ryanofsky force-pushed on Feb 10, 2017

ryanofsky renamed this:
~~Use importmulti timestamp when importing watch only keys~~
Use importmulti timestamp when importing watch only keys (on top of #9682)
on Feb 10, 2017

ryanofsky commented at 9:18 pm on February 10, 2017: member

Rebased on top of #9682 to avoid merge conflicts in the importmulti.py test. No other changes.

f35ce5172bb9c6bec6bb636ea349104b559d36de -> a80f98b1c7a49432dc53d18d0fb51ac334de96be (watchtime.11 -> watchtime.12)

jonasschnelli commented at 2:06 pm on February 14, 2017: contributor

Not sure if this is a nit or the intended behaviour: If I re-import a watch only address (same address I have already imported), I get no notice that it was already present. ~~This may be okay.~~

Another option would be to reject the import with “already exists”. ~~But the current overwrite-behaviour allows to re-set the timestamp which can be handy.~~

jonasschnelli commented at 2:20 pm on February 14, 2017: contributor

Correction of my comment above. If you re-import a watch-only, it does not set the new timestamp but still response with “success”. I think we should do this (fail when reimport a watch-only)…

 0diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp
 1index 0a32259..401614c 100644
 2--- a/src/wallet/rpcdump.cpp
 3+++ b/src/wallet/rpcdump.cpp
 4@@ -922,9 +922,12 @@ UniValue ProcessImport(const UniValue& data, const int64_t timestamp)
 5                     throw JSONRPCError(RPC_WALLET_ERROR, "The wallet already contains the private key for this address or script");
 6                 }
 7 
 8-                pwalletMain->MarkDirty();
 9+                if (pwalletMain->HaveWatchOnly(script)) {
10+                    throw JSONRPCError(RPC_WALLET_ERROR, "The wallet already contains this address or script");
11+                }
12 
13-                if (!pwalletMain->HaveWatchOnly(script) && !pwalletMain->AddWatchOnly(script, timestamp)) {
14+                pwalletMain->MarkDirty();
15+                if (!pwalletMain->AddWatchOnly(script, timestamp)) {
16                     throw JSONRPCError(RPC_WALLET_ERROR, "Error adding address to wallet");
17                 }
18

An alternative would be to overwrite the timestamp with the new one.

ryanofsky commented at 3:14 pm on February 14, 2017: member

Ok, I can add logic to error on attempts to import watch only keys that have already imported. Note that your fix above is incomplete because it only covers importmulti, not importaddress, importpubkey, etc.

I’ll do it in a different PR though, unless you think it needs to be here, because this is longstanding behavior not really related to saving timestamps.

jonasschnelli commented at 4:12 pm on February 14, 2017: contributor

Tested ACK of a58370e6a2d4dce50eefbcab5bde9f14facef8fc a80f98b1c7a49432dc53d18d0fb51ac334de96be on top of master (e87ce95fbdc6ca6ef822c978d98b2acba5948ee1). I agree with @ryanofsky that the overwrite-/already-exists-behavior can be fixed later.

morcos commented at 5:48 pm on February 14, 2017: member

re-utACK a80f98b

sipa commented at 9:06 am on February 15, 2017: member

utACK a80f98b1c7a49432dc53d18d0fb51ac334de96be

laanwj merged this on Feb 15, 2017

laanwj closed this on Feb 15, 2017

laanwj referenced this in commit d8e8b06bd0 on Feb 15, 2017

ryanofsky referenced this in commit 0f25026b55 on Feb 15, 2017

ryanofsky referenced this in commit 07afcd6379 on Feb 15, 2017

ryanofsky referenced this in commit a496e16775 on Feb 21, 2017

ryanofsky referenced this in commit 7759aa23d1 on Mar 3, 2017

practicalswift referenced this in commit 03606b35e4 on Apr 27, 2017

deadalnix referenced this in commit fe14ea286e on Jan 20, 2018

codablock referenced this in commit e99b716e86 on Jan 31, 2018

codablock referenced this in commit 1186bd3519 on Jan 31, 2018

codablock referenced this in commit de17fd766b on Jan 31, 2018

codablock referenced this in commit 43f697866e on Feb 1, 2018

HashUnlimited referenced this in commit c69f3be35e on Feb 27, 2018

HashUnlimited referenced this in commit c22bb21e07 on Feb 27, 2018

HashUnlimited referenced this in commit 4d33acaaba on Feb 28, 2018

lateminer referenced this in commit ada5ec795c on Jan 4, 2019

andvgal referenced this in commit 165188b133 on Jan 6, 2019

CryptoCentric referenced this in commit b0a82fc804 on Feb 28, 2019

CryptoCentric referenced this in commit 45ba8c2f70 on Mar 2, 2019

DrahtBot locked this on Sep 8, 2021

Use importmulti timestamp when importing watch only keys (on top of #9682) #9108