rest: Return 404 in /rest/headers if block hash does not exists #15107

pull promag wants to merge 1 commits into bitcoin:master from promag:2019-rest-header-404 changing 2 files +5 −0

promag commented at 12:25 AM on January 5, 2019: member

This PR changes /rest/headers response to 404 when the given block hash does not exists. Before the response would be an empty array of headers.

Also add a test for /rest/block/ which already returns the 404 error.

Note, will add release note if accepted.
rest: Return 404 in /rest/headers if block hash does not exists e1386cc4f2
fanquake added the label RPC/REST/ZMQ on Jan 5, 2019
MarcoFalke added the label Needs release note on Jan 5, 2019
laanwj commented at 1:01 PM on January 7, 2019: member

I'm a bit confused here. From the fact that these call return an array I surmise it's possible for it to return more than one result as well? If so, I think handling the zero case specially is not a good idea.

If not, then I'm ok with this change.
jgarzik commented at 1:51 PM on January 7, 2019: contributor

concept ACK -- it makes sense to give a definitive and HTTP-standard answer for this condition, rather than forcing the caller to infer it.
promag commented at 3:50 PM on January 7, 2019: member

@laanwj the error already exits, for instance, in /rest/block:

https://github.com/bitcoin/bitcoin/blob/2d5f1ea2e3f9629d313f1d2aff94a15ef8e1128a/src/rest.cpp#L213-L217

in test/functional/interface_rest.py:204 in e1386cc4f2

 200 | @@ -201,6 +201,10 @@ def run_test(self):
 201 |          self.log.info("Test the /block and /headers URIs")
 202 |          bb_hash = self.nodes[0].getbestblockhash()
 203 |  
 204 | +        # Should give 404 if block hash does not exists

PatrickZGW commented at 5:04 PM on January 9, 2019:

        # Should give 404 if block hash does not exist

sipa commented at 6:34 PM on January 9, 2019: member

So with this change you'll get a 404 when the block hash is unknown, but you'll still get an empty array when you request a known block hash not in the main chain. Is that intentional (it may or may not be, I can see arguments both ways).
promag commented at 7:17 PM on January 9, 2019: member

@sipa oh haven't considered that case. IMO that's an advantage of this change. What arguments do you see for the other way?
sipa commented at 2:27 PM on January 11, 2019: member

@promag If the REST interface for blocks is supposed to give access to "the best headers chain", it shouldn't distinguish between unknown or not in the chain.
promag commented at 2:45 PM on January 11, 2019: member
It'd be consistent with /block/<hash> response. Not sure why you say "shouldn't" when for the client this is more informative:
- the client might be in the wrong network
- the node is syncing
- some bug or data corruption on the client
And for the server I don't see harm in distinguishing.

Anyway, if there's agreement to what you say then the documentation could be updated.
laanwj commented at 2:36 PM on January 14, 2019: member

I think it's (marginally) better to update the documentation.

Unless you have a strong use case for distinguishing these in your own code that uses this API?
promag commented at 10:57 PM on January 14, 2019: member

@laanwj no use case.

What @sipa made me realize is that strictly speaking /block and /headers are semantically different.
promag closed this on Jan 14, 2019
promag deleted the branch on Jan 14, 2019
laanwj commented at 1:32 PM on January 16, 2019: member

Also add a test for /rest/block/ which already returns the 404 error.

We could still add this test?
promag commented at 1:35 PM on January 16, 2019: member

I'll submit it along with a documentation update.
laanwj commented at 1:36 PM on January 16, 2019: member

Thanks!
promag commented at 2:53 PM on January 16, 2019: member

I've decided to include it in #15177, it's a small change.
laanwj referenced this in commit 72506ed349 on Jan 21, 2019
fanquake removed the label Needs release note on Mar 20, 2019
PastaPastaPasta referenced this in commit d4cfeb4477 on Jun 26, 2021
PastaPastaPasta referenced this in commit 2887c33c5b on Jun 26, 2021
PastaPastaPasta referenced this in commit 9c9b5efa8e on Jun 27, 2021
PastaPastaPasta referenced this in commit 949f158bc1 on Jun 28, 2021
DrahtBot locked this on Dec 16, 2021

Contributors

Labels

Linked (view graph)

#15177 rest: Improve tests and documention of /headers and /block