Compare commits

...

8 Commits

Author SHA1 Message Date
Reproducibility Matters
b13a31c4c7
Merge 51436f85a2bfcb7874a0c3b12add2df19ff18a3b into 5f4422d68dc3530c353af1f87499de1c864b60ad 2025-03-17 09:50:15 +07: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
TheCharlatan
51436f85a2
init: Take lock on blocks directory in BlockManager ctor
This moves the responsibility of taking the lock for the blocks
directory into the BlockManager. Use the DirectoryLock wrapper to ensure
it is the first resource to be acquired and is released again after use.

This is relevant for the kernel library where the lock should be taken
even if the user fails to explicitly do so.
2025-02-16 22:45:08 +01:00
TheCharlatan
21300478d9
util: Add RAII directory lock
This makes it easier for a class or a struct to own a lock on a
directory for the duration of its lifetime. It is used in the next
commit.
2025-02-16 22:44:59 +01:00
TheCharlatan
de844c79d4
util: Prevent multiple LockDirectory calls within the same process
Previously LockDirectory only prevented concurrent locks across
different processes, but allowed the same process to re-lock on the same
directory.

This change is not immediately relevant for its current use, where the
lock is only supposed to protect against a different process writing on
the same resources.

This change is relevant for future use by the kernel library, where
users of the library might mistakenly create multiple instances of an
object that seek to write to a common resource.
2025-02-14 21:23:34 +01:00
13 changed files with 133 additions and 28 deletions

View File

@ -1134,11 +1134,6 @@ static bool LockDirectory(const fs::path& dir, bool probeOnly)
} // no default case, so the compiler can warn about missing cases
assert(false);
}
static bool LockDirectories(bool probeOnly)
{
return LockDirectory(gArgs.GetDataDirNet(), probeOnly) && \
LockDirectory(gArgs.GetBlocksDirPath(), probeOnly);
}
bool AppInitSanityChecks(const kernel::Context& kernel)
{
@ -1156,7 +1151,8 @@ bool AppInitSanityChecks(const kernel::Context& kernel)
// Probe the directory locks to give an early error message, if possible
// We cannot hold the directory locks here, as the forking for daemon() hasn't yet happened,
// and a fork will cause weird behavior to them.
return LockDirectories(true);
return LockDirectory(gArgs.GetDataDirNet(), /*probeOnly=*/true)
&& LockDirectory(gArgs.GetBlocksDirPath(), /*probeOnly=*/true);
}
bool AppInitLockDirectories()
@ -1164,11 +1160,7 @@ bool AppInitLockDirectories()
// After daemonization get the directory locks again and hold on to them until exit
// This creates a slight window for a race condition to happen, however this condition is harmless: it
// will at most make us exit without printing a message to console.
if (!LockDirectories(false)) {
// Detailed error printed inside LockDirectory
return false;
}
return true;
return LockDirectory(gArgs.GetDataDirNet(), /*probeOnly=*/false);
}
bool AppInitInterfaces(NodeContext& node)

View File

@ -115,9 +115,10 @@ bool StartLogging(const ArgsManager& args)
}
if (!LogInstance().m_log_timestamps)
LogPrintf("Startup time: %s\n", FormatISO8601DateTime(GetTime()));
LogPrintf("Default data directory %s\n", fs::PathToString(GetDefaultDataDir()));
LogPrintf("Using data directory %s\n", fs::PathToString(gArgs.GetDataDirNet()));
LogInfo("Startup time: %s", FormatISO8601DateTime(GetTime()));
LogInfo("Default data directory %s", fs::PathToString(GetDefaultDataDir()));
LogInfo("Using data directory %s", fs::PathToString(gArgs.GetDataDirNet()));
LogInfo("Using blocks directory %s", fs::PathToString(gArgs.GetBlocksDirPath()));
// Only log conf file usage message if conf file actually exists.
fs::path config_file_path = args.GetConfigFilePath();

View File

@ -31,6 +31,7 @@
#include <util/batchpriority.h>
#include <util/check.h>
#include <util/fs.h>
#include <util/fs_helpers.h>
#include <util/signalinterrupt.h>
#include <util/strencodings.h>
#include <util/translation.h>
@ -1150,7 +1151,8 @@ static auto InitBlocksdirXorKey(const BlockManager::Options& opts)
}
BlockManager::BlockManager(const util::SignalInterrupt& interrupt, Options opts)
: m_prune_mode{opts.prune_target > 0},
: m_blocks_dir_lock{DirectoryLock(opts.blocks_dir, "blocks")},
m_prune_mode{opts.prune_target > 0},
m_xor_key{InitBlocksdirXorKey(opts)},
m_opts{std::move(opts)},
m_block_file_seq{FlatFileSeq{m_opts.blocks_dir, "blk", m_opts.fast_prune ? 0x4000 /* 16kB */ : BLOCKFILE_CHUNK_SIZE}},

View File

