Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose block filters over REST #17631

Merged
merged 2 commits into from Dec 20, 2021

Conversation

TheBlueMatt
Copy link
Contributor

@TheBlueMatt TheBlueMatt commented Nov 29, 2019

This adds a new rest endpoint:
/rest/blockfilter/filtertype/requesttype/blockhash (eg
/rest/blockfilter/basic/header/000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f.hex)
which exposes either the filter "header" or the filter data itself.
Most of the code is cribbed from the equivalent RPC.

You can test it at http://bitcoin-rest.bitcoin.ninja/rest//blockfilter/basic/header/000000005b7a58a939b2636f61fa4ddd62258c5fed57667a35d23f2334c4f86d.hex

@promag
Copy link
Member

promag commented Nov 29, 2019

A test would be nice.

@practicalswift
Copy link
Contributor

practicalswift commented Nov 29, 2019

After reading doc/REST-interface.md I'm not entirely clear about the assumed trust boundaries.

What recommendations do we give to our users regarding exposing the REST endpoints publicly? Do the recommendations differ from our recommendations with regards to exposing the JSON-RPC endpoints publicly?

As I've understood it we regard the JSON-RPC interface as as an internal control plane only to be accessible by trusted clients. The assumption we're making from a trust boundary perspective seems to be that we assume that an untrusted clients will never be able to connect to the port serving the JSON-RPC interface (which is the same port as the REST interface).

@laanwj
Copy link
Member

laanwj commented Nov 29, 2019

Concept ACK.

After reading doc/REST-interface.md I'm not entirely clear about the assumed trust boundaries.

That's a fair question (FWIW the limit has always been: only public data, no complex queries, do not parse JSON as input), but I'd suggest opening a new issue for it. Please keep this one for review of the code changes.

@practicalswift
Copy link
Contributor

practicalswift commented Nov 29, 2019

@laanwj Without knowing if consumers are trusted or not it is pretty hard to review it from a security perspective :)

@laanwj
Copy link
Member

laanwj commented Nov 29, 2019

The REST interface is a lightweight interface for querying public data. Consumers are trusted but less so than on RPC (as they don't authenticate). I still wouldn't recommend exposing it directly to the internet. But maybe it's OK to open it "publicly" inside some LAN or VPN that your applications run in.

This is my last general comment on this, please open a new issue if you want to continue this discussion.

doc/REST-interface.md

Speaking of which, please update the documentation to mention this new call.

src/rest.cpp Outdated Show resolved Hide resolved
@jnewbery
Copy link
Member

jnewbery commented Nov 29, 2019

Concept ACK

@TheBlueMatt
Copy link
Contributor Author

TheBlueMatt commented Nov 29, 2019

Added a basic sanity test, redid the way headers work to make it easy to get many of them just like the /headers/ request.

src/rest.cpp Outdated Show resolved Hide resolved
src/rest.cpp Outdated Show resolved Hide resolved
src/rest.cpp Outdated Show resolved Hide resolved
}

