Throw an error for unknown args #13112

Check if already exists with static assert?

Done

nit: Please make this enum class

Done

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.

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.

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.

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

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:

<details><summary></summary>

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;

</details>

nit: Should be static after #13163

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

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

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

nit: Should be static after #13163

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.

nit unrelated change but if you keep add const?

Removed unrelated change

nit, space after , (8 occurrences).

Note: You can install clang-format and run the https://github.com/bitcoin/bitcoin/tree/master/contrib/devtools#clang-format-diffpy script to preempt whitespace nitpicking.

clang-formatted

Maybe I'm missing something but I don't understand the second condition for 2 reasons:

1. could be out of the loop since it doesn't depend on arg;
2. key.size() == 1 means that key is just -.

Yeah, we need to ignore a key of - for bitcoin-cli. I moved the check for that outside of the loop.

Can you move this to a function to avoid duplicate code?

If you do so then I think you can inline GetArgWithoutNet there.

Made it into a function and inlined GetArgWithoutNet.

Early return?

if (option_index == std::string::npos) return key;

Done

Are you sure this is safe? I haven't tested, but following the code, 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?

Checking command line arguments in a constructor generally seems to be bad practice, as it becomes pretty hard to reason about ordering. Is there a way that can be avoided (by passing in the argument value into the constructor instead?).

To get around this problem, I moved the platform style setup out of the constructor and set it up after parseparameters.

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?

Replaced with a switch.

If you'd move this "="-splitting to 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.

Done.

2x const std::string&

Done

emplace returns a pair that indicates if there was an insertion. Could use that and avoid the count above.

Done

Faster: if (arg_map.second.count(arg_no_net)) return true;.

Done