wallet: refactor ScanForWalletTransactions #34681

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:

#bitcoin-core/gui/872 (Menu action to export a watchonly wallet by achow101)
#35123 (wallet: remove outdated arguments from wallet scanning methods by rkrux)
#32489 (wallet: Add exportwatchonlywallet RPC to export a watchonly version of a wallet by achow101)
#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.

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 outdated

 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.

Eunovo commented at 9:25 AM on March 5, 2026:

Maybe try to keep the wallet:: namespace on all occurrences to keep consistency between files.

I'm not sure this is a good reason to use wallet:: in code already within the wallet namespace.

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?

Eunovo commented at 9:56 AM on March 5, 2026:

WalletRescanReserver needs to be forward declared because it has a member that is a reference to the CWallet instance. On the otherhand, ScanResult does not need to be forward declared. I have corrected this in the latest push. Thank you.

polespinasa commented at 1:32 PM on March 5, 2026:

Oh right sorry, I didn't mean WalletRescanReserver. You can remove ChainScanner forward declaration as it is only used as a friend class inside CWallet. Tested and it compiles correctly.

Eunovo commented at 1:59 PM on March 5, 2026:

Fixed in the latest push.

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

 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~

Nevermind, I didn't understand the code :)

Am I right that m_next_block is used to keep track of the next block to be scanned in ReadNextBlock? And here the next block is the current block because the scanning just started and it is the first block to be scanned.

Eunovo commented at 9:27 AM on March 5, 2026:

You are right.

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

  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.

Eunovo commented at 9:39 AM on March 5, 2026:

I disagree. The parameter name represents exactly what it sounds like; it tells ScanBlock to save scan progress if set to true.

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:)

Eunovo commented at 9:46 AM on March 5, 2026:

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

ScanBlock has an if (block.IsNull()) return false; check before calling SyncTransaction

Eunovo commented at 9:50 AM on March 5, 2026:

One thing that does change here is that previously, we fetch the block before checking block_still_active, and I decided to change it so that we don't fetch the block if !block_still_active. I don't see a reason to fetch the block if it's not active, and AFAICT, it doesn't change the result.

polespinasa commented at 1:12 PM on March 5, 2026:

ScanBlock has an if (block.IsNull()) return false; check before calling SyncTransaction

you're right, I have missread the code, though the {...} under the if was regarding the if, didn't see the return 😅

polespinasa commented at 1:13 PM on March 5, 2026:

I decided to change it so that we don't fetch the block if !block_still_active

this makes much sense!

Thanks you can resolve this

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

Currently reviewing c89eb55312def8bee3f6d7ba645c58cd1bc1f393

Will continue, left a few comments

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

polespinasa commented at 1:41 PM on March 4, 2026:

In 2eba8426f711c12f31af244c9964a071452a36b8

Not a change added by this PR but a nit to clean a bit the code.

nit: I think end_hash can be removed. There shouldn't be any need to duplicate tip_hash.

By using both in the code seems like they should have different values but they don't. Using only tip_hash should be fine.

Eunovo commented at 1:50 PM on March 5, 2026:

If max_height is set, end_hash will be the hash of the block at that height; otherwise, it will be the current tip_hash.

Eunovo force-pushed on Mar 5, 2026

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".

16b1ca5efa

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.

ddb5a07eb1

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.

4612fb4b2f

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.

0371771e7b

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.

30fc35fa72

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.

e98e3a76eb

Eunovo force-pushed on Apr 14, 2026

rkrux commented at 2:14 PM on April 22, 2026: contributor

Initial code review at e98e3a76ebbbccbe29d7c4a5f9a36be8e8969a22

I feel RescanFromTime method should also be moved out in this PR because it's sort of a wrapper over ScanForWalletTransactions.

Consider the last two commits from my fork's branch that builds over this branch:

Eunovo commented at 9:29 AM on April 23, 2026: contributor

