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

REST: add blockhash call, fetch blockhash by height #14353

Merged
merged 3 commits into from Jan 23, 2019

Conversation

Projects
None yet
10 participants
@jonasschnelli
Copy link
Member

jonasschnelli commented Sep 29, 2018

Completes the REST interface for trivial block exploring by adding a call that allows to fetch the blockhash in the main chain by a given height.

Show resolved Hide resolved src/rest.cpp Outdated
Show resolved Hide resolved test/functional/interface_rest.py Outdated
Show resolved Hide resolved test/functional/interface_rest.py Outdated
@jgarzik

This comment has been minimized.

Copy link
Contributor

jgarzik commented Sep 30, 2018

concept ACK

Show resolved Hide resolved src/rest.cpp Outdated

@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2018/09/rest_blockhash branch Sep 30, 2018

@jimmysong
Copy link
Contributor

jimmysong left a comment

Thanks @jonasschnelli !

Show resolved Hide resolved src/rest.cpp
@promag

This comment has been minimized.

Copy link
Member

promag commented Oct 1, 2018

Tested ACK ae0a5a1.

What is the use case? Query block hash by height and then query the block? If so could overload /rest/block instead to avoid 2nd round trip?

@jonasschnelli

This comment has been minimized.

Copy link
Member Author

jonasschnelli commented Oct 1, 2018

@promag: the use case is exactly that (find a block by giving a height) (ex: https://bitcointools.jonasschnelli.ch/explorer/height/500000).

You could build it into /rest/block, but IMO we should keep it modular. I guess that is also the reason why there is a "getblock" and a "getblockhash" call on RPC.

@gmaxwell

This comment has been minimized.

Copy link
Member

gmaxwell commented Oct 1, 2018

PR is mistitled, not a "blockheader" call. This PR doesn't really seem to indicate the benefit this provides to the user. Making multiple round trips also has bad performance, if the purpose really is just querying blocks by height this indirection seems like pretextual modularity.

@jonasschnelli jonasschnelli changed the title REST: add blockheader call, fetch blockhash by height REST: add blockhash call, fetch blockhash by height Oct 1, 2018

@jonasschnelli

This comment has been minimized.

Copy link
Member Author

jonasschnelli commented Oct 1, 2018

Changed the title (was wrong, thank @gmaxwell)

I'm happy to add a per-height accessing to /rest/block, but this would mean accessing that new per-height-fetching would always return a block (optionally reduced to only contain txids) which would always access the disk.

Of course, we could then add it to /rest/headers/ as well so one would fetch a single blockheader (or range of headers) per height.

I haven't made a study about use cases but the extra roundtrip made sense to me (costs for http mem only retrieval roundtrips are usually tiny).

@ch4ot1c

This comment has been minimized.

Copy link
Contributor

ch4ot1c commented Oct 2, 2018

Ack, seems marginally beneficial

Show resolved Hide resolved doc/REST-interface.md Outdated

jonasschnelli added a commit to jonasschnelli/dumb-block-explorer that referenced this pull request Oct 7, 2018

@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2018/09/rest_blockhash branch Oct 17, 2018

@promag

This comment has been minimized.

Copy link
Member

promag commented Oct 18, 2018

utACK, but fix first commit message.

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Nov 9, 2018

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #11770 ([REST] add a rest endpoint for estimatesmartfee, docs, and test by joemphilips)

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.

Show resolved Hide resolved src/rest.cpp Outdated
@luke-jr

This comment has been minimized.

Copy link
Member

luke-jr commented Dec 20, 2018

See also #11765

@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2018/09/rest_blockhash branch Jan 3, 2019

@jonasschnelli

This comment has been minimized.

Copy link
Member Author

jonasschnelli commented Jan 3, 2019

Fixed @luke-jr point (invalid JSON respons)
Renamed from blockhash to blockhashbyheight.

test/functional/interface_rest.py Outdated
@@ -198,7 +198,7 @@ def run_test(self):
self.nodes[0].generate(1) # generate block to not affect upcoming tests
self.sync_all()

self.log.info("Test the /block and /headers URIs")
self.log.info("Test the /block, /blockhash and /headers URIs")

This comment has been minimized.

@laanwj

laanwj Jan 4, 2019

Member

nit: this is /blockhashbyheight now

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Jan 4, 2019

utACK (-nit), I'm surprised this didn't exist yet.

doc/REST-interface.md Outdated
#### Blockhash by height
`GET /rest/blockhashbyheight/<HEIGHT>.<bin|hex|json>`

Given a height: returns blockhash of block in main chain at height `<HEIGHT>`.

This comment has been minimized.

@laanwj

laanwj Jan 4, 2019

Member

The help for RPC getblockhash is Returns hash of block in best-block-chain at height provided. I think that's slightly better terminology to use here than "main chain" which could be confused with testnet/mainnet/....

@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2018/09/rest_blockhash branch to d63921b Jan 4, 2019

@jonasschnelli

This comment has been minimized.

Copy link
Member Author

jonasschnelli commented Jan 4, 2019

Fixed documentation and the test comment inconsistency (reported by @laanwj)

@jnewbery
Copy link
Member

jnewbery left a comment

looks good. A few comments, mostly around better test coverage :)

Show resolved Hide resolved test/functional/interface_rest.py Outdated
Show resolved Hide resolved src/rest.cpp Outdated
Show resolved Hide resolved test/functional/interface_rest.py
Show resolved Hide resolved src/rest.cpp

@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2018/09/rest_blockhash branch from d63921b Jan 19, 2019

@jonasschnelli

This comment has been minimized.

Copy link
Member Author

jonasschnelli commented Jan 19, 2019

Fixed points reported by @jnewbery (thanks for the review).

@promag

This comment has been minimized.

Copy link
Member

promag commented Jan 20, 2019

@jonasschnelli commit messages should mention /rest/blockhashbyheight.

jonasschnelli added some commits Sep 29, 2018

@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2018/09/rest_blockhash branch to 42ff30e Jan 21, 2019

@jonasschnelli

This comment has been minimized.

Copy link
Member Author

jonasschnelli commented Jan 21, 2019

Fixed the commit message

@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented Jan 22, 2019

utACK 42ff30e

@promag

This comment has been minimized.

Copy link
Member

promag commented Jan 22, 2019

utACK 42ff30e.

@jonasschnelli jonasschnelli merged commit 42ff30e into bitcoin:master Jan 23, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

jonasschnelli added a commit that referenced this pull request Jan 23, 2019

Merge #14353: REST: add blockhash call, fetch blockhash by height
42ff30e [Docs] add short documentation for /rest/blockhashbyheight (Jonas Schnelli)
579d418 [QA] add rest tests for /rest/blockhashbyheight/<HEIGHT>.<FORMAT> (Jonas Schnelli)
eb9ef04 REST: add "blockhashbyheight" call, fetch blockhash by height (Jonas Schnelli)

Pull request description:

  Completes the REST interface for trivial block exploring by adding a call that allows to fetch the blockhash in the main chain by a given height.

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