cli/gui: support "@height" in place of blockhash for getblock on client side

ajtowns commented at 3:12 am on July 23, 2019: member

Update bitcoin-cli and gui debug console so that RPC calls that accept a block hash to reference a block will also accept a block by height prefixed by @, or @best to reference the tip. The clients will just make an RPC call to getblockhash or getbestblockhash to replace the argument with the hash before making the requested RPC call.

Affected RPCs:

getblockheader
getblock
preciousblock
invalidateblock
getchaintxstats
getblockfilter
getrawtransaction
gettxoutproof

The RPC calls waitforblock, and reconsiderblock are unaffected because specifying a blockhash already in the main chain for those calls is not useful, and getblockstats is unaffected because it already accepts a height.

fanquake added the label RPC/REST/ZMQ on Jul 23, 2019

fanquake added the label Needs Conceptual Review on Jul 23, 2019

ajtowns commented at 3:25 am on July 23, 2019: member

This is an alternate approach to #16345 . It may solve #15412 and remove the desire to drop client-side arg validation per #15448 . Previous discussion (as pointed out in 16345) is in #16317, #14858 and #8457.

There’s a bunch of other ways of doing this:

just keep requiring a call to getblockhash N first – this is less convenient and slower
use the type of the parameter to distinguish heights and hashes – this is what getblockstats does, but it means the type of the parameter is ambiguous which goes back to #15412
add an additional height parameter to the RPC functions and make blockhash or height be alternatives – I tried this at https://github.com/ajtowns/bitcoin/commit/cf0868719748e2bb4f0394924245d4212d0f4260 but it’s more code and not very convenient to use
duplicate the RPC calls that take a blockhash and add “byheight” variants – this is what 16345 does, but it’s a fair chunk of duplicated code, which has received NACKs (see #16345 (comment) and #16345 (comment) )
MarcoFalke suggested that aliases would cover the convenience factor and be useful for other things like “generate”, but notes this isn’t really possible in bitcoin-qt without a lot of work
laanwj suggested that we could add pipelining to json, so a single request would call two RPCs passing the results of the first into the second. This seems pretty complicated, and the cli and gui would need to define some sort of shortcut mechanism to make that accessible as far as I can see.

This way isn’t much code, particularly for supporting more generally, and while having to turn the number into a string is a bit annoying in code, it’s not that bad, and very easy from the command line or the GUI. So up for consideration

(edited to add json pipelining suggestion)

DrahtBot commented at 6:39 am on July 23, 2019: 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:

#19949 (cli: Parse and allow hash value by fjahr)
#19762 (rpc: Allow named and positional arguments to be used together 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.

laanwj commented at 1:34 pm on July 24, 2019: member

I still don’t like overloading the meaning of the parameter, but I’ll stop repeating myself …

(also theoretically this could mess with software that assumes that only valid block hashes would be accepted by the RPC, though passing unchecked user input directly to RPC is arguably a bad idea…)

ajtowns commented at 2:42 pm on July 24, 2019: member

I still don’t like overloading the meaning of the parameter, but I’ll stop repeating myself …

So leave things as-is including the exception for getblockstats hash_or_height and consider this a wontfix, or do any of the other approaches seem appealing? Could be plausible to teach bitcon-cli to accept a 64-byte hex string or a number and make the hash_or_height str-vs-num work a bit better, I think, but everything else seems a lot of hassle to me.

promag commented at 0:43 am on July 25, 2019: member

@ajtowns do you see advantages of these approaches beside being faster to write or slightly fast execution (compared to also calling getblockhash)?

If this goes forward, wouldn’t it be better to prefix with # instead? Or maybe just check the input because an hash can never be an height.

ajtowns commented at 8:31 am on July 25, 2019: member

@ajtowns do you see advantages of these approaches beside being faster to write or slightly fast execution (compared to also calling getblockhash)?

It’s a fair bit less complexity for the person asking for something – you don’t have to cut and paste a blockhash, or have a $( .. ) subcommand in shell, or remember what the name of the getblockhash rpc is. The speed difference is really just a side-effect of that, if you’re making lots of getblock calls, you can lookup the blockhash once, and use the nextblockhash or previousblockhash to avoid extra calls after that, but again it’s more complexity.

If this goes forward, wouldn’t it be better to prefix with # instead?

Did think about that, but # is a comment character in shell, so bitcoin-cli getblock [#500000](/bitcoin-bitcoin/500000/) would need quotes which seems annoying. I think “getblock at 500000” reads pretty well anyway though.

Or maybe just check the input because an hash can never be an height.

Using a non-hex prefix makes it pretty unambiguous you don’t mean a block hash. At least in theory you could have a block with hash 0000000000000000000000000000000000000000000000000000000000512345 eg, and misinterpret it. Also, see #8457 (comment)

sipa commented at 5:01 pm on July 29, 2019: member

Weak concept ACK.

I understand @laanwj’s point of view: ideally, the RPC server code implements simple unambiguous primitive operations, and complexity of how to make them interact is offloaded to the client. In places where user friendliness for CLI users conflicts with that, it can be implemented as extra functionality in bitcoin-cli.

On the other hand, this approach seems actually simpler in total code complexity than having various hooks in bitcoin-cli for intercepting heights and requesting their hashes, and this is a frequently requested feature.

Perhaps an analogy is git’s syntax for specifying commits, where hashes can be provided, but also branch names, and even simple operators on other commits like ~.

ryanofsky commented at 7:16 pm on July 29, 2019: member

Concept ACK. I didn’t follow previous discussions, but I like the @ syntax, think it is simpler and more convenient than the other duplicating / aliasing / pipelining suggestions, and don’t see a real potential for misuse. (Apologies if I missed any other practical objections in previous discussion, they mostly seemed like aesthetic complaints.)

in src/rpc/blockchain.cpp:905 in 792c2a5dbc outdated

842@@ -820,7 +843,7 @@ static UniValue getblock(const JSONRPCRequest& request)
843     RPCHelpMan{"getblock",
844                 "\nIf verbosity is 0, returns a string that is serialized, hex-encoded data for block 'hash'.\n"
845                 "If verbosity is 1, returns an Object with information about block <hash>.\n"
846-                "If verbosity is 2, returns an Object with information about block <hash> and information about each transaction. \n",
847+                "If verbosity is 2, returns an Object with information about block <hash> and information about each transaction.\n",
848                 {
849                     {"blockhash", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The block hash"},

MarcoFalke commented at 7:22 pm on July 29, 2019:

STR_HEX no longer applies and this should be a new type, similar to AMOUNT

jnewbery commented at 3:13 pm on July 30, 2019: member

Weak concept ACK, for exactly the same reasons that sipa gives here: #16439 (comment)

jonasschnelli commented at 6:38 pm on July 30, 2019: contributor

I could live with that concept,… though I still think it’s client functionality.

What if the client (bitcoin-cli) does an extra call for @<height> which would be getblockhash<height>?

For a getblock @<height>, bitcoin-cli would acctually do getblock(getblockhash(<height>))?

I think it’s worth to extent bitcoin-cli for the hight-shortcut but I kida think it’s wasted effort and unnecesarry risks for the daemon.

laanwj commented at 8:11 am on July 31, 2019: member

I understand @laanwj’s point of view: ideally, the RPC server code implements simple unambiguous primitive operations,

Thanks… you’re formulating it better than me

To be clear: it’s not that I dislike the syntax. I’m not NACKing this. I think this would be useful from the command line. But I think doing this on the server side is the wrong way to go. I think this is the wrong direction.

Please also look at it from the perspective of someone writing a RPC wrapper in a static language, say, rust. After this change, the parameter needs to be an enum() which allows for multiple kinds of data, instead of a BlockHash.

promag commented at 8:22 am on July 31, 2019: member

One problem with this is that the height can refer to a different block in a sequence of calls:

0foo [@height](/bitcoin-bitcoin/contributor/height/)
1# a wild reorg
2bar [@height](/bitcoin-bitcoin/contributor/height/)
3# bar operates on a different block than foo

ajtowns commented at 2:14 pm on July 31, 2019: member

One problem with this is that the height can refer to a different block in a sequence of calls: foo [@height](/bitcoin-bitcoin/contributor/height/); reorg; bar [@height](/bitcoin-bitcoin/contributor/height/)

Isn’t that to be expected and not particularly surprising? You get the same behaviour now if you call foo $(getblockhash N); reorg; bar $(getblockhash N).

What if the client (bitcoin-cli) does an extra call for @ which would be getblockhash?

I think you’d have to define a new table “convert height to blockhash params” in rpc/client.cpp as otherwise you’d do weird things like convert a wallet label from “@123” into a block 123’s hash. That in turn means you have to keep your client in sync with the server, more than you would if it was just parsed on the server.

I’ve added in a quick hack to bitcoin-cli to support getblock %height purely on the client side, which actually doesn’t look too bad to me. But taking a quick look at qt/rpcconsole.cpp made my eyes bleed, so will leave trying to make that work until later… The disadvantage of this approach is that every client needs code to support the @ shortcut; so if you’re writing an RPC browser in rust, you’ll have more code to write than if it was just supported on the server side directly. The advantage is it works as soon as you have the updated client, no daemon upgrade/restart needed, which is pretty nice.

Please also look at it from the perspective of someone writing a RPC wrapper in a static language, say, rust. After this change, the parameter needs to be an enum() which allows for multiple kinds of data, instead of a BlockHash.

Note this is already the case for hash_or_height in getblockstats, except that it’s “string or numeric” not just two different types of string.

Are there any other shortcuts like this that seem useful? git comparisons like “foo^” for a parent or ancestor don’t seem like they’d be that useful for blocks if you’re already able to query by height easily, and short-references (ie, just specifying 8 digits if that’s enough to reference a unique id) don’t seem better than the height, and aren’t convenient when the first digits are all zero…

MarcoFalke commented at 3:05 pm on July 31, 2019: member

Please also look at it from the perspective of someone writing a RPC wrapper in a static language, say, rust. After this change, the parameter needs to be an enum() which allows for multiple kinds of data, instead of a BlockHash.

It doesn’t have to be an enum, it can also be a plain blockhash (hex string) and not support the user-facing @123 syntax.

promag commented at 3:13 pm on July 31, 2019: member

Isn’t that to be expected and not particularly surprising? You get the same behaviour

No, you would call getblockhash once and then all other calls would just use that hash.

ryanofsky commented at 3:30 pm on July 31, 2019: member

re: #16439 (comment)

Please also look at it from the perspective of someone writing a RPC wrapper in a static language, say, rust. After this change, the parameter needs to be an enum() which allows for multiple kinds of data, instead of a BlockHash.

Could someone expand on this more? I don’t understand this comment, and don’t understand in general why people seem to like the idea of moving logic from the server to the client. Can someone be specific about an actual advantage that would come from moving logic from server to client, or be clear about what types of logic should be moved? Should every type of logic that it’s possible to move be moved, or is there a different criteria for deciding what belongs in bitcoind vs bitcoin-cli?

If I’m writing a rust client or any other type of client, it would seem like the more logic there is in bitcoin-cli, the more work I have to do in rust to get access to the same functionality, and the greater chance I have of introducing bugs or inconsistencies.

Putting more logic in bitcoin-cli would also seem bad for testing and usability. Right now bitcoin-cli is a minimal wrapper around the RPC interface, so it’s very easy to test things on command line, and then translate command line calls to real code (definitely in python and javascript, and I’m pretty sure it should be similar with rust).

Putting more logic in bitcoin-cli also has some practical problems right now (though these are solvable):

bitcoin-cli has less knowledge of rpc methods and parameters than bitcoind does. So a lot of the transformations you could imagine implementing client side aren’t really possible with current information.
bitcoind and bitcoin-cli aren’t currently doing any version checking, and there aren’t currently any consequences from using an old bitcoin-cli with a new bitcoind or vice versa. If we start adding more logic to bitcoin-cli, interaction across versions will start being something that we’ll want to think about (or prevent).

re: #16439 (comment)

Isn’t that to be expected and not particularly surprising? You get the same behaviour

No, you would call getblockhash once and then all other calls would just use that hash.

This seems like an argument not only for requiring hashes server side, but also for requiring hashes client side, and against both this PR and #16345. I don’t have much opinion on it, but just wanted to be clear about the implications. Maybe it would be a big inconvenience, or maybe it would prevent mistakes by users who forget about the possibilities of reorgs.

promag commented at 3:37 pm on July 31, 2019: member

This seems like an argument not only for requiring hashes server side, but also for requiring hashes client side, and against both this PR and #16345.

~~Yes~~ yes for this PR, not much for the other. This one promotes calls by height which don’t take into account reorgs.

MarcoFalke commented at 3:49 pm on July 31, 2019: member

User should be assumed to know what a reorg is or be educated about them in the rpc help text, imo.

laanwj commented at 4:38 pm on July 31, 2019: member

Could someone expand on this more? I don’t understand this comment, and don’t understand in general why people seem to like the idea of moving logic from the server to the client

The point is that use of the interface will usually be automatic/programmatic. Manual use (though a cli, in whatever programming language) is only a small subset of uses of the RPC interface.

Features that are only useful for manual use (such as this one) should go into the client. Every programmatic use can just as easily call getblockhash to get the hash first and go on with that. That’s even more stable with regard to reorgs (@promag’s scenario).

It seems it needs to be said often enough: the RPC interface does not exist for bitcoin-cli, or for end users. It’s an API for applications to use bitcoin core. If you want to make things friendlier for end-users, the place to do so is not usually the server side.

luke-jr commented at 4:47 pm on July 31, 2019: member

Concept NACK. The RPC interface is for software, not human interaction. The decision to not support heights in places of hashes was intentional.

in src/bitcoin-cli.cpp:444 in 817a8d2e42 outdated

439@@ -440,6 +440,21 @@ static int CommandLineRPC(int argc, char *argv[])
440         const bool fWait = gArgs.GetBoolArg("-rpcwait", false);
441         do {
442             try {
443+                if (method == "getblock") {
444+                    if (args.size() >= 1 && args[0][0] == '%') {

jamesob commented at 4:54 pm on July 31, 2019:

Shouldn’t this be == '@' or am I missing something?

ajtowns commented at 5:12 am on August 1, 2019:

I figured having “@123” exercise the server-side code and “%123” exercise the client-side code would be easier while trying the two approaches out.

jamesob commented at 4:55 pm on July 31, 2019: member

Concept ACK. This seems like a nice convenience that doesn’t introduce any ambiguity or much complexity.

promag commented at 5:14 pm on July 31, 2019: member

Honestly I don’t understand what is so inconvenient.

Here’s some more problems/inconsistencies:

-blocknotify and ZMQ messages notify the block hash, not height, so I’d say most of the time you end up using the hash.

Some (at least getchaintxstats) have the parameter named as blockhash and IMO blockhash=@1000 is inconsistent.

Also, consider this: https://github.com/bitcoin/bitcoin/blob/7821821a23b68cc9ec49d69829ad4c939cb762e8/test/functional/rpc_blockchain.py#L143-L146 You would have to call getblockhash anyway otherwise you wouldn’t be able to call reconsiderblock.

MarcoFalke commented at 7:57 pm on July 31, 2019: member

Would be nice if the gui supported this, even when it is not added to bitcoind

promag commented at 9:26 pm on July 31, 2019: member

@MarcoFalke GUI console supports getblock(getblockhash(1)). Are you suggesting getblock(@1) to result in the previous expression?

jnewbery commented at 4:01 pm on August 6, 2019: member

Restating my Concept ACK here. I prefer the @height on server approach to the %height in client approach.

I’ve skimmed the code and it looks good. Will fully review once commits are cleaned up and this is no longer a draft.

laanwj referenced this in commit d87fae9bde on Aug 26, 2019

ch4ot1c commented at 4:41 am on September 20, 2019: contributor

I think this would make newer developers (rpc consumers) significantly more prone to making errors, particularly missed reorg handling. Though, I’ve experienced the same usage inconveniences.

ajtowns commented at 12:41 pm on January 9, 2020: member

It doesn’t look like there’s anything like consensus to do this on the server side, so I’ve updated the patchset to do @1234 on the client side by doing an extra getblockhash RPC call prior to whatever the actually desired RPC call is. Leaving it as WIP for now since it still needs some docs, better commit messages, and so on.

ajtowns force-pushed on Jan 9, 2020

ajtowns force-pushed on May 28, 2020

ajtowns marked this as ready for review on May 28, 2020

ajtowns renamed this:
~~RPC: support "@height" in place of blockhash for getblock etc~~
cli/gui: support "@height" in place of blockhash for getblock on client side
on May 28, 2020

jonatack commented at 9:56 am on June 3, 2020: member

Concept ACK on the new client-side version; on my review shortlist. Will likely learn something useful for #19133.

DrahtBot added the label Needs rebase on Jun 21, 2020

ajtowns force-pushed on Jun 22, 2020

ajtowns commented at 2:18 am on June 22, 2020: member

Rebased, cleaned up commit slightly, removed unnecessary whitespace change that caused the rebase conflict

DrahtBot removed the label Needs rebase on Jun 22, 2020

in src/rpc/client.h:21 in 93e962bc7c outdated

13@@ -14,6 +14,12 @@ UniValue RPCConvertValues(const std::string& strMethod, const std::vector<std::s
14 /** Convert named arguments to command-specific RPC representation */
15 UniValue RPCConvertNamedValues(const std::string& strMethod, const std::vector<std::string>& strParams);
16 
17+/** Whether this is a positional blockhash parameter */
18+bool RPCConvertBlockhash(const std::string& strMethod, int pos);
19+
20+/** Whether this is a named blockhash parameter */
21+bool RPCConvertNamedBlockhash(const std::string& strMethod, std::string& strParam);

jonatack commented at 10:23 am on June 22, 2020:

79a42cc style nit, for CRPCConvertParam , CRPCConvertTable, and the params of these new bools, this PR is following the naming conventions in this file, if you retouch perhaps update to the conventions in developer-notes.md:

“Do not prefix class names with C”
“Variable (including function arguments) and namespace names are all lowercase and may use _ to separate words (snake_case).”

ajtowns commented at 5:48 am on July 13, 2020:

The class CRPCConvertParam already exists and isn’t changed. Variable/param names updated though.

in test/functional/interface_bitcoin_cli.py:49 in 93e962bc7c outdated

41@@ -42,6 +42,17 @@ def run_test(self):
42         rpc_response = self.nodes[0].getblockchaininfo()
43         assert_equal(cli_response, rpc_response)
44 
45+        header_tests = [ ("@50", self.nodes[0].getblockhash(50)),
46+                         ("@best", self.nodes[0].getbestblockhash()),
47+                       ]
48+        for at,hash in header_tests:
49+            self.log.info("Compare response from getblockheader RPC %s" % (at,))

jonatack commented at 10:43 am on June 22, 2020:

3b5790a Thanks for adding the tests. Do you think testing only getblockheader is enough sanity check coverage for the vRPCConvertBlockhash commands?

0            self.log.info("Compare response from getblockheader RPC {}".format(at))

ajtowns commented at 5:33 am on July 13, 2020:

It’s a client-side only thing, so not seeing a lot of need for massive testing, and .format() is more typing. :)

in doc/release-notes-16439.md:5 in 93e962bc7c outdated

0@@ -0,0 +1,9 @@
1+Specify block by height in CLI or Debug Console
2+-----------------------------------------------
3+
4+The ability to specify a block by a height instead of its hash has been
5+added to bitcoin-cli and the debug console. This is a client-side shortcut

jonatack commented at 10:50 am on June 22, 2020:

93e962b For the title, or in this line, I’m not sure what would be the most understood here: maybe add “GUI” before “debug console”, or s/debug console/GUI console/

ajtowns commented at 3:41 am on July 13, 2020:

Made it “GUI debug console”

in src/qt/rpcconsole.cpp:321 in 93e962bc7c outdated

312+                                std::string gbh_method;
313+                                UniValue gbh_args(UniValue::VARR);
314+                                if (args[i] == "@best") {
315+                                    gbh_method = "getbestblockhash";
316+                                } else {
317+                                    gbh_method = "getblockhash";

jonatack commented at 10:52 am on June 22, 2020:

Might be nice (now or later) to extract the convert blockhash and getbestblockhash-vs-getblockhash logic to live in one place and be callable by both the gui and the cli.

ajtowns commented at 5:49 am on July 13, 2020:

Seems pretty trivial and is only used in two places, so leaving as is

in src/bitcoin-cli.cpp:529 in 93e962bc7c outdated

524+        if (args[i] == "@best") {
525+            bh_method = "getbestblockhash";
526+            bh_args.clear();
527+        }
528+        DefaultRequestHandler rh;
529+        const UniValue reply = ConnectAndCallRPC(&rh, bh_method, bh_args);

jonatack commented at 10:57 am on June 22, 2020:

What are your thoughts between these two versions?

0        DefaultRequestHandler rh;
1        const UniValue reply = ConnectAndCallRPC(&rh, bh_method, bh_args);

0        std::unique_ptr<BaseRequestHandler> rh{MakeUnique<DefaultRequestHandler>()};
1        const UniValue reply = ConnectAndCallRPC(rh.get(), bh_method, bh_args);

ajtowns commented at 4:40 am on June 23, 2020:

Using a unique_ptr seems like more code for no benefit – still has the same lifetime, but it’s more typing and memory allocations?

jnewbery commented at 1:05 pm on June 23, 2020:

~~it’s more typing~~

~~auto!~~

~~memory allocations~~

~~a std::unique_ptr has no additional memory allocations (and no additional memory usage if using the default deleter).~~

~~(I’m not saying you should use a unique_ptr here, just that there’s usually zero downside to using one)~~

EDIT: sorry, I didn’t look at the code closely enough, and thought the comparison was between a raw pointer and a unique_ptr. It’s actually between a struct on the stack and a unique_ptr, in which case aj is correct. Adding a unqiue_ptr is obviously an extra allocation.

jonatack commented at 11:11 am on June 22, 2020: member

Nice improvement.

ACK 93e962bc7cf46d7a90896179f056dc0b39b3f85a code review, tested a few of the commands in the CLI and the GUI console, including passing bad height args to check the error handling. Here or in a follow-up, the RPC help and example docs would need updating.

A few style comments (feel free to ignore) and questions follow.

MarcoFalke referenced this in commit 5802ea6bd3 on Jul 10, 2020

sidhujag referenced this in commit 99d5ee549e on Jul 11, 2020

ajtowns force-pushed on Jul 13, 2020

in doc/release-notes-16439.md:8 in 2ed6d2e0e0 outdated

0@@ -0,0 +1,9 @@
1+Specify block by height in CLI or Debug Console
2+-----------------------------------------------
3+
4+The ability to specify a block by a height instead of its hash
5+has been added to bitcoin-cli and the GUI debug console. This is a
6+client-side shortcut for first querying the hash via `getblockhash` or
7+`getbestblockhash`. The syntax is `getblockheader @123456` to refer to
8+the block at height 123456 or `getblockheader @best` to refer to the tip.

jonatack commented at 4:42 am on July 13, 2020:

Mentioning this only as I was asked to do it recently; feel free to ignore.

0the block at height 123456 or `getblockheader [@best](/bitcoin-bitcoin/contributor/best/)` to refer to the tip. (#16439)

ajtowns commented at 6:18 am on July 13, 2020:

Done

jonatack approved

jonatack commented at 4:45 am on July 13, 2020: member

Code review re-ACK 2ed6d2e per git diff 93e962b 2ed6d2e only change since last review is adding “GUI” for clarification to the release note.

ajtowns commented at 5:51 am on July 13, 2020: member

I haven’t had any good ideas about how to document this – it’s client side, so updating the RPC help doesn’t make sense.

ajtowns force-pushed on Jul 13, 2020

jonatack commented at 4:26 am on July 14, 2020: member

re-ACK 7349254 per git range-diff 5550fa5 2ed6d2e 7349254 changes since last review snakecased variable/param naming and adding the PR number to the release note.

I haven’t had any good ideas about how to document this – it’s client side, so updating the RPC help doesn’t make sense.

Same. Good point.

jonatack commented at 3:47 am on July 20, 2020: member

Since client-side users also refer to RPC helps, maybe annotations to those helps would be ok, e.g. “when called from the cli…” or “cli only: …”

ajtowns commented at 11:01 pm on September 10, 2020: member

@sipa @laanwj @jonasschnelli The PR is implemented purely client side now, care to take another look?

in src/rpc/client.h:21 in 7349254a3f outdated

13@@ -14,6 +14,12 @@ UniValue RPCConvertValues(const std::string& strMethod, const std::vector<std::s
14 /** Convert named arguments to command-specific RPC representation */
15 UniValue RPCConvertNamedValues(const std::string& strMethod, const std::vector<std::string>& strParams);
16 
17+/** Whether this is a positional blockhash parameter */
18+bool RPCConvertBlockhash(const std::string& method, int pos);
19+
20+/** Whether this is a named blockhash parameter */
21+bool RPCConvertNamedBlockhash(const std::string& method, std::string& param);

promag commented at 8:06 am on September 11, 2020:

Unused?

ajtowns commented at 4:45 am on September 14, 2020:

Ooops. Added support for -named arguments in cli, so now it’s used.

in src/bitcoin-cli.cpp:522 in 7349254a3f outdated

517+void ReplaceAtHeightByBlockHash(const std::string& method, std::vector<std::string>& args)
518+{
519+    for (size_t i = 0; i < args.size(); ++i) {
520+        if (!RPCConvertBlockhash(method, i) || args[i].size() <= 1 || args[i][0] != '@') continue;
521+
522+        std::string bh_method = "getblockhash";

promag commented at 8:08 am on September 11, 2020:

Move to else below instead of setting as default?

promag commented at 8:57 am on September 11, 2020: member

I’ve submitted https://github.com/bitcoin-core/gui/pull/88 as an alternative for the GUI console.

ajtowns force-pushed on Sep 14, 2020

in src/bitcoin-cli.cpp:539 in 75fcb3ce43 outdated

534+        const UniValue& result = find_value(reply, "result");
535+        const UniValue& error = find_value(reply, "error");
536+        if (error.isNull() && result.isStr()) {
537+            if (named) {
538+                args[i] = name + "=" + result.get_str();
539+                fprintf(stderr, "set args[i] = %s\n", args[i].c_str());

adamjonas commented at 5:40 pm on September 14, 2020:

It appears this line is getting caught in the linter:

0The locale dependent function fprintf(...) appears to be used:
1src/bitcoin-cli.cpp:                fprintf(stderr, "set args[i] = %s\n", args[i].c_str());
2
3Unnecessary locale dependence can cause bugs that are very
4tricky to isolate and fix. Please avoid using locale dependent
5functions if possible.
6
7Advice not applicable in this specific case? Add an exception
8by updating the ignore list in ./test/lint/lint-locale-dependence.sh

ajtowns commented at 11:01 pm on September 14, 2020:

Good linter, that line shouldn’t have made it past debugging.

ajtowns force-pushed on Sep 14, 2020

DrahtBot added the label Needs rebase on Oct 29, 2020

ajtowns force-pushed on Nov 3, 2020

ajtowns commented at 6:47 am on November 3, 2020: member

Rebased

DrahtBot removed the label Needs rebase on Nov 3, 2020

jonatack commented at 10:36 am on November 3, 2020: member

re-ACK cc7078e31852e049874a1cf081f2a054ba3fe4ec per git range-diff 218fe60 674f961 cc7078e, rebase only

jonasschnelli commented at 10:46 am on November 3, 2020: contributor

See also #20273. Nested commands allow similar functionality with more flexibility.

Getting a block by height bitcoin-cli "getblock(getblockhash(10000))"
Getting the first transaction hash of the previous block by height bitcoin-cli "getblock(getblock(getblockhash(10000))[previousblockhash])[tx][0]"

kristapsk commented at 8:03 pm on November 3, 2020: contributor

I think #20273 is better solution for a problem, server code should not be complicated if that can be avoided, so Concept NACK.

ajtowns commented at 3:49 am on November 9, 2020: member

@kristapsk This doesn’t touch server code…

kristapsk commented at 4:38 am on November 9, 2020: contributor

@ajtowns You’re right, my bad, didn’t look at the actual code here. In any case, still think #20273 is superior to this approach.

DrahtBot added the label Needs rebase on Jan 28, 2021

rpc/client: support code for getblock @123 8ca78cee23

bitcoin-cli: support "getblock @123/@best" 900a9b5dd2

tests: add test for bitcoin-cli getblockheader @50 and @best 4f5b7c9a5c

gui: support "getblock @123/@best" in rpc console f6f113612f

Add release nodes for cli/debug console @123/@best queries 4a8a294813

ajtowns force-pushed on Feb 2, 2021

ajtowns commented at 8:43 am on February 2, 2021: member

Rebased on top of #20012

DrahtBot removed the label Needs rebase on Feb 2, 2021

in src/qt/rpcconsole.cpp:313 in 4a8a294813 outdated

309-                            UniValue params = RPCConvertValues(stack.back()[0], std::vector<std::string>(stack.back().begin() + 1, stack.back().end()));
310                             std::string method = stack.back()[0];
311+                            std::vector<std::string> args{stack.back().begin() + 1, stack.back().end()};
312+
313+                            // Convert block heights "@123" or "@best" to block hash when appropriate
314+                            for (size_t i = 0; i < args.size(); i++) {

jonatack commented at 4:14 pm on February 2, 2021:

0                            for (size_t i = 0; i < args.size(); ++i) {

jonatack approved

jonatack commented at 4:28 pm on February 2, 2021: member

Tested re-ACK 4a8a2948139b91f3a64777d9e66f5dab66072fa7

rpc/client: add @height support for getdeploymentinfo and listsinceblock 6b156548de

MarcoFalke removed the label Needs Conceptual Review on Mar 16, 2022

ajtowns commented at 12:33 pm on March 16, 2022: member

Bumped to add new rpcs…

jonatack commented at 10:53 am on March 17, 2022: member

ACK, modulo may as well squash the new commit into the first one.

I reread the discussion at the top to reswap the context into memory and agree this is better on the client side for the reasons stated in #16439 (comment).

I haven’t had any good ideas about how to document this – it’s client side, so updating the RPC help doesn’t make sense.

Since it needs to be documented somewhere, maybe add a reusable line to the RPC helps mentioning that it’s a client-side shortcut. Human users consume these helps for CLI use in addition to for writing client-side software.

ajtowns commented at 6:46 am on April 22, 2022: member

This isn’t seeing any progress so closing.

ajtowns closed this on Apr 22, 2022

ajtowns added the label Up for grabs on Apr 22, 2022

DrahtBot locked this on Apr 22, 2023

cli/gui: support “@height” in place of blockhash for getblock on client side #16439

Conflicts