[REST] Post-Merge overhaul of the REST API #5326

jonasschnelli · 2014-11-20T13:05:18Z

No description provided.

sipa · 2014-11-20T13:06:18Z

src/rest.cpp

Remove the second '<< binaryTx'. The response is sent twice now (but the Content-Length only covers the first).

oops. was to quick.
Now it seams to be okay:

curl -v http://localhost:18332/rest/tx/3cfe5051f7efe2702c5720b1ce6f5984181b9172a399935ff22a3e08df143c91/dat > test.bin

< Content-Length: 109

ls -la test.bin -rw-r--r-- 1 jonasschnelli staff 109 20 Nov 14:13 test.bin

laanwj · 2014-11-20T13:24:25Z

src/rest.cpp

I think the point of using HTTPReply with headersOnly=true here was to avoid an extra string copy.
You could use HTTPReplyHeader(HTTP_OK, fRun, binaryBlock.size(), "application/octet-stream") << binaryBlock

Okay. That makes sense.

jonasschnelli · 2014-11-20T14:05:48Z

just changed.

Removed binary as default format, made format option mandatory, list available formats if given format is not available (reduced need for documentation).
Also allow .<format> (along to /<format>) as format choosing option.

sipa · 2014-11-20T14:08:01Z

src/rest.cpp

Don't indent cases please.

sipa · 2014-11-20T14:09:43Z

I'm not sure how useful .format is. .extension would be more natural. Of course, you could add dat as equivalent for binary, and .txt as equivalent for hex...

jtimon · 2014-11-20T14:57:54Z

As said on IRC my preference would be to use ?format=hex and restore json as the default.
But this is probably bikeshedding and maybe too late.

jgarzik · 2014-11-20T15:00:51Z

No, it's not too late. Now would be the time for wider criticism of the API, and the fixing of any API bugs.

sipa · 2014-11-20T15:35:02Z

utACK

Diapolo · 2014-11-20T16:03:32Z

src/init.cpp

IMHO better: Accept public REST requests (default: 0)
Can you add the default for -server, too?
See e.g. -forcednsseed style to be able to use %d in the string.

For now i think it make sense to just change the -rest command. Changing the -server command in this pull would mix things up.

laanwj · 2014-11-24T10:36:00Z

src/rpcserver.cpp

Although in practice updates of booleans are likely atomic, this needs a LOCK(cs_rpcWarmup) for completeness' sake

laanwj · 2014-11-24T11:16:51Z

Code looks good to me, and I've done some basic testing and it works as expected.

I am still in doubt whether we should expose the .json format at all, though.

Unlike the binary formats it is entirely Bitcoin Core RPC specific. If this is supposed to be a standard for REST access of blocks/transactions, every nook needs to be documented in a BIP.
It is not complete: for example, JSON format for block shows a list of transactions, but not the full data of the block. Of course with -txindex it's possible to fetch tx-by-tx after that, but that's quite the detour.
Parsing the data and formatting JSON takes a lot of memory, and increased overhead in time (not a concern at this point, but may be for an API designed to be potentially public). These are all things that could be done client-side.

jtimon · 2014-11-24T12:21:34Z

So maybe a format arg (?format=hex) and binary as default?
On Nov 24, 2014 12:17 PM, "Wladimir J. van der Laan" <
[email protected]> wrote:

Code looks good to me, and I've done some basic testing and it works as
expected.

I am still in doubt whether we should expose the .json format at all,
though.

Unlike the binary formats it is entirely Bitcoin Core RPC specific.
If this is supposed to be a standard for REST access of
blocks/transactions, every nook needs to be documented in a BIP.

It is not complete: for example, JSON format for block shows a list
of transactions, but not the full data of the block. Of course with
-txindex it's possible to fetch tx-by-tx after that, but that's quite
the detour.

Parsing the data and formatting JSON takes a lot of memory, and
increased overhead in time (not a concern at this point, but may be for an
API designed to be potentially public).

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

jgarzik · 2014-11-24T13:50:24Z

The JSON mirrors the RPC format output, when possible.

However, the non-native argument is somewhat persuasive. If you wanted to follow the "convert client side" logic to its conclusion, then removing JSON and hex would result. That certainly keeps in the in-bitcoind code at a minimum.

However, two counterpoints:

