wallet, rpc: add UTXO set check and incremental rescan to importdescriptors #33392
Conversation
Add node::FindCoinsByScript and wire it into the node interface as Node::getCoinsByScript(), then implement the DB-backed scan that iterates the on-disk UTXO DB with a CCoinsViewCursor.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33392. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. LLM Linter (✨ experimental)Possible typos and grammar issues:
drahtbot_id_5_m |
musaHaruna
force-pushed
the
feature/scan-utxoset-balance-check
branch
from
845bd13 to
93bacca
Compare
|
It seems kind of weird to me to add this as an option to |
I have not looked into making it an option for Just thinking on a high level and somewhat naively because am a new contributor with little knowledge about the codebase, what if we can atomatically cross check the balance by scanning the utxoset in Thanks for the suggestion. I really appreciate it. |
Yes, I have looked into adding the option to I don’t yet know the full impact on performance, but I’m thinking of making the trade-offs very clear in the RPC docs so users can decide whether to enable it. |
Cool, this is pretty much what I had in mind, I guess I would slightly prefer that full rescan isn't started by default and rather the user receives a clear hint that there might be some funds missing and if they want to make sure to get them they should run a full rescan, basically just like what you are doing now in the return from
Maybe run some benchmarks on the utxo set scan and full rescan and add them to the PR here. I don't have a good feeling for what the relation is and this might influence what reviewers think is better concerning running a full rescan automatically or just returning a warning/hint from Another idea concerning performance: It seems likely that if the birth date is wrong the user might be off by just a few days or weeks, rather than they have a wallet with the satoshi coins. So instead of a full rescan from the start of the chain the rescan could move backwards from the originally supplied birth date and scan the chain in 1000 (or so) block increments and it could stop once the balance matches the one from the utxo set scan. A nice side effect of this is that it means the rescan is also pruning compatible (for as many blocks that are available) but I guess this could be also achieved by starting from pruneheight instead of 0 if pruning is enabled. It's something you need to keep in mind either way. This would also need benchmarks but I can't imagine moving backwards with somewhat large increments would make the process much slower. And ideally the process would exit early in the most common scenario. |
I will run some benchmarks on both utxo set scan and full rescan, update the whole PR to use the new approach you suggested i.e to add the feature on
Just to make sure we’re on the same page — my understanding is that the idea is to: Use chunked backward rescans starting from the supplied birthdate (e.g. 1000-block increments), and stop as soon as the wallet balance matches the UTXO-set scan. This handles the common case where the birthdate is only off by a few days or weeks, so we don’t need to rescan the entire chain. At the same time, respect the pruneheight as the lower bound if pruning is enabled. That means we only scan back as far as blocks are available, and if the discrepancy still isn’t resolved at that point, we’d warn the user that a reindex is required. |
musaHaruna
changed the title
Right, so for example if the supplied birthdate was blockheight
Yeah, there would be some kind of safe abort if the approach for walking backwards runs into unavailable blocks, just like |
luke-jr
left a comment
luke-jr
left a comment
There was a problem hiding this comment.
I agree getbalance* is the wrong place for this kind of check.
But it also will fail to detect incorrect birthdates if the TXOs are spent, so it can't be relied on either...
src/wallet/rpc/coins.cpp
Outdated
| { | ||
| {"dummy", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "Remains for backward compatibility. Must be excluded or set to \"*\"."}, | ||
| {"minconf", RPCArg::Type::NUM, RPCArg::Default{0}, "Only include transactions confirmed at least this many times."}, | ||
| {"include_watchonly", RPCArg::Type::BOOL, RPCArg::Default{false}, "No longer used"}, | ||
| {"avoid_reuse", RPCArg::Type::BOOL, RPCArg::Default{true}, "(only available if avoid_reuse wallet flag is set) Do not include balance in dirty outputs; addresses are considered dirty if they have previously been used in a transaction."}, | ||
| {"scan_utxoset", RPCArg::Type::BOOL, RPCArg::Default{false}, "If true, scan the UTXO set and return scanned UTXO balance alongside the trusted balance."}, |
There was a problem hiding this comment.
This shouldn't be a positional parameter, at least.
musaHaruna
force-pushed
the
feature/scan-utxoset-balance-check
branch
3 times, most recently
from
3e5ead7 to
1c23d97
Compare
musaHaruna
changed the title
musaHaruna
force-pushed
the
feature/scan-utxoset-balance-check
branch
2 times, most recently
from
e45f79b to
82a222b
Compare
src/wallet/rpc/backup.cpp
Outdated
|
|
||
| CAmount utxo_scanned_balance = GetWalletUTXOSetBalance(wallet); | ||
|
|
||
| if (utxo_scanned_balance != bal.m_mine_trusted) { |
There was a problem hiding this comment.
In 82a222b "rpc: extend importdescriptors with UTXO check and incremental rescan"
The lack of nesting in the if blocks makes the code harder to read.
diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp
index 4146a65876..cf8269b2ef 100644
--- a/src/wallet/rpc/backup.cpp
+++ b/src/wallet/rpc/backup.cpp
@@ -507,7 +507,6 @@ RPCHelpMan importdescriptors()
bool have_prune_boundary = false;
int64_t min_trial_start_time = 0;
-
UniValue utxo_diff_obj;
if (do_scan_utxoset && rescan) {
@@ -516,53 +515,53 @@ RPCHelpMan importdescriptors()
CAmount utxo_scanned_balance = GetWalletUTXOSetBalance(wallet);
- if (utxo_scanned_balance != bal.m_mine_trusted) {
- // Incremental-rescan chunking parameters
- const int chunk_blocks = 1000;
- const int64_t avg_block_time = 600; // seconds per block (approx)
+ if (utxo_scanned_balance != bal.m_mine_trusted) {
+ // Incremental-rescan chunking parameters
+ const int chunk_blocks = 1000;
+ const int64_t avg_block_time = 600; // seconds per block (approx)
- // Get tip time and height
- int64_t tip_time = 0;
- int tip_height = 0;
- {
- LOCK(pwallet->cs_wallet);
- CHECK_NONFATAL(pwallet->chain().findBlock(pwallet->GetLastBlockHash(), FoundBlock().time(tip_time).height(tip_height)));
- }
+ // Get tip time and height
+ int64_t tip_time = 0;
+ int tip_height = 0;
+ {
+ LOCK(pwallet->cs_wallet);
+ CHECK_NONFATAL(pwallet->chain().findBlock(pwallet->GetLastBlockHash(), FoundBlock().time(tip_time).height(tip_height)));
+ }
- // If pruned, compute an approximate earliest start time based on prune height
- bool is_pruned = pwallet->chain().havePruned();
- std::optional<int> prune_height_opt = pwallet->chain().getPruneHeight();
- have_prune_boundary = false;
- min_trial_start_time = 0;
- if (is_pruned && prune_height_opt.has_value()) {
- const int prune_height = prune_height_opt.value();
- int64_t blocks_diff = tip_height - prune_height;
- if (blocks_diff < 0) blocks_diff = 0;
- int64_t prune_time_est = tip_time - blocks_diff * avg_block_time;
- if (prune_time_est < 0) prune_time_est = 0;
- min_trial_start_time = prune_time_est;
- have_prune_boundary = true;
- }
+ // If pruned, compute an approximate earliest start time based on prune height
+ bool is_pruned = pwallet->chain().havePruned();
+ std::optional<int> prune_height_opt = pwallet->chain().getPruneHeight();
+ have_prune_boundary = false;
+ min_trial_start_time = 0;
+ if (is_pruned && prune_height_opt.has_value()) {
+ const int prune_height = prune_height_opt.value();
+ int64_t blocks_diff = tip_height - prune_height;
+ if (blocks_diff < 0) blocks_diff = 0;
+ int64_t prune_time_est = tip_time - blocks_diff * avg_block_time;
+ if (prune_time_est < 0) prune_time_est = 0;
+ min_trial_start_time = prune_time_est;
+ have_prune_boundary = true;
+ }
- // Attempt incremental rescans using the helper defined above.
- int out_chunks_tried = 0;
- int64_t out_lowest_ts = 0;
- UniValue early = IncrementalRescansNonOverlap(wallet, tip_time, tip_height, chunk_blocks, avg_block_time, have_prune_boundary,
- min_trial_start_time, utxo_scanned_balance, reserver, response, out_chunks_tried, out_lowest_ts);
+ // Attempt incremental rescans using the helper defined above.
+ int out_chunks_tried = 0;
+ int64_t out_lowest_ts = 0;
+ UniValue early = IncrementalRescansNonOverlap(wallet, tip_time, tip_height, chunk_blocks, avg_block_time, have_prune_boundary,
+ min_trial_start_time, utxo_scanned_balance, reserver, response, out_chunks_tried, out_lowest_ts);
- if (!early.isNull()) {
- // Matched and response already annotated by helper.
- return early;
- }
+ if (!early.isNull()) {
+ // Matched and response already annotated by helper.
+ return early;
+ }
- if (have_prune_boundary) {
- // Set the fallback rescan start to the prune boundary (instead of 0)
- lowest_timestamp = min_trial_start_time;
- } else {
- // Non-pruned node: incremental attempts scanned back to timestamp 0 (genesis)
- lowest_timestamp = 0;
+ if (have_prune_boundary) {
+ // Set the fallback rescan start to the prune boundary (instead of 0)
+ lowest_timestamp = min_trial_start_time;
+ } else {
+ // Non-pruned node: incremental attempts scanned back to timestamp 0 (genesis)
+ lowest_timestamp = 0;
+ }
}
- }
}
|
|
||
| UniValue utxo_diff_obj; | ||
|
|
||
| if (do_scan_utxoset && rescan) { |
There was a problem hiding this comment.
In 82a222b "rpc: extend importdescriptors with UTXO check and incremental rescan"
Why is the UTXO set balance check (and the corresponding incremental block scan incase of balance mismatch) done before the usual rescan that's supposed to be done upon a successful descriptor import? Should this not be done after the usual rescan to find any transactions in the older blocks that might have been missed in the rescan because of incorrect timestamp added by the user?
There was a problem hiding this comment.
The UTXO set check is performed before the main rescan to attempt incremental reconciliation of missing funds in small block chunks, potentially reducing the amount of blocks that need to be rescanned in the case of a large blockchain. Doing it after the main rescan would generally be redundant, because the rescan will already incorporate transactions missed due to incorrect timestamps, and any discrepancy with the UTXO set would likely have been resolved. The current order allows us to detect and fix large balance mismatches earlier in the RPC, without necessarily scanning the entire chain from genesis immediately.
src/wallet/rpc/backup.cpp
Outdated
| }, | ||
| }, | ||
| RPCArgOptions{.oneline_description="requests"}}, | ||
| {"scan_utxoset", RPCArg::Type::BOOL, RPCArg::Default{false}, "If true, scans the UTXO set balance and compare with wallet balance and triggers incremental rescan if discrepency is found."} |
There was a problem hiding this comment.
In fa3ac73 "wallet/rpc: add scan_utxoset arg & docs for importdescriptors"
s/"scan_utxoset"/"verify"
From a user's POV, this feels to me more like a verification step that the wallet might do if set by the user. Let's just call it that to keep it simple for the user?
There was a problem hiding this comment.
Yeah I agree I have changed it to verify_balance I thought just verify is till a bit vague
| # Blank wallets don't have a birth time | ||
| assert 'birthtime' not in wallet_no_scan.getwalletinfo() | ||
| assert 'birthtime' not in wallet_with_scan.getwalletinfo() |
There was a problem hiding this comment.
In 1ed236e "test: add functional test for importdescriptors scan_utxo flag"
What's the reason to assert this at only this stage?
There was a problem hiding this comment.
This assertion ensures that the newly created watch-only wallets start from a completely clean state with no prior transaction history or birthtime. That way, we can be sure that any transactions discovered after the import come solely from the descriptor import and the UTXO scan, not from pre-existing wallet data.
src/wallet/rpc/backup.cpp
Outdated
| UniValue early = IncrementalRescansNonOverlap(wallet, tip_time, tip_height, chunk_blocks, avg_block_time, have_prune_boundary, | ||
| min_trial_start_time, utxo_scanned_balance, reserver, response, out_chunks_tried, out_lowest_ts); | ||
|
|
||
| if (!early.isNull()) { |
There was a problem hiding this comment.
In 82a222b "rpc: extend importdescriptors with UTXO check and incremental rescan"
Shouldn't the timestamp of the imported descriptors be updated as well in this case to avoid data inconsistency?
There was a problem hiding this comment.
Yeah, I think it should be, I will look into how I can do that, am open to suggestions on how that can be done.
src/wallet/rpc/backup.cpp
Outdated
| }}, | ||
| {RPCResult::Type::OBJ, "info", /*optional=*/true, "Optional informational fields. When present, this object will contain details about incremental rescans (examples below).", | ||
| { | ||
| {RPCResult::Type::STR, "utxo_check", /*optional=*/true, "Status of the UTXO check. Example values: 'matched' (wallet DB matches UTXO set)."}, |
There was a problem hiding this comment.
In fa3ac73 "wallet/rpc: add scan_utxoset arg & docs for importdescriptors"
This string property seems unnecessary if it is supposed to have only one value all the time.
| if (!pwallet) return UniValue::VNULL; | ||
| CWallet& wallet{*pwallet}; | ||
|
|
||
| // Make sure the results are valid at least up to the most recent block |
There was a problem hiding this comment.
In fa3ac73 "wallet/rpc: add scan_utxoset arg & docs for importdescriptors"
Why is this comment cut midway?
There was a problem hiding this comment.
I think this is part of backupwallet RPC and I don't think I touch the backupwallet rpc codenthe entire PR.
src/wallet/rpc/backup.cpp
Outdated
| {RPCResult::Type::NUM, "scanned_chunks", /*optional=*/true, "If incremental rescans were performed, the number of chunks scanned before a match was found."}, | ||
| {RPCResult::Type::NUM, "scanned_blocks", /*optional=*/true, "If incremental rescans were performed, the approximate number of blocks scanned (scanned_chunks * chunk_size)."}, |
There was a problem hiding this comment.
In fa3ac73 "wallet/rpc: add scan_utxoset arg & docs for importdescriptors"
I think these don't need to be marked optional when the info object is already marked so. Unless these properties appear optionally inside the info object that I think is not the case.
src/wallet/receive.cpp
Outdated
| bool has_privkeys = spkm->HavePrivateKeys(); | ||
| for (const auto& script : spkm->GetScriptPubKeys()) { | ||
| if (has_privkeys) { | ||
| output_scripts_mine.emplace(script); | ||
| } else { | ||
| output_scripts_watchonly.emplace(script); | ||
| } |
There was a problem hiding this comment.
In ac06ddb "wallet: add GetWalletUTXOSetBalance to calculate balance from UTXO set"
I don't think the bifurcation between output_scripts_mine and output_scripts_watchonly needs to be done anymore because the watch only property in the descriptor wallets is at the wallet level now. Either the whole wallet will be watch-only or all of it will be not. Ref: #32618
| bool have_prune_boundary = false; | ||
| int64_t min_trial_start_time = 0; | ||
|
|
||
|
|
There was a problem hiding this comment.
In 82a222b "rpc: extend importdescriptors with UTXO check and incremental rescan"
Can early return from the function here in case rescan is false.
There was a problem hiding this comment.
Fixed.
Thank you so much for reviewing the code!!!
Introduce a new helper `GetWalletUTXOSetBalance(const CWallet&)` that derives the wallet’s balance directly from the UTXO set rather than from the wallet’s transaction history. Usefull for verifying wallet balance correctness against the chainstate without requiring a full rescan.
Add a new optional boolean RPC argument `scan_utxoset` to importdescriptors and document the response fields used when the scan_utxoset check is enabled.
…ange Add a new overload of CWallet::RescanFromTime that accepts an optional endTime parameter. When provided, the method finds the approximate end height and calls ScanForWalletTransactions with a bounded [start, end] range. This makes incremental or windowed rescans possible while preserving existing behavior when no endTime is specified.
musaHaruna
force-pushed
the
feature/scan-utxoset-balance-check
branch
from
1ed236e to
68d4cfb
Compare
Extend the importdescriptors RPC with an optional `scan_utxoset` parameter. When enabled, the wallet balance is compared against a balance derived from the UTXO set. If a discrepancy is found, the wallet attempts incremental, non-overlapping rescans in fixed-size block chunks until the balances match, allowing for faster recovery in many cases. When a match is detected, the RPC returns early with an `info` object describing the UTXO check and rescan progress. If no match is found, the code falls back to the existing full rescan behavior starting from the earliest descriptor timestamp (or the prune boundary if applicable). Responses are updated to include optional `info` fields with `utxo_check`, `scanned_chunks`, and `scanned_blocks` metadata.
Add a functional test that verifies `importdescriptors` behavior with and without `scan_utxoset`, ensuring wallets correctly detect UTXOs and transactions depending on the timestamp used. The test covers both `timestamp="now"` (no history vs. UTXO set scan) and accurate historical timestamps (both imports discover full history), and also asserts returned metadata fields (`success`, `utxo_check`, `scanned_chunks`, `scanned_blocks`).
musaHaruna
force-pushed
the
feature/scan-utxoset-balance-check
branch
from
68d4cfb to
3e42097
Compare
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
|
🐙 This pull request conflicts with the target branch and needs rebase. |
5 participants
Fixes #28898
When importing descriptors, users may accidentally provide an incorrect birthdate (timestamp). This can cause the wallet to miss relevant historical transactions, leading to incorrect or incomplete balances. Currently, the wallet only relies on rescans starting from the provided timestamp.
This PR extends the importdescriptors RPC with a new optional argument:
scan_utxoset(bool, default=false):If enabled, the wallet will compare its calculated trusted balance against UTXO set balance (by generating the
scriptpukeysof the wallet and comparing it with the chains UTXO setscriptpubkeysto get the accurate balance belonging to the wallet and comparing it with the wallet trusted balance).If the balances match, import continues as normal.
If a discrepancy is detected, the wallet will attempt incremental rescans in chunks of recent blocks until the missing history is found. If the wallet is pruned, incremental rescans will not go earlier than the prune boundary.
Additional information is returned in the RPC response under an
"info"object whenscan_utxosetis used, such as:utxo_check: whether the UTXO set matched the wallet balancescanned_chunks:number of incremental rescan chunks attemptedscanned_blocks: approximate number of blocks scanned during incremental rescansThis helps detect and fix balance mismatches caused by wrong descriptor timestamps.
A big thanks to fjahr for suggesting this approach comment