By returning ScanResult from RescanFromTime and reporting the effectuality of AbortScan.
And consolidate rpc-level error handling across RescanFromTime and
ScanForWalletTransactions.
Note this changes the rescanblockchain scan failure error from
RPC_MISC_ERROR to RPC_WALLET_ERROR, which seems more appropriate and matches the
behavior from the rpcdump methods.
This follows up on #13076.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, 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.
552@@ -557,7 +553,7 @@ UniValue importwallet(const JSONRPCRequest& request)
553 throw JSONRPCError(RPC_WALLET_ERROR, "Wallet is currently rescanning. Abort existing rescan or wait.");
554 }
555
556- int64_t nTimeBegin = 0;
557+ int64_t nTimeBegin = TIMESTAMP_MIN;
This is a dead store since nTimeBegin is unconditionally written to on L573.
Perhaps you could drop the dead store and instead move the nTimeBegin = chainActive.Tip()->GetBlockTime(); to the line after LOCK(pwallet->cs_wallet);?
nTimeBegin is used outside that block for the rescan, line 646.
I prefer to leave this as it seems harmless. Alternative is leaving it uninitialized.
What I meant is that the current code is:
0int64_t nTimeBegin = TIMESTAMP_MIN;
1{
2 nTimeBegin = chainActive.Tip()->GetBlockTime();
3}
4RescanWallet(*pwallet, reserver, nTimeBegin, false /* update */);
Thus the first assignment is redundant since it is overwritten in all possible execution paths.
There are no execution paths where dropping the first assignment would result in an uninitialised variable being read.
Suggested alternative:
0int64_t nTimeBegin;
1{
2 nTimeBegin = chainActive.Tip()->GetBlockTime();
3}
4RescanWallet(*pwallet, reserver, nTimeBegin, false /* update */);
Rebased and substantially refocused this PR to:
ScanResult from RescanFromTimeScanResult::FailedTime to report the best non-failing time - beneficial also because most callers are uninterested in the timeIsScanning andIsAbortingRescan from CWallet, as all scan/abort status is now returned as results from specific operations.
looks like a nice refactor
utACK 4b4cf7cc28347b2d537be89a412093dbd62ea83e
IsScanning as it’s needed for d3e8458365
not sure why github tends to screw up range-diffs sometimes but here’s the last one:
0$ git range-diff 4b4cf7c~2..4b4cf7c 469117d~2..469117d
01: 99a72728a6 = 1: c82f6c7806 Return ScanResult from CWallet::RescanFromTime
12: 4b4cf7cc28 ! 2: 469117d55c Make scan / abort status private to CWallet
2@@ -2,7 +2,7 @@
3
4 Make scan / abort status private to CWallet
5
6- Drop CWallet::IsAbortingRescan and IsScanning by reporting the abort
7+ Drop CWallet::IsAbortingRescan by reporting the abort
8 effectuality from CWallet::AbortScan.
9
10 diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp
11@@ -54,8 +54,7 @@
12 */
13 - void AbortRescan() { fAbortRescan = true; }
14 - bool IsAbortingRescan() { return fAbortRescan; }
15-- bool IsScanning() { return fScanningWallet; }
16 + bool AbortRescan();
17-
18- /**
19- * keystore implementation
20+ bool IsScanning() { return fScanningWallet; }
21+ int64_t ScanningDuration() const { return fScanningWallet ? GetTimeMillis() - m_scanning_start : 0; }
22+ double ScanningProgress() const { return fScanningWallet ? (double) m_scanning_progress : 0; }
re-utACK 469117d55c3601c75883d8ca7ff5e76f9f60da60
1571@@ -1572,7 +1572,7 @@ void CWalletTx::GetAmounts(std::list<COutputEntry>& listReceived,
1572 * @return Earliest timestamp that could be successfully scanned from. Timestamp
1573 * returned will be higher than startTime if relevant blocks could not be read.
int64_t) anymore, this Doxygen comment needs to be updated.
1424- if (pwallet->IsAbortingRescan()) {
1425+ if (result.status == CWallet::ScanResult::USER_ABORT) {
1426 throw JSONRPCError(RPC_MISC_ERROR, "Rescan aborted by user.");
1427- }
1428- if (scannedTime > nLowestTimestamp) {
1429+ } else if (result.status == CWallet::ScanResult::FAILURE) {
switch (result.status) versus if‘ing over enums (e.g. it gives compiler warnings when new fields are added).
827@@ -828,11 +828,12 @@ class CWallet final : public WalletStorage, private interfaces::Chain::Notificat
828 void UnlockAllCoins() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
829 void ListLockedCoins(std::vector<COutPoint>& vOutpts) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
830
831- /*
832- * Rescan abort properties
833+ /**
834+ * Abort wallet rescan
835+ * @return true if the abort was effectual, i.e. the wallet was scanning
*) line before @ return. (See “Coding Style (Doxygen-compatible comments)” in developer.nodes.md)
899+ * Scan the block chain (starting in start_block) for transactions
900+ * from or to us. If fUpdate is true, found transactions that already
901+ * exist in the wallet will be updated.
902+ *
903+ * @param[in] start_block Scan starting block. If block is not on the active
904+ * chain, the scan will return SUCCESS immediately.
ORPHAN_CHAIN ScanResult, even if this is handled the same way as SUCCESS everywhere right now.
1429+ } else if (result.status == CWallet::ScanResult::FAILURE) {
1430+ int64_t scannedTime = TIMESTAMP_MIN;
1431+ if (!pwallet->chain().findBlock(result.last_failed_block, nullptr /* block */, nullptr /* time */, &scannedTime)) {
1432+ throw std::logic_error("ScanForWalletTransactions returned invalid block hash");
1433+ }
1434+ scannedTime += TIMESTAMP_WINDOW + 1;
scannedTime was set to the last block time + TIMESTAMP_WINDOW + 1. Won’t this result in missed scans below, as scannedTime will be low?
I don’t see the difference in behavior - see the original location for comparison: https://github.com/bitcoin/bitcoin/pull/14942/commits/f059899fe3ca88204d9dda4f83c4f8c4548743ea#diff-b2bb174788c7409b671c46ccc86034bdL1607-L1611
Could you call out what you’re referring to any more explicitly, in terms of what has changed?
scannedTime on L#1427. It looks identical.
168@@ -169,6 +169,13 @@ std::shared_ptr<CWallet> LoadWallet(interfaces::Chain& chain, const std::string&
169 return LoadWallet(chain, WalletLocation(name), error, warnings);
170 }
171
172+bool CWallet::AbortRescan()
It’s a bit convoluted, but I think it’s handled gracefully: if fScanningWallet is set to false as fAbortRescan is being set to true (i.e. at the end of a scan), there is no issue as ScanForWalletTransactions sets fAbortRescan to false at its start.
BTW this behavior is unchanged - here’s the prior implementation: https://github.com/bitcoin/bitcoin/pull/14942/files#diff-522490d83dce5375d423b23886e4125eL217-R219
Lookup the failed time just in the case it's needed.
Drop CWallet::IsAbortingRescan by reporting the abort
effectuality from CWallet::AbortScan.
And move to the header.
215@@ -214,9 +216,7 @@ UniValue abortrescan(const JSONRPCRequest& request)
216 },
217 }.Check(request);
218
219- if (!pwallet->IsScanning() || pwallet->IsAbortingRescan()) return false;
220- pwallet->AbortRescan();
221- return true;
222+ return pwallet->AbortRescan();
🐙 This pull request conflicts with the target branch and needs rebase.
