Compare commits

...

10 Commits

Author SHA1 Message Date
Sjors Provoost
a30a966dee
Merge cc1001f3bf17b31512c05fb359e09483a07fb2a3 into 5f4422d68dc3530c353af1f87499de1c864b60ad 2025-03-17 03:54:00 +01:00
merge-script
5f4422d68d
Merge bitcoin/bitcoin#32010: qa: Fix TxIndex race conditions
3301d2cbe8c3b76c97285d75fa59637cb6952d0b qa: Wait for txindex to avoid race condition (Hodlinator)
9bfb0d75ba10591cc6c9620f9fd1ecc0e55e7a48 qa: Remove unnecessary -txindex args (Hodlinator)
7ac281c19cd3d11f316dbbb3308eabf1ad4f26d6 qa: Add missing coverage of corrupt indexes (Hodlinator)

Pull request description:

  - Add synchronization in 3 places where if the Transaction Index happens to be slow, we get rare test failures when querying it for transactions (one such case experienced on Windows, prompting investigation).
  - Remove unnecessary TxIndex initialization in some tests.
  - Add some test coverage where TxIndex aspect could be tested in feature_init.py.

ACKs for top commit:
  fjahr:
    re-ACK 3301d2cbe8c3b76c97285d75fa59637cb6952d0b
  mzumsande:
    Code Review ACK 3301d2cbe8c3b76c97285d75fa59637cb6952d0b
  furszy:
    Code review ACK 3301d2cbe8c3b76c97285d75fa59637cb6952d0b
  Prabhat1308:
    Concept ACK [`3301d2c`](3301d2cbe8)

Tree-SHA512: 7c2019e38455f344856aaf6b381faafbd88d53dc88d13309deb718c1dcfbee4ccca7c7f1b66917395503a6f94c3b216a007ad432cc8b93d0309db9805f38d602
2025-03-17 10:28:14 +08:00
Sjors Provoost
cc1001f3bf
rpc: clarify longpoll behavior
Move the comparison to hashWatchedChain inside the while loop.

Although this early return prevents the GetTransactionsUpdated()
call in cases where the tip updates, it's only done to improve
readability. The check itself is very cheap (although a more
useful check might not be).

Also add code comments.
2025-03-13 12:12:17 +01:00
Sjors Provoost
db14ca3556
Have createNewBlock() wait for a tip
Additionally it returns null if the node started to shutdown before TipBlock() was set.
2025-03-13 12:12:17 +01:00
Sjors Provoost
64a2795fd4
rpc: handle shutdown during long poll and wait methods
The waitTipChanged() now returns nullopt if the node is shutting down.

Previously it would return the last known tip during shutdown, but
this creates an ambiguous circumstance in the scenario where the
node is started and quickly shutdown, before notifications().TipBlock()
is set.

The getblocktemplate, waitfornewblock and waitforblockheight RPC
are updated to handle this. Existing behavior is preserved.

Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
2025-03-13 12:12:17 +01:00
Sjors Provoost
a3bf43343f
rpc: drop unneeded IsRPCRunning() guards
This was preventing the (hidden) waitfornewblock, waitforblock and
waitforblockheight methods from being used in the GUI.

The check was added in d6a5dc4a2eaa0d7348804254ca09e75fc3a858ab
when these RPC methods were first introduced.

They could have been dropped when dca923150e3ac10a57c23a7e29e76516d32ec10d
refactored these methods to use waitTipChanged(), which already
checks for shutdown.

Making this change now simplifies the next commit.
2025-03-13 12:12:17 +01:00
Sjors Provoost
f9cf8bd0ab
Handle negative timeout for waitTipChanged() 2025-03-13 12:12:17 +01:00
Hodlinator
3301d2cbe8
qa: Wait for txindex to avoid race condition
Can be verified to be necessary through adding std::this_thread::sleep_for(0.5s) at the beginning of TxIndex::CustomAppend.
2025-03-10 15:24:16 +01:00
Hodlinator
9bfb0d75ba
qa: Remove unnecessary -txindex args
(Parent commit ensured indexes in feature_init.py are actually used, otherwise they would be removed here as well).
2025-03-07 22:22:31 +01:00
Hodlinator
7ac281c19c
qa: Add missing coverage of corrupt indexes 2025-03-07 22:22:31 +01:00
10 changed files with 133 additions and 49 deletions

View File

