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
564@@ -536,6 +565,56 @@ void ArgsManager::ForceSetArg(const std::string& strArg, const std::string& strV
565 m_override_args[strArg] = {strValue};
566 }
567
568+void ArgsManager::AddArg(const std::string& name, const std::string& help, const bool debug_only, const OptionsCategory& cat)
569+{
570+ m_available_args.emplace(std::pair<OptionsCategory, std::string>(cat, name), std::pair<std::string, bool>(help, debug_only));
Concept ACK.
What about known but not used arguments? For instance, -disablewallet -changetype=bech32 should fail or nor?
117@@ -118,6 +118,31 @@ inline bool IsSwitchChar(char c)
118 #endif
119 }
120
121+enum OptionsCategory
enum class
What about known but not used arguments? For instance,
-disablewallet -changetype=bech32should 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.
The checker currently does not consider parameter interactions.
Got it.
666+ gArgs.AddArg("-help", "", false, OptionsCategory::HIDDEN);
667+ gArgs.AddArg("-socks", "", false, OptionsCategory::HIDDEN);
668+ gArgs.AddArg("-tor", "", false, OptionsCategory::HIDDEN);
669+ gArgs.AddArg("-debugnet", "", false, OptionsCategory::HIDDEN);
670+ gArgs.AddArg("-whitelistalwaysrelay", "", false, OptionsCategory::HIDDEN);
671+ gArgs.AddArg("-prematurewitness", "", false, OptionsCategory::HIDDEN);
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.
564@@ -536,6 +565,58 @@ void ArgsManager::ForceSetArg(const std::string& strArg, const std::string& strV
565 m_override_args[strArg] = {strValue};
566 }
567
568+void ArgsManager::AddArg(const std::string& name, const std::string& help, const bool debug_only, const OptionsCategory& cat)
569+{
570+ std::pair<OptionsCategory, std::string> key(cat, name);
571+ assert(m_available_args.count(key) == 0);
If the m_available_args type remains, then avoid the 2nd lookup:
0auto i = m_available_args.emplace(std::make_pair(cat, name), std::make_pair(help, debug_only));
1assert(i.second);
150@@ -128,6 +151,7 @@ class ArgsManager
151 std::map<std::string, std::vector<std::string>> m_config_args;
152 std::string m_network;
153 std::set<std::string> m_network_only_args;
154+ 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.
0class ArgsManager {
1 struct Arg
2 {
3 OptionsCategory m_category;
4 std::string m_name;
5 bool m_help;
6 bool m_debug_only;
7 };
8
9 std::map<std::string, Arg> m_available_args;
10 std::map<OptionsCategory, std::map<std::string, Arg>> m_args_by_category;
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.
@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.
39@@ -40,10 +40,9 @@ class CBaseChainParams
40 std::unique_ptr<CBaseChainParams> CreateBaseChainParams(const std::string& chain);
41
42 /**
43- * Append the help messages for the chainparams options to the
44- * parameter string.
45+ *Set the arguments for chainparams
46 */
47-void AppendParamsHelpMessages(std::string& strUsage, bool debugHelp=true);
48+void SetParamsArgs();
SetChainParamsBaseOptions
577+ usage += HelpMessageGroup(_("Commands:"));
578+ else if (last_cat == OptionsCategory::REGISTER_COMMANDS)
579+ usage += HelpMessageGroup(_("Register Commands:"));
580+ }
581+ if ((!arg.second.second || (show_debug && arg.second.second)) &&
582+ (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:
0diff --git a/src/bench/bench_bitcoin.cpp b/src/bench/bench_bitcoin.cpp
1index b08d4cd253..21d23f1985 100644
2--- a/src/bench/bench_bitcoin.cpp
3+++ b/src/bench/bench_bitcoin.cpp
4@@ -42,7 +42,7 @@ main(int argc, char** argv)
5 gArgs.ParseParameters(argc, argv);
6
7 if (HelpRequested(gArgs)) {
8- std::cout << gArgs.GetHelpMessage(HelpMessageMode::OTHER);
9+ std::cout << gArgs.GetHelpMessage();
10
11 return 0;
12 }
13diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
14index 8cb886cabd..3f00a95c91 100644
15--- a/src/bitcoin-cli.cpp
16+++ b/src/bitcoin-cli.cpp
17@@ -90,7 +90,7 @@ static int AppInitRPC(int argc, char* argv[])
18 " bitcoin-cli [options] help " + _("List commands") + "\n" +
19 " bitcoin-cli [options] help <command> " + _("Get help for a command") + "\n";
20
21- strUsage += "\n" + gArgs.GetHelpMessage(HelpMessageMode::OTHER);
22+ strUsage += "\n" + gArgs.GetHelpMessage();
23 }
24
25 fprintf(stdout, "%s", strUsage.c_str());
26diff --git a/src/bitcoin-tx.cpp b/src/bitcoin-tx.cpp
27index 359bd7749a..13a4542c9c 100644
28--- a/src/bitcoin-tx.cpp
29+++ b/src/bitcoin-tx.cpp
30@@ -95,7 +95,7 @@ static int AppInitRawTx(int argc, char* argv[])
31 " bitcoin-tx [options] <hex-tx> [commands] " + _("Update hex-encoded bitcoin transaction") + "\n" +
32 " bitcoin-tx [options] -create [commands] " + _("Create hex-encoded bitcoin transaction") + "\n" +
33 "\n";
34- strUsage += gArgs.GetHelpMessage(HelpMessageMode::OTHER);
35+ strUsage += gArgs.GetHelpMessage();
36
37 fprintf(stdout, "%s", strUsage.c_str());
38
39diff --git a/src/bitcoind.cpp b/src/bitcoind.cpp
40index 218a9e9f42..1a004ea4af 100644
41--- a/src/bitcoind.cpp
42+++ b/src/bitcoind.cpp
43@@ -62,6 +62,9 @@ static bool AppInit(int argc, char* argv[])
44 //
45 // If Qt is used, parameters/bitcoin.conf are parsed in qt/bitcoin.cpp's main()
46 SetupArgs();
47+#if HAVE_DECL_DAEMON
48+ gArgs.AddArg("-daemon", _("Run in the background as a daemon and accept commands"), false, OptionsCategory::OPTIONS);
49+#endif
50 gArgs.ParseParameters(argc, argv);
51
52 // Process help and version before taking care about datadir
53@@ -77,7 +80,7 @@ static bool AppInit(int argc, char* argv[])
54 strUsage += "\n" + _("Usage:") + "\n" +
55 " bitcoind [options] " + strprintf(_("Start %s Daemon"), _(PACKAGE_NAME)) + "\n";
56
57- strUsage += "\n" + gArgs.GetHelpMessage(HelpMessageMode::BITCOIND);
58+ strUsage += "\n" + gArgs.GetHelpMessage();
59 }
60
61 fprintf(stdout, "%s", strUsage.c_str());
62diff --git a/src/init.cpp b/src/init.cpp
63index c371e13b21..cb00d58786 100644
64--- a/src/init.cpp
65+++ b/src/init.cpp
66@@ -351,9 +351,6 @@ void SetupArgs()
67 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);
68 gArgs.AddArg("-blocksonly", strprintf(_("Whether to operate in a blocks only mode (default: %u)"), DEFAULT_BLOCKSONLY), true, OptionsCategory::OPTIONS);
69 gArgs.AddArg("-conf=<file>", strprintf(_("Specify configuration file. Relative paths will be prefixed by datadir location. (default: %s)"), BITCOIN_CONF_FILENAME), false, OptionsCategory::OPTIONS);
70-#if HAVE_DECL_DAEMON
71- gArgs.AddArg("-daemon", _("Run in the background as a daemon and accept commands"), false, OptionsCategory::OPTIONS);
72-#endif
73 gArgs.AddArg("-datadir=<dir>", _("Specify data directory"), false, OptionsCategory::OPTIONS);
74 gArgs.AddArg("-dbbatchsize", strprintf("Maximum database write batch size in bytes (default: %u)", nDefaultDbBatchSize), true, OptionsCategory::OPTIONS);
75 gArgs.AddArg("-dbcache=<n>", strprintf(_("Set database cache size in megabytes (%d to %d, default: %d)"), nMinDbCache, nMaxDbCache, nDefaultDbCache), false, OptionsCategory::OPTIONS);
76diff --git a/src/interfaces/node.h b/src/interfaces/node.h
77index a3f637db2d..44f53f7e78 100644
78--- a/src/interfaces/node.h
79+++ b/src/interfaces/node.h
80@@ -7,7 +7,6 @@
81
82 #include <addrdb.h> // For banmap_t
83 #include <amount.h> // For CAmount
84-#include <init.h> // For HelpMessageMode
85 #include <net.h> // For CConnman::NumConnections
86 #include <netaddress.h> // For Network
87
88diff --git a/src/qt/utilitydialog.cpp b/src/qt/utilitydialog.cpp
89index 79be2823a3..993d7454d6 100644
90--- a/src/qt/utilitydialog.cpp
91+++ b/src/qt/utilitydialog.cpp
92@@ -78,7 +78,7 @@ HelpMessageDialog::HelpMessageDialog(interfaces::Node& node, QWidget *parent, bo
93 cursor.insertText(header);
94 cursor.insertBlock();
95
96- std::string strUsage = gArgs.GetHelpMessage(HelpMessageMode::OTHER);
97+ std::string strUsage = gArgs.GetHelpMessage();
98 QString coreOptions = QString::fromStdString(strUsage);
99 text = version + "\n" + header + "\n" + coreOptions;
100
101diff --git a/src/util.cpp b/src/util.cpp
102index f59051e0e1..265f77f6ed 100644
103--- a/src/util.cpp
104+++ b/src/util.cpp
105@@ -543,7 +543,7 @@ void ArgsManager::AddArg(const std::string& name, const std::string& help, const
106 m_available_args.emplace(key, std::pair<std::string, bool>(help, debug_only));
107 }
108
109-std::string ArgsManager::GetHelpMessage(HelpMessageMode mode)
110+std::string ArgsManager::GetHelpMessage()
111 {
112 const bool show_debug = gArgs.GetBoolArg("-help-debug", false);
113
114@@ -578,8 +578,7 @@ std::string ArgsManager::GetHelpMessage(HelpMessageMode mode)
115 else if (last_cat == OptionsCategory::REGISTER_COMMANDS)
116 usage += HelpMessageGroup(_("Register Commands:"));
117 }
118- if ((!arg.second.second || (show_debug && arg.second.second)) &&
119- (arg.first.second != "-daemon" || (arg.first.second == "-daemon" && mode == HelpMessageMode::BITCOIND))) {
120+ if (show_debug||!arg.second.second) {
121 usage += HelpMessageOpt(arg.first.second, arg.second.first);
122 }
123 }
124diff --git a/src/util.h b/src/util.h
125index f36a9b254a..2c8fbfa312 100644
126--- a/src/util.h
127+++ b/src/util.h
128@@ -135,12 +135,6 @@ enum class OptionsCategory
129 REGISTER_COMMANDS
130 };
131
132-/** The help message mode determines what help message to show */
133-enum class HelpMessageMode {
134- BITCOIND,
135- OTHER
136-};
137-
138 class ArgsManager
139 {
140 protected:
141@@ -262,7 +256,7 @@ public:
142 /**
143 * Get the help string
144 */
145- std::string GetHelpMessage(HelpMessageMode mode);
146+ std::string GetHelpMessage();
147 };
148
149 extern ArgsManager gArgs;
21@@ -22,22 +22,27 @@ static const char* DEFAULT_PLOT_PLOTLYURL = "https://cdn.plot.ly/plotly-latest.m
22 static const int64_t DEFAULT_PLOT_WIDTH = 1024;
23 static const int64_t DEFAULT_PLOT_HEIGHT = 768;
24
25+void SetupBenchArgs()
static after #13163
28@@ -29,29 +29,26 @@ static const int DEFAULT_HTTP_CLIENT_TIMEOUT=900;
29 static const bool DEFAULT_NAMED=false;
30 static const int CONTINUE_EXECUTION=-1;
31
32-static std::string HelpMessageCli()
33+void SetupArgs()
static void SetupCliArgs. Including the static after #13163
30@@ -31,6 +31,41 @@ static bool fCreateBlank;
31 static std::map<std::string,UniValue> registers;
32 static const int CONTINUE_EXECUTION=-1;
33
34+void SetupArgs()
67- BITCOIN_QT
68-};
69+/**
70+ * Setup the arguments for gArgs
71+ */
72+void SetupArgs();
SetupServerArgs, since this is shared between bitcoind and the gui.
530@@ -531,6 +531,20 @@ WId BitcoinApplication::getMainWinId() const
531 return window->winId();
532 }
533
534+void SetupUIArgs()
static after #13163
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.
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.
808+ found = true;
809+ break;
810+ }
811+ }
812+ if (!found && !ignore_invalid_keys) {
813+ throw std::runtime_error(strprintf("Invalid configuration value %s", it->string_key.c_str()));
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.
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.
git checkout b819262a5f5b9b07b0375e5573d3e66b5c555aa8 && make check fails and breaks bisect.
git checkout 0ff0ea83ef4b1f1b94988e98602adc7648d114c4 && ./test/functional/test_runner.py fails and breaks bisect.
23@@ -24,14 +24,24 @@ static void ResetArgs(const std::string& strArg)
24
25 // Convert to char*:
26 std::vector<const char*> vecChar;
27- for (std::string& s : vecArg)
28+ for (std::string& s : vecArg) {
44 {
45 SetupBenchArgs();
46- gArgs.ParseParameters(argc, argv);
47+ std::string error;
48+ if (!gArgs.ParseParameters(argc, argv, error)) {
49+ fprintf(stderr,"Error parsing command line arguments: %s\n", error.c_str());
, (8 occurrences).
clang-format and run the https://github.com/bitcoin/bitcoin/tree/master/contrib/devtools#clang-format-diffpy script to preempt whitespace nitpicking.
463+ std::string arg_name = arg.first.second;
464+ size_t eq_index = arg_name.find('=');
465+ if (eq_index != std::string::npos) {
466+ arg_name.erase(eq_index);
467+ }
468+ if (arg_name == ArgsManagerHelper::GetArgWithoutNet(key) || (IsSwitchChar(key[0]) && key.size() == 1)) {
Maybe I’m missing something but I don’t understand the second condition for 2 reasons:
arg;key.size() == 1 means that key is just -.
- for bitcoin-cli. I moved the check for that outside of the loop.
796 m_config_args[strKey].clear();
797 } else {
798 m_config_args[strKey].push_back(strValue);
799 }
800+
801+ // Check that the arg is known
GetArgWithoutNet there.
GetArgWithoutNet.
315@@ -316,6 +316,18 @@ class ArgsManagerHelper {
316 }
317 return InterpretBool(found_result.second); // is set, so evaluate
318 }
319+
320+ // Get arg without network
321+ static inline std::string GetArgWithoutNet(const std::string& key)
322+ {
323+ size_t option_index = key.find('.');
324+ if (option_index == std::string::npos) {
Early return?
0if (option_index == std::string::npos) return key;
559- node->parseParameters(argc, argv);
560-
561 // Do not refer to data directory yet, this can be overridden by Intro::pickDataDirectory
562
563- /// 2. Basic Qt initialization (not dependent on parameters or configuration)
564+ /// 1. Basic Qt initialization (not dependent on parameters or configuration)
BitcoinApplication constructor on L573 does use gArgs.GetArg("-uiplatform".
I’m not sure about unsafe, but it certainly is not good practice to check for whether the argument is set before actually parsing the arguments. This also makes it so that the -uiplatform option is effectively ignored. It appears that I have been misled by the comment on this line.
How would you recommend that an error be shown to the user if they don’t set the correct arguments?
parseparameters.
594- }
595- if (show_debug || !arg.second.second) {
596- usage += HelpMessageOpt(arg.first.second, arg.second.first);
597+ std::string usage = "";
598+ for (const auto& arg_map : m_available_args) {
599+ if (arg_map.first == OptionsCategory::OPTIONS) usage += HelpMessageGroup(_("Options:"));
In commit “Use a struct for arguments and nested map for categories”:
This still violates the style guide; anything that has an else branch or more than a single statement in the then branch needs indentation/braces. What about using a switch here?
switch.
471+{
472+ bool found = false;
473+ for (const auto& arg_map : m_available_args) {
474+ for (const auto& arg : arg_map.second) {
475+ std::string arg_name = arg.first;
476+ size_t eq_index = arg_name.find('=');
AddArg instead, and would store the second part in Arg rather than in the key, this entire loop could be avoided and replaced with a map lookup.
146+ {
147+ std::string m_help_param;
148+ std::string m_help_text;
149+ bool m_debug_only;
150+
151+ Arg(std::string help_param, std::string help_text, bool debug_only) : m_help_param(help_param), m_help_text(help_text), m_debug_only(debug_only) {};
const std::string&
558+ eq_index = name.size();
559+ }
560+
561+ std::map<std::string, Arg>& arg_map = m_available_args[cat];
562+ assert(arg_map.count(name) == 0);
563+ arg_map.emplace(name.substr(0, eq_index), Arg(name.substr(eq_index, name.size() - eq_index), help, debug_only));
emplace returns a pair that indicates if there was an insertion. Could use that and avoid the count above.
477+ arg_no_net = std::string("-") + key.substr(option_index + 1, std::string::npos);
478+ }
479+
480+ bool found = false;
481+ for (const auto& arg_map : m_available_args) {
482+ found |= arg_map.second.find(arg_no_net) != arg_map.second.end();
if (arg_map.second.count(arg_no_net)) return true;.
This is beautiful ✨ , you almost timed this on my issue #1044’s sixth birthday, why didn’t I see this before.
0(13112)$ src/bitcoind -esfsfjkshf
1Error parsing command line arguments: Invalid parameter -esfsfjkshf
2(13112)$ src/bitcoind -sdfasdf -daemon
3Error parsing command line arguments: Invalid parameter -sdfasdf
4(13112)$ src/bitcoind -sdfasdf=2342984 -daemon
5Error parsing command line arguments: Invalid parameter -sdfasdf
6(13112)$ vim ~/.bitcoin/bitcoin.conf # introduce invalid option sfdsf
7(13112)$ src/bitcoind
8Error reading configuration file: Invalid configuration value sfdsf
9(13112)$ DISPLAY=:10.0 src/qt/bitcoin-qt -sdfs
10[gives a warning in GUI, after clicking OK it exits]
11Segmentation fault (core dumped)
12[#0](/bitcoin-bitcoin/0/) QString::~QString ()
13[#1](/bitcoin-bitcoin/1/) PlatformStyle::~PlatformStyle ()
14 at /.../bitcoin/src/qt/platformstyle.h:13
15[#2](/bitcoin-bitcoin/2/) BitcoinApplication::~BitcoinApplication (this=0x7fffffffdfb0) at /.../bitcoin/src/qt/bitcoin.cpp:357
16[#3](/bitcoin-bitcoin/3/) 0x00005555555d9d58 in main (argc=<optimized out>, argv=<optimized out>) at /.../bitcoin/src/qt/bitcoin.cpp:760
So everything works as expected, except there’s a segmentation fault in the GUI on early exit (not sure if this is new with this PR, but it’s triggered here).
Also needs rebase after #13341.
Instead of a single map with the category and name as the key,
make m_available_args contain maps. The key will be the category and
the value is a map which actually contains the arguments for that
category. The nested map's key is the argument name, while the value
is a struct that contains the help text and whether the argument is
a debug only argument.
If an unknown option is given via either the command line args or
the conf file, throw an error and exit
Update tests for ArgsManager knowing args
Ignore unknown options in the config file for bitcoin-cli
Fix tests and bitcoin-cli to match actual options used
This might break one or two Docker containers that use $NETWORK=1 where that’s either “testnet” or “bitcoin”. Cc @NicolasDorier I’ve seen that in some lightning related stuff, though not sure if that was yours.
Anyway, problems like that would be immediately obvious upon the first launch after upgrading.