ArgsManager: support subcommand-specific options

ajtowns commented at 9:00 am on November 6, 2023: contributor

Adds the ability to link particular options to one or more subcommands, and uses this feature in bitcoin-wallet. Separates out the help information for subcommand-specific options (duplicating it if an option applies to multiple subcommands), and provides a function for checking if some options have been specified that only apply to different subcommands.

DrahtBot commented at 9:00 am on November 6, 2023: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/28802.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	ryanofsky
Concept ACK	l0rinc
Approach ACK	stickies-v

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

Conflicts

No conflicts as of last run.

ajtowns requested review from ryanofsky on Nov 6, 2023

ajtowns requested review from achow101 on Nov 6, 2023

ajtowns requested review from maflcko on Nov 6, 2023

ajtowns commented at 9:02 am on November 6, 2023: contributor

Implements this comment from when these commands were introduced originally. Main motivation is that I’d like to add a new subcommand to bitcoin-util unrelated to the existing grind functionality, and have options for it.

ajtowns force-pushed on Nov 6, 2023

DrahtBot added the label CI failed on Nov 6, 2023

maflcko commented at 2:02 pm on November 6, 2023: member

tool_wallet.py fails CI

ajtowns force-pushed on Nov 6, 2023

DrahtBot removed the label CI failed on Nov 6, 2023

in src/common/args.h:67 in 162c5ab647 outdated

63@@ -64,6 +64,8 @@ enum class OptionsCategory {
64     COMMANDS,
65     REGISTER_COMMANDS,
66 
67+    SUBCOMMAND, // Specific to one or more subcommands

achow101 commented at 8:13 pm on November 6, 2023:

In 50368d92ff0f878af2d6a70773bdf6a6e29b25bb “ArgsManager: support subcommand specific options”

I found this name to be confusing since the word “subcommand” implies that the option itself is a subcommand (i.e. the same category as COMMANDS), rather than it being an option to a command. Would suggest renaming this to something like COMMAND_OPT.

ajtowns commented at 11:03 pm on November 6, 2023:

Renamed to COMMAND_OPTIONS and changed “subcommand” to “command” more generally.

l0rinc commented at 9:32 am on October 21, 2025:

The PR description still uses the term subcommand.

Would it be possible to list out the new restrictions in the description for more context, e.g. “When using bitcoin-wallet, providing the -dumpfile option with a command other than dump or createfromdump (for example, with the create command) will cause the program to exit immediately with a descriptive error message”).

ajtowns force-pushed on Nov 6, 2023

DrahtBot added the label CI failed on Nov 6, 2023

ajtowns force-pushed on Nov 6, 2023

DrahtBot removed the label CI failed on Nov 6, 2023

in src/common/args.cpp:587 in 6be2b82e1c outdated

