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

pull MarcoFalke wants to merge 4 commits into bitcoin:master from MarcoFalke:2012-argsCmd changing 7 files +115 −51
  1. 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

  2. MarcoFalke force-pushed on Dec 18, 2020
  3. MarcoFalke force-pushed on Dec 18, 2020
  4. DrahtBot added the label Utils/log/libs on Dec 18, 2020
  5. 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.

  6. MarcoFalke force-pushed on Dec 18, 2020
  7. practicalswift commented at 9:30 am on December 19, 2020: contributor
    Concept ACK
  8. 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
  9. 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
  10. 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.

  11. ajtowns commented at 7:59 am on January 12, 2021: member
    Concept ACK
  12. MarcoFalke force-pushed on Jan 13, 2021
  13. 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
  14. 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.

  15. MarcoFalke force-pushed on Jan 13, 2021
  16. DrahtBot added the label Needs rebase on Jan 14, 2021
  17. MarcoFalke force-pushed on Jan 14, 2021
  18. DrahtBot removed the label Needs rebase on Jan 14, 2021
  19. laanwj added this to the "Blockers" column in a project

  20. 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.
  21. 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
  22. 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
  23. 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

  24. 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
  25. 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
  26. 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
  27. 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?

  28. laanwj commented at 12:53 pm on January 20, 2021: member
    Concept ACK
  29. MarcoFalke force-pushed on Jan 21, 2021
  30. DrahtBot added the label Needs rebase on Jan 21, 2021
  31. wallet: [refactor] Pass ArgsManager to WalletAppInit fac05ccdad
  32. test: Add tests fa06bce4ac
  33. refactor: Move all command dependend checks to ExecuteWalletToolFunc 7777105a24
  34. util: Add ArgsManager::GetCommand() and use it in bitcoin-wallet
    Co-Authored-by: Anthony Towns <aj@erisian.com.au>
    fa61b9d1a6
  35. MarcoFalke force-pushed on Jan 21, 2021
  36. DrahtBot removed the label Needs rebase on Jan 21, 2021
  37. 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
  38. 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.
  39. fjahr approved
  40. fjahr commented at 11:41 pm on January 23, 2021: member

    Code review ACK fa61b9d1a68820758f9540653920deaeae6abe79

    Feel free to ignore my nits.

  41. 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

  42. MarcoFalke merged this on Feb 4, 2021
  43. MarcoFalke closed this on Feb 4, 2021

  44. 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.

  45. MarcoFalke deleted the branch on Feb 4, 2021
  46. sidhujag referenced this in commit 921561a5e1 on Feb 4, 2021
  47. laanwj removed this from the "Blockers" column in a project

  48. DrahtBot locked this on Aug 16, 2022

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-08 16:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me