Skip to content

Commit

Permalink
Use importmulti timestamp when importing watch only keys
Browse files Browse the repository at this point in the history 
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 bitcoin#9034.
  • Loading branch information
@ryanofsky
ryanofsky committed Feb 10, 2017
1 parent a58370e commit a80f98b
Show file tree
Hide file tree
Showing 7 changed files with 107 additions and 35 deletions.
33 changes: 32 additions & 1 deletion qa/rpc-tests/importmulti.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ def run_test (self):
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'
Expand Down Expand Up @@ -59,6 +60,9 @@ def run_test (self):
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
Expand All @@ -73,6 +77,7 @@ def run_test (self):
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")
Expand All @@ -87,6 +92,7 @@ def run_test (self):
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
Expand All @@ -103,6 +109,7 @@ def run_test (self):
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
Expand All @@ -119,6 +126,7 @@ def run_test (self):
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")
Expand All @@ -135,11 +143,11 @@ def run_test (self):
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']
Expand Down Expand Up @@ -170,6 +178,7 @@ def run_test (self):
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")
Expand All @@ -184,6 +193,7 @@ def run_test (self):
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")
Expand All @@ -199,6 +209,7 @@ def run_test (self):
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
Expand All @@ -209,6 +220,7 @@ def run_test (self):
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")
Expand All @@ -222,6 +234,7 @@ def run_test (self):
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)
Expand All @@ -235,6 +248,7 @@ def run_test (self):
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")
Expand All @@ -246,6 +260,8 @@ def run_test (self):
"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)
Expand All @@ -260,6 +276,7 @@ def run_test (self):
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")
Expand All @@ -272,6 +289,8 @@ def run_test (self):
"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)
Expand Down Expand Up @@ -319,6 +338,7 @@ def run_test (self):
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
Expand All @@ -338,6 +358,7 @@ def run_test (self):
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
Expand All @@ -357,6 +378,7 @@ def run_test (self):
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
Expand All @@ -375,6 +397,15 @@ def run_test (self):
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")
Expand Down
3 changes: 3 additions & 0 deletions src/rpc/misc.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()) {
Expand Down
20 changes: 11 additions & 9 deletions src/wallet/rpcdump.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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<CKeyID, int64_t> mapKeyBirth;
std::map<CTxDestination, int64_t> mapKeyBirth;
std::set<CKeyID> setKeyPool;
pwalletMain->GetKeyBirthTimes(mapKeyBirth);
pwalletMain->GetAllReserveKeys(setKeyPool);

// sort time/key pairs
std::vector<std::pair<int64_t, CKeyID> > vKeyBirth;
for (std::map<CKeyID, int64_t>::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<CKeyID>(&entry.first)) { // set and test
vKeyBirth.push_back(std::make_pair(entry.second, *keyID));
}
}
mapKeyBirth.clear();
std::sort(vKeyBirth.begin(), vKeyBirth.end());
Expand Down Expand Up @@ -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");
}

Expand All @@ -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");
}

Expand Down Expand Up @@ -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");
}

Expand All @@ -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");
}

Expand Down Expand Up @@ -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");
}

Expand Down
27 changes: 18 additions & 9 deletions src/wallet/wallet.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -3425,14 +3432,16 @@ class CAffectedKeysVisitor : public boost::static_visitor<void> {
void operator()(const CNoDestination &none) {}
};

void CWallet::GetKeyBirthTimes(std::map<CKeyID, int64_t> &mapKeyBirth) const {
void CWallet::GetKeyBirthTimes(std::map<CTxDestination, int64_t> &mapKeyBirth) const {
AssertLockHeld(cs_wallet); // mapKeyMetadata
mapKeyBirth.clear();

// get birth times for keys with metadata
for (std::map<CKeyID, CKeyMetadata>::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
Expand Down
21 changes: 17 additions & 4 deletions src/wallet/wallet.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -614,6 +614,17 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface

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.
Expand All @@ -638,7 +649,9 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
mapKeyMetadata[keyid] = CKeyMetadata(keypool.nTime);
}

std::map<CKeyID, CKeyMetadata> mapKeyMetadata;
// Map from Key ID (for regular keys) or Script ID (for watch-only keys) to
// key metadata.
std::map<CTxDestination, CKeyMetadata> mapKeyMetadata;

typedef std::map<unsigned int, CMasterKey> MasterKeyMap;
MasterKeyMap mapMasterKeys;
Expand Down Expand Up @@ -728,7 +741,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
//! 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);
Expand All @@ -750,7 +763,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
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);
Expand All @@ -759,7 +772,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
bool ChangeWalletPassphrase(const SecureString& strOldWalletPassphrase, const SecureString& strNewWalletPassphrase);
bool EncryptWallet(const SecureString& strWalletPassphrase);

void GetKeyBirthTimes(std::map<CKeyID, int64_t> &mapKeyBirth) const;
void GetKeyBirthTimes(std::map<CTxDestination, int64_t> &mapKeyBirth) const;

/**
* Increment the next transaction order id
Expand Down
Loading

0 comments on commit a80f98b

Please sign in to comment.