refactor, dbwrapper: Add DBParams and DBOptions structs

Add DBParams and DBOptions structs to remove ArgsManager uses from dbwrapper.

To reduce size of this commit, this moves references to gArgs variable out of
dbwrapper.cpp to calling code in txdb.cpp. But these moves are temporary. The
gArgs references in txdb.cpp are moved out to calling code in init.cpp in later
commits.

This commit does not change behavior.

Author: ryanofsky

src/Makefile.am

@@ -206,6 +206,7 @@ BITCOIN_CORE_H = \
   node/coin.h \
   node/connection_types.h \
   node/context.h \
+  node/database_args.h \
   node/eviction.h \
   node/interface_ui.h \
   node/mempool_args.h \
@@ -390,6 +391,7 @@ libbitcoin_node_a_SOURCES = \
   node/coin.cpp \
   node/connection_types.cpp \
   node/context.cpp \
+  node/database_args.cpp \
   node/eviction.cpp \
   node/interface_ui.cpp \
   node/interfaces.cpp \

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()

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;

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

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

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

src/test/dbwrapper_tests.cpp

@@ -28,7 +28,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;
@@ -47,7 +47,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;
@@ -128,7 +128,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();
@@ -164,7 +164,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'};
@@ -207,7 +207,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<CDBWrapper> dbw = std::make_unique<CDBWrapper>(ph, (1 << 10), false, false, false);
+    std::unique_ptr<CDBWrapper> dbw = std::make_unique<CDBWrapper>(DBParams{.path = ph, .cache_bytes = 1 << 10, .memory_only = false, .wipe_data = false, .obfuscate = false});
     uint8_t key{'k'};
     uint256 in = InsecureRand256();
     uint256 res;
@@ -220,7 +220,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.
@@ -248,7 +248,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<CDBWrapper> dbw = std::make_unique<CDBWrapper>(ph, (1 << 10), false, false, false);
+    std::unique_ptr<CDBWrapper> dbw = std::make_unique<CDBWrapper>(DBParams{.path = ph, .cache_bytes = 1 << 10, .memory_only = false, .wipe_data = false, .obfuscate = false});
     uint8_t key{'k'};
     uint256 in = InsecureRand256();
     uint256 res;
@@ -261,7 +261,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;
@@ -280,7 +280,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;
@@ -348,7 +348,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)};
@@ -390,7 +390,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));

src/txdb.cpp

@@ -6,6 +6,7 @@
 #include <txdb.h>
 
 #include <chain.h>
+#include <node/database_args.h>
 #include <pow.h>
 #include <random.h>
 #include <shutdown.h>
@@ -71,7 +72,13 @@ struct CoinEntry {
 } // namespace
 
 CCoinsViewDB::CCoinsViewDB(fs::path ldb_path, size_t nCacheSize, bool fMemory, bool fWipe) :
-    m_db(std::make_unique<CDBWrapper>(ldb_path, nCacheSize, fMemory, fWipe, true)),
+    m_db{std::make_unique<CDBWrapper>(DBParams{
+        .path = ldb_path,
+        .cache_bytes = nCacheSize,
+        .memory_only = fMemory,
+        .wipe_data = fWipe,
+        .obfuscate = true,
+        .options = [] { DBOptions options; node::ReadDatabaseArgs(gArgs, options); return options; }()})},
     m_ldb_path(ldb_path),
     m_is_memory(fMemory) { }
 
@@ -83,8 +90,13 @@ void CCoinsViewDB::ResizeCache(size_t new_cache_size)
         // 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<CDBWrapper>(
-            m_ldb_path, new_cache_size, m_is_memory, /*fWipe=*/false, /*obfuscate=*/true);
+        m_db = std::make_unique<CDBWrapper>(DBParams{
+            .path = m_ldb_path,
+            .cache_bytes = new_cache_size,
+            .memory_only = m_is_memory,
+            .wipe_data = false,
+            .obfuscate = true,
+            .options = [] { DBOptions options; node::ReadDatabaseArgs(gArgs, options); return options; }()});
     }
 }
 
@@ -176,7 +188,12 @@ 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) {
+CBlockTreeDB::CBlockTreeDB(size_t nCacheSize, bool fMemory, bool fWipe) : CDBWrapper{DBParams{
+    .path = gArgs.GetDataDirNet() / "blocks" / "index",
+    .cache_bytes = nCacheSize,
+    .memory_only = fMemory,
+    .wipe_data = fWipe,
+    .options = [] { DBOptions options; node::ReadDatabaseArgs(gArgs, options); return options; }()}} {
 }
 
 bool CBlockTreeDB::ReadBlockFileInfo(int nFile, CBlockFileInfo &info) {