REST: add blockhash call, fetch blockhash by height

jonasschnelli commented at 7:57 pm on September 29, 2018: contributor

Completes the REST interface for trivial block exploring by adding a call that allows to fetch the blockhash in the main chain by a given height.

jonasschnelli added the label RPC/REST/ZMQ on Sep 29, 2018

in src/rest.cpp:592 in 1477ecb5fe outdated

587+    uint32_t blockheight;
588+    if (!ParseUInt32(height_str, &blockheight)) {
589+        return RESTERR(req, HTTP_BAD_REQUEST, "Invalid height: " + SanitizeString(height_str));
590+    }
591+
592+    LOCK(cs_main);

promag commented at 8:59 pm on September 29, 2018:

Could reduce lock scope.

jonasschnelli commented at 9:34 am on September 30, 2018:

Fixed.

in test/functional/interface_rest.py:232 in 1477ecb5fe outdated

226@@ -227,6 +227,12 @@ def run_test(self):
227         # Check json format
228         block_json_obj = self.test_rest_request("/block/{}".format(bb_hash))
229         assert_equal(block_json_obj['hash'], bb_hash)
230+        assert_equal(self.test_rest_request("/blockhash/{}".format(block_json_obj['height'])), bb_hash)
231+
232+        # Check invalid blockheader requests

promag commented at 9:00 pm on September 29, 2018:

Test without argument?

jonasschnelli commented at 9:34 am on September 30, 2018:

Thanks. Added.

in test/functional/interface_rest.py:201 in 1477ecb5fe outdated

197@@ -198,7 +198,7 @@ def run_test(self):
198         self.nodes[0].generate(1)  # generate block to not affect upcoming tests
199         self.sync_all()
200 
201-        self.log.info("Test the /block and /headers URIs")
202+        self.log.info("Test the /block, /blockheader and /headers URIs")

jgarzik commented at 1:02 am on September 30, 2018:

/blockhash presumably

jonasschnelli commented at 9:34 am on September 30, 2018:

Thanks. Fixed

jgarzik commented at 1:03 am on September 30, 2018: contributor

concept ACK

in src/rest.cpp:593 in 1477ecb5fe outdated

