diff --git a/src/bitcoin-wallet.cpp b/src/bitcoin-wallet.cpp index 9b0136da4c4..f72d33b0bc5 100644 --- a/src/bitcoin-wallet.cpp +++ b/src/bitcoin-wallet.cpp @@ -37,14 +37,14 @@ static void SetupWalletToolArgs(ArgsManager& argsman) argsman.AddArg("-version", "Print version and exit", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-datadir=", "Specify data directory", ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_NEGATION, OptionsCategory::OPTIONS); argsman.AddArg("-wallet=", "Specify wallet name", ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::OPTIONS); - argsman.AddArg("-dumpfile=", "When used with 'dump', writes out the records to this file. When used with 'createfromdump', loads the records into a new wallet.", ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_NEGATION, OptionsCategory::OPTIONS); + argsman.AddArg("-dumpfile=", "When used with 'dump', writes out the records to this file. When used with 'createfromdump', loads the records into a new wallet.", ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_NEGATION, OptionsCategory::COMMAND_OPTIONS); argsman.AddArg("-debug=", "Output debugging information (default: 0).", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST); 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.AddCommand("info", "Get wallet info"); argsman.AddCommand("create", "Create a new descriptor wallet file"); - argsman.AddCommand("dump", "Print out all of the wallet key-value records"); - argsman.AddCommand("createfromdump", "Create new wallet file from dumped records"); + argsman.AddCommand("dump", "Print out all of the wallet key-value records", {"-dumpfile"}); + argsman.AddCommand("createfromdump", "Create new wallet file from dumped records", {"-dumpfile"}); } static std::optional WalletAppInit(ArgsManager& args, int argc, char* argv[]) diff --git a/src/common/args.cpp b/src/common/args.cpp index 12e0f26ed21..e153d886f9c 100644 --- a/src/common/args.cpp +++ b/src/common/args.cpp @@ -387,6 +387,40 @@ std::optional ArgsManager::GetCommand() const return ret; } +bool ArgsManager::CheckCommandOptions(const std::string& command, std::vector* errors) const +{ + LOCK(cs_args); + + auto command_options = m_available_args.find(OptionsCategory::COMMAND_OPTIONS); + if (command_options == m_available_args.end()) { + // There are no command-specific options at all, so everything is fine + return true; + } + + const auto command_args = m_command_args.find(command); + auto is_valid_opt = [&](const auto& opt) EXCLUSIVE_LOCKS_REQUIRED(cs_args) -> bool { + if (command_args == m_command_args.end()) { + // Caller may not have checked that command actually exists + // before calling this function. In that case, treat it as + // having no valid command-specific options. + return false; + } else { + return command_args->second.contains(opt); + } + }; + + bool ok = true; + for (const auto& [arg, _] : command_options->second) { + if (!GetSetting_(arg).isNull() && !is_valid_opt(arg)) { + ok = false; + if (errors != nullptr) { + errors->emplace_back(strprintf("The %s option cannot be used with the '%s' command.", arg, command)); + } + } + } + return ok; +} + std::vector ArgsManager::GetArgs(const std::string& strArg) const { std::vector result; @@ -598,7 +632,7 @@ 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) +void ArgsManager::AddCommand(const std::string& cmd, const std::string& help, std::set options) { Assert(cmd.find('=') == std::string::npos); Assert(cmd.at(0) != '-'); @@ -607,6 +641,18 @@ void ArgsManager::AddCommand(const std::string& cmd, const std::string& help) m_accept_any_command = false; // latch to false std::map& arg_map = m_available_args[OptionsCategory::COMMANDS]; auto ret = arg_map.emplace(cmd, Arg{"", help, ArgsManager::COMMAND}); + if (!options.empty()) { + auto& cmdopts = m_available_args[OptionsCategory::COMMAND_OPTIONS]; + bool command_has_all_options_defined = true; + for (const auto& opt : options) { + if (!cmdopts.contains(opt)) { + command_has_all_options_defined = false; + } + } + Assert(command_has_all_options_defined); + + m_command_args.try_emplace(cmd, std::move(options)); + } Assert(ret.second); // Fail on duplicate commands } @@ -643,6 +689,7 @@ void ArgsManager::ClearArgs() LOCK(cs_args); m_settings = {}; m_available_args.clear(); + m_command_args.clear(); m_network_only_args.clear(); m_config_sections.clear(); } @@ -670,8 +717,20 @@ std::string ArgsManager::GetHelpMessage() const std::string usage; LOCK(cs_args); - for (const auto& arg_map : m_available_args) { - switch(arg_map.first) { + + const auto command_options = m_available_args.find(OptionsCategory::COMMAND_OPTIONS); + const auto for_matching_cmd_opts = [&](const std::set& select, auto&& fn) EXCLUSIVE_LOCKS_REQUIRED(cs_args) { + if (select.empty()) return; + if (command_options == m_available_args.end()) return; + for (const auto& [name, info] : command_options->second) { + if (!show_debug && (info.m_flags & ArgsManager::DEBUG_ONLY)) continue; + if (!select.contains(name)) continue; + fn(name, info); + } + }; + + for (const auto& [category, category_args] : m_available_args) { + switch(category) { case OptionsCategory::OPTIONS: usage += HelpMessageGroup("Options:"); break; @@ -717,22 +776,27 @@ std::string ArgsManager::GetHelpMessage() const case OptionsCategory::CLI_COMMANDS: usage += HelpMessageGroup("CLI Commands:"); break; + case OptionsCategory::COMMAND_OPTIONS: case OptionsCategory::HIDDEN: break; } // no default case, so the compiler can warn about missing cases - // When we get to the hidden options, stop - if (arg_map.first == OptionsCategory::HIDDEN) break; + if (category == OptionsCategory::COMMAND_OPTIONS) continue; - for (const auto& arg : arg_map.second) { - if (show_debug || !(arg.second.m_flags & ArgsManager::DEBUG_ONLY)) { - std::string name; - if (arg.second.m_help_param.empty()) { - name = arg.first; - } else { - name = arg.first + arg.second.m_help_param; + // When we get to the hidden options, stop + if (category == OptionsCategory::HIDDEN) break; + + for (const auto& [arg_name, arg_info] : category_args) { + if (show_debug || !(arg_info.m_flags & ArgsManager::DEBUG_ONLY)) { + usage += HelpMessageOpt(arg_name, arg_info.m_help_param, arg_info.m_help_text); + + if (category == OptionsCategory::COMMANDS) { + const auto cmd_args = m_command_args.find(arg_name); + if (cmd_args == m_command_args.end()) continue; + for_matching_cmd_opts(cmd_args->second, [&](const auto& cmdopt_name, const auto& cmdopt_info) { + usage += HelpMessageOpt(cmdopt_name, cmdopt_info.m_help_param, cmdopt_info.m_help_text, /*subopt=*/true); + }); } - usage += HelpMessageOpt(name, arg.second.m_help_text); } } } @@ -750,19 +814,26 @@ void SetupHelpOptions(ArgsManager& args) args.AddHiddenArgs({"-h", "-?"}); } -static const int screenWidth = 79; -static const int optIndent = 2; -static const int msgIndent = 7; - std::string HelpMessageGroup(const std::string &message) { return std::string(message) + std::string("\n\n"); } -std::string HelpMessageOpt(const std::string &option, const std::string &message) { - return std::string(optIndent,' ') + std::string(option) + - std::string("\n") + std::string(msgIndent,' ') + - FormatParagraph(message, screenWidth - msgIndent, msgIndent) + - std::string("\n\n"); +std::string HelpMessageOpt(std::string_view option, std::string_view help_param, std::string_view message, bool subopt) +{ + constexpr int screen_width = 79; + int opt_indent = 2; + int msg_indent = 7; + + if (subopt) { + int bump = msg_indent - opt_indent; + opt_indent += bump; // opt_indent now at the old msg_indent level + msg_indent += bump; // indent by the same amount + } + int msg_width = screen_width - msg_indent; + + return strprintf("%*s%s%s\n%*s%s\n\n", + opt_indent, "", option, help_param, + msg_indent, "", FormatParagraph(message, msg_width, msg_indent)); } const std::vector TEST_OPTIONS_DOC{ diff --git a/src/common/args.h b/src/common/args.h index de323d3c490..0a3195f89d6 100644 --- a/src/common/args.h +++ b/src/common/args.h @@ -19,6 +19,7 @@ #include #include #include +#include #include #include @@ -67,6 +68,10 @@ enum class OptionsCategory { CLI_COMMANDS, IPC, + // Specific to one or more commands (OptionsCategory::COMMANDS) + // These are only included in help with their associated commands. + COMMAND_OPTIONS, + HIDDEN // Always the last option to avoid printing these in the help }; @@ -142,6 +147,7 @@ private: std::set m_network_only_args GUARDED_BY(cs_args); std::map> m_available_args GUARDED_BY(cs_args); std::optional m_default_flags GUARDED_BY(cs_args){}; + std::map> m_command_args GUARDED_BY(cs_args); bool m_accept_any_command GUARDED_BY(cs_args){true}; std::list m_config_sections GUARDED_BY(cs_args); std::optional m_config_path GUARDED_BY(cs_args); @@ -219,6 +225,16 @@ public: */ std::optional GetCommand() const EXCLUSIVE_LOCKS_REQUIRED(!cs_args); + /** + * Check that any command-specific options the user specified are valid + * for the given command. + * + * @param[in] command The command being run. + * @param[out] errors If non-null, populated with a message for each invalid option. + * @return false if any command-specific options were specified that are not valid for this command + */ + bool CheckCommandOptions(const std::string& command, std::vector* errors = nullptr) const EXCLUSIVE_LOCKS_REQUIRED(!cs_args); + /** * Get blocks directory path * @@ -360,9 +376,9 @@ public: void AddArg(const std::string& name, const std::string& help, unsigned int flags, const OptionsCategory& cat) EXCLUSIVE_LOCKS_REQUIRED(!cs_args); /** - * Add subcommand + * Add command */ - void AddCommand(const std::string& cmd, const std::string& help) EXCLUSIVE_LOCKS_REQUIRED(!cs_args); + void AddCommand(const std::string& cmd, const std::string& help, std::set options = {}) EXCLUSIVE_LOCKS_REQUIRED(!cs_args); /** * Add many hidden arguments @@ -491,10 +507,12 @@ std::string HelpMessageGroup(const std::string& message); /** * Format a string to be used as option description in help messages * - * @param option Option message (e.g. "-rpcuser=") + * @param option Option name (e.g. "-rpcuser") + * @param help_param Help parameter (e.g. "=" or "") * @param message Option description (e.g. "Username for JSON-RPC connections") + * @param subopt True if this is a suboption, instead of a top-level option. * @return the formatted string */ -std::string HelpMessageOpt(const std::string& option, const std::string& message); +std::string HelpMessageOpt(std::string_view option, std::string_view help_param, std::string_view message, bool subopt = false); #endif // BITCOIN_COMMON_ARGS_H diff --git a/src/test/argsman_tests.cpp b/src/test/argsman_tests.cpp index c8802d3641c..da0d6840504 100644 --- a/src/test/argsman_tests.cpp +++ b/src/test/argsman_tests.cpp @@ -634,6 +634,82 @@ BOOST_AUTO_TEST_CASE(util_GetArg) BOOST_CHECK_EQUAL(testArgs.GetArg("pritest4", "default"), "b"); } +BOOST_AUTO_TEST_CASE(util_AddCommand) +{ + enum TestFail { SUCCESS, + PARSE_FAIL, + PARSE_ERROR, + NO_COMMAND, + COMMAND_OPTS_BAD_DETAILS, + COMMAND_OPTS }; + + auto testfn = [&](const auto& argv) -> TestFail { + TestArgsManager test_args; + test_args.AddArg("-opt1=", "Opt 1", ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_NEGATION, OptionsCategory::COMMAND_OPTIONS); + test_args.AddArg("-opt2=", "Opt 2", ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_NEGATION, OptionsCategory::COMMAND_OPTIONS); + test_args.AddArg("-opt3=", "Opt 3", ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_NEGATION, OptionsCategory::OPTIONS); + + test_args.AddCommand("cmd1", "No specific options"); + test_args.AddCommand("cmd2", "Opt 1", {"-opt1"}); + test_args.AddCommand("cmd3", "Opt 1 or 2", {"-opt1", "-opt2"}); + + std::string error; + if (!test_args.ParseParameters(argv.size(), argv.data(), error)) return PARSE_FAIL; + if (!error.empty()) return PARSE_ERROR; + const auto command = test_args.GetCommand(); + if (!command) return NO_COMMAND; + std::vector details; + if (!test_args.CheckCommandOptions(command->command, &details)) { + if (details.empty()) return COMMAND_OPTS_BAD_DETAILS; + return COMMAND_OPTS; + } else if (!details.empty()) { + return COMMAND_OPTS_BAD_DETAILS; + } + return SUCCESS; + }; + + BOOST_CHECK_EQUAL(COMMAND_OPTS, testfn(std::array{"x", "-opt1=foo", "cmd1"})); + BOOST_CHECK_EQUAL(SUCCESS, testfn(std::array{"x", "cmd1", "-opt1=foo"})); // things after the command are "args" and left unparsed, not options + + BOOST_CHECK_EQUAL(SUCCESS, testfn(std::array{"x", "-opt1=foo", "cmd2"})); + BOOST_CHECK_EQUAL(SUCCESS, testfn(std::array{"x", "-opt1=foo", "cmd3"})); + BOOST_CHECK_EQUAL(COMMAND_OPTS, testfn(std::array{"x", "-opt2=foo", "cmd1"})); + BOOST_CHECK_EQUAL(COMMAND_OPTS, testfn(std::array{"x", "-opt2=foo", "cmd2"})); + BOOST_CHECK_EQUAL(SUCCESS, testfn(std::array{"x", "-opt2=foo", "cmd3"})); + BOOST_CHECK_EQUAL(SUCCESS, testfn(std::array{"x", "-opt3=foo", "cmd1"})); + BOOST_CHECK_EQUAL(SUCCESS, testfn(std::array{"x", "-opt3=foo", "cmd2"})); + BOOST_CHECK_EQUAL(SUCCESS, testfn(std::array{"x", "-opt3=foo", "cmd3"})); + + BOOST_CHECK_EQUAL(COMMAND_OPTS, testfn(std::array{"x", "-opt1=foo", "-opt3=bar", "-opt2=baz", "cmd1"})); + BOOST_CHECK_EQUAL(COMMAND_OPTS, testfn(std::array{"x", "-opt1=foo", "-opt3=bar", "-opt2=baz", "cmd2"})); + BOOST_CHECK_EQUAL(SUCCESS, testfn(std::array{"x", "-opt1=foo", "-opt3=bar", "-opt2=baz", "cmd3"})); + + BOOST_CHECK_EQUAL(PARSE_FAIL, testfn(std::array{"x", "cmd4"})); + BOOST_CHECK_EQUAL(NO_COMMAND, testfn(std::array{"x", "-opt3=foo"})); + BOOST_CHECK_EQUAL(PARSE_FAIL, testfn(std::array{"x", "-opt4=foo"})); +} + +BOOST_AUTO_TEST_CASE(util_AddCommand_clearargs_replaces_command_options) +{ + const auto add_command{[&](TestArgsManager& test_args, const std::string& option) { + test_args.AddArg(option, "option", ArgsManager::ALLOW_ANY, OptionsCategory::COMMAND_OPTIONS); + test_args.AddCommand("cmd", "cmd", {option}); + }}; + + TestArgsManager test_args; + add_command(test_args, "-opt1"); + test_args.ClearArgs(); + add_command(test_args, "-opt2"); + + const auto help{test_args.GetHelpMessage()}; + BOOST_CHECK(help.find("-opt2") != std::string::npos); + + test_args.ForceSetArg("-opt2", "1"); + std::vector details; + BOOST_CHECK(test_args.CheckCommandOptions("cmd", &details)); + BOOST_CHECK(details.empty()); +} + BOOST_AUTO_TEST_CASE(util_GetChainTypeString) { TestArgsManager test_args; diff --git a/src/test/fuzz/string.cpp b/src/test/fuzz/string.cpp index a7f8516f111..02292774c17 100644 --- a/src/test/fuzz/string.cpp +++ b/src/test/fuzz/string.cpp @@ -67,7 +67,8 @@ FUZZ_TARGET(string) (void)HelpExampleCli(random_string_1, random_string_2); (void)HelpExampleRpc(random_string_1, random_string_2); (void)HelpMessageGroup(random_string_1); - (void)HelpMessageOpt(random_string_1, random_string_2); + (void)HelpMessageOpt(random_string_1, "", random_string_2); + (void)HelpMessageOpt(random_string_1, random_string_2, ""); (void)IsDeprecatedRPCEnabled(random_string_1); (void)Join(random_string_vector, random_string_1); (void)JSONRPCError(fuzzed_data_provider.ConsumeIntegral(), random_string_1); diff --git a/src/test/fuzz/system.cpp b/src/test/fuzz/system.cpp index 12c70c7a10b..9d0978ce780 100644 --- a/src/test/fuzz/system.cpp +++ b/src/test/fuzz/system.cpp @@ -9,6 +9,7 @@ #include #include +#include #include #include @@ -31,6 +32,7 @@ FUZZ_TARGET(system, .init = initialize_system) { FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size()); ArgsManager args_manager{}; + std::vector command_option_names; if (fuzzed_data_provider.ConsumeBool()) { SetupHelpOptions(args_manager); @@ -59,7 +61,7 @@ FUZZ_TARGET(system, .init = initialize_system) args_manager.SoftSetBoolArg(str_arg, f_value); }, [&] { - const OptionsCategory options_category = fuzzed_data_provider.PickValueInArray({OptionsCategory::OPTIONS, OptionsCategory::CONNECTION, OptionsCategory::WALLET, OptionsCategory::WALLET_DEBUG_TEST, OptionsCategory::ZMQ, OptionsCategory::DEBUG_TEST, OptionsCategory::CHAINPARAMS, OptionsCategory::NODE_RELAY, OptionsCategory::BLOCK_CREATION, OptionsCategory::RPC, OptionsCategory::GUI, OptionsCategory::COMMANDS, OptionsCategory::REGISTER_COMMANDS, OptionsCategory::CLI_COMMANDS, OptionsCategory::IPC, OptionsCategory::HIDDEN}); + const OptionsCategory options_category = fuzzed_data_provider.PickValueInArray({OptionsCategory::OPTIONS, OptionsCategory::CONNECTION, OptionsCategory::WALLET, OptionsCategory::WALLET_DEBUG_TEST, OptionsCategory::ZMQ, OptionsCategory::DEBUG_TEST, OptionsCategory::CHAINPARAMS, OptionsCategory::NODE_RELAY, OptionsCategory::BLOCK_CREATION, OptionsCategory::RPC, OptionsCategory::GUI, OptionsCategory::COMMANDS, OptionsCategory::REGISTER_COMMANDS, OptionsCategory::CLI_COMMANDS, OptionsCategory::IPC, OptionsCategory::COMMAND_OPTIONS, OptionsCategory::HIDDEN}); // Avoid hitting: // common/args.cpp:563: void ArgsManager::AddArg(const std::string &, const std::string &, unsigned int, const OptionsCategory &): Assertion `ret.second' failed. const std::string argument_name = GetArgumentName(fuzzed_data_provider.ConsumeRandomLengthString(16)); @@ -69,6 +71,22 @@ FUZZ_TARGET(system, .init = initialize_system) auto help = fuzzed_data_provider.ConsumeRandomLengthString(16); auto flags = fuzzed_data_provider.ConsumeIntegral() & ~ArgsManager::COMMAND; args_manager.AddArg(argument_name, help, flags, options_category); + if (options_category == OptionsCategory::COMMAND_OPTIONS) { + command_option_names.push_back(argument_name); + } + }, + [&] { + auto cmd = fuzzed_data_provider.ConsumeRandomLengthString(16); + if (cmd.empty() || cmd[0] == '-' || cmd.find('=') != std::string::npos) return; + if (args_manager.GetArgFlags(cmd) != std::nullopt) return; + auto help = fuzzed_data_provider.ConsumeRandomLengthString(16); + std::set options; + for (const auto& opt : command_option_names) { + if (fuzzed_data_provider.ConsumeBool()) { + options.insert(opt); + } + } + args_manager.AddCommand(cmd, help, std::move(options)); }, [&] { // Avoid hitting: @@ -89,6 +107,7 @@ FUZZ_TARGET(system, .init = initialize_system) }, [&] { args_manager.ClearArgs(); + command_option_names.clear(); }, [&] { const std::vector random_arguments = ConsumeRandomLengthStringVector(fuzzed_data_provider); @@ -120,6 +139,10 @@ FUZZ_TARGET(system, .init = initialize_system) } catch (const std::runtime_error&) { } (void)args_manager.GetHelpMessage(); + const auto command = args_manager.GetCommand(); + if (command) { + (void)args_manager.CheckCommandOptions(command->command); + } (void)args_manager.GetUnrecognizedSections(); (void)args_manager.GetUnsuitableSectionOnlyArgs(); (void)args_manager.IsArgNegated(s1); diff --git a/src/wallet/wallettool.cpp b/src/wallet/wallettool.cpp index d1f3a8ce982..336bc45cacf 100644 --- a/src/wallet/wallettool.cpp +++ b/src/wallet/wallettool.cpp @@ -93,9 +93,12 @@ static void WalletShowInfo(CWallet* wallet_instance) bool ExecuteWalletToolFunc(const ArgsManager& args, const std::string& command) { - if (args.IsArgSet("-dumpfile") && command != "dump" && command != "createfromdump") { - tfm::format(std::cerr, "The -dumpfile option can only be used with the \"dump\" and \"createfromdump\" commands.\n"); - return false; + { + std::vector details; + if (!args.CheckCommandOptions(command, &details)) { + tfm::format(std::cerr, "Error: Invalid arguments provided:\n%s\n", util::MakeUnorderedList(details)); + return false; + } } if ((command == "create" || command == "createfromdump") && !args.IsArgSet("-wallet")) { tfm::format(std::cerr, "Wallet name must be provided when creating a new wallet.\n"); diff --git a/test/functional/tool_wallet.py b/test/functional/tool_wallet.py index 737aaf54d16..845d7a9a236 100755 --- a/test/functional/tool_wallet.py +++ b/test/functional/tool_wallet.py @@ -421,6 +421,7 @@ class ToolWalletTest(BitcoinTestFramework): assert not (self.nodes[0].wallets_path / "legacy").exists() self.assert_raises_tool_error("Invalid parameter -descriptors", "-wallet=legacy", "-descriptors=false", "create") assert not (self.nodes[0].wallets_path / "legacy").exists() + self.assert_raises_tool_error("The -dumpfile option cannot be used with the 'create' command.", "-wallet=legacy", "-dumpfile=wallet.dump", "create") def test_no_create_unnamed(self): self.log.info("Test that unnamed (default) wallets cannot be created")