rpc: Add getblockbyheight method #16345

cvengler · 2019-07-05T17:16:35Z

As such a feature get requested a lot (#16317, #14858 and #8457), I've implemented it into a new RPC call called getblockbyheight as it was discussed in #16317
In the past this feature got requested for the getblock command but it isn't a good idea to interpret a string as a number because it breaks API consistency.

The function is basically just a duplicate of getblock with the exception that it gets the hash by the height (first parameter) and not by the hash as a parameter.

It also checks if the block exists by comparing the height with the total height or if the block is negative.

EDIT: I improved the code with the help of fqlx so it no longer contains some of the getblock legacy.

src/rpc/blockchain.cpp

fqlx · 2019-07-05T20:24:07Z

src/rpc/blockchain.cpp

+        CDataStream ssBlock(SER_NETWORK, PROTOCOL_VERSION | RPCSerializationFlags());
+        ssBlock << block;
+        std::string strHex = HexStr(ssBlock.begin(), ssBlock.end());
+        return strHex;


Will std::string be converted to the univalue object? Do we need to cast to a JSON object?

std::string is a valid UniValue object and is used in many other RPCs.

fqlx · 2019-07-05T20:26:40Z

src/rpc/blockchain.cpp

+        block = GetBlockChecked(pblockindex);
+    }
+
+    if (verbosity <= 0)


There is no documentation for when verbosity is less than 0. I don't think it can ever be? Should verbosity be an unsigned int or enum?

Nope verbosity cannot be less than zero

Can we remove this check for less than zero and change verbosity to an unsigned short or preferably an enum of constants 0-3?

IIRC it is undefined behavior to cast an int to an enum that is out of range of the enum. Because any int can be entered in the RPC, it could be out of range of the enum and cause undesirable things. So I think it is actually better to just handle it as an int. Also, UniValue does not have a get_short() function so there would be no way to fetch a short from the request other than casting which I think is undesirable especially when it's easier to just handle it as an int.

I appreciate the response @achow101. I would then suggest we add a validate and transform for the RPC call but that is out of the scope of the PR. Validation from the client is necessary in these cases so we don't get undesirable things and it would keep the code clean.

Validation from the client is not possible. The client is not necessarily bitcoin-cli. It could be any JSON-RPC client. Validation of this must happen server side.

Validation from the client is necessary in these cases so we don't get undesirable things and it would keep the code clean.

No.

@fqlx from the server point of view, never trust the client.

I would test the argument for valid range and throw an rpc error if out of range. Better to accept just the valid values IMO.

agreed on filtering for valid range

src/rpc/blockchain.cpp

fqlx · 2019-07-05T20:31:12Z

src/rpc/blockchain.cpp

+    int verbosity = 1;
+    if (!request.params[1].isNull()) {
+        if(request.params[1].isNum())
+            verbosity = request.params[1].get_int();


I can't tell if this returns negative or >3

get_int can return any valid int.

src/rpc/blockchain.cpp

cvengler · 2019-07-05T22:12:26Z

@fqlx Actually I do not think it is a good idea to move int verbosity to something else.
This could break the API backwards compatibility, maybe some 3rd party APIs are working with negative numbers for the verbosity. If we would use an unsigned variable it would cause an overflow and then the output would be the opposite

cvengler · 2019-07-05T22:33:23Z

@fqlx I made some changes you've suggested.

fqlx · 2019-07-05T22:42:52Z

Can you add tests?

cvengler · 2019-07-05T22:52:29Z

I don't see the need for a test for such a function.

promag · 2019-07-05T23:10:23Z

@emilengler every change should have a test, especially a new RPC. Also in this case you should a release note too.

fqlx · 2019-07-05T23:30:01Z

The tests are good not only to increase confidence in our codebase but to also show a working example how to use it. The code will used by 1000s of people

achow101 · 2019-07-06T05:15:24Z

I don't see the need for a test for such a function.

There should be at least a basic test that checks getblockbyheight(height) is consistent with getblock(getblockhash(height))

achow101 · 2019-07-06T05:29:05Z

Concept ACK

failing a linter:

This diff appears to have added new lines with trailing whitespace.
The following changes were suspected:
diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
@@ -942,0 +943,97 @@ static UniValue getblock(const JSONRPCRequest& request)
+
+
^---- failure generated from test/lint/lint-whitespace.sh

cvengler · 2019-07-06T13:30:59Z

@fqlx @promag @achow101
Test added, can someone take a look at it?

promag

An alternative to overloading hash in getblock is to add an extra parameter to getblock, like:

getblock hash ( height )

Then:

if hash is null then use height
if both are set then they must match.

This would look good if called with named parameter getblock(height=20).

test/functional/rpc_blockchain.py

promag · 2019-07-06T14:59:05Z

I can't unresolve #16345 (comment), but please address my comment there.

cvengler · 2019-07-06T17:18:13Z

@promag I pushed a new commit which fix this and unresolved the conversation.

test/functional/rpc_blockchain.py

fanquake

@emilengler Thanks for contributing. Before this can be merged, or reviewed, this PR needs a few changes.

The macOS build, and others, are currently failing on Travis:

rpc/blockchain.cpp:1019:19: error: calling function 'LookupBlockIndex' requires holding mutex 'cs_main' exclusively [-Werror,-Wthread-safety-analysis]
    pblockindex = LookupBlockIndex(hash);
                  ^
1 error generated.

Once you've fixed any problems, and you believe your code is ready for review, can you please squash your commits. You could either squash everything into a single commit, or two commits. One that adds the new RPC command and a second commit that adds the test. When you do so, make sure you use clear and meaningful commit messages. The general convention is to prefix with the part of the code you are modifying.

I've also updated the PR title and body. We don't use @ mentions in commits or PR text, because they just lead to spam when downstream projects pull our changes.

test/functional/rpc_blockchain.py

src/rpc/blockchain.cpp

Empact · 2019-07-08T22:18:06Z

src/rpc/blockchain.cpp

+    }
+
+    // Check if block exists or is negative
+    if(::ChainActive().Height() < request.params[0].get_int() || request.params[0].get_int() <= -1)


