rpc: Add getblockbyheight method

emilengler commented at 5:16 pm on July 5, 2019: contributor

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.

in src/rpc/blockchain.cpp:1022 in 54cd63a5ff outdated

1015+        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found");
1016+    }
1017+    CBlockIndex* pblockindex = ::ChainActive()[request.params[0].get_int()];
1018+    uint256 hash(ParseHashV(pblockindex->GetBlockHash().GetHex(), "blockhash"));
1019+    const CBlockIndex* tip;
1020+    {

fqlx commented at 8:03 pm on July 5, 2019:

Not needed curly

emilengler commented at 9:53 pm on July 5, 2019:

Agree, a legacy of getblock

fqlx commented at 0:39 am on July 6, 2019:

Hi - I may have made a mistake here. It seems like the curly brace is needed to acquire tsc_main scope lock here. I’m not very familiar with this feature.

See: #16285 (review)

sipa commented at 2:24 am on July 6, 2019:

@fqlx You seem very confused. The opening brace isn’t needed to acquire the cs_main lock; It’s to make the lock be released after the GetBlockChecked call. We want to minimize the time the lock is held for responsivity reasons.

emilengler commented at 10:20 am on July 6, 2019:

@sipa Ok, I’ve re-added it

in src/rpc/blockchain.cpp:1014 in 54cd63a5ff outdated

1009+    }
1010+
1011+    CBlock block;
1012+    // Check if block exists or is negative
1013+    if(::ChainActive().Height() < request.params[0].get_int() || request.params[0].get_int() <= -1)
1014+    {

fqlx commented at 8:06 pm on July 5, 2019:

Use the same curly brace formatting. Either all on the same line or all new line.

in src/rpc/blockchain.cpp:1031 in 54cd63a5ff outdated

1030+    }
1031+
1032+    if (verbosity <= 0)
1033+    {
1034+        CDataStream ssBlock(SER_NETWORK, PROTOCOL_VERSION | RPCSerializationFlags());
1035+        ssBlock << block;

fqlx commented at 8:08 pm on July 5, 2019:

No assignment?

achow101 commented at 5:12 am on July 6, 2019:

<< is the correct operator to serialize an object to a stream.

in src/rpc/blockchain.cpp:1007 in 54cd63a5ff outdated

1003+    int verbosity = 1;
1004+    if (!request.params[1].isNull()) {
1005+        if(request.params[1].isNum())
1006+            verbosity = request.params[1].get_int();
1007+        else
1008+            verbosity = request.params[1].get_bool() ? 1 : 0;

fqlx commented at 8:18 pm on July 5, 2019:

Very strange params[1] is a union of a number and bool. I’m skeptical of this because it will cause more bugs down the line.

emilengler commented at 9:14 pm on July 5, 2019:

The code is mostly a fork from the getblock function which means that I copied this from this function as well but I personally think that this issue is a topic for another pull request.

sipa commented at 9:21 pm on July 5, 2019:

The developer guidelines state that for new code, following the specified coding style is preferable. The only exception is commits which just move code around. See https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md

Empact commented at 10:14 pm on July 8, 2019:

Which is to say, you can make this argument just a number. No need to make this RPC strictly consistent with getblock. This behavior is only maintained in exceptional cases which do not apply here:

0  - *Exception*: Some RPC calls can take both an `int` and `bool`, most notably when a bool was switched
1    to a multi-value, or due to other historical reasons. **Always** have false map to 0 and
2    true to 1 in this case.

in src/rpc/blockchain.cpp:1011 in 54cd63a5ff outdated

1008+            verbosity = request.params[1].get_bool() ? 1 : 0;
1009+    }
1010+
1011+    CBlock block;
1012+    // Check if block exists or is negative
1013+    if(::ChainActive().Height() < request.params[0].get_int() || request.params[0].get_int() <= -1)

fqlx commented at 8:21 pm on July 5, 2019:

Why no check for param[0].isNull()?

emilengler commented at 9:47 pm on July 5, 2019:

What do you mean by isNull? If the param is empty or if the user has selected block zero, which is a valid block

fqlx commented at 10:47 pm on July 5, 2019:

<= -1 should be == BLOCK_DOESNT_EXIST and define block doesn’t exist to -1. I would also bet this constant is define in a header already. Find it please. We also can’t have negative blocks so no need to check for less than -1.

achow101 commented at 5:27 am on July 6, 2019:

<= -1 should be == BLOCK_DOESNT_EXIST and define block doesn’t exist to -1.

Why? This is a user entered value, it could literally be anything, not just -1. A user isn’t going to search specifically for -1 just so they can get an error. It could be -16654 or whatever you want. It is safer to check whether the value is <= -1 rather than -1.

Also, why would this need to be defined as a constant? It really isn’t useful as one.

I would also bet this constant is define in a header already. Find it please. We also can’t have negative blocks so no need to check for less than -1.

It probably isn’t defined anywhere as there is no need to define it anywhere.

fqlx commented at 5:50 am on July 6, 2019:

Why?

It’s easier to read and maintain if we check for the valid paths. The documentation says verbose 0-3, uses a weak type (int vs enum), then check for unrelated int values mid code - it’s confusing. We can’t just arbitrarily pick error cases to check for here. Why not check if the user inputted a string, emoji, a number greater than INT_MAX too?

It probably isn’t defined anywhere as there is no need to define it anywhere.

You’re right it’s not define but it would be useful.

achow101 commented at 6:39 am on July 6, 2019:

It’s easier to read and maintain if we check for the valid paths. The documentation says verbose 0-3, uses a weak type (int vs enum), then check for unrelated int values mid code - it’s confusing.

You’re getting your conversations mixed up. This isn’t about verbosity.

We can’t just arbitrarily pick error cases to check for here.

We aren’t.

Why not check if the user inputted a string, emoji, a number greater than INT_MAX too?

That’s why RPCs should use RPCTypeCheck. That (and UniValue itself) will handle type and integer out of bound errors.

in src/rpc/blockchain.cpp:1024 in 54cd63a5ff outdated

1018+    uint256 hash(ParseHashV(pblockindex->GetBlockHash().GetHex(), "blockhash"));
1019+    const CBlockIndex* tip;
1020+    {
1021+        LOCK(cs_main);
1022+        pblockindex = LookupBlockIndex(hash);
1023+        tip = ::ChainActive().Tip();

fqlx commented at 8:21 pm on July 5, 2019:

Place this assignment below the throw when checking pblockindex

in src/rpc/blockchain.cpp:1026 in 54cd63a5ff outdated

1021+        LOCK(cs_main);
1022+        pblockindex = LookupBlockIndex(hash);
1023+        tip = ::ChainActive().Tip();
1024+
1025+        if (!pblockindex) {
1026+            throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found");

fqlx commented at 8:22 pm on July 5, 2019:

Add the hash to the throw so we can debug why this statement would have failed

in src/rpc/blockchain.cpp:1033 in 54cd63a5ff outdated

1032+    if (verbosity <= 0)
1033+    {
1034+        CDataStream ssBlock(SER_NETWORK, PROTOCOL_VERSION | RPCSerializationFlags());
1035+        ssBlock << block;
1036+        std::string strHex = HexStr(ssBlock.begin(), ssBlock.end());
1037+        return strHex;

fqlx commented at 8:24 pm on July 5, 2019:

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

achow101 commented at 5:13 am on July 6, 2019:

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

in src/rpc/blockchain.cpp:1028 in 54cd63a5ff outdated

1027+        }
1028+
1029+        block = GetBlockChecked(pblockindex);
1030+    }
1031+
1032+    if (verbosity <= 0)

fqlx commented at 8:26 pm on July 5, 2019:

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?

emilengler commented at 9:51 pm on July 5, 2019:

Nope verbosity cannot be less than zero

fqlx commented at 10:02 pm on July 5, 2019:

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

achow101 commented at 5:23 am on July 6, 2019:

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.

fqlx commented at 5:31 am on July 6, 2019:

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.

achow101 commented at 5:42 am on July 6, 2019:

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.

promag commented at 2:29 pm on July 6, 2019:

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

No.

promag commented at 4:12 pm on July 8, 2019:

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

Empact commented at 10:20 pm on July 8, 2019:

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.

instagibbs commented at 5:59 pm on July 10, 2019:

agreed on filtering for valid range

in src/rpc/blockchain.cpp:1002 in 54cd63a5ff outdated

 998+
 999+    if (request.fHelp || !help.IsValidNumArgs(request.params.size())) {
1000+        throw std::runtime_error(help.ToString());
1001+    }
1002+
1003+    int verbosity = 1;

fqlx commented at 8:30 pm on July 5, 2019:

This verbosity level logging should be an enum of constants to help read instead of magic numbers.

e.g. int verbosity = LEVEL1_BLOCKHEIGHT

emilengler commented at 9:57 pm on July 5, 2019:

The input is an integer and the various types of verbosity are magic numbers.

fqlx commented at 10:25 pm on July 5, 2019:

Yes but we only have 4 types. 0-3. It could be an enum

emilengler commented at 5:24 pm on July 6, 2019:

Other functions use this as well, I think this should be improved in another PR

in src/rpc/blockchain.cpp:1005 in 54cd63a5ff outdated

1001+    }
1002+
1003+    int verbosity = 1;
1004+    if (!request.params[1].isNull()) {
1005+        if(request.params[1].isNum())
1006+            verbosity = request.params[1].get_int();

fqlx commented at 8:31 pm on July 5, 2019:

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

achow101 commented at 5:24 am on July 6, 2019:

get_int can return any valid int.

fqlx changes_requested

in src/rpc/blockchain.cpp:1000 in 54cd63a5ff outdated

 995+            + HelpExampleRpc("getblockbyheight", "42")
 996+                },
 997+    };
 998+
 999+    if (request.fHelp || !help.IsValidNumArgs(request.params.size())) {
1000+        throw std::runtime_error(help.ToString());

fqlx commented at 8:33 pm on July 5, 2019:

If help is null this toString will cause undefined behavior

emilengler commented at 9:57 pm on July 5, 2019:

How?

in src/rpc/blockchain.cpp:1015 in 54cd63a5ff outdated

1010+
1011+    CBlock block;
1012+    // Check if block exists or is negative
1013+    if(::ChainActive().Height() < request.params[0].get_int() || request.params[0].get_int() <= -1)
1014+    {
1015+        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found");

fqlx commented at 8:34 pm on July 5, 2019:

Can you add what block/parameter that is not found for debugging

emilengler commented at 9:57 pm on July 5, 2019:

Sure

fqlx changes_requested

emilengler commented at 10:12 pm on July 5, 2019: contributor

@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

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

emilengler commented at 10:33 pm on July 5, 2019: contributor

@fqlx I made some changes you’ve suggested.

fqlx commented at 10:42 pm on July 5, 2019: none

Can you add tests?

emilengler commented at 10:52 pm on July 5, 2019: contributor

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

promag commented at 11:10 pm on July 5, 2019: member

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

fqlx commented at 11:30 pm on July 5, 2019: none

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 commented at 5:15 am on July 6, 2019: member

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 commented at 5:29 am on July 6, 2019: member

Concept ACK

failing a linter:

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

emilengler commented at 1:30 pm on July 6, 2019: contributor

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

in test/functional/rpc_blockchain.py:318 in 1404d1947a outdated

313+        self.log.info("Test getblockbyheight")
314+        node = self.nodes[0]
315+        node.add_p2p_connection(P2PInterface())
316+
317+        self.log.info("Getting block with height 20")
318+        b20 = node.getblockbyheight(20)

promag commented at 2:33 pm on July 6, 2019:

Something like the following should be enough:

0assert_equal(getblock(getblockhash(20)), getblockbyheight)

Then you could also test these errors:

calling without arguments
calling with extra arguments
calling with wrong type (string for instance).

emilengler commented at 2:47 pm on July 6, 2019:

Thank you, I will edit it

promag commented at 2:58 pm on July 6, 2019:

You should assert the whole response, not just the hash.

promag commented at 2:35 pm on July 6, 2019: member

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

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

promag commented at 2:59 pm on July 6, 2019: member

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

emilengler commented at 5:18 pm on July 6, 2019: contributor

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

in test/functional/rpc_blockchain.py:319 in 3f3fc11214 outdated

314+        node = self.nodes[0]
315+        node.add_p2p_connection(P2PInterface())
316+
317+        self.log.info("Test that getblockbyheight is getting block 20")
318+        assert_equal(node.getblock(node.getblockhash(20)), node.getblockbyheight(20))
319+        self.log.info("The block is valid")

fqlx commented at 1:00 am on July 7, 2019:

I’d recommend removing the logging on 317, 319 because it’s not needed and will clutter the console logging. Other than that, things look good.

emilengler commented at 12:13 pm on July 7, 2019:

Good point, removed!

fqlx approved

in test/functional/rpc_blockchain.py:318 in 33b2da3aec outdated

313+        self.log.info("Test getblockbyheight")
314+        node = self.nodes[0]
315+        node.add_p2p_connection(P2PInterface())
316+
317+        assert_equal(node.getblock(node.getblockhash(20)), node.getblockbyheight(20))
318+        self.log.info("The block is valid")

fanquake commented at 1:16 am on July 8, 2019:

No need for the The block is valid logging, or the extra white space here.

emilengler commented at 4:59 pm on July 9, 2019:

Fixed

fanquake commented at 1:25 am on July 8, 2019: member

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

0rpc/blockchain.cpp:1019:19: error: calling function 'LookupBlockIndex' requires holding mutex 'cs_main' exclusively [-Werror,-Wthread-safety-analysis]
1    pblockindex = LookupBlockIndex(hash);
2                  ^
31 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.

fanquake renamed this:
~~RPC: Add getblockbyheight~~
rpc: Add getblockbyheight method
on Jul 8, 2019

in src/rpc/blockchain.cpp:1016 in 33b2da3aec outdated

1011+    if(::ChainActive().Height() < request.params[0].get_int() || request.params[0].get_int() <= -1)
1012+        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block " + std::to_string(request.params[0].get_int()) + " not found");
1013+
1014+    CBlock block;
1015+    CBlockIndex* pblockindex = ::ChainActive()[request.params[0].get_int()];
1016+    uint256 hash(ParseHashV(pblockindex->GetBlockHash().GetHex(), "blockhash"));

MarcoFalke commented at 4:44 pm on July 8, 2019:

Why do you translate the hash into hex and then parse the hex into a hash?

emilengler commented at 7:37 pm on July 8, 2019:

I was doing it the same way as in getblock()

in src/rpc/blockchain.cpp:1022 in 33b2da3aec outdated

1017+    const CBlockIndex* tip;
1018+
1019+    pblockindex = LookupBlockIndex(hash);
1020+
1021+    if (!pblockindex)
1022+        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block with hash \"" + hash.ToString() + "\" not found");

MarcoFalke commented at 4:45 pm on July 8, 2019:

Why would this happen? I’d say never, so it should not throw “invalid key”, but an internal error. Also, missing {}

emilengler commented at 7:36 pm on July 8, 2019:

You are right, was suggested by someone else

in src/rpc/blockchain.cpp:1019 in 41c165266f outdated

1014+    CBlock block;
1015+    CBlockIndex* pblockindex = ::ChainActive()[request.params[0].get_int()];
1016+    uint256 hash(pblockindex->GetBlockHash());
1017+    const CBlockIndex* tip;
1018+
1019+    if (!pblockindex)

Empact commented at 10:16 pm on July 8, 2019:

nit: braceless ifs should be inline

0  - If an `if` only has a single-statement `then`-clause, it can appear
1    on the same line as the `if`, without braces. In every other case,
2    braces are required, and the `then` and `else` clauses must appear
3    correctly indented on a new line.

in src/rpc/blockchain.cpp:1011 in 41c165266f outdated

1006+        else
1007+            verbosity = request.params[1].get_bool() ? 1 : 0;
1008+    }
1009+
1010+    // Check if block exists or is negative
1011+    if(::ChainActive().Height() < request.params[0].get_int() || request.params[0].get_int() <= -1)