@ -18,6 +18,7 @@
#include <sync.h>
#include <uint256.h>
#include <util/fs.h>
#include <util/fs_helpers.h>
#include <util/hasher.h>
#include <array>
@ -127,7 +128,6 @@ struct BlockfileCursor {
std::ostream& operator<<(std::ostream& os, const BlockfileCursor& cursor);
/**
* Maintains a tree of blocks (stored in `m_block_index`) which is consulted
* to determine where the most-work tip is.
@ -141,6 +141,7 @@ class BlockManager
friend ChainstateManager;
private:
DirectoryLock m_blocks_dir_lock;
const CChainParams& GetParams() const { return m_opts.chainparams; }
const Consensus::Params& GetConsensus() const { return m_opts.chainparams.GetConsensus(); }
/**

View File

@ -1244,13 +1244,13 @@ BOOST_AUTO_TEST_CASE(test_LockDirectory)
BOOST_CHECK_EQUAL(util::LockDirectory(dirname, lockname), util::LockResult::Success);
// Another lock on the directory from the same thread should succeed
BOOST_CHECK_EQUAL(util::LockDirectory(dirname, lockname), util::LockResult::Success);
BOOST_CHECK_EQUAL(util::LockDirectory(dirname, lockname), util::LockResult::ErrorLock);
// Another lock on the directory from a different thread within the same process should succeed
util::LockResult threadresult;
std::thread thr([&] { threadresult = util::LockDirectory(dirname, lockname); });
thr.join();
BOOST_CHECK_EQUAL(threadresult, util::LockResult::Success);
BOOST_CHECK_EQUAL(threadresult, util::LockResult::ErrorLock);
#ifndef WIN32
// Try to acquire lock in child process while we're holding it, this should fail.
BOOST_CHECK_EQUAL(write(fd[1], &LockCommand, 1), 1);
@ -1291,6 +1291,30 @@ BOOST_AUTO_TEST_CASE(test_LockDirectory)
BOOST_CHECK_EQUAL(processstatus, 0);
BOOST_CHECK_EQUAL(util::LockDirectory(dirname, lockname, true), util::LockResult::Success);
{
auto lock{DirectoryLock(dirname, "test")};
BOOST_CHECK_THROW(DirectoryLock(dirname, "test"), std::runtime_error);
}
{
BOOST_CHECK_NO_THROW(DirectoryLock(dirname, "test"));
}
{
DirectoryLock lock1(dirname, "test");
DirectoryLock lock2(std::move(lock1));
BOOST_CHECK_THROW(DirectoryLock(dirname, "test"), std::runtime_error);
}
{
auto dirname_move = dirname / "move";
fs::create_directories(dirname_move);
DirectoryLock lock1(dirname, "test");
DirectoryLock lock2(dirname_move, "test");
lock2 = std::move(lock1);
BOOST_CHECK_THROW(DirectoryLock(dirname, "test"), std::runtime_error);
BOOST_CHECK_NO_THROW(DirectoryLock(dirname_move, "test"));
}
// Restore SIGCHLD
signal(SIGCHLD, old_handler);
BOOST_CHECK_EQUAL(close(fd[1]), 0); // Close our side of the socketpair

View File

@ -11,6 +11,7 @@
#include <sync.h>
#include <util/fs.h>
#include <util/syserror.h>
#include <util/translation.h>
#include <cerrno>
#include <fstream>
@ -57,7 +58,8 @@ LockResult LockDirectory(const fs::path& directory, const fs::path& lockfile_nam
// If a lock for this directory already exists in the map, don't try to re-lock it
if (dir_locks.count(fs::PathToString(pathLockFile))) {
return LockResult::Success;
LogError("Error while attempting to lock directory %s: Lock already taken", fs::PathToString(directory));
return LockResult::ErrorLock;
}
// Create empty lock file if it doesn't exist.
@ -90,6 +92,50 @@ void ReleaseDirectoryLocks()
dir_locks.clear();
}
DirectoryLock::DirectoryLock(fs::path dir_path, std::string name)
: m_path{dir_path},
m_name{name}
{
// Ensures only a single lock is taken on the provided directory.
switch (util::LockDirectory(m_path, ".lock", false)) {
case util::LockResult::ErrorWrite:
throw std::runtime_error(strprintf(_("Cannot write to %s directory '%s'; check permissions."), m_name, fs::PathToString(m_path)).original);
case util::LockResult::ErrorLock:
throw std::runtime_error(strprintf(_("Cannot obtain a lock on %s directory %s. %s is probably already running."), m_name, fs::PathToString(m_path), CLIENT_NAME).original);
case util::LockResult::Success:
return;
} // no default case, so the compiler can warn about missing cases
assert(false);
}
DirectoryLock::DirectoryLock(DirectoryLock&& other) noexcept
: m_path{std::move(other.m_path)},
m_name{std::move(other.m_name)}
{
other.m_path.clear();
other.m_name.clear();
}
DirectoryLock& DirectoryLock::operator=(DirectoryLock&& other) noexcept
{
if (this != &other) {
if (!m_path.empty()) {
UnlockDirectory(m_path, ".lock");
}
m_path = std::move(other.m_path);
other.m_path.clear();
m_name = std::move(other.m_name);
other.m_name.clear();
}
return *this;
}
DirectoryLock::~DirectoryLock()
{
if (!m_path.empty()) UnlockDirectory(m_path, ".lock");
}
bool CheckDiskSpace(const fs::path& dir, uint64_t additional_bytes)
{
constexpr uint64_t min_disk_space = 52428800; // 50 MiB

View File

@ -45,6 +45,23 @@ enum class LockResult {
[[nodiscard]] LockResult LockDirectory(const fs::path& directory, const fs::path& lockfile_name, bool probe_only = false);
} // namespace util
void UnlockDirectory(const fs::path& directory, const fs::path& lockfile_name);
class DirectoryLock
{
fs::path m_path;
std::string m_name;
public:
explicit DirectoryLock(fs::path dir_path, std::string name);
~DirectoryLock();
DirectoryLock(const DirectoryLock&) = delete;
DirectoryLock& operator=(const DirectoryLock&) = delete;
DirectoryLock(DirectoryLock&& other) noexcept;
DirectoryLock& operator=(DirectoryLock&& other) noexcept;
};
bool CheckDiskSpace(const fs::path& dir, uint64_t additional_bytes = 0);
/** Get the size of a file by scanning it.

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