refactor: replace `ArgsManager::cs_args RecursiveMutex` with `Mutex`

w0xlt commented at 3:26 pm on March 5, 2026: contributor

Part of #19303.

Replace ArgsManager::cs_args from RecursiveMutex to Mutex.

The conversion follows the pattern established in prior RecursiveMutex removals (e.g. CAddrMan in #19238, CBlockPolicyEstimator in #22014): extract private lock-held helpers with trailing underscore naming (GetSetting_(), GetArgFlags_(), GetPathArg_()), then replace recursive calls in methods that already hold cs_args with those helpers.

DrahtBot added the label Refactoring on Mar 5, 2026

DrahtBot commented at 3:27 pm on March 5, 2026: 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	l0rinc
Concept ACK	theuni, hebasto
Stale ACK	ajtowns, sedited

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34520 (refactor: Add [[nodiscard]] to functions returning bool+mutable ref by maflcko)
#33540 (argsman, cli: GNU-style command-line option parsing (allows options after non-option arguments) by pablomartin4btc)
#28802 (ArgsManager: support command-specific options by ajtowns)
#16545 (refactor: Implement missing error checking for ArgsManager flags 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.

in src/test/argsman_tests.cpp:223 in 92ac4ad8b2 outdated

216@@ -217,29 +217,37 @@ BOOST_AUTO_TEST_CASE(util_ParseParameters)
217     const char *argv_test[] = {"-ignored", "-a", "-b", "-ccc=argument", "-ccc=multiple", "f", "-d=e"};
218 
219     std::string error;
220-    LOCK(testArgs.cs_args);
221     testArgs.SetupArgs({a, b, ccc, d});
222     BOOST_CHECK(testArgs.ParseParameters(0, argv_test, error));
223-    BOOST_CHECK(testArgs.m_settings.command_line_options.empty() && testArgs.m_settings.ro_config.empty());

theuni commented at 3:38 pm on March 5, 2026:

Seems these could all use LockSettings() instead of having to mess with the internal mutex?

ajtowns commented at 9:51 am on March 6, 2026:

With these changed, I believe you can pretty easily change almost all of the the protected: parts of ArgsManager (including cs_args) to private:, just leaving ReadConfigStream() and ReadConfigString() as protected, test-only, methods for testing the behaviour of ReadConfigFiles(). See https://github.com/ajtowns/bitcoin/commits/202603-argsman-private/

w0xlt commented at 10:45 pm on March 6, 2026:

Done. Added a new commit fdc64114dd22d259503a6ac63846e9f4b803854b for this and you as co-author.

theuni commented at 3:42 pm on March 5, 2026: member

Nice. Concept ACK.

w0xlt force-pushed on Mar 5, 2026

in src/common/args.cpp:316 in 59b6cc38b7

314             path = "";
315             return path;
316         }
317     } else {
318-        path = GetDataDirBase();
319+        path = GetDataDir(false);

ajtowns commented at 7:52 am on March 6, 2026:

GetDataDir(/*net_specific=*/false) seems like it would be a win here.

w0xlt commented at 10:44 pm on March 6, 2026:

Done. Thanks.

ajtowns commented at 9:52 am on March 6, 2026: contributor

ACK 2a7f53bb9149eb883c174681add60bf4649ca95a

DrahtBot requested review from theuni on Mar 6, 2026

hebasto commented at 10:04 am on March 6, 2026: member

Concept ACK.

sedited approved

sedited commented at 12:12 pm on March 6, 2026: contributor

ACK 2a7f53bb9149eb883c174681add60bf4649ca95a

ajtown’s suggestions for making these methods private seems nice too.

DrahtBot requested review from hebasto on Mar 6, 2026

in src/common/args.h:438 in 2c06582451 outdated

434@@ -435,13 +435,18 @@ class ArgsManager
435     void LogArgs() const;
436 
437 private:
438+    // Lock-free internal helpers, for use by callers that already hold cs_args.

l0rinc commented at 3:57 pm on March 6, 2026:

2c06582 args: extract lock-requiring internal helpers:

Lock-free is a bit confusing in this context. They require explicit locking.

0    // Internal helpers, for use by callers that already hold `cs_args`.

w0xlt commented at 10:45 pm on March 6, 2026:

Done. Thanks.

in src/common/args.cpp:287 in 2c06582451 outdated

285-    if (IsArgNegated(arg)) return fs::path{};
286-    std::string path_str = GetArg(arg, "");
287+    AssertLockHeld(cs_args);
288+    const auto value = GetSetting_(arg);
289+    if (value.isFalse()) return {};
290+    std::string path_str = SettingToString(value).value_or("");

l0rinc commented at 4:12 pm on March 6, 2026:

2c06582 args: extract lock-requiring internal helpers:

nit:

0    std::string path_str = SettingToString(value, "");

w0xlt commented at 4:27 am on March 7, 2026:

Done. Thanks.

in src/common/args.h:460 in 2c06582451 outdated

459@@ -455,7 +460,7 @@ class ArgsManager
460     void logArgsPrefix(

l0rinc commented at 4:30 pm on March 6, 2026:

2c06582 args: extract lock-requiring internal helpers:

The annotation makes sense, but shouldn’t we also add AssertLockHeld(cs_args); in the implementation?

w0xlt commented at 10:46 pm on March 6, 2026:

Done. Thanks.

in src/common/args.cpp:326 in 2c06582451 outdated

321@@ -308,15 +322,18 @@ fs::path ArgsManager::GetBlocksDirPath() const
322     return path;
323 }
324 
325+fs::path ArgsManager::GetDataDirBase() const { LOCK(cs_args); return GetDataDir(false); }
326+fs::path ArgsManager::GetDataDirNet() const { LOCK(cs_args); return GetDataDir(true); }

l0rinc commented at 4:36 pm on March 6, 2026:

2c06582 args: extract lock-requiring internal helpers:

We don’t usually add multiple commands on the same line, especially when it’s a move + change. But if we do, we could use WITH_LOCK instead - and I’d do this change in a separate commit:

0fs::path ArgsManager::GetDataDirBase() const { return WITH_LOCK(cs_args, return GetDataDir(false)); }
1fs::path ArgsManager::GetDataDirNet() const { return WITH_LOCK(cs_args, return GetDataDir(true)); }

w0xlt commented at 10:46 pm on March 6, 2026:

Done. Thanks.

ajtowns commented at 5:58 am on March 7, 2026:

Not even a nit, but WITH_LOCK seems strictly worse here to me fwiw – more typing (double return) and more internal complexity (an implicit lambda) for no benefit.

l0rinc commented at 9:00 am on March 7, 2026:

I’m fine with multi-line as well, just don’t move methods in a non-trivial commit, and also change the methods and add multiple commands on the same line.

w0xlt force-pushed on Mar 6, 2026

DrahtBot added the label CI failed on Mar 6, 2026

w0xlt force-pushed on Mar 6, 2026

l0rinc commented at 8:33 pm on March 6, 2026: contributor

I have started reviewing it, let me push the comments I have so far.

args: extract lock-requiring internal helpers

Extract GetSetting_(), GetArgFlags_(), and GetPathArg_() as private
lock-held helpers with EXCLUSIVE_LOCKS_REQUIRED(cs_args) annotations.
Public methods delegate to these after acquiring the lock.

Annotate GetDataDir() with EXCLUSIVE_LOCKS_REQUIRED(cs_args) and move
its lock acquisition into GetDataDirBase()/GetDataDirNet().

Annotate logArgsPrefix() with EXCLUSIVE_LOCKS_REQUIRED(cs_args).

This is a pure refactoring with no behavior change, preparing for
the conversion of cs_args from RecursiveMutex to Mutex.

e4e96915db

args: eliminate all recursive locking of cs_args

In methods that already hold cs_args, replace public method calls with
their lock-held private counterparts:

- ParseParameters: GetArgFlags() -> GetArgFlags_()
- GetBlocksDirPath: IsArgSet()/GetPathArg()/GetDataDirBase() ->
  GetSetting_()/GetPathArg_()/GetDataDir()
- ReadSettingsFile: GetArgFlags() -> GetArgFlags_()
- SoftSetArg: IsArgSet()/ForceSetArg() -> GetSetting_()/direct write
- CheckMultipleCLIArgs: IsArgSet() -> GetSetting_()
- logArgsPrefix: GetArgFlags() -> GetArgFlags_()
- ReadConfigStream: GetArgFlags() -> GetArgFlags_()
- ReadConfigFiles: GetPathArg() -> GetPathArg_()

No behavior change. This eliminates all recursive lock acquisitions,
preparing for the conversion of cs_args from RecursiveMutex to Mutex.

e43b202ff9

test: scope cs_args locks to avoid recursive locking

Restructure argsman tests to use scoped LOCK(cs_args) blocks only
around direct protected member access (m_settings, m_network), keeping
public method calls outside lock scopes. This avoids recursive lock
acquisitions that would deadlock with a non-recursive Mutex.

- util_ParseParameters: scope locks around m_settings checks
- util_GetBoolArg: scope lock around m_settings size check
- util_ReadConfigStream: scope lock around m_settings checks
- util_GetArg: scope lock around m_settings writes
- util_ArgsMerge: use SelectConfigNetwork() instead of m_network
- util_ChainMerge: remove unnecessary lock

bfc7468ac0

args: replace cs_args RecursiveMutex with Mutex

Replace the RecursiveMutex with a plain Mutex now that all recursive
lock acquisitions have been eliminated in the preceding commits.

Add EXCLUSIVE_LOCKS_REQUIRED(!cs_args) negative capability annotations
to all public and protected methods that acquire cs_args, following the
pattern established in prior RecursiveMutex conversions (e.g. CAddrMan,
CBlockPolicyEstimator).

2d14a708d9

args: make most ArgsManager members private

Move the first `protected` block (struct Arg, cs_args, m_settings, and
all other member variables) to `private`. Only `ReadConfigStream` and
`ReadConfigString` remain `protected` for test access.

Changes:
- Move `ReadConfigString` from `TestArgsManager` into `ArgsManager`
  itself (declared in args.h, defined in config.cpp) so tests no longer
  need direct access to `cs_args` or `m_settings` for config parsing.
- Replace test-only `SetNetworkOnlyArg` helper with the existing
  `NETWORK_ONLY` flag passed through `SetupArgs`/`AddArg`.
- Remove `TestArgsManager` constructor that cleared
  `m_network_only_args`.
- Remove `using` declarations for `cs_args`, `m_settings`, `GetSetting`,
  and `GetSettingsList` from `TestArgsManager`.
- Clear `m_config_sections` in `ClearArgs()`.

Co-authored-by: Anthony Towns <aj@erisian.com.au>

fdc64114dd

w0xlt force-pushed on Mar 6, 2026

DrahtBot removed the label CI failed on Mar 6, 2026

in src/common/args.cpp:576 in e43b202ff9

571@@ -572,8 +572,8 @@ INSTANTIATE_INT_TYPE(uint64_t);
572 bool ArgsManager::SoftSetArg(const std::string& strArg, const std::string& strValue)
573 {
574     LOCK(cs_args);
575-    if (IsArgSet(strArg)) return false;
576-    ForceSetArg(strArg, strValue);
577+    if (!GetSetting_(strArg).isNull()) return false;
578+    m_settings.forced_settings[SettingName(strArg)] = strValue;

l0rinc commented at 1:27 pm on March 8, 2026:

e43b202 args: eliminate all recursive locking of cs_args:

Nit: how come we’re not creating a ForceSetArg_ here? It’s small so I’m fine with inlining, too.

in src/common/config.cpp:129 in e43b202ff9

124@@ -125,7 +125,8 @@ bool ArgsManager::ReadConfigFiles(std::string& error, bool ignore_invalid_keys)
125         LOCK(cs_args);
126         m_settings.ro_config.clear();
127         m_config_sections.clear();
128-        m_config_path = AbsPathForConfigVal(*this, GetPathArg("-conf", BITCOIN_CONF_FILENAME), /*net_specific=*/false);
129+        const auto conf_val = GetPathArg_("-conf", BITCOIN_CONF_FILENAME);
130+        m_config_path = (conf_val.is_absolute() || conf_val.empty()) ? conf_val : fsbridge::AbsPathJoin(GetDataDir(false), conf_val);

l0rinc commented at 1:32 pm on March 8, 2026:

e43b202 args: eliminate all recursive locking of cs_args:

would it be better to keep using a AbsPathForConfigVal_ abstraction here instead of reimplementing that logic inline? If not, can we unify it with other DataDir calls?

0        m_config_path = (conf_val.is_absolute() || conf_val.empty()) ? conf_val : fsbridge::AbsPathJoin(GetDataDir(/*net_specific=*/false), conf_val);

in src/test/argsman_tests.cpp:81 in bfc7468ac0

77@@ -78,7 +78,6 @@ struct TestArgsManager : public ArgsManager
78     using ArgsManager::GetSettingsList;
79     using ArgsManager::ReadConfigStream;
80     using ArgsManager::cs_args;
81-    using ArgsManager::m_network;
82     using ArgsManager::m_settings;

l0rinc commented at 2:34 pm on March 8, 2026:

bfc7468 test: scope cs_args locks to avoid recursive locking:

Seems like the other two are also unnecessary now

 0diff --git a/src/test/argsman_tests.cpp b/src/test/argsman_tests.cpp
 1--- a/src/test/argsman_tests.cpp	(revision 92583fb1e50592bb924422c6b01caa06760e0a0e)
 2+++ b/src/test/argsman_tests.cpp	(date 1772981835870)
 3@@ -77,8 +77,6 @@
 4     using ArgsManager::GetSetting;
 5     using ArgsManager::GetSettingsList;
 6     using ArgsManager::ReadConfigStream;
 7-    using ArgsManager::cs_args;
 8-    using ArgsManager::m_settings;
 9 };
10 
11 //! Test GetSetting and GetArg type coercion, negation, and default value handling.

in src/test/argsman_tests.cpp:1034 in bfc7468ac0

l0rinc commented at 2:55 pm on March 8, 2026:

bfc7468 test: scope cs_args locks to avoid recursive locking:

nit: we could unify the naming with the new LockSettings additions:

0    args1.LockSettings([&](common::Settings& s) { s.rw_settings["name"] = "value"; });

l0rinc commented at 2:56 pm on March 8, 2026:

bfc7468 test: scope cs_args locks to avoid recursive locking:

0    args2.LockSettings([&](common::Settings& s) { BOOST_CHECK_EQUAL(s.rw_settings["name"].get_str(), "value"); });

l0rinc commented at 3:31 pm on March 8, 2026: contributor

Tested ACK fdc64114dd22d259503a6ac63846e9f4b803854b

Went through this series commit by commit and recreated the changes locally, comparing them with a few alternative approaches along the way. Rebased locally and ran all tests. I left some non-blocking comments and nits in the review, but overall LGTM. Happy to re-ACK if you decide to apply the suggestions.

DrahtBot requested review from sedited on Mar 8, 2026

DrahtBot requested review from ajtowns on Mar 8, 2026

l0rinc approved

refactor: replace ArgsManager::cs_args RecursiveMutex with Mutex #34745

Reviews

Conflicts

refactor: replace `ArgsManager::cs_args RecursiveMutex` with `Mutex` #34745