[REST] added blockhash api, tests and documentation #11765
Conversation
|
Nice. Thanks for contributing. What holds you back in completing this with supporting hex/bin? Should be trivial. |
|
@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! |
|
@aaron-hanson. |
469317d to
e02f4c3
Compare
|
Added bin/hex formats, associated tests and documentation. |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
Instead of forming the stream, use req->WriteReply(HTTP_OK, pindex->GetBlockHash()->GetHex());?
There was a problem hiding this comment.
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?
@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. |
|
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). |
|
BTW nice first contribution. |
src/rest.cpp
Outdated
| return true; | ||
| } | ||
| default: { | ||
| return RESTERR(req, HTTP_NOT_FOUND, "output format not found (available: json)"); |
There was a problem hiding this comment.
Ah, nice catch...details!
|
utACK b026c5f. Needs squash. |
|
Needs rebase. |
|
@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. |
|
@jnewbery - ah sorry, for some reason I missed the previous comments... I'll squash/rebase and retest tonight. |
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).
b026c5f to
1323df9
Compare
jnewbery
left a comment
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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?
| self.nodes[0].generate(1) #generate block to not affect upcoming tests | ||
| self.sync_all() | ||
|
|
||
|
|
There was a problem hiding this comment.
Be aware of #12766, which completely refactors this test (and makes it much better IMO). That obviously conflicts heavily with this change.
|
@jnewbery thanks for the tips! I should have time this week to make the style adjustments and do a new rebase. |
|
This was replaced by #14353. Up for grabs label can be removed. |
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.