Empact commented at 10:18 pm on July 8, 2019:

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

emilengler commented at 5:01 pm on July 9, 2019:

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

Empact commented at 5:28 am on July 10, 2019:

4 times (see below)

promag commented at 7:38 am on July 11, 2019:

nit, add {}

emilengler commented at 5:10 pm on July 9, 2019: contributor

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

in src/rpc/blockchain.cpp:1024 in adb93ca9dd outdated

1019+    if (!pblockindex)
1020+        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid key");
1021+
1022+    {
1023+        LOCK(cs_main);
1024+        pblockindex = LookupBlockIndex(hash);

MarcoFalke commented at 5:57 pm on July 9, 2019:

Can remove this line. pblockindex is already the correct value

emilengler commented at 7:30 pm on July 9, 2019:

Sure thing

emilengler commented at 7:43 pm on July 9, 2019: contributor

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

instagibbs commented at 5:49 pm on July 10, 2019: member

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

Is how we get RPC bugs into releases :D

concept ACK

in src/rpc/blockchain.cpp:1016 in 6c1f05bd33 outdated

1012@@ -1013,21 +1013,18 @@ static UniValue getblockbyheight(const JSONRPCRequest& request)
1013 
1014     CBlock block;
1015     CBlockIndex* pblockindex = ::ChainActive()[request.params[0].get_int()];
1016-    uint256 hash(ParseHashV(pblockindex->GetBlockHash().GetHex(), "blockhash"));

instagibbs commented at 5:50 pm on July 10, 2019:

This is the wrong commit for logic changes(stick to tests only!), please split up this commit. Ask if you need help.

emilengler commented at 6:07 pm on July 10, 2019:

I’ve written you a message on freenode if you are instagibbs there. Can we discuss this here? I would post the chat log here afterwards with your permission

instagibbs commented at 6:22 pm on July 10, 2019:

we fixed this on the interwebs :+1:

emilengler commented at 6:25 pm on July 10, 2019:

For future readers: https://pastebin.com/33uYhx7i

in src/rpc/blockchain.cpp:1012 in f0a41d5853 outdated

1007+            verbosity = request.params[1].get_bool() ? 1 : 0;
1008+    }
1009+
1010+    // Check if block exists or is negative
1011+    if(::ChainActive().Height() < request.params[0].get_int() || request.params[0].get_int() <= -1)
1012+        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block " + std::to_string(request.params[0].get_int()) + " not found");

