refactor: Split ArgsManager out of util/system #24455

By splitting ArgsManager and related functions out of the grab-bag util/system file, this enables more specific includes, ~referencing ArgsManager without pulling in gArgs~, and eliminate a circular dependency between chainparamsbase and utils/system.

The proposed new modules are:

• util/args_manager - ArgsManager and related functions
• util/args - gArgs only - separate to support the goal of limiting the scope of gArgs
• util/system - a variety of file system and sytem-related functions

Other notable points:

• I find it a bit concerning that ArgsManager::ReadConfigFiles calls ClearPathCache and CheckDataDirOption on gArgs rather than the current ArgsManager instance in the status quo.

The new files are nearly unchanged from their sources, so git diff master --color-moved=dimmed-zebra could be helpful.

Concept ACK!

In fact I did almost the exact same thing here 😆: https://github.com/dongcarl/bitcoin/commit/81d1857599b51e82cc63ae734b7834085661cf99

Happy to use your changes though if you're planning on proactively rebasing over master when merge conflicts arise!

One thing: the only difference I can spot between your changes and mine is that you didn't move BITCOIN_CONF_FILENAME and BITCOIN_SETTINGS_FILENAME out of util/system.h, I think doing that makes the split even better? Lmk!

This is pretty reasonable. I know we don't generally like to move code around, but this move was probably inevitable.

One suggestion would be to rename "util/args_manager" to "util/args". I think "_manager" suffix does not contribute or clarify much, and will make it awkward to add other argument-handling functions and classes to the same file without attaching them to the ArgsManager class. ArgsManager class is already monolithic and it should be possible to break up or extend without putting everything inside.

Related to this, I don't necessarily see a need to put the gArgs variable in its own file, but if this is needed to get rid of circular dependencies, or if it just seems like a good idea, I'd suggest putting it in "util/gargs" or "util/args_global" instead of "util/args"

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #24922 (Isolate the storage abstraction layer from the application/serialization layer by TheQuantumPhysicist)
• #24915 (lint: Convert lint-circular-dependencies.sh to Python by Smlep)
• #24914 (wallet: Load database records in a particular order by achow101)
• #24845 (wallet: createTransaction, return proper error description for "too-long-mempool-chain" + introduce generic Result classes by furszy)
• #24831 (tidy: add include-what-you-use by fanquake)
• #24830 (init: Allow -proxy="" setting values by ryanofsky)
• #24812 (util/check: Add CHECK_NONFATAL identity function and NONFATAL_UNREACHABLE macro by aureleoules)
• #24764 (Modernize util/strencodings and util/string: string_view and optional by sipa)
• #24757 (build, ci: add DEBUG_LOCKCONTENTION to --enable-debug and CI by jonatack)
• #24742 ([POC] build: prune Boost headers in depends by fanquake)
• #24676 ([WIP] [kernelheaders 1/n] Cleave LevelDB headers from our header tree by dongcarl)
• #24675 (util: Use ArgsManager::GetPathArg more widely by hebasto)
• #24479 (Bugfix: util: Correctly handle Number value types in GetArg/GetBoolArg by luke-jr)
• #24232 (assumeutxo: add init and completion logic by jamesob)
• #24230 (indexes: Stop using node internal types and locking cs_main, improve sync logic by ryanofsky)
• #23561 (BIP324: Handshake prerequisites by dhruv)
• #23444 (fuzz: Add regression test for wallet crash by MarcoFalke)
• #23233 (BIP324: Add encrypted p2p transport {de}serializer by dhruv)
• #22953 (refactor: introduce single-separator split helper (boost::split replacement) by theStack)
• #20205 (wallet: Properly support a wallet id by achow101)
• #19792 (rpc: Add dumpcoinstats by fjahr)
• #17786 (refactor: Nuke policy/fees->mempool circular dependencies by hebasto)
• #16673 (Relog configuration args on debug.log rotation by LarryRuane)
• #15936 (Unify bitcoin-qt and bitcoind persistent settings by ryanofsky)

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.

@ryanofsky thanks, I consolidated args_manager into args - I agree the cost in complexity weighs against the minor benefit of limiting gArgs access.

In fact I did almost the exact same thing here 😆: https://github.com/dongcarl/bitcoin/commit/81d1857599b51e82cc63ae734b7834085661cf99 @dongcarl Nice! I'm open to rebasing this, and I think it'd be positive to evaluate it independently. I could go either way on the filenames - with this args is the larger file, and system includes a variety of filesystem-related functionality. 🤷

Concept ACK, nice work

In commit 066ba007bed96552cfdc6a6ed91199ddc1cace8c (refactor: Split ArgsManager out of util/system), I wonder whether it would be good to introduce a constant for -avoidpartialspends to reduce a risk of a typo.

Also this PR feels big, would it be reasonable to split it into multiple PRs? I mean having a PR for 066ba007bed96552cfdc6a6ed91199ddc1cace8c is quite easy to review and it can get code review ACKs almost immediately imho. Plus one would avoid the need for many rebases which this big PR risks. I can probably give a hand with it if you need/want.

@kiminuo thanks, re "this PR feels big" I think you make a good point, I'll split out a prep work PR. As for the risk of typos, that hasn't been a practice elsewhere in the codebase, but I have an idea or two of an alternative that could apply across the board.

See #24811 for the setup commits of this PR.

This PR impacting the need for util/system includes led me to audit those, and the necessity of them. I could perhaps reorder the commits or do a follow up to shrink it down further.

<!--cf906140f33d8803c4a75a2196329ecb-->

🐙 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".}

@Empact I'm wondering if you'd be open to splitting off the last two commits off to their own PR (or just this PR) not based on #24811?

I think "removing references to gArgs and header tree cleanup" (which is what #24811 is doing) probably stands on its own merit and can be done either before or after the "split util/args and util/system" work here.

Selfishly, I also want to get the last 2 commits reviewed and merged ASAP since it'll soon become a blocker for libbitcoinkernel work.

@Empact I'm wondering if you'd be open to splitting off the last two commits off to their own PR (or just this PR) not based on #24811?

Sure, I'll be happy to try that, but I think some of the prior work is necessary to enable the separation. I'll let you know what I find.

Closing in favor of my next attempt: https://github.com/bitcoin/bitcoin/pull/25152