wallet: Make scan / abort status private to CWallet #14942

Empact commented at 12:31 AM on December 13, 2018: member

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.

fanquake added the label Wallet on Dec 13, 2018

DrahtBot commented at 2:02 AM on December 13, 2018: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#18278 (interfaces: Describe and follow some code conventions by ryanofsky)
#17954 (wallet: Remove calls to Chain::Lock methods by ryanofsky)
#10102 ([experimental] Multiprocess bitcoin by ryanofsky)

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.

Empact renamed this:
~~Wallet: Return a ScanResult from CWallet::RescanFromTime~~
wallet: Return a ScanResult from CWallet::RescanFromTime
on Dec 14, 2018

in src/wallet/rpcdump.cpp:556 in a71986a023 outdated

 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;

practicalswift commented at 9:49 AM on January 5, 2019:

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);?

Empact commented at 8:53 AM on January 7, 2019:

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.

practicalswift commented at 9:07 AM on January 7, 2019:

What I meant is that the current code is:

int64_t nTimeBegin = TIMESTAMP_MIN;
{
    nTimeBegin = chainActive.Tip()->GetBlockTime();
}
RescanWallet(*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:

int64_t nTimeBegin;
{
    nTimeBegin = chainActive.Tip()->GetBlockTime();
}
RescanWallet(*pwallet, reserver, nTimeBegin, false /* update */);

Empact commented at 9:29 AM on January 7, 2019:

Gotcha, I'd rather leave as is, as the change is pretty well orthogonal.

DrahtBot added the label Needs rebase on Jan 30, 2019

Empact force-pushed on Jan 31, 2019

Empact renamed this:
~~wallet: Return a ScanResult from CWallet::RescanFromTime~~
wallet: Make scan / abort status private to CWallet
on Jan 31, 2019

DrahtBot removed the label Needs rebase on Jan 31, 2019

Empact commented at 1:18 AM on January 31, 2019: member

Rebased and substantially refocused this PR to:

Return ScanResult from RescanFromTime
Add ScanResult::FailedTime to report the best non-failing time - beneficial also because most callers are uninterested in the time
Drop ~IsScanning and~ IsAbortingRescan from CWallet, as all scan/abort status is now returned as results from specific operations.

Empact force-pushed on Jan 31, 2019

Empact force-pushed on Feb 2, 2019

DrahtBot added the label Needs rebase on Feb 5, 2019

Empact force-pushed on Feb 8, 2019

DrahtBot removed the label Needs rebase on Feb 8, 2019

DrahtBot added the label Needs rebase on Mar 21, 2019

Empact force-pushed on Apr 3, 2019

DrahtBot removed the label Needs rebase on Apr 3, 2019

DrahtBot added the label Needs rebase on Apr 9, 2019

Empact force-pushed on Apr 14, 2019

Empact commented at 3:59 AM on April 14, 2019: member

Rebased

DrahtBot removed the label Needs rebase on Apr 15, 2019

DrahtBot added the label Needs rebase on May 6, 2019

jb55 commented at 6:46 AM on May 14, 2019: member

looks like a nice refactor

utACK 4b4cf7cc28347b2d537be89a412093dbd62ea83e

Empact force-pushed on May 17, 2019

Empact commented at 3:02 AM on May 17, 2019: member

Rebased, and restored IsScanning as it's needed for d3e8458365

DrahtBot removed the label Needs rebase on May 17, 2019

jb55 commented at 4:28 PM on May 17, 2019: member

not sure why github tends to screw up range-diffs sometimes but here's the last one:

$ git range-diff 4b4cf7c~2..4b4cf7c 469117d~2..469117d

1:  99a72728a6 = 1:  c82f6c7806 Return ScanResult from CWallet::RescanFromTime
2:  4b4cf7cc28 ! 2:  469117d55c Make scan / abort status private to CWallet
@@ -2,7 +2,7 @@
 
     Make scan / abort status private to CWallet
 
-    Drop CWallet::IsAbortingRescan and IsScanning by reporting the abort
+    Drop CWallet::IsAbortingRescan by reporting the abort
     effectuality from CWallet::AbortScan.
 
  diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp
@@ -54,8 +54,7 @@
       */
 -    void AbortRescan() { fAbortRescan = true; }
 -    bool IsAbortingRescan() { return fAbortRescan; }
--    bool IsScanning() { return fScanningWallet; }
 +    bool AbortRescan();
- 
-     /**
-      * keystore implementation
+     bool IsScanning() { return fScanningWallet; }
+     int64_t ScanningDuration() const { return fScanningWallet ? GetTimeMillis() - m_scanning_start : 0; }
+     double ScanningProgress() const { return fScanningWallet ? (double) m_scanning_progress : 0; }

re-utACK 469117d55c3601c75883d8ca7ff5e76f9f60da60

DrahtBot added the label Needs rebase on Oct 29, 2019

Empact force-pushed on Jan 17, 2020

DrahtBot removed the label Needs rebase on Jan 17, 2020

in src/wallet/wallet.cpp:1573 in bcb3fc0046 outdated

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.

theStack commented at 12:26 PM on February 7, 2020:

Now that this method doesn't return a timestamp (int64_t) anymore, this Doxygen comment needs to be updated.

Empact commented at 6:16 PM on March 9, 2020:

Updated, thanks!

DrahtBot added the label Needs rebase on Mar 7, 2020

Empact force-pushed on Mar 9, 2020

DrahtBot removed the label Needs rebase on Mar 9, 2020

in src/wallet/rpcdump.cpp:1425 in 319aeba7d4 outdated

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

kallewoof commented at 2:56 AM on March 10, 2020:

Tiny preference of switch (result.status) versus if'ing over enums (e.g. it gives compiler warnings when new fields are added).

in src/wallet/wallet.h:834 in 319aeba7d4 outdated

 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

kallewoof commented at 3:02 AM on March 10, 2020:

Add empty ( *) line before @ return. (See "Coding Style (Doxygen-compatible comments)" in developer.nodes.md)

in src/wallet/wallet.h:905 in 319aeba7d4 outdated

 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.

kallewoof commented at 3:05 AM on March 10, 2020:

This feels weird to me, and feels like it should have a new ORPHAN_CHAIN ScanResult, even if this is handled the same way as SUCCESS everywhere right now.

in src/wallet/rpcdump.cpp:1433 in 319aeba7d4 outdated

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;

kallewoof commented at 4:56 AM on March 10, 2020:

This differs from the original code, where scannedTime was set to the last block time + TIMESTAMP_WINDOW + 1. Won't this result in missed scans below, as scannedTime will be low?

Empact commented at 12:15 PM on March 10, 2020:

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?

kallewoof commented at 12:51 PM on March 10, 2020:

No, sorry, you're right. I missed the passing of scannedTime on L#1427. It looks identical.

kallewoof commented at 4:57 AM on March 10, 2020: member

Looks like an improvement. Left comments.

in src/wallet/wallet.cpp:172 in 319aeba7d4 outdated

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

promag commented at 8:23 AM on March 10, 2020:

Looks like you could have a race in this method?

Empact commented at 6:14 PM on March 11, 2020:

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

Return ScanResult from CWallet::RescanFromTime

Lookup the failed time just in the case it's needed.

cc120d788a

Make scan / abort status private to CWallet

Drop CWallet::IsAbortingRescan by reporting the abort
effectuality from CWallet::AbortScan.

235c18b045

doc: Update CWallet::RescanFromTime return type documentation

And move to the header.

71b93603be

in src/wallet/rpcdump.cpp:219 in 319aeba7d4 outdated

 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();

promag commented at 2:24 PM on March 10, 2020:

Why does it matter the return value? Isn't even documented.

Empact force-pushed on Mar 11, 2020

DrahtBot commented at 10:03 PM on March 23, 2020: member

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

DrahtBot added the label Needs rebase on Mar 23, 2020

laanwj commented at 12:36 PM on September 9, 2021: member

Any plans for this? It's needed rebase for more than a year, no other activity.

DrahtBot commented at 12:29 PM on December 22, 2021: member

There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

MarcoFalke closed this on Dec 22, 2021

MarcoFalke commented at 12:37 PM on December 22, 2021: member

Closing for now due to inactivity for more than a year. Can be reopened anytime. Just leave a comment here or in IRC.

DrahtBot locked this on Dec 22, 2022