588+    if (!ParseUInt32(height_str, &blockheight)) {
589+        return RESTERR(req, HTTP_BAD_REQUEST, "Invalid height: " + SanitizeString(height_str));
590+    }
591+
592+    LOCK(cs_main);
593+    if (blockheight < 0 || blockheight > chainActive.Height()) {

jimmysong commented at 1:59 am on September 30, 2018:

Is the blockheight < 0 really necessary if it’s using ParseUInt32? We know it’s an unsigned integer, no?

jonasschnelli commented at 9:34 am on September 30, 2018:

Thanks. Nah,.. it’s not necessary. Removed the <0 check.

jonasschnelli force-pushed on Sep 30, 2018

jimmysong approved

jimmysong commented at 10:05 pm on September 30, 2018: contributor

Thanks @jonasschnelli !

in src/rest.cpp:595 in ae0a5a1fce outdated

590+    }
591+
592+    CBlockIndex* pblockindex = nullptr;
593+    {
594+        LOCK(cs_main);
595+        if (blockheight > chainActive.Height()) {

promag commented at 10:48 pm on September 30, 2018:

0rest.cpp:595:25: warning: comparison of integers of different signs: 'uint32_t' (aka 'unsigned int') and 'int' [-Wsign-compare]
1        if (blockheight > chainActive.Height()) {
2            ~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~
31 warning generated.

jonasschnelli commented at 7:44 pm on October 17, 2018:

Fixed.

promag commented at 5:13 pm on October 1, 2018: member

Tested ACK ae0a5a1.

What is the use case? Query block hash by height and then query the block? If so could overload /rest/block instead to avoid 2nd round trip?

jonasschnelli commented at 7:01 pm on October 1, 2018: contributor

@promag: the use case is exactly that (find a block by giving a height) (ex: https://bitcointools.jonasschnelli.ch/explorer/height/500000).

You could build it into /rest/block, but IMO we should keep it modular. I guess that is also the reason why there is a “getblock” and a “getblockhash” call on RPC.

gmaxwell commented at 7:33 pm on October 1, 2018: contributor

PR is mistitled, not a “blockheader” call. This PR doesn’t really seem to indicate the benefit this provides to the user. Making multiple round trips also has bad performance, if the purpose really is just querying blocks by height this indirection seems like pretextual modularity.

jonasschnelli renamed this:
~~REST: add blockheader call, fetch blockhash by height~~
REST: add blockhash call, fetch blockhash by height
on Oct 1, 2018

jonasschnelli commented at 8:02 pm on October 1, 2018: contributor

Changed the title (was wrong, thank @gmaxwell)

I’m happy to add a per-height accessing to /rest/block, but this would mean accessing that new per-height-fetching would always return a block (optionally reduced to only contain txids) which would always access the disk.

Of course, we could then add it to /rest/headers/ as well so one would fetch a single blockheader (or range of headers) per height.

I haven’t made a study about use cases but the extra roundtrip made sense to me (costs for http mem only retrieval roundtrips are usually tiny).

ch4ot1c commented at 8:58 pm on October 2, 2018: contributor

Ack, seems marginally beneficial

in doc/REST-interface.md:37 in ae0a5a1fce outdated

30@@ -31,6 +31,11 @@ With the /notxdetails/ option JSON response will only contain the transaction ha
31 
32 Given a block hash: returns <COUNT> amount of blockheaders in upward direction.
33 
34+#### Blockhash
35+`GET /rest/blockhash/<HEIGHT>.<bin|hex|json>`
36+
37+Given a height: returns blockhash of block in main chain at height <HEIGHT>.

ch4ot1c commented at 9:00 pm on October 2, 2018:

<HEIGHT> doesn’t appear in the Markdown preview.

jonasschnelli commented at 7:44 pm on October 17, 2018:

Fixed

jonasschnelli referenced this in commit 5505b19c29 on Oct 7, 2018

jonasschnelli force-pushed on Oct 17, 2018

promag commented at 1:38 am on October 18, 2018: member

utACK, but fix first commit message.

DrahtBot commented at 6:50 pm on November 9, 2018: 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:

#11770 ([REST] add a rest endpoint for estimatesmartfee, docs, and test by joemphilips)

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.

in src/rest.cpp:615 in 8e9cb6b221 outdated

610+        req->WriteReply(HTTP_OK, pblockindex->GetBlockHash().GetHex() + "\n");
611+        return true;
612+    }
613+    case RetFormat::JSON: {
614+        req->WriteHeader("Content-Type", "application/json");
615+        req->WriteReply(HTTP_OK, UniValue(pblockindex->GetBlockHash().GetHex()).write() + "\n");

luke-jr commented at 2:58 am on December 20, 2018:

Bare strings are not valid JSON

luke-jr commented at 2:59 am on December 20, 2018: member

See also #11765

jonasschnelli force-pushed on Jan 3, 2019

jonasschnelli commented at 10:05 pm on January 3, 2019: contributor

Fixed @luke-jr point (invalid JSON respons) Renamed from blockhash to blockhashbyheight.

in test/functional/interface_rest.py:201 in 3a934118f8 outdated

197@@ -198,7 +198,7 @@ def run_test(self):
198         self.nodes[0].generate(1)  # generate block to not affect upcoming tests
199         self.sync_all()
200 
201-        self.log.info("Test the /block and /headers URIs")
202+        self.log.info("Test the /block, /blockhash and /headers URIs")

laanwj commented at 9:43 am on January 4, 2019:

nit: this is /blockhashbyheight now

laanwj commented at 9:43 am on January 4, 2019: member

utACK (-nit), I’m surprised this didn’t exist yet.

in doc/REST-interface.md:37 in 3a934118f8 outdated

30@@ -31,6 +31,11 @@ With the /notxdetails/ option JSON response will only contain the transaction ha
31 
32 Given a block hash: returns <COUNT> amount of blockheaders in upward direction.
33 
34+#### Blockhash by height
35+`GET /rest/blockhashbyheight/<HEIGHT>.<bin|hex|json>`
36+
37+Given a height: returns blockhash of block in main chain at height `<HEIGHT>`.

laanwj commented at 9:48 am on January 4, 2019:

The help for RPC getblockhash is Returns hash of block in best-block-chain at height provided. I think that’s slightly better terminology to use here than “main chain” which could be confused with testnet/mainnet/….

jonasschnelli force-pushed on Jan 4, 2019

jonasschnelli commented at 6:55 pm on January 4, 2019: contributor

Fixed documentation and the test comment inconsistency (reported by @laanwj)

in test/functional/interface_rest.py:233 in d63921b007 outdated

226@@ -227,6 +227,13 @@ def run_test(self):
227         # Check json format
228         block_json_obj = self.test_rest_request("/block/{}".format(bb_hash))
229         assert_equal(block_json_obj['hash'], bb_hash)
230+        assert_equal(self.test_rest_request("/blockhashbyheight/{}".format(block_json_obj['height']))['blockhash'], bb_hash)
231+
232+        # Check invalid blockhashbyheight requests
233+        self.test_rest_request("/blockhashbyheight/abc", ret_type=RetType.OBJ, status=400)

jnewbery commented at 9:23 pm on January 18, 2019:

Suggestion: you could test the error messages with something like:

0        resp = self.test_rest_request("/blockhashbyheight/abc", req_type=ReqType.JSON, ret_type=RetType.OBJ, status=400)
1        assert_equal(resp.read().decode('utf-8').rstrip(), "Invalid height: abc")

(or by enhancing the test_rest_request() method to assert error messages)

in src/rest.cpp:602 in d63921b007 outdated

597+        }
598+        pblockindex = chainActive[blockheight];
599+    }
600+    switch (rf) {
601+    case RetFormat::BINARY: {
602+        CDataStream ss_blockhash(SER_NETWORK, PROTOCOL_VERSION | RPCSerializationFlags());

jnewbery commented at 9:43 pm on January 18, 2019:

RPCSerializationFlags() is not strictly required since you’re not serializing any transactions (and the only flag that this function can toggle is the SERIALIZE_TRANSACTION_NO_WITNESS flag).

If you want to keep this here, perhaps we should consider adding it to all the CDataStreams in this file.

in test/functional/interface_rest.py:230 in d63921b007 outdated

226@@ -227,6 +227,13 @@ def run_test(self):
227         # Check json format
228         block_json_obj = self.test_rest_request("/block/{}".format(bb_hash))
229         assert_equal(block_json_obj['hash'], bb_hash)
230+        assert_equal(self.test_rest_request("/blockhashbyheight/{}".format(block_json_obj['height']))['blockhash'], bb_hash)

jnewbery commented at 10:06 pm on January 18, 2019:

It’d be good to also test the bytes and hex versions. Something like this should do the trick:

0        resp_hex = self.test_rest_request("/blockhashbyheight/{}".format(block_json_obj['height']),
1                                          req_type=ReqType.HEX, ret_type=RetType.OBJ)
2        assert_equal(resp_hex.read().decode('utf-8').rstrip(), bb_hash)
3        resp_bytes = self.test_rest_request("/blockhashbyheight/{}".format(block_json_obj['height']),
4                                            req_type=ReqType.BIN, ret_type=RetType.BYTES)
5        blockhash = binascii.hexlify(resp_bytes[::-1]).decode('utf-8')
6        assert_equal(blockhash, bb_hash)

in src/rest.cpp:621 in d63921b007 outdated

616+        resp.pushKV("blockhash", pblockindex->GetBlockHash().GetHex());
617+        req->WriteReply(HTTP_OK, resp.write() + "\n");
618+        return true;
619+    }
620+    default: {
621+        return RESTERR(req, HTTP_NOT_FOUND, "output format not found (available: " + AvailableDataFormatsString() + ")");

jnewbery commented at 10:08 pm on January 18, 2019:

I think this should be HTTP_BAD_REQUEST rather than HTTP_NOT_FOUND, but I see that you’ve just copied this from the other methods, so no need to change this.

jnewbery commented at 10:09 pm on January 18, 2019: member

looks good. A few comments, mostly around better test coverage :)

jonasschnelli force-pushed on Jan 19, 2019

jonasschnelli commented at 6:11 am on January 19, 2019: contributor

Fixed points reported by @jnewbery (thanks for the review).

promag commented at 10:28 am on January 20, 2019: member

@jonasschnelli commit messages should mention /rest/blockhashbyheight.

REST: add "blockhashbyheight" call, fetch blockhash by height eb9ef04c4e

[QA] add rest tests for /rest/blockhashbyheight/<HEIGHT>.<FORMAT> 579d418f74

[Docs] add short documentation for /rest/blockhashbyheight 42ff30ec60

jonasschnelli force-pushed on Jan 21, 2019

jonasschnelli commented at 9:55 pm on January 21, 2019: contributor

Fixed the commit message

jnewbery commented at 10:19 pm on January 22, 2019: member

utACK 42ff30ec60c6ab6a9555da57a435b09cf217ee84

promag commented at 11:58 pm on January 22, 2019: member

utACK 42ff30e.

jonasschnelli merged this on Jan 23, 2019

jonasschnelli closed this on Jan 23, 2019

jonasschnelli referenced this in commit 82cf6813a4 on Jan 23, 2019

jasonbcox referenced this in commit 488bfc24b6 on Sep 30, 2020

PastaPastaPasta referenced this in commit ea2917a451 on Jun 26, 2021

PastaPastaPasta referenced this in commit d2cd397ea6 on Jun 26, 2021

PastaPastaPasta referenced this in commit 6850906f05 on Jun 27, 2021

PastaPastaPasta referenced this in commit 517788dabc on Jun 28, 2021

DrahtBot locked this on Dec 16, 2021

REST: add blockhash call, fetch blockhash by height #14353

Conflicts