@ -89,20 +89,25 @@ public:
/**
* Waits for the connected tip to change. During node initialization, this will
* wait until the tip is connected.
* wait until the tip is connected (regardless of `timeout`).
*
* @param[in] current_tip block hash of the current chain tip. Function waits
* for the chain tip to differ from this.
* @param[in] timeout how long to wait for a new tip
* @returns Hash and height of the current chain tip after this call.
* @param[in] timeout how long to wait for a new tip (default is forever)
*
* @retval BlockRef hash and height of the current chain tip after this call.
* @retval std::nullopt if the node is shut down.
*/
virtual BlockRef waitTipChanged(uint256 current_tip, MillisecondsDouble timeout = MillisecondsDouble::max()) = 0;
virtual std::optional<BlockRef> waitTipChanged(uint256 current_tip, MillisecondsDouble timeout = MillisecondsDouble::max()) = 0;
/**
* Construct a new block template
* Construct a new block template.
*
* During node initialization, this will wait until the tip is connected.
*
* @param[in] options options for creating the block
* @returns a block template
* @retval BlockTemplate a block template.
* @retval std::nullptr if the node is shut down.
*/
virtual std::unique_ptr<BlockTemplate> createNewBlock(const node::BlockCreateOptions& options = {}) = 0;

View File

