wallet: Make scan / abort status private to CWallet by Empact · Pull Request #14942 · bitcoin/bitcoin

Empact · 2018-12-13T00:31:12Z

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.

DrahtBot · 2018-12-13T02:02:24Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

interfaces: Describe and follow some code conventions #18278 (interfaces: Describe and follow some code conventions by ryanofsky)
wallet: Remove calls to Chain::Lock methods #17954 (wallet: Remove calls to Chain::Lock methods by ryanofsky)
Multiprocess bitcoin #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.

src/wallet/rpcdump.cpp

Empact · 2019-01-31T01:18:05Z

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 · 2019-04-14T03:59:00Z

Rebased

jb55 · 2019-05-14T06:46:46Z

looks like a nice refactor

utACK 4b4cf7cc28347b2d537be89a412093dbd62ea83e

Empact · 2019-05-17T03:02:56Z

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

jb55 · 2019-05-17T16:28:58Z

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

src/wallet/wallet.cpp

kallewoof

Looks like an improvement. Left comments.

src/wallet/rpcdump.cpp

src/wallet/wallet.h

kallewoof · 2020-03-10T03:05:11Z

src/wallet/wallet.h

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.

src/wallet/rpcdump.cpp

promag · 2020-03-10T08:23:02Z

src/wallet/wallet.cpp

Looks like you could have a race in this method?

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

promag · 2020-03-10T14:24:20Z

src/wallet/rpcdump.cpp

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

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.

DrahtBot · 2020-03-23T22:03:45Z

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

laanwj · 2021-09-09T12:36:18Z

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

DrahtBot · 2021-12-22T12:29:40Z

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.

maflcko · 2021-12-22T12:37:22Z

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

fanquake added the Wallet label Dec 13, 2018

Empact mentioned this pull request Dec 14, 2018

Fix ScanForWalletTransactions to return an enum indicating scan result: success / failure / user_abort #13076

Merged

Empact changed the title ~~Wallet: Return a ScanResult from CWallet::RescanFromTime~~ wallet: Return a ScanResult from CWallet::RescanFromTime Dec 14, 2018

practicalswift reviewed Jan 5, 2019

View reviewed changes

src/wallet/rpcdump.cpp Outdated Show resolved Hide resolved

DrahtBot added the Needs rebase label Jan 30, 2019

Empact force-pushed the rescan-from-time-result branch from a71986a to 94accf9 Compare January 31, 2019 01:12

Empact changed the title ~~wallet: Return a ScanResult from CWallet::RescanFromTime~~ wallet: Make scan / abort status private to CWallet Jan 31, 2019

DrahtBot removed the Needs rebase label Jan 31, 2019

Empact force-pushed the rescan-from-time-result branch 3 times, most recently from fd61c8b to 4cfd0a8 Compare February 2, 2019 10:15

DrahtBot added the Needs rebase label Feb 5, 2019

Empact force-pushed the rescan-from-time-result branch from 4cfd0a8 to 9d6a502 Compare February 8, 2019 22:27

DrahtBot removed the Needs rebase label Feb 8, 2019

DrahtBot added the Needs rebase label Mar 21, 2019

Empact force-pushed the rescan-from-time-result branch from 9d6a502 to 741f2fe Compare April 3, 2019 04:47

DrahtBot added Needs rebase and removed Needs rebase labels Apr 3, 2019

Empact force-pushed the rescan-from-time-result branch from 741f2fe to 4b4cf7c Compare April 14, 2019 03:58

DrahtBot removed the Needs rebase label Apr 15, 2019

DrahtBot added the Needs rebase label May 6, 2019

Empact force-pushed the rescan-from-time-result branch from 4b4cf7c to 469117d Compare May 17, 2019 02:58

DrahtBot removed the Needs rebase label May 17, 2019

DrahtBot added the Needs rebase label Oct 29, 2019

Empact force-pushed the rescan-from-time-result branch from 469117d to c26c232 Compare January 17, 2020 02:35

DrahtBot removed the Needs rebase label Jan 17, 2020

theStack reviewed Feb 7, 2020

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Feb 11, 2020

wallet: Remove calls to Chain::Lock methods #17954

Merged

This was referenced Mar 2, 2020

wallet/refactor: refer to CWallet immutably when possible #18241

Merged

interfaces: Describe and follow some code conventions #18278

Merged

DrahtBot added the Needs rebase label Mar 7, 2020

Empact force-pushed the rescan-from-time-result branch from c26c232 to 319aeba Compare March 9, 2020 17:56

DrahtBot removed the Needs rebase label Mar 9, 2020

kallewoof reviewed Mar 10, 2020

View reviewed changes

promag reviewed Mar 10, 2020

View reviewed changes

src/wallet/rpcdump.cpp Outdated

Copy link

Contributor

promag Mar 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Empact added 3 commits March 10, 2020 12:53

Return ScanResult from CWallet::RescanFromTime

cc120d7

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

Make scan / abort status private to CWallet

235c18b

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

doc: Update CWallet::RescanFromTime return type documentation

71b9360

And move to the header.

Empact force-pushed the rescan-from-time-result branch from 319aeba to 71b9360 Compare March 11, 2020 18:15

DrahtBot mentioned this pull request Mar 20, 2020

Multiprocess bitcoin #10102

Draft

DrahtBot added the Needs rebase label Mar 23, 2020

maflcko closed this Dec 22, 2021

bitcoin locked and limited conversation to collaborators Dec 22, 2022

Conversation

Empact commented Dec 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Dec 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Uh oh!

Empact commented Jan 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Empact commented Apr 14, 2019

Uh oh!

jb55 commented May 14, 2019

Uh oh!

Empact commented May 17, 2019

Uh oh!

jb55 commented May 17, 2019

Uh oh!

Uh oh!

kallewoof left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kallewoof Mar 10, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

promag Mar 10, 2020

Choose a reason for hiding this comment

Uh oh!

Empact Mar 11, 2020

Choose a reason for hiding this comment

Uh oh!

promag Mar 10, 2020

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Mar 23, 2020

Uh oh!

laanwj commented Sep 9, 2021

Uh oh!

DrahtBot commented Dec 22, 2021

Uh oh!

maflcko commented Dec 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

Empact commented Dec 13, 2018 •

edited

Loading

DrahtBot commented Dec 13, 2018 •

edited

Loading

Empact commented Jan 31, 2019 •

edited

Loading