wallet: move rescan logic into ChainScanner and wallet/scan #34681

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

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

CWallet previously owned all rescan-related concerns: atomic state variables, the WalletRescanReserver RAII type, RescanFromTime, and the entire block-scanning loop (ScanForWalletTransactions) as a single large member function.

This PR separates those concerns into a dedicated ChainScanner class, introduced in wallet/scan.h and wallet/scan.cpp:

Scan state (fAbortRescan, fScanningWallet, progress, start time, passphrase flag) moves into ChainScanner atomics, exposed through GetScanner().
WalletRescanReserver
RescanFromTime becomes ChainScanner::ScanFromTime, keeping all scan entry points in one place.
ScanForWalletTransactions is replaced by ChainScanner::Scan and decomposed into focused helpers: ShouldFetchBlock, ScanBlock, ReadNextBlock, UpdateProgress, UpdateTipIfChanged, and ProcessBlock.

CWallet retains only a ChainScanner member and a GetScanner() accessor. Callers that previously reached into CWallet for scan state now go through GetScanner().

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.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34681.

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/700 (Dialog for allowing the user to choose the change output when bumping a tx by achow101)
#35377 (wallet: Allow importing of descriptors without private keys when the wallet has the private keys by achow101)
#35011 (ci, iwyu: Fix warnings in src/script and treat them as errors by BrandonOdiwuor)
#34861 (wallet: Add importdescriptors interface by polespinasa)
#33392 (wallet, rpc: add UTXO set check and incremental rescan to importdescriptors by musaHaruna)
#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

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.

DrahtBot added the label Needs rebase on May 13, 2026

wallet: introduce ChainScanner as a CWallet member

Move scan state atomics (abort, scanning, passphrase, start time,
progress) into ChainScanner and expose it via GetScanner(). All
callers use GetScanner().Scan() directly.

The newly added `m_scanner` is an incomplete type so CWallet's
constructor and destructor is moved into wallet.cpp where
the type is complete.

This change introduces a new circular dependency of the form
"wallet/scan -> wallet/wallet -> wallet/scan" which is added to
`EXPECTED_CIRCULAR_DEPENDENCIES` because the alternative solution
was unneccessarily complicated.

d4862c4577

wallet/scan: move RescanFromTime to ChainScanner as ScanFromTime

Callers now reach this via GetScanner().ScanFromTime() rather than
a CWallet method, keeping all scan logic in ChainScanner.

f58f428020

wallet/scan: move WalletRescanReserver to scan files 1743e14505

wallet/scan: extract block filter matching to ShouldFetchBlock 94cbf98b7b

wallet/scan: extract block scanning logic to ScanBlock cdbd071254

wallet/scan: extract ReadNextBlock e7874923fe

wallet/scan: extract progress tracking helpers from `ChainScanner::Scan` 610fc4e6c6

wallet/scan: extract ProcessBlock and simplify Scan loop e3258c921f

Eunovo force-pushed on May 27, 2026

DrahtBot removed the label Needs rebase on May 27, 2026

Eunovo renamed this:
~~wallet: refactor ScanForWalletTransactions~~
wallet: move rescan logic into ChainScanner and wallet/scan
on May 27, 2026

Eunovo commented at 10:59 AM on May 27, 2026: contributor

@rkrux @polespinasa This PR has been heavily reworked since your last look following the comment in #34681 (review). All scanning-related state and logic have been moved to wallet/scan, and CWallet now has a ChainScanner member.

rkrux commented at 2:04 PM on May 28, 2026: contributor

Great that more of the scanning logic could be extracted out from the main wallet file, I will take a look.

DrahtBot added the label Needs rebase on May 29, 2026

DrahtBot commented at 11:36 AM on May 29, 2026: contributor

🐙 This pull request conflicts with the target branch and needs rebase.