Part of #34400
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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process. A summary of reviews will appear here.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
exportwatchonlywallet RPC to export a watchonly version of a wallet 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.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
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:
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.
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.
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;
in a2930eceaac1356e7bdc0438dee315f5b18377ce
ScanResult and WalletRescanReserver are already declared later in the file.
Why can't we move the "real" declaration up here?
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.
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.
Fixed in the latest push.
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}};
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.
You are right.
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);
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.
I disagree. The parameter name represents exactly what it sounds like; it tells ScanBlock to save scan progress if set to true.
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) {
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:)
Taking this out of
ScanBlockandif (!block.isNull(){...}opens the door to a block being null and still callSyncTransaction(...).
ScanBlock has an if (block.IsNull()) return false; check before calling SyncTransaction
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.
ScanBlockhas anif (block.IsNull()) return false;check before callingSyncTransaction
you're right, I have missread the code, though the {...} under the if was regarding the if, didn't see the return 😅
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
Currently reviewing c89eb55312def8bee3f6d7ba645c58cd1bc1f393
Will continue, left a few comments
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.
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.
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".
Pure extraction of block filter matching logic into a dedicated method.
This prepares for further refactoring of the scanning loop.
Pure extraction of block transaction processing logic into a dedicated
method. This isolates the logic for fetching blocks and syncing their
transactions.
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.
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.
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.