nit: having a blockheight local will make this more legible imo

It is only used 2 times, I think it is not necessary.

4 times (see below)

nit, add {}

cvengler · 2019-07-09T17:10:10Z

@fanquake Thank you for writing such an informative comment!
I have fixed to compilation errors and squashed to commits.

src/rpc/blockchain.cpp

cvengler · 2019-07-09T19:43:42Z

I've also updated the commit messages with the rpc prefix

instagibbs · 2019-07-10T17:49:44Z

I don't see the need for a test for such a function.

Is how we get RPC bugs into releases :D

concept ACK

src/rpc/blockchain.cpp

instagibbs · 2019-07-10T17:53:01Z

src/rpc/blockchain.cpp

+
+    // Check if block exists or is negative
+    if(::ChainActive().Height() < request.params[0].get_int() || request.params[0].get_int() <= -1)
+        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block " + std::to_string(request.params[0].get_int()) + " not found");


feel-free-to-ignore-nit: error could be RPC_INVALID_PARAMETER

instagibbs · 2019-07-10T17:55:37Z

test/functional/rpc_blockchain.py

+        node = self.nodes[0]
+        node.add_p2p_connection(P2PInterface())
+        assert_equal(node.getblock(node.getblockhash(20)), node.getblockbyheight(20))
+


good follow-up testing would be to make sure -1, 0, getblockcount(), and getblockcount()+1 args respond properly.

also, exercising the 2nd arg once in the test would be 👍

instagibbs · 2019-07-10T17:59:34Z

src/rpc/blockchain.cpp

+        block = GetBlockChecked(pblockindex);
+    }
+
+    if (verbosity <= 0)


agreed on filtering for valid range

src/rpc/blockchain.cpp

promag · 2019-07-11T07:38:23Z

src/rpc/blockchain.cpp

+    }
+
+    // Check if block exists or is negative
+    if(::ChainActive().Height() < request.params[0].get_int() || request.params[0].get_int() <= -1)


nit, add {}

cvengler · 2019-07-17T15:44:00Z

@promag First there was a type sorry
I meant:
I think it should be added due to lots of requests.

To the second point: I don't have numbers to prove it but I'm very sure that 2 separate calls are slower.

promag · 2019-07-17T15:53:29Z

I know it's slower, but how much? How much this saves?

sipa · 2019-07-17T16:45:11Z

I don't think speed is a good rationale for this PR. If speed was really a concern, you wouldn't be using Bitcoin Core RPCs in the first place.

The only real reason to want this is convenience. I'm sure that's the real reason this is often requested, and I'm mildly in favor of it because of that.

maflcko · 2019-07-17T17:16:52Z

See also " GUI: Add generate method #16000 ".

Basically it would be nice to have a way to set aliases like generate=generatetoaddress(getnewaddress()) or getblockbyheight=getblock(getblockhash($1)). This is really easy if you call the rpc from bash or python, but not when calling from the gui.

ajtowns · 2019-07-18T01:05:19Z

The only real reason to want this is convenience.

How about just treating a blockhash "@123" as a request of the block at height 123, rather than an error for not being 64 hex digits? That's pretty convenient for manual use from the cli and gui, and pretty easy to code (and thus to do for other RPCs that want a block hash):

$ bitcoin-cli -regtest getblock $(bitcoin-cli -regtest getblockhash 0) | grep ^...hash
  "hash": "0f9188f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e2206",
