Compare commits

...

6 Commits

Author SHA1 Message Date
Antoine Poinsot
6dd8f44eda
Merge ff0194a7ce9dabf1b31b64ca584e45840dce8141 into 5f4422d68dc3530c353af1f87499de1c864b60ad 2025-03-17 03:54:22 +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
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
Antoine Poinsot
ff0194a7ce miniscript: convert non-critical asserts to CHECK_NONFATAL
The Miniscript code contains assertions to prevent ending up in an insane state or prevent UB, but
also to enforce logical invariants. For the latter it is not necessary to crash the program if they
are broken. Raising an exception suffices, especially as this code is often called through the RPC
interface which can in turn handle the exception and the user can report it to developers.

This is based on previous work from Pieter Wuille.
2025-01-23 11:10:13 -05:00
8 changed files with 85 additions and 60 deletions

View File

@ -19,20 +19,20 @@ namespace internal {
Type SanitizeType(Type e) {
int num_types = (e << "K"_mst) + (e << "V"_mst) + (e << "B"_mst) + (e << "W"_mst);
if (num_types == 0) return ""_mst; // No valid type, don't care about the rest
assert(num_types == 1); // K, V, B, W all conflict with each other
assert(!(e << "z"_mst) || !(e << "o"_mst)); // z conflicts with o
assert(!(e << "n"_mst) || !(e << "z"_mst)); // n conflicts with z
assert(!(e << "n"_mst) || !(e << "W"_mst)); // n conflicts with W
assert(!(e << "V"_mst) || !(e << "d"_mst)); // V conflicts with d
assert(!(e << "K"_mst) || (e << "u"_mst)); // K implies u
assert(!(e << "V"_mst) || !(e << "u"_mst)); // V conflicts with u
assert(!(e << "e"_mst) || !(e << "f"_mst)); // e conflicts with f
assert(!(e << "e"_mst) || (e << "d"_mst)); // e implies d
assert(!(e << "V"_mst) || !(e << "e"_mst)); // V conflicts with e
assert(!(e << "d"_mst) || !(e << "f"_mst)); // d conflicts with f
assert(!(e << "V"_mst) || (e << "f"_mst)); // V implies f
assert(!(e << "K"_mst) || (e << "s"_mst)); // K implies s
assert(!(e << "z"_mst) || (e << "m"_mst)); // z implies m
CHECK_NONFATAL(num_types == 1); // K, V, B, W all conflict with each other
CHECK_NONFATAL(!(e << "z"_mst) || !(e << "o"_mst)); // z conflicts with o
CHECK_NONFATAL(!(e << "n"_mst) || !(e << "z"_mst)); // n conflicts with z
CHECK_NONFATAL(!(e << "n"_mst) || !(e << "W"_mst)); // n conflicts with W
CHECK_NONFATAL(!(e << "V"_mst) || !(e << "d"_mst)); // V conflicts with d
CHECK_NONFATAL(!(e << "K"_mst) || (e << "u"_mst)); // K implies u
CHECK_NONFATAL(!(e << "V"_mst) || !(e << "u"_mst)); // V conflicts with u
CHECK_NONFATAL(!(e << "e"_mst) || !(e << "f"_mst)); // e conflicts with f
CHECK_NONFATAL(!(e << "e"_mst) || (e << "d"_mst)); // e implies d
CHECK_NONFATAL(!(e << "V"_mst) || !(e << "e"_mst)); // V conflicts with e
CHECK_NONFATAL(!(e << "d"_mst) || !(e << "f"_mst)); // d conflicts with f
CHECK_NONFATAL(!(e << "V"_mst) || (e << "f"_mst)); // V implies f
CHECK_NONFATAL(!(e << "K"_mst) || (e << "s"_mst)); // K implies s
CHECK_NONFATAL(!(e << "z"_mst) || (e << "m"_mst)); // z implies m
return e;
}
@ -40,46 +40,46 @@ Type ComputeType(Fragment fragment, Type x, Type y, Type z, const std::vector<Ty
size_t data_size, size_t n_subs, size_t n_keys, MiniscriptContext ms_ctx) {
// Sanity check on data
if (fragment == Fragment::SHA256 || fragment == Fragment::HASH256) {
assert(data_size == 32);
CHECK_NONFATAL(data_size == 32);
} else if (fragment == Fragment::RIPEMD160 || fragment == Fragment::HASH160) {
assert(data_size == 20);
CHECK_NONFATAL(data_size == 20);
} else {
assert(data_size == 0);
CHECK_NONFATAL(data_size == 0);
}
// Sanity check on k
if (fragment == Fragment::OLDER || fragment == Fragment::AFTER) {
assert(k >= 1 && k < 0x80000000UL);
CHECK_NONFATAL(k >= 1 && k < 0x80000000UL);
} else if (fragment == Fragment::MULTI || fragment == Fragment::MULTI_A) {
assert(k >= 1 && k <= n_keys);
CHECK_NONFATAL(k >= 1 && k <= n_keys);
} else if (fragment == Fragment::THRESH) {
assert(k >= 1 && k <= n_subs);
CHECK_NONFATAL(k >= 1 && k <= n_subs);
} else {
assert(k == 0);
CHECK_NONFATAL(k == 0);
}
// Sanity check on subs
if (fragment == Fragment::AND_V || fragment == Fragment::AND_B || fragment == Fragment::OR_B ||
fragment == Fragment::OR_C || fragment == Fragment::OR_I || fragment == Fragment::OR_D) {
assert(n_subs == 2);
CHECK_NONFATAL(n_subs == 2);
} else if (fragment == Fragment::ANDOR) {
assert(n_subs == 3);
CHECK_NONFATAL(n_subs == 3);
} else if (fragment == Fragment::WRAP_A || fragment == Fragment::WRAP_S || fragment == Fragment::WRAP_C ||
fragment == Fragment::WRAP_D || fragment == Fragment::WRAP_V || fragment == Fragment::WRAP_J ||
fragment == Fragment::WRAP_N) {
assert(n_subs == 1);
CHECK_NONFATAL(n_subs == 1);
} else if (fragment != Fragment::THRESH) {
assert(n_subs == 0);
CHECK_NONFATAL(n_subs == 0);
}
// Sanity check on keys
if (fragment == Fragment::PK_K || fragment == Fragment::PK_H) {
assert(n_keys == 1);
CHECK_NONFATAL(n_keys == 1);
} else if (fragment == Fragment::MULTI) {
assert(n_keys >= 1 && n_keys <= MAX_PUBKEYS_PER_MULTISIG);
assert(!IsTapscript(ms_ctx));
CHECK_NONFATAL(n_keys >= 1 && n_keys <= MAX_PUBKEYS_PER_MULTISIG);
CHECK_NONFATAL(!IsTapscript(ms_ctx));
} else if (fragment == Fragment::MULTI_A) {
assert(n_keys >= 1 && n_keys <= MAX_PUBKEYS_PER_MULTI_A);
assert(IsTapscript(ms_ctx));
CHECK_NONFATAL(n_keys >= 1 && n_keys <= MAX_PUBKEYS_PER_MULTI_A);
CHECK_NONFATAL(IsTapscript(ms_ctx));
} else {
assert(n_keys == 0);
CHECK_NONFATAL(n_keys == 0);
}
// Below is the per-fragment logic for computing the expression types.

View File

@ -659,7 +659,8 @@ private:
stack.pop_back();
}
// The final remaining results element is the root result, return it.
assert(results.size() == 1);
assert(results.size() >= 1);
CHECK_NONFATAL(results.size() == 1);
return std::move(results[0]);
}
@ -1225,7 +1226,7 @@ private:
// The dissatisfaction consists of as many empty vectors as there are keys, which is the same as
// satisfying 0 keys.
auto& nsat{sats[0]};
assert(node.k != 0);
CHECK_NONFATAL(node.k != 0);
assert(node.k <= sats.size());
return {std::move(nsat), std::move(sats[node.k])};
}
@ -1392,38 +1393,38 @@ private:
// (the actual satisfaction code in ProduceInputHelper does not use GetType)
// For 'z' nodes, available satisfactions/dissatisfactions must have stack size 0.
if (node.GetType() << "z"_mst && ret.nsat.available != Availability::NO) assert(ret.nsat.stack.size() == 0);
if (node.GetType() << "z"_mst && ret.sat.available != Availability::NO) assert(ret.sat.stack.size() == 0);
if (node.GetType() << "z"_mst && ret.nsat.available != Availability::NO) CHECK_NONFATAL(ret.nsat.stack.size() == 0);
if (node.GetType() << "z"_mst && ret.sat.available != Availability::NO) CHECK_NONFATAL(ret.sat.stack.size() == 0);
// For 'o' nodes, available satisfactions/dissatisfactions must have stack size 1.
if (node.GetType() << "o"_mst && ret.nsat.available != Availability::NO) assert(ret.nsat.stack.size() == 1);
if (node.GetType() << "o"_mst && ret.sat.available != Availability::NO) assert(ret.sat.stack.size() == 1);
if (node.GetType() << "o"_mst && ret.nsat.available != Availability::NO) CHECK_NONFATAL(ret.nsat.stack.size() == 1);
if (node.GetType() << "o"_mst && ret.sat.available != Availability::NO) CHECK_NONFATAL(ret.sat.stack.size() == 1);
// For 'n' nodes, available satisfactions/dissatisfactions must have stack size 1 or larger. For satisfactions,
// the top element cannot be 0.
if (node.GetType() << "n"_mst && ret.sat.available != Availability::NO) assert(ret.sat.stack.size() >= 1);
if (node.GetType() << "n"_mst && ret.nsat.available != Availability::NO) assert(ret.nsat.stack.size() >= 1);
if (node.GetType() << "n"_mst && ret.sat.available != Availability::NO) assert(!ret.sat.stack.back().empty());
if (node.GetType() << "n"_mst && ret.sat.available != Availability::NO) CHECK_NONFATAL(ret.sat.stack.size() >= 1);
if (node.GetType() << "n"_mst && ret.nsat.available != Availability::NO) CHECK_NONFATAL(ret.nsat.stack.size() >= 1);
if (node.GetType() << "n"_mst && ret.sat.available != Availability::NO) CHECK_NONFATAL(!ret.sat.stack.back().empty());
// For 'd' nodes, a dissatisfaction must exist, and they must not need a signature. If it is non-malleable,
// it must be canonical.
if (node.GetType() << "d"_mst) assert(ret.nsat.available != Availability::NO);
if (node.GetType() << "d"_mst) assert(!ret.nsat.has_sig);
if (node.GetType() << "d"_mst && !ret.nsat.malleable) assert(!ret.nsat.non_canon);
if (node.GetType() << "d"_mst) CHECK_NONFATAL(ret.nsat.available != Availability::NO);
if (node.GetType() << "d"_mst) CHECK_NONFATAL(!ret.nsat.has_sig);
if (node.GetType() << "d"_mst && !ret.nsat.malleable) CHECK_NONFATAL(!ret.nsat.non_canon);
// For 'f'/'s' nodes, dissatisfactions/satisfactions must have a signature.
if (node.GetType() << "f"_mst && ret.nsat.available != Availability::NO) assert(ret.nsat.has_sig);
if (node.GetType() << "s"_mst && ret.sat.available != Availability::NO) assert(ret.sat.has_sig);
if (node.GetType() << "f"_mst && ret.nsat.available != Availability::NO) CHECK_NONFATAL(ret.nsat.has_sig);
if (node.GetType() << "s"_mst && ret.sat.available != Availability::NO) CHECK_NONFATAL(ret.sat.has_sig);
// For non-malleable 'e' nodes, a non-malleable dissatisfaction must exist.
if (node.GetType() << "me"_mst) assert(ret.nsat.available != Availability::NO);
if (node.GetType() << "me"_mst) assert(!ret.nsat.malleable);
if (node.GetType() << "me"_mst) CHECK_NONFATAL(ret.nsat.available != Availability::NO);
if (node.GetType() << "me"_mst) CHECK_NONFATAL(!ret.nsat.malleable);
// For 'm' nodes, if a satisfaction exists, it must be non-malleable.
if (node.GetType() << "m"_mst && ret.sat.available != Availability::NO) assert(!ret.sat.malleable);
if (node.GetType() << "m"_mst && ret.sat.available != Availability::NO) CHECK_NONFATAL(!ret.sat.malleable);
// If a non-malleable satisfaction exists, it must be canonical.
if (ret.sat.available != Availability::NO && !ret.sat.malleable) assert(!ret.sat.non_canon);
if (ret.sat.available != Availability::NO && !ret.sat.malleable) CHECK_NONFATAL(!ret.sat.non_canon);
return ret;
};
@ -1604,7 +1605,8 @@ public:
case Fragment::THRESH:
return static_cast<uint32_t>(std::count(subs.begin(), subs.end(), true)) >= node.k;
default: // wrappers
assert(subs.size() == 1);
assert(subs.size() >= 1);
CHECK_NONFATAL(subs.size() == 1);
return subs[0];
}
});
@ -2157,7 +2159,8 @@ inline NodeRef<Key> Parse(Span<const char> in, const Ctx& ctx)
}
// Sanity checks on the produced miniscript
assert(constructed.size() == 1);
assert(constructed.size() >= 1);
CHECK_NONFATAL(constructed.size() == 1);
assert(constructed[0]->ScriptSize() == script_size);
if (in.size() > 0) return {};
NodeRef<Key> tl_node = std::move(constructed.front());

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",