[kernel 3e/n] Decouple `CDBWrapper` and its kernel users from `ArgsManager`

dongcarl commented at 10:52 pm on July 15, 2022: contributor

This is part of the libbitcoinkernel project: #24303, https://github.com/bitcoin/bitcoin/projects/18

This PR is NOT dependent on any other PRs.

This PR introduces ::Options structs and ApplyArgsManOptions functions for CDBWrapper and its wrappers and descendants in libbitcoinkernel. Namely CBlockTreeDB and CCoinsViewDB.

libbitcoinkernel code can now instantiate these classes by filling in an ::Options struct without referencing gArgs, and non-libbitcoinkernel code can use ApplyArgsManOptions with a local ArgsManager.

The ::Options struct and ApplyArgsManOptions duo has been used in many previous libbitcoinkernel ArgsManager decoupling PRs. See: https://github.com/bitcoin/bitcoin/projects/18

dongcarl force-pushed on Jul 16, 2022

DrahtBot commented at 1:40 am on July 16, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25862 (refactor, kernel: Remove gArgs accesses from dbwrapper and txdb by ryanofsky)
#25786 (refactor: Make adjusted time type safe by MarcoFalke)
#25781 (Remove almost all blockstorage globals by MarcoFalke)
#25704 (refactor: Remove almost all validation option globals by MarcoFalke)
#25665 (refactor: Add util::Result failure values, multiple error and warning messages by ryanofsky)
#24230 (indexes: Stop using node internal types and locking cs_main, improve sync logic by ryanofsky)
#23443 (p2p: Erlay support signaling by naumenkogs)
#22663 (dbwrapper: properly destroy objects in case CDBWrapper ctor throws by Crypt-iQ)

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.

in src/dbwrapper.cpp:128 in 52dcc69b9a outdated

113@@ -114,48 +114,48 @@ static leveldb::Options GetOptions(size_t nCacheSize)
114     return options;

MarcoFalke commented at 11:33 am on July 18, 2022:

first commit: commit message is wrong, you aren’t touching CCoinsViewDB

dongcarl commented at 6:36 pm on July 18, 2022:

Fixed in b37269bedb

in src/txdb.cpp:101 in 2708fcb2ac outdated

 97+                .db_path = m_ldb_path,
 98+                .cache_size = new_cache_size,
 99+                .in_memory = m_is_memory,
100+                .wipe_existing = false,
101+                .obfuscate_data = true,
102+                .do_compact = false,

MarcoFalke commented at 11:37 am on July 18, 2022:

2708fcb2accf4baae6652c7d5d9267550eff0c09: Why is this false?

dongcarl commented at 6:36 pm on July 18, 2022:

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.

in src/bench/checkblock.cpp:8 in 54ccfa7dc0 outdated

7@@ -8,6 +8,7 @@
8 #include <chainparams.h>

MarcoFalke commented at 11:43 am on July 18, 2022:

54ccfa7dc0f26d3fa4390252c78afdd0fe12bbfb: Did you mean to add any file(s) to the iwyu CI?

dongcarl commented at 6:38 pm on July 18, 2022:

Unfortunately not… I think when we get to header pruning there will be a lot of IWYU additions, but it’s too early and make the diffs more noisy.

MarcoFalke commented at 12:19 pm on July 19, 2022:

Done in https://github.com/bitcoin/bitcoin/pull/25645

in src/init.cpp:1448 in 1ed5723bc9 outdated