instagibbs commented at 5:53 pm on July 10, 2019:

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

emilengler commented at 6:07 pm on July 10, 2019:

Ok

in test/functional/rpc_blockchain.py:317 in 6c1f05bd33 outdated

308@@ -307,6 +309,13 @@ def assert_waitforheight(height, timeout=2):
309         assert_waitforheight(current_height)
310         assert_waitforheight(current_height + 1)
311 
312+    def _test_getblockbyheight(self):
313+        self.log.info("Test getblockbyheight")
314+        node = self.nodes[0]
315+        node.add_p2p_connection(P2PInterface())
316+        assert_equal(node.getblock(node.getblockhash(20)), node.getblockbyheight(20))
317+

instagibbs commented at 5:55 pm on July 10, 2019:

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

instagibbs commented at 6:02 pm on July 10, 2019:

also, exercising the 2nd arg once in the test would be :+1:

in src/rpc/blockchain.cpp:2487 in f0a41d5853 outdated

2486@@ -2388,6 +2487,7 @@ static const CRPCCommand commands[] =
2487     { "blockchain",         "getbestblockhash",       &getbestblockhash,       {} },
2488     { "blockchain",         "getblockcount",          &getblockcount,          {} },
2489     { "blockchain",         "getblock",               &getblock,               {"blockhash","verbosity|verbose"} },
2490+    { "blockchain",         "getblockbyheight",       &getblockbyheight,       {"blockheight","verbosity|verbose"} },

