Skip to content

Extend support for nested commands to bitcoin-cli #20273

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed

Conversation

jonasschnelli
Copy link
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.

@jonasschnelli
Copy link
Contributor Author

The first commit might help to avoid reviewing moved code.

@jonasschnelli jonasschnelli force-pushed the 2020/10/client_rpc_nested branch from 7d392d0 to 7587eed Compare October 30, 2020 16:25
@kristapsk
Copy link
Contributor

Tests are failing for me.

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

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

@jonasschnelli jonasschnelli force-pushed the 2020/10/client_rpc_nested branch 2 times, most recently from a614f7f to f48000e Compare October 31, 2020 16:20
@jonasschnelli
Copy link
Contributor Author

jonasschnelli commented Oct 31, 2020

@kristapsk: should be fixed now.
What currently is not working are nested commands in conjunction with descriptors. I'll work on a fix soon.

@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 3, 2020

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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.

@ajtowns
Copy link
Contributor

ajtowns commented Nov 9, 2020

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

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

You could use shell aliases to reduce typing:

$ bc () { bitcoin-cli "$@"; }
$ bcfield () { local F="$1"; shift; bitcoin-cli "$@" | jq -r "$F"; }
$ 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.

@jonasschnelli
Copy link
Contributor Author

@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).

@ajtowns
Copy link
Contributor

ajtowns commented Nov 9, 2020

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

@jonasschnelli
Copy link
Contributor Author

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

@jonatack
Copy link
Member

Concept ACK

1 similar comment
@theStack
Copy link
Contributor

theStack commented Jan 2, 2021

Concept ACK

@jonasschnelli jonasschnelli force-pushed the 2020/10/client_rpc_nested branch from c5053ff to 027ff50 Compare March 10, 2021 08:31
@jonasschnelli
Copy link
Contributor Author

Rebased

@ghost
Copy link

ghost commented Mar 11, 2021

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:

satoshi@ubuntu:~$ bitcoin-cli getblock(getblock(getbestblockhash())[previousblockhash])[tx][0]

bash: syntax error near unexpected token `('

With quotes:

satoshi@ubuntu:~$ bitcoin-cli "getblock(getblock(getbestblockhash())[previousblockhash])[tx][0]"

c06f7c267bae5c5994cd4584f930f4c4800170c4ecb96ebf6290b6956dc23303

@JeremyRubin
Copy link
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.

@ghost
Copy link

ghost commented Mar 11, 2021

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?

@kristapsk
Copy link
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.

@ghost
Copy link

ghost commented Mar 11, 2021

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"

@DrahtBot
Copy link
Contributor

🐙 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".

@DrahtBot
Copy link
Contributor

There hasn't been much activity lately and the patch still needs rebase. What is the status here?
  • 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.

@maflcko maflcko closed this Dec 15, 2021
@bitcoin bitcoin locked and limited conversation to collaborators Dec 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants