[moveonly] Extract RescanWallet to handle a simple rescan

Empact commented at 4:47 pm on July 13, 2018: member

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

in src/wallet/wallet.h:940 in 3259024e70 outdated

932@@ -935,7 +933,6 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface
933     void BlockConnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex *pindex, const std::vector<CTransactionRef>& vtxConflicted) override;
934     void BlockDisconnected(const std::shared_ptr<const CBlock>& pblock) override;
935     bool AddToWalletIfInvolvingMe(const CTransactionRef& tx, const CBlockIndex* pIndex, int posInBlock, bool fUpdate) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
936-    int64_t RescanFromTime(int64_t startTime, const WalletRescanReserver& reserver, bool update);

MarcoFalke commented at 4:56 pm on July 13, 2018:

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.

Empact commented at 6:39 pm on July 13, 2018:

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.

MarcoFalke commented at 6:49 pm on July 13, 2018:

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

Empact commented at 7:01 pm on July 13, 2018:

Nice, hadn’t see that. 👍

Empact force-pushed on Jul 13, 2018

Empact commented at 6:45 pm on July 13, 2018: member

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 force-pushed on Jul 13, 2018

Empact commented at 7:00 pm on July 13, 2018: member

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

DrahtBot commented at 7:20 pm on July 13, 2018: member

#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.

fanquake added the label Wallet on Jul 13, 2018

MarcoFalke deleted a comment on Jul 14, 2018

sipa commented at 1:10 am on July 14, 2018: member

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.

DrahtBot added the label Needs rebase on Jul 14, 2018

MarcoFalke commented at 11:53 am on July 14, 2018: member

utACK second commit, if the first one is dropped.

[moveonly] Extract RescanWallet to handle a simple rescan

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

3fe836b78d

Empact force-pushed on Jul 14, 2018

Empact commented at 5:09 pm on July 14, 2018: member

Dropped the first commit, and added handling of the ShutdownRequested rescan abort condition: https://github.com/bitcoin/bitcoin/blob/b25a4c2284babdf1e8cf0ec3b1402200dd25f33f/src/wallet/wallet.cpp#L1759-L1763

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

DrahtBot removed the label Needs rebase on Jul 14, 2018

Empact force-pushed on Jul 14, 2018

Empact commented at 5:28 pm on July 14, 2018: member

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

promag commented at 8:20 pm on July 14, 2018: member

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

 0enum WalletScanError
 1{
 2    NONE = 0, // when ScanForWalletTransactions return is true
 3    ABORTED,
 4    SHUTDOWN_REQUEST,
 5};
 6struct WalletScanResult {
 7    WalletScanError error;
 8    std::string message;
 9    // other outputs
10};
11bool 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 commented at 8:44 pm on July 14, 2018: member

@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 commented at 8:47 pm on July 14, 2018: member

I’ll split these up for separate consideration.

Empact force-pushed on Jul 14, 2018

Empact renamed this:
~~rpc: Report when rescan aborts due to ShutdownRequested~~
Extract RescanWallet to handle a simple rescan
on Jul 14, 2018

promag commented at 9:16 pm on July 14, 2018: member

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

promag commented at 11:16 am on July 17, 2018: member

utACK 3fe836b.

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

MarcoFalke added the label Refactoring on Jul 17, 2018

MarcoFalke commented at 12:16 pm on July 17, 2018: member

utACK 3fe836b78d504942e8850b607453886969f57e27

Empact renamed this:
~~Extract RescanWallet to handle a simple rescan~~
[moveonly] Extract RescanWallet to handle a simple rescan
on Jul 17, 2018

in src/wallet/wallet.h:65 in 3fe836b78d

61@@ -62,8 +62,6 @@ static const bool DEFAULT_WALLET_RBF = false;
62 static const bool DEFAULT_WALLETBROADCAST = true;
63 static const bool DEFAULT_DISABLE_WALLET = false;
64 
65-static const int64_t TIMESTAMP_MIN = 0;

laanwj commented at 1:31 pm on July 25, 2018:

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

laanwj merged this on Jul 25, 2018

laanwj closed this on Jul 25, 2018

laanwj referenced this in commit 2d41af1728 on Jul 25, 2018

Empact deleted the branch on Jul 25, 2018

jasonbcox referenced this in commit be3bf6094d on Sep 13, 2019

UdjinM6 referenced this in commit 22d5b8732e on Jun 29, 2021

UdjinM6 referenced this in commit a90aa02aa9 on Jun 29, 2021

UdjinM6 referenced this in commit 72f546770f on Jul 1, 2021

UdjinM6 referenced this in commit 2ac755f635 on Jul 2, 2021

UdjinM6 referenced this in commit ab9b8d3944 on Jul 2, 2021

DrahtBot locked this on Sep 8, 2021

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