Merge #20715: util: Add ArgsManager::GetCommand() and use it in bitcoin-wallet

fa61b9d util: Add ArgsManager::GetCommand() and use it in bitcoin-wallet (MarcoFalke)
7777105 refactor: Move all command dependend checks to ExecuteWalletToolFunc (MarcoFalke)
fa06bce test: Add tests (MarcoFalke)
fac05cc wallet: [refactor] Pass ArgsManager to WalletAppInit (MarcoFalke)

Pull request description:

  This not only moves the parsing responsibility out from the wallet tool, but it also makes it easier to implement bitcoin-util #19937

  Fixes: #20902

ACKs for top commit:
  ajtowns:
    ACK fa61b9d
  fjahr:
    Code review ACK fa61b9d

Tree-SHA512: 79622b806e8bf9dcd0dc24a8a6687345710df57720992e83a41cd8d6762a6dc112044ebc58fcf6e8fbf45de29a79b04873c5b8c2494a1eaaf902a2884703e47b

Author: MarcoFalke

src/bitcoin-wallet.cpp

@@ -33,59 +33,60 @@ static void SetupWalletToolArgs(ArgsManager& argsman)
     argsman.AddArg("-format=<format>", "The format of the wallet file to create. Either \"bdb\" or \"sqlite\". Only used with 'createfromdump'", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
     argsman.AddArg("-printtoconsole", "Send trace/debug info to console (default: 1 when no -debug is true, 0 otherwise).", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
 
-    argsman.AddArg("info", "Get wallet info", ArgsManager::ALLOW_ANY, OptionsCategory::COMMANDS);
-    argsman.AddArg("create", "Create new wallet file", ArgsManager::ALLOW_ANY, OptionsCategory::COMMANDS);
-    argsman.AddArg("salvage", "Attempt to recover private keys from a corrupt wallet. Warning: 'salvage' is experimental.", ArgsManager::ALLOW_ANY, OptionsCategory::COMMANDS);
-    argsman.AddArg("dump", "Print out all of the wallet key-value records", ArgsManager::ALLOW_ANY, OptionsCategory::COMMANDS);
-    argsman.AddArg("createfromdump", "Create new wallet file from dumped records", ArgsManager::ALLOW_ANY, OptionsCategory::COMMANDS);
+    argsman.AddCommand("info", "Get wallet info", OptionsCategory::COMMANDS);
+    argsman.AddCommand("create", "Create new wallet file", OptionsCategory::COMMANDS);
+    argsman.AddCommand("salvage", "Attempt to recover private keys from a corrupt wallet. Warning: 'salvage' is experimental.", OptionsCategory::COMMANDS);
+    argsman.AddCommand("dump", "Print out all of the wallet key-value records", OptionsCategory::COMMANDS);
+    argsman.AddCommand("createfromdump", "Create new wallet file from dumped records", OptionsCategory::COMMANDS);
 }
 
-static bool WalletAppInit(int argc, char* argv[])
+static bool WalletAppInit(ArgsManager& args, int argc, char* argv[])
 {
-    SetupWalletToolArgs(gArgs);
+    SetupWalletToolArgs(args);
     std::string error_message;
-    if (!gArgs.ParseParameters(argc, argv, error_message)) {
+    if (!args.ParseParameters(argc, argv, error_message)) {
         tfm::format(std::cerr, "Error parsing command line arguments: %s\n", error_message);
         return false;
     }
-    if (argc < 2 || HelpRequested(gArgs) || gArgs.IsArgSet("-version")) {
+    if (argc < 2 || HelpRequested(args) || args.IsArgSet("-version")) {
         std::string strUsage = strprintf("%s bitcoin-wallet version", PACKAGE_NAME) + " " + FormatFullVersion() + "\n";
-            if (!gArgs.IsArgSet("-version")) {
-                strUsage += "\n"
-                    "bitcoin-wallet is an offline tool for creating and interacting with " PACKAGE_NAME " wallet files.\n"
-                    "By default bitcoin-wallet will act on wallets in the default mainnet wallet directory in the datadir.\n"
-                    "To change the target wallet, use the -datadir, -wallet and -testnet/-regtest arguments.\n\n"
-                    "Usage:\n"
-                    "  bitcoin-wallet [options] <command>\n";
-                strUsage += "\n" + gArgs.GetHelpMessage();
-            }
+        if (!args.IsArgSet("-version")) {
+            strUsage += "\n"
+                        "bitcoin-wallet is an offline tool for creating and interacting with " PACKAGE_NAME " wallet files.\n"
+                        "By default bitcoin-wallet will act on wallets in the default mainnet wallet directory in the datadir.\n"
+                        "To change the target wallet, use the -datadir, -wallet and -testnet/-regtest arguments.\n\n"
+                        "Usage:\n"
+                        "  bitcoin-wallet [options] <command>\n";
+            strUsage += "\n" + args.GetHelpMessage();
+        }
         tfm::format(std::cout, "%s", strUsage);
         return false;
     }
 
     // check for printtoconsole, allow -debug
-    LogInstance().m_print_to_console = gArgs.GetBoolArg("-printtoconsole", gArgs.GetBoolArg("-debug", false));
+    LogInstance().m_print_to_console = args.GetBoolArg("-printtoconsole", args.GetBoolArg("-debug", false));
 
     if (!CheckDataDirOption()) {
-        tfm::format(std::cerr, "Error: Specified data directory \"%s\" does not exist.\n", gArgs.GetArg("-datadir", ""));
+        tfm::format(std::cerr, "Error: Specified data directory \"%s\" does not exist.\n", args.GetArg("-datadir", ""));
         return false;
     }
     // Check for chain settings (Params() calls are only valid after this clause)
-    SelectParams(gArgs.GetChainName());
+    SelectParams(args.GetChainName());
 
     return true;
 }
 
 int main(int argc, char* argv[])
 {
+    ArgsManager& args = gArgs;
 #ifdef WIN32
     util::WinCmdLineArgs winArgs;
     std::tie(argc, argv) = winArgs.get();
 #endif
     SetupEnvironment();
     RandomInit();
     try {
-        if (!WalletAppInit(argc, argv)) return EXIT_FAILURE;
+        if (!WalletAppInit(args, argc, argv)) return EXIT_FAILURE;
     } catch (const std::exception& e) {
         PrintExceptionContinue(&e, "WalletAppInit()");
         return EXIT_FAILURE;
@@ -94,33 +95,19 @@ int main(int argc, char* argv[])
         return EXIT_FAILURE;
     }
 
-    std::string method {};
-    for(int i = 1; i < argc; ++i) {
-        if (!IsSwitchChar(argv[i][0])) {
-            if (!method.empty()) {
-                tfm::format(std::cerr, "Error: two methods provided (%s and %s). Only one method should be provided.\n", method, argv[i]);
-                return EXIT_FAILURE;
-            }
-            method = argv[i];
-        }
-    }
-
-    if (method.empty()) {
+    const auto command = args.GetCommand();
+    if (!command) {
         tfm::format(std::cerr, "No method provided. Run `bitcoin-wallet -help` for valid methods.\n");
         return EXIT_FAILURE;
     }
-
-    // A name must be provided when creating a file
-    if (method == "create" && !gArgs.IsArgSet("-wallet")) {
-        tfm::format(std::cerr, "Wallet name must be provided when creating a new wallet.\n");
+    if (command->args.size() != 0) {
+        tfm::format(std::cerr, "Error: Additional arguments provided (%s). Methods do not take arguments. Please refer to `-help`.\n", Join(command->args, ", "));
         return EXIT_FAILURE;
     }
 
-    std::string name = gArgs.GetArg("-wallet", "");
-
     ECCVerifyHandle globalVerifyHandle;
     ECC_Start();
-    if (!WalletTool::ExecuteWalletToolFunc(gArgs, method, name)) {
+    if (!WalletTool::ExecuteWalletToolFunc(args, command->command)) {
         return EXIT_FAILURE;
     }
     ECC_Stop();

src/test/fuzz/system.cpp

@@ -54,7 +54,7 @@ FUZZ_TARGET(system)
                 if (args_manager.GetArgFlags(argument_name) != nullopt) {
                     return;
                 }
-                args_manager.AddArg(argument_name, fuzzed_data_provider.ConsumeRandomLengthString(16), fuzzed_data_provider.ConsumeIntegral<unsigned int>(), options_category);
+                args_manager.AddArg(argument_name, fuzzed_data_provider.ConsumeRandomLengthString(16), fuzzed_data_provider.ConsumeIntegral<unsigned int>() & ~ArgsManager::COMMAND, options_category);
             },
             [&] {
                 // Avoid hitting:

src/util/system.cpp

@@ -3,14 +3,15 @@
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
-#include <sync.h>
 #include <util/system.h>
 
 #ifdef HAVE_BOOST_PROCESS
 #include <boost/process.hpp>
 #endif // HAVE_BOOST_PROCESS
 
 #include <chainparamsbase.h>
+#include <sync.h>
+#include <util/check.h>
 #include <util/strencodings.h>
 #include <util/string.h>
 #include <util/translation.h>
@@ -310,8 +311,22 @@ bool ArgsManager::ParseParameters(int argc, const char* const argv[], std::strin
             key[0] = '-';
 #endif
 
-        if (key[0] != '-')
+        if (key[0] != '-') {
+            if (!m_accept_any_command && m_command.empty()) {
+                // The first non-dash arg is a registered command
+                Optional<unsigned int> flags = GetArgFlags(key);
+                if (!flags || !(*flags & ArgsManager::COMMAND)) {
+                    error = strprintf("Invalid command '%s'", argv[i]);
+                    return false;
+                }
+            }
+            m_command.push_back(key);
+            while (++i < argc) {
+                // The remaining args are command args
+                m_command.push_back(argv[i]);
+            }
             break;
+        }
 
         // Transform --foo to -foo
         if (key.length() > 1 && key[1] == '-')
@@ -359,6 +374,26 @@ Optional<unsigned int> ArgsManager::GetArgFlags(const std::string& name) const
     return nullopt;
 }
 
+std::optional<const ArgsManager::Command> ArgsManager::GetCommand() const
+{
+    Command ret;
+    LOCK(cs_args);
+    auto it = m_command.begin();
+    if (it == m_command.end()) {
+        // No command was passed
+        return std::nullopt;
+    }
+    if (!m_accept_any_command) {
+        // The registered command
+        ret.command = *(it++);
+    }
+    while (it != m_command.end()) {
+        // The unregistered command and args (if any)
+        ret.args.push_back(*(it++));
+    }
+    return ret;
+}
+
 std::vector<std::string> ArgsManager::GetArgs(const std::string& strArg) const
 {
     std::vector<std::string> result;
@@ -504,8 +539,22 @@ void ArgsManager::ForceSetArg(const std::string& strArg, const std::string& strV
     m_settings.forced_settings[SettingName(strArg)] = strValue;
 }
 
+void ArgsManager::AddCommand(const std::string& cmd, const std::string& help, const OptionsCategory& cat)
+{
+    Assert(cmd.find('=') == std::string::npos);
+    Assert(cmd.at(0) != '-');
+
+    LOCK(cs_args);
+    m_accept_any_command = false; // latch to false
+    std::map<std::string, Arg>& arg_map = m_available_args[cat];
+    auto ret = arg_map.emplace(cmd, Arg{"", help, ArgsManager::COMMAND});
+    Assert(ret.second); // Fail on duplicate commands
+}
+
 void ArgsManager::AddArg(const std::string& name, const std::string& help, unsigned int flags, const OptionsCategory& cat)
 {
+    Assert((flags & ArgsManager::COMMAND) == 0); // use AddCommand
+
     // Split arg name from its help param
     size_t eq_index = name.find('=');
     if (eq_index == std::string::npos) {

src/util/system.h

@@ -181,6 +181,7 @@ class ArgsManager
         NETWORK_ONLY = 0x200,
         // This argument's value is sensitive (such as a password).
         SENSITIVE = 0x400,
+        COMMAND = 0x800,
     };
 
 protected:
@@ -193,9 +194,11 @@ class ArgsManager
 
     mutable RecursiveMutex cs_args;
     util::Settings m_settings GUARDED_BY(cs_args);
+    std::vector<std::string> m_command GUARDED_BY(cs_args);
     std::string m_network GUARDED_BY(cs_args);
     std::set<std::string> m_network_only_args GUARDED_BY(cs_args);
     std::map<OptionsCategory, std::map<std::string, Arg>> m_available_args GUARDED_BY(cs_args);
+    bool m_accept_any_command GUARDED_BY(cs_args){true};
     std::list<SectionInfo> m_config_sections GUARDED_BY(cs_args);
 
     [[nodiscard]] bool ReadConfigStream(std::istream& stream, const std::string& filepath, std::string& error, bool ignore_invalid_keys = false);
@@ -246,6 +249,20 @@ class ArgsManager
      */
     const std::list<SectionInfo> GetUnrecognizedSections() const;
 
+    struct Command {
+        /** The command (if one has been registered with AddCommand), or empty */
+        std::string command;
+        /**
+         * If command is non-empty: Any args that followed it
+         * If command is empty: The unregistered command and any args that followed it
+         */
+        std::vector<std::string> args;
+    };
+    /**
+     * Get the command and command args (returns std::nullopt if no command provided)
+     */
+    std::optional<const Command> GetCommand() const;
+
     /**
      * Return a vector of strings of the given argument
      *
@@ -331,6 +348,11 @@ class ArgsManager
      */
     void AddArg(const std::string& name, const std::string& help, unsigned int flags, const OptionsCategory& cat);
 
+    /**
+     * Add subcommand
+     */
+    void AddCommand(const std::string& cmd, const std::string& help, const OptionsCategory& cat);
+
     /**
      * Add many hidden arguments
      */

src/wallet/wallettool.cpp

@@ -103,10 +103,8 @@ static void WalletShowInfo(CWallet* wallet_instance)
     tfm::format(std::cout, "Address Book: %zu\n", wallet_instance->m_address_book.size());
 }
 
-bool ExecuteWalletToolFunc(const ArgsManager& args, const std::string& command, const std::string& name)
+bool ExecuteWalletToolFunc(const ArgsManager& args, const std::string& command)
 {
-    const fs::path path = fsbridge::AbsPathJoin(GetWalletDir(), name);
-
     if (args.IsArgSet("-format") && command != "createfromdump") {
         tfm::format(std::cerr, "The -format option can only be used with the \"createfromdump\" command.\n");
         return false;
@@ -119,6 +117,12 @@ bool ExecuteWalletToolFunc(const ArgsManager& args, const std::string& command,
         tfm::format(std::cerr, "The -descriptors option can only be used with the 'create' command.\n");
         return false;
     }
+    if (command == "create" && !args.IsArgSet("-wallet")) {
+        tfm::format(std::cerr, "Wallet name must be provided when creating a new wallet.\n");
+        return false;
+    }
+    const std::string name = args.GetArg("-wallet", "");
+    const fs::path path = fsbridge::AbsPathJoin(GetWalletDir(), name);
 
     if (command == "create") {
         DatabaseOptions options;

src/wallet/wallettool.h

@@ -10,7 +10,7 @@
 namespace WalletTool {
 
 void WalletShowInfo(CWallet* wallet_instance);
-bool ExecuteWalletToolFunc(const ArgsManager& args, const std::string& command, const std::string& file);
+bool ExecuteWalletToolFunc(const ArgsManager& args, const std::string& command);
 
 } // namespace WalletTool
 

test/functional/tool_wallet.py

@@ -183,11 +183,13 @@ def do_tool_createfromdump(self, wallet_name, dumpfile, file_format=None):
 
     def test_invalid_tool_commands_and_args(self):
         self.log.info('Testing that various invalid commands raise with specific error messages')
-        self.assert_raises_tool_error('Invalid command: foo', 'foo')
+        self.assert_raises_tool_error("Error parsing command line arguments: Invalid command 'foo'", 'foo')
         # `bitcoin-wallet help` raises an error. Use `bitcoin-wallet -help`.
-        self.assert_raises_tool_error('Invalid command: help', 'help')
-        self.assert_raises_tool_error('Error: two methods provided (info and create). Only one method should be provided.', 'info', 'create')
+        self.assert_raises_tool_error("Error parsing command line arguments: Invalid command 'help'", 'help')
+        self.assert_raises_tool_error('Error: Additional arguments provided (create). Methods do not take arguments. Please refer to `-help`.', 'info', 'create')
         self.assert_raises_tool_error('Error parsing command line arguments: Invalid parameter -foo', '-foo')
+        self.assert_raises_tool_error('No method provided. Run `bitcoin-wallet -help` for valid methods.')
+        self.assert_raises_tool_error('Wallet name must be provided when creating a new wallet.', 'create')
         locked_dir = os.path.join(self.options.tmpdir, "node0", "regtest", "wallets")
         error = 'Error initializing wallet database environment "{}"!'.format(locked_dir)
         if self.options.descriptors: