A few followups after introducing `/rest/blockpart/` endpoint #34074

pull romanz wants to merge 6 commits into bitcoin:master from romanz:followups-33657 changing 4 files +23 −20
  1. romanz commented at 9:26 AM on December 14, 2025: contributor

    The commits below should resolve a few leftovers from #33657.

  2. rest: reformat `uri_prefixes` initializer list
    There was an extra indentation level (found during #33657):
```
$ git show -U0 07135290c1 | ./contrib/devtools/clang-format-diff.py -p1 -i -v
```
    599effdeab
  3. DrahtBot renamed this:
    A few #33657-related followups
    A few #33657-related followups
    on Dec 14, 2025
  4. DrahtBot commented at 9:26 AM on December 14, 2025: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34074.

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK l0rinc, hodlinator
    Stale ACK maflcko

    If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #33904 (rest: add interface for gettxspendingprevout rpc by kevkevinpal)

    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.

    <!--5faf32d7da4f0f540f40219e4f7537a3-->

  5. romanz marked this as a draft on Dec 14, 2025
  6. blockstorage: simplify partial block read validation
    Use `SaturatingAdd` following https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2610832092.
    41118e17f8
  7. romanz force-pushed on Dec 14, 2025
  8. DrahtBot added the label CI failed on Dec 14, 2025
  9. DrahtBot commented at 9:45 AM on December 14, 2025: contributor

    <!--85328a0da195eb286784d51f73fa0af9-->

    🚧 At least one of the CI tasks failed. <sub>Task macOS native: https://github.com/bitcoin/bitcoin/actions/runs/20205915800/job/58004478080</sub> <sub>LLM reason (✨ experimental): Blockmanager tests failed due to an assertion failure (has_value()) aborting blockmanager_tests.</sub>

    <details><summary>Hints</summary>

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

    </details>

  10. romanz marked this as ready for review on Dec 14, 2025
  11. l0rinc commented at 10:58 AM on December 14, 2025: contributor

    A few quick notes:

  12. DrahtBot removed the label CI failed on Dec 14, 2025
  13. romanz renamed this:
    A few #33657-related followups
    A few followups after introducing `/rest/blockpart/` endpoint
    on Dec 14, 2025
  14. l0rinc commented at 11:48 AM on December 14, 2025: contributor

    code review ACK 02ae5aaa5ab104731933548d884e1f5ce0c29a15

  15. fanquake commented at 12:22 PM on December 14, 2025: member

    cc @hodlinator

  16. maflcko commented at 9:08 AM on December 15, 2025: member

    lgtm, just some nits addressed.

    review ACK 02ae5aaa5ab104731933548d884e1f5ce0c29a15 🏄

    <details><summary>Show signature</summary>

    Signature:

    untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 02ae5aaa5ab104731933548d884e1f5ce0c29a15 🏄
+jzwSGMwUFlzRZStnnObOKp6w5zRtczGt9jP+5LsoQg605AreQtRhmb99bjvUTymCftIKPVOHc3J5KEjtfopCg==

    </details>

  17. in doc/release-notes-33657.md:4 in 02ae5aaa5a 
       0 | @@ -1,5 +1,5 @@
   1 |  New REST API
   2 |  ------------
   3 |  
   4 | -- A new REST API endpoint (`/rest/blockpart/BLOCKHASH.bin?offset=X&size=Y`) has been introduced
   5 | +- A new REST API endpoint (`/rest/blockpart/BLOCKHASH.<bin|hex>?offset=<OFFSET>&size=<SIZE>`) has been introduced
    hodlinator commented at 9:31 AM on December 15, 2025:

    nit: It seems more consistent with REST-interface.md to also call out that the block hash with angle brackets:

    - A new REST API endpoint (`/rest/blockpart/<BLOCK-HASH>.<bin|hex>?offset=<OFFSET>&size=<SIZE>`) has been introduced
  18. hodlinator approved
  19. hodlinator commented at 9:32 AM on December 15, 2025: contributor

    crACK 02ae5aaa5ab104731933548d884e1f5ce0c29a15

  20. maflcko commented at 10:48 AM on December 15, 2025: member

    Could also add a test for the missing case?

    self.test_rest_request(f"/blockpart/{blockhash}", query_params={"offset":0,"size":1}, status=400, req_type=ReqType.JSON, ret_type=RetType.OBJ)

    for:

            return RESTERR(req, HTTP_BAD_REQUEST, "JSON output is not supported for this request type");
  21. rest: update release notes for `/blockpart/` endpoint 89eb531024
  22. rest: deduplicate `interface_rest.py` negative tests 55d0d19b5c
  23. romanz force-pushed on Dec 15, 2025
  24. l0rinc commented at 10:43 PM on December 15, 2025: contributor

    reACK 823d4d8a5b71b7c740a6552f72fbfdb9b2813ca6

  25. DrahtBot requested review from maflcko on Dec 15, 2025
  26. DrahtBot requested review from hodlinator on Dec 15, 2025
  27. in test/functional/interface_rest.py:492 in 823d4d8a5b 
     484 | @@ -485,13 +485,15 @@ def get_block_part(status: int = 200, **kwargs):
 485 |  
 486 |              get_block_part(status=400, query_params={"offset": 0, "size": 0})
 487 |              get_block_part(status=400, query_params={"offset": len(block_bin), "size": 0})
 488 | -            get_block_part(status=400, query_params={"offset": len(block_bin) + 1, "size": 1})
 489 |              get_block_part(status=400, query_params={"offset": len(block_bin), "size": 1})
 490 |              get_block_part(status=400, query_params={"offset": len(block_bin) + 1, "size": 1})
 491 |              get_block_part(status=400, query_params={"offset": 0, "size": len(block_bin) + 1})
 492 |  
 493 |          self.test_rest_request(f"/blockpart/{blockhash}", status=400, req_type=ReqType.JSON, ret_type=RetType.OBJ)
    hodlinator commented at 9:13 AM on December 16, 2025:

    nit: This one could also do with more precision, to disambiguate whether it's testing for failure because of req_type=JSON or because of omitting offset/size:

            res = self.test_rest_request(f"/blockpart/{blockhash}", status=400, req_type=ReqType.BIN, ret_type=RetType.OBJ)
        assert res.read().decode().startswith("Block part offset missing or invalid")

    (Edit: removed , f"Got: {resp}" residue).

    l0rinc commented at 9:58 AM on December 16, 2025:

    If you decide to take this, please consider adding the reason as well, as described by @ryanofsky in #33657 (review)

    hodlinator commented at 9:29 AM on December 17, 2025:

    Thanks for taking this!

    Regarding including the reason, it seems fairly hard coded: "Bad Request", "OK", etc. So only useful to avoid having to know/look up the HTTP status codes.

    ryanofsky commented at 7:37 PM on December 17, 2025:

    re: #34074 (review)

    Regarding including the reason, it seems fairly hard coded: "Bad Request", "OK", etc. So only useful to avoid having to know/look up the HTTP status codes.

    Haven't tested this but it would be surprising if true. There are lots of specific reasons returned in the src/rest.cpp file like specific block hashes not found, undo data missing, output format not supported, pruned data errors, etc

    hodlinator commented at 1:58 PM on January 3, 2026:

    Errors are typically reported using RESTERR()... https://github.com/bitcoin/bitcoin/blob/59b93f11e8600d5224359f4d05619c0f56aef1e6/src/rest.cpp#L88 https://github.com/bitcoin/bitcoin/blob/59b93f11e8600d5224359f4d05619c0f56aef1e6/src/rest.cpp#L70-L75 ...and the message is written to the response body, we call evhttp_send_reply(..., nStatus, const char* reason=nullptr, ...). https://github.com/bitcoin/bitcoin/blob/59b93f11e8600d5224359f4d05619c0f56aef1e6/src/httpserver.cpp#L654-L666

    New PR (#32061) replacing libevent also uses custom body + fixed reason strings: https://github.com/bitcoin/bitcoin/blob/625088468a3f2f1438fd71d1b87210e16904fa58/src/httpserver.cpp#L554-L564 https://github.com/bitcoin/bitcoin/blob/625088468a3f2f1438fd71d1b87210e16904fa58/src/httpserver.cpp#L501-L502 https://github.com/bitcoin/bitcoin/blob/625088468a3f2f1438fd71d1b87210e16904fa58/src/rpc/protocol.h#L23-L35

    https://www.rfc-editor.org/rfc/rfc2616#page-40

    The reason phrases listed here are only recommendations -- they MAY be replaced by local equivalents without affecting the protocol.

    That sounds more like a localization-thing.

    Per IBM (https://www.ibm.com/docs/en/cics-ts/6.x?topic=concepts-status-codes-reason-phrases):

    The HTTP headers for the response, or the response body, or both, may provide further instructions and information for the client. The HTTP specifications include requirements and suggestions for the content of responses with each status code.

    So I think our behavior on the C++ side is arguably correct.

  28. hodlinator approved
  29. hodlinator commented at 9:14 AM on December 16, 2025: contributor

    re-ACK 823d4d8a5b71b7c740a6552f72fbfdb9b2813ca6

  30. romanz force-pushed on Dec 16, 2025
  31. maflcko commented at 10:31 PM on December 16, 2025: member

    review ACK 584f0cbf7932b4e5cf629c90fc7453d94a890abe 🐟

    <details><summary>Show signature</summary>

    Signature:

    untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 584f0cbf7932b4e5cf629c90fc7453d94a890abe 🐟
XqTEdhrBKv72tnR50ZH6e0yvoufXGrXV0f6BwBhThKf5e0o94RDxj7MrO+vdMdDkqmhoTaXnaAGVu4ZLNYWJDA==

    </details>

  32. DrahtBot requested review from hodlinator on Dec 16, 2025
  33. DrahtBot requested review from l0rinc on Dec 16, 2025
  34. in test/functional/interface_rest.py:492 in 584f0cbf79 
     489 |              get_block_part(status=400, query_params={"offset": len(block_bin), "size": 1})
 490 |              get_block_part(status=400, query_params={"offset": len(block_bin) + 1, "size": 1})
 491 |              get_block_part(status=400, query_params={"offset": 0, "size": len(block_bin) + 1})
 492 |  
 493 | -        self.test_rest_request(f"/blockpart/{blockhash}", status=400, req_type=ReqType.JSON, ret_type=RetType.OBJ)
 494 | +        res = self.test_rest_request(f"/blockpart/{blockhash}", status=400, req_type=ReqType.JSON, ret_type=RetType.OBJ)
    l0rinc commented at 10:54 PM on December 16, 2025:

    I think this was supposed to be a BIN to explain it's not a json failure

            res = self.test_rest_request(f"/blockpart/{blockhash}", status=400, req_type=ReqType.BIN, ret_type=RetType.OBJ)
    romanz commented at 11:15 PM on December 16, 2025:

    Updated 7fe94a0493

  35. DrahtBot requested review from l0rinc on Dec 16, 2025
  36. rest: add a test for unsuported `/blockpart/` request type 7fe94a0493
  37. rest: print also HTTP response reason in case of an error 59b93f11e8
  38. romanz force-pushed on Dec 16, 2025
  39. DrahtBot added the label CI failed on Dec 16, 2025
  40. l0rinc commented at 11:30 PM on December 16, 2025: contributor

    ACK 59b93f11e8600d5224359f4d05619c0f56aef1e6

    Since last ack the reason was added for status failure and non-json test was added for missing offset

  41. DrahtBot removed the label CI failed on Dec 17, 2025
  42. hodlinator approved
  43. hodlinator commented at 9:31 AM on December 17, 2025: contributor

    re-ACK 59b93f11e8600d5224359f4d05619c0f56aef1e6

    Tested modifying the expected status codes in the functional test a bit to see how the reason string is printed.

  44. fanquake merged this on Dec 17, 2025
  45. fanquake closed this on Dec 17, 2025
  46. romanz deleted the branch on Dec 17, 2025
  47. sedited referenced this in commit f8db928648 on Dec 27, 2025
  48. joshdoman referenced this in commit 57661ceac9 on Dec 27, 2025
Contributors
romanzDrahtBotl0rincfanquakemaflckohodlinatorryanofsky


l0rincmaflcko

Linked (view graph)
#33657 rest: allow reading partial block data from storage
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-28 06:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me