Skip to content

Commit 9321df4

Browse files
achow101achow101
committed
Merge bitcoin/bitcoin#25862: refactor, kernel: Remove gArgs accesses from dbwrapper and txdb
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
2 parents f722a9b + aadd7c5 commit 9321df4

21 files changed

+193
-73
lines changed

‎src/Makefile.am‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -204,8 +204,10 @@ BITCOIN_CORE_H = \
204204
node/chainstate.h \
205205
node/chainstatemanager_args.h \
206206
node/coin.h \
207+
node/coins_view_args.h \
207208
node/connection_types.h \
208209
node/context.h \
210+
node/database_args.h \
209211
node/eviction.h \
210212
node/interface_ui.h \
211213
node/mempool_args.h \
@@ -388,8 +390,10 @@ libbitcoin_node_a_SOURCES = \
388390
node/chainstate.cpp \
389391
node/chainstatemanager_args.cpp \
390392
node/coin.cpp \
393+
node/coins_view_args.cpp \
391394
node/connection_types.cpp \
392395
node/context.cpp \
396+
node/database_args.cpp \
393397
node/eviction.cpp \
394398
node/interface_ui.cpp \
395399
node/interfaces.cpp \

‎src/bitcoin-chainstate.cpp‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,7 @@ int main(int argc, char* argv[])
8282
// SETUP: Chainstate
8383
const ChainstateManager::Options chainman_opts{
8484
.chainparams = chainparams,
85+
.datadir = gArgs.GetDataDirNet(),
8586
.adjusted_time_callback = NodeClock::now,
8687
};
8788
ChainstateManager chainman{chainman_opts};

‎src/dbwrapper.cpp‎

Lines changed: 16 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -127,48 +127,48 @@ static leveldb::Options GetOptions(size_t nCacheSize)
127127
return options;
128128
}
129129

