From 2eaeded37f3a07c35eef38d9a80c1e5fbd4d41ee Mon Sep 17 00:00:00 2001
From: Ryan Ofsky <ryan@ofsky.org>
Date: Tue, 16 Aug 2022 23:32:55 -0400
Subject: [PATCH 1/4] 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.
---
 src/Makefile.am              |  2 ++
 src/dbwrapper.cpp            | 32 ++++++++++++++++----------------
 src/dbwrapper.h              | 33 ++++++++++++++++++++++++---------
 src/index/base.cpp           |  9 ++++++++-
 src/node/database_args.cpp   | 18 ++++++++++++++++++
 src/node/database_args.h     | 15 +++++++++++++++
 src/test/dbwrapper_tests.cpp | 22 +++++++++++-----------
 src/txdb.cpp                 | 25 +++++++++++++++++++++----
 8 files changed, 115 insertions(+), 41 deletions(-)
 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 5830090ada..6d5f2eebd3 100644
--- a/src/Makefile.am
+++ b/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 \
diff --git a/src/dbwrapper.cpp b/src/dbwrapper.cpp
index 6efaf2ec19..0c6debfa80 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<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
index b389d039fb..578d9880ac 100644
--- 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 @@ private:
     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
index 1d5c0dbe24..6f2ce2efe4 100644
--- 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/node/database_args.cpp b/src/node/database_args.cpp
new file mode 100644
index 0000000000..2c53b4b47e
--- /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 <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
new file mode 100644
index 0000000000..001976f219
--- /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/dbwrapper_tests.cpp b/src/test/dbwrapper_tests.cpp
index 7ad123754b..1c26acb7f5 100644
--- a/src/test/dbwrapper_tests.cpp
+++ b/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));
diff --git a/src/txdb.cpp b/src/txdb.cpp
index c12b540b9b..697eaa926d 100644
--- a/src/txdb.cpp
+++ b/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) {

From c00fa1a7341d3f47f992e0beb043da655cbca777 Mon Sep 17 00:00:00 2001
From: Ryan Ofsky <ryan@ofsky.org>
Date: Tue, 16 Aug 2022 23:32:55 -0400
Subject: [PATCH 2/4] refactor, txdb: Add CoinsViewOptions struct

Add CoinsViewOptions struct to remove ArgsManager uses from txdb.

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

This commit does not change behavior.
---
 src/Makefile.am              |  2 ++
 src/node/coins_view_args.cpp | 16 ++++++++++++++++
 src/node/coins_view_args.h   | 15 +++++++++++++++
 src/test/coins_tests.cpp     |  4 ++--
 src/txdb.cpp                 | 34 +++++++++++-----------------------
 src/txdb.h                   | 18 ++++++++++++------
 src/validation.cpp           | 21 +++++++++++++--------
 src/validation.h             |  2 +-
 8 files changed, 72 insertions(+), 40 deletions(-)
 create mode 100644 src/node/coins_view_args.cpp
 create mode 100644 src/node/coins_view_args.h

diff --git a/src/Makefile.am b/src/Makefile.am
index 6d5f2eebd3..7b9ffe427d 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -204,6 +204,7 @@ 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 \
@@ -389,6 +390,7 @@ 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 \
diff --git a/src/node/coins_view_args.cpp b/src/node/coins_view_args.cpp
new file mode 100644
index 0000000000..67c9b8dbac
--- /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 <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
new file mode 100644
index 0000000000..71a7a671fd
--- /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/test/coins_tests.cpp b/src/test/coins_tests.cpp
index 55ecd41af1..eb33e0028e 100644
--- 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/txdb.cpp b/src/txdb.cpp
index 697eaa926d..5803bc7b31 100644
--- a/src/txdb.cpp
+++ b/src/txdb.cpp
@@ -71,32 +71,22 @@ struct CoinEntry {
 
 } // namespace
 
-CCoinsViewDB::CCoinsViewDB(fs::path ldb_path, size_t nCacheSize, bool fMemory, bool fWipe) :
-    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) { }
+CCoinsViewDB::CCoinsViewDB(DBParams db_params, CoinsViewOptions options) :
+    m_db_params{std::move(db_params)},
+    m_options{std::move(options)},
+    m_db{std::make_unique<CDBWrapper>(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<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; }()});
+        m_db_params.cache_bytes = new_cache_size;
+        m_db_params.wipe_data = false;
+        m_db = std::make_unique<CDBWrapper>(m_db_params);
     }
 }
 
@@ -127,8 +117,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();
@@ -159,13 +147,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);
                 }
diff --git a/src/txdb.h b/src/txdb.h
index e3422846c0..bfbfab57a4 100644
--- a/src/txdb.h
+++ b/src/txdb.h
@@ -45,18 +45,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<CDBWrapper> 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;
     bool HaveCoin(const COutPoint &outpoint) const override;
diff --git a/src/validation.cpp b/src/validation.cpp
index f0ffb748dd..971eb346a4 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -28,6 +28,8 @@
 #include <node/blockstorage.h>
 #include <node/interface_ui.h>
 #include <node/utxo_snapshot.h>
