From 7d61e03c701215e2956353195dff4734210a2765 Mon Sep 17 00:00:00 2001 From: w0xlt <94266259+w0xlt@users.noreply.github.com> Date: Wed, 4 Mar 2026 11:58:12 -0800 Subject: [PATCH 1/5] 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. --- src/common/args.cpp | 52 ++++++++++++++++++++++++++++++++++++--------- src/common/args.h | 13 ++++++++---- 2 files changed, 51 insertions(+), 14 deletions(-) diff --git a/src/common/args.cpp b/src/common/args.cpp index 5c8589cf440..a460c0bdfc0 100644 --- a/src/common/args.cpp +++ b/src/common/args.cpp @@ -255,9 +255,9 @@ bool ArgsManager::ParseParameters(int argc, const char* const argv[], std::strin return true; } -std::optional ArgsManager::GetArgFlags(const std::string& name) const +std::optional ArgsManager::GetArgFlags_(const std::string& name) const { - LOCK(cs_args); + AssertLockHeld(cs_args); for (const auto& arg_map : m_available_args) { const auto search = arg_map.second.find(name); if (search != arg_map.second.end()) { @@ -267,22 +267,36 @@ std::optional ArgsManager::GetArgFlags(const std::string& name) co return m_default_flags; } +std::optional ArgsManager::GetArgFlags(const std::string& name) const +{ + LOCK(cs_args); + return GetArgFlags_(name); +} + void ArgsManager::SetDefaultFlags(std::optional flags) { LOCK(cs_args); m_default_flags = flags; } -fs::path ArgsManager::GetPathArg(std::string arg, const fs::path& default_value) const +fs::path ArgsManager::GetPathArg_(std::string arg, const fs::path& default_value) const { - if (IsArgNegated(arg)) return fs::path{}; - std::string path_str = GetArg(arg, ""); + AssertLockHeld(cs_args); + const auto value = GetSetting_(arg); + if (value.isFalse()) return {}; + std::string path_str = SettingToString(value, ""); if (path_str.empty()) return default_value; fs::path result = fs::PathFromString(path_str).lexically_normal(); // Remove trailing slash, if present. return result.has_filename() ? result : result.parent_path(); } +fs::path ArgsManager::GetPathArg(std::string arg, const fs::path& default_value) const +{ + LOCK(cs_args); + return GetPathArg_(std::move(arg), default_value); +} + fs::path ArgsManager::GetBlocksDirPath() const { LOCK(cs_args); @@ -308,15 +322,25 @@ fs::path ArgsManager::GetBlocksDirPath() const return path; } +fs::path ArgsManager::GetDataDirBase() const { + LOCK(cs_args); + return GetDataDir(/*net_specific=*/false); +} + +fs::path ArgsManager::GetDataDirNet() const { + LOCK(cs_args); + return GetDataDir(/*net_specific=*/true); +} + fs::path ArgsManager::GetDataDir(bool net_specific) const { - LOCK(cs_args); + AssertLockHeld(cs_args); fs::path& path = net_specific ? m_cached_network_datadir_path : m_cached_datadir_path; // Used cached path if available if (!path.empty()) return path; - const fs::path datadir{GetPathArg("-datadir")}; + const fs::path datadir{GetPathArg_("-datadir")}; if (!datadir.empty()) { path = fs::absolute(datadir); if (!fs::is_directory(path)) { @@ -853,15 +877,22 @@ std::variant ArgsManager::GetChainArg() const bool ArgsManager::UseDefaultSection(const std::string& arg) const { + AssertLockHeld(cs_args); return m_network == ChainTypeToString(ChainType::MAIN) || !m_network_only_args.contains(arg); } +common::SettingsValue ArgsManager::GetSetting_(const std::string& arg) const +{ + AssertLockHeld(cs_args); + return common::GetSetting( + m_settings, m_network, SettingName(arg), !UseDefaultSection(arg), + /*ignore_nonpersistent=*/false, /*get_chain_type=*/false); +} + common::SettingsValue ArgsManager::GetSetting(const std::string& arg) const { LOCK(cs_args); - return common::GetSetting( - m_settings, m_network, SettingName(arg), !UseDefaultSection(arg), - /*ignore_nonpersistent=*/false, /*get_chain_type=*/false); + return GetSetting_(arg); } std::vector ArgsManager::GetSettingsList(const std::string& arg) const @@ -875,6 +906,7 @@ void ArgsManager::logArgsPrefix( const std::string& section, const std::map>& args) const { + AssertLockHeld(cs_args); std::string section_str = section.empty() ? "" : "[" + section + "] "; for (const auto& arg : args) { for (const auto& value : arg.second) { diff --git a/src/common/args.h b/src/common/args.h index ea4e173bf72..0bcf32d1f88 100644 --- a/src/common/args.h +++ b/src/common/args.h @@ -229,14 +229,14 @@ protected: * * @return Absolute path on success, otherwise an empty path when a non-directory path would be returned */ - fs::path GetDataDirBase() const { return GetDataDir(false); } + fs::path GetDataDirBase() const; /** * Get data directory path with appended network identifier * * @return Absolute path on success, otherwise an empty path when a non-directory path would be returned */ - fs::path GetDataDirNet() const { return GetDataDir(true); } + fs::path GetDataDirNet() const; /** * Clear cached directory paths @@ -435,13 +435,18 @@ protected: void LogArgs() const; private: + // Internal helpers, for use by callers that already hold `cs_args`. + common::SettingsValue GetSetting_(const std::string& arg) const EXCLUSIVE_LOCKS_REQUIRED(cs_args); + std::optional GetArgFlags_(const std::string& name) const EXCLUSIVE_LOCKS_REQUIRED(cs_args); + fs::path GetPathArg_(std::string arg, const fs::path& default_value = {}) const EXCLUSIVE_LOCKS_REQUIRED(cs_args); + /** * Get data directory path * * @param net_specific Append network identifier to the returned path * @return Absolute path on success, otherwise an empty path when a non-directory path would be returned */ - fs::path GetDataDir(bool net_specific) const; + fs::path GetDataDir(bool net_specific) const EXCLUSIVE_LOCKS_REQUIRED(cs_args); /** * Return -regtest/-signet/-testnet/-testnet4/-chain= setting as a ChainType enum if a @@ -455,7 +460,7 @@ private: void logArgsPrefix( const std::string& prefix, const std::string& section, - const std::map>& args) const; + const std::map>& args) const EXCLUSIVE_LOCKS_REQUIRED(cs_args); }; extern ArgsManager gArgs; From 70b51fef7afd705d109718447097935e49cc0c25 Mon Sep 17 00:00:00 2001 From: w0xlt <94266259+w0xlt@users.noreply.github.com> Date: Wed, 4 Mar 2026 12:05:43 -0800 Subject: [PATCH 2/5] 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. --- src/common/args.cpp | 20 ++++++++++---------- src/common/config.cpp | 5 +++-- 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/src/common/args.cpp b/src/common/args.cpp index a460c0bdfc0..be8c513339c 100644 --- a/src/common/args.cpp +++ b/src/common/args.cpp @@ -206,7 +206,7 @@ bool ArgsManager::ParseParameters(int argc, const char* const argv[], std::strin if (key[0] != '-') { if (!m_accept_any_command && m_command.empty()) { // The first non-dash arg is a registered command - std::optional flags = GetArgFlags(key); + std::optional flags = GetArgFlags_(key); if (!flags || !(*flags & ArgsManager::COMMAND)) { error = strprintf("Invalid command '%s'", argv[i]); return false; @@ -227,7 +227,7 @@ bool ArgsManager::ParseParameters(int argc, const char* const argv[], std::strin // Transform -foo to foo key.erase(0, 1); KeyInfo keyinfo = InterpretKey(key); - std::optional flags = GetArgFlags('-' + keyinfo.name); + std::optional flags = GetArgFlags_('-' + keyinfo.name); // Unknown command line options and command line options with dot // characters (which are returned from InterpretKey with nonempty @@ -306,14 +306,14 @@ fs::path ArgsManager::GetBlocksDirPath() const // this function if (!path.empty()) return path; - if (IsArgSet("-blocksdir")) { - path = fs::absolute(GetPathArg("-blocksdir")); + if (!GetSetting_("-blocksdir").isNull()) { + path = fs::absolute(GetPathArg_("-blocksdir")); if (!fs::is_directory(path)) { path = ""; return path; } } else { - path = GetDataDirBase(); + path = GetDataDir(/*net_specific=*/false); } path /= fs::PathFromString(BaseParams().DataDir()); @@ -443,7 +443,7 @@ bool ArgsManager::ReadSettingsFile(std::vector* errors) } for (const auto& setting : m_settings.rw_settings) { KeyInfo key = InterpretKey(setting.first); // Split setting key into section and argname - if (!GetArgFlags('-' + key.name)) { + if (!GetArgFlags_('-' + key.name)) { LogWarning("Ignoring unknown rw_settings value %s", setting.first); } } @@ -579,8 +579,8 @@ INSTANTIATE_INT_TYPE(uint64_t); bool ArgsManager::SoftSetArg(const std::string& strArg, const std::string& strValue) { LOCK(cs_args); - if (IsArgSet(strArg)) return false; - ForceSetArg(strArg, strValue); + if (!GetSetting_(strArg).isNull()) return false; + m_settings.forced_settings[SettingName(strArg)] = strValue; return true; } @@ -653,7 +653,7 @@ void ArgsManager::CheckMultipleCLIArgs() const auto cmds = m_available_args.find(OptionsCategory::CLI_COMMANDS); if (cmds != m_available_args.end()) { for (const auto& [cmd, argspec] : cmds->second) { - if (IsArgSet(cmd)) { + if (!GetSetting_(cmd).isNull()) { found.push_back(cmd); } } @@ -910,7 +910,7 @@ void ArgsManager::logArgsPrefix( std::string section_str = section.empty() ? "" : "[" + section + "] "; for (const auto& arg : args) { for (const auto& value : arg.second) { - std::optional flags = GetArgFlags('-' + arg.first); + std::optional flags = GetArgFlags_('-' + arg.first); if (flags) { std::string value_str = (*flags & SENSITIVE) ? "****" : value.write(); LogInfo("%s %s%s=%s\n", prefix, section_str, arg.first, value_str); diff --git a/src/common/config.cpp b/src/common/config.cpp index a2cf9381af2..85bea6740b6 100644 --- a/src/common/config.cpp +++ b/src/common/config.cpp @@ -99,7 +99,7 @@ bool ArgsManager::ReadConfigStream(std::istream& stream, const std::string& file } for (const std::pair& option : options) { KeyInfo key = InterpretKey(option.first); - std::optional flags = GetArgFlags('-' + key.name); + std::optional flags = GetArgFlags_('-' + key.name); if (!IsConfSupported(key, error)) return false; if (flags) { std::optional value = InterpretValue(key, &option.second, *flags, error); @@ -125,7 +125,8 @@ bool ArgsManager::ReadConfigFiles(std::string& error, bool ignore_invalid_keys) LOCK(cs_args); m_settings.ro_config.clear(); m_config_sections.clear(); - m_config_path = AbsPathForConfigVal(*this, GetPathArg("-conf", BITCOIN_CONF_FILENAME), /*net_specific=*/false); + const auto conf_val = GetPathArg_("-conf", BITCOIN_CONF_FILENAME); + m_config_path = (conf_val.is_absolute() || conf_val.empty()) ? conf_val : fsbridge::AbsPathJoin(GetDataDir(/*net_specific=*/false), conf_val); } const auto conf_path{GetConfigFilePath()}; From 3a16ec8582bee1dfc82857205ba1037c17a735cb Mon Sep 17 00:00:00 2001 From: w0xlt <94266259+w0xlt@users.noreply.github.com> Date: Wed, 4 Mar 2026 12:15:23 -0800 Subject: [PATCH 3/5] 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 --- src/test/argsman_tests.cpp | 116 +++++++++++++++++++------------------ 1 file changed, 60 insertions(+), 56 deletions(-) diff --git a/src/test/argsman_tests.cpp b/src/test/argsman_tests.cpp index e91ae5e96f3..3500bc5cc96 100644 --- a/src/test/argsman_tests.cpp +++ b/src/test/argsman_tests.cpp @@ -78,7 +78,6 @@ struct TestArgsManager : public ArgsManager using ArgsManager::GetSettingsList; using ArgsManager::ReadConfigStream; using ArgsManager::cs_args; - using ArgsManager::m_network; using ArgsManager::m_settings; }; @@ -217,29 +216,34 @@ BOOST_AUTO_TEST_CASE(util_ParseParameters) const char *argv_test[] = {"-ignored", "-a", "-b", "-ccc=argument", "-ccc=multiple", "f", "-d=e"}; std::string error; - LOCK(testArgs.cs_args); testArgs.SetupArgs({a, b, ccc, d}); BOOST_CHECK(testArgs.ParseParameters(0, argv_test, error)); - BOOST_CHECK(testArgs.m_settings.command_line_options.empty() && testArgs.m_settings.ro_config.empty()); + testArgs.LockSettings([&](const common::Settings& s) { + BOOST_CHECK(s.command_line_options.empty() && s.ro_config.empty()); + }); BOOST_CHECK(testArgs.ParseParameters(1, argv_test, error)); - BOOST_CHECK(testArgs.m_settings.command_line_options.empty() && testArgs.m_settings.ro_config.empty()); + testArgs.LockSettings([&](const common::Settings& s) { + BOOST_CHECK(s.command_line_options.empty() && s.ro_config.empty()); + }); BOOST_CHECK(testArgs.ParseParameters(7, argv_test, error)); // expectation: -ignored is ignored (program name argument), // -a, -b and -ccc end up in map, -d ignored because it is after // a non-option argument (non-GNU option parsing) - BOOST_CHECK(testArgs.m_settings.command_line_options.size() == 3 && testArgs.m_settings.ro_config.empty()); BOOST_CHECK(testArgs.IsArgSet("-a") && testArgs.IsArgSet("-b") && testArgs.IsArgSet("-ccc") && !testArgs.IsArgSet("f") && !testArgs.IsArgSet("-d")); - BOOST_CHECK(testArgs.m_settings.command_line_options.contains("a") && testArgs.m_settings.command_line_options.contains("b") && testArgs.m_settings.command_line_options.contains("ccc") - && !testArgs.m_settings.command_line_options.contains("f") && !testArgs.m_settings.command_line_options.contains("d")); + testArgs.LockSettings([&](const common::Settings& s) { + BOOST_CHECK(s.command_line_options.size() == 3 && s.ro_config.empty()); + BOOST_CHECK(s.command_line_options.contains("a") && s.command_line_options.contains("b") && s.command_line_options.contains("ccc") + && !s.command_line_options.contains("f") && !s.command_line_options.contains("d")); - BOOST_CHECK(testArgs.m_settings.command_line_options["a"].size() == 1); - BOOST_CHECK(testArgs.m_settings.command_line_options["a"].front().get_str() == ""); - BOOST_CHECK(testArgs.m_settings.command_line_options["ccc"].size() == 2); - BOOST_CHECK(testArgs.m_settings.command_line_options["ccc"].front().get_str() == "argument"); - BOOST_CHECK(testArgs.m_settings.command_line_options["ccc"].back().get_str() == "multiple"); + BOOST_CHECK(s.command_line_options.at("a").size() == 1); + BOOST_CHECK(s.command_line_options.at("a").front().get_str() == ""); + BOOST_CHECK(s.command_line_options.at("ccc").size() == 2); + BOOST_CHECK(s.command_line_options.at("ccc").front().get_str() == "argument"); + BOOST_CHECK(s.command_line_options.at("ccc").back().get_str() == "multiple"); + }); BOOST_CHECK(testArgs.GetArgs("-ccc").size() == 2); } @@ -331,7 +335,6 @@ BOOST_AUTO_TEST_CASE(util_GetBoolArg) const char *argv_test[] = { "ignored", "-a", "-nob", "-c=0", "-d=1", "-e=false", "-f=true"}; std::string error; - LOCK(testArgs.cs_args); testArgs.SetupArgs({a, b, c, d, e, f}); BOOST_CHECK(testArgs.ParseParameters(7, argv_test, error)); @@ -340,8 +343,9 @@ BOOST_AUTO_TEST_CASE(util_GetBoolArg) BOOST_CHECK(testArgs.IsArgSet({'-', opt}) || !opt); // Nothing else should be in the map - BOOST_CHECK(testArgs.m_settings.command_line_options.size() == 6 && - testArgs.m_settings.ro_config.empty()); + testArgs.LockSettings([&](const common::Settings& s) { + BOOST_CHECK(s.command_line_options.size() == 6 && s.ro_config.empty()); + }); // The -no prefix should get stripped on the way in. BOOST_CHECK(!testArgs.IsArgSet("-nob")); @@ -437,7 +441,6 @@ BOOST_AUTO_TEST_CASE(util_ReadConfigStream) "iii=2\n"; TestArgsManager test_args; - LOCK(test_args.cs_args); const auto a = std::make_pair("-a", ArgsManager::ALLOW_ANY); const auto b = std::make_pair("-b", ArgsManager::ALLOW_ANY); const auto ccc = std::make_pair("-ccc", ArgsManager::ALLOW_ANY); @@ -454,24 +457,26 @@ BOOST_AUTO_TEST_CASE(util_ReadConfigStream) // expectation: a, b, ccc, d, fff, ggg, h, i end up in map // so do sec1.ccc, sec1.d, sec1.h, sec2.ccc, sec2.iii - BOOST_CHECK(test_args.m_settings.command_line_options.empty()); - BOOST_CHECK(test_args.m_settings.ro_config.size() == 3); - BOOST_CHECK(test_args.m_settings.ro_config[""].size() == 8); - BOOST_CHECK(test_args.m_settings.ro_config["sec1"].size() == 3); - BOOST_CHECK(test_args.m_settings.ro_config["sec2"].size() == 2); + test_args.LockSettings([&](const common::Settings& s) { + BOOST_CHECK(s.command_line_options.empty()); + BOOST_CHECK(s.ro_config.size() == 3); + BOOST_CHECK(s.ro_config.at("").size() == 8); + BOOST_CHECK(s.ro_config.at("sec1").size() == 3); + BOOST_CHECK(s.ro_config.at("sec2").size() == 2); - BOOST_CHECK(test_args.m_settings.ro_config[""].contains("a")); - BOOST_CHECK(test_args.m_settings.ro_config[""].contains("b")); - BOOST_CHECK(test_args.m_settings.ro_config[""].contains("ccc")); - BOOST_CHECK(test_args.m_settings.ro_config[""].contains("d")); - BOOST_CHECK(test_args.m_settings.ro_config[""].contains("fff")); - BOOST_CHECK(test_args.m_settings.ro_config[""].contains("ggg")); - BOOST_CHECK(test_args.m_settings.ro_config[""].contains("h")); - BOOST_CHECK(test_args.m_settings.ro_config[""].contains("i")); - BOOST_CHECK(test_args.m_settings.ro_config["sec1"].contains("ccc")); - BOOST_CHECK(test_args.m_settings.ro_config["sec1"].contains("h")); - BOOST_CHECK(test_args.m_settings.ro_config["sec2"].contains("ccc")); - BOOST_CHECK(test_args.m_settings.ro_config["sec2"].contains("iii")); + BOOST_CHECK(s.ro_config.at("").contains("a")); + BOOST_CHECK(s.ro_config.at("").contains("b")); + BOOST_CHECK(s.ro_config.at("").contains("ccc")); + BOOST_CHECK(s.ro_config.at("").contains("d")); + BOOST_CHECK(s.ro_config.at("").contains("fff")); + BOOST_CHECK(s.ro_config.at("").contains("ggg")); + BOOST_CHECK(s.ro_config.at("").contains("h")); + BOOST_CHECK(s.ro_config.at("").contains("i")); + BOOST_CHECK(s.ro_config.at("sec1").contains("ccc")); + BOOST_CHECK(s.ro_config.at("sec1").contains("h")); + BOOST_CHECK(s.ro_config.at("sec2").contains("ccc")); + BOOST_CHECK(s.ro_config.at("sec2").contains("iii")); + }); BOOST_CHECK(test_args.IsArgSet("-a")); BOOST_CHECK(test_args.IsArgSet("-b")); @@ -604,25 +609,26 @@ BOOST_AUTO_TEST_CASE(util_ReadConfigStream) BOOST_AUTO_TEST_CASE(util_GetArg) { TestArgsManager testArgs; - LOCK(testArgs.cs_args); - testArgs.m_settings.command_line_options.clear(); - testArgs.m_settings.command_line_options["strtest1"] = {"string..."}; - // strtest2 undefined on purpose - testArgs.m_settings.command_line_options["inttest1"] = {"12345"}; - testArgs.m_settings.command_line_options["inttest2"] = {"81985529216486895"}; - // inttest3 undefined on purpose - testArgs.m_settings.command_line_options["booltest1"] = {""}; - // booltest2 undefined on purpose - testArgs.m_settings.command_line_options["booltest3"] = {"0"}; - testArgs.m_settings.command_line_options["booltest4"] = {"1"}; + testArgs.LockSettings([&](common::Settings& s) { + s.command_line_options.clear(); + s.command_line_options["strtest1"] = {"string..."}; + // strtest2 undefined on purpose + s.command_line_options["inttest1"] = {"12345"}; + s.command_line_options["inttest2"] = {"81985529216486895"}; + // inttest3 undefined on purpose + s.command_line_options["booltest1"] = {""}; + // booltest2 undefined on purpose + s.command_line_options["booltest3"] = {"0"}; + s.command_line_options["booltest4"] = {"1"}; - // priorities - testArgs.m_settings.command_line_options["pritest1"] = {"a", "b"}; - testArgs.m_settings.ro_config[""]["pritest2"] = {"a", "b"}; - testArgs.m_settings.command_line_options["pritest3"] = {"a"}; - testArgs.m_settings.ro_config[""]["pritest3"] = {"b"}; - testArgs.m_settings.command_line_options["pritest4"] = {"a","b"}; - testArgs.m_settings.ro_config[""]["pritest4"] = {"c","d"}; + // priorities + s.command_line_options["pritest1"] = {"a", "b"}; + s.ro_config[""]["pritest2"] = {"a", "b"}; + s.command_line_options["pritest3"] = {"a"}; + s.ro_config[""]["pritest3"] = {"b"}; + s.command_line_options["pritest4"] = {"a","b"}; + s.ro_config[""]["pritest4"] = {"c","d"}; + }); BOOST_CHECK_EQUAL(testArgs.GetArg("strtest1", "default"), "string..."); BOOST_CHECK_EQUAL(testArgs.GetArg("strtest2", "default"), "default"); @@ -815,11 +821,10 @@ BOOST_FIXTURE_TEST_CASE(util_ArgsMerge, ArgsMergeTestingSetup) ForEachMergeSetup([&](const ActionList& arg_actions, const ActionList& conf_actions, bool soft_set, bool force_set, const std::string& section, const std::string& network, bool net_specific) { TestArgsManager parser; - LOCK(parser.cs_args); std::string desc = "net="; desc += network; - parser.m_network = network; + parser.SelectConfigNetwork(network); const std::string& name = net_specific ? "wallet" : "server"; const std::string key = "-" + name; @@ -949,7 +954,6 @@ BOOST_FIXTURE_TEST_CASE(util_ChainMerge, ChainMergeTestingSetup) ForEachMergeSetup([&](const ActionList& arg_actions, const ActionList& conf_actions) { TestArgsManager parser; - LOCK(parser.cs_args); parser.AddArg("-regtest", "regtest", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); parser.AddArg("-testnet4", "testnet4", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); @@ -1027,14 +1031,14 @@ BOOST_AUTO_TEST_CASE(util_ReadWriteSettings) // Test writing setting. TestArgsManager args1; args1.ForceSetArg("-datadir", fs::PathToString(m_path_root)); - args1.LockSettings([&](common::Settings& settings) { settings.rw_settings["name"] = "value"; }); + args1.LockSettings([&](common::Settings& s) { s.rw_settings["name"] = "value"; }); args1.WriteSettingsFile(); // Test reading setting. TestArgsManager args2; args2.ForceSetArg("-datadir", fs::PathToString(m_path_root)); args2.ReadSettingsFile(); - args2.LockSettings([&](common::Settings& settings) { BOOST_CHECK_EQUAL(settings.rw_settings["name"].get_str(), "value"); }); + args2.LockSettings([&](common::Settings& s) { BOOST_CHECK_EQUAL(s.rw_settings["name"].get_str(), "value"); }); // Test error logging, and remove previously written setting. { From 22b40f34f33f5e5555d0b378f005516ecfb45e6c Mon Sep 17 00:00:00 2001 From: w0xlt <94266259+w0xlt@users.noreply.github.com> Date: Wed, 4 Mar 2026 12:25:40 -0800 Subject: [PATCH 4/5] 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). --- src/common/args.h | 96 +++++++++++++++++++++++------------------------ 1 file changed, 48 insertions(+), 48 deletions(-) diff --git a/src/common/args.h b/src/common/args.h index 0bcf32d1f88..fa22c36db7f 100644 --- a/src/common/args.h +++ b/src/common/args.h @@ -135,7 +135,7 @@ protected: unsigned int m_flags; }; - mutable RecursiveMutex cs_args; + mutable Mutex cs_args; common::Settings m_settings GUARDED_BY(cs_args); std::vector m_command GUARDED_BY(cs_args); std::string m_network GUARDED_BY(cs_args); @@ -149,7 +149,7 @@ protected: mutable fs::path m_cached_datadir_path GUARDED_BY(cs_args); mutable fs::path m_cached_network_datadir_path GUARDED_BY(cs_args); - [[nodiscard]] bool ReadConfigStream(std::istream& stream, const std::string& filepath, std::string& error, bool ignore_invalid_keys = false); + [[nodiscard]] bool ReadConfigStream(std::istream& stream, const std::string& filepath, std::string& error, bool ignore_invalid_keys = false) EXCLUSIVE_LOCKS_REQUIRED(!cs_args); /** * Returns true if settings values from the default section should be used, @@ -166,12 +166,12 @@ protected: * false if "-nosetting" argument was passed, and a string if a "-setting=value" * argument was passed. */ - common::SettingsValue GetSetting(const std::string& arg) const; + common::SettingsValue GetSetting(const std::string& arg) const EXCLUSIVE_LOCKS_REQUIRED(!cs_args); /** * Get list of setting values. */ - std::vector GetSettingsList(const std::string& arg) const; + std::vector GetSettingsList(const std::string& arg) const EXCLUSIVE_LOCKS_REQUIRED(!cs_args); ArgsManager(); ~ArgsManager(); @@ -179,16 +179,16 @@ protected: /** * Select the network in use */ - void SelectConfigNetwork(const std::string& network); + void SelectConfigNetwork(const std::string& network) EXCLUSIVE_LOCKS_REQUIRED(!cs_args); - [[nodiscard]] bool ParseParameters(int argc, const char* const argv[], std::string& error); + [[nodiscard]] bool ParseParameters(int argc, const char* const argv[], std::string& error) EXCLUSIVE_LOCKS_REQUIRED(!cs_args); /** * Return config file path (read-only) */ - fs::path GetConfigFilePath() const; - void SetConfigFilePath(fs::path); - [[nodiscard]] bool ReadConfigFiles(std::string& error, bool ignore_invalid_keys = false); + fs::path GetConfigFilePath() const EXCLUSIVE_LOCKS_REQUIRED(!cs_args); + void SetConfigFilePath(fs::path) EXCLUSIVE_LOCKS_REQUIRED(!cs_args); + [[nodiscard]] bool ReadConfigFiles(std::string& error, bool ignore_invalid_keys = false) EXCLUSIVE_LOCKS_REQUIRED(!cs_args); /** * Log warnings for options in m_section_only_args when @@ -196,12 +196,12 @@ protected: * on the command line or in a network-specific section in the * config file. */ - std::set GetUnsuitableSectionOnlyArgs() const; + std::set GetUnsuitableSectionOnlyArgs() const EXCLUSIVE_LOCKS_REQUIRED(!cs_args); /** * Log warnings for unrecognized section names in the config file. */ - std::list GetUnrecognizedSections() const; + std::list GetUnrecognizedSections() const EXCLUSIVE_LOCKS_REQUIRED(!cs_args); struct Command { /** The command (if one has been registered with AddCommand), or empty */ @@ -215,33 +215,33 @@ protected: /** * Get the command and command args (returns std::nullopt if no command provided) */ - std::optional GetCommand() const; + std::optional GetCommand() const EXCLUSIVE_LOCKS_REQUIRED(!cs_args); /** * Get blocks directory path * * @return Blocks path which is network specific */ - fs::path GetBlocksDirPath() const; + fs::path GetBlocksDirPath() const EXCLUSIVE_LOCKS_REQUIRED(!cs_args); /** * Get data directory path * * @return Absolute path on success, otherwise an empty path when a non-directory path would be returned */ - fs::path GetDataDirBase() const; + fs::path GetDataDirBase() const EXCLUSIVE_LOCKS_REQUIRED(!cs_args); /** * Get data directory path with appended network identifier * * @return Absolute path on success, otherwise an empty path when a non-directory path would be returned */ - fs::path GetDataDirNet() const; + fs::path GetDataDirNet() const EXCLUSIVE_LOCKS_REQUIRED(!cs_args); /** * Clear cached directory paths */ - void ClearPathCache(); + void ClearPathCache() EXCLUSIVE_LOCKS_REQUIRED(!cs_args); /** * Return a vector of strings of the given argument @@ -249,7 +249,7 @@ protected: * @param strArg Argument to get (e.g. "-foo") * @return command-line arguments */ - std::vector GetArgs(const std::string& strArg) const; + std::vector GetArgs(const std::string& strArg) const EXCLUSIVE_LOCKS_REQUIRED(!cs_args); /** * Return true if the given argument has been manually set @@ -257,7 +257,7 @@ protected: * @param strArg Argument to get (e.g. "-foo") * @return true if the argument has been set */ - bool IsArgSet(const std::string& strArg) const; + bool IsArgSet(const std::string& strArg) const EXCLUSIVE_LOCKS_REQUIRED(!cs_args); /** * Return true if the argument was originally passed as a negated option, @@ -266,7 +266,7 @@ protected: * @param strArg Argument to get (e.g. "-foo") * @return true if the argument was passed negated */ - bool IsArgNegated(const std::string& strArg) const; + bool IsArgNegated(const std::string& strArg) const EXCLUSIVE_LOCKS_REQUIRED(!cs_args); /** * Return string argument or default value @@ -275,8 +275,8 @@ protected: * @param strDefault (e.g. "1") * @return command-line argument or default value */ - std::string GetArg(const std::string& strArg, const std::string& strDefault) const; - std::optional GetArg(const std::string& strArg) const; + std::string GetArg(const std::string& strArg, const std::string& strDefault) const EXCLUSIVE_LOCKS_REQUIRED(!cs_args); + std::optional GetArg(const std::string& strArg) const EXCLUSIVE_LOCKS_REQUIRED(!cs_args); /** * Return path argument or default value @@ -288,7 +288,7 @@ protected: * for examples or implementation for details). If argument is empty or not * set, default_value is returned unchanged. */ - fs::path GetPathArg(std::string arg, const fs::path& default_value = {}) const; + fs::path GetPathArg(std::string arg, const fs::path& default_value = {}) const EXCLUSIVE_LOCKS_REQUIRED(!cs_args); /** * Return integer argument or default value @@ -298,13 +298,13 @@ protected: * @return command-line argument (0 if invalid number) or default value */ template - Int GetArg(const std::string& strArg, Int nDefault) const; + Int GetArg(const std::string& strArg, Int nDefault) const EXCLUSIVE_LOCKS_REQUIRED(!cs_args); template - std::optional GetArg(const std::string& strArg) const; + std::optional GetArg(const std::string& strArg) const EXCLUSIVE_LOCKS_REQUIRED(!cs_args); - int64_t GetIntArg(const std::string& strArg, int64_t nDefault) const { return GetArg(strArg, nDefault); } - std::optional GetIntArg(const std::string& strArg) const { return GetArg(strArg); } + int64_t GetIntArg(const std::string& strArg, int64_t nDefault) const EXCLUSIVE_LOCKS_REQUIRED(!cs_args) { return GetArg(strArg, nDefault); } + std::optional GetIntArg(const std::string& strArg) const EXCLUSIVE_LOCKS_REQUIRED(!cs_args) { return GetArg(strArg); } /** * Return boolean argument or default value @@ -313,8 +313,8 @@ protected: * @param fDefault (true or false) * @return command-line argument or default value */ - bool GetBoolArg(const std::string& strArg, bool fDefault) const; - std::optional GetBoolArg(const std::string& strArg) const; + bool GetBoolArg(const std::string& strArg, bool fDefault) const EXCLUSIVE_LOCKS_REQUIRED(!cs_args); + std::optional GetBoolArg(const std::string& strArg) const EXCLUSIVE_LOCKS_REQUIRED(!cs_args); /** * Set an argument if it doesn't already have a value @@ -323,7 +323,7 @@ protected: * @param strValue Value (e.g. "1") * @return true if argument gets set, false if it already had a value */ - bool SoftSetArg(const std::string& strArg, const std::string& strValue); + bool SoftSetArg(const std::string& strArg, const std::string& strValue) EXCLUSIVE_LOCKS_REQUIRED(!cs_args); /** * Set a boolean argument if it doesn't already have a value @@ -332,97 +332,97 @@ protected: * @param fValue Value (e.g. false) * @return true if argument gets set, false if it already had a value */ - bool SoftSetBoolArg(const std::string& strArg, bool fValue); + bool SoftSetBoolArg(const std::string& strArg, bool fValue) EXCLUSIVE_LOCKS_REQUIRED(!cs_args); // Forces an arg setting. Called by SoftSetArg() if the arg hasn't already // been set. Also called directly in testing. - void ForceSetArg(const std::string& strArg, const std::string& strValue); + void ForceSetArg(const std::string& strArg, const std::string& strValue) EXCLUSIVE_LOCKS_REQUIRED(!cs_args); /** * Returns the appropriate chain type from the program arguments. * @return ChainType::MAIN by default; raises runtime error if an invalid * combination, or unknown chain is given. */ - ChainType GetChainType() const; + ChainType GetChainType() const EXCLUSIVE_LOCKS_REQUIRED(!cs_args); /** * Returns the appropriate chain type string from the program arguments. * @return ChainType::MAIN string by default; raises runtime error if an * invalid combination is given. */ - std::string GetChainTypeString() const; + std::string GetChainTypeString() const EXCLUSIVE_LOCKS_REQUIRED(!cs_args); /** * Add argument */ - void AddArg(const std::string& name, const std::string& help, unsigned int flags, const OptionsCategory& cat); + void AddArg(const std::string& name, const std::string& help, unsigned int flags, const OptionsCategory& cat) EXCLUSIVE_LOCKS_REQUIRED(!cs_args); /** * Add subcommand */ - void AddCommand(const std::string& cmd, const std::string& help); + void AddCommand(const std::string& cmd, const std::string& help) EXCLUSIVE_LOCKS_REQUIRED(!cs_args); /** * Add many hidden arguments */ - void AddHiddenArgs(const std::vector& args); + void AddHiddenArgs(const std::vector& args) EXCLUSIVE_LOCKS_REQUIRED(!cs_args); /** * Clear available arguments */ - void ClearArgs(); + void ClearArgs() EXCLUSIVE_LOCKS_REQUIRED(!cs_args); /** * Check CLI command args * * @throws std::runtime_error when multiple CLI_COMMAND arguments are specified */ - void CheckMultipleCLIArgs() const; + void CheckMultipleCLIArgs() const EXCLUSIVE_LOCKS_REQUIRED(!cs_args); /** * Get the help string */ - std::string GetHelpMessage() const; + std::string GetHelpMessage() const EXCLUSIVE_LOCKS_REQUIRED(!cs_args); /** * Return Flags for known arg. * Return default flags for unknown arg. */ - std::optional GetArgFlags(const std::string& name) const; + std::optional GetArgFlags(const std::string& name) const EXCLUSIVE_LOCKS_REQUIRED(!cs_args); /** * Set default flags to return for an unknown arg. */ - void SetDefaultFlags(std::optional); + void SetDefaultFlags(std::optional) EXCLUSIVE_LOCKS_REQUIRED(!cs_args); /** * Get settings file path, or return false if read-write settings were * disabled with -nosettings. */ - bool GetSettingsPath(fs::path* filepath = nullptr, bool temp = false, bool backup = false) const; + bool GetSettingsPath(fs::path* filepath = nullptr, bool temp = false, bool backup = false) const EXCLUSIVE_LOCKS_REQUIRED(!cs_args); /** * Read settings file. Push errors to vector, or log them if null. */ - bool ReadSettingsFile(std::vector* errors = nullptr); + bool ReadSettingsFile(std::vector* errors = nullptr) EXCLUSIVE_LOCKS_REQUIRED(!cs_args); /** * Write settings file or backup settings file. Push errors to vector, or * log them if null. */ - bool WriteSettingsFile(std::vector* errors = nullptr, bool backup = false) const; + bool WriteSettingsFile(std::vector* errors = nullptr, bool backup = false) const EXCLUSIVE_LOCKS_REQUIRED(!cs_args); /** * Get current setting from config file or read/write settings file, * ignoring nonpersistent command line or forced settings values. */ - common::SettingsValue GetPersistentSetting(const std::string& name) const; + common::SettingsValue GetPersistentSetting(const std::string& name) const EXCLUSIVE_LOCKS_REQUIRED(!cs_args); /** * Access settings with lock held. */ template - void LockSettings(Fn&& fn) + void LockSettings(Fn&& fn) EXCLUSIVE_LOCKS_REQUIRED(!cs_args) { LOCK(cs_args); fn(m_settings); @@ -432,7 +432,7 @@ protected: * Log the config file options and the command line arguments, * useful for troubleshooting. */ - void LogArgs() const; + void LogArgs() const EXCLUSIVE_LOCKS_REQUIRED(!cs_args); private: // Internal helpers, for use by callers that already hold `cs_args`. @@ -454,7 +454,7 @@ private: * name was set. Raise an exception if an invalid combination of flags was * provided. */ - std::variant GetChainArg() const; + std::variant GetChainArg() const EXCLUSIVE_LOCKS_REQUIRED(!cs_args); // Helper function for LogArgs(). void logArgsPrefix( From 20fb7618b001937befbf8757e5881674b4a55147 Mon Sep 17 00:00:00 2001 From: w0xlt <94266259+w0xlt@users.noreply.github.com> Date: Fri, 6 Mar 2026 11:03:57 -0800 Subject: [PATCH 5/5] 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 --- src/common/args.cpp | 1 + src/common/args.h | 10 ++++++---- src/common/config.cpp | 13 +++++++++++++ src/test/argsman_tests.cpp | 35 ++++++++++++----------------------- 4 files changed, 32 insertions(+), 27 deletions(-) diff --git a/src/common/args.cpp b/src/common/args.cpp index be8c513339c..2a9a3c19fa5 100644 --- a/src/common/args.cpp +++ b/src/common/args.cpp @@ -644,6 +644,7 @@ void ArgsManager::ClearArgs() m_settings = {}; m_available_args.clear(); m_network_only_args.clear(); + m_config_sections.clear(); } void ArgsManager::CheckMultipleCLIArgs() const diff --git a/src/common/args.h b/src/common/args.h index fa22c36db7f..de323d3c490 100644 --- a/src/common/args.h +++ b/src/common/args.h @@ -127,7 +127,7 @@ public: COMMAND = 0x800, }; -protected: +private: struct Arg { std::string m_help_param; @@ -149,8 +149,6 @@ protected: mutable fs::path m_cached_datadir_path GUARDED_BY(cs_args); mutable fs::path m_cached_network_datadir_path GUARDED_BY(cs_args); - [[nodiscard]] bool ReadConfigStream(std::istream& stream, const std::string& filepath, std::string& error, bool ignore_invalid_keys = false) EXCLUSIVE_LOCKS_REQUIRED(!cs_args); - /** * Returns true if settings values from the default section should be used, * depending on the current network and whether the setting is @@ -158,7 +156,11 @@ protected: */ bool UseDefaultSection(const std::string& arg) const EXCLUSIVE_LOCKS_REQUIRED(cs_args); - public: +protected: + [[nodiscard]] bool ReadConfigStream(std::istream& stream, const std::string& filepath, std::string& error, bool ignore_invalid_keys = false) EXCLUSIVE_LOCKS_REQUIRED(!cs_args); + [[nodiscard]] bool ReadConfigString(const std::string& str_config) EXCLUSIVE_LOCKS_REQUIRED(!cs_args); + +public: /** * Get setting value. * diff --git a/src/common/config.cpp b/src/common/config.cpp index 85bea6740b6..a05c20d6f8f 100644 --- a/src/common/config.cpp +++ b/src/common/config.cpp @@ -19,6 +19,7 @@ #include #include #include +#include #include #include #include @@ -119,6 +120,18 @@ bool ArgsManager::ReadConfigStream(std::istream& stream, const std::string& file return true; } +bool ArgsManager::ReadConfigString(const std::string& str_config) +{ + std::istringstream streamConfig(str_config); + { + LOCK(cs_args); + m_settings.ro_config.clear(); + m_config_sections.clear(); + } + std::string error; + return ReadConfigStream(streamConfig, "", error); +} + bool ArgsManager::ReadConfigFiles(std::string& error, bool ignore_invalid_keys) { { diff --git a/src/test/argsman_tests.cpp b/src/test/argsman_tests.cpp index 3500bc5cc96..c8802d3641c 100644 --- a/src/test/argsman_tests.cpp +++ b/src/test/argsman_tests.cpp @@ -51,22 +51,9 @@ BOOST_AUTO_TEST_CASE(util_datadir) struct TestArgsManager : public ArgsManager { - TestArgsManager() { m_network_only_args.clear(); } void ReadConfigString(const std::string& str_config) { - std::istringstream streamConfig(str_config); - { - LOCK(cs_args); - m_settings.ro_config.clear(); - m_config_sections.clear(); - } - std::string error; - BOOST_REQUIRE(ReadConfigStream(streamConfig, "", error)); - } - void SetNetworkOnlyArg(const std::string& arg) - { - LOCK(cs_args); - m_network_only_args.insert(arg); + BOOST_REQUIRE(ArgsManager::ReadConfigString(str_config)); } void SetupArgs(const std::vector>& args) { @@ -74,11 +61,9 @@ struct TestArgsManager : public ArgsManager AddArg(arg.first, "", arg.second, OptionsCategory::OPTIONS); } } - using ArgsManager::GetSetting; - using ArgsManager::GetSettingsList; + + // make protected methods available for testing using ArgsManager::ReadConfigStream; - using ArgsManager::cs_args; - using ArgsManager::m_settings; }; //! Test GetSetting and GetArg type coercion, negation, and default value handling. @@ -584,9 +569,12 @@ BOOST_AUTO_TEST_CASE(util_ReadConfigStream) // Test section only options - test_args.SetNetworkOnlyArg("-d"); - test_args.SetNetworkOnlyArg("-ccc"); - test_args.SetNetworkOnlyArg("-h"); + const auto ccc2 = std::make_pair("-ccc", ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY); + const auto d2 = std::make_pair("-d", ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY); + const auto h2 = std::make_pair("-h", ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY); + test_args.ClearArgs(); + test_args.SetupArgs({a, b, ccc2, d2, e, fff, ggg, h2, i, iii}); + test_args.ReadConfigString(str_config); test_args.SelectConfigNetwork(ChainTypeToString(ChainType::MAIN)); BOOST_CHECK(test_args.GetArg("-d", "xxx") == "e"); @@ -828,8 +816,9 @@ BOOST_FIXTURE_TEST_CASE(util_ArgsMerge, ArgsMergeTestingSetup) const std::string& name = net_specific ? "wallet" : "server"; const std::string key = "-" + name; - parser.AddArg(key, name, ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); - if (net_specific) parser.SetNetworkOnlyArg(key); + unsigned int flags = ArgsManager::ALLOW_ANY; + if (net_specific) flags |= ArgsManager::NETWORK_ONLY; + parser.AddArg(key, name, flags, OptionsCategory::OPTIONS); auto args = GetValues(arg_actions, section, name, "a"); std::vector argv = {"ignored"};