wallet: refactor ScanForWalletTransactions

Eunovo commented at 11:36 am on February 26, 2026: contributor

This PR refactors the wallet scanning logic for improved readability and maintainability, and prepares the scanning logic for the changes in #34400.

ScanForWalletTransactions is expanded into a class, and the main scanning loop is simplified by delegating some logic to member functions. No behaviour change is introduced in this PR.

DrahtBot added the label Wallet on Feb 26, 2026

DrahtBot commented at 11:37 am on February 26, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34400 (wallet: parallel fast rescan (approx 5x speed up with 16 threads) by Eunovo)
#33008 (wallet: support bip388 policy with external signer by Sjors)
#30343 (wallet, logging: Replace WalletLogPrintf() with LogInfo() by ryanofsky)
#27865 (wallet: Track no-longer-spendable TXOs separately by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

wallet: move scanning logic to wallet/scan.cpp

Move rescan logic to new class to allow the scanning loop
to be simplified by delegating some logic to member
functions in future commits.

CWallet::ScanForWalletTransactions impl is moved to scan.cpp
to prevent circular dependency of the form
"wallet/wallet -> wallet/scan -> wallet/wallet".

a2930eceaa

wallet/scan: extract block filtering logic to ShouldFetchBlock method

Pure extraction of block filter matching logic into a dedicated method.
This prepares for further refactoring of the scanning loop.

bf772392fd

wallet/scan: extract block scanning logic to ScanBlock method

Pure extraction of block transaction processing logic into a dedicated
method. This isolates the logic for fetching blocks and syncing their
transactions.

c7f8368246

wallet/scan: extract block iteration logic into ReadNextBlock

Extract block reading logic into ReadNextBlock method which returns
std::optional<pair<hash, height>>.
Introduce m_next_block member to track iteration state, consolidating
the loop termination logic into ReadNextBlock.

c89eb55312

wallet/scan: extract progress tracking helper methods

Extract UpdateProgress, UpdateTipIfChanged, and LogScanStatus methods.
Move progress tracking variables to class members. This simplifies the
main scanning loop and groups related progress tracking logic together.

f2c2949f16

wallet/scan: simplify ChainScanner::Scan main loop

Extract ProcessBlock method and simplify the main scanning loop. This
final refactoring demonstrates the clean separation of concerns with
each helper method handling a specific aspect of the scanning process.

8f5ef8d369

Eunovo force-pushed on Feb 26, 2026

DrahtBot added the label CI failed on Feb 26, 2026

DrahtBot removed the label CI failed on Feb 26, 2026

in src/wallet/rpc/transactions.cpp:902 in a2930eceaa

895@@ -896,14 +896,14 @@ RPCHelpMan rescanblockchain()
896         CHECK_NONFATAL(pwallet->chain().findAncestorByHeight(pwallet->GetLastBlockHash(), start_height, FoundBlock().hash(start_block)));
897     }
898 
899-    CWallet::ScanResult result =
900+    ScanResult result =
901         pwallet->ScanForWalletTransactions(start_block, start_height, stop_height, reserver, /*fUpdate=*/true, /*save_progress=*/false);
902     switch (result.status) {
903-    case CWallet::ScanResult::SUCCESS:
904+    case ScanResult::SUCCESS:

polespinasa commented at 6:31 pm on February 27, 2026:

in a2930eceaac1356e7bdc0438dee315f5b18377ce

Before the scan result was in the CWallet namespace CWallet::ScanResult::SUCCESS, but now in some places we have wallet::ScanResult::SUCCESS and in others there’s only ScanResult::SUCCESS. Maybe try to keep the wallet:: namespace on all occurrences to keep consistency between files.

in src/wallet/wallet.h:308 in a2930eceaa

301@@ -302,7 +302,11 @@ struct CRecipient
302     bool fSubtractFeeFromAmount;
303 };
304 
305-class WalletRescanReserver; //forward declarations for ScanForWalletTransactions/RescanFromTime
306+//forward declarations for ScanForWalletTransactions/RescanFromTime
307+class WalletRescanReserver;
308+class ChainScanner;
309+struct ScanResult;

polespinasa commented at 6:14 pm on March 2, 2026:

in a2930eceaac1356e7bdc0438dee315f5b18377ce

ScanResult and WalletRescanReserver are already declared later in the file. Why can’t we move the “real” declaration up here?

in src/wallet/scan.cpp:203 in c89eb55312

198+    ScanResult result;
199+    double progress_begin = chain.guessVerificationProgress(m_start_block);
200+    double progress_end = chain.guessVerificationProgress(end_hash);
201+    double progress_current = progress_begin;
202+    int block_height = m_start_height;
203+    m_next_block = {{m_start_block, m_start_height}};

polespinasa commented at 6:50 pm on March 2, 2026:

In c89eb55312d m_next_block is unused? I think this line can be deleted

in src/wallet/scan.h:35 in c7f8368246

31@@ -32,6 +32,7 @@ class ChainScanner {
32     bool m_save_progress;
33 
34     bool ShouldFetchBlock(const std::unique_ptr<FastWalletRescanFilter>& filter, const uint256& block_hash, int block_height);
35+    bool ScanBlock(const uint256& block_hash, int block_height, bool save_progress);

polespinasa commented at 7:48 pm on March 2, 2026:

in c7f8368246bf7c474e65c2df07c2aaea7f468089

I think save_progress is not a good name here anymore if it is the combination of m_save_progress and next_interval.

in src/wallet/scan.cpp:205 in c7f8368246

220-                    break;
221-                }
222-                for (size_t posInBlock = 0; posInBlock < block.vtx.size(); ++posInBlock) {
223-                    m_wallet.SyncTransaction(block.vtx[posInBlock], TxStateConfirmed{block_hash, block_height, static_cast<int>(posInBlock)}, m_fUpdate, /*rescanning_old_block=*/true);
224-                }
225+            if (!block_still_active) {

polespinasa commented at 7:58 pm on March 2, 2026:

in c7f8368246bf7c474e65c2df07c2aaea7f468089

Taking this out of ScanBlock and if (!block.isNull(){...} opens the door to a block being null and still call SyncTransaction(...).

This is a behavior change and not only code extraction.

I am not familiar enough to know with this part of the code to know if this matter or if this is ok. Pointing it out just in case:)

polespinasa commented at 9:25 pm on March 2, 2026: member

Currently reviewing c89eb55312def8bee3f6d7ba645c58cd1bc1f393

Will continue, left a few comments

wallet: refactor ScanForWalletTransactions #34681

Reviews

Conflicts