refactor, kernel: Remove gArgs accesses from dbwrapper and txdb

ryanofsky commented at 2:51 pm on August 17, 2022: contributor

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

DrahtBot added the label Needs rebase on Aug 17, 2022

ryanofsky force-pushed on Aug 17, 2022

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

Rebased ba91ffa1ce09f8732cb9dbb0c7eddc3199c81b9d -> 841c3a1205befa4882cc2d2014229df9b27c38a1 (pr/dbargs.1 -> pr/dbargs.2, compare) due to conflicts with #25815 and #25077 Updated 841c3a1205befa4882cc2d2014229df9b27c38a1 -> b86aabc5207af8347307ee66b9a783a1e94e13ba (pr/dbargs.2 -> pr/dbargs.3, compare) to fix some CI compile errors on some platforms Updated b86aabc5207af8347307ee66b9a783a1e94e13ba -> 13d466e247e77ea9f603d0205e484726508d7b99 (pr/dbargs.3 -> pr/dbargs.4, compare) to fix more CI compiler problems https://cirrus-ci.com/task/5035326870650880 Updated 13d466e247e77ea9f603d0205e484726508d7b99 -> 370189fe960924bf254591316ab03971cef98335 (pr/dbargs.4 -> pr/dbargs.5, compare) updating bitcoin-chainstate example

ryanofsky force-pushed on Aug 17, 2022

DrahtBot removed the label Needs rebase on Aug 17, 2022

ryanofsky force-pushed on Aug 17, 2022

DrahtBot commented at 5:38 pm on August 17, 2022: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	TheCharlatan, furszy, achow101
Concept ACK	theStack, hebasto

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26966 (index: blockfilter initial sync speedup, parallelize process by furszy)
#26762 (refactor: Make CCheckQueue RAII-styled by hebasto)
#24230 (indexes: Stop using node internal types and locking cs_main, improve sync logic by ryanofsky)
#24199 (Add cs_main annotation to WriteBatchSync(), drop lock in CDiskBlockIndex by jonatack)
#19463 (Prune locks by luke-jr)

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.

glozow added the label Refactoring on Aug 18, 2022

ryanofsky force-pushed on Aug 18, 2022

DrahtBot added the label Needs rebase on Aug 22, 2022

in src/validation.h:881 in 370189fe96 outdated

877@@ -881,6 +878,7 @@ class ChainstateManager
878      */
879     RecursiveMutex& GetMutex() const LOCK_RETURNED(::cs_main) { return ::cs_main; }
880 
881+    Options m_options;

maflcko commented at 10:49 am on August 22, 2022:

This will make internal code really verbose, forcing stuff like m_options.m_max_tip_age in every place an option is used (there are many places)

ryanofsky commented at 1:33 pm on August 22, 2022:

This will make internal code really verbose, forcing stuff like m_options.m_max_tip_age in every place an option is used (there are many places)

Is this comment made in context of your remove globals PR #25704? Was going to review that today!

IMO, m_options.max_tip_age instead of m_max_tip_age is clarity not verbosity. I think in cases where an option is accessed 1-3 times, this also should be less verbose than the alternative, because the ChainstateManager class doesn’t need to repeat all the individual option definitions and constructor initializations. Even cases where the same option is accessed >3 times, you could add a 1-line accessor method like auto GetMaxTipAge() { return options.m_max_tip_age; } to abbreviate.

But unless it’s obvious people reading code “max tip age” is an external setting, I do think “m_options.max_tip_age” is a clearer way of writing it than m_max_tip_age or GetMaxTipAge(). (Also the options member name should probably be max_tip_age not m_max_tip_age because Options is a open struct not an OOP class with private state.)

maflcko commented at 1:44 pm on August 22, 2022:

Oh, I meant m_chainman.m_options.max_tip_age (when accessed through the chainstate). Though, happy to change.

Maybe split up this change into another pull so that both pulls can work on the same base (maybe even avoiding a conflict) without risking to refactor the same lines of code twice in a short period of time?

ryanofsky commented at 5:38 pm on August 22, 2022:

re: #25862 (review)

Maybe split up this change into another pull so that both pulls can work on the same base (maybe even avoiding a conflict) without risking to refactor the same lines of code twice in a short period of time?

Thanks, makes sense. I posted #25905 with just this change.