instagibbs commented at 6:00 pm on July 10, 2019:

Do we need to support both named args like getblock? I don’t really know the story on this.

emilengler commented at 6:09 pm on July 10, 2019:

Same, the reason why the RPC call is very similar to getblock is becuase I have forked it. That’s why I also removed some of the legacy getblock stuff

rpc: Add getblockbyheight a23fe8cf8a

rpc: Add test for getblockbyheight ac13a74c6e

in src/rpc/blockchain.cpp:1015 in ac13a74c6e outdated

1010+    // Check if block exists or is negative
1011+    if(::ChainActive().Height() < request.params[0].get_int() || request.params[0].get_int() <= -1)
1012+        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block " + std::to_string(request.params[0].get_int()) + " not found");
1013+
1014+    CBlock block;
1015+    CBlockIndex* pblockindex = ::ChainActive()[request.params[0].get_int()];

promag commented at 7:37 am on July 11, 2019:

👀 this should have cs_main locked?

emilengler commented at 5:01 pm on July 11, 2019:

Why?

promag commented at 5:59 pm on July 11, 2019:

Because the chain “can” change concurrently. Similarly, note that below cs_main is locked in order to access the chain tip.

promag commented at 6:02 pm on July 11, 2019:

Also note that getblockhash RPC also locks cs_main.

emilengler commented at 8:01 pm on July 11, 2019:

Ok, I will change this tomorrow

emilengler commented at 8:40 pm on July 12, 2019:

Where I need to LOCK?

MarcoFalke commented at 8:45 pm on July 12, 2019:

You could move this line down to where you already lock

0    CBlockIndex* pblockindex;

emilengler commented at 8:53 pm on July 12, 2019:

Thank you, I thought I would need a new lock

emilengler commented at 9:53 pm on July 12, 2019:

Done

emilengler commented at 5:00 pm on July 11, 2019: contributor

Why does travis starts responding with errors? The codebase hasn’t changed

abitfan commented at 10:40 am on July 12, 2019: contributor

Concept ACK NACK for another rpc call. The reason #8457 was rejected was due to overloading APIs however this is invalidated by the existence of getblockstats.

emilengler commented at 5:35 pm on July 12, 2019: contributor

@abitfan See #16317

emilengler commented at 6:45 pm on July 12, 2019: contributor

Seems that travis failed again, I will check the code again

emilengler commented at 9:18 pm on July 12, 2019: contributor

Finally Travis has succeeded. I will do some small changes to the code which were suggested above and then this can be merged I think.

jonasschnelli commented at 9:36 pm on July 12, 2019: contributor

What is wrong with getblock(getblockhash(<height>))? Is the intention of this PR to increase fetch performance by height?

emilengler commented at 9:39 pm on July 12, 2019: contributor

@jonasschnelli Such a feature get requested a lot

DrahtBot commented at 5:05 am on July 13, 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:

#16728 (move-only: move coins statistics utils out of RPC by jamesob)
#16439 (RPC: support “@height” in place of blockhash for getblock etc by ajtowns)
#16365 (Log RPC parameters (arguments) if -debug=rpcparams by LarryRuane)
#15606 ([experimental] UTXO snapshots by jamesob)

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 commented at 3:16 pm on July 16, 2019: member

An alternative to overloading hash in getblock is to add an extra parameter to getblock, like:
0getblock 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).

I think I agree with this – here’s a draft of what that might look like shamelessly built on top of this PR while removing most of it :) https://github.com/ajtowns/bitcoin/commit/cf0868719748e2bb4f0394924245d4212d0f4260 @emilengler Feel free to steal the patch, merge it into your own, take the credit, do what you like; or ignore it and get this merged, whatever :)

jonasschnelli commented at 3:32 pm on July 16, 2019: contributor

I’m unsure about this. Overlapping features for a machine-2-machine API (our RPC API) seems non-ideal to me. IMO getblockhash (or /rest/blockhashbyheight) is the one call that deals with heights.

I would accept the argument that performance is an issue (since one needs to execute two calls for getting a block at a certain height),… but since the RPC interface with its JSON overhead is already slow, I don’t think it matters that much.

My humble opinion is to avoid adding client features on the server-side.

emilengler commented at 3:30 pm on July 17, 2019: contributor

@jonasschnelli #16317 The feature get requested a lot (around once or twice per semester). I think it should be added to to lots of requests. It wouldn’t make in terms of speed such a difference. It is even slower to first call getblockhash and then getblock

promag commented at 3:40 pm on July 17, 2019: member

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

I think it should be added to to lots of requests.

What you mean?

It is even slower to first call getblockhash and then getblock

Do you have numbers to prove this? I suspect it is far from being a bottleneck.

emilengler commented at 3:44 pm on July 17, 2019: contributor

@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 commented at 3:53 pm on July 17, 2019: member

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

sipa commented at 4:45 pm on July 17, 2019: member

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.

