[REST] Post-Merge overhaul of the REST API

jonasschnelli commented at 1:05 pm on November 20, 2014: contributor

in src/rest.cpp: in b0973705d4 outdated

146@@ -147,7 +147,7 @@ static bool rest_tx(AcceptedConnection *conn,
147     switch (rf) {
148     case RF_BINARY: {
149         string binaryTx = ssTx.str();
150-        conn->stream() << HTTPReply(HTTP_OK, binaryTx, fRun, true, "application/octet-stream") << binaryTx << std::flush;
151+        conn->stream() << HTTPReply(HTTP_OK, binaryTx, fRun, false, "application/octet-stream") << binaryTx << std::flush;

sipa commented at 1:06 pm on November 20, 2014:

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

jonasschnelli commented at 1:14 pm on November 20, 2014:

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

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

0< Content-Length: 109

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

jonasschnelli force-pushed on Nov 20, 2014

in src/rest.cpp: in f3d52fbb50 outdated

 98@@ -99,7 +99,7 @@ static bool rest_block(AcceptedConnection *conn,
 99     switch (rf) {
100     case RF_BINARY: {
101         string binaryBlock = ssBlock.str();
102-        conn->stream() << HTTPReply(HTTP_OK, binaryBlock, fRun, true, "application/octet-stream") << binaryBlock << std::flush;
103+        conn->stream() << HTTPReply(HTTP_OK, binaryBlock, fRun, false, "application/octet-stream") << std::flush;

laanwj commented at 1:24 pm on November 20, 2014:

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

jonasschnelli commented at 1:29 pm on November 20, 2014:

Okay. That makes sense.

jonasschnelli renamed this:
~~[REST] fix headersonly flag for BINARY responses~~
[REST] fix headersonly flag for BINARY responses / overhaul format handling
on Nov 20, 2014

jonasschnelli commented at 2:05 pm on November 20, 2014: contributor

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.

in src/rest.cpp: in 9d7953028b outdated

169@@ -145,25 +170,28 @@ static bool rest_tx(AcceptedConnection *conn,
170     ssTx << tx;
171 
172     switch (rf) {
173-    case RF_BINARY: {
174-        string binaryTx = ssTx.str();
175-        conn->stream() << HTTPReply(HTTP_OK, binaryTx, fRun, true, "application/octet-stream") << binaryTx << std::flush;
176-        return true;
177-    }
178+        case RF_BINARY: {

sipa commented at 2:08 pm on November 20, 2014:

Don’t indent cases please.

in src/rest.cpp: in 9d7953028b outdated

55 {
56-    for (unsigned int i = 0; i < ARRAYLEN(rf_names); i++)
57-        if (format == rf_names[i].name)
58-            return rf_names[i].rf;
59+    boost::split(params, strReq, boost::is_any_of("/"));
60+    if(params.size() <= 1) {

sipa commented at 2:08 pm on November 20, 2014:

Spaces after ifs please.

jonasschnelli force-pushed on Nov 20, 2014

sipa commented at 2:09 pm on November 20, 2014: member

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…

jonasschnelli force-pushed on Nov 20, 2014

jtimon commented at 2:57 pm on November 20, 2014: contributor

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 commented at 3:00 pm on November 20, 2014: contributor

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

jonasschnelli force-pushed on Nov 20, 2014

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

sipa commented at 3:35 pm on November 20, 2014: member

utACK

in src/init.cpp: in 5292a682fc outdated

356@@ -357,6 +357,7 @@ std::string HelpMessage(HelpMessageMode mode)
357 
358     strUsage += "\n" + _("RPC server options:") + "\n";
359     strUsage += "  -server                " + _("Accept command line and JSON-RPC commands") + "\n";
360+    strUsage += "  -rest                  " + _("Accept public REST requests") + "\n";

Diapolo commented at 4:03 pm on November 20, 2014:

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.

jonasschnelli commented at 3:18 pm on November 21, 2014:

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

in src/rest.cpp: in 5292a682fc outdated

82+            formats.append(".");
83+            formats.append(rf_names[i].name);
84+            formats.append(",");
85+        }
86+    
87+    if(formats.length()>0)

Diapolo commented at 4:04 pm on November 20, 2014:

Can you use the correct style here? if (formats.length() > 0) { and so on.

in src/rest.cpp: in 5292a682fc outdated

213@@ -187,6 +214,9 @@ bool HTTPReq_REST(AcceptedConnection *conn,
214                   bool fRun)
215 {
216     try {
217+        if(RPCIsInWarmup())
218+            throw RESTERR(HTTP_SERVICE_UNAVAILABLE, "Service currently not available (Loading block index...).");

Diapolo commented at 4:06 pm on November 20, 2014:

Is it true we are for sure at the loading block index stage? If no a simple loading would be better suiting.

laanwj commented at 4:26 pm on November 20, 2014:

It just means we’re in warmup mode (during initialization). Let’s just put that in the message.

jonasschnelli commented at 3:16 pm on November 21, 2014:

I just thought we should answer with the same text as we answer RPC calls during “warmup”. There we just answer with “loading block index…”. Should we change it to just “loading block index…”?

Diapolo commented at 3:39 pm on November 21, 2014:

Perhaps it makes sense to also change the string at it’s original place in the code then.

jonasschnelli commented at 7:36 pm on November 21, 2014:

What do you propose: “Service currently not available (warmup mode).”?

jgarzik commented at 7:47 pm on November 21, 2014:

Service temporarily unavailable.

jonasschnelli commented at 8:27 pm on November 21, 2014:

Okay. Set.

jonasschnelli force-pushed on Nov 21, 2014

in src/rpcserver.cpp: in 4820cae4a8 outdated

761@@ -762,6 +762,11 @@ void SetRPCWarmupFinished()
762     fRPCInWarmup = false;
763 }
764 
765+bool RPCIsInWarmup()
766+{
767+    return fRPCInWarmup;

laanwj commented at 10:36 am on November 24, 2014:

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

in src/rest.cpp: in 4820cae4a8 outdated

213@@ -187,6 +214,9 @@ bool HTTPReq_REST(AcceptedConnection *conn,
214                   bool fRun)
215 {
216     try {
217+        if(RPCIsInWarmup())
218+            throw RESTERR(HTTP_SERVICE_UNAVAILABLE, "Service temporarily unavailable.");

laanwj commented at 10:49 am on November 24, 2014:

Let’s add the RPC warmup message here see https://github.com/laanwj/bitcoin/commit/c7e20830ddaa6a919f3f73e9d4fa30fab901561d

laanwj commented at 11:16 am on November 24, 2014: member

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 commented at 12:21 pm on November 24, 2014: contributor

So maybe a format arg (?format=hex) and binary as default? On Nov 24, 2014 12:17 PM, “Wladimir J. van der Laan” < notifications@github.com> 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 commented at 1:50 pm on November 24, 2014: contributor

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 commented at 1:52 pm on November 24, 2014: contributor

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 commented at 4:50 pm on November 24, 2014: contributor

Okay. I will separate the pulls.

laanwj commented at 10:45 am on November 26, 2014: member

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:

0pref("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.

[REST] fix headersonly flag for BINARY responses 210eba9fdb

[REST] give an appropriate response in warmup phase 78bdc8103f

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

jonasschnelli force-pushed on Nov 26, 2014

jonasschnelli commented at 1:05 pm on November 26, 2014: contributor

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.

laanwj merged this on Nov 26, 2014

laanwj closed this on Nov 26, 2014

laanwj referenced this in commit 108b19f7ef on Nov 26, 2014

in src/init.cpp: in 5dc713bfc7

351@@ -352,6 +352,7 @@ std::string HelpMessage(HelpMessageMode mode)
352 
353     strUsage += "\n" + _("RPC server options:") + "\n";
354     strUsage += "  -server                " + _("Accept command line and JSON-RPC commands") + "\n";
355+    strUsage += "  -rest                  " + strprintf(_("Accept public REST requests (default: %u)"), 0) + "\n";

Diapolo commented at 2:27 pm on November 26, 2014:

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

laanwj commented at 2:33 pm on November 26, 2014:

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

jonasschnelli commented at 12:23 pm on November 27, 2014: contributor

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

DrahtBot locked this on Sep 8, 2021

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