579@@ -557,6 +580,13 @@ void ArgsManager::AddCommand(const std::string& cmd, const std::string& help)
580     m_accept_any_command = false; // latch to false
581     std::map<std::string, Arg>& arg_map = m_available_args[OptionsCategory::COMMANDS];
582     auto ret = arg_map.emplace(cmd, Arg{"", help, ArgsManager::COMMAND});
583+    if (!options.empty()) {
584+        std::set<std::string> args;
585+        for (const auto& a : options) {
586+            args.insert(a);
587+        }

stickies-v commented at 5:08 pm on November 7, 2023:

When changing options into an rvalue std::vector<std::string>&& options={}, can simplify this to:

0        std::set<std::string> args(std::move(options.begin()), std::move(options.end()));

ajtowns commented at 2:41 pm on December 5, 2023:

Changed options to a std::set<std::string>&&.

in src/common/args.cpp:721 in 6be2b82e1c outdated

717@@ -676,10 +718,11 @@ std::string HelpMessageGroup(const std::string &message) {
718     return std::string(message) + std::string("\n\n");
719 }
720 
721-std::string HelpMessageOpt(const std::string &option, const std::string &message) {
722-    return std::string(optIndent,' ') + std::string(option) +
723-           std::string("\n") + std::string(msgIndent,' ') +
724-           FormatParagraph(message, screenWidth - msgIndent, msgIndent) +
725+std::string HelpMessageOpt(const std::string& option, const std::string& help_param, const std::string &message, bool suboption) {

stickies-v commented at 2:49 pm on November 8, 2023:

nit: Works fine for now, but perhaps a uint8_t nesting_level is a bit more general and not more verbose?

ajtowns commented at 2:42 pm on December 5, 2023:

Added a HelpMessageSubOpt function instead of a bool argument

in src/common/args.cpp:741 in 6be2b82e1c outdated

694+                             if ((cmdopt.second.m_flags & ArgsManager::DEBUG_ONLY) && !show_debug) continue;
695+                             if (cmdopt_set.count(cmdopt.first)) {
696+                                 usage += HelpMessageOpt(cmdopt.first, cmdopt.second.m_help_param, cmdopt.second.m_help_text, true);
697+                             }
698+                         }
699+                    }

stickies-v commented at 5:55 pm on November 8, 2023:

I found this quite difficult to read, could be improved with structured bindings, consistent naming, and some other slight improvements, I think. Pushed 3 commits to https://github.com/bitcoin/bitcoin/compare/6be2b82e1ce2207b0b9af01ced67d71361a2047b...stickies-v:bitcoin:pr/28802-suggested-simplifications - what do you think?

Potentially, this could be made more straightforward by having m_command_args use iterators into m_available_args, but that’s probably not worth the iterator management overhead.

ajtowns commented at 2:45 pm on December 5, 2023:

I’ve reworked this differently; wanted to keep the help order for each command to match how options are declared, and avoid using .at() so that -help still mostly works even if there’s bugs in how options are declared.

in src/common/args.h:223 in 6be2b82e1c outdated

211@@ -209,6 +212,11 @@ class ArgsManager
212      */
213     std::optional<const Command> GetCommand() const;
214 
215+    /**
216+     * Check for invalid command options
217+     */
218+    bool CheckCommandOptions(const std::string& command, std::vector<std::string>* errors = nullptr) const;

stickies-v commented at 7:03 pm on November 8, 2023:

Could also use a util::Result<void> return type to avoid the out-parameter?

 0diff --git a/src/common/args.cpp b/src/common/args.cpp
 1index b9454d3814..f7509b3e7b 100644
 2--- a/src/common/args.cpp
 3+++ b/src/common/args.cpp
 4@@ -15,6 +15,7 @@
 5 #include <util/check.h>
 6 #include <util/fs.h>
 7 #include <util/fs_helpers.h>
 8+#include <util/result.h>
 9 #include <util/strencodings.h>
10 
11 #ifdef WIN32
12@@ -358,27 +359,25 @@ std::optional<const ArgsManager::Command> ArgsManager::GetCommand() const
13     return ret;
14 }
15 
16-bool ArgsManager::CheckCommandOptions(const std::string& command, std::vector<std::string>* errors) const
17+util::Result<void> ArgsManager::CheckCommandOptions(const std::string& command) const
18 {
19     LOCK(cs_args);
20 
21     auto command_options = m_available_args.find(OptionsCategory::COMMAND_OPTIONS);
22-    if (command_options == m_available_args.end()) return true;
23+    if (command_options == m_available_args.end()) return {};
24 
25     const std::set<std::string> dummy;
26     auto command_args = m_command_args.find(command);
27     const std::set<std::string>& valid_opts = (command_args == m_command_args.end() ? dummy : command_args->second);
28 
29-    bool ok = true;
30+    std::vector<std::string> errors;
31     for (const auto& opts : command_options->second) {
32         if (!IsArgSet(opts.first)) continue;
33         if (valid_opts.count(opts.first)) continue;
34-        if (errors != nullptr) {
35-            errors->emplace_back(strprintf("The %s option cannot be used with the '%s' command.", opts.first, command));
36-            ok = false;
37-        }
38+        errors.emplace_back(strprintf("The %s option cannot be used with the '%s' command.", opts.first, command));
39     }
40-    return ok;
41+
42+    return errors.empty() ? util::Result<void>{} : util::Error{Untranslated(MakeUnorderedList(errors))};
43 }
44 
45 std::vector<std::string> ArgsManager::GetArgs(const std::string& strArg) const
46diff --git a/src/common/args.h b/src/common/args.h
47index 8aa6644bd1..b8442a7c81 100644
48--- a/src/common/args.h
49+++ b/src/common/args.h
50@@ -23,6 +23,11 @@
51 
52 class ArgsManager;
53 
54+namespace util {
55+    template<typename T>
56+    class Result;
57+} //namespace util
58+
59 extern const char * const BITCOIN_CONF_FILENAME;
60 extern const char * const BITCOIN_SETTINGS_FILENAME;
61 
62@@ -215,7 +220,7 @@ protected:
63     /**
64      * Check for invalid command options
65      */
66-    bool CheckCommandOptions(const std::string& command, std::vector<std::string>* errors = nullptr) const;
67+    util::Result<void> CheckCommandOptions(const std::string& command) const;
68 
69     /**
70      * Get blocks directory path
71diff --git a/src/wallet/wallettool.cpp b/src/wallet/wallettool.cpp
72index d4ee54dada..dfe5ca5807 100644
73--- a/src/wallet/wallettool.cpp
74+++ b/src/wallet/wallettool.cpp
75@@ -116,9 +116,8 @@ static void WalletShowInfo(CWallet* wallet_instance)
76 bool ExecuteWalletToolFunc(const ArgsManager& args, const std::string& command)
77 {
78     {
79-        std::vector<std::string> details;
80-        if (!args.CheckCommandOptions(command, &details)) {
81-            tfm::format(std::cerr, "Error: Invalid arguments provided:\n%s\n", MakeUnorderedList(details));
82+        if (const auto valid_command{args.CheckCommandOptions(command)}; !valid_command) {
83+            tfm::format(std::cerr, "Error: Invalid arguments provided:\n%s\n", util::ErrorString(valid_command).original);
84             return false;
85         }
86     }

ajtowns commented at 2:49 pm on December 5, 2023:

That doesn’t really seem much simpler to me, and constructing strings that way doesn’t seem a good match for the bilingual_str that Result contains. So going to leave that out of this PR.

in src/common/args.cpp:370 in 6be2b82e1c outdated

357@@ -358,6 +358,29 @@ std::optional<const ArgsManager::Command> ArgsManager::GetCommand() const
358     return ret;
359 }
360 
361+bool ArgsManager::CheckCommandOptions(const std::string& command, std::vector<std::string>* errors) const
362+{
363+    LOCK(cs_args);
364+
365+    auto command_options = m_available_args.find(OptionsCategory::COMMAND_OPTIONS);

stickies-v commented at 7:08 pm on November 8, 2023:

nit: list initialization, here and in a few other places

0    auto command_options{m_available_args.find(OptionsCategory::COMMAND_OPTIONS)};

ajtowns commented at 2:53 pm on December 5, 2023:

We have plenty of auto foo = bar initialisations, and it at least used to be less ambiguous to write things that way, so I don’t think this nit’s a good one.

stickies-v commented at 8:54 pm on November 8, 2023: contributor

Approach ACK

ajtowns force-pushed on Dec 5, 2023

DrahtBot added the label CI failed on Dec 5, 2023

ajtowns force-pushed on Dec 5, 2023

DrahtBot removed the label CI failed on Dec 5, 2023

DrahtBot added the label CI failed on Dec 7, 2023

DrahtBot removed the label CI failed on Dec 12, 2023

DrahtBot added the label CI failed on Jan 16, 2024

ajtowns force-pushed on Feb 13, 2024

DrahtBot removed the label CI failed on Feb 13, 2024

DrahtBot added the label Needs rebase on Mar 11, 2024

ajtowns force-pushed on Mar 12, 2024

DrahtBot removed the label Needs rebase on Mar 13, 2024

ajtowns requested review from stickies-v on Apr 10, 2024

ajtowns requested review from achow101 on Apr 10, 2024

DrahtBot added the label CI failed on Jun 13, 2024

ajtowns force-pushed on Jun 17, 2024

DrahtBot removed the label CI failed on Jun 17, 2024

DrahtBot added the label Needs rebase on Sep 4, 2024

ajtowns force-pushed on Sep 6, 2024

DrahtBot removed the label Needs rebase on Sep 6, 2024

DrahtBot added the label CI failed on Sep 6, 2024

DrahtBot removed the label CI failed on Sep 7, 2024

DrahtBot added the label CI failed on Sep 12, 2024

DrahtBot removed the label CI failed on Sep 15, 2024

DrahtBot added the label CI failed on Mar 13, 2025

DrahtBot commented at 11:29 am on March 15, 2025: contributor

Needs rebase, if still relevant

ajtowns marked this as a draft on Mar 16, 2025

ajtowns commented at 6:05 am on March 16, 2025: contributor

Leaving as draft pending #31250

DrahtBot added the label Needs rebase on Apr 25, 2025

ajtowns force-pushed on May 14, 2025

DrahtBot removed the label Needs rebase on May 14, 2025

DrahtBot removed the label CI failed on May 14, 2025

ajtowns force-pushed on May 14, 2025

ajtowns marked this as ready for review on May 14, 2025

ajtowns commented at 4:38 am on May 14, 2025: contributor

Rebased and undrafted

DrahtBot added the label Needs rebase on Sep 25, 2025

in src/common/args.cpp:376 in 0d1d11fc02

371+    const std::set<std::string>& valid_opts = (command_args == m_command_args.end() ? dummy : command_args->second);
372+
373+    bool ok = true;
374+    for (const auto& opts : command_options->second) {
375+        if (!IsArgSet(opts.first)) continue;
376+        if (valid_opts.count(opts.first)) continue;

l0rinc commented at 8:37 am on October 21, 2025:

0        if (valid_opts.contains(opts.first)) continue;

or even better, to avoid having the dummy, we could make this a predicate:

0auto is_valid_opt{[&](std::string_view opt) {
1    return command_args != m_command_args.end() && command_args->second.contains(opt);
2}};
3...
4if (is_valid_opt(opt_name)) continue;

in src/common/args.cpp:657 in 0d1d11fc02

652+    {
653+        if (select.empty()) return;
654+        if (m_iter == m_end) return;
655+        for (const auto& [cmdopt_name, cmdopt_info] : m_iter->second) {
656+            if (!with_debug && (cmdopt_info.m_flags & ArgsManager::DEBUG_ONLY)) continue;
657+            if (!select.count(cmdopt_name)) continue;

l0rinc commented at 8:39 am on October 21, 2025:

0            if (!select.contains(cmdopt_name)) continue;

in src/common/args.h:70 in 0d1d11fc02 outdated

65@@ -66,6 +66,8 @@ enum class OptionsCategory {
66     CLI_COMMANDS,
67     IPC,
68 
69+    COMMAND_OPTIONS, // Specific to one or more commands

l0rinc commented at 8:42 am on October 21, 2025:

This seems safe, just have to make sure: is it safe to add in the middle of this enum (e.g. is its index persisted anywhere?). If the placement doesn’t matter, can we add the options closer to the command values (before IPC)?

ajtowns commented at 4:21 am on November 18, 2025:

Placement doesn’t matter, but I don’t think moving closer to COMMANDS, REGISTER_COMMANDS or CLI_COMMANDS really makes sense – COMMAND_OPTIONS relates to argsman.AddCommand() commands (used by bitcoin-wallet and bitcoin-util), which differ from COMMANDS (used by bitcoin-tx), REGISTER_COMMANDS (also used by bitcoin-tx), and CLI_COMMANDS (used by bitcoin-cli for its non-rpc commands, -generate, -netinfo, -addrinfo, -getinfo).

in src/common/args.h:360 in a8c6d63e3a outdated

349@@ -347,9 +350,9 @@ class ArgsManager
350     void AddArg(const std::string& name, const std::string& help, unsigned int flags, const OptionsCategory& cat);
351 
352     /**
353-     * Add subcommand
354+     * Add command

l0rinc commented at 8:45 am on October 21, 2025:

Is this comment still useful now?

ajtowns commented at 4:23 am on November 18, 2025:

It’s a docstring, so shows up in the doxygen docs https://doxygen.bitcoincore.org/class_args_manager.html#ab71d531303f7e6fe3bbf67683a414a2e ; maybe it’s not that useful, but doesn’t seem worth removing outright

in src/common/args.h:144 in 0d1d11fc02 outdated

138@@ -137,6 +139,7 @@ class ArgsManager
139     std::string m_network GUARDED_BY(cs_args);
140     std::set<std::string> m_network_only_args GUARDED_BY(cs_args);
141     std::map<OptionsCategory, std::map<std::string, Arg>> m_available_args GUARDED_BY(cs_args);
142+    std::map<std::string, std::set<std::string>> m_command_args GUARDED_BY(cs_args);

l0rinc commented at 8:49 am on October 21, 2025:

This line seems to conflict with latest master.

nit: is it important for the set to be sorted here?

ajtowns commented at 4:24 am on November 18, 2025:

It’s looked up by the key in CheckCommandOptions, so sorting seems fine to me?

in src/common/args.cpp:791 in 0d1d11fc02

785@@ -720,6 +786,12 @@ bool HasTestOption(const ArgsManager& args, const std::string& test_option)
786     });
787 }
788 
789+std::string HelpMessageSubOpt(const std::string& option, const std::string& help_param, const std::string &message)
790+{
791+

l0rinc commented at 8:51 am on October 21, 2025:

nit: formatting

in src/common/args.cpp:773 in 0d1d11fc02

772+std::string HelpMessageOpt(const std::string& option, const std::string& help_param, const std::string &message, int indent)
773+{
774+    return std::string(optIndent+indent,' ') + option + help_param +
775+           std::string("\n") + std::string(msgIndent+indent,' ') +
776+           FormatParagraph(message, screenWidth - msgIndent - indent, msgIndent + indent) +
777            std::string("\n\n");

l0rinc commented at 9:33 am on October 21, 2025:

This seems more complicated than I think it should be, strprintf has baked-in indentation

0    return strprintf("%*s%s%s\n%*s%s\n\n",
1                     optIndent + indent, "",option, help_param,
2                     msgIndent + indent, "", FormatParagraph(message, screenWidth - msgIndent - indent, msgIndent + indent));

(Though I would have expected FormatParagraph to be able to apply the indentation at the beginning as well, which would enable)

0    return strprintf("%*s%s%s\n%*s%s\n\n",
1                     optIndent + indent, "",option, help_param,
2                     FormatParagraph(message, screenWidth - msgIndent - indent, msgIndent + indent));

ajtowns commented at 4:36 am on November 18, 2025:

FormatParagraph() only indenting when it sees a \n seems plausible for dealing with an indented continuation (ie, "Hello " + FormatParagraph("world!\n", 79, 4)) and the current behaviour is tested, so I’d rather not touch that.

in src/common/args.cpp:768 in 0d1d11fc02

764@@ -700,10 +765,11 @@ std::string HelpMessageGroup(const std::string &message) {
765     return std::string(message) + std::string("\n\n");
766 }
767 
768-std::string HelpMessageOpt(const std::string &option, const std::string &message) {
769-    return std::string(optIndent,' ') + std::string(option) +
770-           std::string("\n") + std::string(msgIndent,' ') +
771-           FormatParagraph(message, screenWidth - msgIndent, msgIndent) +
772+std::string HelpMessageOpt(const std::string& option, const std::string& help_param, const std::string &message, int indent)

l0rinc commented at 9:36 am on October 21, 2025:

slightly unrelated: maybe we could use std::string_view here instead

in src/common/args.cpp:665 in 0d1d11fc02 outdated

646+      m_iter{available_args.find(OptionsCategory::COMMAND_OPTIONS)}
647+    {
648+    }
649+
650+    template <typename Fn>
651+    void Iterate(const std::set<std::string>& select, bool with_debug, Fn&& fn) const

l0rinc commented at 10:12 am on October 21, 2025:

Most of the new code is completely uncovered: https://corecheck.dev/bitcoin/bitcoin/pulls/28802

Adding temporary exceptions inside indicates that tool_wallet.py and wallet_encryption.py cover some of the functions here. I understand that argsman_tests.cpp is already very weak coverage-wise, but would be great to add some coverage for the new code at least.

ajtowns commented at 5:31 am on November 18, 2025:

Added some unit test coverage

in src/common/args.cpp:374 in 0d1d11fc02

369+    const std::set<std::string> dummy;
370+    auto command_args = m_command_args.find(command);
371+    const std::set<std::string>& valid_opts = (command_args == m_command_args.end() ? dummy : command_args->second);
372+
373+    bool ok = true;
374+    for (const auto& opts : command_options->second) {

l0rinc commented at 10:19 am on October 21, 2025:

we could destructure here as well:

0    for (const auto& [arg, _] : command_options->second) {

or

0    for (const auto& arg : command_options->second | std::views::keys) {

in src/common/args.cpp:669 in 0d1d11fc02 outdated

650+    template <typename Fn>
651+    void Iterate(const std::set<std::string>& select, bool with_debug, Fn&& fn) const
652+    {
653+        if (select.empty()) return;
654+        if (m_iter == m_end) return;
655+        for (const auto& [cmdopt_name, cmdopt_info] : m_iter->second) {

l0rinc commented at 10:22 am on October 21, 2025:

nit: when is it opt and when cmdopt?

in src/bitcoin-wallet.cpp:45 in 0d1d11fc02 outdated

43 
44     argsman.AddCommand("info", "Get wallet info");
45     argsman.AddCommand("create", "Create a new descriptor wallet file");
46-    argsman.AddCommand("dump", "Print out all of the wallet key-value records");
47-    argsman.AddCommand("createfromdump", "Create new wallet file from dumped records");
48+    argsman.AddCommand("dump", "Print out all of the wallet key-value records", {"-dumpfile"});

l0rinc commented at 10:31 am on October 21, 2025:

are there any other such relations between command and arg that we could check this way?

ajtowns commented at 5:29 am on November 18, 2025:

Not currently, I think

in src/common/args.cpp:369 in 0d1d11fc02

364+    LOCK(cs_args);
365+
366+    auto command_options = m_available_args.find(OptionsCategory::COMMAND_OPTIONS);
367+    if (command_options == m_available_args.end()) return true;
368+
369+    const std::set<std::string> dummy;

l0rinc commented at 10:38 am on October 21, 2025:

this isn’t really a dummy, it’s just empty - but can be improved by a higher-level lambda, as mentioned in another comment

in src/common/args.cpp:371 in 0d1d11fc02

366+    auto command_options = m_available_args.find(OptionsCategory::COMMAND_OPTIONS);
367+    if (command_options == m_available_args.end()) return true;
368+
369+    const std::set<std::string> dummy;
370+    auto command_args = m_command_args.find(command);
371+    const std::set<std::string>& valid_opts = (command_args == m_command_args.end() ? dummy : command_args->second);

l0rinc commented at 10:38 am on October 21, 2025:

Same here: can we use unordered_set instead?

l0rinc commented at 10:47 am on October 21, 2025: contributor

Concept ACK, it makes sense to associate specific command-line options with specific commands - like function names with args. I can imagine this being useful in other cases as well. Given that the PR needs a rebase, I have commented on every nit I could find - and I would need full test coverage for this to be comfortable acking, but like the commit split otherwise. If you can, I would appreciate adding slightly more context to the PR description and the commit messages.

achow101 commented at 9:51 pm on November 17, 2025: member

Are you still working on this? This has needed rebase for a few months now.

ArgsManager: support command-specific options e2debfa5d3

bitcoin-wallet: use command-specific options 42f24f4433

ArgsManager: automate checking for correct command options 6105207f61

tests: Add some test coverage for ArgsManager::AddCommand cce7eba489

ajtowns force-pushed on Nov 18, 2025

ajtowns commented at 5:35 am on November 18, 2025: contributor

If you can, I would appreciate adding slightly more context to the PR description and the commit messages.

Sorry, these seem completely self-explanatory to me, so I don’t see what to add. Happy to answer questions or take suggestions though.

DrahtBot removed the label Needs rebase on Nov 18, 2025

ryanofsky commented at 3:49 pm on November 18, 2025: contributor

Concept ACK cce7eba489d5fb8b9f603584b5939dd42ccc6e8. This PR improves documentation and error handling for subcommand specific options.

I do think the PR could use a more detailed description. (I wasn’t very familiar with existing subcommand support so I didn’t know what was changing.)

Right now, ArgsManager supports subcommands like bitcoin-wallet dump and supports subcommand-specific options like -dumpfile, e.g.:

0bitcoin-wallet -wallet=foo -dumpfile=foo.txt dump

But ArgsManager doesn’t know that -dumpfile belongs only to certain subcommands, so it:

lists -dumpfile in the global help rather than under the relevant subcommands, and
does not verify that -dumpfile is used with a compatible subcommand, leaving that error handling to the wallet code.

This PR improves on that, changing help and error output as follows:

 0@@ -14,10 +14,6 @@
 1   -datadir=<dir>
 2        Specify data directory
 3 
 4-  -dumpfile=<file name>
 5-       When used with 'dump', writes out the records to this file. When used
 6-       with 'createfromdump', loads the records into a new wallet.
 7-
 8   -help
 9        Print this help message and exit (also -h or -?)
10 
11@@ -73,9 +69,19 @@
12   createfromdump
13        Create new wallet file from dumped records
14 
15+       -dumpfile=<file name>
16+            When used with 'dump', writes out the records to this file. When
17+            used with 'createfromdump', loads the records into a
18+            new wallet.
19+
20   dump
21        Print out all of the wallet key-value records
22 
23+       -dumpfile=<file name>
24+            When used with 'dump', writes out the records to this file. When
25+            used with 'createfromdump', loads the records into a
26+            new wallet.
27+
28   info
29        Get wallet info

0 $ bitcoin-wallet -regtest -wallet=foo -dumpfile=out info
1-The -dumpfile option can only be used with the "dump" and "createfromdump" commands.
2+Error: Invalid arguments provided:
3+- The -dumpfile option cannot be used with the 'info' command.

Limitations

As can be seen above, if an option applies to multiple subcommands, it must share the same help text. There’s no way to provide subcommand-specific descriptions.

This does not change option parsing rules. Options still cannot appear after the subcommand:

0$ bitcoin-wallet -wallet=foo dump -dumpfile=foo.txt
1Error: Additional arguments provided (-dumpfile=foo.txt). Methods do not take arguments. Please refer to `-help`.

Nevertheless, this change should be a strict improvement.

ajtowns commented at 1:29 am on November 19, 2025: contributor

I do think the PR could use a more detailed description.

Should I cut and paste what you wrote below into the PR or commit description? Sorry, I need an ELI5 here :)

ryanofsky commented at 4:03 pm on November 19, 2025: contributor

Should I cut and paste what you wrote below into the PR or commit description?

Feel free to use & change any of it. I mostly wrote that for myself to be able to remember what changes this PR was making. I do think the current description is perfectly ok and accurate, but I skipped over this PR many times not really knowing what it is was doing or if I should take the time to find out, so adding some more details could help this get more attention.

in src/common/args.cpp:627 in e2debfa5d3 outdated

620@@ -618,14 +621,46 @@ void ArgsManager::CheckMultipleCLIArgs() const
621     }
622 }
623 
624+namespace {
625+/** Helper class for iterating over COMMAND_OPTIONS applicable to a given command */
626+template <typename T>
627+class CommandOptionsGetter

