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

 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

 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

  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.

 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.

 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

 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

 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:

tfm::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

 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

 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

 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

 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.