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

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
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:

#31250 (wallet: Disable creating and loading legacy wallets by achow101)
#29838 (Feature: Use different datadirs for different signets by BrandonOdiwuor)
#28710 (Remove the legacy wallet and BDB dependency by achow101)

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.

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:738 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:220 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 commented at 11:30 pm on June 13, 2024: contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/22587328812

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

ArgsManager: support command-specific options 08160c5931

bitcoin-wallet: use command-specific options 9504ba1385

ArgsManager: automate checking for correct command options 80c0927769

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

ArgsManager: support subcommand-specific options #28802

Code Coverage

Reviews

Conflicts