Fix ScanForWalletTransactions to return an enum indicating scan result: success / failure / user_abort #13076
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1613,8 +1613,9 @@ int64_t CWallet::RescanFromTime(int64_t startTime, const WalletRescanReserver& r | |
| } | ||
|
|
||
| if (startBlock) { | ||
| const CBlockIndex *failedBlock, *stop_block; | ||
| // TODO: this should take into account failure by ScanResult::USER_ABORT | ||
| if (ScanResult::FAILURE == ScanForWalletTransactions(startBlock, nullptr, reserver, failedBlock, stop_block, update)) { | ||
| return failedBlock->GetBlockTimeMax() + TIMESTAMP_WINDOW + 1; | ||
| } | ||
| } | ||
|
|
@@ -1626,18 +1627,22 @@ int64_t CWallet::RescanFromTime(int64_t startTime, const WalletRescanReserver& r | |
| * from or to us. If fUpdate is true, found transactions that already | ||
| * exist in the wallet will be updated. | ||
| * | ||
| * @param[in] pindexStop if not a nullptr, the scan will stop at this block-index | ||
| * @param[out] failed_block if FAILURE is returned, the most recent block | ||
| * that could not be scanned, otherwise nullptr | ||
| * @param[out] stop_block the most recent block that could be scanned, | ||
| * otherwise nullptr if no block could be scanned | ||
| * | ||
| * @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. | ||
| * | ||
| * @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. | ||
| */ | ||
| CWallet::ScanResult CWallet::ScanForWalletTransactions(const CBlockIndex* const pindexStart, const CBlockIndex* const pindexStop, const WalletRescanReserver& reserver, const CBlockIndex*& failed_block, const CBlockIndex*& stop_block, bool fUpdate) | ||
| { | ||
| int64_t nNow = GetTime(); | ||
| const CChainParams& chainParams = Params(); | ||
|
|
@@ -1647,8 +1652,8 @@ CBlockIndex* CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, CBlock | |
| assert(pindexStop->nHeight >= pindexStart->nHeight); | ||
| } | ||
|
|
||
| const CBlockIndex* pindex = pindexStart; | ||
| failed_block = nullptr; | ||
|
|
||
| if (pindex) WalletLogPrintf("Rescan started from block %d...\n", pindex->nHeight); | ||
|
|
||
|
|
@@ -1669,8 +1674,7 @@ CBlockIndex* CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, CBlock | |
| } | ||
| } | ||
| double progress_current = progress_begin; | ||
| while (pindex && !fAbortRescan && !ShutdownRequested()) { | ||
| if (pindex->nHeight % 100 == 0 && progress_end - progress_begin > 0.0) { | ||
| ShowProgress(strprintf("%s " + _("Rescanning..."), GetDisplayName()), std::max(1, std::min(99, (int)((progress_current - progress_begin) / (progress_end - progress_begin) * 100)))); | ||
| } | ||
|
|
@@ -1686,14 +1690,17 @@ CBlockIndex* CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, CBlock | |
| if (pindex && !chainActive.Contains(pindex)) { | ||
| // Abort scan if current block is no longer active, to prevent | ||
| // marking transactions as coming from the wrong block. | ||
| failed_block = pindex; | ||
|
There was a problem hiding this comment. I think you might want to drop this line and just return true here, to fix the regression described here: #12275 (comment). There was a problem hiding this comment. Not sure about that - my inclination is to maintain stricter checking at the risk of spurious failures over less strict checking at the risk of silent failure. How about opening a separate PR for that so it can be considered separately? |
||
| break; | ||
| } | ||
| for (size_t posInBlock = 0; posInBlock < block.vtx.size(); ++posInBlock) { | ||
| SyncTransaction(block.vtx[posInBlock], pindex, posInBlock, fUpdate); | ||
| } | ||
| // scan succeeded, record block as most recent successfully scanned | ||
| stop_block = pindex; | ||
| } else { | ||
| // could not scan block, keep scanning but record this block as the most recent failure | ||
| failed_block = pindex; | ||
| } | ||
| if (pindex == pindexStop) { | ||
| break; | ||
|
|
@@ -1709,14 +1716,20 @@ CBlockIndex* CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, CBlock | |
| } | ||
| } | ||
| } | ||
| ShowProgress(strprintf("%s " + _("Rescanning..."), GetDisplayName()), 100); // hide progress dialog in GUI | ||
| 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; | ||
| } | ||
| } | ||
| if (failed_block) { | ||
| return ScanResult::FAILURE; | ||
| } else { | ||
| return ScanResult::SUCCESS; | ||
| } | ||
| } | ||
|
|
||
| void CWallet::ReacceptWalletTransactions() | ||
|
|
@@ -4166,11 +4179,11 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain, | |
| nStart = GetTimeMillis(); | ||
| { | ||
| WalletRescanReserver reserver(walletInstance.get()); | ||
| const CBlockIndex *stop_block, *failed_block; | ||
| if (!reserver.reserve() || (ScanResult::SUCCESS != walletInstance->ScanForWalletTransactions(pindexRescan, nullptr, reserver, failed_block, stop_block, true))) { | ||
| InitError(_("Failed to rescan the wallet during initialization")); | ||
| return nullptr; | ||
| } | ||
| } | ||
| walletInstance->WalletLogPrintf("Rescan completed in %15dms\n", GetTimeMillis() - nStart); | ||
| walletInstance->ChainStateFlushed(chainActive.GetLocator()); | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code is not setting
stop_blocknull here, which seems unintended and could lead to uninitialized variables. Would be good to fix this or add documentation if it was done intentionally.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, right, the code does contradict the comments on line 1634. Will open a PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#14957