New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[kernel 3e/n] Decouple CDBWrapper
and its kernel users from ArgsManager
#25623
[kernel 3e/n] Decouple CDBWrapper
and its kernel users from ArgsManager
#25623
Conversation
26c3ad9
to
1ed5723
Compare
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1ed5723 🔶
Show signature
Signature:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
1ed5723bc9c69bf2d7c2a902e44f3fc46b1165a7 🔶
-----BEGIN PGP SIGNATURE-----
iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUhBHwv/bXhMtBcq5mbrytbt6HR1RqodiVJY8Lco4+XaZJ9SvabncUowi665bc9Q
pTlEAdBYb/RXxJhbRzZe9PTmTpqqZVbOtHaTM/TFfrJMhCSvpuCTKd28Fxav5MMV
En+4Na6IjeZ/NdRT6u2QIM4XgC3AUv3o1rsU8Uw+MHbrAcY+1sn8HuELNxcSg1dL
nUrACZIFIzsiOmNctDP4q+C9bJtnYywdV2DgT7hOED42c5A0cWxanVMvMNMVWAy+
X4m8fnMYjXL4+XmNiMGfrswHscNtWI6jdZd6uIL+kjunQFuPA08S3LVEl2UQKYav
a7G8TuH0OUXIJK6j9VFb1nPotMIbj+bgmvWObBS2R2pRpFglR45IgPmSYq+EnKyk
9pJUT6sdhrGj5RLEHF7tdF4scgqAqpilAmn95TfynEx20KAIhYse8PyRILzdTp2X
M/stGVjV9UyggbepiVfQ0TJC//LlwrGRNFlK8XLf7BHUDikjT8Hc9S3q/HeHtzDZ
zDqoqjm9
=XJoH
-----END PGP SIGNATURE-----
src/txdb.cpp
Outdated
.in_memory = m_is_memory, | ||
.wipe_existing = false, | ||
.obfuscate_data = true, | ||
.do_compact = false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2708fcb: Why is this false?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a behavior change that I should probably document in the commit message. But I'd also like to hear if others think this is reasonable:
In my mind, options like wipe_existing
and do_compact
are "oneshot" parameters that should be latched to false
after they've been applied for the first time. So in places like ResizeCache
I think it makes more sense to supply false
for these.
1ed5723
to
ed7ae6a
Compare
dd421fd
to
961c86a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review ACK 961c86a, but I think this would be better if the PR stopped adding ArgsManager::GetArg calls where they don't belong. I don't think there's a good reason to add new calls that will need to be deleted later from validation code when we can just delete them now. I know you aren't really concerned with indexing code, but I don't think it is great to add gArgs usages to indexing code either.
The main culprit here is gArgs.GetBoolArg("-forcecompactdb", false)
calls and these can be eliminated by adding a node::ReadDatabaseArgs
function analogous to the wallet::ReadDatabaseArgs
function. I implemented this in f103f4d (tag) and also removed gArgs accesses from src/index/ code in the process
src/dbwrapper.h
Outdated
@@ -236,6 +237,7 @@ class CDBWrapper | |||
.in_memory = fMemory, | |||
.wipe_existing = fWipe, | |||
.obfuscate_data = obfuscate, | |||
.do_compact = gArgs.GetBoolArg("-forcecompactdb", false), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In commit "CDBWrapper: Pass in -forcecompactdb via ::Options" (d29f09a)
gArgs access doesn't really belong in dbwrapper.h header either
.in_memory = f_memory, | ||
.wipe_existing = f_wipe, | ||
.obfuscate_data = f_obfuscate, | ||
.do_compact = gArgs.GetBoolArg("-forcecompactdb", false), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In commit "Use CDBWrapper::Options ctor for non-test callers" (c5566d4)
Would be good not to add news gArgs calls here and below
src/bitcoin-chainstate.cpp
Outdated
@@ -80,6 +80,10 @@ int main(int argc, char* argv[]) | |||
const ChainstateManager::Options chainman_opts{ | |||
.chainparams = chainparams, | |||
.adjusted_time_callback = static_cast<int64_t (*)()>(GetTime), | |||
.block_tree_db_opts = { | |||
.db_path = gArgs.GetDataDirNet() / "blocks" / "index", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In commit "CBlockTreeDB: Add ::Options, init with BlockManager" (961c86a)
Should probably use abs_datadir
variable, not gArgs
src/txdb.cpp
Outdated
.in_memory = fMemory, | ||
.wipe_existing = fWipe, | ||
.obfuscate_data = true, | ||
.do_compact = gArgs.GetBoolArg("-forcecompactdb", false), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In commit "Use CDBWrapper::Options ctor for non-test callers" (c5566d4)
Moving gArgs.GetBoolArg("-forcecompactdb", false)
call out of cdbwrapper code into txdb code seems like a step backwards or maybe sideways, since txdb code shouldn't be using ArgsManager either
@ryanofsky So I think we only add (net) 2 calls to gArgs, one in It is very difficult to maintain behavior and build-ability between commits without having to add oddly placed references to
in a single commit, which would likely be very hard to review. So we have to have some (admittedly awkward) resting points in these bubbling up chains just so that it's not a huge commit. I do admit that I can do a better job of adding code comments so that it's clear these |
961c86a
to
ccdfad0
Compare
I've thought about it and perhaps it's best to do both This is not merge-able yet since |
ccdfad0
to
76381db
Compare
2e79fb6 validation tests: Use existing {Chainstate,Block}Man (Carl Dong) Pull request description: This is split up because it is needed for two changes: * bitcoin/bitcoin#25781 * bitcoin/bitcoin#25623 ACKs for top commit: adam2k: ACK tested 2e79fb6 aureleoules: ACK 2e79fb6. Tree-SHA512: 2cd6a2fec19545f8ffc77e37ccb793aa6cb5815bb1b5e560c0345af6e0f890fd500ae3297b044d3f6f613b8dd7fd4553f5fc2824013342b9e25af1fe2b624967
Could rebase after ##25815 ? |
60d3ffa
to
06bd11d
Compare
|
The non-Options constructor is kept (it now calls the Options version of the constructor) so that we can fix callsites one-by-one without breaking builds, it will be removed later in this patchset.
Add an ApplyArgsManOptions function that currently just calls the ApplyArgsManOptions for CBlockTreeDB::Options. In a future commit in this patchset, this function will also call the ApplyArgsManOptions for CCoinsViewDB::Options.
Add a CBlockTreeDB::Options member to ChainstateManager::Options, and pass it down from ChainstateManager's ctor to BlockManager's ctor to CBlockTreeDB's new ctor that takes only a CBlockTreeDB::Options. This removes the need to reach into ChainstateManager -> BlockManager -> CBlockTreeDB right after you initialize the ChainstateManager, which was ugly. See node/chainstate.cpp and test/util/setup_common.cpp diffs. Notes: 1. In init.cpp, we needed to move the ChainstateManager initialization into the catch_exceptions block, otherwise exceptions thrown by the CBlockTreeDB ctor previously caught because they were in LoadChainstate would no longer be caught. 2. We also needed to add a data dir member to ChainstateManager::Options so that we can determine the default db_path for CBlockTreeDB if none is specified (See validation.h hunk). It does seem to make sense that ChainstateManager would know about what its data dir is anyway.
Since each BlockManager's m_block_tree_db will now be initialized with the BlockManager ctor, we don't need it to be a unique_ptr any more.
Add a CCoinsViewDB::Options member to ChainstateManager::Options, store it as a member in ChainstateManager's ctor, and pass it down to CChainState's ctor when CSM::{InitializeChainstate,ActivateSnapshot} is called. CChainState's ctor will then pass it down to CCoinsViews' new ctor that only takes a CBlockTreeDB::Options. This removes the need to call InitCoinsDB right after you construct your CChainState to have a working CChainState at all. In fact, CChainState::InitCoinsDB is entirely removed in this commit.
Since each CChainState's CoinsViewDB will now be initialized with the CChainState ctor, we don't need it to be a unique_ptr any more. Question for reviewers: Is deferring its destruction in Shutdown codepaths safe?
Also use scopes for freeing temporary CDBWrappers within tests.
Now that the non-Options constructor is no longer used anywhere, remove it.
Construct ChainstateManager with total_coinstip_cache so that we don't have to reach into it later in node::LoadChainstate to set it. Allows us to remove the CacheSizes parameter from node::LoadChainstate and the member from BasicTestingSetup. Done so in this commit.
06bd11d
to
ce290c4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tend to NACK. I think PR is doing some good things, but it is also doing some bad or questionable things. I think the main goal of the PR (decoupling txdb and dbwrapper from ArgsManager) can be achieved more simply. Would suggest starting from #25862 and building from there. Other changes in this PR if they are worth pursuing do not have to be done at the same time as removing ArgsManager calls.
The parts of the PR I think are good are:
- Removing
gArgs
usages fromtxdb
anddbwrapper
. - Introducing a struct to group
dbwrapper
parameters.
The parts of this PR I think are bad are:
-
Making kernel API more complicated. The change makes
bitcoin-chainstate.cpp
example program harder to understand, and inappropriately exposes low level database settings (paths, obfuscation, cache size settings) to kernel applications that will mostly likely break if applications try to set them. I'm in favor of exposing low-level options, but not at cost of making simple applications harder to write. I also think this type of change should be done in a targeted PR, not as an accidental byproduct of internal refactoring. -
Replacing a single
GetBoolArg("-forcecompactdb")
call in dbwrapper code, with 3 separate calls outside of it. This is layer violation exposing low-level database performance options to higher level application code. It also makes it unnecessarily difficult to add new debug/performance options because it now requires adding multiple struct fields and changing multiple layers of code. -
Defining the overlapping fields in 3 different structs (
DBWrapperOpts
,BlockTreeDBOpts
,CoinsViewDBOpts
) and requiring boilerplate code to convert between struct types. There isn't a real justification for this complexity right now.
I think #25862 is a clean minimal change that does the work needed to make kernel code not depend on ArgsManager. There may be other changes in this PR that are worth keeping, but I don't think there's a reason to bundle them all together.
}, | ||
.data_dir = gArgs.GetDataDirNet(), | ||
.coins_view_db_opts = { | ||
.cache_size = static_cast<size_t>(cache_sizes.coins_db), // FIXME |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FIXME?
🐙 This pull request conflicts with the target branch and needs rebase. Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft". |
Closing in favor of #25862, lets get this done! |
…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
This is part of the
libbitcoinkernel
project: #24303, https://github.com/bitcoin/bitcoin/projects/18This PR is NOT dependent on any other PRs.
This PR introduces
::Options
structs andApplyArgsManOptions
functions forCDBWrapper
and its wrappers and descendants inlibbitcoinkernel
. NamelyCBlockTreeDB
andCCoinsViewDB
.libbitcoinkernel
code can now instantiate these classes by filling in an::Options
struct without referencinggArgs
, and non-libbitcoinkernel
code can useApplyArgsManOptions
with a localArgsManager
.The
::Options
struct andApplyArgsManOptions
duo has been used in many previouslibbitcoinkernel
ArgsManager
decoupling PRs. See: https://github.com/bitcoin/bitcoin/projects/18