I don't see the need for a non-standard "bitcoin-format" header...

Accept: application/x-bitcoin-block, application/json
Accept-Encoding: hex

@luke-jr I agree, and make the default for Accept: / be to output json so curl usually works the way people expect.

I think passing the requested format as an HTTP header is awkward and hard to use. I'd say either a /rest/tx// or ?format=. This also allows us to for example at some point build a minimal HTML interface that links to it, without breaking compatibility or changing the earlier interface.

Yes, please don't invent a new HTTP header: this is exactly what the Accept is for. Allowing a ?format= GET parameter alternative covers cases where specifying headers is inconvenient or impossible.

Using Accept-Encoding for 'hex' struck me as a little weird, but having read the spec again it appears to be correct. I'm not sure why you'd ever use hex though when sending raw binary data over HTTP is fine, and when that is not an option the less verbose base64 is commonplace.. If this were me writing the patch, I'd support the following formats:

Raw/binary

Accept: application/octet-stream
?format=raw

Base64-encoded binary

Accept: application/octet-stream
Accept-Encoding: base64
?format=base64

JSON

Accept: application/json
?format=json

XML

Accept: application/xml
?format=xml

Note that with Accept-Encoding the server notifies the client that the requested encoding was honored by including a Content-Encoding in the response.

Also, I would suggest using the base url /api/v1/, for hopefully obvious reasons.

The Accept/Accept-Encoding feedback seems to be in line with HTTP spec, and widely requested.

On the version number in API: it's already there, in one sense. The API major version number will change very infrequently -- perhaps once a decade, if the bitcoind JSON-RPC compatibility is any guide.

As such, it is trivial to direct API version 2 callers to /rest2/

Any change outside a major compatibility break may easily be handled within the /rest/ namespace.

If people want to bikeshed and strongly prefer /rest1/ that's fine. Just pointing out how infrequent are major version number changes, and the fact that the current scheme already handles major version changes.

Added: In the past, /api/v1 has been suggested for the RPC interface. At this point, "api" is too generic I think.

@sipa makes a fair point about requiring a header being a bit more difficult. Software such as dumb browsers do not permit easy HTTP header modification. However, query strings are bloody ugly.

github.com-style clean URLs seem like a smart way to go, e.g.

    GET /rest/tx/TX-HASH/json

to get the non-default JSON output.

Easy enough to add modifiers after the TX-HASH.

Updated commits and pull req description to indicate use of "clean" URLs.
Non-standard header "Bitcoin-Format" removed.

GET /rest/tx/TX-HASH/json

Is that actually how github does it? TX-HASH.json might be a better choice, I think. Dumb clients which ignore the headers will assume it's json from the extension.

@jgarzik thank you! This is the beginning of a usable, clean API which appeals to merchants and PSPs.

@maaku agree on providing .json instead of /json - seems to be the way everyone does it (without saying anything about whether this is good or bad).

Excuse me if this is out of place, but why implement this in bitcoind?

Writing an external program that wraps bitcoind RPC calls and allows HTTP querying makes much more sense to me. As far as I can see this adds no new information retrievable from bitcoind, it only changes the protocol/format. A simple Python script should be able to do this, no?

Just for fun, I created a simple Python script that does this: https://github.com/runeksvendsen/btchttp/blob/master/btchttp.py

It only supports JSON right now, but should easily extensible.

+1 for @runeksvendsen : I'd rather ship a version of his btchttp.py in contrib/ than make core bitcoind bigger.

The risk in moving this to contrib is that the majority of peers wouldn't
use it, and then when ISPs start blocking the original bitcoin protocol the
network is more likely to die. What we really need is HTTPS so that it's
harder to block.

On Sunday, August 11, 2013, Gavin Andresen wrote:

+1 for @runeksvendsen https://github.com/runeksvendsen : I'd rather
ship a version of his btchttp.py in contrib/ than make core bitcoind bigger.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/2844#issuecomment-22447903
.

@rebroad This is not an interface intended to be exposed to the internet. You can do so of course, but it's not a replacement for the P2P system (it's more an addition to RPC). It's just an interface to ease debugging, or help other local applications that need access to raw block/transaction data.

Regarding whether this belongs in bitcoind, I'm in the middle. I understand the concern about not bloating bitcoind even further, but if such a feature means more people running a local bitcoind instead of relying on some centralized webservice, I'm all for it. The same goes for an address index, and a potential minimal built-in block explorer. However, I'm not sure what this adds that isn't available already. Humans will not use this interface, IMHO, and external applications can already use getblock/getrawtransaction.

I fear several commenters here read the pull request title, and did not examine what the code actually does.

Some salient points:

• Dramatically easier interface for developers and general queries
• Out of the box SSL support, already built into bitcoind
• This pull adds functionality not available via RPC (there is no getrawblock) -- thus the btchttp.py example does not provide what this pull request provides.
• Wider selection of command line tools work out of the box with HTTP REST, versus JSON-RPC
• Nobody will write their own proxy to accomplish this
• A contributed proxy, shipped with bitcoind, might be used -- but it is obviously a fragile, two-process unsupported solution

@jgarzik It's clearly a compromise between usability and bloat, which may or may not be worth it (see my other comment regarding that), however, there IS a getrawblock, it's called getblock [hash] false.

Rebased. The first commit is a cleanup candidate for immediate inclusion, too.

Updated for @maaku 's suggestion of HASH.EXTENSION, where the extension (.json, .txt, .dat) selects the format.

Updated OP examples.

ACK

Rebased for CreateNewBlock() update.

JFYI: Since it was easy, I implemented the following on a side branch:

     GET /rest/block/template.(dat|txt)

to download the binary (/hex) encoding of a miner block template. No fee or sigop information is provided, just straight CBlock and nothing else.

It seems nice and efficient for a pool server to simply request the binary block from a trusted node, and vary bits of the header and coinbase from there.

As noted at top, this is on a side branch, and will not be added to this pull req.

Merge-ready

Rebased. Merge-ready.

Note: I also have a HTTP REST interface for "getblocktemplate" on a local side branch.

Pull-tester error: rest.h is mentioned in the Makefile.am, but there ain't no rest.h committed.

Also, when testing: Is it supposed to be ...TXHASH.json or TXHSAH/json ? The former complains "invalid hash", the latter works.

I like the interface.

Regarding bloat/redundancy, is this going to deprecate the JSON API calls for doing non-authenticated, non-wallet queries eventually?

It also makes sense to have a different interface for block chain data if we want to split the wallet off into another executable eventually. The "block chain daemon" part wouldn't need a JSON RPC interface at all.

That would mean another per-instance-port to configure, another set of bind directives, another set of allowip rules... not sure I'm so happy about that.

What I like about this pull is how little impact it has on existing code.

But that would veer me in the direction of "implement this as an additional application/script on top of RPC" instead of inside Bitcoin Core itself (as noted by @runeksvendsen).

Edit BTW: Encouraging people to open up the current web server code to a wider scope does have its risks, even if not on a port with RPC. For example the code can't cope with DoS very well, but it may also have other issues which haven't been problematic before because RPC needs to be kept private. It means this code needs additional hardening.

I can't help but think how this could probably be done in just a dozen or two lines of python-bitcoinlib code... and it could be in a totally separate process, even a separate user.

@petertodd Wouldn't be difficult to port https://github.com/runeksvendsen/btchttp/blob/master/btchttp.py from bitcoinrpc to python-bitcoinlib.

Ummmm....WHAT? I dont see a single ACK on this pull.

There is one actually, by me, from a long time ago. Though I was surprised too that it just got merged (and several bugs got fixed just after merging even...).

There was one irc discussion

<jgarzik_> wumpus, HTTP REST, yea or nay?
<jgarzik_> I think it's usefully small and compact, and will be used extensively by upper layer services
<wumpus> ACK

which doesnt even read as "go ahead and merge" to me.

@TheBlueMatt There have been many small snippets of discussion in email or irc across many months. That was a final poke, not the entire discussion.

There was no objection to this being on the 0.10 list that circulated, and the general response from committers was either apathy or ack.

@jgarzik My criticism isnt that there wasnt consensus that this should
be merged, but that it was not obvious here. From reading only this
commit and without spending a bunch of time searching for discussions,
it looks like this was merged without review. If @wumpus or @gmaxwell
could posthumously ack this, that'd be fine.

On 11/19/14 04:01, Jeff Garzik wrote:

@TheBlueMatt https://github.com/TheBlueMatt There have been many small
snippets of discussion in email or irc across many months. That was a
final poke, not the entire discussion.

There was no objection to this being on the 0.10 list that circulated,
and the general response from committers was either apathy or ack.

—
Reply to this email directly or view it on GitHub
#2844 (comment).

Wrong @wumpus. That other guy really ought to not use my name on IRC, either, given my history with IRC.

ehh, sorry, meant @laanwj

@TheBlueMatt No objection - I'm still kind of ambivalent whether this belongs in core, but I like the idea of a HTTP REST standard for getting read-only public data from Bitcoin nodes. So posthumous ACK.

@wumpus Let's very much not get into a I'm older-than-you-and-have-been-using-this-name-longer turf war here. I don't care. I think it's possible to block mentions from a project, that would be a practical solution.

So it seems the binary data option doesn't work (Content-Length: 0 is sent), did anyone ever test that?

Also, it seems the interface is /rest/TYPE/HASH/FORMAT, rather than the /rest/TYPE/HASH.EXT that the discussion above suggests.

It seems like nobody actually tried this once (I'm at fault too for that, and perhaps my very old review was too fast...). I started reviewing it again when the discussion on IRC about it started a few days ago, but then it was suddenly merged.

I started testing some hours ago and start now address issues at #5326.

@sipa Yes, it was extensively tested pre-rebase.

Very little testing post-rebase, and HTTPReply() was an area that had a prior rebase bug as well.

For the record in addition to other objections others have already covered, it bothers me that we merged a whole new set of features without a set of unittests for it. In particular, testing this feature with a simple Python script that just grabbed some sample URL's and checked the results against vectors would have been really easy and would have caught the rebase breakage.

@petertodd maybe something to add to #5326.

I'm already writing some unit tests for the REST interface.
But i wait with the pull until #5326 gets merged. I don't want to push all the times and "clear" previous ACKs.

@jonasschnelli Thanks! Yeah, that's the kind of thing that should have been a part of the original pull-req.

For instance, I would consider the lack of unit tests for my own pull-req #2340 to be a reasonable reason to NACK merge it, even though it's much simplier and creating unit tests for it is much more difficult. To skip that step in a case where writing tests is pretty easy is something I think we should avoid doing in the future.