Throw an error for unknown args #13112

achow101 · 2018-04-29T03:18:50Z

Following #13190, gArgs is aware of all of the command line arguments. This PR has gArgs check whether the arguments provided are actually valid arguments. When an unknown argument is encountered, an error is printed to stderr and the program exist.

Since gArgs is used for everything that has command line arguments, bitcoind, bitcoin-cli, bitcoin-qt, bitcoin-tx, and bench_bitcoin are all effected by this change and all now have the same argument checking behavior.

Closes #1044

promag

Concept ACK.

What about known but not used arguments? For instance, -disablewallet -changetype=bech32 should fail or nor?

promag · 2018-04-29T07:56:57Z

src/util.cpp

@@ -536,6 +565,56 @@ void ArgsManager::ForceSetArg(const std::string& strArg, const std::string& strV
    m_override_args[strArg] = {strValue};
 }

+void ArgsManager::AddArg(const std::string& name, const std::string& help, const bool debug_only, const OptionsCategory& cat)
+{
+    m_available_args.emplace(std::pair<OptionsCategory, std::string>(cat, name), std::pair<std::string, bool>(help, debug_only));


Check if already exists with static assert?

maflcko · 2018-04-29T11:06:20Z

Ping @meshcollider who has been working on something similar.

maflcko · 2018-04-29T11:18:11Z

src/util.h

@@ -118,6 +118,31 @@ inline bool IsSwitchChar(char c)
 #endif
 }

+enum OptionsCategory


nit: Please make this enum class

maflcko · 2018-04-29T11:23:44Z

Compile error due to missing override in https://github.com/achow101/bitcoin/blob/67699a9fd9a4c279c31f1d61ef44a16032739375/src/init.cpp#L79, I presume.

achow101 · 2018-04-29T15:13:31Z

What about known but not used arguments? For instance, -disablewallet -changetype=bech32 should fail or nor?

This will not fail since -changetype is a valid argument independent of -disablewallet. The checker currently does not consider parameter interactions. Essentially it just checks for whether an argument is part of the help text.

Compile error due to missing override in https://github.com/achow101/bitcoin/blob/67699a9fd9a4c279c31f1d61ef44a16032739375/src/init.cpp#L79, I presume.

Fixed I think.

promag · 2018-04-30T08:22:50Z

The checker currently does not consider parameter interactions.

Got it.

promag

Should old arguments be in a legacy category? Otherwise launching with an outdated configuration might unnecessarily throw an error.

promag · 2018-04-30T12:47:10Z

src/init.cpp

+    gArgs.AddArg("-tor", "", false, OptionsCategory::HIDDEN);
+    gArgs.AddArg("-debugnet", "", false, OptionsCategory::HIDDEN);
+    gArgs.AddArg("-whitelistalwaysrelay", "", false, OptionsCategory::HIDDEN);
+    gArgs.AddArg("-prematurewitness", "", false, OptionsCategory::HIDDEN);


This must stay even if #13120 is merged? Ping @MarcoFalke.

Nah, this should be rebased and removed after (and if) #13120 is merged

Agree in this case, not sure about arguments removed in the future.

I mean there is no urgent need to remove them. If they stick around for one release (marked as "deprecated, but silently ignored") that is fine.

jnewbery · 2018-04-30T13:00:06Z

Concept ACK

achow101 · 2018-04-30T15:11:07Z

Should old arguments be in a legacy category? Otherwise launching with an outdated configuration might unnecessarily throw an error.

There could be a deprecated category where a warning is given if any arguments are in that category. Then we can remove the arguments entirely later.

meshcollider · 2018-05-02T08:58:30Z

Apologies for the delay in replying, I'm very swamped with work at the moment.
It's clear that something needs to be done to fix the arguments being silently ignored, I'm fine with this approach over the one I've been working on since it is an immediate fix. If my rework would still be useful for other reasons it could still be done after this is merged anyway. Either way this seems like a nice cleanup of the helptext output at the same time.
So Concept ACK from me.

maflcko · 2018-05-02T18:38:22Z

src/util.cpp

+void ArgsManager::AddArg(const std::string& name, const std::string& help, const bool debug_only, const OptionsCategory& cat)
+{
+    std::pair<OptionsCategory, std::string> key(cat, name);
+    assert(m_available_args.count(key) == 0);


nit: I guess the key can just be the name, as two names with a different category don't make sense?

It's a hack to make std::map sort the keys by category and then alphabetically when iterating over the map so that the help text comes out correctly.

If the m_available_args type remains, then avoid the 2nd lookup:

auto i = m_available_args.emplace(std::make_pair(cat, name), std::make_pair(help, debug_only)); assert(i.second);

Still, it should reject duplicate names as @MarcoFalke suggests?

I doubt it matters in practice, so I consider it a code style nit.

promag · 2018-05-02T22:57:28Z

src/util.cpp

+void ArgsManager::AddArg(const std::string& name, const std::string& help, const bool debug_only, const OptionsCategory& cat)
+{
+    std::pair<OptionsCategory, std::string> key(cat, name);
+    assert(m_available_args.count(key) == 0);


If the m_available_args type remains, then avoid the 2nd lookup:

auto i = m_available_args.emplace(std::make_pair(cat, name), std::make_pair(help, debug_only)); assert(i.second);

promag · 2018-05-02T22:59:37Z

src/util.cpp

+void ArgsManager::AddArg(const std::string& name, const std::string& help, const bool debug_only, const OptionsCategory& cat)
+{
+    std::pair<OptionsCategory, std::string> key(cat, name);
+    assert(m_available_args.count(key) == 0);


Still, it should reject duplicate names as @MarcoFalke suggests?

promag · 2018-05-02T22:59:44Z

src/util.h

@@ -128,6 +151,7 @@ class ArgsManager
    std::map<std::string, std::vector<std::string>> m_config_args;
    std::string m_network;
    std::set<std::string> m_network_only_args;
+    std::map<std::pair<OptionsCategory, std::string>, std::pair<std::string, bool>> m_available_args;


Alternative that keeps argument sorted and can simplify GetHelpMessage (don't mind the names) and also prevent adding duplicate arguments.

class ArgsManager { struct Arg { OptionsCategory m_category; std::string m_name; bool m_help; bool m_debug_only; }; std::map<std::string, Arg> m_available_args; std::map<OptionsCategory, std::map<std::string, Arg>> m_args_by_category;

That's certainly an option, but I think I'll leave it as is.

I think @promag's suggestion is better; it's more readable (struct with named arguments), and should be more efficient when looking up arguments.

Also, it can avoid duplicating the Arg objects by using std::map<OptionsCategory, std::map<std::string, const Arg*>> m_args_by_category instead.

@sipa bit late for that now though. The commit with that change was merged in a different PR with just that commit.

@achow101 I disagree - the current data structure were sufficient for the use case in that PR (producing the help message), plus that PR did the bulk of the needs-frequent-rebase changes (the gArgs.AddArg calls everywhere).

I believe that a better data structure is possible for the new use case this PR introduces, and will probably simplify the PR too.

I've made some changes to the structure that are similar to what was suggested. Now I am using a single map which has a map nested inside. The key for the outer map is the category, and the value is a map for all of the args in that category. I am also using a struct to hold the help text and debug_only bool. This should be more readable and make the help text printing function less hacky.

achow101 · 2018-05-05T05:25:49Z

Rebased

maflcko

utACK the first commit (eba9252867ffdb5a0e45f34eb1e281d92a0990f4).

This commit seems useful on its own and I suggest to separate it out into a new pull to aid review and hopefully decrease the need to rebase the whole pr too often.

I have left some bike shedding comments and one conceptual feedback for the first commit including a proposed diff.

Edit: Note that this review is erroneously collapsed in GitHub's web view.

maflcko · 2018-05-06T22:02:07Z

src/chainparamsbase.h

 */
-void AppendParamsHelpMessages(std::string& strUsage, bool debugHelp=true);
+void SetParamsArgs();


For clarity I'd prefer if this was either a static class member or named appropriately, like SetChainParamsBaseOptions

maflcko · 2018-05-06T22:14:41Z

src/util.cpp

+                usage += HelpMessageGroup(_("Register Commands:"));
+        }
+        if ((!arg.second.second || (show_debug && arg.second.second)) &&
+            (arg.first.second != "-daemon" || (arg.first.second == "-daemon" && mode == HelpMessageMode::BITCOIND))) {


Forcing the args manager be aware of this particular option smells wrong. Also, the boolean logic could be simplified to just show_debug || !arg.second.second.

For your convenience I have prepared a suitable diff that cleanly applies and simplifies the code:

diff --git a/src/bench/bench_bitcoin.cpp b/src/bench/bench_bitcoin.cpp index b08d4cd253..21d23f1985 100644 --- a/src/bench/bench_bitcoin.cpp +++ b/src/bench/bench_bitcoin.cpp @@ -42,7 +42,7 @@ main(int argc, char** argv) gArgs.ParseParameters(argc, argv); if (HelpRequested(gArgs)) { - std::cout << gArgs.GetHelpMessage(HelpMessageMode::OTHER); + std::cout << gArgs.GetHelpMessage(); return 0; } diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp index 8cb886cabd..3f00a95c91 100644 --- a/src/bitcoin-cli.cpp +++ b/src/bitcoin-cli.cpp @@ -90,7 +90,7 @@ static int AppInitRPC(int argc, char* argv[]) " bitcoin-cli [options] help " + _("List commands") + "\n" + " bitcoin-cli [options] help <command> " + _("Get help for a command") + "\n"; - strUsage += "\n" + gArgs.GetHelpMessage(HelpMessageMode::OTHER); + strUsage += "\n" + gArgs.GetHelpMessage(); } fprintf(stdout, "%s", strUsage.c_str()); diff --git a/src/bitcoin-tx.cpp b/src/bitcoin-tx.cpp index 359bd7749a..13a4542c9c 100644 --- a/src/bitcoin-tx.cpp +++ b/src/bitcoin-tx.cpp @@ -95,7 +95,7 @@ static int AppInitRawTx(int argc, char* argv[]) " bitcoin-tx [options] <hex-tx> [commands] " + _("Update hex-encoded bitcoin transaction") + "\n" + " bitcoin-tx [options] -create [commands] " + _("Create hex-encoded bitcoin transaction") + "\n" + "\n"; - strUsage += gArgs.GetHelpMessage(HelpMessageMode::OTHER); + strUsage += gArgs.GetHelpMessage(); fprintf(stdout, "%s", strUsage.c_str()); diff --git a/src/bitcoind.cpp b/src/bitcoind.cpp index 218a9e9f42..1a004ea4af 100644 --- a/src/bitcoind.cpp +++ b/src/bitcoind.cpp @@ -62,6 +62,9 @@ static bool AppInit(int argc, char* argv[]) // // If Qt is used, parameters/bitcoin.conf are parsed in qt/bitcoin.cpp's main() SetupArgs(); +#if HAVE_DECL_DAEMON + gArgs.AddArg("-daemon", _("Run in the background as a daemon and accept commands"), false, OptionsCategory::OPTIONS); +#endif gArgs.ParseParameters(argc, argv); // Process help and version before taking care about datadir @@ -77,7 +80,7 @@ static bool AppInit(int argc, char* argv[]) strUsage += "\n" + _("Usage:") + "\n" + " bitcoind [options] " + strprintf(_("Start %s Daemon"), _(PACKAGE_NAME)) + "\n"; - strUsage += "\n" + gArgs.GetHelpMessage(HelpMessageMode::BITCOIND); + strUsage += "\n" + gArgs.GetHelpMessage(); } fprintf(stdout, "%s", strUsage.c_str()); diff --git a/src/init.cpp b/src/init.cpp index c371e13b21..cb00d58786 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -351,9 +351,6 @@ void SetupArgs() gArgs.AddArg("-blockreconstructionextratxn=<n>", strprintf(_("Extra transactions to keep in memory for compact block reconstructions (default: %u)"), DEFAULT_BLOCK_RECONSTRUCTION_EXTRA_TXN), false, OptionsCategory::OPTIONS); gArgs.AddArg("-blocksonly", strprintf(_("Whether to operate in a blocks only mode (default: %u)"), DEFAULT_BLOCKSONLY), true, OptionsCategory::OPTIONS); gArgs.AddArg("-conf=<file>", strprintf(_("Specify configuration file. Relative paths will be prefixed by datadir location. (default: %s)"), BITCOIN_CONF_FILENAME), false, OptionsCategory::OPTIONS); -#if HAVE_DECL_DAEMON - gArgs.AddArg("-daemon", _("Run in the background as a daemon and accept commands"), false, OptionsCategory::OPTIONS); -#endif gArgs.AddArg("-datadir=<dir>", _("Specify data directory"), false, OptionsCategory::OPTIONS); gArgs.AddArg("-dbbatchsize", strprintf("Maximum database write batch size in bytes (default: %u)", nDefaultDbBatchSize), true, OptionsCategory::OPTIONS); gArgs.AddArg("-dbcache=<n>", strprintf(_("Set database cache size in megabytes (%d to %d, default: %d)"), nMinDbCache, nMaxDbCache, nDefaultDbCache), false, OptionsCategory::OPTIONS); diff --git a/src/interfaces/node.h b/src/interfaces/node.h index a3f637db2d..44f53f7e78 100644 --- a/src/interfaces/node.h +++ b/src/interfaces/node.h @@ -7,7 +7,6 @@ #include <addrdb.h> // For banmap_t #include <amount.h> // For CAmount -#include <init.h> // For HelpMessageMode #include <net.h> // For CConnman::NumConnections #include <netaddress.h> // For Network diff --git a/src/qt/utilitydialog.cpp b/src/qt/utilitydialog.cpp index 79be2823a3..993d7454d6 100644 --- a/src/qt/utilitydialog.cpp +++ b/src/qt/utilitydialog.cpp @@ -78,7 +78,7 @@ HelpMessageDialog::HelpMessageDialog(interfaces::Node& node, QWidget *parent, bo cursor.insertText(header); cursor.insertBlock(); - std::string strUsage = gArgs.GetHelpMessage(HelpMessageMode::OTHER); + std::string strUsage = gArgs.GetHelpMessage(); QString coreOptions = QString::fromStdString(strUsage); text = version + "\n" + header + "\n" + coreOptions; diff --git a/src/util.cpp b/src/util.cpp index f59051e0e1..265f77f6ed 100644 --- a/src/util.cpp +++ b/src/util.cpp @@ -543,7 +543,7 @@ void ArgsManager::AddArg(const std::string& name, const std::string& help, const m_available_args.emplace(key, std::pair<std::string, bool>(help, debug_only)); } -std::string ArgsManager::GetHelpMessage(HelpMessageMode mode) +std::string ArgsManager::GetHelpMessage() { const bool show_debug = gArgs.GetBoolArg("-help-debug", false); @@ -578,8 +578,7 @@ std::string ArgsManager::GetHelpMessage(HelpMessageMode mode) else if (last_cat == OptionsCategory::REGISTER_COMMANDS) usage += HelpMessageGroup(_("Register Commands:")); } - if ((!arg.second.second || (show_debug && arg.second.second)) && - (arg.first.second != "-daemon" || (arg.first.second == "-daemon" && mode == HelpMessageMode::BITCOIND))) { + if (show_debug||!arg.second.second) { usage += HelpMessageOpt(arg.first.second, arg.second.first); } } diff --git a/src/util.h b/src/util.h index f36a9b254a..2c8fbfa312 100644 --- a/src/util.h +++ b/src/util.h @@ -135,12 +135,6 @@ enum class OptionsCategory REGISTER_COMMANDS }; -/** The help message mode determines what help message to show */ -enum class HelpMessageMode { - BITCOIND, - OTHER -}; - class ArgsManager { protected: @@ -262,7 +256,7 @@ public: /** * Get the help string */ - std::string GetHelpMessage(HelpMessageMode mode); + std::string GetHelpMessage(); }; extern ArgsManager gArgs;

maflcko · 2018-05-06T22:16:38Z

src/bench/bench_bitcoin.cpp

@@ -22,22 +22,27 @@ static const char* DEFAULT_PLOT_PLOTLYURL = "https://cdn.plot.ly/plotly-latest.m
 static const int64_t DEFAULT_PLOT_WIDTH = 1024;
 static const int64_t DEFAULT_PLOT_HEIGHT = 768;

+void SetupBenchArgs()


nit: Should be static after #13163

maflcko · 2018-05-06T22:17:24Z

src/bitcoin-cli.cpp

@@ -29,29 +29,26 @@ static const int DEFAULT_HTTP_CLIENT_TIMEOUT=900;
 static const bool DEFAULT_NAMED=false;
 static const int CONTINUE_EXECUTION=-1;

-static std::string HelpMessageCli()
+void SetupArgs()


Nit: More suitable name could be static void SetupCliArgs. Including the static after #13163

maflcko · 2018-05-06T22:18:18Z

src/bitcoin-tx.cpp

@@ -31,6 +31,41 @@ static bool fCreateBlank;
 static std::map<std::string,UniValue> registers;
 static const int CONTINUE_EXECUTION=-1;

+void SetupArgs()


Nit: More suitable name could be static void SetupTxArgs, including the static after #13163

maflcko · 2018-05-06T22:22:30Z

src/init.h

+/**
+ * Setup the arguments for gArgs
+ */
+void SetupArgs();


nit: A more suitable name would be SetupServerArgs, since this is shared between bitcoind and the gui.

maflcko · 2018-05-06T22:22:59Z

src/qt/bitcoin.cpp

@@ -531,6 +531,20 @@ WId BitcoinApplication::getMainWinId() const
    return window->winId();
 }

+void SetupUIArgs()


nit: Should be static after #13163

achow101 · 2018-05-08T18:28:00Z

Addressed all of @MarcoFalke's comments.

I've separated the first commit of this PR into #13190 and so this is now on top of that.

4d4185a Make gArgs aware of the arguments (Andrew Chow) Pull request description: Instead of each binary generating and printing it's own help string, have gArgs know what the args are and generate the help string. This is the first commit of #13112 pulled out. Tree-SHA512: d794c872b834bc56c78d947549f9cf046a8174984ab0c7b4ba06bc51d36dca11a4ed88afafe76bb4f776cdba042e17e30b9c2ed7b195bef7df77a1327823f989

achow101 · 2018-05-09T18:46:51Z

Rebased onto master following merge of #13190

maflcko · 2018-05-09T18:56:31Z

src/util.cpp

+            }
+        }
+        if (!found && !ignore_invalid_keys) {
+            throw std::runtime_error(strprintf("Invalid configuration value %s", it->string_key.c_str()));


Wouldn't it be more straightforward to return false; instead of throwing an exception here? If you need to pass a string you could pass in a non-const reference to an std::string.

Also, why is it required in the first place to ignore invalid keys for the cli?

It feels clunky to me to pass down a string for the error message than just throwing an exception.

cli needs to ignore invalid keys in the bitcoin.conf because those "invalid keys" are usually just options for bitcoind that cli is not aware of. But cli still needs to read the bitcoin.conf to retrieve the rpcpassword.

Using an exception for control flow is just another instance of "goto", imo. Even in python (where using exceptions for control flow is considered pythonic) we had serious issues that took several years to fix up completely.
If you prefer to pass a result object with a boolean indicating success and an optional error string, I think that is fine. But using an std::runtime_error as the class for this object seems confusing and is not type safe.

I changed this so std::string& is passed down for the error and that ParseParameters, ReadConfigFiles, and ReadConfigStream all return a bool.

This commit contains 2 refactors. 1. mark "const" on ArgsManager::GetHelpMessage and IsArgKnown. 2. remove unused "error" argument from ArgsManager::IsArgKnown. Firstly, I mark "const" on where it is possible to. It is mentioned before (e.g. bitcoin#13190 (review)). And about 2nd change, ArgsManager::IsArgKnown was added at commit #4f8704d which was merged at PR bitcoin#13112. But from its beggining, "error" argument never be used. I think it should be refactored.

Return `EXIT_SUCCESS` from `main()` on error, not the bool `false` (introduced in bitcoin#13112). This is the correct value to return on error, and also shuts up a clang warning. Also add a final return for clarity.

bc8aa2b don't translate command line options (Michael Polzer) Pull request description: sneaked in with 4f8704d bitcoin#13112 Tree-SHA512: 90489e7f4eb689d205c0b5e2f8d673e8283f2f0a855c9cb3909b8cb1cfd6b4b18b4643624c0e4f21ba03a15f2ed70dca186fd11cce2d57841424964b13b390c2

This commit contains 2 refactors. 1. mark "const" on ArgsManager::GetHelpMessage and IsArgKnown. 2. remove unused "error" argument from ArgsManager::IsArgKnown. Firstly, I mark "const" on where it is possible to. It is mentioned before (e.g. bitcoin#13190 (review)). And about 2nd change, ArgsManager::IsArgKnown was added at commit #4f8704d which was merged at PR bitcoin#13112. But from its beggining, "error" argument never be used. I think it should be refactored.

9030557 Test gArgs erroring on unknown args (Andrew Chow) 4f8704d Give an error and exit if there are unknown parameters (Andrew Chow) 174f7c8 Use a struct for arguments and nested map for categories (Andrew Chow) Pull request description: Following bitcoin#13190, gArgs is aware of all of the command line arguments. This PR has gArgs check whether the arguments provided are actually valid arguments. When an unknown argument is encountered, an error is printed to stderr and the program exist. Since gArgs is used for everything that has command line arguments, `bitcoind`, `bitcoin-cli`, `bitcoin-qt`, `bitcoin-tx`, and `bench_bitcoin` are all effected by this change and all now have the same argument checking behavior. Closes dashpay#1044 Tree-SHA512: 388201319a7d6493204bb5433da47e8e6c8266882e809f6df45f86d925f1f320f2fd13edb3e57ffc6a37415dfdfc689f83929452bca224229783accb367032e7

493a166 bench: Don't return a bool from main (Wladimir J. van der Laan) Pull request description: Return `1` from `main()` on error, not the bool `false` (introduced in bitcoin#13112). This is the correct value to return on error, and also shuts up a clang warning. Tree-SHA512: 52a0f1b2f6ae2697555f71ee2019ce657046f7f379f1f4faf3cce9d5f3fb21fcdc43a4c84895a2a8b6929997ba70bbe87c231f2f9553215b84c22333810d58d9