+#include <node/coins_view_args.h>
+#include <node/database_args.h>
 #include <policy/policy.h>
 #include <policy/rbf.h>
 #include <policy/settings.h>
@@ -1511,13 +1513,9 @@ CAmount GetBlockSubsidy(int nHeight, const Consensus::Params& consensusParams)
     return nSubsidy;
 }
 
-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()
 {
@@ -1546,7 +1544,14 @@ void Chainstate::InitCoinsDB(
     }
 
     m_coins_views = std::make_unique<CoinsViews>(
-        leveldb_name, cache_size_bytes, in_memory, should_wipe);
+        DBParams{
+            .path = gArgs.GetDataDirNet() / leveldb_name,
+            .cache_bytes = cache_size_bytes,
+            .memory_only = in_memory,
+            .wipe_data = should_wipe,
+            .obfuscate = true,
+            .options = [] { DBOptions options; node::ReadDatabaseArgs(gArgs, options); return options; }()},
+        [] { CoinsViewOptions options; node::ReadCoinsViewArgs(gArgs, options); return options; }());
 }
 
 void Chainstate::InitCoinsCache(size_t cache_size_bytes)
diff --git a/src/validation.h b/src/validation.h
index 7170467b00..13a59d32dd 100644
--- a/src/validation.h
+++ b/src/validation.h
@@ -408,7 +408,7 @@ public:
     //! 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 0352258148c51572426666d337c7b28d0033376c Mon Sep 17 00:00:00 2001
From: Ryan Ofsky <ryan@ofsky.org>
Date: Tue, 16 Aug 2022 23:32:55 -0400
Subject: [PATCH 3/4] refactor, txdb: Use DBParams struct in CBlockTreeDB

Use DBParams struct to remove ArgsManager uses from txdb.

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

This commit does not change behavior.
---
 src/node/chainstate.cpp        | 9 ++++++++-
 src/test/util/setup_common.cpp | 5 ++++-
 src/txdb.cpp                   | 9 ---------
 src/txdb.h                     | 3 +--
 4 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/src/node/chainstate.cpp b/src/node/chainstate.cpp
index ba1024d22e..fa521a9e03 100644
--- a/src/node/chainstate.cpp
+++ b/src/node/chainstate.cpp
@@ -10,12 +10,14 @@
 #include <consensus/params.h>
 #include <logging.h>
 #include <node/blockstorage.h>
+#include <node/database_args.h>
 #include <node/caches.h>
 #include <sync.h>
 #include <threadsafety.h>
 #include <tinyformat.h>
 #include <txdb.h>
 #include <uint256.h>
+#include <util/system.h>
 #include <util/time.h>
 #include <util/translation.h>
 #include <validation.h>
