backport!: bitcoin#23123, #23147 (wallet rescan related, breaking changes)

doc/release-notes-remove-rescan.md

@@ -0,0 +1,9 @@
+Notable changes
+===============
+
+Rescan startup parameter removed
+--------------------------------
+
+The `-rescan` startup parameter has been removed. Wallets which require
+rescanning due to corruption will still be rescanned on startup.
+Otherwise, please use the `rescanblockchain` RPC to trigger a rescan.

src/dummywallet.cpp

@@ -49,8 +49,6 @@ void DummyWalletInit::AddWalletOptions(ArgsManager& argsman) const
         "-keypool=<n>",
         "-maxapsfee=<n>",
         "-maxtxfee=<amt>",
-        "-rescan=<mode>",
-        "-salvagewallet",
         "-signer=<cmd>",
         "-spendzeroconfchange",
         "-wallet=<path>",

src/init.cpp

@@ -592,7 +592,7 @@ void SetupServerArgs(ArgsManager& argsman)
         -GetNumCores(), llmq::MAX_BLSCHECK_THREADS, llmq::DEFAULT_BLSCHECK_THREADS), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
     argsman.AddArg("-persistmempool", strprintf("Whether to save the mempool on shutdown and load on restart (default: %u)", DEFAULT_PERSIST_MEMPOOL), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
     argsman.AddArg("-pid=<file>", strprintf("Specify pid file. Relative paths will be prefixed by a net-specific datadir location. (default: %s)", BITCOIN_PID_FILENAME), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
-    argsman.AddArg("-prune=<n>", strprintf("Reduce storage requirements by enabling pruning (deleting) of old blocks. This allows the pruneblockchain RPC to be called to delete specific blocks, and enables automatic pruning of old blocks if a target size in MiB is provided. This mode is incompatible with -txindex, -addressindex, -spentindex, -rescan and -disablegovernance=false. "
+    argsman.AddArg("-prune=<n>", strprintf("Reduce storage requirements by enabling pruning (deleting) of old blocks. This allows the pruneblockchain RPC to be called to delete specific blocks, and enables automatic pruning of old blocks if a target size in MiB is provided. This mode is incompatible with -txindex, -addressindex, -spentindex and -disablegovernance=false. "
             "Warning: Reverting this setting requires re-downloading the entire blockchain. "
             "(default: 0 = disable pruning blocks, 1 = allow manual pruning via RPC, >%u = automatically prune block files to stay under the specified target size in MiB)", MIN_DISK_SPACE_FOR_BLOCK_FILES / 1024 / 1024), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
     argsman.AddArg("-settings=<file>", strprintf("Specify path to dynamic settings data file. Can be disabled with -nosettings. File is written at runtime and not meant to be edited by users (use %s instead for custom settings). Relative paths will be prefixed by datadir location. (default: %s)", BITCOIN_CONF_FILENAME, BITCOIN_SETTINGS_FILENAME), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);

src/wallet/init.cpp

@@ -65,8 +65,6 @@ void WalletInit::AddWalletOptions(ArgsManager& argsman) const
     argsman.AddArg("-instantsendnotify=<cmd>", "Execute command when a wallet InstantSend transaction is successfully locked. %s in cmd is replaced by TxID and %w is replaced by wallet name. %w is not currently implemented on Windows. On systems where %w is supported, it should NOT be quoted because this would break shell escaping used to invoke the command.", ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
 #endif
     argsman.AddArg("-keypool=<n>", strprintf("Set key pool size to <n> (default: %u). Warning: Smaller sizes may increase the risk of losing funds when restoring from an old backup, if none of the addresses in the original keypool have been used.", DEFAULT_KEYPOOL_SIZE), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
-    argsman.AddArg("-rescan=<mode>", "Rescan the block chain for missing wallet transactions on startup"
-                                            " (1 = start from wallet creation time, 2 = start from genesis block)", ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
 #ifdef ENABLE_EXTERNAL_SIGNER
     argsman.AddArg("-signer=<cmd>", "External signing tool, see doc/external-signer.md", ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
 #endif

src/wallet/rpc/backup.cpp

@@ -1665,7 +1665,7 @@ RPCHelpMan importmulti()
                                       "and coins using this key may not appear in the wallet. This error could be "
                                       "caused by pruning or data corruption (see dashd log for details) and could "
                                       "be dealt with by downloading and rescanning the relevant blocks (see -reindex "
-                                      "and -rescan options).",
+                                      "option and rescanblockchain RPC).",
                                 GetImportTimestamp(request, now), scannedTime - TIMESTAMP_WINDOW - 1, TIMESTAMP_WINDOW)));
                     response.push_back(std::move(result));
                 }
@@ -1964,7 +1964,7 @@ RPCHelpMan importdescriptors() {
                                       "and coins using this desc may not appear in the wallet. This error could be "
                                       "caused by pruning or data corruption (see dashd log for details) and could "
                                       "be dealt with by downloading and rescanning the relevant blocks (see -reindex "
-                                      "and -rescan options).",
+                                      "option and rescanblockchain RPC).",
                                 GetImportTimestamp(request, now), scanned_time - TIMESTAMP_WINDOW - 1, TIMESTAMP_WINDOW)));
                     response.push_back(std::move(result));
                 }

src/wallet/rpc/wallet.cpp

@@ -461,7 +461,7 @@ static RPCHelpMan loadwallet()
     return RPCHelpMan{"loadwallet",
                 "\nLoads a wallet from a wallet file or directory."
                 "\nNote that all wallet command-line options used when starting dashd will be"
-                "\napplied to the new wallet (eg, rescan, etc).\n",
+                "\napplied to the new wallet.\n",
                 {
                     {"filename", RPCArg::Type::STR, RPCArg::Optional::NO, "The wallet directory or .dat file."},
                     {"load_on_startup", RPCArg::Type::BOOL, RPCArg::Optional::OMITTED_NAMED_ARG, "Save wallet name to persistent settings and load on startup. True to add wallet to startup list, false to remove, null to leave unchanged."},

src/wallet/salvage.cpp

@@ -47,7 +47,7 @@ bool RecoverDatabaseFile(const ArgsManager& args, const fs::path& file_path, bil
     // Call Salvage with fAggressive=true to
     // get as much data as possible.
     // Rewrite salvaged data to fresh wallet file
-    // Set -rescan so any missing transactions will be
+    // Rescan so any missing transactions will be
     // found.
     int64_t now = GetTime();
     std::string newFilename = strprintf("%s.%d.bak", filename, now);

src/wallet/test/wallet_tests.cpp

@@ -276,8 +276,8 @@ BOOST_FIXTURE_TEST_CASE(importmulti_rescan, TestChain100Setup)
                       "seconds of key creation, and could contain transactions pertaining to the key. As a result, "
                       "transactions and coins using this key may not appear in the wallet. This error could be caused "
                       "by pruning or data corruption (see dashd log for details) and could be dealt with by "
-                      "downloading and rescanning the relevant blocks (see -reindex and -rescan "
-                      "options).\"}},{\"success\":true}]",
+                      "downloading and rescanning the relevant blocks (see -reindex option and rescanblockchain "
+                      "RPC).\"}},{\"success\":true}]",
                               0, oldTip->GetBlockTimeMax(), TIMESTAMP_WINDOW));
         RemoveWallet(context, wallet, /*load_on_start=*/std::nullopt);
     }

src/wallet/wallet.cpp

@@ -1932,7 +1932,7 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc
     WalletLogPrintf("Rescan started from block %s... (%s)\n", start_block.ToString(),
                     fast_rescan_filter ? "fast variant using block filters" : "slow variant inspecting all blocks");
 
-    ShowProgress(strprintf("%s " + _("Rescanning…").translated, GetDisplayName()), 0); // show rescan progress in GUI as dialog or on splashscreen, if -rescan on startup
+    ShowProgress(strprintf("%s " + _("Rescanning…").translated, GetDisplayName()), 0); // show rescan progress in GUI as dialog or on splashscreen, if rescan required on startup (e.g. due to corruption)
     uint256 tip_hash = WITH_LOCK(cs_wallet, return GetLastBlockHash());
     uint256 end_hash = tip_hash;
     if (max_height) chain().findAncestorByHeight(tip_hash, *max_height, FoundBlock().hash(end_hash));
@@ -2439,15 +2439,12 @@ DBErrors CWallet::LoadWallet()
         }
     }
 
-    if (nLoadWalletRet != DBErrors::LOAD_OK)
-        return nLoadWalletRet;
-
     /* If the CoinJoin salt is not set, try to set a new random hash as the salt */
-    if (GetCoinJoinSalt().IsNull() && !SetCoinJoinSalt(GetRandHash())) {
+    if (nLoadWalletRet == DBErrors::LOAD_OK && GetCoinJoinSalt().IsNull() && !SetCoinJoinSalt(GetRandHash())) {
         return DBErrors::LOAD_FAIL;
     }
 
-    return DBErrors::LOAD_OK;
+    return nLoadWalletRet;
 }
 
 // Goes through all wallet transactions and checks if they are masternode collaterals, in which case these are locked
@@ -3102,6 +3099,7 @@ std::shared_ptr<CWallet> CWallet::Create(WalletContext& context, const std::stri
     if (!walletInstance->AutoBackupWallet(fs::PathFromString(walletFile), error, warnings) && !error.original.empty()) {
         return nullptr;
     }
+    bool rescan_required = false;
     DBErrors nLoadWalletRet = walletInstance->LoadWallet();
     if (nLoadWalletRet != DBErrors::LOAD_OK)
     {
@@ -3127,6 +3125,10 @@ std::shared_ptr<CWallet> CWallet::Create(WalletContext& context, const std::stri
         {
             error = strprintf(_("Wallet needed to be rewritten: restart %s to complete"), PACKAGE_NAME);
             return nullptr;
+        } else if (nLoadWalletRet == DBErrors::NEED_RESCAN) {
+            warnings.push_back(strprintf(_("Error reading %s! Transaction data may be missing or incorrect."
+                                           " Rescanning wallet."), walletFile));
+            rescan_required = true;
         }
         else {
             error = strprintf(_("Error loading %s"), walletFile);
@@ -3382,7 +3384,7 @@ std::shared_ptr<CWallet> CWallet::Create(WalletContext& context, const std::stri
 
     NotifyWalletLoading(context, walletInstance);
 
-    if (chain && !AttachChain(walletInstance, *chain, error, warnings)) {
+    if (chain && !AttachChain(walletInstance, *chain, rescan_required, error, warnings)) {
         walletInstance->m_chain_notifications_handler.reset(); // Reset this pointer so that the wallet will actually be unloaded
         return nullptr;
     }
@@ -3407,7 +3409,7 @@ std::shared_ptr<CWallet> CWallet::Create(WalletContext& context, const std::stri
     return walletInstance;
 }
 
-bool CWallet::AttachChain(const std::shared_ptr<CWallet>& walletInstance, interfaces::Chain& chain, bilingual_str& error, std::vector<bilingual_str>& warnings)
+bool CWallet::AttachChain(const std::shared_ptr<CWallet>& walletInstance, interfaces::Chain& chain, const bool rescan_required, bilingual_str& error, std::vector<bilingual_str>& warnings)
 {
     LOCK(walletInstance->cs_wallet);
     // allow setting the chain if it hasn't been set already but prevent changing it
@@ -3441,8 +3443,9 @@ bool CWallet::AttachChain(const std::shared_ptr<CWallet>& walletInstance, interf
     walletInstance->m_attaching_chain = true; //ignores chainStateFlushed notifications
     walletInstance->m_chain_notifications_handler = walletInstance->chain().handleNotifications(walletInstance);
 
+    // If rescan_required = true, rescan_height remains equal to 0
     int rescan_height = 0;
-    if (!gArgs.GetBoolArg("-rescan", false))
+    if (!rescan_required)
     {
         WalletBatch batch(walletInstance->GetDatabase());
         CBlockLocator locator;

src/wallet/wallet.h

@@ -441,7 +441,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
      * block locator and m_last_block_processed, and registering for
      * notifications about new blocks and transactions.
      */
-    static bool AttachChain(const std::shared_ptr<CWallet>& wallet, interfaces::Chain& chain, bilingual_str& error, std::vector<bilingual_str>& warnings);
+    static bool AttachChain(const std::shared_ptr<CWallet>& wallet, interfaces::Chain& chain, const bool rescan_required, bilingual_str& error, std::vector<bilingual_str>& warnings);
 
 public:
     /**

src/wallet/walletdb.cpp

@@ -813,6 +813,7 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
 {
     CWalletScanState wss;
     bool fNoncriticalErrors = false;
+    bool rescan_required = false;
     DBErrors result = DBErrors::LOAD_OK;
 
     LOCK(pwallet->cs_wallet);
@@ -886,7 +887,7 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
                     fNoncriticalErrors = true; // ... but do warn the user there is something wrong.
                     if (strType == DBKeys::TX)
                         // Rescan if there is a bad transaction record:
-                        gArgs.SoftSetBoolArg("-rescan", true);
+                        rescan_required = true;
                 }
             }
             if (!strErr.empty())
@@ -950,8 +951,11 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
         }
     }
 
-    if (fNoncriticalErrors && result == DBErrors::LOAD_OK)
+    if (rescan_required && result == DBErrors::LOAD_OK) {
+        result = DBErrors::NEED_RESCAN;
+    } else if (fNoncriticalErrors && result == DBErrors::LOAD_OK) {
         result = DBErrors::NONCRITICAL_ERROR;
+    }
 
     // Any wallet corruption at all: skip any rewriting or
     // upgrading, we don't want to make it worse.

src/wallet/walletdb.h

@@ -56,7 +56,8 @@ enum class DBErrors
     TOO_NEW,
     EXTERNAL_SIGNER_SUPPORT_REQUIRED,
     LOAD_FAIL,
-    NEED_REWRITE
+    NEED_REWRITE,
+    NEED_RESCAN
 };
 
 namespace DBKeys {

src/wallet/wallettool.cpp

@@ -92,6 +92,10 @@ static std::shared_ptr<CWallet> MakeWallet(const std::string& name, const fs::pa
         } else if (load_wallet_ret == DBErrors::NEED_REWRITE) {
             tfm::format(std::cerr, "Wallet needed to be rewritten: restart %s to complete", PACKAGE_NAME);
             return nullptr;
+        } else if (load_wallet_ret == DBErrors::NEED_RESCAN) {
+            tfm::format(std::cerr, "Error reading %s! Some transaction data might be missing or"
+                           " incorrect. Wallet requires a rescan.",
+                name);
         } else {
             tfm::format(std::cerr, "Error loading %s", name);
             return nullptr;

test/functional/feature_notifications.py

@@ -11,7 +11,6 @@
 from test_framework.test_framework import DashTestFramework
 from test_framework.util import (
     assert_equal,
-    force_finish_mnsync,
 )
 
 # Linux allow all characters other than \x00
@@ -56,7 +55,6 @@ def setup_network(self):
             f"-shutdownnotify=echo > {self.shutdownnotify_file}",
             f"-chainlocknotify=echo > {os.path.join(self.chainlocknotify_dir, '%s')}",
         ], [
-            "-rescan",
             f"-walletnotify=echo %h_%b > {os.path.join(self.walletnotify_dir, notify_outputname('%w', '%s'))}",
             f"-instantsendnotify=echo > {os.path.join(self.instantsendnotify_dir, notify_outputname('%w', '%s'))}",
         ],
@@ -89,17 +87,15 @@ def run_test(self):
 
             # directory content should equal the generated transaction hashes
             tx_details = list(map(lambda t: (t['txid'], t['blockheight'], t['blockhash']), self.nodes[1].listtransactions("*", block_count)))
-            self.stop_node(1)
             self.expect_wallet_notify(tx_details)
 
             self.log.info("test -walletnotify after rescan")
-            # restart node to rescan to force wallet notifications
-            self.start_node(1)
-            force_finish_mnsync(self.nodes[1])
-            self.connect_nodes(0, 1)
-
+            # rescan to force wallet notifications
+            self.nodes[1].rescanblockchain()
             self.wait_until(lambda: len(os.listdir(self.walletnotify_dir)) == block_count, timeout=10)
 
+            self.connect_nodes(0, 1)
+
             # directory content should equal the generated transaction hashes
             tx_details = list(map(lambda t: (t['txid'], t['blockheight'], t['blockhash']), self.nodes[1].listtransactions("*", block_count)))
             self.expect_wallet_notify(tx_details)

test/functional/wallet_basic.py

@@ -620,23 +620,17 @@ def run_test(self):
                 assert label in self.nodes[0].listlabels()
         self.nodes[0].rpc.ensure_ascii = True  # restore to default
 
-        # maintenance tests
-        maintenance = [
-            '-rescan',
-            '-reindex',
-        ]
+        # -reindex tests
         chainlimit = 6
-        for m in maintenance:
-            self.log.info("Test " + m)
-            self.stop_nodes()
-            # set lower ancestor limit for later
-            self.start_node(0, [m, "-walletrejectlongchains=0", "-limitancestorcount=" + str(chainlimit)])
-            self.start_node(1, [m, "-limitancestorcount=" + str(chainlimit)])
-            self.start_node(2, [m, "-limitancestorcount=" + str(chainlimit)])
-            if m == '-reindex':
-                # reindex will leave rpc warm up "early"; Wait for it to finish
-                self.wait_until(lambda: [block_count] * 3 == [self.nodes[i].getblockcount() for i in range(3)])
-            assert_equal(balance_nodes, [self.nodes[i].getbalance() for i in range(3)])
+        self.log.info("Test -reindex")
+        self.stop_nodes()
+        # set lower ancestor limit for later
+        self.start_node(0, ["-reindex", "-walletrejectlongchains=0", "-limitancestorcount=" + str(chainlimit)])
+        self.start_node(1, ["-reindex", "-limitancestorcount=" + str(chainlimit)])
+        self.start_node(2, ["-reindex", "-limitancestorcount=" + str(chainlimit)])
+        # reindex will leave rpc warm up "early"; Wait for it to finish
+        self.wait_until(lambda: [block_count] * 3 == [self.nodes[i].getblockcount() for i in range(3)])
+        assert_equal(balance_nodes, [self.nodes[i].getbalance() for i in range(3)])
 
         # Exercise listsinceblock with the last two blocks
         coinbase_tx_1 = self.nodes[0].listsinceblock(blocks[0])

test/functional/wallet_hd.py

@@ -109,7 +109,7 @@ def run_test(self):
         self.sync_all()
 
         # Needs rescan
-        self.restart_node(1, extra_args=self.extra_args[1] + ['-rescan'])
+        self.nodes[1].rescanblockchain()
         assert_equal(self.nodes[1].getbalance(), NUM_HD_ADDS + 1)
 
         # Try a RPC based rescan

test/functional/wallet_upgradetohd.py

@@ -188,9 +188,7 @@ def run_test(self):
         self.log.info("Same mnemonic, same mnemonic passphrase, encrypt wallet on upgrade, should recover all coins after rescan")
         walletpass = "111pass222"
         assert node.upgradetohd(mnemonic[0], "", walletpass)
-        node.stop()
-        node.wait_until_stopped()
-        self.start_node(0, extra_args=['-rescan'])
+        self.nodes[0].rescanblockchain()
         assert_raises_rpc_error(-13, "Error: Please enter the wallet passphrase with walletpassphrase first.", node.dumphdinfo)
         node.walletpassphrase(walletpass, 100)
         assert_equal(mnemonic, get_mnemonic(node))
@@ -211,9 +209,7 @@ def run_test(self):
         # Null characters are allowed in wallet passphrases since v23
         walletpass = "111\0pass222"
         node.encryptwallet(walletpass)
-        node.stop()
-        node.wait_until_stopped()
-        self.start_node(0, extra_args=['-rescan'])
+        self.nodes[0].rescanblockchain()
         assert_raises_rpc_error(-13, "Error: Wallet encrypted but passphrase not supplied to RPC.", node.upgradetohd, mnemonic[0])
         assert_raises_rpc_error(-14, "Error: The wallet passphrase entered was incorrect", node.upgradetohd, mnemonic[0], "", "111")
         assert node.upgradetohd(mnemonic[0], "", walletpass)