MarcoFalke commented at 5:16 pm on July 17, 2019: member

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 commented at 1:05 am on July 18, 2019: member

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](/bitcoin-bitcoin/contributor/0/) | grep ^...hash
  "hash":  "0f9188f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e2206",

 0+inline uint256 GetBlockHashFromParam(const UniValue& param, const std::string& name) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
 1+{
 2+    const std::string& v = param.get_str();
 3+    if (v.size() > 1 && v[0] == '@') {
 4+        // treat as height, and lookup
 5+        int32_t height;
 6+        if (!ParseInt32(v.substr(1), &height)) {
 7+            throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Target block height %s must be @ followed by a number", v));
 8+        }
 9+        if (height < 0) {
10+            throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Target block height %d is negative", height));
11+        }
12+        const int current_tip = ::ChainActive().Height();
13+        if (height > current_tip) {
14+            throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Target block height %d after current tip %d", height, current_tip));
15+        }
16+        return *(::ChainActive()[height]->phashBlock);
17+    } else {
18+        return ParseHashV(param, name);
19+    }
20+}
21+
22 static UniValue getblock(const JSONRPCRequest& request)
23 {
24     const RPCHelpMan help{"getblock",
25@@ -904,8 +926,6 @@ static UniValue getblock(const JSONRPCRequest& request)
26         throw std::runtime_error(help.ToString());
27     }
28 
29-    uint256 hash(ParseHashV(request.params[0], "blockhash"));
30-
31     int verbosity = 1;
32     if (!request.params[1].isNull()) {
33         if(request.params[1].isNum())
34@@ -919,6 +939,7 @@ static UniValue getblock(const JSONRPCRequest& request)
35     const CBlockIndex* tip;
36     {
37         LOCK(cs_main);
38+        uint256 hash(GetBlockHashFromParam(request.params[0], "blockhash"));
39         pblockindex = LookupBlockIndex(hash);
40         tip = ::ChainActive().Tip();

emilengler commented at 3:42 pm on July 18, 2019: contributor

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

promag commented at 3:49 pm on July 18, 2019: member

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.

emilengler commented at 7:51 pm on July 18, 2019: contributor

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 commented at 9:52 pm on July 18, 2019: member

I went ahead and made a quick profile:

 0count = 5000
 1node.generate(count)
 2
 3for i in range(count):
 4    node.getblock(self.nodes[0].getblockhash(i))
 5
 6# takes ~ 3742ms
 7
 8for i in range(count):
 9    node.getblockbyheight(i)
10
11# 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.

emilengler commented at 3:34 pm on July 19, 2019: contributor

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

emilengler commented at 1:06 pm on July 20, 2019: contributor

Currently there are: 3 ACKs 1 NACK

in src/rpc/blockchain.cpp:1022 in 8d2c01f341 outdated

1017+    const CBlockIndex* tip;
1018+    {
1019+        LOCK(cs_main);
1020+        // Get index and hash
1021+        pblockindex = ::ChainActive()[request.params[0].get_int()];
1022+        uint256 hash(pblockindex->GetBlockHash());

ajtowns commented at 1:13 pm on July 22, 2019:

hash isn’t used

emilengler commented at 6:56 pm on July 22, 2019:

You’re right

rpc: Code Improvement in getblockbyheight

rpc: Fix whitespace in rpc/blockchain.cpp

rpc: getblockbyheight: Remove hash

373401fa81

ajtowns commented at 5:14 am on July 23, 2019: member

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:

 0$ time for a in `seq 400000 400500`; do bitcoin-cli getblock $(bitcoin-cli getblockhash $a); done | grep -v '"confirmations":' | sha256sum
 128620dea517c27debd958345197dc911f2fe95934742293548e79cd673bfae40  -
 2
 3real	0m15.763s
 4$ time for a in `seq 400000 400500`; do ./bitcoin-cli getblockbyheight $a; done | grep -v '"confirmations":' | sha256sum
 528620dea517c27debd958345197dc911f2fe95934742293548e79cd673bfae40  -
 6
 7real	0m11.514s
 8$ time for a in `seq 400000 400500`; do bitcoin-cli getblock $(bitcoin-cli getblockhash $a); done | grep -v '"confirmations":' | sha256sum
 928620dea517c27debd958345197dc911f2fe95934742293548e79cd673bfae40  -
10
11real	0m15.408s

ajtowns commented at 9:15 am on July 23, 2019: member

@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 commented at 3:42 pm on July 30, 2019: member

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](/bitcoin-bitcoin/contributor/height/) and getblock hash.

ryanofsky commented at 2:28 pm on July 31, 2019: member

@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 https://github.com/bitcoin/bitcoin/blob/3f288a1c05ebcadd7d7709f81c77921ff9e27ba2/src/qt/forms/debugwindow.ui#L564-L566

emilengler commented at 6:36 pm on July 31, 2019: contributor

@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 commented at 11:47 pm on August 19, 2019: member

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

emilengler commented at 2:29 pm on August 20, 2019: contributor

@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 added the label Needs rebase on Aug 27, 2019

DrahtBot commented at 5:44 pm on August 27, 2019: member

laanwj added the label Feature on Sep 30, 2019

emilengler commented at 3:27 pm on October 14, 2019: contributor

Closed because of lack of interest and merging conflicts

emilengler closed this on Oct 14, 2019

emilengler deleted the branch on Oct 14, 2019

laanwj removed the label Needs rebase on Oct 24, 2019

DrahtBot locked this on Dec 16, 2021

rpc: Add getblockbyheight method #16345

Conflicts