From d1104162582b7673bdd4e87a3117b202d66c96ca Mon Sep 17 00:00:00 2001 From: DCG-Claude Date: Thu, 1 Oct 2026 05:59:19 -0500 Subject: [PATCH 1/2] Merge bitcoin/bitcoin#25862: refactor, kernel: Remove gArgs accesses from dbwrapper and txdb MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit aadd7c5b9b43a38beaa954b4cb8c2fff55f2200f refactor, validation: Add ChainstateManagerOpts db options (Ryan Ofsky) 0352258148c51572426666d337c7b28d0033376c refactor, txdb: Use DBParams struct in CBlockTreeDB (Ryan Ofsky) c00fa1a7341d3f47f992e0beb043da655cbca777 refactor, txdb: Add CoinsViewOptions struct (Ryan Ofsky) 2eaeded37f3a07c35eef38d9a80c1e5fbd4d41ee refactor, dbwrapper: Add DBParams and DBOptions structs (Ryan Ofsky) Pull request description: Code in the libbitcoin_kernel library should not be calling `ArgsManager` methods or trying to read options from the command line. Instead it should just get options values from simple structs and function arguments that are passed in externally. This PR removes `gArgs` accesses from `dbwrapper` and `txdb` modules by defining appropriate options structs, and is a followup to PR's #25290 #25487 #25527 which remove other `ArgsManager` calls from kernel modules. This PR does not change behavior in any way. It is a simpler alternative to #25623 because the only thing it does is remove `gArgs` references from kernel code. It avoids other unnecessary changes like adding options to the kernel API (they can be added separately later). ACKs for top commit: TheCharlatan: Code review ACK aadd7c5b9b43a38beaa954b4cb8c2fff55f2200f achow101: ACK aadd7c5b9b43a38beaa954b4cb8c2fff55f2200f furszy: diff ACK aadd7c5b Tree-SHA512: 46dfd5d99ab3110492e7bba97a87122c831b8344caaf7dd2ebdb6e0ad6aa9174d4d1832d6f3a7465eda9294fe50defaa3c000afbbddc4e72838687df09a63ffd Dash adaptations: - src/kernel/chainstatemanager_opts.h: only `fs::path datadir;` added; upstream's conflict context also carried `adjusted_time_callback`, which Dash already removed with bitcoin/bitcoin#28956, so it is not re-added - src/bitcoin-chainstate.cpp, src/init.cpp (AppInitMain), src/test/util/setup_common.cpp, src/test/validation_chainstatemanager_tests.cpp: added `.datadir = ...GetDataDirNet()` without the `.adjusted_time_callback` line from the upstream context, for the same reason - src/validation.cpp: new CoinsViews(DBParams, CoinsViewOptions) ctor placed after Dash's GetBlockSubsidy(const CBlockIndex*, ...), which is kept - src/dbwrapper.h: DBOptions/DBParams added after Dash's CharCast helper, which is kept - src/test/dbwrapper_tests.cpp: unicodepath test keeps Dash's '∋' path name (Dash replaces '₿') and only takes the DBParams constructor change - src/test/util/setup_common.cpp: the CBlockTreeDB(DBParams{...}) change sits next to Dash's mn_sync/clhandler setup, which is kept - src/dbwrapper.h: Dash-only util::MakeDbWrapper (used by evodb, llmq recsigdb/quorumdb/dkgdb and isdb) now builds DBParams; its options come from node::ReadDatabaseArgs(gArgs, ...), like upstream's BaseIndex::DB, so -forcecompactdb still applies to Dash's databases. Added includes for and - src/txdb.cpp: Dash-only CBlockTreeDB::MigrateOldIndexData opens chainstate/timestampindex/spentindex/addressindex CDBWrappers; converted to DBParams{.path, .cache_bytes = 0} (same memory/wipe/obfuscate values as before) - src/node/chainstate.cpp: Dash-only RecoverSnapshotCleanup's CCoinsViewDB converted to {{.path = normal, .cache_bytes = 1 << 20, .obfuscate = true}, {}}; obfuscate=true is kept because the old CCoinsViewDB always set it and upstream now leaves it to the caller - src/Makefile.am: node/database_args.cpp added to libdashkernel_la_SOURCES, because Dash's kernel lib includes index/base.cpp (upstream's does not), which now calls node::ReadDatabaseArgs --- src/Makefile.am | 5 +++ src/bitcoin-chainstate.cpp | 1 + src/dbwrapper.cpp | 32 +++++++-------- src/dbwrapper.h | 41 ++++++++++++++----- src/index/base.cpp | 9 +++- src/init.cpp | 2 + src/kernel/chainstatemanager_opts.h | 6 +++ src/node/chainstate.cpp | 9 +++- src/node/chainstatemanager_args.cpp | 7 ++++ src/node/coins_view_args.cpp | 16 ++++++++ src/node/coins_view_args.h | 15 +++++++ src/node/database_args.cpp | 18 ++++++++ src/node/database_args.h | 15 +++++++ src/test/coins_tests.cpp | 4 +- src/test/dbwrapper_tests.cpp | 22 +++++----- src/test/util/setup_common.cpp | 6 ++- .../validation_chainstatemanager_tests.cpp | 1 + src/txdb.cpp | 34 +++++++-------- src/txdb.h | 21 ++++++---- src/validation.cpp | 19 +++++---- src/validation.h | 2 +- 21 files changed, 206 insertions(+), 79 deletions(-) create mode 100644 src/node/coins_view_args.cpp create mode 100644 src/node/coins_view_args.h create mode 100644 src/node/database_args.cpp create mode 100644 src/node/database_args.h diff --git a/src/Makefile.am b/src/Makefile.am index 30db5207d67c..540ceb68c2ba 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -353,8 +353,10 @@ BITCOIN_CORE_H = \ node/chainstate.h \ node/chainstatemanager_args.h \ node/coin.h \ + node/coins_view_args.h \ node/connection_types.h \ node/context.h \ + node/database_args.h \ node/eviction.h \ node/interface_ui.h \ node/mempool_args.h \ @@ -630,8 +632,10 @@ libbitcoin_node_a_SOURCES = \ node/chainstate.cpp \ node/chainstatemanager_args.cpp \ node/coin.cpp \ + node/coins_view_args.cpp \ node/connection_types.cpp \ node/context.cpp \ + node/database_args.cpp \ node/eviction.cpp \ node/interface_ui.cpp \ node/interfaces.cpp \ @@ -1347,6 +1351,7 @@ libdashkernel_la_SOURCES = \ netbase.cpp \ node/blockstorage.cpp \ node/chainstate.cpp \ + node/database_args.cpp \ node/interface_ui.cpp \ node/transaction.cpp \ node/utxo_snapshot.cpp \ diff --git a/src/bitcoin-chainstate.cpp b/src/bitcoin-chainstate.cpp index 204c4701f08d..0c650737fd0a 100644 --- a/src/bitcoin-chainstate.cpp +++ b/src/bitcoin-chainstate.cpp @@ -92,6 +92,7 @@ int main(int argc, char* argv[]) // SETUP: Chainstate const ChainstateManager::Options chainman_opts{ .chainparams = chainparams, + .datadir = gArgs.GetDataDirNet(), }; ChainstateManager chainman{chainman_opts}; diff --git a/src/dbwrapper.cpp b/src/dbwrapper.cpp index b0aebb51919d..d4ad3d023b52 100644 --- a/src/dbwrapper.cpp +++ b/src/dbwrapper.cpp @@ -127,40 +127,40 @@ static leveldb::Options GetOptions(size_t nCacheSize) return options; } -CDBWrapper::CDBWrapper(const fs::path& path, size_t nCacheSize, bool fMemory, bool fWipe, bool obfuscate) - : m_name{fs::PathToString(path.stem())}, m_path{path}, m_is_memory{fMemory} +CDBWrapper::CDBWrapper(const DBParams& params) + : m_name{fs::PathToString(params.path.stem())}, m_path{params.path}, m_is_memory{params.memory_only} { penv = nullptr; readoptions.verify_checksums = true; iteroptions.verify_checksums = true; iteroptions.fill_cache = false; syncoptions.sync = true; - options = GetOptions(nCacheSize); + options = GetOptions(params.cache_bytes); options.create_if_missing = true; - if (fMemory) { + if (params.memory_only) { penv = leveldb::NewMemEnv(leveldb::Env::Default()); options.env = penv; } else { - if (fWipe) { - LogPrintf("Wiping LevelDB in %s\n", fs::PathToString(path)); - leveldb::Status result = leveldb::DestroyDB(fs::PathToString(path), options); + if (params.wipe_data) { + LogPrintf("Wiping LevelDB in %s\n", fs::PathToString(params.path)); + leveldb::Status result = leveldb::DestroyDB(fs::PathToString(params.path), options); dbwrapper_private::HandleError(result); } - TryCreateDirectories(path); - LogPrintf("Opening LevelDB in %s\n", fs::PathToString(path)); + TryCreateDirectories(params.path); + LogPrintf("Opening LevelDB in %s\n", fs::PathToString(params.path)); } // PathToString() return value is safe to pass to leveldb open function, // because on POSIX leveldb passes the byte string directly to ::open(), and // on Windows it converts from UTF-8 to UTF-16 before calling ::CreateFileW // (see env_posix.cc and env_windows.cc). - leveldb::Status status = leveldb::DB::Open(options, fs::PathToString(path), &pdb); + leveldb::Status status = leveldb::DB::Open(options, fs::PathToString(params.path), &pdb); dbwrapper_private::HandleError(status); LogPrintf("Opened LevelDB successfully\n"); - if (gArgs.GetBoolArg("-forcecompactdb", false)) { - LogPrintf("Starting database compaction of %s\n", fs::PathToString(path)); + if (params.options.force_compact) { + LogPrintf("Starting database compaction of %s\n", fs::PathToString(params.path)); pdb->CompactRange(nullptr, nullptr); - LogPrintf("Finished database compaction of %s\n", fs::PathToString(path)); + LogPrintf("Finished database compaction of %s\n", fs::PathToString(params.path)); } // The base-case obfuscation key, which is a noop. @@ -168,7 +168,7 @@ CDBWrapper::CDBWrapper(const fs::path& path, size_t nCacheSize, bool fMemory, bo bool key_exists = Read(OBFUSCATE_KEY_KEY, obfuscate_key); - if (!key_exists && obfuscate && IsEmpty()) { + if (!key_exists && params.obfuscate && IsEmpty()) { // Initialize non-degenerate obfuscation if it won't upset // existing, non-obfuscated data. std::vector new_key = CreateObfuscateKey(); @@ -177,10 +177,10 @@ CDBWrapper::CDBWrapper(const fs::path& path, size_t nCacheSize, bool fMemory, bo Write(OBFUSCATE_KEY_KEY, new_key); obfuscate_key = new_key; - LogPrintf("Wrote new obfuscate key for %s: %s\n", fs::PathToString(path), HexStr(obfuscate_key)); + LogPrintf("Wrote new obfuscate key for %s: %s\n", fs::PathToString(params.path), HexStr(obfuscate_key)); } - LogPrintf("Using obfuscation key for %s: %s\n", fs::PathToString(path), HexStr(obfuscate_key)); + LogPrintf("Using obfuscation key for %s: %s\n", fs::PathToString(params.path), HexStr(obfuscate_key)); } CDBWrapper::~CDBWrapper() diff --git a/src/dbwrapper.h b/src/dbwrapper.h index 14468696eabd..59bdeb6a58eb 100644 --- a/src/dbwrapper.h +++ b/src/dbwrapper.h @@ -12,6 +12,7 @@ #include #include #include +#include #include @@ -43,6 +44,29 @@ static const size_t DBWRAPPER_PREALLOC_VALUE_SIZE = 1024; inline auto CharCast(const std::byte* data) { return reinterpret_cast(data); } +//! User-controlled performance and debug options. +struct DBOptions { + //! Compact database on startup. + bool force_compact = false; +}; + +//! Application-specific storage settings. +struct DBParams { + //! Location in the filesystem where leveldb data will be stored. + fs::path path; + //! Configures various leveldb cache settings. + size_t cache_bytes; + //! If true, use leveldb's memory environment. + bool memory_only = false; + //! If true, remove all existing data. + bool wipe_data = false; + //! If true, store data obfuscated via simple XOR. If false, XOR with a + //! zero'd byte array. + bool obfuscate = false; + //! Passed-through options. + DBOptions options{}; +}; + class dbwrapper_error : public std::runtime_error { public: @@ -261,15 +285,7 @@ class CDBWrapper bool m_is_memory; public: - /** - * @param[in] path Location in the filesystem where leveldb data will be stored. - * @param[in] nCacheSize Configures various leveldb cache settings. - * @param[in] fMemory If true, use leveldb's memory environment. - * @param[in] fWipe If true, remove all existing data. - * @param[in] obfuscate If true, store data obfuscated via simple XOR. If false, XOR - * with a zero'd byte array. - */ - CDBWrapper(const fs::path& path, size_t nCacheSize, bool fMemory = false, bool fWipe = false, bool obfuscate = false); + CDBWrapper(const DBParams& params); ~CDBWrapper(); CDBWrapper(const CDBWrapper&) = delete; @@ -769,7 +785,12 @@ struct DbWrapperParams static inline std::unique_ptr MakeDbWrapper(const DbWrapperParams& params) { - return std::make_unique(params.path, params.cache_size, params.memory, params.wipe); + return std::make_unique(DBParams{ + .path = params.path, + .cache_bytes = params.cache_size, + .memory_only = params.memory, + .wipe_data = params.wipe, + .options = DBOptions{.force_compact = gArgs.GetBoolArg("-forcecompactdb", false)}}); } } // namespace util diff --git a/src/index/base.cpp b/src/index/base.cpp index 61b7e7ea768f..f051eceeecee 100644 --- a/src/index/base.cpp +++ b/src/index/base.cpp @@ -8,6 +8,7 @@ #include #include #include +#include #include #include #include @@ -43,7 +44,13 @@ CBlockLocator GetLocator(interfaces::Chain& chain, const uint256& block_hash) } BaseIndex::DB::DB(const fs::path& path, size_t n_cache_size, bool f_memory, bool f_wipe, bool f_obfuscate) : - CDBWrapper(path, n_cache_size, f_memory, f_wipe, f_obfuscate) + CDBWrapper{DBParams{ + .path = path, + .cache_bytes = n_cache_size, + .memory_only = f_memory, + .wipe_data = f_wipe, + .obfuscate = f_obfuscate, + .options = [] { DBOptions options; node::ReadDatabaseArgs(gArgs, options); return options; }()}} {} bool BaseIndex::DB::ReadBestBlock(CBlockLocator& locator) const diff --git a/src/init.cpp b/src/init.cpp index 1befdfacd233..2f219dbb3388 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1450,6 +1450,7 @@ bool AppInitParameterInteraction(const ArgsManager& args) { ChainstateManager::Options chainman_opts_dummy{ .chainparams = chainparams, + .datadir = args.GetDataDirNet(), }; if (const auto error{ApplyArgsManOptions(args, chainman_opts_dummy)}) { return InitError(*error); @@ -1969,6 +1970,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) bool fReindexChainState = args.GetBoolArg("-reindex-chainstate", false); ChainstateManager::Options chainman_opts{ .chainparams = chainparams, + .datadir = args.GetDataDirNet(), }; Assert(!ApplyArgsManOptions(args, chainman_opts)); // no error can happen, already checked in AppInitParameterInteraction diff --git a/src/kernel/chainstatemanager_opts.h b/src/kernel/chainstatemanager_opts.h index a353414d8e99..2ff9298c9fcd 100644 --- a/src/kernel/chainstatemanager_opts.h +++ b/src/kernel/chainstatemanager_opts.h @@ -6,6 +6,8 @@ #define BITCOIN_KERNEL_CHAINSTATEMANAGER_OPTS_H #include +#include +#include #include #include @@ -26,6 +28,7 @@ namespace kernel { */ struct ChainstateManagerOpts { const CChainParams& chainparams; + fs::path datadir; std::optional check_block_index{}; bool checkpoints_enabled{DEFAULT_CHECKPOINTS_ENABLED}; //! If set, it will override the minimum work we will assume exists on some valid chain. @@ -34,6 +37,9 @@ struct ChainstateManagerOpts { std::optional assumed_valid_block{}; //! If the tip is older than this, the node is considered to be in initial block download. std::chrono::seconds max_tip_age{DEFAULT_MAX_TIP_AGE}; + DBOptions block_tree_db{}; + DBOptions coins_db{}; + CoinsViewOptions coins_view{}; }; } // namespace kernel diff --git a/src/node/chainstate.cpp b/src/node/chainstate.cpp index 35c676617b1c..2b03e53be04b 100644 --- a/src/node/chainstate.cpp +++ b/src/node/chainstate.cpp @@ -74,7 +74,7 @@ static bool RecoverSnapshotCleanup(CEvoDB& evodb, const fs::path& data_dir, bili if (fs::exists(normal) && !fs::exists(snapshot) && has_metadata) { uint256 coins_tip; try { - CCoinsViewDB coins_db{normal, /*nCacheSize=*/1 << 20, /*fMemory=*/false, /*fWipe=*/false}; + CCoinsViewDB coins_db{{.path = normal, .cache_bytes = 1 << 20, .obfuscate = true}, {}}; coins_tip = coins_db.GetBestBlock(); } catch (const std::exception& e) { error = strprintf(_("Failed to inspect interrupted snapshot cleanup: %s"), e.what()); @@ -152,7 +152,12 @@ static ChainstateLoadResult CompleteChainstateInitialization(ChainstateManager& // new CBlockTreeDB tries to delete the existing file, which // fails if it's still open from the previous loop. Close it first: pblocktree.reset(); - pblocktree.reset(new CBlockTreeDB(cache_sizes.block_tree_db, options.block_tree_db_in_memory, options.reindex)); + pblocktree = std::make_unique(DBParams{ + .path = chainman.m_options.datadir / "blocks" / "index", + .cache_bytes = static_cast(cache_sizes.block_tree_db), + .memory_only = options.block_tree_db_in_memory, + .wipe_data = options.reindex, + .options = chainman.m_options.block_tree_db}); // Initialize llmq_ctx llmq_ctx.reset(); diff --git a/src/node/chainstatemanager_args.cpp b/src/node/chainstatemanager_args.cpp index b0d929626bde..9801e6e959d2 100644 --- a/src/node/chainstatemanager_args.cpp +++ b/src/node/chainstatemanager_args.cpp @@ -5,6 +5,9 @@ #include #include +#include +#include +#include #include #include #include @@ -34,6 +37,10 @@ std::optional ApplyArgsManOptions(const ArgsManager& args, Chains if (auto value{args.GetIntArg("-maxtipage")}) opts.max_tip_age = std::chrono::seconds{*value}; + ReadDatabaseArgs(args, opts.block_tree_db); + ReadDatabaseArgs(args, opts.coins_db); + ReadCoinsViewArgs(args, opts.coins_view); + return std::nullopt; } } // namespace node diff --git a/src/node/coins_view_args.cpp b/src/node/coins_view_args.cpp new file mode 100644 index 000000000000..67c9b8dbacf4 --- /dev/null +++ b/src/node/coins_view_args.cpp @@ -0,0 +1,16 @@ +// Copyright (c) 2022 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include + +#include +#include + +namespace node { +void ReadCoinsViewArgs(const ArgsManager& args, CoinsViewOptions& options) +{ + if (auto value = args.GetIntArg("-dbbatchsize")) options.batch_write_bytes = *value; + if (auto value = args.GetIntArg("-dbcrashratio")) options.simulate_crash_ratio = *value; +} +} // namespace node diff --git a/src/node/coins_view_args.h b/src/node/coins_view_args.h new file mode 100644 index 000000000000..71a7a671fd8e --- /dev/null +++ b/src/node/coins_view_args.h @@ -0,0 +1,15 @@ +// Copyright (c) 2022 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#ifndef BITCOIN_NODE_COINS_VIEW_ARGS_H +#define BITCOIN_NODE_COINS_VIEW_ARGS_H + +class ArgsManager; +struct CoinsViewOptions; + +namespace node { +void ReadCoinsViewArgs(const ArgsManager& args, CoinsViewOptions& options); +} // namespace node + +#endif // BITCOIN_NODE_COINS_VIEW_ARGS_H diff --git a/src/node/database_args.cpp b/src/node/database_args.cpp new file mode 100644 index 000000000000..2c53b4b47e4d --- /dev/null +++ b/src/node/database_args.cpp @@ -0,0 +1,18 @@ +// Copyright (c) 2022 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include + +#include +#include + +namespace node { +void ReadDatabaseArgs(const ArgsManager& args, DBOptions& options) +{ + // Settings here apply to all databases (chainstate, blocks, and index + // databases), but it'd be easy to parse database-specific options by adding + // a database_type string or enum parameter to this function. + if (auto value = args.GetBoolArg("-forcecompactdb")) options.force_compact = *value; +} +} // namespace node diff --git a/src/node/database_args.h b/src/node/database_args.h new file mode 100644 index 000000000000..001976f219a1 --- /dev/null +++ b/src/node/database_args.h @@ -0,0 +1,15 @@ +// Copyright (c) 2022 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#ifndef BITCOIN_NODE_DATABASE_ARGS_H +#define BITCOIN_NODE_DATABASE_ARGS_H + +class ArgsManager; +struct DBOptions; + +namespace node { +void ReadDatabaseArgs(const ArgsManager& args, DBOptions& options); +} // namespace node + +#endif // BITCOIN_NODE_DATABASE_ARGS_H diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp index 7a2a962c15b4..4c997563b84a 100644 --- a/src/test/coins_tests.cpp +++ b/src/test/coins_tests.cpp @@ -280,7 +280,7 @@ BOOST_AUTO_TEST_CASE(coins_cache_simulation_test) CCoinsViewTest base; SimulationTest(&base, false); - CCoinsViewDB db_base{"test", /*nCacheSize=*/1 << 23, /*fMemory=*/true, /*fWipe=*/false}; + CCoinsViewDB db_base{{.path = "test", .cache_bytes = 1 << 23, .memory_only = true}, {}}; SimulationTest(&db_base, true); } @@ -1073,7 +1073,7 @@ void TestFlushBehavior( BOOST_AUTO_TEST_CASE(ccoins_flush_behavior) { // Create two in-memory caches atop a leveldb view. - CCoinsViewDB base{"test", /*nCacheSize=*/ 1 << 23, /*fMemory=*/ true, /*fWipe=*/ false}; + CCoinsViewDB base{{.path = "test", .cache_bytes = 1 << 23, .memory_only = true}, {}}; std::vector> caches; caches.push_back(std::make_unique(&base)); caches.push_back(std::make_unique(caches.back().get())); diff --git a/src/test/dbwrapper_tests.cpp b/src/test/dbwrapper_tests.cpp index 4a008da21d19..203c76272f30 100644 --- a/src/test/dbwrapper_tests.cpp +++ b/src/test/dbwrapper_tests.cpp @@ -29,7 +29,7 @@ BOOST_AUTO_TEST_CASE(dbwrapper) // Perform tests both obfuscated and non-obfuscated. for (const bool obfuscate : {false, true}) { fs::path ph = m_args.GetDataDirBase() / (obfuscate ? "dbwrapper_obfuscate_true" : "dbwrapper_obfuscate_false"); - CDBWrapper dbw(ph, (1 << 20), true, false, obfuscate); + CDBWrapper dbw({.path = ph, .cache_bytes = 1 << 20, .memory_only = true, .wipe_data = false, .obfuscate = obfuscate}); uint8_t key{'k'}; uint256 in = InsecureRand256(); uint256 res; @@ -48,7 +48,7 @@ BOOST_AUTO_TEST_CASE(dbwrapper_basic_data) // Perform tests both obfuscated and non-obfuscated. for (bool obfuscate : {false, true}) { fs::path ph = m_args.GetDataDirBase() / (obfuscate ? "dbwrapper_1_obfuscate_true" : "dbwrapper_1_obfuscate_false"); - CDBWrapper dbw(ph, (1 << 20), false, true, obfuscate); + CDBWrapper dbw({.path = ph, .cache_bytes = 1 << 20, .memory_only = false, .wipe_data = true, .obfuscate = obfuscate}); uint256 res; uint32_t res_uint_32; @@ -129,7 +129,7 @@ BOOST_AUTO_TEST_CASE(dbwrapper_batch) // Perform tests both obfuscated and non-obfuscated. for (const bool obfuscate : {false, true}) { fs::path ph = m_args.GetDataDirBase() / (obfuscate ? "dbwrapper_batch_obfuscate_true" : "dbwrapper_batch_obfuscate_false"); - CDBWrapper dbw(ph, (1 << 20), true, false, obfuscate); + CDBWrapper dbw({.path = ph, .cache_bytes = 1 << 20, .memory_only = true, .wipe_data = false, .obfuscate = obfuscate}); uint8_t key{'i'}; uint256 in = InsecureRand256(); @@ -165,7 +165,7 @@ BOOST_AUTO_TEST_CASE(dbwrapper_iterator) // Perform tests both obfuscated and non-obfuscated. for (const bool obfuscate : {false, true}) { fs::path ph = m_args.GetDataDirBase() / (obfuscate ? "dbwrapper_iterator_obfuscate_true" : "dbwrapper_iterator_obfuscate_false"); - CDBWrapper dbw(ph, (1 << 20), true, false, obfuscate); + CDBWrapper dbw({.path = ph, .cache_bytes = 1 << 20, .memory_only = true, .wipe_data = false, .obfuscate = obfuscate}); // The two keys are intentionally chosen for ordering uint8_t key{'j'}; @@ -208,7 +208,7 @@ BOOST_AUTO_TEST_CASE(existing_data_no_obfuscate) fs::create_directories(ph); // Set up a non-obfuscated wrapper to write some initial data. - std::unique_ptr dbw = std::make_unique(ph, (1 << 10), false, false, false); + std::unique_ptr dbw = std::make_unique(DBParams{.path = ph, .cache_bytes = 1 << 10, .memory_only = false, .wipe_data = false, .obfuscate = false}); uint8_t key{'k'}; uint256 in = InsecureRand256(); uint256 res; @@ -221,7 +221,7 @@ BOOST_AUTO_TEST_CASE(existing_data_no_obfuscate) dbw.reset(); // Now, set up another wrapper that wants to obfuscate the same directory - CDBWrapper odbw(ph, (1 << 10), false, false, true); + CDBWrapper odbw({.path = ph, .cache_bytes = 1 << 10, .memory_only = false, .wipe_data = false, .obfuscate = true}); // Check that the key/val we wrote with unobfuscated wrapper exists and // is readable. @@ -249,7 +249,7 @@ BOOST_AUTO_TEST_CASE(existing_data_reindex) fs::create_directories(ph); // Set up a non-obfuscated wrapper to write some initial data. - std::unique_ptr dbw = std::make_unique(ph, (1 << 10), false, false, false); + std::unique_ptr dbw = std::make_unique(DBParams{.path = ph, .cache_bytes = 1 << 10, .memory_only = false, .wipe_data = false, .obfuscate = false}); uint8_t key{'k'}; uint256 in = InsecureRand256(); uint256 res; @@ -262,7 +262,7 @@ BOOST_AUTO_TEST_CASE(existing_data_reindex) dbw.reset(); // Simulate a -reindex by wiping the existing data store - CDBWrapper odbw(ph, (1 << 10), false, true, true); + CDBWrapper odbw({.path = ph, .cache_bytes = 1 << 10, .memory_only = false, .wipe_data = true, .obfuscate = true}); // Check that the key/val we wrote with unobfuscated wrapper doesn't exist uint256 res2; @@ -281,7 +281,7 @@ BOOST_AUTO_TEST_CASE(existing_data_reindex) BOOST_AUTO_TEST_CASE(iterator_ordering) { fs::path ph = m_args.GetDataDirBase() / "iterator_ordering"; - CDBWrapper dbw(ph, (1 << 20), true, false, false); + CDBWrapper dbw({.path = ph, .cache_bytes = 1 << 20, .memory_only = true, .wipe_data = false, .obfuscate = false}); for (int x=0x00; x<256; ++x) { uint8_t key = x; uint32_t value = x*x; @@ -349,7 +349,7 @@ struct StringContentsSerializer { BOOST_AUTO_TEST_CASE(iterator_string_ordering) { fs::path ph = m_args.GetDataDirBase() / "iterator_string_ordering"; - CDBWrapper dbw(ph, (1 << 20), true, false, false); + CDBWrapper dbw({.path = ph, .cache_bytes = 1 << 20, .memory_only = true, .wipe_data = false, .obfuscate = false}); for (int x = 0; x < 10; ++x) { for (int y = 0; y < 10; ++y) { std::string key{ToString(x)}; @@ -391,7 +391,7 @@ BOOST_AUTO_TEST_CASE(unicodepath) // the ANSI CreateDirectoryA call and the code page isn't UTF8. // It will succeed if created with CreateDirectoryW. fs::path ph = m_args.GetDataDirBase() / "test_runner_∋_🏃_20191128_104644"; - CDBWrapper dbw(ph, (1 << 20)); + CDBWrapper dbw({.path = ph, .cache_bytes = 1 << 20}); fs::path lockPath = ph / "LOCK"; BOOST_CHECK(fs::exists(lockPath)); diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index fcf071dcd598..5433f9573fb7 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -265,10 +265,14 @@ ChainTestingSetup::ChainTestingSetup(const std::string& chainName, const std::ve const ChainstateManager::Options chainman_opts{ .chainparams = chainparams, + .datadir = m_args.GetDataDirNet(), .check_block_index = true, }; m_node.chainman = std::make_unique(chainman_opts); - m_node.chainman->m_blockman.m_block_tree_db = std::make_unique(m_cache_sizes.block_tree_db, true); + m_node.chainman->m_blockman.m_block_tree_db = std::make_unique(DBParams{ + .path = m_args.GetDataDirNet() / "blocks" / "index", + .cache_bytes = static_cast(m_cache_sizes.block_tree_db), + .memory_only = true}); m_node.mn_sync = std::make_unique(std::make_unique(*m_node.connman, *m_node.netfulfilledman)); diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp index e8c97241fd57..4e681c325b7b 100644 --- a/src/test/validation_chainstatemanager_tests.cpp +++ b/src/test/validation_chainstatemanager_tests.cpp @@ -462,6 +462,7 @@ struct SnapshotTestSetup : TestChain100Setup { BOOST_CHECK_EQUAL(chainman.GetAll().size(), 0); const ChainstateManager::Options chainman_opts{ .chainparams = ::Params(), + .datadir = m_args.GetDataDirNet(), }; // For robustness, ensure the old manager is destroyed before creating a // new one. diff --git a/src/txdb.cpp b/src/txdb.cpp index 4ec4ce9f3702..29ed4a0633fa 100644 --- a/src/txdb.cpp +++ b/src/txdb.cpp @@ -69,21 +69,22 @@ struct CoinEntry { } // namespace -CCoinsViewDB::CCoinsViewDB(fs::path ldb_path, size_t nCacheSize, bool fMemory, bool fWipe) : - m_db(std::make_unique(ldb_path, nCacheSize, fMemory, fWipe, true)), - m_ldb_path(ldb_path), - m_is_memory(fMemory) { } +CCoinsViewDB::CCoinsViewDB(DBParams db_params, CoinsViewOptions options) : + m_db_params{std::move(db_params)}, + m_options{std::move(options)}, + m_db{std::make_unique(m_db_params)} { } void CCoinsViewDB::ResizeCache(size_t new_cache_size) { // We can't do this operation with an in-memory DB since we'll lose all the coins upon // reset. - if (!m_is_memory) { + if (!m_db_params.memory_only) { // Have to do a reset first to get the original `m_db` state to release its // filesystem lock. m_db.reset(); - m_db = std::make_unique( - m_ldb_path, new_cache_size, m_is_memory, /*fWipe=*/false, /*obfuscate=*/true); + m_db_params.cache_bytes = new_cache_size; + m_db_params.wipe_data = false; + m_db = std::make_unique(m_db_params); } } @@ -114,8 +115,6 @@ bool CCoinsViewDB::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, boo CDBBatch batch(*m_db); size_t count = 0; size_t changed = 0; - size_t batch_size = (size_t)gArgs.GetIntArg("-dbbatchsize", nDefaultDbBatchSize); - int crash_simulate = gArgs.GetIntArg("-dbcrashratio", 0); assert(!hashBlock.IsNull()); uint256 old_tip = GetBestBlock(); @@ -149,13 +148,13 @@ bool CCoinsViewDB::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, boo } count++; it = erase ? mapCoins.erase(it) : std::next(it); - if (batch.SizeEstimate() > batch_size) { + if (batch.SizeEstimate() > m_options.batch_write_bytes) { LogPrint(BCLog::COINDB, "Writing partial batch of %.2f MiB\n", batch.SizeEstimate() * (1.0 / 1048576.0)); m_db->WriteBatch(batch); batch.Clear(); - if (crash_simulate) { + if (m_options.simulate_crash_ratio) { static FastRandomContext rng; - if (rng.randrange(crash_simulate) == 0) { + if (rng.randrange(m_options.simulate_crash_ratio) == 0) { LogPrintf("Simulating a crash. Goodbye.\n"); _Exit(0); } @@ -178,9 +177,6 @@ size_t CCoinsViewDB::EstimateSize() const return m_db->EstimateSize(DB_COIN, uint8_t(DB_COIN + 1)); } -CBlockTreeDB::CBlockTreeDB(size_t nCacheSize, bool fMemory, bool fWipe) : CDBWrapper(gArgs.GetDataDirNet() / "blocks" / "index", nCacheSize, fMemory, fWipe) { -} - bool CBlockTreeDB::ReadBlockFileInfo(int nFile, CBlockFileInfo &info) { return Read(std::make_pair(DB_BLOCK_FILES, nFile), info); } @@ -538,7 +534,7 @@ bool CBlockTreeDB::MigrateOldIndexData() uint256 best_block_hash; { fs::path chainstate_path = gArgs.GetDataDirNet() / "chainstate"; - CDBWrapper coins_db(chainstate_path, 0, false, false); + CDBWrapper coins_db{DBParams{.path = chainstate_path, .cache_bytes = 0}}; if (!coins_db.Read(DB_BEST_BLOCK, best_block_hash)) { // If we can't read the best block, the indexes will resync from scratch LogPrintf("Warning: Could not read best block from chainstate, migrated indexes will resync\n"); @@ -569,7 +565,7 @@ bool CBlockTreeDB::MigrateOldIndexData() // Migrate timestamp index (only if enabled) if (fTimestampIndex) { const fs::path db_path = indexes_path / "timestampindex"; - CDBWrapper timestamp_db(db_path, 0, false, false); + CDBWrapper timestamp_db{DBParams{.path = db_path, .cache_bytes = 0}}; // Pass nullptr to discard rather than copy if legacy data is stale. CDBWrapper* target = timestampindex_was_current ? ×tamp_db : nullptr; @@ -590,7 +586,7 @@ bool CBlockTreeDB::MigrateOldIndexData() // Migrate spent index (only if enabled) if (fSpentIndex) { const fs::path db_path = indexes_path / "spentindex"; - CDBWrapper spent_db(db_path, 0, false, false); + CDBWrapper spent_db{DBParams{.path = db_path, .cache_bytes = 0}}; CDBWrapper* target = spentindex_was_current ? &spent_db : nullptr; int64_t count = MigrateIndex( @@ -608,7 +604,7 @@ bool CBlockTreeDB::MigrateOldIndexData() // Migrate address index (includes both address and unspent indexes) (only if enabled) if (fAddressIndex) { const fs::path db_path = indexes_path / "addressindex"; - CDBWrapper address_db(db_path, 0, false, false); + CDBWrapper address_db{DBParams{.path = db_path, .cache_bytes = 0}}; CDBWrapper* target = addressindex_was_current ? &address_db : nullptr; diff --git a/src/txdb.h b/src/txdb.h index 15b34b187b35..b4ea64a07147 100644 --- a/src/txdb.h +++ b/src/txdb.h @@ -46,18 +46,24 @@ static const int64_t max_filter_index_cache = 1024; //! Max memory allocated to coin DB specific cache (MiB) static const int64_t nMaxCoinsDBCache = 8; +//! User-controlled performance and debug options. +struct CoinsViewOptions { + //! Maximum database write batch size in bytes. + size_t batch_write_bytes = nDefaultDbBatchSize; + //! If non-zero, randomly exit when the database is flushed with (1/ratio) + //! probability. + int simulate_crash_ratio = 0; +}; + /** CCoinsView backed by the coin database (chainstate/) */ class CCoinsViewDB final : public CCoinsView { protected: + DBParams m_db_params; + CoinsViewOptions m_options; std::unique_ptr m_db; - fs::path m_ldb_path; - bool m_is_memory; public: - /** - * @param[in] ldb_path Location in the filesystem where leveldb data will be stored. - */ - explicit CCoinsViewDB(fs::path ldb_path, size_t nCacheSize, bool fMemory, bool fWipe); + explicit CCoinsViewDB(DBParams db_params, CoinsViewOptions options); bool GetCoin(const COutPoint &outpoint, Coin &coin) const override; @@ -82,8 +88,7 @@ class CCoinsViewDB final : public CCoinsView class CBlockTreeDB : public CDBWrapper { public: - explicit CBlockTreeDB(size_t nCacheSize, bool fMemory = false, bool fWipe = false); - + using CDBWrapper::CDBWrapper; bool WriteBatchSync(const std::vector >& fileInfo, int nLastFile, const std::vector& blockinfo); bool ReadBlockFileInfo(int nFile, CBlockFileInfo &info); bool ReadLastBlockFile(int &nFile); diff --git a/src/validation.cpp b/src/validation.cpp index 397626256a13..f765455e9015 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1889,13 +1889,9 @@ CAmount GetBlockSubsidy(const CBlockIndex* const pindex, const Consensus::Params return GetBlockSubsidyInner(pindex->pprev->nBits, pindex->pprev->nHeight, consensusParams, isV20Active); } -CoinsViews::CoinsViews( - fs::path ldb_name, - size_t cache_size_bytes, - bool in_memory, - bool should_wipe) : m_dbview( - gArgs.GetDataDirNet() / ldb_name, cache_size_bytes, in_memory, should_wipe), - m_catcherview(&m_dbview) {} +CoinsViews::CoinsViews(DBParams db_params, CoinsViewOptions options) + : m_dbview{std::move(db_params), std::move(options)}, + m_catcherview(&m_dbview) {} void CoinsViews::InitCache() { @@ -1952,7 +1948,14 @@ void Chainstate::InitCoinsDB( } m_coins_views = std::make_unique( - leveldb_name, cache_size_bytes, in_memory, should_wipe); + DBParams{ + .path = m_chainman.m_options.datadir / leveldb_name, + .cache_bytes = cache_size_bytes, + .memory_only = in_memory, + .wipe_data = should_wipe, + .obfuscate = true, + .options = m_chainman.m_options.coins_db}, + m_chainman.m_options.coins_view); } void Chainstate::InitCoinsCache(size_t cache_size_bytes) diff --git a/src/validation.h b/src/validation.h index e0c4087df5f0..f7216984375f 100644 --- a/src/validation.h +++ b/src/validation.h @@ -446,7 +446,7 @@ class CoinsViews { //! state to disk, which should not be done until the health of the database is verified. //! //! All arguments forwarded onto CCoinsViewDB. - CoinsViews(fs::path ldb_name, size_t cache_size_bytes, bool in_memory, bool should_wipe); + CoinsViews(DBParams db_params, CoinsViewOptions options); //! Initialize the CCoinsViewCache member. void InitCache() EXCLUSIVE_LOCKS_REQUIRED(::cs_main); From 8b9cea6bd01880345216973882a6e4b32bd48ac7 Mon Sep 17 00:00:00 2001 From: DCG-Claude Date: Thu, 1 Oct 2026 05:59:20 -0500 Subject: [PATCH 2/2] Merge bitcoin/bitcoin#27499: net processing, refactor: Decouple PeerManager from gArgs 23c7b51ddd2888cf7fb260c439f004bd28768473 [net processing] Move -capturemessages to PeerManager::Options (dergoegge) bd59bda26b67b53293a5cef7433e992203da325a [net processing] Move -blockreconstructionextratxn to PeerManager::Options (dergoegge) 567c4e0b6a3fadd2fd1be732076026bf491519b2 [net processing] Move -maxorphantx to PeerManager::Options (dergoegge) fa9e6d80d1c55f8b1bb2691bfd67e8c2b7189b38 [net processing] Move -txreconciliation to PeerManager::Options (dergoegge) 4cfb7b925f8fea818f03433882138a7d3d7e179a [net processing] Use ignore_incoming_txs from m_opts (dergoegge) 8b877259217c6da316153afb136a55c2dbd401c2 [net processing] Introduce PeerManager options (dergoegge) Pull request description: This PR decouples `PeerManager` from our global args manager by introducing `PeerManager::Options`. ACKs for top commit: stickies-v: re-ACK 23c7b51ddd2888cf7fb260c439f004bd28768473 TheCharlatan: ACK 23c7b51ddd2888cf7fb260c439f004bd28768473 Tree-SHA512: cd807b36ec018010e11935d3539fa7ed5015fdfb531d13a042a65b54ee8533a35a919a6a6c5fa293b5cba76000e9403c64dfd790fb9c649b7838544929b1fee8 Dash adaptations: - src/net_processing.h: PeerManager::Options has `uint32_t max_orphan_txs_size{DEFAULT_MAX_ORPHAN_TRANSACTIONS_SIZE * 1000000}` instead of upstream's `max_orphan_txs{DEFAULT_MAX_ORPHAN_TRANSACTIONS}`, because Dash limits orphans by total size (-maxorphantxsize, MB), not by count (-maxorphantx). The DEFAULT_MAX_ORPHAN_TRANSACTIONS constant is not added (Dash doesn't have it), and DEFAULT_MAX_ORPHAN_TRANSACTIONS_SIZE is kept - src/net_processing.h: DEFAULT_TXRECONCILIATION_ENABLE moved here from node/txreconciliation.h as upstream does, placed above Dash's DEFAULT_MAX_ORPHAN_TRANSACTIONS_SIZE - src/net_processing.h/.cpp: PeerManager::make and the PeerManagerImpl constructor keep all of Dash's extra parameters (dstxman, mn_metaman, mn_sync, sporkman, chainlocks, clhandler, nodeman, dmnman, cj_walletman, isman, llmq_ctx); only the trailing `bool ignore_incoming_txs` becomes `Options opts`, and the m_opts{opts} initializer goes after m_clhandler - src/net_processing.cpp: the orphan-limit line becomes `m_orphanage.LimitOrphans(m_opts.max_orphan_txs_size)` in place of Dash's inline gArgs -maxorphantxsize*1000000 computation. Dash's ForgetTx() call and comments are kept (upstream's m_txrequest.ForgetTxHash/GetWitnessHash lines are not brought in) - src/net_processing.cpp: AddToCompactExtraTransactions keeps Dash's GetInstanceHash() (upstream uses GetWitnessHash()); only the gArgs read becomes m_opts.max_extra_txs - src/net_processing.cpp: upstream's `-#include ` hunk doesn't apply because Dash's net_processing.cpp never included it. util/system.h stays because Dash still uses gArgs there for -pushversion and the devnet name - src/node/peerman_args.cpp: includes instead of (Dash hasn't done the common/args split, bitcoin#27419, and ArgsManager lives in util/system.h); reads -maxorphantxsize and stores clamp(>=0)*1000000 into max_orphan_txs_size, the same value Dash computed inline before; the other three options match upstream - src/init.cpp: peerman_opts is built and ApplyArgsManOptions is called just before Dash's PeerManager::make, whose Dash arguments are unchanged, with peerman_opts as the last argument. The node/txreconciliation.h include becomes node/peerman_args.h, and Dash's node/sync_manager.h include stays - src/test/util/setup_common.{h,cpp}: Dash builds peers through its own MakePeerManager(connman, node, banman, ...) helper, so the helper's last parameter changes from `bool ignore_incoming_txs` to `PeerManager::Options opts`. TestingSetup builds peerman_opts with ApplyArgsManOptions(*m_node.args, ...) as upstream does. setup_common.h now includes because Options must be a complete type there (node/context.h only forward-declares PeerManager). Upstream's connman line in this hunk is not taken, because Dash creates connman earlier in ChainTestingSetup - src/test/denialofservice_tests.cpp: the four call sites keep Dash's MakePeerManager helper and pass `{}` instead of `/*ignore_incoming_txs=*/false`, which matches upstream's use of `{}` - src/test/net_peer_connection_tests.cpp (Dash-only caller, not touched upstream): its MakePeerManager call now passes `{}` for the new Options parameter --- src/Makefile.am | 2 ++ src/init.cpp | 9 +++++-- src/net_processing.cpp | 37 ++++++++++++-------------- src/net_processing.h | 13 ++++++++- src/node/peerman_args.cpp | 27 +++++++++++++++++++ src/node/peerman_args.h | 12 +++++++++ src/node/txreconciliation.h | 2 -- src/test/denialofservice_tests.cpp | 8 +++--- src/test/net_peer_connection_tests.cpp | 2 +- src/test/util/setup_common.cpp | 11 +++++--- src/test/util/setup_common.h | 3 ++- 11 files changed, 91 insertions(+), 35 deletions(-) create mode 100644 src/node/peerman_args.cpp create mode 100644 src/node/peerman_args.h diff --git a/src/Makefile.am b/src/Makefile.am index 540ceb68c2ba..212da93938b9 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -363,6 +363,7 @@ BITCOIN_CORE_H = \ node/mempool_persist_args.h \ node/miner.h \ node/minisketchwrapper.h \ + node/peerman_args.h \ node/psbt.h \ node/sync_manager.h \ node/transaction.h \ @@ -643,6 +644,7 @@ libbitcoin_node_a_SOURCES = \ node/mempool_persist_args.cpp \ node/miner.cpp \ node/minisketchwrapper.cpp \ + node/peerman_args.cpp \ node/psbt.cpp \ node/sync_manager.cpp \ node/transaction.cpp \ diff --git a/src/init.cpp b/src/init.cpp index 2f219dbb3388..30936f791d85 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -56,8 +56,8 @@ #include #include #include +#include #include -#include #include #include #include @@ -2184,12 +2184,17 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) } #endif + PeerManager::Options peerman_opts{ + .ignore_incoming_txs = ignores_incoming_txs, + }; + ApplyArgsManOptions(args, peerman_opts); + assert(!node.peerman); node.peerman = PeerManager::make(*node.connman, *node.addrman, node.banman.get(), *node.dstxman, chainman, *node.mempool, *node.mn_metaman, *node.mn_sync, *node.sporkman, *node.chainlocks, *node.clhandler, node.active_ctx ? node.active_ctx->nodeman.get() : nullptr, - *node.dmnman, node.cj_walletman.get(), *node.isman, *node.llmq_ctx, ignores_incoming_txs); + *node.dmnman, node.cj_walletman.get(), *node.isman, *node.llmq_ctx, peerman_opts); RegisterValidationInterface(node.peerman.get()); node.ds_notification_interface = std::make_unique( diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 6c6b9ea593ab..b83113cee65e 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -571,7 +571,7 @@ class PeerManagerImpl final : public PeerManager CDeterministicMNManager& dmnman, CJWalletManager* cj_walletman, llmq::CInstantSendManager& isman, - LLMQContext& llmq_ctx, bool ignore_incoming_txs); + LLMQContext& llmq_ctx, Options opts); ~PeerManagerImpl() { @@ -607,7 +607,7 @@ class PeerManagerImpl final : public PeerManager std::optional FetchBlock(NodeId peer_id, const CBlockIndex& block_index) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); bool GetNodeStateStats(NodeId nodeid, CNodeStateStats& stats) const override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); - bool IgnoresIncomingTxs() override { return m_ignore_incoming_txs; } + bool IgnoresIncomingTxs() override { return m_opts.ignore_incoming_txs; } void SendPings() override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);; void PushInventory(NodeId nodeid, const CInv& inv) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); void RelayInv(const CInv& inv) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); @@ -847,8 +847,7 @@ class PeerManagerImpl final : public PeerManager /** Next time to check for stale tip */ std::chrono::seconds m_stale_tip_check_time GUARDED_BY(cs_main){0s}; - /** Whether this node is running in -blocksonly mode */ - const bool m_ignore_incoming_txs; + const Options m_opts; bool RejectIncomingTxs(const CNode& peer) const; @@ -1364,7 +1363,7 @@ void PeerManagerImpl::MaybeSetPeerAsAnnouncingHeaderAndIDs(NodeId nodeid) // When in -blocksonly mode, never request high-bandwidth mode from peers. Our // mempool will not contain the transactions necessary to reconstruct the // compact block. - if (m_ignore_incoming_txs) return; + if (m_opts.ignore_incoming_txs) return; CNodeState* nodestate = State(nodeid); if (!nodestate || !nodestate->m_provides_cmpctblocks) { @@ -1921,13 +1920,12 @@ bool PeerManagerImpl::GetNodeStateStats(NodeId nodeid, CNodeStateStats& stats) c void PeerManagerImpl::AddToCompactExtraTransactions(const CTransactionRef& tx) { - size_t max_extra_txn = gArgs.GetIntArg("-blockreconstructionextratxn", DEFAULT_BLOCK_RECONSTRUCTION_EXTRA_TXN); - if (max_extra_txn <= 0) + if (m_opts.max_extra_txs <= 0) return; if (!vExtraTxnForCompact.size()) - vExtraTxnForCompact.resize(max_extra_txn); + vExtraTxnForCompact.resize(m_opts.max_extra_txs); vExtraTxnForCompact[vExtraTxnForCompactIt] = std::make_pair(tx->GetInstanceHash(), tx); - vExtraTxnForCompactIt = (vExtraTxnForCompactIt + 1) % max_extra_txn; + vExtraTxnForCompactIt = (vExtraTxnForCompactIt + 1) % m_opts.max_extra_txs; } void PeerManagerImpl::Misbehaving(Peer& peer, int howmuch, const std::string& message) @@ -2098,9 +2096,9 @@ std::unique_ptr PeerManager::make(CConnman& connman, AddrMan& addrm CDeterministicMNManager& dmnman, CJWalletManager* cj_walletman, llmq::CInstantSendManager& isman, - LLMQContext& llmq_ctx, bool ignore_incoming_txs) + LLMQContext& llmq_ctx, Options opts) { - return std::make_unique(connman, addrman, banman, dstxman, chainman, pool, mn_metaman, mn_sync, sporkman, chainlocks, clhandler, nodeman, dmnman, cj_walletman, isman, llmq_ctx, ignore_incoming_txs); + return std::make_unique(connman, addrman, banman, dstxman, chainman, pool, mn_metaman, mn_sync, sporkman, chainlocks, clhandler, nodeman, dmnman, cj_walletman, isman, llmq_ctx, opts); } PeerManagerImpl::PeerManagerImpl(CConnman& connman, AddrMan& addrman, BanMan* banman, @@ -2113,7 +2111,7 @@ PeerManagerImpl::PeerManagerImpl(CConnman& connman, AddrMan& addrman, BanMan* ba CDeterministicMNManager& dmnman, CJWalletManager* cj_walletman, llmq::CInstantSendManager& isman, - LLMQContext& llmq_ctx, bool ignore_incoming_txs) + LLMQContext& llmq_ctx, Options opts) : m_chainparams(chainman.GetParams()), m_connman(connman), m_addrman(addrman), @@ -2131,11 +2129,11 @@ PeerManagerImpl::PeerManagerImpl(CConnman& connman, AddrMan& addrman, BanMan* ba m_sporkman(sporkman), m_chainlocks(chainlocks), m_clhandler{clhandler}, - m_ignore_incoming_txs(ignore_incoming_txs) + m_opts{opts} { // While Erlay support is incomplete, it must be enabled explicitly via -txreconciliation. // This argument can go away after Erlay support is complete. - if (gArgs.GetBoolArg("-txreconciliation", DEFAULT_TXRECONCILIATION_ENABLE)) { + if (opts.reconcile_txs) { m_txreconciliation = std::make_unique(TXRECONCILIATION_VERSION); } } @@ -3245,7 +3243,7 @@ void PeerManagerImpl::HeadersDirectFetchBlocks(CNode& pfrom, const Peer& peer, c last_header.nHeight); } if (vGetData.size() > 0) { - if (!m_ignore_incoming_txs && + if (!m_opts.ignore_incoming_txs && nodestate->m_provides_cmpctblocks && vGetData.size() == 1 && mapBlocksInFlight.size() == 1 && @@ -4091,7 +4089,7 @@ void PeerManagerImpl::ProcessMessage( // - we are not in -blocksonly mode. const auto* tx_relay = peer->GetTxRelay(); if (tx_relay && WITH_LOCK(tx_relay->m_bloom_filter_mutex, return tx_relay->m_relay_txs) && - !pfrom.IsAddrFetchConn() && !m_ignore_incoming_txs) { + !pfrom.IsAddrFetchConn() && !m_opts.ignore_incoming_txs) { const uint64_t recon_salt = m_txreconciliation->PreRegisterPeer(pfrom.GetId()); m_connman.PushMessage(&pfrom, msg_maker.Make(NetMsgType::SENDTXRCNCL, TXRECONCILIATION_VERSION, recon_salt)); @@ -5000,8 +4998,7 @@ void PeerManagerImpl::ProcessMessage( ForgetTx(tx.GetHash()); // DoS prevention: do not allow m_orphans to grow unbounded (see CVE-2012-3789) - unsigned int nMaxOrphanTxSize = (unsigned int)std::max((int64_t)0, gArgs.GetIntArg("-maxorphantxsize", DEFAULT_MAX_ORPHAN_TRANSACTIONS_SIZE)) * 1000000; - m_orphanage.LimitOrphans(nMaxOrphanTxSize); + m_orphanage.LimitOrphans(m_opts.max_orphan_txs_size); } else { LogPrint(BCLog::MEMPOOL, "not keeping orphan with rejected parents %s\n",tx.GetHash().ToString()); // We will continue to reject this tx since it has rejected @@ -5916,7 +5913,7 @@ bool PeerManagerImpl::ProcessMessages(CNode* pfrom, std::atomic& interrupt msg.m_recv.data() ); - if (gArgs.GetBoolArg("-capturemessages", false)) { + if (m_opts.capture_messages) { CaptureMessage(pfrom->addr, msg.m_type, MakeUCharSpan(msg.m_recv), /*is_incoming=*/true); } @@ -6274,7 +6271,7 @@ bool PeerManagerImpl::RejectIncomingTxs(const CNode& peer) const if (peer.IsBlockOnlyConn()) return true; if (peer.IsFeelerConn()) return true; // In -blocksonly mode, peers need the 'relay' permission to send txs to us - if (m_ignore_incoming_txs && !peer.HasPermission(NetPermissionFlags::Relay)) return true; + if (m_opts.ignore_incoming_txs && !peer.HasPermission(NetPermissionFlags::Relay)) return true; return false; } diff --git a/src/net_processing.h b/src/net_processing.h index cb5e03a9570e..54f98cd86ae8 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -37,6 +37,8 @@ namespace llmq { class CInstantSendManager; } // namespace llmq +/** Whether transaction reconciliation protocol should be enabled by default. */ +static constexpr bool DEFAULT_TXRECONCILIATION_ENABLE{false}; /** Default for -maxorphantxsize, maximum size in megabytes the orphan map can grow before entries are removed */ static const unsigned int DEFAULT_MAX_ORPHAN_TRANSACTIONS_SIZE = 10; // this allows around 100 TXs of max size (and many more of normal size) /** Default number of orphan+recently-replaced txn to keep around for block reconstruction */ @@ -83,6 +85,15 @@ struct CNodeStateStats { class PeerManagerInternal { public: + struct Options { + /** Whether this node is running in -blocksonly mode */ + bool ignore_incoming_txs{DEFAULT_BLOCKSONLY}; + bool reconcile_txs{DEFAULT_TXRECONCILIATION_ENABLE}; + uint32_t max_orphan_txs_size{DEFAULT_MAX_ORPHAN_TRANSACTIONS_SIZE * 1000000}; + size_t max_extra_txs{DEFAULT_BLOCK_RECONSTRUCTION_EXTRA_TXN}; + bool capture_messages{false}; + }; + virtual void PeerMisbehaving(const NodeId pnode, const int howmuch, const std::string& message = "") = 0; virtual bool PeerIsBanned(const NodeId node_id) = 0; /** Complete this peer's pending announcement of the inv, so it is not requested from them @@ -162,7 +173,7 @@ class PeerManager : public CValidationInterface, public NetEventsInterface, publ CDeterministicMNManager& dmnman, CJWalletManager* cj_walletman, llmq::CInstantSendManager& isman, - LLMQContext& llmq_ctx, bool ignore_incoming_txs); + LLMQContext& llmq_ctx, Options opts); virtual ~PeerManager() { } /** diff --git a/src/node/peerman_args.cpp b/src/node/peerman_args.cpp new file mode 100644 index 000000000000..b930ebf53241 --- /dev/null +++ b/src/node/peerman_args.cpp @@ -0,0 +1,27 @@ +#include + +#include +#include + +#include +#include + +namespace node { + +void ApplyArgsManOptions(const ArgsManager& argsman, PeerManager::Options& options) +{ + if (auto value{argsman.GetBoolArg("-txreconciliation")}) options.reconcile_txs = *value; + + if (auto value{argsman.GetIntArg("-maxorphantxsize")}) { + options.max_orphan_txs_size = uint32_t(std::clamp(*value, 0, std::numeric_limits::max() / 1000000) * 1000000); + } + + if (auto value{argsman.GetIntArg("-blockreconstructionextratxn")}) { + options.max_extra_txs = size_t(std::max(int64_t{0}, *value)); + } + + if (auto value{argsman.GetBoolArg("-capturemessages")}) options.capture_messages = *value; +} + +} // namespace node + diff --git a/src/node/peerman_args.h b/src/node/peerman_args.h new file mode 100644 index 000000000000..73dbdb446c26 --- /dev/null +++ b/src/node/peerman_args.h @@ -0,0 +1,12 @@ +#ifndef BITCOIN_NODE_PEERMAN_ARGS_H +#define BITCOIN_NODE_PEERMAN_ARGS_H + +#include + +class ArgsManager; + +namespace node { +void ApplyArgsManOptions(const ArgsManager& argsman, PeerManager::Options& options); +} // namespace node + +#endif // BITCOIN_NODE_PEERMAN_ARGS_H diff --git a/src/node/txreconciliation.h b/src/node/txreconciliation.h index 4591dd5df7c9..3bbb0773664f 100644 --- a/src/node/txreconciliation.h +++ b/src/node/txreconciliation.h @@ -11,8 +11,6 @@ #include #include -/** Whether transaction reconciliation protocol should be enabled by default. */ -static constexpr bool DEFAULT_TXRECONCILIATION_ENABLE{false}; /** Supported transaction reconciliation protocol version */ static constexpr uint32_t TXRECONCILIATION_VERSION{1}; diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp index 9286528571f1..5d6cc3e6aedb 100644 --- a/src/test/denialofservice_tests.cpp +++ b/src/test/denialofservice_tests.cpp @@ -146,7 +146,7 @@ BOOST_AUTO_TEST_CASE(stale_tip_peer_management) { NodeId id{0}; auto connman = std::make_unique(0x1337, 0x1337, *m_node.addrman, *m_node.netgroupman); - auto peerLogic = MakePeerManager(*connman, m_node, /*banman=*/nullptr, /*ignore_incoming_txs=*/false); + auto peerLogic = MakePeerManager(*connman, m_node, /*banman=*/nullptr, {}); constexpr int max_outbound_full_relay = MAX_OUTBOUND_FULL_RELAY_CONNECTIONS; CConnman::Options options; @@ -246,7 +246,7 @@ BOOST_AUTO_TEST_CASE(block_relay_only_eviction) { NodeId id{0}; auto connman = std::make_unique(0x1337, 0x1337, *m_node.addrman, *m_node.netgroupman); - auto peerLogic = MakePeerManager(*connman, m_node, /*banman=*/nullptr, /*ignore_incoming_txs=*/false); + auto peerLogic = MakePeerManager(*connman, m_node, /*banman=*/nullptr, {}); constexpr int max_outbound_block_relay{MAX_BLOCK_RELAY_ONLY_CONNECTIONS}; constexpr int64_t MINIMUM_CONNECT_TIME{30}; @@ -309,7 +309,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement) auto banman = std::make_unique(m_args.GetDataDirBase() / "banlist", nullptr, DEFAULT_MISBEHAVING_BANTIME); auto connman = std::make_unique(0x1337, 0x1337, *m_node.addrman, *m_node.netgroupman); - auto peerLogic = MakePeerManager(*connman, m_node, banman.get(), /*ignore_incoming_txs=*/false); + auto peerLogic = MakePeerManager(*connman, m_node, banman.get(), {}); CNetAddr tor_netaddr; BOOST_REQUIRE( @@ -410,7 +410,7 @@ BOOST_AUTO_TEST_CASE(DoS_bantime) auto banman = std::make_unique(m_args.GetDataDirBase() / "banlist", nullptr, DEFAULT_MISBEHAVING_BANTIME); auto connman = std::make_unique(0x1337, 0x1337, *m_node.addrman, *m_node.netgroupman); - auto peerLogic = MakePeerManager(*connman, m_node, banman.get(), /*ignore_incoming_txs=*/false); + auto peerLogic = MakePeerManager(*connman, m_node, banman.get(), {}); banman->ClearBanned(); int64_t nStartTime = GetTime(); diff --git a/src/test/net_peer_connection_tests.cpp b/src/test/net_peer_connection_tests.cpp index da234a6ae088..9c1ed911a87b 100644 --- a/src/test/net_peer_connection_tests.cpp +++ b/src/test/net_peer_connection_tests.cpp @@ -83,7 +83,7 @@ static void AddPeer(NodeId& id, std::vector& nodes, PeerManager& peerman BOOST_AUTO_TEST_CASE(test_addnode_getaddednodeinfo_and_connection_detection) { auto connman = std::make_unique(0x1337, 0x1337, *m_node.addrman, *m_node.netgroupman); - auto peerman = MakePeerManager(*connman, m_node, /*banman=*/nullptr, /*ignore_incoming_txs=*/false); + auto peerman = MakePeerManager(*connman, m_node, /*banman=*/nullptr, {}); NodeId id{0}; std::vector nodes; diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index 5433f9573fb7..98b6fd0ad038 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -34,6 +34,7 @@ #include #include #include +#include #include #include #include @@ -119,11 +120,11 @@ std::ostream& operator<<(std::ostream& os, const uint256& num) std::unique_ptr MakePeerManager(CConnman& connman, NodeContext& node, BanMan* banman, - bool ignore_incoming_txs) + PeerManager::Options opts) { return PeerManager::make(connman, *node.addrman, banman, *node.dstxman, *node.chainman, *node.mempool, *node.mn_metaman, *node.mn_sync, *node.sporkman, *node.chainlocks, *node.clhandler, /*nodeman=*/nullptr, *node.dmnman, node.cj_walletman.get(), - *node.isman, *node.llmq_ctx, ignore_incoming_txs); + *node.isman, *node.llmq_ctx, opts); } struct NetworkSetup @@ -405,8 +406,10 @@ TestingSetup::TestingSetup( #endif // ENABLE_WALLET m_node.banman = std::make_unique(m_args.GetDataDirBase() / "banlist", nullptr, DEFAULT_MISBEHAVING_BANTIME); - m_node.peerman = MakePeerManager(*m_node.connman, m_node, m_node.banman.get(), - /*ignore_incoming_txs=*/false); + PeerManager::Options peerman_opts; + ApplyArgsManOptions(*m_node.args, peerman_opts); + m_node.peerman = MakePeerManager(*m_node.connman, m_node, m_node.banman.get(), peerman_opts); + { CConnman::Options options; options.m_msgproc = m_node.peerman.get(); diff --git a/src/test/util/setup_common.h b/src/test/util/setup_common.h index 73652650a83d..a2d353f99e3f 100644 --- a/src/test/util/setup_common.h +++ b/src/test/util/setup_common.h @@ -8,6 +8,7 @@ #include #include +#include #include #include #include // IWYU pragma: export @@ -54,7 +55,7 @@ static constexpr CAmount CENT{1000000}; std::unique_ptr MakePeerManager(CConnman& connman, node::NodeContext& node, BanMan* banman, - bool ignore_incoming_txs); + PeerManager::Options opts); /** Basic testing setup. * This just configures logging, data dir and chain parameters.