1443+
1444         const ChainstateManager::Options chainman_opts{
1445             .chainparams = chainparams,
1446             .adjusted_time_callback = GetAdjustedTime,
1447+            .block_tree_db_opts = {
1448+                .db_path = gArgs.GetDataDirNet() / "blocks" / "index",

MarcoFalke commented at 12:04 pm on July 18, 2022:

1ed5723bc9c69bf2d7c2a902e44f3fc46b1165a7: It is highly illegal to use gArgs in init.cpp in new code, when args can be used.

dongcarl commented at 6:37 pm on July 18, 2022:

Fixed in ed7ae6a08d 😅

MarcoFalke approved

MarcoFalke commented at 12:32 pm on July 18, 2022: member

1ed5723bc9c69bf2d7c2a902e44f3fc46b1165a7 🔶

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 31ed5723bc9c69bf2d7c2a902e44f3fc46b1165a7 🔶
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUhBHwv/bXhMtBcq5mbrytbt6HR1RqodiVJY8Lco4+XaZJ9SvabncUowi665bc9Q
 8pTlEAdBYb/RXxJhbRzZe9PTmTpqqZVbOtHaTM/TFfrJMhCSvpuCTKd28Fxav5MMV
 9En+4Na6IjeZ/NdRT6u2QIM4XgC3AUv3o1rsU8Uw+MHbrAcY+1sn8HuELNxcSg1dL
10nUrACZIFIzsiOmNctDP4q+C9bJtnYywdV2DgT7hOED42c5A0cWxanVMvMNMVWAy+
11X4m8fnMYjXL4+XmNiMGfrswHscNtWI6jdZd6uIL+kjunQFuPA08S3LVEl2UQKYav
12a7G8TuH0OUXIJK6j9VFb1nPotMIbj+bgmvWObBS2R2pRpFglR45IgPmSYq+EnKyk
139pJUT6sdhrGj5RLEHF7tdF4scgqAqpilAmn95TfynEx20KAIhYse8PyRILzdTp2X
14M/stGVjV9UyggbepiVfQ0TJC//LlwrGRNFlK8XLf7BHUDikjT8Hc9S3q/HeHtzDZ
15zDqoqjm9
16=XJoH
17-----END PGP SIGNATURE-----

dongcarl force-pushed on Jul 18, 2022

DrahtBot added the label Needs rebase on Jul 19, 2022

dongcarl force-pushed on Jul 20, 2022

DrahtBot removed the label Needs rebase on Jul 20, 2022

DrahtBot added the label Needs rebase on Jul 20, 2022

in src/dbwrapper.h:240 in d29f09adfa outdated

236@@ -236,6 +237,7 @@ class CDBWrapper
237                 .in_memory = fMemory,
238                 .wipe_existing = fWipe,
239                 .obfuscate_data = obfuscate,
240+                .do_compact = gArgs.GetBoolArg("-forcecompactdb", false),

ryanofsky commented at 9:09 pm on July 20, 2022:

In commit “CDBWrapper: Pass in -forcecompactdb via ::Options” (d29f09adfaed0abeff0243509cdd49e4f52ebf9a)

gArgs access doesn’t really belong in dbwrapper.h header either

in src/index/base.cpp:55 in c5566d4899 outdated

52+          .db_path = path,
53+          .cache_size = n_cache_size,
54+          .in_memory = f_memory,
55+          .wipe_existing = f_wipe,
56+          .obfuscate_data = f_obfuscate,
57+          .do_compact = gArgs.GetBoolArg("-forcecompactdb", false),

ryanofsky commented at 9:10 pm on July 20, 2022:

In commit “Use CDBWrapper::Options ctor for non-test callers” (c5566d4899d75d9030901092ecf82c97e277d6c2)

Would be good not to add news gArgs calls here and below

in src/bitcoin-chainstate.cpp:85 in 961c86a113 outdated

79@@ -80,6 +80,10 @@ int main(int argc, char* argv[])
80     const ChainstateManager::Options chainman_opts{
81         .chainparams = chainparams,
82         .adjusted_time_callback = static_cast<int64_t (*)()>(GetTime),
83+        .block_tree_db_opts = {
84+            .db_path = gArgs.GetDataDirNet() / "blocks" / "index",

ryanofsky commented at 9:14 pm on July 20, 2022:

In commit “CBlockTreeDB: Add ::Options, init with BlockManager” (961c86a113875366154175677ca4acc326c49e79)

Should probably use abs_datadir variable, not gArgs

in src/txdb.cpp:81 in c5566d4899 outdated

80+          .db_path = ldb_path,
81+          .cache_size = nCacheSize,
82+          .in_memory = fMemory,
83+          .wipe_existing = fWipe,
84+          .obfuscate_data = true,
85+          .do_compact = gArgs.GetBoolArg("-forcecompactdb", false),

ryanofsky commented at 9:19 pm on July 20, 2022:

In commit “Use CDBWrapper::Options ctor for non-test callers” (c5566d4899d75d9030901092ecf82c97e277d6c2)

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 referenced this in commit 67697da461 on Jul 20, 2022

ryanofsky approved

ryanofsky commented at 9:32 pm on July 20, 2022: contributor

Code review ACK 961c86a113875366154175677ca4acc326c49e79, 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 f103f4df57627cd0634b38c99e0aa7da33aedc37 (tag) and also removed gArgs accesses from src/index/ code in the process

ryanofsky referenced this in commit 3b3a74692f on Jul 20, 2022

ryanofsky referenced this in commit f103f4df57 on Jul 20, 2022

dongcarl commented at 8:34 pm on July 21, 2022: contributor

@ryanofsky So I think we only add (net) 2 calls to gArgs, one in src/index/ and one in src/txdb. I plan to remove the one in src/txdb in the next PR, and add an ApplyArgsManOptions function to this PR so that it’s easier to remove the reference in src/index/ in the future.

It is very difficult to maintain behavior and build-ability between commits without having to add oddly placed references to gArgs here and there (e.g. the one you pointed out in dbwrapper.h), since in order to correctly avoid the oddly placed gArgs references we’d have to bubble up the CDBWrapper::Options struct all the way to the top in a single commit. This would entail bubbling up both:

CDBWrapper::Options through CBlockTreeDB, through BlockManager, through ChainstateManager, to init (currently done in this PR)
CDBWrapper::Options through CCoinsViewDB, through CoinsViews, through InitCoinsDB (or, as I have it in my branch, through CChainState’s constructor), through LoadChainstate, to init (to be done in the next PR)

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 gArgs references are not meant to stay. I’d also welcome discussing alternative strategies that I might not be thinking of right now.

dongcarl force-pushed on Jul 27, 2022

dongcarl commented at 9:13 pm on July 27, 2022: contributor

Push 961c86a113875366154175677ca4acc326c49e79 -> ccdfad0aadc7c6424f9d1fff1ae7242c8eecb542

Add commits that also decouples CCoinsViewDB from ArgsManager
Reorganize commits

I’ve thought about it and perhaps it’s best to do both CBlockTreeDB and CCoinsViewDB at once. This reduces the net addition of gArgs references to just the one instance in BaseIndex::DB::DB(...) and is probably less confusing to reviewers, see Ryan’s review above.

This is not merge-able yet since ActivateSnapshot’s call to std::unique_ptr<CChainState> does not take into account ArgsManager overrides. I will address this in a future push by adding a CCoinsViewDB::Options m_coinsdb_opts member to ChainstateManager.

dongcarl marked this as a draft on Jul 27, 2022

DrahtBot removed the label Needs rebase on Jul 27, 2022

dongcarl force-pushed on Jul 28, 2022

in src/kernel/dbwrapper_options.h:26 in 76381dbc98 outdated

16+    size_t cache_size;
17+    bool in_memory = false;
18+    bool wipe_existing = false;
19+    bool obfuscate_data = false;
20+    bool do_compact = false;
21+};

ryanofsky commented at 10:40 pm on July 29, 2022:

You can see the other change I suggested here #25623#pullrequestreview-1045680511, but I think it simplifies and clarifies code to move the do_compact option to a separate struct, as it’s a low-level performance tweak that is passed directly from the user to the low-level DB code, unlike the other parameters which are under control of the high level application and indexing code.

Doing this simplifies code by avoiding the need to have 3 near-duplicate structures DBWrapperOpts BlockTreeDBOpts CoinsViewDBOpts that require boilerplate copying individual fields between them. Instead you canhave 2 structures, with no duplicate fields, that just pass data directly where it needs to go.

ryanofsky commented at 3:43 pm on August 1, 2022: contributor

re: #25623 (comment)

in order to correctly avoid the oddly placed gArgs references we’d have to bubble up the CDBWrapper::Options struct all the way to the top in a single commit […] which would likely be very hard to review

I think we will never resolve this disagreement, since we keep on having it! But the two of us have pretty different ideas of what makes things hard to review. A commit like this that makes a focused mechanical change seems extremely easy to review. Things that make a PR hard to review are:

Commits that mix different types of changes like refactoring & behavior changes, or style cleanups with refactoring
Commits that are not monotonic improvements, and make some things better, other things worse
Commits that seem unrelated or don’t clearly state motivations
Multiple commits that churn the same code repeatedly and make it hard to see what direction the PR is moving and actually evaluate whether it is an improvement (not just whether it introduces bugs).

I’d rather look at longer individual commits that are conceptually focused than smaller commits that make less sense individually and make it harder to see a PR’s purpose and direction.

This is just some general feedback though. I do think these changes are good and I’m happy to review the code regardless of how changes are split up.

DrahtBot added the label Needs rebase on Aug 3, 2022

in src/node/blockstorage.h:143 in ea713b5e63 outdated

139@@ -140,6 +140,8 @@ class BlockManager
140     std::unordered_map<std::string, PruneLockInfo> m_prune_locks GUARDED_BY(::cs_main);
141 
142 public:
143+    explicit BlockManager(const CBlockTreeDB::Options& block_tree_db_opts);

MarcoFalke commented at 10:57 am on August 3, 2022:

Wouldn’t this be better wrapped into a blockmanopts struct? (Happy to do it myself in another pull if you agree, since I need it elsewhere)

dongcarl commented at 2:18 pm on August 3, 2022:

Yeah agree, didn’t do it here since this is the only opt, but feel free to do it in your pull!

MarcoFalke commented at 5:13 pm on August 4, 2022:

See #25781 :black_cat:

dongcarl commented at 6:35 pm on August 3, 2022: contributor

re: #25623 (comment)

I pretty much agree with everything you said there. I think most times we disagree about whether things are “making things worse” or “a lateral change”. Though I will admit I sometimes prioritize the numerical size of commits instead of the “conceptual size”, which is something I will watch out for in the future.

Thinking more about my past PRs, I’m surprised you even made it through any of the g_chainman deglobalization PRs, perhaps the disdain for mutable globals overweighed the endless lateral move commits I was pushing XD

dongcarl force-pushed on Aug 4, 2022

dongcarl commented at 9:04 pm on August 4, 2022: contributor

Push 76381dbc9889472f76c3122535a98e10a6e4b0fc -> 60d3ffa1180666b39efcfecc8ffecadaadcdf844

This push contains many improvements over the one before

Fixed the ActivateSnapshot problem described here: #25623 (comment)
Added a lot more context and explication in commit messages
Add commit to also make BlockManager::m_block_tree_db a simple object member (before we only made CChainState::m_coins_view a simple object)
Remove *_in_memory members from node::ChainstateLoadOptions as we subsume them into the respective ::Options structs
Initialize ChainstateManager::m_total_coins{tip,db}_cache in ChainstateManager’s constructor, completely eliminating the CacheSizes parameter from LoadChainstate and avoid the ugly “reaching in to set stuff” code.
Merged some commits together so that there are less intermediary codebase states that can confuse reviewers (thanks Ryan!)

dongcarl marked this as ready for review on Aug 4, 2022

dongcarl renamed this:
~~[kernel 3e/n] Decouple `CDBWrapper` and `CBlockTreeDB` from `ArgsManager`~~
[kernel 3e/n] Decouple `CDBWrapper` and its kernel users from `ArgsManager`
on Aug 4, 2022

DrahtBot removed the label Needs rebase on Aug 4, 2022

dongcarl commented at 9:10 pm on August 4, 2022: contributor

Question for reviewers: Is it okay that m_coins_views is being destructed later on in the shutdown process after 9292058ae39a130de5de45166fa7e35153fe1ef2?

dongcarl commented at 11:00 pm on August 4, 2022: contributor

When you pushed a big change and are waiting to see if the CI passes

MarcoFalke referenced this in commit e5d8b65423 on Aug 11, 2022

MarcoFalke commented at 6:53 pm on August 11, 2022: member

Could rebase after #https://github.com/bitcoin/bitcoin/pull/25815 ?

dongcarl force-pushed on Aug 11, 2022

dongcarl commented at 10:46 pm on August 11, 2022: contributor

Push 60d3ffa118 -> 06bd11d505

Rebased over #25815
Updated PR description

DBWrapper: Add constructor using Options struct

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.

cb3ad361d1

DBWrapper: Pass in -forcecompactdb via ::Options e8c8d1d89f

BlockTreeDB: Add yet unused ::Options, ApplyArgsManOptions 5d04d046b4

ChainManOpts: Add CBlockTreeDB::Options member

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.

09488528cb

BlockTreeDB: Construct with {Chain,Block}Manager ctor

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.

422ee31897

Make BlockManager::m_block_tree_db a simple object

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.

a2703121f3

CoinsViewDB: Add yet unused ::Options, ApplyArgsManOptions 055700f36d

CoinsViewDB: Construct with ChainState{,Manager} ctor

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.

0bec058a24

Make CChainState::m_coins_view a simple object

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?

9be1517980

Use CDBWrapper::Options ctor for remaining non-test callers 4d2bb690a5

Use CDBWrapper::Options ctor for test callers

Also use scopes for freeing temporary CDBWrappers within tests.

c50d5ecf77

DBWrapper: Remove non-Options constructor

Now that the non-Options constructor is no longer used anywhere, remove
it.

3b6c0ffca8

ChainManOpts: Add coinstip cache size member

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.

ce290c4132

dongcarl force-pushed on Aug 16, 2022

in src/init.cpp:1416 in ce290c4132

1412+                .cache_size = static_cast<size_t>(cache_sizes.block_tree_db),
1413+                .wipe_existing = do_reindex,
1414+            },
1415+            .data_dir = gArgs.GetDataDirNet(),
1416+            .coins_view_db_opts = {
1417+                .cache_size = static_cast<size_t>(cache_sizes.coins_db), // FIXME

ryanofsky commented at 6:02 am on August 17, 2022:

FIXME?

ryanofsky commented at 3:31 pm on August 17, 2022: contributor

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 from txdb and dbwrapper.
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.

DrahtBot added the label Needs rebase on Aug 22, 2022

DrahtBot commented at 10:38 am on August 22, 2022: contributor

🐙 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”.

dongcarl commented at 6:50 pm on September 26, 2022: contributor

Closing in favor of #25862, lets get this done!

dongcarl closed this on Sep 26, 2022

achow101 referenced this in commit 9321df4487 on Feb 17, 2023

bitcoin locked this on Sep 26, 2023

[kernel 3e/n] Decouple CDBWrapper and its kernel users from ArgsManager #25623

Conflicts

[kernel 3e/n] Decouple `CDBWrapper` and its kernel users from `ArgsManager` #25623