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

ryanofsky · 2016-11-08T22:05:25Z

(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 · 2016-11-09T05:37:19Z

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

.........
fundrawtransaction.py:
Initializing test directory /tmp/testz6xi8e_p/637
start_node: bitcoind started, waiting for RPC to come up
start_node: RPC succesfully started
start_node: bitcoind started, waiting for RPC to come up
start_node: RPC succesfully started
start_node: bitcoind started, waiting for RPC to come up
start_node: RPC succesfully started
start_node: bitcoind started, waiting for RPC to come up
start_node: RPC succesfully started
Mining blocks...
start_node: bitcoind started, waiting for RPC to come up
start_node: RPC succesfully started
start_node: bitcoind started, waiting for RPC to come up
start_node: RPC succesfully started
start_node: bitcoind started, waiting for RPC to come up
start_node: RPC succesfully started
start_node: bitcoind started, waiting for RPC to come up
start_node: RPC succesfully started
Stopping nodes
Cleaning up
Tests successful
stderr:
Warning: Error reading wallet.dat! All keys read correctly, but transaction data or address book entries might be missing or incorrect.
Pass: False, Duration: 41 s

ryanofsky · 2016-11-10T19:54:21Z

Fixed fundrawtransaction test, and added importmulti tests.

jonasschnelli · 2017-01-01T10:13:42Z

Concept ACK.
Plans to test this soon.

gmaxwell · 2017-01-19T20:28:18Z

utACK.

jonasschnelli · 2017-01-20T09:43:20Z

src/wallet/wallet.cpp

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

+void CWallet::UpdateTimeFirstKey(int nCreateTime)


shouldn't this be int64_t nCreateTime.

Good catch, fixed in 6891d5e.

jonasschnelli · 2017-01-20T09:45:21Z

src/wallet/walletdb.cpp

        {
-            CPubKey vchPubKey;
-            ssKey >> vchPubKey;
+            uint160 keyId;


Is there a reason for not using CKeyID here?

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

jonasschnelli · 2017-01-20T09:46:04Z

src/wallet/wallet.cpp

@@ -3247,7 +3262,7 @@ void CWallet::GetKeyBirthTimes(std::map<CKeyID, int64_t> &mapKeyBirth) const {
    mapKeyBirth.clear();

    // get birth times for keys with metadata
-    for (std::map<CKeyID, CKeyMetadata>::const_iterator it = mapKeyMetadata.begin(); it != mapKeyMetadata.end(); it++)
+    for (std::map<uint160, CKeyMetadata>::const_iterator it = mapKeyMetadata.begin(); it != mapKeyMetadata.end(); it++)


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

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

morcos · 2017-01-23T21:20:26Z

utACK 6891d5e

jtimon · 2017-01-23T23:34:07Z

Concept ACK

TheBlueMatt · 2017-01-25T18:31:06Z

src/rpc/misc.cpp

        {
-            ret.push_back(Pair("hdkeypath", pwalletMain->mapKeyMetadata[keyID].hdKeypath));
-            ret.push_back(Pair("hdmasterkeyid", pwalletMain->mapKeyMetadata[keyID].hdMasterKeyID.GetHex()));
+            auto it = pwalletMain->mapKeyMetadata.find(CScriptID(scriptPubKey));


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.

Changed lookup order in 1ea9543. 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.

TheBlueMatt · 2017-01-25T19:01:38Z

src/wallet/wallet.cpp

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


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

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

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

Thanks, added the two missing timestamp arguments in f87008a, 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 1b99696. 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.

TheBlueMatt · 2017-01-25T19:04:01Z

src/wallet/walletdb.cpp

    return Write(std::make_pair(std::string("watchs"), *(const CScriptBase*)(&dest)), '1');
 }

 bool CWalletDB::EraseWatchOnly(const CScript &dest)
 {
    nWalletDBUpdated++;
+    if (!Erase(std::make_pair(std::string("watchmeta"), *(const CScriptBase*)(&dest))))


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

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.

TheBlueMatt · 2017-01-25T19:11:24Z

src/wallet/walletdb.cpp

 {
    nWalletDBUpdated++;
+    if (!Write(std::make_pair(std::string("watchmeta"), *(const CScriptBase*)(&dest)),
+               keyMeta, false))


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

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

jtimon · 2017-02-01T01:56:19Z

src/wallet/walletdb.cpp

@@ -463,11 +463,6 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
            wss.nKeyMeta++;

            pwallet->LoadKeyMetadata(vchPubKey, keyMeta);
-


Missing this here?

pwallet->UpdateTimeFirstKey(keyMeta.nCreateTime);

EDIT: no, LoadKeyMetadata calls it.

jtimon · 2017-02-01T02:02:43Z

utACK individual commits: 99c1aa1 6891d5e

Can we squash already?

ryanofsky · 2017-02-01T16:27:08Z

Squashed 6891d5e -> 89011a3, and will look into Matt's comments.

Change validateaddress RPC back to preferring regular key metadata instead of watch-only key metadata as suggested by Matt Corallo <git@bluematt.me> in bitcoin#9108 (comment)

Don't use fOverwrite=false option when writing watchonly key metadata. Noticed by Matt Corallo <git@bluematt.me> in bitcoin#9108 (comment)

Fix two AddWatchOnly calls that were missing timestamps noticed by Matt Corallo <git@bluematt.me> in bitcoin#9108 (comment). Also expand tests to check that these and other timestamps are recorded.

ryanofsky · 2017-02-02T19:18:07Z

Squashed 1b99696 -> 3d6dbed (watchtime.3 -> watchtime.4)

ryanofsky · 2017-02-02T19:39:57Z

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

TheBlueMatt · 2017-02-03T21:45:16Z

src/wallet/wallet.h

+     * Because this is an inherited virtual method, it is accessible despite
+     * being marked private, but it is marked private anyway to encourage use of
+     * the other AddWatchOnly which accepts a timestamp and sets nTimeFirstKey
+     * more intelligently for more efficient rescans.


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?

Added in 5dbd05d

TheBlueMatt · 2017-02-03T21:51:41Z

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

Expand comment as suggested by Matt Corallo <git@bluematt.me> in bitcoin#9108 (comment)

jonasschnelli · 2017-02-14T14:20:28Z

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

diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp
index 0a32259..401614c 100644
--- a/src/wallet/rpcdump.cpp
+++ b/src/wallet/rpcdump.cpp
@@ -922,9 +922,12 @@ UniValue ProcessImport(const UniValue& data, const int64_t timestamp)
                     throw JSONRPCError(RPC_WALLET_ERROR, "The wallet already contains the private key for this address or script");
                 }
 
-                pwalletMain->MarkDirty();
+                if (pwalletMain->HaveWatchOnly(script)) {
+                    throw JSONRPCError(RPC_WALLET_ERROR, "The wallet already contains this address or script");
+                }
 
-                if (!pwalletMain->HaveWatchOnly(script) && !pwalletMain->AddWatchOnly(script, timestamp)) {
+                pwalletMain->MarkDirty();
+                if (!pwalletMain->AddWatchOnly(script, timestamp)) {
                     throw JSONRPCError(RPC_WALLET_ERROR, "Error adding address to wallet");
                 }

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

ryanofsky · 2017-02-14T15:14:13Z

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 · 2017-02-14T16:12:59Z

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

morcos · 2017-02-14T17:48:05Z

re-utACK a80f98b

sipa · 2017-02-15T09:06:20Z

utACK a80f98b

… (on top of #9682) a80f98b Use importmulti timestamp when importing watch only keys (Russell Yanofsky) a58370e Dedup nTimeFirstKey update logic (Russell Yanofsky)

A new AssertLockHeld(cs_wallet) call was added in commit a58370e "Dedup nTimeFirstKey update logic" (part of PR bitcoin#9108). The lock held assertion will fail when loading prexisting wallets files from before the bitcoin#9108 merge that have watch-only keys.

Previously if an existing watch only key was reimported with a new timestamp, the new timestamp would not be saved in the key metadata, and would not be used to update the wallet nTimeFirstKey value (which could cause rescanning to start at the wrong point and miss transactions). Issue was pointed out by Jonas Schnelli <dev@jonasschnelli.ch> in bitcoin#9108 (comment)

Summary: Previously if an existing watch only key was reimported with a new timestamp, the new timestamp would not be saved in the key metadata, and would not be used to update the wallet nTimeFirstKey value (which could cause rescanning to start at the wrong point and miss transactions). Issue was pointed out by Jonas Schnelli <dev@jonasschnelli.ch> in bitcoin/bitcoin#9108 (comment) Backport core's PR9818 and PR11483 Test Plan: make check ./test/functional/test_runner.py importmulti Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D976

…ly keys (on top of bitcoin#9682) a80f98b Use importmulti timestamp when importing watch only keys (Russell Yanofsky) a58370e Dedup nTimeFirstKey update logic (Russell Yanofsky)

…itcoin#9682) bitcoin#9108 https://github.com/bitcoin/bitcoin/pull/9108/files

A new AssertLockHeld(cs_wallet) call was added in commit a58370e "Dedup nTimeFirstKey update logic" (part of PR bitcoin#9108). The lock held assertion will fail when loading prexisting wallets files from before the bitcoin#9108 merge that have watch-only keys.

Previously if an existing watch only key was reimported with a new timestamp, the new timestamp would not be saved in the key metadata, and would not be used to update the wallet nTimeFirstKey value (which could cause rescanning to start at the wrong point and miss transactions). Issue was pointed out by Jonas Schnelli <dev@jonasschnelli.ch> in bitcoin#9108 (comment) Conflicts: qa/rpc-tests/importmulti.py

A new AssertLockHeld(cs_wallet) call was added in commit a58370e "Dedup nTimeFirstKey update logic" (part of PR bitcoin#9108). The lock held assertion will fail when loading prexisting wallets files from before the bitcoin#9108 merge that have watch-only keys.

…ly keys (on top of bitcoin#9682) a80f98b Use importmulti timestamp when importing watch only keys (Russell Yanofsky) a58370e Dedup nTimeFirstKey update logic (Russell Yanofsky)

ryanofsky mentioned this pull request Nov 8, 2016

importmulti does not respect the given timestamp #9034

Closed

fanquake added Wallet RPC/REST/ZMQ labels Nov 9, 2016

ryanofsky force-pushed the watchtime branch 2 times, most recently from 6ec339c to 6a52c4d Compare November 10, 2016 19:53

ryanofsky changed the title ~~WIP: Use importmulti timestamp when importing watch only keys~~ Use importmulti timestamp when importing watch only keys Nov 10, 2016

laanwj added this to the 0.14.0 milestone Jan 19, 2017

jonasschnelli suggested changes Jan 20, 2017

View reviewed changes

TheBlueMatt reviewed Jan 25, 2017

View reviewed changes

jtimon reviewed Feb 1, 2017

View reviewed changes

ryanofsky force-pushed the watchtime branch from 6891d5e to 89011a3 Compare February 1, 2017 16:26

ryanofsky force-pushed the watchtime branch from 1b99696 to 3d6dbed Compare February 2, 2017 19:16

ryanofsky force-pushed the watchtime branch from 3d6dbed to b78ec3c Compare February 2, 2017 19:39

TheBlueMatt reviewed Feb 3, 2017

View reviewed changes

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Feb 3, 2017

fixup! Use importmulti timestamp when importing watch only keys

5dbd05d

Expand comment as suggested by Matt Corallo <git@bluematt.me> in bitcoin#9108 (comment)

ryanofsky force-pushed the watchtime branch from 5dbd05d to b8fca20 Compare February 3, 2017 22:06

laanwj merged commit a80f98b into bitcoin:master Feb 15, 2017

ryanofsky mentioned this pull request Feb 15, 2017

Add missing cs_wallet lock that triggers new lock held assertion #9771

Merged

ryanofsky mentioned this pull request Feb 21, 2017

Save watch only key timestamps when reimporting keys #9818

Merged

ryanofsky mentioned this pull request Mar 1, 2017

Factor out CWallet::nTimeSmart computation into a method. #9369

Merged

HashUnlimited pushed a commit to chaincoin/chaincoin that referenced this pull request Feb 27, 2018

Use importmulti timestamp when importing watch only keys (on top of b…

c69f3be

…itcoin#9682) bitcoin#9108 https://github.com/bitcoin/bitcoin/pull/9108/files

jnewbery mentioned this pull request Jan 31, 2019

Remove wallet global nTimeFirstKey #15306

Closed

ryanofsky mentioned this pull request Jul 16, 2019

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

Merged

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

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

		@@ -463,11 +463,6 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
		wss.nKeyMeta++;

		pwallet->LoadKeyMetadata(vchPubKey, keyMeta);

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

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

Uh oh!

Conversation

ryanofsky commented Nov 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented Nov 9, 2016

Uh oh!

ryanofsky commented Nov 10, 2016

Uh oh!

jonasschnelli commented Jan 1, 2017

Uh oh!

gmaxwell commented Jan 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morcos commented Jan 23, 2017

Uh oh!

jtimon commented Jan 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtimon Feb 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtimon commented Feb 1, 2017

Uh oh!

ryanofsky commented Feb 1, 2017

Uh oh!

ryanofsky commented Feb 2, 2017

Uh oh!

ryanofsky commented Feb 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Feb 3, 2017

Uh oh!

jonasschnelli commented Feb 14, 2017

Uh oh!

ryanofsky commented Feb 14, 2017

Uh oh!

jonasschnelli commented Feb 14, 2017

Uh oh!

morcos commented Feb 14, 2017

Uh oh!

sipa commented Feb 15, 2017

Uh oh!

Uh oh!

ryanofsky commented Nov 8, 2016 •

edited

Loading

jtimon Feb 1, 2017 •

edited

Loading