From fac05ccdade8b34c969b9cd9b37b355bc0aabf9c Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Fri, 18 Dec 2020 15:14:08 +0100 Subject: [PATCH 1/4] wallet: [refactor] Pass ArgsManager to WalletAppInit --- src/bitcoin-wallet.cpp | 41 +++++++++++++++++++++-------------------- 1 file changed, 21 insertions(+), 20 deletions(-) diff --git a/src/bitcoin-wallet.cpp b/src/bitcoin-wallet.cpp index 3e8e5fc7bc5..435bf289b44 100644 --- a/src/bitcoin-wallet.cpp +++ b/src/bitcoin-wallet.cpp @@ -40,44 +40,45 @@ static void SetupWalletToolArgs(ArgsManager& argsman) argsman.AddArg("createfromdump", "Create new wallet file from dumped records", ArgsManager::ALLOW_ANY, 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] \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] \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(); @@ -85,7 +86,7 @@ int main(int argc, char* argv[]) 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; @@ -111,16 +112,16 @@ int main(int argc, char* argv[]) } // A name must be provided when creating a file - if (method == "create" && !gArgs.IsArgSet("-wallet")) { + if (method == "create" && !args.IsArgSet("-wallet")) { tfm::format(std::cerr, "Wallet name must be provided when creating a new wallet.\n"); return EXIT_FAILURE; } - std::string name = gArgs.GetArg("-wallet", ""); + std::string name = args.GetArg("-wallet", ""); ECCVerifyHandle globalVerifyHandle; ECC_Start(); - if (!WalletTool::ExecuteWalletToolFunc(gArgs, method, name)) { + if (!WalletTool::ExecuteWalletToolFunc(args, method, name)) { return EXIT_FAILURE; } ECC_Stop(); From fa06bce4ac17f93decd4ee38c956e7aa55983f0d Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Thu, 21 Jan 2021 13:57:59 +0100 Subject: [PATCH 2/4] test: Add tests --- test/functional/tool_wallet.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/functional/tool_wallet.py b/test/functional/tool_wallet.py index 8a1af24dcfe..cbc6feaa0e0 100755 --- a/test/functional/tool_wallet.py +++ b/test/functional/tool_wallet.py @@ -188,6 +188,8 @@ class ToolWalletTest(BitcoinTestFramework): 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 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: From 7777105a24a36b62df35d12ecf6c6370671568c8 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Thu, 21 Jan 2021 13:59:49 +0100 Subject: [PATCH 3/4] refactor: Move all command dependend checks to ExecuteWalletToolFunc --- src/bitcoin-wallet.cpp | 10 +--------- src/wallet/wallettool.cpp | 10 +++++++--- src/wallet/wallettool.h | 2 +- 3 files changed, 9 insertions(+), 13 deletions(-) diff --git a/src/bitcoin-wallet.cpp b/src/bitcoin-wallet.cpp index 435bf289b44..0b55b587fda 100644 --- a/src/bitcoin-wallet.cpp +++ b/src/bitcoin-wallet.cpp @@ -111,17 +111,9 @@ int main(int argc, char* argv[]) return EXIT_FAILURE; } - // A name must be provided when creating a file - if (method == "create" && !args.IsArgSet("-wallet")) { - tfm::format(std::cerr, "Wallet name must be provided when creating a new wallet.\n"); - return EXIT_FAILURE; - } - - std::string name = args.GetArg("-wallet", ""); - ECCVerifyHandle globalVerifyHandle; ECC_Start(); - if (!WalletTool::ExecuteWalletToolFunc(args, method, name)) { + if (!WalletTool::ExecuteWalletToolFunc(args, method)) { return EXIT_FAILURE; } ECC_Stop(); diff --git a/src/wallet/wallettool.cpp b/src/wallet/wallettool.cpp index bc90491a2c1..b2cb0bf4792 100644 --- a/src/wallet/wallettool.cpp +++ b/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; diff --git a/src/wallet/wallettool.h b/src/wallet/wallettool.h index f544a6f7274..f4516bb5bc8 100644 --- a/src/wallet/wallettool.h +++ b/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 From fa61b9d1a68820758f9540653920deaeae6abe79 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Wed, 13 Jan 2021 09:00:43 +0100 Subject: [PATCH 4/4] util: Add ArgsManager::GetCommand() and use it in bitcoin-wallet Co-Authored-by: Anthony Towns --- src/bitcoin-wallet.cpp | 30 ++++++++----------- src/test/fuzz/system.cpp | 2 +- src/util/system.cpp | 53 ++++++++++++++++++++++++++++++++-- src/util/system.h | 22 ++++++++++++++ test/functional/tool_wallet.py | 6 ++-- 5 files changed, 89 insertions(+), 24 deletions(-) diff --git a/src/bitcoin-wallet.cpp b/src/bitcoin-wallet.cpp index 0b55b587fda..b84d909b07e 100644 --- a/src/bitcoin-wallet.cpp +++ b/src/bitcoin-wallet.cpp @@ -33,11 +33,11 @@ static void SetupWalletToolArgs(ArgsManager& argsman) argsman.AddArg("-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(ArgsManager& args, int argc, char* argv[]) @@ -95,25 +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; } + 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; + } ECCVerifyHandle globalVerifyHandle; ECC_Start(); - if (!WalletTool::ExecuteWalletToolFunc(args, method)) { + if (!WalletTool::ExecuteWalletToolFunc(args, command->command)) { return EXIT_FAILURE; } ECC_Stop(); diff --git a/src/test/fuzz/system.cpp b/src/test/fuzz/system.cpp index 47b38b6d23d..3621702e458 100644 --- a/src/test/fuzz/system.cpp +++ b/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(), options_category); + args_manager.AddArg(argument_name, fuzzed_data_provider.ConsumeRandomLengthString(16), fuzzed_data_provider.ConsumeIntegral() & ~ArgsManager::COMMAND, options_category); }, [&] { // Avoid hitting: diff --git a/src/util/system.cpp b/src/util/system.cpp index d1fb9216429..9fe9d674115 100644 --- a/src/util/system.cpp +++ b/src/util/system.cpp @@ -3,7 +3,6 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. -#include #include #ifdef HAVE_BOOST_PROCESS @@ -11,6 +10,8 @@ #endif // HAVE_BOOST_PROCESS #include +#include +#include #include #include #include @@ -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 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 ArgsManager::GetArgFlags(const std::string& name) const return nullopt; } +std::optional 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 ArgsManager::GetArgs(const std::string& strArg) const { std::vector 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& 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) { diff --git a/src/util/system.h b/src/util/system.h index 010fc5b49fc..e8e1fab3338 100644 --- a/src/util/system.h +++ b/src/util/system.h @@ -183,6 +183,7 @@ public: NETWORK_ONLY = 0x200, // This argument's value is sensitive (such as a password). SENSITIVE = 0x400, + COMMAND = 0x800, }; protected: @@ -195,9 +196,11 @@ protected: mutable RecursiveMutex cs_args; util::Settings m_settings GUARDED_BY(cs_args); + std::vector m_command GUARDED_BY(cs_args); std::string m_network GUARDED_BY(cs_args); std::set m_network_only_args GUARDED_BY(cs_args); std::map> m_available_args GUARDED_BY(cs_args); + bool m_accept_any_command GUARDED_BY(cs_args){true}; std::list 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); @@ -248,6 +251,20 @@ public: */ const std::list 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 args; + }; + /** + * Get the command and command args (returns std::nullopt if no command provided) + */ + std::optional GetCommand() const; + /** * Return a vector of strings of the given argument * @@ -333,6 +350,11 @@ public: */ 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 */ diff --git a/test/functional/tool_wallet.py b/test/functional/tool_wallet.py index cbc6feaa0e0..cca984b2927 100755 --- a/test/functional/tool_wallet.py +++ b/test/functional/tool_wallet.py @@ -183,10 +183,10 @@ class ToolWalletTest(BitcoinTestFramework): 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')