Skip to content

Commit

Permalink
Return a status enum from ScanForWalletTransactions
Browse files Browse the repository at this point in the history 
Summary:
```
Return the failed block as an out var.

This clarifies the outcome as the prior return value could
be null due to user abort or failure.
```

Partial backport of core [[bitcoin/bitcoin#13076 | PR13076]]:
bitcoin/bitcoin@3002d6c

Depends on D5561.

Test Plan:
  ninja all check check-functional

Reviewers: #bitcoin_abc, deadalnix

Reviewed By: #bitcoin_abc, deadalnix

Subscribers: deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D5562
  • Loading branch information
@Empact @Fabcien
Empact authored and Fabcien committed Mar 26, 2020
1 parent 4e8e7bf commit 14fb215
Show file tree
Hide file tree
Showing 6 changed files with 88 additions and 48 deletions.
9 changes: 7 additions & 2 deletions src/qt/test/wallettests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -130,8 +130,13 @@ void TestGUI() {
auto locked_chain = wallet->chain().lock();
WalletRescanReserver reserver(wallet.get());
reserver.reserve();
wallet->ScanForWalletTransactions(::ChainActive().Genesis(), nullptr,
reserver, true);
const CBlockIndex *const null_block = nullptr;
const CBlockIndex *stop_block;
QCOMPARE(wallet->ScanForWalletTransactions(
::ChainActive().Genesis(), nullptr, reserver, stop_block,
true /* fUpdate */),
CWallet::ScanResult::SUCCESS);
QCOMPARE(stop_block, null_block);
}
wallet->SetBroadcastTransactions(true);

Expand Down
10 changes: 10 additions & 0 deletions src/test/setup_common.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
#include <scheduler.h>
#include <util/system.h>

#include <type_traits>

/**
* Version of Boost::test prior to 1.64 have issues when dealing with nullptr_t.
* In order to work around this, we ensure that the null pointers are typed in a
Expand All @@ -24,6 +26,14 @@
*/
#define NULLPTR(T) static_cast<T *>(nullptr)

// Enable BOOST_CHECK_EQUAL for enum class types
template <typename T>
std::ostream &operator<<(
typename std::enable_if<std::is_enum<T>::value, std::ostream>::type &stream,
const T &e) {
return stream << static_cast<typename std::underlying_type<T>::type>(e);
}

/**
* This global and the helpers that use it are not thread-safe.
*
Expand Down
24 changes: 13 additions & 11 deletions src/wallet/rpcwallet.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4092,18 +4092,20 @@ UniValue rescanblockchain(const Config &config, const JSONRPCRequest &request) {
}
}

const CBlockIndex *stopBlock = pwallet->ScanForWalletTransactions(
pindexStart, pindexStop, reserver, true);
if (!stopBlock) {
if (pwallet->IsAbortingRescan()) {
const CBlockIndex *stopBlock;
CWallet::ScanResult result = pwallet->ScanForWalletTransactions(
pindexStart, pindexStop, reserver, stopBlock, true);
switch (result) {
case CWallet::ScanResult::SUCCESS:
stopBlock = pindexStop ? pindexStop : pChainTip;
break;
case CWallet::ScanResult::FAILURE:
throw JSONRPCError(
RPC_MISC_ERROR,
"Rescan failed. Potentially corrupted data files.");
case CWallet::ScanResult::USER_ABORT:
throw JSONRPCError(RPC_MISC_ERROR, "Rescan aborted.");
}
// if we got a nullptr returned, ScanForWalletTransactions did rescan up
// to the requested stopindex
stopBlock = pindexStop ? pindexStop : pChainTip;
} else {
throw JSONRPCError(RPC_MISC_ERROR,
"Rescan failed. Potentially corrupted data files.");
// no default case, so the compiler can warn about missing cases
}
UniValue response(UniValue::VOBJ);
response.pushKV("start_height", pindexStart->nHeight);
Expand Down
25 changes: 18 additions & 7 deletions src/wallet/test/wallet_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ BOOST_FIXTURE_TEST_CASE(rescan, TestChain100Setup) {
auto chain = interfaces::MakeChain();

// Cap last block file size, and mine new block in a new block file.
CBlockIndex *const nullBlock = nullptr;
const CBlockIndex *const null_block = nullptr;
CBlockIndex *oldTip = ::ChainActive().Tip();
GetBlockFileInfo(oldTip->GetBlockPos().nFile)->nSize = MAX_BLOCKFILE_SIZE;
CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey()));
Expand All @@ -54,8 +54,11 @@ BOOST_FIXTURE_TEST_CASE(rescan, TestChain100Setup) {
AddKey(wallet, coinbaseKey);
WalletRescanReserver reserver(&wallet);
reserver.reserve();
BOOST_CHECK_EQUAL(nullBlock, wallet.ScanForWalletTransactions(
oldTip, nullptr, reserver));
const CBlockIndex *stop_block;
BOOST_CHECK_EQUAL(wallet.ScanForWalletTransactions(
oldTip, nullptr, reserver, stop_block),
CWallet::ScanResult::SUCCESS);
BOOST_CHECK_EQUAL(stop_block, null_block);
BOOST_CHECK_EQUAL(wallet.GetImmatureBalance(), 100 * COIN);
}

Expand All @@ -71,8 +74,11 @@ BOOST_FIXTURE_TEST_CASE(rescan, TestChain100Setup) {
AddKey(wallet, coinbaseKey);
WalletRescanReserver reserver(&wallet);
reserver.reserve();
BOOST_CHECK_EQUAL(oldTip, wallet.ScanForWalletTransactions(
oldTip, nullptr, reserver));
const CBlockIndex *stop_block;
BOOST_CHECK_EQUAL(wallet.ScanForWalletTransactions(
oldTip, nullptr, reserver, stop_block),
CWallet::ScanResult::FAILURE);
BOOST_CHECK_EQUAL(oldTip, stop_block);
BOOST_CHECK_EQUAL(wallet.GetImmatureBalance(), 50 * COIN);
}

Expand Down Expand Up @@ -310,8 +316,13 @@ class ListCoinsTestingSetup : public TestChain100Setup {
AddKey(*wallet, coinbaseKey);
WalletRescanReserver reserver(wallet.get());
reserver.reserve();
wallet->ScanForWalletTransactions(::ChainActive().Genesis(), nullptr,
reserver);
const CBlockIndex *const null_block = nullptr;
const CBlockIndex *stop_block;
BOOST_CHECK_EQUAL(
wallet->ScanForWalletTransactions(ChainActive().Genesis(), nullptr,
reserver, stop_block),
CWallet::ScanResult::SUCCESS);
BOOST_CHECK_EQUAL(stop_block, null_block);
}

~ListCoinsTestingSetup() { wallet.reset(); }
Expand Down
56 changes: 33 additions & 23 deletions src/wallet/wallet.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1802,9 +1802,11 @@ int64_t CWallet::RescanFromTime(int64_t startTime,
}

if (startBlock) {
const CBlockIndex *const failedBlock =
ScanForWalletTransactions(startBlock, nullptr, reserver, update);
if (failedBlock) {
const CBlockIndex *failedBlock;
// TODO: this should take into account failure by ScanResult::USER_ABORT
if (ScanResult::FAILURE ==
ScanForWalletTransactions(startBlock, nullptr, reserver,
failedBlock, update)) {
return failedBlock->GetBlockTimeMax() + TIMESTAMP_WINDOW + 1;
}
}
Expand All @@ -1816,20 +1818,24 @@ int64_t CWallet::RescanFromTime(int64_t startTime,
* us. If fUpdate is true, found transactions that already exist in the wallet
* will be updated.
*
* Returns null if scan was successful. Otherwise, if a complete rescan was not
* possible (due to pruning or corruption), returns pointer to the most recent
* block that could not be scanned.
* @param[in] pindexStop if not a nullptr, the scan will stop at this
* block-index
* @param[out] failed_block if FAILURE is returned, will be set to the most
* recent block that could not be scanned, otherwise nullptr.
*
* If pindexStop is not a nullptr, the scan will stop at the block-index
* defined by pindexStop
* @return ScanResult indicating success or failure of the scan. SUCCESS if
* scan was successful. FAILURE if a complete rescan was not possible (due to
* pruning or corruption). USER_ABORT if the rescan was aborted before it
* could complete.
*
* Caller needs to make sure pindexStop (and the optional pindexStart) are on
* the main chain after to the addition of any new keys you want to detect
* @pre Caller needs to make sure pindexStop (and the optional pindexStart) are
* on the main chain after to the addition of any new keys you want to detect
* transactions for.
*/
const CBlockIndex *CWallet::ScanForWalletTransactions(
CWallet::ScanResult CWallet::ScanForWalletTransactions(
const CBlockIndex *const pindexStart, const CBlockIndex *const pindexStop,
const WalletRescanReserver &reserver, bool fUpdate) {
const WalletRescanReserver &reserver, const CBlockIndex *&failed_block,
bool fUpdate) {
int64_t nNow = GetTime();

assert(reserver.isReserved());
Expand All @@ -1838,7 +1844,7 @@ const CBlockIndex *CWallet::ScanForWalletTransactions(
}

const CBlockIndex *pindex = pindexStart;
const CBlockIndex *ret = nullptr;
failed_block = nullptr;

if (pindex) {
WalletLogPrintf("Rescan started from block %d...\n", pindex->nHeight);
Expand Down Expand Up @@ -1893,7 +1899,7 @@ const CBlockIndex *CWallet::ScanForWalletTransactions(
// Abort scan if current block is no longer active, to
// prevent marking transactions as coming from the wrong
// block.
ret = pindex;
failed_block = pindex;
break;
}
for (size_t posInBlock = 0; posInBlock < block.vtx.size();
Expand All @@ -1902,7 +1908,7 @@ const CBlockIndex *CWallet::ScanForWalletTransactions(
fUpdate);
}
} else {
ret = pindex;
failed_block = pindex;
}
if (pindex == pindexStop) {
break;
Expand All @@ -1921,20 +1927,22 @@ const CBlockIndex *CWallet::ScanForWalletTransactions(
}
}

// Hide progress dialog in GUI.
ShowProgress(strprintf("%s " + _("Rescanning..."), GetDisplayName()),
100);
if (pindex && fAbortRescan) {
WalletLogPrintf("Rescan aborted at block %d. Progress=%f\n",
pindex->nHeight, progress_current);
return ScanResult::USER_ABORT;
} else if (pindex && ShutdownRequested()) {
WalletLogPrintf("Rescan interrupted by shutdown request at block "
"%d. Progress=%f\n",
pindex->nHeight, progress_current);
return ScanResult::USER_ABORT;
}

// Hide progress dialog in GUI.
ShowProgress(strprintf("%s " + _("Rescanning..."), GetDisplayName()),
100);
}
return ret;

return failed_block ? ScanResult::FAILURE : ScanResult::SUCCESS;
}

void CWallet::ReacceptWalletTransactions() {
Expand Down Expand Up @@ -4714,13 +4722,15 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(
nStart = GetTimeMillis();
{
WalletRescanReserver reserver(walletInstance.get());
if (!reserver.reserve()) {
const CBlockIndex *stop_block;
if (!reserver.reserve() ||
(ScanResult::SUCCESS !=
walletInstance->ScanForWalletTransactions(
pindexRescan, nullptr, reserver, stop_block, true))) {
InitError(
_("Failed to rescan the wallet during initialization"));
return nullptr;
}
walletInstance->ScanForWalletTransactions(pindexRescan, nullptr,
reserver, true);
}
walletInstance->WalletLogPrintf("Rescan completed in %15dms\n",
GetTimeMillis() - nStart);
Expand Down
12 changes: 7 additions & 5 deletions src/wallet/wallet.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1062,11 +1062,13 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface {
BlockDisconnected(const std::shared_ptr<const CBlock> &pblock) override;
int64_t RescanFromTime(int64_t startTime,
const WalletRescanReserver &reserver, bool update);
const CBlockIndex *
ScanForWalletTransactions(const CBlockIndex *const pindexStart,
const CBlockIndex *const pindexStop,
const WalletRescanReserver &reserver,
bool fUpdate = false);

enum class ScanResult { SUCCESS, FAILURE, USER_ABORT };
ScanResult ScanForWalletTransactions(const CBlockIndex *const pindexStart,
const CBlockIndex *const pindexStop,
const WalletRescanReserver &reserver,
const CBlockIndex *&failed_block,
bool fUpdate = false);
void TransactionRemovedFromMempool(const CTransactionRef &ptx) override;
void ReacceptWalletTransactions();
void ResendWalletTransactions(int64_t nBestBlockTime,
Expand Down

0 comments on commit 14fb215

Please sign in to comment.