From 42d5d5336319aaf0f07345037db78239d9e012fc Mon Sep 17 00:00:00 2001 From: Fabian Jahr Date: Mon, 30 Sep 2024 01:42:47 +0200 Subject: [PATCH 1/5] interfaces: Add helper function for wallet on pruning --- src/interfaces/chain.h | 3 +++ src/node/interfaces.cpp | 6 ++++++ 2 files changed, 9 insertions(+) diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h index 4e858d1f898..c9ef46243c4 100644 --- a/src/interfaces/chain.h +++ b/src/interfaces/chain.h @@ -289,6 +289,9 @@ public: //! Check if any block has been pruned. virtual bool havePruned() = 0; + //! Get the current prune height. + virtual std::optional getPruneHeight() = 0; + //! Check if the node is ready to broadcast transactions. virtual bool isReadyToBroadcast() = 0; diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index f3b8c6a072f..15140b942f6 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -46,6 +46,7 @@ #include #include #include +#include #include #include #include @@ -770,6 +771,11 @@ public: LOCK(::cs_main); return chainman().m_blockman.m_have_pruned; } + std::optional getPruneHeight() override + { + LOCK(chainman().GetMutex()); + return GetPruneHeight(chainman().m_blockman, chainman().ActiveChain()); + } bool isReadyToBroadcast() override { return !chainman().m_blockman.LoadingBlocks() && !isInitialBlockDownload(); } bool isInitialBlockDownload() override { From 27f99b6d63b7ca2d4fcb9db3e88ed66c024c59d5 Mon Sep 17 00:00:00 2001 From: Fabian Jahr Date: Tue, 1 Oct 2024 01:38:38 +0200 Subject: [PATCH 2/5] validation: Don't assume m_chain_tx_count in GuessVerificationProgress In the context of an a descriptor import during assumeutxo background sync, the progress can not be estimated due to m_chain_tx_count being set to 0. --- src/validation.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 3e8a7cf520e..25ba5107603 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -5623,9 +5623,8 @@ double ChainstateManager::GuessVerificationProgress(const CBlockIndex* pindex) c return 0.0; } - if (!Assume(pindex->m_chain_tx_count > 0)) { - LogWarning("Internal bug detected: block %d has unset m_chain_tx_count (%s %s). Please report this issue here: %s\n", - pindex->nHeight, CLIENT_NAME, FormatFullVersion(), CLIENT_BUGREPORT); + if (pindex->m_chain_tx_count == 0) { + LogDebug(BCLog::VALIDATION, "Block %d has unset m_chain_tx_count. Unable to estimate verification progress.\n", pindex->nHeight); return 0.0; } From d73ae603d44f93e4d6c5116f235dd11a0bdbf89c Mon Sep 17 00:00:00 2001 From: Fabian Jahr Date: Tue, 1 Oct 2024 01:39:58 +0200 Subject: [PATCH 3/5] rpc: Improve importdescriptor RPC error messages Particularly add more details in the case of pruning or assumeutxo. --- src/wallet/rpc/backup.cpp | 31 +++++++++++++++++++------------ 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp index bfd7249c046..823efb878e1 100644 --- a/src/wallet/rpc/backup.cpp +++ b/src/wallet/rpc/backup.cpp @@ -1745,20 +1745,27 @@ RPCHelpMan importdescriptors() if (scanned_time <= GetImportTimestamp(request, now) || results.at(i).exists("error")) { response.push_back(results.at(i)); } else { + std::string error_msg{strprintf("Rescan failed for descriptor with timestamp %d. There " + "was an error reading a block from time %d, which is after or within %d seconds " + "of key creation, and could contain transactions pertaining to the desc. As a " + "result, transactions and coins using this desc may not appear in the wallet.", + GetImportTimestamp(request, now), scanned_time - TIMESTAMP_WINDOW - 1, TIMESTAMP_WINDOW)}; + if (pwallet->chain().havePruned()) { + error_msg += strprintf(" 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 option and rescanblockchain RPC)."); + } else if (pwallet->chain().hasAssumedValidChain()) { + error_msg += strprintf(" This error is likely caused by an in-progress assumeutxo " + "background sync. Check logs or getchainstates RPC for assumeutxo background " + "sync progress and try again later."); + } else { + error_msg += strprintf(" This error could potentially caused by data corruption. If " + "the issue persists you may want to reindex (see -reindex option)."); + } + UniValue result = UniValue(UniValue::VOBJ); result.pushKV("success", UniValue(false)); - result.pushKV( - "error", - JSONRPCError( - RPC_MISC_ERROR, - strprintf("Rescan failed for descriptor with timestamp %d. There was an error reading a " - "block from time %d, which is after or within %d seconds of key creation, and " - "could contain transactions pertaining to the desc. As a result, transactions " - "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 " - "option and rescanblockchain RPC).", - GetImportTimestamp(request, now), scanned_time - TIMESTAMP_WINDOW - 1, TIMESTAMP_WINDOW))); + result.pushKV("error", JSONRPCError(RPC_MISC_ERROR, error_msg)); response.push_back(std::move(result)); } } From 595edee169045b6735b76ff9721677f0e43f13e5 Mon Sep 17 00:00:00 2001 From: Alfonso Roman Zubeldia <19962151+alfonsoromanz@users.noreply.github.com> Date: Thu, 22 Aug 2024 10:47:18 -0300 Subject: [PATCH 4/5] test, assumeutxo: import descriptors during background sync --- test/functional/wallet_assumeutxo.py | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/test/functional/wallet_assumeutxo.py b/test/functional/wallet_assumeutxo.py index ac904d6ce4a..192085c16be 100755 --- a/test/functional/wallet_assumeutxo.py +++ b/test/functional/wallet_assumeutxo.py @@ -7,11 +7,11 @@ See feature_assumeutxo.py for background. ## Possible test improvements -- TODO: test import descriptors while background sync is in progress - TODO: test loading a wallet (backup) on a pruned node """ from test_framework.address import address_to_scriptpubkey +from test_framework.descriptors import descsum_create from test_framework.test_framework import BitcoinTestFramework from test_framework.messages import COIN from test_framework.util import ( @@ -20,6 +20,7 @@ from test_framework.util import ( ensure_for, ) from test_framework.wallet import MiniWallet +from test_framework.wallet_util import get_generate_key START_HEIGHT = 199 SNAPSHOT_BASE_HEIGHT = 299 @@ -49,6 +50,13 @@ class AssumeutxoTest(BitcoinTestFramework): self.add_nodes(3) self.start_nodes(extra_args=self.extra_args) + def import_descriptor(self, node, wallet_name, key, timestamp): + import_request = [{"desc": descsum_create("pkh(" + key.pubkey + ")"), + "timestamp": timestamp, + "label": "Descriptor import test"}] + wrpc = node.get_wallet_rpc(wallet_name) + return wrpc.importdescriptors(import_request) + def run_test(self): """ Bring up two (disconnected) nodes, mine some new blocks on the first, @@ -157,6 +165,17 @@ class AssumeutxoTest(BitcoinTestFramework): self.log.info("Backup from before the snapshot height can't be loaded during background sync") assert_raises_rpc_error(-4, "Wallet loading failed. Error loading wallet. Wallet requires blocks to be downloaded, and software does not currently support loading wallets while blocks are being downloaded out of order when using assumeutxo snapshots. Wallet should be able to load successfully after node sync reaches height 299", n1.restorewallet, "w2", "backup_w2.dat") + self.log.info("Test loading descriptors during background sync") + wallet_name = "w1" + n1.createwallet(wallet_name, disable_private_keys=True) + key = get_generate_key() + time = n1.getblockchaininfo()['time'] + timestamp = 0 + expected_error_message = f"Rescan failed for descriptor with timestamp {timestamp}. There was an error reading a block from time {time}, which is after or within 7200 seconds of key creation, and could contain transactions pertaining to the desc. As a result, transactions and coins using this desc may not appear in the wallet. This error is likely caused by an in-progress assumeutxo background sync. Check logs or getchainstates RPC for assumeutxo background sync progress and try again later." + result = self.import_descriptor(n1, wallet_name, key, timestamp) + assert_equal(result[0]['error']['code'], -1) + assert_equal(result[0]['error']['message'], expected_error_message) + PAUSE_HEIGHT = FINAL_HEIGHT - 40 self.log.info("Restarting node to stop at height %d", PAUSE_HEIGHT) @@ -204,6 +223,11 @@ class AssumeutxoTest(BitcoinTestFramework): self.wait_until(lambda: len(n2.getchainstates()['chainstates']) == 1) ensure_for(duration=1, f=lambda: (n2.getbalance() == 34)) + self.log.info("Ensuring descriptors can be loaded after background sync") + n1.loadwallet(wallet_name) + result = self.import_descriptor(n1, wallet_name, key, timestamp) + assert_equal(result[0]['success'], True) + if __name__ == '__main__': AssumeutxoTest(__file__).main() From 9d2d9f7ce29636f08322df70cf6abec8e0ca3727 Mon Sep 17 00:00:00 2001 From: Fabian Jahr Date: Mon, 23 Sep 2024 21:05:37 +0200 Subject: [PATCH 5/5] rpc: Include assumeutxo as a failure reason of rescanblockchain --- src/wallet/rpc/transactions.cpp | 11 +++++++++-- test/functional/wallet_assumeutxo.py | 4 ++++ 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/src/wallet/rpc/transactions.cpp b/src/wallet/rpc/transactions.cpp index 61cf36a6c10..f3cfdfcc831 100644 --- a/src/wallet/rpc/transactions.cpp +++ b/src/wallet/rpc/transactions.cpp @@ -6,6 +6,7 @@ #include #include #include +#include #include #include #include @@ -909,9 +910,15 @@ RPCHelpMan rescanblockchain() } } - // We can't rescan beyond non-pruned blocks, stop and throw an error + // We can't rescan unavailable blocks, stop and throw an error if (!pwallet->chain().hasBlocks(pwallet->GetLastBlockHash(), start_height, stop_height)) { - throw JSONRPCError(RPC_MISC_ERROR, "Can't rescan beyond pruned data. Use RPC call getblockchaininfo to determine your pruned height."); + if (pwallet->chain().havePruned() && pwallet->chain().getPruneHeight() >= start_height) { + throw JSONRPCError(RPC_MISC_ERROR, "Can't rescan beyond pruned data. Use RPC call getblockchaininfo to determine your pruned height."); + } + if (pwallet->chain().hasAssumedValidChain()) { + throw JSONRPCError(RPC_MISC_ERROR, "Failed to rescan unavailable blocks likely due to an in-progress assumeutxo background sync. Check logs or getchainstates RPC for assumeutxo background sync progress and try again later."); + } + throw JSONRPCError(RPC_MISC_ERROR, "Failed to rescan unavailable blocks, potentially caused by data corruption. If the issue persists you may want to reindex (see -reindex option)."); } CHECK_NONFATAL(pwallet->chain().findAncestorByHeight(pwallet->GetLastBlockHash(), start_height, FoundBlock().hash(start_block))); diff --git a/test/functional/wallet_assumeutxo.py b/test/functional/wallet_assumeutxo.py index 192085c16be..d817d33f1c8 100755 --- a/test/functional/wallet_assumeutxo.py +++ b/test/functional/wallet_assumeutxo.py @@ -176,6 +176,10 @@ class AssumeutxoTest(BitcoinTestFramework): assert_equal(result[0]['error']['code'], -1) assert_equal(result[0]['error']['message'], expected_error_message) + self.log.info("Test that rescanning blocks from before the snapshot fails when blocks are not available from the background sync yet") + w1 = n1.get_wallet_rpc(wallet_name) + assert_raises_rpc_error(-1, "Failed to rescan unavailable blocks likely due to an in-progress assumeutxo background sync. Check logs or getchainstates RPC for assumeutxo background sync progress and try again later.", w1.rescanblockchain, 100) + PAUSE_HEIGHT = FINAL_HEIGHT - 40 self.log.info("Restarting node to stop at height %d", PAUSE_HEIGHT)