Skip to content

Commit d8e8b06

Browse files
laanwjlaanwj
committed
Merge bitcoin#9108: Use importmulti timestamp when importing watch only keys (on top of bitcoin#9682)
a80f98b Use importmulti timestamp when importing watch only keys (Russell Yanofsky) a58370e Dedup nTimeFirstKey update logic (Russell Yanofsky)
2 parents 4c69d68 + a80f98b commit d8e8b06

File tree

7 files changed

+130
-57
lines changed

7 files changed

+130
-57
lines changed

‎qa/rpc-tests/importmulti.py

Lines changed: 32 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ def run_test (self):
2020
print ("Mining blocks...")
2121
self.nodes[0].generate(1)
2222
self.nodes[1].generate(1)
23+
timestamp = self.nodes[1].getblock(self.nodes[1].getbestblockhash())['mediantime']
2324

2425
# keyword definition
2526
PRIV_KEY = 'privkey'
@@ -59,6 +60,9 @@ def run_test (self):
5960
address_assert = self.nodes[1].validateaddress(address['address'])
6061
assert_equal(address_assert['iswatchonly'], True)
6162
assert_equal(address_assert['ismine'], False)
63+
assert_equal(address_assert['timestamp'], timestamp)
64+
watchonly_address = address['address']
65+
watchonly_timestamp = timestamp
6266

6367

6468
# ScriptPubKey + internal
@@ -73,6 +77,7 @@ def run_test (self):
7377
address_assert = self.nodes[1].validateaddress(address['address'])
7478
assert_equal(address_assert['iswatchonly'], True)
7579
assert_equal(address_assert['ismine'], False)
80+
assert_equal(address_assert['timestamp'], timestamp)
7681

7782
# ScriptPubKey + !internal
7883
print("Should not import a scriptPubKey without internal flag")
@@ -87,6 +92,7 @@ def run_test (self):
8792
address_assert = self.nodes[1].validateaddress(address['address'])
8893
assert_equal(address_assert['iswatchonly'], False)
8994
assert_equal(address_assert['ismine'], False)
95+
assert_equal('timestamp' in address_assert, False)
9096

9197

9298
# Address + Public key + !Internal
@@ -103,6 +109,7 @@ def run_test (self):
103109
address_assert = self.nodes[1].validateaddress(address['address'])
104110
assert_equal(address_assert['iswatchonly'], True)
105111
assert_equal(address_assert['ismine'], False)
112+
assert_equal(address_assert['timestamp'], timestamp)
106113

107114

108115
# ScriptPubKey + Public key + internal
@@ -119,6 +126,7 @@ def run_test (self):
119126
address_assert = self.nodes[1].validateaddress(address['address'])
120127
assert_equal(address_assert['iswatchonly'], True)
121128
assert_equal(address_assert['ismine'], False)
129+
assert_equal(address_assert['timestamp'], timestamp)
122130

123131
# ScriptPubKey + Public key + !internal
124132
print("Should not import a scriptPubKey without internal and with public key")
@@ -135,11 +143,11 @@ def run_test (self):
135143
address_assert = self.nodes[1].validateaddress(address['address'])
136144
assert_equal(address_assert['iswatchonly'], False)
137145
assert_equal(address_assert['ismine'], False)
146+
assert_equal('timestamp' in address_assert, False)
138147

139148
# Address + Private key + !watchonly
140149
print("Should import an address with private key")
141150
address = self.nodes[0].validateaddress(self.nodes[0].getnewaddress())
142-
timestamp = self.nodes[1].getblock(self.nodes[1].getbestblockhash())['mediantime']
143151
result = self.nodes[1].importmulti([{
144152
"scriptPubKey": {
145153
"address": address['address']
@@ -170,6 +178,7 @@ def run_test (self):
170178
address_assert = self.nodes[1].validateaddress(address['address'])
171179
assert_equal(address_assert['iswatchonly'], False)
172180
assert_equal(address_assert['ismine'], False)
181+
assert_equal('timestamp' in address_assert, False)
173182

174183
# ScriptPubKey + Private key + internal
175184
print("Should import a scriptPubKey with internal and with private key")
@@ -184,6 +193,7 @@ def run_test (self):
184193
address_assert = self.nodes[1].validateaddress(address['address'])
185194
assert_equal(address_assert['iswatchonly'], False)
186195
assert_equal(address_assert['ismine'], True)
196+
assert_equal(address_assert['timestamp'], timestamp)
187197

188198
# ScriptPubKey + Private key + !internal
189199
print("Should not import a scriptPubKey without internal and with private key")
@@ -199,6 +209,7 @@ def run_test (self):
199209
address_assert = self.nodes[1].validateaddress(address['address'])
200210
assert_equal(address_assert['iswatchonly'], False)
201211
assert_equal(address_assert['ismine'], False)
212+
assert_equal('timestamp' in address_assert, False)
202213

203214

204215
# P2SH address
@@ -209,6 +220,7 @@ def run_test (self):
209220
self.nodes[1].generate(100)
210221
transactionid = self.nodes[1].sendtoaddress(multi_sig_script['address'], 10.00)
211222
self.nodes[1].generate(1)
223+
timestamp = self.nodes[1].getblock(self.nodes[1].getbestblockhash())['mediantime']
212224
transaction = self.nodes[1].gettransaction(transactionid)
213225

214226
print("Should import a p2sh")
@@ -222,6 +234,7 @@ def run_test (self):
222234
address_assert = self.nodes[1].validateaddress(multi_sig_script['address'])
223235
assert_equal(address_assert['isscript'], True)
224236
assert_equal(address_assert['iswatchonly'], True)
237+
assert_equal(address_assert['timestamp'], timestamp)
225238
p2shunspent = self.nodes[1].listunspent(0,999999, [multi_sig_script['address']])[0]
226239
assert_equal(p2shunspent['spendable'], False)
227240
assert_equal(p2shunspent['solvable'], False)
@@ -235,6 +248,7 @@ def run_test (self):
235248
self.nodes[1].generate(100)
236249
transactionid = self.nodes[1].sendtoaddress(multi_sig_script['address'], 10.00)
237250
self.nodes[1].generate(1)
251+
timestamp = self.nodes[1].getblock(self.nodes[1].getbestblockhash())['mediantime']
238252
transaction = self.nodes[1].gettransaction(transactionid)
239253

240254
print("Should import a p2sh with respective redeem script")
@@ -246,6 +260,8 @@ def run_test (self):
246260
"redeemscript": multi_sig_script['redeemScript']
247261
}])
248262
assert_equal(result[0]['success'], True)
263+
address_assert = self.nodes[1].validateaddress(multi_sig_script['address'])
264+
assert_equal(address_assert['timestamp'], timestamp)
249265

250266
p2shunspent = self.nodes[1].listunspent(0,999999, [multi_sig_script['address']])[0]
251267
assert_equal(p2shunspent['spendable'], False)
@@ -260,6 +276,7 @@ def run_test (self):
260276
self.nodes[1].generate(100)
261277
transactionid = self.nodes[1].sendtoaddress(multi_sig_script['address'], 10.00)
262278
self.nodes[1].generate(1)
279+
timestamp = self.nodes[1].getblock(self.nodes[1].getbestblockhash())['mediantime']
263280
transaction = self.nodes[1].gettransaction(transactionid)
264281

265282
print("Should import a p2sh with respective redeem script and private keys")
@@ -272,6 +289,8 @@ def run_test (self):
272289
"keys": [ self.nodes[0].dumpprivkey(sig_address_1['address']), self.nodes[0].dumpprivkey(sig_address_2['address'])]
273290
}])
274291
assert_equal(result[0]['success'], True)
292+
address_assert = self.nodes[1].validateaddress(multi_sig_script['address'])
293+
assert_equal(address_assert['timestamp'], timestamp)
275294

276295
p2shunspent = self.nodes[1].listunspent(0,999999, [multi_sig_script['address']])[0]
277296
assert_equal(p2shunspent['spendable'], False)
@@ -319,6 +338,7 @@ def run_test (self):
319338
address_assert = self.nodes[1].validateaddress(address['address'])
320339
assert_equal(address_assert['iswatchonly'], False)
321340
assert_equal(address_assert['ismine'], False)
341+
assert_equal('timestamp' in address_assert, False)
322342

323343

324344
# ScriptPubKey + Public key + internal + Wrong pubkey
@@ -338,6 +358,7 @@ def run_test (self):
338358
address_assert = self.nodes[1].validateaddress(address['address'])
339359
assert_equal(address_assert['iswatchonly'], False)
340360
assert_equal(address_assert['ismine'], False)
361+
assert_equal('timestamp' in address_assert, False)
341362

342363

343364
# Address + Private key + !watchonly + Wrong private key
@@ -357,6 +378,7 @@ def run_test (self):
357378
address_assert = self.nodes[1].validateaddress(address['address'])
358379
assert_equal(address_assert['iswatchonly'], False)
359380
assert_equal(address_assert['ismine'], False)
381+
assert_equal('timestamp' in address_assert, False)
360382