maflcko commented at 6:43 am on August 23, 2022:

I wonder if the same should be done to blockman (#25781), but there the symbols are chainstate.m_chainman.m_blockman.m_prune_mode, which would turn into chainstate.m_chainman.m_blockman.m_options.prune_mode. Idk, both seem equally verbose.

ryanofsky commented at 10:14 am on August 23, 2022:

I’d probably opt to group the blockman options into a struct, but I don’t think there is as much benefit because there are only 2 options instead of 6+.

I do see difference between the verbosity of chainstate.m_chainman.m_blockman (which is probably bad) and the verbosity of m_blockman.m_options.prune_mode (which is probably good). The verbosity in the first case is a “message chain” code smell (https://oowisdom.csse.canterbury.ac.nz/index.php/Message_chain_smell, https://en.wikipedia.org/wiki/Law_of_Demeter) indicating chainstate & chainman classes are might be doing too many things are and could be too closely coupled to block manager. The verbosity in the second case is caused by organizing related variables into a struct so they can be distinguished and treated as a logical unit.

I wouldn’t want to hide an indication of poor code quality in one part of code by avoiding a potential improvement to a different part of the code. But I don’t think stakes are very high, so any choice is probably reasonable here.

ryanofsky referenced this in commit 6db2c00adb on Aug 22, 2022

ryanofsky force-pushed on Aug 22, 2022

ryanofsky commented at 5:39 pm on August 22, 2022: contributor

Rebased 370189fe960924bf254591316ab03971cef98335 -> 1c77e2e0dbf18043175fbb9b8632a64b74672678 (pr/dbargs.5 -> pr/dbargs.6, compare) due to conflict with #25786

DrahtBot removed the label Needs rebase on Aug 22, 2022

ryanofsky referenced this in commit 7bc33a88f7 on Aug 24, 2022

maflcko referenced this in commit d36bec9b3b on Aug 25, 2022

DrahtBot added the label Needs rebase on Aug 25, 2022

ryanofsky referenced this in commit f6b9334c80 on Aug 26, 2022

ryanofsky force-pushed on Aug 26, 2022

ryanofsky commented at 5:37 pm on August 26, 2022: contributor

Rebased 1c77e2e0dbf18043175fbb9b8632a64b74672678 -> 4e65b3b2224562c90e2ab16a7bd319bdf19da1a8 (pr/dbargs.6 -> pr/dbargs.7, compare) fixing conflict with #25905

DrahtBot removed the label Needs rebase on Aug 26, 2022

fanquake requested review from theuni on Sep 4, 2022

TheCharlatan approved

TheCharlatan commented at 10:16 am on September 18, 2022: contributor

Code review ACK 4e65b3b2224562c90e2ab16a7bd319bdf19da1a8

There is another usage of gArgs of in validation.cpp, Chainstate::FlushStateToDisk, that can now be replaced with m_chainman.m_options.datadir, but this can be done in a follow up PR.

achow101 commented at 7:57 pm on October 12, 2022: member

cc @jonatack @LarryRuane @stickies-v

in src/bitcoin-chainstate.cpp:86 in 4e65b3b222 outdated

82@@ -83,6 +83,7 @@ int main(int argc, char* argv[])
83     const ChainstateManager::Options chainman_opts{
84         .chainparams = chainparams,
85         .adjusted_time_callback = NodeClock::now,
86+        .datadir = abs_datadir,

maflcko commented at 11:58 am on October 13, 2022:

Why is this correct? Either .datadir is the net_dir, in which case this seems wrong or it isn’t, in which case the other code is wrong.

ryanofsky commented at 2:19 pm on October 14, 2022:

re: #25862 (review)

Why is this correct? Either .datadir is the net_dir, in which case this seems wrong or it isn’t, in which case the other code is wrong.

Good catch. I changed abs_datadir to gArgs.GetDataDirNet() to append the net directory.

in src/init.cpp:1412 in 4e65b3b222 outdated

1408             .adjusted_time_callback = GetAdjustedTime,
1409+            .datadir = args.GetDataDirNet(),
1410         };
1411+        node::ReadDatabaseArgs(args, chainman_opts.block_tree_db);
1412+        node::ReadDatabaseArgs(args, chainman_opts.coins_db);
1413+        node::ReadCoinsViewArgs(args, chainman_opts.coins_view);

maflcko commented at 12:02 pm on October 13, 2022:

I’d say we should remove LOC in init.cpp, not add them. What about moving this into a new function called ApplyArgsManOptions?

Also, no need to re-parse this in the loop? Could move outside like fae180c7fd85ef8a1aeb489691779c215cb9d5a2

ryanofsky commented at 2:16 pm on October 14, 2022:

re: #25862 (review)

I’d say we should remove LOC in init.cpp, not add them. What about moving this into a new function called ApplyArgsManOptions?

Also, no need to re-parse this in the loop? Could move outside like fae180c

Nice suggestions, implemented both

in src/node/chainstate.cpp:44 in 4e65b3b222 outdated

40@@ -41,7 +41,12 @@ ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSize
41     // new CBlockTreeDB tries to delete the existing file, which
42     // fails if it's still open from the previous loop. Close it first:
43     pblocktree.reset();
44-    pblocktree.reset(new CBlockTreeDB(cache_sizes.block_tree_db, options.block_tree_db_in_memory, options.reindex));
45+    pblocktree.reset(new CBlockTreeDB{DBParams{

maflcko commented at 12:04 pm on October 13, 2022:

make_unique while touching this?

ryanofsky commented at 2:17 pm on October 14, 2022:

re: #25862 (review)

make_unique while touching this?

Thanks, done

maflcko commented at 12:14 pm on October 13, 2022: member

I wonder if this can be split into more commits, maybe doing it per-arg or for several args in batches? While lines would be touched repeatedly, this would simplify review and make errors like the datadir and datadir_net confusion more obvious.

concpet ack, haven’t reviewed closely but there may be bugs

DrahtBot added the label Needs rebase on Oct 13, 2022

ryanofsky force-pushed on Oct 14, 2022

ryanofsky commented at 8:07 pm on October 14, 2022: contributor

Rebased 4e65b3b2224562c90e2ab16a7bd319bdf19da1a8 -> ee6058f7a0e448c32a38a024f3d845aef10b4a7a (pr/dbargs.7 -> pr/dbargs.8, compare due to conflict with #25667. Also fixed datadir bug in experimental bitcoin-chainstate datadir test program, and implemented review suggestions to clean up init.cpp

ryanofsky commented at 8:14 pm on October 14, 2022: contributor

I wonder if this can be split into more commits, maybe doing it per-arg or for several args in batches? While lines would be touched repeatedly, this would simplify review and make errors like the datadir and datadir_net confusion more obvious.

Split this into commits. This did require making a few temporary changes in the commits that touch the same lines more than once, but I pointed out temporary changes in commit messages to make them easier to review.

DrahtBot removed the label Needs rebase on Oct 14, 2022

DrahtBot added the label Needs rebase on Oct 26, 2022

ryanofsky force-pushed on Oct 26, 2022

ryanofsky commented at 2:08 pm on October 26, 2022: contributor

Rebased ee6058f7a0e448c32a38a024f3d845aef10b4a7a -> 52625219eb38f39c56f287161e85443ef48135f2 (pr/dbargs.8 -> pr/dbargs.9, compare) due to conflict with #25704

DrahtBot removed the label Needs rebase on Oct 26, 2022

in src/txdb.cpp:197 in 13121a701b outdated

193+CBlockTreeDB::CBlockTreeDB(size_t nCacheSize, bool fMemory, bool fWipe) : CDBWrapper{DBParams{
194+    .path = gArgs.GetDataDirNet() / "blocks" / "index",
195+    .cache_bytes = nCacheSize,
196+    .memory_only = fMemory,
197+    .wipe_data = fWipe,
198+    .obfuscate = false,

theStack commented at 5:49 pm on December 5, 2022:

nit: I think not explicitly setting obfuscate here would be slightly more appropriate for a strict refactor, since it wasn’t set in the original code either? (on master, the default argument for the CDBWrapper ctor was taken, i.e. I’d argue in this PR the default DBParams.obfuscate value should be considered)

ryanofsky commented at 4:38 pm on January 6, 2023:

re: #25862 (review)

Makes sense, dropped this.

theStack commented at 6:12 pm on December 5, 2022: contributor

Concept ACK

hebasto commented at 4:03 pm on December 14, 2022: member

Concept ACK.

ryanofsky force-pushed on Jan 6, 2023

ryanofsky commented at 4:46 pm on January 6, 2023: contributor

Updated 52625219eb38f39c56f287161e85443ef48135f2 -> 4177b1247c77eab8e42ae20da74e63b6e808caa1 (pr/dbargs.9 -> pr/dbargs.10, compare) with suggested change to first commit (no changes overall) Rebased 4177b1247c77eab8e42ae20da74e63b6e808caa1 -> 2367163c0c37bed517587d8d3005102d28c790cc (pr/dbargs.10 -> pr/dbargs.11, compare) due to conflicts with #26251 and #25704

DrahtBot added the label Needs rebase on Jan 16, 2023

janus referenced this in commit a3f7d496e2 on Jan 20, 2023

ryanofsky force-pushed on Jan 20, 2023

DrahtBot removed the label Needs rebase on Jan 21, 2023

in src/test/util/setup_common.cpp:184 in 2367163c0c outdated

179@@ -180,8 +180,8 @@ ChainTestingSetup::ChainTestingSetup(const std::string& chainName, const std::ve
180 
181     const ChainstateManager::Options chainman_opts{
182         .chainparams = chainparams,
183+        .datadir = m_args.GetDataDirNet(),
184         .adjusted_time_callback = GetAdjustedTime,
185-        .check_block_index = true,

furszy commented at 1:47 pm on January 26, 2023:

Not going against this change, but this line removal doesn’t look to be part of a “non-behavior change” commit.

ryanofsky commented at 9:12 pm on February 13, 2023:

re: #25862 (review)

In commit “refactor, validation: Add ChainstateManagerOpts db options” (cde3882cd4d4d1ee5682cab47d05e786cc798eb8)

Not going against this change, but this line removal doesn’t look to be part of a “non-behavior change” commit.

Good catch. It looks like this line got dropped due to a bad rebase. It was not supposed to be deleted.

furszy commented at 1:50 pm on January 26, 2023: member

Light code review ACK 2367163c

Side note, have not being able to find a test that verifies whether the generated dbs are obfuscated or not. Might be good to add one if there isn’t any.

DrahtBot added the label Needs rebase on Jan 30, 2023

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.

2eaeded37f

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.

c00fa1a734

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.

0352258148

refactor, validation: Add ChainstateManagerOpts db options

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

This commit does not change behavior.

aadd7c5b9b

ryanofsky force-pushed on Feb 10, 2023

DrahtBot removed the label Needs rebase on Feb 10, 2023

ryanofsky force-pushed on Feb 13, 2023

ryanofsky commented at 9:40 pm on February 13, 2023: contributor

Thanks for the review!

Updated cde3882cd4d4d1ee5682cab47d05e786cc798eb8 -> aadd7c5b9b43a38beaa954b4cb8c2fff55f2200f (pr/dbargs.12 -> pr/dbargs.13, compare) adding back lost test setup noticed by furszy #25862#pullrequestreview-1271082651

TheCharlatan commented at 8:08 am on February 14, 2023: contributor

Code review ACK aadd7c5b9b43a38beaa954b4cb8c2fff55f2200f

DrahtBot requested review from furszy on Feb 14, 2023

furszy commented at 2:00 pm on February 16, 2023: member

diff ACK aadd7c5b

ryanofsky commented at 7:25 pm on February 17, 2023: contributor

This has 2 code reviews. Not sure if it might be ready for merge, or if it needs another reviewer.

achow101 commented at 9:45 pm on February 17, 2023: member

ACK aadd7c5b9b43a38beaa954b4cb8c2fff55f2200f

achow101 merged this on Feb 17, 2023

achow101 closed this on Feb 17, 2023

sidhujag referenced this in commit afe23d0a1e on Feb 25, 2023

fanquake referenced this in commit b175bdb9b2 on Mar 14, 2023

fanquake referenced this in commit e695d8536e on Mar 16, 2023

fanquake referenced this in commit 369d4c03b7 on Apr 3, 2023

fanquake referenced this in commit c2f2abd0a4 on May 11, 2023

bitcoin locked this on Feb 17, 2024

refactor, kernel: Remove gArgs accesses from dbwrapper and txdb #25862

Reviews

Conflicts