From a80f98b1c7a49432dc53d18d0fb51ac334de96be Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Tue, 8 Nov 2016 16:55:02 -0500 Subject: [PATCH] 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. --- qa/rpc-tests/importmulti.py | 33 ++++++++++++++++++++++++++++++++- src/rpc/misc.cpp | 3 +++ src/wallet/rpcdump.cpp | 20 +++++++++++--------- src/wallet/wallet.cpp | 27 ++++++++++++++++++--------- src/wallet/wallet.h | 21 +++++++++++++++++---- src/wallet/walletdb.cpp | 36 +++++++++++++++++++++++++----------- src/wallet/walletdb.h | 2 +- 7 files changed, 107 insertions(+), 35 deletions(-) diff --git a/qa/rpc-tests/importmulti.py b/qa/rpc-tests/importmulti.py index 52e40d6c1..02a932e73 100755 --- a/qa/rpc-tests/importmulti.py +++ b/qa/rpc-tests/importmulti.py @@ -20,6 +20,7 @@ class ImportMultiTest (BitcoinTestFramework): print ("Mining blocks...") self.nodes[0].generate(1) self.nodes[1].generate(1) + timestamp = self.nodes[1].getblock(self.nodes[1].getbestblockhash())['mediantime'] # keyword definition PRIV_KEY = 'privkey' @@ -59,6 +60,9 @@ class ImportMultiTest (BitcoinTestFramework): address_assert = self.nodes[1].validateaddress(address['address']) assert_equal(address_assert['iswatchonly'], True) assert_equal(address_assert['ismine'], False) + assert_equal(address_assert['timestamp'], timestamp) + watchonly_address = address['address'] + watchonly_timestamp = timestamp # ScriptPubKey + internal @@ -73,6 +77,7 @@ class ImportMultiTest (BitcoinTestFramework): address_assert = self.nodes[1].validateaddress(address['address']) assert_equal(address_assert['iswatchonly'], True) assert_equal(address_assert['ismine'], False) + assert_equal(address_assert['timestamp'], timestamp) # ScriptPubKey + !internal print("Should not import a scriptPubKey without internal flag") @@ -87,6 +92,7 @@ class ImportMultiTest (BitcoinTestFramework): address_assert = self.nodes[1].validateaddress(address['address']) assert_equal(address_assert['iswatchonly'], False) assert_equal(address_assert['ismine'], False) + assert_equal('timestamp' in address_assert, False) # Address + Public key + !Internal @@ -103,6 +109,7 @@ class ImportMultiTest (BitcoinTestFramework): address_assert = self.nodes[1].validateaddress(address['address']) assert_equal(address_assert['iswatchonly'], True) assert_equal(address_assert['ismine'], False) + assert_equal(address_assert['timestamp'], timestamp) # ScriptPubKey + Public key + internal @@ -119,6 +126,7 @@ class ImportMultiTest (BitcoinTestFramework): address_assert = self.nodes[1].validateaddress(address['address']) assert_equal(address_assert['iswatchonly'], True) assert_equal(address_assert['ismine'], False) + assert_equal(address_assert['timestamp'], timestamp) # ScriptPubKey + Public key + !internal print("Should not import a scriptPubKey without internal and with public key") @@ -135,11 +143,11 @@ class ImportMultiTest (BitcoinTestFramework): address_assert = self.nodes[1].validateaddress(address['address']) assert_equal(address_assert['iswatchonly'], False) assert_equal(address_assert['ismine'], False) + assert_equal('timestamp' in address_assert, False) # 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())['mediantime'] result = self.nodes[1].importmulti([{ "scriptPubKey": { "address": address['address'] @@ -170,6 +178,7 @@ class ImportMultiTest (BitcoinTestFramework): address_assert = self.nodes[1].validateaddress(address['address']) assert_equal(address_assert['iswatchonly'], False) assert_equal(address_assert['ismine'], False) + assert_equal('timestamp' in address_assert, False) # ScriptPubKey + Private key + internal print("Should import a scriptPubKey with internal and with private key") @@ -184,6 +193,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) # ScriptPubKey + Private key + !internal print("Should not import a scriptPubKey without internal and with private key") @@ -199,6 +209,7 @@ class ImportMultiTest (BitcoinTestFramework): address_assert = self.nodes[1].validateaddress(address['address']) assert_equal(address_assert['iswatchonly'], False) assert_equal(address_assert['ismine'], False) + assert_equal('timestamp' in address_assert, False) # P2SH address @@ -209,6 +220,7 @@ class ImportMultiTest (BitcoinTestFramework): self.nodes[1].generate(100) transactionid = self.nodes[1].sendtoaddress(multi_sig_script['address'], 10.00) self.nodes[1].generate(1) + timestamp = self.nodes[1].getblock(self.nodes[1].getbestblockhash())['mediantime'] transaction = self.nodes[1].gettransaction(transactionid) print("Should import a p2sh") @@ -222,6 +234,7 @@ class ImportMultiTest (BitcoinTestFramework): address_assert = self.nodes[1].validateaddress(multi_sig_script['address']) assert_equal(address_assert['isscript'], True) assert_equal(address_assert['iswatchonly'], True) + assert_equal(address_assert['timestamp'], timestamp) p2shunspent = self.nodes[1].listunspent(0,999999, [multi_sig_script['address']])[0] assert_equal(p2shunspent['spendable'], False) assert_equal(p2shunspent['solvable'], False) @@ -235,6 +248,7 @@ class ImportMultiTest (BitcoinTestFramework): self.nodes[1].generate(100) transactionid = self.nodes[1].sendtoaddress(multi_sig_script['address'], 10.00) self.nodes[1].generate(1) + timestamp = self.nodes[1].getblock(self.nodes[1].getbestblockhash())['mediantime'] transaction = self.nodes[1].gettransaction(transactionid) print("Should import a p2sh with respective redeem script") @@ -246,6 +260,8 @@ class ImportMultiTest (BitcoinTestFramework): "redeemscript": multi_sig_script['redeemScript'] }]) assert_equal(result[0]['success'], True) + address_assert = self.nodes[1].validateaddress(multi_sig_script['address']) + assert_equal(address_assert['timestamp'], timestamp) p2shunspent = self.nodes[1].listunspent(0,999999, [multi_sig_script['address']])[0] assert_equal(p2shunspent['spendable'], False) @@ -260,6 +276,7 @@ class ImportMultiTest (BitcoinTestFramework): self.nodes[1].generate(100) transactionid = self.nodes[1].sendtoaddress(multi_sig_script['address'], 10.00) self.nodes[1].generate(1) + timestamp = self.nodes[1].getblock(self.nodes[1].getbestblockhash())['mediantime'] transaction = self.nodes[1].gettransaction(transactionid) print("Should import a p2sh with respective redeem script and private keys") @@ -272,6 +289,8 @@ class ImportMultiTest (BitcoinTestFramework): "keys": [ self.nodes[0].dumpprivkey(sig_address_1['address']), self.nodes[0].dumpprivkey(sig_address_2['address'])] }]) assert_equal(result[0]['success'], True) + address_assert = self.nodes[1].validateaddress(multi_sig_script['address']) + assert_equal(address_assert['timestamp'], timestamp) p2shunspent = self.nodes[1].listunspent(0,999999, [multi_sig_script['address']])[0] assert_equal(p2shunspent['spendable'], False) @@ -319,6 +338,7 @@ class ImportMultiTest (BitcoinTestFramework): address_assert = self.nodes[1].validateaddress(address['address']) assert_equal(address_assert['iswatchonly'], False) assert_equal(address_assert['ismine'], False) + assert_equal('timestamp' in address_assert, False) # ScriptPubKey + Public key + internal + Wrong pubkey @@ -338,6 +358,7 @@ class ImportMultiTest (BitcoinTestFramework): address_assert = self.nodes[1].validateaddress(address['address']) assert_equal(address_assert['iswatchonly'], False) assert_equal(address_assert['ismine'], False) + assert_equal('timestamp' in address_assert, False) # Address + Private key + !watchonly + Wrong private key @@ -357,6 +378,7 @@ class ImportMultiTest (BitcoinTestFramework): address_assert = self.nodes[1].validateaddress(address['address']) assert_equal(address_assert['iswatchonly'], False) assert_equal(address_assert['ismine'], False) + assert_equal('timestamp' in address_assert, False) # ScriptPubKey + Private key + internal + Wrong private key @@ -375,6 +397,15 @@ class ImportMultiTest (BitcoinTestFramework): address_assert = self.nodes[1].validateaddress(address['address']) assert_equal(address_assert['iswatchonly'], False) assert_equal(address_assert['ismine'], False) + assert_equal('timestamp' in address_assert, False) + + # restart nodes to check for proper serialization/deserialization of watch only address + stop_nodes(self.nodes) + self.nodes = start_nodes(2, self.options.tmpdir) + address_assert = self.nodes[1].validateaddress(watchonly_address) + assert_equal(address_assert['iswatchonly'], True) + assert_equal(address_assert['ismine'], False) + assert_equal(address_assert['timestamp'], watchonly_timestamp); # Bad or missing timestamps print("Should throw on invalid or missing timestamp values") diff --git a/src/rpc/misc.cpp b/src/rpc/misc.cpp index 25fad3c2e..6fd50127b 100644 --- a/src/rpc/misc.cpp +++ b/src/rpc/misc.cpp @@ -208,6 +208,9 @@ UniValue validateaddress(const JSONRPCRequest& request) if (pwalletMain) { const auto& meta = pwalletMain->mapKeyMetadata; auto it = address.GetKeyID(keyID) ? meta.find(keyID) : meta.end(); + if (it == meta.end()) { + it = meta.find(CScriptID(scriptPubKey)); + } if (it != meta.end()) { ret.push_back(Pair("timestamp", it->second.nCreateTime)); if (!it->second.hdKeypath.empty()) { diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp index f358d720c..0a3225937 100644 --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -161,7 +161,7 @@ void ImportScript(const CScript& script, const string& strLabel, bool isRedeemSc pwalletMain->MarkDirty(); - if (!pwalletMain->HaveWatchOnly(script) && !pwalletMain->AddWatchOnly(script)) + if (!pwalletMain->HaveWatchOnly(script) && !pwalletMain->AddWatchOnly(script, 0 /* nCreateTime */)) throw JSONRPCError(RPC_WALLET_ERROR, "Error adding address to wallet"); if (isRedeemScript) { @@ -575,15 +575,17 @@ UniValue dumpwallet(const JSONRPCRequest& request) if (!file.is_open()) throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot open wallet dump file"); - std::map mapKeyBirth; + std::map mapKeyBirth; std::set setKeyPool; pwalletMain->GetKeyBirthTimes(mapKeyBirth); pwalletMain->GetAllReserveKeys(setKeyPool); // sort time/key pairs std::vector > vKeyBirth; - for (std::map::const_iterator it = mapKeyBirth.begin(); it != mapKeyBirth.end(); it++) { - vKeyBirth.push_back(std::make_pair(it->second, it->first)); + for (const auto& entry : mapKeyBirth) { + if (const CKeyID* keyID = boost::get(&entry.first)) { // set and test + vKeyBirth.push_back(std::make_pair(entry.second, *keyID)); + } } mapKeyBirth.clear(); std::sort(vKeyBirth.begin(), vKeyBirth.end()); @@ -720,7 +722,7 @@ UniValue ProcessImport(const UniValue& data, const int64_t timestamp) pwalletMain->MarkDirty(); - if (!pwalletMain->HaveWatchOnly(redeemScript) && !pwalletMain->AddWatchOnly(redeemScript)) { + if (!pwalletMain->HaveWatchOnly(redeemScript) && !pwalletMain->AddWatchOnly(redeemScript, timestamp)) { throw JSONRPCError(RPC_WALLET_ERROR, "Error adding address to wallet"); } @@ -737,7 +739,7 @@ UniValue ProcessImport(const UniValue& data, const int64_t timestamp) pwalletMain->MarkDirty(); - if (!pwalletMain->HaveWatchOnly(redeemDestination) && !pwalletMain->AddWatchOnly(redeemDestination)) { + if (!pwalletMain->HaveWatchOnly(redeemDestination) && !pwalletMain->AddWatchOnly(redeemDestination, timestamp)) { throw JSONRPCError(RPC_WALLET_ERROR, "Error adding address to wallet"); } @@ -830,7 +832,7 @@ UniValue ProcessImport(const UniValue& data, const int64_t timestamp) pwalletMain->MarkDirty(); - if (!pwalletMain->HaveWatchOnly(pubKeyScript) && !pwalletMain->AddWatchOnly(pubKeyScript)) { + if (!pwalletMain->HaveWatchOnly(pubKeyScript) && !pwalletMain->AddWatchOnly(pubKeyScript, timestamp)) { throw JSONRPCError(RPC_WALLET_ERROR, "Error adding address to wallet"); } @@ -848,7 +850,7 @@ UniValue ProcessImport(const UniValue& data, const int64_t timestamp) pwalletMain->MarkDirty(); - if (!pwalletMain->HaveWatchOnly(scriptRawPubKey) && !pwalletMain->AddWatchOnly(scriptRawPubKey)) { + if (!pwalletMain->HaveWatchOnly(scriptRawPubKey) && !pwalletMain->AddWatchOnly(scriptRawPubKey, timestamp)) { throw JSONRPCError(RPC_WALLET_ERROR, "Error adding address to wallet"); } @@ -922,7 +924,7 @@ UniValue ProcessImport(const UniValue& data, const int64_t timestamp) pwalletMain->MarkDirty(); - if (!pwalletMain->HaveWatchOnly(script) && !pwalletMain->AddWatchOnly(script)) { + if (!pwalletMain->HaveWatchOnly(script) && !pwalletMain->AddWatchOnly(script, timestamp)) { throw JSONRPCError(RPC_WALLET_ERROR, "Error adding address to wallet"); } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 192af22cb..f8f5a9306 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -206,11 +206,11 @@ bool CWallet::AddCryptedKey(const CPubKey &vchPubKey, return false; } -bool CWallet::LoadKeyMetadata(const CPubKey &pubkey, const CKeyMetadata &meta) +bool CWallet::LoadKeyMetadata(const CTxDestination& keyID, const CKeyMetadata &meta) { AssertLockHeld(cs_wallet); // mapKeyMetadata UpdateTimeFirstKey(meta.nCreateTime); - mapKeyMetadata[pubkey.GetID()] = meta; + mapKeyMetadata[keyID] = meta; return true; } @@ -256,15 +256,22 @@ bool CWallet::LoadCScript(const CScript& redeemScript) return CCryptoKeyStore::AddCScript(redeemScript); } -bool CWallet::AddWatchOnly(const CScript &dest) +bool CWallet::AddWatchOnly(const CScript& dest) { if (!CCryptoKeyStore::AddWatchOnly(dest)) return false; - nTimeFirstKey = 1; // No birthday information for watch-only keys. + const CKeyMetadata& meta = mapKeyMetadata[CScriptID(dest)]; + UpdateTimeFirstKey(meta.nCreateTime); NotifyWatchonlyChanged(true); if (!fFileBacked) return true; - return CWalletDB(strWalletFile).WriteWatchOnly(dest); + return CWalletDB(strWalletFile).WriteWatchOnly(dest, meta); +} + +bool CWallet::AddWatchOnly(const CScript& dest, int64_t nCreateTime) +{ + mapKeyMetadata[CScriptID(dest)].nCreateTime = nCreateTime; + return AddWatchOnly(dest); } bool CWallet::RemoveWatchOnly(const CScript &dest) @@ -3425,14 +3432,16 @@ public: void operator()(const CNoDestination &none) {} }; -void CWallet::GetKeyBirthTimes(std::map &mapKeyBirth) const { +void CWallet::GetKeyBirthTimes(std::map &mapKeyBirth) const { AssertLockHeld(cs_wallet); // mapKeyMetadata mapKeyBirth.clear(); // get birth times for keys with metadata - for (std::map::const_iterator it = mapKeyMetadata.begin(); it != mapKeyMetadata.end(); it++) - if (it->second.nCreateTime) - mapKeyBirth[it->first] = it->second.nCreateTime; + for (const auto& entry : mapKeyMetadata) { + if (entry.second.nCreateTime) { + mapKeyBirth[entry.first] = entry.second.nCreateTime; + } + } // map in which we'll infer heights of other keys CBlockIndex *pindexMax = chainActive[std::max(0, chainActive.Height() - 144)]; // the tip can be reorganized; use a 144-block safety margin diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 8d3ee9ddd..990c3bdf4 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -614,6 +614,17 @@ private: int64_t nTimeFirstKey; + /** + * Private version of AddWatchOnly method which does not accept a + * timestamp, and which will reset the wallet's nTimeFirstKey value to 1 if + * the watch key did not previously have a timestamp associated with it. + * 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. + */ + bool AddWatchOnly(const CScript& dest) override; + public: /* * Main wallet lock. @@ -638,7 +649,9 @@ public: mapKeyMetadata[keyid] = CKeyMetadata(keypool.nTime); } - std::map mapKeyMetadata; + // Map from Key ID (for regular keys) or Script ID (for watch-only keys) to + // key metadata. + std::map mapKeyMetadata; typedef std::map MasterKeyMap; MasterKeyMap mapMasterKeys; @@ -728,7 +741,7 @@ public: //! Adds a key to the store, without saving it to disk (used by LoadWallet) bool LoadKey(const CKey& key, const CPubKey &pubkey) { return CCryptoKeyStore::AddKeyPubKey(key, pubkey); } //! Load metadata (used by LoadWallet) - bool LoadKeyMetadata(const CPubKey &pubkey, const CKeyMetadata &metadata); + bool LoadKeyMetadata(const CTxDestination& pubKey, const CKeyMetadata &metadata); bool LoadMinVersion(int nVersion) { AssertLockHeld(cs_wallet); nWalletVersion = nVersion; nWalletMaxVersion = std::max(nWalletMaxVersion, nVersion); return true; } void UpdateTimeFirstKey(int64_t nCreateTime); @@ -750,7 +763,7 @@ public: bool GetDestData(const CTxDestination &dest, const std::string &key, std::string *value) const; //! Adds a watch-only address to the store, and saves it to disk. - bool AddWatchOnly(const CScript &dest); + bool AddWatchOnly(const CScript& dest, int64_t nCreateTime); bool RemoveWatchOnly(const CScript &dest); //! Adds a watch-only address to the store, without saving it to disk (used by LoadWallet) bool LoadWatchOnly(const CScript &dest); @@ -759,7 +772,7 @@ public: bool ChangeWalletPassphrase(const SecureString& strOldWalletPassphrase, const SecureString& strNewWalletPassphrase); bool EncryptWallet(const SecureString& strWalletPassphrase); - void GetKeyBirthTimes(std::map &mapKeyBirth) const; + void GetKeyBirthTimes(std::map &mapKeyBirth) const; /** * Increment the next transaction order id diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index 15e1e4c45..106a59d56 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -120,15 +120,19 @@ bool CWalletDB::WriteCScript(const uint160& hash, const CScript& redeemScript) return Write(std::make_pair(std::string("cscript"), hash), *(const CScriptBase*)(&redeemScript), false); } -bool CWalletDB::WriteWatchOnly(const CScript &dest) +bool CWalletDB::WriteWatchOnly(const CScript &dest, const CKeyMetadata& keyMeta) { nWalletDBUpdateCounter++; + if (!Write(std::make_pair(std::string("watchmeta"), *(const CScriptBase*)(&dest)), keyMeta)) + return false; return Write(std::make_pair(std::string("watchs"), *(const CScriptBase*)(&dest)), '1'); } bool CWalletDB::EraseWatchOnly(const CScript &dest) { nWalletDBUpdateCounter++; + if (!Erase(std::make_pair(std::string("watchmeta"), *(const CScriptBase*)(&dest)))) + return false; return Erase(std::make_pair(std::string("watchs"), *(const CScriptBase*)(&dest))); } @@ -259,6 +263,7 @@ class CWalletScanState { public: unsigned int nKeys; unsigned int nCKeys; + unsigned int nWatchKeys; unsigned int nKeyMeta; bool fIsEncrypted; bool fAnyUnordered; @@ -266,7 +271,7 @@ public: vector vWalletUpgrade; CWalletScanState() { - nKeys = nCKeys = nKeyMeta = 0; + nKeys = nCKeys = nWatchKeys = nKeyMeta = 0; fIsEncrypted = false; fAnyUnordered = false; nFileVersion = 0; @@ -348,16 +353,13 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue, } else if (strType == "watchs") { + wss.nWatchKeys++; CScript script; ssKey >> *(CScriptBase*)(&script); char fYes; ssValue >> fYes; if (fYes == '1') pwallet->LoadWatchOnly(script); - - // Watch-only addresses have no birthday information for now, - // so set the wallet birthday to the beginning of time. - pwallet->UpdateTimeFirstKey(1); } else if (strType == "key" || strType == "wkey") { @@ -458,15 +460,27 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue, } wss.fIsEncrypted = true; } - else if (strType == "keymeta") + else if (strType == "keymeta" || strType == "watchmeta") { - CPubKey vchPubKey; - ssKey >> vchPubKey; + CTxDestination keyID; + if (strType == "keymeta") + { + CPubKey vchPubKey; + ssKey >> vchPubKey; + keyID = vchPubKey.GetID(); + } + else if (strType == "watchmeta") + { + CScript script; + ssKey >> *(CScriptBase*)(&script); + keyID = CScriptID(script); + } + CKeyMetadata keyMeta; ssValue >> keyMeta; wss.nKeyMeta++; - pwallet->LoadKeyMetadata(vchPubKey, keyMeta); + pwallet->LoadKeyMetadata(keyID, keyMeta); } else if (strType == "defaultkey") { @@ -620,7 +634,7 @@ DBErrors CWalletDB::LoadWallet(CWallet* pwallet) wss.nKeys, wss.nCKeys, wss.nKeyMeta, wss.nKeys + wss.nCKeys); // nTimeFirstKey is only reliable if all keys have metadata - if ((wss.nKeys + wss.nCKeys) != wss.nKeyMeta) + if ((wss.nKeys + wss.nCKeys + wss.nWatchKeys) != wss.nKeyMeta) pwallet->UpdateTimeFirstKey(1); BOOST_FOREACH(uint256 hash, wss.vWalletUpgrade) diff --git a/src/wallet/walletdb.h b/src/wallet/walletdb.h index 8437a95ba..c7c65465d 100644 --- a/src/wallet/walletdb.h +++ b/src/wallet/walletdb.h @@ -135,7 +135,7 @@ public: bool WriteCScript(const uint160& hash, const CScript& redeemScript); - bool WriteWatchOnly(const CScript &script); + bool WriteWatchOnly(const CScript &script, const CKeyMetadata &keymeta); bool EraseWatchOnly(const CScript &script); bool WriteBestBlock(const CBlockLocator& locator);