I feel RescanFromTime method should also be moved out in this PR because it's sort of a wrapper over ScanForWalletTransaction

Thanks for the review. I'm not quite convinced that RescanFromTime should be moved out. ScanForWalletTransaction was substantially large, which created the need to split its logic into multiple functions that could be in one class, ChainScanner. RescanFromTime doesn't have the same problem. The ScanForWalletTransaction function itself remains part of CWallet; its implementation was only moved to resolve a circular dependency issue.

I think it's fine that RescanFromTime remains a member function of CWallet. It could be argued that moving it out as you show in https://github.com/rkrux/bitcoin/commit/f2ece2bcda062e13c694be178c21aeb8bca5506f makes its api less ideal, as it now requires an extra parameter.

in src/wallet/scan.cpp:41 in 16b1ca5efa

  36 | + */
  37 | +ScanResult CWallet::ScanForWalletTransactions(const uint256& start_block, int start_height, std::optional<int> max_height, const WalletRescanReserver& reserver, bool fUpdate, bool save_progress)
  38 | +{
  39 | +    ChainScanner scanner{*this, reserver, start_block, start_height, max_height, fUpdate, save_progress};
  40 | +    return scanner.Scan();
  41 | +}

rkrux commented at 11:50 AM on April 23, 2026:

In 16b1ca5efa16b8ea0dcc9464bef285401c5203d4 "wallet: move scanning logic to wallet/scan.cpp" and re: #34681 (comment) -

I don't like this ScanForWalletTransactions implementation at all. I didn't share all my thoughts earlier, following is what I think about the wallet structure now that we are adding wallet/scan.

The ScanForWalletTransaction function itself remains part of CWallet; its implementation was only moved to resolve a circular dependency issue.

Fair point but this in itself is a sign that there is no need for ScanForWalletTransactions anymore. Combine that with the fact it's just a wrapper over ChainScanner, the case for its removal becomes stronger.

However, I do realise it's not easy to remove ScanForWalletTransactions straightaway because of its usage inside RescanFromTime and more importantly AttachChain, which is used a few times within wallet/wallet. The only way I see ScanForWalletTransactions getting removed is by removing its usages gradually:

Removing AttachChain from wallet/wallet is a big task and goes outside the scope of this PR.
RescanFromTime is fairly straightforward that can be done much quicker and that's why I suggested moving it within wallet/scan in this PR.

It could be argued that moving it out as you show in https://github.com/rkrux/bitcoin/commit/f2ece2bcda062e13c694be178c21aeb8bca5506f makes its api less ideal, as it now requires an extra parameter.

The second commit in my fork branch that I shared can be avoided because it addresses a different point of removing it from CWallet but the first commit is a simple move.

Also, now that wallet/scan is being added that uses wallet/wallet and moves few scanning objects such as ScanForWalletTransactions (& corresponding ChainScanner) and FastWalletRescanFilter, I don't see a strong reason for why we need to have a partial split of the chain scanning stuff in wallet/wallet (such as WalletRescanReserver, ScanResult) and the remaining in wallet/scan. Moving all the chain scanning stuff to wallet/scan would remove the scope of circular dependency altogether and provide a clearer separation of concerns.

But as mentioned earlier, it would be a bigger refactor that goes outside the scope of this PR. I'm quite excited by the chain scanning speedup we'd get in #34400 and don't want to derail this PR by suggesting that broader refactor.

That said, I'm also fine with not moving RescanFromTime in this PR because it's a caller of ScanForWalletTransactions and its movement is not fully required in this PR.

I will review the remaining commits soon.

Eunovo commented at 12:35 PM on April 23, 2026:

There are more scanning-related member functions like AbortRescan(), ScanningProgress() and ScanningDuration(), etc. It might be a good idea to move everything to ChainScanner and create a ChainScanner scanner member in CWallet and expose a const ChainScanner& getScanner() function that can be used to access all scanning-related functions and state.