@@ -64,7 +66,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 = gArgs.GetDataDirNet() / "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 = [] { DBOptions options; node::ReadDatabaseArgs(gArgs, options); return options; }()});
 
     if (options.reindex) {
         pblocktree->WriteReindexing(true);
diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
index 6e72f69968..2318380c6e 100644
--- a/src/test/util/setup_common.cpp
+++ b/src/test/util/setup_common.cpp
@@ -184,7 +184,10 @@ ChainTestingSetup::ChainTestingSetup(const std::string& chainName, const std::ve
         .check_block_index = true,
     };
     m_node.chainman = std::make_unique<ChainstateManager>(chainman_opts);
-    m_node.chainman->m_blockman.m_block_tree_db = std::make_unique<CBlockTreeDB>(m_cache_sizes.block_tree_db, true);
+    m_node.chainman->m_blockman.m_block_tree_db = std::make_unique<CBlockTreeDB>(DBParams{
+        .path = m_args.GetDataDirNet() / "blocks" / "index",
+        .cache_bytes = static_cast<size_t>(m_cache_sizes.block_tree_db),
+        .memory_only = true});
 
     constexpr int script_check_threads = 2;
     StartScriptCheckWorkerThreads(script_check_threads);
diff --git a/src/txdb.cpp b/src/txdb.cpp
index 5803bc7b31..7257fb4959 100644
--- a/src/txdb.cpp
+++ b/src/txdb.cpp
@@ -6,7 +6,6 @@
 #include <txdb.h>
 
 #include <chain.h>
-#include <node/database_args.h>
 #include <pow.h>
 #include <random.h>
 #include <shutdown.h>
@@ -176,14 +175,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{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) {
     return Read(std::make_pair(DB_BLOCK_FILES, nFile), info);
 }
diff --git a/src/txdb.h b/src/txdb.h
index bfbfab57a4..8a876349fb 100644
--- a/src/txdb.h
+++ b/src/txdb.h
@@ -86,8 +86,7 @@ public:
 class CBlockTreeDB : public CDBWrapper
 {
 public:
-    explicit CBlockTreeDB(size_t nCacheSize, bool fMemory = false, bool fWipe = false);
-
+    using CDBWrapper::CDBWrapper;
     bool WriteBatchSync(const std::vector<std::pair<int, const CBlockFileInfo*> >& fileInfo, int nLastFile, const std::vector<const CBlockIndex*>& blockinfo);
     bool ReadBlockFileInfo(int nFile, CBlockFileInfo &info);
     bool ReadLastBlockFile(int &nFile);

From aadd7c5b9b43a38beaa954b4cb8c2fff55f2200f Mon Sep 17 00:00:00 2001
From: Ryan Ofsky <ryan@ofsky.org>
Date: Tue, 16 Aug 2022 23:32:55 -0400
Subject: [PATCH 4/4] refactor, validation: Add ChainstateManagerOpts db
 options

Use ChainstateManagerOpts struct to remove ArgsManager uses from validation.cpp.

This commit does not change behavior.
---
 src/bitcoin-chainstate.cpp                      | 1 +
 src/init.cpp                                    | 2 ++
 src/kernel/chainstatemanager_opts.h             | 6 ++++++
 src/node/chainstate.cpp                         | 6 ++----
 src/node/chainstatemanager_args.cpp             | 7 +++++++
 src/test/util/setup_common.cpp                  | 1 +
 src/test/validation_chainstatemanager_tests.cpp | 1 +
 src/validation.cpp                              | 8 +++-----
 8 files changed, 23 insertions(+), 9 deletions(-)

diff --git a/src/bitcoin-chainstate.cpp b/src/bitcoin-chainstate.cpp
index d972b71a65..423fa79c6f 100644
--- 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/init.cpp b/src/init.cpp
index 73ae36e4f7..49040c6f63 100644
--- 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
index 226bb6031e..2395f60164 100644
--- 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
index fa521a9e03..4741c4c421 100644
--- a/src/node/chainstate.cpp
+++ b/src/node/chainstate.cpp
@@ -10,14 +10,12 @@
 #include <consensus/params.h>
 #include <logging.h>
 #include <node/blockstorage.h>
-#include <node/database_args.h>
 #include <node/caches.h>
 #include <sync.h>
 #include <threadsafety.h>
 #include <tinyformat.h>
 #include <txdb.h>
 #include <uint256.h>
-#include <util/system.h>
 #include <util/time.h>
 #include <util/translation.h>
 #include <validation.h>
@@ -67,11 +65,11 @@ ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSize
     // fails if it's still open from the previous loop. Close it first:
     pblocktree.reset();
     pblocktree = std::make_unique<CBlockTreeDB>(DBParams{
-        .path = gArgs.GetDataDirNet() / "blocks" / "index",
+        .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 = [] { DBOptions options; node::ReadDatabaseArgs(gArgs, options); return options; }()});
+        .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
index b0d929626b..9801e6e959 100644
--- 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/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
index 2318380c6e..1fc8a02975 100644
--- a/src/test/util/setup_common.cpp
+++ b/src/test/util/setup_common.cpp
@@ -180,6 +180,7 @@ ChainTestingSetup::ChainTestingSetup(const std::string& chainName, const std::ve
 
     const ChainstateManager::Options chainman_opts{
         .chainparams = chainparams,
+        .datadir = m_args.GetDataDirNet(),
         .adjusted_time_callback = GetAdjustedTime,
         .check_block_index = true,
     };
diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp
index 56867a584b..63523d3e2e 100644
--- a/src/test/validation_chainstatemanager_tests.cpp
+++ b/src/test/validation_chainstatemanager_tests.cpp
@@ -374,6 +374,7 @@ struct SnapshotTestSetup : TestChain100Setup {
             BOOST_CHECK_EQUAL(chainman.GetAll().size(), 0);
             const ChainstateManager::Options chainman_opts{
                 .chainparams = ::Params(),
+                .datadir = m_args.GetDataDirNet(),
                 .adjusted_time_callback = GetAdjustedTime,
             };
             // For robustness, ensure the old manager is destroyed before creating a
diff --git a/src/validation.cpp b/src/validation.cpp
index 971eb346a4..c839647b29 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -28,8 +28,6 @@
 #include <node/blockstorage.h>
 #include <node/interface_ui.h>
 #include <node/utxo_snapshot.h>
-#include <node/coins_view_args.h>
-#include <node/database_args.h>
 #include <policy/policy.h>
 #include <policy/rbf.h>
 #include <policy/settings.h>
@@ -1545,13 +1543,13 @@ void Chainstate::InitCoinsDB(
 
     m_coins_views = std::make_unique<CoinsViews>(
         DBParams{
-            .path = gArgs.GetDataDirNet() / leveldb_name,
+            .path = m_chainman.m_options.datadir / leveldb_name,
             .cache_bytes = cache_size_bytes,
             .memory_only = in_memory,
             .wipe_data = should_wipe,
             .obfuscate = true,
-            .options = [] { DBOptions options; node::ReadDatabaseArgs(gArgs, options); return options; }()},
-        [] { CoinsViewOptions options; node::ReadCoinsViewArgs(gArgs, options); return options; }());
+            .options = m_chainman.m_options.coins_db},
+        m_chainman.m_options.coins_view);
 }
 
 void Chainstate::InitCoinsCache(size_t cache_size_bytes)