foster/bitcoin-bitcoin-core
0
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-02-01 09:35:52 -05:00
Code Issues Activity

Merge bitcoin/bitcoin#30909: wallet, assumeutxo: Don't Assume m_chain_tx_count, Improve wallet RPC errors

Browse source 
9d2d9f7ce2 rpc: Include assumeutxo as a failure reason of rescanblockchain (Fabian Jahr)
595edee169 test, assumeutxo: import descriptors during background sync (Alfonso Roman Zubeldia)
d73ae603d4 rpc: Improve importdescriptor RPC error messages (Fabian Jahr)
27f99b6d63 validation: Don't assume m_chain_tx_count in GuessVerificationProgress (Fabian Jahr)
42d5d53363 interfaces: Add helper function for wallet on pruning (Fabian Jahr)

Pull request description:

  A test that is added as part of #30455 uncovered this issue: The `GuessVerificationProgress` function is used during during descriptor import and relies on `m_chain_tx_count`. In #29370 an [`Assume` was added](0fd915ee6b) expecting the `m_chaint_tx_count` to be set. However, as the test uncovered, `GuessVerificationProgress` is called with background sync blocks that have `m_chaint_tx_count = 0` when they have not been downloaded and processed yet.

  The simple fix is to remove the `Assume`. Users should not be thrown off by the `Internal bug detected` error. The behavior of `importdescriptor` is kept consistent with the behavior for blocks missing due to pruning.

  The test by alfonsoromanz is cherry-picked here to show that the [CI errors](https://cirrus-ci.com/task/5110045812195328?logs=ci#L2535) should be fixed by this change.

  This PR also improves error messages returned by the `importdescriptors` and `rescanblockchain` RPCs. The error message now changes depending on the situation of the node, i.e. if pruning is happening or an assumutxo backgroundsync is active.

ACKs for top commit:
  achow101:
    ACK 9d2d9f7ce2
  mzumsande:
    Code Review ACK 9d2d9f7ce2
  furszy:
    Code review ACK 9d2d9f7ce2

Tree-SHA512: b841a9b371e5eb8eb3bfebca35645ff2fdded7a3e5e06308d46a33a51ca42cc4c258028c9958fbbb6cda9bb990e07ab8d8504dd9ec6705ef78afe0435912b365
This commit is contained in:
Ava Chow 2025-01-31 15:45:14 -05:00
commit 85f96b01b7
No known key found for this signature in database
GPG key ID: 17565732E08E5E41
6 changed files with 68 additions and 18 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

3
src/interfaces/chain.h
View file

@ -289,6 +289,9 @@ public:
//! Check if any block has been pruned.
virtual bool havePruned() = 0;
//! Get the current prune height.
virtual std::optional<int> getPruneHeight() = 0;
//! Check if the node is ready to broadcast transactions.
virtual bool isReadyToBroadcast() = 0;

6
src/node/interfaces.cpp
View file

@ -46,6 +46,7 @@
#include <policy/settings.h>
#include <primitives/block.h>
#include <primitives/transaction.h>
#include <rpc/blockchain.h>
#include <rpc/protocol.h>
#include <rpc/server.h>
#include <support/allocators/secure.h>
@ -770,6 +771,11 @@ public:
LOCK(::cs_main);
return chainman().m_blockman.m_have_pruned;
}
std::optional<int> getPruneHeight() override
{
LOCK(chainman().GetMutex());
return GetPruneHeight(chainman().m_blockman, chainman().ActiveChain());
}
bool isReadyToBroadcast() override { return !chainman().m_blockman.LoadingBlocks() && !isInitialBlockDownload(); }
bool isInitialBlockDownload() override
{

5
src/validation.cpp
View file

@ -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;
}

31
src/wallet/rpc/backup.cpp
View file

@ -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));
}
}

9
src/wallet/rpc/transactions.cpp
View file

@ -6,6 +6,7 @@
#include <key_io.h>
#include <policy/rbf.h>
#include <rpc/util.h>
#include <rpc/blockchain.h>
#include <util/vector.h>
#include <wallet/receive.h>
#include <wallet/rpc/util.h>
@ -909,10 +910,16 @@ 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)) {
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)));
}

30
test/functional/wallet_assumeutxo.py
View file

@ -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,21 @@ 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)
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)
@ -204,6 +227,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()