@ -1070,24 +1070,41 @@ public:
return BlockRef{tip->GetBlockHash(), tip->nHeight};
}
BlockRef waitTipChanged(uint256 current_tip, MillisecondsDouble timeout) override
std::optional<BlockRef> waitTipChanged(uint256 current_tip, MillisecondsDouble timeout) override
{
Assume(timeout >= 0ms); // No internal callers should use a negative timeout
if (timeout < 0ms) timeout = 0ms;
if (timeout > std::chrono::years{100}) timeout = std::chrono::years{100}; // Upper bound to avoid UB in std::chrono
auto deadline{std::chrono::steady_clock::now() + timeout};
{
WAIT_LOCK(notifications().m_tip_block_mutex, lock);
notifications().m_tip_block_cv.wait_for(lock, timeout, [&]() EXCLUSIVE_LOCKS_REQUIRED(notifications().m_tip_block_mutex) {
// We need to wait for m_tip_block to be set AND for the value
// to differ from the current_tip value.
return (notifications().TipBlock() && notifications().TipBlock() != current_tip) || chainman().m_interrupt;
// For callers convenience, wait longer than the provided timeout
// during startup for the tip to be non-null. That way this function
// always returns valid tip information when possible and only
// returns null when shutting down, not when timing out.
notifications().m_tip_block_cv.wait(lock, [&]() EXCLUSIVE_LOCKS_REQUIRED(notifications().m_tip_block_mutex) {
return notifications().TipBlock() || chainman().m_interrupt;
});
if (chainman().m_interrupt) return {};
// At this point TipBlock is set, so continue to wait until it is
// different then `current_tip` provided by caller.
notifications().m_tip_block_cv.wait_until(lock, deadline, [&]() EXCLUSIVE_LOCKS_REQUIRED(notifications().m_tip_block_mutex) {
return Assume(notifications().TipBlock()) != current_tip || chainman().m_interrupt;
});
}
// Must release m_tip_block_mutex before locking cs_main, to avoid deadlocks.
LOCK(::cs_main);
return BlockRef{chainman().ActiveChain().Tip()->GetBlockHash(), chainman().ActiveChain().Tip()->nHeight};
if (chainman().m_interrupt) return {};
// Must release m_tip_block_mutex before getTip() locks cs_main, to
// avoid deadlocks.
return getTip();
}
std::unique_ptr<BlockTemplate> createNewBlock(const BlockCreateOptions& options) override
{
// Ensure m_tip_block is set so consumers of BlockTemplate can rely on that.
if (!waitTipChanged(uint256::ZERO, MillisecondsDouble::max())) return {};
BlockAssembler::Options assemble_options{options};
ApplyArgsManOptions(*Assert(m_node.args), assemble_options);
return std::make_unique<BlockTemplateImpl>(assemble_options, BlockAssembler{chainman().ActiveChainstate(), context()->mempool.get(), assemble_options}.CreateNewBlock(), m_node);

View File

@ -64,6 +64,7 @@
using kernel::CCoinsStats;
using kernel::CoinStatsHashType;
using interfaces::BlockRef;
using interfaces::Mining;
using node::BlockManager;
using node::NodeContext;
@ -286,14 +287,17 @@ static RPCHelpMan waitfornewblock()
NodeContext& node = EnsureAnyNodeContext(request.context);
Mining& miner = EnsureMining(node);
auto block{CHECK_NONFATAL(miner.getTip()).value()};
if (IsRPCRunning()) {
block = timeout ? miner.waitTipChanged(block.hash, std::chrono::milliseconds(timeout)) : miner.waitTipChanged(block.hash);
}
// Abort if RPC came out of warmup too early
BlockRef current_block{CHECK_NONFATAL(miner.getTip()).value()};
std::optional<BlockRef> block = timeout ? miner.waitTipChanged(current_block.hash, std::chrono::milliseconds(timeout)) :
miner.waitTipChanged(current_block.hash);
// Return current block upon shutdown
if (block) current_block = *block;
UniValue ret(UniValue::VOBJ);
ret.pushKV("hash", block.hash.GetHex());
ret.pushKV("height", block.height);
ret.pushKV("hash", current_block.hash.GetHex());
ret.pushKV("height", current_block.height);
return ret;
},
};
@ -332,22 +336,28 @@ static RPCHelpMan waitforblock()
NodeContext& node = EnsureAnyNodeContext(request.context);
Mining& miner = EnsureMining(node);
auto block{CHECK_NONFATAL(miner.getTip()).value()};
// Abort if RPC came out of warmup too early
BlockRef current_block{CHECK_NONFATAL(miner.getTip()).value()};
const auto deadline{std::chrono::steady_clock::now() + 1ms * timeout};
while (IsRPCRunning() && block.hash != hash) {
while (current_block.hash != hash) {
std::optional<BlockRef> block;
if (timeout) {
auto now{std::chrono::steady_clock::now()};
if (now >= deadline) break;
const MillisecondsDouble remaining{deadline - now};
block = miner.waitTipChanged(block.hash, remaining);
block = miner.waitTipChanged(current_block.hash, remaining);
} else {
block = miner.waitTipChanged(block.hash);
block = miner.waitTipChanged(current_block.hash);
}
// Return current block upon shutdown
if (!block) break;
current_block = *block;
}
UniValue ret(UniValue::VOBJ);
ret.pushKV("hash", block.hash.GetHex());
ret.pushKV("height", block.height);
ret.pushKV("hash", current_block.hash.GetHex());
ret.pushKV("height", current_block.height);
return ret;
},
};
@ -387,23 +397,29 @@ static RPCHelpMan waitforblockheight()
NodeContext& node = EnsureAnyNodeContext(request.context);
Mining& miner = EnsureMining(node);
auto block{CHECK_NONFATAL(miner.getTip()).value()};
// Abort if RPC came out of warmup too early
BlockRef current_block{CHECK_NONFATAL(miner.getTip()).value()};
const auto deadline{std::chrono::steady_clock::now() + 1ms * timeout};
while (IsRPCRunning() && block.height < height) {
while (current_block.height < height) {
std::optional<BlockRef> block;
if (timeout) {
auto now{std::chrono::steady_clock::now()};
if (now >= deadline) break;
const MillisecondsDouble remaining{deadline - now};
block = miner.waitTipChanged(block.hash, remaining);
block = miner.waitTipChanged(current_block.hash, remaining);
} else {
block = miner.waitTipChanged(block.hash);
block = miner.waitTipChanged(current_block.hash);
}
// Return current block on shutdown
if (!block) break;
current_block = *block;
}
UniValue ret(UniValue::VOBJ);
ret.pushKV("hash", block.hash.GetHex());
ret.pushKV("height", block.height);
ret.pushKV("hash", current_block.hash.GetHex());
ret.pushKV("height", current_block.height);
return ret;
},
};

View File

@ -46,6 +46,7 @@
#include <memory>
#include <stdint.h>
using interfaces::BlockRef;
using interfaces::BlockTemplate;
using interfaces::Mining;
using node::BlockAssembler;
@ -775,9 +776,22 @@ static RPCHelpMan getblocktemplate()
static unsigned int nTransactionsUpdatedLast;
const CTxMemPool& mempool = EnsureMemPool(node);
if (!lpval.isNull())
{
// Wait to respond until either the best block changes, OR a minute has passed and there are more transactions
// Long Polling (BIP22)
if (!lpval.isNull()) {
/**
* Wait to respond until either the best block changes, OR there are more
* transactions.
*
* The check for new transactions first happens after 1 minute and
* subsequently every 10 seconds. BIP22 does not require this particular interval.
* On mainnet the mempool changes frequently enough that in practice this RPC
* returns after 60 seconds, or sooner if the best block changes.
*
* getblocktemplate is unlikely to be called by bitcoin-cli, so
* -rpcclienttimeout is not a concern. BIP22 recommends a long request timeout.
*
* The longpollid is assumed to be a tip hash if it has the right format.
*/
uint256 hashWatchedChain;
unsigned int nTransactionsUpdatedLastLP;
@ -786,6 +800,8 @@ static RPCHelpMan getblocktemplate()
// Format: <hashBestChain><nTransactionsUpdatedLast>
const std::string& lpstr = lpval.get_str();
// Assume the longpollid is a block hash. If it's not then we return
// early below.
hashWatchedChain = ParseHashV(lpstr.substr(0, 64), "longpollid");
nTransactionsUpdatedLastLP = LocaleIndependentAtoi<int64_t>(lpstr.substr(64));
}
@ -800,12 +816,20 @@ static RPCHelpMan getblocktemplate()
LEAVE_CRITICAL_SECTION(cs_main);
{
MillisecondsDouble checktxtime{std::chrono::minutes(1)};
while (tip == hashWatchedChain && IsRPCRunning()) {
tip = miner.waitTipChanged(hashWatchedChain, checktxtime).hash;
// Timeout: Check transactions for update
// without holding the mempool lock to avoid deadlocks
if (mempool.GetTransactionsUpdated() != nTransactionsUpdatedLastLP)
while (IsRPCRunning()) {
// If hashWatchedChain is not a real block hash, this will
// return immediately.
std::optional<BlockRef> maybe_tip{miner.waitTipChanged(hashWatchedChain, checktxtime)};
// Node is shutting down
if (!maybe_tip) break;
tip = maybe_tip->hash;
if (tip != hashWatchedChain) break;
// Check transactions for update without holding the mempool
// lock to avoid deadlocks.
if (mempool.GetTransactionsUpdated() != nTransactionsUpdatedLastLP) {
break;
}
checktxtime = std::chrono::seconds(10);
}
}

View File

@ -88,7 +88,7 @@ class InitTest(BitcoinTestFramework):
args = ['-txindex=1', '-blockfilterindex=1', '-coinstatsindex=1']
for terminate_line in lines_to_terminate_after:
self.log.info(f"Starting node and will exit after line {terminate_line}")
self.log.info(f"Starting node and will terminate after line {terminate_line}")
with node.busy_wait_for_debug_log([terminate_line]):
if platform.system() == 'Windows':
# CREATE_NEW_PROCESS_GROUP is required in order to be able
@ -108,12 +108,22 @@ class InitTest(BitcoinTestFramework):
'blocks/index/*.ldb': 'Error opening block database.',
'chainstate/*.ldb': 'Error opening coins database.',
'blocks/blk*.dat': 'Error loading block database.',
'indexes/txindex/MANIFEST*': 'LevelDB error: Corruption: CURRENT points to a non-existent file',
# Removing these files does not result in a startup error:
# 'indexes/blockfilter/basic/*.dat', 'indexes/blockfilter/basic/db/*.*', 'indexes/coinstats/db/*.*',
# 'indexes/txindex/*.log', 'indexes/txindex/CURRENT', 'indexes/txindex/LOCK'
}
files_to_perturb = {
'blocks/index/*.ldb': 'Error loading block database.',
'chainstate/*.ldb': 'Error opening coins database.',
'blocks/blk*.dat': 'Corrupted block database detected.',
'indexes/blockfilter/basic/db/*.*': 'LevelDB error: Corruption',
'indexes/coinstats/db/*.*': 'LevelDB error: Corruption',
'indexes/txindex/*.log': 'LevelDB error: Corruption',
'indexes/txindex/CURRENT': 'LevelDB error: Corruption',
# Perturbing these files does not result in a startup error:
# 'indexes/blockfilter/basic/*.dat', 'indexes/txindex/MANIFEST*', 'indexes/txindex/LOCK'
}
for file_patt, err_fragment in files_to_delete.items():
@ -135,9 +145,10 @@ class InitTest(BitcoinTestFramework):
self.stop_node(0)
self.log.info("Test startup errors after perturbing certain essential files")
dirs = ["blocks", "chainstate", "indexes"]
for file_patt, err_fragment in files_to_perturb.items():
shutil.copytree(node.chain_path / "blocks", node.chain_path / "blocks_bak")
shutil.copytree(node.chain_path / "chainstate", node.chain_path / "chainstate_bak")
for dir in dirs:
shutil.copytree(node.chain_path / dir, node.chain_path / f"{dir}_bak")
target_files = list(node.chain_path.glob(file_patt))
for target_file in target_files:
@ -151,10 +162,9 @@ class InitTest(BitcoinTestFramework):
start_expecting_error(err_fragment)
shutil.rmtree(node.chain_path / "blocks")
shutil.rmtree(node.chain_path / "chainstate")
shutil.move(node.chain_path / "blocks_bak", node.chain_path / "blocks")
shutil.move(node.chain_path / "chainstate_bak", node.chain_path / "chainstate")
for dir in dirs:
shutil.rmtree(node.chain_path / dir)
shutil.move(node.chain_path / f"{dir}_bak", node.chain_path / dir)
def init_pid_test(self):
BITCOIN_PID_FILENAME_CUSTOM = "my_fancy_bitcoin_pid_file.foobar"

View File

@ -45,6 +45,7 @@ from test_framework.util import (
assert_equal,
assert_greater_than,
assert_raises_rpc_error,
sync_txindex,
)
from test_framework.wallet import MiniWallet
from test_framework.wallet_util import generate_keypair
@ -270,6 +271,7 @@ class MempoolAcceptanceTest(BitcoinTestFramework):
self.log.info('A coinbase transaction')
# Pick the input of the first tx we created, so it has to be a coinbase tx
sync_txindex(self, node)
raw_tx_coinbase_spent = node.getrawtransaction(txid=node.decoderawtransaction(hexstring=raw_tx_in_block)['vin'][0]['txid'])
tx = tx_from_hex(raw_tx_coinbase_spent)
self.check_mempool_result(

View File

@ -34,6 +34,7 @@ from test_framework.util import (
assert_equal,
assert_greater_than,
assert_raises_rpc_error,
sync_txindex,
)
from test_framework.wallet import (
getnewdestination,
@ -70,7 +71,7 @@ class RawTransactionsTest(BitcoinTestFramework):
self.num_nodes = 3
self.extra_args = [
["-txindex"],
["-txindex"],
[],
["-fastprune", "-prune=1"],
]
# whitelist peers to speed up tx relay / mempool sync
@ -109,6 +110,7 @@ class RawTransactionsTest(BitcoinTestFramework):
self.log.info(f"Test getrawtransaction {'with' if n == 0 else 'without'} -txindex")
if n == 0:
sync_txindex(self, self.nodes[n])
# With -txindex.
# 1. valid parameters - only supply txid
assert_equal(self.nodes[n].getrawtransaction(txId), tx['hex'])

View File

@ -12,6 +12,7 @@ from test_framework.test_framework import BitcoinTestFramework
from test_framework.util import (
assert_equal,
assert_raises_rpc_error,
sync_txindex,
)
from test_framework.wallet import MiniWallet
@ -77,6 +78,7 @@ class MerkleBlockTest(BitcoinTestFramework):
assert_equal(sorted(self.nodes[0].verifytxoutproof(self.nodes[0].gettxoutproof([txid1, txid2]))), sorted(txlist))
assert_equal(sorted(self.nodes[0].verifytxoutproof(self.nodes[0].gettxoutproof([txid2, txid1]))), sorted(txlist))
# We can always get a proof if we have a -txindex
sync_txindex(self, self.nodes[1])
assert_equal(self.nodes[0].verifytxoutproof(self.nodes[1].gettxoutproof([txid_spent])), [txid_spent])
# We can't get a proof if we specify transactions from different blocks
assert_raises_rpc_error(-5, "Not all transactions found in specified or retrieved block", self.nodes[0].gettxoutproof, [txid1, txid3])

View File

@ -592,3 +592,10 @@ def find_vout_for_address(node, txid, addr):
if addr == tx["vout"][i]["scriptPubKey"]["address"]:
return i
raise RuntimeError("Vout not found for address: txid=%s, addr=%s" % (txid, addr))
def sync_txindex(test_framework, node):
test_framework.log.debug("Waiting for node txindex to sync")
sync_start = int(time.time())
test_framework.wait_until(lambda: node.getindexinfo("txindex")["txindex"]["synced"])
test_framework.log.debug(f"Synced in {time.time() - sync_start} seconds")

View File

@ -117,7 +117,6 @@ class AddressInputTypeGrouping(BitcoinTestFramework):
self.extra_args = [
[
"-addresstype=bech32",
"-txindex",
],
[
"-addresstype=p2sh-segwit",