[moveonly] Extract RescanWallet to handle a simple rescan #13658

Empact · 2018-07-13T16:47:10Z

Where the outcome does not depend on the result, apart from a simple
success check.

maflcko · 2018-07-13T16:56:47Z

src/wallet/wallet.h

Is there any reason why not just leave this here? It could very well be used by the gui or some other interface to the wallet in the future and had to be moved back. Generally, I'd prefer if code was only moved when necessary, since unnecessary code movement/refactoring makes backports easier to mess up and consumes scarce review resources.

This move reduces the surface area of CWallet, which is pretty sprawling at the moment (wallet.cpp is ~4500 lines, the 3rd-largest *.cpp file, and the largest outside /qt/), and localizes this code to one implementation file from being more broadly available (wallet.h is included into 26 files).

Grant the concerns you note are serious, and we should draw the line somewhere, but if the codebase is to be in play for a long time, it needs to evolve and progress. Moving incrementally toward minimizing scope of code, dependencies, etc. is attractive from that perspective as it should have payoffs in reducing the overall complexity of the codebase, which should ease changes on an ongoing and cumulative basis.

Other approaches to making better use of scarce review resources come to mind:

I've thought of implementing a web site to helps rank PRs by ACKs and other indications of importance. Current solutions like: https://github.com/bitcoin/bitcoin/projects/8 are pretty limited and not decentralized and ACKs are not currently well-exposed across PRs in aggregate.

We could add a feature to output the changes between force pushes to the same PR, so that reviewers could review just the incremental subset, and the historical changes are recorded.

We could potentially examine and report changes to the build output to give reviewers a sense of the consequence to the output to the binaries.

Seems like these are the sorts of things that will help the project to scale and accelerate.

I've thought of implementing a web site to helps rank PRs by ACKs and other indications of importance. Current solutions like: https://github.com/bitcoin/bitcoin/projects/8 are pretty limited and not decentralized and ACKs are not currently well-exposed across PRs in aggregate.

Are you aware of https://bitcoinacks.com/?desc=1&flt3_label_not_contains=Needs+rebase&flt0_merged_empty=1&flt4_ci_contains=Success&flt1_closed_empty=1&sort=7&flt2_mergeable_contains=Mergeable

Nice, hadn't see that. 👍

Empact · 2018-07-13T18:45:20Z

This refactor raises the following questions:

Why does rpc importmulti call pwallet->ReacceptWalletTransactions before aborting on pwallet->IsAbortingRescan(). Should it flip that ordering like the others do?
Why do 3 of the 5 calls call pwallet->ReacceptWalletTransactions, while rpc importwallet calls pwallet->MarkDirty() and rpc importprivkey calls neither? This seems like a possible bug in rpc importprivkey.

Empact · 2018-07-13T19:00:16Z

Updated to minimize git diff --color-moved=dimmed_zebra --minimal head^^

DrahtBot · 2018-07-13T19:20:18Z

Note to reviewers: This pull request conflicts with the following ones:

[WIP] Importmulti private key support #12705 ([WIP] Importmulti private key support by kallewoof)

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.

sipa · 2018-07-14T01:10:12Z

I don't like this. Code that interacts with the wallet should be in the wallet, not in the RPC code. I'm aware that there is an enormous amount of internal logic already in the RPC code, but we should be moving stuff in the other direction.

maflcko · 2018-07-14T11:53:03Z

utACK second commit, if the first one is dropped.

Where the outcome does not depend on the result, apart from a simple success check.

Empact · 2018-07-14T17:09:27Z

Dropped the first commit, and added handling of the ShutdownRequested rescan abort condition:

bitcoin/src/wallet/wallet.cpp

Lines 1759 to 1763 in b25a4c2

    
           if (pindex && fAbortRescan) { 
        
               LogPrintf("Rescan aborted at block %d. Progress=%f\n", pindex->nHeight, progress_current); 
        
           } else if (pindex && ShutdownRequested()) { 
        
               LogPrintf("Rescan interrupted by shutdown request at block %d. Progress=%f\n", pindex->nHeight, progress_current); 
        
           }

Empact · 2018-07-14T17:28:20Z

Added full handing of rescan failure conditions to CWallet::CreateWalletFromFile

promag · 2018-07-14T20:20:12Z

Kind of NACK the current approach. If you want to touch this consider improving CWallet::ScanForWalletTransactions signature, like:

enum WalletScanError
{
    NONE = 0, // when ScanForWalletTransactions return is true
    ABORTED,
    SHUTDOWN_REQUEST,
};
struct WalletScanResult {
    WalletScanError error;
    std::string message;
    // other outputs
};
bool CWallet::ScanForWalletTransactions(WalletScanResult& result, ...);

This way the callers don't have to check what caused the error.

Edit: I like the RescanWallet abstraction in rpcdump.cpp

Empact · 2018-07-14T20:44:35Z

@promag I did something like that here #13076, and while I think the result is ok, I've been thinking more of updating it to return bool as prompted by jimpo in #13582:
#13582 (comment)

Have that follow-up in mind.

Empact · 2018-07-14T20:47:33Z

I'll split these up for separate consideration.

promag · 2018-07-14T21:16:38Z

@Empact my suggestion is in line with @jimpo suggestion.

promag · 2018-07-17T11:16:38Z

utACK 3fe836b.

Side note, not sure if this should be tagged moveonly.

maflcko · 2018-07-17T12:16:22Z

utACK 3fe836b

laanwj · 2018-07-25T13:31:17Z

src/wallet/wallet.h

 static const bool DEFAULT_WALLETBROADCAST = true;
 static const bool DEFAULT_DISABLE_WALLET = false;

-static const int64_t TIMESTAMP_MIN = 0;


good to move this locally, it's a really aspecific constant name

3fe836b [moveonly] Extract RescanWallet to handle a simple rescan (Ben Woosley) Pull request description: Where the outcome does not depend on the result, apart from a simple success check. Tree-SHA512: e0d29c6fc0c7f99a730289e5a80deb586b2848aead56b5198a71ef01f65374812468dfd57be0b8b076eb9be4090d5101d28d979a1d5c3d2f1caeca77b303e90e

Summary: 3fe836b [moveonly] Extract RescanWallet to handle a simple rescan (Ben Woosley) Pull request description: Where the outcome does not depend on the result, apart from a simple success check. Tree-SHA512: e0d29c6fc0c7f99a730289e5a80deb586b2848aead56b5198a71ef01f65374812468dfd57be0b8b076eb9be4090d5101d28d979a1d5c3d2f1caeca77b303e90e Backport of Core PR12658 bitcoin/bitcoin#13658 Test Plan: make check ./bitcoin-qt -> help -> debug -> console -> rescanblockchain ./bitcoind ./bitcoin-cli rescanblockchain Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: jasonbcox, O1 Bitcoin ABC, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D4020

…le rescan 3fe836b [moveonly] Extract RescanWallet to handle a simple rescan (Ben Woosley) Pull request description: Where the outcome does not depend on the result, apart from a simple success check. Tree-SHA512: e0d29c6fc0c7f99a730289e5a80deb586b2848aead56b5198a71ef01f65374812468dfd57be0b8b076eb9be4090d5101d28d979a1d5c3d2f1caeca77b303e90e

maflcko reviewed Jul 13, 2018

View reviewed changes

Empact force-pushed the rescan-from-time branch from 110d5e7 to c180c06 Compare July 13, 2018 18:11

Empact force-pushed the rescan-from-time branch 4 times, most recently from c764dd6 to 65c06a3 Compare July 13, 2018 18:59

DrahtBot mentioned this pull request Jul 13, 2018

[moveonly] Extract CWallet::MarkInputsDirty, and privatize AddToWalletIfInvolvingMe #13651

Merged

fanquake added the Wallet label Jul 13, 2018

bitcoin deleted a comment from DrahtBot Jul 14, 2018

DrahtBot added the Needs rebase label Jul 14, 2018

[moveonly] Extract RescanWallet to handle a simple rescan

3fe836b

Where the outcome does not depend on the result, apart from a simple success check.

Empact force-pushed the rescan-from-time branch from 65c06a3 to 348dd37 Compare July 14, 2018 17:06

Empact changed the title ~~[wallet] [moveonly] Move rescanning from time logic into wallet/rpcdump.cpp~~ rpc: Report when rescan aborts due to ShutdownRequested Jul 14, 2018

DrahtBot removed the Needs rebase label Jul 14, 2018

Empact force-pushed the rescan-from-time branch from 348dd37 to f3f1d1c Compare July 14, 2018 17:27

Empact force-pushed the rescan-from-time branch from f3f1d1c to 3fe836b Compare July 14, 2018 20:48

Empact changed the title ~~rpc: Report when rescan aborts due to ShutdownRequested~~ Extract RescanWallet to handle a simple rescan Jul 14, 2018

maflcko added the Refactoring label Jul 17, 2018

Empact changed the title ~~Extract RescanWallet to handle a simple rescan~~ [moveonly] Extract RescanWallet to handle a simple rescan Jul 17, 2018

laanwj reviewed Jul 25, 2018

View reviewed changes

laanwj merged commit 3fe836b into bitcoin:master Jul 25, 2018

Empact deleted the rescan-from-time branch July 25, 2018 14:41

Bushstar mentioned this pull request Aug 13, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#358

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

[moveonly] Extract RescanWallet to handle a simple rescan #13658

[moveonly] Extract RescanWallet to handle a simple rescan #13658

Conversation

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

Uh oh!

Uh oh!

maflcko Jul 13, 2018

Choose a reason for hiding this comment

Uh oh!

Empact Jul 13, 2018

Choose a reason for hiding this comment

Uh oh!

maflcko Jul 13, 2018

Choose a reason for hiding this comment

Uh oh!

Empact Jul 13, 2018

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

Empact commented Jul 13, 2018

Uh oh!

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

Uh oh!

Uh oh!

sipa commented Jul 14, 2018

Uh oh!

maflcko commented Jul 14, 2018

Uh oh!

Empact commented Jul 14, 2018

Uh oh!

Empact commented Jul 14, 2018

Uh oh!

promag commented Jul 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Empact commented Jul 14, 2018

Uh oh!

Empact commented Jul 14, 2018

Uh oh!

promag commented Jul 14, 2018

Uh oh!

promag commented Jul 17, 2018

Uh oh!

maflcko commented Jul 17, 2018

Uh oh!

laanwj Jul 25, 2018

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Empact commented Jul 13, 2018 •

edited

Loading

Empact commented Jul 13, 2018 •

edited

Loading

DrahtBot commented Jul 13, 2018 •

edited

Loading

promag commented Jul 14, 2018 •

edited

Loading