doc: add information about status code 404 for some endpoints (rest) #24084

brunoerg commented at 11:30 AM on January 17, 2022: contributor

This PR adds an explanation about status code 404 for 2 endpoints (/rest/tx/ and /rest/blockhashbyheight/) inREST-interface.md. There are other endpoints that already cover it.

DrahtBot added the label Docs on Jan 17, 2022

shaavan commented at 2:57 PM on January 19, 2022: contributor

I have not yet reviewed this PR. However, I was curious about one change that has been done here.

Responds with 404 if block height is out of range.

I googled for the error code's meaning:

**404:** an error message displayed by a browser indicating that an internet address cannot be found
**400:** Bad Request response status code indicates that the server cannot or will not process the request due to something that is perceived to be a client error

Giving an out-of-range block height is more of a bad request than a cannot found error, won't you think? Again, I am not talking about this PR per se, but in general about returning 404 for out-of-range block height.

brunoerg commented at 3:12 PM on January 19, 2022: contributor

Giving an out-of-range block height is more of a bad request than a cannot found error, won't you think?

No, the HTTP 404 Not Found response status code indicates that the server cannot find the requested resource. When you pass a valid block height but it wasn't able to find it, it means 404, because your request is valid but the server wasn't able to find the requested resource (e.g. your node can be out of date). However, if you pass an invalid block height like: 'abc', 'dksaodka', '3j213j1k13j', the server won't be able to process it, so, it should return 500 or 400. @shaavan

in doc/REST-interface.md:74 in 1c3bab363a outdated

  70 | @@ -71,6 +71,7 @@ Responds with 404 if the block doesn't exist.
  71 |  `GET /rest/blockhashbyheight/<HEIGHT>.<bin|hex|json>`
  72 |  
  73 |  Given a height: returns hash of block in best-block-chain at height provided.
  74 | +Responds with 404 if block height is out of range.

unknown commented at 8:58 PM on January 19, 2022:

Responds with 404 if block not found

brunoerg commented at 9:38 PM on January 19, 2022:

Agreed

shaavan commented at 5:09 AM on January 20, 2022:

This suggestion improves the statement and goes along with the classical use of error 404.

brunoerg force-pushed on Jan 19, 2022

brunoerg commented at 9:42 PM on January 19, 2022: contributor

force-pushed addressing @prayank23's comment

unknown approved

unknown commented at 9:43 PM on January 19, 2022: none

ACK https://github.com/bitcoin/bitcoin/pull/24084/commits/9491932779d0564e8db50619d4b371798eaa974d

shaavan commented at 5:13 AM on January 20, 2022: contributor

Concept ACK

I think you should merge the commits together.

w0xlt approved

w0xlt commented at 3:09 PM on January 23, 2022: contributor

ACK https://github.com/bitcoin/bitcoin/pull/24084/commits/9491932779d0564e8db50619d4b371798eaa974d

I agree that the commits can be merged together.

doc: add info about status code 404 for some rest endpoints 0811cbfc28

brunoerg force-pushed on Jan 24, 2022

brunoerg commented at 11:40 AM on January 24, 2022: contributor

force-pushed squashing the commits

unknown approved

unknown commented at 11:58 AM on January 24, 2022: none

reACK https://github.com/bitcoin/bitcoin/pull/24084/commits/0811cbfc2868ee80c522fd426f188f10b06cd421

shaavan approved

shaavan commented at 1:40 PM on January 24, 2022: contributor

ACK 0811cbfc2868ee80c522fd426f188f10b06cd421

Changes since my last review:

The commits have been squashed together.

luke-jr commented at 3:43 AM on January 29, 2022: member

Weak Concept NACK. It seems to me REST implies usage of standard HTTP error codes already, and it doesn't need to be re-stated explicitly here.

brunoerg commented at 12:12 PM on February 6, 2022: contributor

Weak Concept NACK. It seems to me REST implies usage of standard HTTP error codes already, and it doesn't need to be re-stated explicitly here. @luke-jr, I think it is important to mention the status code to clarify how it works when you pass a valid txid/blockhash but it wasn't able to find it or when you pass an invalid txid/blockhash. In my case, I didn't know if when I pass an invalid txid, it would try to find it and return not found or if it validates the txid and returns 500 because it is wrong.

By the way, there are other endpoints with this same approach: https://github.com/bitcoin/bitcoin/blob/1e7564eca8a688f39c75540877ec3bdfdde766b1/doc/REST-interface.md?plain=1#L43

DrahtBot commented at 1:40 PM on September 23, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16545 (refactor: Implement missing error checking for ArgsManager flags by ryanofsky)

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.

fanquake merged this on Oct 2, 2022

fanquake closed this on Oct 2, 2022

bitcoin locked this on Oct 2, 2023