Remove `-rescan` startup parameter

meshcollider commented at 4:37 am on September 29, 2021: contributor

Remove the -rescan startup parameter.

Rescans can be run with the rescanblockchain RPC.

Rescans are still done on wallet-load if needed due to corruption, for example.

Remove outdated dummy wallet -salvagewallet arg 6c006495ef

meshcollider added the label Wallet on Sep 29, 2021

in src/wallet/wallettool.cpp:58 in cd4a8165c6 outdated

53@@ -54,7 +54,8 @@ static std::shared_ptr<CWallet> MakeWallet(const std::string& name, const fs::pa
54     std::shared_ptr<CWallet> wallet_instance{new CWallet(nullptr /* chain */, name, std::move(database)), WalletToolReleaseWallet};
55     DBErrors load_wallet_ret;
56     try {
57-        load_wallet_ret = wallet_instance->LoadWallet();
58+        bool rescan_required = false;
59+        load_wallet_ret = wallet_instance->LoadWallet(rescan_required);

MarcoFalke commented at 5:56 am on September 29, 2021:

nit: Could leave it uninitialized to clarify this is an unused out param? (Same for other places)

0        bool dummy;
1        load_wallet_ret = wallet_instance->LoadWallet(/*rescan_required=*/dummy);

meshcollider commented at 6:49 am on September 29, 2021:

Done :)

MarcoFalke approved

MarcoFalke commented at 5:57 am on September 29, 2021: member

Concept ACK. See also #13044

in test/functional/feature_notifications.py:95 in 718fc90b69 outdated

90             self.wait_until(lambda: len(os.listdir(self.walletnotify_dir)) == block_count, timeout=10)
91 
92-            # directory content should equal the generated transaction hashes
93-            tx_details = list(map(lambda t: (t['txid'], t['blockheight'], t['blockhash']), self.nodes[1].listtransactions("*", block_count)))
94-            self.stop_node(1)
95-            self.expect_wallet_notify(tx_details)

MarcoFalke commented at 6:11 am on September 29, 2021:

shouldn’t this be adjusted to test that the rescan RPC also notifies?

meshcollider commented at 6:43 am on September 29, 2021:

Good point, done.

MarcoFalke approved

MarcoFalke commented at 7:06 am on September 29, 2021: member

Approach ACK 256314b984a215fab0cf16b13a234ae002e987c4

Didn’t review closely

fanquake commented at 7:17 am on September 29, 2021: member

Concept ACK

Sjors commented at 8:35 am on September 29, 2021: member

utACK 256314b

laanwj commented at 10:24 am on September 29, 2021: member

Concept and code review ACK 256314b984a215fab0cf16b13a234ae002e987c4

in src/wallet/wallet.h:661 in a751305ed7 outdated

657@@ -658,7 +658,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
658     CAmount GetDebit(const CTransaction& tx, const isminefilter& filter) const;
659     void chainStateFlushed(const CBlockLocator& loc) override;
660 
661-    DBErrors LoadWallet();
662+    DBErrors LoadWallet(bool& rescan_required);

achow101 commented at 5:20 pm on September 29, 2021:

In a751305ed7d0e085d80c76dfe1b419858ce331df “Corrupt wallet tx shouldn’t trigger rescan of all wallets”

It seems like it would be cleaner to add a DBErrors:RESCAN_REQUIRED instead of this rescan_required parameter and requiring a bunch of callers to provide a dummy bool.

Sjors commented at 7:49 pm on September 29, 2021:

I like this approach too.

promag commented at 9:16 pm on September 29, 2021:

That requires changing https://github.com/bitcoin/bitcoin/blob/419afa93419e6840f78cb94b4a39d826eb10e139/src/wallet/wallet.cpp#L2546

A low effort alternative is to use pointer:

0DBErrors LoadWallet(bool* rescan_required = nullptr);

or overload

0DBErrors LoadWallet() { bool dummy; return LoadWallet(dummy); }

meshcollider commented at 10:05 pm on September 29, 2021:

I’ll use a pointer as @promag suggests :+1: I was just erring on the side of caution when it came to hiding the parameter, so it wasn’t accidentally ignored.

achow101 commented at 10:19 pm on September 29, 2021:

I really don’t think it should be an out parameter since we have this enum for loading errors.

meshcollider commented at 10:39 pm on September 29, 2021:

Ok fair enough, I’ll use your approach

DrahtBot commented at 5:38 pm on September 29, 2021: 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:

#22928 (refactor: Remove gArgs from wallet.h and wallet.cpp (2) by kiminuo)
#22868 (wallet: Call load handlers without cs_wallet locked by promag)
#22805 (refactor: use CWallet const shared pointers in dump{privkey,wallet} by theStack)
#22787 (refactor: actual immutable pointing by kallewoof)
#22235 (script: add script to generate example bitcoin.conf by josibake)
#21726 (Improve Indices on pruned nodes via prune blockers by fjahr)
#15719 (Wallet passive startup 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.

Corrupt wallet tx shouldn't trigger rescan of all wallets f963b0fa8c

Remove -rescan startup parameter bccd1d942d

Add rescan removal release note dc3ec74d67

meshcollider commented at 11:13 pm on September 29, 2021: contributor

Modified second commit to use DBErrors::RESCAN_REQUIRED instead of an out bool, as suggested by @achow101

achow101 commented at 1:10 am on September 30, 2021: member

ACK dc3ec74d67abc85e8f724648f93efdd097e6f783

laanwj commented at 6:49 pm on September 30, 2021: member

re-ACK dc3ec74d67abc85e8f724648f93efdd097e6f783

laanwj referenced this in commit 571bb94dfb on Sep 30, 2021

in src/wallet/walletdb.h:52 in f963b0fa8c outdated

47@@ -48,7 +48,8 @@ enum class DBErrors
48     NONCRITICAL_ERROR,
49     TOO_NEW,
50     LOAD_FAIL,
51-    NEED_REWRITE
52+    NEED_REWRITE,
53+    RESCAN_REQUIRED

ryanofsky commented at 6:55 pm on September 30, 2021:

In commit “Corrupt wallet tx shouldn’t trigger rescan of all wallets” (f963b0fa8cdd5223feb828c5faf6c57bc4107c8a)

Minor: Might call this NEED_RESCAN to be consistent with NEED_REWRITE

laanwj commented at 7:47 pm on September 30, 2021:

I think this is a good suggestion to be more consistent. Maybe in a follow-up PR.

ryanofsky approved

ryanofsky commented at 6:59 pm on September 30, 2021: member

Code review ACK dc3ec74d67abc85e8f724648f93efdd097e6f783. Looks great!

meshcollider commented at 7:12 pm on September 30, 2021: contributor

Merged here: 571bb94dfb5047c9be8fcbae5dae71de7256b86c

meshcollider closed this on Sep 30, 2021

sidhujag referenced this in commit 9e031d84be on Oct 1, 2021

fanquake referenced this in commit 33b0696d31 on Oct 1, 2021

sidhujag referenced this in commit e2498f0a20 on Oct 1, 2021

DrahtBot locked this on Oct 30, 2022

Remove -rescan startup parameter #23123

Conflicts

Remove `-rescan` startup parameter #23123