130-
CDBWrapper::CDBWrapper(const fs::path& path, size_t nCacheSize, bool fMemory, bool fWipe, bool obfuscate)
131-
: m_name{fs::PathToString(path.stem())}, m_path{path}, m_is_memory{fMemory}
130+
CDBWrapper::CDBWrapper(const DBParams& params)
131+
: m_name{fs::PathToString(params.path.stem())}, m_path{params.path}, m_is_memory{params.memory_only}
132132
{
133133
penv = nullptr;
134134
readoptions.verify_checksums = true;
135135
iteroptions.verify_checksums = true;
136136
iteroptions.fill_cache = false;
137137
syncoptions.sync = true;
138-
options = GetOptions(nCacheSize);
138+
options = GetOptions(params.cache_bytes);
139139
options.create_if_missing = true;
140-
if (fMemory) {
140+
if (params.memory_only) {
141141
penv = leveldb::NewMemEnv(leveldb::Env::Default());
142142
options.env = penv;
143143
} else {
144-
if (fWipe) {
145-
LogPrintf("Wiping LevelDB in %s\n", fs::PathToString(path));
146-
leveldb::Status result = leveldb::DestroyDB(fs::PathToString(path), options);
144+
if (params.wipe_data) {
145+
LogPrintf("Wiping LevelDB in %s\n", fs::PathToString(params.path));
146+
leveldb::Status result = leveldb::DestroyDB(fs::PathToString(params.path), options);
147147
dbwrapper_private::HandleError(result);
148148
}
149-
TryCreateDirectories(path);
150-
LogPrintf("Opening LevelDB in %s\n", fs::PathToString(path));
149+
TryCreateDirectories(params.path);
150+
LogPrintf("Opening LevelDB in %s\n", fs::PathToString(params.path));
151151
}
152152
// PathToString() return value is safe to pass to leveldb open function,
153153
// because on POSIX leveldb passes the byte string directly to ::open(), and
154154
// on Windows it converts from UTF-8 to UTF-16 before calling ::CreateFileW
155155
// (see env_posix.cc and env_windows.cc).
156-
leveldb::Status status = leveldb::DB::Open(options, fs::PathToString(path), &pdb);
156+
leveldb::Status status = leveldb::DB::Open(options, fs::PathToString(params.path), &pdb);
157157
dbwrapper_private::HandleError(status);
158158
LogPrintf("Opened LevelDB successfully\n");
159159

160-
if (gArgs.GetBoolArg("-forcecompactdb", false)) {
161-
LogPrintf("Starting database compaction of %s\n", fs::PathToString(path));
160+
if (params.options.force_compact) {
161+
LogPrintf("Starting database compaction of %s\n", fs::PathToString(params.path));
162162
pdb->CompactRange(nullptr, nullptr);
163-
LogPrintf("Finished database compaction of %s\n", fs::PathToString(path));
163+
LogPrintf("Finished database compaction of %s\n", fs::PathToString(params.path));
164164
}
165165

166166
// The base-case obfuscation key, which is a noop.
167167
obfuscate_key = std::vector<unsigned char>(OBFUSCATE_KEY_NUM_BYTES, '\000');
168168

169169
bool key_exists = Read(OBFUSCATE_KEY_KEY, obfuscate_key);
170170

171-
if (!key_exists && obfuscate && IsEmpty()) {
171+
if (!key_exists && params.obfuscate && IsEmpty()) {
172172
// Initialize non-degenerate obfuscation if it won't upset
173173
// existing, non-obfuscated data.
174174
std::vector<unsigned char> new_key = CreateObfuscateKey();
@@ -177,10 +177,10 @@ CDBWrapper::CDBWrapper(const fs::path& path, size_t nCacheSize, bool fMemory, bo
177177
Write(OBFUSCATE_KEY_KEY, new_key);
178178
obfuscate_key = new_key;
179179

180-
LogPrintf("Wrote new obfuscate key for %s: %s\n", fs::PathToString(path), HexStr(obfuscate_key));
180+
LogPrintf("Wrote new obfuscate key for %s: %s\n", fs::PathToString(params.path), HexStr(obfuscate_key));
181181
}
182182

183-
LogPrintf("Using obfuscation key for %s: %s\n", fs::PathToString(path), HexStr(obfuscate_key));
183+
LogPrintf("Using obfuscation key for %s: %s\n", fs::PathToString(params.path), HexStr(obfuscate_key));
184184
}
185185

186186
CDBWrapper::~CDBWrapper()

‎src/dbwrapper.h‎

Lines changed: 24 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,29 @@ class Env;
3131
static const size_t DBWRAPPER_PREALLOC_KEY_SIZE = 64;
3232
static const size_t DBWRAPPER_PREALLOC_VALUE_SIZE = 1024;
3333

34+
//! User-controlled performance and debug options.
35+
struct DBOptions {
36+
//! Compact database on startup.
37+
bool force_compact = false;
38+
};
39+
40+
//! Application-specific storage settings.
41+
struct DBParams {
42+
//! Location in the filesystem where leveldb data will be stored.
43+
fs::path path;
44+
//! Configures various leveldb cache settings.
45+
size_t cache_bytes;
46+
//! If true, use leveldb's memory environment.
47+
bool memory_only = false;
48+
//! If true, remove all existing data.
49+
bool wipe_data = false;
50+
//! If true, store data obfuscated via simple XOR. If false, XOR with a
51+
//! zero'd byte array.
52+
bool obfuscate = false;
53+
//! Passed-through options.
54+
DBOptions options{};
55+
};
56+
3457
class dbwrapper_error : public std::runtime_error
3558
{
3659
public:
@@ -230,15 +253,7 @@ class CDBWrapper
230253
bool m_is_memory;
231254

232255
public:
233-
/**
234-
* @param[in] path Location in the filesystem where leveldb data will be stored.
235-
* @param[in] nCacheSize Configures various leveldb cache settings.
236-
* @param[in] fMemory If true, use leveldb's memory environment.
237-
* @param[in] fWipe If true, remove all existing data.
238-
* @param[in] obfuscate If true, store data obfuscated via simple XOR. If false, XOR
239-
* with a zero'd byte array.
240-
*/
241-
CDBWrapper(const fs::path& path, size_t nCacheSize, bool fMemory = false, bool fWipe = false, bool obfuscate = false);
256+
CDBWrapper(const DBParams& params);
242257
~CDBWrapper();
243258

244259
CDBWrapper(const CDBWrapper&) = delete;

‎src/index/base.cpp‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
#include <kernel/chain.h>
99
#include <node/blockstorage.h>
1010
#include <node/context.h>
11+
#include <node/database_args.h>
1112
#include <node/interface_ui.h>
1213
#include <shutdown.h>
1314
#include <tinyformat.h>
@@ -48,7 +49,13 @@ CBlockLocator GetLocator(interfaces::Chain& chain, const uint256& block_hash)
4849
}
4950

5051
BaseIndex::DB::DB(const fs::path& path, size_t n_cache_size, bool f_memory, bool f_wipe, bool f_obfuscate) :
51-
CDBWrapper(path, n_cache_size, f_memory, f_wipe, f_obfuscate)
52+
CDBWrapper{DBParams{
53+
.path = path,
54+
.cache_bytes = n_cache_size,
55+
.memory_only = f_memory,
56+
.wipe_data = f_wipe,
57+
.obfuscate = f_obfuscate,
58+
.options = [] { DBOptions options; node::ReadDatabaseArgs(gArgs, options); return options; }()}}
5259
{}
5360

5461
bool BaseIndex::DB::ReadBestBlock(CBlockLocator& locator) const

‎src/init.cpp‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1046,6 +1046,7 @@ bool AppInitParameterInteraction(const ArgsManager& args, bool use_syscall_sandb
10461046
{
10471047
ChainstateManager::Options chainman_opts_dummy{
10481048
.chainparams = chainparams,
1049+
.datadir = args.GetDataDirNet(),
10491050
};
10501051
if (const auto error{ApplyArgsManOptions(args, chainman_opts_dummy)}) {
10511052
return InitError(*error);
@@ -1444,6 +1445,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
14441445
bool fReindexChainState = args.GetBoolArg("-reindex-chainstate", false);
14451446
ChainstateManager::Options chainman_opts{
14461447
.chainparams = chainparams,
1448+
.datadir = args.GetDataDirNet(),
14471449
.adjusted_time_callback = GetAdjustedTime,
14481450
};
14491451
Assert(!ApplyArgsManOptions(args, chainman_opts)); // no error can happen, already checked in AppInitParameterInteraction

‎src/kernel/chainstatemanager_opts.h‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@
66
#define BITCOIN_KERNEL_CHAINSTATEMANAGER_OPTS_H
77

88
#include <arith_uint256.h>
9+
#include <dbwrapper.h>
10+
#include <txdb.h>
911
#include <uint256.h>
1012
#include <util/time.h>
1113

@@ -27,6 +29,7 @@ namespace kernel {
2729
*/
2830
struct ChainstateManagerOpts {
2931
const CChainParams& chainparams;
32+
fs::path datadir;
3033
const std::function<NodeClock::time_point()> adjusted_time_callback{nullptr};
3134
std::optional<bool> check_block_index{};
3235
bool checkpoints_enabled{DEFAULT_CHECKPOINTS_ENABLED};
@@ -36,6 +39,9 @@ struct ChainstateManagerOpts {
3639
std::optional<uint256> assumed_valid_block{};
3740
//! If the tip is older than this, the node is considered to be in initial block download.
3841
std::chrono::seconds max_tip_age{DEFAULT_MAX_TIP_AGE};
42+
DBOptions block_tree_db{};
43+
DBOptions coins_db{};
44+
CoinsViewOptions coins_view{};
3945
};
4046

4147
} // namespace kernel

‎src/node/chainstate.cpp‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,12 @@ ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSize
6464
// new CBlockTreeDB tries to delete the existing file, which
6565
// fails if it's still open from the previous loop. Close it first:
6666
pblocktree.reset();
67-
pblocktree.reset(new CBlockTreeDB(cache_sizes.block_tree_db, options.block_tree_db_in_memory, options.reindex));
67+
pblocktree = std::make_unique<CBlockTreeDB>(DBParams{
68+
.path = chainman.m_options.datadir / "blocks" / "index",
69+
.cache_bytes = static_cast<size_t>(cache_sizes.block_tree_db),
70+
.memory_only = options.block_tree_db_in_memory,
71+
.wipe_data = options.reindex,
72+
.options = chainman.m_options.block_tree_db});
6873

6974
if (options.reindex) {
7075
pblocktree->WriteReindexing(true);

‎src/node/chainstatemanager_args.cpp‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,9 @@
55
#include <node/chainstatemanager_args.h>
66

77
#include <arith_uint256.h>
8+
#include <kernel/chainstatemanager_opts.h>
9+
#include <node/coins_view_args.h>
10+
#include <node/database_args.h>
811
#include <tinyformat.h>
912
#include <uint256.h>
1013
#include <util/strencodings.h>
@@ -34,6 +37,10 @@ std::optional<bilingual_str> ApplyArgsManOptions(const ArgsManager& args, Chains
3437

3538
if (auto value{args.GetIntArg("-maxtipage")}) opts.max_tip_age = std::chrono::seconds{*value};
3639

40+
ReadDatabaseArgs(args, opts.block_tree_db);
41+
ReadDatabaseArgs(args, opts.coins_db);
42+
ReadCoinsViewArgs(args, opts.coins_view);
43+
3744
return std::nullopt;
3845
}
3946
} // namespace node

‎src/node/coins_view_args.cpp‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
// Copyright (c) 2022 The Bitcoin Core developers
2+
// Distributed under the MIT software license, see the accompanying
3+
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
5+
#include <node/coins_view_args.h>
6+
7+
#include <txdb.h>
8+
#include <util/system.h>
9+
10+
namespace node {
11+
void ReadCoinsViewArgs(const ArgsManager& args, CoinsViewOptions& options)
12+
{
13+
if (auto value = args.GetIntArg("-dbbatchsize")) options.batch_write_bytes = *value;
14+
if (auto value = args.GetIntArg("-dbcrashratio")) options.simulate_crash_ratio = *value;
15+
}
16+
} // namespace node

0 commit comments

Comments
 (0)