ryanofsky commented at 4:26 pm on November 19, 2025:

In commit “ArgsManager: support command-specific options” (e2debfa5d38ef06f0676049310088c9e7cf69c34)

Any plans to reuse this class? It would seem simpler to inline:

 0--- a/src/common/args.cpp
 1+++ b/src/common/args.cpp
 2@@ -646,35 +646,6 @@ void ArgsManager::CheckMultipleCLIArgs() const
 3     }
 4 }
 5 
 6-namespace {
 7-/** Helper class for iterating over COMMAND_OPTIONS applicable to a given command */
 8-template <typename T>
 9-class CommandOptionsGetter
10-{
11-private:
12-    const typename T::const_iterator m_end;
13-    const typename T::const_iterator m_iter;
14-public:
15-    CommandOptionsGetter(const T& available_args)
16-    : m_end{available_args.end()},
17-      m_iter{available_args.find(OptionsCategory::COMMAND_OPTIONS)}
18-    {
19-    }
20-
21-    template <typename Fn>
22-    void Iterate(const std::set<std::string>& select, bool with_debug, Fn&& fn) const
23-    {
24-        if (select.empty()) return;
25-        if (m_iter == m_end) return;
26-        for (const auto& [cmdopt_name, cmdopt_info] : m_iter->second) {
27-            if (!with_debug && (cmdopt_info.m_flags & ArgsManager::DEBUG_ONLY)) continue;
28-            if (!select.contains(cmdopt_name)) continue;
29-            fn(cmdopt_name, cmdopt_info);
30-        }
31-    }
32-};
33-} // anonymous namespace
34-
35 std::string ArgsManager::GetHelpMessage() const
36 {
37     const bool show_debug = GetBoolArg("-help-debug", false);
38@@ -682,7 +653,7 @@ std::string ArgsManager::GetHelpMessage() const
39     std::string usage;
40     LOCK(cs_args);
41 
42-    const auto command_options = CommandOptionsGetter(m_available_args);
43+    const auto command_options = m_available_args.find(OptionsCategory::COMMAND_OPTIONS);
44 
45     for (const auto& [category, category_args] : m_available_args) {
46         switch(category) {
47@@ -748,10 +719,12 @@ std::string ArgsManager::GetHelpMessage() const
48 
49                 if (category == OptionsCategory::COMMANDS) {
50                     const auto cmd_args = m_command_args.find(arg_name);
51-                    if (cmd_args != m_command_args.end()) {
52-                        command_options.Iterate(cmd_args->second, show_debug, [&](const auto& cmdopt_name, const auto& cmdopt_info) {
53+                    if (cmd_args != m_command_args.end() && command_options != m_available_args.end()) {
54+                        for (const auto& [cmdopt_name, cmdopt_info] : command_options->second) {
55+                            if (!show_debug && (cmdopt_info.m_flags & ArgsManager::DEBUG_ONLY)) continue;
56+                            if (!cmd_args->second.contains(cmdopt_name)) continue;
57                             usage += HelpMessageSubOpt(cmdopt_name, cmdopt_info.m_help_param, cmdopt_info.m_help_text);
58-                        });
59+                        }
60                     }
61                 }
62             }

in src/common/args.h:484 in e2debfa5d3 outdated

477@@ -474,10 +478,22 @@ std::string HelpMessageGroup(const std::string& message);
478 /**
479  * Format a string to be used as option description in help messages
480  *
481- * @param option Option message (e.g. "-rpcuser=<user>")
482+ * @param option Option name (e.g. "-rpcuser")
483+ * @param help_param Help parameter (e.g. "=<user>" or "")
484+ * @param message Option description (e.g. "Username for JSON-RPC connections")
485+ * @param indent Additional indentation

ryanofsky commented at 4:53 pm on November 19, 2025:

In commit “ArgsManager: support command-specific options” (e2debfa5d38ef06f0676049310088c9e7cf69c34)

Giving this function an indent parameter doesn’t seem right semantically since the other parameters are higher-level parameters unrelated to formatting, and the function is still hardcoding most other formatting value internally. This also increases code complexity by requiring a separate HelpMessageSubOpt function, because the caller doesn’t have access to the formatting constants. Would suggest keeping this high-level and simplifying code with a change like:

 0--- a/src/common/args.cpp
 1+++ b/src/common/args.cpp
 2@@ -750,7 +750,7 @@ std::string ArgsManager::GetHelpMessage() const
 3                     const auto cmd_args = m_command_args.find(arg_name);
 4                     if (cmd_args != m_command_args.end()) {
 5                         command_options.Iterate(cmd_args->second, show_debug, [&](const auto& cmdopt_name, const auto& cmdopt_info) {
 6-                            usage += HelpMessageSubOpt(cmdopt_name, cmdopt_info.m_help_param, cmdopt_info.m_help_text);
 7+                            usage += HelpMessageOpt(cmdopt_name, cmdopt_info.m_help_param, cmdopt_info.m_help_text, /*subopt=*/true);
 8                         });
 9                     }
10                 }
11@@ -779,8 +779,9 @@ std::string HelpMessageGroup(const std::string &message) {
12     return std::string(message) + std::string("\n\n");
13 }
14 
15-std::string HelpMessageOpt(std::string_view option, std::string_view help_param, std::string_view message, int indent)
16+std::string HelpMessageOpt(std::string_view option, std::string_view help_param, std::string_view message, bool subopt)
17 {
18+    const int indent{subopt ? msgIndent - optIndent : 0};
19     return strprintf("%*s%s%s\n%*s%s\n\n",
20            (optIndent + indent), "", option, help_param,
21            (msgIndent + indent), "", FormatParagraph(message, screenWidth - msgIndent - indent, msgIndent + indent));
22@@ -800,11 +801,6 @@ bool HasTestOption(const ArgsManager& args, const std::string& test_option)
23     });
24 }
25 
26-std::string HelpMessageSubOpt(std::string_view option, std::string_view help_param, std::string_view message)
27-{
28-    return HelpMessageOpt(option, help_param, message, msgIndent - optIndent);
29-}
30-
31 fs::path GetDefaultDataDir()
32 {
33     // Windows:
34--- a/src/common/args.h
35+++ b/src/common/args.h
36@@ -486,19 +486,9 @@ std::string HelpMessageGroup(const std::string& message);
37  * [@param](/bitcoin-bitcoin/contributor/param/) option Option name (e.g. "-rpcuser")
38  * [@param](/bitcoin-bitcoin/contributor/param/) help_param Help parameter (e.g. "=<user>" or "")
39  * [@param](/bitcoin-bitcoin/contributor/param/) message Option description (e.g. "Username for JSON-RPC connections")
40- * [@param](/bitcoin-bitcoin/contributor/param/) indent Additional indentation
41+ * [@param](/bitcoin-bitcoin/contributor/param/) subopt True if this is suboption, instead of a top level option.
42  * [@return](/bitcoin-bitcoin/contributor/return/) the formatted string
43  */
44-std::string HelpMessageOpt(std::string_view option, std::string_view help_param, std::string_view message, int indent=0);
45-
46-/**
47- * Same as HelpMessageOpt, but indents for command-specific options
48- *
49- * [@param](/bitcoin-bitcoin/contributor/param/) option Option name (e.g. "-rpcuser")
50- * [@param](/bitcoin-bitcoin/contributor/param/) help_param Help parameter (e.g. "=<user>" or "")
51- * [@param](/bitcoin-bitcoin/contributor/param/) message Option description (e.g. "Username for JSON-RPC connections")
52- * [@return](/bitcoin-bitcoin/contributor/return/) the formatted string
53- */
54-std::string HelpMessageSubOpt(std::string_view option, std::string_view help_param, std::string_view message);
55+std::string HelpMessageOpt(std::string_view option, std::string_view help_param, std::string_view message, bool subopt=false);
56 
57 #endif // BITCOIN_COMMON_ARGS_H

in src/common/args.h:70 in e2debfa5d3 outdated

66@@ -66,6 +67,8 @@ enum class OptionsCategory {
67     CLI_COMMANDS,
68     IPC,
69 
70+    COMMAND_OPTIONS, // Specific to one or more commands

ryanofsky commented at 4:58 pm on November 19, 2025:

In commit “ArgsManager: support command-specific options” (e2debfa5d38ef06f0676049310088c9e7cf69c34)

Might be worth noting that options with this category will be excluded from help output unless they are referenced by an AddCommand call.

Relatedly it might be good to add a check that every option in the COMMAND_OPTIONS category is actually referenced by an AddCommand call.

in src/common/args.cpp:566 in e2debfa5d3 outdated

561@@ -562,6 +562,9 @@ void ArgsManager::AddCommand(const std::string& cmd, const std::string& help)
562     m_accept_any_command = false; // latch to false
563     std::map<std::string, Arg>& arg_map = m_available_args[OptionsCategory::COMMANDS];
564     auto ret = arg_map.emplace(cmd, Arg{"", help, ArgsManager::COMMAND});
565+    if (!options.empty()) {
566+        m_command_args.try_emplace(cmd, std::move(options));

ryanofsky commented at 5:00 pm on November 19, 2025:

In commit “ArgsManager: support command-specific options” (e2debfa5d38ef06f0676049310088c9e7cf69c34)

Should the options list be validated against known options? Seems like it could be easy to make a typo and pass a bad list of options to this function, causing help output to be incomplete without it being obvious.

in src/test/argsman_tests.cpp:663 in cce7eba489

658+        if (!test_args.ParseParameters(argv.size(), argv.data(), error)) return PARSE_FAIL;
659+        if (!error.empty()) return PARSE_ERROR;
660+        const auto command = test_args.GetCommand();
661+        if (!command) return NO_COMMAND;
662+        std::vector<std::string> details;
663+        if (!test_args.CheckCommandOptions(command->command, &details)) return COMMAND_OPTS;

ryanofsky commented at 5:09 pm on November 19, 2025:

In commit “tests: Add some test coverage for ArgsManager::AddCommand” (cce7eba489d5fb8b9f603584b5939dd42ccc6e8d)

Might be good to check details is empty on success, nonempty on failure

ryanofsky approved

ryanofsky commented at 5:12 pm on November 19, 2025: contributor

Code review ACK cce7eba489d5fb8b9f603584b5939dd42ccc6e8d. Left various suggestions, but none are important so feel free to ignore.

DrahtBot requested review from l0rinc on Nov 19, 2025

ArgsManager: support subcommand-specific options #28802

Code Coverage & Benchmarks

Reviews

Conflicts

Limitations