Merge bitcoin/bitcoin#25862: refactor, kernel: Remove gArgs accesses from dbwrapper and txdb

achow101 · achow101 · commit 9321df448768 · 2023-02-17T16:54:55.000-05:00
aadd7c5 refactor, validation: Add ChainstateManagerOpts db options (Ryan Ofsky) 0352258 refactor, txdb: Use DBParams struct in CBlockTreeDB (Ryan Ofsky) c00fa1a refactor, txdb: Add CoinsViewOptions struct (Ryan Ofsky) 2eaeded 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 aadd7c5 achow101: ACK aadd7c5 furszy: diff ACK aadd7c5 Tree-SHA512: 46dfd5d99ab3110492e7bba97a87122c831b8344caaf7dd2ebdb6e0ad6aa9174d4d1832d6f3a7465eda9294fe50defaa3c000afbbddc4e72838687df09a63ffd
diff --git a/src/Makefile.am b/src/Makefile.am
@@ -204,8 +204,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 \
@@ -388,8 +390,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 \
diff --git a/src/bitcoin-chainstate.cpp b/src/bitcoin-chainstate.cpp
@@ -82,6 +82,7 @@ int main(int argc, char* argv[])
     // SETUP: Chainstate
     const ChainstateManager::Options chainman_opts{
         .chainparams = chainparams,
+        .datadir = gArgs.GetDataDirNet(),
         .adjusted_time_callback = NodeClock::now,
     };
     ChainstateManager chainman{chainman_opts};
diff --git a/src/dbwrapper.cpp b/src/dbwrapper.cpp
@@ -127,48 +127,48 @@ 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.
     obfuscate_key = std::vector<unsigned char>(OBFUSCATE_KEY_NUM_BYTES, '\000');
 
     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<unsigned char> 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
@@ -31,6 +31,29 @@ class Env;
 static const size_t DBWRAPPER_PREALLOC_KEY_SIZE = 64;
 static const size_t DBWRAPPER_PREALLOC_VALUE_SIZE = 1024;
 
+//! 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:
@@ -230,15 +253,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;
diff --git a/src/index/base.cpp b/src/index/base.cpp
@@ -8,6 +8,7 @@
 #include <kernel/chain.h>
 #include <node/blockstorage.h>
 #include <node/context.h>
+#include <node/database_args.h>
 #include <node/interface_ui.h>
 #include <shutdown.h>
 #include <tinyformat.h>
@@ -48,7 +49,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
@@ -1046,6 +1046,7 @@ bool AppInitParameterInteraction(const ArgsManager& args, bool use_syscall_sandb
     {
         ChainstateManager::Options chainman_opts_dummy{
             .chainparams = chainparams,
+            .datadir = args.GetDataDirNet(),
         };
         if (const auto error{ApplyArgsManOptions(args, chainman_opts_dummy)}) {
             return InitError(*error);
@@ -1444,6 +1445,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(),
         .adjusted_time_callback = GetAdjustedTime,
     };
     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
@@ -6,6 +6,8 @@
 #define BITCOIN_KERNEL_CHAINSTATEMANAGER_OPTS_H
 
 #include <arith_uint256.h>
+#include <dbwrapper.h>
+#include <txdb.h>
 #include <uint256.h>
 #include <util/time.h>
 
@@ -27,6 +29,7 @@ namespace kernel {
  */
 struct ChainstateManagerOpts {
     const CChainParams& chainparams;
+    fs::path datadir;
     const std::function<NodeClock::time_point()> adjusted_time_callback{nullptr};
     std::optional<bool> check_block_index{};
     bool checkpoints_enabled{DEFAULT_CHECKPOINTS_ENABLED};
@@ -36,6 +39,9 @@ struct ChainstateManagerOpts {
     std::optional<uint256> 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
@@ -64,7 +64,12 @@ ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSize
     // 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<CBlockTreeDB>(DBParams{
+        .path = chainman.m_options.datadir / "blocks" / "index",
+        .cache_bytes = static_cast<size_t>(cache_sizes.block_tree_db),
+        .memory_only = options.block_tree_db_in_memory,
+        .wipe_data = options.reindex,
+        .options = chainman.m_options.block_tree_db});
 
     if (options.reindex) {
         pblocktree->WriteReindexing(true);
diff --git a/src/node/chainstatemanager_args.cpp b/src/node/chainstatemanager_args.cpp
@@ -5,6 +5,9 @@
 #include <node/chainstatemanager_args.h>
 
 #include <arith_uint256.h>
+#include <kernel/chainstatemanager_opts.h>
+#include <node/coins_view_args.h>
+#include <node/database_args.h>
 #include <tinyformat.h>
 #include <uint256.h>
 #include <util/strencodings.h>
@@ -34,6 +37,10 @@ std::optional<bilingual_str> 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
@@ -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 <node/coins_view_args.h>
+
+#include <txdb.h>
+#include <util/system.h>
+
+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
@@ -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
@@ -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 <node/database_args.h>
+
+#include <dbwrapper.h>
+#include <util/system.h>
+
+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
@@ -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
@@ -278,7 +278,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);
 }
 
@@ -1064,7 +1064,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<std::unique_ptr<CCoinsViewCacheTest>> caches;
     caches.push_back(std::make_unique<CCoinsViewCacheTest>(&base));
     caches.push_back(std::make_unique<CCoinsViewCacheTest>(caches.back().get()));
diff --git a/src/test/dbwrapper_tests.cpp b/src/test/dbwrapper_tests.cpp
diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp
diff --git a/src/txdb.cpp b/src/txdb.cpp
diff --git a/src/txdb.h b/src/txdb.h
diff --git a/src/validation.cpp b/src/validation.cpp
diff --git a/src/validation.h b/src/validation.h