$ bitcoin-cli -regtest getblock @0 | grep ^...hash
  "hash":  "0f9188f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e2206",

+inline uint256 GetBlockHashFromParam(const UniValue& param, const std::string& name) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
+{
+    const std::string& v = param.get_str();
+    if (v.size() > 1 && v[0] == '@') {
+        // treat as height, and lookup
+        int32_t height;
+        if (!ParseInt32(v.substr(1), &height)) {
+            throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Target block height %s must be @ followed by a number", v));
+        }
+        if (height < 0) {
+            throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Target block height %d is negative", height));
+        }
+        const int current_tip = ::ChainActive().Height();
+        if (height > current_tip) {
+            throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Target block height %d after current tip %d", height, current_tip));
+        }
+        return *(::ChainActive()[height]->phashBlock);
+    } else {
+        return ParseHashV(param, name);
+    }
+}
+
 static UniValue getblock(const JSONRPCRequest& request)
 {
     const RPCHelpMan help{"getblock",
@@ -904,8 +926,6 @@ static UniValue getblock(const JSONRPCRequest& request)
         throw std::runtime_error(help.ToString());
     }
 
-    uint256 hash(ParseHashV(request.params[0], "blockhash"));
-
     int verbosity = 1;
     if (!request.params[1].isNull()) {
         if(request.params[1].isNum())
@@ -919,6 +939,7 @@ static UniValue getblock(const JSONRPCRequest& request)
     const CBlockIndex* tip;
     {
         LOCK(cs_main);
+        uint256 hash(GetBlockHashFromParam(request.params[0], "blockhash"));
         pblockindex = LookupBlockIndex(hash);
         tip = ::ChainActive().Tip();

cvengler · 2019-07-18T15:42:23Z

@ajtowns I think it is better to have a seperate command for this. This could cause confusion

promag · 2019-07-18T15:49:18Z

Concept NAK from me sorry, please provide benchmarks to support the new RPC - which IMO is the only valid reason to add redundancy to the RPC interface.

cvengler · 2019-07-18T19:51:44Z

This would make it even faster to get a block just by its height. Currently you need to execute two separate commands which is slower and more complicated in two ways: For the machine and for the user and such a feature get requested a lot

promag · 2019-07-18T21:52:04Z

I went ahead and made a quick profile:

count = 5000
node.generate(count)

for i in range(count):
    node.getblock(self.nodes[0].getblockhash(i))

# takes ~ 3742ms

for i in range(count):
    node.getblockbyheight(i)

# takes ~ 2193ms

Note that it fetches 5000 blocks but these are really small blocks and you should try with bigger blocks - I think it will make a difference in the comparison - I mean getblockbyheight would just be slightly faster.

cvengler · 2019-07-19T15:34:42Z

@promag Thanks for doing the test but it is even faster for the user to type the command

cvengler · 2019-07-20T13:06:55Z

Currently there are:
3 ACKs
1 NACK

src/rpc/blockchain.cpp

rpc: Fix whitespace in rpc/blockchain.cpp rpc: getblockbyheight: Remove hash

ajtowns · 2019-07-23T05:14:01Z

Note that it fetches 5000 blocks but these are really small blocks and you should try with bigger blocks - I think it will make a difference in the comparison - I mean getblockbyheight would just be slightly faster.

It looks like even with bigger blocks avoiding the two RPCs is a fair bit faster:

$ time for a in `seq 400000 400500`; do bitcoin-cli getblock $(bitcoin-cli getblockhash $a); done | grep -v '"confirmations":' | sha256sum
28620dea517c27debd958345197dc911f2fe95934742293548e79cd673bfae40  -

real	0m15.763s
$ time for a in `seq 400000 400500`; do ./bitcoin-cli getblockbyheight $a; done | grep -v '"confirmations":' | sha256sum
28620dea517c27debd958345197dc911f2fe95934742293548e79cd673bfae40  -

real	0m11.514s
$ time for a in `seq 400000 400500`; do bitcoin-cli getblock $(bitcoin-cli getblockhash $a); done | grep -v '"confirmations":' | sha256sum
28620dea517c27debd958345197dc911f2fe95934742293548e79cd673bfae40  -

real	0m15.408s

ajtowns · 2019-07-23T09:15:04Z

@emilengler note that this call can't be cached whereas getblock by hash can.

Strictly speaking, "getblock(hash)" can't be cached either, because it includes a "confirmations" key which changes every time a new block gets mined

jnewbery · 2019-07-30T15:42:14Z

I'm a mild concept NACK and an approach NACK on this.

mild concept NACK: unless there's a very compelling reason, I don't think the RPC interface should offer redundant ways to do the same thing. Increasing the surface area of the RPC interface increases the maintenance burden, so we should try to keep it minimal wherever possible.
approach NACK: this approach duplicates a lot of code, both in the implementation and in the RPC help text. That could be improved by factoring both the logic and help text out, and calling it from both getblock and getblockatheight.

Overall, I prefer #16439. I'm not a fan of magic incantations like @height in general, but that PR seems like a pretty clean implementation and there's absolutely no ambiguity between getblock @height and getblock hash.

ryanofsky · 2019-07-31T14:28:11Z

@emilengler re: your question in IRC

[01:04:54] <emilengler> How the CTRL+L shortcut is being handled in bitcoin-qt? Over a QAction or a QShortcut?

It's just a property of the clear button

bitcoin/src/qt/forms/debugwindow.ui

Lines 564 to 566 in 3f288a1

    
           <property name="shortcut"> 
        
            <string notr="true">Ctrl+L</string> 
        
           </property>

cvengler · 2019-07-31T18:36:44Z

@ryanofsky Thank you, already got this :)
But I don't think this is the right place to discuss this.
An IRC direct message had done the same

luke-jr · 2019-08-19T23:47:59Z

Concept NACK. getblock(getblockhash(1)) is simple enough.

cvengler · 2019-08-20T14:29:03Z

@luke-jr It probably is but in my opinion it isn't really shown that using RPC commands as parameters is possible (at least I didn't saw something like this)

DrahtBot · 2019-08-27T17:44:36Z

Needs rebase

cvengler · 2019-10-14T15:27:53Z

Closed because of lack of interest and merging conflicts

fqlx suggested changes Jul 5, 2019

View reviewed changes

src/rpc/blockchain.cpp Show resolved Hide resolved

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved

fanquake added the RPC/REST/ZMQ label Jul 5, 2019

promag reviewed Jul 6, 2019

View reviewed changes

test/functional/rpc_blockchain.py Outdated Show resolved Hide resolved

fqlx approved these changes Jul 7, 2019

View reviewed changes

test/functional/rpc_blockchain.py Outdated Show resolved Hide resolved

fanquake reviewed Jul 8, 2019

View reviewed changes

test/functional/rpc_blockchain.py Outdated Show resolved Hide resolved

fanquake changed the title ~~RPC: Add getblockbyheight~~ rpc: Add getblockbyheight method Jul 8, 2019

jnewbery mentioned this pull request Jul 8, 2019

Future PRs bitcoin-core-review-club/website#14

Closed

maflcko reviewed Jul 8, 2019

View reviewed changes

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved

Empact reviewed Jul 8, 2019

View reviewed changes

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved

Empact reviewed Jul 8, 2019

View reviewed changes

maflcko reviewed Jul 9, 2019

View reviewed changes

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved

instagibbs reviewed Jul 10, 2019

View reviewed changes

cvengler added 2 commits July 10, 2019 20:19

rpc: Add getblockbyheight

a23fe8c

rpc: Add test for getblockbyheight

ac13a74

promag reviewed Jul 11, 2019

View reviewed changes

ajtowns reviewed Jul 22, 2019

View reviewed changes

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved

rpc: Code Improvement in getblockbyheight

373401f

rpc: Fix whitespace in rpc/blockchain.cpp rpc: getblockbyheight: Remove hash

ajtowns mentioned this pull request Jul 23, 2019

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

Closed

DrahtBot added the Needs rebase label Aug 27, 2019

laanwj added the Feature label Sep 30, 2019

cvengler closed this Oct 14, 2019

cvengler deleted the getblockbyheight branch October 14, 2019 15:27

laanwj removed the Needs rebase label Oct 24, 2019

PastaPastaPasta mentioned this pull request Sep 24, 2020

Add block height support in rpc call getblock dashpay/dash#2517

Open

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

rpc: Add getblockbyheight method #16345

rpc: Add getblockbyheight method #16345

Uh oh!

Conversation

cvengler commented Jul 5, 2019 • edited by fanquake Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cvengler commented Jul 5, 2019

Uh oh!

cvengler commented Jul 5, 2019

Uh oh!

fqlx commented Jul 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cvengler commented Jul 5, 2019

Uh oh!

promag commented Jul 5, 2019

Uh oh!

fqlx commented Jul 5, 2019

Uh oh!

achow101 commented Jul 6, 2019

Uh oh!

achow101 commented Jul 6, 2019

Uh oh!

cvengler commented Jul 6, 2019

Uh oh!

promag left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

promag commented Jul 6, 2019

Uh oh!

cvengler commented Jul 6, 2019

Uh oh!

Uh oh!

fanquake left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cvengler commented Jul 5, 2019 •

edited by fanquake

Loading

fqlx commented Jul 5, 2019 •

edited

Loading

promag left a comment •

edited

Loading