361383

362384
# ScriptPubKey + Private key + internal + Wrong private key
@@ -375,6 +397,15 @@ def run_test (self):
375397
address_assert = self.nodes[1].validateaddress(address['address'])
376398
assert_equal(address_assert['iswatchonly'], False)
377399
assert_equal(address_assert['ismine'], False)
400+
assert_equal('timestamp' in address_assert, False)
401+
402+
# restart nodes to check for proper serialization/deserialization of watch only address
403+
stop_nodes(self.nodes)
404+
self.nodes = start_nodes(2, self.options.tmpdir)
405+
address_assert = self.nodes[1].validateaddress(watchonly_address)
406+
assert_equal(address_assert['iswatchonly'], True)
407+
assert_equal(address_assert['ismine'], False)
408+
assert_equal(address_assert['timestamp'], watchonly_timestamp);
378409

379410
# Bad or missing timestamps
380411
print("Should throw on invalid or missing timestamp values")

‎src/rpc/misc.cpp

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -208,6 +208,9 @@ UniValue validateaddress(const JSONRPCRequest& request)
208208
if (pwalletMain) {
209209
const auto& meta = pwalletMain->mapKeyMetadata;
210210
auto it = address.GetKeyID(keyID) ? meta.find(keyID) : meta.end();
211+
if (it == meta.end()) {
212+
it = meta.find(CScriptID(scriptPubKey));
213+
}
211214
if (it != meta.end()) {
212215
ret.push_back(Pair("timestamp", it->second.nCreateTime));
213216
if (!it->second.hdKeypath.empty()) {

‎src/wallet/rpcdump.cpp

Lines changed: 15 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -143,7 +143,7 @@ UniValue importprivkey(const JSONRPCRequest& request)
143143
throw JSONRPCError(RPC_WALLET_ERROR, "Error adding key to wallet");
144144

145145
// whenever a key is imported, we need to scan the whole chain
146-
pwalletMain->nTimeFirstKey = 1; // 0 would be considered 'no value'
146+
pwalletMain->UpdateTimeFirstKey(1);
147147

148148
if (fRescan) {
149149
pwalletMain->ScanForWalletTransactions(chainActive.Genesis(), true);
@@ -161,7 +161,7 @@ void ImportScript(const CScript& script, const string& strLabel, bool isRedeemSc
161161

162162
pwalletMain->MarkDirty();
163163

164-
if (!pwalletMain->HaveWatchOnly(script) && !pwalletMain->AddWatchOnly(script))
164+
if (!pwalletMain->HaveWatchOnly(script) && !pwalletMain->AddWatchOnly(script, 0 /* nCreateTime */))
165165
throw JSONRPCError(RPC_WALLET_ERROR, "Error adding address to wallet");
166166

167167
if (isRedeemScript) {
@@ -500,8 +500,7 @@ UniValue importwallet(const JSONRPCRequest& request)
500500
while (pindex && pindex->pprev && pindex->GetBlockTime() > nTimeBegin - 7200)
501501
pindex = pindex->pprev;
502502

503-
if (!pwalletMain->nTimeFirstKey || nTimeBegin < pwalletMain->nTimeFirstKey)
504-
pwalletMain->nTimeFirstKey = nTimeBegin;
503+
pwalletMain->UpdateTimeFirstKey(nTimeBegin);
505504

506505
LogPrintf("Rescanning last %i blocks\n", chainActive.Height() - pindex->nHeight + 1);
507506
pwalletMain->ScanForWalletTransactions(pindex);
@@ -576,15 +575,17 @@ UniValue dumpwallet(const JSONRPCRequest& request)
576575
if (!file.is_open())
577576
throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot open wallet dump file");
578577

579-
std::map<CKeyID, int64_t> mapKeyBirth;
578+
std::map<CTxDestination, int64_t> mapKeyBirth;
580579
std::set<CKeyID> setKeyPool;
581580
pwalletMain->GetKeyBirthTimes(mapKeyBirth);
582581
pwalletMain->GetAllReserveKeys(setKeyPool);
583582

584583
// sort time/key pairs
585584
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));
585+
for (const auto& entry : mapKeyBirth) {
586+
if (const CKeyID* keyID = boost::get<CKeyID>(&entry.first)) { // set and test
587+
vKeyBirth.push_back(std::make_pair(entry.second, *keyID));
588+
}
588589
}
589590
mapKeyBirth.clear();
590591
std::sort(vKeyBirth.begin(), vKeyBirth.end());
@@ -721,7 +722,7 @@ UniValue ProcessImport(const UniValue& data, const int64_t timestamp)
721722

722723
pwalletMain->MarkDirty();
723724

724-
if (!pwalletMain->HaveWatchOnly(redeemScript) && !pwalletMain->AddWatchOnly(redeemScript)) {
725+
if (!pwalletMain->HaveWatchOnly(redeemScript) && !pwalletMain->AddWatchOnly(redeemScript, timestamp)) {
725726
throw JSONRPCError(RPC_WALLET_ERROR, "Error adding address to wallet");
726727
}
727728

@@ -738,7 +739,7 @@ UniValue ProcessImport(const UniValue& data, const int64_t timestamp)
738739

739740
pwalletMain->MarkDirty();
740741

741-
if (!pwalletMain->HaveWatchOnly(redeemDestination) && !pwalletMain->AddWatchOnly(redeemDestination)) {
742+
if (!pwalletMain->HaveWatchOnly(redeemDestination) && !pwalletMain->AddWatchOnly(redeemDestination, timestamp)) {
742743
throw JSONRPCError(RPC_WALLET_ERROR, "Error adding address to wallet");
743744
}
744745

@@ -782,9 +783,7 @@ UniValue ProcessImport(const UniValue& data, const int64_t timestamp)
782783
throw JSONRPCError(RPC_WALLET_ERROR, "Error adding key to wallet");
783784
}
784785

785-
if (timestamp < pwalletMain->nTimeFirstKey) {
786-
pwalletMain->nTimeFirstKey = timestamp;
787-
}
786+
pwalletMain->UpdateTimeFirstKey(timestamp);
788787
}
789788
}
790789

@@ -833,7 +832,7 @@ UniValue ProcessImport(const UniValue& data, const int64_t timestamp)
833832

834833
pwalletMain->MarkDirty();
835834

836-
if (!pwalletMain->HaveWatchOnly(pubKeyScript) && !pwalletMain->AddWatchOnly(pubKeyScript)) {
835+
if (!pwalletMain->HaveWatchOnly(pubKeyScript) && !pwalletMain->AddWatchOnly(pubKeyScript, timestamp)) {
837836
throw JSONRPCError(RPC_WALLET_ERROR, "Error adding address to wallet");
838837
}
839838

@@ -851,7 +850,7 @@ UniValue ProcessImport(const UniValue& data, const int64_t timestamp)
851850

852851
pwalletMain->MarkDirty();
853852

854-
if (!pwalletMain->HaveWatchOnly(scriptRawPubKey) && !pwalletMain->AddWatchOnly(scriptRawPubKey)) {
853+
if (!pwalletMain->HaveWatchOnly(scriptRawPubKey) && !pwalletMain->AddWatchOnly(scriptRawPubKey, timestamp)) {
855854
throw JSONRPCError(RPC_WALLET_ERROR, "Error adding address to wallet");
856855
}
857856

@@ -912,9 +911,7 @@ UniValue ProcessImport(const UniValue& data, const int64_t timestamp)
912911
throw JSONRPCError(RPC_WALLET_ERROR, "Error adding key to wallet");
913912
}
914913

915-
if (timestamp < pwalletMain->nTimeFirstKey) {
916-
pwalletMain->nTimeFirstKey = timestamp;
917-
}
914+
pwalletMain->UpdateTimeFirstKey(timestamp);
918915

919916
success = true;
920917
}
@@ -927,7 +924,7 @@ UniValue ProcessImport(const UniValue& data, const int64_t timestamp)
927924

928925
pwalletMain->MarkDirty();
929926

930-
if (!pwalletMain->HaveWatchOnly(script) && !pwalletMain->AddWatchOnly(script)) {
927+
if (!pwalletMain->HaveWatchOnly(script) && !pwalletMain->AddWatchOnly(script, timestamp)) {
931928
throw JSONRPCError(RPC_WALLET_ERROR, "Error adding address to wallet");
932929
}
933930

‎src/wallet/wallet.cpp

Lines changed: 32 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -113,8 +113,7 @@ CPubKey CWallet::GenerateNewKey()
113113
assert(secret.VerifyPubKey(pubkey));
114114

115115
mapKeyMetadata[pubkey.GetID()] = metadata;
116-
if (!nTimeFirstKey || nCreationTime < nTimeFirstKey)
117-
nTimeFirstKey = nCreationTime;
116+
UpdateTimeFirstKey(nCreationTime);
118117

119118
if (!AddKeyPubKey(secret, pubkey))
120119
throw std::runtime_error(std::string(__func__) + ": AddKey failed");
@@ -207,13 +206,11 @@ bool CWallet::AddCryptedKey(const CPubKey &vchPubKey,
207206
return false;
208207
}
209208

210-
bool CWallet::LoadKeyMetadata(const CPubKey &pubkey, const CKeyMetadata &meta)
209+
bool CWallet::LoadKeyMetadata(const CTxDestination& keyID, const CKeyMetadata &meta)
211210
{
212211
AssertLockHeld(cs_wallet); // mapKeyMetadata
213-
if (meta.nCreateTime && (!nTimeFirstKey || meta.nCreateTime < nTimeFirstKey))
214-
nTimeFirstKey = meta.nCreateTime;
215-
216-
mapKeyMetadata[pubkey.GetID()] = meta;
212+
UpdateTimeFirstKey(meta.nCreateTime);
213+
mapKeyMetadata[keyID] = meta;
217214
return true;
218215
}
219216

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

222+
void CWallet::UpdateTimeFirstKey(int64_t nCreateTime)
223+
{
224+
AssertLockHeld(cs_wallet);
225+
if (nCreateTime <= 1) {
226+
// Cannot determine birthday information, so set the wallet birthday to
227+
// the beginning of time.
228+
nTimeFirstKey = 1;
229+
} else if (!nTimeFirstKey || nCreateTime < nTimeFirstKey) {
230+
nTimeFirstKey = nCreateTime;
231+
}
232+
}
233+
225234
bool CWallet::AddCScript(const CScript& redeemScript)
226235
{
227236
if (!CCryptoKeyStore::AddCScript(redeemScript))
@@ -247,15 +256,22 @@ bool CWallet::LoadCScript(const CScript& redeemScript)
247256
return CCryptoKeyStore::AddCScript(redeemScript);
248257
}
249258

250-
bool CWallet::AddWatchOnly(const CScript &dest)
259+
bool CWallet::AddWatchOnly(const CScript& dest)
251260
{
252261
if (!CCryptoKeyStore::AddWatchOnly(dest))
253262
return false;
254-
nTimeFirstKey = 1; // No birthday information for watch-only keys.
263+
const CKeyMetadata& meta = mapKeyMetadata[CScriptID(dest)];
264+
UpdateTimeFirstKey(meta.nCreateTime);
255265
NotifyWatchonlyChanged(true);
256266
if (!fFileBacked)
257267
return true;
258-
return CWalletDB(strWalletFile).WriteWatchOnly(dest);
268+
return CWalletDB(strWalletFile).WriteWatchOnly(dest, meta);
269+
}
270+
271+
bool CWallet::AddWatchOnly(const CScript& dest, int64_t nCreateTime)
272+
{
273+
mapKeyMetadata[CScriptID(dest)].nCreateTime = nCreateTime;
274+
return AddWatchOnly(dest);
259275
}
260276

261277
bool CWallet::RemoveWatchOnly(const CScript &dest)
@@ -3416,14 +3432,16 @@ class CAffectedKeysVisitor : public boost::static_visitor<void> {
34163432
void operator()(const CNoDestination &none) {}
34173433
};
34183434

3419-
void CWallet::GetKeyBirthTimes(std::map<CKeyID, int64_t> &mapKeyBirth) const {
3435+
void CWallet::GetKeyBirthTimes(std::map<CTxDestination, int64_t> &mapKeyBirth) const {
34203436
AssertLockHeld(cs_wallet); // mapKeyMetadata
34213437
mapKeyBirth.clear();
34223438

34233439
// get birth times for keys with metadata
3424-
for (std::map<CKeyID, CKeyMetadata>::const_iterator it = mapKeyMetadata.begin(); it != mapKeyMetadata.end(); it++)
3425-
if (it->second.nCreateTime)
3426-
mapKeyBirth[it->first] = it->second.nCreateTime;
3440+
for (const auto& entry : mapKeyMetadata) {
3441+
if (entry.second.nCreateTime) {
3442+
mapKeyBirth[entry.first] = entry.second.nCreateTime;
3443+
}
3444+
}
34273445

34283446
// map in which we'll infer heights of other keys
34293447
CBlockIndex *pindexMax = chainActive[std::max(0, chainActive.Height() - 144)]; // the tip can be reorganized; use a 144-block safety margin

0 commit comments

Comments
 (0)