BlockFilterIndex* index = GetBlockFilterIndex(filtertype);
if (!index) {
Copy link

@paymog paymog Dec 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the code for extracting filtertype, index and blockhash is shared between these two functions. Should this extraction code be pulled out into their own functions?

Copy link
Contributor Author

@TheBlueMatt TheBlueMatt Dec 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets leave DRY'ing up rest.cpp for a separate commit. I played with it a bit and there isn't an obvious solution here that doesn't end up adding more lines, but the whole of REST probably could get DRY'd up a lot especially in the results-providing section.

src/rest.cpp Outdated
LOCK(cs_main);
block_index = LookupBlockIndex(block_hash);
if (!block_index) {
return RESTERR(req, HTTP_NOT_FOUND, uriParts[1] + " not found");
Copy link

@paymog paymog Dec 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: can the error message be changed to "Block " + uriParts[1] + " not found"?

Copy link
Contributor Author

@TheBlueMatt TheBlueMatt Dec 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, its coped from the block code, so to keep it the same everywhere I'll leave it.

@jonasschnelli
Copy link
Contributor

jonasschnelli commented Dec 1, 2019

Concept ACK

@TheBlueMatt TheBlueMatt force-pushed the 2019-11-filter-rest branch 2 times, most recently from 8c33533 to 3ab6abc Compare Dec 3, 2019
@DrahtBot
Copy link
Contributor

DrahtBot commented Dec 5, 2019

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

Conflicts

No conflicts as of last run.

src/rest.cpp Outdated Show resolved Hide resolved
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jan 5, 2020
This adds a new rest endpoint:
/rest/blockfilter/filtertype/requesttype/blockhash (eg
/rest/blockfilter/basic/header/000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f.hex)
which exposes either the filter "header" or the filter data itself.
Most of the code is cribbed from the equivalent RPC.

Github-Pull: bitcoin#17631
Rebased-From: aeca9ab
@TheBlueMatt
Copy link
Contributor Author

TheBlueMatt commented May 1, 2020

Rebased.

@TheBlueMatt TheBlueMatt force-pushed the 2019-11-filter-rest branch 2 times, most recently from b555be4 to dc3a7e8 Compare May 1, 2020
@TheBlueMatt TheBlueMatt force-pushed the 2019-11-filter-rest branch 3 times, most recently from 9ff0076 to 16d8d2d Compare May 2, 2020
Copy link
Member

@jnewbery jnewbery left a comment

Couple of nits. Otherwise looks good.

Commit order is slightly confused. It adds code in one commit then changes it in later commits. I'd suggest putting the fixes first, and then adding the new block filter code in the final commit.

src/rest.cpp Outdated Show resolved Hide resolved
src/rest.cpp Outdated Show resolved Hide resolved
@naumenkogs
Copy link
Member

naumenkogs commented May 18, 2020

Code review ACK 16d8d2d.
Perhaps would be good to address the style suggestion above.

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 9, 2020
This adds a new rest endpoint:
/rest/blockfilter/filtertype/requesttype/blockhash (eg
/rest/blockfilter/basic/header/000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f.hex)
which exposes either the filter "header" or the filter data itself.
Most of the code is cribbed from the equivalent RPC.

Github-Pull: bitcoin#17631
Rebased-From: aeca9ab
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 9, 2020
@MarcoFalke
Copy link
Member

MarcoFalke commented Dec 20, 2021

re-ack

This feature was roughly ready when the pull was opened more than two years ago. It's been held back on mostly non-behaviour changing "nits", which still haven't been addressed yet. I don't think they'll be addressed any time soon, so it might be better to fix them in a follow-up at this point. Going to merge to move this forward.

@MarcoFalke MarcoFalke merged commit 70d6a09 into bitcoin:master Dec 20, 2021
15 checks passed
@jnewbery
Copy link
Member

jnewbery commented Dec 20, 2021

It's been held back on mostly non-behaviour changing "nits", which still haven't been addressed yet. I don't think they'll be addressed any time soon, so it might be better to fix them in a follow-up at this point.

@MarcoFalke I'm sure this wasn't your intention, but it seems very inconsistent that for most contributors PRs will be closed if review comments haven't been addressed, but in this case the PR was merged despite review comments not being addressed for many months. It shouldn't be the case that certain contributors can get their PRs merged even if they don't follow the project style guide or respond to review comments.

I also disagree that #17631 (comment) is a "nit". It's feedback on the interface design and changing it later would be an API break.

@MarcoFalke
Copy link
Member

MarcoFalke commented Dec 20, 2021

It shouldn't be the case that certain contributors can get their PRs merged even if they don't follow the project style guide or respond to review comments.

Style-only comments are not mandatory to address. This has always been the case, unless the style issue is so "severe" that a linter enforces it. If there are any style issues here that I missed and should be blocking, someone should add a linter to enforce them. Though, I couldn't find any.

I also disagree that #17631 (comment) is a "nit". It's feedback on the interface design and changing it later would be an API break.

Apologies, my bad. I wasn't aware there is a suggestion that requests to change the behaviour.

@jnewbery
Copy link
Member

jnewbery commented Dec 20, 2021

Style-only comments are not mandatory to address. This has always been the case, unless the style issue is so "severe" that a linter enforces it. If there are any style issues here that I missed and should be blocking, someone should add a linter to enforce them. Though, I couldn't find any.

I disagree - all review comments should be addressed, even if that only means responding with a comment "I don't intend to change this because ...". If we don't have a culture where all review comments are responded to, then it falls to the maintainers to make the decision about whether the ignored review comments are important or not, and sometimes important review comments fall through the gaps.

@MarcoFalke MarcoFalke added this to the 23.0 milestone Dec 20, 2021
@MarcoFalke
Copy link
Member

MarcoFalke commented Dec 20, 2021

Ok, again apologies for missing the comment(s). I've marked this "Up for grabs", so that someone can address the feedback before the next major release (23.0). If no one does, I'll probably revert the changes.

@JeremyRubin
Copy link
Contributor

JeremyRubin commented Dec 20, 2021

It also feels premature given my nack + alternative path to implementing this in #23309 not having been reviewed -- were it just my NACK and no alternate, I'd agree that perhpas it was sufficiently addressed, but it was my NACK + Alternative + Acks on that approach from > 1 contributor.

@MarcoFalke
Copy link
Member

MarcoFalke commented Dec 20, 2021

I didn't see any NACK.

Screenshot 2021-12-20 at 17-50-57 Expose block filters over REST by TheBlueMatt · Pull Request #17631 · bitcoin bitcoin

I have no idea how to deal with this, but maybe it could make sense to re-NACK a pull request whenever there is a change (push)? Or maybe use the "GitHub review" and click the red cross, which might not be hidden?

@MarcoFalke
Copy link
Member

MarcoFalke commented Dec 20, 2021

And to comment on the content on the NACK: I don't see how this and #23309 are mutually exclusive.

@luke-jr
Copy link
Member

luke-jr commented Dec 20, 2021

I agree with Marco that this has been basically ready for years, even if it might be imperfect. It's not too late for further improvements in followups, but blocking this on it was letting the perfect be the enemy of the good IMO.

That being said, it does seem to be a change in merge policy (IMO for the better), and should probably be discussed with the project and applied consistently.

@JeremyRubin
Copy link
Contributor

JeremyRubin commented Dec 20, 2021

@MarcoFalke well it's a Concept Nack, so it doesn't get stale with changes (as opposed to an Approach Nack + Concept Ack).

W.r.t mutually exclusive, the contention that I was holding (and @practicalswift too I think?) is that a REST API exposed for unauthenticated users without a strong statement of the security model is probably something we shouldn't be maintaining or increasing dependency on for core, and a preference for doing it as a layer on top of a more strongly audited and maintained interface (JSON RPC) being superior. So extending the REST API and increasing dependency on it for consumers of Core is counter to the goal of removing it and replacing with 'userland' proxies.

@sipa
Copy link
Member

sipa commented Dec 20, 2021

@JeremyRubin I think the discussion whether to provide a wrapper, and then later whether or not to deprecate/remove the existing REST interface is mostly independent from the question of what features should be exposed by the REST interface (whether that's provided by bitcoind directly or by a wrapper). Whatever risks exists by having a REST interface in the first place aren't worsened by adding another method.

I do agree the merging procedure followed here was dubious, though.

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 20, 2021
2b64fa3 Update REST docs with new accessors (Matt Corallo)
ef7c822 Expose block filters over REST. (Matt Corallo)

Pull request description:

  This adds a new rest endpoint:
  /rest/blockfilter/filtertype/requesttype/blockhash (eg
  /rest/blockfilter/basic/header/000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f.hex)
  which exposes either the filter "header" or the filter data itself.
  Most of the code is cribbed from the equivalent RPC.

  You can test it at http://bitcoin-rest.bitcoin.ninja/rest//blockfilter/basic/header/000000005b7a58a939b2636f61fa4ddd62258c5fed57667a35d23f2334c4f86d.hex

ACKs for top commit:
  dergoegge:
    ACK 2b64fa3 - Adding blockfilters to the REST interface is analogous to serving other public data such as transactions or blocks.

Tree-SHA512: d487bc694266375c94d6fcf2e9d788a8a42a3b94e8d3290e46335a64cbcde55084ce5ea6119b79a4065888d94d7c3ae25a59a901fa46e3711f0eb296add12696
@MarcoFalke
Copy link
Member

MarcoFalke commented Dec 21, 2021

I do agree the merging procedure followed here was dubious, though.

Ok, maybe I am being blind, but can someone explain to me what was wrong about the merging procedure?

Did this have too little review?

I'd say no. There were:

If it is not possible to merge a pull request with 8 ACKs, then I honestly don't know how much review is needed to get anything merged. I expect that 99.8% of all pull request have less than 8 ACKs.

Was this merged before a comment should have been replied to?

Yes, I missed the comment/question about the interface, and it should have been replied to before merge. Luckily it has been replied to after merge (and in my view resolved). The only unaddressed stuff now is refactoring nit comments.

I don't think the merging procedure changed here. I still think that comments should be replied to before merge. However, I also think that style comments that haven't been replied to should not block a pull request indefinitely. If the overall direction of a pull request has received overall support (which this pull request did), then I think style-nits can be dropped or fixed in a follow-up. If the overall direction of a pull request is not agreed upon, or if the style-nits are rendering a follow-up to be a re-implementation, then I think it should not be merged (not applicable to this pull request).

Something else?

Again, I am not seeing anything else wrong, so please elaborate.

@jnewbery
Copy link
Member

jnewbery commented Dec 21, 2021

can someone explain to me what was wrong about the merging procedure?

Was this merged before a comment should have been replied to?

Yes

@MarcoFalke - this is all I'm claiming - that review comments should be acknowledged and responded to before merge. The author doesn't have to agree with them or implement them, but if someone has taken the time to review the PR, the author should respond to that review before the PR is merged. That happens in 99% of PRs, and I don't see why this one should have been different.

For what it's worth, I agree with @stickies-v that we should make the REST API RESTful. I don't feel very strongly about it because I don't use the REST API myself, but I think the discussion should be had before merge.

And to be clear, I think you do a fantastic job as a maintainer. In this instance you missed something, but it should never have been your responsibility to assess whether that comment should have been ignored. It's the author's responsibility to address all feedback on their PR.

@stickies-v
Copy link
Contributor

stickies-v commented Dec 21, 2021

FWIW - I've marked my RESTful comment as resolved, since @dergoegge pointed out this is consistent with another endpoint. As such, a follow-up PR is probably the more elegant way to go. I'm also happy to incorporate the other unaddressed style change comments on #17631 into this new PR.

I appreciate everyone's efforts in both doing the merging and keeping the process honest. I agree that requiring an author's explicit acknowledgement (or dismissal) of all (non-spam) comments would, in my view, be preferable.

@MarcoFalke MarcoFalke removed this from the 23.0 milestone Dec 22, 2021
@MarcoFalke
Copy link
Member

MarcoFalke commented Dec 22, 2021

Looks like there is a follow up in #23836, so I've removed the "up for grabs" label again.

fanquake added a commit to bitcoin-core/gui that referenced this pull request Jan 2, 2022
4523d28 [test] compare filter and header with the result of the getblockfilter RPC (Niklas Gögge)
3a2464f [rest] drop superfluous rpc serializations flags for block filters (Niklas Gögge)
064abd1 [rest] add a more verbose error message for invalid header counts (Niklas Gögge)
83b8f3a [refactor] various style fix-ups (Niklas Gögge)

Pull request description:

  This PR addresses unresolved review comments from [#17631](bitcoin/bitcoin#17631).
  This includes:
  * various style fix-ups
  * returning a more verbose error message for invalid header counts
  * removing superfluous rpc serializations flags for block filters
  * improving the test to include comparing the block filters returned from the rest  with the ones returned from the `getblockfilter` RPC.

ACKs for top commit:
  jnewbery:
    ACK 4523d28
  brunoerg:
    tACK 4523d28

Tree-SHA512: 634e6b2ae3e1d5f31675a50cfe11a5e74bf5a51b9e7b512d9e18879bf2ed424fc0ac6ec633023f192e3ad12cf0c73b0b51de79dd7ec00844dba3e1493d823b8c
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 2, 2022
4523d28 [test] compare filter and header with the result of the getblockfilter RPC (Niklas Gögge)
3a2464f [rest] drop superfluous rpc serializations flags for block filters (Niklas Gögge)
064abd1 [rest] add a more verbose error message for invalid header counts (Niklas Gögge)
83b8f3a [refactor] various style fix-ups (Niklas Gögge)

Pull request description:

  This PR addresses unresolved review comments from [bitcoin#17631](bitcoin#17631).
  This includes:
  * various style fix-ups
  * returning a more verbose error message for invalid header counts
  * removing superfluous rpc serializations flags for block filters
  * improving the test to include comparing the block filters returned from the rest  with the ones returned from the `getblockfilter` RPC.

ACKs for top commit:
  jnewbery:
    ACK 4523d28
  brunoerg:
    tACK 4523d28

Tree-SHA512: 634e6b2ae3e1d5f31675a50cfe11a5e74bf5a51b9e7b512d9e18879bf2ed424fc0ac6ec633023f192e3ad12cf0c73b0b51de79dd7ec00844dba3e1493d823b8c
@stickies-v
Copy link
Contributor

stickies-v commented Jan 18, 2022

#24098 is now ready for review to address the last unaddressed comments (by myself) on this PR.

MarcoFalke added a commit that referenced this pull request Apr 6, 2022
54b39cf Add release notes (stickies-v)
f959fc0 Update /<count>/ endpoints to use a '?count=' query parameter instead (stickies-v)
a094976 Add GetQueryParameter helper function (stickies-v)
fff771e Handle query string when parsing data format (stickies-v)
c1aad1b scripted-diff: rename RetFormat to RESTResponseFormat (stickies-v)
9f1c547 Refactoring: move declarations to rest.h (stickies-v)

Pull request description:

  In RESTful APIs, [typically](https://rapidapi.com/blog/api-glossary/parameters/query/) path parameters  (e.g. `/some/unique/resource/`) are 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, ...

  As first [discussed in #17631](#17631 (comment)), the [current REST api](https://github.com/bitcoin/bitcoin/blob/master/doc/REST-interface.md) contains two endpoints `/headers/` and `/blockfilterheaders/` that rather unexpectedly use path parameters to control how many (filter) headers are returned in the response. While this is no critical issue, it is unintuitive and we are still early enough to easily phase this behaviour out and ensure new endpoints (if any) do not have to stick to non-standard behaviour just for internal consistency.

  In this PR, a new `HTTPRequest::GetQueryParameter` method is introduced to easily parse query parameters, as well as two new `/headers/` and `/blockfilterheaders/` endpoints that use a count query parameter are introduced. The old path parameter-based endpoints are kept without too much overhead, but the documentation now points to the new query parameter-based endpoints as the default interface to encourage standardness.

  ## Behaviour change
  ### New endpoints and default values
  `/headers/` and `/blockfilterheaders/` now have 2 new endpoints that contain query parameters (`?count=<count>`) instead of path parameters (`/<count>/`), as described in REST-interface.md. Since query parameters can easily have default values, I have set this at 5 for both endpoints.

  **headers**
  `GET /rest/headers/<BLOCK-HASH>.<bin|hex|json>?count=<COUNT=5>`
  should now be used instead of
  `GET /rest/headers/<COUNT>/<BLOCK-HASH>.<bin|hex|json>`

  **blockfilterheaders**
  `GET /rest/blockfilterheaders/<FILTERTYPE>/<BLOCK-HASH>.<bin|hex|json>?count=<COUNT=5>`
  should now be used instead of
  `GET /rest/blockfilterheaders/<FILTERTYPE>/<COUNT>/<BLOCK-HASH>.<bin|hex|json>`

  ### Some previously invalid API calls are now valid
  API calls that contained query strings in the URI could not be parsed prior to this PR. This PR changes behaviour in that previously invalid calls (e.g. `GET /rest/headers/5/somehash.json?someunusedparam=foo`) would now become valid, as the query parameters are properly parsed, and discarded if unused.
  For example, prior to this PR, adding an irrelevant `someparam` parameter would be illegal:
  ```
  GET /rest/headers/5/0000004c6aad0c89c1c060e8e116dcd849e0554935cd78ff9c6a398abeac6eda.json?someparam=true
  ->
  Invalid hash: 0000004c6aad0c89c1c060e8e116dcd849e0554935cd78ff9c6a398abeac6eda.json?someparam=true
  ```
  **This behaviour change affects all rest endpoints, not just the 2 new ones introduced here.**

  *(Note: I'd be open to implementing additional logic to refuse requests containing unrecognized query parameters to minimize behaviour change, but for the endpoints that we currently have I don't really see the point for that added complexity. E.g. I don't see any scenarios where misspelling a parameter could lead to harmful outcomes)*

  ## Using the REST API

  To run the API HTTP server, start a bitcoind instance with the `-rest` flag enabled. To use the
  `blockfilterheaders` endpoint, you'll also need to set `-blockfilterindex=1`:
  ```
  ./bitcoind -signet -rest -blockfilterindex=1
  ```

  As soon as bitcoind is fully up and running, you should be able to query the API, for example by
  using curl on the command line: ```curl "127.0.0.1:38332/rest/chaininfo.json"```.
  To more easily parse the JSON output, you can also use tools like 'jq' or `json_pp`, e.g.:
  ```
  curl -s "localhost:38332/rest/blockfilterheaders/basic/0000004c6aad0c89c1c060e8e116dcd849e0554935cd78ff9c6a398abeac6eda.json?count=2" | json_pp .
  ```

  ## To do
  - [x] update `doc/release-notes`

  ## Feedback
  This is my first PR (hooray!). Please don't hold back on any feedback/comments/nits/... you may have, big or small, whether they are code, process, language, ... related. I welcome private messages too if there's anything you don't want to clutter the PR with. I'm here to learn and am grateful for everyone's input.

ACKs for top commit:
  stickies-v:
    I've had to push a tiny doc update to `REST-interface.md` (`git range-diff 219d728 9aac438 54b39cf`) since this was not merged for v23, but since there are no significant changes beyond theStack and jnewbery's ACKs I think this PR is now ready to be considered for merging? @MarcoFalke
  jnewbery:
    ACK 54b39cf
  theStack:
    re-ACK 54b39cf

Tree-SHA512: 3b393ffde34f25605ca12c0b1300799a19684b816a1d03aed38b0f5439df47bfe6a589ffbcd7b83fd2def6c9d00a1bae5e45b1d18df4ae998c617c709990f83f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment