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

Reviewers, this pull request conflicts with the following ones:

#33229 (multiprocess: Don’t require bitcoin -m argument when IPC options are used 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.

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:221 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:366 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

ArgsManager: support command-specific options a8c6d63e3a

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

bitcoin-wallet: use command-specific options 1b89dcb26b

ArgsManager: automate checking for correct command options 0d1d11fc02

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

DrahtBot commented at 11:24 pm on September 25, 2025: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

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:69 in 0d1d11fc02

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

in src/common/args.h:353 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?

in src/common/args.h:142 in 0d1d11fc02

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?

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

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:651 in 0d1d11fc02

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.

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:655 in 0d1d11fc02

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

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?

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 an empty

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.

ArgsManager: support subcommand-specific options #28802

Code Coverage & Benchmarks

Reviews

Conflicts