The primary argument used on IRC seemed to be a worry about localhost XSS, and all this objection to JSON seems like a parallel construction to achieve that.
Adding REST equivalent of "getutxos" is one of the proposed next steps for the interface, and JSON would seem to be one of the two logical output formats for that (the other being the binary result that @mikehearn's "getutxos" P2P patch added)

jgarzik · 2014-11-24T13:52:03Z

The fix + flag should be separate from the output format pull request. We can go ahead and merge the fix and -rest flag, IMO.

jonasschnelli · 2014-11-24T16:50:10Z

Okay. I will separate the pulls.

laanwj · 2014-11-26T10:45:47Z

The primary argument used on IRC seemed to be a worry about localhost XSS, and all this objection to JSON seems like a parallel construction to achieve that.

No, that was IMO addressed by putting the functionality behind an option. Even without JSON, XSS fingerprinting may be possible by checking for absence/presence of errors. BTW I recently learnt that Tor Browser Bundle explicltly works around this kind of local fingerprinting, that's nice:

pref("network.proxy.no_proxies_on", ""); // For fingerprinting and local service vulns (#10419)

Adding REST equivalent of "getutxos" is one of the proposed next steps for the interface, and JSON would seem to be one of the two logical output formats for that (the other being the binary result that @mikehearn's "getutxos" P2P patch added)

I don't have anything against returning well-defined JSON data. If there are multiple things to return they need to be packed into a structure, and getutxo is an excellent example of that.

However that is entirely different from bitcoind's lossy RPC format for blocks, which is here used because the code happens to be available.

So maybe a format arg (?format=hex) and binary as default?

Let's not have a default. People can specify the format that they want.

jonasschnelli · 2014-11-26T13:05:59Z

Just updated.
Format specific changes are removed from this pull (i'll open a new pull for this).
Now we only deal with HTTP Response fix, -rest flag (default off), warmup response.

5dc713b [REST] set REST API behind "-rest" option (Jonas Schnelli) 78bdc81 [REST] give an appropriate response in warmup phase (Jonas Schnelli) 210eba9 [REST] fix headersonly flag for BINARY responses (Jonas Schnelli)

Diapolo · 2014-11-26T14:27:34Z

src/init.cpp

@laanwj Here is another new translation string ;)? Still hoping you merge the others ^^.

I know, but this one is unavoidable, and not just some cosmetic change.

jonasschnelli · 2014-11-27T12:23:51Z

Also see #5376 for further format-specific discussions.
Also referencing #5379 (unit-tests).

sipa reviewed Nov 20, 2014
View reviewed changes

jonasschnelli mentioned this pull request Nov 20, 2014

Add unauthenticated HTTP REST interface #2844

Merged

jonasschnelli force-pushed the rest-stuff branch 2 times, most recently from 80783e2 to f3d52fb Compare November 20, 2014 13:15

laanwj reviewed Nov 20, 2014
View reviewed changes

jonasschnelli changed the title ~~[REST] fix headersonly flag for BINARY responses~~ [REST] fix headersonly flag for BINARY responses / overhaul format handling Nov 20, 2014

sipa reviewed Nov 20, 2014
View reviewed changes

src/rest.cpp Outdated

Copy link

Member

sipa Nov 20, 2014

Choose a reason for hiding this comment

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

Don't indent cases please.

jonasschnelli force-pushed the rest-stuff branch from 9d79530 to c273e11 Compare November 20, 2014 14:09

jonasschnelli force-pushed the rest-stuff branch from c273e11 to 6f38f6d Compare November 20, 2014 14:10

jonasschnelli force-pushed the rest-stuff branch from 6f38f6d to eb1cbe5 Compare November 20, 2014 15:09

jonasschnelli changed the title ~~[REST] fix headersonly flag for BINARY responses / overhaul format handling~~ [REST] Post-Merge overhaul of the REST API Nov 20, 2014

sipa mentioned this pull request Nov 20, 2014

[REST] adding basic documentation #5329

Merged

Diapolo reviewed Nov 20, 2014
View reviewed changes

jonasschnelli force-pushed the rest-stuff branch 2 times, most recently from 1e1868e to 4820cae Compare November 21, 2014 20:26

laanwj reviewed Nov 24, 2014
View reviewed changes

jonasschnelli added 3 commits November 26, 2014 13:42

[REST] fix headersonly flag for BINARY responses

210eba9

[REST] give an appropriate response in warmup phase

78bdc81

[REST] set REST API behind "-rest" option

5dc713b

jonasschnelli force-pushed the rest-stuff branch from 4820cae to 5dc713b Compare November 26, 2014 12:55

laanwj merged commit 5dc713b into bitcoin:master Nov 26, 2014

Diapolo reviewed Nov 26, 2014
View reviewed changes

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

[REST] Post-Merge overhaul of the REST API #5326

[REST] Post-Merge overhaul of the REST API #5326

Uh oh!

Conversation

jonasschnelli commented Nov 20, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Nov 20, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa commented Nov 20, 2014

Uh oh!

jtimon commented Nov 20, 2014

Uh oh!

jgarzik commented Nov 20, 2014

Uh oh!

sipa commented Nov 20, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Nov 24, 2014

Uh oh!

jtimon commented Nov 24, 2014

Uh oh!

jgarzik commented Nov 24, 2014

Uh oh!

jgarzik commented Nov 24, 2014

Uh oh!

jonasschnelli commented Nov 24, 2014

Uh oh!

laanwj commented Nov 26, 2014

Uh oh!

jonasschnelli commented Nov 26, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Nov 27, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants