New issue

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

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

Already on GitHub? Sign in to your account

[REST] added blockhash api, tests and documentation #11765

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@aaron-hanson
Contributor

aaron-hanson commented Nov 25, 2017

Added a /rest/blockhash/.json endpoint, so that the user can fetch a block hash by height via REST (analogous to the 'getblockhash' RPC method).

For someone wanting to gather block or header data via REST only, there was no way to begin fetching blocks/headers at specific heights without knowing the block hashes at those heights. This endpoint might also come in handy for someone wanting to quickly verify a block existing at a specific height in the active chain.

@jonasschnelli

This comment has been minimized.

Member

jonasschnelli commented Nov 25, 2017

Nice. Thanks for contributing.
Concept ACK.

What holds you back in completing this with supporting hex/bin? Should be trivial.

@aaron-hanson

This comment has been minimized.

Contributor

aaron-hanson commented Nov 25, 2017

@jonasschnelli Yeah I noticed the hex/bin pattern but wasn't entirely sure if I should use those here too. I suppose I wasn't sure exactly what should be serialized in this case, as the other endpoints using bin/hex are serializing whole class/struct instances like CBlockHeader or CBlock, whereas this is a simple hash string. I can certainly add that support.

Thanks for taking a look!

@jonasschnelli

This comment has been minimized.

Member

jonasschnelli commented Nov 25, 2017

@aaron-hanson.
You can just create a data stream and push the hashes onto the stream. A rest client could save ~50% brutto bandwidth over a JSON/hexstring.

CDataStream ssFooBar(SER_NETWORK, PROTOCOL_VERSION);
ssGetUTXOResponse << pindex->GetBlockHash();
@aaron-hanson

This comment has been minimized.

Contributor

aaron-hanson commented Nov 25, 2017

Added bin/hex formats, associated tests and documentation.

@promag

Concept ACK. I agree it should be possible to query block by height.

There are only tests for successful calls. IMHO there should be tests for the errors too:

  • RESTERR(req, HTTP_BAD_REQUEST, "Parse error");
  • RESTERR(req, HTTP_BAD_REQUEST, "Block height out of range: " + strHeight).

Another option would be to overload /rest/block/[hash|height].[bin,hex.json]. Note that json response includes the hash.

std::string strHex = HexStr(ssGetBlockHashResponse.begin(), ssGetBlockHashResponse.end()) + "\n";
req->WriteHeader("Content-Type", "text/plain");
req->WriteReply(HTTP_OK, strHex);

This comment has been minimized.

@jonasschnelli

jonasschnelli Nov 25, 2017

Member

Instead of forming the stream, use req->WriteReply(HTTP_OK, pindex->GetBlockHash()->GetHex());?

This comment has been minimized.

@aaron-hanson

aaron-hanson Nov 25, 2017

Contributor

Yeah I was wondering about this...I had just followed the pattern for the other endpoints' hex formats. Directly writing the output of pindex->GetBlockHash().GetHex() reverses the order of the bytes as compared to HexStr(). I'm not sure which is correct?

@aaron-hanson

This comment has been minimized.

Contributor

aaron-hanson commented Nov 25, 2017

Another option would be to overload /rest/block/[hash|height].[bin,hex.json]. Note that json response includes the hash.

@promag - I thought about this too... After researching a bit I found an older issue (#6011) about essentially the same thing, but in RPC. I assumed from the opinions there that I should probably just add this blockhash endpoint and not overload the block endpoint.

@promag

This comment has been minimized.

Member

promag commented Nov 26, 2017

As others say in #6011, it was possible to query block by height but with 2 calls. With REST call there is no way unless you walk back from the tip. Unless we want to mirror the RPC interface, I think overloading sounds cooler.

In that scenario, the difference between the 2 endpoints would be cache headers, since by hash the block is immutable but not by height (at least near the tip).

@promag

This comment has been minimized.

Member

promag commented Nov 26, 2017

BTW nice first contribution.

@promag

New tests looks good.

return true;
}
default: {
return RESTERR(req, HTTP_NOT_FOUND, "output format not found (available: json)");

This comment has been minimized.

@promag

promag Nov 26, 2017

Member

Only json format? 😄

This comment has been minimized.

@aaron-hanson

aaron-hanson Nov 26, 2017

Contributor

Ah, nice catch...details!

@promag

This comment has been minimized.

Member

promag commented Nov 27, 2017

utACK b026c5f. Needs squash.

@promag

This comment has been minimized.

Member

promag commented Feb 3, 2018

Needs rebase.

@jnewbery

This comment has been minimized.

Member

jnewbery commented Apr 3, 2018

@aaron-hanson - are you still working on this? It needs rebasing and squashing.

Suggest we close with 'up for grabs' if there's no activity on this PR soon.

@aaron-hanson

This comment has been minimized.

Contributor

aaron-hanson commented Apr 3, 2018

@jnewbery - ah sorry, for some reason I missed the previous comments... I'll squash/rebase and retest tonight.

[REST] added blockhash api, tests and documentation
Added a /rest/blockhash/<HEIGHT>.<bin|hex|json> endpoint, so that the user can fetch a block hash by height via REST (analogous to the 'getblockhash' RPC method).
@aaron-hanson

This comment has been minimized.

Contributor

aaron-hanson commented Apr 4, 2018

@jnewbery @promag - squashed and rebased.

@jnewbery

Generally looks great. Thanks for the contribution and the good tests.

There are a few code style nits, which I haven't commented on inline. Take a look at https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style. Notably:

  • variables should be snake case
  • switch code block should be indented
  • the then clause for if statements should either be on the same line or enclosed in braces.

(I realise that you were probably copying surrounding style, but new code should adhere to the style guildelines where possible.)

switch (rf) {
case RetFormat::BINARY: {
CDataStream ssGetBlockHashResponse(SER_NETWORK, PROTOCOL_VERSION);
ssGetBlockHashResponse << pindex->GetBlockHash();

This comment has been minimized.

@jnewbery

jnewbery Apr 4, 2018

Member

Could the call to pindex->GetBlockHash() be refactored out of the switch statement, both to reduce code repetition and to minimise the scope of where cs_main is held?

@@ -223,6 +223,56 @@ def run_test(self):
self.nodes[0].generate(1) #generate block to not affect upcoming tests
self.sync_all()

This comment has been minimized.

@jnewbery

jnewbery Apr 4, 2018

Member

Be aware of #12766, which completely refactors this test (and makes it much better IMO). That obviously conflicts heavily with this change.

@aaron-hanson

This comment has been minimized.

Contributor

aaron-hanson commented Jun 18, 2018

@jnewbery thanks for the tips! I should have time this week to make the style adjustments and do a new rebase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment