util: Add ArgsManager::GetCommand() and use it in bitcoin-wallet

MarcoFalke commented at 2:07 pm on December 18, 2020: member

This not only moves the parsing responsibility out from the wallet tool, but it also makes it easier to implement bitcoin-util #19937

Fixes: #20902

MarcoFalke force-pushed on Dec 18, 2020

DrahtBot added the label Utils/log/libs on Dec 18, 2020

DrahtBot commented at 4:57 pm on December 18, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#21052 (refactor: Replace fs::unique_path with GetUniquePath(path) calls by kiminuo)
#19460 (multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

MarcoFalke force-pushed on Dec 18, 2020

practicalswift commented at 9:30 am on December 19, 2020: contributor

Concept ACK

in src/util/system.cpp:539 in faaf1b26c1 outdated

532@@ -518,6 +533,11 @@ void ArgsManager::AddArg(const std::string& name, const std::string& help, unsig
533     auto ret = arg_map.emplace(arg_name, Arg{name.substr(eq_index, name.size() - eq_index), help, flags});
534     assert(ret.second); // Make sure an insertion actually happened
535 
536+    if (flags & ArgsManager::COMMAND) {
537+        Assert(flags == ArgsManager::COMMAND); // Combination not allowed
538+        Assert(name.find('=') == std::string::npos);
539+        Assert(name.find('-') == std::string::npos);

ajtowns commented at 5:04 am on January 12, 2021:

Why ban a dash anywhere in the command, rather than just at the start?

MarcoFalke commented at 9:14 am on January 13, 2021:

Thanks, fixed

in src/util/system.cpp:322 in faaf1b26c1 outdated

319+            if (!flags || !(*flags & ArgsManager::COMMAND)) {
320+                error = strprintf("Invalid command '%s'", argv[i]);
321+                return false;
322+            }
323+            m_commands.push_back(key);
324+            continue;

ajtowns commented at 7:50 am on January 12, 2021:

I’m not sure multiple (sub)commands makes sense for us. At present, I think we have:

bitcoind - doesn’t take subcommands
bitcoin-wallet - wants a single subcommand
bitcoin-tx - wants multiple instructions with parameters
bitcoin-cli - wants to do its own parsing of stuff after the options
bitcoin-qt - does its own parsing of stuff after the options for payment server support

For bitcoin-util, I think we want it to be more like bitcoin-wallet, and if we were to do a psbt version of bitcoin-tx’s multiple commands in order to construct a psbt, I think that would look like bitcoin-util psbt <subsubcmd> <subsubcmd> ...

So I don’t think it makes much sense to support multiple subcommands, particularly without letting them have their own params. Which I think means you could have a nicer implementation with something like https://github.com/ajtowns/bitcoin/commits/202101-getcommand which lets you just say AddCmd(...) to start introducing new commands, don’t have to explicitly set accept_any_command when parsing, and can just invoke GetCommand() and GetArguments() to get the pre-parsed results. Thoughts?

I’ve added a second commit on that branch that allows the ArgsManager options to come after the command, which is the same as this PR, but differs from current master (which leaves later options up to the command – so bitcoin-cli treats them as rpc args, while bitcoin-wallet currently just silently ignores them).

MarcoFalke commented at 9:14 am on January 13, 2021:

Thanks, took most of your changes

in src/bitcoin-wallet.cpp:40 in faaf1b26c1 outdated

40-    argsman.AddArg("createfromdump", "Create new wallet file from dumped records", ArgsManager::ALLOW_ANY, OptionsCategory::COMMANDS);
41+    argsman.AddArg("info", "Get wallet info", ArgsManager::COMMAND, OptionsCategory::COMMANDS);
42+    argsman.AddArg("create", "Create new wallet file", ArgsManager::COMMAND, OptionsCategory::COMMANDS);
43+    argsman.AddArg("salvage", "Attempt to recover private keys from a corrupt wallet. Warning: 'salvage' is experimental.", ArgsManager::COMMAND, OptionsCategory::COMMANDS);
44+    argsman.AddArg("dump", "Print out all of the wallet key-value records", ArgsManager::COMMAND, OptionsCategory::COMMANDS);
45+    argsman.AddArg("createfromdump", "Create new wallet file from dumped records", ArgsManager::COMMAND, OptionsCategory::COMMANDS);

ajtowns commented at 7:57 am on January 12, 2021:

I think this would be a lot better if we could link options to commands (so -format could be tied to createfromdump directly rather than having it hidden in the help text and having a special cased if statement). Some options would need to be available for multiple (but not all) commands (-dumpfile for dump and createfromdump eg).

This seems like a bit more of an intrusive change to make, and probably doesn’t have to be done immediately though.

MarcoFalke commented at 8:13 am on January 12, 2021:

This seems like a bit more of an intrusive change to make, and probably doesn’t have to be done immediately though.

Indeed. Will leave this for a follow-up. This should be a minimal bugfix/feature commit.

ajtowns commented at 7:59 am on January 12, 2021: member

Concept ACK

MarcoFalke force-pushed on Jan 13, 2021

MarcoFalke renamed this:
~~util: Add ArgsManager::GetCommands() and use it in bitcoin-wallet~~
util: Add ArgsManager::GetCommand() and use it in bitcoin-wallet
on Jan 13, 2021

DrahtBot commented at 11:46 am on January 13, 2021: member

🕵️ @jonatack has been requested to review this pull request as specified in the REVIEWERS file.

MarcoFalke force-pushed on Jan 13, 2021

DrahtBot added the label Needs rebase on Jan 14, 2021

MarcoFalke force-pushed on Jan 14, 2021

DrahtBot removed the label Needs rebase on Jan 14, 2021

laanwj added this to the "Blockers" column in a project

in src/util/system.cpp:377 in fad369f043 outdated

373@@ -359,6 +374,19 @@ Optional<unsigned int> ArgsManager::GetArgFlags(const std::string& name) const
374     return nullopt;
375 }
376 
377+bool ArgsManager::GetCommand(std::string& cmd, std::vector<std::string>& args) const

ajtowns commented at 6:11 am on January 15, 2021:

Might be good to call it AddCommand and GetCommand for consistency?

MarcoFalke commented at 12:57 pm on January 21, 2021:

I took the naming from https://github.com/ajtowns/bitcoin/commit/ff48eb93bd485fb5e722d47690508f2a3506d30c. Nonetheless, changed in the latest force push.

in src/util/system.cpp:380 in fad369f043 outdated

373@@ -359,6 +374,19 @@ Optional<unsigned int> ArgsManager::GetArgFlags(const std::string& name) const
374     return nullopt;
375 }
376 
377+bool ArgsManager::GetCommand(std::string& cmd, std::vector<std::string>& args) const
378+{
379+    LOCK(cs_args);
380+    assert(!m_accept_any_command); // AddCmd must have been called before this

ajtowns commented at 6:12 am on January 15, 2021:

I wonder if it wouldn’t be better to always copy the remaining args into m_command and have the only difference be that you get an “unknown command” when m_accept_any_command is set

MarcoFalke commented at 12:57 pm on January 21, 2021:

Thanks. Fixed, I think

in src/bitcoin-wallet.cpp:100 in fad369f043 outdated

107-    }
108-
109-    if (method.empty()) {
110+    std::string method;
111+    std::vector<std::string> cmdargs;
112+    if (!args.GetCommand(method, cmdargs)) {

ajtowns commented at 6:15 am on January 15, 2021:

This feels a bit messy; when trying this for bitcoin-util, I added a struct CmdArg { string cmd; vector<string> args; }; to track them both.

MarcoFalke commented at 12:57 pm on January 21, 2021:

Thanks. Fixed, I think

ajtowns commented at 6:17 am on January 15, 2021: member

ACK fad369f04361e2e3a834589b63fab160b9b4e1b2

Looks good; comments are just ideas for consideration. Patch for bitcoin-util at https://github.com/ajtowns/bitcoin/commits/202101-util-addcmd

in src/bitcoin-wallet.cpp:105 in fad369f043 outdated

112+    if (!args.GetCommand(method, cmdargs)) {
113         tfm::format(std::cerr, "No method provided. Run `bitcoin-wallet -help` for valid methods.\n");
114         return EXIT_FAILURE;
115     }
116+    if (cmdargs.size() != 0) {
117+        tfm::format(std::cerr, "Error: Additional arguments provided (%s). Please refer to `-help`. Commands do not take arguments.\n", Join(cmdargs, ", "));

fjahr commented at 8:20 pm on January 16, 2021:

nit: I would slightly prefer it this way around:

0tfm::format(std::cerr, "Error: Additional arguments provided (%s). Commands do not take arguments. Please refer to `-help`. \n", Join(cmdargs, ", "));

MarcoFalke commented at 12:58 pm on January 21, 2021:

Thanks, fixed

in src/bitcoin-wallet.cpp:98 in fad369f043 outdated

105-            method = argv[i];
106-        }
107-    }
108-
109-    if (method.empty()) {
110+    std::string method;

fjahr commented at 8:21 pm on January 16, 2021:

nit: Rename to command?

MarcoFalke commented at 12:58 pm on January 21, 2021:

Thanks, fixed

in src/bitcoin-wallet.cpp:100 in fad369f043 outdated

108-
109-    if (method.empty()) {
110+    std::string method;
111+    std::vector<std::string> cmdargs;
112+    if (!args.GetCommand(method, cmdargs)) {
113         tfm::format(std::cerr, "No method provided. Run `bitcoin-wallet -help` for valid methods.\n");

fjahr commented at 8:22 pm on January 16, 2021:

maybe also rename “method” to “command” here?

MarcoFalke commented at 12:58 pm on January 21, 2021:

Seems unrelated, so I am going to leave as is for now to minimize the diff

fjahr commented at 1:26 pm on January 17, 2021: member

Code review ACK fad369f04361e2e3a834589b63fab160b9b4e1b2

This works, however I liked @ajtowns comments and I think consistency whether method or command is used would be good. It seems method is getting replaced by command, is that a general naming scheme change?

laanwj commented at 12:53 pm on January 20, 2021: member

Concept ACK

MarcoFalke force-pushed on Jan 21, 2021

DrahtBot added the label Needs rebase on Jan 21, 2021

wallet: [refactor] Pass ArgsManager to WalletAppInit fac05ccdad

test: Add tests fa06bce4ac

refactor: Move all command dependend checks to ExecuteWalletToolFunc 7777105a24

util: Add ArgsManager::GetCommand() and use it in bitcoin-wallet

Co-Authored-by: Anthony Towns <aj@erisian.com.au>

fa61b9d1a6

MarcoFalke force-pushed on Jan 21, 2021

DrahtBot removed the label Needs rebase on Jan 21, 2021

in test/functional/tool_wallet.py:191 in fa06bce4ac outdated

187@@ -188,6 +188,8 @@ def test_invalid_tool_commands_and_args(self):
188         self.assert_raises_tool_error('Invalid command: help', 'help')
189         self.assert_raises_tool_error('Error: two methods provided (info and create). Only one method should be provided.', 'info', 'create')
190         self.assert_raises_tool_error('Error parsing command line arguments: Invalid parameter -foo', '-foo')
191+        self.assert_raises_tool_error('No method provided. Run `bitcoin-wallet -help` for valid methods.')

fjahr commented at 11:29 pm on January 23, 2021:

in fa06bce4ac17f93decd4ee38c956e7aa55983f0d:

nit: Commit message could have been a bit more descriptive ;)

MarcoFalke commented at 8:16 am on January 24, 2021:

Thanks, will change on the next push

in src/util/system.h:254 in fa61b9d1a6

250@@ -248,6 +251,20 @@ class ArgsManager
251      */
252     const std::list<SectionInfo> GetUnrecognizedSections() const;
253 
254+    struct Command {

fjahr commented at 11:38 pm on January 23, 2021:

in fa61b9d1a68820758f9540653920deaeae6abe79:

nit: seeing this now I think I would have named it CommandCall or something similar to express that it’s command + args. But feel free to ignore my name bikeshedding.

MarcoFalke commented at 8:16 am on January 24, 2021:

#20715 (review) names it CmdArg. So I could call it CommandArg or CommandArgs (because there might be multiple args to the command). Not sure what to do here, but I think I’ll leave this as is for now and let the naming committee determine a suitable name and let them change it after merge.

fjahr approved

fjahr commented at 11:41 pm on January 23, 2021: member

Code review ACK fa61b9d1a68820758f9540653920deaeae6abe79

Feel free to ignore my nits.

ajtowns commented at 5:27 am on February 4, 2021: member

ACK fa61b9d1a68820758f9540653920deaeae6abe79

Looks good to me, though the third commit has a typo in the description: “dependend”. Updated https://github.com/ajtowns/bitcoin/commits/202101-util-addcmd

MarcoFalke merged this on Feb 4, 2021

MarcoFalke closed this on Feb 4, 2021

MarcoFalke commented at 8:13 am on February 4, 2021: member

third commit has a typo in the description: “dependend”.

Good catch. Though, I’ll leave this as-is to minimize the re-review backlog.

MarcoFalke deleted the branch on Feb 4, 2021

sidhujag referenced this in commit 921561a5e1 on Feb 4, 2021

laanwj removed this from the "Blockers" column in a project

DrahtBot locked this on Aug 16, 2022

util: Add ArgsManager::GetCommand() and use it in bitcoin-wallet #20715

Conflicts