rest: Further API and code cleanup with query parameters

stickies-v commented at 8:34 pm on July 30, 2022: contributor

Introduction

In RESTful APIs, path parameters (e.g. /some/unique/resource/) are typically used to represent resources, and query parameters (e.g. ?sort=asc) are used to control how these resources are being loaded through e.g. sorting, pagination, filtering, …

The design of the current REST API endpoints is quite non-standard in a few ways, I’ve listed some examples below under “Undesirable behaviour examples”. Generally, it relies on a number of path (or path-like) parameters that really should be query parameters. Besides being unintuitive for users, I also found the code can be much cleaner when using a generalized interface for query and path parameters. The first work towards this was already done in #24098, but there are still a number of issues outstanding, as e.g. discussed here and here.

Suggested changes

I suggest (and have already prepared the code) to clean up the interface and the code. Besides perhaps the -deprecatedrest startup flag, no new user-facing functionality is introduced. Specifically, the relevant proposed changes are:

introduce a -deprecatedrest startup option to allow users to preserve REST endpoint functionality while they upgrade their dependencies, similar to -deprecatedrpc
expand the HTTPRequest interface to include easy-to-read functions to access path parameters, like #24098 introduced for query parameters: HTTPRequest::GetPath() and HTTPRequest::GetPathParameter(). This allows cleaning up the interface quite a bit, including removing the duplicated strURIpart that was passed as a third argument to each endpoint (but this is unintuitive and duplicated: the path data is already in the HTTPRequest object).
further streamline the endpoint URIs:
- in all endpoints: remove the path-like dot-separated format strings (e.g. .json) in favour of a query parameter (?format=json), which can default to json (especially useful in endpoints that only accept json)
- use ?from_blockhash=<blockhash> query parameter instead of a path parameter in /rest/headers and /rest/blockfilterheaders/, since it is a collection endpoint (multiple resources are returned)
- deprecate /rest/block/notxdetails in favour of a ?txdetails=<true|false> parameter in /rest/block

The suggested implementation keeps everything backwards compatible (with the -deprecatedrest functions enabled). Even though it means there is a bit more review overhead, I think the tradeoff is acceptable - I did my best to minimize code complexity due to this compatibility.

PRs

The suggested changes can largely be separated into 4 small-ish PRs (there are some dependencies, so they build sequentially - but shouldn’t be too hard to rebase/reorder/leave things out). I have released the PRs as draft already, just to make it easier to look at the proposed changes.

Discussion questions/notes

I’ve already written most of the code, but before fleshing out the PRs I’m first looking to get some feedback on the general concept and approach.

A couple of notes/things to discuss:

the LoC change is quite significant, is this a wortwhile upgrade?
should we maintain backwards compatibility with -deprecatedrest, not have backwards compatibility at all, or hide the backwards compatibility from the user?
-deprecatedrest is documented to, like the RPC, operate on a per-endpoint basis. However, since the first PR (format string) affects all endpoints, I thought that would not be user-friendly and instead opted to group them by functional change. Going forward, since we now have a rather extensible interface with the query parameters, I expect we’ll use -deprecatedrest on a per-endpoint basis indeed.

Undesirable behaviour examples

Require the user to explicitly define parameters where only 1 value is allowed anyway. For example, rest/chaininfo/ only accepts json as a format but throws if we do not explicitly define json to be the format
duplicate deploymentinto endpoints required
Unnecessary code coupling in ParseDataFormat() where we need one function to get the entire query path and then magically remove the format string (e.g. .json) from it and return it separately.
Two different endpoints to support the notxdetails option in /rest/block, this could just be one endpoint with an optional default query parameter
APIs can now be upgraded in a backwards-compatible way more easily, since query parameters are named and thus do not rely on position and can be optional

fanquake added the label RPC/REST/ZMQ on Jul 30, 2022

fjahr commented at 4:45 pm on August 7, 2022: contributor

My impression from looking at the REST interface the last time was also that it’s not standard in several ways. But it also seemed unclear who is actually using the REST interface if I remember correctly. That made it hard to decide both if anyone is facing issues from the changes and even if it’s actually worth making improvements at all.

Is there a list of projects that are using the REST interface? I think that would be very helpful anyway, in order to collect targeted feedback from the actual users that these changes will affect.

stickies-v commented at 3:45 pm on August 8, 2022: contributor

and even if it’s actually worth making improvements at all.

I think there’s also a bit of a chicken-and-egg problem here: since the REST API is limited in functionality and not very easy-to-use, it will have little adoption, and because it has little adoption, it’s not worked on much.

Is there a list of projects that are using the REST interface? I think that would be very helpful anyway, in order to collect targeted feedback from the actual users that these changes will affect.

I agree, that would be useful to have. I have not seen such a list, and don’t know about any big projects that currently use it. If I find anything, I’ll keep track of it.

0xB10C commented at 12:38 pm on August 9, 2022: contributor

Is there a list of projects that are using the REST interface? I think that would be very helpful anyway, in order to collect targeted feedback from the actual users that these changes will affect.

I use the REST interface in two of my projects:

