rest: Use from_blockhash and txdetails query parameters

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

Draft, for reference only to support #25752 for discussion on concept and approach. Code and tests should be fully functional, but still to be considered rough. Commits specific to this PR start from rest: use from_blockhash query parameter.

Brief summary

from_blockhash

/rest/headers and /rest/blockfilterheaders are collection endpoints, i.e. they return an array of objects. The was used not reference that specific resource, but to return all objects since that block hash. Filtering parameters like this are normally provided via query parameters, for reasons such as being composable (multiple filters can be combined) and supporting default values.

As such, this PR removes removes the <BLOCK-HASH> from the path and inserts the from_blockhash query parameter for /rest/headers and /rest/blockfilterheaders. Behaviour can be reverted by using -deprecatedrest=blockhash.

txdetails

In /rest/block/notxdetails/, notxdetails is not a resource - it controls how much information we want to have included in the response. As above, this PR converts the /notxdetails path parameter into a ?txdetails=<true|false> query parameter. Behaviour can be reverted by using -deprecatedrest=txdetails.

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

DrahtBot added the label Needs rebase on Jul 30, 2022

stickies-v force-pushed on Jul 30, 2022

DrahtBot removed the label Needs rebase on Jul 30, 2022

DrahtBot commented at 4:59 am on July 31, 2022: contributor

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

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25760 (rest: clean-up for mempool endpoints by brunoerg)
#25722 (refactor: Use util::Result class for wallet loading by ryanofsky)
#25721 (refactor: Replace BResult with util::Result by ryanofsky)
#25665 (refactor: Add util::Result class and use it in LoadChainstate by ryanofsky)
#25412 (rest: add /deploymentinfo endpoint by brunoerg)

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.

DrahtBot added the label Needs rebase on Aug 5, 2022

DrahtBot commented at 1:54 pm on August 5, 2022: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

refactor: separate format string parsing from ParseDataFormat

In future commits, we'll need to directly convert a format string
into RESTResponseFormat without all the other logic from ParseDataFormat

acb64e9f59

add -deprecatedrest startup option

Equivalent of -deprecatedrpc for the REST API. Allows the user to
keep the specified deprecated functionality unchanged

fee8638274

rest: move format string to query

Instead of specifying the response format as an e.g. ".json" path
parameter, the format string is now expected to be in the query
string "?format=json". This allows for a more standardized URI
format, and removes the dependency on too tightly coupled code like
ParseDataFormat().

Can be overridden with -deprecatedrest=format, which enables automatic
parsing of the URI to move format strings from the path to the query.

206a18452a

refactor: remove unused ParseDataFormat

No longer necessary since response format is a query parameter

901b1323b6

rest: store endpoint prefix data in HTTPRequest

By storing the endpoint prefix (e.g. "/rest/headers/") in the HTTPRequest,
we can more easily extract the (relative) path from a URI in subsequent
commits. Currently, this is done in http_request_cb().

c71a1cff43

rest: add GetPath() and GetPathParameter()

Utility functions to easily get the query path. GetPath() returns
the entire path as a vector of strings, and GetPathParameter()
allows to query for individual path parameters with a similar
interface as GetQueryParameter().

1bca36cee5

rest: use GetPath()

For readability, also standardize variable name into 'path'

a09781f662

rest: add explicit deprecation path for count

Unless -deprecatedrest=count is enabled, all count parameters are
now expected to be a query instead of path parameter.

aaf758420e

rest: remove dependency on strURIpart in handler

strURIpart was generally used to access path data, even though this
data was already container inside the HTTPRequest. With can now use
the new GetPath() and GetPathParameter() interface.

Renamed some parameters in function signature to minimize LoC changes

9c7b3035b4

rest: remove path from handler signature

Since we now exclusively access the path through the HTTPRequest,
we can remove the unused strURIpart path argument to simplify the
rest endpoint interface.

23fb5ca8ae

rest: use from_blockhash query parameter

/rest/headers and /rest/blockfilterheaders are collection endpoints,
i.e. they return an array of objects. One of the filters they support
is to define the block hash from which to return objects. In REST APIs,
filter parameters are usually implemented as query parameters instead
of path parameters, for example so they can be more easily combined
with other filters.

7dc230ba3a

rest: make txdetails a query parameter

instead of path parameter; reduces complexity

00de819d92

stickies-v force-pushed on Aug 5, 2022

DrahtBot commented at 1:40 am on December 12, 2022: contributor

There hasn’t been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

DrahtBot commented at 1:17 am on March 12, 2023: contributor

There hasn’t been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

maflcko commented at 10:16 am on March 13, 2023: member

@stickies-v Looks like this was opened as draft a year ago with no further activity from you. Can this be closed?

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

Closing as per #25752 (comment)

stickies-v closed this on Mar 13, 2023

bitcoin locked this on Mar 12, 2024

rest: Use from_blockhash and txdetails query parameters #25755

Brief summary

from_blockhash

txdetails

Reviews

Conflicts