Extend support for nested commands to bitcoin-cli #20273

pull jonasschnelli wants to merge 6 commits into bitcoin:master from jonasschnelli:2020/10/client_rpc_nested changing 6 files +393 −281
  1. jonasschnelli commented at 4:17 pm on October 30, 2020: contributor

    Currently, only the GUI supports nested commands like getblock(getblock(getbestblockhash())[previousblockhash])[size] (just an example) (see #7783).

    This PR extends the support for nested commands to bitcoin-cli (entirely client side).

    See https://github.com/bitcoin/bitcoin/blob/0.20/src/qt/rpcconsole.cpp#L397 for more information.

  2. jonasschnelli added the label Scripts and tools on Oct 30, 2020
  3. jonasschnelli commented at 4:19 pm on October 30, 2020: contributor
    The first commit might help to avoid reviewing moved code.
  4. jonasschnelli force-pushed on Oct 30, 2020
  5. kristapsk commented at 6:26 pm on October 30, 2020: contributor

    Tests are failing for me.

    0rpc_deriveaddresses.py --usecli       | ✖ Failed  | 2 s
    1wallet_createwallet.py --usecli       | ✖ Failed  | 2 s
    2wallet_multiwallet.py --usecli        | ✖ Failed  | 2 s
    3wallet_watchonly.py --usecli          | ✖ Failed  | 1 s
    

    Failing with test_framework.authproxy.JSONRPCException: Requested wallet does not exist or is not loaded (-18).

  6. jonasschnelli force-pushed on Oct 31, 2020
  7. jonasschnelli force-pushed on Oct 31, 2020
  8. jonasschnelli commented at 4:22 pm on October 31, 2020: contributor
    @kristapsk: should be fixed now. What currently is not working are nested commands in conjunction with descriptors. I’ll work on a fix soon.
  9. jonasschnelli force-pushed on Nov 1, 2020
  10. jonasschnelli force-pushed on Nov 1, 2020
  11. DrahtBot commented at 11:15 am on November 3, 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:

    • #21430 (build: Add -Werror=implicit-fallthrough compile flag by hebasto)
    • #21415 (refactor: remove Optional & nullopt by fanquake)
    • #20452 (util: Replace use of locale dependent atoi(…) with locale-independent std::from_chars(…) (C++17) by practicalswift)
    • #16439 (cli/gui: support “@height” in place of blockhash for getblock on client side by ajtowns)

    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.

  12. ajtowns commented at 4:44 am on November 9, 2020: member

    The command line already lets you do nested calls natively, eg:

    0$ bitcoin-cli getblock $(bitcoin-cli getblockheader $(bitcoin-cli getbestblockhash) | jq -r .previousblockhash) | jq -r .size
    

    You could use shell aliases to reduce typing:

    0$ bc () { bitcoin-cli "$@"; }
    1$ bcfield () { local F="$1"; shift; bitcoin-cli "$@" | jq -r "$F"; }
    2$ bcfield size getblock $(bcfield previousblockhash getblockheader $(bc getbestblockhash))
    

    Personally, the only time it makes a difference to me is when I want to quickly query blocks around a particular height (eg for a in {100..200}; do bitcoin-cli getblockheader @$a; done | grep versionHex), so the approach in #16439 is quicker and easier (less typing and don’t have to deal with brackets). Otherwise I’m pretty much writing a script, and the extra typing/aliases is no big deal.

  13. jonasschnelli commented at 8:46 am on November 9, 2020: contributor

    @ajtowns: Good point. Yes. You can also do it with shell magic. Though you could also argue that writing a shell alias (or short script) could replace bitcoin-cli entirely (use cURL instead). Also for #16439, a shell alias/script would be trivial.

    I guess bitcoin-cli is a utility. Something that should allow one to perform a task quick and fast (to not make you use cURl or other low level http/RPC clients).

    This PR is not a replacement for #16439. It goes in a similar direction and may allow solve the same issues.

    IMO this PR extends the power-commands (nested options plus subelement access) we have in the GUI to the CLI which I think is desirable (also for further extensions).

  14. ajtowns commented at 12:27 pm on November 9, 2020: member

    @ajtowns: Good point. Yes. You can also do it with shell magic. Though you could also argue that writing a shell alias (or short script) could replace bitcoin-cli entirely (use cURL instead). Also for #16439, a shell alias/script would be trivial.

    More or less: the disadvantage of a shell alias (eg bitcoin-cli getblockheader $(bcbest)) is that the alias can’t inherit -conf, -datadir, -testnet, -signet, or -regtest from the “parent” bitcoin-cli command, while getblockheader(getbestblockhash) or @best will automatically adapt to the right context.

    For me, that matters when I’m doing a quick oneliner, but not when I’m doing anything more complicated. YMMV obviously.

  15. jonasschnelli commented at 7:26 am on November 10, 2020: contributor

    More or less: the disadvantage of a shell alias (eg bitcoin-cli getblockheader $(bcbest)) is that the alias can’t inherit -conf, -datadir, -testnet, -signet, or -regtest from the “parent” bitcoin-cli command, while getblockheader(getbestblockhash) or @best will automatically adapt to the right context.

    Indeed. That would also speak for this PR over shell aliases.

    I think #16439 could be integrated into this PR. There could be special commands like @best or @H(400000) that just translates into getbestblockhash() or getblockhash(400000).

  16. jonatack commented at 3:15 pm on December 17, 2020: member
    Concept ACK
  17. theStack commented at 4:43 pm on January 2, 2021: member
    Concept ACK
  18. DrahtBot added the label Needs rebase on Jan 27, 2021
  19. MOVEONLY: move RPCParseCommandLine to nested.cpp/.h (won't compile) d52fc07e6a
  20. Factor out RPC consoles ParseCommandLine. Add callbacks for flexibility e147b44f3a
  21. Add support for command nesting in bitcoin-cli 951e1b3f42
  22. avoid atoi in nested.cpp 28f3a40d41
  23. Use CHECK_NONFATAL() instead of assert() in nested.cpp 65ef219e6b
  24. Extend bitcoin-cli test to cover nested calls 027ff50331
  25. jonasschnelli force-pushed on Mar 10, 2021
  26. jonasschnelli commented at 8:40 am on March 10, 2021: contributor
    Rebased
  27. DrahtBot removed the label Needs rebase on Mar 10, 2021
  28. ghost commented at 0:12 am on March 11, 2021: none

    Concept ACK

    Compiled successfully on Ubuntu 20.04.2 LTS. Tests passed.

    I am not sure about the correct syntax for nesting so got error without quotes.

    Error:

    0satoshi@ubuntu:~$ bitcoin-cli getblock(getblock(getbestblockhash())[previousblockhash])[tx][0]
    1
    2bash: syntax error near unexpected token `('
    

    With quotes:

    0satoshi@ubuntu:~$ bitcoin-cli "getblock(getblock(getbestblockhash())[previousblockhash])[tx][0]"
    1
    2c06f7c267bae5c5994cd4584f930f4c4800170c4ecb96ebf6290b6956dc23303
    
  29. JeremyRubin commented at 1:30 am on March 11, 2021: contributor

    mixed feelings on this, general concept NACK – had no idea you could do this from the GUI, and I generally think the cli should be for dumb rpc calls, including a mini programming langauge seems to me to be asking for trouble.

    OTOH I do really like the concept of being able to atomically make RPC queries.

  30. ghost commented at 2:53 am on March 11, 2021: none

    including a mini programming langauge seems to me to be asking for trouble

    Security? Interesting point. Although GUI console has limited access and usage, can we try few things in it? Or enable this only for testnet, regtest etc. and experiment for few weeks/months?

  31. kristapsk commented at 3:02 pm on March 11, 2021: contributor

    I am not sure about the correct syntax for nesting so got error without quotes. @prayank23, that’s issue of shell, nothing we can do about here, just echo ( in bash will fail too.

  32. ghost commented at 6:44 pm on March 11, 2021: none

    that’s issue of shell, nothing we can do about here, just echo ( in bash will fail too.

    Okay. I was interested in this PR because nesting helps in creating scripts for specific tasks. I have used cmdlets in PowerShell, the pipe symbol is used when one cmdlet pipes a result to another cmdlet. For example,

    Get-MsolUser | Set-MsolUser -PasswordNeverExpires $true Will perform a task (Set one parameter true) on all the results returned in the first cmdlet.

    A similar thing in Bitcoin Core can be:

    listunspent 100 9999999 | setlabel "100 Conf UTXO"

  33. DrahtBot commented at 12:45 pm on March 17, 2021: member

    🐙 This pull request conflicts with the target branch and needs rebase.

    Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

  34. DrahtBot added the label Needs rebase on Mar 17, 2021
  35. DrahtBot commented at 11:21 am on December 15, 2021: member
    • Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
    • Is it no longer relevant? ➡️ Please close.
    • Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.
  36. MarcoFalke added the label Up for grabs on Dec 15, 2021
  37. MarcoFalke closed this on Dec 15, 2021

  38. DrahtBot locked this on Dec 15, 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-11-22 03:12 UTC

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