From d6958b741e1a28511005446105646597e8cf161b Mon Sep 17 00:00:00 2001 From: random-zebra Date: Tue, 30 Mar 2021 11:49:05 +0200 Subject: [PATCH 1/7] [RPC][Tests] Use 2 hour grace period for key timestamps in importmulti >>> backports bitcoin@e662af358305b9fa4da772ec37c8356a9fc92ab6 --- src/chain.h | 7 +++++++ src/wallet/rpcdump.cpp | 12 +++++++++--- src/wallet/wallet.cpp | 4 ++-- 3 files changed, 18 insertions(+), 5 deletions(-) diff --git a/src/chain.h b/src/chain.h index eff3d71635e5..2ccc1851f9bd 100644 --- a/src/chain.h +++ b/src/chain.h @@ -22,6 +22,13 @@ #include +/** + * Timestamp window used as a grace period by code that compares external + * timestamps (such as timestamps passed to RPCs, or wallet key creation times) + * to block timestamps. + */ +static constexpr int64_t TIMESTAMP_WINDOW = 2 * 60 * 60; + class CBlockFileInfo { public: diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp index 71e87194d2ac..6511d5f0c0dd 100644 --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -413,7 +413,7 @@ UniValue importwallet(const JSONRPCRequest& request) pwalletMain->ShowProgress("", 100); // hide progress dialog in GUI CBlockIndex* pindex = chainActive.Tip(); - while (pindex && pindex->pprev && pindex->GetBlockTime() > nTimeBegin - 7200) + while (pindex && pindex->pprev && pindex->GetBlockTime() > nTimeBegin - TIMESTAMP_WINDOW) pindex = pindex->pprev; if (!pwalletMain->nTimeFirstKey || nTimeBegin < pwalletMain->nTimeFirstKey) @@ -933,13 +933,18 @@ UniValue importmulti(const JSONRPCRequest& mainRequest) " [ (array of json objects)\n" " {\n" " \"scriptPubKey\": \"script\" | { \"address\":\"address\" }, (string / JSON, required) Type of scriptPubKey (string for script, json for address)\n" + " \"timestamp\": timestamp | \"now\" (integer / string, required) Creation time of the key in seconds since epoch (Jan 1 1970 GMT),\n" + " or the string \"now\" to substitute the current synced blockchain time. The timestamp of the oldest\n" + " key will determine how far back blockchain rescans need to begin for missing wallet transactions.\n" + " \"now\" can be specified to bypass scanning, for keys which are known to never have been used, and\n" + " 0 can be specified to scan the entire blockchain. Blocks up to 2 hours before the earliest key\n" + " creation time of all keys being imported by the importmulti call will be scanned.\n" " \"redeemscript\": \"script\", (string, optional) Allowed only if the scriptPubKey is a P2SH address or a P2SH scriptPubKey\n" " \"pubkeys\": [\"pubKey\", ... ], (array, optional) Array of strings giving pubkeys that must occur in the output or redeemscript\n" " \"keys\": [\"key\", ... ], (array, optional) Array of strings giving private keys whose corresponding public keys must occur in the output or redeemscript\n" " \"internal\": true|false, (boolean, optional, default: false) Stating whether matching outputs should be be treated as not incoming payments\n" " \"watchonly\": true|false, (boolean, optional, default: false) Stating whether matching outputs should be considered watched even when they're not spendable, only allowed if keys are empty\n" " \"label\": label, (string, optional, default: '') Label to assign to the address, only allowed with internal=false\n" - " \"timestamp\": 1454686740, (integer, optional, default now) Timestamp\n" " }\n" " ,...\n" " ]\n" @@ -1023,7 +1028,8 @@ UniValue importmulti(const JSONRPCRequest& mainRequest) } if (fRescan && fRunScan && requests.size() && nLowestTimestamp <= chainActive.Tip()->GetBlockTimeMax()) { - CBlockIndex* pindex = nLowestTimestamp > minimumTimestamp ? chainActive.FindEarliestAtLeast(nLowestTimestamp) : chainActive.Genesis(); + CBlockIndex* pindex = nLowestTimestamp > minimumTimestamp ? chainActive.FindEarliestAtLeast(std::max(nLowestTimestamp - TIMESTAMP_WINDOW, 0)) + : chainActive.Genesis(); if (pindex) { pwalletMain->ScanForWalletTransactions(pindex, nullptr, true); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index be3fc351f44f..d50193abcf47 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1834,7 +1834,7 @@ CBlockIndex* CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, CBlock LOCK(cs_main); // no need to read and scan block, if block was created before // our wallet birthday (as adjusted for block time variability) - while (pindex && nTimeFirstKey && (pindex->GetBlockTime() < (nTimeFirstKey - 7200))) + while (pindex && nTimeFirstKey && (pindex->GetBlockTime() < (nTimeFirstKey - TIMESTAMP_WINDOW))) pindex = chainActive.Next(pindex); } @@ -3881,7 +3881,7 @@ void CWallet::GetKeyBirthTimes(std::map& mapKeyBirth) const // Extract block timestamps for those keys for (std::map::const_iterator it = mapKeyFirstBlock.begin(); it != mapKeyFirstBlock.end(); it++) - mapKeyBirth[it->first] = it->second->GetBlockTime() - 7200; // block times can be 2h off + mapKeyBirth[it->first] = it->second->GetBlockTime() - TIMESTAMP_WINDOW; // block times can be 2h off } bool CWallet::AddDestData(const CTxDestination& dest, const std::string& key, const std::string& value) From d92f8831d71bb1340c60fd84b3dfd634dd61cc2a Mon Sep 17 00:00:00 2001 From: random-zebra Date: Tue, 30 Mar 2021 12:22:09 +0200 Subject: [PATCH 2/7] [BUG][Wallet] Fix watchonly selection in AvailableCoins `coinControl->fAllowWatchOnly` should only affect the spendability of a coin. A wo script is spendable if it is solvable and fAllowWatchOnly is true. When fAllowWatchOnly is false, the coin should not be skipped in CheckOutputAvailability, but included as non-spendable, and then skipped in AvailableCoins when `coinsFilter.fOnlySpendable` is true. --- src/wallet/wallet.cpp | 4 +--- test/functional/wallet_importmulti.py | 10 +++++----- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index d50193abcf47..12aee3896e53 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2468,9 +2468,6 @@ CWallet::OutputAvailabilityResult CWallet::CheckOutputAvailability( // Check If not mine if (mine == ISMINE_NO) return res; - // Check if watch only utxo are allowed - if (mine == ISMINE_WATCH_ONLY && coinControl && !coinControl->fAllowWatchOnly) return res; - // Skip locked utxo if (!fIncludeLocked && IsLockedCoin(wtxid, outIndex) && nCoinType != ONLY_10000) return res; @@ -2923,6 +2920,7 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CScript scriptChange; CWallet::AvailableCoinsFilter coinFilter; + coinFilter.fOnlySpendable = true; coinFilter.fIncludeDelegated = fIncludeDelegated; coinFilter.nCoinType = coin_type; diff --git a/test/functional/wallet_importmulti.py b/test/functional/wallet_importmulti.py index 8e9721933353..2bef488179a7 100755 --- a/test/functional/wallet_importmulti.py +++ b/test/functional/wallet_importmulti.py @@ -247,7 +247,7 @@ def run_test (self): 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']], 2)[0] + p2shunspent = self.nodes[1].listunspent(0, 999999, [multi_sig_script['address']])[0] assert_equal(p2shunspent['spendable'], False) assert_equal(p2shunspent['solvable'], False) @@ -272,8 +272,8 @@ def run_test (self): 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']], 2)[0] - assert_equal(p2shunspent['spendable'], True) + p2shunspent = self.nodes[1].listunspent(0, 999999, [multi_sig_script['address']])[0] + assert_equal(p2shunspent['spendable'], False) assert_equal(p2shunspent['solvable'], True) # P2SH + Redeem script + Private Keys + !Watchonly @@ -301,8 +301,8 @@ def run_test (self): 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']], 2)[0] - assert_equal(p2shunspent['spendable'], True) + p2shunspent = self.nodes[1].listunspent(0, 999999, [multi_sig_script['address']])[0] + assert_equal(p2shunspent['spendable'], False) assert_equal(p2shunspent['solvable'], True) # P2SH + Redeem script + Private Keys + Watchonly From df20205276c9836de9b06bb9a33276f70ba60a3c Mon Sep 17 00:00:00 2001 From: random-zebra Date: Tue, 30 Mar 2021 14:19:38 +0200 Subject: [PATCH 3/7] [BUG] net: Consistently use GetTimeMicros() for inactivity checks >>> backports bitcoin@99464bc38e9575ff47f8e33223b252dcea2055e3 The use of mocktime in test logic means that comparisons between GetTime() and GetTimeMicros()/1000000 are unreliable since the former can use mocktime values while the latter always gets the system clock; this changes the networking code's inactivity checks to consistently use the system clock for inactivity comparisons. Also remove some hacks from setmocktime() that are no longer needed, now that we're using the system clock for nLastSend and nLastRecv. --- src/net.cpp | 6 +++--- src/qt/rpcconsole.cpp | 6 +++--- src/rpc/misc.cpp | 12 +++++------- src/utiltime.cpp | 5 +++++ src/utiltime.h | 11 +++++++++++ 5 files changed, 27 insertions(+), 13 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index b418096a7dc2..abcae46a4670 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -823,7 +823,7 @@ size_t CConnman::SocketSendData(CNode* pnode) nBytes = send(pnode->hSocket, reinterpret_cast(data.data()) + pnode->nSendOffset, data.size() - pnode->nSendOffset, MSG_NOSIGNAL | MSG_DONTWAIT); } if (nBytes > 0) { - pnode->nLastSend = GetTime(); + pnode->nLastSend = GetSystemTimeInSeconds(); pnode->nSendBytes += nBytes; pnode->nSendOffset += nBytes; nSentSize += nBytes; @@ -1311,7 +1311,7 @@ void CConnman::ThreadSocketHandler() // // Inactivity checking // - int64_t nTime = GetTime(); + int64_t nTime = GetSystemTimeInSeconds(); if (nTime - pnode->nTimeConnected > 60) { if (pnode->nLastRecv == 0 || pnode->nLastSend == 0) { LogPrint(BCLog::NET, "socket no message in first 60 seconds, %d %d from %d\n", pnode->nLastRecv != 0, pnode->nLastSend != 0, pnode->id); @@ -2401,7 +2401,7 @@ unsigned int CConnman::GetReceiveFloodSize() const { return nReceiveFloodSize; } unsigned int CConnman::GetSendBufferSize() const{ return nSendBufferMaxSize; } CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn, SOCKET hSocketIn, const CAddress& addrIn, uint64_t nKeyedNetGroupIn, uint64_t nLocalHostNonceIn, const std::string& addrNameIn, bool fInboundIn) : - nTimeConnected(GetTime()), + nTimeConnected(GetSystemTimeInSeconds()), addr(addrIn), fInbound(fInboundIn), id(idIn), diff --git a/src/qt/rpcconsole.cpp b/src/qt/rpcconsole.cpp index fefb6d1cc997..950859ce5b73 100644 --- a/src/qt/rpcconsole.cpp +++ b/src/qt/rpcconsole.cpp @@ -901,11 +901,11 @@ void RPCConsole::updateNodeDetail(const CNodeCombinedStats* stats) peerAddrDetails += "
" + tr("via %1").arg(QString::fromStdString(stats->nodeStats.addrLocal)); ui->peerHeading->setText(peerAddrDetails); ui->peerServices->setText(GUIUtil::formatServicesStr(stats->nodeStats.nServices)); - ui->peerLastSend->setText(stats->nodeStats.nLastSend ? GUIUtil::formatDurationStr(GetTime() - stats->nodeStats.nLastSend) : tr("never")); - ui->peerLastRecv->setText(stats->nodeStats.nLastRecv ? GUIUtil::formatDurationStr(GetTime() - stats->nodeStats.nLastRecv) : tr("never")); + ui->peerLastSend->setText(stats->nodeStats.nLastSend ? GUIUtil::formatDurationStr(GetSystemTimeInSeconds() - stats->nodeStats.nLastSend) : tr("never")); + ui->peerLastRecv->setText(stats->nodeStats.nLastRecv ? GUIUtil::formatDurationStr(GetSystemTimeInSeconds() - stats->nodeStats.nLastRecv) : tr("never")); ui->peerBytesSent->setText(FormatBytes(stats->nodeStats.nSendBytes)); ui->peerBytesRecv->setText(FormatBytes(stats->nodeStats.nRecvBytes)); - ui->peerConnTime->setText(GUIUtil::formatDurationStr(GetTime() - stats->nodeStats.nTimeConnected)); + ui->peerConnTime->setText(GUIUtil::formatDurationStr(GetSystemTimeInSeconds() - stats->nodeStats.nTimeConnected)); ui->peerPingTime->setText(GUIUtil::formatPingTime(stats->nodeStats.dPingTime)); ui->peerPingWait->setText(GUIUtil::formatPingTime(stats->nodeStats.dPingWait)); ui->timeoffset->setText(GUIUtil::formatTimeOffset(stats->nodeStats.nTimeOffset)); diff --git a/src/rpc/misc.cpp b/src/rpc/misc.cpp index 1818e0916152..794b7fb4fd63 100644 --- a/src/rpc/misc.cpp +++ b/src/rpc/misc.cpp @@ -607,18 +607,16 @@ UniValue setmocktime(const JSONRPCRequest& request) if (!Params().IsRegTestNet()) throw std::runtime_error("setmocktime for regression testing (-regtest mode) only"); + // For now, don't change mocktime if we're in the middle of validation, as + // this could have an effect on mempool time-based eviction, as well as + // IsCurrentForFeeEstimation() and IsInitialBlockDownload(). + // TODO: figure out the right way to synchronize around mocktime, and + // ensure all callsites of GetTime() are accessing this safely. LOCK(cs_main); RPCTypeCheck(request.params, {UniValue::VNUM}); SetMockTime(request.params[0].get_int64()); - uint64_t t = GetTime(); - if(g_connman) { - g_connman->ForEachNode([t](CNode* pnode) { - pnode->nLastSend = pnode->nLastRecv = t; - }); - } - return NullUniValue; } diff --git a/src/utiltime.cpp b/src/utiltime.cpp index 75c2a3b1f1a3..ab59da754057 100644 --- a/src/utiltime.cpp +++ b/src/utiltime.cpp @@ -43,6 +43,11 @@ int64_t GetTimeMillis() return now; } +int64_t GetSystemTimeInSeconds() +{ + return GetTimeMicros()/1000000; +} + int64_t GetTimeMicros() { int64_t now = (boost::posix_time::microsec_clock::universal_time() - diff --git a/src/utiltime.h b/src/utiltime.h index b1447f7df3d5..d34bd2fab243 100644 --- a/src/utiltime.h +++ b/src/utiltime.h @@ -10,9 +10,20 @@ #include #include +/** + * GetTimeMicros() and GetTimeMillis() both return the system time, but in + * different units. GetTime() returns the sytem time in seconds, but also + * supports mocktime, where the time can be specified by the user, eg for + * testing (eg with the setmocktime rpc, or -mocktime argument). + * + * TODO: Rework these functions to be type-safe (so that we don't inadvertently + * compare numbers with different units, or compare a mocktime to system time). + */ + int64_t GetTime(); int64_t GetTimeMillis(); int64_t GetTimeMicros(); +int64_t GetSystemTimeInSeconds(); // Like GetTime(), but not mockable void SetMockTime(int64_t nMockTimeIn); void MilliSleep(int64_t n); From c15cd17d263273b6f90dfca82e31c578f7188507 Mon Sep 17 00:00:00 2001 From: random-zebra Date: Wed, 31 Mar 2021 12:35:20 +0200 Subject: [PATCH 4/7] Return errors from importmulti if complete rescans are not successful >>> adapts bitcoin@e2e2f4c856363bbb0e3b5ba4df225f3754c3db39 +bitcoin@306bd72157f089b962b9c537bbacf710a4158647 --- src/validation.cpp | 5 + src/validation.h | 3 + src/wallet/rpcdump.cpp | 26 +++++- src/wallet/test/wallet_tests.cpp | 152 +++++++++++++++++++++++++++++++ 4 files changed, 184 insertions(+), 2 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 037033e25387..925cdd1342b3 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -4161,6 +4161,11 @@ std::string CBlockFileInfo::ToString() const return strprintf("CBlockFileInfo(blocks=%u, size=%u, heights=%u...%u, time=%s...%s)", nBlocks, nSize, nHeightFirst, nHeightLast, DateTimeStrFormat("%Y-%m-%d", nTimeFirst), DateTimeStrFormat("%Y-%m-%d", nTimeLast)); } +CBlockFileInfo* GetBlockFileInfo(size_t n) +{ + return &vinfoBlockFile.at(n); +} + static const uint64_t MEMPOOL_DUMP_VERSION = 1; bool LoadMempool(CTxMemPool& pool) diff --git a/src/validation.h b/src/validation.h index 3c2b61cebf2a..fc01a0557428 100644 --- a/src/validation.h +++ b/src/validation.h @@ -401,6 +401,9 @@ static const unsigned int REJECT_ALREADY_KNOWN = 0x101; /** Transaction conflicts with a transaction already known */ static const unsigned int REJECT_CONFLICT = 0x102; +/** Get block file info entry for one block file */ +CBlockFileInfo* GetBlockFileInfo(size_t n); + /** Dump the mempool to disk. */ bool DumpMempool(const CTxMemPool& pool); diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp index 6511d5f0c0dd..36eb118a3c64 100644 --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -1030,11 +1030,33 @@ UniValue importmulti(const JSONRPCRequest& mainRequest) if (fRescan && fRunScan && requests.size() && nLowestTimestamp <= chainActive.Tip()->GetBlockTimeMax()) { CBlockIndex* pindex = nLowestTimestamp > minimumTimestamp ? chainActive.FindEarliestAtLeast(std::max(nLowestTimestamp - TIMESTAMP_WINDOW, 0)) : chainActive.Genesis(); - + CBlockIndex* scannedRange = nullptr; if (pindex) { - pwalletMain->ScanForWalletTransactions(pindex, nullptr, true); + scannedRange = pwalletMain->ScanForWalletTransactions(pindex, nullptr, true); pwalletMain->ReacceptWalletTransactions(); } + + if (!scannedRange || scannedRange->nHeight > pindex->nHeight) { + const std::vector& results = response.getValues(); + response.clear(); + response.setArray(); + size_t i = 0; + for (const UniValue& request : requests.getValues()) { + // If key creation date is within the successfully scanned + // range, or if the import result already has an error set, let + // the result stand unmodified. Otherwise replace the result + // with an error message. + if (GetImportTimestamp(request, now) - TIMESTAMP_WINDOW >= scannedRange->GetBlockTimeMax() || results.at(i).exists("error")) { + response.push_back(results.at(i)); + } else { + UniValue result = UniValue(UniValue::VOBJ); + result.pushKV("success", UniValue(false)); + result.pushKV("error", JSONRPCError(RPC_MISC_ERROR, strprintf("Failed to rescan before time %d, transactions may be missing.", scannedRange->GetBlockTimeMax()))); + response.push_back(std::move(result)); + } + ++i; + } + } } return response; diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 276cef991098..50782e8705b6 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -6,6 +6,7 @@ #include "wallet/test/wallet_test_fixture.h" #include "consensus/merkle.h" +#include "rpc/server.h" #include "txmempool.h" #include "validation.h" #include "wallet/wallet.h" @@ -15,6 +16,12 @@ #include #include +#include + +extern UniValue importmulti(const JSONRPCRequest& request); +extern UniValue dumpwallet(const JSONRPCRequest& request); +extern UniValue importwallet(const JSONRPCRequest& request); + // how many times to run all the tests to have a chance to catch errors that only show up with particular random shuffles #define RUN_TESTS 100 @@ -306,6 +313,151 @@ BOOST_AUTO_TEST_CASE(coin_selection_tests) empty_wallet(); } +static void AddKey(CWallet& wallet, const CKey& key) +{ + LOCK(wallet.cs_wallet); + wallet.AddKeyPubKey(key, key.GetPubKey()); +} + +BOOST_FIXTURE_TEST_CASE(rescan, TestChain100Setup) +{ + // Cap last block file size, and mine new block in a new block file. + CBlockIndex* const nullBlock = nullptr; + CBlockIndex* oldTip = chainActive.Tip(); + GetBlockFileInfo(oldTip->GetBlockPos().nFile)->nSize = MAX_BLOCKFILE_SIZE; + CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())); + CBlockIndex* newTip = chainActive.Tip(); + + LOCK(cs_main); + + // Verify ScanForWalletTransactions picks up transactions in both the old + // and new block files. + { + CWallet wallet; + WITH_LOCK(wallet.cs_wallet, wallet.SetLastBlockProcessed(newTip); ); + AddKey(wallet, coinbaseKey); + BOOST_CHECK_EQUAL(nullBlock, wallet.ScanForWalletTransactions(oldTip, nullptr)); + BOOST_CHECK_EQUAL(wallet.GetImmatureBalance(), 500 * COIN); + } + + // !TODO: Prune the older block file. + /* + PruneOneBlockFile(oldTip->GetBlockPos().nFile); + UnlinkPrunedFiles({oldTip->GetBlockPos().nFile}); + + // Verify ScanForWalletTransactions only picks transactions in the new block + // file. + { + CWallet wallet; + AddKey(wallet, coinbaseKey); + BOOST_CHECK_EQUAL(oldTip, wallet.ScanForWalletTransactions(oldTip, nullptr)); + BOOST_CHECK_EQUAL(wallet.GetImmatureBalance(), 50 * COIN); + } + */ + + // Verify importmulti RPC returns failure for a key whose creation time is + // before the missing block, and success for a key whose creation time is + // after. + { + /* currently failing due to off-by-one bug fixed in btc#10403 + CWallet wallet; + WITH_LOCK(wallet.cs_wallet, wallet.SetLastBlockProcessed(newTip); ); + CWallet *backup = ::pwalletMain; + ::pwalletMain = &wallet; + UniValue keys; + keys.setArray(); + UniValue key; + key.setObject(); + key.pushKV("scriptPubKey", HexStr(GetScriptForRawPubKey(coinbaseKey.GetPubKey()))); + key.pushKV("timestamp", 0); + key.pushKV("internal", UniValue(true)); + keys.push_back(key); + key.clear(); + key.setObject(); + CKey futureKey; + futureKey.MakeNewKey(true); + key.pushKV("scriptPubKey", HexStr(GetScriptForRawPubKey(futureKey.GetPubKey()))); + key.pushKV("timestamp", newTip->GetBlockTimeMax() + TIMESTAMP_WINDOW + 1); + key.pushKV("internal", UniValue(true)); + keys.push_back(key); + JSONRPCRequest request; + request.params.setArray(); + request.params.push_back(keys); + + UniValue response = importmulti(request); + BOOST_CHECK_EQUAL(response.write(), + strprintf("[{\"success\":false,\"error\":{\"code\":-1,\"message\":\"Rescan failed for key with creation " + "timestamp %d. There was an error reading a block from time %d, which is after or within %d " + "seconds of key creation, and could contain transactions pertaining to the key. As a result, " + "transactions and coins using this key may not appear in the wallet. This error could be caused " + "by pruning or data corruption (see bitcoind log for details) and could be dealt with by " + "downloading and rescanning the relevant blocks (see -reindex and -rescan " + "options).\"}},{\"success\":true}]", + 0, oldTip->GetBlockTimeMax(), TIMESTAMP_WINDOW)); + ::pwalletMain = backup; + */ + } +} + +// Verify importwallet RPC starts rescan at earliest block with timestamp +// greater or equal than key birthday. Previously there was a bug where +// importwallet RPC would start the scan at the latest block with timestamp less +// than or equal to key birthday. +BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup) +{ + CWallet *pwalletMainBackup = ::pwalletMain; + // Create one block + const int64_t BLOCK_TIME = chainActive.Tip()->GetBlockTimeMax() + 15; + SetMockTime(BLOCK_TIME); + coinbaseTxns.emplace_back(*CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())).vtx[0]); + + // Set key birthday to block time increased by the timestamp window, so + // rescan will start at the block time. + const int64_t KEY_TIME = BLOCK_TIME + TIMESTAMP_WINDOW; + SetMockTime(KEY_TIME); + coinbaseTxns.emplace_back(*CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())).vtx[0]); + + // Import key into wallet and call dumpwallet to create backup file. + { + CWallet wallet; + { + LOCK(wallet.cs_wallet); + wallet.mapKeyMetadata[coinbaseKey.GetPubKey().GetID()].nCreateTime = KEY_TIME; + wallet.AddKeyPubKey(coinbaseKey, coinbaseKey.GetPubKey()); + } + + JSONRPCRequest request; + request.params.setArray(); + request.params.push_back((pathTemp / "wallet.backup").string()); + ::pwalletMain = &wallet; + ::dumpwallet(request); + } + + // Call importwallet RPC and verify all blocks with timestamps >= BLOCK_TIME + // were scanned, and no prior blocks were scanned. + { + CWallet wallet; + + JSONRPCRequest request; + request.params.setArray(); + request.params.push_back((pathTemp / "wallet.backup").string()); + ::pwalletMain = &wallet; + ::importwallet(request); + + LOCK(wallet.cs_wallet); + BOOST_CHECK_EQUAL(wallet.mapWallet.size(), 2); + BOOST_CHECK_EQUAL(coinbaseTxns.size(), 102); + for (size_t i = 0; i < coinbaseTxns.size(); ++i) { + bool found = wallet.GetWalletTx(coinbaseTxns[i].GetHash()); + bool expected = i >= 100; + BOOST_CHECK_EQUAL(found, expected); + } + } + + SetMockTime(0); + ::pwalletMain = pwalletMainBackup; +} + void removeTxFromMempool(CWalletTx& wtx) { LOCK(mempool.cs); From e50dc0ad1f82ab7bf01d5647c636eb5cecbcba94 Mon Sep 17 00:00:00 2001 From: random-zebra Date: Wed, 31 Mar 2021 21:23:46 +0200 Subject: [PATCH 5/7] Fix importmulti failure to return rescan errors >>> adapts bitcoin@4d2d6045a4784d576d56299244b9f76a5909904b --- src/wallet/rpcdump.cpp | 18 +++++++++++++----- src/wallet/test/wallet_tests.cpp | 13 ++----------- 2 files changed, 15 insertions(+), 16 deletions(-) diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp index 36eb118a3c64..d739f91e6615 100644 --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -1030,13 +1030,13 @@ UniValue importmulti(const JSONRPCRequest& mainRequest) if (fRescan && fRunScan && requests.size() && nLowestTimestamp <= chainActive.Tip()->GetBlockTimeMax()) { CBlockIndex* pindex = nLowestTimestamp > minimumTimestamp ? chainActive.FindEarliestAtLeast(std::max(nLowestTimestamp - TIMESTAMP_WINDOW, 0)) : chainActive.Genesis(); - CBlockIndex* scannedRange = nullptr; + CBlockIndex* scanFailed = nullptr; if (pindex) { - scannedRange = pwalletMain->ScanForWalletTransactions(pindex, nullptr, true); + scanFailed = pwalletMain->ScanForWalletTransactions(pindex, nullptr, true); pwalletMain->ReacceptWalletTransactions(); } - if (!scannedRange || scannedRange->nHeight > pindex->nHeight) { + if (scanFailed) { const std::vector& results = response.getValues(); response.clear(); response.setArray(); @@ -1046,12 +1046,20 @@ UniValue importmulti(const JSONRPCRequest& mainRequest) // range, or if the import result already has an error set, let // the result stand unmodified. Otherwise replace the result // with an error message. - if (GetImportTimestamp(request, now) - TIMESTAMP_WINDOW >= scannedRange->GetBlockTimeMax() || results.at(i).exists("error")) { + if (GetImportTimestamp(request, now) - TIMESTAMP_WINDOW >= scanFailed->GetBlockTimeMax() || results.at(i).exists("error")) { response.push_back(results.at(i)); } else { UniValue result = UniValue(UniValue::VOBJ); result.pushKV("success", UniValue(false)); - result.pushKV("error", JSONRPCError(RPC_MISC_ERROR, strprintf("Failed to rescan before time %d, transactions may be missing.", scannedRange->GetBlockTimeMax()))); + result.pushKV("error", JSONRPCError(RPC_MISC_ERROR, + strprintf("Rescan failed for key with creation timestamp %d. There was an error reading a " + "block from time %d, which is after or within %d seconds of key creation, and " + "could contain transactions pertaining to the key. As a result, transactions " + "and coins using this key may not appear in the wallet. This error could be " + "caused by pruning or data corruption (see pivxd log for details) and could " + "be dealt with by downloading and rescanning the relevant blocks (see -reindex " + "and -rescan options).", + GetImportTimestamp(request, now), scanFailed->GetBlockTimeMax(), TIMESTAMP_WINDOW))); response.push_back(std::move(result)); } ++i; diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 50782e8705b6..b2709e095ffd 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -359,7 +359,6 @@ BOOST_FIXTURE_TEST_CASE(rescan, TestChain100Setup) // before the missing block, and success for a key whose creation time is // after. { - /* currently failing due to off-by-one bug fixed in btc#10403 CWallet wallet; WITH_LOCK(wallet.cs_wallet, wallet.SetLastBlockProcessed(newTip); ); CWallet *backup = ::pwalletMain; @@ -385,17 +384,9 @@ BOOST_FIXTURE_TEST_CASE(rescan, TestChain100Setup) request.params.push_back(keys); UniValue response = importmulti(request); - BOOST_CHECK_EQUAL(response.write(), - strprintf("[{\"success\":false,\"error\":{\"code\":-1,\"message\":\"Rescan failed for key with creation " - "timestamp %d. There was an error reading a block from time %d, which is after or within %d " - "seconds of key creation, and could contain transactions pertaining to the key. As a result, " - "transactions and coins using this key may not appear in the wallet. This error could be caused " - "by pruning or data corruption (see bitcoind log for details) and could be dealt with by " - "downloading and rescanning the relevant blocks (see -reindex and -rescan " - "options).\"}},{\"success\":true}]", - 0, oldTip->GetBlockTimeMax(), TIMESTAMP_WINDOW)); + // !TODO: after pruning, check that the rescan for the first key fails. + BOOST_CHECK_EQUAL(response.write(), "[{\"success\":true},{\"success\":true}]"); ::pwalletMain = backup; - */ } } From 623e146611ada232789a510e8fac0f53ca2ce8ff Mon Sep 17 00:00:00 2001 From: random-zebra Date: Tue, 30 Mar 2021 14:04:19 +0200 Subject: [PATCH 6/7] [Tests] Fix and re-enable wallet_import_rescan --- test/functional/test_runner.py | 39 +++++++++++++------------ test/functional/wallet_import_rescan.py | 27 ++++++++++------- 2 files changed, 37 insertions(+), 29 deletions(-) diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index b7608937940d..e6e234dae490 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -65,40 +65,41 @@ 'wallet_hd.py', # ~ 300 sec 'wallet_zapwallettxes.py', # ~ 300 sec 'p2p_time_offset.py', # ~ 267 sec - 'rpc_fundrawtransaction.py', # ~ 260 sec - 'mining_pos_coldStaking.py', # ~ 215 sec - 'wallet_abandonconflict.py', # ~ 212 sec - 'feature_logging.py', # ~ 200 sec - 'feature_blockindexstats.py', # ~ 197 sec - 'rpc_rawtransaction.py', # ~ 193 sec - 'wallet_keypool_topup.py', # ~ 174 sec - 'wallet_txn_doublespend.py --mineblock', # ~ 157 sec - 'wallet_txn_clone.py --mineblock', # ~ 157 sec + 'rpc_fundrawtransaction.py', # ~ 227 sec + 'mining_pos_coldStaking.py', # ~ 220 sec + 'wallet_import_rescan.py', # ~ 204 sec + 'feature_logging.py', # ~ 195 sec + 'wallet_abandonconflict.py', # ~ 188 sec + 'feature_blockindexstats.py', # ~ 167 sec 'wallet_importmulti.py', # ~ 157 sec - 'rpc_spork.py', # ~ 156 sec - 'interface_rest.py', # ~ 154 sec - 'feature_proxy.py', # ~ 143 sec - 'feature_uacomment.py', # ~ 130 sec + 'wallet_keypool_topup.py', # ~ 153 sec + 'rpc_spork.py', # ~ 144 sec + 'wallet_txn_doublespend.py --mineblock', # ~ 143 sec + 'wallet_txn_clone.py --mineblock', # ~ 143 sec + 'feature_proxy.py', # ~ 138 sec + 'rpc_rawtransaction.py', # ~ 134 sec 'mining_pos_reorg.py', # ~ 128 sec - 'wallet_upgrade.py', # ~ 124 sec - 'wallet_import_stakingaddress.py', # ~ 123 sec + 'feature_uacomment.py', # ~ 125 sec + 'interface_rest.py', # ~ 120 sec # vv Tests less than 2m vv + 'wallet_upgrade.py', # ~ 119 sec 'p2p_disconnect_ban.py', # ~ 118 sec - 'wallet_listreceivedby.py', # ~ 117 sec - 'mining_pos_fakestake.py', # ~ 113 sec 'feature_reindex.py', # ~ 110 sec 'interface_http.py', # ~ 105 sec 'feature_blockhashcache.py', # ~ 100 sec 'wallet_listtransactions.py', # ~ 97 sec + 'wallet_listreceivedby.py', # ~ 94 sec + 'mining_pos_fakestake.py', # ~ 94 sec 'mempool_reorg.py', # ~ 92 sec + 'interface_zmq.py', # ~ 90 sec 'wallet_encryption.py', # ~ 89 sec + 'wallet_import_stakingaddress.py', # ~ 86 sec 'wallet_keypool.py', # ~ 88 sec 'wallet_dump.py', # ~ 83 sec 'rpc_net.py', # ~ 83 sec 'rpc_bip38.py', # ~ 82 sec #'rpc_deprecated.py', # ~ 80 sec (disabled for now, no deprecated RPC commands to test) - 'interface_zmq.py', # ~ 95 sec 'interface_bitcoin_cli.py', # ~ 80 sec 'mempool_packages.py', # ~ 63 sec @@ -125,7 +126,6 @@ # 'mining_prioritisetransaction.py', # 'p2p_invalid_block.py', # 'p2p_invalid_tx.py', - # 'wallet_import_rescan.py', # 'mining_basic.py', # 'wallet_bumpfee.py', # 'wallet_listsinceblock.py', @@ -213,6 +213,7 @@ 'sapling_wallet_nullifiers.py', 'sapling_mempool.py', 'wallet_importmulti.py', + 'wallet_import_rescan.py', ] # Place the lists with the longest tests (on average) first diff --git a/test/functional/wallet_import_rescan.py b/test/functional/wallet_import_rescan.py index 86c8bd7544c4..eb3a0199fee0 100755 --- a/test/functional/wallet_import_rescan.py +++ b/test/functional/wallet_import_rescan.py @@ -7,11 +7,11 @@ Test rescan behavior of importaddress, importpubkey, importprivkey, and importmulti RPCs with different types of keys and rescan options. -In the first part of the test, node 0 creates an address for each type of -import RPC call and sends BTC to it. Then other nodes import the addresses, -and the test makes listtransactions and getbalance calls to confirm that the -importing node either did or did not execute rescans picking up the send -transactions. +In the first part of the test, node 1 creates an address for each type of +import RPC call and node 0 sends BTC to it. Then other nodes import the +addresses, and the test makes listtransactions and getbalance calls to confirm +that the importing node either did or did not execute rescans picking up the +send transactions. In the second part of the test, node 0 sends more BTC to each address, and the test makes more listtransactions and getbalance calls to confirm that the @@ -20,13 +20,18 @@ """ from test_framework.test_framework import PivxTestFramework -from test_framework.util import (assert_raises_rpc_error, connect_nodes, assert_equal, set_node_times) +from test_framework.util import ( + assert_raises_rpc_error, + connect_nodes, + assert_equal, + set_node_times +) import collections import enum import itertools -Call = enum.Enum("Call", "single") +Call = enum.Enum("Call", "single multi") Data = enum.Enum("Data", "address pub priv") Rescan = enum.Enum("Rescan", "no yes late_timestamp") @@ -69,7 +74,7 @@ def do_import(self, timestamp): def check(self, txid=None, amount=None, confirmations=None): """Verify that listreceivedbyaddress returns return expected values.""" - addresses = self.node.listreceivedbyaddress(0, True, self.address['address']) + addresses = self.node.listreceivedbyaddress(0, True, True, self.address['address']) if self.expected_txs: assert_equal(len(addresses[0]["txids"]), self.expected_txs) @@ -91,7 +96,8 @@ def check(self, txid=None, amount=None, confirmations=None): # List of Variants for each way a key or address could be imported. -IMPORT_VARIANTS = [Variant(*variants) for variants in itertools.product(Call, Data, Rescan, (False, True))] +# !TODO: import on pruned node +IMPORT_VARIANTS = [Variant(*variants) for variants in itertools.product(Call, Data, Rescan, [False])] # List of nodes to import keys to. Half the nodes will have pruning disabled, # half will have it enabled. Different nodes will be used for imports that are @@ -125,7 +131,8 @@ def run_test(self): # Create one transaction on node 0 with a unique amount for # each possible type of wallet import RPC. for i, variant in enumerate(IMPORT_VARIANTS): - variant.address = self.nodes[1].validateaddress(self.nodes[1].getnewaddress()) + variant.label = "label {} {}".format(i, variant) + variant.address = self.nodes[1].validateaddress(self.nodes[1].getnewaddress(variant.label)) variant.key = self.nodes[1].dumpprivkey(variant.address["address"]) variant.initial_amount = 10 - (i + 1) / 4.0 variant.initial_txid = self.nodes[0].sendtoaddress(variant.address["address"], variant.initial_amount) From 465d1eb58cecdd127dcc5e8c2f38b9cc6ff2222a Mon Sep 17 00:00:00 2001 From: random-zebra Date: Thu, 8 Apr 2021 13:42:30 +0200 Subject: [PATCH 7/7] [BUG] Wallet: skip non-spendable outputs in CWallet::StakeableCoins --- src/wallet/wallet.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 12aee3896e53..2770f8e4eb8e 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2668,7 +2668,7 @@ bool CWallet::StakeableCoins(std::vector* pCoins) false, false); // fIncludeLocked - if (!res.available) continue; + if (!res.available || !res.spendable) continue; // found valid coin if (!pCoins) return true;