My mempool-observer queries /rest/mempool/contents.json as I’ve found it to be faster than thegetrawmempool RPC, especially significant when my mempool is full. Also, using the REST interface instead of the RPC interface, I don’t have to bake in any user/pass configuration. Also, no dependency on JSON RPC library. Using e.g. a Python/other RPC->REST proxy as discussed in #23259 would work, but also means an additional service that can break.
A WIP project of mine similar to https://forkmonitor.info uses the getchaintips RPC (I think this information could also be exposed via REST) and queries headers both via getblockheader and the old /rest/headers/<COUNT>/<BLOCK-HASH>.bin REST interface. I need to query many headers, so I prefer getting 2000 headers via REST than headers one-by-one via the getblockheader RPC (maybe getting a range of headers should be added to the RPC interface). I currently use the old, deprecated REST interface as I want to support older Bitcoin Core versions (similar to forkmonitor.info I want to query e.g. Bitcoin Core version v0.10 and v23 to compare their chain tips). Going forward, I think I will have to keep functionality for the deprecated interface and add the new interface.

Happy to provide more usage feedback - feel free to ask.

I think, if we keep the REST interface it’s worth modernizing it. The proposed changes in your first three PRs seem reasonable. Having a -deprecatedrest option for a while (one or two major releases) makes sense, but the old endpoints should be dropped eventually.

JeremyRubin commented at 1:18 am on August 10, 2022: contributor

why not just /rest/1/ or something to denote request versions? and then we can eventually drop the “implicit v0” query version / only turn it on if -deprecatedrest=0 is set? (parsed as comma separated list of deprecated rest versions)?

This way a node can serve both upgraded and unupgraded applications (e.g., suppose you run @0xB10C’s services that upgrade AND some lightning thing @TheBlueMatt uses rest for, then both can work)

stickies-v commented at 1:26 pm on August 11, 2022: contributor

Thanks for the feedback and use case 0xB10C, that’s helpful to know. I think extending the RPC to support fetching multiple blockheaders would make sense if that otherwise prevents the REST API from being updated.

why not just /rest/1/ or something to denote request versions?

I initially implemented it that way and it would be my preferred approach, but since it (admittedly not by a crazy amount) increased the amount of code to review for (what I assumed to be) no substantial benefit in this particular case so I scrapped it. If indeed we want one bitcoind to be able to serve both v0 and v1 endpoints then that would be the way to go. But, practically, I’m not sure there’s real demand for that? E.g. in your example if just one of both (external) services supports both versions, then your REST server can just run that shared version. This seems to be the case in this example since @0xB10C plans to add support for both interfaces. It would generally be a better design though, so I’ll look into it again - thanks for the feedback.

brunoerg commented at 9:42 pm on August 16, 2022: contributor

I use the REST Interface and I agree it should be improved.

in all endpoints: remove the path-like dot-separated format strings (e.g. .json) in favour of a query parameter (?format=json), which can default to json (especially useful in endpoints that only accept json)

I agree 1000%, format is definitely something we can use with query.

deprecate /rest/block/notxdetails in favour of a ?txdetails=<true|false> parameter in /rest/block

Maybe +0. I don’t think it’s a bad practice having /notxdetails in this case. But ok if we decide to change it.

the LoC change is quite significant, is this a wortwhile upgrade? Specially about the path-like dot-separated format strings, I think so. I’d like to see the REST interface more friendly and robust, and more used.

Another improvement I’ve thought about REST Interface, maybe we could add authentication/authorization and could work with more sensitive endpoints. I couldn’t find why we don’t have auth yet (no one worked on that or is there any limitation?).

kouloumos commented at 6:24 pm on August 18, 2022: contributor

Although #7759 was about streaming the entire UTXO set, chunking it was further discussed in #14584. Is pagination/chunking something that could become easier through the proposed changes? Maybe by supporting a skip query parameter?

stickies-v commented at 11:27 am on March 13, 2023: contributor

Closing this (and draft PRs). I still think the suggested changes are an improvement, and that the state of our current interfaces is an impediment to (new) developers building on Bitcoin Core instead of using third-party services, but as evidenced by my lack of activity on this issue I don’t feel anymore like this is currently a particularly worthwhile endeavour given current development resources and other active projects that require developer attention.

Moreover, I think I’m now leaning more towards Bitcoin Core having a single fully functional, fully standard, performant interface (whether that’s RPC or REST can be debated) to reduce maintenance burden. Optionally, light-weight wrapper services (e.g. #23259) can be provided to make it easier for developers to build on Core with technologies they’re familiar with, but this need not necessarily be a Core project.

Thank you everyone for your input on this issue, my apologies for not making better use of your time here.

Is pagination/chunking something that could become easier through the proposed changes?

Each endpoint has to implement pagination because it’s resource specific, but adding a skip parameter is trivial since #24098. Typically paginated REST responses include some headers such as X-Page, X-Total-Pages, … for which additional helper utilities could be added to avoid code duplication - those are not included in any of the PRs in this issue.

stickies-v closed this on Mar 13, 2023

achow101 referenced this in commit ebb15ea75a on Mar 15, 2023

sidhujag referenced this in commit 0dea134c10 on Mar 16, 2023

sidhujag referenced this in commit 595099b00b on Mar 16, 2023

bitcoin locked this on Mar 12, 2024

rest: Further API and code cleanup with query parameters #25752

Introduction

Suggested changes

PRs

Discussion questions/notes

Undesirable behaviour examples