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

[REST] JSON support for /rest/headers #5486

Merged
merged 1 commit into from Jul 10, 2015

Conversation

@jonasschnelli
Copy link
Member

jonasschnelli commented Dec 16, 2014

also includes REST documentation update for /rest/headers

@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2014_12_rest_docu branch Dec 16, 2014
@laanwj laanwj added the REST label Jan 8, 2015
@fanquake
Copy link
Member

fanquake commented Mar 26, 2015

Needs rebase

@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2014_12_rest_docu branch Mar 26, 2015
@jonasschnelli
Copy link
Member Author

jonasschnelli commented Mar 26, 2015

Rebased.
I still think it would make sense to merge this. At least it would add documentation for rest/getheaders (which currently is missing). I could also try to split of the documentation change for separate pull-in.
The JSON support for getheaders would round off/complete the json/bin/hex output support.

@jgarzik
Copy link
Contributor

jgarzik commented Mar 26, 2015

Code appears correct... I just wonder who would use JSON header queries.

@jonasschnelli
Copy link
Member Author

jonasschnelli commented Mar 26, 2015

@jgarzik couldn't we ask the same for json blocks? I just think we should have consistent output support over all rest calls.

@laanwj
Copy link
Member

laanwj commented Mar 26, 2015

Well at least blocks contain transactions. None of the information in headers is that useful in itself, and don't you get a similar view with /rest/block/notxdetails/?

@jonasschnelli
Copy link
Member Author

jonasschnelli commented Mar 26, 2015

IMO JSON output support is for users who don't like to deserialize data. /rest/block/notxdetails/ does not allow one to retrieve multiple blocks/headers at one time. Thats why there is a /rest/getheaders call. It looks somehow bad if we have only one rest call without JSON support and users there need to deserialize data (which mean the need a library or have to learn more about the protocol).

@laanwj
Copy link
Member

laanwj commented Mar 26, 2015

Yes, I understand the purpose but I'm with @jgarzik in wondering why anyone would use this, as the header has very little informational value outside its (hashable, unmalleable) binary representation. But I'm not strongly against it either it's not a lot of code. And there's something to be said for consistency.

@laanwj
Copy link
Member

laanwj commented Mar 27, 2015

utACK

@paveljanik
Copy link
Contributor

paveljanik commented Apr 16, 2015

ACK (I was about to add the missing /rest/headers documentation ;-)

@jonasschnelli
Copy link
Member Author

jonasschnelli commented Apr 16, 2015

Yes. This PR also includes the missing docs for already merged rest headers call (it's a separate commit).

@laanwj
Copy link
Member

laanwj commented Jul 2, 2015

After #6247 this can use the existing BlockHeaderToJSON (see my comment about overlap there)

@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2014_12_rest_docu branch 7 times, most recently Jul 2, 2015
@jonasschnelli
Copy link
Member Author

jonasschnelli commented Jul 2, 2015

Rebased and make use of the UniValue blockheaderToJSON(const CBlockIndex* blockindex) from #6247.

@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2014_12_rest_docu branch Jul 5, 2015
@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2014_12_rest_docu branch to c45c7ea Jul 5, 2015
@laanwj
Copy link
Member

laanwj commented Jul 10, 2015

ACK

@laanwj laanwj merged commit c45c7ea into bitcoin:master Jul 10, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
laanwj added a commit that referenced this pull request Jul 10, 2015
c45c7ea [REST] add JSON support for /rest/headers/ (Jonas Schnelli)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.