From b9ab4c2e87b7df2effa71c74b817c884ce51a25d Mon Sep 17 00:00:00 2001 From: "W. J. van der Laan" Date: Thu, 30 Sep 2021 20:43:41 +0200 Subject: [PATCH] Merge bitcoin/bitcoin#23123: Remove `-rescan` startup parameter dc3ec74d67abc85e8f724648f93efdd097e6f783 Add rescan removal release note (Samuel Dobson) bccd1d942d971e70e7a0f4f5628e1b74b3ac15e0 Remove -rescan startup parameter (Samuel Dobson) f963b0fa8cdd5223feb828c5faf6c57bc4107c8a Corrupt wallet tx shouldn't trigger rescan of all wallets (Samuel Dobson) 6c006495ef07f163d0734ec35d3cd1589a4aae9d Remove outdated dummy wallet -salvagewallet arg (Samuel Dobson) Pull request description: 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. ACKs for top commit: achow101: ACK dc3ec74d67abc85e8f724648f93efdd097e6f783 laanwj: re-ACK dc3ec74d67abc85e8f724648f93efdd097e6f783 Tree-SHA512: 608360d0e7d73737fd3ef408b01b33d97a75eebccd70c6d1b47a32fecb99b9105b520b111b225beb10611c09aa840a2b6d2b6e6e54be5d0362829e757289de5c --- contrib/debian/examples/dash.conf | 2 +- doc/release-notes-remove-rescan.md | 9 ++++++++ src/dummywallet.cpp | 2 -- src/init.cpp | 2 +- src/wallet/init.cpp | 2 -- src/wallet/rpcdump.cpp | 4 ++-- src/wallet/rpcwallet.cpp | 2 +- src/wallet/salvage.cpp | 2 +- src/wallet/test/wallet_tests.cpp | 4 ++-- src/wallet/wallet.cpp | 23 +++++++++++++-------- src/wallet/wallet.h | 2 +- src/wallet/walletdb.cpp | 8 ++++++-- src/wallet/walletdb.h | 3 ++- src/wallet/wallettool.cpp | 4 ++++ test/functional/feature_notifications.py | 11 ++++------ test/functional/wallet_basic.py | 26 +++++++++--------------- test/functional/wallet_hd.py | 2 +- 17 files changed, 60 insertions(+), 48 deletions(-) create mode 100644 doc/release-notes-remove-rescan.md diff --git a/contrib/debian/examples/dash.conf b/contrib/debian/examples/dash.conf index 3bb5690ca4968..4f3644c800fc4 100644 --- a/contrib/debian/examples/dash.conf +++ b/contrib/debian/examples/dash.conf @@ -154,7 +154,7 @@ #coinstatsindex=1 # Enable pruning to reduce storage requirements by deleting old blocks. -# This mode is incompatible with -txindex, -coinstatsindex and -rescan. +# This mode is incompatible with -txindex and -coinstatsindex. # 0 = default (no pruning). # 1 = allows manual pruning via RPC. # >=945 = target to stay under in MiB. diff --git a/doc/release-notes-remove-rescan.md b/doc/release-notes-remove-rescan.md new file mode 100644 index 0000000000000..fcbe7e6a82881 --- /dev/null +++ b/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. \ No newline at end of file diff --git a/src/dummywallet.cpp b/src/dummywallet.cpp index a9e5ae9486a8f..7085ec37b2e2a 100644 --- a/src/dummywallet.cpp +++ b/src/dummywallet.cpp @@ -38,8 +38,6 @@ void DummyWalletInit::AddWalletOptions(ArgsManager& argsman) const "-keypool=", "-maxapsfee=", "-maxtxfee=", - "-rescan=", - "-salvagewallet", "-spendzeroconfchange", "-wallet=", "-walletbackupsdir=", diff --git a/src/init.cpp b/src/init.cpp index 18decacf04163..5fac147141969 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -539,7 +539,7 @@ void SetupServerArgs(ArgsManager& argsman) -GetNumCores(), MAX_SCRIPTCHECK_THREADS, DEFAULT_SCRIPTCHECK_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=", 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=", 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, -coinstatsindex, -rescan and -disablegovernance=false. " + argsman.AddArg("-prune=", 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, 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=", 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); diff --git a/src/wallet/init.cpp b/src/wallet/init.cpp index 15b3306f82e13..6023ff90b8606 100644 --- a/src/wallet/init.cpp +++ b/src/wallet/init.cpp @@ -61,8 +61,6 @@ void WalletInit::AddWalletOptions(ArgsManager& argsman) const argsman.AddArg("-instantsendnotify=", "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=", strprintf("Set key pool size to (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=", "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); argsman.AddArg("-spendzeroconfchange", strprintf("Spend unconfirmed change when sending transactions (default: %u)", DEFAULT_SPEND_ZEROCONF_CHANGE), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET); argsman.AddArg("-wallet=", "Specify wallet path to load at startup. Can be used multiple times to load multiple wallets. Path is to a directory containing wallet data and log files. If the path is not absolute, it is interpreted relative to . This only loads existing wallets and does not create new ones. For backwards compatibility this also accepts names of existing top-level data files in .", ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::WALLET); argsman.AddArg("-walletbackupsdir=", "Specify full path to directory for automatic wallet backups (must exist)", ArgsManager::ALLOW_ANY, OptionsCategory::WALLET); diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp index 16ac2596ab11d..0e1f17dde48c2 100644 --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -1626,7 +1626,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)); } @@ -1924,7 +1924,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 bitcoind 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)); } diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index fdea130b65c7a..0609553bc9650 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -2856,7 +2856,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, /* default */ "null", "Save wallet name to persistent settings and load on startup. True to add wallet to startup list, false to remove, null to leave unchanged."}, diff --git a/src/wallet/salvage.cpp b/src/wallet/salvage.cpp index 40efed40e60d6..05d65333be328 100644 --- a/src/wallet/salvage.cpp +++ b/src/wallet/salvage.cpp @@ -45,7 +45,7 @@ bool RecoverDatabaseFile(const fs::path& file_path, bilingual_str& error, std::v // 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); diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index ef6218ed6f712..96ccc0ff0e546 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -248,8 +248,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).\"}},{\"success\":true}]", 0, oldTip->GetBlockTimeMax(), TIMESTAMP_WINDOW)); RemoveWallet(wallet, std::nullopt); } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 14ba64998d6ac..52d52d7b8a78c 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2033,7 +2033,7 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc WalletLogPrintf("Rescan started from block %s...\n", start_block.ToString()); - 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)); @@ -4014,15 +4014,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())) { 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 @@ -4702,6 +4699,7 @@ std::shared_ptr CWallet::Create(interfaces::Chain* chain, interfaces::C // TODO: Can't use std::make_shared because we need a custom deleter but // should be possible to use std::allocate_shared. std::shared_ptr walletInstance(new CWallet(chain, coinjoin_loader, name, std::move(database)), ReleaseWallet); + bool rescan_required = false; // TODO: refactor this condition: validation of error looks like workaround if (!walletInstance->AutoBackupWallet(fs::PathFromString(walletFile), error, warnings) && !error.original.empty()) { return nullptr; @@ -4727,6 +4725,14 @@ std::shared_ptr CWallet::Create(interfaces::Chain* chain, interfaces::C { error = strprintf(_("Wallet needed to be rewritten: restart %s to complete"), PACKAGE_NAME); return nullptr; + } else if (nLoadWalletRet == DBErrors::RESCAN_REQUIRED) { + warnings.push_back(strprintf(_("Error reading %s! Transaction data may be missing or incorrect." + " Rescanning wallet."), walletFile)); + rescan_required = true; + } else if (nLoadWalletRet == DBErrors::RESCAN_REQUIRED) { + 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); @@ -4949,7 +4955,7 @@ std::shared_ptr CWallet::Create(interfaces::Chain* chain, interfaces::C // Try to top up keypool. No-op if the wallet is locked. walletInstance->TopUpKeyPool(); - if (chain && !AttachChain(walletInstance, *chain, error, warnings)) { + if (chain && !AttachChain(walletInstance, *chain, rescan_required, error, warnings)) { return nullptr; } @@ -4980,7 +4986,7 @@ std::shared_ptr CWallet::Create(interfaces::Chain* chain, interfaces::C return walletInstance; } -bool CWallet::AttachChain(const std::shared_ptr& walletInstance, interfaces::Chain& chain, bilingual_str& error, std::vector& warnings) +bool CWallet::AttachChain(const std::shared_ptr& walletInstance, interfaces::Chain& chain, const bool rescan_required, bilingual_str& error, std::vector& warnings) { LOCK(walletInstance->cs_wallet); // allow setting the chain if it hasn't been set already but prevent changing it @@ -5000,8 +5006,9 @@ bool CWallet::AttachChain(const std::shared_ptr& 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; diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 194d734061595..3b6b25ea5d135 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -853,7 +853,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& wallet, interfaces::Chain& chain, bilingual_str& error, std::vector& warnings); + static bool AttachChain(const std::shared_ptr& wallet, interfaces::Chain& chain, const bool rescan_required, bilingual_str& error, std::vector& warnings); public: /** diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index 9042bd25bcd68..9732325392495 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -741,6 +741,7 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet) { CWalletScanState wss; bool fNoncriticalErrors = false; + bool rescan_required = false; DBErrors result = DBErrors::LOAD_OK; LOCK(pwallet->cs_wallet); @@ -802,7 +803,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()) @@ -842,8 +843,11 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet) ((DescriptorScriptPubKeyMan*)spk_man)->AddCryptedKey(desc_key_pair.first.second, desc_key_pair.second.first, desc_key_pair.second.second); } - if (fNoncriticalErrors && result == DBErrors::LOAD_OK) + if (rescan_required && result == DBErrors::LOAD_OK) { + result = DBErrors::RESCAN_REQUIRED; + } 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. diff --git a/src/wallet/walletdb.h b/src/wallet/walletdb.h index 7c4427123dc6d..b8bd73740bfc6 100644 --- a/src/wallet/walletdb.h +++ b/src/wallet/walletdb.h @@ -54,7 +54,8 @@ enum class DBErrors NONCRITICAL_ERROR, TOO_NEW, LOAD_FAIL, - NEED_REWRITE + NEED_REWRITE, + RESCAN_REQUIRED }; namespace DBKeys { diff --git a/src/wallet/wallettool.cpp b/src/wallet/wallettool.cpp index 96ca344c4e545..0c7bbfb7333e4 100644 --- a/src/wallet/wallettool.cpp +++ b/src/wallet/wallettool.cpp @@ -86,6 +86,10 @@ static std::shared_ptr 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::RESCAN_REQUIRED) { + 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; diff --git a/test/functional/feature_notifications.py b/test/functional/feature_notifications.py index 44957d0eecec1..0e0e52705fc79 100755 --- a/test/functional/feature_notifications.py +++ b/test/functional/feature_notifications.py @@ -45,7 +45,6 @@ def setup_network(self): # -alertnotify and -blocknotify on node0, walletnotify on node1 self.extra_args[0].append("-alertnotify=echo > {}".format(os.path.join(self.alertnotify_dir, '%s'))) self.extra_args[0].append("-blocknotify=echo > {}".format(os.path.join(self.blocknotify_dir, '%s'))) - self.extra_args[1].append("-rescan") self.extra_args[1].append("-walletnotify=echo %h_%b > {}".format(os.path.join(self.walletnotify_dir, notify_outputname('%w', '%s')))) # -chainlocknotify on node0, -instantsendnotify on node1 @@ -79,17 +78,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) diff --git a/test/functional/wallet_basic.py b/test/functional/wallet_basic.py index ea2b49b994946..1d84785c77ebe 100755 --- a/test/functional/wallet_basic.py +++ b/test/functional/wallet_basic.py @@ -540,23 +540,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("check " + m) - self.stop_nodes() - # set lower ancestor limit for later - self.start_node(0, [m, "-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', "-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]) diff --git a/test/functional/wallet_hd.py b/test/functional/wallet_hd.py index a9b7e052e878f..24999a982ecda 100755 --- a/test/functional/wallet_hd.py +++ b/test/functional/wallet_hd.py @@ -102,7 +102,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