Remove `-rescan` startup parameter #23123

pull meshcollider wants to merge 4 commits into bitcoin:master from meshcollider:202109_remove_rescan changing 17 files +56 −46

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)

        bool dummy;
        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:

DBErrors LoadWallet(bool* rescan_required = nullptr);

or overload

DBErrors 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